Re: [PATCH v3 5/7] rebase: fix rewritten list for failed pick

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

Hi Dscho

On 23/08/2023 09:55, Johannes Schindelin wrote:
> Hi Phillip,
> 
> On Tue, 1 Aug 2023, Phillip Wood via GitGitGadget wrote:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> 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-sha". Then when the rebase is continued that
>> OID is added to the list of rewritten commits. Unfortunately if a commit
>> cannot be picked because it would overwrite an untracked file we still
>> write the "stopped-sha1" file. This means that when the rebase is
>> continued the commit is added into the list of rewritten commits even
>> though it has not been picked yet.
>>
>> Fix this by not calling error_with_patch() for failed commands. The pick
>> has failed so there is nothing to commit and therefore we do not want to
>> set up the state files for committing staged changes when the rebase
>> continues. This change means we no-longer write a patch for the failed
>> command or display the error message printed by error_with_patch(). As
>> the command has failed the patch isn't really useful and in any case the
>> user can inspect the commit associated with the failed command by
>> inspecting REBASE_HEAD. Unless the user has disabled it we already print
>> an advice message that is more helpful than the message from
>> error_with_patch() which the user will still see. Even if the advice is
>> disabled the user will see the messages from the merge machinery
>> detailing the problem.
>>
>> To simplify writing REBASE_HEAD in this case pick_one_commit() is
>> modified to avoid duplicating the code that adds the failed command
>> back into the todo list.
> 
> This motivates the change well, and answered all but one of the questions
> I had about it, being:
> 
>> diff --git a/sequencer.c b/sequencer.c
>> index 62277e7bcc1..e25abfd2fb4 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> [...]
>> @@ -4658,12 +4659,8 @@ static int pick_one_commit(struct repository *r,
>>   			     check_todo);
>>   	if (is_rebase_i(opts) && res < 0) {
>>   		/* Reschedule */
>> -		advise(_(rescheduled_advice),
>> -		       get_item_line_length(todo_list, todo_list->current),
>> -		       get_item_line(todo_list, todo_list->current));
>> -		todo_list->current--;
> 
> Why is it okay to remove this decrement?
> 
> Here is why: The code that calls `save_todo()` in the `if (reschedule)`
> block of the loop of `pick_commits()` _duplicates_ the logic that is
> removed here, including the advice and the decrementing of `current`.
> 
> I had to instrument the code before and after this patch to figure this
> out, as I had missed the fact that the now-remaining code also decremented
> the `current` attribute.
> 
> So: All is good with this patch. If you'd like to amend the commit message
> accordingly, I would not be opposed, but I could now live equally as
> easily without it.

I'll try and add something to the commit message when I re-roll

Thanks

Phillip

>> -		if (save_todo(todo_list, opts))
>> -			return -1;
>> +		*reschedule = 1;
>> +		return -1;
>>   	}
>>   	if (item->command == TODO_EDIT) {
>>   		struct commit *commit = item->commit;
> 
> I'd like to point out how delighted I am about this careful test case:
> 
>> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
>> index 96ae0edf1e1..4938ebb1c17 100755
>> --- a/t/t3430-rebase-merges.sh
>> +++ b/t/t3430-rebase-merges.sh
>> @@ -165,12 +165,12 @@ test_expect_success 'failed `merge -C` writes patch (may be rescheduled, too)' '
>>   	test_config sequence.editor \""$PWD"/replace-editor.sh\" &&
>>   	test_tick &&
>>   	test_must_fail git rebase -ir HEAD &&
>> +	test_cmp_rev REBASE_HEAD H^0 &&
>>   	grep "^merge -C .* G$" .git/rebase-merge/done &&
>>   	grep "^merge -C .* G$" .git/rebase-merge/git-rebase-todo &&
>> -	test_path_is_file .git/rebase-merge/patch &&
>> +	test_path_is_missing .git/rebase-merge/patch &&
>>
>>   	: fail because of merge conflict &&
>> -	rm G.t .git/rebase-merge/patch &&
>>   	git reset --hard conflicting-G &&
>>   	test_must_fail git rebase --continue &&
>>   	! grep "^merge -C .* G$" .git/rebase-merge/git-rebase-todo &&
>> diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh
>> index 5f3ff051ca2..ad7f8c6f002 100755
>> --- a/t/t5407-post-rewrite-hook.sh
>> +++ b/t/t5407-post-rewrite-hook.sh
>> @@ -17,6 +17,12 @@ test_expect_success 'setup' '
>>   	git checkout A^0 &&
>>   	test_commit E bar E &&
>>   	test_commit F foo F &&
>> +	git checkout B &&
>> +	git merge E &&
>> +	git tag merge-E &&
>> +	test_commit G G &&
>> +	test_commit H H &&
>> +	test_commit I I &&
>>   	git checkout main &&
>>
>>   	test_hook --setup post-rewrite <<-EOF
>> @@ -173,6 +179,48 @@ test_fail_interactive_rebase () {
>>   	)
>>   }
>>
>> +test_expect_success 'git rebase with failed pick' '
>> +	clear_hook_input &&
>> +	cat >todo <<-\EOF &&
>> +	exec >bar
>> +	merge -C merge-E E
>> +	exec >G
>> +	pick G
>> +	exec >H 2>I
>> +	pick H
>> +	fixup I
>> +	EOF
>> +
>> +	(
>> +		set_replace_editor todo &&
>> +		test_must_fail git rebase -i D D 2>err
>> +	) &&
>> +	grep "would be overwritten" err &&
>> +	rm bar &&
>> +
>> +	test_must_fail git rebase --continue 2>err &&
>> +	grep "would be overwritten" err &&
>> +	rm G &&
>> +
>> +	test_must_fail git rebase --continue 2>err &&
>> +	grep "would be overwritten" err &&
>> +	rm H &&
>> +
>> +	test_must_fail git rebase --continue 2>err &&
>> +	grep "would be overwritten" err &&
>> +	rm I &&
>> +
>> +	git rebase --continue &&
>> +	echo rebase >expected.args &&
>> +	cat >expected.data <<-EOF &&
>> +	$(git rev-parse merge-E) $(git rev-parse HEAD~2)
>> +	$(git rev-parse G) $(git rev-parse HEAD~1)
>> +	$(git rev-parse H) $(git rev-parse HEAD)
>> +	$(git rev-parse I) $(git rev-parse HEAD)
>> +	EOF
>> +	verify_hook_input
>> +'
>> +
>>   test_expect_success 'git rebase -i (unchanged)' '
>>   	git reset --hard D &&
>>   	clear_hook_input &&
> 
> Here is my ACK.
> 
> Thank you,
> Johannes