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 <inThis 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.