patch, 6 partst2500: add various tests for nuking untracked files

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

>
> 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/" ?

> From: Elijah Newren <newren@gmail.com>
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  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;
>  	}

>
>
> On Sat, Sep 18 2021, Elijah Newren via GitGitGadget wrote:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > ---
> >  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" ?

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

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

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

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

> From: Elijah Newren <newren@gmail.com>
> 
> 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.

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

> [...]
> 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);
> +	}

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

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

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

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

>
> On 19/09/2021 00:15, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
> >
> > 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.

>
> > [...]
> > 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?

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

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

> On Mon, Sep 20, 2021 at 3:19 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> On 19/09/2021 00:15, Elijah Newren via GitGitGadget wrote:
>>> From: Elijah Newren <newren@gmail.com>
>>>
>>> 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?

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

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

>>> [...]
>>> 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.
> 
>>> 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.
>>
>>> 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'
>>
>>> 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.
> 
>>> 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 20/09/2021 17:05, Elijah Newren wrote:
> > On Mon, Sep 20, 2021 at 3:19 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> >>
> >> On 19/09/2021 00:15, Elijah Newren via GitGitGadget wrote:
> >>> From: Elijah Newren <newren@gmail.com>
> >>>
> >>> 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.
>
> >>> 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.
> >
> >>>      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'
>
> >>> 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.

> From: Elijah Newren <newren@gmail.com>
>
> 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 <newren@gmail.com>
> ---
>  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

> truly minor nit.
> On 19/09/2021 00:15, Elijah Newren via GitGitGadget wrote:
>> From: Elijah Newren <newren@gmail.com>
>>
>> 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 <newren@gmail.com>
>> ---
>>  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?
>> +	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.

>> +	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 /.  /. /  ?
>> +	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
>>

>
> On 19/09/2021 11:52, Philip Oakley wrote:
> > truly minor nit.
> > On 19/09/2021 00:15, Elijah Newren via GitGitGadget wrote:
> >> From: Elijah Newren <newren@gmail.com>
> >>
> >> 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 <newren@gmail.com>
> >> ---
> >>  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?

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

>
> From: Elijah Newren <newren@gmail.com>
>
> 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

>
> On Sun, 19 Sept 2021 at 00:15, Elijah Newren via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > From: Elijah Newren <newren@gmail.com>
> >
> > 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
>
> Assuming you're talking about this code in git-p4.py:
>
>             print("Synchronizing p4 checkout...")
>             if new_client_dir:
>                 # old one was destroyed, and maybe nobody told p4
>                 p4_sync("...", "-f")
>             else:
>                 p4_sync("...")

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

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index b5d477919a7..52826e0d145 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -641,23 +641,37 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
>  {
>  	struct unpack_trees_options opts;
>  	struct tree_desc tree_desc;
> +	int unpack_trees_ret;
>  
>  	memset(&opts, 0, sizeof(opts));
>  	opts.head_idx = -1;
>  	opts.update = worktree;
>  	opts.skip_unmerged = !worktree;
> -	opts.reset = 1;
> +	opts.reset = o->force ? UNPACK_RESET_OVERWRITE_UNTRACKED :
> +				UNPACK_RESET_PROTECT_UNTRACKED;
>  	opts.merge = 1;
>  	opts.fn = oneway_merge;
>  	opts.verbose_update = o->show_progress;
>  	opts.src_index = &the_index;
>  	opts.dst_index = &the_index;
> +	if (o->overwrite_ignore) {
> +		opts.dir = xcalloc(1, sizeof(*opts.dir));
> +		opts.dir->flags |= DIR_SHOW_IGNORED;
> +		setup_standard_excludes(opts.dir);
> +	}

>  	init_checkout_metadata(&opts.meta, info->refname,
>  			       info->commit ? &info->commit->object.oid : null_oid(),
>  			       NULL);
>  	parse_tree(tree);
>  	init_tree_desc(&tree_desc, tree->buffer, tree->size);
> -	switch (unpack_trees(1, &tree_desc, &opts)) {
> +	unpack_trees_ret = unpack_trees(1, &tree_desc, &opts);
> +
> +	if (o->overwrite_ignore) {
> +		dir_clear(opts.dir);
> +		FREE_AND_NULL(opts.dir);

> +	}
> +
> +	switch (unpack_trees_ret) {
>  	case -2:
>  		*writeout_error = 1;
>  		/*
> diff --git a/builtin/read-tree.c b/builtin/read-tree.c
> index 485e7b04794..740fc0335af 100644
> --- a/builtin/read-tree.c
> +++ b/builtin/read-tree.c
> @@ -174,6 +174,9 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
>  	if (1 < opts.merge + opts.reset + prefix_set)
>  		die("Which one? -m, --reset, or --prefix?");
>  
> +	if (opts.reset)
> +		opts.reset = UNPACK_RESET_OVERWRITE_UNTRACKED;
> +

> diff --git a/builtin/reset.c b/builtin/reset.c
> index 43e855cb887..a12ee986e9f 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,20 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t
>  		break;
>  	case HARD:
>  		opts.update = 1;
> -		/* fallthrough */
> +		opts.reset = UNPACK_RESET_OVERWRITE_UNTRACKED;
> +		break;
> +	case MIXED:
> +		opts.reset = UNPACK_RESET_PROTECT_UNTRACKED;
> +		/* but opts.update=0, so working tree not updated */
> +		break;
>  	default:
> -		opts.reset = 1;
> +		BUG("invalid reset_type passed to reset_index");
> +	}
> +	if (opts.reset == UNPACK_RESET_PROTECT_UNTRACKED) {

> @@ -104,6 +116,10 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t
>  	ret = 0;
>  
>  out:
> +	if (opts.reset == UNPACK_RESET_PROTECT_UNTRACKED) {
> +		dir_clear(opts.dir);
> +		FREE_AND_NULL(opts.dir);

> diff --git a/builtin/stash.c b/builtin/stash.c
> index 8f42360ca91..563f590afbd 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -237,6 +237,7 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)
>  	struct tree_desc t[MAX_UNPACK_TREES];
>  	struct tree *tree;
>  	struct lock_file lock_file = LOCK_INIT;
> +	int unpack_trees_ret;
>  
>  	read_cache_preload(NULL);
>  	if (refresh_cache(REFRESH_QUIET))
> @@ -256,11 +257,23 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)
>  	opts.src_index = &the_index;
>  	opts.dst_index = &the_index;
>  	opts.merge = 1;
> -	opts.reset = reset;
> +	opts.reset = reset ? UNPACK_RESET_PROTECT_UNTRACKED : 0;
> +	if (opts.reset) {
> +		opts.dir = xcalloc(1, sizeof(*opts.dir));
> +		opts.dir->flags |= DIR_SHOW_IGNORED;
> +		setup_standard_excludes(opts.dir);
> +	}
>  	opts.update = update;
>  	opts.fn = oneway_merge;
>  
> -	if (unpack_trees(nr_trees, t, &opts))
> +	unpack_trees_ret = unpack_trees(nr_trees, t, &opts);
> +
> +	if (opts.reset) {
> +		dir_clear(opts.dir);
> +		FREE_AND_NULL(opts.dir);

> +	}
> +
> +	if (unpack_trees_ret)
>  		return -1;
>  
>  	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
> diff --git a/reset.c b/reset.c
> index 79310ae071b..1695f3828c5 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 = UNPACK_RESET_PROTECT_UNTRACKED;
> +		unpack_tree_opts.dir = xcalloc(1, sizeof(*unpack_tree_opts.dir));
> +		unpack_tree_opts.dir->flags |= DIR_SHOW_IGNORED;
> +		setup_standard_excludes(unpack_tree_opts.dir);
> +	}
>  
>  	if (repo_read_index_unmerged(r) < 0) {
>  		ret = error(_("could not read index"));
> @@ -131,6 +136,10 @@ reset_head_refs:
>  			    oid_to_hex(oid), "1", NULL);
>  
>  leave_reset_head:
> +	if (unpack_tree_opts.dir) {
> +		dir_clear(unpack_tree_opts.dir);
> +		FREE_AND_NULL(unpack_tree_opts.dir);

> +	}
>  	strbuf_release(&msg);
>  	rollback_lock_file(&lock);
>  	clear_unpack_trees_porcelain(&unpack_tree_opts);
> 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 5786645f315..fcbe63bbed9 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -1693,6 +1693,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>  	struct pattern_list pl;
>  	int free_pattern_list = 0;
>  
> +	if (o->reset == UNPACK_RESET_INVALID)
> +		BUG("o->reset had a value of 1; should be UNPACK_TREES_*_UNTRACKED");
> +
>  	if (len > MAX_UNPACK_TREES)
>  		die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES);
>  
> @@ -2218,7 +2221,8 @@ static int verify_absent_1(const struct cache_entry *ce,
>  	int len;
>  	struct stat st;
>  
> -	if (o->index_only || o->reset || !o->update)
> +	if (o->index_only || !o->update ||
> +	    o->reset == UNPACK_RESET_OVERWRITE_UNTRACKED)
>  		return 0;
>  
>  	len = check_leading_path(ce->name, ce_namelen(ce), 0);
> diff --git a/unpack-trees.h b/unpack-trees.h
> index 2d88b19dca7..1f386fb16cc 100644
> --- a/unpack-trees.h
> +++ b/unpack-trees.h
> @@ -45,9 +45,15 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
>   */
>  void clear_unpack_trees_porcelain(struct unpack_trees_options *opts);
>  
> +enum unpack_trees_reset_type {
> +	UNPACK_RESET_NONE = 0,    /* traditional "false" value; still valid */
> +	UNPACK_RESET_INVALID = 1, /* "true" no longer valid; use below values */
> +	UNPACK_RESET_PROTECT_UNTRACKED,
> +	UNPACK_RESET_OVERWRITE_UNTRACKED
> +};
> +
>  struct unpack_trees_options {
> -	unsigned int reset,
> -		     merge,
> +	unsigned int merge,
>  		     update,
>  		     clone,
>  		     index_only,
> @@ -64,6 +70,7 @@ struct unpack_trees_options {
>  		     exiting_early,
>  		     show_all_errors,
>  		     dry_run;
> +	enum unpack_trees_reset_type reset;
>  	const char *prefix;
>  	int cache_bottom;
>  	struct dir_struct *dir;

>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > Also, note that 'git checkout <pathspec>' currently also allows
> > overwriting untracked files.  That case should also be fixed, ...
>
> I wasted a few minutes wondering about the example.  Please make it
> clear that you are checking out of a tree-ish that is different from
> HEAD, as there will by definition no "overwriting untracked" if you
> are checking out of the index.
>
> E.g. "git checkout <tree-ish> -- <pathspec>".
>
> With this command line:
>
>    $ git checkout HEAD~24 -- path
>
> where path used to be there as late as 24 revisions ago, but since
> then we removed, and the user wants to materialize the file out of
> the old version, path, be it tracked, untracked, or even a
> directory, should be made identical to the copy from the given
> version, no?  Where does the "should also be fixed" come from?

> > 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);
>
> Do these three lines make a recurring pattern when opts.reset is set?
> I am wondering if this can be done more centrally by the unpack-trees
> machinery (i.e. "gee this one has o->reset set to X, so let's set up
> the o->dir before doing anything").

> > diff --git a/builtin/checkout.c b/builtin/checkout.c
> > index b5d477919a7..52826e0d145 100644
> > --- a/builtin/checkout.c
> > +++ b/builtin/checkout.c
> > @@ -641,23 +641,37 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
> >  {
> >       struct unpack_trees_options opts;
> >       struct tree_desc tree_desc;
> > +     int unpack_trees_ret;
> >
> >       memset(&opts, 0, sizeof(opts));
> >       opts.head_idx = -1;
> >       opts.update = worktree;
> >       opts.skip_unmerged = !worktree;
> > -     opts.reset = 1;
> > +     opts.reset = o->force ? UNPACK_RESET_OVERWRITE_UNTRACKED :
> > +                             UNPACK_RESET_PROTECT_UNTRACKED;
> >       opts.merge = 1;
> >       opts.fn = oneway_merge;
> >       opts.verbose_update = o->show_progress;
> >       opts.src_index = &the_index;
> >       opts.dst_index = &the_index;
> > +     if (o->overwrite_ignore) {
> > +             opts.dir = xcalloc(1, sizeof(*opts.dir));
> > +             opts.dir->flags |= DIR_SHOW_IGNORED;
> > +             setup_standard_excludes(opts.dir);
> > +     }
>
> If our longer term goal is to decide classification of files not in
> the index (currently, "ignored" and "untracked", but we may want to
> add a new "precious" class) and (across various commands that build
> on the unpack-trees infrastructure) to protect the "untracked" and
> "precious" ones, with --[no-]overwrite-{ignore,untracked} options as
> escape hatches, uniformly, perhaps the --[no-]-overwrite-ignore
> option may be stolen from here and shifted to unpack_tree_options to
> help us going in that direction?  This is just an observation for
> longer term, not a suggestion to include the first step for such a
> move in this series.

>
> >       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);
>
> This dir_clear() is also a recurring theme.  See below.
>
> > +     }
> > +
> > +     switch (unpack_trees_ret) {
> >       case -2:
> >               *writeout_error = 1;
> >               /*
> > diff --git a/builtin/read-tree.c b/builtin/read-tree.c
> > index 485e7b04794..740fc0335af 100644
> > --- a/builtin/read-tree.c
> > +++ b/builtin/read-tree.c
> > @@ -174,6 +174,9 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
> >       if (1 < opts.merge + opts.reset + prefix_set)
> >               die("Which one? -m, --reset, or --prefix?");
> >
> > +     if (opts.reset)
> > +             opts.reset = UNPACK_RESET_OVERWRITE_UNTRACKED;
> > +
>
> We do not do anything about opts.dir here by default, which means we
> by default do not overwrite ignored, but that's OK, because this old
> command explicitly takes --exclude-per-directory to tell it what the
> command should consider "ignored" and with the option given we do
> prepare opts.dir just fine.
>
> > diff --git a/builtin/reset.c b/builtin/reset.c
> > index 43e855cb887..a12ee986e9f 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,20 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t
> >               break;
> >       case HARD:
> >               opts.update = 1;
> > -             /* fallthrough */
> > +             opts.reset = UNPACK_RESET_OVERWRITE_UNTRACKED;
> > +             break;
> > +     case MIXED:
> > +             opts.reset = UNPACK_RESET_PROTECT_UNTRACKED;
> > +             /* but opts.update=0, so working tree not updated */
> > +             break;
> >       default:
> > -             opts.reset = 1;
> > +             BUG("invalid reset_type passed to reset_index");
> > +     }
> > +     if (opts.reset == UNPACK_RESET_PROTECT_UNTRACKED) {
>
> Unlike the one in "am", this cares .reset being a particular value,
> not just being non-zero.  Puzzling.
>
> It is a bit counter-intuitive in that we do not allow overwrite
> ignored (which is currently a synonym for "expendable") when .reset
> is set to allow us to ovewrite 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);
>
> > @@ -104,6 +116,10 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t
> >       ret = 0;
> >
> >  out:
> > +     if (opts.reset == UNPACK_RESET_PROTECT_UNTRACKED) {
> > +             dir_clear(opts.dir);
> > +             FREE_AND_NULL(opts.dir);
>
> This dir_clear() is also a recurring theme.  See below.
>
> > diff --git a/builtin/stash.c b/builtin/stash.c
> > index 8f42360ca91..563f590afbd 100644
> > --- a/builtin/stash.c
> > +++ b/builtin/stash.c
> > @@ -237,6 +237,7 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)
> >       struct tree_desc t[MAX_UNPACK_TREES];
> >       struct tree *tree;
> >       struct lock_file lock_file = LOCK_INIT;
> > +     int unpack_trees_ret;
> >
> >       read_cache_preload(NULL);
> >       if (refresh_cache(REFRESH_QUIET))
> > @@ -256,11 +257,23 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)
> >       opts.src_index = &the_index;
> >       opts.dst_index = &the_index;
> >       opts.merge = 1;
> > -     opts.reset = reset;
> > +     opts.reset = reset ? UNPACK_RESET_PROTECT_UNTRACKED : 0;
> > +     if (opts.reset) {
> > +             opts.dir = xcalloc(1, sizeof(*opts.dir));
> > +             opts.dir->flags |= DIR_SHOW_IGNORED;
> > +             setup_standard_excludes(opts.dir);
> > +     }
> >       opts.update = update;
> >       opts.fn = oneway_merge;
> >
> > -     if (unpack_trees(nr_trees, t, &opts))
> > +     unpack_trees_ret = unpack_trees(nr_trees, t, &opts);
> > +
> > +     if (opts.reset) {
> > +             dir_clear(opts.dir);
> > +             FREE_AND_NULL(opts.dir);
>
> This dir_clear() is also a recurring theme.  Why aren't their guards
> uniformly "if (opts.dir)"?  The logic to decide if we set up opts.dir
> or not may be far from here and may be different from code path to
> code path, but the need to clear opts.dir should not have to care
> why opts.dir was populated, no?

> > +     }
> > +
> > +     if (unpack_trees_ret)
> >               return -1;
> >
> >       if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
> > diff --git a/reset.c b/reset.c
> > index 79310ae071b..1695f3828c5 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 = UNPACK_RESET_PROTECT_UNTRACKED;
> > +             unpack_tree_opts.dir = xcalloc(1, sizeof(*unpack_tree_opts.dir));
> > +             unpack_tree_opts.dir->flags |= DIR_SHOW_IGNORED;
> > +             setup_standard_excludes(unpack_tree_opts.dir);
> > +     }
> >
> >       if (repo_read_index_unmerged(r) < 0) {
> >               ret = error(_("could not read index"));
> > @@ -131,6 +136,10 @@ reset_head_refs:
> >                           oid_to_hex(oid), "1", NULL);
> >
> >  leave_reset_head:
> > +     if (unpack_tree_opts.dir) {
> > +             dir_clear(unpack_tree_opts.dir);
> > +             FREE_AND_NULL(unpack_tree_opts.dir);
>
> Yes, I think this is the right way to decide if we call dir_clear(),
> and all other hunks in this patch should do the same.

> 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.
> [...]
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -356,6 +356,11 @@ static int add_worktree(const char *path, const char *refname,
> +               /*
> +                * 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);

>
> On Fri, Sep 24, 2021 at 2:37 AM Elijah Newren via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > 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.
> > [...]
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > ---
> > diff --git a/builtin/worktree.c b/builtin/worktree.c
> > @@ -356,6 +356,11 @@ static int add_worktree(const char *path, const char *refname,
> > +               /*
> > +                * 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);
>
> I understand that this comment helps you or some other person auditing
> similar cases in the future, however, as a standalone comment for a
> reader who isn't aware of the intention, it seems more confusing than
> illuminating. It also detracts from the important purpose of `--hard`
> here, which is that it is necessary in order to get `git reset` to
> actually "checkout" the files into the empty directory, so use of
> `--hard` is not an accident or carelessness.

> These days, we'd probably just use:
>
>     git restore --no-recurse-submodules .
>
> instead (including the final `.`) to achieve the same, and that
> wouldn't need any sort of cuationary comment like the one being added
> by this patch. So, perhaps that's a better way to go, or maybe it's
> outside the scope of this series...

>
> > read-tree, merge-recursive: overwrite ignored files by default
>
> When this patch shipped in v1.34, a test broke in a project of mine
> (https://github.com/buildinspace/peru/blob/e9ba6e0024ea08105a8d027f958899cca39aeb9a/tests/test_cache.py#L111-L117)
> that was relying on git read-tree *not* to respect .gitignore files.
> (Obligatory https://xkcd.com/1172.) That peru tool is using git
> plumbing commands to manage trees of files, but it tries to keep this
> implementation detail internal, and behaving differently in the
> presence of a .gitignore file belonging to the user would leak this
> internal implementation detail. I've been trying to figure out a way
> to reproduce the Git 1.33 behavior in Git 1.34, but so far I haven't
> found any flags or configs to do that. (For example, putting !* in
> .git/info/exclude doesn't seem to help, I think because a .gitignore
> file in the working tree takes precedence.) Can anyone suggest another
> workaround?
>
> This is my first mail to this list, so please let me know if I mess up
> the etiquette.

> From: Elijah Newren <newren@gmail.com>
>
> Currently, every caller of unpack_trees() that wants to ensure ignored
> files are overwritten by default needs to:
>    * allocate unpack_trees_options.dir
>    * flip the DIR_SHOW_IGNORED flag in unpack_trees_options.dir->flags
>    * call setup_standard_excludes
> AND then after the call to unpack_trees() needs to
>    * call dir_clear()
>    * deallocate unpack_trees_options.dir
> That's a fair amount of boilerplate, and every caller uses identical
> code.  Make this easier by instead introducing a new boolean value where
> the default value (0) does what we want so that new callers of
> unpack_trees() automatically get the appropriate behavior.  And move all
> the handling of unpack_trees_options.dir into unpack_trees() itself.
>
> While preserve_ignored = 0 is the behavior we feel is the appropriate
> default, we defer fixing commands to use the appropriate default until a
> later commit.  So, this commit introduces several locations where we
> manually set preserve_ignored=1.  This makes it clear where code paths
> were previously preserving ignored files when they should not have been;
> a future commit will flip these to instead use a value of 0 to get the
> behavior we want.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  builtin/am.c        |  3 +++
>  builtin/checkout.c  | 11 ++---------
>  builtin/clone.c     |  2 ++
>  builtin/merge.c     |  2 ++
>  builtin/read-tree.c | 13 +++----------
>  builtin/reset.c     |  2 ++
>  builtin/stash.c     |  3 +++
>  merge-ort.c         |  8 +-------
>  merge-recursive.c   |  8 +-------
>  merge.c             |  8 +-------
>  reset.c             |  2 ++
>  sequencer.c         |  2 ++
>  unpack-trees.c      | 10 ++++++++++
>  unpack-trees.h      |  1 +
>  14 files changed, 35 insertions(+), 40 deletions(-)
>
> diff --git a/builtin/am.c b/builtin/am.c
> index e4a0ff9cd7c..1ee70692bc3 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 5335435d616..5e7957dd068 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -648,6 +648,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
>  	opts.skip_unmerged = !worktree;
>  	opts.reset = 1;
>  	opts.merge = 1;
> +	opts.preserve_ignored = 0;
>  	opts.fn = oneway_merge;
>  	opts.verbose_update = o->show_progress;
>  	opts.src_index = &the_index;
> @@ -746,11 +747,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
>  				       new_branch_info->commit ?
>  				       &new_branch_info->commit->object.oid :
>  				       &new_branch_info->oid, NULL);
> -		if (opts->overwrite_ignore) {
> -			topts.dir = xcalloc(1, sizeof(*topts.dir));
> -			topts.dir->flags |= DIR_SHOW_IGNORED;
> -			setup_standard_excludes(topts.dir);
> -		}
> +		topts.preserve_ignored = !opts->overwrite_ignore;
>  		tree = parse_tree_indirect(old_branch_info->commit ?
>  					   &old_branch_info->commit->object.oid :
>  					   the_hash_algo->empty_tree);
> @@ -760,10 +757,6 @@ static int merge_working_tree(const struct checkout_opts *opts,
>  		init_tree_desc(&trees[1], tree->buffer, tree->size);
>  
>  		ret = unpack_trees(2, trees, &topts);
> -		if (topts.dir) {
> -			dir_clear(topts.dir);
> -			FREE_AND_NULL(topts.dir);
> -		}
>  		clear_unpack_trees_porcelain(&topts);
>  		if (ret == -1) {
>  			/*
> diff --git a/builtin/clone.c b/builtin/clone.c
> index ff1d3d447a3..be1c3840d62 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -687,6 +687,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 3fbdacc7db4..1e5fff095fc 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 73cb957a69b..443d206eca6 100644
> --- a/builtin/read-tree.c
> +++ b/builtin/read-tree.c
> @@ -201,11 +201,9 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
>  	if ((opts.update || opts.index_only) && !opts.merge)
>  		die("%s is meaningless without -m, --reset, or --prefix",
>  		    opts.update ? "-u" : "-i");
> -	if (opts.update && !opts.reset) {
> -		CALLOC_ARRAY(opts.dir, 1);
> -		opts.dir->flags |= DIR_SHOW_IGNORED;
> -		setup_standard_excludes(opts.dir);
> -	}
> +	if (opts.update && !opts.reset)
> +		opts.preserve_ignored = 0;
> +	/* otherwise, opts.preserve_ignored is irrelevant */
>  	if (opts.merge && !opts.index_only)
>  		setup_work_tree();
>  
> @@ -245,11 +243,6 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
>  	if (unpack_trees(nr_trees, t, &opts))
>  		return 128;
>  
> -	if (opts.dir) {
> -		dir_clear(opts.dir);
> -		FREE_AND_NULL(opts.dir);
> -	}
> -
>  	if (opts.debug_unpack || opts.dry_run)
>  		return 0; /* do not write the index out */
>  
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 51c9e2f43ff..7f38656f018 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -67,6 +67,8 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t
>  	case KEEP:
>  	case MERGE:
>  		opts.update = 1;
> +		/* FIXME: Default should be to remove ignored files */
> +		opts.preserve_ignored = 1;
>  		break;
>  	case HARD:
>  		opts.update = 1;
> diff --git a/builtin/stash.c b/builtin/stash.c
> index 8f42360ca91..88287b890d5 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -258,6 +258,9 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)
>  	opts.merge = 1;
>  	opts.reset = reset;
>  	opts.update = update;
> +	if (update && !reset)
> +		/* FIXME: Default should be to remove ignored files */
> +		opts.preserve_ignored = 1;
>  	opts.fn = oneway_merge;
>  
>  	if (unpack_trees(nr_trees, t, &opts))
> diff --git a/merge-ort.c b/merge-ort.c
> index 35aa979c3a4..0d64ec716bd 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -4045,11 +4045,7 @@ static int checkout(struct merge_options *opt,
>  	unpack_opts.quiet = 0; /* FIXME: sequencer might want quiet? */
>  	unpack_opts.verbose_update = (opt->verbosity > 2);
>  	unpack_opts.fn = twoway_merge;
> -	if (1/* FIXME: opts->overwrite_ignore*/) {
> -		CALLOC_ARRAY(unpack_opts.dir, 1);
> -		unpack_opts.dir->flags |= DIR_SHOW_IGNORED;
> -		setup_standard_excludes(unpack_opts.dir);
> -	}
> +	unpack_opts.preserve_ignored = 0; /* FIXME: !opts->overwrite_ignore*/
>  	parse_tree(prev);
>  	init_tree_desc(&trees[0], prev->buffer, prev->size);
>  	parse_tree(next);
> @@ -4057,8 +4053,6 @@ static int checkout(struct merge_options *opt,
>  
>  	ret = unpack_trees(2, trees, &unpack_opts);
>  	clear_unpack_trees_porcelain(&unpack_opts);
> -	dir_clear(unpack_opts.dir);
> -	FREE_AND_NULL(unpack_opts.dir);
>  	return ret;
>  }
>  
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 233d9f686ad..2be3f5d4044 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -411,9 +411,7 @@ static int unpack_trees_start(struct merge_options *opt,
>  	else {
>  		opt->priv->unpack_opts.update = 1;
>  		/* FIXME: should only do this if !overwrite_ignore */
> -		CALLOC_ARRAY(opt->priv->unpack_opts.dir, 1);
> -		opt->priv->unpack_opts.dir->flags |= DIR_SHOW_IGNORED;
> -		setup_standard_excludes(opt->priv->unpack_opts.dir);
> +		opt->priv->unpack_opts.preserve_ignored = 0;
>  	}
>  	opt->priv->unpack_opts.merge = 1;
>  	opt->priv->unpack_opts.head_idx = 2;
> @@ -428,10 +426,6 @@ static int unpack_trees_start(struct merge_options *opt,
>  	init_tree_desc_from_tree(t+2, merge);
>  
>  	rc = unpack_trees(3, t, &opt->priv->unpack_opts);
> -	if (opt->priv->unpack_opts.dir) {
> -		dir_clear(opt->priv->unpack_opts.dir);
> -		FREE_AND_NULL(opt->priv->unpack_opts.dir);
> -	}
>  	cache_tree_free(&opt->repo->index->cache_tree);
>  
>  	/*
> diff --git a/merge.c b/merge.c
> index 6e736881d90..2382ff66d35 100644
> --- a/merge.c
> +++ b/merge.c
> @@ -53,7 +53,6 @@ int checkout_fast_forward(struct repository *r,
>  	struct unpack_trees_options opts;
>  	struct tree_desc t[MAX_UNPACK_TREES];
>  	int i, nr_trees = 0;
> -	struct dir_struct dir = DIR_INIT;
>  	struct lock_file lock_file = LOCK_INIT;
>  
>  	refresh_index(r->index, REFRESH_QUIET, NULL, NULL, NULL);
> @@ -80,11 +79,7 @@ int checkout_fast_forward(struct repository *r,
>  	}
>  
>  	memset(&opts, 0, sizeof(opts));
> -	if (overwrite_ignore) {
> -		dir.flags |= DIR_SHOW_IGNORED;
> -		setup_standard_excludes(&dir);
> -		opts.dir = &dir;
> -	}
> +	opts.preserve_ignored = !overwrite_ignore;
>  
>  	opts.head_idx = 1;
>  	opts.src_index = r->index;
> @@ -101,7 +96,6 @@ int checkout_fast_forward(struct repository *r,
>  		clear_unpack_trees_porcelain(&opts);
>  		return -1;
>  	}
> -	dir_clear(&dir);
>  	clear_unpack_trees_porcelain(&opts);
>  
>  	if (write_locked_index(r->index, &lock_file, COMMIT_LOCK))
> diff --git a/reset.c b/reset.c
> index 79310ae071b..41b3e2d88de 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 614d56f5e21..098566c68d9 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3699,6 +3699,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 8ea0a542da8..1e4eae1dc7d 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -1707,6 +1707,12 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>  		ensure_full_index(o->dst_index);
>  	}
>  
> +	if (!o->preserve_ignored) {
> +		CALLOC_ARRAY(o->dir, 1);
> +		o->dir->flags |= DIR_SHOW_IGNORED;
> +		setup_standard_excludes(o->dir);
> +	}
> +
>  	if (!core_apply_sparse_checkout || !o->update)
>  		o->skip_sparse_checkout = 1;
>  	if (!o->skip_sparse_checkout && !o->pl) {
> @@ -1868,6 +1874,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>  done:
>  	if (free_pattern_list)
>  		clear_pattern_list(&pl);
> +	if (o->dir) {
> +		dir_clear(o->dir);
> +		FREE_AND_NULL(o->dir);
> +	}
>  	trace2_region_leave("unpack_trees", "unpack_trees", the_repository);
>  	trace_performance_leave("unpack_trees");
>  	return ret;
> diff --git a/unpack-trees.h b/unpack-trees.h
> index 2d88b19dca7..f98cfd49d7b 100644
> --- a/unpack-trees.h
> +++ b/unpack-trees.h
> @@ -49,6 +49,7 @@ struct unpack_trees_options {
>  	unsigned int reset,
>  		     merge,
>  		     update,
> +		     preserve_ignored,
>  		     clone,
>  		     index_only,
>  		     nontrivial_merge,

>
> On Mon, Sep 27 2021, Elijah Newren via GitGitGadget wrote:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > Currently, every caller of unpack_trees() that wants to ensure ignored
> > files are overwritten by default needs to:
> >    * allocate unpack_trees_options.dir
> >    * flip the DIR_SHOW_IGNORED flag in unpack_trees_options.dir->flags
> >    * call setup_standard_excludes
> > AND then after the call to unpack_trees() needs to
> >    * call dir_clear()
> >    * deallocate unpack_trees_options.dir
> > That's a fair amount of boilerplate, and every caller uses identical
> > code.  Make this easier by instead introducing a new boolean value where
> > the default value (0) does what we want so that new callers of
> > unpack_trees() automatically get the appropriate behavior.  And move all
> > the handling of unpack_trees_options.dir into unpack_trees() itself.
> >
> > While preserve_ignored = 0 is the behavior we feel is the appropriate
> > default, we defer fixing commands to use the appropriate default until a
> > later commit.  So, this commit introduces several locations where we
> > manually set preserve_ignored=1.  This makes it clear where code paths
> > were previously preserving ignored files when they should not have been;
> > a future commit will flip these to instead use a value of 0 to get the
> > behavior we want.
> >
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > ---
> >  builtin/am.c        |  3 +++
> >  builtin/checkout.c  | 11 ++---------
> >  builtin/clone.c     |  2 ++
> >  builtin/merge.c     |  2 ++
> >  builtin/read-tree.c | 13 +++----------
> >  builtin/reset.c     |  2 ++
> >  builtin/stash.c     |  3 +++
> >  merge-ort.c         |  8 +-------
> >  merge-recursive.c   |  8 +-------
> >  merge.c             |  8 +-------
> >  reset.c             |  2 ++
> >  sequencer.c         |  2 ++
> >  unpack-trees.c      | 10 ++++++++++
> >  unpack-trees.h      |  1 +
> >  14 files changed, 35 insertions(+), 40 deletions(-)
> >
> > diff --git a/builtin/am.c b/builtin/am.c
> > index e4a0ff9cd7c..1ee70692bc3 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 5335435d616..5e7957dd068 100644
> > --- a/builtin/checkout.c
> > +++ b/builtin/checkout.c
> > @@ -648,6 +648,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
> >       opts.skip_unmerged = !worktree;
> >       opts.reset = 1;
> >       opts.merge = 1;
> > +     opts.preserve_ignored = 0;
> >       opts.fn = oneway_merge;
> >       opts.verbose_update = o->show_progress;
> >       opts.src_index = &the_index;
> > @@ -746,11 +747,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
> >                                      new_branch_info->commit ?
> >                                      &new_branch_info->commit->object.oid :
> >                                      &new_branch_info->oid, NULL);
> > -             if (opts->overwrite_ignore) {
> > -                     topts.dir = xcalloc(1, sizeof(*topts.dir));
> > -                     topts.dir->flags |= DIR_SHOW_IGNORED;
> > -                     setup_standard_excludes(topts.dir);
> > -             }
> > +             topts.preserve_ignored = !opts->overwrite_ignore;
> >               tree = parse_tree_indirect(old_branch_info->commit ?
> >                                          &old_branch_info->commit->object.oid :
> >                                          the_hash_algo->empty_tree);
> > @@ -760,10 +757,6 @@ static int merge_working_tree(const struct checkout_opts *opts,
> >               init_tree_desc(&trees[1], tree->buffer, tree->size);
> >
> >               ret = unpack_trees(2, trees, &topts);
> > -             if (topts.dir) {
> > -                     dir_clear(topts.dir);
> > -                     FREE_AND_NULL(topts.dir);
> > -             }
> >               clear_unpack_trees_porcelain(&topts);
> >               if (ret == -1) {
> >                       /*
> > diff --git a/builtin/clone.c b/builtin/clone.c
> > index ff1d3d447a3..be1c3840d62 100644
> > --- a/builtin/clone.c
> > +++ b/builtin/clone.c
> > @@ -687,6 +687,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 3fbdacc7db4..1e5fff095fc 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 73cb957a69b..443d206eca6 100644
> > --- a/builtin/read-tree.c
> > +++ b/builtin/read-tree.c
> > @@ -201,11 +201,9 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
> >       if ((opts.update || opts.index_only) && !opts.merge)
> >               die("%s is meaningless without -m, --reset, or --prefix",
> >                   opts.update ? "-u" : "-i");
> > -     if (opts.update && !opts.reset) {
> > -             CALLOC_ARRAY(opts.dir, 1);
> > -             opts.dir->flags |= DIR_SHOW_IGNORED;
> > -             setup_standard_excludes(opts.dir);
> > -     }
> > +     if (opts.update && !opts.reset)
> > +             opts.preserve_ignored = 0;
> > +     /* otherwise, opts.preserve_ignored is irrelevant */
> >       if (opts.merge && !opts.index_only)
> >               setup_work_tree();
> >
> > @@ -245,11 +243,6 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
> >       if (unpack_trees(nr_trees, t, &opts))
> >               return 128;
> >
> > -     if (opts.dir) {
> > -             dir_clear(opts.dir);
> > -             FREE_AND_NULL(opts.dir);
> > -     }
> > -
> >       if (opts.debug_unpack || opts.dry_run)
> >               return 0; /* do not write the index out */
> >
> > diff --git a/builtin/reset.c b/builtin/reset.c
> > index 51c9e2f43ff..7f38656f018 100644
> > --- a/builtin/reset.c
> > +++ b/builtin/reset.c
> > @@ -67,6 +67,8 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t
> >       case KEEP:
> >       case MERGE:
> >               opts.update = 1;
> > +             /* FIXME: Default should be to remove ignored files */
> > +             opts.preserve_ignored = 1;
> >               break;
> >       case HARD:
> >               opts.update = 1;
> > diff --git a/builtin/stash.c b/builtin/stash.c
> > index 8f42360ca91..88287b890d5 100644
> > --- a/builtin/stash.c
> > +++ b/builtin/stash.c
> > @@ -258,6 +258,9 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)
> >       opts.merge = 1;
> >       opts.reset = reset;
> >       opts.update = update;
> > +     if (update && !reset)
> > +             /* FIXME: Default should be to remove ignored files */
> > +             opts.preserve_ignored = 1;
> >       opts.fn = oneway_merge;
> >
> >       if (unpack_trees(nr_trees, t, &opts))
> > diff --git a/merge-ort.c b/merge-ort.c
> > index 35aa979c3a4..0d64ec716bd 100644
> > --- a/merge-ort.c
> > +++ b/merge-ort.c
> > @@ -4045,11 +4045,7 @@ static int checkout(struct merge_options *opt,
> >       unpack_opts.quiet = 0; /* FIXME: sequencer might want quiet? */
> >       unpack_opts.verbose_update = (opt->verbosity > 2);
> >       unpack_opts.fn = twoway_merge;
> > -     if (1/* FIXME: opts->overwrite_ignore*/) {
> > -             CALLOC_ARRAY(unpack_opts.dir, 1);
> > -             unpack_opts.dir->flags |= DIR_SHOW_IGNORED;
> > -             setup_standard_excludes(unpack_opts.dir);
> > -     }
> > +     unpack_opts.preserve_ignored = 0; /* FIXME: !opts->overwrite_ignore*/
> >       parse_tree(prev);
> >       init_tree_desc(&trees[0], prev->buffer, prev->size);
> >       parse_tree(next);
> > @@ -4057,8 +4053,6 @@ static int checkout(struct merge_options *opt,
> >
> >       ret = unpack_trees(2, trees, &unpack_opts);
> >       clear_unpack_trees_porcelain(&unpack_opts);
> > -     dir_clear(unpack_opts.dir);
> > -     FREE_AND_NULL(unpack_opts.dir);
> >       return ret;
> >  }
> >
> > diff --git a/merge-recursive.c b/merge-recursive.c
> > index 233d9f686ad..2be3f5d4044 100644
> > --- a/merge-recursive.c
> > +++ b/merge-recursive.c
> > @@ -411,9 +411,7 @@ static int unpack_trees_start(struct merge_options *opt,
> >       else {
> >               opt->priv->unpack_opts.update = 1;
> >               /* FIXME: should only do this if !overwrite_ignore */
> > -             CALLOC_ARRAY(opt->priv->unpack_opts.dir, 1);
> > -             opt->priv->unpack_opts.dir->flags |= DIR_SHOW_IGNORED;
> > -             setup_standard_excludes(opt->priv->unpack_opts.dir);
> > +             opt->priv->unpack_opts.preserve_ignored = 0;
> >       }
> >       opt->priv->unpack_opts.merge = 1;
> >       opt->priv->unpack_opts.head_idx = 2;
> > @@ -428,10 +426,6 @@ static int unpack_trees_start(struct merge_options *opt,
> >       init_tree_desc_from_tree(t+2, merge);
> >
> >       rc = unpack_trees(3, t, &opt->priv->unpack_opts);
> > -     if (opt->priv->unpack_opts.dir) {
> > -             dir_clear(opt->priv->unpack_opts.dir);
> > -             FREE_AND_NULL(opt->priv->unpack_opts.dir);
> > -     }
> >       cache_tree_free(&opt->repo->index->cache_tree);
> >
> >       /*
> > diff --git a/merge.c b/merge.c
> > index 6e736881d90..2382ff66d35 100644
> > --- a/merge.c
> > +++ b/merge.c
> > @@ -53,7 +53,6 @@ int checkout_fast_forward(struct repository *r,
> >       struct unpack_trees_options opts;
> >       struct tree_desc t[MAX_UNPACK_TREES];
> >       int i, nr_trees = 0;
> > -     struct dir_struct dir = DIR_INIT;
> >       struct lock_file lock_file = LOCK_INIT;
> >
> >       refresh_index(r->index, REFRESH_QUIET, NULL, NULL, NULL);
> > @@ -80,11 +79,7 @@ int checkout_fast_forward(struct repository *r,
> >       }
> >
> >       memset(&opts, 0, sizeof(opts));
> > -     if (overwrite_ignore) {
> > -             dir.flags |= DIR_SHOW_IGNORED;
> > -             setup_standard_excludes(&dir);
> > -             opts.dir = &dir;
> > -     }
> > +     opts.preserve_ignored = !overwrite_ignore;
> >
> >       opts.head_idx = 1;
> >       opts.src_index = r->index;
> > @@ -101,7 +96,6 @@ int checkout_fast_forward(struct repository *r,
> >               clear_unpack_trees_porcelain(&opts);
> >               return -1;
> >       }
> > -     dir_clear(&dir);
> >       clear_unpack_trees_porcelain(&opts);
> >
> >       if (write_locked_index(r->index, &lock_file, COMMIT_LOCK))
> > diff --git a/reset.c b/reset.c
> > index 79310ae071b..41b3e2d88de 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 614d56f5e21..098566c68d9 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -3699,6 +3699,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 8ea0a542da8..1e4eae1dc7d 100644
> > --- a/unpack-trees.c
> > +++ b/unpack-trees.c
> > @@ -1707,6 +1707,12 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
> >               ensure_full_index(o->dst_index);
> >       }
> >
> > +     if (!o->preserve_ignored) {
> > +             CALLOC_ARRAY(o->dir, 1);
> > +             o->dir->flags |= DIR_SHOW_IGNORED;
> > +             setup_standard_excludes(o->dir);
> > +     }
> > +
> >       if (!core_apply_sparse_checkout || !o->update)
> >               o->skip_sparse_checkout = 1;
> >       if (!o->skip_sparse_checkout && !o->pl) {
> > @@ -1868,6 +1874,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
> >  done:
> >       if (free_pattern_list)
> >               clear_pattern_list(&pl);
> > +     if (o->dir) {
> > +             dir_clear(o->dir);
> > +             FREE_AND_NULL(o->dir);
> > +     }
> >       trace2_region_leave("unpack_trees", "unpack_trees", the_repository);
> >       trace_performance_leave("unpack_trees");
> >       return ret;
> > diff --git a/unpack-trees.h b/unpack-trees.h
> > index 2d88b19dca7..f98cfd49d7b 100644
> > --- a/unpack-trees.h
> > +++ b/unpack-trees.h
> > @@ -49,6 +49,7 @@ struct unpack_trees_options {
> >       unsigned int reset,
> >                    merge,
> >                    update,
> > +                  preserve_ignored,
> >                    clone,
> >                    index_only,
> >                    nontrivial_merge,
>
> I think getting rid of the boilerplate makes sense, but it doesn't sound
> from the commit message like you've considered just making that "struct
> dir*" member a "struct dir" instead.
>
> That simplifies things a lot, i.e. we can just DIR_INIT it, and don't
> need every caller to malloc/free it.

> On Wed, Sep 29, 2021 at 2:27 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>> On Mon, Sep 27 2021, Elijah Newren via GitGitGadget wrote:
>>
>> > From: Elijah Newren <newren@gmail.com>
>> >
>> > Currently, every caller of unpack_trees() that wants to ensure ignored
>> > files are overwritten by default needs to:
>> >    * allocate unpack_trees_options.dir
>> >    * flip the DIR_SHOW_IGNORED flag in unpack_trees_options.dir->flags
>> >    * call setup_standard_excludes
>> > AND then after the call to unpack_trees() needs to
>> >    * call dir_clear()
>> >    * deallocate unpack_trees_options.dir
>> > That's a fair amount of boilerplate, and every caller uses identical
>> > code.  Make this easier by instead introducing a new boolean value where
>> > the default value (0) does what we want so that new callers of
>> > unpack_trees() automatically get the appropriate behavior.  And move all
>> > the handling of unpack_trees_options.dir into unpack_trees() itself.
>> >
>> > While preserve_ignored = 0 is the behavior we feel is the appropriate
>> > default, we defer fixing commands to use the appropriate default until a
>> > later commit.  So, this commit introduces several locations where we
>> > manually set preserve_ignored=1.  This makes it clear where code paths
>> > were previously preserving ignored files when they should not have been;
>> > a future commit will flip these to instead use a value of 0 to get the
>> > behavior we want.
>> >
>> > Signed-off-by: Elijah Newren <newren@gmail.com>
>> > ---
>> >  builtin/am.c        |  3 +++
>> >  builtin/checkout.c  | 11 ++---------
>> >  builtin/clone.c     |  2 ++
>> >  builtin/merge.c     |  2 ++
>> >  builtin/read-tree.c | 13 +++----------
>> >  builtin/reset.c     |  2 ++
>> >  builtin/stash.c     |  3 +++
>> >  merge-ort.c         |  8 +-------
>> >  merge-recursive.c   |  8 +-------
>> >  merge.c             |  8 +-------
>> >  reset.c             |  2 ++
>> >  sequencer.c         |  2 ++
>> >  unpack-trees.c      | 10 ++++++++++
>> >  unpack-trees.h      |  1 +
>> >  14 files changed, 35 insertions(+), 40 deletions(-)
>> >
>> > diff --git a/builtin/am.c b/builtin/am.c
>> > index e4a0ff9cd7c..1ee70692bc3 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 5335435d616..5e7957dd068 100644
>> > --- a/builtin/checkout.c
>> > +++ b/builtin/checkout.c
>> > @@ -648,6 +648,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
>> >       opts.skip_unmerged = !worktree;
>> >       opts.reset = 1;
>> >       opts.merge = 1;
>> > +     opts.preserve_ignored = 0;
>> >       opts.fn = oneway_merge;
>> >       opts.verbose_update = o->show_progress;
>> >       opts.src_index = &the_index;
>> > @@ -746,11 +747,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
>> >                                      new_branch_info->commit ?
>> >                                      &new_branch_info->commit->object.oid :
>> >                                      &new_branch_info->oid, NULL);
>> > -             if (opts->overwrite_ignore) {
>> > -                     topts.dir = xcalloc(1, sizeof(*topts.dir));
>> > -                     topts.dir->flags |= DIR_SHOW_IGNORED;
>> > -                     setup_standard_excludes(topts.dir);
>> > -             }
>> > +             topts.preserve_ignored = !opts->overwrite_ignore;
>> >               tree = parse_tree_indirect(old_branch_info->commit ?
>> >                                          &old_branch_info->commit->object.oid :
>> >                                          the_hash_algo->empty_tree);
>> > @@ -760,10 +757,6 @@ static int merge_working_tree(const struct checkout_opts *opts,
>> >               init_tree_desc(&trees[1], tree->buffer, tree->size);
>> >
>> >               ret = unpack_trees(2, trees, &topts);
>> > -             if (topts.dir) {
>> > -                     dir_clear(topts.dir);
>> > -                     FREE_AND_NULL(topts.dir);
>> > -             }
>> >               clear_unpack_trees_porcelain(&topts);
>> >               if (ret == -1) {
>> >                       /*
>> > diff --git a/builtin/clone.c b/builtin/clone.c
>> > index ff1d3d447a3..be1c3840d62 100644
>> > --- a/builtin/clone.c
>> > +++ b/builtin/clone.c
>> > @@ -687,6 +687,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 3fbdacc7db4..1e5fff095fc 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 73cb957a69b..443d206eca6 100644
>> > --- a/builtin/read-tree.c
>> > +++ b/builtin/read-tree.c
>> > @@ -201,11 +201,9 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
>> >       if ((opts.update || opts.index_only) && !opts.merge)
>> >               die("%s is meaningless without -m, --reset, or --prefix",
>> >                   opts.update ? "-u" : "-i");
>> > -     if (opts.update && !opts.reset) {
>> > -             CALLOC_ARRAY(opts.dir, 1);
>> > -             opts.dir->flags |= DIR_SHOW_IGNORED;
>> > -             setup_standard_excludes(opts.dir);
>> > -     }
>> > +     if (opts.update && !opts.reset)
>> > +             opts.preserve_ignored = 0;
>> > +     /* otherwise, opts.preserve_ignored is irrelevant */
>> >       if (opts.merge && !opts.index_only)
>> >               setup_work_tree();
>> >
>> > @@ -245,11 +243,6 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
>> >       if (unpack_trees(nr_trees, t, &opts))
>> >               return 128;
>> >
>> > -     if (opts.dir) {
>> > -             dir_clear(opts.dir);
>> > -             FREE_AND_NULL(opts.dir);
>> > -     }
>> > -
>> >       if (opts.debug_unpack || opts.dry_run)
>> >               return 0; /* do not write the index out */
>> >
>> > diff --git a/builtin/reset.c b/builtin/reset.c
>> > index 51c9e2f43ff..7f38656f018 100644
>> > --- a/builtin/reset.c
>> > +++ b/builtin/reset.c
>> > @@ -67,6 +67,8 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t
>> >       case KEEP:
>> >       case MERGE:
>> >               opts.update = 1;
>> > +             /* FIXME: Default should be to remove ignored files */
>> > +             opts.preserve_ignored = 1;
>> >               break;
>> >       case HARD:
>> >               opts.update = 1;
>> > diff --git a/builtin/stash.c b/builtin/stash.c
>> > index 8f42360ca91..88287b890d5 100644
>> > --- a/builtin/stash.c
>> > +++ b/builtin/stash.c
>> > @@ -258,6 +258,9 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)
>> >       opts.merge = 1;
>> >       opts.reset = reset;
>> >       opts.update = update;
>> > +     if (update && !reset)
>> > +             /* FIXME: Default should be to remove ignored files */
>> > +             opts.preserve_ignored = 1;
>> >       opts.fn = oneway_merge;
>> >
>> >       if (unpack_trees(nr_trees, t, &opts))
>> > diff --git a/merge-ort.c b/merge-ort.c
>> > index 35aa979c3a4..0d64ec716bd 100644
>> > --- a/merge-ort.c
>> > +++ b/merge-ort.c
>> > @@ -4045,11 +4045,7 @@ static int checkout(struct merge_options *opt,
>> >       unpack_opts.quiet = 0; /* FIXME: sequencer might want quiet? */
>> >       unpack_opts.verbose_update = (opt->verbosity > 2);
>> >       unpack_opts.fn = twoway_merge;
>> > -     if (1/* FIXME: opts->overwrite_ignore*/) {
>> > -             CALLOC_ARRAY(unpack_opts.dir, 1);
>> > -             unpack_opts.dir->flags |= DIR_SHOW_IGNORED;
>> > -             setup_standard_excludes(unpack_opts.dir);
>> > -     }
>> > +     unpack_opts.preserve_ignored = 0; /* FIXME: !opts->overwrite_ignore*/
>> >       parse_tree(prev);
>> >       init_tree_desc(&trees[0], prev->buffer, prev->size);
>> >       parse_tree(next);
>> > @@ -4057,8 +4053,6 @@ static int checkout(struct merge_options *opt,
>> >
>> >       ret = unpack_trees(2, trees, &unpack_opts);
>> >       clear_unpack_trees_porcelain(&unpack_opts);
>> > -     dir_clear(unpack_opts.dir);
>> > -     FREE_AND_NULL(unpack_opts.dir);
>> >       return ret;
>> >  }
>> >
>> > diff --git a/merge-recursive.c b/merge-recursive.c
>> > index 233d9f686ad..2be3f5d4044 100644
>> > --- a/merge-recursive.c
>> > +++ b/merge-recursive.c
>> > @@ -411,9 +411,7 @@ static int unpack_trees_start(struct merge_options *opt,
>> >       else {
>> >               opt->priv->unpack_opts.update = 1;
>> >               /* FIXME: should only do this if !overwrite_ignore */
>> > -             CALLOC_ARRAY(opt->priv->unpack_opts.dir, 1);
>> > -             opt->priv->unpack_opts.dir->flags |= DIR_SHOW_IGNORED;
>> > -             setup_standard_excludes(opt->priv->unpack_opts.dir);
>> > +             opt->priv->unpack_opts.preserve_ignored = 0;
>> >       }
>> >       opt->priv->unpack_opts.merge = 1;
>> >       opt->priv->unpack_opts.head_idx = 2;
>> > @@ -428,10 +426,6 @@ static int unpack_trees_start(struct merge_options *opt,
>> >       init_tree_desc_from_tree(t+2, merge);
>> >
>> >       rc = unpack_trees(3, t, &opt->priv->unpack_opts);
>> > -     if (opt->priv->unpack_opts.dir) {
>> > -             dir_clear(opt->priv->unpack_opts.dir);
>> > -             FREE_AND_NULL(opt->priv->unpack_opts.dir);
>> > -     }
>> >       cache_tree_free(&opt->repo->index->cache_tree);
>> >
>> >       /*
>> > diff --git a/merge.c b/merge.c
>> > index 6e736881d90..2382ff66d35 100644
>> > --- a/merge.c
>> > +++ b/merge.c
>> > @@ -53,7 +53,6 @@ int checkout_fast_forward(struct repository *r,
>> >       struct unpack_trees_options opts;
>> >       struct tree_desc t[MAX_UNPACK_TREES];
>> >       int i, nr_trees = 0;
>> > -     struct dir_struct dir = DIR_INIT;
>> >       struct lock_file lock_file = LOCK_INIT;
>> >
>> >       refresh_index(r->index, REFRESH_QUIET, NULL, NULL, NULL);
>> > @@ -80,11 +79,7 @@ int checkout_fast_forward(struct repository *r,
>> >       }
>> >
>> >       memset(&opts, 0, sizeof(opts));
>> > -     if (overwrite_ignore) {
>> > -             dir.flags |= DIR_SHOW_IGNORED;
>> > -             setup_standard_excludes(&dir);
>> > -             opts.dir = &dir;
>> > -     }
>> > +     opts.preserve_ignored = !overwrite_ignore;
>> >
>> >       opts.head_idx = 1;
>> >       opts.src_index = r->index;
>> > @@ -101,7 +96,6 @@ int checkout_fast_forward(struct repository *r,
>> >               clear_unpack_trees_porcelain(&opts);
>> >               return -1;
>> >       }
>> > -     dir_clear(&dir);
>> >       clear_unpack_trees_porcelain(&opts);
>> >
>> >       if (write_locked_index(r->index, &lock_file, COMMIT_LOCK))
>> > diff --git a/reset.c b/reset.c
>> > index 79310ae071b..41b3e2d88de 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 614d56f5e21..098566c68d9 100644
>> > --- a/sequencer.c
>> > +++ b/sequencer.c
>> > @@ -3699,6 +3699,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 8ea0a542da8..1e4eae1dc7d 100644
>> > --- a/unpack-trees.c
>> > +++ b/unpack-trees.c
>> > @@ -1707,6 +1707,12 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>> >               ensure_full_index(o->dst_index);
>> >       }
>> >
>> > +     if (!o->preserve_ignored) {
>> > +             CALLOC_ARRAY(o->dir, 1);
>> > +             o->dir->flags |= DIR_SHOW_IGNORED;
>> > +             setup_standard_excludes(o->dir);
>> > +     }
>> > +
>> >       if (!core_apply_sparse_checkout || !o->update)
>> >               o->skip_sparse_checkout = 1;
>> >       if (!o->skip_sparse_checkout && !o->pl) {
>> > @@ -1868,6 +1874,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>> >  done:
>> >       if (free_pattern_list)
>> >               clear_pattern_list(&pl);
>> > +     if (o->dir) {
>> > +             dir_clear(o->dir);
>> > +             FREE_AND_NULL(o->dir);
>> > +     }
>> >       trace2_region_leave("unpack_trees", "unpack_trees", the_repository);
>> >       trace_performance_leave("unpack_trees");
>> >       return ret;
>> > diff --git a/unpack-trees.h b/unpack-trees.h
>> > index 2d88b19dca7..f98cfd49d7b 100644
>> > --- a/unpack-trees.h
>> > +++ b/unpack-trees.h
>> > @@ -49,6 +49,7 @@ struct unpack_trees_options {
>> >       unsigned int reset,
>> >                    merge,
>> >                    update,
>> > +                  preserve_ignored,
>> >                    clone,
>> >                    index_only,
>> >                    nontrivial_merge,
>>
>> I think getting rid of the boilerplate makes sense, but it doesn't sound
>> from the commit message like you've considered just making that "struct
>> dir*" member a "struct dir" instead.
>>
>> That simplifies things a lot, i.e. we can just DIR_INIT it, and don't
>> need every caller to malloc/free it.
>
> See the next patch in the series.  :-)

>> Sometimes a pointer makes sense, but in this case the "struct
>> unpack_trees_options" can just own it.
>
> I did make it internal to unpack_trees_options in the next patch, but
> kept it as a pointer just because that let me know whether it was used
> or not.  I guess I could have added a boolean as well.  But I don't
> actually allocate anything, because it's either a NULL pointer, or a
> pointer to something on the stack.  So, I do get to just use DIR_INIT.

>
> On Wed, Sep 29 2021, Elijah Newren wrote:
>
> > On Wed, Sep 29, 2021 at 2:27 AM Ævar Arnfjörð Bjarmason
> > <avarab@gmail.com> wrote:
> >>

> >>
> >> I think getting rid of the boilerplate makes sense, but it doesn't sound
> >> from the commit message like you've considered just making that "struct
> >> dir*" member a "struct dir" instead.
> >>
> >> That simplifies things a lot, i.e. we can just DIR_INIT it, and don't
> >> need every caller to malloc/free it.
> >
> > See the next patch in the series.  :-)
>
> Ah!
>
> >> Sometimes a pointer makes sense, but in this case the "struct
> >> unpack_trees_options" can just own it.
> >
> > I did make it internal to unpack_trees_options in the next patch, but
> > kept it as a pointer just because that let me know whether it was used
> > or not.  I guess I could have added a boolean as well.  But I don't
> > actually allocate anything, because it's either a NULL pointer, or a
> > pointer to something on the stack.  So, I do get to just use DIR_INIT.
>
> I think I'm probably missing something. I just made it allocated on the
> stack by the caller using "struct unpack_trees_options", but then you
> end up having a dir* in the struct, but that's only filled in as a
> pointer to the stack variable? Maybe there's some subtlety I'm missing
> here...

> On Wed, Sep 29, 2021 at 11:32 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>> On Wed, Sep 29 2021, Elijah Newren wrote:
>>
>> > On Wed, Sep 29, 2021 at 2:27 AM Ævar Arnfjörð Bjarmason
>> > <avarab@gmail.com> wrote:
>> >>
> ...
>> >>
>> >> I think getting rid of the boilerplate makes sense, but it doesn't sound
>> >> from the commit message like you've considered just making that "struct
>> >> dir*" member a "struct dir" instead.
>> >>
>> >> That simplifies things a lot, i.e. we can just DIR_INIT it, and don't
>> >> need every caller to malloc/free it.
>> >
>> > See the next patch in the series.  :-)
>>
>> Ah!
>>
>> >> Sometimes a pointer makes sense, but in this case the "struct
>> >> unpack_trees_options" can just own it.
>> >
>> > I did make it internal to unpack_trees_options in the next patch, but
>> > kept it as a pointer just because that let me know whether it was used
>> > or not.  I guess I could have added a boolean as well.  But I don't
>> > actually allocate anything, because it's either a NULL pointer, or a
>> > pointer to something on the stack.  So, I do get to just use DIR_INIT.
>>
>> I think I'm probably missing something. I just made it allocated on the
>> stack by the caller using "struct unpack_trees_options", but then you
>> end up having a dir* in the struct, but that's only filled in as a
>> pointer to the stack variable? Maybe there's some subtlety I'm missing
>> here...
>
> As per the next patch:
>
> int unpack_trees(..., struct unpack_trees_options *o)
> {
>     struct dir_struct dir = DIR_INIT;
>     ...
>     if (!o->preserve_ignored) {
>         /* Setup 'dir', make o->dir point to it */
>         ....
>         o->dir = &dir;
>     }
>     ...
>     if (o->dir)
>         /* cleanup */
>     ....
> }
>
> The caller doesn't touch o->dir (other than initializing it to zeros);
> unpack_trees() is wholly responsible for it.  I'd kind of like to
> entirely remove dir from unpack_trees_options(), but I need a way of
> passing it down through all the other functions in unpack-trees.c, and
> leaving it in unpack_trees_options seems the easiest way to do so.  So
> I just marked it as "for internal use only".

>
> On Wed, Sep 29 2021, Elijah Newren wrote:
>
> > On Wed, Sep 29, 2021 at 11:32 AM Ævar Arnfjörð Bjarmason
> > <avarab@gmail.com> wrote:
> >>
> >> On Wed, Sep 29 2021, Elijah Newren wrote:
> >>

> > As per the next patch:
> >
> > int unpack_trees(..., struct unpack_trees_options *o)
> > {
> >     struct dir_struct dir = DIR_INIT;
> >     ...
> >     if (!o->preserve_ignored) {
> >         /* Setup 'dir', make o->dir point to it */
> >         ....
> >         o->dir = &dir;
> >     }
> >     ...
> >     if (o->dir)
> >         /* cleanup */
> >     ....
> > }
> >
> > The caller doesn't touch o->dir (other than initializing it to zeros);
> > unpack_trees() is wholly responsible for it.  I'd kind of like to
> > entirely remove dir from unpack_trees_options(), but I need a way of
> > passing it down through all the other functions in unpack-trees.c, and
> > leaving it in unpack_trees_options seems the easiest way to do so.  So
> > I just marked it as "for internal use only".
>
> I think I understand *how* it works, I'm puzzled by why you went for
> this whole level of indirection when you're using a struct on the stack
> in the end anyway, just ... put that in "struct unpack_trees_options"?
>
> Anyway, I see I have only myself to blame here, as you added these leak
> fixes in the v2 in response to some of my offhand comments.
>
> FWIW I then went on to do some deeper fixes not just on these leaks but
> the surrounding leaks, which will be blocked by 2/11 & 05/11 of this
> topic for a while. I suppose I only have myself to blame :)
>
> Below is a patch-on-top that I think makes this whole thing much simpler
> by doing away with the pointer entirely.
>
> I suppose this is also a partial reply to
> https://lore.kernel.org/git/CABPp-BG_qigBoirMGR-Yk9Niyxt0UmYCEqojsYxbSEarLAmraA@mail.gmail.com/;
> but I quite dislike this pattern of including a pointer like this where
> it's not needed just for the practicalities of memory management.
>
> I.e. here you use DIR_INIT. In my local patches to fix up the wider
> memory leaks in this area I've got DIR_INIT also using a STRBUF_INIT,
> and DIR_INIT will in turn be referenced by a
> UNPACK_TREES_OPTIONS_INIT. It's quite nice if you're having to
> initialize with "UNPACK_TREES_OPTIONS_INIT" have that initialization
> work all the way down the chain, and not need e.g. a manual
> strbuf_init(), dir_init() etc.

> course you need some "release()" method for the
> UNPACK_TREES_OPTIONS_INIT, which in turn needs to call the dir_release()
> (well, "dir_clear()" in that case), and it needs to call
> "strbuf_release()". It's just nicer if that boilerplate is all on
> destruction, but not also on struct/object setup.

> We do need that setup in some cases (although a lot could just be
> replaced by lazy initialization), but if we don't....
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index a7e1712d236..de5cc6cd025 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -1694,15 +1694,12 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>         static struct cache_entry *dfc;
>         struct pattern_list pl;
>         int free_pattern_list = 0;
> -       struct dir_struct dir = DIR_INIT;
>
>         if (o->reset == UNPACK_RESET_INVALID)
>                 BUG("o->reset had a value of 1; should be UNPACK_TREES_*_UNTRACKED");
>
>         if (len > MAX_UNPACK_TREES)
>                 die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES);
> -       if (o->dir)
> -               BUG("o->dir is for internal use only");

>         trace_performance_enter();
>         trace2_region_enter("unpack_trees", "unpack_trees", the_repository);
> @@ -1718,9 +1715,8 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>                 BUG("UNPACK_RESET_OVERWRITE_UNTRACKED incompatible with preserved ignored files");
>
>         if (!o->preserve_ignored) {
> -               o->dir = &dir;
> -               o->dir->flags |= DIR_SHOW_IGNORED;
> -               setup_standard_excludes(o->dir);
> +               o->dir.flags |= DIR_SHOW_IGNORED;
> +               setup_standard_excludes(&o->dir);
>         }
>
>         if (!core_apply_sparse_checkout || !o->update)
> @@ -1884,10 +1880,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>  done:
>         if (free_pattern_list)
>                 clear_pattern_list(&pl);
> -       if (o->dir) {
> -               dir_clear(o->dir);
> -               o->dir = NULL;
> -       }
> +       dir_clear(&o->dir);

>         trace2_region_leave("unpack_trees", "unpack_trees", the_repository);
>         trace_performance_leave("unpack_trees");
>         return ret;
> @@ -2153,8 +2146,7 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,
>         pathbuf = xstrfmt("%.*s/", namelen, ce->name);
>
>         memset(&d, 0, sizeof(d));
> -       if (o->dir)
> -               d.exclude_per_dir = o->dir->exclude_per_dir;
> +       d.exclude_per_dir = o->dir.exclude_per_dir;
>         i = read_directory(&d, o->src_index, pathbuf, namelen+1, NULL);
>         if (i)
>                 return add_rejected_path(o, ERROR_NOT_UPTODATE_DIR, ce->name);
> @@ -2201,8 +2193,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 (o->dir &&
> -           is_excluded(o->dir, o->src_index, name, &dtype))
> +       if (is_excluded(&o->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 71ffb7eeb0c..a8afbb20170 100644
> --- a/unpack-trees.h
> +++ b/unpack-trees.h
> @@ -5,6 +5,7 @@
>  #include "strvec.h"
>  #include "string-list.h"
>  #include "tree-walk.h"
> +#include "dir.h"
>
>  #define MAX_UNPACK_TREES MAX_TRAVERSE_TREES
>
> @@ -95,7 +96,7 @@ struct unpack_trees_options {
>         struct index_state result;
>
>         struct pattern_list *pl; /* for internal use */
> -       struct dir_struct *dir; /* for internal use only */
> +       struct dir_struct dir; /* for internal use only */
>         struct checkout_metadata meta;
>  };
>

> On Thu, Sep 30, 2021 at 7:15 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>> On Wed, Sep 29 2021, Elijah Newren wrote:
>>
>> > On Wed, Sep 29, 2021 at 11:32 AM Ævar Arnfjörð Bjarmason
>> > <avarab@gmail.com> wrote:
>> >>
>> >> On Wed, Sep 29 2021, Elijah Newren wrote:
>> >>
> ...
>> > As per the next patch:
>> >
>> > int unpack_trees(..., struct unpack_trees_options *o)
>> > {
>> >     struct dir_struct dir = DIR_INIT;
>> >     ...
>> >     if (!o->preserve_ignored) {
>> >         /* Setup 'dir', make o->dir point to it */
>> >         ....
>> >         o->dir = &dir;
>> >     }
>> >     ...
>> >     if (o->dir)
>> >         /* cleanup */
>> >     ....
>> > }
>> >
>> > The caller doesn't touch o->dir (other than initializing it to zeros);
>> > unpack_trees() is wholly responsible for it.  I'd kind of like to
>> > entirely remove dir from unpack_trees_options(), but I need a way of
>> > passing it down through all the other functions in unpack-trees.c, and
>> > leaving it in unpack_trees_options seems the easiest way to do so.  So
>> > I just marked it as "for internal use only".
>>
>> I think I understand *how* it works, I'm puzzled by why you went for
>> this whole level of indirection when you're using a struct on the stack
>> in the end anyway, just ... put that in "struct unpack_trees_options"?
>>
>> Anyway, I see I have only myself to blame here, as you added these leak
>> fixes in the v2 in response to some of my offhand comments.
>>
>> FWIW I then went on to do some deeper fixes not just on these leaks but
>> the surrounding leaks, which will be blocked by 2/11 & 05/11 of this
>> topic for a while. I suppose I only have myself to blame :)
>>
>> Below is a patch-on-top that I think makes this whole thing much simpler
>> by doing away with the pointer entirely.
>>
>> I suppose this is also a partial reply to
>> https://lore.kernel.org/git/CABPp-BG_qigBoirMGR-Yk9Niyxt0UmYCEqojsYxbSEarLAmraA@mail.gmail.com/;
>> but I quite dislike this pattern of including a pointer like this where
>> it's not needed just for the practicalities of memory management.
>>
>> I.e. here you use DIR_INIT. In my local patches to fix up the wider
>> memory leaks in this area I've got DIR_INIT also using a STRBUF_INIT,
>> and DIR_INIT will in turn be referenced by a
>> UNPACK_TREES_OPTIONS_INIT. It's quite nice if you're having to
>> initialize with "UNPACK_TREES_OPTIONS_INIT" have that initialization
>> work all the way down the chain, and not need e.g. a manual
>> strbuf_init(), dir_init() etc.
>
> And you can keep using UNPACK_TREES_OPTIONS_INIT, because the
> unpack_trees_opts->dir should be initialized to NULL.

>> I removed the dir_init() in ce93a4c6127 (dir.[ch]: replace dir_init()
>> with DIR_INIT, 2021-07-01),
>
> I might be going on a tangent here, but looking at that patch, I'm
> worried that dir_init() was buggy and that you perpetuated that bug
> with DIR_INIT.  Note that dir_struct has a struct strbuf basebuf
> member, which neither dir_init() or DIR_INIT initialize properly
> (using either strbuf_init() or STRBUF_INIT).  As far as I can tell,
> dir.c relies on either strbuf_add() calls to just happen to work with
> this incorrectly initialized strbuf, or else use the strbuf_init()
> call in prep_exclude() to do so, using the following snippet:
>
>     if (!dir->basebuf.buf)
>         strbuf_init(&dir->basebuf, PATH_MAX);
>
> However, earlier in that same function we see
>
>     if (stk->baselen <= baselen &&
>         !strncmp(dir->basebuf.buf, base, stk->baselen))
>             break;
>
> So either that function can never have dir->basebuf.buf be NULL and
> the strbuf_init() is dead code, or else it's possible for us to
> trigger a segfault.  If it's the former, it may just be a ticking time
> bomb that will transform into the latter with some other change,
> because it's not at all obvious to me how dir->basebuf gets
> initialized appropriately to avoid that strncmp call.  Perhaps there
> is some invariant where exclude_stack is only set up by previous calls
> to prep_exclude() and those won't set up exclude_stack until first
> initializing basebuf.  But that really at least deserves a comment
> about how we're abusing basebuf, and would probably be cleaner if we
> initialized basebuf to STRBUF_INIT.

>> course you need some "release()" method for the
>> UNPACK_TREES_OPTIONS_INIT, which in turn needs to call the dir_release()
>> (well, "dir_clear()" in that case), and it needs to call
>> "strbuf_release()". It's just nicer if that boilerplate is all on
>> destruction, but not also on struct/object setup.
>
> The caller should *not* be initializing or tearing down
> unpack_trees_options->dir beyond setting that field to NULL; it should
> then leave it alone.

>> We do need that setup in some cases (although a lot could just be
>> replaced by lazy initialization), but if we don't....
>>
>> diff --git a/unpack-trees.c b/unpack-trees.c
>> index a7e1712d236..de5cc6cd025 100644
>> --- a/unpack-trees.c
>> +++ b/unpack-trees.c
>> @@ -1694,15 +1694,12 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>>         static struct cache_entry *dfc;
>>         struct pattern_list pl;
>>         int free_pattern_list = 0;
>> -       struct dir_struct dir = DIR_INIT;
>>
>>         if (o->reset == UNPACK_RESET_INVALID)
>>                 BUG("o->reset had a value of 1; should be UNPACK_TREES_*_UNTRACKED");
>>
>>         if (len > MAX_UNPACK_TREES)
>>                 die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES);
>> -       if (o->dir)
>> -               BUG("o->dir is for internal use only");
>
> I think this was an important check that you've tossed without
> replacement.  Historically, callers set up and tweaked o->dir with
> various values.  With my patch, we are no longer allowing that..which
> introduces a transition problem -- people might have written or are
> now writing patches that make new calls of unpack_trees() previous to
> this change of mine, but submit them after this change of mine gets
> merged.  Without this check I added, they'd probably just do a
> mechanical `o->dir->` change to `o->dir.` and assume it's good...and
> then possibly have ugly bugs to hunt down.
>
> So, I think it's helpful to have a check that provides an early
> warning that tweaking o->dir is not only no longer required, but also
> no longer allowed.

>>         trace_performance_enter();
>>         trace2_region_enter("unpack_trees", "unpack_trees", the_repository);
>> @@ -1718,9 +1715,8 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>>                 BUG("UNPACK_RESET_OVERWRITE_UNTRACKED incompatible with preserved ignored files");
>>
>>         if (!o->preserve_ignored) {
>> -               o->dir = &dir;
>> -               o->dir->flags |= DIR_SHOW_IGNORED;
>> -               setup_standard_excludes(o->dir);
>> +               o->dir.flags |= DIR_SHOW_IGNORED;
>> +               setup_standard_excludes(&o->dir);
>>         }
>>
>>         if (!core_apply_sparse_checkout || !o->update)
>> @@ -1884,10 +1880,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>>  done:
>>         if (free_pattern_list)
>>                 clear_pattern_list(&pl);
>> -       if (o->dir) {
>> -               dir_clear(o->dir);
>> -               o->dir = NULL;
>> -       }
>> +       dir_clear(&o->dir);
>
> Unconditionally calling dir_clear()...

>>         trace2_region_leave("unpack_trees", "unpack_trees", the_repository);
>>         trace_performance_leave("unpack_trees");
>>         return ret;
>> @@ -2153,8 +2146,7 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,
>>         pathbuf = xstrfmt("%.*s/", namelen, ce->name);
>>
>>         memset(&d, 0, sizeof(d));
>> -       if (o->dir)
>> -               d.exclude_per_dir = o->dir->exclude_per_dir;
>> +       d.exclude_per_dir = o->dir.exclude_per_dir;
>>         i = read_directory(&d, o->src_index, pathbuf, namelen+1, NULL);
>>         if (i)
>>                 return add_rejected_path(o, ERROR_NOT_UPTODATE_DIR, ce->name);
>> @@ -2201,8 +2193,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 (o->dir &&
>> -           is_excluded(o->dir, o->src_index, name, &dtype))
>> +       if (is_excluded(&o->dir, o->src_index, name, &dtype))
>
> Unconditionally calling is_excluded()...

>>                 /*
>>                  * ce->name is explicitly excluded, so it is Ok to
>>                  * overwrite it.
>> diff --git a/unpack-trees.h b/unpack-trees.h
>> index 71ffb7eeb0c..a8afbb20170 100644
>> --- a/unpack-trees.h
>> +++ b/unpack-trees.h
>> @@ -5,6 +5,7 @@
>>  #include "strvec.h"
>>  #include "string-list.h"
>>  #include "tree-walk.h"
>> +#include "dir.h"
>>
>>  #define MAX_UNPACK_TREES MAX_TRAVERSE_TREES
>>
>> @@ -95,7 +96,7 @@ struct unpack_trees_options {
>>         struct index_state result;
>>
>>         struct pattern_list *pl; /* for internal use */
>> -       struct dir_struct *dir; /* for internal use only */
>> +       struct dir_struct dir; /* for internal use only */
>>         struct checkout_metadata meta;
>>  };
>>
>
> Not only did you drop the important safety check that o->dir not be
> setup by the caller (which needs to be reinstated in some form), your
> solution also involves unconditionally calling dir_clear() and
> is_excluded().  It is not clear to me that those calls are safe...and
> that they will continue to be safe in the future.

> Even if it is safe
> and will continue to be, I don't think this should be squashed into my
> patches.  I think it should be a separate patch with its own commit
> message that explicitly calls out this assumption.  Especially since
> this is dir.c, which is an area where attempting to fix one very
> simple little bug results in years of refactoring and fixing all kinds
> of historical messes, sometimes waiting a year and a half for
> responses to RFCs/review requests, and where we have to sometimes just
> give up on attempting to understand the purpose of various bits of
> code and instead rely on the regression tests and hope they are good
> enough.  I still think that dir.c deserves a little warning at the
> top, like the one I suggested in [1].
>
> [1] https://lore.kernel.org/git/CABPp-BFiwzzUgiTj_zu+vF5x20L0=1cf25cHwk7KZQj2YkVzXw@mail.gmail.com/

> Aside about safety: One thing I'll sometimes do when I'm unsure about
> those sorts of fixes is to have my new INIT set a new "sentinel" field
> to "12345" or whatever, then just BUG() out in an entry point in the API
> that you can't avoid calling if it's not set like that, e.g. dir_clear()
> or whatever the setup/work function is.

>
> On Thu, Sep 30 2021, Elijah Newren wrote:
>
> > On Thu, Sep 30, 2021 at 7:15 AM Ævar Arnfjörð Bjarmason
> > <avarab@gmail.com> wrote:
> >>
> >> On Wed, Sep 29 2021, Elijah Newren wrote:
> >>
> >> > On Wed, Sep 29, 2021 at 11:32 AM Ævar Arnfjörð Bjarmason
> >> > <avarab@gmail.com> wrote:
> >> >>
> >> >> On Wed, Sep 29 2021, Elijah Newren wrote:
> >> >>
> > ...
> >> > As per the next patch:
> >> >
> >> > int unpack_trees(..., struct unpack_trees_options *o)
> >> > {
> >> >     struct dir_struct dir = DIR_INIT;
> >> >     ...
> >> >     if (!o->preserve_ignored) {
> >> >         /* Setup 'dir', make o->dir point to it */
> >> >         ....
> >> >         o->dir = &dir;
> >> >     }
> >> >     ...
> >> >     if (o->dir)
> >> >         /* cleanup */
> >> >     ....
> >> > }
> >> >
> >> > The caller doesn't touch o->dir (other than initializing it to zeros);
> >> > unpack_trees() is wholly responsible for it.  I'd kind of like to
> >> > entirely remove dir from unpack_trees_options(), but I need a way of
> >> > passing it down through all the other functions in unpack-trees.c, and
> >> > leaving it in unpack_trees_options seems the easiest way to do so.  So
> >> > I just marked it as "for internal use only".
> >>
> >> I think I understand *how* it works, I'm puzzled by why you went for
> >> this whole level of indirection when you're using a struct on the stack
> >> in the end anyway, just ... put that in "struct unpack_trees_options"?
> >>
> >> Anyway, I see I have only myself to blame here, as you added these leak
> >> fixes in the v2 in response to some of my offhand comments.
> >>
> >> FWIW I then went on to do some deeper fixes not just on these leaks but
> >> the surrounding leaks, which will be blocked by 2/11 & 05/11 of this
> >> topic for a while. I suppose I only have myself to blame :)
> >>
> >> Below is a patch-on-top that I think makes this whole thing much simpler
> >> by doing away with the pointer entirely.
> >>
> >> I suppose this is also a partial reply to
> >> https://lore.kernel.org/git/CABPp-BG_qigBoirMGR-Yk9Niyxt0UmYCEqojsYxbSEarLAmraA@mail.gmail.com/;
> >> but I quite dislike this pattern of including a pointer like this where
> >> it's not needed just for the practicalities of memory management.
> >>
> >> I.e. here you use DIR_INIT. In my local patches to fix up the wider
> >> memory leaks in this area I've got DIR_INIT also using a STRBUF_INIT,
> >> and DIR_INIT will in turn be referenced by a
> >> UNPACK_TREES_OPTIONS_INIT. It's quite nice if you're having to
> >> initialize with "UNPACK_TREES_OPTIONS_INIT" have that initialization
> >> work all the way down the chain, and not need e.g. a manual
> >> strbuf_init(), dir_init() etc.
> >
> > And you can keep using UNPACK_TREES_OPTIONS_INIT, because the
> > unpack_trees_opts->dir should be initialized to NULL.
>
> But I don't want it initialized to NULL, I want DIR_INIT....

> >> I removed the dir_init() in ce93a4c6127 (dir.[ch]: replace dir_init()
> >> with DIR_INIT, 2021-07-01),
> >
> > I might be going on a tangent here, but looking at that patch, I'm
> > worried that dir_init() was buggy and that you perpetuated that bug
> > with DIR_INIT.  Note that dir_struct has a struct strbuf basebuf
> > member, which neither dir_init() or DIR_INIT initialize properly
> > (using either strbuf_init() or STRBUF_INIT).  As far as I can tell,
> > dir.c relies on either strbuf_add() calls to just happen to work with
> > this incorrectly initialized strbuf, or else use the strbuf_init()
> > call in prep_exclude() to do so, using the following snippet:
> >
> >     if (!dir->basebuf.buf)
> >         strbuf_init(&dir->basebuf, PATH_MAX);
> >
> > However, earlier in that same function we see
> >
> >     if (stk->baselen <= baselen &&
> >         !strncmp(dir->basebuf.buf, base, stk->baselen))
> >             break;
> >
> > So either that function can never have dir->basebuf.buf be NULL and
> > the strbuf_init() is dead code, or else it's possible for us to
> > trigger a segfault.  If it's the former, it may just be a ticking time
> > bomb that will transform into the latter with some other change,
> > because it's not at all obvious to me how dir->basebuf gets
> > initialized appropriately to avoid that strncmp call.  Perhaps there
> > is some invariant where exclude_stack is only set up by previous calls
> > to prep_exclude() and those won't set up exclude_stack until first
> > initializing basebuf.  But that really at least deserves a comment
> > about how we're abusing basebuf, and would probably be cleaner if we
> > initialized basebuf to STRBUF_INIT.
>
> ...because yes, I forgot about that when sending you the diff-on-top,
> sorry. Yes that's buggy with the diff-on-top I sent you.

> I've got that fixed in the version I have. I.e. first I add a
> UNPACK_TREES_OPTIONS_INIT macro, then deal with that lazy initialization
> case (at which point DIR_INIT starts initializing that strbuf), then
> change the "dir_struct" from a pointer to embedding it, and finally fix
> a memory leak with that new API.
>
> WIP patches here:
> https://github.com/avar/git/compare/avar/post-sanitize-leak-test-mode-add-and-use-revisions-release...avar/post-sanitize-leak-test-mode-unpack-trees-and-dir

> >> but would probably need to bring it back, of
> >
> > If you need to bring it back, it's unrelated to my changes here, and
> > would only be because of the lack of basebuf initialization above.
>
> Yes, in this case. I mean that generally speaking I think it's a good
> pattern to use to have structs be initialized by macros like this,
> because it means you can embed them N levels deep (as we sometimes do)
> without having to call functions to initialize them.
>
> So yes, in this case as long as DIR_INIT is { 0 } it doesn't matter, but
> it does as soon as it has a member that needs initialization, and
> generally speaking for any FOO_INIT that needs a BAR_INIT ...

> >> course you need some "release()" method for the
> >> UNPACK_TREES_OPTIONS_INIT, which in turn needs to call the dir_release()
> >> (well, "dir_clear()" in that case), and it needs to call
> >> "strbuf_release()". It's just nicer if that boilerplate is all on
> >> destruction, but not also on struct/object setup.
> >
> > The caller should *not* be initializing or tearing down
> > unpack_trees_options->dir beyond setting that field to NULL; it should
> > then leave it alone.
>
> s/NULL/DIR_INIT/ in my version, but yes.
>
> >> We do need that setup in some cases (although a lot could just be
> >> replaced by lazy initialization), but if we don't....
> >>
> >> diff --git a/unpack-trees.c b/unpack-trees.c
> >> index a7e1712d236..de5cc6cd025 100644
> >> --- a/unpack-trees.c
> >> +++ b/unpack-trees.c
> >> @@ -1694,15 +1694,12 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
> >>         static struct cache_entry *dfc;
> >>         struct pattern_list pl;
> >>         int free_pattern_list = 0;
> >> -       struct dir_struct dir = DIR_INIT;
> >>
> >>         if (o->reset == UNPACK_RESET_INVALID)
> >>                 BUG("o->reset had a value of 1; should be UNPACK_TREES_*_UNTRACKED");
> >>
> >>         if (len > MAX_UNPACK_TREES)
> >>                 die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES);
> >> -       if (o->dir)
> >> -               BUG("o->dir is for internal use only");
> >
> > I think this was an important check that you've tossed without
> > replacement.  Historically, callers set up and tweaked o->dir with
> > various values.  With my patch, we are no longer allowing that..which
> > introduces a transition problem -- people might have written or are
> > now writing patches that make new calls of unpack_trees() previous to
> > this change of mine, but submit them after this change of mine gets
> > merged.  Without this check I added, they'd probably just do a
> > mechanical `o->dir->` change to `o->dir.` and assume it's good...and
> > then possibly have ugly bugs to hunt down.
> >
> > So, I think it's helpful to have a check that provides an early
> > warning that tweaking o->dir is not only no longer required, but also
> > no longer allowed.
>
> The compiler will catch any such use of the pointer version on a
> mis-merge, or do you just mean that the person running into that might
> get the resolution wrong? I.e. before we could check o->dir being NULL
> for "do we have an exclude", but &o->dir will always be true?

> >>         trace_performance_enter();
> >>         trace2_region_enter("unpack_trees", "unpack_trees", the_repository);
> >> @@ -1718,9 +1715,8 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
> >>                 BUG("UNPACK_RESET_OVERWRITE_UNTRACKED incompatible with preserved ignored files");
> >>
> >>         if (!o->preserve_ignored) {
> >> -               o->dir = &dir;
> >> -               o->dir->flags |= DIR_SHOW_IGNORED;
> >> -               setup_standard_excludes(o->dir);
> >> +               o->dir.flags |= DIR_SHOW_IGNORED;
> >> +               setup_standard_excludes(&o->dir);
> >>         }
> >>
> >>         if (!core_apply_sparse_checkout || !o->update)
> >> @@ -1884,10 +1880,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
> >>  done:
> >>         if (free_pattern_list)
> >>                 clear_pattern_list(&pl);
> >> -       if (o->dir) {
> >> -               dir_clear(o->dir);
> >> -               o->dir = NULL;
> >> -       }
> >> +       dir_clear(&o->dir);
> >
> > Unconditionally calling dir_clear()...
>
> As before I'm not sure about bugs in the ad-hoc patch on top, but I
> don't think this is a bug in my version linked above.
>
> I.e. it's zero'd out, and the dir_clear() either ends up calling
> free(NULL) or tries to loop over 0..N where N will be 0, no?
>
> >>         trace2_region_leave("unpack_trees", "unpack_trees", the_repository);
> >>         trace_performance_leave("unpack_trees");
> >>         return ret;
> >> @@ -2153,8 +2146,7 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,
> >>         pathbuf = xstrfmt("%.*s/", namelen, ce->name);
> >>
> >>         memset(&d, 0, sizeof(d));
> >> -       if (o->dir)
> >> -               d.exclude_per_dir = o->dir->exclude_per_dir;
> >> +       d.exclude_per_dir = o->dir.exclude_per_dir;
> >>         i = read_directory(&d, o->src_index, pathbuf, namelen+1, NULL);
> >>         if (i)
> >>                 return add_rejected_path(o, ERROR_NOT_UPTODATE_DIR, ce->name);
> >> @@ -2201,8 +2193,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 (o->dir &&
> >> -           is_excluded(o->dir, o->src_index, name, &dtype))
> >> +       if (is_excluded(&o->dir, o->src_index, name, &dtype))
> >
> > Unconditionally calling is_excluded()...
>
> Which will just return "it's not", won't it? Just lik dir_clear() deals
> with an "empty" dir_struct. There's existing callers of both with that
> pattern in e.g. builtin/{add,clean}.c.

> >>                 /*
> >>                  * ce->name is explicitly excluded, so it is Ok to
> >>                  * overwrite it.
> >> diff --git a/unpack-trees.h b/unpack-trees.h
> >> index 71ffb7eeb0c..a8afbb20170 100644
> >> --- a/unpack-trees.h
> >> +++ b/unpack-trees.h
> >> @@ -5,6 +5,7 @@
> >>  #include "strvec.h"
> >>  #include "string-list.h"
> >>  #include "tree-walk.h"
> >> +#include "dir.h"
> >>
> >>  #define MAX_UNPACK_TREES MAX_TRAVERSE_TREES
> >>
> >> @@ -95,7 +96,7 @@ struct unpack_trees_options {
> >>         struct index_state result;
> >>
> >>         struct pattern_list *pl; /* for internal use */
> >> -       struct dir_struct *dir; /* for internal use only */
> >> +       struct dir_struct dir; /* for internal use only */
> >>         struct checkout_metadata meta;
> >>  };
> >>
> >
> > Not only did you drop the important safety check that o->dir not be
> > setup by the caller (which needs to be reinstated in some form), your
> > solution also involves unconditionally calling dir_clear() and
> > is_excluded().  It is not clear to me that those calls are safe...and
> > that they will continue to be safe in the future.
>
> It is a common pattern we rely on, e.g. strbuf_release() and various
> other custom free-like functions generally act as NOOP if they've got
> nothing to do, just like free()...

> > Even if it is safe
> > and will continue to be, I don't think this should be squashed into my
> > patches.  I think it should be a separate patch with its own commit
> > message that explicitly calls out this assumption.  Especially since
> > this is dir.c, which is an area where attempting to fix one very
> > simple little bug results in years of refactoring and fixing all kinds
> > of historical messes, sometimes waiting a year and a half for
> > responses to RFCs/review requests, and where we have to sometimes just
> > give up on attempting to understand the purpose of various bits of
> > code and instead rely on the regression tests and hope they are good
> > enough.  I still think that dir.c deserves a little warning at the
> > top, like the one I suggested in [1].
> >
> > [1] https://lore.kernel.org/git/CABPp-BFiwzzUgiTj_zu+vF5x20L0=1cf25cHwk7KZQj2YkVzXw@mail.gmail.com/
>
> *nod* I can always submit something like this afterwards.

> Just on this series: Perhaps this discussion is a sign that this memory
> leak fixing should be its own cleanup series where we could hash out any
> approaches to doing that? I.e. as noted before I realize I'm to blame
> for suggesting it in the first place, but those parts of these changes
> don't seem like they're needed by other parts of the series (I tried
> compiling with the two relevant patches ejected out).
>
> Having a humongous set of memory leak fixes locally at this point, I
> think it's generally not very worth the effort to fix a leak in b()
> where a() calls b() and b() calls c(), and all of [abc]() are
> leaking. I.e. often narrowly fixing leaks in b() will lead to different
> solutions than if you're trying to resolve all of [abc](), as their
> interaction comes into play.

> Aside about safety: One thing I'll sometimes do when I'm unsure about
> those sorts of fixes is to have my new INIT set a new "sentinel" field
> to "12345" or whatever, then just BUG() out in an entry point in the API
> that you can't avoid calling if it's not set like that, e.g. dir_clear()
> or whatever the setup/work function is.
>
> We don't have 100% test coverage, but we usually have at least *some*,
> and doing that is good about catching e.g. a memset() at a distance, as
> happens in this code with the merge code embedding the relevant struct
> and memsetting it, which might be missed in some migration of just a
> grep for "struct dir_struct" or whatever...

> On Fri, Oct 1, 2021 at 1:47 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>>
>> On Thu, Sep 30 2021, Elijah Newren wrote:
>>
>> > On Thu, Sep 30, 2021 at 7:15 AM Ævar Arnfjörð Bjarmason
>> > <avarab@gmail.com> wrote:
>> >>
>> >> On Wed, Sep 29 2021, Elijah Newren wrote:
>> >>
>> >> > On Wed, Sep 29, 2021 at 11:32 AM Ævar Arnfjörð Bjarmason
>> >> > <avarab@gmail.com> wrote:
>> >> >>
>> >> >> On Wed, Sep 29 2021, Elijah Newren wrote:
>> >> >>
>> > ...
>> >> > As per the next patch:
>> >> >
>> >> > int unpack_trees(..., struct unpack_trees_options *o)
>> >> > {
>> >> >     struct dir_struct dir = DIR_INIT;
>> >> >     ...
>> >> >     if (!o->preserve_ignored) {
>> >> >         /* Setup 'dir', make o->dir point to it */
>> >> >         ....
>> >> >         o->dir = &dir;
>> >> >     }
>> >> >     ...
>> >> >     if (o->dir)
>> >> >         /* cleanup */
>> >> >     ....
>> >> > }
>> >> >
>> >> > The caller doesn't touch o->dir (other than initializing it to zeros);
>> >> > unpack_trees() is wholly responsible for it.  I'd kind of like to
>> >> > entirely remove dir from unpack_trees_options(), but I need a way of
>> >> > passing it down through all the other functions in unpack-trees.c, and
>> >> > leaving it in unpack_trees_options seems the easiest way to do so.  So
>> >> > I just marked it as "for internal use only".
>> >>
>> >> I think I understand *how* it works, I'm puzzled by why you went for
>> >> this whole level of indirection when you're using a struct on the stack
>> >> in the end anyway, just ... put that in "struct unpack_trees_options"?
>> >>
>> >> Anyway, I see I have only myself to blame here, as you added these leak
>> >> fixes in the v2 in response to some of my offhand comments.
>> >>
>> >> FWIW I then went on to do some deeper fixes not just on these leaks but
>> >> the surrounding leaks, which will be blocked by 2/11 & 05/11 of this
>> >> topic for a while. I suppose I only have myself to blame :)
>> >>
>> >> Below is a patch-on-top that I think makes this whole thing much simpler
>> >> by doing away with the pointer entirely.
>> >>
>> >> I suppose this is also a partial reply to
>> >> https://lore.kernel.org/git/CABPp-BG_qigBoirMGR-Yk9Niyxt0UmYCEqojsYxbSEarLAmraA@mail.gmail.com/;
>> >> but I quite dislike this pattern of including a pointer like this where
>> >> it's not needed just for the practicalities of memory management.
>> >>
>> >> I.e. here you use DIR_INIT. In my local patches to fix up the wider
>> >> memory leaks in this area I've got DIR_INIT also using a STRBUF_INIT,
>> >> and DIR_INIT will in turn be referenced by a
>> >> UNPACK_TREES_OPTIONS_INIT. It's quite nice if you're having to
>> >> initialize with "UNPACK_TREES_OPTIONS_INIT" have that initialization
>> >> work all the way down the chain, and not need e.g. a manual
>> >> strbuf_init(), dir_init() etc.
>> >
>> > And you can keep using UNPACK_TREES_OPTIONS_INIT, because the
>> > unpack_trees_opts->dir should be initialized to NULL.
>>
>> But I don't want it initialized to NULL, I want DIR_INIT....
>
> Why?  For what purpose?  How does that help anything?  I've seen you
> say that you want it, but I haven't yet seen you state how it helps
> you do anything easier.

> What I really want, though, is not even to have it be a pointer, but
> to avoid exposing internal implementation details inside a struct that
> is meant to convey the public API.  Instead unpack-trees should do
> something similar to merge-ort, where it hides all those internal-only
> details (by e.g. having a void* priv that happens to point to a struct
> unpack_trees_options_internal, the latter of which is only defined in
> unpack_trees.c).  However, I didn't want to go through that work for
> just one member.

> But you've inspired me to check if there are other fields that
> shouldn't be exposed.  Turns out that there is a lot of cruft in
> unpack_trees_options that callers shouldn't be messing with (and which
> isn't at all clear to people trying to use the API): cache_bottom,
> dir, msgs, msgs_to_free, nontrivial_merge, skip_sparse_checkout,
> show_all_errors (!), unpack_rejects, df_conflict_entry, merge_size,
> result, and perhaps pl.  A few of those have gotten slightly
> entangled.  And there may have been others that people just started
> setting because it was an existing field, and now I can't
> differentiate between an intentional API usage passing some kind of
> interesting value and an accidental setting of something meant to be
> internal.
>
> So maybe I'll submit some patches on top that rip these direct members
> out of of unpack_trees_options and push them inside some opaque
> struct.

>> >> I removed the dir_init() in ce93a4c6127 (dir.[ch]: replace dir_init()
>> >> with DIR_INIT, 2021-07-01),
>> >
>> > I might be going on a tangent here, but looking at that patch, I'm
>> > worried that dir_init() was buggy and that you perpetuated that bug
>> > with DIR_INIT.  Note that dir_struct has a struct strbuf basebuf
>> > member, which neither dir_init() or DIR_INIT initialize properly
>> > (using either strbuf_init() or STRBUF_INIT).  As far as I can tell,
>> > dir.c relies on either strbuf_add() calls to just happen to work with
>> > this incorrectly initialized strbuf, or else use the strbuf_init()
>> > call in prep_exclude() to do so, using the following snippet:
>> >
>> >     if (!dir->basebuf.buf)
>> >         strbuf_init(&dir->basebuf, PATH_MAX);
>> >
>> > However, earlier in that same function we see
>> >
>> >     if (stk->baselen <= baselen &&
>> >         !strncmp(dir->basebuf.buf, base, stk->baselen))
>> >             break;
>> >
>> > So either that function can never have dir->basebuf.buf be NULL and
>> > the strbuf_init() is dead code, or else it's possible for us to
>> > trigger a segfault.  If it's the former, it may just be a ticking time
>> > bomb that will transform into the latter with some other change,
>> > because it's not at all obvious to me how dir->basebuf gets
>> > initialized appropriately to avoid that strncmp call.  Perhaps there
>> > is some invariant where exclude_stack is only set up by previous calls
>> > to prep_exclude() and those won't set up exclude_stack until first
>> > initializing basebuf.  But that really at least deserves a comment
>> > about how we're abusing basebuf, and would probably be cleaner if we
>> > initialized basebuf to STRBUF_INIT.
>>
>> ...because yes, I forgot about that when sending you the diff-on-top,
>> sorry. Yes that's buggy with the diff-on-top I sent you.
>
> That bug didn't come from the diff-on-top you sent me, it came from
> the commit already merged to master -- ce93a4c6127  (dir.[ch]: replace
> dir_init() with DIR_INIT, 2021-07-01), merged as part of
> ab/struct-init on Jul 16.

>> I've got that fixed in the version I have. I.e. first I add a
>> UNPACK_TREES_OPTIONS_INIT macro, then deal with that lazy initialization
>> case (at which point DIR_INIT starts initializing that strbuf), then
>> change the "dir_struct" from a pointer to embedding it, and finally fix
>> a memory leak with that new API.
>>
>> WIP patches here:
>> https://github.com/avar/git/compare/avar/post-sanitize-leak-test-mode-add-and-use-revisions-release...avar/post-sanitize-leak-test-mode-unpack-trees-and-dir
>
> Yes, that fixes DIR_INIT nicely.  Looks good!
>
>> >> but would probably need to bring it back, of
>> >
>> > If you need to bring it back, it's unrelated to my changes here, and
>> > would only be because of the lack of basebuf initialization above.
>>
>> Yes, in this case. I mean that generally speaking I think it's a good
>> pattern to use to have structs be initialized by macros like this,
>> because it means you can embed them N levels deep (as we sometimes do)
>> without having to call functions to initialize them.
>>
>> So yes, in this case as long as DIR_INIT is { 0 } it doesn't matter, but
>> it does as soon as it has a member that needs initialization, and
>> generally speaking for any FOO_INIT that needs a BAR_INIT ...
>
> Callers SHOULD NOT call a function to initialize
> unpacked_trees_opts->dir in my patches.  It's a ****BUG**** if they do
> so.  So if you're complaining that my changes force callers to also
> invoke some additional function, then I think you're just not
> understanding my patch.
>
> So, I still see no reason given for wanting opts->dir to be a struct.
> But maybe we can fix this by just removing 'dir' (and several other
> members) from opts, so that callers can't initialize it in any way to
> anything.

>> >> course you need some "release()" method for the
>> >> UNPACK_TREES_OPTIONS_INIT, which in turn needs to call the dir_release()
>> >> (well, "dir_clear()" in that case), and it needs to call
>> >> "strbuf_release()". It's just nicer if that boilerplate is all on
>> >> destruction, but not also on struct/object setup.
>> >
>> > The caller should *not* be initializing or tearing down
>> > unpack_trees_options->dir beyond setting that field to NULL; it should
>> > then leave it alone.
>>
>> s/NULL/DIR_INIT/ in my version, but yes.
>>
>> >> We do need that setup in some cases (although a lot could just be
>> >> replaced by lazy initialization), but if we don't....
>> >>
>> >> diff --git a/unpack-trees.c b/unpack-trees.c
>> >> index a7e1712d236..de5cc6cd025 100644
>> >> --- a/unpack-trees.c
>> >> +++ b/unpack-trees.c
>> >> @@ -1694,15 +1694,12 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>> >>         static struct cache_entry *dfc;
>> >>         struct pattern_list pl;
>> >>         int free_pattern_list = 0;
>> >> -       struct dir_struct dir = DIR_INIT;
>> >>
>> >>         if (o->reset == UNPACK_RESET_INVALID)
>> >>                 BUG("o->reset had a value of 1; should be UNPACK_TREES_*_UNTRACKED");
>> >>
>> >>         if (len > MAX_UNPACK_TREES)
>> >>                 die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES);
>> >> -       if (o->dir)
>> >> -               BUG("o->dir is for internal use only");
>> >
>> > I think this was an important check that you've tossed without
>> > replacement.  Historically, callers set up and tweaked o->dir with
>> > various values.  With my patch, we are no longer allowing that..which
>> > introduces a transition problem -- people might have written or are
>> > now writing patches that make new calls of unpack_trees() previous to
>> > this change of mine, but submit them after this change of mine gets
>> > merged.  Without this check I added, they'd probably just do a
>> > mechanical `o->dir->` change to `o->dir.` and assume it's good...and
>> > then possibly have ugly bugs to hunt down.
>> >
>> > So, I think it's helpful to have a check that provides an early
>> > warning that tweaking o->dir is not only no longer required, but also
>> > no longer allowed.
>>
>> The compiler will catch any such use of the pointer version on a
>> mis-merge, or do you just mean that the person running into that might
>> get the resolution wrong? I.e. before we could check o->dir being NULL
>> for "do we have an exclude", but &o->dir will always be true?
>
> The compiler catches and reports, and then the human sees the error
> and just transliterates "o->dir->" to "o->dir.".  And then it
> compiles, and the person assumes they fixed it correctly, but the
> transliteration was WRONG and has subtle bugs because they had been
> setting up o->dir with special values and something needs to warn them
> that they shouldn't be touching o->dir anymore.  You removed the
> safety check that would have let them know that their straightforward
> transliteration was wrong.  I added that safety check intentionally,
> and don't like seeing it ripped out without a replacement.

>> >>         trace_performance_enter();
>> >>         trace2_region_enter("unpack_trees", "unpack_trees", the_repository);
>> >> @@ -1718,9 +1715,8 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>> >>                 BUG("UNPACK_RESET_OVERWRITE_UNTRACKED incompatible with preserved ignored files");
>> >>
>> >>         if (!o->preserve_ignored) {
>> >> -               o->dir = &dir;
>> >> -               o->dir->flags |= DIR_SHOW_IGNORED;
>> >> -               setup_standard_excludes(o->dir);
>> >> +               o->dir.flags |= DIR_SHOW_IGNORED;
>> >> +               setup_standard_excludes(&o->dir);
>> >>         }
>> >>
>> >>         if (!core_apply_sparse_checkout || !o->update)
>> >> @@ -1884,10 +1880,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>> >>  done:
>> >>         if (free_pattern_list)
>> >>                 clear_pattern_list(&pl);
>> >> -       if (o->dir) {
>> >> -               dir_clear(o->dir);
>> >> -               o->dir = NULL;
>> >> -       }
>> >> +       dir_clear(&o->dir);
>> >
>> > Unconditionally calling dir_clear()...
>>
>> As before I'm not sure about bugs in the ad-hoc patch on top, but I
>> don't think this is a bug in my version linked above.
>>
>> I.e. it's zero'd out, and the dir_clear() either ends up calling
>> free(NULL) or tries to loop over 0..N where N will be 0, no?
>>
>> >>         trace2_region_leave("unpack_trees", "unpack_trees", the_repository);
>> >>         trace_performance_leave("unpack_trees");
>> >>         return ret;
>> >> @@ -2153,8 +2146,7 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,
>> >>         pathbuf = xstrfmt("%.*s/", namelen, ce->name);
>> >>
>> >>         memset(&d, 0, sizeof(d));
>> >> -       if (o->dir)
>> >> -               d.exclude_per_dir = o->dir->exclude_per_dir;
>> >> +       d.exclude_per_dir = o->dir.exclude_per_dir;
>> >>         i = read_directory(&d, o->src_index, pathbuf, namelen+1, NULL);
>> >>         if (i)
>> >>                 return add_rejected_path(o, ERROR_NOT_UPTODATE_DIR, ce->name);
>> >> @@ -2201,8 +2193,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 (o->dir &&
>> >> -           is_excluded(o->dir, o->src_index, name, &dtype))
>> >> +       if (is_excluded(&o->dir, o->src_index, name, &dtype))
>> >
>> > Unconditionally calling is_excluded()...
>>
>> Which will just return "it's not", won't it? Just lik dir_clear() deals
>> with an "empty" dir_struct. There's existing callers of both with that
>> pattern in e.g. builtin/{add,clean}.c.
>
> That's a question that someone writing this patch should investigate
> and document.  I read through is_excluded() and the functions it
> calls, and I _think_ it's possibly correct in the current code.  But
> it's not trivial to verify.  And I'd look a bit closer at it to be
> sure it's correct before making this change.
>
> Yeah, I'm being pretty picky here, but this is dir.c we are dealing with.

>> Maybe I've missed an edge case, but I think the only reason that "o->dir
>> &&" was there was because it was dynamically malloc'd before, but in my
>> version where we'll always have it initialized...
>
> I think after checking the code to verify this, that it deserves being
> mentioned in the commit message (at least given that this is dir.c
> we're talking about).

>> >>                 /*
>> >>                  * ce->name is explicitly excluded, so it is Ok to
>> >>                  * overwrite it.
>> >> diff --git a/unpack-trees.h b/unpack-trees.h
>> >> index 71ffb7eeb0c..a8afbb20170 100644
>> >> --- a/unpack-trees.h
>> >> +++ b/unpack-trees.h
>> >> @@ -5,6 +5,7 @@
>> >>  #include "strvec.h"
>> >>  #include "string-list.h"
>> >>  #include "tree-walk.h"
>> >> +#include "dir.h"
>> >>
>> >>  #define MAX_UNPACK_TREES MAX_TRAVERSE_TREES
>> >>
>> >> @@ -95,7 +96,7 @@ struct unpack_trees_options {
>> >>         struct index_state result;
>> >>
>> >>         struct pattern_list *pl; /* for internal use */
>> >> -       struct dir_struct *dir; /* for internal use only */
>> >> +       struct dir_struct dir; /* for internal use only */
>> >>         struct checkout_metadata meta;
>> >>  };
>> >>
>> >
>> > Not only did you drop the important safety check that o->dir not be
>> > setup by the caller (which needs to be reinstated in some form), your
>> > solution also involves unconditionally calling dir_clear() and
>> > is_excluded().  It is not clear to me that those calls are safe...and
>> > that they will continue to be safe in the future.
>>
>> It is a common pattern we rely on, e.g. strbuf_release() and various
>> other custom free-like functions generally act as NOOP if they've got
>> nothing to do, just like free()...
>
> Yeah, "common pattern" is a useful way to want things to behave, and
> people assuming that in other parts of git's codebase is probably
> fine, but I don't trust dir.c to already behave according to common
> patterns.  My mistrust of it is deep enough that I think someone
> should verify the relevant parts of dir.c do behave that way before
> making changes like this, and then explicitly call it out in the
> commit message when the change is made.  dir.c had some weird dragons
> in it.  And after being repeatedly sucked back into dir.c problems for
> years because of weird side effects, and seeing how many piles of
> nearly-cancelling bugs it had, I don't trust it anymore.

>> > Even if it is safe
>> > and will continue to be, I don't think this should be squashed into my
>> > patches.  I think it should be a separate patch with its own commit
>> > message that explicitly calls out this assumption.  Especially since
>> > this is dir.c, which is an area where attempting to fix one very
>> > simple little bug results in years of refactoring and fixing all kinds
>> > of historical messes, sometimes waiting a year and a half for
>> > responses to RFCs/review requests, and where we have to sometimes just
>> > give up on attempting to understand the purpose of various bits of
>> > code and instead rely on the regression tests and hope they are good
>> > enough.  I still think that dir.c deserves a little warning at the
>> > top, like the one I suggested in [1].
>> >
>> > [1] https://lore.kernel.org/git/CABPp-BFiwzzUgiTj_zu+vF5x20L0=1cf25cHwk7KZQj2YkVzXw@mail.gmail.com/
>>
>> *nod* I can always submit something like this afterwards.
>
> :-)
>
>> Just on this series: Perhaps this discussion is a sign that this memory
>> leak fixing should be its own cleanup series where we could hash out any
>> approaches to doing that? I.e. as noted before I realize I'm to blame
>> for suggesting it in the first place, but those parts of these changes
>> don't seem like they're needed by other parts of the series (I tried
>> compiling with the two relevant patches ejected out).
>>
>> Having a humongous set of memory leak fixes locally at this point, I
>> think it's generally not very worth the effort to fix a leak in b()
>> where a() calls b() and b() calls c(), and all of [abc]() are
>> leaking. I.e. often narrowly fixing leaks in b() will lead to different
>> solutions than if you're trying to resolve all of [abc](), as their
>> interaction comes into play.
>
> The patch we are commenting on isn't a leakfix, and is very much
> integral to the series for reasons other than actual leakfix (which
> occurred two patches before this one).  It's simplifying the API to
> not require a bunch of boilerplate setup of opts->dir and instead
> requiring callers to just set a simple boolean INSTEAD (and only if
> they want non-default behavior, rather than requiring setting
> opts->dir by all those who did want default behavior).  That way, when
> I need to make sure several additional callers who should have gotten
> the default behavior actually get it, they don't have to call a bunch
> of functions to setup and cleanup opts->dir.

> On Fri, Oct 01 2021, Elijah Newren wrote:
>
>> On Fri, Oct 1, 2021 at 1:47 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>>>
>>> On Thu, Sep 30 2021, Elijah Newren wrote:
>>>
>>> > On Thu, Sep 30, 2021 at 7:15 AM Ævar Arnfjörð Bjarmason
>>> > <avarab@gmail.com> wrote:
>>> >>
>>> >> On Wed, Sep 29 2021, Elijah Newren wrote:

>>> > I might be going on a tangent here, but looking at that patch, I'm
>>> > worried that dir_init() was buggy and that you perpetuated that bug
>>> > with DIR_INIT.  Note that dir_struct has a struct strbuf basebuf
>>> > member, which neither dir_init() or DIR_INIT initialize properly
>>> > (using either strbuf_init() or STRBUF_INIT).  As far as I can tell,
>>> > dir.c relies on either strbuf_add() calls to just happen to work with
>>> > this incorrectly initialized strbuf, or else use the strbuf_init()
>>> > call in prep_exclude() to do so, using the following snippet:
>>> >
>>> >     if (!dir->basebuf.buf)
>>> >         strbuf_init(&dir->basebuf, PATH_MAX);
>>> >
>>> > However, earlier in that same function we see
>>> >
>>> >     if (stk->baselen <= baselen &&
>>> >         !strncmp(dir->basebuf.buf, base, stk->baselen))
>>> >             break;
>>> >
>>> > So either that function can never have dir->basebuf.buf be NULL and
>>> > the strbuf_init() is dead code, or else it's possible for us to
>>> > trigger a segfault.  If it's the former, it may just be a ticking time
>>> > bomb that will transform into the latter with some other change,
>>> > because it's not at all obvious to me how dir->basebuf gets
>>> > initialized appropriately to avoid that strncmp call.  Perhaps there
>>> > is some invariant where exclude_stack is only set up by previous calls
>>> > to prep_exclude() and those won't set up exclude_stack until first
>>> > initializing basebuf.  But that really at least deserves a comment
>>> > about how we're abusing basebuf, and would probably be cleaner if we
>>> > initialized basebuf to STRBUF_INIT.
>>>
>>> ...because yes, I forgot about that when sending you the diff-on-top,
>>> sorry. Yes that's buggy with the diff-on-top I sent you.
>>
>> That bug didn't come from the diff-on-top you sent me, it came from
>> the commit already merged to master -- ce93a4c6127  (dir.[ch]: replace
>> dir_init() with DIR_INIT, 2021-07-01), merged as part of
>> ab/struct-init on Jul 16.
>
> Ah, I misunderstood you there. I'll look at that / fix it. Sorry.

>
> On Sat, Oct 02 2021, Ævar Arnfjörð Bjarmason wrote:
>
> > On Fri, Oct 01 2021, Elijah Newren wrote:
> >
> >> On Fri, Oct 1, 2021 at 1:47 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> >>>
> >>> On Thu, Sep 30 2021, Elijah Newren wrote:
> >>>
> >>> > On Thu, Sep 30, 2021 at 7:15 AM Ævar Arnfjörð Bjarmason
> >>> > <avarab@gmail.com> wrote:
> >>> >>
> >>> >> On Wed, Sep 29 2021, Elijah Newren wrote:
> [...]
> >>> > I might be going on a tangent here, but looking at that patch, I'm
> >>> > worried that dir_init() was buggy and that you perpetuated that bug
> >>> > with DIR_INIT.  Note that dir_struct has a struct strbuf basebuf
> >>> > member, which neither dir_init() or DIR_INIT initialize properly
> >>> > (using either strbuf_init() or STRBUF_INIT).  As far as I can tell,
> >>> > dir.c relies on either strbuf_add() calls to just happen to work with
> >>> > this incorrectly initialized strbuf, or else use the strbuf_init()
> >>> > call in prep_exclude() to do so, using the following snippet:
> >>> >
> >>> >     if (!dir->basebuf.buf)
> >>> >         strbuf_init(&dir->basebuf, PATH_MAX);
> >>> >
> >>> > However, earlier in that same function we see
> >>> >
> >>> >     if (stk->baselen <= baselen &&
> >>> >         !strncmp(dir->basebuf.buf, base, stk->baselen))
> >>> >             break;
> >>> >
> >>> > So either that function can never have dir->basebuf.buf be NULL and
> >>> > the strbuf_init() is dead code, or else it's possible for us to
> >>> > trigger a segfault.  If it's the former, it may just be a ticking time
> >>> > bomb that will transform into the latter with some other change,
> >>> > because it's not at all obvious to me how dir->basebuf gets
> >>> > initialized appropriately to avoid that strncmp call.  Perhaps there
> >>> > is some invariant where exclude_stack is only set up by previous calls
> >>> > to prep_exclude() and those won't set up exclude_stack until first
> >>> > initializing basebuf.  But that really at least deserves a comment
> >>> > about how we're abusing basebuf, and would probably be cleaner if we
> >>> > initialized basebuf to STRBUF_INIT.
> >>>
> >>> ...because yes, I forgot about that when sending you the diff-on-top,
> >>> sorry. Yes that's buggy with the diff-on-top I sent you.
> >>
> >> That bug didn't come from the diff-on-top you sent me, it came from
> >> the commit already merged to master -- ce93a4c6127  (dir.[ch]: replace
> >> dir_init() with DIR_INIT, 2021-07-01), merged as part of
> >> ab/struct-init on Jul 16.
> >
> > Ah, I misunderstood you there. I'll look at that / fix it. Sorry.
>
> Just to tie up this loose end: Yes this control flow suck, and I've got
> some patches to unpack-trees.[ch] & dir.[ch] I'm about to submit to fix
> it. But just to comment on the existing behavior of the code, i.e. your
> (above):
>
>     "So either that function can never have dir->basebuf.buf be NULL and
>     the strbuf_init() is dead code, or else it's possible for us to
>     trigger a segfault.".
>
> I hadn't had time to look into it when I said I'd fix it, but now that I
> have I found thath there's nothing to fix, and this code wasn't buggy
> either before or after my ce93a4c6127 (dir.[ch]: replace dir_init() with
> DIR_INIT, 2021-07-01). I.e. we do have the invariant you mentioned.
>
> The dir.[ch] API has always relied on the "struct dir_struct" being
> zero'd out. First with memset() before your eceba532141 (dir: fix
> problematic API to avoid memory leaks, 2020-08-18), and after my
> ce93a4c6127 with the DIR_INIT, which both amount to the same thing.
>
> We both missed a caller that used neither dir_init() nor uses DIR_INIT
> now, but it uses "{ 0 }", so it's always zero'd.
>
> Now, of course it being zero'd *would* segfault if you feed
> "dir->basebuf.buf" to strncmp() as you note above, but that code isn't
> reachable. The structure of that function is (pseudocode):
>
> void prep_exclude(...)
> {
>         struct exclude_stack *stk = NULL;
>         [...]
>
>         while ((stk = dir->exclude_stack) != NULL)
>                 /* the strncmp() against "dir->basebuf.buf" is here */
>
>         /* maybe we'll early return here */
>
>         if (!dir->basebuf.buf)
>                 strbuf_init(&dir->basebuf, PATH_MAX);
>
>         /*
>          * Code that sets dir->exclude_stack to non-NULL for the first
>          * time follows...
>          */
> }
>
> I.e. dir->exclude_stack is *only* referenced in this function and
> dir_clear() (where we also check it for NULL first).
>
> It's state management between calls to prep_exclude(). So that that
> initial while-loop can only be entered the the >1th time prep_exclude()
> is called.
>
> We'll then either have reached that strbuf_init() already, or if we took
> an early return before the strbuf_init() we couldn't have set
> dir->exclude_stack either. So that "dir->basebuf.buf" dereference is
> safe in either case.

> > So maybe I'll submit some patches on top that rip these direct members
> > out of of unpack_trees_options and push them inside some opaque
> > struct.
>
> Sure, that sounds good. I only had a mild objection to doing it in a way
> where you'll need that sort of code I removed in the linked commit in
> prep_exclude() because you were trying not to expose that at any cost,
> including via some *_INIT macro. I.e. if it's private we can just name
> it "priv_*" or have a :
>
>     struct dont_touch_this {
>         struct dir_struct dir;
>     };
>
> Which are both ways of /messaging/ that it's private, and since the
> target audience is just the rest of the git.git codebase I think that
> ultimately something that 1) sends the right message 2) makes accidents
> pretty much impossible suffices. I.e. you don't accidentally introduce a
> new API user accessing a field called "->priv_*" or
> "->private_*". Someone will review those patches...

> On Sat, Oct 2, 2021 at 2:07 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>>
>> On Fri, Oct 01 2021, Elijah Newren wrote:
>>
> ...
>> > So maybe I'll submit some patches on top that rip these direct members
>> > out of of unpack_trees_options and push them inside some opaque
>> > struct.
>>
>> Sure, that sounds good. I only had a mild objection to doing it in a way
>> where you'll need that sort of code I removed in the linked commit in
>> prep_exclude() because you were trying not to expose that at any cost,
>> including via some *_INIT macro. I.e. if it's private we can just name
>> it "priv_*" or have a :
>>
>>     struct dont_touch_this {
>>         struct dir_struct dir;
>>     };
>>
>> Which are both ways of /messaging/ that it's private, and since the
>> target audience is just the rest of the git.git codebase I think that
>> ultimately something that 1) sends the right message 2) makes accidents
>> pretty much impossible suffices. I.e. you don't accidentally introduce a
>> new API user accessing a field called "->priv_*" or
>> "->private_*". Someone will review those patches...
>
> An internal struct with all the members meant to be internal-only
> provides nearly all the advantages that I was going for with the
> opaque struct, while also being a smaller change than what I was
> thinking of doing.  I like that idea; thanks for the suggestion.

>
>
> On Mon, Oct 04 2021, Elijah Newren wrote:
>
> > On Sat, Oct 2, 2021 at 2:07 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> >>
> >> On Fri, Oct 01 2021, Elijah Newren wrote:
> >>
> > ...
> >> > So maybe I'll submit some patches on top that rip these direct members
> >> > out of of unpack_trees_options and push them inside some opaque
> >> > struct.
> >>
> >> Sure, that sounds good. I only had a mild objection to doing it in a way
> >> where you'll need that sort of code I removed in the linked commit in
> >> prep_exclude() because you were trying not to expose that at any cost,
> >> including via some *_INIT macro. I.e. if it's private we can just name
> >> it "priv_*" or have a :
> >>
> >>     struct dont_touch_this {
> >>         struct dir_struct dir;
> >>     };
> >>
> >> Which are both ways of /messaging/ that it's private, and since the
> >> target audience is just the rest of the git.git codebase I think that
> >> ultimately something that 1) sends the right message 2) makes accidents
> >> pretty much impossible suffices. I.e. you don't accidentally introduce a
> >> new API user accessing a field called "->priv_*" or
> >> "->private_*". Someone will review those patches...
> >
> > An internal struct with all the members meant to be internal-only
> > provides nearly all the advantages that I was going for with the
> > opaque struct, while also being a smaller change than what I was
> > thinking of doing.  I like that idea; thanks for the suggestion.
>
> Yeah, just to provide an explicit example something like the below. It
> compiles to the same assembly (at least under -O3, didn't exhaustively
> try other optimization levels).
>
> I'm rather "meh" on it v.s. just prefixing the relevant member names
> with "priv_" or "private_", but it results in the same semantics &
> machine code, so it's effectively just a way of doing the labeling for
> human consumption.
>
> diff --git a/dir.c b/dir.c
> index 39fce3bcba7..a714640e782 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1533,12 +1533,12 @@ static void prep_exclude(struct dir_struct *dir,
>          * which originate from directories not in the prefix of the
>          * path being checked.
>          */
> -       while ((stk = dir->exclude_stack) != NULL) {
> +       while ((stk = dir->private.exclude_stack) != NULL) {
>                 if (stk->baselen <= baselen &&
>                     !strncmp(dir->basebuf.buf, base, stk->baselen))
>                         break;
> -               pl = &group->pl[dir->exclude_stack->exclude_ix];
> -               dir->exclude_stack = stk->prev;
> +               pl = &group->pl[dir->private.exclude_stack->exclude_ix];
> +               dir->private.exclude_stack = stk->prev;
>                 dir->pattern = NULL;
>                 free((char *)pl->src); /* see strbuf_detach() below */
>                 clear_pattern_list(pl);
> @@ -1584,7 +1584,7 @@ static void prep_exclude(struct dir_struct *dir,
>                                                  base + current,
>                                                  cp - base - current);
>                 }
> -               stk->prev = dir->exclude_stack;
> +               stk->prev = dir->private.exclude_stack;
>                 stk->baselen = cp - base;
>                 stk->exclude_ix = group->nr;
>                 stk->ucd = untracked;
> @@ -1605,7 +1605,7 @@ static void prep_exclude(struct dir_struct *dir,
>                             dir->pattern->flags & PATTERN_FLAG_NEGATIVE)
>                                 dir->pattern = NULL;
>                         if (dir->pattern) {
> -                               dir->exclude_stack = stk;
> +                               dir->private.exclude_stack = stk;
>                                 return;
>                         }
>                 }
> @@ -1662,7 +1662,7 @@ static void prep_exclude(struct dir_struct *dir,
>                         invalidate_gitignore(dir->untracked, untracked);
>                         oidcpy(&untracked->exclude_oid, &oid_stat.oid);
>                 }
> -               dir->exclude_stack = stk;
> +               dir->private.exclude_stack = stk;
>                 current = stk->baselen;
>         }
>         strbuf_setlen(&dir->basebuf, baselen);
> @@ -3302,7 +3302,7 @@ void dir_clear(struct dir_struct *dir)
>         free(dir->ignored);
>         free(dir->entries);
>
> -       stk = dir->exclude_stack;
> +       stk = dir->private.exclude_stack;
>         while (stk) {
>                 struct exclude_stack *prev = stk->prev;
>                 free(stk);
> diff --git a/dir.h b/dir.h
> index 83f46c0fb4c..d30d294308d 100644
> --- a/dir.h
> +++ b/dir.h
> @@ -209,6 +209,11 @@ struct untracked_cache {
>   * record the paths discovered. A single `struct dir_struct` is used regardless
>   * of whether or not the traversal recursively descends into subdirectories.
>   */
> +
> +struct dir_struct_private {
> +       struct exclude_stack *exclude_stack;
> +};
> +
>  struct dir_struct {
>
>         /* The number of members in `entries[]` array. */
> @@ -327,7 +332,7 @@ struct dir_struct {
>          * (sub)directory in the traversal. Exclude points to the
>          * matching exclude struct if the directory is excluded.
>          */
> -       struct exclude_stack *exclude_stack;
> +       struct dir_struct_private private;
>         struct path_pattern *pattern;
>         struct strbuf basebuf;

>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > Changes since v2 (all due to Junio's request to consolidate
> > unpack_trees_options.dir handling):
>
> Heh, don't blame me.  I even explicitly said it was merely an
> observation for longer term, not a suggestion to include the first
> step for such a move in this series.

>
> On Mon, Sep 27, 2021 at 1:36 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >
> > > Changes since v2 (all due to Junio's request to consolidate
> > > unpack_trees_options.dir handling):
> >
> > Heh, don't blame me.  I even explicitly said it was merely an
> > observation for longer term, not a suggestion to include the first
> > step for such a move in this series.
>
> Well...the repetitive code for setting up and clearing out
> unpack_trees_options.dir that already existed (and which my series was
> copying to more places) bugged me too, but I was worried that it was a
> bit messy to clean up (and the fact that it took five patches suggests
> it was).  But then you also brought it up as an issue when reviewing,
> so I figured I might as well dive in...

> We have multiple codepaths that delete untracked files/directories but
> shouldn't. There are also some codepaths where we delete untracked
> files/directories intentionally (based on mailing list discussion), but
> where that intent is not documented. We also have some codepaths that
> preserve ignored files, which shouldn't. Fix the documentation, add several
> new (mostly failing) testcases, fix some of the new testcases, and add
> comments about some potential remaining problems. (I found these as a
> side-effect of looking at [1], though [2] pointed out one explicitly while I
> was working on it.)
> 
> Note that I'm using Junio's declaration about checkout -f and reset --hard
> (and also presuming that since read-tree --reset is porcelain that its
> behavior should be left alone)[3] in this series.

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

> On Sun, Oct 3, 2021 at 6:12 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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.

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

> [...]
>   * 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.

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

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

>
> Hi Elijah
>
> On 27/09/2021 17:33, Elijah Newren via GitGitGadget wrote:
> > We have multiple codepaths that delete untracked files/directories but
> > shouldn't. There are also some codepaths where we delete untracked
> > files/directories intentionally (based on mailing list discussion), but
> > where that intent is not documented. We also have some codepaths that
> > preserve ignored files, which shouldn't. Fix the documentation, add several
> > new (mostly failing) testcases, fix some of the new testcases, and add
> > comments about some potential remaining problems. (I found these as a
> > side-effect of looking at [1], though [2] pointed out one explicitly while I
> > was working on it.)
> >
> > Note that I'm using Junio's declaration about checkout -f and reset --hard
> > (and also presuming that since read-tree --reset is porcelain that its
> > behavior should be left alone)[3] in this series.
> >
>
> I've had a read through and I don't have any specific comments, I like
> the way you have simplified adding the standard excludes for callers and
> making the existing value of reset invalid when converting to an enum. I
> think there is a small risk someone will complain about read-tree
> changing how it handles ignored files, but hopefully everyone was just
> passing ".gitignore" to --exclude-per-directory and they wont mind
> 'read-tree -m -u' removing ignored files now.