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

Re: [PATCH 2/3] git add: make -p/-i aware of sparse index

From
Elijah Newren <newren@gmail.com>
Date
May 10, 2025, 04:38 UTC
Message-ID
<CABPp-BEmMaFQxE9NQgM8M=cgfBHY1p56vnBt7R4CfuiXnq++4Q@mail.gmail.com>
In-Reply-To
<63caae876343faa9609516b0797fd9ff19f7a881.1746579321.git.gitgitgadget@gmail.com>

On Tue, May 6, 2025 at 5:55 PM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 7 quoted lines
>
> From: Derrick Stolee <stolee@gmail.com>
>
> It is slow to expand a sparse index in-memory due to parsing of trees.
> We aim to minimize that performance cost when possible. 'git add -p'
> uses 'git apply' child processes to modify the index, but still there
> are some expansions that occur.

still there are some expansions that occur...outside of those child processes? Is that what you're trying to say, or was it something else?

Show 11 quoted lines
> It turns out that control flows out of cmd_add() in the interactive
> cases before the lines that confirm that the builtin is integrated with
> the sparse index. We need to move that earlier to ensure it prevents a
> full index expansion on read.
>
> Add more test cases that confirm that these interactive add options work
> with the sparse index. One interesting aspect here is that the '-i'
> option avoids expanding the sparse index when a sparse directory exists
> on disk while the '-p' option does hit the ensure_full_index() method.
> This leaves some room for improvement, but this case should be atypical
> as users should remain within their sparse-checkout.

It's not clear whether this paragraph is talking about existing state (before the patch) or desired state (after the patch). I think based on the context it's the former, but the last sentence sounds more like a future work direction that makes it very unclear, to me at least.

Show 100 quoted lines
> Signed-off-by: Derrick Stolee <stolee@gmail.com>
> ---
>  builtin/add.c                            |  7 +--
>  t/t1092-sparse-checkout-compatibility.sh | 56 ++++++++++++++++++++++++
>  2 files changed, 60 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index 747511b68bc3..7c292ffdc6c2 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -390,6 +390,10 @@ int cmd_add(int argc,
>
>         argc = parse_options(argc, argv, prefix, builtin_add_options,
>                           builtin_add_usage, PARSE_OPT_KEEP_ARGV0);
> +
> +       prepare_repo_settings(repo);
> +       repo->settings.command_requires_full_index = 0;
> +
>         if (patch_interactive)
>                 add_interactive = 1;
>         if (add_interactive) {
> @@ -426,9 +430,6 @@ int cmd_add(int argc,
>         add_new_files = !take_worktree_changes && !refresh_only && !add_renormalize;
>         require_pathspec = !(take_worktree_changes || (0 < addremove_explicit));
>
> -       prepare_repo_settings(repo);
> -       repo->settings.command_requires_full_index = 0;
> -
>         repo_hold_locked_index(repo, &lock_file, LOCK_DIE_ON_ERROR);
>
>         /*
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index ab8bd371eff3..0dc5dd27184d 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -384,6 +384,38 @@ test_expect_success 'add, commit, checkout' '
>         test_all_match git checkout -
>  '
>
> +test_expect_success 'git add -p' '
> +       init_repos &&
> +
> +       write_script edit-contents <<-\EOF &&
> +       echo text >>$1
> +       EOF
> +
> +       # Does not expand when edits are within sparse checkout.
> +       run_on_all ../edit-contents deep/a &&
> +       run_on_all ../edit-contents deep/deeper1/a &&
> +
> +       test_write_lines y n >in &&
> +       run_on_all git add -p <in &&
> +       test_all_match git status --porcelain=v2 &&
> +       test_all_match git reset &&
> +
> +       test_write_lines u 1 "" q >in &&
> +       run_on_all git add -i <in &&
> +       test_all_match git status --porcelain=v2 &&
> +       test_all_match git reset --hard &&
> +
> +       run_on_sparse mkdir -p folder1 &&
> +       run_on_all ../edit-contents folder1/a &&
> +       test_write_lines y n y >in &&
> +       run_on_all git add -p <in &&
> +       test_sparse_match git status --porcelain=v2 &&
> +       test_sparse_match git reset &&
> +       test_write_lines u 2 3 "" q >in &&
> +       run_on_all git add -i <in &&
> +       test_sparse_match git status --porcelain=v2
> +'
> +
>  test_expect_success 'deep changes during checkout' '
>         init_repos &&
>
> @@ -2391,6 +2423,30 @@ test_expect_success 'sparse-index is not expanded: git apply' '
>         ensure_not_expanded apply --cached ../patch-outside
>  '
>
> +test_expect_success 'sparse-index is not expanded: git add -p' '
> +       init_repos &&
> +
> +       # Does not expand when edits are within sparse checkout.
> +       echo "new content" >sparse-index/deep/a &&
> +       echo "new content" >sparse-index/deep/deeper1/a &&
> +       test_write_lines y n >in &&
> +       ensure_not_expanded add -p <in &&
> +       git -C sparse-index reset &&
> +       ensure_not_expanded add -i <in &&
> +
> +       mkdir -p sparse-index/folder1 &&
> +       echo "new content" >sparse-index/folder1/a &&
> +
> +       # -p does expand when edits are outside sparse checkout.
> +       test_write_lines y n y >in &&
> +       ensure_expanded add -p <in &&
> +
> +       # but -i does not expand.
> +       git -C sparse-index reset &&
> +       test_write_lines u 2 3 "" q >in &&
> +       ensure_not_expanded add -i <in

This has the same error as patch 1, in that you assume your reset above (which wasn't even a reset --hard) will re-sparsify the index. Since it doesn't, your test is misleading and only shows that when already expanded to include the files of interest it doesn't expand any further. To re-sparsify your index before the `add -i` call, you'll need to do a `git reset --hard && git sparse-checkout reapply` and then recreate folder1/a with "new content" again...and then run your 'add -i' command.

Anyway, now I think I understand the expected meaning of the final paragraph of your commit message, but you were tripped up by assuming the index was re-sparsified between your steps when it wasn't.

Previous: Derrick Stolee via GitGitGadgetNext: Derrick Stolee
Message 6 of 22 in “Integrate the sparse index with 'git apply' and 'git add -p/-i'”
  1. 0/3 Integrate the sparse index with 'git apply' and 'git add -p/-i'Derrick Stolee via GitGitGadget, May 7, 2025
  2. 1/3 apply: integrate with the sparse indexDerrick Stolee via GitGitGadget, May 7, 2025
  3. Elijah NewrenMay 10, 2025
  4. Derrick StoleeMay 16, 2025
  5. 2/3 git add: make -p/-i aware of sparse indexDerrick Stolee via GitGitGadget, May 7, 2025
  6. Elijah NewrenMay 10, 2025
  7. Derrick StoleeMay 16, 2025
  8. 3/3 p2000: add performance test for 'git add -p'Derrick Stolee via GitGitGadget, May 7, 2025
  9. Elijah NewrenMay 10, 2025
  10. Junio C HamanoMay 8, 2025
  11. Phillip WoodMay 14, 2025
  12. Derrick StoleeMay 16, 2025
  13. phillip.wood123@gmail.comMay 20, 2025
  14. 0/4 Integrate the sparse index with 'git apply' and interactive add, checkout, and resetDerrick Stolee via GitGitGadget, May 16, 2025
  15. 1/4 apply: integrate with the sparse indexDerrick Stolee via GitGitGadget, May 16, 2025
  16. 2/4 git add: make -p/-i aware of sparse indexDerrick Stolee via GitGitGadget, May 16, 2025
  17. 3/4 reset: integrate sparse index with --patchDerrick Stolee via GitGitGadget, May 16, 2025
  18. Elijah NewrenMay 16, 2025
  19. 4/4 p2000: add performance test for patch-mode commandsDerrick Stolee via GitGitGadget, May 16, 2025
  20. Elijah NewrenMay 16, 2025
  21. Derrick StoleeMay 16, 2025
  22. Junio C HamanoMay 16, 2025

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.