Re: [PATCH 6/8] sparse-index: complete partial expansion

From: Derrick Stolee <derrickstolee@github.com>
To: Victoria Dye <vdye@github.com>,
	Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org
Cc: gitster@pobox.com, shaoxuan.yuan02@gmail.com,
	Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH 6/8] sparse-index: complete partial expansion
Date: Tue, 17 May 2022 09:23:28 -0400	[thread overview]
Message-ID: <f756a125-2d88-3a32-1ae1-652932c663b2@github.com> (raw)
In-Reply-To: <ac3869a5-3592-5408-587a-178afde3f7e9@github.com>

On 5/16/2022 4:38 PM, Victoria Dye wrote:
> Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <dstolee@microsoft.com>

>> +
>> +		/* We now have a sparse directory entry. Should we expand? */
>> +		if (pl &&
>> +		    path_matches_pattern_list(ce->name, ce->ce_namelen,
>> +					      NULL, &dtype,
>> +					      pl, istate) <= 0) {
> 
> If I'm reading this correctly, what this is doing is:
> 
> - if we have a sparse directory entry
> - ...and we're expanding only what matches the pattern list (i.e., not
>   'ensure_full_index')
> - ...and that sparse directory path is either *not matching* or *undecided
>   whether it matches* the pattern list
> - ...then we add the sparse directory to the result index and continue. 
> 
> The part that's confusing me is the "<= 0", which means a return value of
> 'UNDECIDED' from 'path_matches_pattern_list' adds the sparse directory
> as-is. At the moment, it looks like 'UNDECIDED' is only returned if not
> using cone patterns (so it shouldn't make a functional difference at this
> point), but wouldn't that return value indicate that the pattern *may or may
> not* match the path, so we should continue on to 'read_tree_at()'?
> 
> All that to say, should the condition be:
> 
> 		/* We now have a sparse directory entry. Should we expand? */
> 		if (pl &&
> 		    path_matches_pattern_list(ce->name, ce->ce_namelen,
> 					      NULL, &dtype,
> 					      pl, istate) == NOT_MATCHED) {
> 
> to reflect that a sparse directory should only be added to the index if we
> *know* it isn't matched?
> 
> To be clear, this is ultimately a non-functional nit - my question is mostly
> to make sure I understand the intent of the code.

You are 100% correct, and using "== NOT_MATCHED" makes the intention clear,
as well as being more robust to a breakage in path_matches_pattern_list().

Thanks,
-Stolee