Re: [PATCH v2 5/6] rebase: fix rewritten list for failed pick

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jul 25, 2023, 15:42 UTC
Message-ID: <fdb605bf-938d-6135-b341-6cf20600abaf@gmail.com>
In-Reply-To: <kl6lmt0sr16k.fsf@chooglen-macbookpro.roam.corp.google.com>

Hi Glen

On 21/06/2023 21:49, Glen Choo wrote:

Show 15 quoted lines

> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> When rebasing commands are moved from the todo list in "git-rebase-todo"
>> to the "done" file just before they are executed. This means that if a
>> command fails because it would overwrite an untracked file it has to be
>> added back into the todo list before the rebase stops for the user to
>> fix the problem. Unfortunately the way this is done results in the
>> failed pick being recorded as rewritten.
> 
> I could not make the connection from the described problem to the
> proposed solution. In particular, I couldn't tell what about "the way
> this is done" that causes the incorrect behavior (e.g. are we failing to
> clean up something? are we writing the wrong set of metadata?).

Yes, on reflection that first paragraph is not very helpful. I've updated it to

git rebase keeps a list that maps the OID of each commit before it was rebased to the OID of the equivalent commit after the rebase. This list is used to drive the "post-rewrite" hook that is called at the end of a successful rebase. When a rebase stops for the user to resolve merge conflicts the OID of the commit being picked is written to ".git/rebase-merge/stopped-sha1" and when the rebase is continued that OID is added to the list of rewritten commits. Unfortunately when a commit cannot be picked because it would overwrite an untracked file we still write the "stopped-sha1" file and so when the rebase is continued the commit is added into the list of rewritten commits even though it has not been picked yet.

Hopefully that is more helpful

Show 20 quoted lines

>> Fix this by not calling error_with_patch() for failed commands.
> 
> So unfortunately , I wasn't sure how this solution would fix the
> problem, and I didn't dive too deeply into this patch.
> 
>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
>> index c1fe55dc2c1..a657167befd 100755
>> --- a/t/t3404-rebase-interactive.sh
>> +++ b/t/t3404-rebase-interactive.sh
>> @@ -1289,6 +1289,10 @@ test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '
>>   	test_cmp_rev HEAD F &&
>>   	rm file6 &&
>>   	test_path_is_missing .git/rebase-merge/author-script &&
>> +	test_path_is_missing .git/rebase-merge/patch &&
>> +	test_path_is_missing .git/MERGE_MSG &&
>> +	test_path_is_missing .git/rebase-merge/message &&
>> +	test_path_is_missing .git/rebase-merge/stopped-sha &&
> 
> This also seems to be testing implementation details, and if so, it
> would be worth removing them.

With the exception of the "patch" file which exists solely for the benefit of the user this is testing an invariant of the implementation which isn't ideal. I'm worried that removing these checks will mask some subtle regression in the future. I think it is unlikely that the names of these files will change in the future as we try to avoid changes that would cause a rebase to fail if git is upgraded while it has stopped for the user to resolve conflicts. I did think about whether we could add some BUG() statements to sequencer.c instead. Unfortunately I don't think it is that easy for the sequencer to know when these files should be missing without relying on the logic that we are tying to test.

Best Wishes

Phillip