D7730: rebase: make sure pruning does not confuse rebase (issue6180)

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

D7730: rebase: make sure pruning does not confuse rebase (issue6180)

marmoute (Pierre-Yves David)
khanchi97 created this revision.
Herald added a reviewer: martinvonz.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Before this patch, if a user is rebasing a stack of commits and
  hit a conflict in between and decided to drop that commit (the commit
  which is being rebased but hit conflict) and pruned it, now what
  `hg rebase --continue` does is: skip that dropped commit and move
  on to rebase the next commit and gets confused here because wdir
  has two parents which is because while we skipped that dropped
  commit wdir had two parents and we didn't update that to one parent.
 
  Changes in test file demonstrate the fixed behavior.

REPOSITORY
  rHG Mercurial

BRANCH
  default

REVISION DETAIL
  https://phab.mercurial-scm.org/D7730

AFFECTED FILES
  hgext/rebase.py
  tests/test-rebase-conflicts.t

CHANGE DETAILS

diff --git a/tests/test-rebase-conflicts.t b/tests/test-rebase-conflicts.t
--- a/tests/test-rebase-conflicts.t
+++ b/tests/test-rebase-conflicts.t
@@ -478,14 +478,13 @@
   $ hg resolve -m
   (no more unresolved files)
   continue: hg rebase --continue
-XXX: it should have rebased revision 3 since it made changes unrelated to
-destination, so no reason to say "its destination already has all its changes"
   $ hg rebase -c
   note: not rebasing 2:06a50ac6b5ab "conflict in a", it has no successor
   rebasing 3:aea370672fd7 "add b" (tip)
-  note: not rebasing 3:aea370672fd7 "add b" (tip), its destination already has all its changes
   $ hg tglog
-  @  1:draft 'edit a'
+  @  4:draft 'add b'
+  |
+  o  1:draft 'edit a'
   |
   o  0:draft 'add a'
   
diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -594,6 +594,10 @@
                 adjustdest(repo, rev, self.destmap, self.state, self.skipped)
             )
             self.state[rev] = dest
+            # since we are done, make sure wdir has one parent (issue6180)
+            if len(repo[None].parents()) == 2:
+                p1 = repo[None].p1().node()
+                repo.setparents(p1)
         elif self.state[rev] == revtodo:
             ui.status(_(b'rebasing %s\n') % desc)
             progressfn(ctx)



To: khanchi97, martinvonz, #hg-reviewers
Cc: mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

D7730: rebase: make sure pruning does not confuse rebase (issue6180)

marmoute (Pierre-Yves David)
khanchi97 added a comment.


  Ping.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7730/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7730

To: khanchi97, martinvonz, #hg-reviewers
Cc: mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

D7730: rebase: make sure pruning does not confuse rebase (issue6180)

marmoute (Pierre-Yves David)
In reply to this post by marmoute (Pierre-Yves David)
martinvonz added inline comments.

INLINE COMMENTS

> rebase.py:597
>              self.state[rev] = dest
> +            # since we are done, make sure wdir has one parent (issue6180)
> +            if len(repo[None].parents()) == 2:

I think it's incorrect that rebase sets two parents while the merge is being resolved, but that's out of scope for this patch.

> rebase.py:598
> +            # since we are done, make sure wdir has one parent (issue6180)
> +            if len(repo[None].parents()) == 2:
> +                p1 = repo[None].p1().node()

Should this be `self.wctx.parents()` to work with in-memory rebase?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7730/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7730

To: khanchi97, martinvonz, #hg-reviewers
Cc: mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

D7730: rebase: make sure pruning does not confuse rebase (issue6180)

marmoute (Pierre-Yves David)
In reply to this post by marmoute (Pierre-Yves David)
pulkit added a comment.


  Unrelated to the fix, we need better way to skip commits during rebasing. Pruning manually is not a good option, IIRC git rebase have a `--skip` flag.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7730/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7730

To: khanchi97, martinvonz, #hg-reviewers
Cc: pulkit, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

D7730: rebase: make sure pruning does not confuse rebase (issue6180)

marmoute (Pierre-Yves David)
In reply to this post by marmoute (Pierre-Yves David)
khanchi97 updated this revision to Diff 19283.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D7730?vs=18939&id=19283

BRANCH
  default

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7730/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7730

AFFECTED FILES
  hgext/rebase.py
  tests/test-rebase-conflicts.t

CHANGE DETAILS

diff --git a/tests/test-rebase-conflicts.t b/tests/test-rebase-conflicts.t
--- a/tests/test-rebase-conflicts.t
+++ b/tests/test-rebase-conflicts.t
@@ -476,14 +476,13 @@
   $ hg resolve -m
   (no more unresolved files)
   continue: hg rebase --continue
-XXX: it should have rebased revision 3 since it made changes unrelated to
-destination, so no reason to say "its destination already has all its changes"
   $ hg rebase -c
   note: not rebasing 2:06a50ac6b5ab "conflict in a", it has no successor
   rebasing 3:aea370672fd7 "add b" (tip)
-  note: not rebasing 3:aea370672fd7 "add b" (tip), its destination already has all its changes
   $ hg tglog
-  @  1:draft 'edit a'
+  @  4:draft 'add b'
+  |
+  o  1:draft 'edit a'
   |
   o  0:draft 'add a'
   
diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -594,6 +594,10 @@
                 adjustdest(repo, rev, self.destmap, self.state, self.skipped)
             )
             self.state[rev] = dest
+            # since we are done, make sure wdir has one parent (issue6180)
+            if len(self.wctx.parents()) == 2:
+                p1 = self.wctx.p1().node()
+                repo.setparents(p1)
         elif self.state[rev] == revtodo:
             ui.status(_(b'rebasing %s\n') % desc)
             progressfn(ctx)



To: khanchi97, martinvonz, #hg-reviewers
Cc: pulkit, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

D7730: rebase: make sure pruning does not confuse rebase (issue6180)

marmoute (Pierre-Yves David)
In reply to this post by marmoute (Pierre-Yves David)
khanchi97 added a comment.


  In D7730#114953 <https://phab.mercurial-scm.org/D7730#114953>, @pulkit wrote:
 
  > Unrelated to the fix, we need better way to skip commits during rebasing. Pruning manually is not a good option, IIRC git rebase have a `--skip` flag.
 
  Yeah, that's a good idea. We should also have --skip flag to skip the commit on which rebase got interrupted.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7730/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7730

To: khanchi97, martinvonz, #hg-reviewers
Cc: pulkit, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

D7730: rebase: make sure pruning does not confuse rebase (issue6180)

marmoute (Pierre-Yves David)
In reply to this post by marmoute (Pierre-Yves David)
khanchi97 added inline comments.

INLINE COMMENTS

> martinvonz wrote in rebase.py:597
> I think it's incorrect that rebase sets two parents while the merge is being resolved, but that's out of scope for this patch.

I will look into it.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7730/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7730

To: khanchi97, martinvonz, #hg-reviewers
Cc: pulkit, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

D7730: rebase: make sure pruning does not confuse rebase (issue6180)

marmoute (Pierre-Yves David)
In reply to this post by marmoute (Pierre-Yves David)
martinvonz added inline comments.

INLINE COMMENTS

> khanchi97 wrote in rebase.py:597
> I will look into it.

No need, I've already sent D7827 <https://phab.mercurial-scm.org/D7827>

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7730/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7730

To: khanchi97, martinvonz, #hg-reviewers
Cc: pulkit, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

D7730: rebase: make sure pruning does not confuse rebase (issue6180)

marmoute (Pierre-Yves David)
In reply to this post by marmoute (Pierre-Yves David)
martinvonz added inline comments.

INLINE COMMENTS

> rebase.py:600
> +                p1 = self.wctx.p1().node()
> +                repo.setparents(p1)
>          elif self.state[rev] == revtodo:

Actually, doesn't this need to be `wctx.setparents()` (which you can do now that D7822 <https://phab.mercurial-scm.org/D7822> has been queued) in order to work with in-memory rebase? Maybe time to add a test case with in-memory rebase?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7730/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7730

To: khanchi97, martinvonz, #hg-reviewers
Cc: pulkit, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

D7730: rebase: make sure pruning does not confuse rebase (issue6180)

marmoute (Pierre-Yves David)
In reply to this post by marmoute (Pierre-Yves David)
marmoute added a comment.


  I think is would be simpler and sfare to prevent unrelated operation during rebase. If the user cannot prune here we won't have to deal with it. This woudl also apply to other operation that can alter the repository, like another rebase, amend or a pull. Starting using a unified and safe approach seems to provide more benefit with less chance of UI inconsistency.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7730/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7730

To: khanchi97, martinvonz, #hg-reviewers
Cc: marmoute, pulkit, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

D7730: rebase: make sure pruning does not confuse rebase (issue6180)

marmoute (Pierre-Yves David)
In reply to this post by marmoute (Pierre-Yves David)
khanchi97 updated this revision to Diff 19816.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D7730?vs=19283&id=19816

BRANCH
  default

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7730/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7730

AFFECTED FILES
  hgext/rebase.py
  tests/test-rebase-conflicts.t

CHANGE DETAILS

diff --git a/tests/test-rebase-conflicts.t b/tests/test-rebase-conflicts.t
--- a/tests/test-rebase-conflicts.t
+++ b/tests/test-rebase-conflicts.t
@@ -481,14 +481,13 @@
   $ hg resolve -m
   (no more unresolved files)
   continue: hg rebase --continue
-XXX: it should have rebased revision 3 since it made changes unrelated to
-destination, so no reason to say "its destination already has all its changes"
   $ hg rebase -c
   note: not rebasing 2:06a50ac6b5ab "conflict in a", it has no successor
   rebasing 3:aea370672fd7 "add b" (tip)
-  note: not rebasing 3:aea370672fd7 "add b" (tip), its destination already has all its changes
   $ hg tglog
-  @  1:draft 'edit a'
+  @  4:draft 'add b'
+  |
+  o  1:draft 'edit a'
   |
   o  0:draft 'add a'
   
diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -594,6 +594,10 @@
                 adjustdest(repo, rev, self.destmap, self.state, self.skipped)
             )
             self.state[rev] = dest
+            # since we are done, make sure wdir has one parent (issue6180)
+            if len(self.wctx.parents()) == 2:
+                p1 = self.wctx.p1().node()
+                self.wctx.setparents(p1)
         elif self.state[rev] == revtodo:
             ui.status(_(b'rebasing %s\n') % desc)
             progressfn(ctx)



To: khanchi97, martinvonz, #hg-reviewers
Cc: marmoute, pulkit, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

D7730: rebase: make sure pruning does not confuse rebase (issue6180)

marmoute (Pierre-Yves David)
In reply to this post by marmoute (Pierre-Yves David)
pulkit added a comment.


  In D7730#117268 <https://phab.mercurial-scm.org/D7730#117268>, @marmoute wrote:
 
  > I think is would be simpler and sfare to prevent unrelated operation during rebase. If the user cannot prune here we won't have to deal with it. This woudl also apply to other operation that can alter the repository, like another rebase, amend or a pull. Starting using a unified and safe approach seems to provide more benefit with less chance of UI inconsistency.
 
  I agree. We should disallow prune if an unfinished operation exists.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7730/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7730

To: khanchi97, martinvonz, #hg-reviewers
Cc: marmoute, pulkit, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

D7730: rebase: make sure pruning does not confuse rebase (issue6180)

marmoute (Pierre-Yves David)
In reply to this post by marmoute (Pierre-Yves David)
This revision now requires changes to proceed.
marmoute added a comment.
marmoute requested changes to this revision.


  In D7730#119191 <https://phab.mercurial-scm.org/D7730#119191>, @pulkit wrote:
 
  > In D7730#117268 <https://phab.mercurial-scm.org/D7730#117268>, @marmoute wrote:
  >
  >> I think is would be simpler and sfare to prevent unrelated operation during rebase. If the user cannot prune here we won't have to deal with it. This woudl also apply to other operation that can alter the repository, like another rebase, amend or a pull. Starting using a unified and safe approach seems to provide more benefit with less chance of UI inconsistency.
  >
  > I agree. We should disallow prune if an unfinished operation exists.
 
  Okay, lets to in this direction then.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7730/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7730

To: khanchi97, martinvonz, #hg-reviewers, marmoute
Cc: marmoute, pulkit, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel