diff mbox series

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

Message ID df4019458665eccf2b16cdf1d6c1061186a62711.1690903412.git.gitgitgadget@gmail.com (mailing list archive)
State Accepted
Commit e032abd5a0bec40a700527f71073f3ba21fce7d6
Headers show
Series rebase -i: impove handling of failed commands | expand

Commit Message

Phillip Wood Aug. 1, 2023, 3:23 p.m. UTC
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.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c                   | 19 +++++---------
 t/t3404-rebase-interactive.sh |  6 +++++
 t/t3430-rebase-merges.sh      |  4 +--
 t/t5407-post-rewrite-hook.sh  | 48 +++++++++++++++++++++++++++++++++++
 4 files changed, 63 insertions(+), 14 deletions(-)

Comments

Johannes Schindelin Aug. 23, 2023, 8:55 a.m. UTC | #1
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.

> -		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
Phillip Wood Sept. 4, 2023, 2:31 p.m. UTC | #2
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
diff mbox series

Patch

diff --git a/sequencer.c b/sequencer.c
index 62277e7bcc1..e25abfd2fb4 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4155,6 +4155,7 @@  static int do_merge(struct repository *r,
 	if (ret < 0) {
 		error(_("could not even attempt to merge '%.*s'"),
 		      merge_arg_len, arg);
+		unlink(git_path_merge_msg(r));
 		goto leave_merge;
 	}
 	/*
@@ -4645,7 +4646,7 @@  N_("Could not execute the todo command\n"
 static int pick_one_commit(struct repository *r,
 			   struct todo_list *todo_list,
 			   struct replay_opts *opts,
-			   int *check_todo)
+			   int *check_todo, int* reschedule)
 {
 	int res;
 	struct todo_item *item = todo_list->items + todo_list->current;
@@ -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--;
-		if (save_todo(todo_list, opts))
-			return -1;
+		*reschedule = 1;
+		return -1;
 	}
 	if (item->command == TODO_EDIT) {
 		struct commit *commit = item->commit;
@@ -4763,7 +4760,8 @@  static int pick_commits(struct repository *r,
 			}
 		}
 		if (item->command <= TODO_SQUASH) {
-			res = pick_one_commit(r, todo_list, opts, &check_todo);
+			res = pick_one_commit(r, todo_list, opts, &check_todo,
+					      &reschedule);
 			if (!res && item->command == TODO_EDIT)
 				return 0;
 		} else if (item->command == TODO_EXEC) {
@@ -4817,10 +4815,7 @@  static int pick_commits(struct repository *r,
 			if (save_todo(todo_list, opts))
 				return -1;
 			if (item->commit)
-				return error_with_patch(r,
-							item->commit,
-							arg, item->arg_len,
-							opts, res, 0);
+				write_rebase_head(&item->commit->object.oid);
 		} else if (is_rebase_i(opts) && check_todo && !res &&
 			   reread_todo_if_changed(r, todo_list, opts)) {
 			return -1;
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index ff0afad63e2..6d3788c588b 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1287,7 +1287,9 @@  test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '
 	>file6 &&
 	test_must_fail git rebase --continue &&
 	test_cmp_rev HEAD F &&
+	test_cmp_rev REBASE_HEAD I &&
 	rm file6 &&
+	test_path_is_missing .git/rebase-merge/patch &&
 	git rebase --continue &&
 	test_cmp_rev HEAD I
 '
@@ -1305,7 +1307,9 @@  test_expect_success 'rebase -i commits that overwrite untracked files (squash)'
 	>file6 &&
 	test_must_fail git rebase --continue &&
 	test_cmp_rev HEAD F &&
+	test_cmp_rev REBASE_HEAD I &&
 	rm file6 &&
+	test_path_is_missing .git/rebase-merge/patch &&
 	git rebase --continue &&
 	test $(git cat-file commit HEAD | sed -ne \$p) = I &&
 	git reset --hard original-branch2
@@ -1323,7 +1327,9 @@  test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' '
 	>file6 &&
 	test_must_fail git rebase --continue &&
 	test $(git cat-file commit HEAD | sed -ne \$p) = F &&
+	test_cmp_rev REBASE_HEAD I &&
 	rm file6 &&
+	test_path_is_missing .git/rebase-merge/patch &&
 	git rebase --continue &&
 	test $(git cat-file commit HEAD | sed -ne \$p) = I
 '
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 &&