Re: [PATCH 4/4] checkout: prevent losing staged changes with --merge

[Elijah Newren <newren@gmail.com>]

On Fri, Mar 22, 2019 at 2:32 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> 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@mail.gmail.com
>
> Reported-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  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.