Re: [PATCH v2 09/12] builtin/show-ref: ensure mutual exclusiveness of subcommands
From: Taylor Blau <hidden>
Date: 2023-10-30 19:31:35
On Thu, Oct 26, 2023 at 11:56:57AM +0200, Patrick Steinhardt wrote:
quoted hunk ↗ jump to hunk
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 <redacted> --- 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