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@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.
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:
signature.asc
Description: PGP signature
