[PATCH 4 of 8] merge: change debug logging and progress
Pierre-Yves David
pierre-yves.david at ens-lyon.org
Thu May 8 14:56:06 CDT 2014
On 05/08/2014 04:33 AM, Mads Kiilerich wrote:
> On 05/07/2014 11:07 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 1398125425 -7200
>>> # Tue Apr 22 02:10:25 2014 +0200
>>> # Branch stable
>>> # Node ID feac4ee682bea8ebb19ead775bdb9ef413da8e3e
>>> # Parent 28162b4271a8a4c4cdee6532c49aa5316f9133d5
>>> merge: change debug logging and progress
>>>
>>> Preparing for action list split-up ...
>>
>> Description is not very helpful
>
> There is not much to say. The patch moves debug statements around
> without really changing anything. Arguably, it temporarily makes the
> code worse. Then only justification is that it makes it easier to review
> the test changes ... and in the end the 8th don't change test output at
> all.
That already a significant amount said. It should be in the commit
description in the first place.
>
>>
>>>
>>> diff --git a/mercurial/merge.py b/mercurial/merge.py
>>> --- a/mercurial/merge.py
>>> +++ b/mercurial/merge.py
>>> @@ -571,9 +571,9 @@ def getremove(repo, mctx, overwrite, arg
>>> wwrite = repo.wwrite
>>> audit = repo.wopener.audit
>>> i = 0
>>> - for arg in args:
>>> - f = arg[0]
>>> - if arg[1] == 'r':
>>> + for f, m, args, msg in args:
>>> + repo.ui.debug(" %s: %s -> %s\n" % (f, msg, m))
>>> + if m == 'r':
>>> if verbose:
>>> repo.ui.note(_("removing %s\n") % f)
>>> audit(f)
>>> @@ -585,7 +585,7 @@ def getremove(repo, mctx, overwrite, arg
>>> else:
>>> if verbose:
>>> repo.ui.note(_("getting %s\n") % f)
>>> - wwrite(f, fctx(f).data(), arg[2][0])
>>> + wwrite(f, fctx(f).data(), args[0])
>>> if i == 100:
>>> yield i, f
>>> i = 0
>>> @@ -612,12 +612,11 @@ def applyupdates(repo, actions, wctx, mc
>>> # prescan for merges
>>> for a in actions:
>>> f, m, args, msg = a
>>> - repo.ui.debug(" %s: %s -> %s\n" % (f, msg, m))
>>> if m == "m": # merge
>>> f1, f2, fa, move, anc = args
>>> if f == '.hgsubstate': # merged internally
>>> continue
>>> - repo.ui.debug(" preserving %s for resolve of %s\n" %
>>> (f1, f))
>>> + repo.ui.debug(" preserving %s for resolve of %s\n" %
>>> (f1, f))
>>
>> unexpected space change?
>
> Not really. The debug statement will no longer have the single space
> indented "->" before it. Having this message double indented will thus
> no longer make sense.
Ha!, documentation of output change that should have been in the
description.
>>> fcl = wctx[f1]
>>> fco = mctx[f2]
>>> actx = repo[anc]
>>> @@ -647,7 +646,7 @@ def applyupdates(repo, actions, wctx, mc
>>> updated = len(updateactions)
>>> removeactions = [a for a in workeractions if a[1] == 'r']
>>> removed = len(removeactions)
>>> - actions = [a for a in actions if a[1] not in 'grk']
>>> + actions = [a for a in actions if a[1] not in 'gr']
>>>
>>> hgsub = [a[1] for a in workeractions if a[0] == '.hgsubstate']
>>> if hgsub and hgsub[0] == 'r':
>>> @@ -671,10 +670,30 @@ def applyupdates(repo, actions, wctx, mc
>>> if hgsub and hgsub[0] == 'g':
>>> subrepo.submerge(repo, wctx, mctx, wctx, overwrite)
>>>
>>> - for i, a in enumerate(actions):
>>> - f, m, args, msg = a
>>> - progress(_updating, z + i + 1, item=f, total=numupdates,
>>> unit=_files)
>>> - if m == "m": # merge
>>> + for f, m, args, msg in actions:
>>> +
>>> + # forget (manifest only, just log it) (must come first)
>>> + if m == "f":
>>> + repo.ui.debug(" %s: %s -> f\n" % (f, msg))
>>> + z += 1
>>> + progress(_updating, z, item=f, total=numupdates,
>>> unit=_files)
>>> +
>>> + # re-add (manifest only, just log it)
>>> + elif m == "a":
>>> + repo.ui.debug(" %s: %s -> a\n" % (f, msg))
>>> + z += 1
>>> + progress(_updating, z, item=f, total=numupdates,
>>> unit=_files)
>>> +
>>> + # keep (noop, just log it)
>>> + elif m == "k":
>>> + repo.ui.debug(" %s: %s -> k\n" % (f, msg))
>>> + # no progress
>>> +
>>> + # merge
>>> + elif m == "m":
>>> + repo.ui.debug(" %s: %s -> m\n" % (f, msg))
>>> + z += 1
>>> + progress(_updating, z, item=f, total=numupdates,
>>> unit=_files)
>>
>> Those almost identical and repeated call to ui.debug(…) are bugging
>> me. Can't we factor them?
>
> Look at how it is after the 8th patch where we gain code readability and
> efficiency. It is possible that we could introduce a "process this
> action list and do some logging and progress reporting and apply this
> function" later.
Ok, please justify this in the description too.
>
>>
>>> f1, f2, fa, move, anc = args
>>> if f == '.hgsubstate': # subrepo states need updating
>>> subrepo.submerge(repo, wctx, mctx,
>>> wctx.ancestor(mctx),
>>> @@ -689,35 +708,61 @@ def applyupdates(repo, actions, wctx, mc
>>> updated += 1
>>> else:
>>> merged += 1
>>> - elif m == "dm": # directory rename, move local
>>> +
>>> + # directory rename, move local
>>> + elif m == "dm":
>>> + repo.ui.debug(" %s: %s -> dm\n" % (f, msg))
>>> + z += 1
>>> + progress(_updating, z, item=f, total=numupdates,
>>> unit=_files)
>>> f0, flags = args
>>> repo.ui.note(_("moving %s to %s\n") % (f0, f))
>>> audit(f)
>>> repo.wwrite(f, wctx.filectx(f0).data(), flags)
>>> util.unlinkpath(repo.wjoin(f0))
>>> updated += 1
>>> - elif m == "dg": # local directory rename, get
>>> +
>>> + # local directory rename, get
>>> + elif m == "dg":
>>> + repo.ui.debug(" %s: %s -> dg\n" % (f, msg))
>>> + z += 1
>>> + progress(_updating, z, item=f, total=numupdates,
>>> unit=_files)
>>> f0, flags = args
>>> repo.ui.note(_("getting %s to %s\n") % (f0, f))
>>> repo.wwrite(f, mctx.filectx(f0).data(), flags)
>>> updated += 1
>>> - elif m == "dr": # divergent renames
>>> +
>>> + # divergent renames
>>> + elif m == "dr":
>>> + repo.ui.debug(" %s: %s -> dr\n" % (f, msg))
>>> + z += 1
>>> + progress(_updating, z, item=f, total=numupdates,
>>> unit=_files)
>>> fl, = args
>>> repo.ui.warn(_("note: possible conflict - %s was renamed "
>>> "multiple times to:\n") % f)
>>> for nf in fl:
>>> repo.ui.warn(" %s\n" % nf)
>>> - elif m == "rd": # rename and delete
>>> +
>>> + # rename and delete
>>> + elif m == "rd":
>>> + repo.ui.debug(" %s: %s -> rd\n" % (f, msg))
>>> + z += 1
>>> + progress(_updating, z, item=f, total=numupdates,
>>> unit=_files)
>>> fl, = args
>>> repo.ui.warn(_("note: possible conflict - %s was deleted "
>>> "and renamed to:\n") % f)
>>> for nf in fl:
>>> repo.ui.warn(" %s\n" % nf)
>>> - elif m == "e": # exec
>>> +
>>> + # exec
>>> + elif m == "e":
>>> + repo.ui.debug(" %s: %s -> e\n" % (f, msg))
>>> + z += 1
>>> + progress(_updating, z, item=f, total=numupdates,
>>> unit=_files)
>>> flags, = args
>>> audit(f)
>>> util.setflags(repo.wjoin(f), 'l' in flags, 'x' in flags)
>>> updated += 1
>>> +
>>> ms.commit()
>>> progress(_updating, None, total=numupdates, unit=_files)
>>>
>>> @@ -744,7 +789,7 @@ def calculateupdates(repo, wctx, mctx, a
>>> actions = manifestmerge(repo, wctx, mctx, ancestor,
>>> branchmerge, force,
>>> partial, acceptremote,
>>> followcopies)
>>> - for a in sorted(actions):
>>> + for a in sorted(actions, key=lambda a: (a[1], a)):
>>
>> New obscure key added to the sorted call?
>
> It is only temporary so the convoluted code gives the same output as the
> simplified code will do in step 8.
Code comment + changeset description.
Looks like there was much to said in this commit message. At least to
explain the reviewer what was going on.
--
Pierre-Yves
More information about the Mercurial-devel
mailing list