Re: [PATCH v2 09/12] builtin/show-ref: ensure mutual exclusiveness of subcommands

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 30, 2023, 19:31 UTC
Message-ID: <ZUAElIb7mjoBBRcn@nand.local>
In-Reply-To: <5ba566723e8742e6df150b12f1d044089ff62b59.1698314128.git.ps@pks.im>

On Thu, Oct 26, 2023 at 11:56:57AM +0200, Patrick Steinhardt wrote:

Show 29 quoted lines

> The git-show-ref(1) command has three different modes, of which one is
> implicit and the other two can be chosen explicitly by passing a flag.
> But while these modes are standalone and cause us to execute completely
> separate code paths, we gladly accept the case where a user asks for
> both `--exclude-existing` and `--verify` at the same time even though it
> is not obvious what will happen. Spoiler: we ignore `--verify` and
> execute the `--exclude-existing` mode.
>
> Let's explicitly detect this invalid usage and die in case both modes
> were requested.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/show-ref.c  | 4 ++++
>  t/t1403-show-ref.sh | 5 +++++
>  2 files changed, 9 insertions(+)
>
> diff --git a/builtin/show-ref.c b/builtin/show-ref.c
> index 87bc45d2d13..1768aef77b3 100644
> --- a/builtin/show-ref.c
> +++ b/builtin/show-ref.c
> @@ -271,6 +271,10 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
>  	argc = parse_options(argc, argv, prefix, show_ref_options,
>  			     show_ref_usage, 0);
>
> +	if ((!!exclude_existing_opts.enabled + !!verify) > 1)
> +		die(_("only one of '%s' or '%s' can be given"),
> +		    "--exclude-existing", "--verify");
> +

This is technically correct, but I was surprised to see it written this way instead of

    if (exclude_existing_opts.enabled && verify)
        die(...);

I don't think it's a big deal either way, I was just curious why you chose one over the other.

> +test_expect_success 'show-ref sub-modes are mutually exclusive' '
> +	test_must_fail git show-ref --verify --exclude-existing 2>err &&
> +	grep "only one of ${SQ}--exclude-existing${SQ} or ${SQ}--verify${SQ} can be given" err
> +'

grepping is fine here, but since you have the exact error message, it may be worth switching to test_cmp.

Thanks, Taylor