Patchwork [2,of,2] rebase: check for target rev in abort's dstates list (issue4896)

login
register
mail settings
Submitter Christian Delahousse
Date Oct. 13, 2015, 10:07 p.m.
Message ID <8927b37165a7664ea4a5.1444774075@dev4253.prn1.facebook.com>
Download mbox | patch
Permalink /patch/11028/
State Accepted
Headers show

Comments

Christian Delahousse - Oct. 13, 2015, 10:07 p.m.
# HG changeset patch
# User Christian Delahousse <cdelahousse@fb.com>
# Date 1444770411 25200
#      Tue Oct 13 14:06:51 2015 -0700
# Node ID 8927b37165a7664ea4a5addefc7d9dbffdcae225
# Parent  4a52314eb7a983d2d0adbaadbe4d122132455e2a
rebase: check for target rev in abort's dstates list (issue4896)

When rebasing a set of changes onto a public changeset and having the first one
be skipped, abort fails. This fix adds a check to not allow the target rev into
the dstate list.
Augie Fackler - Oct. 14, 2015, 2:15 p.m.
On Tue, Oct 13, 2015 at 03:07:55PM -0700, Christian Delahousse wrote:
> # HG changeset patch
> # User Christian Delahousse <cdelahousse@fb.com>
> # Date 1444770411 25200
> #      Tue Oct 13 14:06:51 2015 -0700
> # Node ID 8927b37165a7664ea4a5addefc7d9dbffdcae225
> # Parent  4a52314eb7a983d2d0adbaadbe4d122132455e2a
> rebase: check for target rev in abort's dstates list (issue4896)

queued these, thanks

>
> When rebasing a set of changes onto a public changeset and having the first one
> be skipped, abort fails. This fix adds a check to not allow the target rev into
> the dstate list.
>
> diff --git a/hgext/rebase.py b/hgext/rebase.py
> --- a/hgext/rebase.py
> +++ b/hgext/rebase.py
> @@ -917,7 +917,11 @@
>
>      activebookmark: the name of the bookmark that should be active after the
>          restore'''
> -    dstates = [s for s in state.values() if s >= 0]
> +
> +    # If the first commits in the rebased set get skipped during the rebase,
> +    # their values within the state mapping will be the target rev id. The
> +    # dstates list must must not contain the target rev (issue4896)
> +    dstates = [s for s in state.values() if s >= 0 and s != target]
>      immutable = [d for d in dstates if not repo[d].mutable()]
>      cleanup = True
>      if immutable:
> diff --git a/tests/test-rebase-abort.t b/tests/test-rebase-abort.t
> --- a/tests/test-rebase-abort.t
> +++ b/tests/test-rebase-abort.t
> @@ -322,3 +322,38 @@
>    commit: (clean)
>    update: 1 new changesets, 2 branch heads (merge)
>    phases: 4 draft
> +
> +test aborting a rebase succeeds after rebasing with skipped commits onto a
> +public changeset (issue4896)
> +
> +  $ hg init failonpublic
> +  $ cd failonpublic
> +  $ echo 'content' > root
> +  $ hg commit -A -m 'root' -q
> +
> +set up public branch
> +  $ echo 'content' > disappear
> +  $ hg commit -A -m 'disappear public' -q
> +commit will cause merge conflict on rebase
> +  $ echo '' > root
> +  $ hg commit -m 'remove content public' -q
> +  $ hg phase --public
> +
> +setup the draft branch that will be rebased onto public commit
> +  $ hg up -r 0 -q
> +  $ echo 'content' > disappear
> +commit will disappear
> +  $ hg commit -A -m 'disappear draft' -q
> +  $ echo 'addedcontADDEDentadded' > root
> +commit will cause merge conflict on rebase
> +  $ hg commit -m 'add content draft' -q
> +
> +  $ hg rebase -d 'public()' --tool :merge -q
> +  note: rebase of 3:0682fd3dabf5 created no changes to commit
> +  warning: conflicts during merge.
> +  merging root incomplete! (edit conflicts, then use 'hg resolve --mark')
> +  unresolved conflicts (see hg resolve, then hg rebase --continue)
> +  [1]
> +  $ hg rebase --abort
> +  rebase aborted
> +
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - Oct. 14, 2015, 2:20 p.m.
On 10/13/2015 03:07 PM, Christian Delahousse wrote:
> # HG changeset patch
> # User Christian Delahousse <cdelahousse@fb.com>
> # Date 1444770411 25200
> #      Tue Oct 13 14:06:51 2015 -0700
> # Node ID 8927b37165a7664ea4a5addefc7d9dbffdcae225
> # Parent  4a52314eb7a983d2d0adbaadbe4d122132455e2a
> rebase: check for target rev in abort's dstates list (issue4896)
>
> When rebasing a set of changes onto a public changeset and having the first one
> be skipped, abort fails. This fix adds a check to not allow the target rev into
> the dstate list.

We use the first line of the summary to help writing the changelog. So 
we try to focus on high level effect in there.

I would use something like:

rebase: properly abort when destination is public (issue4896)

Then detailing the case more in detail and got into lower level details 
regarding dstates.
Augie Fackler - Oct. 14, 2015, 3:07 p.m.
On Tue, Oct 13, 2015 at 03:07:55PM -0700, Christian Delahousse wrote:
> # HG changeset patch
> # User Christian Delahousse <cdelahousse@fb.com>
> # Date 1444770411 25200
> #      Tue Oct 13 14:06:51 2015 -0700
> # Node ID 8927b37165a7664ea4a5addefc7d9dbffdcae225
> # Parent  4a52314eb7a983d2d0adbaadbe4d122132455e2a
> rebase: check for target rev in abort's dstates list (issue4896)

Dropping this from my stack for now. test-rebase-abort.t sends its regards.

>
> When rebasing a set of changes onto a public changeset and having the first one
> be skipped, abort fails. This fix adds a check to not allow the target rev into
> the dstate list.
>
> diff --git a/hgext/rebase.py b/hgext/rebase.py
> --- a/hgext/rebase.py
> +++ b/hgext/rebase.py
> @@ -917,7 +917,11 @@
>
>      activebookmark: the name of the bookmark that should be active after the
>          restore'''
> -    dstates = [s for s in state.values() if s >= 0]
> +
> +    # If the first commits in the rebased set get skipped during the rebase,
> +    # their values within the state mapping will be the target rev id. The
> +    # dstates list must must not contain the target rev (issue4896)
> +    dstates = [s for s in state.values() if s >= 0 and s != target]
>      immutable = [d for d in dstates if not repo[d].mutable()]
>      cleanup = True
>      if immutable:
> diff --git a/tests/test-rebase-abort.t b/tests/test-rebase-abort.t
> --- a/tests/test-rebase-abort.t
> +++ b/tests/test-rebase-abort.t
> @@ -322,3 +322,38 @@
>    commit: (clean)
>    update: 1 new changesets, 2 branch heads (merge)
>    phases: 4 draft
> +
> +test aborting a rebase succeeds after rebasing with skipped commits onto a
> +public changeset (issue4896)
> +
> +  $ hg init failonpublic
> +  $ cd failonpublic
> +  $ echo 'content' > root
> +  $ hg commit -A -m 'root' -q
> +
> +set up public branch
> +  $ echo 'content' > disappear
> +  $ hg commit -A -m 'disappear public' -q
> +commit will cause merge conflict on rebase
> +  $ echo '' > root
> +  $ hg commit -m 'remove content public' -q
> +  $ hg phase --public
> +
> +setup the draft branch that will be rebased onto public commit
> +  $ hg up -r 0 -q
> +  $ echo 'content' > disappear
> +commit will disappear
> +  $ hg commit -A -m 'disappear draft' -q
> +  $ echo 'addedcontADDEDentadded' > root
> +commit will cause merge conflict on rebase
> +  $ hg commit -m 'add content draft' -q
> +
> +  $ hg rebase -d 'public()' --tool :merge -q
> +  note: rebase of 3:0682fd3dabf5 created no changes to commit
> +  warning: conflicts during merge.
> +  merging root incomplete! (edit conflicts, then use 'hg resolve --mark')
> +  unresolved conflicts (see hg resolve, then hg rebase --continue)
> +  [1]
> +  $ hg rebase --abort
> +  rebase aborted
> +
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel

Patch

diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -917,7 +917,11 @@ 
 
     activebookmark: the name of the bookmark that should be active after the
         restore'''
-    dstates = [s for s in state.values() if s >= 0]
+
+    # If the first commits in the rebased set get skipped during the rebase,
+    # their values within the state mapping will be the target rev id. The
+    # dstates list must must not contain the target rev (issue4896)
+    dstates = [s for s in state.values() if s >= 0 and s != target]
     immutable = [d for d in dstates if not repo[d].mutable()]
     cleanup = True
     if immutable:
diff --git a/tests/test-rebase-abort.t b/tests/test-rebase-abort.t
--- a/tests/test-rebase-abort.t
+++ b/tests/test-rebase-abort.t
@@ -322,3 +322,38 @@ 
   commit: (clean)
   update: 1 new changesets, 2 branch heads (merge)
   phases: 4 draft
+
+test aborting a rebase succeeds after rebasing with skipped commits onto a
+public changeset (issue4896)
+
+  $ hg init failonpublic
+  $ cd failonpublic
+  $ echo 'content' > root
+  $ hg commit -A -m 'root' -q
+
+set up public branch
+  $ echo 'content' > disappear
+  $ hg commit -A -m 'disappear public' -q
+commit will cause merge conflict on rebase
+  $ echo '' > root
+  $ hg commit -m 'remove content public' -q
+  $ hg phase --public
+
+setup the draft branch that will be rebased onto public commit
+  $ hg up -r 0 -q
+  $ echo 'content' > disappear
+commit will disappear
+  $ hg commit -A -m 'disappear draft' -q
+  $ echo 'addedcontADDEDentadded' > root
+commit will cause merge conflict on rebase
+  $ hg commit -m 'add content draft' -q
+
+  $ hg rebase -d 'public()' --tool :merge -q
+  note: rebase of 3:0682fd3dabf5 created no changes to commit
+  warning: conflicts during merge.
+  merging root incomplete! (edit conflicts, then use 'hg resolve --mark')
+  unresolved conflicts (see hg resolve, then hg rebase --continue)
+  [1]
+  $ hg rebase --abort
+  rebase aborted
+