From: Jeff King <peff@peff.net>
Date: Wed, 21 Jul 2021 08:10:59 GMT
Subject: Re: [PATCH] multi-pack-index: fix potential segfault without sub-command
Message-ID: <YPfWkzRtQKthOgZx@coredump.intra.peff.net>
In-Reply-To: <8c0bb3e0dc121bd68f7014000fbb60b28750a0fe.1626715096.git.me@ttaylorr.com>

On Mon, Jul 19, 2021 at 01:18:49PM -0400, Taylor Blau wrote:

> Since cd57bc41bb (builtin/multi-pack-index.c: display usage on
> unrecognized command, 2021-03-30) we have used a "usage" label to avoid
> having two separate callers of usage_with_options (one when no arguments
> are given, and another for unrecognized sub-commands).
> 
> But the first caller has been broken since cd57bc41bb, since it will
> happily jump to usage without arguments, and then pass argv[0] to the
> "unrecognized subcommand" error.
> 
> Many compilers will save us from a segfault here, but the end result is
> ugly, since it mentions an unrecognized subcommand when we didn't even
> pass one, and (on GCC) includes "(null)" in its output.
> 
> Move the "usage" label down past the error about unrecognized
> subcommands so that it is only triggered when it should be. While we're
> at it, bulk up our test coverage in this area, too.

Good find. The code change seems obviously correct.

> +test_expect_success 'usage shown without sub-command' '
> +	test_expect_code 129 git multi-pack-index 2>err &&
> +	! test_i18ngrep "unrecognized subcommand" err
> +'

I think we're avoiding test_i18ngrep in new code these days.

-Peff