{"thread":{"id":"50803","subject":"[PATCH 0/4] prevent 'checkout -m' from losing staged changes","startedAt":"2019-03-22T09:32:07Z","lastAt":"2019-03-25T16:07:43Z","messageCount":7,"participants":["Nguyễn Thái Ngọc Duy","Philip Oakley","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"372214","messageId":"20190322093138.13765-1-pclouds@gmail.com","threadId":"50803","inReplyTo":null,"subject":"[PATCH 0/4] prevent 'checkout -m' from losing staged changes","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-03-22T09:31:34Z","receivedAt":"2019-03-22T09:32:07Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"During the 'git switch' discusion, Phillip found out that staged changes\ncould be silently lost when doing 'git checkout -m'. The first attempt\n[2] tries to fix it by warning and moving on, because t7201.10 would\nfail if we simply die() when there are staged changes.\n\nThis round, I update read-tree to be used in that test instead of\n'checkout -m'. Then we are able to die().\n\nI start a new thread to keep spam level down for other people, and to\nbreak free from 'git switch' thread because it's unrelated now.\n\nJonathan is included, just in case he still remembers something from\n6a143aa2b2 (checkout -m: attempt merge when deletion of path was\nstaged, 2014-08-12)\n\n[1] https://public-inbox.org/git/7d3742d6-73e4-2750-6ecb-9edf761d96dd@gmail.com/\n[2] https://public-inbox.org/git/20190319093910.20229-1-pclouds@gmail.com/\n\nNguyễn Thái Ngọc Duy (4):\n  unpack-trees: keep gently check inside add_rejected_path\n  unpack-trees: rename \"gently\" flag to \"quiet\"\n  read-tree: add --quiet\n  checkout: prevent losing staged changes with --merge\n\n Documentation/git-read-tree.txt |  4 ++++\n builtin/checkout.c              | 13 +++++++++++--\n builtin/read-tree.c             |  1 +\n t/t7201-co.sh                   |  9 ++-------\n unpack-trees.c                  | 25 +++++++++++--------------\n unpack-trees.h                  |  2 +-\n 6 files changed, 30 insertions(+), 24 deletions(-)\n\n-- \n2.21.0.548.gd3c7d92dc2\n\n"},{"id":"372215","messageId":"20190322093138.13765-2-pclouds@gmail.com","threadId":"50803","inReplyTo":"20190322093138.13765-1-pclouds@gmail.com","subject":"[PATCH 1/4] unpack-trees: keep gently check inside add_rejected_path","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-03-22T09:31:35Z","receivedAt":"2019-03-22T09:32:13Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This basically follows the footsteps of 6a143aa2b2 (checkout -m:\nattempt merge when deletion of path was staged - 2014-08-12) where\nthere gently check is moved inside reject_merge() so that callers do\nnot accidentally forget it.\n\nadd_rejected_path() has the same usage pattern. All call sites check\ngently first, then decide to call add_rejected_path() if needed. Move\nthe check inside.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n unpack-trees.c | 21 +++++++++------------\n 1 file changed, 9 insertions(+), 12 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 22c41a3ba8..e6c1cc8302 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -219,6 +219,9 @@ static int add_rejected_path(struct unpack_trees_options *o,\n \t\t\t     enum unpack_trees_error_types e,\n \t\t\t     const char *path)\n {\n+\tif (o->gently)\n+\t\treturn -1;\n+\n \tif (!o->show_all_errors)\n \t\treturn error(ERRORMSG(o, e), super_prefixed(path));\n \n@@ -268,8 +271,7 @@ static int check_submodule_move_head(const struct cache_entry *ce,\n \t\tflags |= SUBMODULE_MOVE_HEAD_FORCE;\n \n \tif (submodule_move_head(ce->name, old_id, new_id, flags))\n-\t\treturn o->gently ? -1 :\n-\t\t\t\t   add_rejected_path(o, ERROR_WOULD_LOSE_SUBMODULE, ce->name);\n+\t\treturn add_rejected_path(o, ERROR_WOULD_LOSE_SUBMODULE, ce->name);\n \treturn 0;\n }\n \n@@ -1645,8 +1647,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n static int reject_merge(const struct cache_entry *ce,\n \t\t\tstruct unpack_trees_options *o)\n {\n-\treturn o->gently ? -1 :\n-\t\tadd_rejected_path(o, ERROR_WOULD_OVERWRITE, ce->name);\n+\treturn add_rejected_path(o, ERROR_WOULD_OVERWRITE, ce->name);\n }\n \n static int same(const struct cache_entry *a, const struct cache_entry *b)\n@@ -1693,8 +1694,7 @@ static int verify_uptodate_1(const struct cache_entry *ce,\n \t\t\tint r = check_submodule_move_head(ce,\n \t\t\t\t\"HEAD\", oid_to_hex(&ce->oid), o);\n \t\t\tif (r)\n-\t\t\t\treturn o->gently ? -1 :\n-\t\t\t\t\tadd_rejected_path(o, error_type, ce->name);\n+\t\t\t\treturn add_rejected_path(o, error_type, ce->name);\n \t\t\treturn 0;\n \t\t}\n \n@@ -1712,8 +1712,7 @@ static int verify_uptodate_1(const struct cache_entry *ce,\n \t}\n \tif (errno == ENOENT)\n \t\treturn 0;\n-\treturn o->gently ? -1 :\n-\t\tadd_rejected_path(o, error_type, ce->name);\n+\treturn add_rejected_path(o, error_type, ce->name);\n }\n \n int verify_uptodate(const struct cache_entry *ce,\n@@ -1835,8 +1834,7 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,\n \t\td.exclude_per_dir = o->dir->exclude_per_dir;\n \ti = read_directory(&d, o->src_index, pathbuf, namelen+1, NULL);\n \tif (i)\n-\t\treturn o->gently ? -1 :\n-\t\t\tadd_rejected_path(o, ERROR_NOT_UPTODATE_DIR, ce->name);\n+\t\treturn add_rejected_path(o, ERROR_NOT_UPTODATE_DIR, ce->name);\n \tfree(pathbuf);\n \treturn cnt;\n }\n@@ -1905,8 +1903,7 @@ static int check_ok_to_remove(const char *name, int len, int dtype,\n \t\t\treturn 0;\n \t}\n \n-\treturn o->gently ? -1 :\n-\t\tadd_rejected_path(o, error_type, name);\n+\treturn add_rejected_path(o, error_type, name);\n }\n \n /*\n-- \n2.21.0.548.gd3c7d92dc2\n\n"},{"id":"372216","messageId":"20190322093138.13765-3-pclouds@gmail.com","threadId":"50803","inReplyTo":"20190322093138.13765-1-pclouds@gmail.com","subject":"[PATCH 2/4] unpack-trees: rename \"gently\" flag to \"quiet\"","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-03-22T09:31:36Z","receivedAt":"2019-03-22T09:32:19Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The gently flag was added in 17e4642667 (Add flag to make unpack_trees()\nnot print errors. - 2008-02-07) to suppress error messages. The name\n\"gently\" does not quite express that. Granted, being quiet is gentle but\nit could mean not performing some other actions. Rename the flag to\n\"quiet\" to be more on point.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/checkout.c | 2 +-\n unpack-trees.c     | 6 +++---\n unpack-trees.h     | 2 +-\n 3 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 0e6037b296..22fb6c0cae 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -700,7 +700,7 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\ttopts.initial_checkout = is_cache_unborn();\n \t\ttopts.update = 1;\n \t\ttopts.merge = 1;\n-\t\ttopts.gently = opts->merge && old_branch_info->commit;\n+\t\ttopts.quiet = opts->merge && old_branch_info->commit;\n \t\ttopts.verbose_update = opts->show_progress;\n \t\ttopts.fn = twoway_merge;\n \t\tif (opts->overwrite_ignore) {\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex e6c1cc8302..2e5d7b202e 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -219,7 +219,7 @@ static int add_rejected_path(struct unpack_trees_options *o,\n \t\t\t     enum unpack_trees_error_types e,\n \t\t\t     const char *path)\n {\n-\tif (o->gently)\n+\tif (o->quiet)\n \t\treturn -1;\n \n \tif (!o->show_all_errors)\n@@ -1042,7 +1042,7 @@ static int unpack_nondirectories(int n, unsigned long mask,\n static int unpack_failed(struct unpack_trees_options *o, const char *message)\n {\n \tdiscard_index(&o->result);\n-\tif (!o->gently && !o->exiting_early) {\n+\tif (!o->quiet && !o->exiting_early) {\n \t\tif (message)\n \t\t\treturn error(\"%s\", message);\n \t\treturn -1;\n@@ -2343,7 +2343,7 @@ int bind_merge(const struct cache_entry * const *src,\n \t\treturn error(\"Cannot do a bind merge of %d trees\",\n \t\t\t     o->merge_size);\n \tif (a && old)\n-\t\treturn o->gently ? -1 :\n+\t\treturn o->quiet ? -1 :\n \t\t\terror(ERRORMSG(o, ERROR_BIND_OVERLAP),\n \t\t\t      super_prefixed(a->name),\n \t\t\t      super_prefixed(old->name));\ndiff --git a/unpack-trees.h b/unpack-trees.h\nindex 0135080a7b..d344d7d296 100644\n--- a/unpack-trees.h\n+++ b/unpack-trees.h\n@@ -56,7 +56,7 @@ struct unpack_trees_options {\n \t\t     diff_index_cached,\n \t\t     debug_unpack,\n \t\t     skip_sparse_checkout,\n-\t\t     gently,\n+\t\t     quiet,\n \t\t     exiting_early,\n \t\t     show_all_errors,\n \t\t     dry_run;\n-- \n2.21.0.548.gd3c7d92dc2\n\n"},{"id":"372217","messageId":"20190322093138.13765-4-pclouds@gmail.com","threadId":"50803","inReplyTo":"20190322093138.13765-1-pclouds@gmail.com","subject":"[PATCH 3/4] read-tree: add --quiet","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-03-22T09:31:37Z","receivedAt":"2019-03-22T09:32:24Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"read-tree is basically the front end of unpack-trees code and shoud\nexpose all of its functionality (unless it's designed for internal\nuse). This \"opts.quiet\" (formerly \"opts.gently\") was added for\nbuiltin/checkout.c but there is no reason why other read-tree users\nwon't find this useful.\n\nThe test that is updated to run 'read-tree --quiet' was added because\nunpack-trees was accidentally not being quiet [1] in 6a143aa2b2\n(checkout -m: attempt merge when deletion of path was staged -\n2014-08-12). Because checkout is the only \"opts.quiet\" user, there was\nno other way to test quiet behavior. But we can now test it directly.\n\n6a143aa2b2 was manually reverted to verify that read-tree --quiet\nworks correctly (i.e. test_must_be_empty fails).\n\n[1] the commit message there say \"errors out instead of performing a\n    merge\" but I'm pretty sure the \"performing a merge\" happens anyway\n    even before that commit. That line should say \"errors out\n    _in addition to_ performing a merge\"\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n Documentation/git-read-tree.txt | 4 ++++\n builtin/read-tree.c             | 1 +\n t/t7201-co.sh                   | 3 +++\n 3 files changed, 8 insertions(+)\n\ndiff --git a/Documentation/git-read-tree.txt b/Documentation/git-read-tree.txt\nindex 5c70bc2878..1e81f9c4e6 100644\n--- a/Documentation/git-read-tree.txt\n+++ b/Documentation/git-read-tree.txt\n@@ -128,6 +128,10 @@ OPTIONS\n \tInstead of reading tree object(s) into the index, just empty\n \tit.\n \n+-q::\n+--quiet::\n+\tQuiet, suppress feedback messages.\n+\n <tree-ish#>::\n \tThe id of the tree object(s) to be read/merged.\n \ndiff --git a/builtin/read-tree.c b/builtin/read-tree.c\nindex 9083dcfa28..5c9c082595 100644\n--- a/builtin/read-tree.c\n+++ b/builtin/read-tree.c\n@@ -154,6 +154,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n \t\t{ OPTION_CALLBACK, 0, \"recurse-submodules\", NULL,\n \t\t\t    \"checkout\", \"control recursive updating of submodules\",\n \t\t\t    PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater },\n+\t\tOPT__QUIET(&opts.quiet, N_(\"suppress feedback messages\")),\n \t\tOPT_END()\n \t};\n \ndiff --git a/t/t7201-co.sh b/t/t7201-co.sh\nindex 72b9b375ba..f165582019 100755\n--- a/t/t7201-co.sh\n+++ b/t/t7201-co.sh\n@@ -223,6 +223,9 @@ test_expect_success 'switch to another branch while carrying a deletion' '\n \ttest_must_fail git checkout simple 2>errs &&\n \ttest_i18ngrep overwritten errs &&\n \n+\ttest_must_fail git read-tree --quiet -m -u HEAD simple 2>errs &&\n+\ttest_must_be_empty errs &&\n+\n \tgit checkout --merge simple 2>errs &&\n \ttest_i18ngrep ! overwritten errs &&\n \tgit ls-files -u &&\n-- \n2.21.0.548.gd3c7d92dc2\n\n"},{"id":"372218","messageId":"20190322093138.13765-5-pclouds@gmail.com","threadId":"50803","inReplyTo":"20190322093138.13765-1-pclouds@gmail.com","subject":"[PATCH 4/4] checkout: prevent losing staged changes with --merge","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-03-22T09:31:38Z","receivedAt":"2019-03-22T09:32:30Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"When --merge is specified, we may need to do a real merge (instead of\nthree-way tree unpacking), the steps are best seen in git-checkout.sh\nversion before it's removed:\n\n    # Match the index to the working tree, and do a three-way.\n    git diff-files --name-only | git update-index --remove --stdin &&\n    work=`git write-tree` &&\n    git read-tree $v --reset -u $new || exit\n\n    git merge-recursive $old -- $new $work\n\n    # Do not register the cleanly merged paths in the index yet.\n    # this is not a real merge before committing, but just carrying\n    # the working tree changes along.\n    unmerged=`git ls-files -u`\n    git read-tree $v --reset $new\n    case \"$unmerged\" in\n    '')     ;;\n    *)\n            (\n                    z40=0000000000000000000000000000000000000000\n                    echo \"$unmerged\" |\n                    sed -e 's/^[0-7]* [0-9a-f]* /'\"0 $z40 /\"\n                    echo \"$unmerged\"\n            ) | git update-index --index-info\n            ;;\n    esac\n\nNotice the last 'read-tree --reset' step. We restore worktree back to\n'new' tree after worktree's messed up by merge-recursive. If there are\nstaged changes before this whole command sequence is executed, they\nare lost because they are unlikely part of the 'new' tree to be\nrestored.\n\nThere is no easy way to fix this. Elijah may have something up his\nsleeves [1], but until then, check if there are staged changes and\nrefuse to run and lose them. The user would need to do \"git reset\" to\ncontinue in this case.\n\nA note about the test update. 'checkout -m' in that test will fail\nbecause a deletion is staged. This 'checkout -m' was previously needed\nto verify quietness behavior of unpack-trees. But a different check\nhas been put in place in the last patch. We can safely drop\n'checkout -m' now.\n\n[1] CABPp-BFoL_U=bzON4SEMaQSKU2TKwnOgNqjt5MUaOejTKGUJxw@mail.gmail.com\n\nReported-by: Phillip Wood <phillip.wood@dunelm.org.uk>\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/checkout.c | 11 ++++++++++-\n t/t7201-co.sh      | 10 +---------\n 2 files changed, 11 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 22fb6c0cae..7cd01f62be 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -725,7 +725,10 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\t\t */\n \t\t\tstruct tree *result;\n \t\t\tstruct tree *work;\n+\t\t\tstruct tree *old_tree;\n \t\t\tstruct merge_options o;\n+\t\t\tstruct strbuf sb = STRBUF_INIT;\n+\n \t\t\tif (!opts->merge)\n \t\t\t\treturn 1;\n \n@@ -735,6 +738,12 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\t\t */\n \t\t\tif (!old_branch_info->commit)\n \t\t\t\treturn 1;\n+\t\t\told_tree = get_commit_tree(old_branch_info->commit);\n+\n+\t\t\tif (repo_index_has_changes(the_repository, old_tree, &sb))\n+\t\t\t\tdie(_(\"cannot continue with staged changes in \"\n+\t\t\t\t      \"the following files:\\n%s\"), sb.buf);\n+\t\t\tstrbuf_release(&sb);\n \n \t\t\t/* Do more real merge */\n \n@@ -772,7 +781,7 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\t\tret = merge_trees(&o,\n \t\t\t\t\t  get_commit_tree(new_branch_info->commit),\n \t\t\t\t\t  work,\n-\t\t\t\t\t  get_commit_tree(old_branch_info->commit),\n+\t\t\t\t\t  old_tree,\n \t\t\t\t\t  &result);\n \t\t\tif (ret < 0)\n \t\t\t\texit(128);\ndiff --git a/t/t7201-co.sh b/t/t7201-co.sh\nindex f165582019..5990299fc9 100755\n--- a/t/t7201-co.sh\n+++ b/t/t7201-co.sh\n@@ -224,15 +224,7 @@ test_expect_success 'switch to another branch while carrying a deletion' '\n \ttest_i18ngrep overwritten errs &&\n \n \ttest_must_fail git read-tree --quiet -m -u HEAD simple 2>errs &&\n-\ttest_must_be_empty errs &&\n-\n-\tgit checkout --merge simple 2>errs &&\n-\ttest_i18ngrep ! overwritten errs &&\n-\tgit ls-files -u &&\n-\ttest_must_fail git cat-file -t :0:two &&\n-\ttest \"$(git cat-file -t :1:two)\" = blob &&\n-\ttest \"$(git cat-file -t :2:two)\" = blob &&\n-\ttest_must_fail git cat-file -t :3:two\n+\ttest_must_be_empty errs\n '\n \n test_expect_success 'checkout to detach HEAD (with advice declined)' '\n-- \n2.21.0.548.gd3c7d92dc2\n\n"},{"id":"372259","messageId":"04d888be-d66b-110a-b1e4-35ff4897fcab@iee.org","threadId":"50803","inReplyTo":"20190322093138.13765-4-pclouds@gmail.com","subject":"Re: [PATCH 3/4] read-tree: add --quiet","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2019-03-22T17:18:44Z","receivedAt":"2019-03-22T17:18:48Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"On 22/03/2019 09:31, Nguyễn Thái Ngọc Duy wrote:\n> read-tree is basically the front end of unpack-trees code and shoud\n\ns/shoud/should/\n\nPhilip\n\n> expose all of its functionality (unless it's designed for internal\n> use). This \"opts.quiet\" (formerly \"opts.gently\") was added for\n> builtin/checkout.c but there is no reason why other read-tree users\n> won't find this useful.\n>\n> The test that is updated to run 'read-tree --quiet' was added because\n> unpack-trees was accidentally not being quiet [1] in 6a143aa2b2\n> (checkout -m: attempt merge when deletion of path was staged -\n> 2014-08-12). Because checkout is the only \"opts.quiet\" user, there was\n> no other way to test quiet behavior. But we can now test it directly.\n>\n> 6a143aa2b2 was manually reverted to verify that read-tree --quiet\n> works correctly (i.e. test_must_be_empty fails).\n>\n> [1] the commit message there say \"errors out instead of performing a\n>      merge\" but I'm pretty sure the \"performing a merge\" happens anyway\n>      even before that commit. That line should say \"errors out\n>      _in addition to_ performing a merge\"\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>   Documentation/git-read-tree.txt | 4 ++++\n>   builtin/read-tree.c             | 1 +\n>   t/t7201-co.sh                   | 3 +++\n>   3 files changed, 8 insertions(+)\n>\n> diff --git a/Documentation/git-read-tree.txt b/Documentation/git-read-tree.txt\n> index 5c70bc2878..1e81f9c4e6 100644\n> --- a/Documentation/git-read-tree.txt\n> +++ b/Documentation/git-read-tree.txt\n> @@ -128,6 +128,10 @@ OPTIONS\n>   \tInstead of reading tree object(s) into the index, just empty\n>   \tit.\n>   \n> +-q::\n> +--quiet::\n> +\tQuiet, suppress feedback messages.\n> +\n>   <tree-ish#>::\n>   \tThe id of the tree object(s) to be read/merged.\n>   \n> diff --git a/builtin/read-tree.c b/builtin/read-tree.c\n> index 9083dcfa28..5c9c082595 100644\n> --- a/builtin/read-tree.c\n> +++ b/builtin/read-tree.c\n> @@ -154,6 +154,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n>   \t\t{ OPTION_CALLBACK, 0, \"recurse-submodules\", NULL,\n>   \t\t\t    \"checkout\", \"control recursive updating of submodules\",\n>   \t\t\t    PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater },\n> +\t\tOPT__QUIET(&opts.quiet, N_(\"suppress feedback messages\")),\n>   \t\tOPT_END()\n>   \t};\n>   \n> diff --git a/t/t7201-co.sh b/t/t7201-co.sh\n> index 72b9b375ba..f165582019 100755\n> --- a/t/t7201-co.sh\n> +++ b/t/t7201-co.sh\n> @@ -223,6 +223,9 @@ test_expect_success 'switch to another branch while carrying a deletion' '\n>   \ttest_must_fail git checkout simple 2>errs &&\n>   \ttest_i18ngrep overwritten errs &&\n>   \n> +\ttest_must_fail git read-tree --quiet -m -u HEAD simple 2>errs &&\n> +\ttest_must_be_empty errs &&\n> +\n>   \tgit checkout --merge simple 2>errs &&\n>   \ttest_i18ngrep ! overwritten errs &&\n>   \tgit ls-files -u &&\n"},{"id":"372438","messageId":"CABPp-BGcfEuMCcRvALWHO26frj3GRO_Wo620YgXwgriLSMggJA@mail.gmail.com","threadId":"50803","inReplyTo":"20190322093138.13765-5-pclouds@gmail.com","subject":"Re: [PATCH 4/4] checkout: prevent losing staged changes with --merge","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-03-25T16:07:28Z","receivedAt":"2019-03-25T16:07:43Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Mar 22, 2019 at 2:32 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:\n>\n> When --merge is specified, we may need to do a real merge (instead of\n> three-way tree unpacking), the steps are best seen in git-checkout.sh\n> version before it's removed:\n>\n>     # Match the index to the working tree, and do a three-way.\n>     git diff-files --name-only | git update-index --remove --stdin &&\n>     work=`git write-tree` &&\n>     git read-tree $v --reset -u $new || exit\n>\n>     git merge-recursive $old -- $new $work\n>\n>     # Do not register the cleanly merged paths in the index yet.\n>     # this is not a real merge before committing, but just carrying\n>     # the working tree changes along.\n>     unmerged=`git ls-files -u`\n>     git read-tree $v --reset $new\n>     case \"$unmerged\" in\n>     '')     ;;\n>     *)\n>             (\n>                     z40=0000000000000000000000000000000000000000\n>                     echo \"$unmerged\" |\n>                     sed -e 's/^[0-7]* [0-9a-f]* /'\"0 $z40 /\"\n>                     echo \"$unmerged\"\n>             ) | git update-index --index-info\n>             ;;\n>     esac\n>\n> Notice the last 'read-tree --reset' step. We restore worktree back to\n> 'new' tree after worktree's messed up by merge-recursive. If there are\n> staged changes before this whole command sequence is executed, they\n> are lost because they are unlikely part of the 'new' tree to be\n> restored.\n>\n> There is no easy way to fix this. Elijah may have something up his\n> sleeves [1], but until then, check if there are staged changes and\n> refuse to run and lose them. The user would need to do \"git reset\" to\n> continue in this case.\n>\n> A note about the test update. 'checkout -m' in that test will fail\n> because a deletion is staged. This 'checkout -m' was previously needed\n> to verify quietness behavior of unpack-trees. But a different check\n> has been put in place in the last patch. We can safely drop\n> 'checkout -m' now.\n>\n> [1] CABPp-BFoL_U=bzON4SEMaQSKU2TKwnOgNqjt5MUaOejTKGUJxw@mail.gmail.com\n>\n> Reported-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  builtin/checkout.c | 11 ++++++++++-\n>  t/t7201-co.sh      | 10 +---------\n>  2 files changed, 11 insertions(+), 10 deletions(-)\n>\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 22fb6c0cae..7cd01f62be 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -725,7 +725,10 @@ static int merge_working_tree(const struct checkout_opts *opts,\n>                          */\n>                         struct tree *result;\n>                         struct tree *work;\n> +                       struct tree *old_tree;\n>                         struct merge_options o;\n> +                       struct strbuf sb = STRBUF_INIT;\n> +\n>                         if (!opts->merge)\n>                                 return 1;\n>\n> @@ -735,6 +738,12 @@ static int merge_working_tree(const struct checkout_opts *opts,\n>                          */\n>                         if (!old_branch_info->commit)\n>                                 return 1;\n> +                       old_tree = get_commit_tree(old_branch_info->commit);\n> +\n> +                       if (repo_index_has_changes(the_repository, old_tree, &sb))\n> +                               die(_(\"cannot continue with staged changes in \"\n> +                                     \"the following files:\\n%s\"), sb.buf);\n> +                       strbuf_release(&sb);\n>\n>                         /* Do more real merge */\n>\n> @@ -772,7 +781,7 @@ static int merge_working_tree(const struct checkout_opts *opts,\n>                         ret = merge_trees(&o,\n>                                           get_commit_tree(new_branch_info->commit),\n>                                           work,\n> -                                         get_commit_tree(old_branch_info->commit),\n> +                                         old_tree,\n>                                           &result);\n>                         if (ret < 0)\n>                                 exit(128);\n> diff --git a/t/t7201-co.sh b/t/t7201-co.sh\n> index f165582019..5990299fc9 100755\n> --- a/t/t7201-co.sh\n> +++ b/t/t7201-co.sh\n> @@ -224,15 +224,7 @@ test_expect_success 'switch to another branch while carrying a deletion' '\n>         test_i18ngrep overwritten errs &&\n>\n>         test_must_fail git read-tree --quiet -m -u HEAD simple 2>errs &&\n> -       test_must_be_empty errs &&\n> -\n> -       git checkout --merge simple 2>errs &&\n> -       test_i18ngrep ! overwritten errs &&\n> -       git ls-files -u &&\n> -       test_must_fail git cat-file -t :0:two &&\n> -       test \"$(git cat-file -t :1:two)\" = blob &&\n> -       test \"$(git cat-file -t :2:two)\" = blob &&\n> -       test_must_fail git cat-file -t :3:two\n> +       test_must_be_empty errs\n>  '\n\nAh, I see you avoid the other bug in checkout by just removing its\nusage.  Seems fair enough; I can add a separate test to demonstrate\nthat bug when I get some time to work on it.  Hopefully I'll find a\nquick fix too.\n"}]}