diff mbox series

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

Message ID 4b231e9beb43e4fac6457b9bf86e4c1db39c4238.1679903703.git.gitgitgadget@gmail.com (mailing list archive)
State Accepted
Commit 24fc2cde6448233dbbe1f6695577a49679ccc98b
Headers show
Series builtin/sparse-checkout: add check-rules command | expand

Commit Message

William Sprent March 27, 2023, 7:55 a.m. UTC 
From: William Sprent <williams@unity3d.com>

In preparation for adding a sub-command to 'sparse-checkout' that can be
run in a bare repository, remove the 'NEED_WORK_TREE' flag from its
entry in the 'commands' array of 'git.c'.

To avoid that this changes any behaviour, add calls to
'setup_work_tree()' to all of the 'sparse-checkout' sub-commands and add
tests that verify that 'sparse-checkout <cmd>' still fail with a clear
error message telling the user that the command needs a work tree.

Signed-off-by: William Sprent <williams@unity3d.com>
---
 builtin/sparse-checkout.c          |  6 ++++++
 git.c                              |  2 +-
 t/t1091-sparse-checkout-builtin.sh | 33 ++++++++++++++++++++++++++++++
 3 files changed, 40 insertions(+), 1 deletion(-)

Comments

Junio C Hamano March 27, 2023, 5:51 p.m. UTC | #1 
"William Sprent via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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.
SZEDER Gábor April 7, 2023, 4:16 p.m. UTC | #2 
On Mon, Mar 27, 2023 at 10:51:04AM -0700, Junio C Hamano wrote:
> "William Sprent via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> > 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.

It's easy enough to do in this particular case: there is an
OPT_SUBCOMMAND_F() variant which takes an additional flags parameter,
so we could add a PARSE_OPT_SETUP_WORK_TREE flag, check it in e.g.
parse_subcommand(), and act accordingly if it's set.

However, this wouldn't work when the command has a default operation
mode and is invoked without any subcommands.  And I'm not sure about
doing this in parse-options, because it's about, well, parsing
options, not about doing fancy setup stuff.
Junio C Hamano April 7, 2023, 4:38 p.m. UTC | #3 
SZEDER Gábor <szeder.dev@gmail.com> writes:

>>                 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.
>
> It's easy enough to do in this particular case: there is an
> OPT_SUBCOMMAND_F() variant which takes an additional flags parameter,
> so we could add a PARSE_OPT_SETUP_WORK_TREE flag, check it in e.g.
> parse_subcommand(), and act accordingly if it's set.
>
> However, this wouldn't work when the command has a default operation
> mode and is invoked without any subcommands.  And I'm not sure about
> doing this in parse-options, because it's about, well, parsing
> options, not about doing fancy setup stuff.

Yes, exactly.  What I was imagining was more along the lines of

	parse_opt_subcommand_fn *fn = NULL;
	parse_opt_subcommand_fn *fn_with_setup = NULL;
	options[] = {
		OPT_SUBCOMMAND("list", &fn_with_setup, sparse_checkout_list),
		...
		OPT_SUBCOMMAND("check-rules", &fn, sparse_check_rules),
	};

	parse_options(...);

        if (fn_with_setup) {
		setup_worktree();
		fn = fn_with_setup;
	}
	fn(...);

But of course as a "safety" measure, one options[] array can all
point at the same "fn" variable or parse_options() becomes unhappy,
so the above does not work out of the box.
diff mbox series

Patch

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"));
 
@@ -448,6 +449,7 @@  static int sparse_checkout_init(int argc, const char **argv, const char *prefix)
 		OPT_END(),
 	};
 
+	setup_work_tree();
 	repo_read_index(the_repository);
 
 	init_opts.cone_mode = -1;
@@ -760,6 +762,7 @@  static int sparse_checkout_add(int argc, const char **argv, const char *prefix)
 		OPT_END(),
 	};
 
+	setup_work_tree();
 	if (!core_apply_sparse_checkout)
 		die(_("no sparse-checkout to add to"));
 
@@ -806,6 +809,7 @@  static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
 		OPT_END(),
 	};
 
+	setup_work_tree();
 	repo_read_index(the_repository);
 
 	set_opts.cone_mode = -1;
@@ -855,6 +859,7 @@  static int sparse_checkout_reapply(int argc, const char **argv,
 		OPT_END(),
 	};
 
+	setup_work_tree();
 	if (!core_apply_sparse_checkout)
 		die(_("must be in a sparse-checkout to reapply sparsity patterns"));
 
@@ -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);
diff --git a/git.c b/git.c
index 6171fd6769d..5adc835cf10 100644
--- a/git.c
+++ b/git.c
@@ -583,7 +583,7 @@  static struct cmd_struct commands[] = {
 	{ "show-branch", cmd_show_branch, RUN_SETUP },
 	{ "show-index", cmd_show_index, RUN_SETUP_GENTLY },
 	{ "show-ref", cmd_show_ref, RUN_SETUP },
-	{ "sparse-checkout", cmd_sparse_checkout, RUN_SETUP | NEED_WORK_TREE },
+	{ "sparse-checkout", cmd_sparse_checkout, RUN_SETUP },
 	{ "stage", cmd_add, RUN_SETUP | NEED_WORK_TREE },
 	{ "stash", cmd_stash, RUN_SETUP | NEED_WORK_TREE },
 	{ "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 627267be153..7216267aec7 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -882,4 +882,37 @@  test_expect_success 'by default, non-cone mode will warn on individual files' '
 	grep "pass a leading slash before paths.*if you want a single file" warning
 '
 
+test_expect_success 'setup bare repo' '
+	git clone --bare "file://$(pwd)/repo" bare
+'
+test_expect_success 'list fails outside work tree' '
+	test_must_fail git -C bare sparse-checkout list 2>err &&
+	test_i18ngrep "this operation must be run in a work tree" err
+'
+
+test_expect_success 'add fails outside work tree' '
+	test_must_fail git -C bare sparse-checkout add deeper 2>err &&
+	test_i18ngrep "this operation must be run in a work tree" err
+'
+
+test_expect_success 'set fails outside work tree' '
+	test_must_fail git -C bare sparse-checkout set deeper 2>err &&
+	test_i18ngrep "this operation must be run in a work tree" err
+'
+
+test_expect_success 'init fails outside work tree' '
+	test_must_fail git -C bare sparse-checkout init 2>err &&
+	test_i18ngrep "this operation must be run in a work tree" err
+'
+
+test_expect_success 'reapply fails outside work tree' '
+	test_must_fail git -C bare sparse-checkout reapply 2>err &&
+	test_i18ngrep "this operation must be run in a work tree" err
+'
+
+test_expect_success 'disable fails outside work tree' '
+	test_must_fail git -C bare sparse-checkout disable 2>err &&
+	test_i18ngrep "this operation must be run in a work tree" err
+'
+
 test_done