(#1088) post-rewrite hook does not track intermediate commits from an interactive rebase #1098
Conversation
cshinaver
force-pushed
the
cs-1088
branch
4 times, most recently
from
7ab7c72
to
7c75b50
Compare
|
The test that is failing is doing a commit in the middle of a rebase. The code I removed is meant to preserve commits done in the middle of the rebase. is this intentional? The final commits should be reserved, but if we want to track the intermediate commits, then all of git's automatically created intermediate commits are also tracked (#1088 ) |
There was a problem hiding this comment.
Thanks for looking into this! It's been a thorn in many people's sides for a while.
I noticed that this code was added to prevent aborted rebases from adding intermediate commits, but the test written for that still passes
It looks like the test test_rebase_no_process_new_commits_until_conclusion is failing in CI. Unfortunately, due to bad reasons, you might have to run cargo build --workspace after your change and then run tests (related to https://github.com/arxanas/git-branchless/wiki/Runbook#testing-subcommands-in-isolation). You should also make sure that you're testing with a recent version of Git, as the test body is gated behind Git::supports_reference_transactions.
The final commits should be reserved, but if we want to track the intermediate commits, then all of git's automatically created intermediate commits are also tracked
I think it's okay to not preserve intermediate commits directly as long as 1) the final commits are preserved and 2) aborting the rebase does not preserve the final commits. There may have been a workflow where one uses edit and creates new commits during a rebase, which, in principle, should be tracked, but I am willing to sacrifice that workflow if necessary to support the fixup workflow.
I suspect (but haven't investigated) that the fixup rebase command in Git does not register the fixed-up commits to be emitted as part of the post-rewrite hook, so, as far as git-branchless knows, they're legitimate commits. If so, then, in principle, it would be good to upstream a fix for that to Git, but it wouldn't solve most of our users' fixup problems for a long while.
I would be willing to accept some kind of hack/workaround for fixup specifically, like maybe ignoring commits whose commit messages start with # (which it looks like interactive fixup commits do, but not sure about fixup! with git rebase --autosquash). Another idea is that, during the post-commit hook, you could check the current status of the rebase and see if the most recent command was a fixup, although I'm not sure how feasible it is.
|
In the failing test, a commit ( |
There was a problem hiding this comment.
Thanks, I think the approach/tests are good; we should just try to not expand the testing API surface unless necessary. (It's already bigger than it should be — for example, practically none of the tests actually need to set a timestamp explicitly, and it's just convention by now...)
| @@ -498,14 +498,15 @@ then you can only run tests in the main `git-branchless` and \ | |||
| /// used to set the commit timestamp, which is factored into the commit | |||
| /// hash. The filename is always appended to the message prefix. | |||
| #[instrument] | |||
| pub fn commit_file_with_contents_and_message( | |||
| pub fn commit_file_with_contents_and_message_and_file_name( | |||
There was a problem hiding this comment.
I would rather keep the API smaller. Can you revert this change and call write_file followed by commit_file instead? (Or feel free to use git.run(&["commit", ...]); the timestamp should still be stable for tests.)
| @@ -148,19 +133,6 @@ pub fn hook_post_rewrite( | |||
| let event_log_db = EventLogDb::new(&conn)?; | |||
| let event_tx_id = event_log_db.make_transaction_id(now, "hook-post-rewrite")?; | |||
|
|
|||
| { | |||
There was a problem hiding this comment.
Is there code that also writes the deferred commits that is now also obsolete? If so, we should delete it too. Or, alternatively, keep all the code but comment out the add_events line with a note, as we might need similar code in the future to fix certain other bugs.
| run_in_pty_with_command( | ||
| &git, | ||
| &["rebase"], | ||
| &["-i", "--autosquash", "master"], | ||
| &[ | ||
| PtyAction::WaitUntilContains(" "), | ||
| PtyAction::Write(CARRIAGE_RETURN), | ||
| ], | ||
| )?; |
There was a problem hiding this comment.
Can we just write git.run(&["rebase", "--autosquash", "master"])? It seems to work in my testing. If so, do that and revert the changes to run_in_pty.
Fixes #1088
I was able to repro the issue from #1088. It seems that we are tracking the intermediate commits created by a rebase and adding them to the events db during the post rewrite hook. I noticed that this code was added to prevent aborted rebases from adding intermediate commits, but the test written for that still passes. Looking for feedback on the approach from @arxanas regarding omitting deferred commits from the event log during the post-rewrite hook.