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

login
register
mail settings
Submitter Greg Ward
Date June 22, 2014, 11:52 p.m.
Message ID <4ab7a80fc11f275c03d4.1403481156@lucifer.gerg.ca>
Download mbox | patch
Permalink /patch/5041/
State Changes Requested
Headers show

Comments

Greg Ward - June 22, 2014, 11:52 p.m.
# 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)

This means anyone used to running "hg fold REV" will have to change
their habit to "hg fold REV::". The upside is that the new interface
is simpler, easier to describe, and more consistent with other hg
commands.

UI and code are modeled on graft, which similiarly takes one or more
input revisions, either with or without '-r'.
Pierre-Yves David - June 23, 2014, 9:29 a.m.
On 06/22/2014 04:52 PM, 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)

No thanks.


This use the be the default and ended up being very confusing. The vast 
majority of fold operation involve the working directory and an 
ancestors (something a descendant). The default should reflect that.
Also, we should not requires user to have a PhD in revset to use Mercurial.

So, I'm deeply convince the default should be "fold between . and REV".

How ever I agree than having different behavior for `hg fold REV` and 
`hg fold --rev REV` sucks. I also agree that the "do an in memory fold 
of unrelated changeset" usecase is nice. We should think about a 
dedicated flag for this case.
Greg Ward - June 24, 2014, 1:02 a.m.
On 23 June 2014, Pierre-Yves David said:
> On 06/22/2014 04:52 PM, 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)
> 
> No thanks.

Darn. I guess I misunderstood our previous discussion about this.

> This use the be the default and ended up being very confusing. The
> vast majority of fold operation involve the working directory and an
> ancestors (something a descendant). The default should reflect that.
> Also, we should not requires user to have a PhD in revset to use Mercurial.

"REV::" is not a PhD-level revset.

> So, I'm deeply convince the default should be "fold between . and REV".

OK, fine. I happen to disagree, but whatever.

> How ever I agree than having different behavior for `hg fold REV`
> and `hg fold --rev REV` sucks.

OK, I'll see if I can fix that.

> I also agree that the "do an in
> memory fold of unrelated changeset" usecase is nice. We should think
> about a dedicated flag for this case.

--only? Example usage:

  hg fold --only R1 R2 ... Rn

combines R1, R2, and Rn to make a new changeset.

       Greg
Sean Farley - June 24, 2014, 5:56 a.m.
Greg Ward writes:

> On 23 June 2014, Pierre-Yves David said:
>> On 06/22/2014 04:52 PM, 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)
>> 
>> No thanks.
>
> Darn. I guess I misunderstood our previous discussion about this.
>
>> This use the be the default and ended up being very confusing. The
>> vast majority of fold operation involve the working directory and an
>> ancestors (something a descendant). The default should reflect that.
>> Also, we should not requires user to have a PhD in revset to use Mercurial.
>
> "REV::" is not a PhD-level revset.
>
>> So, I'm deeply convince the default should be "fold between . and REV".
>
> OK, fine. I happen to disagree, but whatever.

I strongly agree with Greg on this one. 'hg fold' is horribly
confusing. I posit that your sample space is too small.

>> How ever I agree than having different behavior for `hg fold REV`
>> and `hg fold --rev REV` sucks.
>
> OK, I'll see if I can fix that.
>
>> I also agree that the "do an in
>> memory fold of unrelated changeset" usecase is nice. We should think
>> about a dedicated flag for this case.

What is this? Git? If you're so strongly convinced that 'hg fold' should
default to .^::. then why don't you make an alias (or potentially, a
revset alias)?
Augie Fackler - June 24, 2014, 6:57 p.m.
On Mon, Jun 23, 2014 at 09:02:14PM -0400, Greg Ward wrote:
> On 23 June 2014, Pierre-Yves David said:
> > On 06/22/2014 04:52 PM, 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)
> >
> > No thanks.
>
> Darn. I guess I misunderstood our previous discussion about this.
>
> > This use the be the default and ended up being very confusing. The
> > vast majority of fold operation involve the working directory and an
> > ancestors (something a descendant). The default should reflect that.
> > Also, we should not requires user to have a PhD in revset to use Mercurial.
>
> "REV::" is not a PhD-level revset.
>
> > So, I'm deeply convince the default should be "fold between . and REV".
>
> OK, fine. I happen to disagree, but whatever.

I agree with Greg here - the magic default of 'hg fold $REV' -> fold
together $REV::. feels a little spooky. This feels like the right case
for an alias to me.

(It's been stated by many that histedits behavior in this area is
confusing, and with the benefit of hindsight and the existence of
evolution I'm starting to come around on that point.)

>
> > How ever I agree than having different behavior for `hg fold REV`
> > and `hg fold --rev REV` sucks.
>
> OK, I'll see if I can fix that.
>
> > I also agree that the "do an in
> > memory fold of unrelated changeset" usecase is nice. We should think
> > about a dedicated flag for this case.
>
> --only? Example usage:
>
>   hg fold --only R1 R2 ... Rn
>
> combines R1, R2, and Rn to make a new changeset.
>
>        Greg
> --
> Greg Ward                            http://www.gerg.ca
> <greg@gerg.ca>                       @gergdotca
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Greg Ward - June 26, 2014, 12:47 p.m.
On 24 June 2014, Augie Fackler said:
> I agree with Greg here - the magic default of 'hg fold $REV' -> fold
> together $REV::. feels a little spooky. This feels like the right case
> for an alias to me.

OR if it's as important a use case as Pierre-Yves claims, add an
option. E.g. make

  hg fold [-r] REV...

(as implemented in my patch) the default. Then add

  hg fold --from REV

to restore the current (REV::.) behaviour.

Hmmm. Time for a poll? Here are the options:

  1) status quo: "hg fold REV" and "hg fold -r REV" are different

  2) my proposal: "hg fold [-r] REV...", with --from option
     to satisfy people who like the current "hg fold REV" semantics

  3) Pierre-Yves' proposal: "hg fold [-r] REV" same as current
     "hg fold REV", with "hg fold --only REV..." option to
     satisfy people who like the current "hg fold -r REV..." semantics

I'm pretty sure no one will vote for #1. Go ahead, surprise me!

Sounds like Sean, Augie, and I are for #2.

Clearly Pierre-Yves likes #3. Who else?

Vote early, vote often.

       Greg
Angel Ezquerra - June 26, 2014, 1:23 p.m.
El 26/06/2014 14:47, "Greg Ward" <greg@gerg.ca> escribió:
>
> On 24 June 2014, Augie Fackler said:
> > I agree with Greg here - the magic default of 'hg fold $REV' -> fold
> > together $REV::. feels a little spooky. This feels like the right case
> > for an alias to me.
>
> OR if it's as important a use case as Pierre-Yves claims, add an
> option. E.g. make
>
>   hg fold [-r] REV...
>
> (as implemented in my patch) the default. Then add
>
>   hg fold --from REV
>
> to restore the current (REV::.) behaviour.
>
> Hmmm. Time for a poll? Here are the options:
>
>   1) status quo: "hg fold REV" and "hg fold -r REV" are different
>
>   2) my proposal: "hg fold [-r] REV...", with --from option
>      to satisfy people who like the current "hg fold REV" semantics
>
>   3) Pierre-Yves' proposal: "hg fold [-r] REV" same as current
>      "hg fold REV", with "hg fold --only REV..." option to
>      satisfy people who like the current "hg fold -r REV..." semantics
>
> I'm pretty sure no one will vote for #1. Go ahead, surprise me!
>
> Sounds like Sean, Augie, and I are for #2.
>
> Clearly Pierre-Yves likes #3. Who else?
>
> Vote early, vote often.

I think I prefer #2.

Cheers,

Angel
Sean Farley - June 26, 2014, 6:55 p.m.
Greg Ward writes:

> On 24 June 2014, Augie Fackler said:
>> I agree with Greg here - the magic default of 'hg fold $REV' -> fold
>> together $REV::. feels a little spooky. This feels like the right case
>> for an alias to me.
>
> OR if it's as important a use case as Pierre-Yves claims, add an
> option. E.g. make
>
>   hg fold [-r] REV...
>
> (as implemented in my patch) the default. Then add
>
>   hg fold --from REV
>
> to restore the current (REV::.) behaviour.
>
> Hmmm. Time for a poll? Here are the options:
>
>   1) status quo: "hg fold REV" and "hg fold -r REV" are different
>
>   2) my proposal: "hg fold [-r] REV...", with --from option
>      to satisfy people who like the current "hg fold REV" semantics
>
>   3) Pierre-Yves' proposal: "hg fold [-r] REV" same as current
>      "hg fold REV", with "hg fold --only REV..." option to
>      satisfy people who like the current "hg fold -r REV..." semantics
>
> I'm pretty sure no one will vote for #1. Go ahead, surprise me!
>
> Sounds like Sean, Augie, and I are for #2.

There's a big of hack we could do here:

a) $ hg fold
b) $ hg fold SINGLEREV
c) $ hg fold REVSET 

Currently, (a) doesn't make sense so we could have it default to folding
with the parent '.'. Also, (b), I think, only makes sense if you want to
fold from '.' to SINGLEREV. That leaves (c) to be an arbitrary fold that
could work with an in-memory implementation.

> Clearly Pierre-Yves likes #3. Who else?

No one. Pierre-Yves is dangerously close to:

https://yourlogicalfallacyis.com/anecdotal
https://yourlogicalfallacyis.com/appeal-to-nature
Jordi Gutiérrez Hermoso - June 26, 2014, 7:38 p.m.
On Thu, 2014-06-26 at 13:55 -0500, Sean Farley wrote:
> Greg Ward writes:
> 
> > On 24 June 2014, Augie Fackler said:
> >> I agree with Greg here - the magic default of 'hg fold $REV' -> fold
> >> together $REV::. feels a little spooky. This feels like the right case
> >> for an alias to me.
> >
> > OR if it's as important a use case as Pierre-Yves claims, add an
> > option. E.g. make
> >
> >   hg fold [-r] REV...
> >
> > (as implemented in my patch) the default. Then add
> >
> >   hg fold --from REV
> >
> > to restore the current (REV::.) behaviour.
> >
> > Hmmm. Time for a poll? Here are the options:
> >
> >   1) status quo: "hg fold REV" and "hg fold -r REV" are different
> >
> >   2) my proposal: "hg fold [-r] REV...", with --from option
> >      to satisfy people who like the current "hg fold REV" semantics
> >
> >   3) Pierre-Yves' proposal: "hg fold [-r] REV" same as current
> >      "hg fold REV", with "hg fold --only REV..." option to
> >      satisfy people who like the current "hg fold -r REV..." semantics
> >
> > I'm pretty sure no one will vote for #1. Go ahead, surprise me!
> >
> > Sounds like Sean, Augie, and I are for #2.
[snip]
> > Clearly Pierre-Yves likes #3. Who else?
> 
> No one. Pierre-Yves is dangerously close to:

I like #3, and I have two votes from a git users that it would be a
good thing. I also got a vote for #3 from two seasoned git users to
whom I described this feature. I have one vote against from a user who
is new to git and hg and has used the current behaviour in error and
doesn't know how to recover from it easily.

I think there is a general principle that could be invoked here,
though: explicit is better than implicit. It would be best if there is
no implicit magic with "." that can confuse people like my n00b who
doesn't know how to easily recover from these folds If you really
wanted to fold with ".", then you have to explicitly say it, at least
by default.

Looks like I just talked myself into #2 as well... Huh.

- Jordi G. H.
Pierre-Yves David - June 26, 2014, 8:49 p.m.
TL;DR; new user (1) are working copy centric and should (2) should not 
be exposed to "revset" when unnecessary.

On 06/24/2014 02:02 AM, Greg Ward wrote:
> On 23 June 2014, Pierre-Yves David said:
>> On 06/22/2014 04:52 PM, 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)
>>
>> No thanks.
>
> Darn. I guess I misunderstood our previous discussion about this.
>
>> This use the be the default and ended up being very confusing. The
>> vast majority of fold operation involve the working directory and an
>> ancestors (something a descendant). The default should reflect that.
>> Also, we should not requires user to have a PhD in revset to use Mercurial.
>
> "REV::" is not a PhD-level revset.

Ok, it's not a super complicated revset, but it is a revset (or at least 
a "magic" invocation.

The principle I'm defending here is "simple action should be simple to 
do". Fold is a command that also target new user with basic knownledge 
of mercurial. I strongly believe those users should not be exposed to 
".^::." if we can avoid it.

As a side benefit, delaying the exposure to "x::y" lower the odds user 
will confuse it with "x:y" and saws his own leg by mistake.

>> So, I'm deeply convince the default should be "fold between . and REV".
>
> OK, fine. I happen to disagree, but whatever.

The logic here is that people usually works from a working copy perspective.

commit: record they working copy,
merge:  add other change in they working copy,
update: change content of working copy to anyther changeset,
rebase: (by default) move changesets under working copy to another head,
graft:  get a commit and put it on the working copy parent,
tag:    tags the working copy parent
revert: put content in the working copy parent

(we can also add uncommit to the list)

You have to be a fairly advance user of Mercurial to start thinking 
about the whole graph and performs more abstract action to change its 
shape. I believe such users will have no trouble using a special flag in 
such case.

The first version of fold had the behavior you describe, but seeing tens 
of Logilab people using it convinced me that the current default was a 
better choice for new users. The few usage of `hg fold` I'm seeing at 
facebook goes in the same direction.

>> How ever I agree than having different behavior for `hg fold REV`
>> and `hg fold --rev REV` sucks.
>
> OK, I'll see if I can fix that.
>
>> I also agree that the "do an in
>> memory fold of unrelated changeset" usecase is nice. We should think
>> about a dedicated flag for this case.
>
> --only? Example usage:
>
>    hg fold --only R1 R2 ... Rn
>
> combines R1, R2, and Rn to make a new changeset.

"--only" would work. JordiGH is proposing "--exact"

I'm fan of neither but I think I like --exact more.
Pierre-Yves David - June 26, 2014, 9:22 p.m.
On 06/24/2014 07:57 PM, Augie Fackler wrote:
> On Mon, Jun 23, 2014 at 09:02:14PM -0400, Greg Ward wrote:
>> On 23 June 2014, Pierre-Yves David said:
>>> On 06/22/2014 04:52 PM, 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)
>>>
>>> No thanks.
>>
>> Darn. I guess I misunderstood our previous discussion about this.
>>
>>> This use the be the default and ended up being very confusing. The
>>> vast majority of fold operation involve the working directory and an
>>> ancestors (something a descendant). The default should reflect that.
>>> Also, we should not requires user to have a PhD in revset to use Mercurial.
>>
>> "REV::" is not a PhD-level revset.
>>
>>> So, I'm deeply convince the default should be "fold between . and REV".
>>
>> OK, fine. I happen to disagree, but whatever.
>
> I agree with Greg here - the magic default of 'hg fold $REV' -> fold
> together $REV::. feels a little spooky. This feels like the right case
> for an alias to me.
>
> (It's been stated by many that histedits behavior in this area is
> confusing, and with the benefit of hindsight and the existence of
> evolution I'm starting to come around on that point.)

Do you mean the fact that histedit will work on "REV::."?

I see this as a bit different.

First, now that we use "REV::." instead of "heads(REV)::." the behavior 
is much less disturbing.

Second, I feel like the current behavior is not too bad. In most case 
people want to rewrite the history they are based on, so having that by 
default sounds sensible. What histedit currently lack is a --exact flag 
  (kind of like rebase have a --rev).

Third, histedit is an advanced command for advance users. Such people 
have already a deeper understanding of DVCS graph, more confidence in 
doing graph change unrelated to working directory parent and have less 
trouble with revset.
The `hg fold` command is target at basic users too.


(I had a fourth but I got interrupted and lost it)
Pierre-Yves David - June 26, 2014, 9:50 p.m.
On 06/26/2014 01:47 PM, Greg Ward wrote:
> On 24 June 2014, Augie Fackler said:
>> I agree with Greg here - the magic default of 'hg fold $REV' -> fold
>> together $REV::. feels a little spooky. This feels like the right case
>> for an alias to me.
>
> OR if it's as important a use case as Pierre-Yves claims, add an
> option. E.g. make
>
>    hg fold [-r] REV...
>
> (as implemented in my patch) the default. Then add
>
>    hg fold --from REV
>
> to restore the current (REV::.) behaviour.
>
> Hmmm. Time for a poll? Here are the options:

In my opinion, this thread as already growth far too much. It is time to 
slow down and take the smallest sensible step. Lets fix the very 
confusing behavior between `hg fold REV` and `hg fold --rev REV` and 
keep the two options.

 From there we can see how often people get to add this --exact flag and 
how angry they when they have to do it. Because for now I just have the 
Logilab data set where about every body gave me a strange look when 
introduced to `hg fold .^::.`

We'll definitely revisit the UI before freezing it. hopefully with more 
data at that point.

Patch

diff --git a/hgext/evolve.py b/hgext/evolve.py
--- a/hgext/evolve.py
+++ b/hgext/evolve.py
@@ -2072,32 +2072,22 @@ 
         lockmod.release(lock, wlock)
 
 @command('^fold|squash',
-    [('r', 'rev', [], _("explicitly specify the full set of revision to fold")),
+    [('r', 'rev', [], _('revisions to fold'), _('REV')),
     ] + commitopts + commitopts2,
     # allow to choose the seed ?
-    _('rev'))
+    _('[-r] REV...'))
 def fold(ui, repo, *revs, **opts):
-    """Fold multiple revisions into a single one
+    """combine multiple revisions into a single successor
 
-    The revisions from your current working directory to the given one are folded
-    into a single successor revision.
-
-    you can alternatively use --rev to explicitly specify revisions to be folded,
-    ignoring the current working directory parent.
+    The specified revisions are combined to create a single successor
+    revision that includes the changes from all of them.
     """
     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')
-        return 1
+        raise util.Abort('no revisions specified')
+    revs = scmutil.revrange(repo, revs)
+
     roots = repo.revs('roots(%ld)', revs)
     if len(roots) > 1:
         raise util.Abort("set has multiple roots")
diff --git a/tests/test-evolve.t b/tests/test-evolve.t
--- a/tests/test-evolve.t
+++ b/tests/test-evolve.t
@@ -614,12 +614,9 @@ 
 
   $ 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 6 # want to run hg fold 6
+  $ hg fold 6::
   2 changesets folded
   1 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ glog
@@ -735,7 +732,7 @@ 
 Test fold with commit messages
 
   $ cd ../work
-  $ hg fold .^ --message "Folding with custom commit message"
+  $ hg fold -r '(.^)::' --message "Folding with custom commit message"
   2 changesets folded
   1 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ glog
@@ -754,7 +751,7 @@ 
   >                   commit message
   > EOF
 
-  $ hg fold .^ --logfile commit-message
+  $ hg fold .^ -r . --logfile commit-message
   2 changesets folded
   1 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ hg qlog
diff --git a/tests/test-tutorial.t b/tests/test-tutorial.t
--- a/tests/test-tutorial.t
+++ b/tests/test-tutorial.t
@@ -473,25 +473,22 @@ 
 The tutorial part is not written yet but can use `hg fold`:
 
   $ hg help fold
-  hg fold rev
+  hg fold [-r] REV...
   
   aliases: squash
   
-  Fold multiple revisions into a single one
+  combine multiple revisions into a single successor
   
-      The revisions from your current working directory to the given one are
-      folded into a single successor revision.
-  
-      you can alternatively use --rev to explicitly specify revisions to be
-      folded, ignoring the current working directory parent.
+      The specified revisions are combined to create a single successor revision
+      that includes the changes from all of them.
   
   options:
   
-   -r --rev VALUE [+] explicitly specify the full set of revision to fold
-   -m --message TEXT  use text as commit message
-   -l --logfile FILE  read commit message from file
-   -d --date DATE     record the specified date as commit date
-   -u --user USER     record the specified user as committer
+   -r --rev REV [+]  revisions to fold
+   -m --message TEXT use text as commit message
+   -l --logfile FILE read commit message from file
+   -d --date DATE    record the specified date as commit date
+   -u --user USER    record the specified user as committer
   
   [+] marked option can be specified multiple times