Patchwork [2,of,2] rebase: only pick branches without obsolete changeset with -b (issue5300)

login
register
mail settings
Submitter Denis Laxalde
Date Aug. 30, 2017, 1:54 p.m.
Message ID <092bccbfdc5d29fe5fe2.1504101255@marimba>
Download mbox | patch
Permalink /patch/23506/
State Changes Requested
Headers show

Comments

Denis Laxalde - Aug. 30, 2017, 1:54 p.m.
# HG changeset patch
# User Denis Laxalde <denis@laxalde.org>
# Date 1504025723 -7200
#      Tue Aug 29 18:55:23 2017 +0200
# Node ID 092bccbfdc5d29fe5fe2f79c18d49bd842437707
# Parent  28e449777fb7452e569292184a18f358186248a3
rebase: only pick branches without obsolete changeset with -b (issue5300)

When trying to rebase the following graph with -b 24, the command will abort
because "this rebase will cause divergence from: 21".

  @  24
  |
  o  23
  |
  | o  22 (orphan)
  | |
  | x  21 (rewritten as 23)
  |/
  o  20

By computing the set of revisions to rebase excluding branches with changesets
that would get divergent, we let the rebase proceed (and rebase 20::24 in the
example above) while leaving already unstable branches (21::) behind. The
reason for leaving unstable things behind is that when the user explicitly
specifies the --base, we know which part of the graph they're interested in.
Augie Fackler - Aug. 30, 2017, 6:20 p.m.
(+junw, martinvonz)

On Wed, Aug 30, 2017 at 03:54:15PM +0200, Denis Laxalde wrote:
> # HG changeset patch
> # User Denis Laxalde <denis@laxalde.org>
> # Date 1504025723 -7200
> #      Tue Aug 29 18:55:23 2017 +0200
> # Node ID 092bccbfdc5d29fe5fe2f79c18d49bd842437707
> # Parent  28e449777fb7452e569292184a18f358186248a3
> rebase: only pick branches without obsolete changeset with -b (issue5300)

These look fine to me, but I know junw has done some refactoring here,
and I'd like him and/or Martin to take a look.

Thanks!

>
> When trying to rebase the following graph with -b 24, the command will abort
> because "this rebase will cause divergence from: 21".
>
>   @  24
>   |
>   o  23
>   |
>   | o  22 (orphan)
>   | |
>   | x  21 (rewritten as 23)
>   |/
>   o  20
>
> By computing the set of revisions to rebase excluding branches with changesets
> that would get divergent, we let the rebase proceed (and rebase 20::24 in the
> example above) while leaving already unstable branches (21::) behind. The
> reason for leaving unstable things behind is that when the user explicitly
> specifies the --base, we know which part of the graph they're interested in.
>
> diff --git a/hgext/rebase.py b/hgext/rebase.py
> --- a/hgext/rebase.py
> +++ b/hgext/rebase.py
> @@ -755,6 +755,8 @@ def _definesets(ui, repo, destf=None, sr
>              roots += list(repo.revs('children(%d) & ancestors(%ld)', bp, bs))
>
>          rebaseset = repo.revs('%ld::', roots)
> +        if obsolete.isenabled(repo, obsolete.allowunstableopt):
> +            rebaseset -= repo.revs('only(obsolete(), (%ld))::', base)
>
>          if not rebaseset:
>              # transform to list because smartsets are not comparable to
> diff --git a/tests/test-rebase-obsolete.t b/tests/test-rebase-obsolete.t
> --- a/tests/test-rebase-obsolete.t
> +++ b/tests/test-rebase-obsolete.t
> @@ -858,6 +858,89 @@ With experimental.allowdivergence=True,
>    phases: 8 draft
>    content-divergent: 2 changesets
>
> +rebase -b leaves branches with unstable changesets
> +  $ hg log -G -T "{rev}:{node|short} {desc|firstline} ({instabilities})\n"
> +  @  17:61bd55f69bc4 bar foo ()
> +  |
> +  o  16:5f53594f6882 P (content-divergent)
> +  |
> +  | o  14:77d874d096a2 10' (content-divergent)
> +  | |
> +  o |  12:3eb461388009 john doe ()
> +  |/
> +  o  9:4be60e099a77 C ()
> +  |
> +  o  6:9c48361117de D ()
> +  |
> +  o  2:261e70097290 B2 ()
> +  |
> +  o  0:4a2df7238c3b A ()
> +
> +  $ hg up --quiet 6
> +  $ echo X > X
> +  $ hg add X
> +  $ hg ci -mX
> +  created new head
> +  $ hg id -in
> +  77b9c700c48f 18
> +  $ echo Y > Y
> +  $ hg add Y
> +  $ hg ci -mY
> +  $ hg id -in
> +  3011703f72e2 19
> +  $ hg up --quiet 18
> +  $ echo Z > Z
> +  $ hg add Z
> +  $ hg ci -m Z
> +  created new head
> +  $ echo ZZ >> Z
> +  $ hg ci -m ZZ
> +  $ hg id -i
> +  cdec77b938cb
> +  $ echo Z? >> Z
> +  $ hg ci -m WIP
> +  $ hg up --quiet '.^'
> +  $ hg ci --amend -m Zz
> +  $ echo ZZZ > Z
> +  $ hg ci -mZZZ
> +  $ hg log -G -T "{rev}:{node|short} {desc|firstline} ({instabilities})\n"
> +  @  24:cec38798f9cb ZZZ ()
> +  |
> +  o  23:d8c4115abbc7 Zz ()
> +  |
> +  | o  22:2238c5536cc2 WIP (orphan)
> +  | |
> +  | x  21:cdec77b938cb ZZ ()
> +  |/
> +  o  20:cb4bc2b895c2 Z ()
> +  |
> +  | o  19:3011703f72e2 Y ()
> +  |/
> +  o  18:77b9c700c48f X ()
> +  |
> +  | o  17:61bd55f69bc4 bar foo ()
> +  | |
> +  | o  16:5f53594f6882 P (content-divergent)
> +  | |
> +  | | o  14:77d874d096a2 10' (content-divergent)
> +  | | |
> +  | o |  12:3eb461388009 john doe ()
> +  | |/
> +  | o  9:4be60e099a77 C ()
> +  |/
> +  o  6:9c48361117de D ()
> +  |
> +  o  2:261e70097290 B2 ()
> +  |
> +  o  0:4a2df7238c3b A ()
> +
> +  $ hg rebase -b . -d 19
> +  rebasing 20:cb4bc2b895c2 "Z"
> +  rebasing 23:d8c4115abbc7 "Zz"
> +  rebasing 24:cec38798f9cb "ZZZ" (tip)
> +  $ hg strip --config extensions.strip= --no-backup 18:: --quiet
> +  $ hg up 17 --quiet
> +
>  rebase --continue + skipped rev because their successors are in destination
>  we make a change in trunk and work on conflicting changes to make rebase abort.
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Jun Wu - Aug. 31, 2017, 7:09 p.m.
Excerpts from Denis Laxalde's message of 2017-08-30 15:54:15 +0200:
> # HG changeset patch
> # User Denis Laxalde <denis@laxalde.org>
> # Date 1504025723 -7200
> #      Tue Aug 29 18:55:23 2017 +0200
> # Node ID 092bccbfdc5d29fe5fe2f79c18d49bd842437707
> # Parent  28e449777fb7452e569292184a18f358186248a3
> rebase: only pick branches without obsolete changeset with -b (issue5300)
> 
> When trying to rebase the following graph with -b 24, the command will abort
> because "this rebase will cause divergence from: 21".
> 
>   @  24
>   |
>   o  23
>   |
>   | o  22 (orphan)
>   | |
>   | x  21 (rewritten as 23)
>   |/
>   o  20

This example is incomplete. If there is "-d 22", nothing breaks since only
23 and 24 will be rebased. Adding an ancestor is important:

   @  24
   |
   o  23
   |
   | o  22 (orphan)
   | |
   | x  21 (rewritten as 23)
   |/
   o  20
   |
   | o  19
   |/
   o  18

Then "hg rebase -b 24 -d 19". The rebaseset will include both 21 and 23.

This is not "-b" specific. It also affects "-r 21+23" or "-s 20".
A previous discussion and proposed solutions can be found at:
https://www.mercurial-scm.org/wiki/CEDRebase.

It seems your approach is similar to "Option 2 (skip troublemakers)". I
think it could be formalized by:

  - Make it generic - work with -s and -r
  - Revise the revset (see below)
  - Add more tests about corner cases (see below)
  - Respect "rebaseskipobsolete" config option
  - Print a message about rebase is doing (maybe "unexpected") things

> By computing the set of revisions to rebase excluding branches with changesets
> that would get divergent, we let the rebase proceed (and rebase 20::24 in the
> example above) while leaving already unstable branches (21::) behind. The
> reason for leaving unstable things behind is that when the user explicitly
> specifies the --base, we know which part of the graph they're interested in.
> 
> diff --git a/hgext/rebase.py b/hgext/rebase.py
> --- a/hgext/rebase.py
> +++ b/hgext/rebase.py
> @@ -755,6 +755,8 @@ def _definesets(ui, repo, destf=None, sr
>              roots += list(repo.revs('children(%d) & ancestors(%ld)', bp, bs))
>  
>          rebaseset = repo.revs('%ld::', roots)
> +        if obsolete.isenabled(repo, obsolete.allowunstableopt):
> +            rebaseset -= repo.revs('only(obsolete(), (%ld))::', base)

This seems to also select innocent ancestors of 'obsolete()':

  o D
  |
  x C <- part of obsolete()
  |
  o B <- will be selected (probably shouldn't)
  |
  o A <- base

Not all "obsolete()" revisions should be skipped, only those with successors
in the same rebaseset. Obsoleted revisions without successors should not
have their children removed. Obsoleted revisions with a unique successor
outside the current rebaseset worth the divergence warning.

Could you also add tests about those corner cases?

>          if not rebaseset:
>              # transform to list because smartsets are not comparable to
> diff --git a/tests/test-rebase-obsolete.t b/tests/test-rebase-obsolete.t
> --- a/tests/test-rebase-obsolete.t
> +++ b/tests/test-rebase-obsolete.t

Tip: You can use drawdag to write tests. That makes review easier.
I had to modify your test change and stop in the middle to investigate
obsolete markers.  drawdag examples can be found in this test file.

> [...]

Patch

diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -755,6 +755,8 @@  def _definesets(ui, repo, destf=None, sr
             roots += list(repo.revs('children(%d) & ancestors(%ld)', bp, bs))
 
         rebaseset = repo.revs('%ld::', roots)
+        if obsolete.isenabled(repo, obsolete.allowunstableopt):
+            rebaseset -= repo.revs('only(obsolete(), (%ld))::', base)
 
         if not rebaseset:
             # transform to list because smartsets are not comparable to
diff --git a/tests/test-rebase-obsolete.t b/tests/test-rebase-obsolete.t
--- a/tests/test-rebase-obsolete.t
+++ b/tests/test-rebase-obsolete.t
@@ -858,6 +858,89 @@  With experimental.allowdivergence=True, 
   phases: 8 draft
   content-divergent: 2 changesets
 
+rebase -b leaves branches with unstable changesets
+  $ hg log -G -T "{rev}:{node|short} {desc|firstline} ({instabilities})\n"
+  @  17:61bd55f69bc4 bar foo ()
+  |
+  o  16:5f53594f6882 P (content-divergent)
+  |
+  | o  14:77d874d096a2 10' (content-divergent)
+  | |
+  o |  12:3eb461388009 john doe ()
+  |/
+  o  9:4be60e099a77 C ()
+  |
+  o  6:9c48361117de D ()
+  |
+  o  2:261e70097290 B2 ()
+  |
+  o  0:4a2df7238c3b A ()
+  
+  $ hg up --quiet 6
+  $ echo X > X
+  $ hg add X
+  $ hg ci -mX
+  created new head
+  $ hg id -in
+  77b9c700c48f 18
+  $ echo Y > Y
+  $ hg add Y
+  $ hg ci -mY
+  $ hg id -in
+  3011703f72e2 19
+  $ hg up --quiet 18
+  $ echo Z > Z
+  $ hg add Z
+  $ hg ci -m Z
+  created new head
+  $ echo ZZ >> Z
+  $ hg ci -m ZZ
+  $ hg id -i
+  cdec77b938cb
+  $ echo Z? >> Z
+  $ hg ci -m WIP
+  $ hg up --quiet '.^'
+  $ hg ci --amend -m Zz
+  $ echo ZZZ > Z
+  $ hg ci -mZZZ
+  $ hg log -G -T "{rev}:{node|short} {desc|firstline} ({instabilities})\n"
+  @  24:cec38798f9cb ZZZ ()
+  |
+  o  23:d8c4115abbc7 Zz ()
+  |
+  | o  22:2238c5536cc2 WIP (orphan)
+  | |
+  | x  21:cdec77b938cb ZZ ()
+  |/
+  o  20:cb4bc2b895c2 Z ()
+  |
+  | o  19:3011703f72e2 Y ()
+  |/
+  o  18:77b9c700c48f X ()
+  |
+  | o  17:61bd55f69bc4 bar foo ()
+  | |
+  | o  16:5f53594f6882 P (content-divergent)
+  | |
+  | | o  14:77d874d096a2 10' (content-divergent)
+  | | |
+  | o |  12:3eb461388009 john doe ()
+  | |/
+  | o  9:4be60e099a77 C ()
+  |/
+  o  6:9c48361117de D ()
+  |
+  o  2:261e70097290 B2 ()
+  |
+  o  0:4a2df7238c3b A ()
+  
+  $ hg rebase -b . -d 19
+  rebasing 20:cb4bc2b895c2 "Z"
+  rebasing 23:d8c4115abbc7 "Zz"
+  rebasing 24:cec38798f9cb "ZZZ" (tip)
+  $ hg strip --config extensions.strip= --no-backup 18:: --quiet
+  $ hg up 17 --quiet
+
 rebase --continue + skipped rev because their successors are in destination
 we make a change in trunk and work on conflicting changes to make rebase abort.