Patchwork [1,of,1,V2] histedit: abort if there are multiple roots in "--outgoing" revisions

login
register
mail settings
Submitter Katsunori FUJIWARA
Date Sept. 9, 2013, 1:51 p.m.
Message ID <f0edc4e70f976b20d0c8.1378734690@feefifofum>
Download mbox | patch
Permalink /patch/2415/
State Superseded
Headers show

Comments

Katsunori FUJIWARA - Sept. 9, 2013, 1:51 p.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1378733863 -32400
#      Mon Sep 09 22:37:43 2013 +0900
# Node ID f0edc4e70f976b20d0c886a287b472a629789329
# Parent  1d07bf106c2ad1c7ef5e257e754ca8d858bd04b0
histedit: abort if there are multiple roots in "--outgoing" revisions

Before this patch, if there are multiple roots in "--outgoing"
revisions, result of "histedit --outgoing" depends on the parent of
the working directory. It succeeds only when the parent of the working
directory is a descendant of the oldest root in "--outgoing"
revisions, and fails otherwise.

It seems to be ambiguous and difficult for users.

This patch makes "histedit --outgoing" abort if there are multiple
roots in "--outgoing" revisions always.

This patch also recommends to use "min(outgoing() and ::.)" or similar
revset specification in such ambiguous situation by online help.
Matt Mackall - Sept. 14, 2013, 8:12 p.m.
On Thu, 2013-09-12 at 22:33 -0500, Kevin Bullock wrote:
> On 9 Sep 2013, at 8:51 AM, FUJIWARA Katsunori wrote:
> 
> > # HG changeset patch
> > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > # Date 1378733863 -32400
> > #      Mon Sep 09 22:37:43 2013 +0900
> > # Node ID f0edc4e70f976b20d0c886a287b472a629789329
> > # Parent  1d07bf106c2ad1c7ef5e257e754ca8d858bd04b0
> > histedit: abort if there are multiple roots in "--outgoing" revisions
> 
> LGTM except that I think we need to bikeshed the messages we give the
> user here. Telling them the outgoing changesets have "multiple roots"
> will be terrifying. I don't have any better suggestions, though (yet).

Can we bikeshed by follow-up patch then?

This appears to be a bugfix that belongs on stable, yes?
Katsunori FUJIWARA - Sept. 16, 2013, 7:39 p.m.
At Thu, 12 Sep 2013 22:33:04 -0500,
Kevin Bullock wrote:
> 
> On 9 Sep 2013, at 8:51 AM, FUJIWARA Katsunori wrote:
> 
> > # HG changeset patch
> > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > # Date 1378733863 -32400
> > #      Mon Sep 09 22:37:43 2013 +0900
> > # Node ID f0edc4e70f976b20d0c886a287b472a629789329
> > # Parent  1d07bf106c2ad1c7ef5e257e754ca8d858bd04b0
> > histedit: abort if there are multiple roots in "--outgoing" revisions
> 
> LGTM except that I think we need to bikeshed the messages we give the user here. Telling them the outgoing changesets have "multiple roots" will be terrifying. I don't have any better suggestions, though (yet).

What about the idea blow ?

  - show abort message "there are outgoing revisions which may confuse
    users" (and the hint to lead users to "hg help histedit"), and

  - explain more detail about "--outgoing", like:

      For safety, this command is aborted, also if there are outgoing
      revisions which may confuse users: for example, there may be
      another unexpected outgoing branches, if number of "root
      revisions in outgoings" is two or more.

      Use "min(outgoing() and ::.)" or similar revset expression as
      ANCESTOR instead of "--outgoing" option, to specify only one
      root of outgoing ancestors of the working directory in such
      ambiguous situation. Please see :hg:`help revsets` for detail
      about selecting revisions.


I hope revise for messages by native speakers :-)


----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Katsunori FUJIWARA - Sept. 16, 2013, 8:25 p.m.
At Sat, 14 Sep 2013 15:12:13 -0500,
Matt Mackall wrote:
> 
> On Thu, 2013-09-12 at 22:33 -0500, Kevin Bullock wrote:
> > On 9 Sep 2013, at 8:51 AM, FUJIWARA Katsunori wrote:
> > 
> > > # HG changeset patch
> > > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > > # Date 1378733863 -32400
> > > #      Mon Sep 09 22:37:43 2013 +0900
> > > # Node ID f0edc4e70f976b20d0c886a287b472a629789329
> > > # Parent  1d07bf106c2ad1c7ef5e257e754ca8d858bd04b0
> > > histedit: abort if there are multiple roots in "--outgoing" revisions
> > 
> > LGTM except that I think we need to bikeshed the messages we give the
> > user here. Telling them the outgoing changesets have "multiple roots"
> > will be terrifying. I don't have any better suggestions, though (yet).
> 
> Can we bikeshed by follow-up patch then?
> 
> This appears to be a bugfix that belongs on stable, yes?

I don't think strongly that this should be fiexed on stable, because
(1) no issue has been reported yet about "histedit --outgoing" AFAIK
(according to BTS search result), and (2) this problem doesn't seem so
serious. And, I dropped "STABLE" flag from this patch series.

But I don't have any objection to apply this on stable, of course :-)


----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Matt Mackall - Sept. 16, 2013, 9:01 p.m.
On Tue, 2013-09-17 at 05:25 +0900, FUJIWARA Katsunori wrote:
> At Sat, 14 Sep 2013 15:12:13 -0500,
> Matt Mackall wrote:
> > 
> > On Thu, 2013-09-12 at 22:33 -0500, Kevin Bullock wrote:
> > > On 9 Sep 2013, at 8:51 AM, FUJIWARA Katsunori wrote:
> > > 
> > > > # HG changeset patch
> > > > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > > > # Date 1378733863 -32400
> > > > #      Mon Sep 09 22:37:43 2013 +0900
> > > > # Node ID f0edc4e70f976b20d0c886a287b472a629789329
> > > > # Parent  1d07bf106c2ad1c7ef5e257e754ca8d858bd04b0
> > > > histedit: abort if there are multiple roots in "--outgoing" revisions
> > > 
> > > LGTM except that I think we need to bikeshed the messages we give the
> > > user here. Telling them the outgoing changesets have "multiple roots"
> > > will be terrifying. I don't have any better suggestions, though (yet).
> > 
> > Can we bikeshed by follow-up patch then?
> > 
> > This appears to be a bugfix that belongs on stable, yes?
> 
> I don't think strongly that this should be fiexed on stable, because
> (1) no issue has been reported yet about "histedit --outgoing" AFAIK
> (according to BTS search result), and (2) this problem doesn't seem so
> serious. And, I dropped "STABLE" flag from this patch series.

Having a BTS issue is not a requirement for fixing a bug. If histedit
already does something vaguely reasonable in this case, then perhaps
it's not a bug. But it seems like it's doing something silly currently.
Augie Fackler - Sept. 19, 2013, 2:40 p.m.
On Mon, Sep 16, 2013 at 04:01:01PM -0500, Matt Mackall wrote:
> On Tue, 2013-09-17 at 05:25 +0900, FUJIWARA Katsunori wrote:
> > At Sat, 14 Sep 2013 15:12:13 -0500,
> > Matt Mackall wrote:
> > >
> > > On Thu, 2013-09-12 at 22:33 -0500, Kevin Bullock wrote:
> > > > On 9 Sep 2013, at 8:51 AM, FUJIWARA Katsunori wrote:
> > > >
> > > > > # HG changeset patch
> > > > > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > > > > # Date 1378733863 -32400
> > > > > #      Mon Sep 09 22:37:43 2013 +0900
> > > > > # Node ID f0edc4e70f976b20d0c886a287b472a629789329
> > > > > # Parent  1d07bf106c2ad1c7ef5e257e754ca8d858bd04b0
> > > > > histedit: abort if there are multiple roots in "--outgoing" revisions
> > > >
> > > > LGTM except that I think we need to bikeshed the messages we give the
> > > > user here. Telling them the outgoing changesets have "multiple roots"
> > > > will be terrifying. I don't have any better suggestions, though (yet).
> > >
> > > Can we bikeshed by follow-up patch then?
> > >
> > > This appears to be a bugfix that belongs on stable, yes?
> >
> > I don't think strongly that this should be fiexed on stable, because
> > (1) no issue has been reported yet about "histedit --outgoing" AFAIK
> > (according to BTS search result), and (2) this problem doesn't seem so
> > serious. And, I dropped "STABLE" flag from this patch series.
>
> Having a BTS issue is not a requirement for fixing a bug. If histedit
> already does something vaguely reasonable in this case, then perhaps
> it's not a bug. But it seems like it's doing something silly currently.

Stable seems appropriate for this, yes.


>
> --
> Mathematics is the supreme nostalgia of our time.
>
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel

Patch

diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -422,7 +422,12 @@ 
     outgoing = discovery.findcommonoutgoing(repo, other, revs, force=force)
     if not outgoing.missing:
         raise util.Abort(_('no outgoing ancestors'))
-    return outgoing.missing[0]
+    roots = list(repo.set("roots(%ln)", outgoing.missing))
+    if 1 < len(roots):
+        msg = _('there are multiple roots in outgoing revisions')
+        hint = _('see "hg help histedit" for detail about --outgoing')
+        raise util.Abort(msg, hint=hint)
+    return repo.lookup(roots[0])
 
 actiontable = {'p': pick,
                'pick': pick,
@@ -457,6 +462,13 @@ 
     With --outgoing, this edits changesets not found in the
     destination repository. If URL of the destination is omitted, the
     'default-push' (or 'default') path will be used.
+
+    This command is aborted, if there are multiple roots in revisions
+    specified by --outgoing, to avoid editing unexpected
+    revisions. Use "min(outgoing() and ::.)" or similar revset
+    specification instead of --outgoing to specify the root of
+    outgoing ancestors of the working directory in such ambiguous
+    situation.
     """
     # TODO only abort if we try and histedit mq patches, not just
     # blanket if mq patches are applied somewhere
diff --git a/tests/test-histedit-outgoing.t b/tests/test-histedit-outgoing.t
--- a/tests/test-histedit-outgoing.t
+++ b/tests/test-histedit-outgoing.t
@@ -102,4 +102,38 @@ 
   #  m, mess = edit message without changing commit content
   #
   0 files updated, 0 files merged, 0 files removed, 0 files unresolved
+
+test to check number of roots in outgoing revisions
+
+  $ hg -q outgoing -G --template '{node|short}({branch})' '../r'
+  @  f26599ee3441(foo)
+  
+  o  652413bf663e(default)
+  |
+  o  e860deea161a(default)
+  |
+  o  055a42cdd887(default)
+  
+  $ HGEDITOR=cat hg -q histedit --outgoing '../r'
+  abort: there are multiple roots in outgoing revisions
+  (see "hg help histedit" for detail about --outgoing)
+  [255]
+
+  $ hg -q update -C 2
+  $ echo aa >> a
+  $ hg -q commit -m 'another head on default'
+  $ hg -q outgoing -G --template '{node|short}({branch})' '../r#default'
+  @  3879dc049647(default)
+  
+  o  652413bf663e(default)
+  |
+  o  e860deea161a(default)
+  |
+  o  055a42cdd887(default)
+  
+  $ HGEDITOR=cat hg -q histedit --outgoing '../r#default'
+  abort: there are multiple roots in outgoing revisions
+  (see "hg help histedit" for detail about --outgoing)
+  [255]
+
   $ cd ..