Thread (256 messages) 256 messages, 11 authors, 2019-07-02

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

From: Junio C Hamano <hidden>
Date: 2019-03-18 04:00:20 

Nguyễn Thái Ngọc Duy  [off-list ref] 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 <redacted>
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 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.
quoted hunk ↗ jump to hunk
 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.
quoted hunk ↗ jump to hunk
+'
+
+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);
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help