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

Re: [PATCH v2 5/7] read-tree: narrow scope of index expansion for '--prefix'

From
Elijah Newren <newren@gmail.com>
Date
Feb 26, 2022, 07:52 UTC
Message-ID
<CABPp-BE6n2-v7-91b-aXOsMod6d3dA4zZE4nNyzxac+tVpaaCw@mail.gmail.com>
In-Reply-To
<686e9c8e-7c5b-7cbc-4772-92b9754caa07@github.com>
Hi Victoria,
On Fri, Feb 25, 2022 at 12:25 PM Victoria Dye <vdye@github.com> wrote:
Show 89 quoted lines
>
> Elijah Newren wrote:
> > On Thu, Feb 24, 2022 at 2:34 PM Victoria Dye via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >>
> >> From: Victoria Dye <vdye@github.com>
> >>
> >> When 'git read-tree' is provided with a prefix, expand the index only if the
> >> prefix is equivalent to a sparse directory or contained within one. If the
> >> index is not expanded in these cases, 'ce_in_traverse_path' will indicate
> >> that the relevant sparse directory is not in the prefix/traverse path,
> >> skipping past it and not unpacking the appropriate tree(s).
> >>
> >> If the prefix is in-cone, its sparse subdirectories (if any) will be
> >> traversed correctly without index expansion.
> >>
> >> Signed-off-by: Victoria Dye <vdye@github.com>
> >> ---
> >>  builtin/read-tree.c                      |  3 +--
> >>  t/t1092-sparse-checkout-compatibility.sh |  8 ++++++-
> >>  unpack-trees.c                           | 30 ++++++++++++++++++++++++
> >>  3 files changed, 38 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/builtin/read-tree.c b/builtin/read-tree.c
> >> index c2fdbc2657f..a7b7f822281 100644
> >> --- a/builtin/read-tree.c
> >> +++ b/builtin/read-tree.c
> >> @@ -213,8 +213,7 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
> >>         if (opts.merge && !opts.index_only)
> >>                 setup_work_tree();
> >>
> >> -       /* TODO: audit sparse index behavior in unpack_trees */
> >> -       if (opts.skip_sparse_checkout || opts.prefix)
> >> +       if (opts.skip_sparse_checkout)
> >>                 ensure_full_index(&the_index);
> >>
> >>         if (opts.merge) {
> >> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> >> index ae44451a0a9..a404be0a10f 100755
> >> --- a/t/t1092-sparse-checkout-compatibility.sh
> >> +++ b/t/t1092-sparse-checkout-compatibility.sh
> >> @@ -1415,7 +1415,13 @@ test_expect_success 'sparse index is not expanded: read-tree' '
> >>         do
> >>                 ensure_not_expanded read-tree -mu $MERGE_TREES &&
> >>                 ensure_not_expanded reset --hard HEAD || return 1
> >> -       done
> >> +       done &&
> >> +
> >> +       rm -rf sparse-index/deep/deeper2 &&
> >> +       ensure_not_expanded add . &&
> >> +       ensure_not_expanded commit -m "test" &&
> >> +
> >> +       ensure_not_expanded read-tree --prefix=deep/deeper2 -u deepest
> >>  '
> >>
> >>  test_expect_success 'ls-files' '
> >> diff --git a/unpack-trees.c b/unpack-trees.c
> >> index 360844bda3a..dba122a02bb 100644
> >> --- a/unpack-trees.c
> >> +++ b/unpack-trees.c
> >> @@ -1739,6 +1739,36 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
> >>                 setup_standard_excludes(o->dir);
> >>         }
> >>
> >> +       /*
> >> +        * If the prefix is equal to or contained within a sparse directory, the
> >> +        * index needs to be expanded to traverse with the specified prefix.
> >> +        */
> >> +       if (o->prefix && o->src_index->sparse_index) {
> >> +               int prefix_len = strlen(o->prefix);
> >> +
> >> +               while (prefix_len > 0 && o->prefix[prefix_len - 1] == '/')
> >> +                       prefix_len--;
> >> +
> >> +               if (prefix_len > 0) {
> >
> > Is this condition check necessary?  If we want some safety check here,
> > could it instead be something like
> >
> >    if (prefix_len <= 0)
> >        BUG("Broken prefix passed to unpack_trees");
> >
>
> This condition was intended to skip unnecessary computation for the
> (probably unlikely, but still technically valid) case where the prefix is
> the repo root (e.g., '--prefix=/') - because the repo root is represented
> with only directory separator(s), `prefix_len` would be 0 after removing
> trailing '/'. In that scenario, the index won't need to be expanded, so we
> don't need to go looking in the index for that path.

Wow, that doesn't result in an error?!? That surprises me. I never even thought to test such a thing. Clearly, the following related command does give an error:

  git read-tree --prefix=/toplevel/ $TREE
(namely, "error: invalid path '/toplevel/subpath'")

whereas, the reason a single slash is accepted it because git is trying to be forgiving and treat the following two commands the same:

  git read-tree --prefix=subdir/ $TREE
  git read-tree --prefix=subdir $TREE

i.e. it tries to allow the trailing slash to be optional. And, by implementation quirk, making a trailing slash be optional turns out to mean that --prefix=/ is treated the same as no prefix at all, because empty string prefix just happens to give the same behavior as NULL prefix.

I think we should just throw an error if prefix starts with '/'. unpack_trees() can make it be a BUG() (and at the beginning of the function rather than down at this point and only inside some conditional). builtin/read-tree.c, the only thing that ever sets prefix in unpack_trees_options, should die() if it's passed something that starts with a '/'. Having paths start with a '/' is antithetical to how "prefix" is used throughout the codebase, though I guess I can see people making that mistake if they are used to gitignore-style patterns instead.

Show 55 quoted lines
> None of that is particularly clear from reading the patch, though, so I'll
> add a comment & test covering it explicitly.
>
> > and then dedent the following code?  (Or are callers allowed to not
> > sanitize their input before passing to unpack_trees(), meaning that we
> > should use a die() rather than a BUG()?)
> >
> > To test this idea, near the top of unpack_trees(), I added:
> >     if (o->prefix)
> >         assert(*o->prefix && *o->prefix != '/');
> > and reran all tests.  They all ran without hitting that assertion.  FWIW.
> >
> >> +                       struct strbuf ce_prefix = STRBUF_INIT;
> >> +                       strbuf_grow(&ce_prefix, prefix_len + 1);
> >> +                       strbuf_add(&ce_prefix, o->prefix, prefix_len);
> >> +                       strbuf_addch(&ce_prefix, '/');
> >> +
> >> +                       /*
> >> +                        * If the prefix is not inside the sparse cone, then the
> >> +                        * index is explicitly expanded if it is found as a sparse
> >> +                        * directory, or implicitly expanded (by 'index_name_pos')
> >> +                        * if the path is inside a sparse directory.
> >> +                        */
> >> +                       if (!path_in_cone_mode_sparse_checkout(ce_prefix.buf, o->src_index) &&
> >> +                           index_name_pos(o->src_index, ce_prefix.buf, ce_prefix.len) >= 0)
> >
> > style nit: Can you rewrap both the comments and the code at 80 characters?
> >
>
> I couldn't think of a way to wrap the condition that wouldn't make it more
> difficult to read. The best I could come up with was:
>
>                         if (!path_in_cone_mode_sparse_checkout(ce_prefix.buf,
>                                                                o->src_index) &&
>                             index_name_pos(o->src_index,
>                                            ce_prefix.buf,
>                                            ce_prefix.len) >= 0)
>                                 ensure_full_index(o->src_index);
>
>
> which, to me, is a bit hard to parse. Alternatively, though, I can move the
> prefix-checking logic into its own function (kind of like
> 'pathspec_needs_expanded_index(...)' in [1]), in which case I won't need to
> change the current wrapping to keep it under 80 characters.
>
> [1] https://lore.kernel.org/git/822d7344587f698e73abba1ca726c3a905f7b403.1638201164.git.gitgitgadget@gmail.com/
>
> > It took me a bit of playing and testing to understand these two lines.
> > The comment helps, but it's still a bit dense to unpack; somehow I
> > didn't understand that the comment was referring to index_name_pos()'s
> > call to ensure_full_index().  Once I understood that, it all looks
> > good.
> >
>
> Sorry about that, I'll revise to make that clearer.
Thanks.  :-)
Show 12 quoted lines
> >
> >> +                               ensure_full_index(o->src_index);
> >> +
> >> +                       strbuf_release(&ce_prefix);
> >> +               }
> >> +       }
> >> +
> >>         if (!core_apply_sparse_checkout || !o->update)
> >>                 o->skip_sparse_checkout = 1;
> >>         if (!o->skip_sparse_checkout && !o->pl) {
> >> --
> >> gitgitgadget
Previous: Victoria DyeNext: Victoria Dye
Message 23 of 48 in “Sparse index: integrate with 'read-tree'”
  1. 0/7 Sparse index: integrate with 'read-tree'Victoria Dye via GitGitGadget, Feb 23, 2022
  2. 1/7 sparse-index: prevent repo root from becoming sparseVictoria Dye via GitGitGadget, Feb 23, 2022
  3. Derrick StoleeFeb 24, 2022
  4. Victoria DyeFeb 24, 2022
  5. 2/7 status: fix nested sparse directory diff in sparse indexVictoria Dye via GitGitGadget, Feb 23, 2022
  6. 4/7 read-tree: integrate with sparse indexVictoria Dye via GitGitGadget, Feb 23, 2022
  7. 3/7 read-tree: expand sparse checkout test coverageVictoria Dye via GitGitGadget, Feb 23, 2022
  8. 5/7 read-tree: narrow scope of index expansion for '--prefix'Victoria Dye via GitGitGadget, Feb 23, 2022
  9. 6/7 read-tree: make two-way merge sparse-awareVictoria Dye via GitGitGadget, Feb 23, 2022
  10. Elijah NewrenFeb 26, 2022
  11. Victoria DyeFeb 28, 2022
  12. Elijah NewrenMar 1, 2022
  13. 7/7 read-tree: make three-way merge sparse-awareVictoria Dye via GitGitGadget, Feb 23, 2022
  14. Derrick StoleeFeb 24, 2022
  15. 0/7 Sparse index: integrate with 'read-tree'Victoria Dye via GitGitGadget, Feb 24, 2022
  16. 1/7 sparse-index: prevent repo root from becoming sparseVictoria Dye via GitGitGadget, Feb 24, 2022
  17. 2/7 status: fix nested sparse directory diff in sparse indexVictoria Dye via GitGitGadget, Feb 24, 2022
  18. Elijah NewrenFeb 25, 2022
  19. Victoria DyeFeb 28, 2022
  20. 5/7 read-tree: narrow scope of index expansion for '--prefix'Victoria Dye via GitGitGadget, Feb 24, 2022
  21. Elijah NewrenFeb 25, 2022
  22. Victoria DyeFeb 25, 2022
  23. Elijah NewrenFeb 26, 2022
  24. Victoria DyeFeb 28, 2022
  25. 3/7 read-tree: expand sparse checkout test coverageVictoria Dye via GitGitGadget, Feb 24, 2022
  26. Elijah NewrenFeb 26, 2022
  27. Victoria DyeFeb 28, 2022
  28. Ævar Arnfjörð BjarmasonFeb 28, 2022
  29. Victoria DyeFeb 28, 2022
  30. Ævar Arnfjörð BjarmasonFeb 28, 2022
  31. 7/7 read-tree: make three-way merge sparse-awareVictoria Dye via GitGitGadget, Feb 24, 2022
  32. 4/7 read-tree: integrate with sparse indexVictoria Dye via GitGitGadget, Feb 24, 2022
  33. 6/7 read-tree: make two-way merge sparse-awareVictoria Dye via GitGitGadget, Feb 24, 2022
  34. Elijah NewrenFeb 26, 2022
  35. 0/8 Sparse index: integrate with 'read-tree'Victoria Dye via GitGitGadget, Mar 1, 2022
  36. 1/8 sparse-index: prevent repo root from becoming sparseVictoria Dye via GitGitGadget, Mar 1, 2022
  37. 2/8 status: fix nested sparse directory diff in sparse indexVictoria Dye via GitGitGadget, Mar 1, 2022
  38. 3/8 read-tree: explicitly disallow prefixes with a leading '/'Victoria Dye via GitGitGadget, Mar 1, 2022
  39. 8/8 read-tree: make three-way merge sparse-awareVictoria Dye via GitGitGadget, Mar 1, 2022
  40. 6/8 read-tree: narrow scope of index expansion for '--prefix'Victoria Dye via GitGitGadget, Mar 1, 2022
  41. Glen ChooMar 3, 2022
  42. Victoria DyeMar 3, 2022
  43. Glen ChooMar 4, 2022
  44. 7/8 read-tree: make two-way merge sparse-awareVictoria Dye via GitGitGadget, Mar 1, 2022
  45. 5/8 read-tree: integrate with sparse indexVictoria Dye via GitGitGadget, Mar 1, 2022
  46. 4/8 read-tree: expand sparse checkout test coverageVictoria Dye via GitGitGadget, Mar 1, 2022
  47. Elijah NewrenMar 2, 2022
  48. Derrick StoleeMar 2, 2022

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.