Fix various issues around removal of untracked files/directories

This series depends on en/am-abort-fix.

We have multiple codepaths that delete untracked files/directories but shouldn't. There are also some codepaths where we delete untracked files/directories intentionally (based on mailing list discussion), but where that intent is not documented. Fix the documentation, add several new (mostly failing) testcases, fix some of the new testcases, and add comments about some potential remaining problems. (I found these as a side-effect of looking at [1], though [2] pointed out one explicitly while I was working on it.)

Note that I'm using Junio's declaration about checkout -f and reset --hard (and also presuming that since read-tree --reset is porcelain that its behavior should be left alone)[3] in this series.

SIDENOTE about treating (some) ignored files as precious:

There's another related topic here that came up in the mailing list threads that is separate even if similar: namely, treating ignored files as precious instead of deleting them. I do not try to handle that here, but I believe that would actually be relatively easy to handle. If you leave unpack_trees_options->dir as NULL, then ignored files are treated as precious (my original patch 2 made that mistake). There's a few other locations that already optionally set up unpack_trees_options->dir (a quick search for "overwrite_ignore" and "overwrite-ignore" will find them), so we'd just need to implement that option flag in more places corresponding to the new callsites (and perhaps make a global core.overwrite_ignored config option to affect all of these). Of course, doing so would globally treat ignored files as precious rather than allowing them to be configured on a per-path basis, but honestly I think the idea of configuring ignored files as precious on a per-path basis sounds like insanity. (We have enough bugs with untracked and ignored files without adding yet another type. Also, tla/baz was excessively confusing to me due in part to the number of types of files and I'd rather not see such ideas ported to git. And, of course, configuring per-path rules sounds like lots of work for end users to configure. There may be additional reasons against it.) So, if someone wants to pursue the precious-ignored concept then I'd much rather see it done as a global setting. Just my $0.02.

[1] https://lore.kernel.org/git/[email protected]/ [2]
https://lore.kernel.org/git/[email protected]/
[3] https://lore.kernel.org/git/[email protected]/

Elijah Newren (6):
t2500: add various tests for nuking untracked files
Split unpack_trees 'reset' flag into two for untracked handling
unpack-trees: avoid nuking untracked dir in way of unmerged file
unpack-trees: avoid nuking untracked dir in way of locally deleted
file
Comment important codepaths regarding nuking untracked files/dirs
Documentation: call out commands that nuke untracked files/directories

 Documentation/git-checkout.txt   |   5 +-
 Documentation/git-read-tree.txt  |   5 +-
 Documentation/git-reset.txt      |   3 +-
 builtin/am.c                     |   6 +-
 builtin/checkout.c               |  10 +-
 builtin/read-tree.c              |  11 +-
 builtin/reset.c                  |  15 +-
 builtin/stash.c                  |   3 +-
 builtin/submodule--helper.c      |   4 +
 builtin/worktree.c               |   5 +
 contrib/rerere-train.sh          |   2 +-
 reset.c                          |   9 +-
 submodule.c                      |   1 +
 t/t1013-read-tree-submodule.sh   |   4 +-
 t/t2500-untracked-overwriting.sh | 244 +++++++++++++++++++++++++++++++
 unpack-trees.c                   |  56 +++++--
 unpack-trees.h                   |   4 +-
 17 files changed, 359 insertions(+), 28 deletions(-)
 create mode 100755 t/t2500-untracked-overwriting.sh

base-commit: c5ead19ea282a288e01d86536349a4ae4a093e4b
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1036%2Fnewren%2Funtracked_removal-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1036/newren/untracked_removal-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1036

gitgitgadget

From: Elijah Newren <[email protected]>

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.

---
 t/t2500-untracked-overwriting.sh | 244 +++++++++++++++++++++++++++++++
 1 file changed, 244 insertions(+)
 create mode 100755 t/t2500-untracked-overwriting.sh

new file mode 100755
index 00000000000..a1a6dfa671e
@@ -0,0 +1,244 @@
+#!/bin/sh
+
+test_description='Test handling of overwriting untracked files'
+
+. ./test-lib.sh
+
+test_setup_reset () {
+	test_create_repo 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 () {
+	test_create_repo 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 () {
+	test_create_repo 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

gitgitgadget

Ah, from your f0d4d398e2 ("test-lib: split up and deprecate test_create_repo()", 2021-05-10). Interesting history; I'll switchover to git init.

Yeah, I'll switch it to grep "Updating .foo.t. would lose untracked files" error

to make it clearer (where I'm using '.' instead of attempting to escape single quote characters appropriately).

From: Elijah Newren <[email protected]>

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. Split this into two separate fields:
reset_nuke_untracked
reset_keep_untracked
and, since many code paths in unpack_trees need to be followed for both of these flags, introduce a third one for convenience:
reset_either
which is simply an or-ing of the other two.

Modify existing callers so that
read-tree --reset
reset --hard
checkout --force
continue using reset_nuke_untracked, but so that 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 reset_keep_untracked field.

---
 builtin/am.c                     |  6 +++++-
 builtin/checkout.c               | 10 +++++++++-
 builtin/read-tree.c              | 11 ++++++++---
 builtin/reset.c                  | 15 +++++++++++++--
 builtin/stash.c                  |  2 +-
 reset.c                          |  9 +++++++--
 t/t1013-read-tree-submodule.sh   |  4 ++--
 t/t2500-untracked-overwriting.sh |  6 +++---
 unpack-trees.c                   | 17 ++++++++++++-----
 unpack-trees.h                   |  4 +++-
 10 files changed, 63 insertions(+), 21 deletions(-)

index c79e0167e98..dbe6cbe6a33 100644
@@ -1918,8 +1918,12 @@ 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;
+	opts.reset_keep_untracked = reset;
 	opts.fn = twoway_merge;
+	/* Setup opts.dir so that ignored files in the way get overwritten */
+	opts.dir = xcalloc(1, sizeof(*opts.dir));
+	opts.dir->flags |= DIR_SHOW_IGNORED;
+	setup_standard_excludes(opts.dir);
 	init_tree_desc(&t[0], head->buffer, head->size);
 	init_tree_desc(&t[1], remote->buffer, remote->size);
 

index b5d477919a7..ab0bb4d94f0 100644
@@ -646,12 +646,20 @@ 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;
+	if (o->force)
+		opts.reset_nuke_untracked = 1;
+	else
+		opts.reset_keep_untracked = 1;
 	opts.merge = 1;
 	opts.fn = oneway_merge;
 	opts.verbose_update = o->show_progress;
 	opts.src_index = &the_index;
 	opts.dst_index = &the_index;
+	if (o->overwrite_ignore) {
+		opts.dir = xcalloc(1, sizeof(*opts.dir));
+		opts.dir->flags |= DIR_SHOW_IGNORED;
+		setup_standard_excludes(opts.dir);
+	}
 	init_checkout_metadata(&opts.meta, info->refname,
 			       info->commit ? &info->commit->object.oid : null_oid(),
 			       NULL);

index 485e7b04794..8b94e1aa261 100644
@@ -133,7 +133,7 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
 			 N_("3-way merge if no file level merging required")),
 		OPT_BOOL(0, "aggressive", &opts.aggressive,
 			 N_("3-way merge in presence of adds and removes")),
-		OPT_BOOL(0, "reset", &opts.reset,
+		OPT_BOOL(0, "reset", &opts.reset_keep_untracked,
 			 N_("same as -m, but discard unmerged entries")),
 		{ OPTION_STRING, 0, "prefix", &opts.prefix, N_("<subdirectory>/"),
 		  N_("read the tree into the index under <subdirectory>/"),

@@ -162,6 +162,11 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
 	opts.head_idx = -1;
 	opts.src_index = &the_index;
 	opts.dst_index = &the_index;
+	if (opts.reset_keep_untracked) {
+		opts.dir = xcalloc(1, sizeof(*opts.dir));
+		opts.dir->flags |= DIR_SHOW_IGNORED;
+		setup_standard_excludes(opts.dir);
+	}
 
 	git_config(git_read_tree_config, NULL);
 

@@ -171,7 +176,7 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
 	hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
 
 	prefix_set = opts.prefix ? 1 : 0;
-	if (1 < opts.merge + opts.reset + prefix_set)
+	if (1 < opts.merge + opts.reset_keep_untracked + prefix_set)
 		die("Which one? -m, --reset, or --prefix?");
 
 	/*

@@ -183,7 +188,7 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
 	 * mode.
 	 */
 
-	if (opts.reset || opts.merge || opts.prefix) {
+	if (opts.reset_keep_untracked || opts.merge || opts.prefix) {
 		if (read_cache_unmerged() && (opts.prefix || opts.merge))
 			die(_("You need to resolve your current index first"));
 		stage = opts.merge = 1;

index 43e855cb887..ba39c4882a6 100644
@@ -10,6 +10,7 @@
 #define USE_THE_INDEX_COMPATIBILITY_MACROS
 #include "builtin.h"
 #include "config.h"
+#include "dir.h"
 #include "lockfile.h"
 #include "tag.h"
 #include "object.h"

@@ -70,9 +71,19 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t
 		break;
 	case HARD:
 		opts.update = 1;
-		/* fallthrough */
+		opts.reset_nuke_untracked = 1;
+		break;
+	case MIXED:
+		opts.reset_keep_untracked = 1; /* but opts.update=0, so untracked left alone */
+		break;
 	default:
-		opts.reset = 1;
+		BUG("invalid reset_type passed to reset_index");
+	}
+	if (opts.reset_keep_untracked) {
+		/* Setup opts.dir so we can overwrite ignored files */
+		opts.dir = xcalloc(1, sizeof(*opts.dir));
+		opts.dir->flags |= DIR_SHOW_IGNORED;
+		setup_standard_excludes(opts.dir);
 	}
 
 	read_cache_unmerged();

index 8f42360ca91..4ceb3581b47 100644
@@ -256,7 +256,7 @@ 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_keep_untracked = reset;
 	opts.update = update;
 	opts.fn = oneway_merge;
 

index 79310ae071b..0880c76aef9 100644
@@ -1,5 +1,6 @@
 #include "git-compat-util.h"
 #include "cache-tree.h"
+#include "dir.h"
 #include "lockfile.h"
 #include "refs.h"
 #include "reset.h"

@@ -57,8 +58,12 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,
 	unpack_tree_opts.update = 1;
 	unpack_tree_opts.merge = 1;
 	init_checkout_metadata(&unpack_tree_opts.meta, switch_to_branch, oid, NULL);
-	if (!detach_head)
-		unpack_tree_opts.reset = 1;
+	if (!detach_head) {
+		unpack_tree_opts.reset_keep_untracked = 1;
+		unpack_tree_opts.dir = xcalloc(1, sizeof(*unpack_tree_opts.dir));
+		unpack_tree_opts.dir->flags |= DIR_SHOW_IGNORED;
+		setup_standard_excludes(unpack_tree_opts.dir);
+	}
 
 	if (repo_read_index_unmerged(r) < 0) {
 		ret = error(_("could not read index"));

index b6df7444c05..4e485c223ad 100755
@@ -10,10 +10,10 @@ KNOWN_FAILURE_SUBMODULE_OVERWRITE_IGNORED_UNTRACKED=1
 
 test_submodule_switch_recursing_with_args "read-tree -u -m"
 
-test_submodule_forced_switch_recursing_with_args "read-tree -u --reset"
+test_submodule_switch_recursing_with_args "read-tree -u --reset"
 
 test_submodule_switch "read-tree -u -m"
 
-test_submodule_forced_switch "read-tree -u --reset"
+test_submodule_switch "read-tree -u --reset"
 
 test_done

index a1a6dfa671e..786ec33d63a 100755
@@ -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 &&

index 5786645f315..d952eebe96a 100644
@@ -301,7 +301,7 @@ static int check_submodule_move_head(const struct cache_entry *ce,
 	if (!sub)
 		return 0;
 
-	if (o->reset)
+	if (o->reset_nuke_untracked)
 		flags |= SUBMODULE_MOVE_HEAD_FORCE;
 
 	if (submodule_move_head(ce->name, old_id, new_id, flags))

@@ -1696,6 +1696,13 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 	if (len > MAX_UNPACK_TREES)
 		die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES);
 
+	if (o->reset_nuke_untracked && o->reset_keep_untracked)
+		BUG("reset_nuke_untracked and reset_keep_untracked are incompatible");
+
+	o->reset_either = 0;
+	if (o->reset_nuke_untracked || o->reset_keep_untracked)
+		o->reset_either = 1;
+
 	trace_performance_enter();
 	trace2_region_enter("unpack_trees", "unpack_trees", the_repository);
 

@@ -1989,7 +1996,7 @@ static int verify_uptodate_1(const struct cache_entry *ce,
 	 */
 	if ((ce->ce_flags & CE_VALID) || ce_skip_worktree(ce))
 		; /* keep checking */
-	else if (o->reset || ce_uptodate(ce))
+	else if (o->reset_either || ce_uptodate(ce))
 		return 0;
 
 	if (!lstat(ce->name, &st)) {

@@ -2218,7 +2225,7 @@ 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->reset_nuke_untracked || !o->update)
 		return 0;
 
 	len = check_leading_path(ce->name, ce_namelen(ce), 0);

@@ -2585,7 +2592,7 @@ int twoway_merge(const struct cache_entry * const *src,
 
 	if (current) {
 		if (current->ce_flags & CE_CONFLICTED) {
-			if (same(oldtree, newtree) || o->reset) {
+			if (same(oldtree, newtree) || o->reset_either) {
 				if (!newtree)
 					return deleted_entry(current, current, o);
 				else

@@ -2683,7 +2690,7 @@ int oneway_merge(const struct cache_entry * const *src,
 
 	if (old && same(old, a)) {
 		int update = 0;
-		if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) &&
+		if (o->reset_either && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) &&
 			!(old->ce_flags & CE_FSMONITOR_VALID)) {
 			struct stat st;
 			if (lstat(old->name, &st) ||

index 2d88b19dca7..c419bf8b1f9 100644
@@ -46,7 +46,9 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
 void clear_unpack_trees_porcelain(struct unpack_trees_options *opts);
 
 struct unpack_trees_options {
-	unsigned int reset,
+	unsigned int reset_nuke_untracked,
+		     reset_keep_untracked,
+		     reset_either, /* internal use only */
 		     merge,
 		     update,
 		     clone,

gitgitgadget

No, much like other callsites that set this up (e.g. builtin/checkout.c), there isn't a place that frees it. In copying how they worked, I also copied their bugs. ;-)

I'm tempted to move this code into setup_unpack_trees_porcelain() and then free it in clear_unpack_trees_porcelain()...though not all callers make use of those functions. Hmm...

No, it only sets opts.reset_keep_untracked to 1 when o->force is false. If o->force is true, it instead sets opts.reset_nuke_untracked to 1. There's no mistake there.

Yep.

See [1] for an alternative approach that used an enum instead of adding mutually exclusive flags.

it would be nice if read-tree callers could choose whether they want to remove untracked files or not - that could always be added later. This patch changes the behavior of 'git read-tree -m -u' (and other commands) so that they will overwrite ignored files - I'm in favor of that change but it would be good to spell out the change in the commit message.

I often use checkout --force to clear unwanted changes when I'm switching branches, I'd prefer it if it did not remove untracked files.

This is great. In the discussion around [1] there is a mention of 'git checkout <pathspec>' which also overwrites untracked files. It does not use unpack_trees() so is arguably outside the scope of what you're doing here but it might be worth mentioning.

Oh, interesting. Any reason you didn't pursue that old series further?

Those commands made no distinction between untracked and ignored files previously, and overwrote all of them. This patch changes those commands so that they stop overwriting untracked files, unless those files are ignored. So, there's no change in behavior for ignored files, only for non-ignored untracked files.

Your suggestion to point out the behavior relative to ignored files in the commit message, though, is probably a good idea. I should mention that ignored files will continue to be removed by these commands.

I originally started down that path to see what it looked like, but Junio weighed in and explicitly called out checkout --force as being a command that should remove untracked files in the way. See
https://lore.kernel.org/git/[email protected]/. Seems you also felt that way previously, at
https://lore.kernel.org/git/[email protected]/
-- any reason for your change of opinion?

Oh, that's interesting. Yeah, that's worth mentioning and perhaps digging into.

Oh, um...I've basically implemented a --exclude-standard and assumed it was passed, ignoring whatever setting of opts.dir was already set up by exclude-per-directory. Oops.

Goes away entirely if we adopt your enum suggestion.

I think the enum approach makes sense; I'll try it out.

Mainly lack of time/distracted by other things. I was also not that confident about modifying the unpack_trees() code. Duy was very helpful but then moved on quite soon after I posted that series I think and there didn't seem to be much interest from others.

Are you sure, I thought 'read-tree -m -u' unlike 'read-tree --reset -u' refused to overwrite untracked and ignored files currently.

I've no recollection of writing that email! When I was writing today I thought that 'checkout -f' and 'switch --discard-changes' behaved the same way but it appears from that other message that they do not so maybe it is OK for 'checkout -f' to nuke everything if there is a safe alternative available in the form of 'switch --discard-changes'

It'd be fantastic to fix that if you have the time and inclination to dig into it.

Best Wishes

Phillip

Doh, I was thinking of read-tree --reset -u rather than read-tree -m -u, despite the fact that you explicitly called out (and I even quoted you on) the latter. You are right.

I won't include it in this series, but I'll throw it on my (long) pile of things to perhaps look at later.

Thanks for the suggestions and pointers in your reviews!

From: Elijah Newren <[email protected]>

---
 t/t2500-untracked-overwriting.sh |  2 +-
 unpack-trees.c                   | 35 ++++++++++++++++++++++++++++----
 2 files changed, 32 insertions(+), 5 deletions(-)

index 786ec33d63a..017946a494f 100755
@@ -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 &&

index d952eebe96a..3b3d1c0ff40 100644
@@ -2163,9 +2163,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;

@@ -2200,6 +2206,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

@@ -2220,6 +2230,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;

@@ -2245,7 +2256,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;

@@ -2260,7 +2272,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);
 	}
 }
 

@@ -2270,14 +2282,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,

@@ -2351,6 +2372,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);
 	}
 

gitgitgadget

From: Elijah Newren <[email protected]>

---
 t/t2500-untracked-overwriting.sh | 2 +-
 unpack-trees.c                   | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

index 017946a494f..d4d9dc928aa 100755
@@ -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 &&

index 3b3d1c0ff40..858595a13f1 100644
@@ -2395,7 +2395,11 @@ 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);

gitgitgadget

Yeah, that makes sense.

That's kind of misleading. ;-) You trimmed out a single line here, and in particular one that only contained a trailing curly brace. Thus, your "trimming" here actually made things longer.

No, the whitespace addition was after making the if-block above it more complex with the extra else block. That if-block is now approximately 2/3 of the length of the function, and is the part that is relevant to the comment above it. Since the code that follows the if-block is separate from the comment above and the if-block became more complex, it felt natural to add a bit of spacing. So, it wasn't stray, but intentional and related to the changes above.

From: Elijah Newren <[email protected]>

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

---
 builtin/stash.c             | 1 +
 builtin/submodule--helper.c | 4 ++++
 builtin/worktree.c          | 5 +++++
 contrib/rerere-train.sh     | 2 +-
 submodule.c                 | 1 +
 5 files changed, 12 insertions(+), 1 deletion(-)

index 4ceb3581b47..ce5e0364c68 100644
@@ -1519,6 +1519,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)) {

index ef2776a9e45..a49242d15ae 100644
@@ -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) {

index 0d0a80da61f..383947ff54f 100644
@@ -356,6 +356,11 @@ static int add_worktree(const char *path, const char *refname,
 	if (opts->checkout) {
 		cp.argv = NULL;
 		strvec_clear(&cp.args);
+		/*
+		 * NOTE: reset --hard is okay here, because 'worktree add'
+		 * refuses to work in an extant non-empty directory, so there
+		 * is no risk of deleting untracked files.
+		 */
 		strvec_pushl(&cp.args, "reset", "--hard", "--no-recurse-submodules", NULL);
 		if (opts->quiet)
 			strvec_push(&cp.args, "--quiet");

index eeee45dd341..75125d6ae00 100755
@@ -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"

index 8e611fe1dbf..a9b71d585cf 100644
@@ -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());

gitgitgadget

Assuming you're talking about this code in git-p4.py:

print("Synchronizing p4 checkout...")
if new_client_dir:
# old one was destroyed, and maybe nobody told p4 p4_sync("...", "-f")
else:
p4_sync("...")

This is doing a Perforce sync in the P4 repo, not the git repo.

In the usual/happy case, this directory already exists, the Perforce server knows about its state, and a normal "p4 sync ..." will bring it up to date.

But, if someone manually deleted the directory then "p4 sync ..." will only update modified files, and all sorts of things will then go wrong (e.g. the files we updated in the git view won't be present, and git-p4 will fall flat on its face).

So in this case, do a forced sync, which syncs everything ignoring the P4 server's idea of what files are/not present.

Luke

No, I was talking about this code:

                system([ "git", "checkout", "-f" ])

From: Elijah Newren <[email protected]>

Some commands have traditionally also removed untracked files (or directories) that were in the way of a tracked file we needed. Document these cases.

---
 Documentation/git-checkout.txt  | 5 +++--
 Documentation/git-read-tree.txt | 5 +++--
 Documentation/git-reset.txt     | 3 ++-
 3 files changed, 8 insertions(+), 5 deletions(-)

index b1a6fe44997..d473c9bf387 100644
@@ -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.

index 5fa8bab64c2..4731ec3283f 100644
@@ -39,8 +39,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

index 252e2d4e47d..6f7685f53d5 100644
@@ -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 `<commit>` are discarded.
+	working tree since `<commit>` 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

gitgitgadget

truly minor nit.

I may have been mistaken about any project decision. I had a look around the archives and only came up with a 2008 post [1] by Junio that, at the time, was looking for two spaces after the full stop.

It's not clear if we consider the man pages to be 'typeset' such that a single space would be the norm, or mono-spaced 'typewriter' style (two spaces). There's much commentary in the Wikipedia article [2].

So still minor.
--
Philip

[1] https://lore.kernel.org/git/[email protected]/
[2] https://en.wikipedia.org/wiki/Sentence_spacing

Note that the original also had a double space after full stop (looking at the previous sentence).

Double space is better as per Junio's declaration here:
https://lore.kernel.org/git/[email protected]/

However, it's so minor that I wouldn't normally bother to change it specifically. In this case, I originally was tweaking the sentence before as well, and when modifying both sentences I just naturally put two spaces after the full stop between them. But then I re-read and decided to reword and ended up restoring the original first sentence and didn't even notice that resulted in a change of spacing at the end of the sentence.

Yes, the double space is "more correct" in the sources as per Junio's declaration in the link I provided above, but I agree it's pretty minor.

This series depends on en/am-abort-fix.

We have multiple codepaths that delete untracked files/directories but shouldn't. There are also some codepaths where we delete untracked files/directories intentionally (based on mailing list discussion), but where that intent is not documented. Fix the documentation, add several new (mostly failing) testcases, fix some of the new testcases, and add comments about some potential remaining problems. (I found these as a side-effect of looking at [1], though [2] pointed out one explicitly while I was working on it.)

Note that I'm using Junio's declaration about checkout -f and reset --hard (and also presuming that since read-tree --reset is porcelain that its behavior should be left alone)[3] in this series.

Changes since v1:

* Various small cleanups (suggested by Ævar)
* Fixed memory leaks of unpack_trees_opts->dir (also suggested by Ævar)
* Use an enum for unpack_trees_options->reset, instead of multiple fields (suggested by Phillip)
* Avoid changing behavior for cases not setting unpack_trees_options.reset

read-tree -m -u or the various reset flavors run internally by rebase/sequencer); we can revisit that later.

SIDENOTE about treating (some) ignored files as precious:

There's another related topic here that came up in the mailing list threads that is separate even if similar: namely, treating ignored files as precious instead of deleting them. I do not try to handle that here, but I believe that would actually be relatively easy to handle. If you leave unpack_trees_options->dir as NULL, then ignored files are treated as precious (my original patch 2 made that mistake). There's a few other locations that already optionally set up unpack_trees_options->dir (a quick search for "overwrite_ignore" and "overwrite-ignore" will find them), so we'd just need to implement that option flag in more places corresponding to the new callsites (and perhaps make a global core.overwrite_ignored config option to affect all of these). Of course, doing so would globally treat ignored files as precious rather than allowing them to be configured on a per-path basis, but honestly I think the idea of configuring ignored files as precious on a per-path basis sounds like insanity. (We have enough bugs with untracked and ignored files without adding yet another type. Also, tla/baz was excessively confusing to me due in part to the number of types of files and I'd rather not see such ideas ported to git. And, of course, configuring per-path rules sounds like lots of work for end users to configure. There may be additional reasons against it.) So, if someone wants to pursue the precious-ignored concept then I'd much rather see it done as a global setting. Just my $0.02.

[1] https://lore.kernel.org/git/[email protected]/ [2]
https://lore.kernel.org/git/[email protected]/
[3] https://lore.kernel.org/git/[email protected]/

Elijah Newren (6):
t2500: add various tests for nuking untracked files
Change unpack_trees' 'reset' flag into an enum
unpack-trees: avoid nuking untracked dir in way of unmerged file
unpack-trees: avoid nuking untracked dir in way of locally deleted
file
Comment important codepaths regarding nuking untracked files/dirs
Documentation: call out commands that nuke untracked files/directories

 Documentation/git-checkout.txt   |   5 +-
 Documentation/git-read-tree.txt  |   5 +-
 Documentation/git-reset.txt      |   3 +-
 builtin/am.c                     |  13 +-
 builtin/checkout.c               |  18 ++-
 builtin/read-tree.c              |   3 +
 builtin/reset.c                  |  20 ++-
 builtin/stash.c                  |  18 ++-
 builtin/submodule--helper.c      |   4 +
 builtin/worktree.c               |   5 +
 contrib/rerere-train.sh          |   2 +-
 reset.c                          |  13 +-
 submodule.c                      |   1 +
 t/t2500-untracked-overwriting.sh | 244 +++++++++++++++++++++++++++++++
 unpack-trees.c                   |  44 +++++-
 unpack-trees.h                   |  11 +-
 16 files changed, 387 insertions(+), 22 deletions(-)
 create mode 100755 t/t2500-untracked-overwriting.sh

base-commit: c5ead19ea282a288e01d86536349a4ae4a093e4b
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1036%2Fnewren%2Funtracked_removal-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1036/newren/untracked_removal-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1036

Range-diff vs v1:

1: b634136a74b ! 1: 9460a49c7ed t2500: add various tests for nuking untracked files

     @@ t/t2500-untracked-overwriting.sh (new)
      +. ./test-lib.sh
      +
      +test_setup_reset () {
     -+	test_create_repo reset_$1 &&
     ++	git init reset_$1 &&
      +	(
      +		cd reset_$1 &&
      +		test_commit init &&
     @@ t/t2500-untracked-overwriting.sh (new)
      +
      +		test_must_fail git reset --merge work 2>error &&
      +		test_cmp expect foo.t/file &&
     -+		grep "Updating.*foo.t.*would lose untracked files" error
     ++		grep "Updating .foo.t. would lose untracked files" error
      +	)
      +'
      +
     @@ t/t2500-untracked-overwriting.sh (new)
      +'
      +
      +test_setup_checkout_m () {
     -+	test_create_repo checkout &&
     ++	git init checkout &&
      +	(
      +		cd checkout &&
      +		test_commit init &&
     @@ t/t2500-untracked-overwriting.sh (new)
      +'
      +
      +test_setup_sequencing () {
     -+	test_create_repo sequencing_$1 &&
     ++	git init sequencing_$1 &&
      +	(
      +		cd sequencing_$1 &&
      +		test_commit init &&
 2:  45bd05a945f ! 2:  b77692b8f49 Split unpack_trees 'reset' flag into two for untracked handling
     @@ Metadata
      Author: Elijah Newren <[email protected]>
      
       ## Commit message ##
     -    Split unpack_trees 'reset' flag into two for untracked handling
     +    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.  Split this into two separate fields:
     -       reset_nuke_untracked
     -       reset_keep_untracked
     -    and, since many code paths in unpack_trees need to be followed for both
     -    of these flags, introduce a third one for convenience:
     -       reset_either
     -    which is simply an or-ing of the other two.
     +    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, define with the enum a special
     +    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 reset_nuke_untracked, but so that other callers,
     -    including
     +    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 reset_keep_untracked field.
     +    will use the new UNPACK_RESET_PROTECT_UNTRACKED value.
     +
     +    In order to protect untracked files but still allow deleting of ignored
     +    files, we also have to setup unpack_trees_opt.dir.  It may make sense to
     +    set up unpack_trees_opt.dir in more cases, but here I tried to only do
     +    so in cases where we switched from deleting all untracked files to
     +    avoiding doing so (i.e. where we now use
     +    UNPACK_RESET_PROTECT_UNTRACKED).
     +
     +    Also, note that 'git checkout <pathspec>' currently also allows
     +    overwriting untracked files.  That case should also be fixed, but it
     +    does not use unpack_trees() and thus is outside the scope of the current
     +    changes.
      
          Signed-off-by: Elijah Newren <[email protected]>
      
     @@ builtin/am.c: static int fast_forward_to(struct tree *head, struct tree *remote,
       	opts.update = 1;
       	opts.merge = 1;
      -	opts.reset = reset;
     -+	opts.reset_keep_untracked = reset;
     ++	opts.reset = reset ? UNPACK_RESET_PROTECT_UNTRACKED : 0;
       	opts.fn = twoway_merge;
     -+	/* Setup opts.dir so that ignored files in the way get overwritten */
     -+	opts.dir = xcalloc(1, sizeof(*opts.dir));
     -+	opts.dir->flags |= DIR_SHOW_IGNORED;
     -+	setup_standard_excludes(opts.dir);
     ++	if (opts.reset) {
     ++		/* Allow ignored files in the way to get overwritten */
     ++		opts.dir = xcalloc(1, sizeof(*opts.dir));
     ++		opts.dir->flags |= DIR_SHOW_IGNORED;
     ++		setup_standard_excludes(opts.dir);
     ++	}
       	init_tree_desc(&t[0], head->buffer, head->size);
       	init_tree_desc(&t[1], remote->buffer, remote->size);
       
     +@@ builtin/am.c: static int fast_forward_to(struct tree *head, struct tree *remote, int reset)
     + 		return -1;
     + 	}
     + 
     ++	if (opts.reset) {
     ++		dir_clear(opts.dir);
     ++		FREE_AND_NULL(opts.dir);
     ++	}
     ++
     + 	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
     + 		die(_("unable to write new index file"));
     + 
      
       ## builtin/checkout.c ##
      @@ builtin/checkout.c: static int reset_tree(struct tree *tree, const struct checkout_opts *o,
     + {
     + 	struct unpack_trees_options opts;
     + 	struct tree_desc tree_desc;
     ++	int unpack_trees_ret;
     + 
     + 	memset(&opts, 0, sizeof(opts));
       	opts.head_idx = -1;
       	opts.update = worktree;
       	opts.skip_unmerged = !worktree;
      -	opts.reset = 1;
     -+	if (o->force)
     -+		opts.reset_nuke_untracked = 1;
     -+	else
     -+		opts.reset_keep_untracked = 1;
     ++	opts.reset = o->force ? UNPACK_RESET_OVERWRITE_UNTRACKED :
     ++				UNPACK_RESET_PROTECT_UNTRACKED;
       	opts.merge = 1;
       	opts.fn = oneway_merge;
       	opts.verbose_update = o->show_progress;
     @@ builtin/checkout.c: static int reset_tree(struct tree *tree, const struct checko
       	init_checkout_metadata(&opts.meta, info->refname,
       			       info->commit ? &info->commit->object.oid : null_oid(),
       			       NULL);
     + 	parse_tree(tree);
     + 	init_tree_desc(&tree_desc, tree->buffer, tree->size);
     +-	switch (unpack_trees(1, &tree_desc, &opts)) {
     ++	unpack_trees_ret = unpack_trees(1, &tree_desc, &opts);
     ++
     ++	if (o->overwrite_ignore) {
     ++		dir_clear(opts.dir);
     ++		FREE_AND_NULL(opts.dir);
     ++	}
     ++
     ++	switch (unpack_trees_ret) {
     + 	case -2:
     + 		*writeout_error = 1;
     + 		/*
      
       ## builtin/read-tree.c ##
      @@ builtin/read-tree.c: int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
     - 			 N_("3-way merge if no file level merging required")),
     - 		OPT_BOOL(0, "aggressive", &opts.aggressive,
     - 			 N_("3-way merge in presence of adds and removes")),
     --		OPT_BOOL(0, "reset", &opts.reset,
     -+		OPT_BOOL(0, "reset", &opts.reset_keep_untracked,
     - 			 N_("same as -m, but discard unmerged entries")),
     - 		{ OPTION_STRING, 0, "prefix", &opts.prefix, N_("<subdirectory>/"),
     - 		  N_("read the tree into the index under <subdirectory>/"),
     -@@ builtin/read-tree.c: int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
     - 	opts.head_idx = -1;
     - 	opts.src_index = &the_index;
     - 	opts.dst_index = &the_index;
     -+	if (opts.reset_keep_untracked) {
     -+		opts.dir = xcalloc(1, sizeof(*opts.dir));
     -+		opts.dir->flags |= DIR_SHOW_IGNORED;
     -+		setup_standard_excludes(opts.dir);
     -+	}
     - 
     - 	git_config(git_read_tree_config, NULL);
     - 
     -@@ builtin/read-tree.c: int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
     - 	hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
     - 
     - 	prefix_set = opts.prefix ? 1 : 0;
     --	if (1 < opts.merge + opts.reset + prefix_set)
     -+	if (1 < opts.merge + opts.reset_keep_untracked + prefix_set)
     + 	if (1 < opts.merge + opts.reset + prefix_set)
       		die("Which one? -m, --reset, or --prefix?");
       
     ++	if (opts.reset)
     ++		opts.reset = UNPACK_RESET_OVERWRITE_UNTRACKED;
     ++
       	/*
     -@@ builtin/read-tree.c: int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
     - 	 * mode.
     - 	 */
     - 
     --	if (opts.reset || opts.merge || opts.prefix) {
     -+	if (opts.reset_keep_untracked || opts.merge || opts.prefix) {
     - 		if (read_cache_unmerged() && (opts.prefix || opts.merge))
     - 			die(_("You need to resolve your current index first"));
     - 		stage = opts.merge = 1;
     + 	 * NEEDSWORK
     + 	 *
      
       ## builtin/reset.c ##
      @@
     @@ builtin/reset.c: static int reset_index(const char *ref, const struct object_id
       	case HARD:
       		opts.update = 1;
      -		/* fallthrough */
     -+		opts.reset_nuke_untracked = 1;
     ++		opts.reset = UNPACK_RESET_OVERWRITE_UNTRACKED;
      +		break;
      +	case MIXED:
     -+		opts.reset_keep_untracked = 1; /* but opts.update=0, so untracked left alone */
     ++		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");
      +	}
     -+	if (opts.reset_keep_untracked) {
     ++	if (opts.reset == UNPACK_RESET_PROTECT_UNTRACKED) {
      +		/* Setup opts.dir so we can overwrite ignored files */
      +		opts.dir = xcalloc(1, sizeof(*opts.dir));
      +		opts.dir->flags |= DIR_SHOW_IGNORED;
     @@ builtin/reset.c: static int reset_index(const char *ref, const struct object_id
       	}
       
       	read_cache_unmerged();
     +@@ builtin/reset.c: static int reset_index(const char *ref, const struct object_id *oid, int reset_t
     + 	ret = 0;
     + 
     + out:
     ++	if (opts.reset == UNPACK_RESET_PROTECT_UNTRACKED) {
     ++		dir_clear(opts.dir);
     ++		FREE_AND_NULL(opts.dir);
     ++	}
     + 	for (i = 0; i < nr; i++)
     + 		free((void *)desc[i].buffer);
     + 	return ret;
      
       ## builtin/stash.c ##
     +@@ builtin/stash.c: static int reset_tree(struct object_id *i_tree, int update, int reset)
     + 	struct tree_desc t[MAX_UNPACK_TREES];
     + 	struct tree *tree;
     + 	struct lock_file lock_file = LOCK_INIT;
     ++	int unpack_trees_ret;
     + 
     + 	read_cache_preload(NULL);
     + 	if (refresh_cache(REFRESH_QUIET))
      @@ builtin/stash.c: 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_keep_untracked = reset;
     ++	opts.reset = reset ? UNPACK_RESET_PROTECT_UNTRACKED : 0;
     ++	if (opts.reset) {
     ++		opts.dir = xcalloc(1, sizeof(*opts.dir));
     ++		opts.dir->flags |= DIR_SHOW_IGNORED;
     ++		setup_standard_excludes(opts.dir);
     ++	}
       	opts.update = update;
       	opts.fn = oneway_merge;
       
     +-	if (unpack_trees(nr_trees, t, &opts))
     ++	unpack_trees_ret = unpack_trees(nr_trees, t, &opts);
     ++
     ++	if (opts.reset) {
     ++		dir_clear(opts.dir);
     ++		FREE_AND_NULL(opts.dir);
     ++	}
     ++
     ++	if (unpack_trees_ret)
     + 		return -1;
     + 
     + 	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
      
       ## reset.c ##
      @@
     @@ reset.c: int reset_head(struct repository *r, struct object_id *oid, const char
      -	if (!detach_head)
      -		unpack_tree_opts.reset = 1;
      +	if (!detach_head) {
     -+		unpack_tree_opts.reset_keep_untracked = 1;
     ++		unpack_tree_opts.reset = UNPACK_RESET_PROTECT_UNTRACKED;
      +		unpack_tree_opts.dir = xcalloc(1, sizeof(*unpack_tree_opts.dir));
      +		unpack_tree_opts.dir->flags |= DIR_SHOW_IGNORED;
      +		setup_standard_excludes(unpack_tree_opts.dir);
     @@ reset.c: int reset_head(struct repository *r, struct object_id *oid, const char
       
       	if (repo_read_index_unmerged(r) < 0) {
       		ret = error(_("could not read index"));
     -
     - ## t/t1013-read-tree-submodule.sh ##
     -@@ t/t1013-read-tree-submodule.sh: KNOWN_FAILURE_SUBMODULE_OVERWRITE_IGNORED_UNTRACKED=1
     - 
     - test_submodule_switch_recursing_with_args "read-tree -u -m"
     - 
     --test_submodule_forced_switch_recursing_with_args "read-tree -u --reset"
     -+test_submodule_switch_recursing_with_args "read-tree -u --reset"
     +@@ reset.c: reset_head_refs:
     + 			    oid_to_hex(oid), "1", NULL);
       
     - test_submodule_switch "read-tree -u -m"
     - 
     --test_submodule_forced_switch "read-tree -u --reset"
     -+test_submodule_switch "read-tree -u --reset"
     - 
     - test_done
     + leave_reset_head:
     ++	if (unpack_tree_opts.dir) {
     ++		dir_clear(unpack_tree_opts.dir);
     ++		FREE_AND_NULL(unpack_tree_opts.dir);
     ++	}
     + 	strbuf_release(&msg);
     + 	rollback_lock_file(&lock);
     + 	clear_unpack_trees_porcelain(&unpack_tree_opts);
      
       ## t/t2500-untracked-overwriting.sh ##
      @@ t/t2500-untracked-overwriting.sh: test_setup_checkout_m () {
     @@ t/t2500-untracked-overwriting.sh: test_expect_failure 'git rebase --abort and un
       		cd sequencing_rebase_fast_forward_and_untracked &&
      
       ## unpack-trees.c ##
     -@@ unpack-trees.c: static int check_submodule_move_head(const struct cache_entry *ce,
     - 	if (!sub)
     - 		return 0;
     - 
     --	if (o->reset)
     -+	if (o->reset_nuke_untracked)
     - 		flags |= SUBMODULE_MOVE_HEAD_FORCE;
     - 
     - 	if (submodule_move_head(ce->name, old_id, new_id, flags))
      @@ unpack-trees.c: int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
     - 	if (len > MAX_UNPACK_TREES)
     - 		die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES);
     + 	struct pattern_list pl;
     + 	int free_pattern_list = 0;
       
     -+	if (o->reset_nuke_untracked && o->reset_keep_untracked)
     -+		BUG("reset_nuke_untracked and reset_keep_untracked are incompatible");
     ++	if (o->reset == UNPACK_RESET_INVALID)
     ++		BUG("o->reset had a value of 1; should be UNPACK_TREES_*_UNTRACKED");
      +
     -+	o->reset_either = 0;
     -+	if (o->reset_nuke_untracked || o->reset_keep_untracked)
     -+		o->reset_either = 1;
     -+
     - 	trace_performance_enter();
     - 	trace2_region_enter("unpack_trees", "unpack_trees", the_repository);
     - 
     -@@ unpack-trees.c: static int verify_uptodate_1(const struct cache_entry *ce,
     - 	 */
     - 	if ((ce->ce_flags & CE_VALID) || ce_skip_worktree(ce))
     - 		; /* keep checking */
     --	else if (o->reset || ce_uptodate(ce))
     -+	else if (o->reset_either || ce_uptodate(ce))
     - 		return 0;
     + 	if (len > MAX_UNPACK_TREES)
     + 		die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES);
       
     - 	if (!lstat(ce->name, &st)) {
      @@ unpack-trees.c: 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->reset_nuke_untracked || !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);
     -@@ unpack-trees.c: int twoway_merge(const struct cache_entry * const *src,
     - 
     - 	if (current) {
     - 		if (current->ce_flags & CE_CONFLICTED) {
     --			if (same(oldtree, newtree) || o->reset) {
     -+			if (same(oldtree, newtree) || o->reset_either) {
     - 				if (!newtree)
     - 					return deleted_entry(current, current, o);
     - 				else
     -@@ unpack-trees.c: int oneway_merge(const struct cache_entry * const *src,
     - 
     - 	if (old && same(old, a)) {
     - 		int update = 0;
     --		if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) &&
     -+		if (o->reset_either && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) &&
     - 			!(old->ce_flags & CE_FSMONITOR_VALID)) {
     - 			struct stat st;
     - 			if (lstat(old->name, &st) ||
      
       ## unpack-trees.h ##
      @@ unpack-trees.h: 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,
     -+	unsigned int reset_nuke_untracked,
     -+		     reset_keep_untracked,
     -+		     reset_either, /* internal use only */
     - 		     merge,
     +-		     merge,
     ++	unsigned int merge,
       		     update,
       		     clone,
     + 		     index_only,
     +@@ unpack-trees.h: struct unpack_trees_options {
     + 		     exiting_early,
     + 		     show_all_errors,
     + 		     dry_run;
     ++	enum unpack_trees_reset_type reset;
     + 	const char *prefix;
     + 	int cache_bottom;
     + 	struct dir_struct *dir;
 3:  a69117a1c9e = 3:  208f3b3ebe5 unpack-trees: avoid nuking untracked dir in way of unmerged file
 4:  01bf850bb0f ! 4:  0a0997d081b unpack-trees: avoid nuking untracked dir in way of locally deleted file
     @@ unpack-trees.c: 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;
     ++	} 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))
 5:  60c5d6b4615 = 5:  4b78a526d2a Comment important codepaths regarding nuking untracked files/dirs
 6:  6ea23d165cf = 6:  993451a8036 Documentation: call out commands that nuke untracked files/directories

gitgitgadget

From: Elijah Newren <[email protected]>

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.

---
 t/t2500-untracked-overwriting.sh | 244 +++++++++++++++++++++++++++++++
 1 file changed, 244 insertions(+)
 create mode 100755 t/t2500-untracked-overwriting.sh

new file mode 100755
index 00000000000..2412d121ea8
@@ -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

gitgitgadget

From: Elijah Newren <[email protected]>

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, define with the enum a special 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.

In order to protect untracked files but still allow deleting of ignored files, we also have to setup unpack_trees_opt.dir. It may make sense to set up unpack_trees_opt.dir in more cases, but here I tried to only do so in cases where we switched from deleting all untracked files to avoiding doing so (i.e. where we now use UNPACK_RESET_PROTECT_UNTRACKED).

Also, note that 'git checkout <pathspec>' currently also allows overwriting untracked files. That case should also be fixed, but it does not use unpack_trees() and thus is outside the scope of the current changes.

---
 builtin/am.c                     | 13 ++++++++++++-
 builtin/checkout.c               | 18 ++++++++++++++++--
 builtin/read-tree.c              |  3 +++
 builtin/reset.c                  | 20 ++++++++++++++++++--
 builtin/stash.c                  | 17 +++++++++++++++--
 reset.c                          | 13 +++++++++++--
 t/t2500-untracked-overwriting.sh |  6 +++---
 unpack-trees.c                   |  6 +++++-
 unpack-trees.h                   | 11 +++++++++--
 9 files changed, 92 insertions(+), 15 deletions(-)

index c79e0167e98..b17baa67ad8 100644
@@ -1918,8 +1918,14 @@ 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;
+	opts.reset = reset ? UNPACK_RESET_PROTECT_UNTRACKED : 0;
 	opts.fn = twoway_merge;
+	if (opts.reset) {
+		/* Allow ignored files in the way to get overwritten */
+		opts.dir = xcalloc(1, sizeof(*opts.dir));
+		opts.dir->flags |= DIR_SHOW_IGNORED;
+		setup_standard_excludes(opts.dir);
+	}
 	init_tree_desc(&t[0], head->buffer, head->size);
 	init_tree_desc(&t[1], remote->buffer, remote->size);
 

@@ -1928,6 +1934,11 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)
 		return -1;
 	}
 
+	if (opts.reset) {
+		dir_clear(opts.dir);
+		FREE_AND_NULL(opts.dir);
+	}
+
 	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
 		die(_("unable to write new index file"));
 

index b5d477919a7..52826e0d145 100644
@@ -641,23 +641,37 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
 {
 	struct unpack_trees_options opts;
 	struct tree_desc tree_desc;
+	int unpack_trees_ret;
 
 	memset(&opts, 0, sizeof(opts));
 	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.merge = 1;
 	opts.fn = oneway_merge;
 	opts.verbose_update = o->show_progress;
 	opts.src_index = &the_index;
 	opts.dst_index = &the_index;
+	if (o->overwrite_ignore) {
+		opts.dir = xcalloc(1, sizeof(*opts.dir));
+		opts.dir->flags |= DIR_SHOW_IGNORED;
+		setup_standard_excludes(opts.dir);
+	}
 	init_checkout_metadata(&opts.meta, info->refname,
 			       info->commit ? &info->commit->object.oid : null_oid(),
 			       NULL);
 	parse_tree(tree);
 	init_tree_desc(&tree_desc, tree->buffer, tree->size);
-	switch (unpack_trees(1, &tree_desc, &opts)) {
+	unpack_trees_ret = unpack_trees(1, &tree_desc, &opts);
+
+	if (o->overwrite_ignore) {
+		dir_clear(opts.dir);
+		FREE_AND_NULL(opts.dir);
+	}
+
+	switch (unpack_trees_ret) {
 	case -2:
 		*writeout_error = 1;
 		/*

index 485e7b04794..740fc0335af 100644
@@ -174,6 +174,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
 	 *

index 43e855cb887..a12ee986e9f 100644
@@ -10,6 +10,7 @@
 #define USE_THE_INDEX_COMPATIBILITY_MACROS
 #include "builtin.h"
 #include "config.h"
+#include "dir.h"
 #include "lockfile.h"
 #include "tag.h"
 #include "object.h"

@@ -70,9 +71,20 @@ 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");
+	}
+	if (opts.reset == UNPACK_RESET_PROTECT_UNTRACKED) {
+		/* Setup opts.dir so we can overwrite ignored files */
+		opts.dir = xcalloc(1, sizeof(*opts.dir));
+		opts.dir->flags |= DIR_SHOW_IGNORED;
+		setup_standard_excludes(opts.dir);
 	}
 
 	read_cache_unmerged();

@@ -104,6 +116,10 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t
 	ret = 0;
 
 out:
+	if (opts.reset == UNPACK_RESET_PROTECT_UNTRACKED) {
+		dir_clear(opts.dir);
+		FREE_AND_NULL(opts.dir);
+	}
 	for (i = 0; i < nr; i++)
 		free((void *)desc[i].buffer);
 	return ret;

index 8f42360ca91..563f590afbd 100644
@@ -237,6 +237,7 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)
 	struct tree_desc t[MAX_UNPACK_TREES];
 	struct tree *tree;
 	struct lock_file lock_file = LOCK_INIT;
+	int unpack_trees_ret;
 
 	read_cache_preload(NULL);
 	if (refresh_cache(REFRESH_QUIET))

@@ -256,11 +257,23 @@ 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;
+	if (opts.reset) {
+		opts.dir = xcalloc(1, sizeof(*opts.dir));
+		opts.dir->flags |= DIR_SHOW_IGNORED;
+		setup_standard_excludes(opts.dir);
+	}
 	opts.update = update;
 	opts.fn = oneway_merge;
 
-	if (unpack_trees(nr_trees, t, &opts))
+	unpack_trees_ret = unpack_trees(nr_trees, t, &opts);
+
+	if (opts.reset) {
+		dir_clear(opts.dir);
+		FREE_AND_NULL(opts.dir);
+	}
+
+	if (unpack_trees_ret)
 		return -1;
 
 	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))

index 79310ae071b..1695f3828c5 100644
@@ -1,5 +1,6 @@
 #include "git-compat-util.h"
 #include "cache-tree.h"
+#include "dir.h"
 #include "lockfile.h"
 #include "refs.h"
 #include "reset.h"

@@ -57,8 +58,12 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,
 	unpack_tree_opts.update = 1;
 	unpack_tree_opts.merge = 1;
 	init_checkout_metadata(&unpack_tree_opts.meta, switch_to_branch, oid, NULL);
-	if (!detach_head)
-		unpack_tree_opts.reset = 1;
+	if (!detach_head) {
+		unpack_tree_opts.reset = UNPACK_RESET_PROTECT_UNTRACKED;
+		unpack_tree_opts.dir = xcalloc(1, sizeof(*unpack_tree_opts.dir));
+		unpack_tree_opts.dir->flags |= DIR_SHOW_IGNORED;
+		setup_standard_excludes(unpack_tree_opts.dir);
+	}
 
 	if (repo_read_index_unmerged(r) < 0) {
 		ret = error(_("could not read index"));

@@ -131,6 +136,10 @@ reset_head_refs:
 			    oid_to_hex(oid), "1", NULL);
 
 leave_reset_head:
+	if (unpack_tree_opts.dir) {
+		dir_clear(unpack_tree_opts.dir);
+		FREE_AND_NULL(unpack_tree_opts.dir);
+	}
 	strbuf_release(&msg);
 	rollback_lock_file(&lock);
 	clear_unpack_trees_porcelain(&unpack_tree_opts);

index 2412d121ea8..18604360df8 100755
@@ -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 &&

index 5786645f315..fcbe63bbed9 100644
@@ -1693,6 +1693,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 	struct pattern_list pl;
 	int free_pattern_list = 0;
 
+	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);
 

@@ -2218,7 +2221,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);

index 2d88b19dca7..1f386fb16cc 100644
@@ -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,
 		     clone,
 		     index_only,

@@ -64,6 +70,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 dir_struct *dir;

gitgitgadget

I wasted a few minutes wondering about the example. Please make it clear that you are checking out of a tree-ish that is different from HEAD, as there will by definition no "overwriting untracked" if you are checking out of the index.

E.g. "git checkout <tree-ish> -- <pathspec>".

With this command line:

   $ git checkout HEAD~24 -- path

where path used to be there as late as 24 revisions ago, but since then we removed, and the user wants to materialize the file out of the old version, path, be it tracked, untracked, or even a directory, should be made identical to the copy from the given version, no? Where does the "should also be fixed" come from?

Sorry, I should have been more careful. Phillip noted this other case that overwrote untracked files and suggested mentioning it; see
https://lore.kernel.org/git/[email protected]/
and the links from there. Perhaps I would have been better off mentioning that link and the other links in it.

It's a very common pattern...but not universal due to some prevailing weirdness and differences in how commands handle ignored files. But, it looks like that can be fixed up, and then this can be centralized. I've got some patches doing this.

I had thought about that, but I was already pretty deep down the rabbit hole of trying to avoid removing the current working directory as a side effect of other commands. I noted in my cover letter the --no-overwrite-ignore options and how they could be used building on this series, and now you bring it up too. And your previous comment about the common pattern for unpack_trees_options.dir forces me to go further down this hole a bit. So, I've got patches that implement a fair amount of this, but doesn't actually add the new --no-overwrite-ignore options or plumb them through. They do, however, add a FIXME comment where a single boolean value could be set in order to make such options work.

No, when opts.reset == UNPACK_RESET_NUKE_UNTRACKED, unpack_trees() has no way to differentiate between untracked and ignored and thus just deletes them all. Setting up unpack_trees_options.dir so that we can differentiate between untracked and ignored files when we are just going to treat them the same (delete them both) is wasted effort. So the check for a certain type of reset value was merely an optimization. (And that optimization didn't apply in the am case, since it doesn't use UNPACK_RESET_NUKE_UNTRACKED.)

However, consolidating the unpack_trees_options.dir handling into unpack_trees() allows me to better document the optimization and only worry about it in one place, which seems to make the code much clearer.

Yeah, you're right; I should have used "if (opts.dir)" for all these cases.

Will fix, though the consolidation ends up making it just one place anyway.

From: Elijah Newren <[email protected]>

---
 t/t2500-untracked-overwriting.sh |  2 +-
 unpack-trees.c                   | 35 ++++++++++++++++++++++++++++----
 2 files changed, 32 insertions(+), 5 deletions(-)

index 18604360df8..5ec66058cfc 100755
@@ -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 &&

index fcbe63bbed9..f7d0088a4fd 100644
@@ -2159,9 +2159,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;

@@ -2196,6 +2202,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

@@ -2216,6 +2226,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;

@@ -2242,7 +2253,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;

@@ -2257,7 +2269,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);
 	}
 }
 

@@ -2267,14 +2279,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,

@@ -2348,6 +2369,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);
 	}
 

gitgitgadget

From: Elijah Newren <[email protected]>

---
 t/t2500-untracked-overwriting.sh | 2 +-
 unpack-trees.c                   | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

index 5ec66058cfc..5c0bf4d21fc 100755
@@ -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 &&

index f7d0088a4fd..b1e7ee9dfc0 100644
@@ -2392,7 +2392,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);

gitgitgadget

From: Elijah Newren <[email protected]>

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

---
 builtin/stash.c             | 1 +
 builtin/submodule--helper.c | 4 ++++
 builtin/worktree.c          | 5 +++++
 contrib/rerere-train.sh     | 2 +-
 submodule.c                 | 1 +
 5 files changed, 12 insertions(+), 1 deletion(-)

index 563f590afbd..0d071183eba 100644
@@ -1532,6 +1532,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)) {

index ef2776a9e45..a49242d15ae 100644
@@ -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) {

index 0d0a80da61f..383947ff54f 100644
@@ -356,6 +356,11 @@ static int add_worktree(const char *path, const char *refname,
 	if (opts->checkout) {
 		cp.argv = NULL;
 		strvec_clear(&cp.args);
+		/*
+		 * NOTE: reset --hard is okay here, because 'worktree add'
+		 * refuses to work in an extant non-empty directory, so there
+		 * is no risk of deleting untracked files.
+		 */
 		strvec_pushl(&cp.args, "reset", "--hard", "--no-recurse-submodules", NULL);
 		if (opts->quiet)
 			strvec_push(&cp.args, "--quiet");

index eeee45dd341..75125d6ae00 100755
@@ -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"

index 8e611fe1dbf..a9b71d585cf 100644
@@ -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());

gitgitgadget

Fair enough; I'll strike it.

Yeah, that'd make sense. Though it'd make even more sense to get rid of the subprocess forking. Definitely something for a different series, though.

From: Elijah Newren <[email protected]>

Some commands have traditionally also removed untracked files (or directories) that were in the way of a tracked file we needed. Document these cases.

---
 Documentation/git-checkout.txt  | 5 +++--
 Documentation/git-read-tree.txt | 5 +++--
 Documentation/git-reset.txt     | 3 ++-
 3 files changed, 8 insertions(+), 5 deletions(-)

index b1a6fe44997..d473c9bf387 100644
@@ -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.

index 5fa8bab64c2..4731ec3283f 100644
@@ -39,8 +39,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

index 252e2d4e47d..6f7685f53d5 100644
@@ -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 `<commit>` are discarded.
+	working tree since `<commit>` 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

gitgitgadget

We have multiple codepaths that delete untracked files/directories but shouldn't. There are also some codepaths where we delete untracked files/directories intentionally (based on mailing list discussion), but where that intent is not documented. We also have some codepaths that preserve ignored files, which shouldn't. Fix the documentation, add several new (mostly failing) testcases, fix some of the new testcases, and add comments about some potential remaining problems. (I found these as a side-effect of looking at [1], though [2] pointed out one explicitly while I was working on it.)

Note that I'm using Junio's declaration about checkout -f and reset --hard (and also presuming that since read-tree --reset is porcelain that its behavior should be left alone)[3] in this series.

Changes since v2 (all due to Junio's request to consolidate unpack_trees_options.dir handling):

* fix some (pre-existing) memory leaks, in preparation for consolidating some common code (new patch 2)
* New patches (3 & 6) to make a few more commands remove ignored files by default -- which also fixes an existing testcase
* New patches (4 & 5) to consolidate the various places handling unpack_trees_options.dir and default to treating ignored files as expendable within unpack_trees(). These change also make it very easy to add --no-overwrite-ignore options in the future to additional commands (checkout and merge already have such an option, though merge only passes that along to the fast-forwarding backend)

Changes since v1:

* Various small cleanups (suggested by Ævar)
* Fixed memory leaks of unpack_trees_opts->dir (also suggested by Ævar)
* Use an enum for unpack_trees_options->reset, instead of multiple fields (suggested by Phillip)
* Avoid changing behavior for cases not setting unpack_trees_options.reset

read-tree -m -u or the various reset flavors run internally by rebase/sequencer); we can revisit that later.

SIDENOTE about treating ignored files as precious:

The patches are now getting pretty close to being able to handle ignored files as precious. The only things left would be making merge pass the --no-overwrite-ignore option along to more backends, and adding the --no-overwrite-ignore option that both checkout and merge take to more commands. There's already comments in the code about what boolean would need to be set by that flag. And then perhaps also make a global core.overwrite_ignored config option to affect all of these. Granted, doing this would globally treat ignored files as precious rather than allowing them to be configured on a per-path basis, but honestly I think the idea of configuring ignored files as precious on a per-path basis sounds like insanity. (We have enough bugs with untracked and ignored files without adding yet another type. And, of course, configuring per-path rules sounds like lots of work for end users to configure. There may be additional reasons against it.) So, if someone wants to pursue the precious-ignored concept then I'd much rather see it done as a global setting. Just my $0.02.

[1] https://lore.kernel.org/git/[email protected]/ [2]
https://lore.kernel.org/git/[email protected]/
[3] https://lore.kernel.org/git/[email protected]/

Elijah Newren (11):
t2500: add various tests for nuking untracked files checkout, read-tree: fix leak of unpack_trees_options.dir read-tree, merge-recursive: overwrite ignored files by default
unpack-trees: introduce preserve_ignored to unpack_trees_options
unpack-trees: make dir an internal-only struct
Remove ignored files by default when they are in the way Change unpack_trees' 'reset' flag into an enum
unpack-trees: avoid nuking untracked dir in way of unmerged file
unpack-trees: avoid nuking untracked dir in way of locally deleted
file
Comment important codepaths regarding nuking untracked files/dirs
Documentation: call out commands that nuke untracked files/directories

 Documentation/git-checkout.txt   |   5 +-
 Documentation/git-read-tree.txt  |  23 +--
 Documentation/git-reset.txt      |   3 +-
 builtin/am.c                     |   3 +-
 builtin/checkout.c               |  10 +-
 builtin/clone.c                  |   1 +
 builtin/merge.c                  |   1 +
 builtin/read-tree.c              |  26 ++--
 builtin/reset.c                  |  10 +-
 builtin/stash.c                  |   5 +-
 builtin/submodule--helper.c      |   4 +
 contrib/rerere-train.sh          |   2 +-
 merge-ort.c                      |   8 +-
 merge-recursive.c                |   5 +-
 merge.c                          |   8 +-
 reset.c                          |   3 +-
 sequencer.c                      |   1 +
 submodule.c                      |   1 +
 t/t1013-read-tree-submodule.sh   |   1 -
 t/t2500-untracked-overwriting.sh | 244 +++++++++++++++++++++++++++++++
 t/t7112-reset-submodule.sh       |   1 -
 unpack-trees.c                   |  61 +++++++-
 unpack-trees.h                   |  14 +-
 23 files changed, 366 insertions(+), 74 deletions(-)
 create mode 100755 t/t2500-untracked-overwriting.sh

base-commit: ddb1055343948e0d0bc81f8d20245f1ada6430a0
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1036%2Fnewren%2Funtracked_removal-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1036/newren/untracked_removal-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1036

Range-diff vs v2:

  1:  9460a49c7ed =  1:  66270ffc74e t2500: add various tests for nuking untracked files
  -:  ----------- >  2:  0c74285b253 checkout, read-tree: fix leak of unpack_trees_options.dir
  -:  ----------- >  3:  2501a0c552a read-tree, merge-recursive: overwrite ignored files by default
  -:  ----------- >  4:  f1a0700e598 unpack-trees: introduce preserve_ignored to unpack_trees_options
  -:  ----------- >  5:  0d119142778 unpack-trees: make dir an internal-only struct
  -:  ----------- >  6:  b7fe354efff Remove ignored files by default when they are in the way
  2:  b77692b8f49 !  7:  9eb20121fc3 Change unpack_trees' 'reset' flag into an enum
     @@ Commit message
          (i.e. "true") can be split into two:
             UNPACK_RESET_PROTECT_UNTRACKED,
             UNPACK_RESET_OVERWRITE_UNTRACKED
     -    In order to catch accidental misuses, define with the enum a special
     -    value of
     +    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().
      
     @@ Commit message
             numerous callers from rebase/sequencer to reset_head()
          will use the new UNPACK_RESET_PROTECT_UNTRACKED value.
      
     -    In order to protect untracked files but still allow deleting of ignored
     -    files, we also have to setup unpack_trees_opt.dir.  It may make sense to
     -    set up unpack_trees_opt.dir in more cases, but here I tried to only do
     -    so in cases where we switched from deleting all untracked files to
     -    avoiding doing so (i.e. where we now use
     -    UNPACK_RESET_PROTECT_UNTRACKED).
     +    Also, note that it has been reported that 'git checkout <treeish>
     +    <pathspec>' 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.
      
     -    Also, note that 'git checkout <pathspec>' currently also allows
     -    overwriting untracked files.  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/[email protected]/
      
          Signed-off-by: Elijah Newren <[email protected]>
      
     @@ builtin/am.c: static int fast_forward_to(struct tree *head, struct tree *remote,
       	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;
     -+	if (opts.reset) {
     -+		/* Allow ignored files in the way to get overwritten */
     -+		opts.dir = xcalloc(1, sizeof(*opts.dir));
     -+		opts.dir->flags |= DIR_SHOW_IGNORED;
     -+		setup_standard_excludes(opts.dir);
     -+	}
       	init_tree_desc(&t[0], head->buffer, head->size);
       	init_tree_desc(&t[1], remote->buffer, remote->size);
     - 
     -@@ builtin/am.c: static int fast_forward_to(struct tree *head, struct tree *remote, int reset)
     - 		return -1;
     - 	}
     - 
     -+	if (opts.reset) {
     -+		dir_clear(opts.dir);
     -+		FREE_AND_NULL(opts.dir);
     -+	}
     -+
     - 	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
     - 		die(_("unable to write new index file"));
     - 
      
       ## builtin/checkout.c ##
      @@ builtin/checkout.c: static int reset_tree(struct tree *tree, const struct checkout_opts *o,
     - {
     - 	struct unpack_trees_options opts;
     - 	struct tree_desc tree_desc;
     -+	int unpack_trees_ret;
     - 
     - 	memset(&opts, 0, sizeof(opts));
       	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;
     - 	opts.dst_index = &the_index;
     -+	if (o->overwrite_ignore) {
     -+		opts.dir = xcalloc(1, sizeof(*opts.dir));
     -+		opts.dir->flags |= DIR_SHOW_IGNORED;
     -+		setup_standard_excludes(opts.dir);
     -+	}
     - 	init_checkout_metadata(&opts.meta, info->refname,
     - 			       info->commit ? &info->commit->object.oid : null_oid(),
     - 			       NULL);
     - 	parse_tree(tree);
     - 	init_tree_desc(&tree_desc, tree->buffer, tree->size);
     --	switch (unpack_trees(1, &tree_desc, &opts)) {
     -+	unpack_trees_ret = unpack_trees(1, &tree_desc, &opts);
     -+
     -+	if (o->overwrite_ignore) {
     -+		dir_clear(opts.dir);
     -+		FREE_AND_NULL(opts.dir);
     -+	}
     -+
     -+	switch (unpack_trees_ret) {
     - 	case -2:
     - 		*writeout_error = 1;
     - 		/*
      
       ## builtin/read-tree.c ##
      @@ builtin/read-tree.c: int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
     @@ builtin/read-tree.c: int cmd_read_tree(int argc, const char **argv, const char *
       	 *
      
       ## builtin/reset.c ##
     -@@
     - #define USE_THE_INDEX_COMPATIBILITY_MACROS
     - #include "builtin.h"
     - #include "config.h"
     -+#include "dir.h"
     - #include "lockfile.h"
     - #include "tag.h"
     - #include "object.h"
      @@ builtin/reset.c: static int reset_index(const char *ref, const struct object_id *oid, int reset_t
       		break;
       	case HARD:
     @@ builtin/reset.c: static int reset_index(const char *ref, const struct object_id
       	default:
      -		opts.reset = 1;
      +		BUG("invalid reset_type passed to reset_index");
     -+	}
     -+	if (opts.reset == UNPACK_RESET_PROTECT_UNTRACKED) {
     -+		/* Setup opts.dir so we can overwrite ignored files */
     -+		opts.dir = xcalloc(1, sizeof(*opts.dir));
     -+		opts.dir->flags |= DIR_SHOW_IGNORED;
     -+		setup_standard_excludes(opts.dir);
       	}
       
       	read_cache_unmerged();
     -@@ builtin/reset.c: static int reset_index(const char *ref, const struct object_id *oid, int reset_t
     - 	ret = 0;
     - 
     - out:
     -+	if (opts.reset == UNPACK_RESET_PROTECT_UNTRACKED) {
     -+		dir_clear(opts.dir);
     -+		FREE_AND_NULL(opts.dir);
     -+	}
     - 	for (i = 0; i < nr; i++)
     - 		free((void *)desc[i].buffer);
     - 	return ret;
      
       ## builtin/stash.c ##
     -@@ builtin/stash.c: static int reset_tree(struct object_id *i_tree, int update, int reset)
     - 	struct tree_desc t[MAX_UNPACK_TREES];
     - 	struct tree *tree;
     - 	struct lock_file lock_file = LOCK_INIT;
     -+	int unpack_trees_ret;
     - 
     - 	read_cache_preload(NULL);
     - 	if (refresh_cache(REFRESH_QUIET))
      @@ builtin/stash.c: 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;
     -+	if (opts.reset) {
     -+		opts.dir = xcalloc(1, sizeof(*opts.dir));
     -+		opts.dir->flags |= DIR_SHOW_IGNORED;
     -+		setup_standard_excludes(opts.dir);
     -+	}
       	opts.update = update;
     +-	if (update && !reset)
     ++	if (update)
     + 		opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */
       	opts.fn = oneway_merge;
       
     --	if (unpack_trees(nr_trees, t, &opts))
     -+	unpack_trees_ret = unpack_trees(nr_trees, t, &opts);
     -+
     -+	if (opts.reset) {
     -+		dir_clear(opts.dir);
     -+		FREE_AND_NULL(opts.dir);
     -+	}
     -+
     -+	if (unpack_trees_ret)
     - 		return -1;
     - 
     - 	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
      
       ## reset.c ##
     -@@
     - #include "git-compat-util.h"
     - #include "cache-tree.h"
     -+#include "dir.h"
     - #include "lockfile.h"
     - #include "refs.h"
     - #include "reset.h"
      @@ reset.c: int reset_head(struct repository *r, struct object_id *oid, const char *action,
     - 	unpack_tree_opts.update = 1;
     - 	unpack_tree_opts.merge = 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)
     + 	if (!detach_head)
      -		unpack_tree_opts.reset = 1;
     -+	if (!detach_head) {
      +		unpack_tree_opts.reset = UNPACK_RESET_PROTECT_UNTRACKED;
     -+		unpack_tree_opts.dir = xcalloc(1, sizeof(*unpack_tree_opts.dir));
     -+		unpack_tree_opts.dir->flags |= DIR_SHOW_IGNORED;
     -+		setup_standard_excludes(unpack_tree_opts.dir);
     -+	}
       
       	if (repo_read_index_unmerged(r) < 0) {
       		ret = error(_("could not read index"));
     -@@ reset.c: reset_head_refs:
     - 			    oid_to_hex(oid), "1", NULL);
     - 
     - leave_reset_head:
     -+	if (unpack_tree_opts.dir) {
     -+		dir_clear(unpack_tree_opts.dir);
     -+		FREE_AND_NULL(unpack_tree_opts.dir);
     -+	}
     - 	strbuf_release(&msg);
     - 	rollback_lock_file(&lock);
     - 	clear_unpack_trees_porcelain(&unpack_tree_opts);
      
       ## t/t2500-untracked-overwriting.sh ##
      @@ t/t2500-untracked-overwriting.sh: test_setup_checkout_m () {
     @@ t/t2500-untracked-overwriting.sh: test_expect_failure 'git rebase --abort and un
      
       ## unpack-trees.c ##
      @@ unpack-trees.c: int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
     - 	struct pattern_list pl;
       	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)
     +@@ unpack-trees.c: 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;
      @@ unpack-trees.c: static int verify_absent_1(const struct cache_entry *ce,
       	int len;
       	struct stat st;
     @@ unpack-trees.h: void setup_unpack_trees_porcelain(struct unpack_trees_options *o
      -		     merge,
      +	unsigned int merge,
       		     update,
     + 		     preserve_ignored,
       		     clone,
     - 		     index_only,
      @@ unpack-trees.h: struct unpack_trees_options {
       		     exiting_early,
       		     show_all_errors,
     @@ unpack-trees.h: struct unpack_trees_options {
      +	enum unpack_trees_reset_type reset;
       	const char *prefix;
       	int cache_bottom;
     - 	struct dir_struct *dir;
     + 	struct pathspec *pathspec;
  3:  208f3b3ebe5 =  8:  e4c42d43b09 unpack-trees: avoid nuking untracked dir in way of unmerged file
  4:  0a0997d081b =  9:  1a770681704 unpack-trees: avoid nuking untracked dir in way of locally deleted file
  5:  4b78a526d2a ! 10:  6b42a80bf3d Comment important codepaths regarding nuking untracked files/dirs

@@ Commit message
* 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.
+

## builtin/stash.c ##
@@ builtin/submodule--helper.c: static int add_submodule(const struct add_data *add

if (add_data->branch) {

- ## builtin/worktree.c ##
-@@ builtin/worktree.c: static int add_worktree(const char *path, const char *refname,
- if (opts->checkout) {
- cp.argv = NULL;
- strvec_clear(&cp.args);
-+ /*
-+ * NOTE: reset --hard is okay here, because 'worktree add'
-+ * refuses to work in an extant non-empty directory, so there
-+ * is no risk of deleting untracked files.
-+ */
- strvec_pushl(&cp.args, "reset", "--hard", "--no-recurse-submodules", NULL);
- if (opts->quiet)
- strvec_push(&cp.args, "--quiet");
-
## contrib/rerere-train.sh ##
@@ contrib/rerere-train.sh: do
git checkout -q $commit -- .
6: 993451a8036 = 11: de416f887d7 Documentation: call out commands that nuke untracked files/directories

gitgitgadget

From: Elijah Newren <[email protected]>

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.

---
 t/t2500-untracked-overwriting.sh | 244 +++++++++++++++++++++++++++++++
 1 file changed, 244 insertions(+)
 create mode 100755 t/t2500-untracked-overwriting.sh

new file mode 100755
index 00000000000..2412d121ea8
@@ -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

gitgitgadget

From: Elijah Newren <[email protected]>

---
 builtin/checkout.c  | 4 ++++
 builtin/read-tree.c | 5 +++++
 2 files changed, 9 insertions(+)

index 8c69dcdf72a..5335435d616 100644
@@ -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) {
 			/*

index 485e7b04794..96102c222bf 100644
@@ -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 */
 

gitgitgadget

From: Elijah Newren <[email protected]>

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=<gitignore>, 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.

---
 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(-)

index 5fa8bab64c2..0222a27c5af 100644
@@ -10,8 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git read-tree' [[-m [--trivial] [--aggressive] | --reset | --prefix=<prefix>]
-		[-u [--exclude-per-directory=<gitignore>] | -i]]
-		[--index-output=<file>] [--no-sparse-checkout]
+		[-u | -i]] [--index-output=<file>] [--no-sparse-checkout]
 		(--empty | <tree-ish1> [<tree-ish2> [<tree-ish3>]])
 
 

@@ -88,21 +87,6 @@ OPTIONS
 	The command will refuse to overwrite entries that already
 	existed in the original index file.
 
---exclude-per-directory=<gitignore>::
-	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=<file>::
 	Instead of writing the results out to `$GIT_INDEX_FILE`,
 	write the resulting index in the named file.  While the