[PATCH 6 of 8] merge: use separate lists for each action type

[Pierre-Yves David]

On 05/01/2014 04:42 PM, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski at unity3d.com>
> # Date 1393550758 -3600
> #      Fri Feb 28 02:25:58 2014 +0100
> # Branch stable
> # Node ID 68cb67727ca4447bad6349b6f7d75d799d35f6ed
> # Parent  9d786e6be4319bf31467eb715fb432c68ccd4507
> merge: use separate lists for each action type
>
> This replaces the grand unified action list that had multiple action types as
> tuples in one big list. That list was iterated multiple times just to find
> actions of a specific type. This data model also made some code more
> convoluted than necessary.
>
> Instead we now store actions as a tuple of lists. Using multiple lists gives a
> bit of cut'n'pasted code but also enables other optimizations.

I like this change very much.

> This patch uses 'if True:' to preserve indentations and help reviewing. It also
> limits the number of conflicts with other pending patches. It can trivially be
> cleaned up later.
>
> The changes to test output are due to changes in the ordering of debug output.
> That is mainly because we now do the debug logging for files when we actually
> process them. Files are also processed in a slightly different but still
> correct order. It is now primarily ordered by action type, secondarily by
> filename.

I though this reordering already happened in patch 2 of this series?

> diff --git a/mercurial/merge.py b/mercurial/merge.py
> --- a/mercurial/merge.py
> +++ b/mercurial/merge.py
> @@ -310,63 +310,44 @@ def _forgetremoved(wctx, mctx, branchmer
>       as removed.
>       """
>
> -    actions = []
> -    state = branchmerge and 'r' or 'f'
> +    ractions = []
> +    factions = xactions = []
> +    if branchmerge:
> +        xactions = ractions
>       for f in wctx.deleted():
>           if f not in mctx:
> -            actions.append((f, state, None, "forget deleted"))
> +            xactions.append((f, None, "forget deleted"))
>
>       if not branchmerge:
>           for f in wctx.removed():
>               if f not in mctx:
> -                actions.append((f, "f", None, "forget removed"))
> +                factions.append((f, None, "forget removed"))
>
> -    return actions
> +    return ractions, factions
>
>   def _checkcollision(repo, wmf, actions):
>       # build provisional merged manifest up
>       pmmf = set(wmf)
>
> -    def addop(f, args):
> -        pmmf.add(f)
> -    def removeop(f, args):
> +    # k, dr, e and rd are nop
> +    for m in 'a', 'f', 'g', 'cd', 'dc':
> +        for f, args, msg in actions[m]:
> +            pmmf.add(f)
> +    for f, args, msg in actions['r']:
>           pmmf.discard(f)
> -    def nop(f, args):
> -        pass
> -
> -    def renamemoveop(f, args):
> +    for f, args, msg in actions['dm']:
>           f2, flags = args
>           pmmf.discard(f2)
>           pmmf.add(f)
> -    def renamegetop(f, args):
> +    for f, args, msg in actions['dg']:
>           f2, flags = args
>           pmmf.add(f)
> -    def mergeop(f, args):
> +    for f, args, msg in actions['m']:
>           f1, f2, fa, move, anc = args
>           if move:
>               pmmf.discard(f1)
>           pmmf.add(f)
>
> -    opmap = {
> -        "a": addop,
> -        "dm": renamemoveop,
> -        "dg": renamegetop,
> -        "dr": nop,
> -        "e": nop,
> -        "k": nop,
> -        "f": addop, # untracked file should be kept in working directory
> -        "g": addop,
> -        "m": mergeop,
> -        "r": removeop,
> -        "rd": nop,
> -        "cd": addop,
> -        "dc": addop,
> -    }
> -    for f, m, args, msg in actions:
> -        op = opmap.get(m)
> -        assert op, m
> -        op(f, args)
> -
>       # check case-folding collision in provisional merged manifest
>       foldmap = {}
>       for f in sorted(pmmf):
> @@ -386,7 +367,9 @@ def manifestmerge(repo, wctx, p2, pa, br
>       acceptremote = accept the incoming changes without prompting
>       """
>
> -    actions, copy, movewithdir = [], {}, {}
> +    actions = dict((m, []) for m in ['a', 'f', 'g', 'cd', 'dc', 'r',
> +                                     'dm', 'dg', 'm', 'dr', 'e', 'rd', 'k'])

This line is twice the standard size, you can probably switch to a 
version for loop.

> @@ -803,126 +775,120 @@ def calculateupdates(repo, wctx, mctx, a
>               actions = manifestmerge(repo, wctx, mctx, ancestor,
>                                       branchmerge, force,
>                                       partial, acceptremote, followcopies)
> -            for a in sorted(actions, key=lambda a: (a[1], a)):
> -                f, m, args, msg = a
> -                repo.ui.debug(' %s: %s -> %s\n' % (f, msg, m))
> -                if f in fbids:
> -                    fbids[f].append(a)
> -                else:
> -                    fbids[f] = [a]
> +            for m, l in sorted(actions.items()):
> +                for a in l:
> +                    f, args, msg = a
> +                    repo.ui.debug(' %s: %s -> %s\n' % (f, msg, m))
> +                    if f in fbids:
> +                        d = fbids[f]
> +                        if m in d:
> +                            d[m].append(a)
> +                        else:
> +                            d[m] = [a]
> +                    else:
> +                        fbids[f] = {m: [a]}
>
>           # Pick the best bid for each file
>           repo.ui.note(_('\nauction for merging merge bids\n'))
> -        actions = []
> +        actions = dict((m, []) for m in actions.keys())
>           for f, bidsl in sorted(fbids.items()):
>               # Consensus?
> -            a0 = bidsl[0]
> -            if util.all(a == a0 for a in bidsl[1:]): # len(bidsl) is > 1
> -                repo.ui.note(" %s: consensus for %s\n" % (f, a0[1]))
> -                actions.append(a0)
> -                continue
> -            # Group bids by kind of action
> -            bids = {}
> -            for a in bidsl:
> -                m = a[1]
> -                if m in bids:
> -                    bids[m].append(a)
> -                else:
> -                    bids[m] = [a]
> +            if len(bidsl) == 1: # one kind of method
> +                m, l = bidsl.items()[0]
> +                if util.all(a == l[0] for a in l[1:]): # len(bidsl) is > 1
> +                    repo.ui.note(" %s: consensus for %s\n" % (f, m))
> +                    actions[m].append(l[0])
> +                    continue

I do not understand the "# one kind of method"


-- 
Pierre-Yves David