Re: [PATCH 2/5] add: allow operating on a sparse-only index

From: Derrick Stolee <stolee@gmail.com>
Date: Jul 21, 2021, 22:50 UTC
Message-ID: <1c7170b3-c7e3-8cb6-0cb9-5dc96c7e7ce1@gmail.com>
In-Reply-To: <xmqqfsw7cnsv.fsf@gitster.g>

On 7/21/21 6:19 PM, Junio C Hamano wrote:

Show 14 quoted lines

> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> Disable command_requires_full_index for 'git add'. This does not require
>> any additional removals of ensure_full_index(). The main reason is that
>> 'git add' discovers changes based on the pathspec and the worktree
>> itself. These are then inserted into the index directly, and calls to
>> index_name_pos() or index_file_exists() already call expand_to_path() at
>> the appropriate time to support a sparse-index.
> 
> OK.  With that explained, it still is quite surprising that we only
> need this change (eh, rather, doing this change is safe without
> doing anything else).

Yes, all of the hard work was done by the earlier work to expand a sparse index when we search for a specific path that lands within a sparse directory. See 95e0321 (read-cache: expand on query into sparse-directory entry, 2021-04-01) for the specifics.

Show 8 quoted lines

>> -	# This "git add folder1/a" fails with a warning
>> -	# in the sparse repos, differing from the full
>> -	# repo. This is intentional.
>> -	test_sparse_match test_must_fail git add folder1/a &&
>> -	test_sparse_match test_must_fail git add --refresh folder1/a &&
>> -	test_all_match git status --porcelain=v2 &&
> 
> And nice to see a known limitation lifted.

Thank you for pointing this out. This actually starts to _fail_ now that we allow sparse indexes in 'git add', but it's because the error messages don't match, not that the 'test_must_fail' is violated.

Patch 4 adds a similar test that is then set to work in patch 5. That allows us a clear way to describe the behavior change and to motivate the fix in patch 5. This could be explained better, perhaps by merging Patch 4 into this one. That helps describe how this specific case changes behavior (for the worse) in this patch, but is handled in a careful way later, once the behavior change is documented.

If there is a better way to reorganize these patches, then I could try another approach.

Thanks, -Stolee