From 446cc5544a3cb8dbd0ddac5bc5b160a687eb6471 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Fri, 24 Sep 2021 06:37:04 +0000 Subject: [PATCH 01/11] t2500: add various tests for nuking untracked files Noting that unpack_trees treats reset=1 & update=1 as license to nuke untracked files, I looked for code paths that use this combination and tried to generate testcases which demonstrated unintentional loss of untracked files and directories. I found several. I also include testcases for `git reset --{hard,merge,keep}`. A hard reset is perhaps the most direct test of unpack_tree's reset=1 behavior, but we cannot make `git reset --hard` preserve untracked files without some migration work. Also, the two commands `checkout --force` (because of the --force) and `read-tree --reset` (because it's plumbing and we need to keep it backward compatible) were left out as we expect those to continue removing untracked files and directories. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- t/t2500-untracked-overwriting.sh | 244 +++++++++++++++++++++++++++++++ 1 file changed, 244 insertions(+) create mode 100755 t/t2500-untracked-overwriting.sh diff --git a/t/t2500-untracked-overwriting.sh b/t/t2500-untracked-overwriting.sh new file mode 100755 index 0000000000..2412d121ea --- /dev/null +++ b/t/t2500-untracked-overwriting.sh @@ -0,0 +1,244 @@ +#!/bin/sh + +test_description='Test handling of overwriting untracked files' + +. ./test-lib.sh + +test_setup_reset () { + git init reset_$1 && + ( + cd reset_$1 && + test_commit init && + + git branch stable && + git branch work && + + git checkout work && + test_commit foo && + + git checkout stable + ) +} + +test_expect_success 'reset --hard will nuke untracked files/dirs' ' + test_setup_reset hard && + ( + cd reset_hard && + git ls-tree -r stable && + git log --all --name-status --oneline && + git ls-tree -r work && + + mkdir foo.t && + echo precious >foo.t/file && + echo foo >expect && + + git reset --hard work && + + # check that untracked directory foo.t/ was nuked + test_path_is_file foo.t && + test_cmp expect foo.t + ) +' + +test_expect_success 'reset --merge will preserve untracked files/dirs' ' + test_setup_reset merge && + ( + cd reset_merge && + + mkdir foo.t && + echo precious >foo.t/file && + cp foo.t/file expect && + + test_must_fail git reset --merge work 2>error && + test_cmp expect foo.t/file && + grep "Updating .foo.t. would lose untracked files" error + ) +' + +test_expect_success 'reset --keep will preserve untracked files/dirs' ' + test_setup_reset keep && + ( + cd reset_keep && + + mkdir foo.t && + echo precious >foo.t/file && + cp foo.t/file expect && + + test_must_fail git reset --merge work 2>error && + test_cmp expect foo.t/file && + grep "Updating.*foo.t.*would lose untracked files" error + ) +' + +test_setup_checkout_m () { + git init checkout && + ( + cd checkout && + test_commit init && + + test_write_lines file has some >filler && + git add filler && + git commit -m filler && + + git branch stable && + + git switch -c work && + echo stuff >notes.txt && + test_write_lines file has some words >filler && + git add notes.txt filler && + git commit -m filler && + + git checkout stable + ) +} + +test_expect_failure 'checkout -m does not nuke untracked file' ' + test_setup_checkout_m && + ( + cd checkout && + + # Tweak filler + test_write_lines this file has some >filler && + # Make an untracked file, save its contents in "expect" + echo precious >notes.txt && + cp notes.txt expect && + + test_must_fail git checkout -m work && + test_cmp expect notes.txt + ) +' + +test_setup_sequencing () { + git init sequencing_$1 && + ( + cd sequencing_$1 && + test_commit init && + + test_write_lines this file has some words >filler && + git add filler && + git commit -m filler && + + mkdir -p foo/bar && + test_commit foo/bar/baz && + + git branch simple && + git branch fooey && + + git checkout fooey && + git rm foo/bar/baz.t && + echo stuff >>filler && + git add -u && + git commit -m "changes" && + + git checkout simple && + echo items >>filler && + echo newstuff >>newfile && + git add filler newfile && + git commit -m another + ) +} + +test_expect_failure 'git rebase --abort and untracked files' ' + test_setup_sequencing rebase_abort_and_untracked && + ( + cd sequencing_rebase_abort_and_untracked && + git checkout fooey && + test_must_fail git rebase simple && + + cat init.t && + git rm init.t && + echo precious >init.t && + cp init.t expect && + git status --porcelain && + test_must_fail git rebase --abort && + test_cmp expect init.t + ) +' + +test_expect_failure 'git rebase fast forwarding and untracked files' ' + test_setup_sequencing rebase_fast_forward_and_untracked && + ( + cd sequencing_rebase_fast_forward_and_untracked && + git checkout init && + echo precious >filler && + cp filler expect && + test_must_fail git rebase init simple && + test_cmp expect filler + ) +' + +test_expect_failure 'git rebase --autostash and untracked files' ' + test_setup_sequencing rebase_autostash_and_untracked && + ( + cd sequencing_rebase_autostash_and_untracked && + git checkout simple && + git rm filler && + mkdir filler && + echo precious >filler/file && + cp filler/file expect && + git rebase --autostash init && + test_path_is_file filler/file + ) +' + +test_expect_failure 'git stash and untracked files' ' + test_setup_sequencing stash_and_untracked_files && + ( + cd sequencing_stash_and_untracked_files && + git checkout simple && + git rm filler && + mkdir filler && + echo precious >filler/file && + cp filler/file expect && + git status --porcelain && + git stash push && + git status --porcelain && + test_path_is_file filler/file + ) +' + +test_expect_failure 'git am --abort and untracked dir vs. unmerged file' ' + test_setup_sequencing am_abort_and_untracked && + ( + cd sequencing_am_abort_and_untracked && + git format-patch -1 --stdout fooey >changes.mbox && + test_must_fail git am --3way changes.mbox && + + # Delete the conflicted file; we will stage and commit it later + rm filler && + + # Put an unrelated untracked directory there + mkdir filler && + echo foo >filler/file1 && + echo bar >filler/file2 && + + test_must_fail git am --abort 2>errors && + test_path_is_dir filler && + grep "Updating .filler. would lose untracked files in it" errors + ) +' + +test_expect_failure 'git am --skip and untracked dir vs deleted file' ' + test_setup_sequencing am_skip_and_untracked && + ( + cd sequencing_am_skip_and_untracked && + git checkout fooey && + git format-patch -1 --stdout simple >changes.mbox && + test_must_fail git am --3way changes.mbox && + + # Delete newfile + rm newfile && + + # Put an unrelated untracked directory there + mkdir newfile && + echo foo >newfile/file1 && + echo bar >newfile/file2 && + + # Change our mind about resolutions, just skip this patch + test_must_fail git am --skip 2>errors && + test_path_is_dir newfile && + grep "Updating .newfile. would lose untracked files in it" errors + ) +' + +test_done From c512d27e787e9eb325731ab6aa7ac126f8cf6126 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Mon, 27 Sep 2021 16:33:39 +0000 Subject: [PATCH 02/11] checkout, read-tree: fix leak of unpack_trees_options.dir Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- builtin/checkout.c | 4 ++++ builtin/read-tree.c | 5 +++++ 2 files changed, 9 insertions(+) diff --git a/builtin/checkout.c b/builtin/checkout.c index 8c69dcdf72..5335435d61 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -760,6 +760,10 @@ static int merge_working_tree(const struct checkout_opts *opts, init_tree_desc(&trees[1], tree->buffer, tree->size); ret = unpack_trees(2, trees, &topts); + if (topts.dir) { + dir_clear(topts.dir); + FREE_AND_NULL(topts.dir); + } clear_unpack_trees_porcelain(&topts); if (ret == -1) { /* diff --git a/builtin/read-tree.c b/builtin/read-tree.c index 485e7b0479..96102c222b 100644 --- a/builtin/read-tree.c +++ b/builtin/read-tree.c @@ -250,6 +250,11 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix) if (unpack_trees(nr_trees, t, &opts)) return 128; + if (opts.dir) { + dir_clear(opts.dir); + FREE_AND_NULL(opts.dir); + } + if (opts.debug_unpack || opts.dry_run) return 0; /* do not write the index out */ From 491a7575f188cbf6cfb5be75981a40beb4c22b44 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Mon, 27 Sep 2021 16:33:40 +0000 Subject: [PATCH 03/11] read-tree, merge-recursive: overwrite ignored files by default This fixes a long-standing patchwork of ignored files handling in read-tree and merge-recursive, called out and suggested by Junio long ago. Quoting from commit dcf0c16ef1 ("core.excludesfile clean-up" 2007-11-16): git-read-tree takes --exclude-per-directory=, not because the flexibility was needed. Again, this was because the option predates the standardization of the ignore files. ... On the other hand, I think it makes perfect sense to fix git-read-tree, git-merge-recursive and git-clean to follow the same rule as other commands. I do not think of a valid use case to give an exclude-per-directory that is nonstandard to read-tree command, outside a "negative" test in the t1004 test script. This patch is the first step to untangle this mess. The next step would be to teach read-tree, merge-recursive and clean (in C) to use setup_standard_excludes(). History shows each of these were partially or fully fixed: * clean was taught the new trick in 1617adc7a0 ("Teach git clean to use setup_standard_excludes()", 2007-11-14). * read-tree was primarily used by checkout & merge scripts. checkout and merge later became builtins and were both fixed to use the new setup_standard_excludes() handling in fc001b526c ("checkout,merge: loosen overwriting untracked file check based on info/exclude", 2011-11-27). So the primary users were fixed, though read-tree itself was not. * merge-recursive has now been replaced as the default merge backend by merge-ort. merge-ort fixed this by using setup_standard_excludes() starting early in its implementation; see commit 6681ce5cf6 ("merge-ort: add implementation of checkout()", 2020-12-13), largely due to its design depending on checkout() and thus being influenced by the checkout code. However, merge-recursive itself was not fixed here, in part because its design meant it had difficulty differentiating between untracked files, ignored files, leftover tracked files that haven't been removed yet due to order of processing files, and files written by itself due to collisions). Make the conversion more complete by now handling read-tree and handling at least the unpack_trees() portion of merge-recursive. While merge-recursive is on its way out, fixing the unpack_trees() portion is easy and facilitates some of the later changes in this series. Note that fixing read-tree makes the --exclude-per-directory option to read-tree useless, so we remove it from the documentation (though we continue to accept it if passed). The read-tree changes happen to fix a bug in t1013. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- Documentation/git-read-tree.txt | 18 +----------------- builtin/read-tree.c | 25 ++++++++++--------------- merge-recursive.c | 11 ++++++++++- t/t1013-read-tree-submodule.sh | 1 - 4 files changed, 21 insertions(+), 34 deletions(-) diff --git a/Documentation/git-read-tree.txt b/Documentation/git-read-tree.txt index 5fa8bab64c..0222a27c5a 100644 --- a/Documentation/git-read-tree.txt +++ b/Documentation/git-read-tree.txt @@ -10,8 +10,7 @@ SYNOPSIS -------- [verse] 'git read-tree' [[-m [--trivial] [--aggressive] | --reset | --prefix=] - [-u [--exclude-per-directory=] | -i]] - [--index-output=] [--no-sparse-checkout] + [-u | -i]] [--index-output=] [--no-sparse-checkout] (--empty | [ []]) @@ -88,21 +87,6 @@ OPTIONS The command will refuse to overwrite entries that already existed in the original index file. ---exclude-per-directory=:: - When running the command with `-u` and `-m` options, the - merge result may need to overwrite paths that are not - tracked in the current branch. The command usually - refuses to proceed with the merge to avoid losing such a - path. However this safety valve sometimes gets in the - way. For example, it often happens that the other - branch added a file that used to be a generated file in - your branch, and the safety valve triggers when you try - to switch to that branch after you ran `make` but before - running `make clean` to remove the generated file. This - option tells the command to read per-directory exclude - file (usually '.gitignore') and allows such an untracked - but explicitly ignored file to be overwritten. - --index-output=:: Instead of writing the results out to `$GIT_INDEX_FILE`, write the resulting index in the named file. While the diff --git a/builtin/read-tree.c b/builtin/read-tree.c index 96102c222b..73cb957a69 100644 --- a/builtin/read-tree.c +++ b/builtin/read-tree.c @@ -38,7 +38,7 @@ static int list_tree(struct object_id *oid) } static const char * const read_tree_usage[] = { - N_("git read-tree [(-m [--trivial] [--aggressive] | --reset | --prefix=) [-u [--exclude-per-directory=] | -i]] [--no-sparse-checkout] [--index-output=] (--empty | [ []])"), + N_("git read-tree [(-m [--trivial] [--aggressive] | --reset | --prefix=) [-u | -i]] [--no-sparse-checkout] [--index-output=] (--empty | [ []])"), NULL }; @@ -53,24 +53,16 @@ static int index_output_cb(const struct option *opt, const char *arg, static int exclude_per_directory_cb(const struct option *opt, const char *arg, int unset) { - struct dir_struct *dir; struct unpack_trees_options *opts; BUG_ON_OPT_NEG(unset); opts = (struct unpack_trees_options *)opt->value; - if (opts->dir) - die("more than one --exclude-per-directory given."); - - dir = xcalloc(1, sizeof(*opts->dir)); - dir->flags |= DIR_SHOW_IGNORED; - dir->exclude_per_dir = arg; - opts->dir = dir; - /* We do not need to nor want to do read-directory - * here; we are merely interested in reusing the - * per directory ignore stack mechanism. - */ + if (!opts->update) + die("--exclude-per-directory is meaningless unless -u"); + if (strcmp(arg, ".gitignore")) + die("--exclude-per-directory argument must be .gitignore"); return 0; } @@ -209,8 +201,11 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix) if ((opts.update || opts.index_only) && !opts.merge) die("%s is meaningless without -m, --reset, or --prefix", opts.update ? "-u" : "-i"); - if ((opts.dir && !opts.update)) - die("--exclude-per-directory is meaningless unless -u"); + if (opts.update && !opts.reset) { + CALLOC_ARRAY(opts.dir, 1); + opts.dir->flags |= DIR_SHOW_IGNORED; + setup_standard_excludes(opts.dir); + } if (opts.merge && !opts.index_only) setup_work_tree(); diff --git a/merge-recursive.c b/merge-recursive.c index 840599fd53..2b791d1afb 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -408,8 +408,13 @@ static int unpack_trees_start(struct merge_options *opt, memset(&opt->priv->unpack_opts, 0, sizeof(opt->priv->unpack_opts)); if (opt->priv->call_depth) opt->priv->unpack_opts.index_only = 1; - else + else { opt->priv->unpack_opts.update = 1; + /* FIXME: should only do this if !overwrite_ignore */ + CALLOC_ARRAY(opt->priv->unpack_opts.dir, 1); + opt->priv->unpack_opts.dir->flags |= DIR_SHOW_IGNORED; + setup_standard_excludes(opt->priv->unpack_opts.dir); + } opt->priv->unpack_opts.merge = 1; opt->priv->unpack_opts.head_idx = 2; opt->priv->unpack_opts.fn = threeway_merge; @@ -423,6 +428,10 @@ static int unpack_trees_start(struct merge_options *opt, init_tree_desc_from_tree(t+2, merge); rc = unpack_trees(3, t, &opt->priv->unpack_opts); + if (opt->priv->unpack_opts.dir) { + dir_clear(opt->priv->unpack_opts.dir); + FREE_AND_NULL(opt->priv->unpack_opts.dir); + } cache_tree_free(&opt->repo->index->cache_tree); /* diff --git a/t/t1013-read-tree-submodule.sh b/t/t1013-read-tree-submodule.sh index b6df7444c0..bfc90d4cf2 100755 --- a/t/t1013-read-tree-submodule.sh +++ b/t/t1013-read-tree-submodule.sh @@ -6,7 +6,6 @@ test_description='read-tree 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 "read-tree -u -m" From 04988c8d182da945cd9420274f33487157c5636f Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Mon, 27 Sep 2021 16:33:41 +0000 Subject: [PATCH 04/11] 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/am.c | 3 +++ builtin/checkout.c | 11 ++--------- builtin/clone.c | 2 ++ builtin/merge.c | 2 ++ builtin/read-tree.c | 13 +++---------- builtin/reset.c | 2 ++ builtin/stash.c | 3 +++ merge-ort.c | 8 +------- merge-recursive.c | 8 +------- merge.c | 8 +------- reset.c | 2 ++ sequencer.c | 2 ++ unpack-trees.c | 10 ++++++++++ unpack-trees.h | 1 + 14 files changed, 35 insertions(+), 40 deletions(-) diff --git a/builtin/am.c b/builtin/am.c index 72cb3cc070..567ecd882b 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1919,6 +1919,9 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset) opts.update = 1; opts.merge = 1; opts.reset = reset; + if (!reset) + /* FIXME: Default should be to remove ignored files */ + opts.preserve_ignored = 1; 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 5335435d61..5e7957dd06 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -648,6 +648,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o, opts.skip_unmerged = !worktree; opts.reset = 1; opts.merge = 1; + opts.preserve_ignored = 0; opts.fn = oneway_merge; opts.verbose_update = o->show_progress; opts.src_index = &the_index; @@ -746,11 +747,7 @@ static int merge_working_tree(const struct checkout_opts *opts, new_branch_info->commit ? &new_branch_info->commit->object.oid : &new_branch_info->oid, NULL); - if (opts->overwrite_ignore) { - topts.dir = xcalloc(1, sizeof(*topts.dir)); - topts.dir->flags |= DIR_SHOW_IGNORED; - setup_standard_excludes(topts.dir); - } + topts.preserve_ignored = !opts->overwrite_ignore; tree = parse_tree_indirect(old_branch_info->commit ? &old_branch_info->commit->object.oid : the_hash_algo->empty_tree); @@ -760,10 +757,6 @@ static int merge_working_tree(const struct checkout_opts *opts, init_tree_desc(&trees[1], tree->buffer, tree->size); ret = unpack_trees(2, trees, &topts); - if (topts.dir) { - dir_clear(topts.dir); - FREE_AND_NULL(topts.dir); - } clear_unpack_trees_porcelain(&topts); if (ret == -1) { /* diff --git a/builtin/clone.c b/builtin/clone.c index b93bcd460e..2ff9e9ca74 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -803,6 +803,8 @@ 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.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 d2c52b6e97..89c99cf28c 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -681,6 +681,8 @@ 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; trees[nr_trees] = parse_tree_indirect(common); if (!trees[nr_trees++]) return -1; diff --git a/builtin/read-tree.c b/builtin/read-tree.c index 73cb957a69..443d206eca 100644 --- a/builtin/read-tree.c +++ b/builtin/read-tree.c @@ -201,11 +201,9 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix) if ((opts.update || opts.index_only) && !opts.merge) die("%s is meaningless without -m, --reset, or --prefix", opts.update ? "-u" : "-i"); - if (opts.update && !opts.reset) { - CALLOC_ARRAY(opts.dir, 1); - opts.dir->flags |= DIR_SHOW_IGNORED; - setup_standard_excludes(opts.dir); - } + if (opts.update && !opts.reset) + opts.preserve_ignored = 0; + /* otherwise, opts.preserve_ignored is irrelevant */ if (opts.merge && !opts.index_only) setup_work_tree(); @@ -245,11 +243,6 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix) if (unpack_trees(nr_trees, t, &opts)) return 128; - if (opts.dir) { - dir_clear(opts.dir); - FREE_AND_NULL(opts.dir); - } - if (opts.debug_unpack || opts.dry_run) return 0; /* do not write the index out */ diff --git a/builtin/reset.c b/builtin/reset.c index 51c9e2f43f..7f38656f01 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -67,6 +67,8 @@ 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; break; case HARD: opts.update = 1; 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)) diff --git a/merge-ort.c b/merge-ort.c index 515dc39b7f..610c3913b5 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -4045,11 +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; - if (1/* FIXME: opts->overwrite_ignore*/) { - CALLOC_ARRAY(unpack_opts.dir, 1); - unpack_opts.dir->flags |= DIR_SHOW_IGNORED; - setup_standard_excludes(unpack_opts.dir); - } + unpack_opts.preserve_ignored = 0; /* FIXME: !opts->overwrite_ignore*/ parse_tree(prev); init_tree_desc(&trees[0], prev->buffer, prev->size); parse_tree(next); @@ -4057,8 +4053,6 @@ static int checkout(struct merge_options *opt, ret = unpack_trees(2, trees, &unpack_opts); clear_unpack_trees_porcelain(&unpack_opts); - dir_clear(unpack_opts.dir); - FREE_AND_NULL(unpack_opts.dir); return ret; } diff --git a/merge-recursive.c b/merge-recursive.c index 2b791d1afb..94112aaabe 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -411,9 +411,7 @@ static int unpack_trees_start(struct merge_options *opt, else { opt->priv->unpack_opts.update = 1; /* FIXME: should only do this if !overwrite_ignore */ - CALLOC_ARRAY(opt->priv->unpack_opts.dir, 1); - opt->priv->unpack_opts.dir->flags |= DIR_SHOW_IGNORED; - setup_standard_excludes(opt->priv->unpack_opts.dir); + opt->priv->unpack_opts.preserve_ignored = 0; } opt->priv->unpack_opts.merge = 1; opt->priv->unpack_opts.head_idx = 2; @@ -428,10 +426,6 @@ static int unpack_trees_start(struct merge_options *opt, init_tree_desc_from_tree(t+2, merge); rc = unpack_trees(3, t, &opt->priv->unpack_opts); - if (opt->priv->unpack_opts.dir) { - dir_clear(opt->priv->unpack_opts.dir); - FREE_AND_NULL(opt->priv->unpack_opts.dir); - } cache_tree_free(&opt->repo->index->cache_tree); /* diff --git a/merge.c b/merge.c index 6e736881d9..2382ff66d3 100644 --- a/merge.c +++ b/merge.c @@ -53,7 +53,6 @@ int checkout_fast_forward(struct repository *r, struct unpack_trees_options opts; struct tree_desc t[MAX_UNPACK_TREES]; int i, nr_trees = 0; - struct dir_struct dir = DIR_INIT; struct lock_file lock_file = LOCK_INIT; refresh_index(r->index, REFRESH_QUIET, NULL, NULL, NULL); @@ -80,11 +79,7 @@ int checkout_fast_forward(struct repository *r, } memset(&opts, 0, sizeof(opts)); - if (overwrite_ignore) { - dir.flags |= DIR_SHOW_IGNORED; - setup_standard_excludes(&dir); - opts.dir = &dir; - } + opts.preserve_ignored = !overwrite_ignore; opts.head_idx = 1; opts.src_index = r->index; @@ -101,7 +96,6 @@ int checkout_fast_forward(struct repository *r, clear_unpack_trees_porcelain(&opts); return -1; } - dir_clear(&dir); clear_unpack_trees_porcelain(&opts); if (write_locked_index(r->index, &lock_file, COMMIT_LOCK)) diff --git a/reset.c b/reset.c index 79310ae071..41b3e2d88d 100644 --- a/reset.c +++ b/reset.c @@ -56,6 +56,8 @@ 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; 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 6831663692..695750ef0b 100644 --- a/sequencer.c +++ b/sequencer.c @@ -3690,6 +3690,8 @@ 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; init_checkout_metadata(&unpack_tree_opts.meta, name, &oid, NULL); if (repo_read_index_unmerged(r)) { diff --git a/unpack-trees.c b/unpack-trees.c index a903e71386..0f0d8ab718 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -1705,6 +1705,12 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options ensure_full_index(o->dst_index); } + if (!o->preserve_ignored) { + CALLOC_ARRAY(o->dir, 1); + o->dir->flags |= DIR_SHOW_IGNORED; + setup_standard_excludes(o->dir); + } + if (!core_apply_sparse_checkout || !o->update) o->skip_sparse_checkout = 1; if (!o->skip_sparse_checkout && !o->pl) { @@ -1866,6 +1872,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options done: if (free_pattern_list) clear_pattern_list(&pl); + if (o->dir) { + dir_clear(o->dir); + FREE_AND_NULL(o->dir); + } trace2_region_leave("unpack_trees", "unpack_trees", the_repository); trace_performance_leave("unpack_trees"); return ret; diff --git a/unpack-trees.h b/unpack-trees.h index 2d88b19dca..f98cfd49d7 100644 --- a/unpack-trees.h +++ b/unpack-trees.h @@ -49,6 +49,7 @@ struct unpack_trees_options { unsigned int reset, merge, update, + preserve_ignored, clone, index_only, nontrivial_merge, From c42e0b64093306d59372df288f9b4086290623f5 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Mon, 27 Sep 2021 16:33:42 +0000 Subject: [PATCH 05/11] unpack-trees: make dir an internal-only struct Avoid accidental misuse or confusion over ownership by clearly making unpack_trees_options.dir an internal-only variable. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- unpack-trees.c | 7 +++++-- unpack-trees.h | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/unpack-trees.c b/unpack-trees.c index 0f0d8ab718..9ccb991084 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -1692,9 +1692,12 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options static struct cache_entry *dfc; struct pattern_list pl; int free_pattern_list = 0; + struct dir_struct dir = DIR_INIT; if (len > MAX_UNPACK_TREES) die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES); + if (o->dir) + BUG("o->dir is for internal use only"); trace_performance_enter(); trace2_region_enter("unpack_trees", "unpack_trees", the_repository); @@ -1706,7 +1709,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options } if (!o->preserve_ignored) { - CALLOC_ARRAY(o->dir, 1); + o->dir = &dir; o->dir->flags |= DIR_SHOW_IGNORED; setup_standard_excludes(o->dir); } @@ -1874,7 +1877,7 @@ done: clear_pattern_list(&pl); if (o->dir) { dir_clear(o->dir); - FREE_AND_NULL(o->dir); + o->dir = NULL; } trace2_region_leave("unpack_trees", "unpack_trees", the_repository); trace_performance_leave("unpack_trees"); diff --git a/unpack-trees.h b/unpack-trees.h index f98cfd49d7..61da25dafe 100644 --- a/unpack-trees.h +++ b/unpack-trees.h @@ -67,7 +67,6 @@ struct unpack_trees_options { dry_run; const char *prefix; int cache_bottom; - struct dir_struct *dir; struct pathspec *pathspec; merge_fn_t fn; const char *msgs[NB_UNPACK_TREES_WARNING_TYPES]; @@ -89,6 +88,7 @@ struct unpack_trees_options { struct index_state result; struct pattern_list *pl; /* for internal use */ + struct dir_struct *dir; /* for internal use only */ struct checkout_metadata meta; }; From 1b5f37334a2603c7134da7accba76276d8d31cf6 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Mon, 27 Sep 2021 16:33:43 +0000 Subject: [PATCH 06/11] 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(-) 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" From 480d3d6bf90a6ec5b8f02d672f1d4027a4889106 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Mon, 27 Sep 2021 16:33:44 +0000 Subject: [PATCH 07/11] 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(-) 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; From 1fdd51aa13c27403c37b57ead32ef79b81d8128b Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Mon, 27 Sep 2021 16:33:45 +0000 Subject: [PATCH 08/11] unpack-trees: avoid nuking untracked dir in way of unmerged file Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- t/t2500-untracked-overwriting.sh | 2 +- unpack-trees.c | 35 ++++++++++++++++++++++++++++---- 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/t/t2500-untracked-overwriting.sh b/t/t2500-untracked-overwriting.sh index 18604360df..5ec66058cf 100755 --- a/t/t2500-untracked-overwriting.sh +++ b/t/t2500-untracked-overwriting.sh @@ -197,7 +197,7 @@ test_expect_failure 'git stash and untracked files' ' ) ' -test_expect_failure 'git am --abort and untracked dir vs. unmerged file' ' +test_expect_success 'git am --abort and untracked dir vs. unmerged file' ' test_setup_sequencing am_abort_and_untracked && ( cd sequencing_am_abort_and_untracked && diff --git a/unpack-trees.c b/unpack-trees.c index 3fff506156..821f532c85 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -2176,9 +2176,15 @@ static int icase_exists(struct unpack_trees_options *o, const char *name, int le return src && !ie_match_stat(o->src_index, src, st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE); } +enum absent_checking_type { + COMPLETELY_ABSENT, + ABSENT_ANY_DIRECTORY +}; + static int check_ok_to_remove(const char *name, int len, int dtype, const struct cache_entry *ce, struct stat *st, enum unpack_trees_error_types error_type, + enum absent_checking_type absent_type, struct unpack_trees_options *o) { const struct cache_entry *result; @@ -2213,6 +2219,10 @@ static int check_ok_to_remove(const char *name, int len, int dtype, return 0; } + /* If we only care about directories, then we can remove */ + if (absent_type == ABSENT_ANY_DIRECTORY) + return 0; + /* * The previous round may already have decided to * delete this path, which is in a subdirectory that @@ -2233,6 +2243,7 @@ static int check_ok_to_remove(const char *name, int len, int dtype, */ static int verify_absent_1(const struct cache_entry *ce, enum unpack_trees_error_types error_type, + enum absent_checking_type absent_type, struct unpack_trees_options *o) { int len; @@ -2259,7 +2270,8 @@ static int verify_absent_1(const struct cache_entry *ce, NULL, o); else ret = check_ok_to_remove(path, len, DT_UNKNOWN, NULL, - &st, error_type, o); + &st, error_type, + absent_type, o); } free(path); return ret; @@ -2274,7 +2286,7 @@ static int verify_absent_1(const struct cache_entry *ce, return check_ok_to_remove(ce->name, ce_namelen(ce), ce_to_dtype(ce), ce, &st, - error_type, o); + error_type, absent_type, o); } } @@ -2284,14 +2296,23 @@ static int verify_absent(const struct cache_entry *ce, { if (!o->skip_sparse_checkout && (ce->ce_flags & CE_NEW_SKIP_WORKTREE)) return 0; - return verify_absent_1(ce, error_type, o); + return verify_absent_1(ce, error_type, COMPLETELY_ABSENT, o); +} + +static int verify_absent_if_directory(const struct cache_entry *ce, + enum unpack_trees_error_types error_type, + struct unpack_trees_options *o) +{ + if (!o->skip_sparse_checkout && (ce->ce_flags & CE_NEW_SKIP_WORKTREE)) + return 0; + return verify_absent_1(ce, error_type, ABSENT_ANY_DIRECTORY, o); } static int verify_absent_sparse(const struct cache_entry *ce, enum unpack_trees_error_types error_type, struct unpack_trees_options *o) { - return verify_absent_1(ce, error_type, o); + return verify_absent_1(ce, error_type, COMPLETELY_ABSENT, o); } static int merged_entry(const struct cache_entry *ce, @@ -2365,6 +2386,12 @@ static int merged_entry(const struct cache_entry *ce, * Previously unmerged entry left as an existence * marker by read_index_unmerged(); */ + if (verify_absent_if_directory(merge, + ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN, o)) { + discard_cache_entry(merge); + return -1; + } + invalidate_ce_path(old, o); } From 56d06fe4aa9089bccb4ff247fc3224fc7431c72d Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Mon, 27 Sep 2021 16:33:46 +0000 Subject: [PATCH 09/11] unpack-trees: avoid nuking untracked dir in way of locally deleted file Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- t/t2500-untracked-overwriting.sh | 2 +- unpack-trees.c | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/t/t2500-untracked-overwriting.sh b/t/t2500-untracked-overwriting.sh index 5ec66058cf..5c0bf4d21f 100755 --- a/t/t2500-untracked-overwriting.sh +++ b/t/t2500-untracked-overwriting.sh @@ -218,7 +218,7 @@ test_expect_success 'git am --abort and untracked dir vs. unmerged file' ' ) ' -test_expect_failure 'git am --skip and untracked dir vs deleted file' ' +test_expect_success 'git am --skip and untracked dir vs deleted file' ' test_setup_sequencing am_skip_and_untracked && ( cd sequencing_am_skip_and_untracked && diff --git a/unpack-trees.c b/unpack-trees.c index 821f532c85..48ca93a725 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -2409,7 +2409,10 @@ static int deleted_entry(const struct cache_entry *ce, if (verify_absent(ce, ERROR_WOULD_LOSE_UNTRACKED_REMOVED, o)) return -1; return 0; + } else if (verify_absent_if_directory(ce, ERROR_WOULD_LOSE_UNTRACKED_REMOVED, o)) { + return -1; } + if (!(old->ce_flags & CE_CONFLICTED) && verify_uptodate(old, o)) return -1; add_entry(o, ce, CE_REMOVE, 0); From 94b7f1563ace91af823125e5b8895cb24b2c0e4a Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Mon, 27 Sep 2021 16:33:47 +0000 Subject: [PATCH 10/11] 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(-) 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()); From 0e29222e0c2f68118cdd3412515539b74433b732 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Mon, 27 Sep 2021 16:33:48 +0000 Subject: [PATCH 11/11] Documentation: call out commands that nuke untracked files/directories Some commands have traditionally also removed untracked files (or directories) that were in the way of a tracked file we needed. Document these cases. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- Documentation/git-checkout.txt | 5 +++-- Documentation/git-read-tree.txt | 5 +++-- Documentation/git-reset.txt | 3 ++- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt index b1a6fe4499..d473c9bf38 100644 --- a/Documentation/git-checkout.txt +++ b/Documentation/git-checkout.txt @@ -118,8 +118,9 @@ OPTIONS -f:: --force:: When switching branches, proceed even if the index or the - working tree differs from `HEAD`. This is used to throw away - local changes. + working tree differs from `HEAD`, and even if there are untracked + files in the way. This is used to throw away local changes and + any untracked files or directories that are in the way. + When checking out paths from the index, do not fail upon unmerged entries; instead, unmerged entries are ignored. diff --git a/Documentation/git-read-tree.txt b/Documentation/git-read-tree.txt index 0222a27c5a..8c3aceb832 100644 --- a/Documentation/git-read-tree.txt +++ b/Documentation/git-read-tree.txt @@ -38,8 +38,9 @@ OPTIONS --reset:: Same as -m, except that unmerged entries are discarded instead - of failing. When used with `-u`, updates leading to loss of - working tree changes will not abort the operation. + of failing. When used with `-u`, updates leading to loss of + working tree changes or untracked files or directories will not + abort the operation. -u:: After a successful merge, update the files in the work diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt index 252e2d4e47..6f7685f53d 100644 --- a/Documentation/git-reset.txt +++ b/Documentation/git-reset.txt @@ -69,7 +69,8 @@ linkgit:git-add[1]). --hard:: Resets the index and working tree. Any changes to tracked files in the - working tree since `` are discarded. + working tree since `` are discarded. Any untracked files or + directories in the way of writing any tracked files are simply deleted. --merge:: Resets the index and updates the files in the working tree that are