Re: [PATCH v4 2/7] merge-resolve: abort if index does not match HEAD

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Jul 22 2022, Elijah Newren via GitGitGadget wrote:

> From: Elijah Newren <newren@gmail.com>
>
> As noted in commit 9822175d2b ("Ensure index matches head before
> invoking merge machinery, round N", 2019-08-17), we have had a very
> long history of problems with failing to enforce the requirement that
> index matches HEAD when starting a merge.  One of the commits
> referenced in the long tale of issues arising from lax enforcement of
> this requirement was commit 55f39cf755 ("merge: fix misleading
> pre-merge check documentation", 2018-06-30), which tried to document
> the requirement and noted there were some exceptions.  As mentioned in
> that commit message, the `resolve` strategy was the one strategy that
> did not have an explicit index matching HEAD check, and the reason it
> didn't was that I wasn't able to discover any cases where the
> implementation would fail to catch the problem and abort, and didn't
> want to introduce unnecessary performance overhead of adding another
> check.
>
> Well, today I discovered a testcase where the implementation does not
> catch the problem and so an explicit check is needed.  Add a testcase
> that previously would have failed, and update git-merge-resolve.sh to
> have an explicit check.  Note that the code is copied from 3ec62ad9ff
> ("merge-octopus: abort if index does not match HEAD", 2016-04-09), so
> that we reuse the same message and avoid making translators need to
> translate some new message.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  builtin/merge.c                          | 20 ++++++++++++++++++
>  git-merge-resolve.sh                     | 10 +++++++++
>  t/t6424-merge-unrelated-index-changes.sh | 26 ++++++++++++++++++++++++
>  3 files changed, 56 insertions(+)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 23170f2d2a6..13884b8e836 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -1599,6 +1599,26 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  		 */
>  		refresh_cache(REFRESH_QUIET);
>  		if (allow_trivial && fast_forward != FF_ONLY) {
> +			/*
> +			 * Must first ensure that index matches HEAD before
> +			 * attempting a trivial merge.
> +			 */
> +			struct tree *head_tree = get_commit_tree(head_commit);
> +			struct strbuf sb = STRBUF_INIT;
> +
> +			if (repo_index_has_changes(the_repository, head_tree,
> +						   &sb)) {
> +				struct strbuf err = STRBUF_INIT;
> +				strbuf_addstr(&err, "error: ");
> +				strbuf_addf(&err, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
> +					    sb.buf);
> +				strbuf_addch(&err, '\n');

At first glance I was expecting this to construct an error message to
emit it somewhere else that stderr, so I wondered if you couldn't use
the "error_routine" facility to avoid re-inventing "error: " etc.,
but...

> +				fputs(err.buf, stderr);

...we emit it to stderr anyway...?

> +				strbuf_release(&err);
> +				strbuf_release(&sb);
> +				return -1;
> +			}
> +
>  			/* See if it is really trivial. */
>  			git_committer_info(IDENT_STRICT);
>  			printf(_("Trying really trivial in-index merge...\n"));
> diff --git a/git-merge-resolve.sh b/git-merge-resolve.sh
> index 343fe7bccd0..77e93121bf8 100755
> --- a/git-merge-resolve.sh
> +++ b/git-merge-resolve.sh
> @@ -5,6 +5,16 @@
>  #
>  # Resolve two trees, using enhanced multi-base read-tree.
>  
> +. git-sh-setup
> +
> +# Abort if index does not match HEAD
> +if ! git diff-index --quiet --cached HEAD --
> +then
> +    gettextln "Error: Your local changes to the following files would be overwritten by merge"
> +    git diff-index --cached --name-only HEAD -- | sed -e 's/^/    /'
> +    exit 2
> +fi

(The "..." continued below)

Just in trying to poke holes in this I made this an "exit 0", and
neither of the tests you added failed, but the last one ("resolve &&
recursive && ort") in the t6424*.sh will fail, is that intentional?

I don't know enough about the context here, but given our *.sh->C
migration elsewhere it's a bit unfortunate to see more *.sh code added
back. We have "git merge" driving this, isn't it OK to have it make this
check before invoking "resolve" (may be a stupid question).

For this code in particular it:

 * Uses spaces, not tabs
 * We lose the diff-index .. --name-only exit code (segfault), but so
   did the older version
 * I wonder if bending over backwards to emit the exact message we
   emitted before is worth it

If you just make this something like (untested):

	{
		gettext "error: " &&
		gettextln "Your local..."
	}

You could re-use the translation from the *.c one (and the "error: " one
we'll get from usage.c).

That leaves "\n %s" as the difference, but we could just remove that
from the _() and emit it unconditionally, no?


>  # The first parameters up to -- are merge bases; the rest are heads.
>  bases= head= remotes= sep_seen=
>  for arg
> diff --git a/t/t6424-merge-unrelated-index-changes.sh b/t/t6424-merge-unrelated-index-changes.sh
> index b6e424a427b..f35d3182b86 100755
> --- a/t/t6424-merge-unrelated-index-changes.sh
> +++ b/t/t6424-merge-unrelated-index-changes.sh
> @@ -114,6 +114,32 @@ test_expect_success 'resolve, non-trivial' '
>  	test_path_is_missing .git/MERGE_HEAD
>  '
>  
> +test_expect_success 'resolve, trivial, related file removed' '
> +	git reset --hard &&
> +	git checkout B^0 &&
> +
> +	git rm a &&
> +	test_path_is_missing a &&
> +
> +	test_must_fail git merge -s resolve C^0 &&
> +
> +	test_path_is_missing a &&
> +	test_path_is_missing .git/MERGE_HEAD
> +'
> +
> +test_expect_success 'resolve, non-trivial, related file removed' '
> +	git reset --hard &&
> +	git checkout B^0 &&
> +
> +	git rm a &&
> +	test_path_is_missing a &&
> +
> +	test_must_fail git merge -s resolve D^0 &&
> +
> +	test_path_is_missing a &&
> +	test_path_is_missing .git/MERGE_HEAD
> +'
> +
>  test_expect_success 'recursive' '
>  	git reset --hard &&
>  	git checkout B^0 &&

...I tried with this change on top, it seems to me like you'd want this
in any case, it passes the tests both with & without the C code change,
so can't we just use error() here?
	
	diff --git a/builtin/merge.c b/builtin/merge.c
	index 7fb4414ebb7..64def49734a 100644
	--- a/builtin/merge.c
	+++ b/builtin/merge.c
	@@ -1621,13 +1621,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
	 
	 			if (repo_index_has_changes(the_repository, head_tree,
	 						   &sb)) {
	-				struct strbuf err = STRBUF_INIT;
	-				strbuf_addstr(&err, "error: ");
	-				strbuf_addf(&err, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
	-					    sb.buf);
	-				strbuf_addch(&err, '\n');
	-				fputs(err.buf, stderr);
	-				strbuf_release(&err);
	+				error(_("Your local changes to the following files would be overwritten by merge:\n  %s"),
	+				      sb.buf);
	 				strbuf_release(&sb);
	 				return -1;
	 			}
	diff --git a/t/t6424-merge-unrelated-index-changes.sh b/t/t6424-merge-unrelated-index-changes.sh
	index c96649448fa..1df130b9ee6 100755
	--- a/t/t6424-merge-unrelated-index-changes.sh
	+++ b/t/t6424-merge-unrelated-index-changes.sh
	@@ -96,7 +96,12 @@ test_expect_success 'resolve, trivial' '
	 
	 	touch random_file && git add random_file &&
	 
	-	test_must_fail git merge -s resolve C^0 &&
	+	sed -e "s/^> //g" >expect <<-\EOF &&
	+	> error: Your local changes to the following files would be overwritten by merge:
	+	>   random_file
	+	EOF
	+	test_must_fail git merge -s resolve C^0 2>actual &&
	+	test_cmp expect actual &&
	 	test_path_is_file random_file &&
	 	git rev-parse --verify :random_file &&
	 	test_path_is_missing .git/MERGE_HEAD