Drop commits that were made empty by rebase

[martinvonz]

Description

When a commit becomes empty when it was rebased, we should drop it. We should probably not drop commits that were already empty before the rebase. However, probably should drop merge commits that became non-merge commits. I think the condition should be both of:

• New commit is empty and not a merge
• Old commit was not empty, or was a merge

By De Morgan's law, the second bullet is "not (empty and not a merge)", which is the negation of the first bullet. So I guess another way of saying it is that if it's now "empty and linear" and wasn't before, then we drop it.

Specifications

• Platform: All
• Version: 0.4.0

[cflewis]

For a concrete example: If you have to drop to Git for some reason (e.g. require use of a helper for authentication), this happens:

$ git pull --rebase origin main
remote: Total 5 (delta 2), reused 5 (delta 2)
Unpacking objects: 100% (5/5), 1.01 KiB | 1.01 MiB/s, done.
From [redacted]
 * branch                    main       -> FETCH_HEAD
   4a38835a359..050d060a412  main       -> origin/main
warning: skipped previously applied commit b313c062900
hint: use --reapply-cherry-picks to include skipped commits
hint: Disable this message with "git config advice.skippedCherryPicks false"
Successfully rebased and updated detached HEAD.

$ jj log
@  oquzrtumwyyl [email protected] 2023-08-03 19:28:35.000 +00:00 6bf91663c18d
│  (empty) (no description set)
◉  uksqlrnrklny [email protected] 2023-08-03 19:27:25.000 +00:00 main* HEAD@git 050d060a4126
╷  [wrapper] Update README.md to refer to recipe_wrapper
╷ ◉  yoxokorxmxyr [email protected] 2023-08-03 19:26:08.000 +00:00 b313c062900a
╭─╯  [wrapper] Update README.md to refer to recipe_wrapper
◉  [redacted]
~

The only solution here is to abandon the earlier commit which is kind of a bummer of an extra step.

A bigger issue is I have also managed to get myself into serious trouble when I pull a commit down while I am working on a new descendant commit, such that jj ends up making a branch of multiple obsolete commits. When I try abandoning one or the other of those the commit gets automatically rebased on top and goes nuts with conflicts. The only way I could dig myself out was to trash the jj files entirely and re-init. Perhaps abandoning the conflict commit might help, I can't remember if I tried that or not.

I'll follow up here if I manage to do it again.

[martinvonz]

When I try abandoning one or the other of those the commit gets automatically rebased on top and goes nuts with conflicts.

It's probably surprising at first, but you should be able to rebase the commits with conflicts onto the updated main branch and the conflicts should go away (IIUC what your situation was). To avoid this confusing-until-you-get-used-to-it state, you could probably have done jj rebase -s <first commit you want to keep> -d main before you abandoned the remainder.

The only way I could dig myself out was to trash the jj files entirely and re-init.

Another option might have been to restore the repo to an earlier state. Use jj op log to find the operation before you "broke" the repo (maybe the jj abandon call) and then jj op restore <operation id>. But, as I said above, I suspect your repo wasn't actually that broken.

[lazywei]

Curious do we already have something planned for this? I've seen some alias in #1415 (comment) but it doesn't seem to be as reliable as it could be. So wondering if we plan to add this feature or if it would best to do it with scripting / alias?

[martinvonz]

I would like to see this implemented. Do you have some time you could spend on it?

[ilyagr]

I'm a little worried about losing the commit description for the commit being dropped. It might be hard to recover. I'm not sure what the best solution to this is. Some possibilities (some could be used together):

• Just print the commit id (and maybe the description) of the commit being abandoned and hope that's enough.

  This would be easier to accept if we had better tools for seeing or moving changes between desriptions of commits. One idea was to just allow jj describe to take more than on -r option and have it then edit several commits' descriptions in one editor.

  But even then, what if the user closes that terminal before they realize they need this info?

• Have this be governed by an option

• Ask user interactively whether they want to drop each commit.

• Instead of deleting the empty commit, do the equivalent of jj rebase -r commit -d commit-.

[joyously]

I'm a little worried about losing the commit description for the commit being dropped. It might be hard to recover.

I was reading the Darcs page on using commands and saw this line under the record command:

Finally, the --logfile option allows you to supply a file that already contains the patch name and description. This is useful if a previous record failed and left a _darcs/patch_description.txt file.

[arxanas]

I think I've said this somewhere on Discord, but my vote would be for jj rebase to not do automatic commit dropping, and instead reserve that for jj sync or similar. There are a lot of different ways that you might want to detect commits that should be dropped (them becoming empty after a rebase is one of them). Ideally, you could still use jj sync to rebase commits from one place to another without specifically working with an upstream remote, such as if you want to apply some patches to multiple long-lived branches.

[lazywei]

Not too familiar with Rust but if I got sometime I will see if I can look into this. I also agree it might be better if it's in a separate command. In sapling after pull the smartlog will mark the merged commit and the user can decide where to rebase from and the merged commit will automatically be hidden.

But for now, I scrambled some scripting into a cli tool here to deal with the simplest case (git fetch + rebase + drop empty), https://github.com/lazywei/fj/, just in case anyone find it also useful.

[martinvonz]

@lazywei, if you're interested, I think it should be pretty simple to implement a jj git sync command. That would be a good start regardless of what we decide about dropping empty commits.

[martinvonz]

The tracking issue for jj git sync itself is #1039 (thanks for reminding me that that exists, @PhilipMetzger :))

[matts1]

Continuing discussions from #2588, I'm trying to implement this feature. We were discussing the name of the flag, and @martinvonz said the following:

Regarding the exact name, do you think we should even call it --skip-emptied instead of --skip-empty to clarify that already empty commits will not be skipped? Or should we drop already empty commits too (maybe we should continue on #229)? Also, should it be "skip" or "abandon"?

I think the answer to all these questions depends on where jj wants to be on the scale of familiarity to git users vs trying something new, so you can better answer that than me. IMO, abandon-empty / abandon-emptied is a technically more accurate name, but may present some confusion to users migrating from git who are familiar with --skip-empty.

I'm unsure about whether to drop already empty commits. It's pretty trivial to do one way or the other, so I can just implement whatever you decide on.

[martinvonz]

IMO, abandon-empty / abandon-emptied is a technically more accurate name, but may present some confusion to users migrating from git who are familiar with --skip-empty.

I agree. I also considered the risk that users will run jj rebase --skip-empty and be confused when it doesn't skip commits that were already empty (if that's what we decide).

I'm unsure about whether to drop already empty commits. It's pretty trivial to do one way or the other, so I can just implement whatever you decide on.

I think it's probably best to not drop commits that were already empty. Since they didn't change as part of the rebase, it doesn't seem that there's a reason for rebase to drop them. Note that the working copy is often empty. Sometimes the user has still added a description. If they then run jj rebase --skip-empty, it might surprise them if the description on the working copy was lost.

I'm trying to implement this feature

Are you doing that by adding an option to DescendantRebaser? That's probably how I would do it. FYI, that type is in need of some cleanup, and maybe an almost complete rewrite. I'm not asking you to do that, and I don't mean that it's going to be a waste to implement support for dropping empty commits without cleaning it up first. I just mean that it's currently messier than I think it needs to be, in case you're wondering why it's so complicated.

[ilyagr]

I like the idea of --skip-emptied or --abandon-emptied. The former sounds slightly more natural to me, but either is fine.

I still have some concerns about losing the descriptions, since it's quite hard to notice whether you lost something important in there, but it seems tough to check whether the descriptions of the commits being rebased are redundant or not. Losing the descriptions seems mostly OK if it's a command-line option.

I think it's better to drop already-empty commits. Then, we're dropping all the commits that are empty after the rebase, which seems simpler to reason about. Losing the descriptions feels more problematic in this case, but it feels the alternative is that accidentally making a commit not completely empty can caused the rebase to lose information (in the description) that you expected to be preserved.

Update: I didn't see @martinvonz comment above as I was writing this. Martin seems much less concerned about losing descriptions, which is I think the main reason our conclusions differ. Perhaps it's fine not to worry about the descriptions.

[matts1]

If we're dropping already empty commits, then --skip-empty is probably better than --skip-emptied.

[martinvonz]

In #3143, I changed the default behavior to keep commits that were already empty.

Unrelated to that, is there anything left to do on this bug, or can we close this?

[cflewis]

Vote to close.

…

On Sun, Mar 3, 2024 at 9:43 PM Martin von Zweigbergk < ***@***.***> wrote: In #3143 <#3143>, I changed the default behavior to keep commits that were already empty. Unrelated to that, is there anything left to do on this bug, or can we close this? — Reply to this email directly, view it on GitHub <#229 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAKVPEH4FOGIO7DFFKAXWTYWQCXHAVCNFSM5UCWYR32U5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCOJXGU3TOMZXHE4Q> . You are receiving this because you commented.Message ID: ***@***.***>

[colemickens]

It can be closed yes, I did notice this. Thanks!

…

On Mon, Mar 4, 2024, 07:01 Chris Lewis ***@***.***> wrote: Vote to close. On Sun, Mar 3, 2024 at 9:43 PM Martin von Zweigbergk < ***@***.***> wrote: > In #3143 <#3143>, I changed the > default behavior to keep commits that were already empty. > > Unrelated to that, is there anything left to do on this bug, or can we > close this? > > — > Reply to this email directly, view it on GitHub > <#229 (comment)>, or > unsubscribe > < https://github.com/notifications/unsubscribe-auth/AAAKVPEH4FOGIO7DFFKAXWTYWQCXHAVCNFSM5UCWYR32U5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCOJXGU3TOMZXHE4Q> > . > You are receiving this because you commented.Message ID: > ***@***.***> > — Reply to this email directly, view it on GitHub <#229 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACP25BIPLDBWZAAGW3HQITYWSEF7AVCNFSM5UCWYR32U5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCOJXGY3TQMRVGQ3A> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>