builtin/show-ref.c: support `--count` for limiting output
PATCH 1/2 builtin/show-ref.c: rename `found_match` to `matches_nr`
In reply to Taylor BlauThe 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.
--- builtin/show-ref.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
builtin/show-ref.c +3 -3
@@ -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 ;
Signature · 1 line
2.36.1.94.gb0d54bedca
cover builtin/show-ref.c: support `--count` for limiting output
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(-)
Signature · 1 line
2.36.1.94.gb0d54bedca
PATCH 2/2 builtin/show-ref.c: limit output with `--count`
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%[email protected]/
--- Documentation/git-show-ref.txt | 7 ++++++- builtin/show-ref.c | 23 ++++++++++++++++++++++- t/t1403-show-ref.sh | 21 +++++++++++++++++++++ 3 files changed, 49 insertions(+), 2 deletions(-)
Documentation/git-show-ref.txt +6 -1
@@ -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>...::
builtin/show-ref.c +22 -1
@@ -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);
t/t1403-show-ref.sh +21 -0
@@ -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
Signature · 1 line
2.36.1.94.gb0d54bedca
Quoted 2 lines · Taylor Blau
This short patch series adds support for a new
--countargument for limiting the output ofshow-ref(à-la thefor-each-refoption 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.
Quoted 4 lines
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
?
Quoted 10 lines · Junio C Hamano
Quoted 2 lines · Taylor Blau
This short patch series adds support for a new
--countargument for limiting the output ofshow-ref(à-la thefor-each-refoption 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).
Quoted 11 lines
Quoted 4 lines
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
Quoted 3 lines · Taylor Blau
If you're strongly opposed to having
show-refmatchfor-each-ref's--countoption, I won't be too sad. But I'm not in a huge rush to replace this series withgit --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`
In reply to Taylor BlauDocumentation/git-show-ref.txt OPTIONS · line 91
Quoted 5 lines · Taylor Blau
+--count::
Documentation/git-show-ref.txt +4
-0+ + 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`
In reply to Taylor BlauDocumentation/git-show-ref.txt SYNOPSIS · line 13
Quoted 10 lines · Taylor Blau
Documentation/git-show-ref.txt +1
-1@@ -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> ]
Quoted 6 lines
Documentation/git-show-ref.txt +6
-0+ if (max_count) { + int compatible = 0; + + if (max_count < 0) + error(_("invalid --count argument: (`%d' < 0)"), + max_count);
Documentation/git-show-ref.txt SYNOPSIS
Quoted 10 lines
Documentation/git-show-ref.txt +10
-0+ 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.
Quoted 11 lines
Documentation/git-show-ref.txt +2
-0+ } + if (exclude_arg) return exclude_existing(exclude_existing_arg);t/t1403-show-ref.sh +0
-0@@ -196,4 +196,25 @@ test_expect_success 'show-ref --verify with dangling ref' ' )
t/t1403-show-ref.sh test_expect_success 'show-ref --verify with dangling ref' ' · line 206
Quoted 10 lines
t/t1403-show-ref.sh +8
-0' +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...
t/t1403-show-ref.sh test_expect_success 'show-ref --verify with dangling ref' ' · line 211
Quoted 5 lines
t/t1403-show-ref.sh +5
-0+' + +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?
t/t1403-show-ref.sh test_expect_success 'show-ref --verify with dangling ref' ' · line 217
Quoted 4 lines
t/t1403-show-ref.sh +4
-0+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.
Quoted 8 lines · Taylor Blau
Quoted 7 lines · Junio C Hamano
Quoted 2 lines · Taylor Blau
This short patch series adds support for a new
--countargument for limiting the output ofshow-ref(à-la thefor-each-refoption by the same name).It makes me wonder why we limit this to show-ref.
Quoted 7 lines
Quoted 3 lines
$ 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=3feels like too broad a stroke. This series at least imitatesfor-each-ref's--countoption, 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 3Quoted 7 lines
Quoted 7 lines
Quoted 4 lines
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
Quoted 7 lines
Quoted 4 lines
av[0] = "sh" av[1] = "-c" av[2] = "git show-ref blah | head -n 1" av[3] = NULLNo, Ruby is perfectly capable of doing that. But it involves an extra process (two, if
headisn'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 "$.: $_";
};' 10Some 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...
Quoted 3 lines
[...]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?
Quoted 7 lines · Junio C Hamano
Quoted 3 lines · Taylor Blau
If you're strongly opposed to having
show-refmatchfor-each-ref's--countoption, I won't be too sad. But I'm not in a huge rush to replace this series withgit --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 2/2] builtin/show-ref.c: limit output with `--count`
In reply to Ævar Arnfjörð BjarmasonQuoted 18 lines · Ævar Arnfjörð Bjarmason
Documentation/git-show-ref.txt SYNOPSIS · line 13
Quoted 10 lines · Taylor Blau
Documentation/git-show-ref.txt +1
-1@@ -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.:
Quoted 1 line
[ [--verify] [--exclude-existing] | --count=<n> ]
Good catch. Should this be squashed into the first example in the SYNOPSIS, the second, or a new one?
Quoted 19 lines
Quoted 6 lines
Documentation/git-show-ref.txt +6
-0+ if (max_count) { + int compatible = 0; + + if (max_count < 0) + error(_("invalid --count argument: (`%d' < 0)"), + max_count);Documentation/git-show-ref.txt SYNOPSIS
Quoted 10 lines
Documentation/git-show-ref.txt +10
-0+ 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().
Quoted 3 lines
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.
Quoted 24 lines
Quoted 11 lines
Documentation/git-show-ref.txt +2
-0+ } + if (exclude_arg) return exclude_existing(exclude_existing_arg);t/t1403-show-ref.sh +0
-0@@ -196,4 +196,25 @@ test_expect_success 'show-ref --verify with dangling ref' ' )t/t1403-show-ref.sh test_expect_success 'show-ref --verify with dangling ref' ' · line 206
Quoted 10 lines
t/t1403-show-ref.sh +8
-0' +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)" errThe use of .. here seems odd...
Quoted 8 lines
t/t1403-show-ref.sh test_expect_success 'show-ref --verify with dangling ref' ' · line 211
Quoted 5 lines
t/t1403-show-ref.sh +5
-0+' + +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 ;).
Quoted 9 lines
t/t1403-show-ref.sh test_expect_success 'show-ref --verify with dangling ref' ' · line 217
Quoted 4 lines
t/t1403-show-ref.sh +4
-0+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" errSeems like you could avoid it entirely by escaping it, e.g. "[-]-" at the beginning, or in this case I think "
test_expect_code129" 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
Quoted 19 lines · Ævar Arnfjörð Bjarmason
Quoted 8 lines · Taylor Blau
Quoted 7 lines · Junio C Hamano
Quoted 2 lines · Taylor Blau
This short patch series adds support for a new
--countargument for limiting the output ofshow-ref(à-la thefor-each-refoption by the same name).It makes me wonder why we limit this to show-ref.
Quoted 7 lines
Quoted 3 lines
$ 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=3feels like too broad a stroke. This series at least imitatesfor-each-ref's--countoption, which makes it feel acceptable to me (if not a little silly).
Quoted 5 lines
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.
Quoted 6 lines
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.
Quoted 28 lines
Quoted 7 lines
Quoted 7 lines
Quoted 4 lines
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
Quoted 7 lines
Quoted 4 lines
av[0] = "sh" av[1] = "-c" av[2] = "git show-ref blah | head -n 1" av[3] = NULLNo, Ruby is perfectly capable of doing that. But it involves an extra process (two, if
headisn'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
Quoted 2 lines
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.
Quoted 11 lines
Isn't that something that would make this workaround unnecessary? Well, maybe not because...
Quoted 3 lines
[...]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`
In reply to Taylor BlauQuoted 19 lines · Taylor Blau
Quoted 18 lines · Ævar Arnfjörð Bjarmason
Documentation/git-show-ref.txt SYNOPSIS · line 13
Quoted 10 lines · Taylor Blau
Documentation/git-show-ref.txt +1
-1@@ -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.:
Quoted 4 lines
Quoted 1 line
[ [--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 :)
Quoted 19 lines
Quoted 19 lines
Quoted 6 lines
Documentation/git-show-ref.txt +6
-0+ if (max_count) { + int compatible = 0; + + if (max_count < 0) + error(_("invalid --count argument: (`%d' < 0)"), + max_count);Documentation/git-show-ref.txt SYNOPSIS
Quoted 10 lines
Documentation/git-show-ref.txt +10
-0+ 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().
Quoted 8 lines
Quoted 3 lines
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.
Quoted 2 lines
And I must have dropped the parameterized msgids on the floor when preparing this patch, since I definitely have it locally. Oops, fixed.
*nod*
Quoted 45 lines
Quoted 24 lines
Quoted 11 lines
Documentation/git-show-ref.txt +2
-0+ } + if (exclude_arg) return exclude_existing(exclude_existing_arg);t/t1403-show-ref.sh +0
-0@@ -196,4 +196,25 @@ test_expect_success 'show-ref --verify with dangling ref' ' )t/t1403-show-ref.sh test_expect_success 'show-ref --verify with dangling ref' ' · line 206
Quoted 10 lines
t/t1403-show-ref.sh +8
-0' +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)" errThe use of .. here seems odd...
Quoted 8 lines
t/t1403-show-ref.sh test_expect_success 'show-ref --verify with dangling ref' ' · line 211
Quoted 5 lines
t/t1403-show-ref.sh +5
-0+' + +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 ;).
Quoted 9 lines
t/t1403-show-ref.sh test_expect_success 'show-ref --verify with dangling ref' ' · line 217
Quoted 4 lines
t/t1403-show-ref.sh +4
-0+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" errSeems like you could avoid it entirely by escaping it, e.g. "[-]-" at the beginning, or in this case I think "
test_expect_code129" would be more than sufficient, we use that to test "had usage spewed at us" elsewhere.
Quoted 4 lines
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`
Quoted 21 lines · Ævar Arnfjörð Bjarmason
Quoted 19 lines · Taylor Blau
Quoted 18 lines · Ævar Arnfjörð Bjarmason
Documentation/git-show-ref.txt SYNOPSIS · line 13
Quoted 10 lines · Taylor Blau
Documentation/git-show-ref.txt +1
-1@@ -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.:
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
[ [--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.