From: Phillip Wood Date: Mon, 20 Sep 2021 18:11:32 GMT Subject: Re: [PATCH 2/6] Split unpack_trees 'reset' flag into two for untracked handling Message-ID: In-Reply-To: On 20/09/2021 17:05, Elijah Newren wrote: > On Mon, Sep 20, 2021 at 3:19 AM Phillip Wood wrote: >> >> On 19/09/2021 00:15, Elijah Newren via GitGitGadget wrote: >>> From: Elijah Newren >>> >>> 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 ' which also overwrites untracked files. It does not >> use unpack_trees() so is arguably outside the scope of what you're doing >> here but it might be worth mentioning. > > Oh, that's interesting. Yeah, that's worth mentioning and perhaps digging into. It'd be fantastic to fix that if you have the time and inclination to dig into it. Best Wishes Phillip >>> [...] >>> 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_("/"), >>> N_("read the tree into the index under /"), >>> @@ -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 >> ' where is ahead of we skip the >> rebase and use reset_head() to checkout 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; >> >> >> o->reset_either = o->reset_nuke_untracked | o->reset_keep_untracked >> > > 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. >