Page MenuHomePhabricator

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

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

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

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.Feb 1 2020, 2:10 AM
rdamazio updated this revision to Diff 19811.

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.Feb 6 2020, 9:55 PM
rdamazio updated this revision to Diff 19981.Feb 6 2020, 10:11 PM
martinvonz requested changes to this revision.Feb 11 2020, 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.Feb 11 2020, 1:25 PM
marmoute requested changes to this revision.Feb 13 2020, 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).

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

The sprint is not happening and I don't think we should wait for the next sprint (fall?) to move forward with this. Here's a Plan Page I wrote down in response to @marmoute's request D8030: on https://www.mercurial-scm.org/wiki/RevisionAsWDirPlan. The decision we reached was to use the name --at-rev on that patch. However, and if I understood correctly, the general sentiment was not that all similar arguments (like the on in this patch) should use that but rather that we should pick a name that sounded for each command. So in the case of this patch, I suppose --from is fine? We should mark it EXPERIMENTAL, though.

mjacob added a subscriber: mjacob.Tue, Mar 10, 11:15 PM

I like the name --from for the option. It would also make sense in combination with a possible future --into option.

hgext/absorb.py
983

I know that it was named like this before, but the name targetctx is quite confusing here, especially considering that the user-facing option will be something like --from or --source.

Can it be changed, in a separate patch, after there is consensus about how the option should be called?

997

My preferred option would be to add obsmarkers from the absorbed changeset to the changed changesets. This is what I would usually do if I do a poor man’s version of this functionality: I uncommit the "source" changeset, absorb, and prune the "source" changeset with the changed changesets as the successor.

It is true that hg evolve is likely to not behave in a useful manner in this case. But as someone who almost always specifies a successor when pruning, I already run into these problems regularly. Fixing that is probably out of scope for this discussion.

I tend to say that adding obsmarkers should be part of this patch, but I’m not very familiar with Mercurial’s preferred workflow for things like this.

I like the name --from for the option. It would also make sense in combination with a possible future --into option.

The unfortunate thing about --from is that it implies a range in hg fold, whereas here it seems to be a single(?) commit. It does make a lot of sense within the context of this command though (i.e. taking stuff from this commit). And it took me awhile to get comfortable with the meaning in hg fold, so maybe it can be changed to something else there? In any event, I don't want to hold this up over an experimental name, just thinking out loud.

I tend to agree with @martinvonz that setting the successor back to the commit it was absorbed into would be confusing, and seems like a way to split the stack into two stacks on that destination, with the corresponding merge conflicts. (Would the new "hg evolve => stabilize all descendants functionality" even stop at the break? I don't think it would.) I get what you're going for though if you want to track content. But it seems like it would cause non power users a lot of confusion.

I like the name --from for the option. It would also make sense in combination with a possible future --into option.

The unfortunate thing about --from is that it implies a range in hg fold, whereas here it seems to be a single(?) commit. It does make a lot of sense within the context of this command though (i.e. taking stuff from this commit). And it took me awhile to get comfortable with the meaning in hg fold, so maybe it can be changed to something else there? In any event, I don't want to hold this up over an experimental name, just thinking out loud.

The --from in hg fold does not only have a different meaning, it’s also a flag that does not take an argument. The hg fold command itself takes revisions as arguments, so people might think they pass a revision to --from while they actually pass a revision to hg fold (having the same meaning as if it is passed to -r). Coincidentally, I was asked for help today from someone misusing the command that way. The hg rewind command also takes a --from option, this time with an argument. I expect that at least one of it might get changed eventually and I think we should not block this patch on that discussion.

Of course, if someone comes up with a name that doesn’t have these problems and is intuitive, we should use that name.

I tend to agree with @martinvonz that setting the successor back to the commit it was absorbed into would be confusing, and seems like a way to split the stack into two stacks on that destination, with the corresponding merge conflicts. (Would the new "hg evolve => stabilize all descendants functionality" even stop at the break? I don't think it would.) I get what you're going for though if you want to track content. But it seems like it would cause non power users a lot of confusion.

I prepared a patch that helps to prevent that the stack gets split: https://foss.heptapod.net/mercurial/evolve/merge_requests/126

As an example:

echo a > a; hg add a; hg ci -m a
echo b > b; hg add b; hg ci -m b
echo c > c; hg add c; hg ci -m c
echo a1 > a; echo b1 > b; hg ci -m 'change a and b'
echo d > d; hg add d; hg ci -m d

Graph:

@  (4) d
|
o  (3) change a and b
|
o  (2) c
|
o  (1) b
|
o  (0) a

First, let’s emulate a run of hg absorb --from 3 that adds markers.

hg up 3
hg uncommit --all
hg up 2 --merge
hg absorb --apply-changes
hg prune -r 5 -s '6 + 7' --split

Graph:

@  (8) c
|
o  (7) b
|
o  (6) a

*  (4) d
|
x  (3) change a and b — split using prune, uncommit as 6, 7
|
x  (2) c — rebased using absorb as 8
|
x  (1) b — rewritten using absorb as 7
|
x  (0) a — amended using absorb as 6

Now, hg evolve -r 4 would have the following result without the above patch:

o  (9) d
|
| @  (8) c
|/
o  (7) b
|
o  (6) a

and with the above patch:

o  (9) d
|
@  (8) c
|
o  (7) b
|
o  (6) a