| Message ID | 5ba566723e8742e6df150b12f1d044089ff62b59.1698314128.git.ps@pks.im (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | show-ref: introduce mode to check for ref existence | expand |
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. > +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
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
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"); + if (exclude_existing_opts.enabled) return cmd_show_ref__exclude_existing(&exclude_existing_opts); else if (verify) diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh index 9252a581abf..4a90a88e05d 100755 --- a/t/t1403-show-ref.sh +++ b/t/t1403-show-ref.sh @@ -196,4 +196,9 @@ test_expect_success 'show-ref --verify with dangling ref' ' ) ' +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 +' + test_done
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(+)