Rewrite instead of abandoning empty commits.

[matts1]

Fixes #2760

Given the tree:

A-B-C
 \
  B2

And the command jj rebase -s B -d B2

We were previously marking B as abandoned, despite the comment stating that we were marking it as being succeeded by B2. This resulted in a call to rewrite(rewrites={}, abandoned={B}) instead of rewrite(rewrites={B=>B2}, abandoned={}), which then made the new parent of C into A instead of B2

[ilyagr]

Ah, so it seems the bug wasn't quite what I thought.

I think that what you wrote, both in the code and the description, sounds reasonable to me. Could you write a test for this, ideally showing both what happened before and what happens now (in the same commit as the fix)? (Make sure to pull from main before writing the test, since I just merged #2738 which slightly changes how tests for rewrite operations are written).

Optionally, if you are willing to write some tests showing the working copy behavior, that would be nice too. I think there's a good chance this PR will just fix all of #2760 without having to do anything special for the working copy, but I'm not entirely sure.

[ilyagr]

In fact, I'm a bit worried that, with this change, in the common case when you are editing an empty commit, jj rebase --skip-empty -d somewhere will cause you to unexpectedly start editing the non-empty parent commit

I'm not sure if this is the best solution, but we could compare the change id of the working copy before and after the rebase. If they are different, you could do an analogue of jj new -m old_working_copy_description.

[matts1]

My preferred solution would be to just set skip-empty to set AbandonNewlyEmpty instead of AbandonAllEmpty, or to add a flag to do something similar. IMO, that's a discussion for another CL though, since this is just a bugfix.

[matts1]

The failing checks appear to be completely unrelated, and they pass locally. I'm guessing it's a flake, which seems especially likely given that the test is for concurrency.

[ilyagr]

My preferred solution would be to just set skip-empty to set AbandonNewlyEmpty instead of AbandonAllEmpty

This wouldn't fix the problem, only make it more rare. The working copy commit can be emptied by a rebase even if it wasn't empty before.

I feel like the behavior of accidentally editing a non-empty commit might be even worse than the original bug. So, I'd prefer doing something about it now. Perhaps if we switch AbandonNewlyEmpty, it will be rare enough to not worry too much about it, but then we might forget to fix it entirely.

[ilyagr]

Thank you very much for all the updates and for responding to the bug so quickly!

I have a bunch of minor nits. If you don't have the time or the energy, I can implement them myself, so they are optional. Also, I wrote them in a hurry, so my edits may need further editing.

Overall, this looks great to me. For the record, I'm not sure that my suggestion to never abandon working copy commits is the best long-term option; we can change it if we think of something better and/or people find this behavior confusing. However, I think it's a good patch for the bug for now.

[ilyagr]

Looks good, thanks again!