| Message ID | 45bd05a945f034d03555f04a1ba85835482dc591.1632006923.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix various issues around removal of untracked files/directories | expand |
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, aside from that perhaps betteras: opts.reset_keep_untracked = o->force; /* or !o->force, depending... */ > 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).
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. Best Wishes Phillip [1] https://lore.kernel.org/git/20190501101403.20294-1-phillip.wood123@gmail.com/ > merge, > update, > clone, >
On Sun, Sep 19, 2021 at 6:51 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > 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? No, much like other callsites that set this up (e.g. builtin/checkout.c), there isn't a place that frees it. In copying how they worked, I also copied their bugs. ;-) I'm tempted to move this code into setup_unpack_trees_porcelain() and then free it in clear_unpack_trees_porcelain()...though not all callers make use of those functions. Hmm... > > 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 No, it only sets opts.reset_keep_untracked to 1 when o->force is false. If o->force is true, it instead sets opts.reset_nuke_untracked to 1. There's no mistake there. > > 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). Yep.
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. 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_("<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 Mon, Sep 20, 2021 at 11:11 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > > 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. Doh, I was thinking of read-tree --reset -u rather than read-tree -m -u, despite the fact that you explicitly called out (and I even quoted you on) the latter. You are right. > > 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. I won't include it in this series, but I'll throw it on my (long) pile of things to perhaps look at later. Thanks for the suggestions and pointers in your reviews!
diff --git a/builtin/am.c b/builtin/am.c index c79e0167e98..dbe6cbe6a33 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1918,8 +1918,12 @@ 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_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); 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 b5d477919a7..ab0bb4d94f0 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -646,12 +646,20 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o, opts.head_idx = -1; opts.update = worktree; opts.skip_unmerged = !worktree; - opts.reset = 1; + 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)); + 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); 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); + } git_config(git_read_tree_config, NULL); @@ -171,7 +176,7 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix) hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR); prefix_set = opts.prefix ? 1 : 0; - if (1 < opts.merge + opts.reset + prefix_set) + if (1 < opts.merge + opts.reset_keep_untracked + prefix_set) die("Which one? -m, --reset, or --prefix?"); /* @@ -183,7 +188,7 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix) * mode. */ - if (opts.reset || opts.merge || opts.prefix) { + if (opts.reset_keep_untracked || opts.merge || opts.prefix) { if (read_cache_unmerged() && (opts.prefix || opts.merge)) die(_("You need to resolve your current index first")); stage = opts.merge = 1; 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"); + } + if (opts.reset_keep_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); } read_cache_unmerged(); diff --git a/builtin/stash.c b/builtin/stash.c index 8f42360ca91..4ceb3581b47 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -256,7 +256,7 @@ 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_keep_untracked = reset; opts.update = update; opts.fn = oneway_merge; 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; + if (!detach_head) { + unpack_tree_opts.reset_keep_untracked = 1; + 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")); diff --git a/t/t1013-read-tree-submodule.sh b/t/t1013-read-tree-submodule.sh index b6df7444c05..4e485c223ad 100755 --- a/t/t1013-read-tree-submodule.sh +++ b/t/t1013-read-tree-submodule.sh @@ -10,10 +10,10 @@ KNOWN_FAILURE_SUBMODULE_OVERWRITE_IGNORED_UNTRACKED=1 test_submodule_switch_recursing_with_args "read-tree -u -m" -test_submodule_forced_switch_recursing_with_args "read-tree -u --reset" +test_submodule_switch_recursing_with_args "read-tree -u --reset" test_submodule_switch "read-tree -u -m" -test_submodule_forced_switch "read-tree -u --reset" +test_submodule_switch "read-tree -u --reset" test_done diff --git a/t/t2500-untracked-overwriting.sh b/t/t2500-untracked-overwriting.sh index a1a6dfa671e..786ec33d63a 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..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; + trace_performance_enter(); trace2_region_enter("unpack_trees", "unpack_trees", the_repository); @@ -1989,7 +1996,7 @@ static int verify_uptodate_1(const struct cache_entry *ce, */ if ((ce->ce_flags & CE_VALID) || ce_skip_worktree(ce)) ; /* keep checking */ - else if (o->reset || ce_uptodate(ce)) + else if (o->reset_either || ce_uptodate(ce)) return 0; if (!lstat(ce->name, &st)) { @@ -2218,7 +2225,7 @@ 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->reset_nuke_untracked || !o->update) return 0; len = check_leading_path(ce->name, ce_namelen(ce), 0); @@ -2585,7 +2592,7 @@ int twoway_merge(const struct cache_entry * const *src, if (current) { if (current->ce_flags & CE_CONFLICTED) { - if (same(oldtree, newtree) || o->reset) { + if (same(oldtree, newtree) || o->reset_either) { if (!newtree) return deleted_entry(current, current, o); else @@ -2683,7 +2690,7 @@ int oneway_merge(const struct cache_entry * const *src, if (old && same(old, a)) { int update = 0; - if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) && + if (o->reset_either && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) && !(old->ce_flags & CE_FSMONITOR_VALID)) { struct stat st; if (lstat(old->name, &st) || 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 */ merge, update, clone,