git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 1/8] submodule--helper: move "config" to a test-tool

From
Glen Choo <chooglen@google.com>
Date
Nov 3, 2022, 22:09 UTC
Message-ID
<kl6lfsezofk9.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To
<patch-1.8-b2649f96715-20221102T074148Z-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 72 quoted lines
> As with other moves to "test-tool" in f322e9f51b5 (Merge branch
> 'ab/submodule-helper-prep', 2022-09-13) the "config" sub-command was
> only used by our own tests.
>
> Let's move it over, and while doing so make it easier to reason about
> by splitting up the various uses for it into separate sub-commands, so
> that we don't need to count arguments to see what it does.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/submodule--helper.c            | 46 --------------
>  t/helper/test-submodule.c              | 84 ++++++++++++++++++++++++++
>  t/t7411-submodule-config.sh            | 28 ++++-----
>  t/t7418-submodule-sparse-gitmodules.sh |  4 +-
>  4 files changed, 100 insertions(+), 62 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index a7683d35299..6250b95a6f7 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2861,51 +2861,6 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
>  	return ret;
>  }
>  
> -static int module_config(int argc, const char **argv, const char *prefix)
> -{
> -	enum {
> -		CHECK_WRITEABLE = 1,
> -		DO_UNSET = 2
> -	} command = 0;
> -	struct option module_config_options[] = {
> -		OPT_CMDMODE(0, "check-writeable", &command,
> -			    N_("check if it is safe to write to the .gitmodules file"),
> -			    CHECK_WRITEABLE),
> -		OPT_CMDMODE(0, "unset", &command,
> -			    N_("unset the config in the .gitmodules file"),
> -			    DO_UNSET),
> -		OPT_END()
> -	};
> -	const char *const git_submodule_helper_usage[] = {
> -		N_("git submodule--helper config <name> [<value>]"),
> -		N_("git submodule--helper config --unset <name>"),
> -		"git submodule--helper config --check-writeable",
> -		NULL
> -	};
> -
> -	argc = parse_options(argc, argv, prefix, module_config_options,
> -			     git_submodule_helper_usage, PARSE_OPT_KEEP_ARGV0);
> -
> -	if (argc == 1 && command == CHECK_WRITEABLE)
> -		return is_writing_gitmodules_ok() ? 0 : -1;
> -
> -	/* Equivalent to ACTION_GET in builtin/config.c */
> -	if (argc == 2 && command != DO_UNSET)
> -		return print_config_from_gitmodules(the_repository, argv[1]);
> -
> -	/* Equivalent to ACTION_SET in builtin/config.c */
> -	if (argc == 3 || (argc == 2 && command == DO_UNSET)) {
> -		const char *value = (argc == 3) ? argv[2] : NULL;
> -
> -		if (!is_writing_gitmodules_ok())
> -			die(_("please make sure that the .gitmodules file is in the working tree"));
> -
> -		return config_set_in_gitmodules_file_gently(argv[1], value);
> -	}
> -
> -	usage_with_options(git_submodule_helper_usage, module_config_options);
> -}
> -
>  static int module_set_url(int argc, const char **argv, const char *prefix)
>  {
>  	int quiet = 0;

"git submodule--helper config" isn't familiar to me, so I did a bit of digging. It looks like it falls into the category of submodule--helper subcommmands that exist to expose functionality that's easy to do in-process, but otherwise inaccessible to git-submodule.sh.

Its last use (I think) was in a6226fd772 (submodule--helper: convert the bulk of cmd_add() to C, 2021-08-10).

Show 130 quoted lines
> @@ -3424,7 +3379,6 @@ static struct cmd_struct commands[] = {
>  	{"summary", module_summary, 0},
>  	{"push-check", push_check, 0},
>  	{"absorbgitdirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
> -	{"config", module_config, 0},
>  	{"set-url", module_set_url, 0},
>  	{"set-branch", module_set_branch, 0},
>  	{"create-branch", module_create_branch, 0},
> diff --git a/t/helper/test-submodule.c b/t/helper/test-submodule.c
> index b7d117cd557..c7cb463a583 100644
> --- a/t/helper/test-submodule.c
> +++ b/t/helper/test-submodule.c
> @@ -111,10 +111,94 @@ static int cmd__submodule_resolve_relative_url(int argc, const char **argv)
>  	return 0;
>  }
>  
> +static int cmd__submodule_config_list(int argc, const char **argv)
> +{
> +	struct option options[] = {
> +		OPT_END()
> +	};
> +	const char *const usage[] = {
> +		"test-tool submodule config-list <key>",
> +		NULL
> +	};
> +	argc = parse_options(argc, argv, "test-tools", options, usage,
> +			     PARSE_OPT_KEEP_ARGV0);
> +
> +	setup_git_directory();
> +
> +	if (argc == 2)
> +		return print_config_from_gitmodules(the_repository, argv[1]);
> +	usage_with_options(usage, options);
> +}
> +
> +static int cmd__submodule_config_set(int argc, const char **argv)
> +{
> +	struct option options[] = {
> +		OPT_END()
> +	};
> +	const char *const usage[] = {
> +		"test-tool submodule config-set <key> <value>",
> +		NULL
> +	};
> +	argc = parse_options(argc, argv, "test-tools", options, usage,
> +			     PARSE_OPT_KEEP_ARGV0);
> +
> +	setup_git_directory();
> +
> +	/* Equivalent to ACTION_SET in builtin/config.c */
> +	if (argc == 3) {
> +		if (!is_writing_gitmodules_ok())
> +			die(_("please make sure that the .gitmodules file is in the working tree"));
> +
> +		return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
> +	}
> +	usage_with_options(usage, options);
> +}
> +
> +static int cmd__submodule_config_unset(int argc, const char **argv)
> +{
> +	struct option options[] = {
> +		OPT_END()
> +	};
> +	const char *const usage[] = {
> +		"test-tool submodule config-unset <key>",
> +		NULL
> +	};
> +
> +	setup_git_directory();
> +
> +	if (argc == 2) {
> +		if (!is_writing_gitmodules_ok())
> +			die(_("please make sure that the .gitmodules file is in the working tree"));
> +		return config_set_in_gitmodules_file_gently(argv[1], NULL);
> +	}
> +	usage_with_options(usage, options);
> +}
> +
> +static int cmd__submodule_config_writeable(int argc, const char **argv)
> +{
> +	struct option options[] = {
> +		OPT_END()
> +	};
> +	const char *const usage[] = {
> +		"test-tool submodule config-writeable",
> +		NULL
> +	};
> +	setup_git_directory();
> +
> +	if (argc == 1)
> +		return is_writing_gitmodules_ok() ? 0 : -1;
> +
> +	usage_with_options(usage, options);
> +}
> +
>  static struct test_cmd cmds[] = {
>  	{ "check-name", cmd__submodule_check_name },
>  	{ "is-active", cmd__submodule_is_active },
>  	{ "resolve-relative-url", cmd__submodule_resolve_relative_url},
> +	{ "config-list", cmd__submodule_config_list },
> +	{ "config-set", cmd__submodule_config_set },
> +	{ "config-unset", cmd__submodule_config_unset },
> +	{ "config-writeable", cmd__submodule_config_writeable },
>  };
>  
>  int cmd__submodule(int argc, const char **argv)
> diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
> index c583c4e373a..2f57ecd177a 100755
> --- a/t/t7411-submodule-config.sh
> +++ b/t/t7411-submodule-config.sh
> @@ -140,15 +140,15 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
>  test_expect_success 'reading submodules config from the working tree with "submodule--helper config"' '
>  	(cd super &&
>  		echo "../submodule" >expect &&
> -		git submodule--helper config submodule.submodule.url >actual &&
> +		test-tool submodule config-list submodule.submodule.url >actual &&
>  		test_cmp expect actual
>  	)
>  '
>  
>  test_expect_success 'unsetting submodules config from the working tree with "submodule--helper config --unset"' '
>  	(cd super &&
> -		git submodule--helper config --unset submodule.submodule.url &&
> -		git submodule--helper config submodule.submodule.url >actual &&
> +		test-tool submodule config-unset submodule.submodule.url &&
> +		test-tool submodule config-list submodule.submodule.url >actual &&
>  		test_must_be_empty actual
>  	)
>  '

In a few of the test names, we still say 'do X with "submodule--helper config [...]"'. I think just we can drop 'with "submodule--helper config [...]"', e.g.

     test_expect_sucess 'unsetting submodules config from the working tree'
Show 37 quoted lines
> @@ -157,8 +157,8 @@ test_expect_success 'unsetting submodules config from the working tree with "sub
>  test_expect_success 'writing submodules config with "submodule--helper config"' '
>  	(cd super &&
>  		echo "new_url" >expect &&
> -		git submodule--helper config submodule.submodule.url "new_url" &&
> -		git submodule--helper config submodule.submodule.url >actual &&
> +		test-tool submodule config-set submodule.submodule.url "new_url" &&
> +		test-tool submodule config-list submodule.submodule.url >actual &&
>  		test_cmp expect actual
>  	)
>  '
> @@ -167,14 +167,14 @@ test_expect_success 'overwriting unstaged submodules config with "submodule--hel
>  	test_when_finished "git -C super checkout .gitmodules" &&
>  	(cd super &&
>  		echo "newer_url" >expect &&
> -		git submodule--helper config submodule.submodule.url "newer_url" &&
> -		git submodule--helper config submodule.submodule.url >actual &&
> +		test-tool submodule config-set submodule.submodule.url "newer_url" &&
> +		test-tool submodule config-list submodule.submodule.url >actual &&
>  		test_cmp expect actual
>  	)
>  '
>  
>  test_expect_success 'writeable .gitmodules when it is in the working tree' '
> -	git -C super submodule--helper config --check-writeable
> +	test-tool -C super submodule config-writeable
>  '
>  
>  test_expect_success 'writeable .gitmodules when it is nowhere in the repository' '
> @@ -183,7 +183,7 @@ test_expect_success 'writeable .gitmodules when it is nowhere in the repository'
>  	(cd super &&
>  		git rm .gitmodules &&
>  		git commit -m "remove .gitmodules from the current branch" &&
> -		git submodule--helper config --check-writeable
> +		test-tool submodule config-writeable
>  	)
>  '

The value of the writeable/unwriteable tests feels quite low to me, but I am ok to keep them just to be safe.

Show 63 quoted lines
>  
> @@ -191,7 +191,7 @@ test_expect_success 'non-writeable .gitmodules when it is in the index but not i
>  	test_when_finished "git -C super checkout .gitmodules" &&
>  	(cd super &&
>  		rm -f .gitmodules &&
> -		test_must_fail git submodule--helper config --check-writeable
> +		test_must_fail test-tool submodule config-writeable
>  	)
>  '
>  
> @@ -200,7 +200,7 @@ test_expect_success 'non-writeable .gitmodules when it is in the current branch
>  	test_when_finished "git -C super reset --hard $ORIG" &&
>  	(cd super &&
>  		git rm .gitmodules &&
> -		test_must_fail git submodule--helper config --check-writeable
> +		test_must_fail test-tool submodule config-writeable
>  	)
>  '
>  
> @@ -208,11 +208,11 @@ test_expect_success 'reading submodules config from the index when .gitmodules i
>  	ORIG=$(git -C super rev-parse HEAD) &&
>  	test_when_finished "git -C super reset --hard $ORIG" &&
>  	(cd super &&
> -		git submodule--helper config submodule.submodule.url "staged_url" &&
> +		test-tool submodule config-set submodule.submodule.url "staged_url" &&
>  		git add .gitmodules &&
>  		rm -f .gitmodules &&
>  		echo "staged_url" >expect &&
> -		git submodule--helper config submodule.submodule.url >actual &&
> +		test-tool submodule config-list submodule.submodule.url >actual &&
>  		test_cmp expect actual
>  	)
>  '
> @@ -223,7 +223,7 @@ test_expect_success 'reading submodules config from the current branch when .git
>  	(cd super &&
>  		git rm .gitmodules &&
>  		echo "../submodule" >expect &&
> -		git submodule--helper config submodule.submodule.url >actual &&
> +		test-tool submodule config-list submodule.submodule.url >actual &&
>  		test_cmp expect actual
>  	)
>  '
> diff --git a/t/t7418-submodule-sparse-gitmodules.sh b/t/t7418-submodule-sparse-gitmodules.sh
> index d5874200fdc..dde11ecce80 100755
> --- a/t/t7418-submodule-sparse-gitmodules.sh
> +++ b/t/t7418-submodule-sparse-gitmodules.sh
> @@ -50,12 +50,12 @@ test_expect_success 'sparse checkout setup which hides .gitmodules' '
>  
>  test_expect_success 'reading gitmodules config file when it is not checked out' '
>  	echo "../submodule" >expect &&
> -	git -C super submodule--helper config submodule.submodule.url >actual &&
> +	test-tool -C super submodule config-list submodule.submodule.url >actual &&
>  	test_cmp expect actual
>  '
>  
>  test_expect_success 'not writing gitmodules config file when it is not checked out' '
> -	test_must_fail git -C super submodule--helper config submodule.submodule.url newurl &&
> +	test_must_fail test-tool -C super submodule config-set submodule.submodule.url newurl &&
>  	test_path_is_missing super/.gitmodules
>  '
>  
> -- 
> 2.38.0.1280.g8136eb6fab2
Previous: Ævar Arnfjörð BjarmasonNext: Ævar Arnfjörð Bjarmason
Message 3 of 35 in “submodule: tests, cleanup to prepare for built-in”
  1. 0/8 submodule: tests, cleanup to prepare for built-inÆvar Arnfjörð Bjarmason, Nov 2, 2022
  2. 1/8 submodule--helper: move "config" to a test-toolÆvar Arnfjörð Bjarmason, Nov 2, 2022
  3. Glen ChooNov 3, 2022
  4. 2/8 submodule tests: add tests for top-level flag outputÆvar Arnfjörð Bjarmason, Nov 2, 2022
  5. Glen ChooNov 3, 2022
  6. 3/8 submodule tests: test for a "foreach" blind-spotÆvar Arnfjörð Bjarmason, Nov 2, 2022
  7. 4/8 submodule.c: refactor recursive block out of absorb functionÆvar Arnfjörð Bjarmason, Nov 2, 2022
  8. 5/8 submodule API & "absorbgitdirs": remove "----recursive" optionÆvar Arnfjörð Bjarmason, Nov 2, 2022
  9. Glen ChooNov 3, 2022
  10. Ævar Arnfjörð BjarmasonNov 4, 2022
  11. FW: [PATCH 5/8] submodule API & "absorbgitdirs": remove "----recursive" optionSimpson, Phyllis, Nov 4, 2022
  12. Glen ChooNov 4, 2022
  13. 6/8 submodule--helper: remove --prefix from "absorbgitdirs"Ævar Arnfjörð Bjarmason, Nov 2, 2022
  14. 7/8 submodule--helper: drop "update --prefix <pfx>" for "-C <pfx> update"Ævar Arnfjörð Bjarmason, Nov 2, 2022
  15. 8/8 submodule--helper: use OPT_SUBCOMMAND() APIÆvar Arnfjörð Bjarmason, Nov 2, 2022
  16. Glen ChooNov 3, 2022
  17. Ævar Arnfjörð BjarmasonNov 4, 2022
  18. Glen ChooNov 4, 2022
  19. Ævar Arnfjörð BjarmasonNov 5, 2022
  20. Glen ChooNov 7, 2022
  21. Glen ChooNov 4, 2022
  22. Taylor BlauNov 4, 2022
  23. 0/9 submodule: tests, cleanup to prepare for built-inÆvar Arnfjörð Bjarmason, Nov 8, 2022
  24. 1/9 submodule--helper: move "config" to a test-toolÆvar Arnfjörð Bjarmason, Nov 8, 2022
  25. 2/9 submodule tests: add tests for top-level flag outputÆvar Arnfjörð Bjarmason, Nov 8, 2022
  26. 3/9 submodule--helper: fix a memory leak in "status"Ævar Arnfjörð Bjarmason, Nov 8, 2022
  27. 5/9 submodule.c: refactor recursive block out of absorb functionÆvar Arnfjörð Bjarmason, Nov 8, 2022
  28. 6/9 submodule API & "absorbgitdirs": remove "----recursive" optionÆvar Arnfjörð Bjarmason, Nov 8, 2022
  29. 7/9 submodule--helper: remove --prefix from "absorbgitdirs"Ævar Arnfjörð Bjarmason, Nov 8, 2022
  30. 4/9 submodule tests: test for a "foreach" blind-spotÆvar Arnfjörð Bjarmason, Nov 8, 2022
  31. 8/9 submodule--helper: drop "update --prefix <pfx>" for "-C <pfx> update"Ævar Arnfjörð Bjarmason, Nov 8, 2022
  32. 9/9 submodule--helper: use OPT_SUBCOMMAND() APIÆvar Arnfjörð Bjarmason, Nov 8, 2022
  33. Glen ChooNov 8, 2022
  34. Ævar Arnfjörð BjarmasonNov 8, 2022
  35. Glen ChooNov 8, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.