Re: [PATCH] unpack-trees: fix oneway_merge accidentally carry over stage index

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

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> One-way merge is supposed to take stat info from the index and
> everything else from the given tree. This implies stage 0 because trees
> can't have non-zero stages. The add_entry(.., old, ...) call however
> will keep stage index from the index.
>
> This is normally not a problem if the entry from the index is
> normal (stage #0). But if there is a conflict, we'll get stage #1 entry
> as "old" and it gets recorded in the final index. Fix it by clearing
> stage mask.
>
> This bug probably comes from b5b425074e (git-read-tree: make one-way
> merge also honor the "update" flag, 2005-06-07). Before this commit, we
> may create the final ("dst") index entry from the one in index, but we
> do clear CE_STAGEMASK.

Wow, good find.  That's an old one.

> I briefly checked two- and three-way merge functions. I think we don't
> have the same problem in those.
>
> Reported-by: Phillip Wood <phillip.wood123@gmail.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  This is one of the two bugs reported by Phillip. It's not tangled with
>  nd/switch-and-restore code changes and I'm sending it separately.

Thanks.

>
>  t/t2026-checkout-force.sh (new +x) | 26 ++++++++++++++++++++++++++

This makes it cumbersome to have the same fix in the maintenance track
as t2026 is already in use over there.  Do we need an entirely new test
just to house this new single test?

By the way, I am beginning to like these "in-line" summaries (as
opposed to the --summary at the end), although I admit that it has
been quite a while since its introduction.  Good job, again.

>  unpack-trees.c                     |  2 +-
>  2 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/t/t2026-checkout-force.sh b/t/t2026-checkout-force.sh
> new file mode 100755
> index 0000000000..272ccf533a
> --- /dev/null
> +++ b/t/t2026-checkout-force.sh
> @@ -0,0 +1,26 @@
> +#!/bin/sh
> +
> +test_description='checkout --force'
> +. ./test-lib.sh
> +
> +test_expect_success 'force checking out a conflict' '
> +	echo a >a &&
> +	git add a &&
> +	git commit -ama &&
> +	A_OBJ=$(git rev-parse :a) &&
> +	git branch topic &&
> +	echo b >a &&
> +	git commit -amb &&
> +	B_OBJ=$(git rev-parse :a) &&
> +	git checkout topic &&
> +	echo c >a &&
> +	C_OBJ=$(git hash-object a) &&
> +	git checkout -m master &&
> +	test_cmp_rev :1:a $A_OBJ &&
> +	test_cmp_rev :2:a $B_OBJ &&
> +	test_cmp_rev :3:a $C_OBJ &&
> +	git checkout -f topic &&
> +	test_cmp_rev :a $A_OBJ

So in short, "checkout -f" should have given us an entry for path
"a", taken from the tip of the 'topic' branch, at stage #0 while
switching to that branch, but it didn't?  That would be a nice
summary to have at the beginning of the log message before going
into the implementation detail of how that happens.

> +'
> +
> +test_done
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 22c41a3ba8..1ccd343cad 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -2386,7 +2386,7 @@ int oneway_merge(const struct cache_entry * const *src,
>  		if (o->update && S_ISGITLINK(old->ce_mode) &&
>  		    should_update_submodules() && !verify_uptodate(old, o))
>  			update |= CE_UPDATE;
> -		add_entry(o, old, update, 0);
> +		add_entry(o, old, update, CE_STAGEMASK);

And the fix is obvious, makes sense and is in line with the
observation you made in the proposed log message.  

Nicely done.

Thanks.

>  		return 0;
>  	}
>  	return merged_entry(a, old, o);