{"thread":{"id":"51019","subject":"[PATCH 0/2] read-tree: improve untracked file support","startedAt":"2019-05-01T10:14:20Z","lastAt":"2019-05-07T11:02:43Z","messageCount":12,"participants":["Phillip Wood","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"374745","messageId":"20190501101403.20294-1-phillip.wood123@gmail.com","threadId":"51019","inReplyTo":null,"subject":"[PATCH 0/2] read-tree: improve untracked file support","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-05-01T10:14:01Z","receivedAt":"2019-05-01T10:14:20Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nThese two patches teach read-tree how to avoid overwriting untracked\nfiles when doing '--reset -u' and also how to respect all of git's\nstandard excludes files. I'd like to see the porcelain commands stop\noverwriting untracked files, this is a first step on the way. I'm not\nsure if we want to add options to the porcelain commands to protect\nuntracked files or just change their behavior and add an option to\noverride that. I'm leaning towards the latter but I'd be interested to\nhear what others think.\n\nPhillip Wood (2):\n  read-tree --reset: add --protect-untracked\n  read-tree: add --exclude-standard\n\n Documentation/git-read-tree.txt | 19 ++++++++--\n builtin/am.c                    |  8 +++--\n builtin/checkout.c              |  2 +-\n builtin/read-tree.c             | 61 +++++++++++++++++++++++++++++--\n builtin/rebase.c                |  2 +-\n builtin/reset.c                 |  2 +-\n builtin/stash.c                 |  7 ++--\n t/lib-read-tree.sh              | 11 ++++++\n t/t1005-read-tree-reset.sh      | 63 +++++++++++++++++++++++++++++++--\n t/t1013-read-tree-submodule.sh  |  3 +-\n unpack-trees.c                  |  3 +-\n unpack-trees.h                  | 10 ++++--\n 12 files changed, 170 insertions(+), 21 deletions(-)\n\n-- \n2.21.0\n\n"},{"id":"374746","messageId":"20190501101403.20294-2-phillip.wood123@gmail.com","threadId":"51019","inReplyTo":"20190501101403.20294-1-phillip.wood123@gmail.com","subject":"[PATCH 1/2] read-tree --reset: add --protect-untracked","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-05-01T10:14:02Z","receivedAt":"2019-05-01T10:14:22Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nCurrently there is no way to get git to discard changes to the\nworktree without overwriting untracked files. `reset --hard`,\n`checkout --force`, `checkout <rev> :/` and `read-tree --reset -u`\nwill all overwrite untracked files. unpack_trees() has known how to\navoid overwriting untracked files since commit fcc387db9b (\"read-tree\n-m -u: do not overwrite or remove untracked working tree files.\",\n2006-05-17) in response to concerns about lost files [1] but those\nprotections do not apply to --reset. This patch adds an\n--protect-untracked option for read-tree/unpack_trees() to apply the\nsame protections with --reset. It does not change the behavior of any\nof the porcelain commands but paves the way for adding options or\nchanging the default for those in future.\n\nNote the actual change in unpack-trees.c is quite simple, the majority\nof this patch is converting existing callers to use the new\nunpack_trees_reset_type enum.\n\n[1] https://public-inbox.org/git/8aa486160605161500m1dd8428cj@mail.gmail.com/\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n\nNotes:\n    adding --protect-untracked to the invocation of\n    test_submodule_forced_switch() in t1013 fixes the known breakage of\n    tests 57 & 58 but breaks test 64 which relies on forcing. I'm not sure\n    that the expected results of 57 & 58 are correct if we're forcing.\n\n Documentation/git-read-tree.txt | 10 +++++++--\n builtin/am.c                    |  8 ++++---\n builtin/checkout.c              |  2 +-\n builtin/read-tree.c             | 12 +++++++++++\n builtin/rebase.c                |  2 +-\n builtin/reset.c                 |  2 +-\n builtin/stash.c                 |  7 ++++---\n t/lib-read-tree.sh              | 11 ++++++++++\n t/t1005-read-tree-reset.sh      | 37 +++++++++++++++++++++++++++++++--\n unpack-trees.c                  |  3 ++-\n unpack-trees.h                  | 10 +++++++--\n 11 files changed, 88 insertions(+), 16 deletions(-)\n\ndiff --git a/Documentation/git-read-tree.txt b/Documentation/git-read-tree.txt\nindex d271842608..67864c6bbc 100644\n--- a/Documentation/git-read-tree.txt\n+++ b/Documentation/git-read-tree.txt\n@@ -40,12 +40,18 @@ OPTIONS\n --reset::\n \tSame as -m, except that unmerged entries are discarded instead\n \tof failing. When used with `-u`, updates leading to loss of\n-\tworking tree changes will not abort the operation.\n+\tworking tree changes will not abort the operation and\n+\tuntracked files will be overwritten. Use `--protect-untracked`\n+\tto avoid overwriting untracked files.\n \n -u::\n \tAfter a successful merge, update the files in the work\n \ttree with the result of the merge.\n \n+--protect-untracked::\n+\tPrevent `--reset` `-u` from overwriting untracked files. Requires\n+\t`--reset` and `-u` (`-m` never overwrites untracked files).\n+\n -i::\n \tUsually a merge requires the index file as well as the\n \tfiles in the working tree to be up to date with the\n@@ -89,7 +95,7 @@ OPTIONS\n \texisted in the original index file.\n \n --exclude-per-directory=<gitignore>::\n-\tWhen running the command with `-u` and `-m` options, the\n+\tWhen using `-u` with `-m` or `--reset` `--protect-untracked` options, the\n \tmerge result may need to overwrite paths that are not\n \ttracked in the current branch.  The command usually\n \trefuses to proceed with the merge to avoid losing such a\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 912d9821b1..a92394b682 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1854,7 +1854,8 @@ static void am_resolve(struct am_state *state)\n  * true, any unmerged entries will be discarded. Returns 0 on success, -1 on\n  * failure.\n  */\n-static int fast_forward_to(struct tree *head, struct tree *remote, int reset)\n+static int fast_forward_to(struct tree *head, struct tree *remote,\n+\t\t\t   enum unpack_trees_reset_type reset)\n {\n \tstruct lock_file lock_file = LOCK_INIT;\n \tstruct unpack_trees_options opts;\n@@ -1942,7 +1943,8 @@ static int clean_index(const struct object_id *head, const struct object_id *rem\n \n \tread_cache_unmerged();\n \n-\tif (fast_forward_to(head_tree, head_tree, 1))\n+\tif (fast_forward_to(head_tree, head_tree,\n+\t\t\t    UNPACK_RESET_OVERWRITE_UNTRACKED))\n \t\treturn -1;\n \n \tif (write_cache_as_tree(&index, 0, NULL))\n@@ -1952,7 +1954,7 @@ static int clean_index(const struct object_id *head, const struct object_id *rem\n \tif (!index_tree)\n \t\treturn error(_(\"Could not parse object '%s'.\"), oid_to_hex(&index));\n \n-\tif (fast_forward_to(index_tree, remote_tree, 0))\n+\tif (fast_forward_to(index_tree, remote_tree, UNPACK_NO_RESET))\n \t\treturn -1;\n \n \tif (merge_tree(remote_tree))\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex ffa776c6e1..e9e70018f9 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -506,7 +506,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,\n \topts.head_idx = -1;\n \topts.update = worktree;\n \topts.skip_unmerged = !worktree;\n-\topts.reset = 1;\n+\topts.reset = UNPACK_RESET_OVERWRITE_UNTRACKED;\n \topts.merge = 1;\n \topts.fn = oneway_merge;\n \topts.verbose_update = o->show_progress;\ndiff --git a/builtin/read-tree.c b/builtin/read-tree.c\nindex 5c9c082595..23735adde9 100644\n--- a/builtin/read-tree.c\n+++ b/builtin/read-tree.c\n@@ -114,6 +114,7 @@ static int git_read_tree_config(const char *var, const char *value, void *cb)\n int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n {\n \tint i, stage = 0;\n+\tint protect_untracked = -1;\n \tstruct object_id oid;\n \tstruct tree_desc t[MAX_UNPACK_TREES];\n \tstruct unpack_trees_options opts;\n@@ -140,6 +141,8 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n \t\t  PARSE_OPT_NONEG },\n \t\tOPT_BOOL('u', NULL, &opts.update,\n \t\t\t N_(\"update working tree with merge result\")),\n+\t\tOPT_BOOL(0, \"protect-untracked\", &protect_untracked,\n+\t\t\t N_(\"do not overwrite untracked files\")),\n \t\t{ OPTION_CALLBACK, 0, \"exclude-per-directory\", &opts,\n \t\t  N_(\"gitignore\"),\n \t\t  N_(\"allow explicitly ignored files to be overwritten\"),\n@@ -209,8 +212,17 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n \tif ((opts.update || opts.index_only) && !opts.merge)\n \t\tdie(\"%s is meaningless without -m, --reset, or --prefix\",\n \t\t    opts.update ? \"-u\" : \"-i\");\n+\tif (protect_untracked >= 0) {\n+\t\tif (!opts.reset || !opts.update)\n+\t\t\tdie(\"-%s-protect-untracked requires --reset and -u\",\n+\t\t\t    protect_untracked ? \"\" : \"-no\");\n+\t\topts.reset = UNPACK_RESET_PROTECT_UNTRACKED;\n+\t}\n \tif ((opts.dir && !opts.update))\n \t\tdie(\"--exclude-per-directory is meaningless unless -u\");\n+\tif (opts.dir && opts.reset == UNPACK_RESET_OVERWRITE_UNTRACKED)\n+\t\twarning(\"--exclude-per-directory without --preserve-untracked \"\n+\t\t\t\"has no effect\");\n \tif (opts.merge && !opts.index_only)\n \t\tsetup_work_tree();\n \ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 2e41ad5644..feb30a71f5 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -398,7 +398,7 @@ static int reset_head(struct object_id *oid, const char *action,\n \tunpack_tree_opts.update = 1;\n \tunpack_tree_opts.merge = 1;\n \tif (!detach_head)\n-\t\tunpack_tree_opts.reset = 1;\n+\t\tunpack_tree_opts.reset = UNPACK_RESET_OVERWRITE_UNTRACKED;\n \n \tif (repo_read_index_unmerged(the_repository) < 0) {\n \t\tret = error(_(\"could not read index\"));\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 26ef9a7bd0..a39dd92fb2 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -70,7 +70,7 @@ static int reset_index(const struct object_id *oid, int reset_type, int quiet)\n \t\topts.update = 1;\n \t\t/* fallthrough */\n \tdefault:\n-\t\topts.reset = 1;\n+\t\topts.reset = UNPACK_RESET_OVERWRITE_UNTRACKED;\n \t}\n \n \tread_cache_unmerged();\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 2a8e6d09b4..175d1b62d3 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -227,7 +227,8 @@ static int clear_stash(int argc, const char **argv, const char *prefix)\n \treturn do_clear_stash();\n }\n \n-static int reset_tree(struct object_id *i_tree, int update, int reset)\n+static int reset_tree(struct object_id *i_tree, int update,\n+\t\t      enum unpack_trees_reset_type reset)\n {\n \tint nr_trees = 1;\n \tstruct unpack_trees_options opts;\n@@ -461,7 +462,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n \t}\n \n \tif (has_index) {\n-\t\tif (reset_tree(&index_tree, 0, 0))\n+\t\tif (reset_tree(&index_tree, 0, UNPACK_NO_RESET))\n \t\t\treturn -1;\n \t} else {\n \t\tstruct strbuf out = STRBUF_INIT;\n@@ -471,7 +472,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n \t\t\treturn -1;\n \t\t}\n \n-\t\tif (reset_tree(&c_tree, 0, 1)) {\n+\t\tif (reset_tree(&c_tree, 0, UNPACK_RESET_OVERWRITE_UNTRACKED)) {\n \t\t\tstrbuf_release(&out);\n \t\t\treturn -1;\n \t\t}\ndiff --git a/t/lib-read-tree.sh b/t/lib-read-tree.sh\nindex b95f485606..22f516d91f 100644\n--- a/t/lib-read-tree.sh\n+++ b/t/lib-read-tree.sh\n@@ -39,3 +39,14 @@ read_tree_u_must_fail () {\n \ttest_cmp pre-dry-run-wt post-dry-run-wt &&\n \ttest_must_fail git read-tree \"$@\"\n }\n+\n+read_tree_u_must_fail_save_err () {\n+\tgit ls-files -s >pre-dry-run &&\n+\tgit diff-files -p >pre-dry-run-wt &&\n+\ttest_must_fail git read-tree -n \"$@\" &&\n+\tgit ls-files -s >post-dry-run &&\n+\tgit diff-files -p >post-dry-run-wt &&\n+\ttest_cmp pre-dry-run post-dry-run &&\n+\ttest_cmp pre-dry-run-wt post-dry-run-wt &&\n+\ttest_must_fail git read-tree \"$@\" 2>actual-err\n+}\ndiff --git a/t/t1005-read-tree-reset.sh b/t/t1005-read-tree-reset.sh\nindex 83b09e1310..6c9dd6805b 100755\n--- a/t/t1005-read-tree-reset.sh\n+++ b/t/t1005-read-tree-reset.sh\n@@ -19,15 +19,48 @@ test_expect_success 'setup' '\n \tgit add df &&\n \techo content >new &&\n \tgit add new &&\n-\tgit commit -m two\n+\tgit commit -m two &&\n+\tgit ls-files >expect-two\n '\n \n-test_expect_success 'reset should work' '\n+test_expect_success '--protect-untracked option sanity checks' '\n+\tread_tree_u_must_fail --reset --protect-untracked HEAD &&\n+\tread_tree_u_must_fail --reset --no-protect-untracked HEAD &&\n+\tread_tree_u_must_fail -m -u --protect-untracked HEAD &&\n+\tread_tree_u_must_fail -m -u --no-protect-untracked\n+'\n+\n+test_expect_success 'reset should reset worktree' '\n+\techo changed >df &&\n \tread_tree_u_must_succeed -u --reset HEAD^ &&\n \tgit ls-files >actual &&\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'reset --protect-untracked protects untracked file' '\n+\techo changed >new &&\n+\tread_tree_u_must_fail_save_err -u --reset --protect-untracked HEAD &&\n+\techo \"error: Untracked working tree file '\\'new\\'' would be overwritten by merge.\" >expected-err &&\n+\ttest_cmp expected-err actual-err\n+'\n+\n+test_expect_success 'reset --protect-untracked protects untracked directory' '\n+\trm new &&\n+\tmkdir new &&\n+\techo untracked >new/untracked &&\n+\tread_tree_u_must_fail_save_err -u --reset --protect-untracked HEAD &&\n+\techo \"error: Updating '\\'new\\'' would lose untracked files in it\" >expected-err &&\n+\ttest_cmp expected-err actual-err\n+'\n+\n+test_expect_success 'reset --protect-untracked resets' '\n+\trm -rf new &&\n+\techo changed >df/file &&\n+\tread_tree_u_must_succeed -u --reset --protect-untracked HEAD &&\n+\tgit ls-files >actual-two &&\n+\ttest_cmp expect-two actual-two\n+'\n+\n test_expect_success 'reset should remove remnants from a failed merge' '\n \tread_tree_u_must_succeed --reset -u HEAD &&\n \tgit ls-files -s >expect &&\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 50189909b8..b1722730fe 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1917,7 +1917,8 @@ static int verify_absent_1(const struct cache_entry *ce,\n \tint len;\n \tstruct stat st;\n \n-\tif (o->index_only || o->reset || !o->update)\n+\tif (o->index_only || o->reset == UNPACK_RESET_OVERWRITE_UNTRACKED ||\n+\t    !o->update)\n \t\treturn 0;\n \n \tlen = check_leading_path(ce->name, ce_namelen(ce));\ndiff --git a/unpack-trees.h b/unpack-trees.h\nindex d344d7d296..732b262c4d 100644\n--- a/unpack-trees.h\n+++ b/unpack-trees.h\n@@ -41,9 +41,15 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,\n  */\n void clear_unpack_trees_porcelain(struct unpack_trees_options *opts);\n \n+enum unpack_trees_reset_type {\n+\tUNPACK_NO_RESET = 0,\n+\tUNPACK_RESET_OVERWRITE_UNTRACKED,\n+\tUNPACK_RESET_PROTECT_UNTRACKED\n+};\n+\n struct unpack_trees_options {\n-\tunsigned int reset,\n-\t\t     merge,\n+\tenum unpack_trees_reset_type reset;\n+\tunsigned int merge,\n \t\t     update,\n \t\t     clone,\n \t\t     index_only,\n-- \n2.21.0\n\n"},{"id":"374747","messageId":"20190501101403.20294-3-phillip.wood123@gmail.com","threadId":"51019","inReplyTo":"20190501101403.20294-1-phillip.wood123@gmail.com","subject":"[PATCH 2/2] read-tree: add --exclude-standard","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-05-01T10:14:03Z","receivedAt":"2019-05-01T10:14:24Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nCurrently there is no way to get read-tree to respect\n.git/info/exclude or core.excludesFile so scripts using `read-tree -u`\nhave subtly different behavior to porcelain commands like checkout\neven when they use --exclude-per-directory. This new option is copied\nfrom ls-tree's --exclude-standard option to setup the standard\nexcludes. The new option is also used to fix a known submodule test\nfailure.\n\nNote that KNOWN_FAILURE_SUBMODULE_OVERWRITE_IGNORED_UNTRACKED is still\nused by t7112-reset-submodule.sh as it is not removed (apparently\nreset does not call setup_standard_excludes()).\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n Documentation/git-read-tree.txt |  9 +++++-\n builtin/read-tree.c             | 55 ++++++++++++++++++++++++++++++---\n t/t1005-read-tree-reset.sh      | 36 ++++++++++++++++++---\n t/t1013-read-tree-submodule.sh  |  3 +-\n 4 files changed, 90 insertions(+), 13 deletions(-)\n\ndiff --git a/Documentation/git-read-tree.txt b/Documentation/git-read-tree.txt\nindex 67864c6bbc..a2b8b73a99 100644\n--- a/Documentation/git-read-tree.txt\n+++ b/Documentation/git-read-tree.txt\n@@ -107,7 +107,14 @@ OPTIONS\n \trunning `make clean` to remove the generated file.  This\n \toption tells the command to read per-directory exclude\n \tfile (usually '.gitignore') and allows such an untracked\n-\tbut explicitly ignored file to be overwritten.\n+\tbut explicitly ignored file to be overwritten. Incompatible\n+\twith `--exclude-standard`.\n+\n+--exclude-standard::\n+\tWhen updating the worktree use the standard Git exclusions:\n+\t.git/info/exclude, .gitignore in each directory, and the user's global\n+\texclusion file when deciding if it is safe to overwrite a file.\n+\tIncompatible with `--exclude-per-directory`.\n \n --index-output=<file>::\n \tInstead of writing the results out to `$GIT_INDEX_FILE`,\ndiff --git a/builtin/read-tree.c b/builtin/read-tree.c\nindex 23735adde9..5df493c4a7 100644\n--- a/builtin/read-tree.c\n+++ b/builtin/read-tree.c\n@@ -50,6 +50,40 @@ static int index_output_cb(const struct option *opt, const char *arg,\n \treturn 0;\n }\n \n+enum exclude_type {\n+\tEXCLUDE_NONE,\n+\tEXCLUDE_PER_DIRECTORY,\n+\tEXCLUDE_STANDARD\n+} exclude_opt = EXCLUDE_NONE;\n+\n+static int exclude_error(enum exclude_type exclude)\n+{\n+\tif (exclude == exclude_opt)\n+\t\treturn error(\"more than one --exclude-per-directory given\");\n+\telse\n+\t\treturn error(\"cannot combine --exclude-per-directory and \"\n+\t\t\t     \"--exclude-standard\");\n+}\n+\n+static int option_parse_exclude_standard(const struct option *opt,\n+\t\t\t\t\t const char *arg, int unset)\n+{\n+\tstruct unpack_trees_options *opts;\n+\n+\tBUG_ON_OPT_NEG(unset);\n+\tBUG_ON_OPT_ARG(arg);\n+\n+\tif (exclude_opt == EXCLUDE_PER_DIRECTORY)\n+\t\treturn exclude_error(EXCLUDE_STANDARD);\n+\n+\topts = (struct unpack_trees_options *)opt->value;\n+\topts->dir = xcalloc(1, sizeof(*opts->dir));\n+\tsetup_standard_excludes(opts->dir);\n+\texclude_opt = EXCLUDE_STANDARD;\n+\n+\treturn 0;\n+}\n+\n static int exclude_per_directory_cb(const struct option *opt, const char *arg,\n \t\t\t\t    int unset)\n {\n@@ -61,12 +95,13 @@ static int exclude_per_directory_cb(const struct option *opt, const char *arg,\n \topts = (struct unpack_trees_options *)opt->value;\n \n \tif (opts->dir)\n-\t\tdie(\"more than one --exclude-per-directory given.\");\n+\t\treturn exclude_error(EXCLUDE_PER_DIRECTORY);\n \n \tdir = xcalloc(1, sizeof(*opts->dir));\n \tdir->flags |= DIR_SHOW_IGNORED;\n \tdir->exclude_per_dir = arg;\n \topts->dir = dir;\n+\texclude_opt = EXCLUDE_PER_DIRECTORY;\n \t/* We do not need to nor want to do read-directory\n \t * here; we are merely interested in reusing the\n \t * per directory ignore stack mechanism.\n@@ -147,6 +182,10 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n \t\t  N_(\"gitignore\"),\n \t\t  N_(\"allow explicitly ignored files to be overwritten\"),\n \t\t  PARSE_OPT_NONEG, exclude_per_directory_cb },\n+\t\t{ OPTION_CALLBACK, 0, \"exclude-standard\", &opts, NULL,\n+\t\t\tN_(\"add the standard git exclusions\"),\n+\t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG,\n+\t\t\toption_parse_exclude_standard },\n \t\tOPT_BOOL('i', NULL, &opts.index_only,\n \t\t\t N_(\"don't check the working tree after merging\")),\n \t\tOPT__DRY_RUN(&opts.dry_run, N_(\"don't update the index or the work tree\")),\n@@ -219,10 +258,16 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n \t\topts.reset = UNPACK_RESET_PROTECT_UNTRACKED;\n \t}\n \tif ((opts.dir && !opts.update))\n-\t\tdie(\"--exclude-per-directory is meaningless unless -u\");\n-\tif (opts.dir && opts.reset == UNPACK_RESET_OVERWRITE_UNTRACKED)\n-\t\twarning(\"--exclude-per-directory without --preserve-untracked \"\n-\t\t\t\"has no effect\");\n+\t\tdie(\"%s requires -u\", exclude_opt == EXCLUDE_STANDARD ?\n+\t\t\t\"--exclude-standard\" :\" --exclude-per-directory\");\n+\tif (opts.dir && opts.reset == UNPACK_RESET_OVERWRITE_UNTRACKED) {\n+\t\tif (exclude_opt == EXCLUDE_STANDARD)\n+\t\t\tdie(\"--reset with --exclude-standard requires \"\n+\t\t\t    \"--protect-untracked\");\n+\t\telse\n+\t\t\twarning(\"--exclude-per-directory without \"\n+\t\t\t\t\"--preserve-untracked has no effect\");\n+\t}\n \tif (opts.merge && !opts.index_only)\n \t\tsetup_work_tree();\n \ndiff --git a/t/t1005-read-tree-reset.sh b/t/t1005-read-tree-reset.sh\nindex 6c9dd6805b..2e2a6a0c69 100755\n--- a/t/t1005-read-tree-reset.sh\n+++ b/t/t1005-read-tree-reset.sh\n@@ -30,6 +30,20 @@ test_expect_success '--protect-untracked option sanity checks' '\n \tread_tree_u_must_fail -m -u --no-protect-untracked\n '\n \n+test_expect_success 'exclude option sanity checks' '\n+\tread_tree_u_must_fail --reset -u --exclude-standard HEAD &&\n+\tread_tree_u_must_fail --reset --protect-untracked --exclude-standard &&\n+\tread_tree_u_must_fail --reset -u --protect-untracked \\\n+\t\t\t      --exclude-standard \\\n+\t\t\t      --exclude-per-directory=.gitignore HEAD &&\n+\tread_tree_u_must_fail --reset -u --protect-untracked \\\n+\t\t\t      --exclude-per-directory=gitignore \\\n+\t\t\t      --exclude-per-directory=.gitignore HEAD &&\n+\tread_tree_u_must_fail --reset --exclude-per-directory=.gitignore HEAD &&\n+\tread_tree_u_must_succeed --reset -u --exclude-per-directory=.gitignore \\\n+\t\t\t\t HEAD\n+'\n+\n test_expect_success 'reset should reset worktree' '\n \techo changed >df &&\n \tread_tree_u_must_succeed -u --reset HEAD^ &&\n@@ -53,12 +67,24 @@ test_expect_success 'reset --protect-untracked protects untracked directory' '\n \ttest_cmp expected-err actual-err\n '\n \n-test_expect_success 'reset --protect-untracked resets' '\n-\trm -rf new &&\n+test_expect_success 'reset --protect-untracked --exclude-standard overwrites ignored path' '\n+\ttest_when_finished \"rm .git/info/exclude\" &&\n+\techo missing >.git/info/exclude &&\n+\tread_tree_u_must_fail -u --reset --protect-untracked \\\n+\t\t\t      --exclude-standard HEAD &&\n+\techo new >.git/info/exclude &&\n \techo changed >df/file &&\n-\tread_tree_u_must_succeed -u --reset --protect-untracked HEAD &&\n-\tgit ls-files >actual-two &&\n-\ttest_cmp expect-two actual-two\n+\tread_tree_u_must_succeed -u --reset --protect-untracked \\\n+\t\t\t\t --exclude-standard HEAD &&\n+\tgit ls-files >actual &&\n+\ttest_cmp expect-two actual\n+'\n+\n+test_expect_success 'reset --protect-untracked resets' '\n+\techo changed >df &&\n+\tread_tree_u_must_succeed -u --reset --protect-untracked HEAD^ &&\n+\tgit ls-files >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'reset should remove remnants from a failed merge' '\ndiff --git a/t/t1013-read-tree-submodule.sh b/t/t1013-read-tree-submodule.sh\nindex 91a6fafcb4..728280d40d 100755\n--- a/t/t1013-read-tree-submodule.sh\n+++ b/t/t1013-read-tree-submodule.sh\n@@ -6,9 +6,8 @@ test_description='read-tree can handle submodules'\n . \"$TEST_DIRECTORY\"/lib-submodule-update.sh\n \n KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS=1\n-KNOWN_FAILURE_SUBMODULE_OVERWRITE_IGNORED_UNTRACKED=1\n \n-test_submodule_switch_recursing_with_args \"read-tree -u -m\"\n+test_submodule_switch_recursing_with_args \"read-tree -u -m --exclude-standard\"\n \n test_submodule_forced_switch_recursing_with_args \"read-tree -u --reset\"\n \n-- \n2.21.0\n\n"},{"id":"374748","messageId":"CACsJy8AeOWEMVcDiJky1SbSitkyBxSWu9CeDf8aspyLA1VDe8g@mail.gmail.com","threadId":"51019","inReplyTo":"20190501101403.20294-2-phillip.wood123@gmail.com","subject":"Re: [PATCH 1/2] read-tree --reset: add --protect-untracked","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-05-01T10:18:58Z","receivedAt":"2019-05-01T10:19:26Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, May 1, 2019 at 5:14 PM Phillip Wood <phillip.wood123@gmail.com> wrote:\n> diff --git a/unpack-trees.h b/unpack-trees.h\n> index d344d7d296..732b262c4d 100644\n> --- a/unpack-trees.h\n> +++ b/unpack-trees.h\n> @@ -41,9 +41,15 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,\n>   */\n>  void clear_unpack_trees_porcelain(struct unpack_trees_options *opts);\n>\n> +enum unpack_trees_reset_type {\n> +       UNPACK_NO_RESET = 0,\n> +       UNPACK_RESET_OVERWRITE_UNTRACKED,\n> +       UNPACK_RESET_PROTECT_UNTRACKED\n> +};\n> +\n>  struct unpack_trees_options {\n> -       unsigned int reset,\n> -                    merge,\n> +       enum unpack_trees_reset_type reset;\n\nCan we add protected_untracked that can be used in combination with\nthe current \"reset\"?\n\nThis opens up (or raises the question about) the opportunity to\nprotect untracked changes on non-reset updates.\n\n> +       unsigned int merge,\n>                      update,\n>                      clone,\n>                      index_only,\n> --\n> 2.21.0\n>\n\n\n-- \nDuy\n"},{"id":"374749","messageId":"CACsJy8CNUEBXmBcQnRHqdeFJtTvBuZ9thP7QPAw-ZOD+2ty3VA@mail.gmail.com","threadId":"51019","inReplyTo":"20190501101403.20294-1-phillip.wood123@gmail.com","subject":"Re: [PATCH 0/2] read-tree: improve untracked file support","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-05-01T10:31:03Z","receivedAt":"2019-05-01T10:31:32Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, May 1, 2019 at 5:14 PM Phillip Wood <phillip.wood123@gmail.com> wrote:\n>\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> These two patches teach read-tree how to avoid overwriting untracked\n> files when doing '--reset -u' and also how to respect all of git's\n> standard excludes files. I'd like to see the porcelain commands stop\n> overwriting untracked files, this is a first step on the way. I'm not\n> sure if we want to add options to the porcelain commands to protect\n> untracked files or just change their behavior and add an option to\n> override that. I'm leaning towards the latter but I'd be interested to\n> hear what others think.\n\nFor new commands like git-restore, it's definitely a good thing to not\noverwrite untracked files. For existing commands I guess we have to go\nover them one by one. For \"git reset --hard\", it should really just\noverwrite whatever needed to get back to the known good state. \"git\ncheckout -f\" , not so sure (seems weird that we need force-level-two\noption to override the protection provided by -f, if we change default\nbehavior)\n-- \nDuy\n"},{"id":"374750","messageId":"CACsJy8DD-Ubb9VyZCWdkpS1OgPpToMmrdMceXPUp3AWbfhvgdQ@mail.gmail.com","threadId":"51019","inReplyTo":"CACsJy8AeOWEMVcDiJky1SbSitkyBxSWu9CeDf8aspyLA1VDe8g@mail.gmail.com","subject":"Re: [PATCH 1/2] read-tree --reset: add --protect-untracked","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-05-01T10:32:37Z","receivedAt":"2019-05-01T10:33:06Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, May 1, 2019 at 5:18 PM Duy Nguyen <pclouds@gmail.com> wrote:\n>\n> On Wed, May 1, 2019 at 5:14 PM Phillip Wood <phillip.wood123@gmail.com> wrote:\n> > diff --git a/unpack-trees.h b/unpack-trees.h\n> > index d344d7d296..732b262c4d 100644\n> > --- a/unpack-trees.h\n> > +++ b/unpack-trees.h\n> > @@ -41,9 +41,15 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,\n> >   */\n> >  void clear_unpack_trees_porcelain(struct unpack_trees_options *opts);\n> >\n> > +enum unpack_trees_reset_type {\n> > +       UNPACK_NO_RESET = 0,\n> > +       UNPACK_RESET_OVERWRITE_UNTRACKED,\n> > +       UNPACK_RESET_PROTECT_UNTRACKED\n> > +};\n> > +\n> >  struct unpack_trees_options {\n> > -       unsigned int reset,\n> > -                    merge,\n> > +       enum unpack_trees_reset_type reset;\n>\n> Can we add protected_untracked that can be used in combination with\n> the current \"reset\"?\n>\n> This opens up (or raises the question about) the opportunity to\n> protect untracked changes on non-reset updates.\n\nBahh. \"protect_untracked\" is already on for non-reset updates. Sorry\nfor the noise.\n-- \nDuy\n"},{"id":"374765","messageId":"d4c36a24-b40c-a6ca-7a05-572ab93a0101@gmail.com","threadId":"51019","inReplyTo":"CACsJy8CNUEBXmBcQnRHqdeFJtTvBuZ9thP7QPAw-ZOD+2ty3VA@mail.gmail.com","subject":"Re: [PATCH 0/2] read-tree: improve untracked file support","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-05-01T14:58:28Z","receivedAt":"2019-05-01T14:58:50Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"\n\nOn 01/05/2019 11:31, Duy Nguyen wrote:\n> On Wed, May 1, 2019 at 5:14 PM Phillip Wood <phillip.wood123@gmail.com> wrote:\n>>\n>> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>>\n>> These two patches teach read-tree how to avoid overwriting untracked\n>> files when doing '--reset -u' and also how to respect all of git's\n>> standard excludes files. I'd like to see the porcelain commands stop\n>> overwriting untracked files, this is a first step on the way. I'm not\n>> sure if we want to add options to the porcelain commands to protect\n>> untracked files or just change their behavior and add an option to\n>> override that. I'm leaning towards the latter but I'd be interested to\n>> hear what others think.\n> \n> For new commands like git-restore, it's definitely a good thing to not\n> overwrite untracked files.\n\nI agree, unfortunately this series does not help with git-restore, only \ngit-switch. For restore on an index without conflicts I think it could \njust use the pathspec in struct unpack_trees_options and set opts.rest = \nUNPACK_RESET_PROTECT_UNTRACKED but that does not help if we want to \nhandle conflicted paths differently to non-conflicted paths.\n\n> For existing commands I guess we have to go\n> over them one by one. For \"git reset --hard\", it should really just\n> overwrite whatever needed to get back to the known good state. \"git\n> checkout -f\" , not so sure (seems weird that we need force-level-two\n> option to override the protection provided by -f, if we change default\n> behavior)\n\nI think it's fine for \"checkout -f\" to overwrite untracked files (and if \n\"switch --discard-changes\" does not then there is no pressing need to \nadd such a mode to checkout), --force is a good name for an option that \nnukes everything that gets in it's way. For \"reset --hard\" I'm not so \nsure, if I have changes to an untracked file I don't wont them \noverwritten by default. There is no porcelain equivalent to \"read-tree \n--reset --protect-untracked -u\" and I was hoping \"reset --hard\" may \nbecome that porcelain equivalent with a new --force or \n--overwrite-untracked option.\n\nFor the various \"foo --abort\" some (most?) are using \"reset --merge\" \nwhich I think declines to overwrite untracked files but rebase uses \n\"reset --hard\" which I'd like to change to protect untracked files in \nthe same way that rebase does for the initial checkout and when picking \ncommits. I haven't thought about stash.\n\nBest Wishes\n\nPhillip\n\n"},{"id":"374810","messageId":"CACsJy8C731LL3z1zaF4mpWmag1k7V3Ny6+ar9dMmHR4hdw_9AA@mail.gmail.com","threadId":"51019","inReplyTo":"20190501101403.20294-2-phillip.wood123@gmail.com","subject":"Re: [PATCH 1/2] read-tree --reset: add --protect-untracked","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-05-02T10:38:54Z","receivedAt":"2019-05-02T10:39:23Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, May 1, 2019 at 5:14 PM Phillip Wood <phillip.wood123@gmail.com> wrote:\n>\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> Currently there is no way to get git to discard changes to the\n> worktree without overwriting untracked files. `reset --hard`,\n> `checkout --force`, `checkout <rev> :/` and `read-tree --reset -u`\n\n\"checkout <rev> :/\" does not use unpack-trees, I think, so it does not\nbelong to this group.\n\n> will all overwrite untracked files. unpack_trees() has known how to\n> avoid overwriting untracked files since commit fcc387db9b (\"read-tree\n> -m -u: do not overwrite or remove untracked working tree files.\",\n> 2006-05-17) in response to concerns about lost files [1] but those\n> protections do not apply to --reset. This patch adds an\n> --protect-untracked option for read-tree/unpack_trees() to apply th\n> same protections with --reset.\n\nYour enum makes me wonder if we should have --reset-tracked instead of\n--protect-untracked (say what you want to reset seems tiny bit easier\nto understand than \"reset except untracked\"). Which leads to another\nquestion, do we ever need --reset-untracked (i.e. overwriting\nuntracked files but never ever touch tracked ones).\n\nI have one use case that --reset-untracked might make sense. Suppose\nyou do \"git reset HEAD^\" where HEAD has some new files. The new files\nwill be left alone in worktree of course, but switching back to HEAD\nafter that, unpack-trees will cry murder because it would otherwise\noverwrite untracked files (that have the exact same content).\n\n--reset-untracked might be useful for that. I'm carrying a patch to\ndetect identical file content though which addresses this very\nspecific case. Maybe it's the only case that --reset-untracked may be\nuseful.\n\n> It does not change the behavior of any\n> of the porcelain commands but paves the way for adding options or\n> changing the default for those in future.\n>\n> Note the actual change in unpack-trees.c is quite simple, the majority\n> of this patch is converting existing callers to use the new\n> unpack_trees_reset_type enum.\n\nThis could be split in a separate patch, then the actual change would\nbecome small and more to the point.\n\n> [1] https://public-inbox.org/git/8aa486160605161500m1dd8428cj@mail.gmail.com/\n>\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n>\n> Notes:\n>     adding --protect-untracked to the invocation of\n>     test_submodule_forced_switch() in t1013 fixes the known breakage of\n>     tests 57 & 58 but breaks test 64 which relies on forcing. I'm not sure\n>     that the expected results of 57 & 58 are correct if we're forcing.\n>\n>  Documentation/git-read-tree.txt | 10 +++++++--\n>  builtin/am.c                    |  8 ++++---\n>  builtin/checkout.c              |  2 +-\n>  builtin/read-tree.c             | 12 +++++++++++\n>  builtin/rebase.c                |  2 +-\n>  builtin/reset.c                 |  2 +-\n>  builtin/stash.c                 |  7 ++++---\n>  t/lib-read-tree.sh              | 11 ++++++++++\n>  t/t1005-read-tree-reset.sh      | 37 +++++++++++++++++++++++++++++++--\n>  unpack-trees.c                  |  3 ++-\n>  unpack-trees.h                  | 10 +++++++--\n>  11 files changed, 88 insertions(+), 16 deletions(-)\n>\n> diff --git a/Documentation/git-read-tree.txt b/Documentation/git-read-tree.txt\n> index d271842608..67864c6bbc 100644\n> --- a/Documentation/git-read-tree.txt\n> +++ b/Documentation/git-read-tree.txt\n> @@ -40,12 +40,18 @@ OPTIONS\n>  --reset::\n>         Same as -m, except that unmerged entries are discarded instead\n>         of failing. When used with `-u`, updates leading to loss of\n> -       working tree changes will not abort the operation.\n> +       working tree changes will not abort the operation and\n> +       untracked files will be overwritten. Use `--protect-untracked`\n> +       to avoid overwriting untracked files.\n>\n>  -u::\n>         After a successful merge, update the files in the work\n>         tree with the result of the merge.\n>\n> +--protect-untracked::\n> +       Prevent `--reset` `-u` from overwriting untracked files. Requires\n> +       `--reset` and `-u` (`-m` never overwrites untracked files).\n> +\n>  -i::\n>         Usually a merge requires the index file as well as the\n>         files in the working tree to be up to date with the\n> @@ -89,7 +95,7 @@ OPTIONS\n>         existed in the original index file.\n>\n>  --exclude-per-directory=<gitignore>::\n> -       When running the command with `-u` and `-m` options, the\n> +       When using `-u` with `-m` or `--reset` `--protect-untracked` options, the\n>         merge result may need to overwrite paths that are not\n>         tracked in the current branch.  The command usually\n>         refuses to proceed with the merge to avoid losing such a\n> diff --git a/builtin/am.c b/builtin/am.c\n> index 912d9821b1..a92394b682 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -1854,7 +1854,8 @@ static void am_resolve(struct am_state *state)\n>   * true, any unmerged entries will be discarded. Returns 0 on success, -1 on\n>   * failure.\n>   */\n> -static int fast_forward_to(struct tree *head, struct tree *remote, int reset)\n> +static int fast_forward_to(struct tree *head, struct tree *remote,\n> +                          enum unpack_trees_reset_type reset)\n>  {\n>         struct lock_file lock_file = LOCK_INIT;\n>         struct unpack_trees_options opts;\n> @@ -1942,7 +1943,8 @@ static int clean_index(const struct object_id *head, const struct object_id *rem\n>\n>         read_cache_unmerged();\n>\n> -       if (fast_forward_to(head_tree, head_tree, 1))\n> +       if (fast_forward_to(head_tree, head_tree,\n> +                           UNPACK_RESET_OVERWRITE_UNTRACKED))\n>                 return -1;\n>\n>         if (write_cache_as_tree(&index, 0, NULL))\n> @@ -1952,7 +1954,7 @@ static int clean_index(const struct object_id *head, const struct object_id *rem\n>         if (!index_tree)\n>                 return error(_(\"Could not parse object '%s'.\"), oid_to_hex(&index));\n>\n> -       if (fast_forward_to(index_tree, remote_tree, 0))\n> +       if (fast_forward_to(index_tree, remote_tree, UNPACK_NO_RESET))\n>                 return -1;\n>\n>         if (merge_tree(remote_tree))\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index ffa776c6e1..e9e70018f9 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -506,7 +506,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,\n>         opts.head_idx = -1;\n>         opts.update = worktree;\n>         opts.skip_unmerged = !worktree;\n> -       opts.reset = 1;\n> +       opts.reset = UNPACK_RESET_OVERWRITE_UNTRACKED;\n>         opts.merge = 1;\n>         opts.fn = oneway_merge;\n>         opts.verbose_update = o->show_progress;\n> diff --git a/builtin/read-tree.c b/builtin/read-tree.c\n> index 5c9c082595..23735adde9 100644\n> --- a/builtin/read-tree.c\n> +++ b/builtin/read-tree.c\n> @@ -114,6 +114,7 @@ static int git_read_tree_config(const char *var, const char *value, void *cb)\n>  int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n>  {\n>         int i, stage = 0;\n> +       int protect_untracked = -1;\n>         struct object_id oid;\n>         struct tree_desc t[MAX_UNPACK_TREES];\n>         struct unpack_trees_options opts;\n> @@ -140,6 +141,8 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n>                   PARSE_OPT_NONEG },\n>                 OPT_BOOL('u', NULL, &opts.update,\n>                          N_(\"update working tree with merge result\")),\n> +               OPT_BOOL(0, \"protect-untracked\", &protect_untracked,\n> +                        N_(\"do not overwrite untracked files\")),\n>                 { OPTION_CALLBACK, 0, \"exclude-per-directory\", &opts,\n>                   N_(\"gitignore\"),\n>                   N_(\"allow explicitly ignored files to be overwritten\"),\n> @@ -209,8 +212,17 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n>         if ((opts.update || opts.index_only) && !opts.merge)\n>                 die(\"%s is meaningless without -m, --reset, or --prefix\",\n>                     opts.update ? \"-u\" : \"-i\");\n> +       if (protect_untracked >= 0) {\n> +               if (!opts.reset || !opts.update)\n> +                       die(\"-%s-protect-untracked requires --reset and -u\",\n> +                           protect_untracked ? \"\" : \"-no\");\n> +               opts.reset = UNPACK_RESET_PROTECT_UNTRACKED;\n> +       }\n>         if ((opts.dir && !opts.update))\n>                 die(\"--exclude-per-directory is meaningless unless -u\");\n> +       if (opts.dir && opts.reset == UNPACK_RESET_OVERWRITE_UNTRACKED)\n> +               warning(\"--exclude-per-directory without --preserve-untracked \"\n> +                       \"has no effect\");\n>         if (opts.merge && !opts.index_only)\n>                 setup_work_tree();\n>\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 2e41ad5644..feb30a71f5 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -398,7 +398,7 @@ static int reset_head(struct object_id *oid, const char *action,\n>         unpack_tree_opts.update = 1;\n>         unpack_tree_opts.merge = 1;\n>         if (!detach_head)\n> -               unpack_tree_opts.reset = 1;\n> +               unpack_tree_opts.reset = UNPACK_RESET_OVERWRITE_UNTRACKED;\n>\n>         if (repo_read_index_unmerged(the_repository) < 0) {\n>                 ret = error(_(\"could not read index\"));\n> diff --git a/builtin/reset.c b/builtin/reset.c\n> index 26ef9a7bd0..a39dd92fb2 100644\n> --- a/builtin/reset.c\n> +++ b/builtin/reset.c\n> @@ -70,7 +70,7 @@ static int reset_index(const struct object_id *oid, int reset_type, int quiet)\n>                 opts.update = 1;\n>                 /* fallthrough */\n>         default:\n> -               opts.reset = 1;\n> +               opts.reset = UNPACK_RESET_OVERWRITE_UNTRACKED;\n>         }\n>\n>         read_cache_unmerged();\n> diff --git a/builtin/stash.c b/builtin/stash.c\n> index 2a8e6d09b4..175d1b62d3 100644\n> --- a/builtin/stash.c\n> +++ b/builtin/stash.c\n> @@ -227,7 +227,8 @@ static int clear_stash(int argc, const char **argv, const char *prefix)\n>         return do_clear_stash();\n>  }\n>\n> -static int reset_tree(struct object_id *i_tree, int update, int reset)\n> +static int reset_tree(struct object_id *i_tree, int update,\n> +                     enum unpack_trees_reset_type reset)\n>  {\n>         int nr_trees = 1;\n>         struct unpack_trees_options opts;\n> @@ -461,7 +462,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n>         }\n>\n>         if (has_index) {\n> -               if (reset_tree(&index_tree, 0, 0))\n> +               if (reset_tree(&index_tree, 0, UNPACK_NO_RESET))\n>                         return -1;\n>         } else {\n>                 struct strbuf out = STRBUF_INIT;\n> @@ -471,7 +472,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n>                         return -1;\n>                 }\n>\n> -               if (reset_tree(&c_tree, 0, 1)) {\n> +               if (reset_tree(&c_tree, 0, UNPACK_RESET_OVERWRITE_UNTRACKED)) {\n>                         strbuf_release(&out);\n>                         return -1;\n>                 }\n> diff --git a/t/lib-read-tree.sh b/t/lib-read-tree.sh\n> index b95f485606..22f516d91f 100644\n> --- a/t/lib-read-tree.sh\n> +++ b/t/lib-read-tree.sh\n> @@ -39,3 +39,14 @@ read_tree_u_must_fail () {\n>         test_cmp pre-dry-run-wt post-dry-run-wt &&\n>         test_must_fail git read-tree \"$@\"\n>  }\n> +\n> +read_tree_u_must_fail_save_err () {\n> +       git ls-files -s >pre-dry-run &&\n> +       git diff-files -p >pre-dry-run-wt &&\n> +       test_must_fail git read-tree -n \"$@\" &&\n> +       git ls-files -s >post-dry-run &&\n> +       git diff-files -p >post-dry-run-wt &&\n> +       test_cmp pre-dry-run post-dry-run &&\n> +       test_cmp pre-dry-run-wt post-dry-run-wt &&\n> +       test_must_fail git read-tree \"$@\" 2>actual-err\n> +}\n> diff --git a/t/t1005-read-tree-reset.sh b/t/t1005-read-tree-reset.sh\n> index 83b09e1310..6c9dd6805b 100755\n> --- a/t/t1005-read-tree-reset.sh\n> +++ b/t/t1005-read-tree-reset.sh\n> @@ -19,15 +19,48 @@ test_expect_success 'setup' '\n>         git add df &&\n>         echo content >new &&\n>         git add new &&\n> -       git commit -m two\n> +       git commit -m two &&\n> +       git ls-files >expect-two\n>  '\n>\n> -test_expect_success 'reset should work' '\n> +test_expect_success '--protect-untracked option sanity checks' '\n> +       read_tree_u_must_fail --reset --protect-untracked HEAD &&\n> +       read_tree_u_must_fail --reset --no-protect-untracked HEAD &&\n> +       read_tree_u_must_fail -m -u --protect-untracked HEAD &&\n> +       read_tree_u_must_fail -m -u --no-protect-untracked\n> +'\n> +\n> +test_expect_success 'reset should reset worktree' '\n> +       echo changed >df &&\n>         read_tree_u_must_succeed -u --reset HEAD^ &&\n>         git ls-files >actual &&\n>         test_cmp expect actual\n>  '\n>\n> +test_expect_success 'reset --protect-untracked protects untracked file' '\n> +       echo changed >new &&\n> +       read_tree_u_must_fail_save_err -u --reset --protect-untracked HEAD &&\n> +       echo \"error: Untracked working tree file '\\'new\\'' would be overwritten by merge.\" >expected-err &&\n> +       test_cmp expected-err actual-err\n> +'\n> +\n> +test_expect_success 'reset --protect-untracked protects untracked directory' '\n> +       rm new &&\n> +       mkdir new &&\n> +       echo untracked >new/untracked &&\n> +       read_tree_u_must_fail_save_err -u --reset --protect-untracked HEAD &&\n> +       echo \"error: Updating '\\'new\\'' would lose untracked files in it\" >expected-err &&\n> +       test_cmp expected-err actual-err\n> +'\n> +\n> +test_expect_success 'reset --protect-untracked resets' '\n> +       rm -rf new &&\n> +       echo changed >df/file &&\n> +       read_tree_u_must_succeed -u --reset --protect-untracked HEAD &&\n> +       git ls-files >actual-two &&\n> +       test_cmp expect-two actual-two\n> +'\n> +\n>  test_expect_success 'reset should remove remnants from a failed merge' '\n>         read_tree_u_must_succeed --reset -u HEAD &&\n>         git ls-files -s >expect &&\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index 50189909b8..b1722730fe 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -1917,7 +1917,8 @@ static int verify_absent_1(const struct cache_entry *ce,\n>         int len;\n>         struct stat st;\n>\n> -       if (o->index_only || o->reset || !o->update)\n> +       if (o->index_only || o->reset == UNPACK_RESET_OVERWRITE_UNTRACKED ||\n> +           !o->update)\n>                 return 0;\n>\n>         len = check_leading_path(ce->name, ce_namelen(ce));\n> diff --git a/unpack-trees.h b/unpack-trees.h\n> index d344d7d296..732b262c4d 100644\n> --- a/unpack-trees.h\n> +++ b/unpack-trees.h\n> @@ -41,9 +41,15 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,\n>   */\n>  void clear_unpack_trees_porcelain(struct unpack_trees_options *opts);\n>\n> +enum unpack_trees_reset_type {\n> +       UNPACK_NO_RESET = 0,\n> +       UNPACK_RESET_OVERWRITE_UNTRACKED,\n> +       UNPACK_RESET_PROTECT_UNTRACKED\n> +};\n> +\n>  struct unpack_trees_options {\n> -       unsigned int reset,\n> -                    merge,\n> +       enum unpack_trees_reset_type reset;\n> +       unsigned int merge,\n>                      update,\n>                      clone,\n>                      index_only,\n> --\n> 2.21.0\n>\n\n\n-- \nDuy\n"},{"id":"374811","messageId":"CACsJy8AAj8cRAc+PnS4OJy8M=K7FjCHUsL6Cx-6+RTAbTA_1fQ@mail.gmail.com","threadId":"51019","inReplyTo":"d4c36a24-b40c-a6ca-7a05-572ab93a0101@gmail.com","subject":"Re: [PATCH 0/2] read-tree: improve untracked file support","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-05-02T10:53:24Z","receivedAt":"2019-05-02T10:53:52Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, May 1, 2019 at 9:58 PM Phillip Wood <phillip.wood123@gmail.com> wrote:\n>\n>\n>\n> On 01/05/2019 11:31, Duy Nguyen wrote:\n> > On Wed, May 1, 2019 at 5:14 PM Phillip Wood <phillip.wood123@gmail.com> wrote:\n> >>\n> >> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n> >>\n> >> These two patches teach read-tree how to avoid overwriting untracked\n> >> files when doing '--reset -u' and also how to respect all of git's\n> >> standard excludes files. I'd like to see the porcelain commands stop\n> >> overwriting untracked files, this is a first step on the way. I'm not\n> >> sure if we want to add options to the porcelain commands to protect\n> >> untracked files or just change their behavior and add an option to\n> >> override that. I'm leaning towards the latter but I'd be interested to\n> >> hear what others think.\n> >\n> > For new commands like git-restore, it's definitely a good thing to not\n> > overwrite untracked files.\n>\n> I agree, unfortunately this series does not help with git-restore, only\n> git-switch. For restore on an index without conflicts I think it could\n> just use the pathspec in struct unpack_trees_options and set opts.rest =\n> UNPACK_RESET_PROTECT_UNTRACKED but that does not help if we want to\n> handle conflicted paths differently to non-conflicted paths.\n\nRight. I got confused. You did mention \"git checkout <rev> :/\" in 1/2,\nwhich is the same as \"git restore --source <rev> --staged --worktree\n:/\" and  can also potentially overwrite untracked files, even though\nit does not use unpack-trees and cannot be fixed with this. Never\nmind. Let's leave git-restore out of the discussion for now.\n\n> > For existing commands I guess we have to go\n> > over them one by one. For \"git reset --hard\", it should really just\n> > overwrite whatever needed to get back to the known good state. \"git\n> > checkout -f\" , not so sure (seems weird that we need force-level-two\n> > option to override the protection provided by -f, if we change default\n> > behavior)\n>\n> I think it's fine for \"checkout -f\" to overwrite untracked files (and if\n> \"switch --discard-changes\" does not then there is no pressing need to\n> add such a mode to checkout), --force is a good name for an option that\n> nukes everything that gets in it's way. For \"reset --hard\" I'm not so\n> sure, if I have changes to an untracked file I don't wont them\n> overwritten by default. There is no porcelain equivalent to \"read-tree\n> --reset --protect-untracked -u\" and I was hoping \"reset --hard\" may\n> become that porcelain equivalent with a new --force or\n> --overwrite-untracked option.\n\nMy (biased, obviously) view is that \"git reset --hard\" is very\ndangerous and I'm not trying to change that, especially when its\nbehavior has been like this since forever and I'm sure it's used in\nscripts.\n\nInstead \"git restore\" should be used when you need \"git reset --hard\nHEAD\", the most often use case. And since it's new, changing default\nbehavior is not a problem. Which brings us back to git-restore :)\n\nBut either way, git-restore or git-reset, I still don't see why\nuntracked files are more valuable in this case than tracked ones to\nchange the default. I can see that sometimes you may want to restore\njust tracked files, or untracked files, almost like filtering with\npathspec.\n\n> For the various \"foo --abort\" some (most?) are using \"reset --merge\"\n> which I think declines to overwrite untracked files but rebase uses\n> \"reset --hard\" which I'd like to change to protect untracked files in\n> the same way that rebase does for the initial checkout and when picking\n> commits. I haven't thought about stash.\n\nYeah it looks like cherry-pick and revert use \"reset --merge\" too\n(reset_for_rollback function). That's all of them. Probably a stupid\nquestion, why can't rebase just use \"rebase --merge\" like everybody\nelse?\n\n>\n> Best Wishes\n>\n> Phillip\n>\n\n\n-- \nDuy\n"},{"id":"374883","messageId":"15dad590-087e-5a48-9238-5d2826950506@gmail.com","threadId":"51019","inReplyTo":"CACsJy8C731LL3z1zaF4mpWmag1k7V3Ny6+ar9dMmHR4hdw_9AA@mail.gmail.com","subject":"Re: [PATCH 1/2] read-tree --reset: add --protect-untracked","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-05-03T15:20:52Z","receivedAt":"2019-05-03T15:20:58Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 02/05/2019 11:38, Duy Nguyen wrote:\n> On Wed, May 1, 2019 at 5:14 PM Phillip Wood <phillip.wood123@gmail.com> wrote:\n>>\n>> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>>\n>> Currently there is no way to get git to discard changes to the\n>> worktree without overwriting untracked files. `reset --hard`,\n>> `checkout --force`, `checkout <rev> :/` and `read-tree --reset -u`\n> \n> \"checkout <rev> :/\" does not use unpack-trees, I think, so it does not\n> belong to this group.\n\nYou're right that it does not use unpack-trees, but I'm making the point \nthat there is no way to discard changes to tracked files without \noverwriting untracked files whatever one uses.\n\n>> will all overwrite untracked files. unpack_trees() has known how to\n>> avoid overwriting untracked files since commit fcc387db9b (\"read-tree\n>> -m -u: do not overwrite or remove untracked working tree files.\",\n>> 2006-05-17) in response to concerns about lost files [1] but those\n>> protections do not apply to --reset. This patch adds an\n>> --protect-untracked option for read-tree/unpack_trees() to apply th\n>> same protections with --reset.\n> \n> Your enum makes me wonder if we should have --reset-tracked instead of\n> --protect-untracked (say what you want to reset seems tiny bit easier\n> to understand than \"reset except untracked\"). Which leads to another\n> question, do we ever need --reset-untracked (i.e. overwriting\n> untracked files but never ever touch tracked ones).\n> \n> I have one use case that --reset-untracked might make sense. Suppose\n> you do \"git reset HEAD^\" where HEAD has some new files. The new files\n> will be left alone in worktree of course, but switching back to HEAD\n> after that, unpack-trees will cry murder because it would otherwise\n> overwrite untracked files (that have the exact same content).\n> \n> --reset-untracked might be useful for that. I'm carrying a patch to\n> detect identical file content though which addresses this very\n> specific case. Maybe it's the only case that --reset-untracked may be\n> useful.\n\nThat patch sounds useful (I played around with \"hg update\" the other day \nand I think it does the same). I think that's probably a common case, it \nalso works if one does \"git checkout <other-branch> file\" then \"git \nreset HEAD\" followed \"git checkout <other-branch>\". --reset-untracked is \njust like --reset which we can't get rid of without breaking backwards \ncompatibility. Having --reset-tracked is maybe better than having a \n--protect-tracked option that only works with --reset -u.\n\n>> It does not change the behavior of any\n>> of the porcelain commands but paves the way for adding options or\n>> changing the default for those in future.\n>>\n>> Note the actual change in unpack-trees.c is quite simple, the majority\n>> of this patch is converting existing callers to use the new\n>> unpack_trees_reset_type enum.\n> \n> This could be split in a separate patch, then the actual change would\n> become small and more to the point.\n\nYes that would make it clearer.\n\nBest Wishes\n\nPhillip\n\n> \n>> [1] https://public-inbox.org/git/8aa486160605161500m1dd8428cj@mail.gmail.com/\n>>\n>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n>> ---\n>>\n>> Notes:\n>>      adding --protect-untracked to the invocation of\n>>      test_submodule_forced_switch() in t1013 fixes the known breakage of\n>>      tests 57 & 58 but breaks test 64 which relies on forcing. I'm not sure\n>>      that the expected results of 57 & 58 are correct if we're forcing.\n>>\n>>   Documentation/git-read-tree.txt | 10 +++++++--\n>>   builtin/am.c                    |  8 ++++---\n>>   builtin/checkout.c              |  2 +-\n>>   builtin/read-tree.c             | 12 +++++++++++\n>>   builtin/rebase.c                |  2 +-\n>>   builtin/reset.c                 |  2 +-\n>>   builtin/stash.c                 |  7 ++++---\n>>   t/lib-read-tree.sh              | 11 ++++++++++\n>>   t/t1005-read-tree-reset.sh      | 37 +++++++++++++++++++++++++++++++--\n>>   unpack-trees.c                  |  3 ++-\n>>   unpack-trees.h                  | 10 +++++++--\n>>   11 files changed, 88 insertions(+), 16 deletions(-)\n>>\n>> diff --git a/Documentation/git-read-tree.txt b/Documentation/git-read-tree.txt\n>> index d271842608..67864c6bbc 100644\n>> --- a/Documentation/git-read-tree.txt\n>> +++ b/Documentation/git-read-tree.txt\n>> @@ -40,12 +40,18 @@ OPTIONS\n>>   --reset::\n>>          Same as -m, except that unmerged entries are discarded instead\n>>          of failing. When used with `-u`, updates leading to loss of\n>> -       working tree changes will not abort the operation.\n>> +       working tree changes will not abort the operation and\n>> +       untracked files will be overwritten. Use `--protect-untracked`\n>> +       to avoid overwriting untracked files.\n>>\n>>   -u::\n>>          After a successful merge, update the files in the work\n>>          tree with the result of the merge.\n>>\n>> +--protect-untracked::\n>> +       Prevent `--reset` `-u` from overwriting untracked files. Requires\n>> +       `--reset` and `-u` (`-m` never overwrites untracked files).\n>> +\n>>   -i::\n>>          Usually a merge requires the index file as well as the\n>>          files in the working tree to be up to date with the\n>> @@ -89,7 +95,7 @@ OPTIONS\n>>          existed in the original index file.\n>>\n>>   --exclude-per-directory=<gitignore>::\n>> -       When running the command with `-u` and `-m` options, the\n>> +       When using `-u` with `-m` or `--reset` `--protect-untracked` options, the\n>>          merge result may need to overwrite paths that are not\n>>          tracked in the current branch.  The command usually\n>>          refuses to proceed with the merge to avoid losing such a\n>> diff --git a/builtin/am.c b/builtin/am.c\n>> index 912d9821b1..a92394b682 100644\n>> --- a/builtin/am.c\n>> +++ b/builtin/am.c\n>> @@ -1854,7 +1854,8 @@ static void am_resolve(struct am_state *state)\n>>    * true, any unmerged entries will be discarded. Returns 0 on success, -1 on\n>>    * failure.\n>>    */\n>> -static int fast_forward_to(struct tree *head, struct tree *remote, int reset)\n>> +static int fast_forward_to(struct tree *head, struct tree *remote,\n>> +                          enum unpack_trees_reset_type reset)\n>>   {\n>>          struct lock_file lock_file = LOCK_INIT;\n>>          struct unpack_trees_options opts;\n>> @@ -1942,7 +1943,8 @@ static int clean_index(const struct object_id *head, const struct object_id *rem\n>>\n>>          read_cache_unmerged();\n>>\n>> -       if (fast_forward_to(head_tree, head_tree, 1))\n>> +       if (fast_forward_to(head_tree, head_tree,\n>> +                           UNPACK_RESET_OVERWRITE_UNTRACKED))\n>>                  return -1;\n>>\n>>          if (write_cache_as_tree(&index, 0, NULL))\n>> @@ -1952,7 +1954,7 @@ static int clean_index(const struct object_id *head, const struct object_id *rem\n>>          if (!index_tree)\n>>                  return error(_(\"Could not parse object '%s'.\"), oid_to_hex(&index));\n>>\n>> -       if (fast_forward_to(index_tree, remote_tree, 0))\n>> +       if (fast_forward_to(index_tree, remote_tree, UNPACK_NO_RESET))\n>>                  return -1;\n>>\n>>          if (merge_tree(remote_tree))\n>> diff --git a/builtin/checkout.c b/builtin/checkout.c\n>> index ffa776c6e1..e9e70018f9 100644\n>> --- a/builtin/checkout.c\n>> +++ b/builtin/checkout.c\n>> @@ -506,7 +506,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,\n>>          opts.head_idx = -1;\n>>          opts.update = worktree;\n>>          opts.skip_unmerged = !worktree;\n>> -       opts.reset = 1;\n>> +       opts.reset = UNPACK_RESET_OVERWRITE_UNTRACKED;\n>>          opts.merge = 1;\n>>          opts.fn = oneway_merge;\n>>          opts.verbose_update = o->show_progress;\n>> diff --git a/builtin/read-tree.c b/builtin/read-tree.c\n>> index 5c9c082595..23735adde9 100644\n>> --- a/builtin/read-tree.c\n>> +++ b/builtin/read-tree.c\n>> @@ -114,6 +114,7 @@ static int git_read_tree_config(const char *var, const char *value, void *cb)\n>>   int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n>>   {\n>>          int i, stage = 0;\n>> +       int protect_untracked = -1;\n>>          struct object_id oid;\n>>          struct tree_desc t[MAX_UNPACK_TREES];\n>>          struct unpack_trees_options opts;\n>> @@ -140,6 +141,8 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n>>                    PARSE_OPT_NONEG },\n>>                  OPT_BOOL('u', NULL, &opts.update,\n>>                           N_(\"update working tree with merge result\")),\n>> +               OPT_BOOL(0, \"protect-untracked\", &protect_untracked,\n>> +                        N_(\"do not overwrite untracked files\")),\n>>                  { OPTION_CALLBACK, 0, \"exclude-per-directory\", &opts,\n>>                    N_(\"gitignore\"),\n>>                    N_(\"allow explicitly ignored files to be overwritten\"),\n>> @@ -209,8 +212,17 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n>>          if ((opts.update || opts.index_only) && !opts.merge)\n>>                  die(\"%s is meaningless without -m, --reset, or --prefix\",\n>>                      opts.update ? \"-u\" : \"-i\");\n>> +       if (protect_untracked >= 0) {\n>> +               if (!opts.reset || !opts.update)\n>> +                       die(\"-%s-protect-untracked requires --reset and -u\",\n>> +                           protect_untracked ? \"\" : \"-no\");\n>> +               opts.reset = UNPACK_RESET_PROTECT_UNTRACKED;\n>> +       }\n>>          if ((opts.dir && !opts.update))\n>>                  die(\"--exclude-per-directory is meaningless unless -u\");\n>> +       if (opts.dir && opts.reset == UNPACK_RESET_OVERWRITE_UNTRACKED)\n>> +               warning(\"--exclude-per-directory without --preserve-untracked \"\n>> +                       \"has no effect\");\n>>          if (opts.merge && !opts.index_only)\n>>                  setup_work_tree();\n>>\n>> diff --git a/builtin/rebase.c b/builtin/rebase.c\n>> index 2e41ad5644..feb30a71f5 100644\n>> --- a/builtin/rebase.c\n>> +++ b/builtin/rebase.c\n>> @@ -398,7 +398,7 @@ static int reset_head(struct object_id *oid, const char *action,\n>>          unpack_tree_opts.update = 1;\n>>          unpack_tree_opts.merge = 1;\n>>          if (!detach_head)\n>> -               unpack_tree_opts.reset = 1;\n>> +               unpack_tree_opts.reset = UNPACK_RESET_OVERWRITE_UNTRACKED;\n>>\n>>          if (repo_read_index_unmerged(the_repository) < 0) {\n>>                  ret = error(_(\"could not read index\"));\n>> diff --git a/builtin/reset.c b/builtin/reset.c\n>> index 26ef9a7bd0..a39dd92fb2 100644\n>> --- a/builtin/reset.c\n>> +++ b/builtin/reset.c\n>> @@ -70,7 +70,7 @@ static int reset_index(const struct object_id *oid, int reset_type, int quiet)\n>>                  opts.update = 1;\n>>                  /* fallthrough */\n>>          default:\n>> -               opts.reset = 1;\n>> +               opts.reset = UNPACK_RESET_OVERWRITE_UNTRACKED;\n>>          }\n>>\n>>          read_cache_unmerged();\n>> diff --git a/builtin/stash.c b/builtin/stash.c\n>> index 2a8e6d09b4..175d1b62d3 100644\n>> --- a/builtin/stash.c\n>> +++ b/builtin/stash.c\n>> @@ -227,7 +227,8 @@ static int clear_stash(int argc, const char **argv, const char *prefix)\n>>          return do_clear_stash();\n>>   }\n>>\n>> -static int reset_tree(struct object_id *i_tree, int update, int reset)\n>> +static int reset_tree(struct object_id *i_tree, int update,\n>> +                     enum unpack_trees_reset_type reset)\n>>   {\n>>          int nr_trees = 1;\n>>          struct unpack_trees_options opts;\n>> @@ -461,7 +462,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n>>          }\n>>\n>>          if (has_index) {\n>> -               if (reset_tree(&index_tree, 0, 0))\n>> +               if (reset_tree(&index_tree, 0, UNPACK_NO_RESET))\n>>                          return -1;\n>>          } else {\n>>                  struct strbuf out = STRBUF_INIT;\n>> @@ -471,7 +472,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n>>                          return -1;\n>>                  }\n>>\n>> -               if (reset_tree(&c_tree, 0, 1)) {\n>> +               if (reset_tree(&c_tree, 0, UNPACK_RESET_OVERWRITE_UNTRACKED)) {\n>>                          strbuf_release(&out);\n>>                          return -1;\n>>                  }\n>> diff --git a/t/lib-read-tree.sh b/t/lib-read-tree.sh\n>> index b95f485606..22f516d91f 100644\n>> --- a/t/lib-read-tree.sh\n>> +++ b/t/lib-read-tree.sh\n>> @@ -39,3 +39,14 @@ read_tree_u_must_fail () {\n>>          test_cmp pre-dry-run-wt post-dry-run-wt &&\n>>          test_must_fail git read-tree \"$@\"\n>>   }\n>> +\n>> +read_tree_u_must_fail_save_err () {\n>> +       git ls-files -s >pre-dry-run &&\n>> +       git diff-files -p >pre-dry-run-wt &&\n>> +       test_must_fail git read-tree -n \"$@\" &&\n>> +       git ls-files -s >post-dry-run &&\n>> +       git diff-files -p >post-dry-run-wt &&\n>> +       test_cmp pre-dry-run post-dry-run &&\n>> +       test_cmp pre-dry-run-wt post-dry-run-wt &&\n>> +       test_must_fail git read-tree \"$@\" 2>actual-err\n>> +}\n>> diff --git a/t/t1005-read-tree-reset.sh b/t/t1005-read-tree-reset.sh\n>> index 83b09e1310..6c9dd6805b 100755\n>> --- a/t/t1005-read-tree-reset.sh\n>> +++ b/t/t1005-read-tree-reset.sh\n>> @@ -19,15 +19,48 @@ test_expect_success 'setup' '\n>>          git add df &&\n>>          echo content >new &&\n>>          git add new &&\n>> -       git commit -m two\n>> +       git commit -m two &&\n>> +       git ls-files >expect-two\n>>   '\n>>\n>> -test_expect_success 'reset should work' '\n>> +test_expect_success '--protect-untracked option sanity checks' '\n>> +       read_tree_u_must_fail --reset --protect-untracked HEAD &&\n>> +       read_tree_u_must_fail --reset --no-protect-untracked HEAD &&\n>> +       read_tree_u_must_fail -m -u --protect-untracked HEAD &&\n>> +       read_tree_u_must_fail -m -u --no-protect-untracked\n>> +'\n>> +\n>> +test_expect_success 'reset should reset worktree' '\n>> +       echo changed >df &&\n>>          read_tree_u_must_succeed -u --reset HEAD^ &&\n>>          git ls-files >actual &&\n>>          test_cmp expect actual\n>>   '\n>>\n>> +test_expect_success 'reset --protect-untracked protects untracked file' '\n>> +       echo changed >new &&\n>> +       read_tree_u_must_fail_save_err -u --reset --protect-untracked HEAD &&\n>> +       echo \"error: Untracked working tree file '\\'new\\'' would be overwritten by merge.\" >expected-err &&\n>> +       test_cmp expected-err actual-err\n>> +'\n>> +\n>> +test_expect_success 'reset --protect-untracked protects untracked directory' '\n>> +       rm new &&\n>> +       mkdir new &&\n>> +       echo untracked >new/untracked &&\n>> +       read_tree_u_must_fail_save_err -u --reset --protect-untracked HEAD &&\n>> +       echo \"error: Updating '\\'new\\'' would lose untracked files in it\" >expected-err &&\n>> +       test_cmp expected-err actual-err\n>> +'\n>> +\n>> +test_expect_success 'reset --protect-untracked resets' '\n>> +       rm -rf new &&\n>> +       echo changed >df/file &&\n>> +       read_tree_u_must_succeed -u --reset --protect-untracked HEAD &&\n>> +       git ls-files >actual-two &&\n>> +       test_cmp expect-two actual-two\n>> +'\n>> +\n>>   test_expect_success 'reset should remove remnants from a failed merge' '\n>>          read_tree_u_must_succeed --reset -u HEAD &&\n>>          git ls-files -s >expect &&\n>> diff --git a/unpack-trees.c b/unpack-trees.c\n>> index 50189909b8..b1722730fe 100644\n>> --- a/unpack-trees.c\n>> +++ b/unpack-trees.c\n>> @@ -1917,7 +1917,8 @@ static int verify_absent_1(const struct cache_entry *ce,\n>>          int len;\n>>          struct stat st;\n>>\n>> -       if (o->index_only || o->reset || !o->update)\n>> +       if (o->index_only || o->reset == UNPACK_RESET_OVERWRITE_UNTRACKED ||\n>> +           !o->update)\n>>                  return 0;\n>>\n>>          len = check_leading_path(ce->name, ce_namelen(ce));\n>> diff --git a/unpack-trees.h b/unpack-trees.h\n>> index d344d7d296..732b262c4d 100644\n>> --- a/unpack-trees.h\n>> +++ b/unpack-trees.h\n>> @@ -41,9 +41,15 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,\n>>    */\n>>   void clear_unpack_trees_porcelain(struct unpack_trees_options *opts);\n>>\n>> +enum unpack_trees_reset_type {\n>> +       UNPACK_NO_RESET = 0,\n>> +       UNPACK_RESET_OVERWRITE_UNTRACKED,\n>> +       UNPACK_RESET_PROTECT_UNTRACKED\n>> +};\n>> +\n>>   struct unpack_trees_options {\n>> -       unsigned int reset,\n>> -                    merge,\n>> +       enum unpack_trees_reset_type reset;\n>> +       unsigned int merge,\n>>                       update,\n>>                       clone,\n>>                       index_only,\n>> --\n>> 2.21.0\n>>\n> \n> \n"},{"id":"375006","messageId":"994d4687-39ed-91b9-cdf9-d4be39cbb2ba@gmail.com","threadId":"51019","inReplyTo":"CACsJy8AAj8cRAc+PnS4OJy8M=K7FjCHUsL6Cx-6+RTAbTA_1fQ@mail.gmail.com","subject":"Re: [PATCH 0/2] read-tree: improve untracked file support","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-05-07T10:01:59Z","receivedAt":"2019-05-07T10:02:05Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 02/05/2019 11:53, Duy Nguyen wrote:\n> On Wed, May 1, 2019 at 9:58 PM Phillip Wood <phillip.wood123@gmail.com> wrote:\n>> On 01/05/2019 11:31, Duy Nguyen wrote:\n>>> On Wed, May 1, 2019 at 5:14 PM Phillip Wood <phillip.wood123@gmail.com> wrote:\n>>>>\n>>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>>>>\n>>>> These two patches teach read-tree how to avoid overwriting untracked\n>>>> files when doing '--reset -u' and also how to respect all of git's\n>>>> standard excludes files. I'd like to see the porcelain commands stop\n>>>> overwriting untracked files, this is a first step on the way. I'm not\n>>>> sure if we want to add options to the porcelain commands to protect\n>>>> untracked files or just change their behavior and add an option to\n>>>> override that. I'm leaning towards the latter but I'd be interested to\n>>>> hear what others think.\n>>>\n>>> For new commands like git-restore, it's definitely a good thing to not\n>>> overwrite untracked files.\n>>\n>> I agree, unfortunately this series does not help with git-restore, only\n>> git-switch. For restore on an index without conflicts I think it could\n>> just use the pathspec in struct unpack_trees_options and set opts.rest =\n>> UNPACK_RESET_PROTECT_UNTRACKED but that does not help if we want to\n>> handle conflicted paths differently to non-conflicted paths.\n> \n> Right. I got confused. You did mention \"git checkout <rev> :/\" in 1/2,\n> which is the same as \"git restore --source <rev> --staged --worktree\n> :/\" and  can also potentially overwrite untracked files, even though\n> it does not use unpack-trees and cannot be fixed with this. Never\n> mind. Let's leave git-restore out of the discussion for now.\n> \n>>> For existing commands I guess we have to go\n>>> over them one by one. For \"git reset --hard\", it should really just\n>>> overwrite whatever needed to get back to the known good state. \"git\n>>> checkout -f\" , not so sure (seems weird that we need force-level-two\n>>> option to override the protection provided by -f, if we change default\n>>> behavior)\n>>\n>> I think it's fine for \"checkout -f\" to overwrite untracked files (and if\n>> \"switch --discard-changes\" does not then there is no pressing need to\n>> add such a mode to checkout), --force is a good name for an option that\n>> nukes everything that gets in it's way. For \"reset --hard\" I'm not so\n>> sure, if I have changes to an untracked file I don't wont them\n>> overwritten by default. There is no porcelain equivalent to \"read-tree\n>> --reset --protect-untracked -u\" and I was hoping \"reset --hard\" may\n>> become that porcelain equivalent with a new --force or\n>> --overwrite-untracked option.\n> \n> My (biased, obviously) view is that \"git reset --hard\" is very\n> dangerous and I'm not trying to change that, especially when its\n> behavior has been like this since forever and I'm sure it's used in\n> scripts.\n> \n> Instead \"git restore\" should be used when you need \"git reset --hard\n> HEAD\", the most often use case. And since it's new, changing default\n> behavior is not a problem. Which brings us back to git-restore :)\n\nDoes restore clean up the branch state like reset? It's tricky because \nyou only want to do that if there is no pathspec (or the pathspec is :/ \nor equivalent - I can't remember if restore always requires paths or not)\n\n> But either way, git-restore or git-reset, I still don't see why\n> untracked files are more valuable in this case than tracked ones to\n> change the default. \n\nMy issue is that is easy to see what changes you're going to lose in \ntracked files by running diff. For untracked files diff just says a new \nfile will be created, it ignores the current contents as the path is in \nthe index so it is easy to overwrite changes without realizing. There's \nalso a philosophical point that git should not be stomping on paths that \nit is not tracking though that's a bit moot if a path is tracked in one \nrevision but not another.\n\n> I can see that sometimes you may want to restore\n> just tracked files, or untracked files, almost like filtering with\n> pathspec.\n> \n>> For the various \"foo --abort\" some (most?) are using \"reset --merge\"\n>> which I think declines to overwrite untracked files but rebase uses\n>> \"reset --hard\" which I'd like to change to protect untracked files in\n>> the same way that rebase does for the initial checkout and when picking\n>> commits. I haven't thought about stash.\n> \n> Yeah it looks like cherry-pick and revert use \"reset --merge\" too\n> (reset_for_rollback function). That's all of them. Probably a stupid\n> question, why can't rebase just use \"rebase --merge\" like everybody\n> else?\n\nI'm not sure - if --merge works for the others I can't see why it \nshouldn't work for rebase as well.\n\nBest Wishes\n\nPhillip\n>> Best Wishes\n>>\n>> Phillip\n>>\n> \n> \n"},{"id":"375020","messageId":"CACsJy8A+EjymUsSHD8Wj3y3=SfCkaWtfYhjqv+OyaXpkrQXkVg@mail.gmail.com","threadId":"51019","inReplyTo":"994d4687-39ed-91b9-cdf9-d4be39cbb2ba@gmail.com","subject":"Re: [PATCH 0/2] read-tree: improve untracked file support","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-05-07T11:02:14Z","receivedAt":"2019-05-07T11:02:43Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, May 7, 2019 at 5:02 PM Phillip Wood <phillip.wood123@gmail.com> wrote:\n> > My (biased, obviously) view is that \"git reset --hard\" is very\n> > dangerous and I'm not trying to change that, especially when its\n> > behavior has been like this since forever and I'm sure it's used in\n> > scripts.\n> >\n> > Instead \"git restore\" should be used when you need \"git reset --hard\n> > HEAD\", the most often use case. And since it's new, changing default\n> > behavior is not a problem. Which brings us back to git-restore :)\n>\n> Does restore clean up the branch state like reset? It's tricky because\n> you only want to do that if there is no pathspec (or the pathspec is :/\n> or equivalent - I can't remember if restore always requires paths or not)\n\nNope. git-restore cares about files, not branches. Yes git-restore\nalways requires paths, just in case people type \"git restore\" and\nexpect to see help usage or something.\n\n> > But either way, git-restore or git-reset, I still don't see why\n> > untracked files are more valuable in this case than tracked ones to\n> > change the default.\n>\n> My issue is that is easy to see what changes you're going to lose in\n> tracked files by running diff. For untracked files diff just says a new\n> file will be created, it ignores the current contents as the path is in\n> the index so it is easy to overwrite changes without realizing. There's\n> also a philosophical point that git should not be stomping on paths that\n> it is not tracking though that's a bit moot if a path is tracked in one\n> revision but not another.\n\nAh good point about diff. If only we had \"git reset --dry-run\" (that\nshows the diff, including untracked files; or perhaps --diff would be\na better name for that imaginary option)\n-- \nDuy\n"}]}