On Thu, Oct 26, 2023 at 11:56:57AM +0200, Patrick Steinhardt wrote:
> 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@xxxxxx>
> ---
> 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
