Re: [PATCH 2/2] merge: make restore_state() do as its name says

From: Junio C Hamano <gitster@pobox.com>
Date: May 19, 2022, 17:44 UTC
Message-ID: <xmqq5ym1pgby.fsf@gitster.g>
In-Reply-To: <88bdca72a780d70e156e22e1ab96dedd368c761b.1652977582.git.gitgitgadget@gmail.com>

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 12 quoted lines

> diff --git a/builtin/merge.c b/builtin/merge.c
> index 00de224a2da..ae3ee3a996b 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -377,11 +377,11 @@ static void restore_state(const struct object_id *head,
>  {
>  	const char *args[] = { "stash", "apply", NULL, NULL };
>  
> -	if (is_null_oid(stash))
> -		return;
> -
>  	reset_hard(head, 1);

when there is only one strategy to be tried, save_state() will never be called. Removing the above safety means the hard-reset is discarding a local change that is not saved anywhere. The reason why the merge stopped may be because such a local change has crashed with the change the merge wanted to bring in, no?

Show 20 quoted lines

> +	if (is_null_oid(stash))
> +		goto refresh_cache;
> +
> +test_description="Test that merge state is as expected after failed merge"
> +
> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> +. ./test-lib.sh
> +
> +test_expect_success 'set up custom strategy' '
> +	test_commit --no-tag "Initial" base base &&
> +git show-ref &&
> +
> +	for b in branch1 branch2 branch3
> +	do
> +		git checkout -b $b main &&
> +		test_commit --no-tag "Change on $b" base $b
> +	done &&
> +
> +	git checkout branch1 &&

Here, perhaps we can make two additional test cases, that try with local change that (1) overlaps with the changes branch2 and branch3 bring in and that (2) does not overlap. I am worried about the case (2) losing the local change due to the call to reset_hard().

Show 6 quoted lines

> +	test_must_fail git merge branch2 branch3 &&
> +	git diff --exit-code --name-status &&
> +	test_path_is_missing .git/MERGE_HEAD
> +'
> +
> +test_done