On Fri, Mar 22, 2019 at 2:32 AM Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> wrote:
>
> When --merge is specified, we may need to do a real merge (instead of
> three-way tree unpacking), the steps are best seen in git-checkout.sh
> version before it's removed:
>
> # Match the index to the working tree, and do a three-way.
> git diff-files --name-only | git update-index --remove --stdin &&
> work=`git write-tree` &&
> git read-tree $v --reset -u $new || exit
>
> git merge-recursive $old -- $new $work
>
> # Do not register the cleanly merged paths in the index yet.
> # this is not a real merge before committing, but just carrying
> # the working tree changes along.
> unmerged=`git ls-files -u`
> git read-tree $v --reset $new
> case "$unmerged" in
> '') ;;
> *)
> (
> z40=0000000000000000000000000000000000000000
> echo "$unmerged" |
> sed -e 's/^[0-7]* [0-9a-f]* /'"0 $z40 /"
> echo "$unmerged"
> ) | git update-index --index-info
> ;;
> esac
>
> Notice the last 'read-tree --reset' step. We restore worktree back to
> 'new' tree after worktree's messed up by merge-recursive. If there are
> staged changes before this whole command sequence is executed, they
> are lost because they are unlikely part of the 'new' tree to be
> restored.
>
> There is no easy way to fix this. Elijah may have something up his
> sleeves [1], but until then, check if there are staged changes and
> refuse to run and lose them. The user would need to do "git reset" to
> continue in this case.
>
> A note about the test update. 'checkout -m' in that test will fail
> because a deletion is staged. This 'checkout -m' was previously needed
> to verify quietness behavior of unpack-trees. But a different check
> has been put in place in the last patch. We can safely drop
> 'checkout -m' now.
>
> [1] CABPp-BFoL_U=bzON4SEMaQSKU2TKwnOgNqjt5MUaOejTKGUJxw@xxxxxxxxxxxxxx
>
> Reported-by: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx>
> ---
> builtin/checkout.c | 11 ++++++++++-
> t/t7201-co.sh | 10 +---------
> 2 files changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 22fb6c0cae..7cd01f62be 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -725,7 +725,10 @@ static int merge_working_tree(const struct checkout_opts *opts,
> */
> struct tree *result;
> struct tree *work;
> + struct tree *old_tree;
> struct merge_options o;
> + struct strbuf sb = STRBUF_INIT;
> +
> if (!opts->merge)
> return 1;
>
> @@ -735,6 +738,12 @@ static int merge_working_tree(const struct checkout_opts *opts,
> */
> if (!old_branch_info->commit)
> return 1;
> + old_tree = get_commit_tree(old_branch_info->commit);
> +
> + if (repo_index_has_changes(the_repository, old_tree, &sb))
> + die(_("cannot continue with staged changes in "
> + "the following files:\n%s"), sb.buf);
> + strbuf_release(&sb);
>
> /* Do more real merge */
>
> @@ -772,7 +781,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
> ret = merge_trees(&o,
> get_commit_tree(new_branch_info->commit),
> work,
> - get_commit_tree(old_branch_info->commit),
> + old_tree,
> &result);
> if (ret < 0)
> exit(128);
> diff --git a/t/t7201-co.sh b/t/t7201-co.sh
> index f165582019..5990299fc9 100755
> --- a/t/t7201-co.sh
> +++ b/t/t7201-co.sh
> @@ -224,15 +224,7 @@ test_expect_success 'switch to another branch while carrying a deletion' '
> test_i18ngrep overwritten errs &&
>
> test_must_fail git read-tree --quiet -m -u HEAD simple 2>errs &&
> - test_must_be_empty errs &&
> -
> - git checkout --merge simple 2>errs &&
> - test_i18ngrep ! overwritten errs &&
> - git ls-files -u &&
> - test_must_fail git cat-file -t :0:two &&
> - test "$(git cat-file -t :1:two)" = blob &&
> - test "$(git cat-file -t :2:two)" = blob &&
> - test_must_fail git cat-file -t :3:two
> + test_must_be_empty errs
> '
Ah, I see you avoid the other bug in checkout by just removing its
usage. Seems fair enough; I can add a separate test to demonstrate
that bug when I get some time to work on it. Hopefully I'll find a
quick fix too.
