Re: [PATCH 2/6] Split unpack_trees 'reset' flag into two for untracked handling

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