[PATCH 0/6] 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/xmqqv93n7q1v.fsf@gitster.g/ [2]
https://lore.kernel.org/git/C357A648-8B13-45C3-9388-C0C7F7D40DAE@gmail.com/
[3] https://lore.kernel.org/git/xmqqr1e2ejs9.fsf@gitster.g/

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 <redacted>

Noting that unpack_trees treats reset=1 & update=1 as license to nuke
untracked files, I looked for code paths that use this combination and
tried to generate testcases which demonstrated unintentional loss of
untracked files and directories.  I found several.

I also include testcases for `git reset --{hard,merge,keep}`.  A hard
reset is perhaps the most direct test of unpack_tree's reset=1 behavior,
but we cannot make `git reset --hard` preserve untracked files without
some migration work.

Also, the two commands `checkout --force` (because of the --force) and
`read-tree --reset` (because it's plumbing and we need to keep it
backward compatible) were left out as we expect those to continue
removing untracked files and directories.

Signed-off-by: Elijah Newren <redacted>
---
 t/t2500-untracked-overwriting.sh | 244 +++++++++++++++++++++++++++++++
 1 file changed, 244 insertions(+)
 create mode 100755 t/t2500-untracked-overwriting.sh

diff --git a/t/t2500-untracked-overwriting.sh b/t/t2500-untracked-overwriting.sh
new file mode 100755
index 00000000000..a1a6dfa671e
--- /dev/null
+++ b/t/t2500-untracked-overwriting.sh

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

From: Elijah Newren <redacted>

Signed-off-by: Elijah Newren <redacted>
---
 t/t2500-untracked-overwriting.sh | 2 +-
 unpack-trees.c                   | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/t/t2500-untracked-overwriting.sh b/t/t2500-untracked-overwriting.sh
index 017946a494f..d4d9dc928aa 100755
--- a/t/t2500-untracked-overwriting.sh
+++ b/t/t2500-untracked-overwriting.sh

@@ -218,7 +218,7 @@ test_expect_success 'git am --abort and untracked dir vs. unmerged file' '
 	)
 '
 
-test_expect_failure 'git am --skip and untracked dir vs deleted file' '
+test_expect_success 'git am --skip and untracked dir vs deleted file' '
 	test_setup_sequencing am_skip_and_untracked &&
 	(
 		cd sequencing_am_skip_and_untracked &&

diff --git a/unpack-trees.c b/unpack-trees.c
index 3b3d1c0ff40..858595a13f1 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c

@@ -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

From: Elijah Newren <redacted>

Signed-off-by: Elijah Newren <redacted>
---
 t/t2500-untracked-overwriting.sh |  2 +-
 unpack-trees.c                   | 35 ++++++++++++++++++++++++++++----
 2 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/t/t2500-untracked-overwriting.sh b/t/t2500-untracked-overwriting.sh
index 786ec33d63a..017946a494f 100755
--- a/t/t2500-untracked-overwriting.sh
+++ b/t/t2500-untracked-overwriting.sh

@@ -197,7 +197,7 @@ test_expect_failure 'git stash and untracked files' '
 	)
 '
 
-test_expect_failure 'git am --abort and untracked dir vs. unmerged file' '
+test_expect_success 'git am --abort and untracked dir vs. unmerged file' '
 	test_setup_sequencing am_abort_and_untracked &&
 	(
 		cd sequencing_am_abort_and_untracked &&

diff --git a/unpack-trees.c b/unpack-trees.c
index d952eebe96a..3b3d1c0ff40 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c

@@ -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 <redacted>

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.

Signed-off-by: Elijah Newren <redacted>
---
 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(-)

diff --git a/builtin/am.c b/builtin/am.c
index c79e0167e98..dbe6cbe6a33 100644
--- a/builtin/am.c
+++ b/builtin/am.c

@@ -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);
 

diff --git a/builtin/checkout.c b/builtin/checkout.c
index b5d477919a7..ab0bb4d94f0 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c

@@ -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);

diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index 485e7b04794..8b94e1aa261 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c

@@ -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;

diff --git a/builtin/reset.c b/builtin/reset.c
index 43e855cb887..ba39c4882a6 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c

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

diff --git a/builtin/stash.c b/builtin/stash.c
index 8f42360ca91..4ceb3581b47 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c

@@ -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;
 

diff --git a/reset.c b/reset.c
index 79310ae071b..0880c76aef9 100644
--- a/reset.c
+++ b/reset.c

@@ -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"));

diff --git a/t/t1013-read-tree-submodule.sh b/t/t1013-read-tree-submodule.sh
index b6df7444c05..4e485c223ad 100755
--- a/t/t1013-read-tree-submodule.sh
+++ b/t/t1013-read-tree-submodule.sh

@@ -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

diff --git a/t/t2500-untracked-overwriting.sh b/t/t2500-untracked-overwriting.sh
index a1a6dfa671e..786ec33d63a 100755
--- a/t/t2500-untracked-overwriting.sh
+++ b/t/t2500-untracked-overwriting.sh

@@ -92,7 +92,7 @@ test_setup_checkout_m () {
 	)
 }
 
-test_expect_failure 'checkout -m does not nuke untracked file' '
+test_expect_success 'checkout -m does not nuke untracked file' '
 	test_setup_checkout_m &&
 	(
 		cd checkout &&

@@ -138,7 +138,7 @@ test_setup_sequencing () {
 	)
 }
 
-test_expect_failure 'git rebase --abort and untracked files' '
+test_expect_success 'git rebase --abort and untracked files' '
 	test_setup_sequencing rebase_abort_and_untracked &&
 	(
 		cd sequencing_rebase_abort_and_untracked &&

@@ -155,7 +155,7 @@ test_expect_failure 'git rebase --abort and untracked files' '
 	)
 '
 
-test_expect_failure 'git rebase fast forwarding and untracked files' '
+test_expect_success 'git rebase fast forwarding and untracked files' '
 	test_setup_sequencing rebase_fast_forward_and_untracked &&
 	(
 		cd sequencing_rebase_fast_forward_and_untracked &&

diff --git a/unpack-trees.c b/unpack-trees.c
index 5786645f315..d952eebe96a 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c

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

diff --git a/unpack-trees.h b/unpack-trees.h
index 2d88b19dca7..c419bf8b1f9 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h

@@ -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

From: Elijah Newren <redacted>

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

Signed-off-by: Elijah Newren <redacted>
---
 Documentation/git-checkout.txt  | 5 +++--
 Documentation/git-read-tree.txt | 5 +++--
 Documentation/git-reset.txt     | 3 ++-
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index b1a6fe44997..d473c9bf387 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt

@@ -118,8 +118,9 @@ OPTIONS
 -f::
 --force::
 	When switching branches, proceed even if the index or the
-	working tree differs from `HEAD`.  This is used to throw away
-	local changes.
+	working tree differs from `HEAD`, and even if there are untracked
+	files in the way.  This is used to throw away local changes and
+	any untracked files or directories that are in the way.
 +
 When checking out paths from the index, do not fail upon unmerged
 entries; instead, unmerged entries are ignored.

diff --git a/Documentation/git-read-tree.txt b/Documentation/git-read-tree.txt
index 5fa8bab64c2..4731ec3283f 100644
--- a/Documentation/git-read-tree.txt
+++ b/Documentation/git-read-tree.txt

@@ -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

diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index 252e2d4e47d..6f7685f53d5 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt

@@ -69,7 +69,8 @@ linkgit:git-add[1]).
 
 --hard::
 	Resets the index and working tree. Any changes to tracked files in the
-	working tree since `<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

From: Elijah Newren <redacted>

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

Signed-off-by: Elijah Newren <redacted>
---
 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(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index 4ceb3581b47..ce5e0364c68 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c

@@ -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)) {

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ef2776a9e45..a49242d15ae 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c

@@ -2864,6 +2864,10 @@ static int add_submodule(const struct add_data *add_data)
 		prepare_submodule_repo_env(&cp.env_array);
 		cp.git_cmd = 1;
 		cp.dir = add_data->sm_path;
+		/*
+		 * NOTE: we only get here if add_data->force is true, so
+		 * passing --force to checkout is reasonable.
+		 */
 		strvec_pushl(&cp.args, "checkout", "-f", "-q", NULL);
 
 		if (add_data->branch) {

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 0d0a80da61f..383947ff54f 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c

@@ -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");

diff --git a/contrib/rerere-train.sh b/contrib/rerere-train.sh
index eeee45dd341..75125d6ae00 100755
--- a/contrib/rerere-train.sh
+++ b/contrib/rerere-train.sh

@@ -91,7 +91,7 @@ do
 		git checkout -q $commit -- .
 		git rerere
 	fi
-	git reset -q --hard
+	git reset -q --hard  # Might nuke untracked files...
 done
 
 if test -z "$branch"

diff --git a/submodule.c b/submodule.c
index 8e611fe1dbf..a9b71d585cf 100644
--- a/submodule.c
+++ b/submodule.c

@@ -1866,6 +1866,7 @@ static void submodule_reset_index(const char *path)
 
 	strvec_pushf(&cp.args, "--super-prefix=%s%s/",
 		     get_super_prefix_or_empty(), path);
+	/* TODO: determine if this might overwright untracked files */
 	strvec_pushl(&cp.args, "read-tree", "-u", "--reset", NULL);
 
 	strvec_push(&cp.args, empty_tree_oid_hex());

-- 
gitgitgadget

truly minor nit.
On 19/09/2021 00:15, Elijah Newren via GitGitGadget wrote:

double space after full stop?

Is the single space to double space change desired?
I had the impression that the project had decided on single spaces, but
I can't see anything in SubmittingPatches or CodingGuidelines. I don't
think there are DocumentationGuidelines.

as above,  s /.  /. /  ?

+	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

--
Philip

On 19/09/2021 11:52, Philip Oakley wrote:

truly minor nit.
On 19/09/2021 00:15, Elijah Newren via GitGitGadget wrote:

quoted

From: Elijah Newren <redacted>

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

Signed-off-by: Elijah Newren <redacted>
---
 Documentation/git-checkout.txt  | 5 +++--
 Documentation/git-read-tree.txt | 5 +++--
 Documentation/git-reset.txt     | 3 ++-
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index b1a6fe44997..d473c9bf387 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt

@@ -118,8 +118,9 @@ OPTIONS
 -f::
 --force::
 	When switching branches, proceed even if the index or the
-	working tree differs from `HEAD`.  This is used to throw away
-	local changes.
+	working tree differs from `HEAD`, and even if there are untracked
+	files in the way.  This is used to throw away local changes and

double space after full stop?

quoted

+	any untracked files or directories that are in the way.
 +
 When checking out paths from the index, do not fail upon unmerged
 entries; instead, unmerged entries are ignored.

diff --git a/Documentation/git-read-tree.txt b/Documentation/git-read-tree.txt
index 5fa8bab64c2..4731ec3283f 100644
--- a/Documentation/git-read-tree.txt
+++ b/Documentation/git-read-tree.txt

@@ -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

Is the single space to double space change desired?
I had the impression that the project had decided on single spaces, but
I can't see anything in SubmittingPatches or CodingGuidelines. I don't
think there are DocumentationGuidelines.

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

quoted

+	working tree changes or untracked files or directories will not
+	abort the operation.
 
 -u::
 	After a successful merge, update the files in the work

diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index 252e2d4e47d..6f7685f53d5 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt

@@ -69,7 +69,8 @@ linkgit:git-add[1]).
 
 --hard::
 	Resets the index and working tree. Any changes to tracked files in the
-	working tree since `<commit>` are discarded.
+	working tree since `<commit>` are discarded.  Any untracked files or

as above,  s /.  /. /  ?

quoted

+	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

[1] https://lore.kernel.org/git/7vfxtu3fku.fsf@gitster.siamese.dyndns.org/
[2] https://en.wikipedia.org/wiki/Sentence_spacing

On Sat, Sep 18 2021, Elijah Newren via GitGitGadget wrote:

+	test_create_repo reset_$1 &&

s/test_create_repo/git init/ these days (also for the rest).

+		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

The test is ambiguous about whether we complain about foo.t/file, or
foo.t, if there was foo.t{file,file-two} would we complain just once or
twice?

I think it's just the directory, but probably worthwhile for the test to
make the distinction. If it's a "a/sub/dir/file" do we complain about
"a/" or "a/sub/dir/" ?

On Sat, Sep 18 2021, Elijah Newren via GitGitGadget wrote:

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

Is the "opts.dir" free'd later somehow?

 	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;

In both cases opts.reset_keep_untracked is set to 1, I assume it's a
mistake, aside from that perhaps betteras:

    opts.reset_keep_untracked = o->force; /* or !o->force, depending... */

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

ditto potential leak.

+		opts.dir = xcalloc(1, sizeof(*opts.dir));
+		opts.dir->flags |= DIR_SHOW_IGNORED;
+		setup_standard_excludes(opts.dir);
+	}

ditto (also more omitted).

On Sat, Sep 18 2021, Elijah Newren via GitGitGadget wrote:

Maybe just "else if" ?

[...]

+

Stray whitespace change

On 19/09/2021 00:15, Elijah Newren via GitGitGadget wrote:

From: Elijah Newren <redacted>

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.

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

Modify existing callers so that
    read-tree --reset

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.

    reset --hard
    checkout --force

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.

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.

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.

Does this clobber any excludes added by --exclude-per-directory?

There is no case SOFT: but in that case we don't call reset_index() so 
we're OK.

Unrelated to this patch but this looks dodgy to me. For 'git rebase 
<upstream> <branch>' where <branch> is ahead of <upstream> we skip the 
rebase and use reset_head() to checkout <branch> without 'detach_head' 
set. I think this should be checking 'reset_hard' instead of 'detach_head'

<bikeshed>
o->reset_either = o->reset_nuke_untracked | o->reset_keep_untracked
</bikeshed>

I think I prefer the enum approach in [1] but I'm biased and I'm not 
sure it's worth getting excited about. Thanks for working on this it 
will be great to have git stop overwriting untracked files so often.

Best Wishes

Phillip

[1] 
https://lore.kernel.org/git/20190501101403.20294-1-phillip.wood123@gmail.com/

  		     merge,
  		     update,
  		     clone,

On Sun, Sep 19, 2021 at 6:47 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

On Sat, Sep 18 2021, Elijah Newren via GitGitGadget wrote:

quoted

+     test_create_repo reset_$1 &&

s/test_create_repo/git init/ these days (also for the rest).

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

quoted

+             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

The test is ambiguous about whether we complain about foo.t/file, or
foo.t, if there was foo.t{file,file-two} would we complain just once or
twice?

I think it's just the directory, but probably worthwhile for the test to
make the distinction. If it's a "a/sub/dir/file" do we complain about
"a/" or "a/sub/dir/" ?

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

On Sun, Sep 19, 2021 at 6:51 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

On Sat, Sep 18 2021, Elijah Newren via GitGitGadget wrote:

quoted

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

Is the "opts.dir" free'd later somehow?

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...

quoted

      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;

In both cases opts.reset_keep_untracked is set to 1, I assume it's a
mistake

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.

quoted

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

ditto potential leak.

quoted

+             opts.dir = xcalloc(1, sizeof(*opts.dir));
+             opts.dir->flags |= DIR_SHOW_IGNORED;
+             setup_standard_excludes(opts.dir);
+     }

ditto (also more omitted).

Yep.

On Mon, Sep 20, 2021 at 3:19 AM Phillip Wood [off-list ref] wrote:

On 19/09/2021 00:15, Elijah Newren via GitGitGadget wrote:

quoted

From: Elijah Newren <redacted>

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.

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

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

quoted

Modify existing callers so that
    read-tree --reset

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.

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.

quoted

    reset --hard
    checkout --force

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.

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/xmqqr1e2ejs9.fsf@gitster.g/.  Seems you
also felt that way previously, at
https://lore.kernel.org/git/d4c36a24-b40c-a6ca-7a05-572ab93a0101@gmail.com/
-- any reason for your change of opinion?

quoted

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.

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, that's interesting.  Yeah, that's worth mentioning and perhaps digging into.

quoted

[...]

diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index 485e7b04794..8b94e1aa261 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c

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

Does this clobber any excludes added by --exclude-per-directory?

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.

quoted

diff --git a/builtin/reset.c b/builtin/reset.c
index 43e855cb887..ba39c4882a6 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c

@@ -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");

There is no case SOFT: but in that case we don't call reset_index() so
we're OK.

quoted

diff --git a/reset.c b/reset.c
index 79310ae071b..0880c76aef9 100644
--- a/reset.c
+++ b/reset.c

@@ -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;

Unrelated to this patch but this looks dodgy to me. For 'git rebase
<upstream> <branch>' where <branch> is ahead of <upstream> we skip the
rebase and use reset_head() to checkout <branch> without 'detach_head'
set. I think this should be checking 'reset_hard' instead of 'detach_head'

quoted

diff --git a/unpack-trees.c b/unpack-trees.c
index 5786645f315..d952eebe96a 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c

@@ -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;

<bikeshed>
o->reset_either = o->reset_nuke_untracked | o->reset_keep_untracked
</bikeshed>

Goes away entirely if we adopt your enum suggestion.

quoted

diff --git a/unpack-trees.h b/unpack-trees.h
index 2d88b19dca7..c419bf8b1f9 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h

@@ -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 */

I think I prefer the enum approach in [1] but I'm biased and I'm not
sure it's worth getting excited about. Thanks for working on this it
will be great to have git stop overwriting untracked files so often.

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

On Sun, Sep 19, 2021 at 6:52 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

On Sat, Sep 18 2021, Elijah Newren via GitGitGadget wrote:

quoted

From: Elijah Newren <redacted>

Signed-off-by: Elijah Newren <redacted>
---
 t/t2500-untracked-overwriting.sh | 2 +-
 unpack-trees.c                   | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/t/t2500-untracked-overwriting.sh b/t/t2500-untracked-overwriting.sh
index 017946a494f..d4d9dc928aa 100755
--- a/t/t2500-untracked-overwriting.sh
+++ b/t/t2500-untracked-overwriting.sh

@@ -218,7 +218,7 @@ test_expect_success 'git am --abort and untracked dir vs. unmerged file' '
      )
 '

-test_expect_failure 'git am --skip and untracked dir vs deleted file' '
+test_expect_success 'git am --skip and untracked dir vs deleted file' '
      test_setup_sequencing am_skip_and_untracked &&
      (
              cd sequencing_am_skip_and_untracked &&

diff --git a/unpack-trees.c b/unpack-trees.c
index 3b3d1c0ff40..858595a13f1 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c

@@ -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;
      }

Maybe just "else if" ?

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.

quoted

+

Stray whitespace change

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.

On Sun, Sep 19, 2021 at 6:36 AM Philip Oakley [off-list ref] wrote:

On 19/09/2021 11:52, Philip Oakley wrote:

quoted

truly minor nit.
On 19/09/2021 00:15, Elijah Newren via GitGitGadget wrote:

quoted

From: Elijah Newren <redacted>

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

Signed-off-by: Elijah Newren <redacted>
---
 Documentation/git-checkout.txt  | 5 +++--
 Documentation/git-read-tree.txt | 5 +++--
 Documentation/git-reset.txt     | 3 ++-
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index b1a6fe44997..d473c9bf387 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt

@@ -118,8 +118,9 @@ OPTIONS
 -f::
 --force::
     When switching branches, proceed even if the index or the
-    working tree differs from `HEAD`.  This is used to throw away
-    local changes.
+    working tree differs from `HEAD`, and even if there are untracked
+    files in the way.  This is used to throw away local changes and

double space after full stop?

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

quoted

quoted

+    any untracked files or directories that are in the way.
 +
 When checking out paths from the index, do not fail upon unmerged
 entries; instead, unmerged entries are ignored.

diff --git a/Documentation/git-read-tree.txt b/Documentation/git-read-tree.txt
index 5fa8bab64c2..4731ec3283f 100644
--- a/Documentation/git-read-tree.txt
+++ b/Documentation/git-read-tree.txt

@@ -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

Is the single space to double space change desired?
I had the impression that the project had decided on single spaces, but
I can't see anything in SubmittingPatches or CodingGuidelines. I don't
think there are DocumentationGuidelines.

Double space is better as per Junio's declaration here:
https://lore.kernel.org/git/xmqqftchkext.fsf@gitster.c.googlers.com/

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.

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.

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.

[1] https://lore.kernel.org/git/7vfxtu3fku.fsf@gitster.siamese.dyndns.org/
[2] https://en.wikipedia.org/wiki/Sentence_spacing

On 20/09/2021 17:05, Elijah Newren wrote:

On Mon, Sep 20, 2021 at 3:19 AM Phillip Wood [off-list ref] wrote:

quoted

On 19/09/2021 00:15, Elijah Newren via GitGitGadget wrote:

quoted

From: Elijah Newren <redacted>

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.

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

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

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.

quoted

quoted

Modify existing callers so that
     read-tree --reset

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.

Those commands made no distinction between untracked and ignored files
previously, and overwrote all of them.

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

 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.

quoted

quoted

     reset --hard
     checkout --force

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.

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/xmqqr1e2ejs9.fsf@gitster.g/.  Seems you
also felt that way previously, at
https://lore.kernel.org/git/d4c36a24-b40c-a6ca-7a05-572ab93a0101@gmail.com/
-- any reason for your change of opinion?

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'

quoted

quoted

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.

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, that's interesting.  Yeah, that's worth mentioning and perhaps digging into.

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

Best Wishes

Phillip

quoted

quoted

[...]

diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index 485e7b04794..8b94e1aa261 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c

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

Does this clobber any excludes added by --exclude-per-directory?

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.

quoted

quoted

diff --git a/builtin/reset.c b/builtin/reset.c
index 43e855cb887..ba39c4882a6 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c

@@ -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");

There is no case SOFT: but in that case we don't call reset_index() so
we're OK.

quoted

diff --git a/reset.c b/reset.c
index 79310ae071b..0880c76aef9 100644
--- a/reset.c
+++ b/reset.c

@@ -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;

Unrelated to this patch but this looks dodgy to me. For 'git rebase
<upstream> <branch>' where <branch> is ahead of <upstream> we skip the
rebase and use reset_head() to checkout <branch> without 'detach_head'
set. I think this should be checking 'reset_hard' instead of 'detach_head'

quoted

diff --git a/unpack-trees.c b/unpack-trees.c
index 5786645f315..d952eebe96a 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c

@@ -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;

<bikeshed>
o->reset_either = o->reset_nuke_untracked | o->reset_keep_untracked
</bikeshed>

Goes away entirely if we adopt your enum suggestion.

quoted

quoted

diff --git a/unpack-trees.h b/unpack-trees.h
index 2d88b19dca7..c419bf8b1f9 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h

@@ -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 */

I think I prefer the enum approach in [1] but I'm biased and I'm not
sure it's worth getting excited about. Thanks for working on this it
will be great to have git stop overwriting untracked files so often.

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

On Mon, Sep 20, 2021 at 11:11 AM Phillip Wood [off-list ref] wrote:

On 20/09/2021 17:05, Elijah Newren wrote:

quoted

On Mon, Sep 20, 2021 at 3:19 AM Phillip Wood [off-list ref] wrote:

quoted

On 19/09/2021 00:15, Elijah Newren via GitGitGadget wrote:

quoted

From: Elijah Newren <redacted>

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.

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

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

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.

quoted

quoted

quoted

Modify existing callers so that
     read-tree --reset

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.

Those commands made no distinction between untracked and ignored files
previously, and overwrote all of them.

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

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.

quoted

 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.

quoted

quoted

     reset --hard
     checkout --force

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.

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/xmqqr1e2ejs9.fsf@gitster.g/.  Seems you
also felt that way previously, at
https://lore.kernel.org/git/d4c36a24-b40c-a6ca-7a05-572ab93a0101@gmail.com/
-- any reason for your change of opinion?

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'

quoted

quoted

quoted

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.

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, that's interesting.  Yeah, that's worth mentioning and perhaps digging into.

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

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!

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
   > 0 (even if it may make sense to nuke ignored files when running either
   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/xmqqv93n7q1v.fsf@gitster.g/ [2]
https://lore.kernel.org/git/C357A648-8B13-45C3-9388-C0C7F7D40DAE@gmail.com/
[3] https://lore.kernel.org/git/xmqqr1e2ejs9.fsf@gitster.g/

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 [off-list ref]
      
       ## 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 [off-list ref]
      
     @@ 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 <redacted>

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.

Signed-off-by: Elijah Newren <redacted>
---
 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(-)

diff --git a/builtin/am.c b/builtin/am.c
index c79e0167e98..b17baa67ad8 100644
--- a/builtin/am.c
+++ b/builtin/am.c

@@ -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);