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

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

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

quoted

From: Elijah Newren <redacted>

Traditionally, unpack_trees_options->reset was used to signal that it
was okay to delete any untracked files in the way.  This was used by
`git read-tree --reset`, but then started appearing in other places as
well.  However, many of the other uses should not be deleting untracked
files in the way.  Split this into two separate fields:
    reset_nuke_untracked
    reset_keep_untracked
and, since many code paths in unpack_trees need to be followed for both
of these flags, introduce a third one for convenience:
    reset_either
which is simply an or-ing of the other two.

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

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

quoted

Modify existing callers so that
    read-tree --reset

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

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

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

quoted

    reset --hard
    checkout --force

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

I originally started down that path to see what it looked like, but
Junio weighed in and explicitly called out checkout --force as being a
command that should remove untracked files in the way.  See
https://lore.kernel.org/git/xmqqr1e2ejs9.fsf@gitster.g/ (local).  Seems you
also felt that way previously, at
https://lore.kernel.org/git/d4c36a24-b40c-a6ca-7a05-572ab93a0101@gmail.com/ (local)
-- any reason for your change of opinion?

quoted

continue using reset_nuke_untracked, but so that other callers,
including
    am
    checkout without --force
    stash  (though currently dead code; reset always had a value of 0)
    numerous callers from rebase/sequencer to reset_head()
will use the new reset_keep_untracked field.

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

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

quoted

[...]

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

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

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

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

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

quoted

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

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

@@ -70,9 +71,19 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t
              break;
      case HARD:
              opts.update = 1;
-             /* fallthrough */
+             opts.reset_nuke_untracked = 1;
+             break;
+     case MIXED:
+             opts.reset_keep_untracked = 1; /* but opts.update=0, so untracked left alone */
+             break;
      default:
-             opts.reset = 1;
+             BUG("invalid reset_type passed to reset_index");

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

quoted

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

@@ -1,5 +1,6 @@
  #include "git-compat-util.h"
  #include "cache-tree.h"
+#include "dir.h"
  #include "lockfile.h"
  #include "refs.h"
  #include "reset.h"

@@ -57,8 +58,12 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,
      unpack_tree_opts.update = 1;
      unpack_tree_opts.merge = 1;
      init_checkout_metadata(&unpack_tree_opts.meta, switch_to_branch, oid, NULL);
-     if (!detach_head)
-             unpack_tree_opts.reset = 1;

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

quoted

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

@@ -301,7 +301,7 @@ static int check_submodule_move_head(const struct cache_entry *ce,
      if (!sub)
              return 0;

-     if (o->reset)
+     if (o->reset_nuke_untracked)
              flags |= SUBMODULE_MOVE_HEAD_FORCE;

      if (submodule_move_head(ce->name, old_id, new_id, flags))

@@ -1696,6 +1696,13 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
      if (len > MAX_UNPACK_TREES)
              die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES);

+     if (o->reset_nuke_untracked && o->reset_keep_untracked)
+             BUG("reset_nuke_untracked and reset_keep_untracked are incompatible");
+
+     o->reset_either = 0;
+     if (o->reset_nuke_untracked || o->reset_keep_untracked)
+             o->reset_either = 1;

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

Goes away entirely if we adopt your enum suggestion.

quoted

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

@@ -46,7 +46,9 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
  void clear_unpack_trees_porcelain(struct unpack_trees_options *opts);

  struct unpack_trees_options {
-     unsigned int reset,
+     unsigned int reset_nuke_untracked,
+                  reset_keep_untracked,
+                  reset_either, /* internal use only */

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

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