| Message ID | 20190317060023.3651-1-pclouds@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | unpack-trees: fix oneway_merge accidentally carry over stage index | expand |
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);
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.
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.
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);
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(-)