Re: [PATCH v3 6/8] attr: be careful about sparse directories

[Derrick Stolee <stolee@gmail.com>]

On 8/19/2021 4:11 AM, Johannes Schindelin wrote:
> On Tue, 17 Aug 2021, Derrick Stolee via GitGitGadget wrote:
>> +	/*
>> +	 * In the case of cone-mode sparse-checkout, getting the
>> +	 * .gitattributes file from a directory is meaningless: all
>> +	 * contained paths will be sparse if the .gitattributes is also
>> +	 * sparse. In the case of a sparse index, it is critical that we
>> +	 * don't go looking for one as it will expand the index.
>> +	 */
>> +	init_sparse_checkout_patterns(istate);
> At first I thought that `init_sparse_checkout_patterns()` is called by
> `path_in_sparse_checkout()` below, and therefore would not be necessary.
> 
> But it is! Without it, we have no way to test whether `use_cone_patterns`
> is set, because, well, it gets set by `init_sparse_checkout_patterns()`.
> 
> Would it therefore make sense to refactor the code to have a
> `path_in_sparse_checkout_cone()` function? Or add a flag
> `only_in_cone_mode` as function parameter to `path_in_sparse_checkout()`?

One way to clean this up as-is would be to replace
 
>> +	if (istate->sparse_checkout_patterns &&
>> +	    istate->sparse_checkout_patterns->use_cone_patterns &&
>> +	    path_in_sparse_checkout(path, istate) == NOT_MATCHED)

with

	if (!path_in_sparse_checkout(path, istate) &&
	    istate->sparse_checkout_patterns->use_cone_patterns)

to get a similar effect. However, it creates wasted pattern-match
checks when not in cone-mode, so you are probably right that a
path_in_cone_mode_sparse_checkout() would be best to short-circuit
in the non-cone-mode case.

This special casing should be rare, so I don't think it is worth
adding a flag to path_in_sparse_checkout() and making those call
sites more complicated.

Thanks,
-Stolee