git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 2/6] Change unpack_trees' 'reset' flag into an enum

From
Elijah Newren <newren@gmail.com>
Date
Sep 26, 2021, 06:50 UTC
Message-ID
<CABPp-BHQif-tRkG8tB7V8B41jFesVbnCK1tsrTZNXg0_WaOcbg@mail.gmail.com>
In-Reply-To
<xmqq7df5x4f1.fsf@gitster.g>
On Fri, Sep 24, 2021 at 10:35 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 22 quoted lines
>
> "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?

Sorry, I should have been more careful. Phillip noted this other case that overwrote untracked files and suggested mentioning it; see https://lore.kernel.org/git/acef3628-9542-d777-2534-577de9707e15@gmail.com/ and the links from there. Perhaps I would have been better off mentioning that link and the other links in it.

Show 21 quoted lines
> > 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").

It's a very common pattern...but not universal due to some prevailing weirdness and differences in how commands handle ignored files. But, it looks like that can be fixed up, and then this can be centralized. I've got some patches doing this.

Show 38 quoted lines
> > 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.

I had thought about that, but I was already pretty deep down the rabbit hole of trying to avoid removing the current working directory as a side effect of other commands. I noted in my cover letter the --no-overwrite-ignore options and how they could be used building on this series, and now you bring it up too. And your previous comment about the common pattern for unpack_trees_options.dir forces me to go further down this hole a bit. So, I've got patches that implement a fair amount of this, but doesn't actually add the new --no-overwrite-ignore options or plumb them through. They do, however, add a FIXME comment where a single boolean value could be set in order to make such options work.

Show 74 quoted lines
>
> >       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.

No, when opts.reset == UNPACK_RESET_NUKE_UNTRACKED, unpack_trees() has no way to differentiate between untracked and ignored and thus just deletes them all. Setting up unpack_trees_options.dir so that we can differentiate between untracked and ignored files when we are just going to treat them the same (delete them both) is wasted effort. So the check for a certain type of reset value was merely an optimization. (And that optimization didn't apply in the am case, since it doesn't use UNPACK_RESET_NUKE_UNTRACKED.)

However, consolidating the unpack_trees_options.dir handling into unpack_trees() allows me to better document the optimization and only worry about it in one place, which seems to make the code much clearer.

Show 54 quoted lines
>
> > +             /* 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?
Yeah, you're right; I should have used "if (opts.dir)" for all these cases.
Show 42 quoted lines
> > +     }
> > +
> > +     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.
Will fix, though the consolidation ends up making it just one place anyway.
Previous: Junio C HamanoNext: Elijah Newren via GitGitGadget
Message 26 of 82 in “Fix various issues around removal of untracked files/directories”
  1. 0/6 Fix various issues around removal of untracked files/directoriesElijah Newren via GitGitGadget, Sep 18, 2021
  2. 1/6 t2500: add various tests for nuking untracked filesElijah Newren via GitGitGadget, Sep 18, 2021
  3. Ævar Arnfjörð BjarmasonSep 19, 2021
  4. Elijah NewrenSep 20, 2021
  5. 4/6 unpack-trees: avoid nuking untracked dir in way of locally deleted fileElijah Newren via GitGitGadget, Sep 18, 2021
  6. Ævar Arnfjörð BjarmasonSep 19, 2021
  7. Elijah NewrenSep 20, 2021
  8. 3/6 unpack-trees: avoid nuking untracked dir in way of unmerged fileElijah Newren via GitGitGadget, Sep 18, 2021
  9. 2/6 Split unpack_trees 'reset' flag into two for untracked handlingElijah Newren via GitGitGadget, Sep 18, 2021
  10. Ævar Arnfjörð BjarmasonSep 19, 2021
  11. Elijah NewrenSep 20, 2021
  12. Phillip WoodSep 20, 2021
  13. Elijah NewrenSep 20, 2021
  14. Phillip WoodSep 20, 2021
  15. Elijah NewrenSep 24, 2021
  16. 6/6 Documentation: call out commands that nuke untracked files/directoriesElijah Newren via GitGitGadget, Sep 18, 2021
  17. Philip OakleySep 19, 2021
  18. Philip OakleySep 19, 2021
  19. Elijah NewrenSep 20, 2021
  20. 5/6 Comment important codepaths regarding nuking untracked files/dirsElijah Newren via GitGitGadget, Sep 18, 2021
  21. Luke DiamandSep 24, 2021
  22. Elijah NewrenSep 24, 2021
  23. 0/6 Fix various issues around removal of untracked files/directoriesElijah Newren via GitGitGadget, Sep 24, 2021
  24. 2/6 Change unpack_trees' 'reset' flag into an enumElijah Newren via GitGitGadget, Sep 24, 2021
  25. Junio C HamanoSep 24, 2021
  26. Elijah NewrenSep 26, 2021
  27. 1/6 t2500: add various tests for nuking untracked filesElijah Newren via GitGitGadget, Sep 24, 2021
  28. 3/6 unpack-trees: avoid nuking untracked dir in way of unmerged fileElijah Newren via GitGitGadget, Sep 24, 2021
  29. 4/6 unpack-trees: avoid nuking untracked dir in way of locally deleted fileElijah Newren via GitGitGadget, Sep 24, 2021
  30. 5/6 Comment important codepaths regarding nuking untracked files/dirsElijah Newren via GitGitGadget, Sep 24, 2021
  31. Eric SunshineSep 24, 2021
  32. Elijah NewrenSep 26, 2021
  33. 6/6 Documentation: call out commands that nuke untracked files/directoriesElijah Newren via GitGitGadget, Sep 24, 2021
  34. 00/11 Fix various issues around removal of untracked files/directoriesElijah Newren via GitGitGadget, Sep 27, 2021
  35. 02/11 checkout, read-tree: fix leak of unpack_trees_options.dirElijah Newren via GitGitGadget, Sep 27, 2021
  36. 03/11 read-tree, merge-recursive: overwrite ignored files by defaultElijah Newren via GitGitGadget, Sep 27, 2021
  37. Jack O'ConnorDec 13, 2021
  38. Elijah NewrenDec 13, 2021
  39. 01/11 t2500: add various tests for nuking untracked filesElijah Newren via GitGitGadget, Sep 27, 2021
  40. 06/11 Remove ignored files by default when they are in the wayElijah Newren via GitGitGadget, Sep 27, 2021
  41. 05/11 unpack-trees: make dir an internal-only structElijah Newren via GitGitGadget, Sep 27, 2021
  42. 04/11 unpack-trees: introduce preserve_ignored to unpack_trees_optionsElijah Newren via GitGitGadget, Sep 27, 2021
  43. Ævar Arnfjörð BjarmasonSep 29, 2021
  44. Elijah NewrenSep 29, 2021
  45. Ævar Arnfjörð BjarmasonSep 29, 2021
  46. Elijah NewrenSep 30, 2021
  47. Ævar Arnfjörð BjarmasonSep 30, 2021
  48. Elijah NewrenOct 1, 2021
  49. Ævar Arnfjörð BjarmasonOct 1, 2021
  50. Ævar Arnfjörð BjarmasonOct 1, 2021
  51. Elijah NewrenOct 1, 2021
  52. Ævar Arnfjörð BjarmasonOct 2, 2021
  53. Ævar Arnfjörð BjarmasonOct 3, 2021
  54. Elijah NewrenOct 4, 2021
  55. Elijah NewrenOct 4, 2021
  56. Ævar Arnfjörð BjarmasonOct 4, 2021
  57. Elijah NewrenOct 4, 2021
  58. 07/11 Change unpack_trees' 'reset' flag into an enumElijah Newren via GitGitGadget, Sep 27, 2021
  59. 08/11 unpack-trees: avoid nuking untracked dir in way of unmerged fileElijah Newren via GitGitGadget, Sep 27, 2021
  60. 09/11 unpack-trees: avoid nuking untracked dir in way of locally deleted fileElijah Newren via GitGitGadget, Sep 27, 2021
  61. 10/11 Comment important codepaths regarding nuking untracked files/dirsElijah Newren via GitGitGadget, Sep 27, 2021
  62. 11/11 Documentation: call out commands that nuke untracked files/directoriesElijah Newren via GitGitGadget, Sep 27, 2021
  63. Junio C HamanoSep 27, 2021
  64. Elijah NewrenSep 27, 2021
  65. Elijah NewrenSep 27, 2021
  66. Phillip WoodSep 30, 2021
  67. 00/10 Fix various issues around removal of untracked files/directoriesÆvar Arnfjörð Bjarmason, Oct 4, 2021
  68. 01/10 t2500: add various tests for nuking untracked filesÆvar Arnfjörð Bjarmason, Oct 4, 2021
  69. 02/10 read-tree, merge-recursive: overwrite ignored files by defaultÆvar Arnfjörð Bjarmason, Oct 4, 2021
  70. 03/10 unpack-trees: introduce preserve_ignored to unpack_trees_optionsÆvar Arnfjörð Bjarmason, Oct 4, 2021
  71. 04/10 unpack-trees: rename "dir" to "private_dir"Ævar Arnfjörð Bjarmason, Oct 4, 2021
  72. 05/10 Remove ignored files by default when they are in the wayÆvar Arnfjörð Bjarmason, Oct 4, 2021
  73. 06/10 Change unpack_trees' 'reset' flag into an enumÆvar Arnfjörð Bjarmason, Oct 4, 2021
  74. 07/10 unpack-trees: avoid nuking untracked dir in way of unmerged fileÆvar Arnfjörð Bjarmason, Oct 4, 2021
  75. 08/10 unpack-trees: avoid nuking untracked dir in way of locally deleted fileÆvar Arnfjörð Bjarmason, Oct 4, 2021
  76. 09/10 Comment important codepaths regarding nuking untracked files/dirsÆvar Arnfjörð Bjarmason, Oct 4, 2021
  77. 10/10 Documentation: call out commands that nuke untracked files/directoriesÆvar Arnfjörð Bjarmason, Oct 4, 2021
  78. Elijah NewrenOct 4, 2021
  79. Ævar Arnfjörð BjarmasonOct 4, 2021
  80. Elijah NewrenOct 5, 2021
  81. Junio C HamanoOct 4, 2021
  82. Elijah NewrenOct 1, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.