Re: [PATCH 2/3] t7450: test submodule urls

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 9, 2024, 21:38 UTC
Message-ID: <xmqqttnmfarm.fsf@gitster.g>
In-Reply-To: <cf7848edffca27931aad02c0652adf2715320d35.1704822817.git.gitgitgadget@gmail.com>

"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 6 quoted lines

> +#define TEST_TOOL_CHECK_URL_USAGE \
> +	"test-tool submodule check-url <url>"
> +static const char *submodule_check_url_usage[] = {
> +	TEST_TOOL_CHECK_URL_USAGE,
> +	NULL
> +};

Granted, the entry that follows this new one already uses the same pattern, but TEST_TOOL_CHECK_URL_USAGE being used only once here and nowhere else, with its name almost as long as the value it expands to, I found it unnecessarily verbose and confusing.

>  #define TEST_TOOL_IS_ACTIVE_USAGE \
>  	"test-tool submodule is-active <name>"
>  static const char *submodule_is_active_usage[] = {

Show 7 quoted lines

> +typedef int (*check_fn_t)(const char *);
> +
>  /*
>   * 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
>   * (which is primarily intended for testing).
>   */

OK. As long as each of the input lines are unique, we can use the usual "does the actual output match the expected?" to test many of them at once, and notice if there is an extra one in the output that shouldn't have been emitted, or there is a missing one that should have.

Show 7 quoted lines

> -static int check_name(int argc, const char **argv)
> +static int check_submodule(int argc, const char **argv, check_fn_t check_fn)
>  {
>  	if (argc > 1) {
>  		while (*++argv) {
> -			if (check_submodule_name(*argv) < 0)
> +			if (check_fn(*argv) < 0)

Quite nice way to reuse what we already have, thanks to [1/3].