Re: [PATCH v2 1/2] builtin/sparse-checkout: remove NEED_WORK_TREE flag

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 27, 2023, 17:51 UTC
Message-ID: <xmqqr0taaxrr.fsf@gitster.g>
In-Reply-To: <4b231e9beb43e4fac6457b9bf86e4c1db39c4238.1679903703.git.gitgitgadget@gmail.com>

"William Sprent via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 20 quoted lines

> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> index c3738154918..5fdc3d9aab5 100644
> --- a/builtin/sparse-checkout.c
> +++ b/builtin/sparse-checkout.c
> @@ -57,6 +57,7 @@ static int sparse_checkout_list(int argc, const char **argv, const char *prefix)
>  	char *sparse_filename;
>  	int res;
>  
> +	setup_work_tree();
>  	if (!core_apply_sparse_checkout)
>  		die(_("this worktree is not sparse"));
> ...
> @@ -898,6 +903,7 @@ static int sparse_checkout_disable(int argc, const char **argv,
>  	 * forcibly return to a dense checkout regardless of initial state.
>  	 */
>  
> +	setup_work_tree();
>  	argc = parse_options(argc, argv, prefix,
>  			     builtin_sparse_checkout_disable_options,
>  			     builtin_sparse_checkout_disable_usage, 0);

I am throwing this out not as "we must tackle this ugliness before this patch can proceed" but as "this is ugly, I wish somebody can figure it out in a cleaner way", so do not take this as a blocking comment or objection, but more as something to think about improving if possible as a bonus point.

It really is a shame that we have a nice "dispatch" table at the beginning of the single caller:

        int cmd_sparse_checkout(int argc, const char **argv, const char *prefix)
        {
                parse_opt_subcommand_fn *fn = NULL;
                struct option builtin_sparse_checkout_options[] = {
                        OPT_SUBCOMMAND("list", &fn, sparse_checkout_list),
                        OPT_SUBCOMMAND("init", &fn, sparse_checkout_init),
                        OPT_SUBCOMMAND("set", &fn, sparse_checkout_set),
                        OPT_SUBCOMMAND("add", &fn, sparse_checkout_add),
                        OPT_SUBCOMMAND("reapply", &fn, sparse_checkout_reapply),
                        OPT_SUBCOMMAND("disable", &fn, sparse_checkout_disable),
                        OPT_END(),
                };

yet we have to sprinkle setup_work_tree() to all of these functions' implementation. If we were able to describe which selected ones do not need the setup call, we could let the parse-options API to look up the function and then before calling "fn" we could make the setup call. That would allow us to maintain the subcommands much nicely.

Thanks.