Patchwork rebase: 'pull --rebase' rebase on new branching only

# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
# Date 1462366525 -7200
#      Wed May 04 14:55:25 2016 +0200
# Node ID 7e72800c070d941f294c4e7ed91db29a08bac073
# Parent  906a1c8a75fd8a18e43e8545eedcbe5222f84647
# EXP-Topic pull.rebase
rebase: 'pull --rebase' rebase on new branching only

Previously, if two heads existed on the current branch before the pull and the
pull add changeset on the one unrelated to the working copy 'hg pull --rebase'
would trigger a rebase on that branch. This might not what the user wants
as they could have resolved the head situation before pulling if they wanted to.

Therefore, we restrict the set of destination candidate to 'hg pull --rebase' to
the set of pulled changesets that create a branching from the current working
copy branch.

On Thu, May 5, 2016 at 9:04 AM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
> # Date 1462366525 -7200
> #      Wed May 04 14:55:25 2016 +0200
> # Node ID 7e72800c070d941f294c4e7ed91db29a08bac073
> # Parent  906a1c8a75fd8a18e43e8545eedcbe5222f84647
> # EXP-Topic pull.rebase
> rebase: 'pull --rebase' rebase on new branching only

Meant for stable?

>
> Previously, if two heads existed on the current branch before the pull and the
> pull add changeset on the one unrelated to the working copy 'hg pull --rebase'
> would trigger a rebase on that branch. This might not what the user wants
> as they could have resolved the head situation before pulling if they wanted to.
>
> Therefore, we restrict the set of destination candidate to 'hg pull --rebase' to
> the set of pulled changesets that create a branching from the current working
> copy branch.
>
> diff -r 906a1c8a75fd -r 7e72800c070d hgext/rebase.py
> --- a/hgext/rebase.py   Wed May 04 06:44:44 2016 +0900
> +++ b/hgext/rebase.py   Wed May 04 14:55:25 2016 +0200
> @@ -1245,15 +1245,18 @@ def pullrebase(orig, ui, repo, *args, **
>                  if 'source' in opts:
>                      del opts['source']
>                  # revsprepull is the len of the repo, not revnum of tip.
> -                destspace = list(repo.changelog.revs(start=revsprepull))
> +                destspace = repo.revs("(%ld and children(::.))::",
> +                                      repo.changelog.revs(start=revsprepull))

Is this equivalent to repo.revs("(%d: and children(::.))::", revsprepull)?

>                  opts['_destspace'] = destspace
>                  try:
>                      rebase(ui, repo, **opts)
>                  except error.NoMergeDestAbort:
>                      # we can maybe update instead
>                      rev, _a, _b = destutil.destupdate(repo)
> -                    if rev == repo['.'].rev():
> -                        ui.status(_('nothing to rebase\n'))
> +                    wcp = repo['.']
> +                    if rev == wcp.rev():
> +                        ui.status(_('nothing to rebase - no branching from %s '
> +                                    'pulled\n') % wcp)
>                      else:
>                          ui.status(_('nothing to rebase - updating instead\n'))
>                          # not passing argument to get the bare update behavior
> @@ -1353,7 +1356,8 @@ def uisetup(ui):
>      #Replace pull with a decorator to provide --rebase option
>      entry = extensions.wrapcommand(commands.table, 'pull', pullrebase)
>      entry[1].append(('', 'rebase', None,
> -                     _("rebase working directory to branch head")))
> +                     _("rebase when pulling new branching from working "
> +                       "directory parent")))
>      entry[1].append(('t', 'tool', '',
>                       _("specify merge tool for rebase")))
>      cmdutil.summaryhooks.add('rebase', summaryhook)
> diff -r 906a1c8a75fd -r 7e72800c070d tests/test-rebase-pull.t
> --- a/tests/test-rebase-pull.t  Wed May 04 06:44:44 2016 +0900
> +++ b/tests/test-rebase-pull.t  Wed May 04 14:55:25 2016 +0200
> @@ -145,14 +145,20 @@ pull --rebase works when a specific revi
>    $ hg ci -Am L1
>    adding L1
>    created new head
> +
> +(strip extra "remote" changeset to ensure we pull new branching)
> +
> +  $ hg strip --rev 'desc(R1)' --config extensions.strip=
> +  saved backup bundle to $TESTTMP/c/.hg/strip-backup/77ae9631bcca-d0f9f803-backup.hg (glob)
> +
>    $ hg pull --rev tip --rebase
>    pulling from $TESTTMP/a (glob)
>    searching for changes
>    adding changesets
>    adding manifests
>    adding file changes
> -  added 2 changesets with 2 changes to 2 files
> -  rebasing 3:ff8d69a621f9 "L1"
> +  added 3 changesets with 3 changes to 3 files (+1 heads)
> +  rebasing 2:ff8d69a621f9 "L1"
>    saved backup bundle to $TESTTMP/c/.hg/strip-backup/ff8d69a621f9-160fa373-backup.hg (glob)
>    $ hg tglog
>    @  5: 'L1'
> @@ -335,7 +341,7 @@ pre-existing heads.
>    adding manifests
>    adding file changes
>    added 1 changesets with 1 changes to 1 files
> -  nothing to rebase
> +  nothing to rebase - no branching from 6dc0ea5dcf55 pulled
>
>  There is two local heads and we pull a third one.
>  The second local head should not confuse the `hg pull rebase`.
> @@ -354,6 +360,14 @@ The second local head should not confuse
>    $ cd ../c
>    $ hg up 'desc(L2)'
>    2 files updated, 0 files merged, 2 files removed, 0 files unresolved
> +
> +(rebase on latest "remote" change to remove pre-existing branching)
> +
> +  $ hg rebase -d 'desc(R6)'
> +  rebasing 7:864e0a2d2614 "L1"
> +  rebasing 8:6dc0ea5dcf55 "L2"
> +  saved backup bundle to $TESTTMP/c/.hg/strip-backup/864e0a2d2614-2f72c89c-backup.hg (glob)
> +
>    $ hg pull --rebase
>    pulling from $TESTTMP/a (glob)
>    searching for changes
> @@ -361,9 +375,9 @@ The second local head should not confuse
>    adding manifests
>    adding file changes
>    added 1 changesets with 1 changes to 1 files (+1 heads)
> -  rebasing 7:864e0a2d2614 "L1"
> -  rebasing 8:6dc0ea5dcf55 "L2"
> -  saved backup bundle to $TESTTMP/c/.hg/strip-backup/864e0a2d2614-2f72c89c-backup.hg (glob)
> +  rebasing 10:40410cfbcfce "L1"
> +  rebasing 11:76f58da1b646 "L2"
> +  saved backup bundle to $TESTTMP/c/.hg/strip-backup/40410cfbcfce-1aff5e0f-backup.hg (glob)
>    $ hg tglog
>    @  12: 'L2'
>    |
> @@ -391,3 +405,34 @@ The second local head should not confuse
>    |
>    o  0: 'C1'
>
> +
> +Pulling change on other pre-existing heads should not trigger a rebase
> +----------------------------------------------------------------------
> +
> +(push the other head in "a" so that we can add changeset on it there)
> +
> +  $ hg push -fr 'desc(M1)' ../a
> +  pushing to ../a
> +  searching for changes
> +  adding changesets
> +  adding manifests
> +  adding file changes
> +  added 1 changesets with 1 changes to 1 files (+1 heads)
> +  $ cd ../a
> +  $ hg up 'desc(M1)'
> +  1 files updated, 0 files merged, 1 files removed, 0 files unresolved
> +  $ echo M2 > M2
> +  $ hg commit -Am M2
> +  adding M2
> +  $ cd ../c
> +
> +Pulling does not create branching, so we do not rebase.
> +
> +  $ hg pull --rebase
> +  pulling from $TESTTMP/a (glob)
> +  searching for changes
> +  adding changesets
> +  adding manifests
> +  adding file changes
> +  added 1 changesets with 1 changes to 1 files
> +  nothing to rebase - no branching from 590d79f77016 pulled
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

On 05/05/2016 10:45 PM, Martin von Zweigbergk wrote:
> On Thu, May 5, 2016 at 9:04 AM, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org> wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
>> # Date 1462366525 -7200
>> #      Wed May 04 14:55:25 2016 +0200
>> # Node ID 7e72800c070d941f294c4e7ed91db29a08bac073
>> # Parent  906a1c8a75fd8a18e43e8545eedcbe5222f84647
>> # EXP-Topic pull.rebase
>> rebase: 'pull --rebase' rebase on new branching only
> Meant for stable?

No, this is a new behavior making 'hg pull --rebase' a bit closer to its 
initial intend (and therefore hopefully more reliable to build work-flow 
on) but the 3.7 behavior was even more baroque so no regression testing 
here.

>> Previously, if two heads existed on the current branch before the pull and the
>> pull add changeset on the one unrelated to the working copy 'hg pull --rebase'
>> would trigger a rebase on that branch. This might not what the user wants
>> as they could have resolved the head situation before pulling if they wanted to.
>>
>> Therefore, we restrict the set of destination candidate to 'hg pull --rebase' to
>> the set of pulled changesets that create a branching from the current working
>> copy branch.
>>
>> diff -r 906a1c8a75fd -r 7e72800c070d hgext/rebase.py
>> --- a/hgext/rebase.py   Wed May 04 06:44:44 2016 +0900
>> +++ b/hgext/rebase.py   Wed May 04 14:55:25 2016 +0200
>> @@ -1245,15 +1245,18 @@ def pullrebase(orig, ui, repo, *args, **
>>                   if 'source' in opts:
>>                       del opts['source']
>>                   # revsprepull is the len of the repo, not revnum of tip.
>> -                destspace = list(repo.changelog.revs(start=revsprepull))
>> +                destspace = repo.revs("(%ld and children(::.))::",
>> +                                      repo.changelog.revs(start=revsprepull))
> Is this equivalent to repo.revs("(%d: and children(::.))::", revsprepull)?

Almost, but if revsprepull is filtered (for some reason), the '%d:' 
version will fail while the version using "%ld" and changelog.revs will 
be robust.

Cheers,

On 5/5/2016 11:57 PM, Pierre-Yves David wrote:
>
>
> On 05/05/2016 10:45 PM, Martin von Zweigbergk wrote:
>> On Thu, May 5, 2016 at 9:04 AM, Pierre-Yves David
>> <pierre-yves.david@ens-lyon.org> wrote:
>>> # HG changeset patch
>>> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
>>> # Date 1462366525 -7200
>>> #      Wed May 04 14:55:25 2016 +0200
>>> # Node ID 7e72800c070d941f294c4e7ed91db29a08bac073
>>> # Parent  906a1c8a75fd8a18e43e8545eedcbe5222f84647
>>> # EXP-Topic pull.rebase
>>> rebase: 'pull --rebase' rebase on new branching only

"new branching" sounds awkward to me as a native english speaker. After 
reviewing what this patch does, I think it would be more clear to say 
"rebase on newly introduced branch only".

>> Meant for stable?
>
> No, this is a new behavior making 'hg pull --rebase' a bit closer to 
> its initial intend (and therefore hopefully more reliable to build 
> work-flow on) but the 3.7 behavior was even more baroque so no 
> regression testing here.

Are you saying that `hg pull --rebase` was intended to have 
non-idempotent semantics? Or am I missing something? Can you spell out 
what you think the original intention was, and why getting closer to it 
is a good thing?

>
>>> Previously, if two heads existed on the current branch before the 
>>> pull and the
>>> pull add changeset on the one unrelated to the working copy 'hg pull 
>>> --rebase'
>>> would trigger a rebase on that branch. This might not what the user 
>>> wants
>>> as they could have resolved the head situation before pulling if 
>>> they wanted to.

The new behavior might also not be what the user wants, so I don't think 
this is a good enough reason to make the change.

The changes I would like to see in pull --rebase are to make it so that 
`hg pull && hg pull --rebase` (eg, "oh crap I forgot the flag") has the 
same result as `hg pull --rebase` from the beginning. This patch doesn't 
move us in that direction (indeed, I would argue that it moves us 
further away), so I'm skeptical of the direction.

That being said, after having written all this, I see that you have had 
a nice long discussion about it already over at 
https://bz.mercurial-scm.org/show_bug.cgi?id=5214, so I'll bow out now, 
haha.


>>>
>>> Therefore, we restrict the set of destination candidate to 'hg pull 
>>> --rebase' to
>>> the set of pulled changesets that create a branching from the 
>>> current working
>>> copy branch.
>>>
>>> diff -r 906a1c8a75fd -r 7e72800c070d hgext/rebase.py
>>> --- a/hgext/rebase.py   Wed May 04 06:44:44 2016 +0900
>>> +++ b/hgext/rebase.py   Wed May 04 14:55:25 2016 +0200
>>> @@ -1245,15 +1245,18 @@ def pullrebase(orig, ui, repo, *args, **
>>>                   if 'source' in opts:
>>>                       del opts['source']
>>>                   # revsprepull is the len of the repo, not revnum 
>>> of tip.
>>> -                destspace = 
>>> list(repo.changelog.revs(start=revsprepull))
>>> +                destspace = repo.revs("(%ld and children(::.))::",
>>> + repo.changelog.revs(start=revsprepull))
>> Is this equivalent to repo.revs("(%d: and children(::.))::", 
>> revsprepull)?
>
> Almost, but if revsprepull is filtered (for some reason), the '%d:' 
> version will fail while the version using "%ld" and changelog.revs 
> will be robust.
>
> Cheers,
>

On 05/07/2016 11:59 PM, Ryan McElroy wrote:
> On 5/5/2016 11:57 PM, Pierre-Yves David wrote:
>>
>>
>> On 05/05/2016 10:45 PM, Martin von Zweigbergk wrote:
>>> On Thu, May 5, 2016 at 9:04 AM, Pierre-Yves David
>>> <pierre-yves.david@ens-lyon.org> wrote:
>>>> # HG changeset patch
>>>> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
>>>> # Date 1462366525 -7200
>>>> #      Wed May 04 14:55:25 2016 +0200
>>>> # Node ID 7e72800c070d941f294c4e7ed91db29a08bac073
>>>> # Parent  906a1c8a75fd8a18e43e8545eedcbe5222f84647
>>>> # EXP-Topic pull.rebase
>>>> rebase: 'pull --rebase' rebase on new branching only
>
> "new branching" sounds awkward to me as a native english speaker. 
> After reviewing what this patch does, I think it would be more clear 
> to say "rebase on newly introduced branch only".

Noted, thanks

>
>>> Meant for stable?
>>
>> No, this is a new behavior making 'hg pull --rebase' a bit closer to 
>> its initial intend (and therefore hopefully more reliable to build 
>> work-flow on) but the 3.7 behavior was even more baroque so no 
>> regression testing here.
>
> Are you saying that `hg pull --rebase` was intended to have 
> non-idempotent semantics? Or am I missing something? Can you spell out 
> what you think the original intention was, and why getting closer to 
> it is a good thing?

So my personal interpretation is that the point of `hg pull --rebase` is 
to "rebase on what you pull" or another phrasing "re-linearize newly 
introduced branch".

Note that the fix for issue5214 (no bookmark involved version) already 
broke the idem-potency, this change is pushing the behavior change further.

Getting closer to it is useful because -in-the-current-state- of 
Mercurial, it is common to have multiple anonymous heads on branch, 
leading to ambiguity on destination and various "surprising" behavior.

Archeology does not raise much details on this.

Let's move to the next block for wider discussion

>
>>
>>>> Previously, if two heads existed on the current branch before the 
>>>> pull and the
>>>> pull add changeset on the one unrelated to the working copy 'hg 
>>>> pull --rebase'
>>>> would trigger a rebase on that branch. This might not what the user 
>>>> wants
>>>> as they could have resolved the head situation before pulling if 
>>>> they wanted to.
>
> The new behavior might also not be what the user wants, so I don't 
> think this is a good enough reason to make the change.

My goal here is to make the -current- state more predictable/useful for 
users. A medium terms solution to this is to vastly improve the 
experience around destination selection. My main lead to make progress 
here is the topic experiment and tracking related feature around this. 
(note that you are the one who convinced me how destination/tracking was 
such an important part of the user experience we need to fix).

That said, I'm proposing this change because I think it is a small 
improvement to the current situation, I'm open to delaying it and having 
the destination improvement first (this year?). Such improvement would 
makes the risk of surprising behavior much lower and this change less 
important.

> The changes I would like to see in pull --rebase are to make it so 
> that `hg pull && hg pull --rebase` (eg, "oh crap I forgot the flag") 
> has the same result as `hg pull --rebase` from the beginning. This 
> patch doesn't move us in that direction (indeed, I would argue that it 
> moves us further away), so I'm skeptical of the direction.

While I'm open to change to my approach/interpretation, I'm not super 
convinced by this argument:

A: if we keep `hg pull --rebase` as an (almost) strict equivalent for 
`hg pull && hg rebase`, there is not much extra effort to run `hg 
rebase` if one forgot to add `--rebase`. So the value of running a full 
pull again is unclear to me.

     (I says almost, because, hg pull --rebase can also perform an update)

B: We make the change proposed in this patch,
B.1: if we get (or by chance, the user already are) to a clear 
destination/tracking situation, `hg rebase` after a pull should behave 
properly,
B.2: if we had another pre-existing head the user do not want to rebase 
to, `hg pull --rebase` now behave better (do nothing) and `hg rebase` 
still "misbehave (rebase on the other, pre-existing, head),
B.3: if we had another pre-existing head and we pull a third one 
(branching from the working copy parent), `hg pull rebase` will pick the 
newly pulled one as destination (solving the ambguity using the fact one 
is newly pulled) and `hg rebase` will properly complain about the ambiguity.

Overall we improve behavior in case B2 and B3, without regressions from 
the previous situation.

Let's make a table to summarize

Situation 1: (wc is on A)
1) 1 head (A) before pull, pull bring another head (N)
2) 2 head (A+B) before pull, pull bring new changeset on another branch; 
A is tip of branch
3) 2 head (A+B) before pull, pull bring new changeset on another branch; 
B is tip of branch
4) 2 head (A+B) before pull, pull bring changeset on B, but no new heads 
(or not one branching from ".")
5) 2 head before pull (A+B), pull bring a new head (N) branching from A
6) 2 head before pull (A+B), pull bring a new head (N) branching from A 
and change on B
(we could have a longer list but I'm trying to keep things "simple"

Behavior:
A: 3.7 behavior "full rebase, tip of branch is destination)
B: 3.8 behavior "pick destination in pulled changeset only"
C: This patch behavior "pick destionation on branch from "::." newly pulled"

For `hg pull --rebase`:
     1    | 2       | 3       | 4       | 5    | 6
A | on N | nothing | on B    | on B    | on N | max(B,N)
B | on N | on B    | on B    | on B    | on N | abort
C | on N | nothing | nothing | nothing | on N | N

for `hg pull && hg rebase`
     1    | 2       | 3       | 4       | 5     | 6
A | on N | nothing | on B    | on B    | on N  | max(B,N)
B | on N | on B    | on B    | on B    | abort | abort
C | on N | on B    | on B    | on B    | abort | abort
(note, 2+3+4 would pick B even without the pull)


> That being said, after having written all this, I see that you have 
> had a nice long discussion about it already over at 
> https://bz.mercurial-scm.org/show_bug.cgi?id=5214, so I'll bow out 
> now, haha.

It is more a nice long monologue, but I'm not sure this issue is the 
right place for a discussion, there is already too many topic 
interleaved there.