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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Glen

On 21/06/2023 21:49, Glen Choo wrote:
> "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

>> 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