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.
