Patchwork [evolve_ext,V2] fold: take an explicit list of revisions (BC)

login
register
mail settings
Submitter Jordi Gutiérrez Hermoso
Date June 24, 2014, 3:14 p.m.
Message ID <1403622878.10572.3.camel@Iris>
Download mbox | patch
Permalink /patch/5062/
State Not Applicable
Headers show

Comments

Jordi Gutiérrez Hermoso - June 24, 2014, 3:14 p.m.
On Sun, 2014-06-22 at 19:52 -0400, Greg Ward wrote:
> # HG changeset patch
> # User Greg Ward <greg@gerg.ca>
> # Date 1403481137 14400
> #      Sun Jun 22 19:52:17 2014 -0400
> # Node ID 4ab7a80fc11f275c03d4ddb94936a0688b71e6bc
> # Parent  2fbba0bf7e7c8cbff1f94bc95c4d6214df85ef81
> fold: take an explicit list of revisions (BC)

I've got the following WIP instead. Still missing tests, which is why
I haven't patchbombed it yet.

# HG changeset patch
# User Jordi Gutiérrez Hermoso <jordigh@octave.org>
# Date 1397267702 14400
#      Fri Apr 11 21:55:02 2014 -0400
# Branch stable
# Node ID 2e7527f1da736908db6100da5fb24108d43cc3ea
# Parent  ff43167ed0ba60038304a08621bda4cbac5e6224
fold: rehaul handling of multiple and single revisions (BC)

The fold command parsed the revision arguments in a very peculiar and
idiosyncratic fashion: either a single revision could be specified or
multiple revisions could be specified with --rev (but not both). This
is inconsistent with the way all other hg commands parse revision
arguments. We have several examples of command where several revisions
are passed, and the --rev option is optional for specifying those
revisions (update, strip, export).

This patch alters the way in which fold parses its revision arguments.
No distinction is made between revisions passed with or without the
--rev argument. Whether a single or multiple revision is specified, it
will all be folded together into one, by connecting them to the parent
of the working directory. If the --exact argument is passed, then the
parent of the working directory is ignored and the specified revisions
must be a single contiguous line.

The docstring and error messages are modified accordingly, as well as
the tests.
Greg Ward - June 26, 2014, 12:41 p.m.
On 24 June 2014, Jordi Gutiérrez Hermoso said:
> On Sun, 2014-06-22 at 19:52 -0400, Greg Ward wrote:
> > # HG changeset patch
> > # User Greg Ward <greg@gerg.ca>
> > # Date 1403481137 14400
> > #      Sun Jun 22 19:52:17 2014 -0400
> > # Node ID 4ab7a80fc11f275c03d4ddb94936a0688b71e6bc
> > # Parent  2fbba0bf7e7c8cbff1f94bc95c4d6214df85ef81
> > fold: take an explicit list of revisions (BC)
> 
> I've got the following WIP instead. Still missing tests, which is why
> I haven't patchbombed it yet.
> 
> # HG changeset patch
> # User Jordi Gutiérrez Hermoso <jordigh@octave.org>
> # Date 1397267702 14400
> #      Fri Apr 11 21:55:02 2014 -0400
> # Branch stable
> # Node ID 2e7527f1da736908db6100da5fb24108d43cc3ea
> # Parent  ff43167ed0ba60038304a08621bda4cbac5e6224
> fold: rehaul handling of multiple and single revisions (BC)
        ^^^^^^

"overhaul"

> The fold command parsed the revision arguments in a very peculiar and
> idiosyncratic fashion: either a single revision could be specified or
> multiple revisions could be specified with --rev (but not both). This
> is inconsistent with the way all other hg commands parse revision
> arguments. We have several examples of command where several revisions
> are passed, and the --rev option is optional for specifying those
> revisions (update, strip, export).
> 
> This patch alters the way in which fold parses its revision arguments.
> No distinction is made between revisions passed with or without the
> --rev argument. Whether a single or multiple revision is specified, it
> will all be folded together into one, by connecting them to the parent
> of the working directory. If the --exact argument is passed, then the
> parent of the working directory is ignored and the specified revisions
> must be a single contiguous line.

*IF* we accept Pierre-Yves' judgement that "specified revision and all
of its descendants up to ." should be the default, then there is no
need to accept multiple revisions. Just make it

  hg fold [-r] REV

and only accept one REV.

> diff --git a/hgext/evolve.py b/hgext/evolve.py
> --- a/hgext/evolve.py
> +++ b/hgext/evolve.py
> @@ -2053,41 +2053,58 @@ def touch(ui, repo, *revs, **opts):
>          lockmod.release(lock, wlock)
>  
>  @command('^fold|squash',
> -    [('r', 'rev', [], _("explicitly specify the full set of revision to fold")),
> +    [('r', 'rev', [], _("revision to fold")),
> +     ('', 'exact', None, _("ignore parent of working directory"))
>      ] + commitopts + commitopts2,
> -    # allow to choose the seed ?
> -    _('rev'))
> +    _('hg fold [OPTION]... [-r] REV'))

Right! Like that usage message!

>  def fold(ui, repo, *revs, **opts):
> -    """Fold multiple revisions into a single one
> +    """fold multiple revisions into a single one
>  
> -    The revisions from your current working directory to the given one are folded
> -    into a single successor revision.
> +    Folds all revisions between the specified revision and the parent
> +    of working directory into a single revision. The folded revisions
> +    will be marked as obsolete and replaced by the resulting revision.

Yes.

> -    you can alternatively use --rev to explicitly specify revisions to be folded,
> -    ignoring the current working directory parent.
> +    If multiple revisions are specified, they will all be folded
> +    together with the parent of the working directory, along with any
> +    intermediate revisions.
> +
> +    If specifying multiple revisions, use --exact for folding those
> +    revisions while ignoring the parent of the working directory. In
> +    this case, the given revisions must form a linear unbroken chain.

No. Yuck. Confusing. Do not like.

>      """
>      revs = list(revs)
> -    if revs:
> -        if opts.get('rev', ()):
> -            raise util.Abort("cannot specify both --rev and a target revision")
> -        targets = scmutil.revrange(repo, revs)
> -        revs = repo.revs('(%ld::.) or (.::%ld)', targets, targets)
> -    elif 'rev' in opts:
> -        revs = scmutil.revrange(repo, opts['rev'])
> -    else:
> -        revs = ()
> +    revs.extend(opts['rev'])

Assuming this is the best way to implement "-r before REV is
optional", all you need to add is

  if len(revs) > 1:
      raise util.Abort(_('too many revisions specified'))
  rev = revs[0]

and then forget about handling multiple revs.

>      if not revs:
> -        ui.write_err('no revision to fold\n')
> +        ui.write_err(_('no revisions specified\n'))
>          return 1

FWIW, graft just aborts in this case. (But see my comment below about
mixing multiple changes together: you're doing it here.)

> +    if len(revs) == 1:
> +        ui.write_err(_('single revision specified, nothing to fold\n'))
> +        return 2

Oh yeah! I forgot to do this in my patch. Good catch.

> +
>      roots = repo.revs('roots(%ld)', revs)
>      if len(roots) > 1:
> -        raise util.Abort("set has multiple roots")
> +        raise util.Abort(_("cannot fold non-linear revisions"),
> +                           hint=_("multiple roots detected"))

You're mixing multiple changes together. Improving error messages,
adding hints, and I18N are all good things ... but they are distinct
things from "overhaul handling of revisions".

       Greg
Jordi Gutiérrez Hermoso - June 26, 2014, 6:05 p.m.
On Thu, 2014-06-26 at 08:41 -0400, Greg Ward wrote:
> On 24 June 2014, Jordi Gutiérrez Hermoso said:

> > fold: rehaul handling of multiple and single revisions (BC)
>         ^^^^^^
> 
> "overhaul"

Oops, how did I think that "rehaul" was a word?

> *IF* we accept Pierre-Yves' judgement that "specified revision and all
> of its descendants up to ." should be the default, then there is no
> need to accept multiple revisions. 

There is, actually, because one thing you might reasonably want to do
is `hg fold -r 'draft()'` or other revsets that might result in
multiple revisions.

> > +    If specifying multiple revisions, use --exact for folding those
> > +    revisions while ignoring the parent of the working directory. In
> > +    this case, the given revisions must form a linear unbroken chain.
> 
> No. Yuck. Confusing. Do not like.

Right, I get this is contentious. Pierre-Yves actually won me over to
his side when he suggested how people would want to use revsets to
fold. Sadly, we can't afford usability studies to decide which way to
go about this. Polling on the mailing list only results in
self-selected respondents who are already a very minority kind of hg
user (i.e. the kind that subscribes to mailing lists).

I really wish we had a general method for deciding these usability
debates, because arguing on the mailing list about them is kind of
pointless.

> >      """
> >      revs = list(revs)
> > -    if revs:
> > -        if opts.get('rev', ()):
> > -            raise util.Abort("cannot specify both --rev and a target revision")
> > -        targets = scmutil.revrange(repo, revs)
> > -        revs = repo.revs('(%ld::.) or (.::%ld)', targets, targets)
> > -    elif 'rev' in opts:
> > -        revs = scmutil.revrange(repo, opts['rev'])
> > -    else:
> > -        revs = ()
> > +    revs.extend(opts['rev'])
> 
> Assuming this is the best way to implement "-r before REV is
> optional", all you need to add is
> 
>   if len(revs) > 1:
>       raise util.Abort(_('too many revisions specified'))
>   rev = revs[0]
> 
> and then forget about handling multiple revs.

As I said above, I really do think we need to handle multiple
revisions, due to revsets.

> >      if not revs:
> > -        ui.write_err('no revision to fold\n')
> > +        ui.write_err(_('no revisions specified\n'))
> >          return 1
> 
> FWIW, graft just aborts in this case. (But see my comment below about
> mixing multiple changes together: you're doing it here.)

Oh, I suppose strip does too. I can amend this to abort. I was just
following the intent of the original, without really thinking about
it.

> You're mixing multiple changes together. Improving error messages,
> adding hints, and I18N are all good things ... but they are distinct
> things from "overhaul handling of revisions".

Blegh, I hate splitting this. The error messages are related to the
new functionality. The new functionality needs to go all at once. If I
make --rev optional, then I would create an intermediate broken commit
where you cannot work without folding with ".". I could also make
intermediate pointless commits that don't use i18n in the error
messages, but why would you want non-i18n messages to begin with? It's
not like adding _() is that big of a change.

I refuse to create intermediate broken states, so I refuse to split
this patch. I also refuse to create artificial working states that do
not reflect the direction that this patch is taking. I do not see
logical boundaries where individual parts of this patch could work
without the rest. Nor do I see a need to do so.

It's a large patch, and sorry it's difficult to review it all at once,
but I feel it's as atomic as it can be for such a large BC.

- Jordi G. H.
Pierre-Yves David - June 26, 2014, 9:24 p.m.
On 06/26/2014 01:41 PM, Greg Ward wrote:
>> >+    if len(revs) == 1:
>> >+        ui.write_err(_('single revision specified, nothing to fold\n'))
>> >+        return 2
> Oh yeah! I forgot to do this in my patch. Good catch.

I love this message! The current behavior mainly a bug. It is useless 
(and looking at Facebook data set, confusing to user).
Pierre-Yves David - June 26, 2014, 9:41 p.m.
On 06/26/2014 07:05 PM, Jordi Gutiérrez Hermoso wrote:
> On Thu, 2014-06-26 at 08:41 -0400, Greg Ward wrote:
>> On 24 June 2014, Jordi Gutiérrez Hermoso said:
>
>>> fold: rehaul handling of multiple and single revisions (BC)
>>          ^^^^^^
>>
>> "overhaul"
>
> Oops, how did I think that "rehaul" was a word?
>
>> *IF* we accept Pierre-Yves' judgement that "specified revision and all
>> of its descendants up to ." should be the default, then there is no
>> need to accept multiple revisions.
>
> There is, actually, because one thing you might reasonably want to do
> is `hg fold -r 'draft()'` or other revsets that might result in
> multiple revisions.

+1 on what JordiGH says. The old behavior of histedit (who insisted in 
picking the head of the revset as a the value) was a major pain. The 
current behavior (without --rev).

As the benefit to be very versatile:, fold with ancestors, fold with 
descendants, fold with both, fold with revset (as long it contains ".").

>>> +    If specifying multiple revisions, use --exact for folding those
>>> +    revisions while ignoring the parent of the working directory. In
>>> +    this case, the given revisions must form a linear unbroken chain.
>>
>> No. Yuck. Confusing. Do not like.
>
>  Sadly, we can't afford usability studies to decide which way to
> go about this.

Can't we? Facebook have a ##### engineers and we are logging every 
single action they do with Mercurial. Not enough data on fold yet, but 
we'll be able to use those those data before freezing the UI.
Greg Ward - June 28, 2014, 1:15 p.m.
[my patch review]
> You're mixing multiple changes together. Improving error messages,
> adding hints, and I18N are all good things ... but they are distinct
> things from "overhaul handling of revisions".

[Jordi's reply]
> Blegh, I hate splitting this. The error messages are related to the
> new functionality. The new functionality needs to go all at once. If I
> make --rev optional, then I would create an intermediate broken commit
> where you cannot work without folding with ".". I could also make
> intermediate pointless commits that don't use i18n in the error
> messages, but why would you want non-i18n messages to begin with? It's
> not like adding _() is that big of a change.

I was specifically talking about these three changes:

>      roots = repo.revs('roots(%ld)', revs)
>      if len(roots) > 1:
> -        raise util.Abort("set has multiple roots")
> +        raise util.Abort(_("cannot fold non-linear revisions"),
> +                           hint=_("multiple roots detected"))
>      root = repo[roots[0]]
>      if root.phase() <= phases.public:
> -        raise util.Abort("can't fold public revisions")
> +        raise util.Abort(_("cannot fold public revisions"))
>      heads = repo.revs('heads(%ld)', revs)
>      if len(heads) > 1:
> -        raise util.Abort("set has multiple heads")
> +        raise util.Abort(_("cannot fold non-linear revisions"),
> +                           hint=_("multiple heads detected"))

which are all improvements to existing error messages. The middle one
can *obviously* be sent independently of your UI changes. And IIUC, it
looks like the first and third can as well. So do that. They're small,
uncontroversial, and unquestionably make evolve better. Modulo a bit
of bikeshedding, this patch should go in *right now*. But combining it
with the UI overhaul means we have to fight over the UI before we can
get better error messages.

(Certain purists might object that you're combining I18N and improved
wording and adding hints in a single changeset. Nonsense. *I* think
it's fine. Hell, you might even through in a conversion from double
quotes to single quotes. ;-)

My bikeshedding contribution to these error messages: none. I think
they're great.

       Greg

Patch

diff --git a/hgext/evolve.py b/hgext/evolve.py
--- a/hgext/evolve.py
+++ b/hgext/evolve.py
@@ -2053,41 +2053,58 @@  def touch(ui, repo, *revs, **opts):
         lockmod.release(lock, wlock)
 
 @command('^fold|squash',
-    [('r', 'rev', [], _("explicitly specify the full set of revision to fold")),
+    [('r', 'rev', [], _("revision to fold")),
+     ('', 'exact', None, _("ignore parent of working directory"))
     ] + commitopts + commitopts2,
-    # allow to choose the seed ?
-    _('rev'))
+    _('hg fold [OPTION]... [-r] REV'))
 def fold(ui, repo, *revs, **opts):
-    """Fold multiple revisions into a single one
+    """fold multiple revisions into a single one
 
-    The revisions from your current working directory to the given one are folded
-    into a single successor revision.
+    Folds all revisions between the specified revision and the parent
+    of working directory into a single revision. The folded revisions
+    will be marked as obsolete and replaced by the resulting revision.
 
-    you can alternatively use --rev to explicitly specify revisions to be folded,
-    ignoring the current working directory parent.
+    If multiple revisions are specified, they will all be folded
+    together with the parent of the working directory, along with any
+    intermediate revisions.
+
+    If specifying multiple revisions, use --exact for folding those
+    revisions while ignoring the parent of the working directory. In
+    this case, the given revisions must form a linear unbroken chain.
     """
     revs = list(revs)
-    if revs:
-        if opts.get('rev', ()):
-            raise util.Abort("cannot specify both --rev and a target revision")
-        targets = scmutil.revrange(repo, revs)
-        revs = repo.revs('(%ld::.) or (.::%ld)', targets, targets)
-    elif 'rev' in opts:
-        revs = scmutil.revrange(repo, opts['rev'])
-    else:
-        revs = ()
+    revs.extend(opts['rev'])
     if not revs:
-        ui.write_err('no revision to fold\n')
+        ui.write_err(_('no revisions specified\n'))
         return 1
+
+    revs = scmutil.revrange(repo, revs)
+
+    if not opts['exact']:
+        # Try to extend given revision starting from the working directory
+        extrevs = repo.revs('(%ld::.) or (.::%ld)', revs, revs)
+        discardedrevs = [r for r in revs if r not in extrevs]
+        if discardedrevs:
+            raise util.Abort(_("cannot fold non-linear revisions"),
+                               hint=_("given revisions are unrelated to parent "
+                                      "of working directory"))
+        revs = extrevs
+
+    if len(revs) == 1:
+        ui.write_err(_('single revision specified, nothing to fold\n'))
+        return 2
+
     roots = repo.revs('roots(%ld)', revs)
     if len(roots) > 1:
-        raise util.Abort("set has multiple roots")
+        raise util.Abort(_("cannot fold non-linear revisions"),
+                           hint=_("multiple roots detected"))
     root = repo[roots[0]]
     if root.phase() <= phases.public:
-        raise util.Abort("can't fold public revisions")
+        raise util.Abort(_("cannot fold public revisions"))
     heads = repo.revs('heads(%ld)', revs)
     if len(heads) > 1:
-        raise util.Abort("set has multiple heads")
+        raise util.Abort(_("cannot fold non-linear revisions"),
+                           hint=_("multiple heads detected"))
     head = repo[heads[0]]
     wlock = lock = None
     try:
@@ -2108,9 +2125,9 @@  def fold(ui, repo, *revs, **opts):
                 commitopts['message'] =  "\n".join(msgs)
                 commitopts['edit'] = True
 
-            newid, _ = rewrite(repo, root, allctx, head,
-                             [root.p1().node(), root.p2().node()],
-                             commitopts=commitopts)
+            newid, unusedvariable = rewrite(repo, root, allctx, head,
+                                            [root.p1().node(), root.p2().node()],
+                                            commitopts=commitopts)
             phases.retractboundary(repo, targetphase, [newid])
             createmarkers(repo, [(ctx, (repo[newid],))
                                  for ctx in allctx])
diff --git a/tests/test-evolve.t b/tests/test-evolve.t
--- a/tests/test-evolve.t
+++ b/tests/test-evolve.t
@@ -613,10 +613,15 @@  Test fold
 
   $ rm *.orig
   $ hg fold
-  no revision to fold
-  [1]
-  $ hg fold 6 --rev 10
-  abort: cannot specify both --rev and a target revision
+  abort: no revisions specified
+  [255]
+  $ hg fold 5 --rev 10
+  abort: cannot fold non-contiguous revisions
+  (multiple roots detected)
+  [255]
+  $ hg fold -r .
+  abort: cannot fold current revision with itself
+  (specify target revision other than current revision)
   [255]
   $ hg fold 6 # want to run hg fold 6
   2 changesets folded