On Tue, Jun 07, 2022 at 10:07:32AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> On Mon, Jun 06 2022, Taylor Blau wrote:
>
> > diff --git a/Documentation/git-show-ref.txt b/Documentation/git-show-ref.txt
> > index ab4d271925..28256c04dd 100644
> > --- a/Documentation/git-show-ref.txt
> > +++ b/Documentation/git-show-ref.txt
> > @@ -10,7 +10,7 @@ SYNOPSIS
> > [verse]
> > 'git show-ref' [-q|--quiet] [--verify] [--head] [-d|--dereference]
> > [-s|--hash[=<n>]] [--abbrev[=<n>]] [--tags]
> > - [--heads] [--] [<pattern>...]
> > + [--heads] [--count=<n>] [--] [<pattern>...]
>
> In addition to what Junio noted, the SYNOPSIS is now inaccurate per your
> documentation. I.e. if this option is incompatible with --verify and
> --exclude-existing we should use "|" to indicate that, e.g.:
>
> [ [--verify] [--exclude-existing] | --count=<n> ]
Good catch. Should this be squashed into the first example in the
SYNOPSIS, the second, or a new one?
> > + if (max_count) {
> > + int compatible = 0;
> > +
> > + if (max_count < 0)
> > + error(_("invalid --count argument: (`%d' < 0)"),
> > + max_count);
> > + else if (verify)
> > + error(_("--count is incompatible with %s"), "--verify");
> > + else if (exclude_arg)
> > + error(_("--count is incompatible with %s"),
> > + "--exclude-existing");
> > + else
> > + compatible = 1;
> > +
> > + if (!compatible)
> > + usage_with_options(show_ref_usage, show_ref_options);
>
> Instead of this "int compatible" and if/else-if" just use usage_msg_optf().
>
> That or die_for_incompatible_opt4(), at least the new _() messages
> should make use of the same translations. I.e. we recently made these
> parameterized.
Good catch again. I wasn't aware of usage_msg_optf(), but it's exactly
what I'm looking for here. It does mean that we'd only print one warning
at a time, but I think that's a fair tradeoff, and unlikely to matter in
practice anyways.
And I must have dropped the parameterized msgids on the floor when
preparing this patch, since I definitely have it locally. Oops, fixed.
> > + }
> > +
> > if (exclude_arg)
> > return exclude_existing(exclude_existing_arg);
> >
> > diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
> > index 9252a581ab..b79e114c1e 100755
> > --- a/t/t1403-show-ref.sh
> > +++ b/t/t1403-show-ref.sh
> > @@ -196,4 +196,25 @@ test_expect_success 'show-ref --verify with dangling ref' '
> > )
> > '
> >
> > +test_expect_success 'show-ref --count limits relevant output' '
> > + git show-ref --heads --count=1 >out &&
> > + test_line_count = 1 out
> > +'
> > +
> > +test_expect_success 'show-ref --count rejects invalid input' '
> > + test_must_fail git show-ref --count=-1 2>err &&
> > + grep "invalid ..count argument: (.-1. < 0)" err
>
> The use of .. here seems odd...
>
> > +'
> > +
> > +test_expect_success 'show-ref --count incompatible with --verify' '
> > + test_must_fail git show-ref --count=1 --verify HEAD 2>err &&
> > + grep "..count is incompatible with ..verify" err
>
> ...i.e. this looks like a way to avoid "--" at the beginning, but then
> why use it in the middle of the regex?
Muscle memory ;).
> > +test_expect_success 'show-ref --count incompatible with --exclude-existing' '
> > + echo "refs/heads/main" >in &&
> > + test_must_fail git show-ref --count=1 --exclude-existing <in 2>err &&
> > + grep "..count is incompatible with ..exclude.existing" err
>
> Seems like you could avoid it entirely by escaping it, e.g. "[-]-" at
> the beginning, or in this case I think "test_expect_code 129" would be
> more than sufficient, we use that to test "had usage spewed at us"
> elsewhere.
I like having the extra test to ensure the error we got made sense, but
I agree either would work. I modified the grep expressions to replace
leading "."'s with "[-]", and "."'s in the middle of the expression with
"-".
Thanks,
Taylor
