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

[Junio C Hamano <gitster@pobox.com>]

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

> +#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[] = {


> +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.

> -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].