[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