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

From: Victoria Dye <vdye@github.com>
Date: Jan 25, 2023, 18:32 UTC
Message-ID: <42e14dda-cd2b-09df-dea8-246b3fcfac42@github.com>
In-Reply-To: <569043fb-9766-037e-c587-1545c2978e7d@unity3d.com>

William Sprent wrote:

Show 20 quoted lines

> 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.
* 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").
* 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'.

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.

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.