This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7630

absorb: make the absorbed changeset be automatically "evolved"
Needs RevisionPublic

Authored by rdamazio on Dec 13 2019, 12:55 AM.

Details

Reviewers
baymax
Group Reviewers
hg-reviewers
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.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

rdamazio created this revision.Dec 13 2019, 12:55 AM
martinvonz added a subscriber: martinvonz.Dec 13 2019, 1:11 AM
martinvonz added inline comments.
tests/test-absorb-rev.t
88–89

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

martinvonz added inline comments.Dec 13 2019, 1:47 AM
tests/test-absorb-rev.t
88–89

Oh, it's added in 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 as a child?

rdamazio added a comment.Dec 13 2019, 1:52 AM

(on mobile) I think it was hg phabsend -r .+.^

martinvonz added a comment.Dec 13 2019, 1:57 AM
In 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.

rdamazio marked 2 inline comments as done.Dec 14 2019, 1:02 AM
rdamazio retitled this revision from RFC: absorb: make the absorbed changeset be automatically "evolved" to absorb: make the absorbed changeset be automatically "evolved".
rdamazio updated this revision to Diff 18727.
rdamazio added a comment.Dec 14 2019, 1:02 AM

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

rdamazio updated this revision to Diff 18729.Dec 14 2019, 1:25 AM
rdamazio updated this revision to Diff 18842.Dec 17 2019, 11:42 PM
rdamazio updated this revision to Diff 18844.Dec 17 2019, 11:46 PM
rdamazio updated this revision to Diff 18874.Dec 19 2019, 2:36 AM
rdamazio updated this revision to Diff 19049.Jan 6 2020, 7:46 PM
pulkit added a subscriber: pulkit.Jan 13 2020, 10:57 AM

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.

martinvonz added a comment.Jan 13 2020, 12:35 PM
In 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, @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?

pulkit added a comment.EditedJan 13 2020, 12:37 PM

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.

marmoute added a subscriber: marmoute.Jan 23 2020, 11:37 AM
In 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).

rdamazio updated this revision to Diff 19812.Feb 1 2020, 2:10 AM
rdamazio added a comment.Feb 1 2020, 2:10 AM
In 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.

It's not *always* empty - if there had been lines that it failed absorb (because it couldn't find where to absorb into), those would be left in the commit.

In D7630#117270, @marmoute wrote:
In 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.

I'm fine with doing this, but is there an efficient way to detect that it became empty?

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).

I'm not following this part - why do you need markers at all?
(you can argue whether you want them - but I don't see why you *need* them)

rdamazio added a comment.Feb 1 2020, 2:12 AM
In D7630#117270, @marmoute wrote:
In 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.

I'm fine with doing this, but is there an efficient way to detect that it became empty?

And by "this" I meant I'm fine with making it disappear if allowemptycommit is False. I don't fully understand how markers help accomplish that.

rdamazio updated this revision to Diff 19982.Feb 6 2020, 10:11 PM
martinvonz added a comment.Feb 11 2020, 1:27 PM
In D7630#118997, @rdamazio wrote:
In D7630#117270, @marmoute wrote:
In 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.

I'm fine with doing this, but is there an efficient way to detect that it became empty?

And by "this" I meant I'm fine with making it disappear if allowemptycommit is False. I don't fully understand how markers help accomplish that.

When you try to create an empty commit, you'll get a None back for the nodeid (from repo.commitctx(), IIRC), so check for that.

marmoute added a comment.Feb 11 2020, 5:40 PM

It looks like this series is introducing UI change of the same kind as the one @martinvonz is looking into for hg copy. I'll try to have a look at both of them tomorrow.

baymax requested changes to this revision.Jun 8 2020, 1:34 PM

There seems to have been no activities on this Diff for the past 3 Months.

By policy, we are automatically moving it out of the need-review state.

Please, move it back to need-review without hesitation if this diff should still be discussed.

:baymax:need-review-idle:

This revision now requires changes to proceed.Jun 8 2020, 1:34 PM

Revision Contents
Changeset List

PathPackages
Mhgext/absorb.py (13 lines)
Mtests/test-absorb-rev.t (79 lines)

Diff 19982

hgext/absorb.py

# TODO: # TODO:
# * Rename config items to [commands] namespace # * Rename config items to [commands] namespace
# * Converge getdraftstack() with other code in core # * Converge getdraftstack() with other code in core
# * move many attributes on fixupstate to be private # * move many attributes on fixupstate to be private
from __future__ import absolute_import from __future__ import absolute_import
import collections import collections
import itertools
from mercurial.i18n import _ from mercurial.i18n import _
from mercurial import ( from mercurial import (
cmdutil, cmdutil,
commands, commands,
context, context,
crecord, crecord,
error, error,
a typical use is like filefixupstates: a typical use is like filefixupstates:
1. call diffwith, to calculate fixups 1. call diffwith, to calculate fixups
2. (optionally), present fixups to the user, or edit fixups 2. (optionally), present fixups to the user, or edit fixups
3. call apply, to apply changes to memory 3. call apply, to apply changes to memory
4. call commit, to commit changes to hg database 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 """([ctx], ui or None) -> None
stack: should be linear, and sorted by topo order - oldest first. 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. all commits in stack are considered mutable.
""" """
assert stack assert stack
self.ui = ui or nullui() self.ui = ui or nullui()
self.opts = opts or {} self.opts = opts or {}
self.stack = stack self.stack = stack
self.fixuptargets = fixuptargets
self.repo = stack[-1].repo().unfiltered() self.repo = stack[-1].repo().unfiltered()
# following fields will be filled later # following fields will be filled later
self.paths = [] # [str] self.paths = [] # [str]
self.status = None # ctx.status output self.status = None # ctx.status output
self.fctxmap = {} # {path: {ctx: fctx}} self.fctxmap = {} # {path: {ctx: fctx}}
self.fixupmap = {} # {path: filefixupstate} self.fixupmap = {} # {path: filefixupstate}
self.replacemap = {} # {oldnode: newnode or None} self.replacemap = {} # {oldnode: newnode or None}
"""make new commits. update self.finalnode, self.replacemap. """make new commits. update self.finalnode, self.replacemap.
it is splitted from "commit" to avoid too much indentation. it is splitted from "commit" to avoid too much indentation.
""" """
# last node (20-char) committed by us # last node (20-char) committed by us
lastcommitted = None lastcommitted = None
# p1 which overrides the parent of the next commit, "None" means use # p1 which overrides the parent of the next commit, "None" means use
# the original parent unchanged # the original parent unchanged
nextp1 = None nextp1 = None
for ctx in self.stack: for ctx in itertools.chain(self.stack, self.fixuptargets):
memworkingcopy = self._getnewfilecontents(ctx) memworkingcopy = self._getnewfilecontents(ctx)
if not memworkingcopy and not lastcommitted: if not memworkingcopy and not lastcommitted:
# nothing changed, nothing commited # nothing changed, nothing commited
nextp1 = ctx nextp1 = ctx
continue continue
if self._willbecomenoop(memworkingcopy, ctx, nextp1): if self._willbecomenoop(memworkingcopy, ctx, nextp1):
# changeset is no longer necessary # changeset is no longer necessary
self.replacemap[ctx.node()] = None self.replacemap[ctx.node()] = None
% limit % limit
) )
if not stack: if not stack:
raise error.Abort(_(b'no mutable changeset to change')) raise error.Abort(_(b'no mutable changeset to change'))
if pats is None: if pats is None:
pats = () pats = ()
if opts is None: if opts is None:
opts = {} 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) matcher = scmutil.match(targetctx, pats, opts)
if opts.get(b'interactive'): if opts.get(b'interactive'):
diff = patch.diff(repo, stack[-1].node(), targetctx.node(), matcher) diff = patch.diff(repo, stack[-1].node(), targetctx.node(), matcher)
origchunks = patch.parsepatch(diff) origchunks = patch.parsepatch(diff)
chunks = cmdutil.recordfilter(ui, origchunks, matcher)[0] chunks = cmdutil.recordfilter(ui, origchunks, matcher)[0]
targetctx = overlaydiffcontext(stack[-1], chunks) targetctx = overlaydiffcontext(stack[-1], chunks)
fm = None fm = None
if opts.get(b'print_changes') or not opts.get(b'apply_changes'): if opts.get(b'print_changes') or not opts.get(b'apply_changes'):

tests/test-absorb-rev.t

[255] [255]
Add an uncommitted working directory change: Add an uncommitted working directory change:
$ echo 6 >> a $ echo 6 >> a
Run absorb: Run absorb:
$ hg absorb --apply-changes --from . $ hg absorb --apply-changes --from .
1 new orphan changesets
2 of 2 chunk(s) applied 2 of 2 chunk(s) applied
martinvonzUnsubmitted
Done

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

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

Oh, it's added in 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 as a child?

martinvonz: Oh, it's added in D7631, which is marked as a *child* of this review. How did that happen? `hg…
Note: this only shows disconnected graphs because the root node was also
absorbed into.
$ hg glog $ hg glog
@ 9a0ec5cae1a1 commit to absorb
|
o 2f7ba78d6abc commit 5 o 2f7ba78d6abc commit 5
| |
o 04c8ba6df782 commit 4 o 04c8ba6df782 commit 4
| |
o 484c6ac0cea3 commit 3 o 484c6ac0cea3 commit 3
| |
o 9b19176bb127 commit 2 o 9b19176bb127 commit 2
| |
o 241ace8326d0 commit 1 o 241ace8326d0 commit 1
@ 1631091f9648 commit to absorb orphan
|
x 4f55fa657dae commit 5
|
x ad8b8b75557f commit 4
|
x 43f0a75bede7 commit 3
|
x 5c5f95224a50 commit 2
|
x 4ec16f85269a commit 1
Check that the pending wdir change was left alone: Check that the pending wdir change was left alone:
$ hg status $ hg status
M a M a
$ hg diff $ hg diff
diff -r [0-9a-f]+ a (re) diff -r [0-9a-f]+ a (re)
--- a/a Thu Jan 01 00:00:00 1970 +0000 --- a/a Thu Jan 01 00:00:00 1970 +0000
+++ b/a Thu Jan 01 00:00:00 1970 +0000 +++ b/a Thu Jan 01 00:00:00 1970 +0000
@@ -3,3 +3,4 @@ @@ -3,3 +3,4 @@
3 3
4d 4d
5e 5e
+6 +6
$ hg update -Cq .
Rebase the absorbed revision on top of the destination (as evolve would): $ hg annotate -c a -r 'wdir()'
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 glog
@ 2f7ba78d6abc commit 5
|
o 04c8ba6df782 commit 4
|
o 484c6ac0cea3 commit 3
|
o 9b19176bb127 commit 2
|
o 241ace8326d0 commit 1
$ hg annotate -c a
241ace8326d0: 1a 241ace8326d0 : 1a
9b19176bb127: 2b 9b19176bb127 : 2b
484c6ac0cea3: 3 484c6ac0cea3 : 3
04c8ba6df782: 4d 04c8ba6df782 : 4d
2f7ba78d6abc: 5e 2f7ba78d6abc : 5e
9a0ec5cae1a1+: 6
TODO: The absorbed commit should have disappeared when it became empty.
$ hg diff -c .
$ hg debugobsolete $(hg log -T "{node}\n" -r .)
1 new obsolescence markers
obsoleted 1 changesets
$ hg update -Cq '.^'
Do it again, but this time with an unrelated commit checked out (plus working Do it again, but this time with an unrelated commit checked out (plus working
directory changes on top): directory changes on top):
$ cat > a <<EOF $ cat > a <<EOF
> 1a > 1a
> 2b > 2b
> 3 > 3
04c8ba6 -4d 04c8ba6 -4d
2f7ba78 -5e 2f7ba78 -5e
04c8ba6 +4f 04c8ba6 +4f
2f7ba78 +5g 2f7ba78 +5g
2 changesets affected 2 changesets affected
2f7ba78 commit 5 2f7ba78 commit 5
04c8ba6 commit 4 04c8ba6 commit 4
1 new orphan changesets
1 of 1 chunk(s) applied 1 of 1 chunk(s) applied
$ hg glog $ hg glog
o da8aed1fcb63 commit to absorb 2
|
o 789b01face13 commit 5 o 789b01face13 commit 5
| |
o 9c83c60f49f2 commit 4 o 9c83c60f49f2 commit 4
| |
| @ dbce69d9fe03 unrelated commit | @ dbce69d9fe03 unrelated commit
| | | |
| | * 410fcc72c5c9 commit to absorb 2 orphan
| | |
| | x 2f7ba78d6abc commit 5
| | |
+---x 04c8ba6df782 commit 4
| |
o | 484c6ac0cea3 commit 3 o | 484c6ac0cea3 commit 3
| | | |
o | 9b19176bb127 commit 2 o | 9b19176bb127 commit 2
|/ |/
o 241ace8326d0 commit 1 o 241ace8326d0 commit 1
$ hg annotate -c a -r 'wdir()' $ hg annotate -c a -r 'wdir()'
241ace8326d0 : 1a 241ace8326d0 : 1a
dbce69d9fe03 : committed unrelated dbce69d9fe03 : committed unrelated
dbce69d9fe03+: pending wdir change dbce69d9fe03+: pending wdir change
$ hg update -Cq . $ hg update -Cq tip
$ 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 glog $ hg glog
o da8aed1fcb63 commit to absorb 2
|
o 789b01face13 commit 5 o 789b01face13 commit 5
| |
o 9c83c60f49f2 commit 4 o 9c83c60f49f2 commit 4
| |
| @ dbce69d9fe03 unrelated commit | o dbce69d9fe03 unrelated commit
| | | |
o | 484c6ac0cea3 commit 3 o | 484c6ac0cea3 commit 3
| | | |
o | 9b19176bb127 commit 2 o | 9b19176bb127 commit 2
|/ |/
o 241ace8326d0 commit 1 o 241ace8326d0 commit 1
$ hg annotate -c a -r tip $ hg annotate -c a
241ace8326d0: 1a 241ace8326d0: 1a
9b19176bb127: 2b 9b19176bb127: 2b
484c6ac0cea3: 3 484c6ac0cea3: 3
9c83c60f49f2: 4f 9c83c60f49f2: 4f
789b01face13: 5g 789b01face13: 5g