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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Jun 07 2022, Taylor Blau wrote:

> 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?

Personally I really don't care if the end-state is good :)

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

Yeah, I think that should be OK. We do that in other cases.

> And I must have dropped the parameterized msgids on the floor when
> preparing this patch, since I definitely have it locally. Oops, fixed.

*nod*

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

Yeah, fair enough, thanks!