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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jun 7, 2022, 08:07 UTC
Message-ID: <220607.86r140vqc4.gmgdl@evledraar.gmail.com>
In-Reply-To: <3fcf1f555715e925385d37712ffe880bb869741e.1654552560.git.me@ttaylorr.com>

On Mon, Jun 06 2022, Taylor Blau wrote:

Show 10 quoted lines

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

Show 16 quoted lines

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

Show 21 quoted lines

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

Show 5 quoted lines

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

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