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 25, 2022, 08:38 UTC
Message-ID
<CABPp-BHx8gFh717bovO6wCo0RA058=YjNtHhRme+Rxh8GOnxbQ@mail.gmail.com>
In-Reply-To
<4f05fa70209768fb20284bd4018a1364567cad5a.1645742073.git.gitgitgadget@gmail.com>

On Thu, Feb 24, 2022 at 2:34 PM Victoria Dye via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 71 quoted lines
>
> 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");

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.
Show 13 quoted lines
> +                       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?

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.

Show 11 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 Dye via GitGitGadgetNext: Victoria Dye
Message 21 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.