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