[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