Re: [PATCH 3/3] merge-recursive: Avoid incorporating uncommitted changes in a merge

[Junio C Hamano <gitster@pobox.com>]

Elijah Newren <newren@gmail.com> writes:

> builtin/merge.c contains this important requirement for merge strategies:
> 	/*
> 	 * At this point, we need a real merge.  No matter what strategy
> 	 * we use, it would operate on the index, possibly affecting the
> 	 * working tree, and when resolved cleanly, have the desired
> 	 * tree in the index -- this means that the index must be in
> 	 * sync with the head commit.  The strategies are responsible
> 	 * to ensure this.
> 	 */
>
> merge-recursive does not do this check directly, instead it relies on
> unpack_trees() to do it.  However, merge_trees() has a special check for
> the merge branch exactly matching the merge base; when it detects that
> situation, it returns early without calling unpack_trees(), because it
> knows that the HEAD commit already has the correct result.  Unfortunately,
> it didn't check that the index matched HEAD, so after it returned, the
> outer logic ended up creating a merge commit that included something
> other than HEAD.

Good.

I actually was imagining that you would shoot for creating an empty
commit and leaving a working tree and the index that are both dirty,
but I do not think it is worth the effort.  Besides, "you have to
start from a clean index" is a much simpler rule to explain than
with "unless the resulting tree is the same as HEAD", especially
when that "unless" is highly unlikely to happen anyway.

Thanks.

>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  merge-recursive.c                        | 7 +++++++
>  t/t6044-merge-unrelated-index-changes.sh | 2 +-
>  2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 2ecf495cc2..780f81a8bd 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -1952,6 +1952,13 @@ int merge_trees(struct merge_options *o,
>  	}
>  
>  	if (oid_eq(&common->object.oid, &merge->object.oid)) {
> +		struct strbuf sb = STRBUF_INIT;
> +
> +		if (index_has_changes(&sb)) {
> +			err(o, _("Dirty index: cannot merge (dirty: %s)"),
> +			    sb.buf);
> +			return 0;
> +		}
>  		output(o, 0, _("Already up to date!"));
>  		*result = head;
>  		return 1;
> diff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh
> index 5e472be92b..23b86fb977 100755
> --- a/t/t6044-merge-unrelated-index-changes.sh
> +++ b/t/t6044-merge-unrelated-index-changes.sh
> @@ -112,7 +112,7 @@ test_expect_success 'recursive' '
>  	test_must_fail git merge -s recursive C^0
>  '
>  
> -test_expect_failure 'recursive, when merge branch matches merge base' '
> +test_expect_success 'recursive, when merge branch matches merge base' '
>  	git reset --hard &&
>  	git checkout B^0 &&