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

[Jun Wu]

Excerpts from Denis Laxalde's message of 2017-08-30 15:54:15 +0200:
> # HG changeset patch
> # User Denis Laxalde <denis at 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.

> [...]