unpack-trees: fix oneway_merge accidentally carry over stage index
diff mbox series

Message ID 20190317060023.3651-1-pclouds@gmail.com
State New
Headers show
Series
  • unpack-trees: fix oneway_merge accidentally carry over stage index
Related show

Commit Message

Duy Nguyen March 17, 2019, 6 a.m. UTC
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.

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.

 t/t2026-checkout-force.sh (new +x) | 26 ++++++++++++++++++++++++++
 unpack-trees.c                     |  2 +-
 2 files changed, 27 insertions(+), 1 deletion(-)

Comments

Junio C Hamano March 18, 2019, 3:58 a.m. UTC | #1
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);
Duy Nguyen March 18, 2019, 9:24 a.m. UTC | #2
On Mon, Mar 18, 2019 at 10:58 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.

Credit goes to Phillip for such attention to detail. If I tested this,
I would have stopped after seeing conflict stages collapsed into one
and missed the stage index. In fact I was swearing "what the hell did
he complain about" when looking at his test script's result, until I
realized stage #1 was indeed wrong.

> >
> >  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?

I could not find any right file to put it in. I guess I could stick it
in t2023-checkout-m.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.

OK. And the last line probably should be :0:a to make it clear we're
looking for stage #0.
Junio C Hamano March 18, 2019, 9:40 a.m. UTC | #3
Duy Nguyen <pclouds@gmail.com> writes:

>> > +     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.
>
> OK. And the last line probably should be :0:a to make it clear we're
> looking for stage #0.

I would say ":a" is plenty clear that it is looking for a merged
entry without getting replaced with an overly explicit ":0:a", but
":0:a" would not hurt, either.

Patch
diff mbox series

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
+'
+
+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);
 		return 0;
 	}
 	return merged_entry(a, old, o);