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

[Jeff King <peff@peff.net>]

On Tue, Jan 09, 2024 at 05:53:36PM +0000, Victoria Dye via GitGitGadget wrote:

> +#define TEST_TOOL_CHECK_URL_USAGE \
> +	"test-tool submodule check-url <url>"

I don't think this command-line "<url>" mode works at all. Your
underlying function can handle either stdin or arguments:

> -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)
>  				return 1;
>  		}
>  	} else {
>  		struct strbuf buf = STRBUF_INIT;
>  		while (strbuf_getline(&buf, stdin) != EOF) {
> -			if (!check_submodule_name(buf.buf))
> +			if (!check_fn(buf.buf))
>  				printf("%s\n", buf.buf);
>  		}
>  		strbuf_release(&buf);

...but the new caller rejects them before we get there:

> +static int cmd__submodule_check_url(int argc, const char **argv)
> +{
> +	struct option options[] = {
> +		OPT_END()
> +	};
> +	argc = parse_options(argc, argv, "test-tools", options,
> +			     submodule_check_url_usage, 0);
> +	if (argc)
> +		usage_with_options(submodule_check_url_usage, options);
> +
> +	return check_submodule(argc, argv, check_submodule_url);
>  }

So you'd want at least:

diff --git a/t/helper/test-submodule.c b/t/helper/test-submodule.c
index da89d265f0..6b964c88ab 100644
--- a/t/helper/test-submodule.c
+++ b/t/helper/test-submodule.c
@@ -88,8 +88,6 @@ static int cmd__submodule_check_url(int argc, const char **argv)
 	};
 	argc = parse_options(argc, argv, "test-tools", options,
 			     submodule_check_url_usage, 0);
-	if (argc)
-		usage_with_options(submodule_check_url_usage, options);
 
 	return check_submodule(argc, argv, check_submodule_url);
 }

but then that reveals another mismatch. In check_submodule() above we
expect argv[0] to be uninteresting (i.e., the name of the program), but
parse_options() will already have thrown it away. So we silently fail to
check the first option (which is especially bad since the only output is
the exit code, and thus the skipped one looks the same as one that
validated correctly).

All of this is inherited from the existing check_name() code, which I
think has all of the same bugs. The test scripts all just use the stdin
mode, so they don't notice. It's not too hard to fix, but maybe it's
worth just ripping out the unreachable code.

-Peff