[PATCH 1 of 2] commit: properly handle the combination of --subrepos and --addremove options
Pierre-Yves David
pierre-yves.david at ens-lyon.org
Tue Sep 2 19:09:48 UTC 2014
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.
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 at selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>
--
Pierre-Yves David
More information about the Mercurial-devel
mailing list