Patchwork D7630: RFC: absorb: make the absorbed changeset be automatically "evolved"

login
register
mail settings
Submitter phabricator
Date Dec. 13, 2019, 5:55 a.m.
Message ID <differential-rev-PHID-DREV-ottoa25dpxrclhdlmk33-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/43775/
State New
Headers show

Comments

phabricator - Dec. 13, 2019, 5:55 a.m.
rdamazio created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  When a committed changeset is absorbed, this will make it so it's not used for
  computing the absorption, but is still recreated at the destination.

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

AFFECTED FILES
  hgext/absorb.py
  tests/test-absorb-rev.t

CHANGE DETAILS




To: rdamazio, #hg-reviewers
Cc: mercurial-devel
phabricator - Dec. 13, 2019, 6:11 a.m.
martinvonz added inline comments.

INLINE COMMENTS

> test-absorb-rev.t:67
>  
>    $ hg absorb --apply-changes -r .
>    2 of 2 chunk(s) applied

`hg absorb` has a `-r` flag? Did you forgot to upload some ancestors of this commit?

REPOSITORY
  rHG Mercurial

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

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

To: rdamazio, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - Dec. 13, 2019, 6:47 a.m.
martinvonz added inline comments.

INLINE COMMENTS

> martinvonz wrote in test-absorb-rev.t:67
> `hg absorb` has a `-r` flag? Did you forgot to upload some ancestors of this commit?

Oh, it's added in D7631 <https://phab.mercurial-scm.org/D7631>, which is marked as a *child* of this review. How did that happen? `hg phabsend` should set the relationships automatically. How did you run that command? Just `hg phabsend .^::.` or  similar? Or did you manually mark D7631 <https://phab.mercurial-scm.org/D7631> as a child?

REPOSITORY
  rHG Mercurial

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

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

To: rdamazio, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - Dec. 13, 2019, 6:52 a.m.
rdamazio added a comment.


  (on mobile) I think it was `hg phabsend -r .+.^`
  
  - F454947: smime.p7s <https://phab.mercurial-scm.org/F454947>

REPOSITORY
  rHG Mercurial

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

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

To: rdamazio, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - Dec. 13, 2019, 6:57 a.m.
martinvonz added a comment.


  In D7630#112243 <https://phab.mercurial-scm.org/D7630#112243>, @rdamazio wrote:
  
  > (on mobile) I think it was `hg phabsend -r .+.^`
  
  Ohh, that makes some sense that phabsend might do it backwards if you pass the revisions backwards, but that's still clearly a bug. I'll report it in bugzilla if it's not already there. I'll manually update the parent/child relationship on these reviews for now.

REPOSITORY
  rHG Mercurial

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

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

To: rdamazio, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - Dec. 14, 2019, 6:02 a.m.
rdamazio added a comment.


  Sorry for the upload mess (though it's arguably a `phabsend` bug :) ). Tried uploading the "right" way now.

REPOSITORY
  rHG Mercurial

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

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

To: rdamazio, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - Jan. 13, 2020, 3:57 p.m.
pulkit added a comment.


  This results in an empty commit which is not similar to what rebase or evolve will generally result in after `D7631` unless `ui.allowemptycommit=True` is set. I think good behavior is to obsolete the absorbed changeset in favour of either it's parent or one of the revs in which it was absorbed.

REPOSITORY
  rHG Mercurial

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

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

To: rdamazio, #hg-reviewers
Cc: pulkit, martinvonz, mercurial-devel
phabricator - Jan. 13, 2020, 5:35 p.m.
martinvonz added a comment.


  In D7630#112603 <https://phab.mercurial-scm.org/D7630#112603>, @rdamazio wrote:
  
  > Sorry for the upload mess (though it's arguably a `phabsend` bug :) ). Tried uploading the "right" way now.
  
  I reported that as https://bz.mercurial-scm.org/show_bug.cgi?id=6241.
  
  In D7630#115300 <https://phab.mercurial-scm.org/D7630#115300>, @pulkit wrote:
  
  > This results in an empty commit which is not similar to what rebase or evolve will generally result in after `D7631` unless `ui.allowemptycommit=True` is set. I think good behavior is to obsolete the absorbed changeset in favour of either it's parent or one of the revs in which it was absorbed.
  
  I made a related comment on the parent patch before I realized that this patch adds obsmarker handling. My suggestion there was to mark all the commits that got absorbed into as successors, and if there's anything left in the absorbed commit, that would be yet another successor. Would that work?

REPOSITORY
  rHG Mercurial

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

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

To: rdamazio, #hg-reviewers
Cc: pulkit, martinvonz, mercurial-devel
phabricator - Jan. 13, 2020, 5:37 p.m.
pulkit added a comment.


  >>>! In D7630#115300 <https://phab.mercurial-scm.org/D7630#115300>, @pulkit wrote:
  
  >> This results in an empty commit which is not similar to what rebase or evolve will generally result in after `D7631` unless `ui.allowemptycommit=True` is set. I think good behavior is to obsolete the absorbed changeset in favour of either it's parent or one of the revs in which it was absorbed.
  >
  > I made a related comment on the parent patch before I realized that this patch adds obsmarker handling. My suggestion there was to mark all the commits that got absorbed into as successors, and if there's anything left in the absorbed commit, that would be yet another successor. Would that work?
  
  Yep, that sounds good.

REPOSITORY
  rHG Mercurial

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

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

To: rdamazio, #hg-reviewers
Cc: pulkit, martinvonz, mercurial-devel
phabricator - Jan. 23, 2020, 4:37 p.m.
marmoute added a comment.


  In D7630#115320 <https://phab.mercurial-scm.org/D7630#115320>, @pulkit wrote:
  
  >>> This results in an empty commit which is not similar to what rebase or evolve will generally result in after `D7631` unless `ui.allowemptycommit=True` is set. I think good behavior is to obsolete the absorbed changeset in favour of either it's parent or one of the revs in which it was absorbed.
  >>
  >> I made a related comment on the parent patch before I realized that this patch adds obsmarker handling. My suggestion there was to mark all the commits that got absorbed into as successors, and if there's anything left in the absorbed commit, that would be yet another successor. Would that work?
  >
  > Yep, that sounds good.
  
  This means generate split+fold markers. That is going to be fun to deal with :-). I dont' really have any much better idea however (the alternative seems to use simple prune markers, which is meh).

REPOSITORY
  rHG Mercurial

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

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

To: rdamazio, #hg-reviewers
Cc: marmoute, pulkit, martinvonz, mercurial-devel

Patch

diff --git a/tests/test-absorb-rev.t b/tests/test-absorb-rev.t
--- a/tests/test-absorb-rev.t
+++ b/tests/test-absorb-rev.t
@@ -65,26 +65,18 @@ 
 Run absorb:
 
   $ hg absorb --apply-changes -r .
-  1 new orphan changesets
   2 of 2 chunk(s) applied
 
 Check that the pending wdir change was left alone:
+TODO: The absorbed commit should have disappeared when it became empty.
 
   $ grep 6 a
   6
-  $ hg update -Cq .
-
-Rebase the absorbed revision on top of the destination (as evolve would):
-TODO: the evolve-type operation should happen automatically for the changeset
-being absorbed, and even through that the pending wdir change should be left
-alone.
-
-  $ hg rebase -d tip -r .
-  rebasing 5:1631091f9648 "commit to absorb"
-  note: not rebasing 5:1631091f9648 "commit to absorb", its destination already has all its changes
 
   $ hg log -G -T '{node|short} {desc} {instabilities}'
-  @  2f7ba78d6abc commit 5
+  @  9a0ec5cae1a1 commit to absorb
+  |
+  o  2f7ba78d6abc commit 5
   |
   o  04c8ba6df782 commit 4
   |
@@ -94,12 +86,15 @@ 
   |
   o  241ace8326d0 commit 1
   
-  $ hg annotate -c a
-  241ace8326d0: 1a
-  9b19176bb127: 2b
-  484c6ac0cea3: 3
-  04c8ba6df782: 4d
-  2f7ba78d6abc: 5e
+  $ hg annotate -c a -r 'wdir()'
+  241ace8326d0 : 1a
+  9b19176bb127 : 2b
+  484c6ac0cea3 : 3
+  04c8ba6df782 : 4d
+  2f7ba78d6abc : 5e
+  9a0ec5cae1a1+: 6
+
+  $ hg diff -c .
 
 Do it again, but this time with an unrelated commit checked out (plus working
 directory changes on top):
@@ -130,7 +125,6 @@ 
   2 changesets affected
   2f7ba78 commit 5
   04c8ba6 commit 4
-  1 new orphan changesets
   1 of 1 chunk(s) applied
 
   $ hg annotate -c a -r 'wdir()'
@@ -138,19 +132,16 @@ 
   dbce69d9fe03 : committed unrelated
   dbce69d9fe03+: pending wdir change
 
-
-  $ hg update -Cq .
-
-  $ hg rebase -d tip -r ${TOABSORB}
-  rebasing \d+:[0-9a-f]+ "commit to absorb 2" (re)
-  note: not rebasing \d+:[0-9a-f]+ "commit to absorb 2", its destination already has all its changes (re)
+  $ hg update -Cq tip
 
   $ hg log -G -T '{node|short} {desc} {instabilities}'
+  @  59655ff113fb commit to absorb 2
+  |
   o  789b01face13 commit 5
   |
   o  9c83c60f49f2 commit 4
   |
-  | @  dbce69d9fe03 unrelated commit
+  | o  dbce69d9fe03 unrelated commit
   | |
   o |  484c6ac0cea3 commit 3
   | |
@@ -159,7 +150,7 @@ 
   o  241ace8326d0 commit 1
   
 
-  $ hg annotate -c a -r tip
+  $ hg annotate -c a
   241ace8326d0: 1a
   9b19176bb127: 2b
   484c6ac0cea3: 3
diff --git a/hgext/absorb.py b/hgext/absorb.py
--- a/hgext/absorb.py
+++ b/hgext/absorb.py
@@ -34,6 +34,7 @@ 
 from __future__ import absolute_import
 
 import collections
+import itertools
 
 from mercurial.i18n import _
 from mercurial import (
@@ -661,16 +662,19 @@ 
         4. call commit, to commit changes to hg database
     """
 
-    def __init__(self, stack, ui=None, opts=None):
+    def __init__(self, stack, fixuptargets=[], ui=None, opts=None):
         """([ctx], ui or None) -> None
 
         stack: should be linear, and sorted by topo order - oldest first.
+        fixuptargets: changeset contexts that need to be fixed up, but are not
+            used for fixup computation.
         all commits in stack are considered mutable.
         """
         assert stack
         self.ui = ui or nullui()
         self.opts = opts or {}
         self.stack = stack
+        self.fixuptargets = fixuptargets
         self.repo = stack[-1].repo().unfiltered()
 
         # following fields will be filled later
@@ -776,7 +780,7 @@ 
         # p1 which overrides the parent of the next commit, "None" means use
         # the original parent unchanged
         nextp1 = None
-        for ctx in self.stack:
+        for ctx in itertools.chain(self.stack, self.fixuptargets):
             memworkingcopy = self._getnewfilecontents(ctx)
             if not memworkingcopy and not lastcommitted:
                 # nothing changed, nothing commited
@@ -1008,7 +1012,10 @@ 
         pats = ()
     if opts is None:
         opts = {}
-    state = fixupstate(stack, ui=ui, opts=opts)
+    fixuptargets = []
+    if targetctx.rev() is not None:
+        fixuptargets.append(targetctx)
+    state = fixupstate(stack, fixuptargets=fixuptargets, ui=ui, opts=opts)
     matcher = scmutil.match(targetctx, pats, opts)
     if opts.get(b'interactive'):
         diff = patch.diff(repo, stack[-1].node(), targetctx.node(), matcher)