[PATCH 1 of 2] commit: properly handle the combination of --subrepos and --addremove options
Angel Ezquerra
angel.ezquerra at gmail.com
Wed Sep 3 16:36:50 CDT 2014
On Tue, Sep 2, 2014 at 9:09 PM, Pierre-Yves David
<pierre-yves.david at ens-lyon.org> wrote:
>
>
> On 08/30/2014 05:09 PM, Angel Ezquerra wrote:
>>
>> # HG changeset patch
>> # User Angel Ezquerra <angel.ezquerra at gmail.com>
>> # Date 1409388881 -7200
>> # Sat Aug 30 10:54:41 2014 +0200
>> # Node ID 7e317616adbe47d79fbf2243711b68f97d941f29
>> # Parent bdc0e04df243d3995c7266bf7d138fddd0449ba6
>> commit: properly handle the combination of --subrepos and --addremove
>> options
>>
>> Up until now calling commit --subrepos --addremove would not run addremove
>> on
>> the subrepositories. Now we do so for mercurial subrepositories. If a
>> non-mercurial subrepository is found it is skipped (and a warning message
>> is
>> shown).
>
>
> The main idea is sounds good to me. But the description (warn and skip)
> mismatch the actual implementation (abort). I feel like the behavior in the
> description is better.
As I said on my reply to the second patch in the series I do not have
a strong opinion on this (although the commit message and the patch
should match, of course). I can either fix the commit message or
change the implementation depending on what you guys think is best.
> I've a couple a nits and a question about the tests
>
>
>>
>> diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
>> old mode 100644
>> new mode 100755
>> --- a/mercurial/scmutil.py
>> +++ b/mercurial/scmutil.py
>> @@ -618,13 +618,26 @@
>> '''Return a matcher that will efficiently match exactly these
>> files.'''
>> return matchmod.exact(repo.root, repo.getcwd(), files)
>>
>> -def addremove(repo, pats=[], opts={}, dry_run=None, similarity=None):
>> +def addremove(repo, pats=[], opts={}, dry_run=None, similarity=None,
>> prefix=''):
>> if dry_run is None:
>> dry_run = opts.get('dry_run')
>> if similarity is None:
>> similarity = float(opts.get('similarity') or 0)
>> +
>> + wctx = repo[None]
>> + if opts.get('subrepos', False):
>> + for (s, sinfo) in wctx.substate.items():
>> + stype = sinfo[2]
>> + if stype != 'hg':
>> + raise util.Abort(_('cannot run addremove on %s
>> subrepository %s '
>> + '(not supported)\n') % (stype, s))
>
>
> 1. For improved readability you should use a temporary variable instead of
> splitting the call in multiple line::
>
> msg = _('cannot run addremove on %s subrepository %s '
> '(not supported)\n')
> raise util.Abort(msg % (stype, s)
Good idea.
>
> 2. Your commit description says:
>
> If a non-mercurial subrepository is found it is skipped (and a
> warning message is shown).
>
> But raising Abort does not seems very warningo-skippish to me.
Right. See above.
>> + for s, sinfo in wctx.substate.items():
>> + srepo = wctx.sub(s)
>> + addremove(srepo._repo, pats=pats, opts=opts, dry_run=dry_run,
>> + similarity=similarity, prefix=os.path.join(prefix,
>> s))
>> +
>
>
> Small nits: I would extract the the os.path.join(prefix, s) part in a tmp
> variable for readability purpose (but we are far deep in the taste and color
> territory)
OK
>> # we'd use status here, except handling of symlinks and ignore is
>> tricky
>> - m = match(repo[None], pats, opts)
>> + m = match(wctx, pats, opts)
>> rejected = []
>> m.bad = lambda x, y: rejected.append(x)
>>
>> @@ -636,10 +649,13 @@
>> for abs in sorted(toprint):
>> if repo.ui.verbose or not m.exact(abs):
>> rel = m.rel(abs)
>> + filename = (pats and rel) or abs
>
> small nits: now that it is no longer a one liner we could also kill this
> horrible construct.
If only we did not have to be 2.4 compatible... :-P
>> + if prefix:
>> + filename = os.path.join(prefix, filename)
>> if abs in unknownset:
>> - status = _('adding %s\n') % ((pats and rel) or abs)
>> + status = _('adding %s\n') % filename
>> else:
>> - status = _('removing %s\n') % ((pats and rel) or abs)
>> + status = _('removing %s\n') % filename
>> repo.ui.status(status)
>>
>> renames = _findrenames(repo, m, added + unknown, removed + deleted,
>> diff --git a/tests/test-subrepo-git.t b/tests/test-subrepo-git.t
>> --- a/tests/test-subrepo-git.t
>> +++ b/tests/test-subrepo-git.t
>> @@ -103,6 +103,11 @@
>> $ echo ggg >> s/g
>> $ hg status --subrepos
>> M s/g
>> + $ hg commit --subrepos --addremove -m ggg
>> + abort: cannot run addremove on git subrepository s (not supported)
>> +
>> + [255]
>
> Suspicious extra blank line. \n in the Abort message?
Yes, that's it.
>> +
>> $ hg commit --subrepos -m ggg
>> committing subrepository s
>> $ hg debugsub
>> diff --git a/tests/test-subrepo-recursion.t
>> b/tests/test-subrepo-recursion.t
>> --- a/tests/test-subrepo-recursion.t
>> +++ b/tests/test-subrepo-recursion.t
>> @@ -179,12 +179,17 @@
>> +z3
>> $ cd ..
>>
>> -Cleanup and final commit:
>>
>> - $ rm -r dir
>> - $ hg commit --subrepos -m 2-3-2
>> +Test commit --addremove --subrepos combination
>> +
>> + $ hg commit --addremove --subrepos -m 2-3-2
>> + adding dir/a.txt
>> committing subrepository foo
>> committing subrepository foo/bar (glob)
>> + $ rm dir/a.txt
>> + $ hg commit --addremove --subrepos -m 2-3-2
>> + removing dir/a.txt
>> +
>>
>> Test explicit path commands within subrepos: add/forget
>> $ echo z1 > foo/bar/z2.txt
>> @@ -206,7 +211,8 @@
>> Log with the relationships between repo and its subrepo:
>>
>> $ hg log --template '{rev}:{node|short} {desc}\n'
>> - 2:1326fa26d0c0 2-3-2
>> + 3:1e8895f052f6 2-3-2
>> + 2:9078d578b14d 2-3-2
>> 1:4b3c9ff4f66b 1-2-1
>> 0:23376cbba0d8 0-0-0
>
>
> Not sure what the meaning of 2-3-2 here. Are you confident they still match
> the content/meaning/spirituality of the underlying content?
I believe so. The original test removed some non tracked files before
committing the subrepos. The new version keeps them there in order to
test the combination of the --addremove and --subrepos flags. I
believe the commit message refers to which revision is committed in
each nested subrepo level (including the top). That did not change
with my test.
I will wait to send a new patch series to get a confirmation on
whether I should remove the abort and just issue a warning message
instead. I'm also sending this email to Kevin to make sure that he is
in the loop.
Cheers,
Angel
More information about the Mercurial-devel
mailing list