[PATCH 6 of 8] merge: use separate lists for each action type
Pierre-Yves David
pierre-yves.david at ens-lyon.org
Thu May 8 15:00:00 CDT 2014
On 05/08/2014 06:20 AM, Mads Kiilerich wrote:
> Heh - this is the patch I have been referring to as the 8th patch ;-)
xoring 14 to all previous mention.
>
> On 05/07/2014 11:30 PM, Pierre-Yves David wrote:
>> 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?
>
> Correct. The patch description should have been updated after I split
> the patch up in the preceding minor parts. This patch do not have any
> test changes.
Ok, please fix the changeset description (and move that part to patch 2)
>
>>
>>> 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.
>
> I don't understand that comment. Please clarify.
when your one liner are two line long, its time to use a plan form
actions = {}
for m in […]:
actions[m] = []
>>> @@ -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"
>
> That is if there is bids for one kind of method, for instance if all
> bids are for merge.
The comment could probably be a bit expanded a bit more so that people
other than its original author can understand it.
--
Pierre-Yves
More information about the Mercurial-devel
mailing list