Re: [PATCH 06/20] submodule--helper: move "is-active" to a test-tool

From: Glen Choo <chooglen@google.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>, git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>,
	"Atharva Raykar" <raykar.ath@gmail.com>,
	"Prathamesh Chavan" <pc44800@gmail.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: Re: [PATCH 06/20] submodule--helper: move "is-active" to a test-tool
Date: Fri, 29 Jul 2022 14:45:19 -0700	[thread overview]
Message-ID: <kl6lv8rfr3tc.fsf@chooglen-macbookpro.roam.corp.google.com> (raw)
In-Reply-To: <patch-06.20-8e4d2b09d56-20220728T161116Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Create a new "test-tool submodule" and move the "is-active" subcommand
> over to it. It was added in 5c2bd8b77ae (submodule--helper: add
> is-active subcommand, 2017-03-16), since
> a452128a36c (submodule--helper: introduce add-config subcommand,
> 2021-08-06) it hasn't been used by git-submodule.sh.
>
> Since we're creating a command dispatch similar to test-tool.c itself
> let's split out the "struct test_cmd" into a new test-tool-utils.h,
> which both this new code and test-tool.c itself can use.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  Makefile                       |  1 +
>  builtin/submodule--helper.c    |  9 ------
>  t/helper/test-submodule.c      | 58 ++++++++++++++++++++++++++++++++++
>  t/helper/test-tool-utils.h     |  9 ++++++
>  t/helper/test-tool.c           |  7 ++--
>  t/helper/test-tool.h           |  1 +
>  t/t7413-submodule-is-active.sh | 32 +++++++++----------
>  7 files changed, 87 insertions(+), 30 deletions(-)
>  create mode 100644 t/helper/test-submodule.c
>  create mode 100644 t/helper/test-tool-utils.h
>
> diff --git a/Makefile b/Makefile
> index 1624471badc..ad7fbd36885 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -785,6 +785,7 @@ TEST_BUILTINS_OBJS += test-strcmp-offset.o
>  TEST_BUILTINS_OBJS += test-string-list.o
>  TEST_BUILTINS_OBJS += test-submodule-config.o
>  TEST_BUILTINS_OBJS += test-submodule-nested-repo-config.o
> +TEST_BUILTINS_OBJS += test-submodule.o
>  TEST_BUILTINS_OBJS += test-subprocess.o
>  TEST_BUILTINS_OBJS += test-trace2.o
>  TEST_BUILTINS_OBJS += test-urlmatch-normalization.o
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 47ed24c6a60..b2fc732b5d8 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2728,14 +2728,6 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
>  	return 0;
>  }
>  
> -static int is_active(int argc, const char **argv, const char *prefix)
> -{
> -	if (argc != 2)
> -		die("submodule--helper is-active takes exactly 1 argument");
> -
> -	return !is_submodule_active(the_repository, argv[1]);
> -}
> -
>  /*
>   * Exit non-zero if any of the submodule names given on the command line is
>   * invalid. If no names are given, filter stdin to print only valid names
> @@ -3313,7 +3305,6 @@ static struct cmd_struct commands[] = {
>  	{"summary", module_summary, 0},
>  	{"push-check", push_check, 0},
>  	{"absorbgitdirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
> -	{"is-active", is_active, 0},
>  	{"check-name", check_name, 0},
>  	{"config", module_config, 0},
>  	{"set-url", module_set_url, 0},
> diff --git a/t/helper/test-submodule.c b/t/helper/test-submodule.c
> new file mode 100644
> index 00000000000..494c6558d9f
> --- /dev/null
> +++ b/t/helper/test-submodule.c
> @@ -0,0 +1,58 @@
> +#include "test-tool.h"
> +#include "test-tool-utils.h"
> +#include "cache.h"
> +#include "parse-options.h"
> +#include "submodule.h"
> +
> +#define TEST_TOOL_IS_ACTIVE_USAGE \
> +	"test-tool submodule is-active <name>"
> +static const char *submodule_is_active_usage[] = {
> +	TEST_TOOL_IS_ACTIVE_USAGE,
> +	NULL
> +};
> +
> +static const char *submodule_usage[] = {
> +	TEST_TOOL_IS_ACTIVE_USAGE,
> +	NULL
> +};
> +
> +static int cmd__submodule_is_active(int argc, const char **argv)
> +{
> +	struct option options[] = {
> +		OPT_END()
> +	};
> +	argc = parse_options(argc, argv, "test-tools", options,
> +			     submodule_is_active_usage, 0);
> +	if (argc != 1)
> +		usage_with_options(submodule_is_active_usage, options);
> +
> +	setup_git_directory();
> +
> +	return !is_submodule_active(the_repository, argv[0]);
> +}
> +
> +static struct test_cmd cmds[] = {
> +	{ "is-active", cmd__submodule_is_active },
> +};
> +
> +int cmd__submodule(int argc, const char **argv)
> +{
> +	struct option options[] = {
> +		OPT_END()
> +	};
> +	size_t i;
> +
> +	argc = parse_options(argc, argv, "test-tools", options, submodule_usage,
> +			     PARSE_OPT_STOP_AT_NON_OPTION);
> +	if (argc < 1)
> +		usage_with_options(submodule_usage, options);
> +
> +	for (i = 0; i < ARRAY_SIZE(cmds); i++)
> +		if (!strcmp(cmds[i].name, argv[0]))
> +			return cmds[i].fn(argc, argv);
> +
> +	usage_msg_optf("unknown subcommand '%s'", submodule_usage, options,
> +		       argv[0]);
> +
> +	return 0;
> +}
> diff --git a/t/helper/test-tool-utils.h b/t/helper/test-tool-utils.h
> new file mode 100644
> index 00000000000..6a0e5e0074f
> --- /dev/null
> +++ b/t/helper/test-tool-utils.h
> @@ -0,0 +1,9 @@
> +#ifndef TEST_TOOL_UTILS_H
> +#define TEST_TOOL_UTILS_H
> +
> +struct test_cmd {
> +	const char *name;
> +	int (*fn)(int argc, const char **argv);
> +};
> +
> +#endif
> diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
> index 318fdbab0c3..7a6a8b88a91 100644
> --- a/t/helper/test-tool.c
> +++ b/t/helper/test-tool.c
> @@ -1,5 +1,6 @@
>  #include "git-compat-util.h"
>  #include "test-tool.h"
> +#include "test-tool-utils.h"
>  #include "trace2.h"
>  #include "parse-options.h"
>  
> @@ -8,11 +9,6 @@ static const char * const test_tool_usage[] = {
>  	NULL
>  };
>  
> -struct test_cmd {
> -	const char *name;
> -	int (*fn)(int argc, const char **argv);
> -};
> -
>  static struct test_cmd cmds[] = {
>  	{ "advise", cmd__advise_if_enabled },
>  	{ "bitmap", cmd__bitmap },
> @@ -78,6 +74,7 @@ static struct test_cmd cmds[] = {
>  	{ "simple-ipc", cmd__simple_ipc },
>  	{ "strcmp-offset", cmd__strcmp_offset },
>  	{ "string-list", cmd__string_list },
> +	{ "submodule", cmd__submodule },
>  	{ "submodule-config", cmd__submodule_config },
>  	{ "submodule-nested-repo-config", cmd__submodule_nested_repo_config },
>  	{ "subprocess", cmd__subprocess },
> diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
> index bb799271631..5f4f69dee81 100644
> --- a/t/helper/test-tool.h
> +++ b/t/helper/test-tool.h
> @@ -68,6 +68,7 @@ int cmd__sigchain(int argc, const char **argv);
>  int cmd__simple_ipc(int argc, const char **argv);
>  int cmd__strcmp_offset(int argc, const char **argv);
>  int cmd__string_list(int argc, const char **argv);
> +int cmd__submodule(int argc, const char **argv);
>  int cmd__submodule_config(int argc, const char **argv);
>  int cmd__submodule_nested_repo_config(int argc, const char **argv);
>  int cmd__subprocess(int argc, const char **argv);

I'm underqualified to comment on structure of test-tool, so I won't
comment on that, but it makes sense to convert code that is only used in
tests into test helpers.

That said, don't we only use "git submodule--helper is-active" inside
the tests for "git submodule--helper is-active"? If so, I'd think the
right thing to do is to just get rid of the code altogether.

The exception IMO, is if we think that we want a kind of "unit test"
that checks our logic for determining if a submodule is active. I don't
really think that's necessary, but if so, we probably want to update the
description of t7413 to describe that intent.

> diff --git a/t/t7413-submodule-is-active.sh b/t/t7413-submodule-is-active.sh
> index c8e7e983317..9ead083371a 100755
> --- a/t/t7413-submodule-is-active.sh
> +++ b/t/t7413-submodule-is-active.sh
> @@ -1,8 +1,8 @@
>  #!/bin/sh
>  
> -test_description='Test submodule--helper is-active
> +test_description='Test with test-tool submodule is-active
>  
> -This test verifies that `git submodue--helper is-active` correctly identifies
> +This test verifies that `test-tool submodule is-active` correctly identifies
>  submodules which are "active" and interesting to the user.
>  '
>  
> @@ -25,13 +25,13 @@ test_expect_success 'setup' '
>  '
>  
>  test_expect_success 'is-active works with urls' '
> -	git -C super submodule--helper is-active sub1 &&
> -	git -C super submodule--helper is-active sub2 &&
> +	test-tool -C super submodule is-active sub1 &&
> +	test-tool -C super submodule is-active sub2 &&
>  
>  	git -C super config --unset submodule.sub1.URL &&
> -	test_must_fail git -C super submodule--helper is-active sub1 &&
> +	test_must_fail test-tool -C super submodule is-active sub1 &&
>  	git -C super config submodule.sub1.URL ../sub &&
> -	git -C super submodule--helper is-active sub1
> +	test-tool -C super submodule is-active sub1
>  '
>  
>  test_expect_success 'is-active works with submodule.<name>.active config' '
> @@ -39,11 +39,11 @@ test_expect_success 'is-active works with submodule.<name>.active config' '
>  	test_when_finished "git -C super config submodule.sub1.URL ../sub" &&
>  
>  	git -C super config --bool submodule.sub1.active "false" &&
> -	test_must_fail git -C super submodule--helper is-active sub1 &&
> +	test_must_fail test-tool -C super submodule is-active sub1 &&
>  
>  	git -C super config --bool submodule.sub1.active "true" &&
>  	git -C super config --unset submodule.sub1.URL &&
> -	git -C super submodule--helper is-active sub1
> +	test-tool -C super submodule is-active sub1
>  '
>  
>  test_expect_success 'is-active works with basic submodule.active config' '
> @@ -53,17 +53,17 @@ test_expect_success 'is-active works with basic submodule.active config' '
>  	git -C super config --add submodule.active "." &&
>  	git -C super config --unset submodule.sub1.URL &&
>  
> -	git -C super submodule--helper is-active sub1 &&
> -	git -C super submodule--helper is-active sub2
> +	test-tool -C super submodule is-active sub1 &&
> +	test-tool -C super submodule is-active sub2
>  '
>  
>  test_expect_success 'is-active correctly works with paths that are not submodules' '
>  	test_when_finished "git -C super config --unset-all submodule.active" &&
>  
> -	test_must_fail git -C super submodule--helper is-active not-a-submodule &&
> +	test_must_fail test-tool -C super submodule is-active not-a-submodule &&
>  
>  	git -C super config --add submodule.active "." &&
> -	test_must_fail git -C super submodule--helper is-active not-a-submodule
> +	test_must_fail test-tool -C super submodule is-active not-a-submodule
>  '
>  
>  test_expect_success 'is-active works with exclusions in submodule.active config' '
> @@ -72,8 +72,8 @@ test_expect_success 'is-active works with exclusions in submodule.active config'
>  	git -C super config --add submodule.active "." &&
>  	git -C super config --add submodule.active ":(exclude)sub1" &&
>  
> -	test_must_fail git -C super submodule--helper is-active sub1 &&
> -	git -C super submodule--helper is-active sub2
> +	test_must_fail test-tool -C super submodule is-active sub1 &&
> +	test-tool -C super submodule is-active sub2
>  '
>  
>  test_expect_success 'is-active with submodule.active and submodule.<name>.active' '
> @@ -85,8 +85,8 @@ test_expect_success 'is-active with submodule.active and submodule.<name>.active
>  	git -C super config --bool submodule.sub1.active "false" &&
>  	git -C super config --bool submodule.sub2.active "true" &&
>  
> -	test_must_fail git -C super submodule--helper is-active sub1 &&
> -	git -C super submodule--helper is-active sub2
> +	test_must_fail test-tool -C super submodule is-active sub1 &&
> +	test-tool -C super submodule is-active sub2
>  '
>  
>  test_expect_success 'is-active, submodule.active and submodule add' '
> -- 
> 2.37.1.1167.g38fda70d8c4