[PATCH 0/2] builtin/show-ref.c: support `--count` for limiting output

[PATCH 0/2] builtin/show-ref.c: support `--count` for limiting output

[Taylor Blau]

This short patch series adds support for a new `--count` argument for limiting
the output of `show-ref` (à-la the `for-each-ref` option by the same name).

This is useful in contexts where a caller wants to avoid enumerating more
references than necessary (e.g., they only care whether a tag exists, but not
how many or what they are called) but doesn't have control of the output stream
(e.g., they are in Ruby and can't pipe the output to `head -n 1`).

The first patch is preparatory, and the second patch implements the new option.
This series doesn't conflict with my recent [1], and can be applied
independently.

Thanks in advance for your review!

[1]: https://lore.kernel.org/git/3fa6932641f18d78156bbf60b1571383f2cb5046.1654293264.git.me@ttaylorr.com/

Taylor Blau (2):
  builtin/show-ref.c: rename `found_match` to `matches_nr`
  builtin/show-ref.c: limit output with `--count`

 Documentation/git-show-ref.txt |  7 ++++++-
 builtin/show-ref.c             | 29 +++++++++++++++++++++++++----
 t/t1403-show-ref.sh            | 21 +++++++++++++++++++++
 3 files changed, 52 insertions(+), 5 deletions(-)

-- 
2.36.1.94.gb0d54bedca

[PATCH 1/2] builtin/show-ref.c: rename `found_match` to `matches_nr`

[Taylor Blau]

The variable `found_match` has been used since 358ddb62cf (Add "git
show-ref" builtin command, 2006-09-15) to keep track of whether or not
any references were shown by `show-ref` so that the command could exit
with status code "1" when no references were presented to the caller.

`found_match` has always been treated as a boolean, even though it is
incremented every time a reference is shown. Prepare for the subsequent
patch, which will treat this variable as a count, by renaming it
accordingly.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/show-ref.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 7f8a5332f8..17d5f98fe4 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -14,7 +14,7 @@ static const char * const show_ref_usage[] = {
 	NULL
 };
 
-static int deref_tags, show_head, tags_only, heads_only, found_match, verify,
+static int deref_tags, show_head, tags_only, heads_only, matches_nr, verify,
 	   quiet, hash_only, abbrev, exclude_arg;
 static const char **pattern;
 static const char *exclude_existing_arg;
@@ -78,7 +78,7 @@ static int show_ref(const char *refname, const struct object_id *oid,
 	}
 
 match:
-	found_match++;
+	matches_nr++;
 
 	show_one(refname, oid);
 
@@ -217,7 +217,7 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
 	if (show_head)
 		head_ref(show_ref, NULL);
 	for_each_ref(show_ref, NULL);
-	if (!found_match) {
+	if (!matches_nr) {
 		if (verify && !quiet)
 			die("No match");
 		return 1;
-- 
2.36.1.94.gb0d54bedca

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

[Taylor Blau]

`git show-ref` is a useful tool for answering existential questions
about whether or not certain (kinds of) references exist or not. For
example, to quickly determine whether or not a repository has any tags,
one could run:

    $ git show-ref -q --tags | head -1

and check whether there was any output.

But certain environments (like spawning Git processes from Ruby code) do
not have ergonomic ways to simulate sending SIGPIPE back to `show-ref`
when they have seen enough output.

Callers _could_ use `for-each-ref`, which has supports a `--count`
option to limit output, but this is sub-par for latency-sensitive
callers since `for-each-ref` buffers all results before printing
anything. In the above instance, even something like:

    $ git for-each-ref --count=1 -- 'refs/tags/*'

will enumerate every tag in a repository before deciding to print just
the first one. (The current behavior exists to accommodate sorting the
output from for-each-ref, and could be improved. See [1] for previous
work in this area).

In the meantime, introduce support for a similar `--count` option in
`show-ref`, so that callers can run:

    $ git show-ref -q --tags --count=1

to quickly determine whether a repository has any (e.g.) tags or not by
only enumerating at most one reference.

(This option is currently "incompatible" with `--verify`, though there
is no reason why the meaning of `--count` couldn't be extended to mean,
"with `--verify`, only verify `<n>` references".)

[1]: https://lore.kernel.org/git/YTNpQ7Od1U%2F5i0R7@coredump.intra.peff.net/

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-show-ref.txt |  7 ++++++-
 builtin/show-ref.c             | 23 ++++++++++++++++++++++-
 t/t1403-show-ref.sh            | 21 +++++++++++++++++++++
 3 files changed, 49 insertions(+), 2 deletions(-)

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>...]
 'git show-ref' --exclude-existing[=<pattern>]
 
 DESCRIPTION
@@ -84,6 +84,11 @@ OPTIONS
 	(4) ignore if refname is a ref that exists in the local repository;
 	(5) otherwise output the line.
 
+--count::
+
+	Do not print more than `<n>` matching references, or print all
+	references if `<n>` is 0. Incompatible with `--exclude-existing`
+	and `--verify`.
 
 <pattern>...::
 
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 17d5f98fe4..f0af8e8eec 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -15,7 +15,7 @@ static const char * const show_ref_usage[] = {
 };
 
 static int deref_tags, show_head, tags_only, heads_only, matches_nr, verify,
-	   quiet, hash_only, abbrev, exclude_arg;
+	   quiet, hash_only, abbrev, exclude_arg, max_count = 0;
 static const char **pattern;
 static const char *exclude_existing_arg;
 
@@ -82,6 +82,8 @@ static int show_ref(const char *refname, const struct object_id *oid,
 
 	show_one(refname, oid);
 
+	if (max_count && matches_nr >= max_count)
+		return -1; /* avoid opening any more refs */
 	return 0;
 }
 
@@ -178,6 +180,7 @@ static const struct option show_ref_options[] = {
 	OPT_CALLBACK_F(0, "exclude-existing", &exclude_existing_arg,
 		       N_("pattern"), N_("show refs from stdin that aren't in local repository"),
 		       PARSE_OPT_OPTARG | PARSE_OPT_NONEG, exclude_existing_callback),
+	OPT_INTEGER(0, "count", &max_count, N_("show only <n> matched refs")),
 	OPT_END()
 };
 
@@ -188,6 +191,24 @@ 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 (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);
+	}
+
 	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
+'
+
+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
+'
+
+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
+'
+
 test_done
-- 
2.36.1.94.gb0d54bedca

Re: [PATCH 0/2] builtin/show-ref.c: support `--count` for limiting output

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> This short patch series adds support for a new `--count` argument for limiting
> the output of `show-ref` (à-la the `for-each-ref` option by the same name).

It makes me wonder why we limit this to show-ref.

    $ git --pipe-to-head-N=3 any-command args...

IOW, having to add an option like this feels absurd.

> This is useful in contexts where a caller wants to avoid enumerating more
> references than necessary (e.g., they only care whether a tag exists, but not
> how many or what they are called) but doesn't have control of the output stream
> (e.g., they are in Ruby and can't pipe the output to `head -n 1`).

Are you saying that Ruby is incapable of run a command line like

   av[0] = "sh"
   av[1] = "-c"
   av[2] = "git show-ref blah | head -n 1"
   av[3] = NULL

?

Re: [PATCH 0/2] builtin/show-ref.c: support `--count` for limiting output

[Taylor Blau]

On Mon, Jun 06, 2022 at 03:52:19PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > This short patch series adds support for a new `--count` argument for limiting
> > the output of `show-ref` (à-la the `for-each-ref` option by the same name).
>
> It makes me wonder why we limit this to show-ref.
>
>     $ git --pipe-to-head-N=3 any-command args...
>
> IOW, having to add an option like this feels absurd.

I don't disagree. But `--pipe-to-head-N=3` feels like too broad a
stroke. This series at least imitates `for-each-ref`'s `--count`
option, which makes it feel acceptable to me (if not a little silly).

> > This is useful in contexts where a caller wants to avoid enumerating more
> > references than necessary (e.g., they only care whether a tag exists, but not
> > how many or what they are called) but doesn't have control of the output stream
> > (e.g., they are in Ruby and can't pipe the output to `head -n 1`).
>
> Are you saying that Ruby is incapable of run a command line like
>
>    av[0] = "sh"
>    av[1] = "-c"
>    av[2] = "git show-ref blah | head -n 1"
>    av[3] = NULL

No, Ruby is perfectly capable of doing that. But it involves an extra
process (two, if `head` isn't a shell builtin) and the additional
overhead of creating a pipe and passing data through it instead of
writing directly to stdout.

That isn't a complete show-stopper in most cases, but in
ultra-latency-sensitive applications like GitHub is using show-ref for,
being able to shave an extra process or so off matters.

If you're strongly opposed to having `show-ref` match `for-each-ref`'s
`--count` option, I won't be too sad. But I'm not in a huge rush to
replace this series with `git --pipe-to-head-N=<n>` either, FWIW.

Thanks,
Taylor

Re: [PATCH 0/2] builtin/show-ref.c: support `--count` for limiting output

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> If you're strongly opposed to having `show-ref` match `for-each-ref`'s
> `--count` option, I won't be too sad. But I'm not in a huge rush to
> replace this series with `git --pipe-to-head-N=<n>` either, FWIW.

Heh, to me "git --pipe-to-head-N=<n>" smells equally absurd, too ;-)

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

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> +--count::
> +
> +	Do not print more than `<n>` matching references, or print all
> +	references if `<n>` is 0. Incompatible with `--exclude-existing`
> +	and `--verify`.

I can easily understand why the option being incompatible with
"--verify", but I do not see a reason why this should not work with
"--exclude-existing" from end user's point of view.

I know it is processed in a completely separate code path, but that
does not stop end users from complaining.

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

[Ævar Arnfjörð Bjarmason]

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

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

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

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

Re: [PATCH 0/2] builtin/show-ref.c: support `--count` for limiting output

[Ævar Arnfjörð Bjarmason]

On Mon, Jun 06 2022, Taylor Blau wrote:

> On Mon, Jun 06, 2022 at 03:52:19PM -0700, Junio C Hamano wrote:
>> Taylor Blau <me@ttaylorr.com> writes:
>>
>> > This short patch series adds support for a new `--count` argument for limiting
>> > the output of `show-ref` (à-la the `for-each-ref` option by the same name).
>>
>> It makes me wonder why we limit this to show-ref.
>>
>>     $ git --pipe-to-head-N=3 any-command args...
>>
>> IOW, having to add an option like this feels absurd.
>
> I don't disagree. But `--pipe-to-head-N=3` feels like too broad a
> stroke. This series at least imitates `for-each-ref`'s `--count`
> option, which makes it feel acceptable to me (if not a little silly).

Yeah, although I do think it's worthwhile to think about where certain
UX decisions are leading us, i.e. the logical conclusion here is to have
every command that emits >1 lines support --count, which as your patch
here shows needs special support, and even in your case you haven't
implemented it in a way that's compatible with all existing options.

B.t.w. why would a --count for --verify not just by supported have these
be equivalent:

    # same
    git tag --count=3 --verify <name>
    git tag --verify <name> | head -n 3

>> > This is useful in contexts where a caller wants to avoid enumerating more
>> > references than necessary (e.g., they only care whether a tag exists, but not
>> > how many or what they are called) but doesn't have control of the output stream
>> > (e.g., they are in Ruby and can't pipe the output to `head -n 1`).
>>
>> Are you saying that Ruby is incapable of run a command line like
>>
>>    av[0] = "sh"
>>    av[1] = "-c"
>>    av[2] = "git show-ref blah | head -n 1"
>>    av[3] = NULL
>
> No, Ruby is perfectly capable of doing that. But it involves an extra
> process (two, if `head` isn't a shell builtin) [...]

Maybe this really is a limitation of ruby, or maybe I'm missing
something, but doesn't it support just opening a process without "sh -c"
and piping the output to your current process, as this perl command
which makes use of execve() will do:

    $ perl -Mautodie=:all -wE '
        my $i = 0; my $lim = shift;
        open my $fh, "-|", qw(git show-ref master);
        while (<$fh>) {
            last if $i++ >= $lim;
            print "$.: $_";
        };' 10

Some quick searching for docs online suggests that Ruby's Open3 and/or
Process.spawn might be the equivalent.

Note that if you replace that qw(git show-ref master) with e.g.:

    "git show-ref master | tail -n 20"

That you'll get 3x execve(), one sh -c, another for "git" and another
for "tail", but in the first case you'll only get the execve().

Isn't that something that would make this workaround unnecessary? Well,
maybe not because...

> [...]and the additional
> overhead of creating a pipe and passing data through it instead of
> writing directly to stdout.

It wouldn't take care of this part, but I'm struggling to think of cases
where you'd be running this in the context of github.com and not already
need to capture the output of the command. I.e. surely you're already
piping stdout/stderr into your program, no?

Re: [PATCH 0/2] builtin/show-ref.c: support `--count` for limiting output

[Derrick Stolee]

On 6/6/2022 7:00 PM, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
> 
>> If you're strongly opposed to having `show-ref` match `for-each-ref`'s
>> `--count` option, I won't be too sad. But I'm not in a huge rush to
>> replace this series with `git --pipe-to-head-N=<n>` either, FWIW.
> 
> Heh, to me "git --pipe-to-head-N=<n>" smells equally absurd, too ;-)

I wonder if we could teach Git to skip an extra process if we see

	git -c "core.pager=head -n=<N>" ...

?

Thanks,
-Stolee

Re: [PATCH 0/2] builtin/show-ref.c: support `--count` for limiting output

[Taylor Blau]

On Tue, Jun 07, 2022 at 10:18:56AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> On Mon, Jun 06 2022, Taylor Blau wrote:
>
> > On Mon, Jun 06, 2022 at 03:52:19PM -0700, Junio C Hamano wrote:
> >> Taylor Blau <me@ttaylorr.com> writes:
> >>
> >> > This short patch series adds support for a new `--count` argument for limiting
> >> > the output of `show-ref` (à-la the `for-each-ref` option by the same name).
> >>
> >> It makes me wonder why we limit this to show-ref.
> >>
> >>     $ git --pipe-to-head-N=3 any-command args...
> >>
> >> IOW, having to add an option like this feels absurd.
> >
> > I don't disagree. But `--pipe-to-head-N=3` feels like too broad a
> > stroke. This series at least imitates `for-each-ref`'s `--count`
> > option, which makes it feel acceptable to me (if not a little silly).
>
> Yeah, although I do think it's worthwhile to think about where certain
> UX decisions are leading us, i.e. the logical conclusion here is to have
> every command that emits >1 lines support --count, which as your patch
> here shows needs special support, and even in your case you haven't
> implemented it in a way that's compatible with all existing options.

To be clear, I don't think adding `--count` to every command is a good
idea. But it exists in `for-each-ref`, and not in `show-ref`, and this
series rectifies that gap in functionality. Perhaps `for-each-ref`
shouldn't have `--count`, but it does, and has since that command's
inception.

> B.t.w. why would a --count for --verify not just by supported have these
> be equivalent:
>
>     # same
>     git tag --count=3 --verify <name>
>     git tag --verify <name> | head -n 3

(I'm not sure if you meant "git tag" here versus "git show-ref", but
either way), `show-ref` in `--verify` mode outputs one line of output
per line of input, so a caller can easily limit the output by limiting
the input.

> >> > This is useful in contexts where a caller wants to avoid enumerating more
> >> > references than necessary (e.g., they only care whether a tag exists, but not
> >> > how many or what they are called) but doesn't have control of the output stream
> >> > (e.g., they are in Ruby and can't pipe the output to `head -n 1`).
> >>
> >> Are you saying that Ruby is incapable of run a command line like
> >>
> >>    av[0] = "sh"
> >>    av[1] = "-c"
> >>    av[2] = "git show-ref blah | head -n 1"
> >>    av[3] = NULL
> >
> > No, Ruby is perfectly capable of doing that. But it involves an extra
> > process (two, if `head` isn't a shell builtin) [...]
>
> Maybe this really is a limitation of ruby, or maybe I'm missing
> something, but doesn't it support just opening a process without "sh -c"
> and piping the output to your current process, as this perl command
> which makes use of execve() will do:
>
>     $ perl -Mautodie=:all -wE '
>         my $i = 0; my $lim = shift;
>         open my $fh, "-|", qw(git show-ref master);
>         while (<$fh>) {
>             last if $i++ >= $lim;
>             print "$.: $_";
>         };' 10
>
> Some quick searching for docs online suggests that Ruby's Open3 and/or
> Process.spawn might be the equivalent.

To be clear, Ruby _does_ support something similar to what you
demonstrated in Perl above, it just isn't easily accessible to our
current infrastructure for spawning Git commands.

> Isn't that something that would make this workaround unnecessary? Well,
> maybe not because...
>
> > [...]and the additional
> > overhead of creating a pipe and passing data through it instead of
> > writing directly to stdout.
>
> It wouldn't take care of this part, but I'm struggling to think of cases
> where you'd be running this in the context of github.com and not already
> need to capture the output of the command. I.e. surely you're already
> piping stdout/stderr into your program, no?

Right, there's already a pipe in place to capture the output, but here
I'm talking about an _additional_ pipe to feed `show-ref` first through
to `head` and _then_ back out to the buffer in the calling Ruby program.

Thanks,
Taylor

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

[Taylor Blau]

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

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

[Ævar Arnfjörð Bjarmason]

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!

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> 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 :)

Heh.  I actually do think that the proposed documentation is
correct; the implementation that excludes these two options
from the "count" feature is buggy.