Patchwork [V2] histedit: pick an appropriate base changeset by default (BC)

login
register
mail settings
Submitter Gregory Szorc
Date Dec. 4, 2015, 6:30 a.m.
Message ID <e622d985cf66f761b487.1449210604@ubuntu-main>
Download mbox | patch
Permalink /patch/11805/
State Superseded
Commit 3d0feb2f978b040be05cfde635eb11b3baadf287
Delegated to: Pierre-Yves David
Headers show

Comments

Gregory Szorc - Dec. 4, 2015, 6:30 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1445712999 -3600
#      Sat Oct 24 19:56:39 2015 +0100
# Node ID e622d985cf66f761b4873d846bae4ebe77a8a471
# Parent  71aa5a26162d6e4a165b68f07b331e3e2eedc117
histedit: pick an appropriate base changeset by default (BC)

Previously, `hg histedit` required a revision argument specifying which
revision to use as the base for the current histedit operation. There
was an undocumented and experimental "histedit.defaultrev" option that
supported defining a single revision to be used if no argument is
passed.

Mercurial knows what changesets can be edited. And in most scenarios,
people want to edit this history of everything on the current head that
is rewritable. Making histedit do this by default and not require
an explicit argument or additional configuration is a major usability
win and will enable more people to use histedit.

This patch changes the behavior of the experimental and undocumented
"histedit.defaultrev" config option to select an appropriate base
revision by default. Comprehensive tests exercising the edge cases
in the new, somewhat complicated default revset have been added.
Surprisingly, no tests broke. I guess we were never testing the
behavior with no ANCESTOR argument (it used to fail with
"abort: histedit requires exactly one ancestor revision"). The new
behavior is much more user friendly.

The functionality for choosing the default base revision has been
moved to destutil.py, where it can easily be monkeypatched.
Augie Fackler - Dec. 4, 2015, 7:51 p.m.
On Thu, Dec 03, 2015 at 10:30:04PM -0800, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1445712999 -3600
> #      Sat Oct 24 19:56:39 2015 +0100
> # Node ID e622d985cf66f761b4873d846bae4ebe77a8a471
> # Parent  71aa5a26162d6e4a165b68f07b331e3e2eedc117
> histedit: pick an appropriate base changeset by default (BC)

+1, marmoute please take a look

I'm not sure the limit of 50 makes a ton of sense, but on the other
hand people with stacks that deep probably deserve some speedbump to
get them reevaluating their life choices.

>
> Previously, `hg histedit` required a revision argument specifying which
> revision to use as the base for the current histedit operation. There
> was an undocumented and experimental "histedit.defaultrev" option that
> supported defining a single revision to be used if no argument is
> passed.
>
> Mercurial knows what changesets can be edited. And in most scenarios,
> people want to edit this history of everything on the current head that
> is rewritable. Making histedit do this by default and not require
> an explicit argument or additional configuration is a major usability
> win and will enable more people to use histedit.
>
> This patch changes the behavior of the experimental and undocumented
> "histedit.defaultrev" config option to select an appropriate base
> revision by default. Comprehensive tests exercising the edge cases
> in the new, somewhat complicated default revset have been added.
> Surprisingly, no tests broke. I guess we were never testing the
> behavior with no ANCESTOR argument (it used to fail with
> "abort: histedit requires exactly one ancestor revision"). The new
> behavior is much more user friendly.
>
> The functionality for choosing the default base revision has been
> moved to destutil.py, where it can easily be monkeypatched.
>
> diff --git a/hgext/histedit.py b/hgext/histedit.py
> --- a/hgext/histedit.py
> +++ b/hgext/histedit.py
> @@ -161,16 +161,17 @@ import os
>  import sys
>
>  from mercurial import bundle2
>  from mercurial import cmdutil
>  from mercurial import discovery
>  from mercurial import error
>  from mercurial import copies
>  from mercurial import context
> +from mercurial import destutil
>  from mercurial import exchange
>  from mercurial import extensions
>  from mercurial import hg
>  from mercurial import node
>  from mercurial import repair
>  from mercurial import scmutil
>  from mercurial import util
>  from mercurial import obsolete
> @@ -745,23 +746,29 @@ actiontable = {'p': pick,
>       ('', 'edit-plan', False, _('edit remaining actions list')),
>       ('k', 'keep', False,
>        _("don't strip old nodes after edit is complete")),
>       ('', 'abort', False, _('abort an edit in progress')),
>       ('o', 'outgoing', False, _('changesets not found in destination')),
>       ('f', 'force', False,
>        _('force outgoing even for unrelated repositories')),
>       ('r', 'rev', [], _('first revision to be edited'), _('REV'))],
> -     _("ANCESTOR | --outgoing [URL]"))
> +     _("[ANCESTOR] | --outgoing [URL]"))
>  def histedit(ui, repo, *freeargs, **opts):
>      """interactively edit changeset history
>
> -    This command edits changesets between ANCESTOR and the parent of
> +    This command edits changesets between an ANCESTOR and the parent of
>      the working directory.
>
> +    The value from the "histedit.defaultrev" config option is used as a
> +    revset to select the base revision when ANCESTOR is not specified.
> +    The first revision returned by the revset is used. By default, this
> +    selects the editable history that is unique to the ancestry of the
> +    working directory up to a limit of 50 changesets.
> +
>      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.
>
>      For safety, this command is also aborted if there are ambiguous
>      outgoing revisions which may confuse users: for example, if there
>      are multiple branches containing outgoing revisions.
>
> @@ -876,20 +883,20 @@ def _histedit(ui, repo, state, *freeargs
>              if revs:
>                  raise error.Abort(_('no revisions allowed with --outgoing'))
>              if len(freeargs) > 1:
>                  raise error.Abort(
>                      _('only one repo argument allowed with --outgoing'))
>          else:
>              revs.extend(freeargs)
>              if len(revs) == 0:
> -                # experimental config: histedit.defaultrev
> -                histeditdefault = ui.config('histedit', 'defaultrev')
> -                if histeditdefault:
> -                    revs.append(histeditdefault)
> +                defaultrev = destutil.desthistedit(ui, repo)
> +                if defaultrev is not None:
> +                    revs.append(defaultrev)
> +
>              if len(revs) != 1:
>                  raise error.Abort(
>                      _('histedit requires exactly one ancestor revision'))
>
>
>      replacements = []
>      state.keep = opts.get('keep', False)
>      supportsmarkers = obsolete.isenabled(repo, obsolete.createmarkersopt)
> diff --git a/mercurial/destutil.py b/mercurial/destutil.py
> --- a/mercurial/destutil.py
> +++ b/mercurial/destutil.py
> @@ -193,8 +193,20 @@ def _destmergebranch(repo):
>      return node
>
>  def destmerge(repo):
>      if repo._activebookmark:
>          node = _destmergebook(repo)
>      else:
>          node = _destmergebranch(repo)
>      return repo[node].rev()
> +
> +def desthistedit(ui, repo):
> +    """Default base revision to edit for `hg histedit`."""
> +    default = ui.config('histedit', 'defaultrev',
> +            'limit(reverse(only(.) and not public() and not ::merge()), 50)')
> +    if default:
> +        revs = repo.revs(default)
> +        if revs:
> +            revs.sort()
> +            return revs.first()
> +
> +    return None
> diff --git a/tests/test-histedit-arguments.t b/tests/test-histedit-arguments.t
> --- a/tests/test-histedit-arguments.t
> +++ b/tests/test-histedit-arguments.t
> @@ -342,8 +342,106 @@ Corrupt histedit state file
>    abort: .*(No such file or directory:|The system cannot find the file specified).* (re)
>    [255]
>  Histedit state has been exited
>    $ hg summary -q
>    parent: 5:63379946892c
>    commit: 1 added, 1 unknown (new branch head)
>    update: 4 new changesets (update)
>
> +  $ cd ..
> +
> +Set up default base revision tests
> +
> +  $ hg init defaultbase
> +  $ cd defaultbase
> +  $ touch foo
> +  $ hg -q commit -A -m root
> +  $ echo 1 > foo
> +  $ hg commit -m 'public 1'
> +  $ hg phase --force --public -r .
> +  $ echo 2 > foo
> +  $ hg commit -m 'draft after public'
> +  $ hg -q up -r 1
> +  $ echo 3 > foo
> +  $ hg commit -m 'head 1 public'
> +  created new head
> +  $ hg phase --force --public -r .
> +  $ echo 4 > foo
> +  $ hg commit -m 'head 1 draft 1'
> +  $ echo 5 > foo
> +  $ hg commit -m 'head 1 draft 2'
> +  $ hg -q up -r 2
> +  $ echo 6 > foo
> +  $ hg commit -m 'head 2 commit 1'
> +  $ echo 7 > foo
> +  $ hg commit -m 'head 2 commit 2'
> +  $ hg -q up -r 2
> +  $ echo 8 > foo
> +  $ hg commit -m 'head 3'
> +  created new head
> +  $ hg -q up -r 2
> +  $ echo 9 > foo
> +  $ hg commit -m 'head 4'
> +  created new head
> +  $ hg merge --tool :local -r 8
> +  0 files updated, 1 files merged, 0 files removed, 0 files unresolved
> +  (branch merge, don't forget to commit)
> +  $ hg commit -m 'merge head 3 into head 4'
> +  $ echo 11 > foo
> +  $ hg commit -m 'commit 1 after merge'
> +  $ echo 12 > foo
> +  $ hg commit -m 'commit 2 after merge'
> +
> +  $ hg log -G -T '{rev}:{node|short} {phase} {desc}\n'
> +  @  12:8cde254db839 draft commit 2 after merge
> +  |
> +  o  11:6f2f0241f119 draft commit 1 after merge
> +  |
> +  o    10:90506cc76b00 draft merge head 3 into head 4
> +  |\
> +  | o  9:f8607a373a97 draft head 4
> +  | |
> +  o |  8:0da92be05148 draft head 3
> +  |/
> +  | o  7:4c35cdf97d5e draft head 2 commit 2
> +  | |
> +  | o  6:931820154288 draft head 2 commit 1
> +  |/
> +  | o  5:8cdc02b9bc63 draft head 1 draft 2
> +  | |
> +  | o  4:463b8c0d2973 draft head 1 draft 1
> +  | |
> +  | o  3:23a0c4eefcbf public head 1 public
> +  | |
> +  o |  2:4117331c3abb draft draft after public
> +  |/
> +  o  1:4426d359ea59 public public 1
> +  |
> +  o  0:54136a8ddf32 public root
> +
> +
> +Default base revision should stop at public changesets
> +
> +  $ hg -q up 8cdc02b9bc63
> +  $ hg histedit --commands - <<EOF
> +  > pick 463b8c0d2973
> +  > pick 8cdc02b9bc63
> +  > EOF
> +  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +
> +Default base revision should stop at branchpoint
> +
> +  $ hg -q up 4c35cdf97d5e
> +  $ hg histedit --commands - <<EOF
> +  > pick 931820154288
> +  > pick 4c35cdf97d5e
> +  > EOF
> +  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +
> +Default base revision should stop at merge commit
> +
> +  $ hg -q up 8cde254db839
> +  $ hg histedit --commands - <<EOF
> +  > pick 6f2f0241f119
> +  > pick 8cde254db839
> +  > EOF
> +  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - Dec. 6, 2015, 8:46 a.m.
On 12/03/2015 10:30 PM, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1445712999 -3600
> #      Sat Oct 24 19:56:39 2015 +0100
> # Node ID e622d985cf66f761b4873d846bae4ebe77a8a471
> # Parent  71aa5a26162d6e4a165b68f07b331e3e2eedc117
> histedit: pick an appropriate base changeset by default (BC)
>
> Previously, `hg histedit` required a revision argument specifying which
> revision to use as the base for the current histedit operation. There
> was an undocumented and experimental "histedit.defaultrev" option that
> supported defining a single revision to be used if no argument is
> passed.

I'm considered bikesheding the config name (to move that in a config 
section dedicated to default destination). However, we already have an 
histedit section (for another debatable option). So lets shove in that 
histedit section, we can still deprecate the whole thing if we come with 
a good standard definition for "your stack".

[...]
> --- a/mercurial/destutil.py
> +++ b/mercurial/destutil.py
> @@ -193,8 +193,20 @@ def _destmergebranch(repo):
>       return node
>
>   def destmerge(repo):
>       if repo._activebookmark:
>           node = _destmergebook(repo)
>       else:
>           node = _destmergebranch(repo)
>       return repo[node].rev()
> +
> +def desthistedit(ui, repo):
> +    """Default base revision to edit for `hg histedit`."""
> +    default = ui.config('histedit', 'defaultrev',
> +            'limit(reverse(only(.) and not public() and not ::merge()), 50)')

Lets put the default in a temporary variable to avoid the huge line wrap.

While we are at it, lets put it as a top level constant to help wrapper 
who would like to change just that.

> +    if default:
> +        revs = repo.revs(default)
> +        if revs:
> +            revs.sort()
> +            return revs.first()

I a got a 5 seconds puzzling about why this you were sorting an already 
sorted revset and why the 'first()' was not part of the revset itself.

While you code is correct (as user config are not guaranteed to have the 
sorting and first), you should probably add a comment about it to make 
sure a future reader do not miss that fact.

Patch

diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -161,16 +161,17 @@  import os
 import sys
 
 from mercurial import bundle2
 from mercurial import cmdutil
 from mercurial import discovery
 from mercurial import error
 from mercurial import copies
 from mercurial import context
+from mercurial import destutil
 from mercurial import exchange
 from mercurial import extensions
 from mercurial import hg
 from mercurial import node
 from mercurial import repair
 from mercurial import scmutil
 from mercurial import util
 from mercurial import obsolete
@@ -745,23 +746,29 @@  actiontable = {'p': pick,
      ('', 'edit-plan', False, _('edit remaining actions list')),
      ('k', 'keep', False,
       _("don't strip old nodes after edit is complete")),
      ('', 'abort', False, _('abort an edit in progress')),
      ('o', 'outgoing', False, _('changesets not found in destination')),
      ('f', 'force', False,
       _('force outgoing even for unrelated repositories')),
      ('r', 'rev', [], _('first revision to be edited'), _('REV'))],
-     _("ANCESTOR | --outgoing [URL]"))
+     _("[ANCESTOR] | --outgoing [URL]"))
 def histedit(ui, repo, *freeargs, **opts):
     """interactively edit changeset history
 
-    This command edits changesets between ANCESTOR and the parent of
+    This command edits changesets between an ANCESTOR and the parent of
     the working directory.
 
+    The value from the "histedit.defaultrev" config option is used as a
+    revset to select the base revision when ANCESTOR is not specified.
+    The first revision returned by the revset is used. By default, this
+    selects the editable history that is unique to the ancestry of the
+    working directory up to a limit of 50 changesets.
+
     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.
 
     For safety, this command is also aborted if there are ambiguous
     outgoing revisions which may confuse users: for example, if there
     are multiple branches containing outgoing revisions.
 
@@ -876,20 +883,20 @@  def _histedit(ui, repo, state, *freeargs
             if revs:
                 raise error.Abort(_('no revisions allowed with --outgoing'))
             if len(freeargs) > 1:
                 raise error.Abort(
                     _('only one repo argument allowed with --outgoing'))
         else:
             revs.extend(freeargs)
             if len(revs) == 0:
-                # experimental config: histedit.defaultrev
-                histeditdefault = ui.config('histedit', 'defaultrev')
-                if histeditdefault:
-                    revs.append(histeditdefault)
+                defaultrev = destutil.desthistedit(ui, repo)
+                if defaultrev is not None:
+                    revs.append(defaultrev)
+
             if len(revs) != 1:
                 raise error.Abort(
                     _('histedit requires exactly one ancestor revision'))
 
 
     replacements = []
     state.keep = opts.get('keep', False)
     supportsmarkers = obsolete.isenabled(repo, obsolete.createmarkersopt)
diff --git a/mercurial/destutil.py b/mercurial/destutil.py
--- a/mercurial/destutil.py
+++ b/mercurial/destutil.py
@@ -193,8 +193,20 @@  def _destmergebranch(repo):
     return node
 
 def destmerge(repo):
     if repo._activebookmark:
         node = _destmergebook(repo)
     else:
         node = _destmergebranch(repo)
     return repo[node].rev()
+
+def desthistedit(ui, repo):
+    """Default base revision to edit for `hg histedit`."""
+    default = ui.config('histedit', 'defaultrev',
+            'limit(reverse(only(.) and not public() and not ::merge()), 50)')
+    if default:
+        revs = repo.revs(default)
+        if revs:
+            revs.sort()
+            return revs.first()
+
+    return None
diff --git a/tests/test-histedit-arguments.t b/tests/test-histedit-arguments.t
--- a/tests/test-histedit-arguments.t
+++ b/tests/test-histedit-arguments.t
@@ -342,8 +342,106 @@  Corrupt histedit state file
   abort: .*(No such file or directory:|The system cannot find the file specified).* (re)
   [255]
 Histedit state has been exited
   $ hg summary -q
   parent: 5:63379946892c 
   commit: 1 added, 1 unknown (new branch head)
   update: 4 new changesets (update)
 
+  $ cd ..
+
+Set up default base revision tests
+
+  $ hg init defaultbase
+  $ cd defaultbase
+  $ touch foo
+  $ hg -q commit -A -m root
+  $ echo 1 > foo
+  $ hg commit -m 'public 1'
+  $ hg phase --force --public -r .
+  $ echo 2 > foo
+  $ hg commit -m 'draft after public'
+  $ hg -q up -r 1
+  $ echo 3 > foo
+  $ hg commit -m 'head 1 public'
+  created new head
+  $ hg phase --force --public -r .
+  $ echo 4 > foo
+  $ hg commit -m 'head 1 draft 1'
+  $ echo 5 > foo
+  $ hg commit -m 'head 1 draft 2'
+  $ hg -q up -r 2
+  $ echo 6 > foo
+  $ hg commit -m 'head 2 commit 1'
+  $ echo 7 > foo
+  $ hg commit -m 'head 2 commit 2'
+  $ hg -q up -r 2
+  $ echo 8 > foo
+  $ hg commit -m 'head 3'
+  created new head
+  $ hg -q up -r 2
+  $ echo 9 > foo
+  $ hg commit -m 'head 4'
+  created new head
+  $ hg merge --tool :local -r 8
+  0 files updated, 1 files merged, 0 files removed, 0 files unresolved
+  (branch merge, don't forget to commit)
+  $ hg commit -m 'merge head 3 into head 4'
+  $ echo 11 > foo
+  $ hg commit -m 'commit 1 after merge'
+  $ echo 12 > foo
+  $ hg commit -m 'commit 2 after merge'
+
+  $ hg log -G -T '{rev}:{node|short} {phase} {desc}\n'
+  @  12:8cde254db839 draft commit 2 after merge
+  |
+  o  11:6f2f0241f119 draft commit 1 after merge
+  |
+  o    10:90506cc76b00 draft merge head 3 into head 4
+  |\
+  | o  9:f8607a373a97 draft head 4
+  | |
+  o |  8:0da92be05148 draft head 3
+  |/
+  | o  7:4c35cdf97d5e draft head 2 commit 2
+  | |
+  | o  6:931820154288 draft head 2 commit 1
+  |/
+  | o  5:8cdc02b9bc63 draft head 1 draft 2
+  | |
+  | o  4:463b8c0d2973 draft head 1 draft 1
+  | |
+  | o  3:23a0c4eefcbf public head 1 public
+  | |
+  o |  2:4117331c3abb draft draft after public
+  |/
+  o  1:4426d359ea59 public public 1
+  |
+  o  0:54136a8ddf32 public root
+  
+
+Default base revision should stop at public changesets
+
+  $ hg -q up 8cdc02b9bc63
+  $ hg histedit --commands - <<EOF
+  > pick 463b8c0d2973
+  > pick 8cdc02b9bc63
+  > EOF
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
+
+Default base revision should stop at branchpoint
+
+  $ hg -q up 4c35cdf97d5e
+  $ hg histedit --commands - <<EOF
+  > pick 931820154288
+  > pick 4c35cdf97d5e
+  > EOF
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
+
+Default base revision should stop at merge commit
+
+  $ hg -q up 8cde254db839
+  $ hg histedit --commands - <<EOF
+  > pick 6f2f0241f119
+  > pick 8cde254db839
+  > EOF
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved