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

Re: [PATCH v2 0/7] Sparse index: integrate with 'read-tree'

From
Elijah Newren <newren@gmail.com>
Date
Feb 26, 2022, 08:46 UTC
Message-ID
<CABPp-BHu2sDFiDU=xGEr-MzqbtGwcwu+_m3b3gMWri561JyqWA@mail.gmail.com>
In-Reply-To
<pull.1157.v2.git.1645742073.gitgitgadget@gmail.com>

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

Show 37 quoted lines
>
> Like previous integrations [1] [2], this series allows 'git read-tree' to
> operate using a sparse index.
>
> The first two patches are bugfixes for issues found while implementing the
> 'read-tree' integration:
>
>  * The first (patch 1/7) fixes an edge case in which a repo with no in-cone
>    files or directories would have its root collapsed into a sparse
>    directory; the fix ensures the root directory is never collapsed into a
>    sparse directory.
>  * The second (patch 2/7) corrects the 'git status' reporting of changes
>    nested inside the subdirectory of a sparse directory, ensuring that the
>    modified file (not the subdirectory) is correctly reported as having
>    changes.
>
> The remainder of the series focuses on utilizing the sparse index in 'git
> read-tree'. After some baseline behavior-establishing tests (patch 3/7),
> sparse index usage is trivially enabled (patch 4/7) for 'read-tree' except:
>
>  * usage with '--prefix'
>  * two- and three-way merge
>
> These cases require additional changes in order to work as expected (i.e.,
> outwardly matching non-sparse index sparse-checkout). For the former, the
> sparse index can be enabled as long as the index is expanded when the prefix
> is a directory outside the sparse cone (patch 5/7). For the latter, sparse
> directories that cannot be trivially merged must have their contents merged
> file-by-file, done by recursively traversing the trees represented by the
> sparse directories (patches 6/7 & 7/7).
>
>
> Changes since V1
> ================
>
>  * switched an empty string check from '!strlen(path)' to the
>    slightly-less-expensive '!*path'

I've read over the series. It was a nice read, well motivated, and split up rather nicely. I only had a few small commetns.

I think it'd be nice to insert another patch into the series that throws an error if the argument to --prefix starts with a '/'. That would also allow you to simplify patch 5/7 a little.

Patch 6/7 has the right idea, but I'm worried about one part of it; a test would go a long way towards verifying whether that aspect is handled correctly or whether my concern is warranted.

I had a couple smaller comments on some of the other patches.
Overall, nicely done.
Show 61 quoted lines
> Thanks!
>
>  * Victoria
>
> [1]
> https://lore.kernel.org/git/pull.1109.v2.git.1641924306.gitgitgadget@gmail.com/
> [2]
> https://lore.kernel.org/git/pull.1048.v6.git.1638201164.gitgitgadget@gmail.com/
>
> Victoria Dye (7):
>   sparse-index: prevent repo root from becoming sparse
>   status: fix nested sparse directory diff in sparse index
>   read-tree: expand sparse checkout test coverage
>   read-tree: integrate with sparse index
>   read-tree: narrow scope of index expansion for '--prefix'
>   read-tree: make two-way merge sparse-aware
>   read-tree: make three-way merge sparse-aware
>
>  builtin/read-tree.c                      |  10 +-
>  dir.c                                    |   7 +-
>  t/perf/p2000-sparse-operations.sh        |   1 +
>  t/t1092-sparse-checkout-compatibility.sh | 129 +++++++++++++++++++++++
>  unpack-trees.c                           | 121 ++++++++++++++++++++-
>  wt-status.c                              |   9 ++
>  6 files changed, 268 insertions(+), 9 deletions(-)
>
>
> base-commit: e6ebfd0e8cbbd10878070c8a356b5ad1b3ca464e
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1157%2Fvdye%2Fsparse%2Fread-tree-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1157/vdye/sparse/read-tree-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/1157
>
> Range-diff vs v1:
>
>  1:  90da1f9f33a ! 1:  744668eeece sparse-index: prevent repo root from becoming sparse
>      @@ Commit message
>           non-cone sparse-checkouts), the new check does not cause additional changes
>           to how sparse patterns are applied.
>
>      +    Helped-by: Derrick Stolee <derrickstolee@github.com>
>           Signed-off-by: Victoria Dye <vdye@github.com>
>
>        ## dir.c ##
>      @@ dir.c: static int path_in_sparse_checkout_1(const char *path,
>       +  * patterns, or the patterns are of the wrong type.
>          */
>       - if (init_sparse_checkout_patterns(istate) ||
>      -+ if (!strlen(path) ||
>      ++ if (!*path ||
>       +     init_sparse_checkout_patterns(istate) ||
>             (require_cone_mode &&
>              !istate->sparse_checkout_patterns->use_cone_patterns))
>  2:  c21c9b9be34 = 2:  f0cff03b95d status: fix nested sparse directory diff in sparse index
>  3:  ac42ae21d4a = 3:  ffe0b6aff2b read-tree: expand sparse checkout test coverage
>  4:  5ee193bfa87 = 4:  cb7e0cf419c read-tree: integrate with sparse index
>  5:  bea482b6b28 = 5:  4f05fa70209 read-tree: narrow scope of index expansion for '--prefix'
>  6:  9fdcab038b2 = 6:  94c2aad2f93 read-tree: make two-way merge sparse-aware
>  7:  1502e9acb32 = 7:  c4080e99d6e read-tree: make three-way merge sparse-aware
>
> --
> gitgitgadget
Previous: Victoria Dye via GitGitGadgetNext: Victoria Dye via GitGitGadget
Message 34 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.