arxanas / git-branchless

High-velocity, monorepo-scale workflow for Git
Apache License 2.0
3.36k stars 80 forks source link

git branchless crashes on empty commits #1205

Open samueltardieu opened 5 months ago

samueltardieu commented 5 months ago

Description of the bug

git branchless refuses to rewrite an empty commit and crashes:

$ git init; git branchless init --main-branch main
$ echo foo > foo
$ git add foo
$ git commit -am "First commit"
$ git commit --allow-empty -m "Second commit with no changes"
$ git reword HEAD^ -f -m "Fix message of first commit"  
The application panicked (crashed).
Message:  assertion failed: !dest_oids.is_empty()
Location: git-branchless-lib/src/core/rewrite/plan.rs:1023

  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ SPANTRACE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

   0: git_branchless_reword::reword with effects=<Output fancy=true> revsets=[Revset("HEAD^")] resolve_revset_options=ResolveRevsetOptions { show_hidden_commits: false } messages=Messages(["Fix message of first commit"]) git_run_info=<GitRunInfo path_to_git="git" working_directory="/tmp/z" env=not shown> force_rewrite_public_commits=true
      at git-branchless-reword/src/lib.rs:62

Backtrace omitted. Run with RUST_BACKTRACE=1 environment variable to display it.
Run with RUST_BACKTRACE=full to include source snippets.

Expected behavior

No error should happen. git branchless should acknowledge the existence of empty commits, or at least accept to rewrite empty commits as empty commits when restacking.

This is a behaviour I encountered when working with patch series created by b4: b4 creates an empty commit at the beginning of the patch series to track the cover letter and the changelog of the patch series.

While b4 + git-branchless could be the perfect combination for working with email-based git workflows, the systematic rejection of empty commits by git-branchless (even when an empty commit is being rewritten) makes it difficult to use.

Actual behavior

A crash happened

Version of rustc

No response

Automated bug report

Software version

git-branchless 0.8.0

Operating system

Linux 6.1.74

Command-line

/home/sam/.nix-profile/bin/git-branchless bug-report 

Environment variables

SHELL=/run/current-system/sw/bin/zsh
EDITOR=vim

Git version

> git version 
git version 2.43.0

Hooks

Show 7 hooks ##### Hook `post-applypatch` ``` #!/bin/sh ## START BRANCHLESS CONFIG git branchless hook post-applypatch "$@" ## END BRANCHLESS CONFIG ``` ##### Hook `post-checkout` ``` #!/bin/sh ## START BRANCHLESS CONFIG git branchless hook post-checkout "$@" ## END BRANCHLESS CONFIG ``` ##### Hook `post-commit` ``` #!/bin/sh ## START BRANCHLESS CONFIG git branchless hook post-commit "$@" ## END BRANCHLESS CONFIG ``` ##### Hook `post-merge` ``` #!/bin/sh ## START BRANCHLESS CONFIG git branchless hook post-merge "$@" ## END BRANCHLESS CONFIG ``` ##### Hook `post-rewrite` ``` #!/bin/sh ## START BRANCHLESS CONFIG git branchless hook post-rewrite "$@" ## END BRANCHLESS CONFIG ``` ##### Hook `pre-auto-gc` ``` #!/bin/sh ## START BRANCHLESS CONFIG git branchless hook pre-auto-gc "$@" ## END BRANCHLESS CONFIG ``` ##### Hook `reference-transaction` ``` #!/bin/sh ## START BRANCHLESS CONFIG # Avoid canceling the reference transaction in the case that `branchless` fails # for whatever reason. git branchless hook reference-transaction "$@" || ( echo 'branchless: Failed to process reference transaction!' echo 'branchless: Some events (e.g. branch updates) may have been lost.' echo 'branchless: This is a bug. Please report it.' ) ## END BRANCHLESS CONFIG ```

Events

Show 5 events ##### Event ID: 7, transaction ID: 5 (message: reword) 1. `RefUpdateEvent { timestamp: 1706552567.2591848, event_tx_id: EventTransactionId(5), ref_name: ReferenceName("refs/heads/main"), old_oid: 9202ffd46a48d38c42c8843eb6f22cb3dcc9f677, new_oid: ad2fd8941a2bb37f80c44b2ba7a25949a9489c7f, message: None }` 1. `RewriteEvent { timestamp: 1706552567.2686825, event_tx_id: EventTransactionId(5), old_commit_oid: 9202ffd46a48d38c42c8843eb6f22cb3dcc9f677, new_commit_oid: ad2fd8941a2bb37f80c44b2ba7a25949a9489c7f }` 1. `WorkingCopySnapshot { timestamp: 1706552567.2729092, event_tx_id: EventTransactionId(5), head_oid: 9202ffd46a48d38c42c8843eb6f22cb3dcc9f677, commit_oid: NonZeroOid(8dcb0ad411c163cc1eaeef0b84e5faf34e404812), ref_name: None }` 1. `RefUpdateEvent { timestamp: 1706552567.2799516, event_tx_id: EventTransactionId(5), ref_name: ReferenceName("HEAD"), old_oid: 9202ffd46a48d38c42c8843eb6f22cb3dcc9f677, new_oid: ad2fd8941a2bb37f80c44b2ba7a25949a9489c7f, message: None }` ``` : @ ad2fd89 4m (> main) xxxxxx xxxxxx xxxx xx xxxxxxx ``` ##### Event ID: 6, transaction ID: 4 (message: post-commit) 1. `CommitEvent { timestamp: 1706552548.0, event_tx_id: EventTransactionId(4), commit_oid: NonZeroOid(9202ffd46a48d38c42c8843eb6f22cb3dcc9f677) }` ``` : @ ad2fd89 4m (> main) xxxxxx xxxxxx xxxx xx xxxxxxx ``` ##### Event ID: 4, transaction ID: 3 (message: reference-transaction) 1. `RefUpdateEvent { timestamp: 1706552548.8607051, event_tx_id: EventTransactionId(3), ref_name: ReferenceName("HEAD"), old_oid: e12c3c1de973ed76b458946ac976eb0c28d30698, new_oid: 9202ffd46a48d38c42c8843eb6f22cb3dcc9f677, message: None }` 1. `RefUpdateEvent { timestamp: 1706552548.8607051, event_tx_id: EventTransactionId(3), ref_name: ReferenceName("refs/heads/main"), old_oid: e12c3c1de973ed76b458946ac976eb0c28d30698, new_oid: 9202ffd46a48d38c42c8843eb6f22cb3dcc9f677, message: None }` ``` : @ ad2fd89 4m (> main) xxxxxx xxxxxx xxxx xx xxxxxxx ``` ##### Event ID: 3, transaction ID: 2 (message: post-commit) 1. `CommitEvent { timestamp: 1706552541.0, event_tx_id: EventTransactionId(2), commit_oid: NonZeroOid(e12c3c1de973ed76b458946ac976eb0c28d30698) }` ``` : @ ad2fd89 4m (> main) xxxxxx xxxxxx xxxx xx xxxxxxx ``` ##### Event ID: 1, transaction ID: 1 (message: reference-transaction) 1. `RefUpdateEvent { timestamp: 1706552541.0822334, event_tx_id: EventTransactionId(1), ref_name: ReferenceName("HEAD"), old_oid: 0000000000000000000000000000000000000000, new_oid: e12c3c1de973ed76b458946ac976eb0c28d30698, message: None }` 1. `RefUpdateEvent { timestamp: 1706552541.0822334, event_tx_id: EventTransactionId(1), ref_name: ReferenceName("refs/heads/main"), old_oid: 0000000000000000000000000000000000000000, new_oid: e12c3c1de973ed76b458946ac976eb0c28d30698, message: None }` ``` : @ ad2fd89 4m (> main) xxxxxx xxxxxx xxxx xx xxxxxxx ```

Version of git-branchless

No response

Version of git

No response

arxanas commented 5 months ago

This behavior makes sense for rebases (in the case that a change was applied upstream), but not for rewords. It looks like it happens here for the in-memory code path:

https://github.com/arxanas/git-branchless/blob/f1c1aa5abf5b9ce79bac7218d6e140f3f4263b13/git-branchless-lib/src/core/rewrite/execute.rs#L698

The on-disk rebase would have to use --keep-empty to accomplish the same thing.

We would add an option here to toggle it (probably skip_empty_commits):

https://github.com/arxanas/git-branchless/blob/f1c1aa5abf5b9ce79bac7218d6e140f3f4263b13/git-branchless-lib/src/core/rewrite/execute.rs#L1195

I don't plan to implement this myself but feel free to try it. You can also try https://github.com/martinvonz/jj as you can use it alongside Git (for b4) and it handles empty commits as part of its core workflow (I forget if you've already tried it).

samueltardieu commented 5 months ago

This behavior makes sense for rebases (in the case that a change was applied upstream), but not for rewords.

While it makes sense to delete commits which were not empty but become empty when rebasing (because the change was applied upstream), I think a commit which was purposefully empty to start with should be kept as-is, as its presence is there for a reason unrelated to the content.

I forget if you've already tried it

Yeah, I even contributed to it. I like jj a lot, especially the ability to move chunks from one commit to the other or the complex graph manipulation, but there are several things I like more about git-branchless for the time being:

arxanas commented 5 months ago

I think a commit which was purposefully empty to start with should be kept as-is,

That makes sense, and is also probably easier to implement. We would just modify the line linked in https://github.com/arxanas/git-branchless/issues/1205#issuecomment-1916088599, and adjust the on-disk rebase to not emit the action to check for empty commits, and it should work. (The on-disk rebase already passes the equivalent of --keep-empty to git rebase and handles empty commit detection itself.)