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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2342 bytes --]

On Mon, Oct 30, 2023 at 03:31:32PM -0400, Taylor Blau wrote:
> 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@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.

Here it doesn't make a lot of sense yet, agreed. But once we add
`exists` as a third mutually-exclusive option it does because of
combinatorial explosion.

> > +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.

Good point. Doubly so because I switch to `test_cmp` in a later patch.
Will change.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]