Re: [PATCH 2/2] builtin/show-ref.c: limit output with `--count`

[Taylor Blau <me@ttaylorr.com>]

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