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

Commit Message

Phillip Wood April 21, 2023, 2:57 p.m. UTC

From: Phillip Wood <[e-mail]>

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.

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 message file 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 in that case and
REBASE_HEAD is still written so the user can inspect the commit
associated with the failed command. Unless the user has disabled it we
print an advice message that is more helpful than the message from
error_with_patch(). If the advice is disabled the user will still 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 <[e-mail]>
---
 sequencer.c                   | 19 +++++++------------
 t/t3404-rebase-interactive.sh | 12 ++++++++++++
 t/t3430-rebase-merges.sh      | 11 ++++++++---
 t/t5407-post-rewrite-hook.sh  | 11 +++++++++++
 4 files changed, 38 insertions(+), 15 deletions(-)

Comments

Glen Choo June 21, 2023, 8:49 p.m. UTC | #1

"Phillip Wood via GitGitGadget" <[e-mail]> writes:

> From: Phillip Wood <[e-mail]>
>
> 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?).

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

Phillip Wood July 25, 2023, 3:42 p.m. UTC | #2

Hi Glen

On 21/06/2023 21:49, Glen Choo wrote:
> "Phillip Wood via GitGitGadget" <[e-mail]> writes:
> 
>> From: Phillip Wood <[e-mail]>
>>
>> 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

Glen Choo July 25, 2023, 4:46 p.m. UTC | #3

Phillip Wood <[e-mail]> writes:

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

Ah, yes that is much easier to visualise and understand. Thanks so much.

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

Unfortunately, it's been a while since I reviewed this patch, so forgive
me if I'm rusty. So you're saying that this test is about checking
invariants that we want to preserve between Git versions. I think that's
a reasonable goal - I am slightly skeptical of whether we should be
doing that ad-hoc like this, but I don't feel strongly about it.

IIRC, there was an earlier patch would be different from an where we
tested that author-script is missing, but what we really want is for the
pick to stop. Is the same thing happening here? E.g. is 'testing for
missing stopped-sha' a stand-in for 'testing that the rewritten list is
correct'? If so, it would be nice to test that specifically, but if
that's infeasible, a clarifying comment will probably suffice.

Phillip Wood July 26, 2023, 1:08 p.m. UTC | #4

Hi Glen

On 25/07/2023 17:46, Glen Choo wrote:
> Phillip Wood <[e-mail]> writes:
>>>> 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.
> 
> Unfortunately, it's been a while since I reviewed this patch, so forgive
> me if I'm rusty. So you're saying that this test is about checking
> invariants that we want to preserve between Git versions.

Not really. One of the reasons why testing the implementation rather 
than the user observable behavior is a bad idea is that when the 
implementation is changed the test is likely to start failing or keep 
passing without checking anything useful. I was trying to say that in 
this case we're unlikely to change this aspect of the implementation 
because it would be tricky to do so without inconveniencing users who 
upgrade git while rebase is stopped for a conflict resolution and so it 
is unlikely that this test will be affected by future changes to the 
implementation.

> IIRC, there was an earlier patch would be different from an where we
> tested that author-script is missing, but what we really want is for the
> pick to stop. Is the same thing happening here? E.g. is 'testing for
> missing stopped-sha' a stand-in for 'testing that the rewritten list is
> correct'? If so, it would be nice to test that specifically, but if
> that's infeasible, a clarifying comment will probably suffice.

Yes this patch adds a test to t5407-post-rewrite-hook.sh to do that but 
it only checks a failing "pick" command. The reason I think it is useful 
to add these test_path_is_missing checks is that they are checking 
failing "squash" and "merge" commands as well. Maybe I should just bite 
the bullet see how tricky it is to extend the post-rewrite-hook test to 
cover those cases as well.

Best Wishes

Phillip

Glen Choo July 26, 2023, 5:48 p.m. UTC | #5

Phillip Wood <[e-mail]> writes:

>> Unfortunately, it's been a while since I reviewed this patch, so forgive
>> me if I'm rusty. So you're saying that this test is about checking
>> invariants that we want to preserve between Git versions.
>
> Not really. One of the reasons why testing the implementation rather 
> than the user observable behavior is a bad idea is that when the 
> implementation is changed the test is likely to start failing or keep 
> passing without checking anything useful. I was trying to say that in 
> this case we're unlikely to change this aspect of the implementation 
> because it would be tricky to do so without inconveniencing users who 
> upgrade git while rebase is stopped for a conflict resolution and so it 
> is unlikely that this test will be affected by future changes to the 
> implementation.

Ah, I see the difference. I think that's it's fair to assume that the
names of the files will be fairly stable, though this series has made it
clear to me that what each file does and when it is written is quite
under-documented, and I wouldn't be surprised to see some of that change
if we start to try to explain the inner workings to ourselves.

> Yes this patch adds a test to t5407-post-rewrite-hook.sh to do that but 
> it only checks a failing "pick" command. The reason I think it is useful 
> to add these test_path_is_missing checks is that they are checking 
> failing "squash" and "merge" commands as well. Maybe I should just bite 
> the bullet see how tricky it is to extend the post-rewrite-hook test to 
> cover those cases as well.

Yes, that would probably be a good idea. Maybe if we combined them into
a test helper that checks all of "pick", "squash" and "merge", which
also has the added benefit of being able to hide implementation details
in case we decide to change them.

Phillip Wood July 28, 2023, 1:19 p.m. UTC | #6

Hi Glen

On 26/07/2023 18:48, Glen Choo wrote:
> Phillip Wood <[e-mail]> writes:
> 
>>> Unfortunately, it's been a while since I reviewed this patch, so forgive
>>> me if I'm rusty. So you're saying that this test is about checking
>>> invariants that we want to preserve between Git versions.
>>
>> Not really. One of the reasons why testing the implementation rather
>> than the user observable behavior is a bad idea is that when the
>> implementation is changed the test is likely to start failing or keep
>> passing without checking anything useful. I was trying to say that in
>> this case we're unlikely to change this aspect of the implementation
>> because it would be tricky to do so without inconveniencing users who
>> upgrade git while rebase is stopped for a conflict resolution and so it
>> is unlikely that this test will be affected by future changes to the
>> implementation.
> 
> Ah, I see the difference. I think that's it's fair to assume that the
> names of the files will be fairly stable, though this series has made it
> clear to me that what each file does and when it is written is quite
> under-documented,

That is certainly true

> and I wouldn't be surprised to see some of that change
> if we start to try to explain the inner workings to ourselves.

I agree. In the end I've removed the state file checks from the tests in 
favor of adding explicit checks that we refuse to commit staged changes 
and adding more cases to the test for the "post-rewrite" hook. I think 
it would probably be useful to add some assertions to the sequencer in a 
future series. We can assert things like "if this state file exists then 
so should these other ones" and "if this state file does not exist then 
these others should not" without relying on the logic in the sequencer. 
The sequencer assertions wouldn't know if the message file should exist 
but know what other files should exist if it does and the tests for 
committing staged changes then effectively check if the message file 
should exist.

Thanks for your comments on this series, I'll send a re-roll next week

Best Wishes

Phillip

>> Yes this patch adds a test to t5407-post-rewrite-hook.sh to do that but
>> it only checks a failing "pick" command. The reason I think it is useful
>> to add these test_path_is_missing checks is that they are checking
>> failing "squash" and "merge" commands as well. Maybe I should just bite
>> the bullet see how tricky it is to extend the post-rewrite-hook test to
>> cover those cases as well.
> 
> Yes, that would probably be a good idea. Maybe if we combined them into
> a test helper that checks all of "pick", "squash" and "merge", which
> also has the added benefit of being able to hide implementation details
> in case we decide to change them.

Patch

diff --git a/sequencer.c b/sequencer.c
index 55bf0a72c3a..db2daecb23e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4141,6 +4141,7 @@  static int do_merge(struct repository *r,
 		error(_("could not even attempt to merge '%.*s'"),
 		      merge_arg_len, arg);
 		unlink(rebase_path_author_script());
+		unlink(git_path_merge_msg(r));
 		goto leave_merge;
 	}
 	/*
@@ -4631,7 +4632,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;
@@ -4644,12 +4645,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;
@@ -4749,7 +4746,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) {
@@ -4803,10 +4801,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 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 &&
 	echo changed >file1 &&
 	git add file1 &&
 	test_must_fail git rebase --continue 2>err &&
@@ -1313,6 +1317,10 @@  test_expect_success 'rebase -i commits that overwrite untracked files (squash)'
 	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 &&
 	git rebase --continue &&
 	test $(git cat-file commit HEAD | sed -ne \$p) = I &&
 	git reset --hard original-branch2
@@ -1332,6 +1340,10 @@  test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' '
 	test $(git cat-file commit HEAD | sed -ne \$p) = 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 &&
 	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 360ec787ffd..18a0bc8fafb 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -167,16 +167,21 @@  test_expect_success 'failed `merge -C` writes patch (may be rescheduled, too)' '
 	test_must_fail git rebase -ir HEAD &&
 	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 &&
 	test_path_is_missing .git/rebase-merge/author-script &&
+	test_path_is_missing .git/MERGE_MSG &&
+	test_path_is_missing .git/rebase-merge/message &&
+	test_path_is_missing .git/rebase-merge/stopped-sha &&
 
 	: 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 &&
 	test_path_is_file .git/rebase-merge/patch &&
-	test_path_is_file .git/rebase-merge/author-script
+	test_path_is_file .git/rebase-merge/author-script &&
+	test_path_is_file .git/MERGE_MSG &&
+	test_path_is_file .git/rebase-merge/message &&
+	test_path_is_file .git/rebase-merge/stopped-sha
 '
 
 test_expect_success 'failed `merge <branch>` does not crash' '
diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh
index 5f3ff051ca2..c490a5137fe 100755
--- a/t/t5407-post-rewrite-hook.sh
+++ b/t/t5407-post-rewrite-hook.sh
@@ -173,6 +173,17 @@  test_fail_interactive_rebase () {
 	)
 }
 
+test_expect_success 'git rebase with failed pick' '
+	test_fail_interactive_rebase "exec_>bar pick 1" --onto C A E &&
+	rm bar &&
+	git rebase --continue &&
+	echo rebase >expected.args &&
+	cat >expected.data <<-EOF &&
+	$(git rev-parse E) $(git rev-parse HEAD)
+	EOF
+	verify_hook_input
+'
+
 test_expect_success 'git rebase -i (unchanged)' '
 	git reset --hard D &&
 	clear_hook_input &&