Page MenuHomePhabricator
Differential D7631

absorb: allowing committed changes to be absorbed into their ancestors
Needs RevisionPublic
Actions

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

Details

Reviewers
martinvonz
marmoute
Group Reviewers
hg-reviewers
Summary

Like before, absorb continues to not touch the working directory, so any
working directory changes are preserved, even if the working directory parent
changes underneath (for instance, because the current revision was replaced
when changes were absorbed into it).

This can also be combined with --interactive to absorb just part of a
commit into its parents.

This initial implementation has the shortcoming that it does not do anything
with the absorbed commit:

  • With obsmarkers, this means the user still needs to evolve, rebase or prune manually
  • Without obsmarkers, the old commits would not be stripped at all because their child was not replaced

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
quark added a subscriber: quark.Dec 13 2019, 1:21 PM

--rev seems ambiguous since there might be different kinds of revisions to specify - target and revisions to edit. Maybe something like --source, --from, --target?

rdamazio retitled this revision from RFC: absorb: allowing committed changes to be absorbed into their ancestors to absorb: allowing committed changes to be absorbed into their ancestors.Dec 14 2019, 1:02 AM
rdamazio updated this revision to Diff 18726.
rdamazio added a comment.Dec 14 2019, 1:02 AM
In D7631#112414, @quark wrote:

--rev seems ambiguous since there might be different kinds of revisions to specify - target and revisions to edit. Maybe something like --source, --from, --target?

Done. Used --source to match rebase.

I'm assuming no fundamental objections then? Removing the "RFC" part so it gets a proper review then.

rdamazio updated this revision to Diff 18728.Dec 14 2019, 1:25 AM
pulkit added a subscriber: pulkit.Dec 17 2019, 7:29 AM

Can you describe the feature a bit in the commit message. Specific things which I feel are missing:

  • what happens to wdir changes
  • what happens if source is a commit is not a head

I understand them but after reading the tests, so might be worth to add them to description.

Also, it will be nice if you add an entry in releasenotes.

hgext/absorb.py
997

need to do more checks here about being public commit etc. rewriteutil.precheck should help.

tests/test-absorb-rev.t
93

hg status|diff should be a better command here.

rdamazio marked 2 inline comments as done.Dec 17 2019, 11:42 PM
rdamazio edited the summary of this revision. (Show Details)
rdamazio updated this revision to Diff 18841.
rdamazio added a comment.Dec 17 2019, 11:42 PM

Can you describe the feature a bit in the commit message. Specific things which I feel are missing:

Done

Also, it will be nice if you add an entry in releasenotes.

Done

hgext/absorb.py
997

Done. Notice that *technically* the user could do such an absorb while in the middle of a merge, but it sounds like a bad idea and inviting troubles, so I'm letting it also disallow that case. I'll be surprised if anyone even notices.

rdamazio updated this revision to Diff 18843.Dec 17 2019, 11:46 PM
pulkit added inline comments.Dec 18 2019, 2:55 AM
hgext/absorb.py
1113

s/--rev/--source

relnotes/next
5

s/--rev/--source

rdamazio marked 2 inline comments as done.Dec 19 2019, 2:36 AM
rdamazio updated this revision to Diff 18873.
rdamazio marked 2 inline comments as done.Dec 19 2019, 2:36 AM
mharbison72 added subscribers: martinvonz, mharbison72.Jan 2 2020, 3:21 PM
In D7631#112604, @rdamazio wrote:
In D7631#112414, @quark wrote:

--rev seems ambiguous since there might be different kinds of revisions to specify - target and revisions to edit. Maybe something like --source, --from, --target?

Done. Used --source to match rebase.

Is --exact from hg fold a better model? I don't feel strongly; I only mention it because hg rebase -s will take that revision and its descendants, so it's more like "stack" in my mind. I'm not sure how many other commands have -s off the top of my head, but @martinvonz
mentioned adding that to hg fix (probably in IRC), and I think mentioned the word "stack" in that context. So I might not be the only one to get slightly tripped up by that.

I'm assuming no fundamental objections then? Removing the "RFC" part so it gets a proper review then.

I like it.

hgext/absorb.py
1141

Should it abort if multiple revisions are given, instead of picking the latest?

rdamazio marked an inline comment as done.Jan 6 2020, 7:46 PM
rdamazio updated this revision to Diff 19048.
rdamazio added a comment.Jan 6 2020, 7:46 PM
In D7631#114393, @mharbison72 wrote:
In D7631#112604, @rdamazio wrote:
In D7631#112414, @quark wrote:

--rev seems ambiguous since there might be different kinds of revisions to specify - target and revisions to edit. Maybe something like --source, --from, --target?

Done. Used --source to match rebase.

Is --exact from hg fold a better model? I don't feel strongly; I only mention it because hg rebase -s will take that revision and its descendants, so it's more like "stack" in my mind. I'm not sure how many other commands have -s off the top of my head, but @martinvonz
mentioned adding that to hg fix (probably in IRC), and I think mentioned the word "stack" in that context. So I might not be the only one to get slightly tripped up by that.

IMHO no, needing --exact is actually confusing to almost every user we've talked to, and they'd instead expect that to be the default behavior, with "fold up to this commit" being the one that needs a specific flag.

I'm assuming no fundamental objections then? Removing the "RFC" part so it gets a proper review then.

I like it.

Thanks

hgext/absorb.py
1141

I suspect other places may want something similar (e.g. it'd make sense in rebase --dest, so I changed revsingle to add the behavior.

pulkit added inline comments.Jan 13 2020, 11:05 AM
hgext/absorb.py
997

Sorry, I misunderstood the patch earlier. rewriteutil.precheck on target rev is not very helpful as we are not obsolete-ing that in this rev, but we are re-writing it's ancestors. So, if target-rev is a head, and evolution.alloworphans=False is set, it will still create orphans.

Not sure what's the best way forward, maybe we should do rewriteutil.precheck for the parent instead until we start obsoleting this rev.

tests/test-absorb-rev.t
87–88

I locally added some hg log --graph calls before and after absorb call to understand what happens. It will be nice to add them as it will make things easier for others to understand.

martinvonz added a comment.Jan 13 2020, 12:31 PM
In D7631#112604, @rdamazio wrote:
In D7631#112414, @quark wrote:

--rev seems ambiguous since there might be different kinds of revisions to specify - target and revisions to edit. Maybe something like --source, --from, --target?

Done. Used --source to match rebase.

Sorry I didn't notice until now, but --source makes me think it will behave like hg rebase --source and absorb from the given commit and all its descendants. I would have preferred --from (and maybe a --into for choosing which commits to absorb into in the future).

hgext/absorb.py
997

Maybe I'm also misunderstanding what this patch does in that case. hg absorb -r A will not obsolete A? I would think it definitely should do that. Perhaps the successors or the absorbed commit should be all the nodes absorbed into as well as any potential leftovers (which were not absorbed).

tests/test-absorb-rev.t
78

nit: I think I've heard that ^ needs to be quoted on Windows, so maybe -s '.^+.'

mharbison72 added inline comments.Jan 16 2020, 11:49 AM
tests/test-absorb-rev.t
78

That's true in cmd.exe, but msys doesn't seem to care. (That said, I thought check-code enforced quoting around ^.)

rdamazio marked 5 inline comments as done.Sat, Feb 1, 2:10 AM
rdamazio updated this revision to Diff 19811.
rdamazio added a comment.Sat, Feb 1, 2:10 AM

Sorry for the delay in replying here.

hgext/absorb.py
997

See the child commit (D7630), which adds the "evolve" operation.

Because of the invariant about parent phases, checking that the revision being absorbed is not public also ensures that everything it's absorbing into is not public. Is that what you were looking for? If the commit A being absorbed is a draft and its parent is public, then absorb just won't find anywhere to absorb the lines and will leave everything in A.

About setting obsmarkers from the absorbed commit into the targets, while that's technically correct, I suspect it'll become a hard-to-navigate mess which adds very little. Do you want me to add that?

rdamazio updated this revision to Diff 19979.Thu, Feb 6, 9:55 PM
rdamazio updated this revision to Diff 19981.Thu, Feb 6, 10:11 PM
martinvonz requested changes to this revision.Tue, Feb 11, 1:25 PM

Sorry about the delay in responding. Please remember to rebase the series to the latest @ on hg-committed (the release notes are otherwise likely to conflict).

hgext/absorb.py
997

I think we should at least have a TODO about adding them.

By the way, without the next patch's auto-evolve feature, I'm not sure we should add such markers. I think they would trick hg evolve into moving any descendant commits onto the topmost commit that was absorbed into, but that's probably not what the user wants (they probably want child commits to be moved onto the absorbed commit's parent).

relnotes/5.3
4–7

please revert (sorry about this annoying effect of copy detection)

This revision now requires changes to proceed.Tue, Feb 11, 1:25 PM
marmoute requested changes to this revision.Thu, Feb 13, 6:31 PM
marmoute added a subscriber: marmoute.

As explained in this comment https://phab.mercurial-scm.org/D8030#120771 I find the idea of usign a changeset in place of the working copy interresting. However, this is a larger big UX change that seems to deserve a wider discussion with more of the community involved. In particular I am not convinced about the --rev flag usage.

See the linked comment for more details.

(Sorry for the delay this will involves, I wish the idea had explicitly surfaced earlier).

Revision Contents
Changeset List

PathPackages
Mhgext/absorb.py (35 lines)
Mmercurial/scmutil.py (8 lines)
Mrelnotes/5.3 (4 lines)
Mrelnotes/next (3 lines)
Ptests/test-absorb-rev.t (632 lines)
Copied from tests/test-absorb.t
Mtests/test-absorb.t (2 lines)

Diff 19981

View Options

hgext/absorb.py

Loading...
View Options

mercurial/scmutil.py

Loading...
View Options

relnotes/5.3

Loading...
View Options

relnotes/next

Loading...
View Options

tests/test-absorb-rev.t

Loading...
View Options

tests/test-absorb.t

Loading...