Page MenuHomePhabricator

amend: add a useless initial version of `amend -r REV `
Needs RevisionPublic

Authored by martinvonz on Jun 21 2021, 8:13 PM.

Details

Reviewers
Alphare
marmoute
Group Reviewers
hg-reviewers
Summary

Having a way of amending the working copy (or part of it) into an
ancestor commit is one of the most requested features from our users
at work. This patch adds an very early version of such a command. So
far the command just creates a temporary commit and hooks into `hg
abort` to uncommit the temporary commit. Later versions will rebase
the temporary commit onto the target commit, fold it into that commit,
and then rebase the rest of the stack on top.

I called the flag -r/--rev instead of --into because I was
concerned that hg amend --into might be confused with hg amend -i.

Diff Detail

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

Event Timeline

martinvonz created this revision.Jun 21 2021, 8:13 PM

This is indeed a very important quality-of-life feature and I'm +1 on adding it to Mercurial.

I have a few questions/remarks however:

  • Just to make sure we're on the same page, you're suggesting to auto-rebase the stack i.e. what's between the commit to amend and the current working directory parent and nothing more, right? We probably want a non-default mode without the auto-rebase, I can see that being useful.
  • I remember sprint discussions wherein people warned that the semantics of --rev were not clear in the context of amend and suggested using something like --into-rev. I personally think --rev feels natural to me and I don't have strong objections to using it, but we should maybe ask the community (with a poll?) and keep the option open while implementing it.
  • Maybe plug into hg continue/abort and suggest using it first, then hg amend --continue/abort?

As a nit, I'd say the .t test should be using (known-bad-output !) and/or (missing-correct-output !) to better signal what is expected than the TODO.

This is indeed a very important quality-of-life feature and I'm +1 on adding it to Mercurial.

Thanks for the quick review. I sent this out early to get feedback on the idea -- particularly on the fact that we'll have hg amend --continue and hg amend --abort -- before I wasted time going too far in the wrong direction.

I have a few questions/remarks however:

  • Just to make sure we're on the same page, you're suggesting to auto-rebase the stack i.e. what's between the commit to amend and the current working directory parent and nothing more, right? We probably want a non-default mode without the auto-rebase, I can see that being useful.

Yes, exactly, I had intended for it to auto-rebase. Sure, I can add a --no-rebase flag too. When would you want that?

By the way, I suppose we may want a hg amend --stop as well, but I probably won't implement that any time soon.

  • I remember sprint discussions wherein people warned that the semantics of --rev were not clear in the context of amend and suggested using something like --into-rev. I personally think --rev feels natural to me and I don't have strong objections to using it, but we should maybe ask the community (with a poll?) and keep the option open while implementing it.

I initially called it hg amend --into, but then someone (in an unrelated discussion) mentioned hg amend -i yesterday and I thought that users might confuse hg amend --into and hg amend -i. I think most users think of the feature as hg amend -r, so I switched to that.

My reason for initially calling it --into is that the command works on two revisions: the working copy and the target commit. Most hg commands default to working on the working copy and use -r to change that (for example, hg branch and hg topic change the branch name in the working copy by default, but you can change the branch/topic of other commits by passing -r). hg amend defaults to amending from the working copy and into the parent of the working copy. It seems unlikely that we'll add support for amending from an arbitrary commit into an ancestor of that commit. If we ever do that, we can make that hg amend --from abc123 --into abc123^^ and keep -r as a deprecated alias for --into.

Sounds okay?

  • Maybe plug into hg continue/abort and suggest using it first, then hg amend --continue/abort?

As a nit, I'd say the .t test should be using (known-bad-output !) and/or (missing-correct-output !) to better signal what is expected than the TODO.

Ah, good point. Those are new enough that I haven't gotten used to using them yet. Will update the patch.

Thanks for the quick review. I sent this out early to get feedback on the idea -- particularly on the fact that we'll have hg amend --continue and hg amend --abort -- before I wasted time going too far in the wrong direction.

Now that I think of it I'm not entirely certain what the intermediate step for amend will be used for. Will it just be in case of rebase conflicts or is there more to it?

Yes, exactly, I had intended for it to auto-rebase.

I meant to clarify that it would not auto-rebase more than the stack (like other heads that are descendants of the amended changeset).

Sure, I can add a --no-rebase flag too. When would you want that?

This would be for small adjustments that have no real impact on what I'm currently working on, I can just throw the amends one after the other. It's pretty niche, but not hard to implement so I thought I'd ask.

By the way, I suppose we may want a hg amend --stop as well, but I probably won't implement that any time soon.

That would be a good idea, maybe we could discuss why it's hard to implement it (if it's the case), but that shouldn't be a blocker for this patch.

I initially called it hg amend --into, but then someone (in an unrelated discussion) mentioned hg amend -i yesterday and I thought that users might confuse hg amend --into and hg amend -i. I think most users think of the feature as hg amend -r, so I switched to that.

Good point.

My reason for initially calling it --into is that the command works on two revisions: the working copy and the target commit. Most hg commands default to working on the working copy and use -r to change that (for example, hg branch and hg topic change the branch name in the working copy by default, but you can change the branch/topic of other commits by passing -r). hg amend defaults to amending from the working copy and into the parent of the working copy. It seems unlikely that we'll add support for amending from an arbitrary commit into an ancestor of that commit. If we ever do that, we can make that hg amend --from abc123 --into abc123^^ and keep -r as a deprecated alias for --into.
Sounds okay?

This sounds okay to me.

  • Maybe plug into hg continue/abort and suggest using it first, then hg amend --continue/abort?

As a nit, I'd say the .t test should be using (known-bad-output !) and/or (missing-correct-output !) to better signal what is expected than the TODO.

Ah, good point. Those are new enough that I haven't gotten used to using them yet. Will update the patch.

Thanks for updating the patch. I'm going to let a few days go by just to let people give their thoughts as this is a pretty impactful feature, but this looks good to me overall.

Thanks for the quick review. I sent this out early to get feedback on the idea -- particularly on the fact that we'll have hg amend --continue and hg amend --abort -- before I wasted time going too far in the wrong direction.

Now that I think of it I'm not entirely certain what the intermediate step for amend will be used for. Will it just be in case of rebase conflicts or is there more to it?

Steps:

  1. Create temporary commit. Once we support hg amend -r target -i, we'll need to create *two* temporary commits -- one to be rebased and folded in to the target, and one for the leftover changes to later be uncommitted and left in the working copy.
  2. Rebase the temporary commit on top of the target
  3. Fold the temporary commit into the target (using rebase --collapse)
  4. Rebase descendants on top of the new folded commit
  5. Update to the new tip commit
  6. Uncommit the temporary working copy if there was one

Conflicts can happen in step 2 or step 4.

Yes, exactly, I had intended for it to auto-rebase.

I meant to clarify that it would not auto-rebase more than the stack (like other heads that are descendants of the amended changeset).

I see. Yes, that's what I meant. It would rebase the commits between the target and the working copy parent. It would not rebase other descendants of the target. It would also not rebase descendants of the working copy parent.

Sure, I can add a --no-rebase flag too. When would you want that?

This would be for small adjustments that have no real impact on what I'm currently working on, I can just throw the amends one after the other. It's pretty niche, but not hard to implement so I thought I'd ask.

I'm not sure I follow. Let's say you have this history:

@ C
|
o B
|
o A

You then make some change and run hg amend -r B --no-rebase. You get this:

@ C
|
x B
|
| B'
|/
o A

Presumably the working copy is now clean again? However, that means that you can't make changes that build on top of the previous amend.

You then make some further changes in the working copy. What do you do now? If you do hg amend -r B, you would presumably get divergence. Should we allow hg amend -r B'? I had been thinking that we restrict hg amend -r to only amend into an ancestor just to keep it simple and because that seems like what people want in 99% of cases.

By the way, I suppose we may want a hg amend --stop as well, but I probably won't implement that any time soon.

That would be a good idea, maybe we could discuss why it's hard to implement it (if it's the case), but that shouldn't be a blocker for this patch.

It might be easy and I might do it if it seems easy.

  • Maybe plug into hg continue/abort and suggest using it first, then hg amend --continue/abort?

Sorry, didn't see this comment earlier. That's what the patch currently does :) Well, there's no support for continuing, but the test case uses hg abort and there's no --abort flag yet. (I just noticed that the hg status -v still refers to hg amend --continue/--abort. I'll try to figure out how to make it not do that.)

As a nit, I'd say the .t test should be using (known-bad-output !) and/or (missing-correct-output !) to better signal what is expected than the TODO.

Ah, good point. Those are new enough that I haven't gotten used to using them yet. Will update the patch.

Thanks for updating the patch. I'm going to let a few days go by just to let people give their thoughts as this is a pretty impactful feature, but this looks good to me overall.

Makes sense. I'll work on making the command actually do something useful in the meantime :)

This is indeed a very important quality-of-life feature and I'm +1 on adding it to Mercurial.

+1

  • I remember sprint discussions wherein people warned that the semantics of --rev were not clear in the context of amend and suggested using something like --into-rev. I personally think --rev feels natural to me and I don't have strong objections to using it, but we should maybe ask the community (with a poll?) and keep the option open while implementing it.

I initially called it hg amend --into, but then someone (in an unrelated discussion) mentioned hg amend -i yesterday and I thought that users might confuse hg amend --into and hg amend -i. I think most users think of the feature as hg amend -r, so I switched to that.
My reason for initially calling it --into is that the command works on two revisions: the working copy and the target commit. Most hg commands default to working on the working copy and use -r to change that (for example, hg branch and hg topic change the branch name in the working copy by default, but you can change the branch/topic of other commits by passing -r). hg amend defaults to amending from the working copy and into the parent of the working copy. It seems unlikely that we'll add support for amending from an arbitrary commit into an ancestor of that commit. If we ever do that, we can make that hg amend --from abc123 --into abc123^^ and keep -r as a deprecated alias for --into.
Sounds okay?

-r seems reasonable to me. The --from abc --into xyz form sounds more like a non contiguous hg fold to me. I haven't given any thought to if adding that functionality there would make the UI too awkward, given that you can select multiple source revs to fold.

The other general thought I had on this is maybe absorb functionality can be leveraged to avoid the merge conflicts when rebasing, but I don't understand that code at all in order to help. I suppose you'd need to have the "absorb this line into a specific revision" functionality implemented that's been talked about before. But the functionality in general seems more absorb-y than amend-y to me.

This is indeed a very important quality-of-life feature and I'm +1 on adding it to Mercurial.

+1

  • I remember sprint discussions wherein people warned that the semantics of --rev were not clear in the context of amend and suggested using something like --into-rev. I personally think --rev feels natural to me and I don't have strong objections to using it, but we should maybe ask the community (with a poll?) and keep the option open while implementing it.

I initially called it hg amend --into, but then someone (in an unrelated discussion) mentioned hg amend -i yesterday and I thought that users might confuse hg amend --into and hg amend -i. I think most users think of the feature as hg amend -r, so I switched to that.
My reason for initially calling it --into is that the command works on two revisions: the working copy and the target commit. Most hg commands default to working on the working copy and use -r to change that (for example, hg branch and hg topic change the branch name in the working copy by default, but you can change the branch/topic of other commits by passing -r). hg amend defaults to amending from the working copy and into the parent of the working copy. It seems unlikely that we'll add support for amending from an arbitrary commit into an ancestor of that commit. If we ever do that, we can make that hg amend --from abc123 --into abc123^^ and keep -r as a deprecated alias for --into.
Sounds okay?

-r seems reasonable to me. The --from abc --into xyz form sounds more like a non contiguous hg fold to me. I haven't given any thought to if adding that functionality there would make the UI too awkward, given that you can select multiple source revs to fold.

Well, hg amend and hg fold sound the same to me :) To me, the difference seems to be that hg fold is about folding many commits, while hg amend is about only two. Also, adding the feature to hg fold is going to be more work because it's not in core.

The other general thought I had on this is maybe absorb functionality can be leveraged to avoid the merge conflicts when rebasing, but I don't understand that code at all in order to help. I suppose you'd need to have the "absorb this line into a specific revision" functionality implemented that's been talked about before. But the functionality in general seems more absorb-y than amend-y to me.

I suspect it would be hard to convince absorb to do this operation. As you said, the "absorb this line into a specific revision" functionality doesn't exist. I think it will be much easier to use rebase internally for this.

-r seems reasonable to me. The --from abc --into xyz form sounds more like a non contiguous hg fold to me. I haven't given any thought to if adding that functionality there would make the UI too awkward, given that you can select multiple source revs to fold.

Well, hg amend and hg fold sound the same to me :) To me, the difference seems to be that hg fold is about folding many commits, while hg amend is about only two.

Ah, OK. The distinction to me is hg amend works on wdir() + . and hg fold works on 2 or more commits. This is just extending the former's . to ::.. This doesn't propose to handle --from abc --into xyz- all I meant was if it does become a thing some day, it sounds like maybe a different command anyway. I could also see the -r arg here accepting multiple revisions when --interactive is given someday, to allow the user to amend in bits and pieces while rebuilding the stack one commit at a time, and that would be outside your definition of hg amend. I'd still expect that to be the same command as this, but I'm not trying to feature creep it :-)

Also, adding the feature to hg fold is going to be more work because it's not in core.

Hmm, yeah. Are you planning on implementing the --from abc --into xyz case in the near term? I probably just wasn't clear enough that I thought this (currently out of scope) functionality is what might belong elsewhere.

The other general thought I had on this is maybe absorb functionality can be leveraged to avoid the merge conflicts when rebasing, but I don't understand that code at all in order to help. I suppose you'd need to have the "absorb this line into a specific revision" functionality implemented that's been talked about before. But the functionality in general seems more absorb-y than amend-y to me.

I suspect it would be hard to convince absorb to do this operation. As you said, the "absorb this line into a specific revision" functionality doesn't exist. I think it will be much easier to use rebase internally for this.

Yeah, after I hit "send", I wondered if absorbing to an ancestor of a commit that last changed the line being absorbed would cause merge conflicts, and require extra work.

At any rate, please don't take any of this as a NACK or discouragement- I think the functionality is useful in whatever form. I'm just thinking out loud because I'm not sure what all is planned, and trying to get the right UI metaphor in my mind. Since it's marked experimental, if people eventually feel it's more related to absorb from the user POV, we could just dispatch that arg on absorb to the rebase-based implementation you're working on, similar to how the amend command is routing it to a completely different implementation here.

Also, adding the feature to hg fold is going to be more work because it's not in core.

Hmm, yeah. Are you planning on implementing the --from abc --into xyz case in the near term? I probably just wasn't clear enough that I thought this (currently out of scope) functionality is what might belong elsewhere.

Nope, no plan to do that. I just used it to explain the argument that hg amend operates on two "commits" (the working copy and another commit) and it would be consistent with at least hg branch and hg topic for hg amend -r to be about which commit to use instead of the working copy (presumably moving the changes from that commit into its parent).

The other general thought I had on this is maybe absorb functionality can be leveraged to avoid the merge conflicts when rebasing, but I don't understand that code at all in order to help. I suppose you'd need to have the "absorb this line into a specific revision" functionality implemented that's been talked about before. But the functionality in general seems more absorb-y than amend-y to me.

I suspect it would be hard to convince absorb to do this operation. As you said, the "absorb this line into a specific revision" functionality doesn't exist. I think it will be much easier to use rebase internally for this.

Yeah, after I hit "send", I wondered if absorbing to an ancestor of a commit that last changed the line being absorbed would cause merge conflicts, and require extra work.
At any rate, please don't take any of this as a NACK or discouragement- I think the functionality is useful in whatever form. I'm just thinking out loud because I'm not sure what all is planned, and trying to get the right UI metaphor in my mind. Since it's marked experimental, if people eventually feel it's more related to absorb from the user POV, we could just dispatch that arg on absorb to the rebase-based implementation you're working on, similar to how the amend command is routing it to a completely different implementation here.

Sure, no problem :) We have talked internally at work about some kind of interactive hg absorb that lets you decide which commit each line should go into. That sounds similar to what you're thinking of.

We have talked internally at work about some kind of interactive hg absorb that lets you decide which commit each line should go into. That sounds similar to what you're thinking of.

Yes, possibly. There's already a -i mode, but it seems to just be a pre-filter for what gets run through the absorb algorithm. I'm not sure what revision selection in that mode would look like. I found the -e mode that allows selecting the revisions to be a bit daunting the first few times I used it, and it doesn't allow resolving ambiguity for lines that could be applied to multiple revisions.

martinvonz planned changes to this revision.Jun 23 2021, 1:43 PM

I'll update this patch and add several more after it in a day or two. (I should have marked it RFC to start with.)

Sure, I can add a --no-rebase flag too. When would you want that?

This would be for small adjustments that have no real impact on what I'm currently working on, I can just throw the amends one after the other. It's pretty niche, but not hard to implement so I thought I'd ask.

I'm not sure I follow. Let's say you have this history:

@ C
|
o B
|
o A

You then make some change and run hg amend -r B --no-rebase. You get this:

@ C
|
x B
|
| B'
|/
o A

Presumably the working copy is now clean again? However, that means that you can't make changes that build on top of the previous amend.
You then make some further changes in the working copy. What do you do now? If you do hg amend -r B, you would presumably get divergence. Should we allow hg amend -r B'? I had been thinking that we restrict hg amend -r to only amend into an ancestor just to keep it simple and because that seems like what people want in 99% of cases.

There is a bunch of small usecase for --no-rebase (that should probably be named --no-evolve

In simple case, the user knows he will amend various bits of the current working copy in different commit of the stack and prefer to run the evolution only once (to reduce conflict resolution and/or to reduce markers creation).

In more advanced case, this might be actually needed to avoid a loop of complicated and unnecessary conflict resolution. Lets imagine the following scenario:

@ C
|
o B
|
o A
  • There are change to amend in A, but they will need adjustement to actually work in A and they will conflict with some content in B (then C)
  • There are change to amend in B, but they will conflict with the change in A and with some change in C

Without --no-evolve, this would mean the following operation

  • hg amend -i --rev A → this requires conflict resolution when evolving B, then C; conflict is resolved with non final content
  • hg amend -i --rev B → this requires conflict resolution when evolving C; conflict is resolved with non final content
  • hg update A; hack hack hack; hg amend; hg next this requires conflict resolution when evolving B,
  • hack hack hack; hg amend; hg next this requires conflict resolution when evolving C,

With --no-evolve, This can simply be:

  • hg amend -i --rev A --no-evolve → no conflict resolution yet
  • hg amend -i --rev B --no-evolve → no conflict resolution yet
  • hg update A; hack hack hack; hg amend; hg next this requires conflict resolution when evolving B; with final content
  • hack hack hack; hg amend; hg next this requires conflict resolution when evolving C,

So the --no-evolve reduced the number of conflict and all the conflict we had to resolve were "meaningful", using "final content"

Sure, I can add a --no-rebase flag too. When would you want that?

This would be for small adjustments that have no real impact on what I'm currently working on, I can just throw the amends one after the other. It's pretty niche, but not hard to implement so I thought I'd ask.

I'm not sure I follow. Let's say you have this history:

@ C
|
o B
|
o A

You then make some change and run hg amend -r B --no-rebase. You get this:

@ C
|
x B
|
| B'
|/
o A

Presumably the working copy is now clean again? However, that means that you can't make changes that build on top of the previous amend.
You then make some further changes in the working copy. What do you do now? If you do hg amend -r B, you would presumably get divergence. Should we allow hg amend -r B'? I had been thinking that we restrict hg amend -r to only amend into an ancestor just to keep it simple and because that seems like what people want in 99% of cases.

There is a bunch of small usecase for --no-rebase (that should probably be named --no-evolve
In simple case, the user knows he will amend various bits of the current working copy in different commit of the stack and prefer to run the evolution only once (to reduce conflict resolution and/or to reduce markers creation).
In more advanced case, this might be actually needed to avoid a loop of complicated and unnecessary conflict resolution. Lets imagine the following scenario:

@ C
|
o B
|
o A
  • There are change to amend in A, but they will need adjustement to actually work in A and they will conflict with some content in B (then C)
  • There are change to amend in B, but they will conflict with the change in A and with some change in C

Without --no-evolve, this would mean the following operation

  • hg amend -i --rev A → this requires conflict resolution when evolving B, then C; conflict is resolved with non final content
  • hg amend -i --rev B → this requires conflict resolution when evolving C; conflict is resolved with non final content
  • hg update A; hack hack hack; hg amend; hg next this requires conflict resolution when evolving B,
  • hack hack hack; hg amend; hg next this requires conflict resolution when evolving C,

With --no-evolve, This can simply be:

  • hg amend -i --rev A --no-evolve → no conflict resolution yet

I imagine that the selected changes are removed from the working copy after this step. Is that what you were thinking as well? Or were you thinking that they would remain in the working copy as well (more like hg rebase --keep if you think of the working copy contents as a commit)?

  • hg amend -i --rev B --no-evolve → no conflict resolution yet
  • hg update A; hack hack hack; hg amend; hg next this requires conflict resolution when evolving B; with final content
  • hack hack hack; hg amend; hg next this requires conflict resolution when evolving C,

So the --no-evolve reduced the number of conflict and all the conflict we had to resolve were "meaningful", using "final content"

Makes sense. On the other hand, if the changes you want to amend into A require you to edit the patch (pressing "e" in the curses UI), then it seems like you'd get additional conflicts by using --no-rebase/--no-evolve. So both --rebase and --no-rebase seem to be useful for avoiding conflicts in some cases.

marmoute added a comment.EditedJun 28 2021, 12:52 PM

With --no-evolve, This can simply be:

  • hg amend -i --rev A --no-evolve → no conflict resolution yet

I imagine that the selected changes are removed from the working copy after this step. Is that what you were thinking as well? Or were you thinking that they would remain in the working copy as well (more like hg rebase --keep if you think of the working copy contents as a commit)?

That is a very good question, that I have not been able to have a clear opinion on so far. (I did not though about it too much either).

I can see argument for both, "if it is integrated somewhere else, it should no longer be in the wdir() diff" and "This is asking for trouble, we better leave it around".

In the usecase were a small type fix is cast-away in an ancestor and the change is not really relevant to the WC, dropping it is "cleaner" and won't introduce conflict.
On the other hand If the usecase is to send something more conflictful and/or more relevant to the WC, keeping it around might make sense ?

I suggest we start with the path of least resistance (probably keeping it in the WC then) and add a # XXX TODO comment near the experimental config so that we don't forget about that before getting the feature out of experiment.

  • hg amend -i --rev B --no-evolve → no conflict resolution yet
  • hg update A; hack hack hack; hg amend; hg next this requires conflict resolution when evolving B; with final content
  • hack hack hack; hg amend; hg next this requires conflict resolution when evolving C,

So the --no-evolve reduced the number of conflict and all the conflict we had to resolve were "meaningful", using "final content"

Makes sense. On the other hand, if the changes you want to amend into A require you to edit the patch (pressing "e" in the curses UI), then it seems like you'd get additional conflicts by using --no-rebase/--no-evolve. So both --rebase and --no-rebase seem to be useful for avoiding conflicts in some cases.

note: In the case I describe above, this would not be simply a "on the fly patch edition" (pressing "e" in the curses UI) since this would probably involves running tests and writing possibly other codes to make it work fine.

I don't think I understand how the "e" editing can introduce more conflict. Can you elaborate ?


To some extend, my opinion on --no-evolve is that there will certainly be case where it make sense to do less work, and we should have the option around to make sure we can handle these case (and get a better understanding of what make sens in these cases)

With --no-evolve, This can simply be:

  • hg amend -i --rev A --no-evolve → no conflict resolution yet

I imagine that the selected changes are removed from the working copy after this step. Is that what you were thinking as well? Or were you thinking that they would remain in the working copy as well (more like hg rebase --keep if you think of the working copy contents as a commit)?

That is a very good question, that I have not been able to have a clear opinion on so far. (I did not though about it too much either).
I can see argument for both, "if it is integrated somewhere else, it should no longer be in the wdir() diff" and "This is asking for trouble, we better leave it around".
In the usecase were a small type fix is cast-away in an ancestor and the change is not really relevant to the WC, dropping it is "cleaner" and won't introduce conflict.
On the other hand If the usecase is to send something more conflictful and/or more relevant to the WC, keeping it around might make sense ?
I suggest we start with the path of least resistance (probably keeping it in the WC then) and add a # XXX TODO comment near the experimental config so that we don't forget about that before getting the feature out of experiment.

I'll start with not even having the flag :P When automatically rebasing the descendants, it seems more clear that the changes should not be left in the working copy. I was planning on explicitly rebasing them and not leaving them in the working copy, not relying on them disappearing only because the changes were already present in an ancestor afterwards. To clarify, I was planning for it to do this:

  1. Create temporary commit from working copy
  2. Rebase temporary commit to target (not using --keep)
  3. Fold the rebased commit into the target
  4. Rebase descendants, not including the temporary commit from step 1

One could imagine using --keep in step two and include the temporary commit in step 4 and just rely on it becoming empty. However, if there were conflicts to resolve in step 2, then you'd run into conflicts again in step 4. It seems better to avoid that.

  • hg amend -i --rev B --no-evolve → no conflict resolution yet
  • hg update A; hack hack hack; hg amend; hg next this requires conflict resolution when evolving B; with final content
  • hack hack hack; hg amend; hg next this requires conflict resolution when evolving C,

So the --no-evolve reduced the number of conflict and all the conflict we had to resolve were "meaningful", using "final content"

Makes sense. On the other hand, if the changes you want to amend into A require you to edit the patch (pressing "e" in the curses UI), then it seems like you'd get additional conflicts by using --no-rebase/--no-evolve. So both --rebase and --no-rebase seem to be useful for avoiding conflicts in some cases.

note: In the case I describe above, this would not be simply a "on the fly patch edition" (pressing "e" in the curses UI) since this would probably involves running tests and writing possibly other codes to make it work fine.
I don't think I understand how the "e" editing can introduce more conflict. Can you elaborate ?

It can introduce more conflicts under the assumption above (that at least I had) about hg amend -r not leaving the changes in the working copy as well. Let's say you have some file that contains content "a" in commit A. The file is unchanged between A and the working copy parent. Your working copy now contains "c" in this file. You realize that the contents in commit A should have been "b", so you run hg amend -r A -i and edit the diff from "change 'a' to 'c'" to "change 'a' to 'b'". When the user then confirms their selection, I was planning to create a commit for "change 'a' to 'b'" and another commit on top for "change 'b' to 'c'" (just like how hg ci -i && hg ci would create two commits). I was planning for the first change to go through steps 2 and 3 and 4 from above. However, the "change 'b' to 'c'" bit would also be rebased in step 4 and would apply cleanly. If we leave the changes in the working copy as well, then they'd conflict when they were rebased onto the folded commit (whether that rebasing was done by hg amend -r A -i --rebase or by hg amend -r A -i; hg evolve).

This series is now ready for review again

Alphare accepted this revision as: Alphare.Jul 12 2021, 4:46 AM

This touches areas of the code I'm less familiar with, but the logic and output both look correct to me.

martinvonz planned changes to this revision.Jul 12 2021, 12:47 PM

I'll add tests for amending into siblings and descendants too, along with code changes necessary to make that work.

martinvonz updated this revision to Diff 29210.Jul 12 2021, 2:43 PM

This series is now ready for review again

I'll try to have a look soon (realistically Thursday, in all cases this week).

marmoute added a comment.EditedJul 15 2021, 7:13 PM

I'll start with not even having the flag :P When automatically rebasing the descendants, it seems more clear that the changes should not be left in the working copy. I was planning on explicitly rebasing them and not leaving them in the working copy, not relying on them disappearing only because the changes were already present in an ancestor afterwards. To clarify, I was planning for it to do this:

  1. Create temporary commit from working copy
  2. Rebase temporary commit to target (not using --keep)
  3. Fold the rebased commit into the target
  4. Rebase descendants, not including the temporary commit from step 1

One could imagine using --keep in step two and include the temporary commit in step 4 and just rely on it becoming empty. However, if there were conflicts to resolve in step 2, then you'd run into conflicts again in step 4. It seems better to avoid that.

Looks like we could have a --no-evolve flag that stop after step 3 and be good. This should fit well with what seems to be the current split of your patch. Also I am not sure why you need a temporary commit here. We should be able to have:

  1. update to target,
  2. amend the target,
  3. evolve affected range (unless --no-evolve) is specified,

(note: further discussion got me to have a more idea about what could be done for --no-evolve, but this the above proposal is a good example a "minimal/simple" approach that still make sense, so I am keeping the comment)

  • hg amend -i --rev B --no-evolve → no conflict resolution yet
  • hg update A; hack hack hack; hg amend; hg next this requires conflict resolution when evolving B; with final content
  • hack hack hack; hg amend; hg next this requires conflict resolution when evolving C,

So the --no-evolve reduced the number of conflict and all the conflict we had to resolve were "meaningful", using "final content"

Makes sense. On the other hand, if the changes you want to amend into A require you to edit the patch (pressing "e" in the curses UI), then it seems like you'd get additional conflicts by using --no-rebase/--no-evolve. So both --rebase and --no-rebase seem to be useful for avoiding conflicts in some cases.

note: In the case I describe above, this would not be simply a "on the fly patch edition" (pressing "e" in the curses UI) since this would probably involves running tests and writing possibly other codes to make it work fine.
I don't think I understand how the "e" editing can introduce more conflict. Can you elaborate ?

It can introduce more conflicts under the assumption above (that at least I had) about hg amend -r not leaving the changes in the working copy as well. Let's say you have some file that contains content "a" in commit A. The file is unchanged between A and the working copy parent. Your working copy now contains "c" in this file. You realize that the contents in commit A should have been "b", so you run hg amend -r A -i and edit the diff from "change 'a' to 'c'" to "change 'a' to 'b'".

That's a pretty advance usecases but that shed some light how what the hg amend core behavior is: alter content of underlying changeset, do not alter¹ the working copy. This apply to both hg amend and hg amend --extract (also known as uncommit). So whatever we goes for should probably leave the working copy untouched.

([1] note: this end up being untrue for hg amend --patch, but that is a quite special case, and we should probably get a hg amend --patch --rev 'wc()' working too.)

When the user then confirms their selection, I was planning to create a commit for "change 'a' to 'b'" and another commit on top for "change 'b' to 'c'" (just like how hg ci -i && hg ci would create two commits).

I am not why we are creating explicit commit here, especially since this seems to introduce more complication than a "preserve the working copy content" approach.

> I was planning for the first change to go through steps 2 and 3 and 4 from above. However, the "change 'b' to 'c'" bit would also be rebased in step 4 and would apply cleanly. If we leave the changes in the working copy as well, then they'd conflict when they were rebased onto the folded commit (whether that rebasing was done by hg amend -r A -i --rebase or by hg amend -r A -i; hg evolve).

Thanks, I now see where the conflict comes from. However I don't see what would be the alternative to "leave the changes in the working copy" here ? Since current working copy content (mostly the final target) is different from what we amended into A.

Can you clarify what are the different alternative you have in mind here ?

marmoute requested changes to this revision.Jul 15 2021, 7:38 PM
marmoute added inline comments.
hgext/amend.py
65–66

It would be nice to have a --stop flag here to interrupt the associated evolution if it gets t too complicated.
Is there a strong reason not to have it with the other from the start ?

163

What happens if --rev target is not an ancestors of . ? Is this allowed, if so, what is the expected behavior ?

In such case, the %d::. will be emptyis ther any bad consequence ?

173–177

relying on another extension behavior seems a bit fragile and hacky. It seems simple enough to keep track of the source revision to actually do so ourself, or am I missing something ?

178

Don't we need to backup the target_node sooner ? Otherwise it will be lost in an InterventionRequired case, will it not?

(side question: do we have a test for this ?)

This revision now requires changes to proceed.Jul 15 2021, 7:38 PM

I'll start with not even having the flag :P When automatically rebasing the descendants, it seems more clear that the changes should not be left in the working copy. I was planning on explicitly rebasing them and not leaving them in the working copy, not relying on them disappearing only because the changes were already present in an ancestor afterwards. To clarify, I was planning for it to do this:

  1. Create temporary commit from working copy
  2. Rebase temporary commit to target (not using --keep)
  3. Fold the rebased commit into the target
  4. Rebase descendants, not including the temporary commit from step 1

One could imagine using --keep in step two and include the temporary commit in step 4 and just rely on it becoming empty. However, if there were conflicts to resolve in step 2, then you'd run into conflicts again in step 4. It seems better to avoid that.

Looks like we could have a --no-evolve flag that stop after step 3 and be good.

Sure, but that can also be added after the series. I agree about adding such a flag (I'm not sure if I think it should be --no-evolve or --no-rebase), but I don't think it's important to get it added in this initial series.

This should fit well with what seems to be the current split of your patch. Also I am not sure why you need a temporary commit here. We should be able to have:

  1. update to target,

The problem is that this step may result in conflicts, and we don't think we have a good way of dealing with that. It seems much easier to create the temporary commit. When we add support for hg amend -i -r, we will create two temporary commits.

  1. amend the target,
  2. evolve affected range (unless --no-evolve) is specified,

(note: further discussion got me to have a more idea about what could be done for --no-evolve, but this the above proposal is a good example a "minimal/simple" approach that still make sense, so I am keeping the comment)

the hg amend core behavior is: alter content of underlying changeset, do not alter¹ the working copy.

(It looks like you forgot to include footnote [1] that you point to here.)

Yes, that's true for plain hg amend. Note that it's not necessarily true (or at least it shouldn't necessarily be true in my opinion) for hg amend -r. For example, let's say you have this history:

@ commit 3, file content: A
|
o commit 2, file content: B
|
o commit 1, file content: A

You now realize that you want the contents in commit 1 to be "B", so you make that change in the working copy and run hg amend -r 1. I think users wouldn't expect "B" to remain in the working copy afterwards.

This apply to bot hg amend and hg amend --extract (also known as uncommit). So whatever we goes for should probably leave the working copy untouched.
(note: this end up being untrue for hg amend --patch, but that is a quite special case, and we should probably get a hg amend --patch --rev 'wc()' working too.)

(I have never used hg amend --patch. Context for others: hg amend --patch and hg amend --extract exist in the evolve extension's version of the command, but do not exist in core's version.)

When the user then confirms their selection, I was planning to create a commit for "change 'a' to 'b'" and another commit on top for "change 'b' to 'c'" (just like how hg ci -i && hg ci would create two commits).

I am not why we are creating explicit commit here, especially since this seems to introduce more complication than a "preserve the working copy content" approach.

I don't think I follow what the "preserve the working copy content" approach is. Can you explain?

> I was planning for the first change to go through steps 2 and 3 and 4 from above. However, the "change 'b' to 'c'" bit would also be rebased in step 4 and would apply cleanly. If we leave the changes in the working copy as well, then they'd conflict when they were rebased onto the folded commit (whether that rebasing was done by hg amend -r A -i --rebase or by hg amend -r A -i; hg evolve).
Thanks, I now see where the conflict comes from. However I don't see what would be the alternative to "leave the changes in the working copy" here ? Since current working copy content (mostly the final target) is different from what we amended into A.
Can you clarify what are the different alternative you have in mind here ?

My proposal is the 4 steps I described. Sorry, I think I don't understand what you're asking me to clarify.

martinvonz marked 3 inline comments as done.Jul 15 2021, 8:27 PM
martinvonz added inline comments.
hgext/amend.py
65–66

It would be nice to have a --stop flag here to interrupt the associated evolution if it gets t too complicated.
Is there a strong reason not to have it with the other from the start ?

Only that it takes more time :P

163

There's actually a test case for that :) It is allowed. This revset is indeed (intentionally) empty in that case, so nothing gets rebased on top.

173–177

I initially did that, but it just seemed unnecessary to replicate rebase's behavior around empty commits etc.

178

I don't think we can get an InterventionRequired yet, so I should technically move that and most of the new state.save_on_conflicts() to the next commit. I would then presumably also move all state management to the next patch. Do you feel strongly that I should or are you happy just knowing why it's done this way?

Alphare requested changes to this revision.Jul 16 2021, 4:11 PM

Sorry I forgot to post this before the week-end.

@martinvonz broader picture question: what is your expected timeline for this getting into core? I'm seeing new questions coming up and it's looking like at least the UX for this new feature needs to be discussed some more.

I wanted to get this feature in before the freeze, but now that I see that it would be rushed at best, I'm hoping we can move this discussion to early August after the freeze to actually give the underlying questions a real shot. We've decided the same for the new dirstate-v2 format internally at Octobus for example.

martinvonz marked 3 inline comments as done.Jul 16 2021, 4:14 PM

Sorry I forgot to post this before the week-end.
@martinvonz broader picture question: what is your expected timeline for this getting into core? I'm seeing new questions coming up and it's looking like at least the UX for this new feature needs to be discussed some more.
I wanted to get this feature in before the freeze, but now that I see that it would be rushed at best, I'm hoping we can move this discussion to early August after the freeze to actually give the underlying questions a real shot. We've decided the same for the new dirstate-v2 format internally at Octobus for example.

Sure, let's wait until after the freeze.

I'll start with not even having the flag :P When automatically rebasing the descendants, it seems more clear that the changes should not be left in the working copy. I was planning on explicitly rebasing them and not leaving them in the working copy, not relying on them disappearing only because the changes were already present in an ancestor afterwards. To clarify, I was planning for it to do this:

  1. Create temporary commit from working copy
  2. Rebase temporary commit to target (not using --keep)
  3. Fold the rebased commit into the target
  4. Rebase descendants, not including the temporary commit from step 1

One could imagine using --keep in step two and include the temporary commit in step 4 and just rely on it becoming empty. However, if there were conflicts to resolve in step 2, then you'd run into conflicts again in step 4. It seems better to avoid that.

Looks like we could have a --no-evolve flag that stop after step 3 and be good.

Sure, but that can also be added after the series. I agree about adding such a flag (I'm not sure if I think it should be --no-evolve or --no-rebase), but I don't think it's important to get it added in this initial series.

It is important to consider it early in the series because it help having a deeper look at what the command/action tries to do and achieve and validate this is going in the right direction. History rewriting is a complex concept and being able to express what we do with a limited set of consistent action is important to make sure we build something users will have confidence into and steer people away from visible and invisible danger. One of the big invisible danger is the risk of creating workflow that seamlessly leads to N² markers creations which this "new" command could lead to. So think about the step by step here early is important as we need to consider if (1) it could exists (2) it could make sense (3) it might be sensible as a default.

Thanks for starting this discussion "early" (before sending more code) so that we could start discussing this early. These discussion around the corner case are very insightful about the use-cases and what might/should be done to solve them.

This last round of discussion make me think that this might not be the right UX to achieve our goal here. However I won't have time to elaborate much more because (1) I need some quiet time to think about it more (2) writing the ideas down will probably cost me multiple hours, hours that I currently do not have as the Windows/py3 testing and issue6528/6538 have been time consuming. I'll come back to you about that.

This should fit well with what seems to be the current split of your patch. Also I am not sure why you need a temporary commit here. We should be able to have:

  1. update to target,

The problem is that this step may result in conflicts, and we don't think we have a good way of dealing with that. It seems much easier to create the temporary commit. When we add support for hg amend -i -r, we will create two temporary commits.

We have had all the necessary information to abort a hg update for years and a GSOC student working on that topic, I though we had hg update --abort for a couple of years already, but it doesn't seems like it. sight

  1. amend the target,
  2. evolve affected range (unless --no-evolve) is specified,

(note: further discussion got me to have a more idea about what could be done for --no-evolve, but this the above proposal is a good example a "minimal/simple" approach that still make sense, so I am keeping the comment)
the hg amend core behavior is: alter content of underlying changeset, do not alter¹ the working copy.

(It looks like you forgot to include footnote [1] that you point to here.)
Yes, that's true for plain hg amend.

I think this is an important property of hg amend and we should probably not break that expectation from users.

Note that it's not necessarily true (or at least it shouldn't necessarily be true in my opinion) for hg amend -r. For example, let's say you have this history:

@ commit 3, file content: A
|
o commit 2, file content: B
|
o commit 1, file content: A

You now realize that you want the contents in commit 1 to be "B", so you make that change in the working copy and run hg amend -r 1. I think users wouldn't expect "B" to remain in the working copy afterwards.

That does not seems like the main usecase of hg amend --rev (or anything that would fit the use case, I have been thinking we discuss). In my mind the current usecase is "I have a bunch of changes in my working copy and I want to amend them in existing commit. However, I don't want them amended in my direct parent, but in other ancestors. That is a usecase similar to hg absorb but with manual selection instead of having it automatically selected. For different usecase I feel like other command/UI are probably better suited.

For the case you describe using update/amend, histedit or anything else seems more suitable

This apply to bot hg amend and hg amend --extract (also known as uncommit). So whatever we goes for should probably leave the working copy untouched.
(note: this end up being untrue for hg amend --patch, but that is a quite special case, and we should probably get a hg amend --patch --rev 'wc()' working too.)

(I have never used hg amend --patch. Context for others: hg amend --patch and hg amend --extract exist in the evolve extension's version of the command, but do not exist in core's version.)

Ah, I did not realized at first that this series was about touching the hostile-fork of amend. This probably explain some of the confusion around "why don't you use rewriteutil" later since evolve's rewriteutil¹ is not around.

For the record since the two code bases has already diverged, I would prefer to see effort to converge them again than further diverging modification to the forked code. The evolve extensions is also still my preferred place for history rewriting UX experiment as it allow broader testing, with a shorter feedback cycle.
That said, I will keep doing my best to help reviewing this MR

[1] most of it is flagged as ready to upstream if someone is motivated.

When the user then confirms their selection, I was planning to create a commit for "change 'a' to 'b'" and another commit on top for "change 'b' to 'c'" (just like how hg ci -i && hg ci would create two commits).

I am not why we are creating explicit commit here, especially since this seems to introduce more complication than a "preserve the working copy content" approach.

I don't think I follow what the "preserve the working copy content" approach is. Can you explain?

This is what we are discussing above. hg amend is a command that leave the working copy content untouched, and I think it is important is stay so.

> I was planning for the first change to go through steps 2 and 3 and 4 from above. However, the "change 'b' to 'c'" bit would also be rebased in step 4 and would apply cleanly. If we leave the changes in the working copy as well, then they'd conflict when they were rebased onto the folded commit (whether that rebasing was done by hg amend -r A -i --rebase or by hg amend -r A -i; hg evolve).
Thanks, I now see where the conflict comes from. However I don't see what would be the alternative to "leave the changes in the working copy" here ? Since current working copy content (mostly the final target) is different from what we amended into A.
Can you clarify what are the different alternative you have in mind here ?

My proposal is the 4 steps I described. Sorry, I think I don't understand what you're asking me to clarify.

I think the "would also be rebased in step 4" make me think you had an alternative in mind. However if I understand your answer right, you did not had any alternative in mind.

hgext/amend.py
163

But in this case, the working copy content get altered and the change dissapear from under the user feet, right?

173–177

Factoring things out seems good, but fully going through another extension introduce awkwardness across the series and I feel like it stacks up significant tech-debt.

I would be much happier if some common utility were used.

178

My point is mostly that we should initialize state[b'target_node'] = target_ctx.node() before starting any action, to make sure it is preserved.

Thanks for starting this discussion "early" (before sending more code) so that we could start discussing this early. These discussion around the corner case are very insightful about the use-cases and what might/should be done to solve them.
This last round of discussion make me think that this might not be the right UX to achieve our goal here. However I won't have time to elaborate much more because (1) I need some quiet time to think about it more (2) writing the ideas down will probably cost me multiple hours, hours that I currently do not have as the Windows/py3 testing and issue6528/6538 have been time consuming. I'll come back to you about that.

Okay, I'll work on other things until then. Just let me know when.

This last round of discussion make me think that this might not be the right UX to achieve our goal here. However I won't have time to elaborate much more because (1) I need some quiet time to think about it more (2) writing the ideas down will probably cost me multiple hours, hours that I currently do not have as the Windows/py3 testing and issue6528/6538 have been time consuming. I'll come back to you about that.

Actually, do you mind sharing at least sentence or so about what you think might be the right UX, even if it's not completely clear to you yet? I just want to get a sense of what you have in mind.

pulkit added a subscriber: pulkit.Jul 20 2021, 4:31 PM

I am also big +1 on adding the feature. I think we can split this into multiple smaller features to split out discussion and ease review.

  1. A patch that introduces amend --rev but aborts if there are conflicts.
  2. Abort, continue and stop functionality and allowing amend --rev to work even if there are conflicts
  3. Auto rebase children functionality

@martinvonz what do you think?

mercurial/state.py
84

This can be a part of separate individual patch.

This last round of discussion make me think that this might not be the right UX to achieve our goal here. However I won't have time to elaborate much more because (1) I need some quiet time to think about it more (2) writing the ideas down will probably cost me multiple hours, hours that I currently do not have as the Windows/py3 testing and issue6528/6538 have been time consuming. I'll come back to you about that.

Actually, do you mind sharing at least sentence or so about what you think might be the right UX, even if it's not completely clear to you yet? I just want to get a sense of what you have in mind.

Ping on this.

This last round of discussion make me think that this might not be the right UX to achieve our goal here. However I won't have time to elaborate much more because (1) I need some quiet time to think about it more (2) writing the ideas down will probably cost me multiple hours, hours that I currently do not have as the Windows/py3 testing and issue6528/6538 have been time consuming. I'll come back to you about that.

Actually, do you mind sharing at least sentence or so about what you think might be the right UX, even if it's not completely clear to you yet? I just want to get a sense of what you have in mind.

Ping on this.

I don't have a simple sentence (nor a simple idea) about this ready to use. I did not have a single second to even think about this discussion as I have been busy with 5.9-freeze related stuff for the past 3 weeks. Since 5.9 is unfortunately not out of the woods yet, do not hold you breath on this.

This last round of discussion make me think that this might not be the right UX to achieve our goal here. However I won't have time to elaborate much more because (1) I need some quiet time to think about it more (2) writing the ideas down will probably cost me multiple hours, hours that I currently do not have as the Windows/py3 testing and issue6528/6538 have been time consuming. I'll come back to you about that.

Actually, do you mind sharing at least sentence or so about what you think might be the right UX, even if it's not completely clear to you yet? I just want to get a sense of what you have in mind.

Ping on this.

I don't have a simple sentence (nor a simple idea) about this ready to use. I did not have a single second to even think about this discussion as I have been busy with 5.9-freeze related stuff for the past 3 weeks. Since 5.9 is unfortunately not out of the woods yet, do not hold you breath on this.

It seems that the freeze is now over, so I just wanted to make sure that this is not forgotten. I don't mean to be pushy, I just thought it's better to ping this now than to wait a month only to find out that it was simply forgotten.

marmoute added a comment.EditedFri, Sep 3, 7:48 PM

This last round of discussion make me think that this might not be the right UX to achieve our goal here. However I won't have time to elaborate much more because (1) I need some quiet time to think about it more (2) writing the ideas down will probably cost me multiple hours, hours that I currently do not have as the Windows/py3 testing and issue6528/6538 have been time consuming. I'll come back to you about that.

Actually, do you mind sharing at least sentence or so about what you think might be the right UX, even if it's not completely clear to you yet? I just want to get a sense of what you have in mind.

Ping on this.

I don't have a simple sentence (nor a simple idea) about this ready to use. I did not have a single second to even think about this discussion as I have been busy with 5.9-freeze related stuff for the past 3 weeks. Since 5.9 is unfortunately not out of the woods yet, do not hold you breath on this.

It seems that the freeze is now over, so I just wanted to make sure that this is not forgotten. I don't mean to be pushy, I just thought it's better to ping this now than to wait a month only to find out that it was simply forgotten.

I have not forgotten and I meant to get to that last week. Unfortunately some IRL situation came up and I have not be able to spend time on this (and even writing this message took a week longer that I wanted). Sorry for the delay. The second half of ext week should be more "normal" and I hope to get to that at that time.