Patchwork [1,of,2] commit: properly handle the combination of --subrepos and --addremove options

login
register
mail settings
Submitter Angel Ezquerra
Date Aug. 30, 2014, 3:09 p.m.
Message ID <7e317616adbe47d79fbf.1409411348@angels-macbook-pro.local>
Download mbox | patch
Permalink /patch/5638/
State Changes Requested
Headers show

Comments

Angel Ezquerra - Aug. 30, 2014, 3:09 p.m.
# HG changeset patch
# User Angel Ezquerra <angel.ezquerra@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).
Pierre-Yves David - Sept. 2, 2014, 7:09 p.m.
On 08/30/2014 05:09 PM, Angel Ezquerra wrote:
> # HG changeset patch
> # User Angel Ezquerra <angel.ezquerra@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.

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)

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.

> +        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)

>       # 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 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?

> +
>     $ 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?

>
> @@ -427,7 +433,7 @@
>     $ hg outgoing -S
>     comparing with $TESTTMP/repo (glob)
>     searching for changes
> -  changeset:   3:2655b8ecc4ee
> +  changeset:   4:12042c836390
>     tag:         tip
>     user:        test
>     date:        Thu Jan 01 00:00:00 1970 +0000
> @@ -457,7 +463,7 @@
>     $ hg incoming -S
>     comparing with $TESTTMP/repo2 (glob)
>     searching for changes
> -  changeset:   3:2655b8ecc4ee
> +  changeset:   4:12042c836390
>     tag:         tip
>     user:        test
>     date:        Thu Jan 01 00:00:00 1970 +0000
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>
Angel Ezquerra - Sept. 3, 2014, 9:36 p.m.
On Tue, Sep 2, 2014 at 9:09 PM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
>
>
> On 08/30/2014 05:09 PM, Angel Ezquerra wrote:
>>
>> # HG changeset patch
>> # User Angel Ezquerra <angel.ezquerra@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

Patch

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))
+        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))
+
     # 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
+            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]
+
   $ 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
 
@@ -427,7 +433,7 @@ 
   $ hg outgoing -S
   comparing with $TESTTMP/repo (glob)
   searching for changes
-  changeset:   3:2655b8ecc4ee
+  changeset:   4:12042c836390
   tag:         tip
   user:        test
   date:        Thu Jan 01 00:00:00 1970 +0000
@@ -457,7 +463,7 @@ 
   $ hg incoming -S
   comparing with $TESTTMP/repo2 (glob)
   searching for changes
-  changeset:   3:2655b8ecc4ee
+  changeset:   4:12042c836390
   tag:         tip
   user:        test
   date:        Thu Jan 01 00:00:00 1970 +0000