Re: [PATCH v2] unpack-trees: fix oneway_merge accidentally carry over stage index
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Mar 19, 2019, 14:06 UTC
- Message-ID
- <228d681e-4cfd-7e2e-8bb9-1624cc244a9f@gmail.com>
- In-Reply-To
- <20190318113822.6195-1-pclouds@gmail.com>
Hi Duy
Thanks for doing this, one minor comment - I try to use phillip.wood@dunelm.org.uk for git as it wont change if I change my email provider.
Best Wishes
Phillip
On 18/03/2019 11:38, Nguyễn Thái Ngọc Duy wrote:
Show 80 quoted lines
> Phillip found out that 'git checkout -f <branch>' does not restore
> conflict/unmerged files correctly. All tracked files should be taken
> from <branch> and all non-zero stages removed. Most of this is true,
> except that the final file could be in stage one instead of zero.
>
> "checkout -f" (among other commands) does this with one-way merge, which
> is supposed to take stat info from the index and everything else from
> the given tree. The add_entry(.., old, ...) call in oneway_merge()
> though 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, stage #0 does not exist
> and we'll get stage #1 entry as "old" variable, which 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>
> ---
> v2 updates log message to describe the problem and moves the test to
> t2023-checkout.m.sh
>
> t/t2023-checkout-m.sh | 24 ++++++++++++++++++++++++
> unpack-trees.c | 2 +-
> 2 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/t/t2023-checkout-m.sh b/t/t2023-checkout-m.sh
> index 7e18985134..fca3f85824 100755
> --- a/t/t2023-checkout-m.sh
> +++ b/t/t2023-checkout-m.sh
> @@ -46,4 +46,28 @@ test_expect_success '-m restores 3-way conflicted+resolved file' '
> test_cmp both.txt.conflicted.cleaned both.txt.cleaned
> '
>
> +test_expect_success 'force checkout a conflict file creates stage zero entry' '
> + git init co-force &&
> + (
> + cd co-force &&
> + 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 :0: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);
>