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

From: Glen Choo <chooglen@google.com>
Date: Jun 21, 2023, 20:49 UTC
Message-ID: <kl6lmt0sr16k.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <f8e64c1b631116367e6e68fcfde711b507a03a94.1682089075.git.gitgitgadget@gmail.com>

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 8 quoted lines

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

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

Show 12 quoted lines

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