From 04988c8d182da945cd9420274f33487157c5636f Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Mon, 27 Sep 2021 16:33:41 +0000 Subject: unpack-trees: introduce preserve_ignored to unpack_trees_options Currently, every caller of unpack_trees() that wants to ensure ignored files are overwritten by default needs to: * allocate unpack_trees_options.dir * flip the DIR_SHOW_IGNORED flag in unpack_trees_options.dir->flags * call setup_standard_excludes AND then after the call to unpack_trees() needs to * call dir_clear() * deallocate unpack_trees_options.dir That's a fair amount of boilerplate, and every caller uses identical code. Make this easier by instead introducing a new boolean value where the default value (0) does what we want so that new callers of unpack_trees() automatically get the appropriate behavior. And move all the handling of unpack_trees_options.dir into unpack_trees() itself. While preserve_ignored = 0 is the behavior we feel is the appropriate default, we defer fixing commands to use the appropriate default until a later commit. So, this commit introduces several locations where we manually set preserve_ignored=1. This makes it clear where code paths were previously preserving ignored files when they should not have been; a future commit will flip these to instead use a value of 0 to get the behavior we want. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- builtin/stash.c | 3 +++ 1 file changed, 3 insertions(+) (limited to 'builtin/stash.c') diff --git a/builtin/stash.c b/builtin/stash.c index 8f42360ca9..88287b890d 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -258,6 +258,9 @@ static int reset_tree(struct object_id *i_tree, int update, int reset) opts.merge = 1; opts.reset = reset; opts.update = update; + if (update && !reset) + /* FIXME: Default should be to remove ignored files */ + opts.preserve_ignored = 1; opts.fn = oneway_merge; if (unpack_trees(nr_trees, t, &opts)) -- cgit v1.3 From 1b5f37334a2603c7134da7accba76276d8d31cf6 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Mon, 27 Sep 2021 16:33:43 +0000 Subject: Remove ignored files by default when they are in the way Change several commands to remove ignored files by default when they are in the way. Since some commands (checkout, merge) take a --no-overwrite-ignore option to allow the user to configure this, and it may make sense to add that option to more commands (and in the case of merge, actually plumb that configuration option through to more of the backends than just the fast-forwarding special case), add little comments about where such flags would be used. Incidentally, this fixes a test failure in t7112. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- builtin/am.c | 3 +-- builtin/clone.c | 3 +-- builtin/merge.c | 3 +-- builtin/reset.c | 3 +-- builtin/stash.c | 3 +-- merge-ort.c | 2 +- reset.c | 3 +-- sequencer.c | 3 +-- t/t7112-reset-submodule.sh | 1 - 9 files changed, 8 insertions(+), 16 deletions(-) (limited to 'builtin/stash.c') diff --git a/builtin/am.c b/builtin/am.c index 567ecd882b..93beb66197 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1920,8 +1920,7 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset) opts.merge = 1; opts.reset = reset; if (!reset) - /* FIXME: Default should be to remove ignored files */ - opts.preserve_ignored = 1; + opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */ opts.fn = twoway_merge; init_tree_desc(&t[0], head->buffer, head->size); init_tree_desc(&t[1], remote->buffer, remote->size); diff --git a/builtin/clone.c b/builtin/clone.c index 2ff9e9ca74..26599f40f2 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -803,8 +803,7 @@ static int checkout(int submodule_progress) opts.update = 1; opts.merge = 1; opts.clone = 1; - /* FIXME: Default should be to remove ignored files */ - opts.preserve_ignored = 1; + opts.preserve_ignored = 0; opts.fn = oneway_merge; opts.verbose_update = (option_verbosity >= 0); opts.src_index = &the_index; diff --git a/builtin/merge.c b/builtin/merge.c index 89c99cf28c..9202587728 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -681,8 +681,7 @@ static int read_tree_trivial(struct object_id *common, struct object_id *head, opts.verbose_update = 1; opts.trivial_merges_only = 1; opts.merge = 1; - /* FIXME: Default should be to remove ignored files */ - opts.preserve_ignored = 1; + opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */ trees[nr_trees] = parse_tree_indirect(common); if (!trees[nr_trees++]) return -1; diff --git a/builtin/reset.c b/builtin/reset.c index 7f38656f01..5df01cc42e 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -67,8 +67,7 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t case KEEP: case MERGE: opts.update = 1; - /* FIXME: Default should be to remove ignored files */ - opts.preserve_ignored = 1; + opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */ break; case HARD: opts.update = 1; diff --git a/builtin/stash.c b/builtin/stash.c index 88287b890d..d60cdaf32f 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -259,8 +259,7 @@ static int reset_tree(struct object_id *i_tree, int update, int reset) opts.reset = reset; opts.update = update; if (update && !reset) - /* FIXME: Default should be to remove ignored files */ - opts.preserve_ignored = 1; + opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */ opts.fn = oneway_merge; if (unpack_trees(nr_trees, t, &opts)) diff --git a/merge-ort.c b/merge-ort.c index 610c3913b5..48fbd52a77 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -4045,7 +4045,7 @@ static int checkout(struct merge_options *opt, unpack_opts.quiet = 0; /* FIXME: sequencer might want quiet? */ unpack_opts.verbose_update = (opt->verbosity > 2); unpack_opts.fn = twoway_merge; - unpack_opts.preserve_ignored = 0; /* FIXME: !opts->overwrite_ignore*/ + unpack_opts.preserve_ignored = 0; /* FIXME: !opts->overwrite_ignore */ parse_tree(prev); init_tree_desc(&trees[0], prev->buffer, prev->size); parse_tree(next); diff --git a/reset.c b/reset.c index 41b3e2d88d..f40a8ecf66 100644 --- a/reset.c +++ b/reset.c @@ -56,8 +56,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action, unpack_tree_opts.fn = reset_hard ? oneway_merge : twoway_merge; unpack_tree_opts.update = 1; unpack_tree_opts.merge = 1; - /* FIXME: Default should be to remove ignored files */ - unpack_tree_opts.preserve_ignored = 1; + unpack_tree_opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */ init_checkout_metadata(&unpack_tree_opts.meta, switch_to_branch, oid, NULL); if (!detach_head) unpack_tree_opts.reset = 1; diff --git a/sequencer.c b/sequencer.c index 695750ef0b..9751e9aa8d 100644 --- a/sequencer.c +++ b/sequencer.c @@ -3690,8 +3690,7 @@ static int do_reset(struct repository *r, unpack_tree_opts.fn = oneway_merge; unpack_tree_opts.merge = 1; unpack_tree_opts.update = 1; - /* FIXME: Default should be to remove ignored files */ - unpack_tree_opts.preserve_ignored = 1; + unpack_tree_opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */ init_checkout_metadata(&unpack_tree_opts.meta, name, &oid, NULL); if (repo_read_index_unmerged(r)) { diff --git a/t/t7112-reset-submodule.sh b/t/t7112-reset-submodule.sh index 19830d9036..a3e2413bc3 100755 --- a/t/t7112-reset-submodule.sh +++ b/t/t7112-reset-submodule.sh @@ -6,7 +6,6 @@ test_description='reset can handle submodules' . "$TEST_DIRECTORY"/lib-submodule-update.sh KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS=1 -KNOWN_FAILURE_SUBMODULE_OVERWRITE_IGNORED_UNTRACKED=1 test_submodule_switch_recursing_with_args "reset --keep" -- cgit v1.3 From 480d3d6bf90a6ec5b8f02d672f1d4027a4889106 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Mon, 27 Sep 2021 16:33:44 +0000 Subject: Change unpack_trees' 'reset' flag into an enum Traditionally, unpack_trees_options->reset was used to signal that it was okay to delete any untracked files in the way. This was used by `git read-tree --reset`, but then started appearing in other places as well. However, many of the other uses should not be deleting untracked files in the way. Change this value to an enum so that a value of 1 (i.e. "true") can be split into two: UNPACK_RESET_PROTECT_UNTRACKED, UNPACK_RESET_OVERWRITE_UNTRACKED In order to catch accidental misuses (i.e. where folks call it the way they traditionally used to), define the special enum value of UNPACK_RESET_INVALID = 1 which will trigger a BUG(). Modify existing callers so that read-tree --reset reset --hard checkout --force continue using the UNPACK_RESET_OVERWRITE_UNTRACKED logic, while other callers, including am checkout without --force stash (though currently dead code; reset always had a value of 0) numerous callers from rebase/sequencer to reset_head() will use the new UNPACK_RESET_PROTECT_UNTRACKED value. Also, note that it has been reported that 'git checkout ' currently also allows overwriting untracked files[1]. That case should also be fixed, but it does not use unpack_trees() and thus is outside the scope of the current changes. [1] https://lore.kernel.org/git/15dad590-087e-5a48-9238-5d2826950506@gmail.com/ Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- builtin/am.c | 5 ++--- builtin/checkout.c | 5 +++-- builtin/read-tree.c | 3 +++ builtin/reset.c | 9 +++++++-- builtin/stash.c | 4 ++-- reset.c | 2 +- t/t2500-untracked-overwriting.sh | 6 +++--- unpack-trees.c | 10 +++++++++- unpack-trees.h | 11 +++++++++-- 9 files changed, 39 insertions(+), 16 deletions(-) (limited to 'builtin/stash.c') diff --git a/builtin/am.c b/builtin/am.c index 93beb66197..ec1c213cb3 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1918,9 +1918,8 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset) opts.dst_index = &the_index; opts.update = 1; opts.merge = 1; - opts.reset = reset; - if (!reset) - opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */ + opts.reset = reset ? UNPACK_RESET_PROTECT_UNTRACKED : 0; + opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */ opts.fn = twoway_merge; init_tree_desc(&t[0], head->buffer, head->size); init_tree_desc(&t[1], remote->buffer, remote->size); diff --git a/builtin/checkout.c b/builtin/checkout.c index 5e7957dd06..cbf73b8c9f 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -646,9 +646,10 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o, opts.head_idx = -1; opts.update = worktree; opts.skip_unmerged = !worktree; - opts.reset = 1; + opts.reset = o->force ? UNPACK_RESET_OVERWRITE_UNTRACKED : + UNPACK_RESET_PROTECT_UNTRACKED; + opts.preserve_ignored = (!o->force && !o->overwrite_ignore); opts.merge = 1; - opts.preserve_ignored = 0; opts.fn = oneway_merge; opts.verbose_update = o->show_progress; opts.src_index = &the_index; diff --git a/builtin/read-tree.c b/builtin/read-tree.c index 443d206eca..2109c4c9e5 100644 --- a/builtin/read-tree.c +++ b/builtin/read-tree.c @@ -166,6 +166,9 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix) if (1 < opts.merge + opts.reset + prefix_set) die("Which one? -m, --reset, or --prefix?"); + if (opts.reset) + opts.reset = UNPACK_RESET_OVERWRITE_UNTRACKED; + /* * NEEDSWORK * diff --git a/builtin/reset.c b/builtin/reset.c index 5df01cc42e..7393595349 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -71,9 +71,14 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t break; case HARD: opts.update = 1; - /* fallthrough */ + opts.reset = UNPACK_RESET_OVERWRITE_UNTRACKED; + break; + case MIXED: + opts.reset = UNPACK_RESET_PROTECT_UNTRACKED; + /* but opts.update=0, so working tree not updated */ + break; default: - opts.reset = 1; + BUG("invalid reset_type passed to reset_index"); } read_cache_unmerged(); diff --git a/builtin/stash.c b/builtin/stash.c index d60cdaf32f..0e3662a230 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -256,9 +256,9 @@ static int reset_tree(struct object_id *i_tree, int update, int reset) opts.src_index = &the_index; opts.dst_index = &the_index; opts.merge = 1; - opts.reset = reset; + opts.reset = reset ? UNPACK_RESET_PROTECT_UNTRACKED : 0; opts.update = update; - if (update && !reset) + if (update) opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */ opts.fn = oneway_merge; diff --git a/reset.c b/reset.c index f40a8ecf66..f214df3d96 100644 --- a/reset.c +++ b/reset.c @@ -59,7 +59,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action, unpack_tree_opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */ init_checkout_metadata(&unpack_tree_opts.meta, switch_to_branch, oid, NULL); if (!detach_head) - unpack_tree_opts.reset = 1; + unpack_tree_opts.reset = UNPACK_RESET_PROTECT_UNTRACKED; if (repo_read_index_unmerged(r) < 0) { ret = error(_("could not read index")); diff --git a/t/t2500-untracked-overwriting.sh b/t/t2500-untracked-overwriting.sh index 2412d121ea..18604360df 100755 --- a/t/t2500-untracked-overwriting.sh +++ b/t/t2500-untracked-overwriting.sh @@ -92,7 +92,7 @@ test_setup_checkout_m () { ) } -test_expect_failure 'checkout -m does not nuke untracked file' ' +test_expect_success 'checkout -m does not nuke untracked file' ' test_setup_checkout_m && ( cd checkout && @@ -138,7 +138,7 @@ test_setup_sequencing () { ) } -test_expect_failure 'git rebase --abort and untracked files' ' +test_expect_success 'git rebase --abort and untracked files' ' test_setup_sequencing rebase_abort_and_untracked && ( cd sequencing_rebase_abort_and_untracked && @@ -155,7 +155,7 @@ test_expect_failure 'git rebase --abort and untracked files' ' ) ' -test_expect_failure 'git rebase fast forwarding and untracked files' ' +test_expect_success 'git rebase fast forwarding and untracked files' ' test_setup_sequencing rebase_fast_forward_and_untracked && ( cd sequencing_rebase_fast_forward_and_untracked && diff --git a/unpack-trees.c b/unpack-trees.c index 9ccb991084..3fff506156 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -1694,6 +1694,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options int free_pattern_list = 0; struct dir_struct dir = DIR_INIT; + if (o->reset == UNPACK_RESET_INVALID) + BUG("o->reset had a value of 1; should be UNPACK_TREES_*_UNTRACKED"); + if (len > MAX_UNPACK_TREES) die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES); if (o->dir) @@ -1708,6 +1711,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options ensure_full_index(o->dst_index); } + if (o->reset == UNPACK_RESET_OVERWRITE_UNTRACKED && + o->preserve_ignored) + BUG("UNPACK_RESET_OVERWRITE_UNTRACKED incompatible with preserved ignored files"); + if (!o->preserve_ignored) { o->dir = &dir; o->dir->flags |= DIR_SHOW_IGNORED; @@ -2231,7 +2238,8 @@ static int verify_absent_1(const struct cache_entry *ce, int len; struct stat st; - if (o->index_only || o->reset || !o->update) + if (o->index_only || !o->update || + o->reset == UNPACK_RESET_OVERWRITE_UNTRACKED) return 0; len = check_leading_path(ce->name, ce_namelen(ce), 0); diff --git a/unpack-trees.h b/unpack-trees.h index 61da25dafe..71ffb7eeb0 100644 --- a/unpack-trees.h +++ b/unpack-trees.h @@ -45,9 +45,15 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts, */ void clear_unpack_trees_porcelain(struct unpack_trees_options *opts); +enum unpack_trees_reset_type { + UNPACK_RESET_NONE = 0, /* traditional "false" value; still valid */ + UNPACK_RESET_INVALID = 1, /* "true" no longer valid; use below values */ + UNPACK_RESET_PROTECT_UNTRACKED, + UNPACK_RESET_OVERWRITE_UNTRACKED +}; + struct unpack_trees_options { - unsigned int reset, - merge, + unsigned int merge, update, preserve_ignored, clone, @@ -65,6 +71,7 @@ struct unpack_trees_options { exiting_early, show_all_errors, dry_run; + enum unpack_trees_reset_type reset; const char *prefix; int cache_bottom; struct pathspec *pathspec; -- cgit v1.3 From 94b7f1563ace91af823125e5b8895cb24b2c0e4a Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Mon, 27 Sep 2021 16:33:47 +0000 Subject: Comment important codepaths regarding nuking untracked files/dirs In the last few commits we focused on code in unpack-trees.c that mistakenly removed untracked files or directories. There may be more of those, but in this commit we change our focus: callers of toplevel commands that are expected to remove untracked files or directories. As noted previously, we have toplevel commands that are expected to delete untracked files such as 'read-tree --reset', 'reset --hard', and 'checkout --force'. However, that does not mean that other highlevel commands that happen to call these other commands thought about or conveyed to users the possibility that untracked files could be removed. Audit the code for such callsites, and add comments near existing callsites to mention whether these are safe or not. My auditing is somewhat incomplete, though; it skipped several cases: * git-rebase--preserve-merges.sh: is in the process of being deprecated/removed, so I won't leave a note that there are likely more bugs in that script. * contrib/git-new-workdir: why is the -f flag being used in a new empty directory?? It shouldn't hurt, but it seems useless. * git-p4.py: Don't see why -f is needed for a new dir (maybe it's not and is just superfluous), but I'm not at all familiar with the p4 stuff * git-archimport.perl: Don't care; arch is long since dead * git-cvs*.perl: Don't care; cvs is long since dead Also, the reset --hard in builtin/worktree.c looks safe, due to only running in an empty directory. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- builtin/stash.c | 1 + builtin/submodule--helper.c | 4 ++++ contrib/rerere-train.sh | 2 +- submodule.c | 1 + 4 files changed, 7 insertions(+), 1 deletion(-) (limited to 'builtin/stash.c') diff --git a/builtin/stash.c b/builtin/stash.c index 0e3662a230..aa31163a5a 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -1521,6 +1521,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q } else { struct child_process cp = CHILD_PROCESS_INIT; cp.git_cmd = 1; + /* BUG: this nukes untracked files in the way */ strvec_pushl(&cp.args, "reset", "--hard", "-q", "--no-recurse-submodules", NULL); if (run_command(&cp)) { diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 4da9781b99..549129bc1b 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2864,6 +2864,10 @@ static int add_submodule(const struct add_data *add_data) prepare_submodule_repo_env(&cp.env_array); cp.git_cmd = 1; cp.dir = add_data->sm_path; + /* + * NOTE: we only get here if add_data->force is true, so + * passing --force to checkout is reasonable. + */ strvec_pushl(&cp.args, "checkout", "-f", "-q", NULL); if (add_data->branch) { diff --git a/contrib/rerere-train.sh b/contrib/rerere-train.sh index eeee45dd34..75125d6ae0 100755 --- a/contrib/rerere-train.sh +++ b/contrib/rerere-train.sh @@ -91,7 +91,7 @@ do git checkout -q $commit -- . git rerere fi - git reset -q --hard + git reset -q --hard # Might nuke untracked files... done if test -z "$branch" diff --git a/submodule.c b/submodule.c index 8e611fe1db..a9b71d585c 100644 --- a/submodule.c +++ b/submodule.c @@ -1866,6 +1866,7 @@ static void submodule_reset_index(const char *path) strvec_pushf(&cp.args, "--super-prefix=%s%s/", get_super_prefix_or_empty(), path); + /* TODO: determine if this might overwright untracked files */ strvec_pushl(&cp.args, "read-tree", "-u", "--reset", NULL); strvec_push(&cp.args, empty_tree_oid_hex()); -- cgit v1.3