Re: [PATCH v2] ls-tree: add --sparse-filter-oid argument

[William Sprent <williams@unity3d.com>]

On 25/01/2023 19.32, Victoria Dye wrote:
> William Sprent wrote:
>> On 24/01/2023 21.11, Victoria Dye wrote>> I haven't looked at your implementation in detail yet, but I did want to
>>> offer a recommendation in case you hadn't considered it: if you want to
>>> allow the use of patterns from a user-specified specific file, it would be
>>> nice to do it in a way that fully replaces the "default" sparse-checkout
>>> settings at the lowest level (i.e., override the values of
>>> 'core_apply_sparse_checkout', 'core_sparse_checkout_cone', and
>>> 'get_sparse_checkout_filename()'). Doing it that way would both make it
>>> easier for other commands to add a '--sparse-patterns' option, and avoid the
>>> separate code path ('path_in_sparse_checkout_1()' vs.
>>> 'recursively_match_path_with_sparse_patterns()', for example) when dealing
>>> with '.git/info/sparse-checkout' patterns vs. manually-specified patterns.
>>>
>>
>> Thanks for the pointers. I'll see what I can do. Do you mean something
>> along the line of the following?
>>
>>     set_sparse_checkout_file(filename);
>>     init_sparse_checkout_patterns(istate);
>>     _ = path_in_sparse_checkout_1(some_path, istate, ...);
>>
> 
> Sort of. I mentioned separating the options into "specify the sparse pattern
> file" and "restrict the displayed files to the active pattern set, if there
> is one". For the former, you might add an option like:
> 
> 	OPT_FILENAME(0, "sparse-patterns", &sparse_pattern_file,
> 		     N_("override sparse-checkout behavior using patterns from <file>"))
> 
> Then do something like what you have, right after option parsing:
> 
> 	if (sparse_pattern_file) {
> 		core_apply_sparse_checkout = 1;
> 		core_sparse_checkout_cone = <???>;
> 		set_sparse_checkout_file(filename);
> 	}
> 
> If this option is specified, but the repo already has sparse
> patterns/settings of its own, you'll need to (carefully) override the repo's
> existing configuration:
> 
> * 'core_apply_sparse_checkout' & 'core_sparse_checkout_cone' are set based
>    on the repo config. You'll need to make sure those values are overridden
>    before loading the sparse-checkout patterns, and also that they're set
>    *after* loading the config.

This sounds a bit easy to get wrong to me, but I assume I can trust that the
config has been loaded by the time 'cmd_ls_tree()' is invoked.

> * Speaking of 'core_sparse_checkout_cone', there are a bunch of ways you
>    might configure "cone-" vs. "non-cone" for your patterns (user-specified
>    with yet another option, always assume one or the other, try deriving from
>    the patterns). My preference would be to always assume "cone mode" - it's
>    the direction Git has been moving with sparse-checkout over the past year,
>    and still automatically "falls back" on non-cone patterns if the patterns
>    can't be used in cone mode (with a warning from
>    'add_pattern_to_hashsets()': "disabling cone pattern matching").

Alright. I've had similar thoughts. But I ended up deciding to respect the
config value since there wouldn't be any way for the user to silence the warning
when passing non-cone mode patterns to the command. I don't feel too strongly about
it, though.

> * If the repo is using a sparse index, the existing sparse directories may
>    not be compatible with the custom patterns. Probably easiest to force use
>    of a full index, e.g. with 'command_requires_full_index = 1'.
> 

OK.

> Fair warning: this probably isn't an exhaustive list of things that would
> need updating, and it'll need thorough testing to make sure there are no
> regressions. But, extending the underlying sparse-checkout infrastructure
> will (ideally) avoid duplicating code and make this behavior reusable across
> other commands.
> 

Alright. I'll give it a shot.

> For the other desired behavior ("limit the files to the active
> sparse-checkout patterns"), you could add an option:
> 
> 	OPT_CALLBACK_F(0, "scope", &sparse_scope, "(sparse|all)",
> 		       N_("specify the scope of results with respect to the sparse-checkout"),
> 		       PARSE_OPT_NONEG, option_parse_scope),
> 
> ...whose callback parses the string arg into a 'restrict_scope'
> boolean/enum/something. Then, wherever in 'ls-files' a tree or the index are
> iterated over, you can gate the per-file operation on:
> 
> 	if (!restrict_scope || path_in_sparse_checkout(<path>, istate)) {
> 		/* do something with <path> */
> 	}
> 
> Note that you should use 'path_in_sparse_checkout()', *not* the
> internal/private function 'path_in_sparse_checkout_1()'; you also don't need
> to explicitly 'init_sparse_checkout_patterns()'. Regardless of whether you
> specified custom patterns or are using the ones in
> '.git/info/sparse-checkout', 'path_in_sparse_checkout()' will initialize &
> use the appropriate patterns.
> 

Yeah. And I wouldn't need the refactor of 'path_in_sparse_checkout_1()'.

Thanks a lot (again) for the pointers.