git fetch https://github.com/gitgitgadget/git pr-1036/newren/untracked_removal-v1

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-1036/newren/untracked_removal-v1

git fetch https://github.com/gitgitgadget/git pr-1036/newren/untracked_removal-v2

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-1036/newren/untracked_removal-v2

From: Elijah Newren <[email protected]>

Currently, every caller of unpack_trees() that wants to ensure ignored
files are overwritten by default needs to:

   * flip the DIR_SHOW_IGNORED flag in unpack_trees_options.dir.flags
   * call setup_standard_excludes(&unpack_trees_options.dir)

Avoid that boilerplate by introducing a new boolean value where
the default value (0) does what we want so that new callers of
unpack_trees() automatically get the appropriate behavior.  And move all
the handling of unpack_trees_options.dir into unpack_trees() itself.

While preserve_ignored = 0 is the behavior we feel is the appropriate
default, we defer fixing commands to use the appropriate default until a
later commit.  So, this commit introduces several locations where we
manually set preserve_ignored=1.  This makes it clear where code paths
were previously preserving ignored files when they should not have been;
a future commit will flip these to instead use a value of 0 to get the
behavior we want.

Signed-off-by: Elijah Newren <[email protected]>
Signed-off-by: Ævar Arnfjörð Bjarmason <[email protected]>
---
 builtin/am.c        | 3 +++
 builtin/checkout.c  | 6 ++----
 builtin/clone.c     | 2 ++
 builtin/merge.c     | 2 ++
 builtin/read-tree.c | 7 +++----
 builtin/reset.c     | 2 ++
 builtin/stash.c     | 3 +++
 merge-ort.c         | 5 +----
 merge-recursive.c   | 4 ++--
 merge.c             | 6 +-----
 reset.c             | 2 ++
 sequencer.c         | 2 ++
 unpack-trees.c      | 5 +++++
 unpack-trees.h      | 1 +
 14 files changed, 31 insertions(+), 19 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 4d4bb473c0f..3c6efe2a46b 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1918,6 +1918,9 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)
 	opts.update = 1;
 	opts.merge = 1;
 	opts.reset = reset;
+	if (!reset)
+		/* FIXME: Default should be to remove ignored files */
+		opts.preserve_ignored = 1;
 	opts.fn = twoway_merge;
 	init_tree_desc(&t[0], head->buffer, head->size);
 	init_tree_desc(&t[1], remote->buffer, remote->size);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index fd76b504861..0c5187025c5 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -648,6 +648,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
 	opts.skip_unmerged = !worktree;
 	opts.reset = 1;
 	opts.merge = 1;
+	opts.preserve_ignored = 0;
 	opts.fn = oneway_merge;
 	opts.verbose_update = o->show_progress;
 	opts.src_index = &the_index;
@@ -749,10 +750,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
 				       new_branch_info->commit ?
 				       &new_branch_info->commit->object.oid :
 				       &new_branch_info->oid, NULL);
-		if (opts->overwrite_ignore) {
-			topts.dir.flags |= DIR_SHOW_IGNORED;
-			setup_standard_excludes(&topts.dir);
-		}
+		topts.preserve_ignored = !opts->overwrite_ignore;
 		tree = parse_tree_indirect(old_branch_info->commit ?
 					   &old_branch_info->commit->object.oid :
 					   the_hash_algo->empty_tree);
diff --git a/builtin/clone.c b/builtin/clone.c
index df3bb9a7884..e76c38e4e81 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -686,6 +686,8 @@ static int checkout(int submodule_progress)
 	opts.update = 1;
 	opts.merge = 1;
 	opts.clone = 1;
+	/* FIXME: Default should be to remove ignored files */
+	opts.preserve_ignored = 1;
 	opts.fn = oneway_merge;
 	opts.verbose_update = (option_verbosity >= 0);
 	opts.src_index = &the_index;
diff --git a/builtin/merge.c b/builtin/merge.c
index 28089e2c5ed..55aac869e5a 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -680,6 +680,8 @@ static int read_tree_trivial(struct object_id *common, struct object_id *head,
 	opts.verbose_update = 1;
 	opts.trivial_merges_only = 1;
 	opts.merge = 1;
+	/* FIXME: Default should be to remove ignored files */
+	opts.preserve_ignored = 1;
 	trees[nr_trees] = parse_tree_indirect(common);
 	if (!trees[nr_trees++])
 		return -1;
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index d0f88bbf3e3..7f3c987b126 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -201,10 +201,9 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
 	if ((opts.update || opts.index_only) && !opts.merge)
 		die("%s is meaningless without -m, --reset, or --prefix",
 		    opts.update ? "-u" : "-i");
-	if (opts.update && !opts.reset) {
-		opts.dir.flags |= DIR_SHOW_IGNORED;
-		setup_standard_excludes(&opts.dir);
-	}
+	if (opts.update && !opts.reset)
+		opts.preserve_ignored = 0;
+	/* otherwise, opts.preserve_ignored is irrelevant */
 	if (opts.merge && !opts.index_only)
 		setup_work_tree();
 
diff --git a/builtin/reset.c b/builtin/reset.c
index 713d084c3eb..73477239146 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -66,6 +66,8 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t
 	case KEEP:
 	case MERGE:
 		opts.update = 1;
+		/* FIXME: Default should be to remove ignored files */
+		opts.preserve_ignored = 1;
 		break;
 	case HARD:
 		opts.update = 1;
diff --git a/builtin/stash.c b/builtin/stash.c
index be6ecb1ae11..78492013529 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -257,6 +257,9 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)
 	opts.merge = 1;
 	opts.reset = reset;
 	opts.update = update;
+	if (update && !reset)
+		/* FIXME: Default should be to remove ignored files */
+		opts.preserve_ignored = 1;
 	opts.fn = oneway_merge;
 
 	if (unpack_trees(nr_trees, t, &opts)) {
diff --git a/merge-ort.c b/merge-ort.c
index 0a5937364c9..e5620bda212 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -4044,10 +4044,7 @@ static int checkout(struct merge_options *opt,
 	unpack_opts.quiet = 0; /* FIXME: sequencer might want quiet? */
 	unpack_opts.verbose_update = (opt->verbosity > 2);
 	unpack_opts.fn = twoway_merge;
-	if (1/* FIXME: opts->overwrite_ignore*/) {
-		unpack_opts.dir.flags |= DIR_SHOW_IGNORED;
-		setup_standard_excludes(&unpack_opts.dir);
-	}
+	unpack_opts.preserve_ignored = 0; /* FIXME: !opts->overwrite_ignore*/
 	parse_tree(prev);
 	init_tree_desc(&trees[0], prev->buffer, prev->size);
 	parse_tree(next);
diff --git a/merge-recursive.c b/merge-recursive.c
index a4131b8837b..5c6b95a79c0 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -410,8 +410,8 @@ static int unpack_trees_start(struct merge_options *opt,
 		opt->priv->unpack_opts.index_only = 1;
 	else {
 		opt->priv->unpack_opts.update = 1;
-		opt->priv->unpack_opts.dir.flags |= DIR_SHOW_IGNORED;
-		setup_standard_excludes(&opt->priv->unpack_opts.dir);
+		/* FIXME: should only do this if !overwrite_ignore */
+		opt->priv->unpack_opts.preserve_ignored = 0;
 	}
 	opt->priv->unpack_opts.merge = 1;
 	opt->priv->unpack_opts.head_idx = 2;
diff --git a/merge.c b/merge.c
index 2e3714ccaa0..e1f3165e407 100644
--- a/merge.c
+++ b/merge.c
@@ -79,11 +79,7 @@ int checkout_fast_forward(struct repository *r,
 		init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);
 	}
 
-	if (overwrite_ignore) {
-		opts.dir.flags |= DIR_SHOW_IGNORED;
-		setup_standard_excludes(&opts.dir);
-	}
-
+	opts.preserve_ignored = !overwrite_ignore;
 	opts.head_idx = 1;
 	opts.src_index = r->index;
 	opts.dst_index = r->index;
diff --git a/reset.c b/reset.c
index f4bf3fbfac0..cd344f47f13 100644
--- a/reset.c
+++ b/reset.c
@@ -56,6 +56,8 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,
 	unpack_tree_opts.fn = reset_hard ? oneway_merge : twoway_merge;
 	unpack_tree_opts.update = 1;
 	unpack_tree_opts.merge = 1;
+	/* FIXME: Default should be to remove ignored files */
+	unpack_tree_opts.preserve_ignored = 1;
 	init_checkout_metadata(&unpack_tree_opts.meta, switch_to_branch, oid, NULL);
 	if (!detach_head)
 		unpack_tree_opts.reset = 1;
diff --git a/sequencer.c b/sequencer.c
index abd85b6c562..669ea15944c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3698,6 +3698,8 @@ static int do_reset(struct repository *r,
 	unpack_tree_opts.fn = oneway_merge;
 	unpack_tree_opts.merge = 1;
 	unpack_tree_opts.update = 1;
+	/* FIXME: Default should be to remove ignored files */
+	unpack_tree_opts.preserve_ignored = 1;
 	init_checkout_metadata(&unpack_tree_opts.meta, name, &oid, NULL);
 
 	if (repo_read_index_unmerged(r)) {
diff --git a/unpack-trees.c b/unpack-trees.c
index 260e7ec5bb4..02bc999c6c3 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1711,6 +1711,11 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 		ensure_full_index(o->dst_index);
 	}
 
+	if (!o->preserve_ignored) {
+		o->dir.flags |= DIR_SHOW_IGNORED;
+		setup_standard_excludes(&o->dir);
+	}
+
 	if (!core_apply_sparse_checkout || !o->update)
 		o->skip_sparse_checkout = 1;
 	if (!o->skip_sparse_checkout && !o->pl) {
diff --git a/unpack-trees.h b/unpack-trees.h
index a8d1f083b33..65a8d99d4ef 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -52,6 +52,7 @@ struct unpack_trees_options {
 	unsigned int reset,
 		     merge,
 		     update,
+		     preserve_ignored,
 		     clone,
 		     index_only,
 		     nontrivial_merge,
-- 
2.33.0.1404.g83021034c5d

From: Elijah Newren <[email protected]>

Until the introduction of the "preserve_ignored" flag in the preceding
commit callers who wanted its behavior needed to adjust "dir.flags"
and call setup_standard_excludes() themselves.

Now that we have no external users of "dir" anymore let's rename it to
"private_dir" and add a comment indicating that we'd like it not to be
messed with by external callers. This should avoid avoid accidental
misuse or confusion over its ownership.

Signed-off-by: Elijah Newren <[email protected]>
Signed-off-by: Ævar Arnfjörð Bjarmason <[email protected]>
---
 unpack-trees.c | 10 +++++-----
 unpack-trees.h |  4 ++--
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index 02bc999c6c3..512011cfa42 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -196,7 +196,7 @@ void unpack_trees_options_init(struct unpack_trees_options *o)
 void unpack_trees_options_release(struct unpack_trees_options *opts)
 {
 	strvec_clear(&opts->msgs_to_free);
-	dir_clear(&opts->dir);
+	dir_clear(&opts->private_dir);
 }
 
 static int do_add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
@@ -1712,8 +1712,8 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 	}
 
 	if (!o->preserve_ignored) {
-		o->dir.flags |= DIR_SHOW_IGNORED;
-		setup_standard_excludes(&o->dir);
+		o->private_dir.flags |= DIR_SHOW_IGNORED;
+		setup_standard_excludes(&o->private_dir);
 	}
 
 	if (!core_apply_sparse_checkout || !o->update)
@@ -2141,7 +2141,7 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,
 	 */
 	pathbuf = xstrfmt("%.*s/", namelen, ce->name);
 
-	d.exclude_per_dir = o->dir.exclude_per_dir;
+	d.exclude_per_dir = o->private_dir.exclude_per_dir;
 	i = read_directory(&d, o->src_index, pathbuf, namelen+1, NULL);
 	dir_clear(&d);
 	free(pathbuf);
@@ -2183,7 +2183,7 @@ static int check_ok_to_remove(const char *name, int len, int dtype,
 	if (ignore_case && icase_exists(o, name, len, st))
 		return 0;
 
-	if (is_excluded(&o->dir, o->src_index, name, &dtype))
+	if (is_excluded(&o->private_dir, o->src_index, name, &dtype))
 		/*
 		 * ce->name is explicitly excluded, so it is Ok to
 		 * overwrite it.
diff --git a/unpack-trees.h b/unpack-trees.h
index 65a8d99d4ef..2eb633bf771 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -70,7 +70,7 @@ struct unpack_trees_options {
 		     dry_run;
 	const char *prefix;
 	int cache_bottom;
-	struct dir_struct dir;
+	struct dir_struct private_dir; /* for internal use only */
 	struct pathspec *pathspec;
 	merge_fn_t fn;
 	const char *msgs[NB_UNPACK_TREES_WARNING_TYPES];
@@ -97,7 +97,7 @@ struct unpack_trees_options {
 
 #define UNPACK_TREES_OPTIONS_INIT { \
 	.msgs_to_free = STRVEC_INIT, \
-	.dir = DIR_INIT, \
+	.private_dir = DIR_INIT, \
 }
 void unpack_trees_options_init(struct unpack_trees_options *o);
 
-- 
2.33.0.1404.g83021034c5d

From: Elijah Newren <[email protected]>

Change several commands to remove ignored files by default when they are
in the way.  Since some commands (checkout, merge) take a
--no-overwrite-ignore option to allow the user to configure this, and it
may make sense to add that option to more commands (and in the case of
merge, actually plumb that configuration option through to more of the
backends than just the fast-forwarding special case), add little
comments about where such flags would be used.

Incidentally, this fixes a test failure in t7112.

Signed-off-by: Elijah Newren <[email protected]>
Signed-off-by: Ævar Arnfjörð Bjarmason <[email protected]>
---
 builtin/am.c               | 3 +--
 builtin/clone.c            | 3 +--
 builtin/merge.c            | 3 +--
 builtin/reset.c            | 3 +--
 builtin/stash.c            | 3 +--
 merge-ort.c                | 2 +-
 reset.c                    | 3 +--
 sequencer.c                | 3 +--
 t/t7112-reset-submodule.sh | 1 -
 9 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 3c6efe2a46b..8cb7e72e6c5 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1919,8 +1919,7 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)
 	opts.merge = 1;
 	opts.reset = reset;
 	if (!reset)
-		/* FIXME: Default should be to remove ignored files */
-		opts.preserve_ignored = 1;
+		opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */
 	opts.fn = twoway_merge;
 	init_tree_desc(&t[0], head->buffer, head->size);
 	init_tree_desc(&t[1], remote->buffer, remote->size);
diff --git a/builtin/clone.c b/builtin/clone.c
index e76c38e4e81..49edb4a2aaa 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -686,8 +686,7 @@ static int checkout(int submodule_progress)
 	opts.update = 1;
 	opts.merge = 1;
 	opts.clone = 1;
-	/* FIXME: Default should be to remove ignored files */
-	opts.preserve_ignored = 1;
+	opts.preserve_ignored = 0;
 	opts.fn = oneway_merge;
 	opts.verbose_update = (option_verbosity >= 0);
 	opts.src_index = &the_index;
diff --git a/builtin/merge.c b/builtin/merge.c
index 55aac869e5a..7f990e36651 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -680,8 +680,7 @@ static int read_tree_trivial(struct object_id *common, struct object_id *head,
 	opts.verbose_update = 1;
 	opts.trivial_merges_only = 1;
 	opts.merge = 1;
-	/* FIXME: Default should be to remove ignored files */
-	opts.preserve_ignored = 1;
+	opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */
 	trees[nr_trees] = parse_tree_indirect(common);
 	if (!trees[nr_trees++])
 		return -1;
diff --git a/builtin/reset.c b/builtin/reset.c
index 73477239146..9d1391335a1 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -66,8 +66,7 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t
 	case KEEP:
 	case MERGE:
 		opts.update = 1;
-		/* FIXME: Default should be to remove ignored files */
-		opts.preserve_ignored = 1;
+		opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */
 		break;
 	case HARD:
 		opts.update = 1;
diff --git a/builtin/stash.c b/builtin/stash.c
index 78492013529..92ad3241270 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -258,8 +258,7 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)
 	opts.reset = reset;
 	opts.update = update;
 	if (update && !reset)
-		/* FIXME: Default should be to remove ignored files */
-		opts.preserve_ignored = 1;
+		opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */
 	opts.fn = oneway_merge;
 
 	if (unpack_trees(nr_trees, t, &opts)) {
diff --git a/merge-ort.c b/merge-ort.c
index e5620bda212..5c443b2b00d 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -4044,7 +4044,7 @@ static int checkout(struct merge_options *opt,
 	unpack_opts.quiet = 0; /* FIXME: sequencer might want quiet? */
 	unpack_opts.verbose_update = (opt->verbosity > 2);
 	unpack_opts.fn = twoway_merge;
-	unpack_opts.preserve_ignored = 0; /* FIXME: !opts->overwrite_ignore*/
+	unpack_opts.preserve_ignored = 0; /* FIXME: !opts->overwrite_ignore */
 	parse_tree(prev);
 	init_tree_desc(&trees[0], prev->buffer, prev->size);
 	parse_tree(next);
diff --git a/reset.c b/reset.c
index cd344f47f13..5f69311b9f4 100644
--- a/reset.c
+++ b/reset.c
@@ -56,8 +56,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,
 	unpack_tree_opts.fn = reset_hard ? oneway_merge : twoway_merge;
 	unpack_tree_opts.update = 1;
 	unpack_tree_opts.merge = 1;
-	/* FIXME: Default should be to remove ignored files */
-	unpack_tree_opts.preserve_ignored = 1;
+	unpack_tree_opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */
 	init_checkout_metadata(&unpack_tree_opts.meta, switch_to_branch, oid, NULL);
 	if (!detach_head)
 		unpack_tree_opts.reset = 1;
diff --git a/sequencer.c b/sequencer.c
index 669ea15944c..05395db9e01 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3698,8 +3698,7 @@ static int do_reset(struct repository *r,
 	unpack_tree_opts.fn = oneway_merge;
 	unpack_tree_opts.merge = 1;
 	unpack_tree_opts.update = 1;
-	/* FIXME: Default should be to remove ignored files */
-	unpack_tree_opts.preserve_ignored = 1;
+	unpack_tree_opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */
 	init_checkout_metadata(&unpack_tree_opts.meta, name, &oid, NULL);
 
 	if (repo_read_index_unmerged(r)) {
diff --git a/t/t7112-reset-submodule.sh b/t/t7112-reset-submodule.sh
index 19830d90365..a3e2413bc33 100755
--- a/t/t7112-reset-submodule.sh
+++ b/t/t7112-reset-submodule.sh
@@ -6,7 +6,6 @@ test_description='reset can handle submodules'
 . "$TEST_DIRECTORY"/lib-submodule-update.sh
 
 KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS=1
-KNOWN_FAILURE_SUBMODULE_OVERWRITE_IGNORED_UNTRACKED=1
 
 test_submodule_switch_recursing_with_args "reset --keep"
 
-- 
2.33.0.1404.g83021034c5d

From: Elijah Newren <[email protected]>

Traditionally, unpack_trees_options->reset was used to signal that it
was okay to delete any untracked files in the way.  This was used by
`git read-tree --reset`, but then started appearing in other places as
well.  However, many of the other uses should not be deleting untracked
files in the way.  Change this value to an enum so that a value of 1
(i.e. "true") can be split into two:
   UNPACK_RESET_PROTECT_UNTRACKED,
   UNPACK_RESET_OVERWRITE_UNTRACKED
In order to catch accidental misuses (i.e. where folks call it the way
they traditionally used to), define the special enum value of
   UNPACK_RESET_INVALID = 1
which will trigger a BUG().

Modify existing callers so that
   read-tree --reset
   reset --hard
   checkout --force
continue using the UNPACK_RESET_OVERWRITE_UNTRACKED logic, while other
callers, including
   am
   checkout without --force
   stash  (though currently dead code; reset always had a value of 0)
   numerous callers from rebase/sequencer to reset_head()
will use the new UNPACK_RESET_PROTECT_UNTRACKED value.

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

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

Signed-off-by: Elijah Newren <[email protected]>
Signed-off-by: Ævar Arnfjörð Bjarmason <[email protected]>
---
 builtin/am.c                     |  5 ++---
 builtin/checkout.c               |  5 +++--
 builtin/read-tree.c              |  3 +++
 builtin/reset.c                  |  9 +++++++--
 builtin/stash.c                  |  4 ++--
 reset.c                          |  2 +-
 t/t2500-untracked-overwriting.sh |  6 +++---
 unpack-trees.c                   | 10 +++++++++-
 unpack-trees.h                   | 11 +++++++++--
 9 files changed, 39 insertions(+), 16 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 8cb7e72e6c5..dfe2bf207e6 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1917,9 +1917,8 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)
 	opts.dst_index = &the_index;
 	opts.update = 1;
 	opts.merge = 1;
-	opts.reset = reset;
-	if (!reset)
-		opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */
+	opts.reset = reset ? UNPACK_RESET_PROTECT_UNTRACKED : 0;
+	opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */
 	opts.fn = twoway_merge;
 	init_tree_desc(&t[0], head->buffer, head->size);
 	init_tree_desc(&t[1], remote->buffer, remote->size);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 0c5187025c5..ee3e450537f 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -646,9 +646,10 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
 	opts.head_idx = -1;
 	opts.update = worktree;
 	opts.skip_unmerged = !worktree;
-	opts.reset = 1;
+	opts.reset = o->force ? UNPACK_RESET_OVERWRITE_UNTRACKED :
+				UNPACK_RESET_PROTECT_UNTRACKED;
+	opts.preserve_ignored = (!o->force && !o->overwrite_ignore);
 	opts.merge = 1;
-	opts.preserve_ignored = 0;
 	opts.fn = oneway_merge;
 	opts.verbose_update = o->show_progress;
 	opts.src_index = &the_index;
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index 7f3c987b126..5e10caeff5a 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -166,6 +166,9 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
 	if (1 < opts.merge + opts.reset + prefix_set)
 		die("Which one? -m, --reset, or --prefix?");
 
+	if (opts.reset)
+		opts.reset = UNPACK_RESET_OVERWRITE_UNTRACKED;
+
 	/*
 	 * NEEDSWORK
 	 *
diff --git a/builtin/reset.c b/builtin/reset.c
index 9d1391335a1..00d2de392a8 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -70,9 +70,14 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t
 		break;
 	case HARD:
 		opts.update = 1;
-		/* fallthrough */
+		opts.reset = UNPACK_RESET_OVERWRITE_UNTRACKED;
+		break;
+	case MIXED:
+		opts.reset = UNPACK_RESET_PROTECT_UNTRACKED;
+		/* but opts.update=0, so working tree not updated */
+		break;
 	default:
-		opts.reset = 1;
+		BUG("invalid reset_type passed to reset_index");
 	}
 
 	read_cache_unmerged();
diff --git a/builtin/stash.c b/builtin/stash.c
index 92ad3241270..061237cf9a4 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -255,9 +255,9 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)
 	opts.src_index = &the_index;
 	opts.dst_index = &the_index;
 	opts.merge = 1;
-	opts.reset = reset;
+	opts.reset = reset ? UNPACK_RESET_PROTECT_UNTRACKED : 0;
 	opts.update = update;
-	if (update && !reset)
+	if (update)
 		opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */
 	opts.fn = oneway_merge;
 
diff --git a/reset.c b/reset.c
index 5f69311b9f4..5788b1926f3 100644
--- a/reset.c
+++ b/reset.c
@@ -59,7 +59,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,
 	unpack_tree_opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */
 	init_checkout_metadata(&unpack_tree_opts.meta, switch_to_branch, oid, NULL);
 	if (!detach_head)
-		unpack_tree_opts.reset = 1;
+		unpack_tree_opts.reset = UNPACK_RESET_PROTECT_UNTRACKED;
 
 	if (repo_read_index_unmerged(r) < 0) {
 		ret = error(_("could not read index"));
diff --git a/t/t2500-untracked-overwriting.sh b/t/t2500-untracked-overwriting.sh
index 2412d121ea8..18604360df8 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 512011cfa42..37f769030ab 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1699,6 +1699,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 	struct pattern_list pl;
 	int free_pattern_list = 0;
 
+	if (o->reset == UNPACK_RESET_INVALID)
+		BUG("o->reset had a value of 1; should be UNPACK_TREES_*_UNTRACKED");
+
 	if (len > MAX_UNPACK_TREES)
 		die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES);
 
@@ -1711,6 +1714,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 		ensure_full_index(o->dst_index);
 	}
 
+	if (o->reset == UNPACK_RESET_OVERWRITE_UNTRACKED &&
+	    o->preserve_ignored)
+		BUG("UNPACK_RESET_OVERWRITE_UNTRACKED incompatible with preserved ignored files");
+
 	if (!o->preserve_ignored) {
 		o->private_dir.flags |= DIR_SHOW_IGNORED;
 		setup_standard_excludes(&o->private_dir);
@@ -2227,7 +2234,8 @@ static int verify_absent_1(const struct cache_entry *ce,
 	int len;
 	struct stat st;
 
-	if (o->index_only || o->reset || !o->update)
+	if (o->index_only || !o->update ||
+	    o->reset == UNPACK_RESET_OVERWRITE_UNTRACKED)
 		return 0;
 
 	len = check_leading_path(ce->name, ce_namelen(ce), 0);
diff --git a/unpack-trees.h b/unpack-trees.h
index 2eb633bf771..1582599606a 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -48,9 +48,15 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
  */
 void unpack_trees_options_release(struct unpack_trees_options *opts);
 
+enum unpack_trees_reset_type {
+	UNPACK_RESET_NONE = 0,    /* traditional "false" value; still valid */
+	UNPACK_RESET_INVALID = 1, /* "true" no longer valid; use below values */
+	UNPACK_RESET_PROTECT_UNTRACKED,
+	UNPACK_RESET_OVERWRITE_UNTRACKED
+};
+
 struct unpack_trees_options {
-	unsigned int reset,
-		     merge,
+	unsigned int merge,
 		     update,
 		     preserve_ignored,
 		     clone,
@@ -68,6 +74,7 @@ struct unpack_trees_options {
 		     exiting_early,
 		     show_all_errors,
 		     dry_run;
+	enum unpack_trees_reset_type reset;
 	const char *prefix;
 	int cache_bottom;
 	struct dir_struct private_dir; /* for internal use only */
-- 
2.33.0.1404.g83021034c5d

From: Elijah Newren <[email protected]>

Signed-off-by: Elijah Newren <[email protected]>
Signed-off-by: Ævar Arnfjörð Bjarmason <[email protected]>
---
 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 18604360df8..5ec66058cfc 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 37f769030ab..8408a8fcfff 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -2173,9 +2173,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;
@@ -2209,6 +2215,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
@@ -2229,6 +2239,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;
@@ -2255,7 +2266,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;
@@ -2270,7 +2282,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);
 	}
 }
 
@@ -2280,14 +2292,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,
@@ -2361,6 +2382,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);
 	}
 
-- 
2.33.0.1404.g83021034c5d

From: Elijah Newren <[email protected]>

Signed-off-by: Elijah Newren <[email protected]>
Signed-off-by: Ævar Arnfjörð Bjarmason <[email protected]>
---
 t/t2500-untracked-overwriting.sh | 2 +-
 unpack-trees.c                   | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/t/t2500-untracked-overwriting.sh b/t/t2500-untracked-overwriting.sh
index 5ec66058cfc..5c0bf4d21fc 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 8408a8fcfff..703e7953d62 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -2405,7 +2405,10 @@ static int deleted_entry(const struct cache_entry *ce,
 		if (verify_absent(ce, ERROR_WOULD_LOSE_UNTRACKED_REMOVED, o))
 			return -1;
 		return 0;
+	} else if (verify_absent_if_directory(ce, ERROR_WOULD_LOSE_UNTRACKED_REMOVED, o)) {
+		return -1;
 	}
+
 	if (!(old->ce_flags & CE_CONFLICTED) && verify_uptodate(old, o))
 		return -1;
 	add_entry(o, ce, CE_REMOVE, 0);
-- 
2.33.0.1404.g83021034c5d

From: Elijah Newren <[email protected]>

In the last few commits we focused on code in unpack-trees.c that
mistakenly removed untracked files or directories.  There may be more of
those, but in this commit we change our focus: callers of toplevel
commands that are expected to remove untracked files or directories.

As noted previously, we have toplevel commands that are expected to
delete untracked files such as 'read-tree --reset', 'reset --hard', and
'checkout --force'.  However, that does not mean that other highlevel
commands that happen to call these other commands thought about or
conveyed to users the possibility that untracked files could be removed.
Audit the code for such callsites, and add comments near existing
callsites to mention whether these are safe or not.

My auditing is somewhat incomplete, though; it skipped several cases:
  * git-rebase--preserve-merges.sh: is in the process of being
    deprecated/removed, so I won't leave a note that there are
    likely more bugs in that script.
  * contrib/git-new-workdir: why is the -f flag being used in a new
    empty directory??  It shouldn't hurt, but it seems useless.
  * git-p4.py: Don't see why -f is needed for a new dir (maybe it's
    not and is just superfluous), but I'm not at all familiar with
    the p4 stuff
  * git-archimport.perl: Don't care; arch is long since dead
  * git-cvs*.perl: Don't care; cvs is long since dead

Also, the reset --hard in builtin/worktree.c looks safe, due to only
running in an empty directory.

Signed-off-by: Elijah Newren <[email protected]>
Signed-off-by: Ævar Arnfjörð Bjarmason <[email protected]>
---
 builtin/stash.c             | 1 +
 builtin/submodule--helper.c | 4 ++++
 contrib/rerere-train.sh     | 2 +-
 submodule.c                 | 1 +
 4 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index 061237cf9a4..fabfb63632e 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1525,6 +1525,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 57f09925157..e40e4b6aacc 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -3091,6 +3091,10 @@ static int add_submodule(const struct add_data *add_data)
 		prepare_submodule_repo_env(&cp.env_array);
 		cp.git_cmd = 1;
 		cp.dir = add_data->sm_path;
+		/*
+		 * NOTE: we only get here if add_data->force is true, so
+		 * passing --force to checkout is reasonable.
+		 */
 		strvec_pushl(&cp.args, "checkout", "-f", "-q", NULL);
 
 		if (add_data->branch) {
diff --git a/contrib/rerere-train.sh b/contrib/rerere-train.sh
index 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 03cf36707ae..9f3d9b7ee73 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1910,6 +1910,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());
-- 
2.33.0.1404.g83021034c5d

From: Elijah Newren <[email protected]>

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

Signed-off-by: Elijah Newren <[email protected]>
Signed-off-by: Ævar Arnfjörð Bjarmason <[email protected]>
---
 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 0222a27c5af..8c3aceb8324 100644
--- a/Documentation/git-read-tree.txt
+++ b/Documentation/git-read-tree.txt
@@ -38,8 +38,9 @@ OPTIONS
 
 --reset::
 	Same as -m, except that unmerged entries are discarded instead
-	of failing. When used with `-u`, updates leading to loss of
-	working tree changes will not abort the operation.
+	of failing.  When used with `-u`, updates leading to loss of
+	working tree changes or untracked files or directories will not
+	abort the operation.
 
 -u::
 	After a successful merge, update the files in the work
diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index 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
-- 
2.33.0.1404.g83021034c5d

On Sun, Oct 3, 2021 at 6:12 PM Ævar Arnfjörð Bjarmason <[email protected]> wrote:
>
> This is an RFC proposed v4 of Elijah's en/removing-untracked-fixes
> series[1] based on top of my memory leak fixes in the "unpack-trees" &
> "dir" APIs[2].
>
> As noted in [2] Elijah and I have been having a back & forth about the
> approach his series takes to fixing memory leaks in those APIs. I
> think submitting working code is more productive than continuing that
> point-by-point discussion, so here we are.
>
> I've avoided making any changes to this series except those narrowly
> required to rebase it on top of mine, and to those parts of Elijah's
> commit messages that became outdated as a result. In particular
> 3/10[3]'s is significantly changed, as much of its commit message
> dicusses complexities that have gone away due to my preceding
> series[2].
>
> The "make dir an internal-only struct" has been replaced by a commit
> that renames that struct member from "dir" to "private_dir". I think
> even that is unnecessary as argued in [4], but I think the judgement
> that something must be done to address that is Elijah's design
> decision, so I did my best to retain it.
>
> I did drop the dynamic allocation & it being a pointer, since with my
> preceding [2] and subsequent unsubmitted memory leak fixes I've got on
> top having it be embedded in "struct unpack_trees_options" makes
> things easier to manage.
>
> Havingn read through all this code quite thoroughly at this point I do
> have other comments on it, but I'll reserve those until we've found
> out what direction we're going forward with vis-a-vis what this will
> be based on top of.
>
> I'm (obviously) hoping for an answer of either on top of my series[2],
> or alternatively that Elijah's series can stick to introducing the
> "preserve_ignored" flag, but not change how the memory
> management/name/type of the embedded "dir" happens (and we could thus
> proceed in parallel).

???

This really bothers me.  I'm not quite sure how to put this into
words, so let me just try my best.  Let me start out by saying that I
think you often provide good feedback and ideas.  Sure, I sometimes
don't agree with some of the feedback or ideas, but overall your
feedback and contributions are definitely valuable.  I also think your
other series you rebased this on has some good ideas and some good
bugfixes.  There is something that seems off here, though.

In this particular case, to start with, Junio already said let's take
v3 as-is[1].  So your series should be rebased on mine, not
vice-versa.

Further while your other series that you are basing this on has some
memory leak fixes; to me, it mostly looks like refactorings for
stylistic code changes. Even though some of those stylistic changes
are good, making a series such as mine that includes bugfixes (to a
user reported bug no less), after multiple rounds and most reviewers
are fine with it, suddenly depend on a new big and unrelated treewide
stylistic refactoring series feels very off to me.  But that doesn't
quite fully explain my misgivings either; there's a bit more:

  * Junio has referred to several of your series as "Meh" and "code
churn".  That makes me think we'd have a higher than normal chance of
a user-reported bug ending up blocked on unrelated stylistic changes.
(Two of them actually, since I have another series depending on this
one that I've waited to submit until this merges to next.)
  * Your stylistic refactorings also manage to confuse the code in
merge-recursive.c, overall making the code potentially much harder to
understand[2][3].  And you open a foot-gun in
clear_unpack_trees_porcelain[3].
  * At least half the series of yours I've reviewed have had
significant bugs[4][5][6] (in addition to [2] and [3]).  This would be
fine if it was complex code that had bugs we were fixing, or if we
were adding new features, but:
  * You submit a huge volume of patches, with a very
disproportionately high ratio of stylistic refactorings rather than
bugfixes and new features.  (This is by no means bad on its own, it's
the combination of this with other factors.)
  * You misrepresent my changes in multiple ways, including ways I had
pointed out corrections for in our previous discussions (including
some of which you acknowledged and agreed with), and you do so even
after you have rebased my patches and added your signed-off-by to them
suggesting you ought to be familiar with them[7].

So, I guess trying to distill what bugs me, I'd say: it seems to me
that you have ignored what Junio said about taking my series, and then
you rebased my series on top of unrelated stylistic churn, with that
churn containing three issues that trigger ongoing misgivings I have
about the care being put behind these refactorings, especially
considering their value compared to the features and bugfixes we are
getting, and you seem to fail to try to understand my changes and
misrepresent them in the process.  I hope I'm not overreacting but
something feels wrong to me here.


A big thumbs down on this reroll.

[1] https://lore.kernel.org/git/[email protected]/
[2] https://lore.kernel.org/git/CABPp-BFYxWXZQXvDSrM1Y1ZaQ1d2TENQDvx1cyawvrDO1OvExA@mail.gmail.com/
[3] https://lore.kernel.org/git/CABPp-BH4ubjJ98Nvgp2iyKxmU9X+ypw4m1o=iL9Z4vSNZ-QTDw@mail.gmail.com/
[4] https://lore.kernel.org/git/CABPp-BGE+e1er6qFuG90j9+eVG34O8TN=imX=jtcb+jbQjN1QQ@mail.gmail.com/
[5] https://lore.kernel.org/git/CABPp-BEPkukGz32rrro1hzMvSQzX4v7U17CAcV-G2NS6v0u55g@mail.gmail.com/
[6] https://lore.kernel.org/git/[email protected]/  [Note:
problem was flagged by j6t; I was about to flag the same problem when
I noticed he had already done so.]
[7] https://lore.kernel.org/git/CABPp-BEr28xzbpEZc5dq-RVDupXy+h-+PH6CoANF4e0kmxqf0Q@mail.gmail.com/

On Mon, Oct 04 2021, Elijah Newren wrote:

> On Sun, Oct 3, 2021 at 6:12 PM Ævar Arnfjörð Bjarmason <[email protected]> wrote:
>>
>> This is an RFC proposed v4 of Elijah's en/removing-untracked-fixes
>> series[1] based on top of my memory leak fixes in the "unpack-trees" &
>> "dir" APIs[2].
>>
>> As noted in [2] Elijah and I have been having a back & forth about the
>> approach his series takes to fixing memory leaks in those APIs. I
>> think submitting working code is more productive than continuing that
>> point-by-point discussion, so here we are.
>>
>> I've avoided making any changes to this series except those narrowly
>> required to rebase it on top of mine, and to those parts of Elijah's
>> commit messages that became outdated as a result. In particular
>> 3/10[3]'s is significantly changed, as much of its commit message
>> dicusses complexities that have gone away due to my preceding
>> series[2].
>>
>> The "make dir an internal-only struct" has been replaced by a commit
>> that renames that struct member from "dir" to "private_dir". I think
>> even that is unnecessary as argued in [4], but I think the judgement
>> that something must be done to address that is Elijah's design
>> decision, so I did my best to retain it.
>>
>> I did drop the dynamic allocation & it being a pointer, since with my
>> preceding [2] and subsequent unsubmitted memory leak fixes I've got on
>> top having it be embedded in "struct unpack_trees_options" makes
>> things easier to manage.
>>
>> Havingn read through all this code quite thoroughly at this point I do
>> have other comments on it, but I'll reserve those until we've found
>> out what direction we're going forward with vis-a-vis what this will
>> be based on top of.
>>
>> I'm (obviously) hoping for an answer of either on top of my series[2],
>> or alternatively that Elijah's series can stick to introducing the
>> "preserve_ignored" flag, but not change how the memory
>> management/name/type of the embedded "dir" happens (and we could thus
>> proceed in parallel).
>
> ???
>
> This really bothers me.  I'm not quite sure how to put this into
> words, so let me just try my best.  Let me start out by saying that I
> think you often provide good feedback and ideas.  Sure, I sometimes
> don't agree with some of the feedback or ideas, but overall your
> feedback and contributions are definitely valuable.  I also think your
> other series you rebased this on has some good ideas and some good
> bugfixes.  There is something that seems off here, though.

Just for Junio / anyone else following along: let's drop this RFC & the
relateded/proposed "unpack-trees & dir APIs: fix memory
leaks". Point-by-point commentary below (probably not needed/interesting
for those just interested in the state of those two serieses).

> In this particular case, to start with, Junio already said let's take
> v3 as-is[1].  So your series should be rebased on mine, not
> vice-versa.

I understand your annoyance at that, I wouldn't have submitted this if
I'd seen that before, I somehow managed to miss that mail in my mail
queue. I believed the status was at "Will merge to 'next'?" upthread of
[1].

We've then been having an extended back & forth about how to manage
"private" data/structs/leak patterns starting at
https://lore.kernel.org/git/[email protected]/.

At least some of which has been confused by my having quoted a working
but out of context diff from what I ended up submitting as
https://lore.kernel.org/git/[email protected]/

So, sorry about stepping on your toes. I figured having a discussion
with working patches would be more productive, and that it would help
focus on the important changes in your series.

E.g. your 2nd and 3rd patch setup a "dir_clear()" that your 4th
consolidates, which as shown in this series are intermediate steps that
can be skipped. So perhaps there's some added churn, but also reduced
churn in your resulting series on top...

> Further while your other series that you are basing this on has some
> memory leak fixes; to me, it mostly looks like refactorings for
> stylistic code changes. [...]

Are you referring to the s/memset/UNPACK_TREES_OPTIONS_INIT/ bulk change
at the start?

I agree that it's not strictly necessary, but it's pretty much the same
as your earlier eceba532141 (dir: fix problematic API to avoid memory
leaks, 2020-08-18), and makes e.g. a later change you seemed to like
possible:
https://lore.kernel.org/git/CABPp-BFpyyJ-e8p5fbmCvyaEsfUow=RP45Nw0ckiwNEvVC4zrg@mail.gmail.com/

> Even though some of those stylistic changes
> are good, making a series such as mine that includes bugfixes (to a
> user reported bug no less), after multiple rounds and most reviewers
> are fine with it, suddenly depend on a new big and unrelated treewide
> stylistic refactoring series feels very off to me.  But that doesn't
> quite fully explain my misgivings either; there's a bit more:

[...]

>   * Junio has referred to several of your series as "Meh" and "code
> churn".  That makes me think we'd have a higher than normal chance of
> a user-reported bug ending up blocked on unrelated stylistic changes.
> (Two of them actually, since I have another series depending on this
> one that I've waited to submit until this merges to next.)

I'll stay out of this area for a while. Sorry about that.

> [...]
>   * You misrepresent my changes in multiple ways, including ways I had
> pointed out corrections for in our previous discussions (including
> some of which you acknowledged and agreed with), and you do so even
> after you have rebased my patches and added your signed-off-by to them
> suggesting you ought to be familiar with them[7].

You're absolutely right about that, and the comment starting at "*Sigh*"
in your linked [7] is entirely accurate. I'd like to apologize for that.

If I was in your shoes I'd be *very* annoyed at that chain from [7] to
the upthread cover-letter here making that false claim again.

For what it's worth I do know that your patches aren't allocating the
"struct dir_struct" on the heap, having rebased them etc. But through
some combination of a brainfart and it being late when I wrote that CL
last night I falsely claimed that they did, PBCAK.

What I *meant* to say was some summary that the your series's end state
has a pointer to a dir struct that's dynamically set up v.s. mine of
just initializing it via the macro from the start.

> So, I guess trying to distill what bugs me, I'd say: it seems to me
> that you have ignored what Junio said about taking my series, and then
> you rebased my series on top of unrelated stylistic churn, with that
> churn containing three issues that trigger ongoing misgivings I have
> about the care being put behind these refactorings, especially
> considering their value compared to the features and bugfixes we are
> getting, and you seem to fail to try to understand my changes and
> misrepresent them in the process.  I hope I'm not overreacting but
> something feels wrong to me here.

I don't think you're overreacting, and sorry again. Hopefully it helps
somewhat that I for the "ignoring Junio [and charging ahead with this]"
and the 2nd false claim about about heap allocation I was (believe it or
not) just honestly mistaken instead of trying to get on your nerves.

As some of my early feedback on the whole topic of gitignore
related-shredding/precious etc. should hopefully indicate I'm really
happy that you've picked up this topic.

I thought this would help it along, but that's clearly not the
case. Sorry again.

Elijah Newren <[email protected]> writes:

> So, I guess trying to distill what bugs me, I'd say: it seems to me
> that you have ignored what Junio said about taking my series, and then
> you rebased my series on top of unrelated stylistic churn, with that
> churn containing three issues that trigger ongoing misgivings I have
> about the care being put behind these refactorings, especially
> considering their value compared to the features and bugfixes we are
> getting, and you seem to fail to try to understand my changes and
> misrepresent them in the process.  I hope I'm not overreacting but
> something feels wrong to me here.

Just one thing.  It is fairly easy to rebase and merge and generally
muck with somebody else's changes without fully understanding it.  I
do that all the time ;-)

I would prefer to see us assume misunderstanding by incompetence,
rather than misrepresentation by malice to further one's own agenda.

Hi Ævar,

On Mon, Oct 4, 2021 at 10:56 AM Ævar Arnfjörð Bjarmason
<[email protected]> wrote:
>
> On Mon, Oct 04 2021, Elijah Newren wrote:
>
> > So, I guess trying to distill what bugs me, I'd say: [...] I hope I'm not overreacting but
> > something feels wrong to me here.
>
> I don't think you're overreacting, and sorry again. Hopefully it helps
> somewhat that I for the "ignoring Junio [and charging ahead with this]"
> and the 2nd false claim about about heap allocation I was (believe it or
> not) just honestly mistaken instead of trying to get on your nerves.

I really wish I could take back that email.  And yes, I was totally
overreacting (in part due to unrelated non-git stuff going on
recently).  I should have waited a day, and then I'd probably realize
it.  I owe you an apology, Ævar.  I'm very sorry.

In regards to the worst part of my email:

"""
  * At least half the series of yours I've reviewed have had
significant bugs[4][5][6] (in addition to [2] and [3]).  This would be
fine if it was complex code that had bugs we were fixing, or if we
were adding new features, but:
"""

I'm no stranger to introducing pretty bad bugs either, some caught in
review (one case of repository corruption caught in a review just last
week!), some making it into releases:

https://lore.kernel.org/git/[email protected]/
https://lore.kernel.org/git/DM6PR00MB06829EC5B85E0C5AC595004E894E9@DM6PR00MB0682.namprd00.prod.outlook.com/
https://lore.kernel.org/git/CABPp-BFWfwkYAPyySjWOMZ02_+YLf=TJ_aVMaHaizJWAsCL67g@mail.gmail.com/
https://lore.kernel.org/git/CABPp-BENB=mqfFU4FGb2OS9VDV=9VdT71HhFLZwtyxD8MpdTMQ@mail.gmail.com/
https://lore.kernel.org/git/CABPp-BEBKyE2NVfREov6k5qML5jryLjtzw=Y21EA=fHXA0PO5A@mail.gmail.com/
https://lore.kernel.org/git/CABPp-BF8eokQTVwgo80ffq3tn=NA=mPf7oymce5P4itDyZBiMg@mail.gmail.com/
https://lore.kernel.org/git/[email protected]/
https://lore.kernel.org/git/CABPp-BHL4P0RxQ6OAuDSev9BXVM0uKTYD3M4JGTQvSwcBv4K0Q@mail.gmail.com/

and I could easily find more, it was pretty easy to come up with that
list of bugs that still make me shudder.  Touching difficult code, or
code one isn't quite as familiar with is more prone to bigger
problems.  That doesn't mean we should avoid working on those areas,
but I'm afraid my email probably served only to scare people away from
doing so.  Treewide refactoring is, by its nature, likely to take one
into many areas of the codebase, and thus it'd be natural to expect
problems that hopefully get caught in review.  The fact that such
treewide refactorings did have problems, was by itself not my point
and not what I'd consider unusual.  I was attempting to make a more
nuanced point about lots of treewide refactorings in a short time
period coupled with lack of understanding of motivation for some of
those refactorings all combined with additional things on top, but
utterly failed at anything more than coming across as a jerk.  I'm
sorry.

Thanks for responding to my email so diplomatically; I'm super
impressed with that.  And just to be clear, I respect your
contributions and hold them in high esteem (prove, i18n, pcre2, faster
send-email to name just a few).  I think that message was
unfortunately completely lost in my email, which is plenty of reason
that I just shouldn't have sent it.