From: William Sprent Date: Thu, 26 Jan 2023 14:55:40 GMT Subject: Re: [PATCH v2] ls-tree: add --sparse-filter-oid argument Message-ID: In-Reply-To: <42e14dda-cd2b-09df-dea8-246b3fcfac42@github.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 ")) > > 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(, istate)) { > /* do something with */ > } > > 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.