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

[Derrick Stolee <stolee@gmail.com>]

On 5/10/25 12:38 AM, Elijah Newren wrote:
> On Tue, May 6, 2025 at 5:55 PM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> 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?
> 
>> 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.

I'll try to rewrite to make this clearer.

>> +       # -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.
Thanks. I didn't like that this was different. I appreciate your
expertise helping to clarify this issue.

Thanks,
-Stolee