Elijah Newren <new...@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 <new...@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 &&

Reply via email to