{"thread":{"id":"57962","subject":"[PATCH 1/2] builtin/show-ref.c: rename `found_match` to `matches_nr`","startedAt":"2022-06-06T21:56:12Z","lastAt":"2022-06-08T16:10:42Z","messageCount":14,"participants":["Taylor Blau","Junio C Hamano","Ævar Arnfjörð Bjarmason","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"456756","messageId":"c02ec101ce70a94eb7cc6085249a40d5b30d1474.1654552560.git.me@ttaylorr.com","threadId":"57962","inReplyTo":"cover.1654552560.git.me@ttaylorr.com","subject":"[PATCH 1/2] builtin/show-ref.c: rename `found_match` to `matches_nr`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-06-06T21:56:05Z","receivedAt":"2022-06-06T21:56:12Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The variable `found_match` has been used since 358ddb62cf (Add \"git\nshow-ref\" builtin command, 2006-09-15) to keep track of whether or not\nany references were shown by `show-ref` so that the command could exit\nwith status code \"1\" when no references were presented to the caller.\n\n`found_match` has always been treated as a boolean, even though it is\nincremented every time a reference is shown. Prepare for the subsequent\npatch, which will treat this variable as a count, by renaming it\naccordingly.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/show-ref.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/show-ref.c b/builtin/show-ref.c\nindex 7f8a5332f8..17d5f98fe4 100644\n--- a/builtin/show-ref.c\n+++ b/builtin/show-ref.c\n@@ -14,7 +14,7 @@ static const char * const show_ref_usage[] = {\n \tNULL\n };\n \n-static int deref_tags, show_head, tags_only, heads_only, found_match, verify,\n+static int deref_tags, show_head, tags_only, heads_only, matches_nr, verify,\n \t   quiet, hash_only, abbrev, exclude_arg;\n static const char **pattern;\n static const char *exclude_existing_arg;\n@@ -78,7 +78,7 @@ static int show_ref(const char *refname, const struct object_id *oid,\n \t}\n \n match:\n-\tfound_match++;\n+\tmatches_nr++;\n \n \tshow_one(refname, oid);\n \n@@ -217,7 +217,7 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)\n \tif (show_head)\n \t\thead_ref(show_ref, NULL);\n \tfor_each_ref(show_ref, NULL);\n-\tif (!found_match) {\n+\tif (!matches_nr) {\n \t\tif (verify && !quiet)\n \t\t\tdie(\"No match\");\n \t\treturn 1;\n-- \n2.36.1.94.gb0d54bedca\n\n"},{"id":"456757","messageId":"cover.1654552560.git.me@ttaylorr.com","threadId":"57962","inReplyTo":null,"subject":"[PATCH 0/2] builtin/show-ref.c: support `--count` for limiting output","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-06-06T21:56:01Z","receivedAt":"2022-06-06T21:56:14Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This short patch series adds support for a new `--count` argument for limiting\nthe output of `show-ref` (à-la the `for-each-ref` option by the same name).\n\nThis is useful in contexts where a caller wants to avoid enumerating more\nreferences than necessary (e.g., they only care whether a tag exists, but not\nhow many or what they are called) but doesn't have control of the output stream\n(e.g., they are in Ruby and can't pipe the output to `head -n 1`).\n\nThe first patch is preparatory, and the second patch implements the new option.\nThis series doesn't conflict with my recent [1], and can be applied\nindependently.\n\nThanks in advance for your review!\n\n[1]: https://lore.kernel.org/git/3fa6932641f18d78156bbf60b1571383f2cb5046.1654293264.git.me@ttaylorr.com/\n\nTaylor Blau (2):\n  builtin/show-ref.c: rename `found_match` to `matches_nr`\n  builtin/show-ref.c: limit output with `--count`\n\n Documentation/git-show-ref.txt |  7 ++++++-\n builtin/show-ref.c             | 29 +++++++++++++++++++++++++----\n t/t1403-show-ref.sh            | 21 +++++++++++++++++++++\n 3 files changed, 52 insertions(+), 5 deletions(-)\n\n-- \n2.36.1.94.gb0d54bedca\n"},{"id":"456758","messageId":"3fcf1f555715e925385d37712ffe880bb869741e.1654552560.git.me@ttaylorr.com","threadId":"57962","inReplyTo":"cover.1654552560.git.me@ttaylorr.com","subject":"[PATCH 2/2] builtin/show-ref.c: limit output with `--count`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-06-06T21:56:07Z","receivedAt":"2022-06-06T21:56:16Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"`git show-ref` is a useful tool for answering existential questions\nabout whether or not certain (kinds of) references exist or not. For\nexample, to quickly determine whether or not a repository has any tags,\none could run:\n\n    $ git show-ref -q --tags | head -1\n\nand check whether there was any output.\n\nBut certain environments (like spawning Git processes from Ruby code) do\nnot have ergonomic ways to simulate sending SIGPIPE back to `show-ref`\nwhen they have seen enough output.\n\nCallers _could_ use `for-each-ref`, which has supports a `--count`\noption to limit output, but this is sub-par for latency-sensitive\ncallers since `for-each-ref` buffers all results before printing\nanything. In the above instance, even something like:\n\n    $ git for-each-ref --count=1 -- 'refs/tags/*'\n\nwill enumerate every tag in a repository before deciding to print just\nthe first one. (The current behavior exists to accommodate sorting the\noutput from for-each-ref, and could be improved. See [1] for previous\nwork in this area).\n\nIn the meantime, introduce support for a similar `--count` option in\n`show-ref`, so that callers can run:\n\n    $ git show-ref -q --tags --count=1\n\nto quickly determine whether a repository has any (e.g.) tags or not by\nonly enumerating at most one reference.\n\n(This option is currently \"incompatible\" with `--verify`, though there\nis no reason why the meaning of `--count` couldn't be extended to mean,\n\"with `--verify`, only verify `<n>` references\".)\n\n[1]: https://lore.kernel.org/git/YTNpQ7Od1U%2F5i0R7@coredump.intra.peff.net/\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/git-show-ref.txt |  7 ++++++-\n builtin/show-ref.c             | 23 ++++++++++++++++++++++-\n t/t1403-show-ref.sh            | 21 +++++++++++++++++++++\n 3 files changed, 49 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-show-ref.txt b/Documentation/git-show-ref.txt\nindex ab4d271925..28256c04dd 100644\n--- a/Documentation/git-show-ref.txt\n+++ b/Documentation/git-show-ref.txt\n@@ -10,7 +10,7 @@ SYNOPSIS\n [verse]\n 'git show-ref' [-q|--quiet] [--verify] [--head] [-d|--dereference]\n \t     [-s|--hash[=<n>]] [--abbrev[=<n>]] [--tags]\n-\t     [--heads] [--] [<pattern>...]\n+\t     [--heads] [--count=<n>] [--] [<pattern>...]\n 'git show-ref' --exclude-existing[=<pattern>]\n \n DESCRIPTION\n@@ -84,6 +84,11 @@ OPTIONS\n \t(4) ignore if refname is a ref that exists in the local repository;\n \t(5) otherwise output the line.\n \n+--count::\n+\n+\tDo not print more than `<n>` matching references, or print all\n+\treferences if `<n>` is 0. Incompatible with `--exclude-existing`\n+\tand `--verify`.\n \n <pattern>...::\n \ndiff --git a/builtin/show-ref.c b/builtin/show-ref.c\nindex 17d5f98fe4..f0af8e8eec 100644\n--- a/builtin/show-ref.c\n+++ b/builtin/show-ref.c\n@@ -15,7 +15,7 @@ static const char * const show_ref_usage[] = {\n };\n \n static int deref_tags, show_head, tags_only, heads_only, matches_nr, verify,\n-\t   quiet, hash_only, abbrev, exclude_arg;\n+\t   quiet, hash_only, abbrev, exclude_arg, max_count = 0;\n static const char **pattern;\n static const char *exclude_existing_arg;\n \n@@ -82,6 +82,8 @@ static int show_ref(const char *refname, const struct object_id *oid,\n \n \tshow_one(refname, oid);\n \n+\tif (max_count && matches_nr >= max_count)\n+\t\treturn -1; /* avoid opening any more refs */\n \treturn 0;\n }\n \n@@ -178,6 +180,7 @@ static const struct option show_ref_options[] = {\n \tOPT_CALLBACK_F(0, \"exclude-existing\", &exclude_existing_arg,\n \t\t       N_(\"pattern\"), N_(\"show refs from stdin that aren't in local repository\"),\n \t\t       PARSE_OPT_OPTARG | PARSE_OPT_NONEG, exclude_existing_callback),\n+\tOPT_INTEGER(0, \"count\", &max_count, N_(\"show only <n> matched refs\")),\n \tOPT_END()\n };\n \n@@ -188,6 +191,24 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, show_ref_options,\n \t\t\t     show_ref_usage, 0);\n \n+\tif (max_count) {\n+\t\tint compatible = 0;\n+\n+\t\tif (max_count < 0)\n+\t\t\terror(_(\"invalid --count argument: (`%d' < 0)\"),\n+\t\t\t      max_count);\n+\t\telse if (verify)\n+\t\t\terror(_(\"--count is incompatible with %s\"), \"--verify\");\n+\t\telse if (exclude_arg)\n+\t\t\terror(_(\"--count is incompatible with %s\"),\n+\t\t\t      \"--exclude-existing\");\n+\t\telse\n+\t\t\tcompatible = 1;\n+\n+\t\tif (!compatible)\n+\t\t\tusage_with_options(show_ref_usage, show_ref_options);\n+\t}\n+\n \tif (exclude_arg)\n \t\treturn exclude_existing(exclude_existing_arg);\n \ndiff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh\nindex 9252a581ab..b79e114c1e 100755\n--- a/t/t1403-show-ref.sh\n+++ b/t/t1403-show-ref.sh\n@@ -196,4 +196,25 @@ test_expect_success 'show-ref --verify with dangling ref' '\n \t)\n '\n \n+test_expect_success 'show-ref --count limits relevant output' '\n+\tgit show-ref --heads --count=1 >out &&\n+\ttest_line_count = 1 out\n+'\n+\n+test_expect_success 'show-ref --count rejects invalid input' '\n+\ttest_must_fail git show-ref --count=-1 2>err &&\n+\tgrep \"invalid ..count argument: (.-1. < 0)\" err\n+'\n+\n+test_expect_success 'show-ref --count incompatible with --verify' '\n+\ttest_must_fail git show-ref --count=1 --verify HEAD 2>err &&\n+\tgrep \"..count is incompatible with ..verify\" err\n+'\n+\n+test_expect_success 'show-ref --count incompatible with --exclude-existing' '\n+\techo \"refs/heads/main\" >in &&\n+\ttest_must_fail git show-ref --count=1 --exclude-existing <in 2>err &&\n+\tgrep \"..count is incompatible with ..exclude.existing\" err\n+'\n+\n test_done\n-- \n2.36.1.94.gb0d54bedca\n"},{"id":"456761","messageId":"xmqqo7z54d7g.fsf@gitster.g","threadId":"57962","inReplyTo":"cover.1654552560.git.me@ttaylorr.com","subject":"Re: [PATCH 0/2] builtin/show-ref.c: support `--count` for limiting output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-06T22:52:19Z","receivedAt":"2022-06-06T22:52:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> This short patch series adds support for a new `--count` argument for limiting\n> the output of `show-ref` (à-la the `for-each-ref` option by the same name).\n\nIt makes me wonder why we limit this to show-ref.\n\n    $ git --pipe-to-head-N=3 any-command args...\n\nIOW, having to add an option like this feels absurd.\n\n> This is useful in contexts where a caller wants to avoid enumerating more\n> references than necessary (e.g., they only care whether a tag exists, but not\n> how many or what they are called) but doesn't have control of the output stream\n> (e.g., they are in Ruby and can't pipe the output to `head -n 1`).\n\nAre you saying that Ruby is incapable of run a command line like\n\n   av[0] = \"sh\"\n   av[1] = \"-c\"\n   av[2] = \"git show-ref blah | head -n 1\"\n   av[3] = NULL\n\n?\n"},{"id":"456762","messageId":"Yp6GRccc5UElNrZb@nand.local","threadId":"57962","inReplyTo":"xmqqo7z54d7g.fsf@gitster.g","subject":"Re: [PATCH 0/2] builtin/show-ref.c: support `--count` for limiting output","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-06-06T22:57:09Z","receivedAt":"2022-06-06T22:57:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Jun 06, 2022 at 03:52:19PM -0700, Junio C Hamano wrote:\n> Taylor Blau <me@ttaylorr.com> writes:\n>\n> > This short patch series adds support for a new `--count` argument for limiting\n> > the output of `show-ref` (à-la the `for-each-ref` option by the same name).\n>\n> It makes me wonder why we limit this to show-ref.\n>\n>     $ git --pipe-to-head-N=3 any-command args...\n>\n> IOW, having to add an option like this feels absurd.\n\nI don't disagree. But `--pipe-to-head-N=3` feels like too broad a\nstroke. This series at least imitates `for-each-ref`'s `--count`\noption, which makes it feel acceptable to me (if not a little silly).\n\n> > This is useful in contexts where a caller wants to avoid enumerating more\n> > references than necessary (e.g., they only care whether a tag exists, but not\n> > how many or what they are called) but doesn't have control of the output stream\n> > (e.g., they are in Ruby and can't pipe the output to `head -n 1`).\n>\n> Are you saying that Ruby is incapable of run a command line like\n>\n>    av[0] = \"sh\"\n>    av[1] = \"-c\"\n>    av[2] = \"git show-ref blah | head -n 1\"\n>    av[3] = NULL\n\nNo, Ruby is perfectly capable of doing that. But it involves an extra\nprocess (two, if `head` isn't a shell builtin) and the additional\noverhead of creating a pipe and passing data through it instead of\nwriting directly to stdout.\n\nThat isn't a complete show-stopper in most cases, but in\nultra-latency-sensitive applications like GitHub is using show-ref for,\nbeing able to shave an extra process or so off matters.\n\nIf you're strongly opposed to having `show-ref` match `for-each-ref`'s\n`--count` option, I won't be too sad. But I'm not in a huge rush to\nreplace this series with `git --pipe-to-head-N=<n>` either, FWIW.\n\nThanks,\nTaylor\n"},{"id":"456763","messageId":"xmqqk09t4cun.fsf@gitster.g","threadId":"57962","inReplyTo":"Yp6GRccc5UElNrZb@nand.local","subject":"Re: [PATCH 0/2] builtin/show-ref.c: support `--count` for limiting output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-06T23:00:00Z","receivedAt":"2022-06-06T23:00:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> If you're strongly opposed to having `show-ref` match `for-each-ref`'s\n> `--count` option, I won't be too sad. But I'm not in a huge rush to\n> replace this series with `git --pipe-to-head-N=<n>` either, FWIW.\n\nHeh, to me \"git --pipe-to-head-N=<n>\" smells equally absurd, too ;-)\n"},{"id":"456764","messageId":"xmqqczfl4ce1.fsf@gitster.g","threadId":"57962","inReplyTo":"3fcf1f555715e925385d37712ffe880bb869741e.1654552560.git.me@ttaylorr.com","subject":"Re: [PATCH 2/2] builtin/show-ref.c: limit output with `--count`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-06T23:09:58Z","receivedAt":"2022-06-06T23:10:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> +--count::\n> +\n> +\tDo not print more than `<n>` matching references, or print all\n> +\treferences if `<n>` is 0. Incompatible with `--exclude-existing`\n> +\tand `--verify`.\n\nI can easily understand why the option being incompatible with\n\"--verify\", but I do not see a reason why this should not work with\n\"--exclude-existing\" from end user's point of view.\n\nI know it is processed in a completely separate code path, but that\ndoes not stop end users from complaining.\n\n\n"},{"id":"456772","messageId":"220607.86r140vqc4.gmgdl@evledraar.gmail.com","threadId":"57962","inReplyTo":"3fcf1f555715e925385d37712ffe880bb869741e.1654552560.git.me@ttaylorr.com","subject":"Re: [PATCH 2/2] builtin/show-ref.c: limit output with `--count`","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-07T08:07:32Z","receivedAt":"2022-06-07T08:19:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jun 06 2022, Taylor Blau wrote:\n\n> diff --git a/Documentation/git-show-ref.txt b/Documentation/git-show-ref.txt\n> index ab4d271925..28256c04dd 100644\n> --- a/Documentation/git-show-ref.txt\n> +++ b/Documentation/git-show-ref.txt\n> @@ -10,7 +10,7 @@ SYNOPSIS\n>  [verse]\n>  'git show-ref' [-q|--quiet] [--verify] [--head] [-d|--dereference]\n>  \t     [-s|--hash[=<n>]] [--abbrev[=<n>]] [--tags]\n> -\t     [--heads] [--] [<pattern>...]\n> +\t     [--heads] [--count=<n>] [--] [<pattern>...]\n\nIn addition to what Junio noted, the SYNOPSIS is now inaccurate per your\ndocumentation. I.e. if this option is incompatible with --verify and\n--exclude-existing we should use \"|\" to indicate that, e.g.:\n\n\t[ [--verify] [--exclude-existing] | --count=<n> ]\n\n> +\tif (max_count) {\n> +\t\tint compatible = 0;\n> +\n> +\t\tif (max_count < 0)\n> +\t\t\terror(_(\"invalid --count argument: (`%d' < 0)\"),\n> +\t\t\t      max_count);\n> +\t\telse if (verify)\n> +\t\t\terror(_(\"--count is incompatible with %s\"), \"--verify\");\n> +\t\telse if (exclude_arg)\n> +\t\t\terror(_(\"--count is incompatible with %s\"),\n> +\t\t\t      \"--exclude-existing\");\n> +\t\telse\n> +\t\t\tcompatible = 1;\n> +\n> +\t\tif (!compatible)\n> +\t\t\tusage_with_options(show_ref_usage, show_ref_options);\n\nInstead of this \"int compatible\" and if/else-if\" just use usage_msg_optf().\n\nThat or die_for_incompatible_opt4(), at least the new _() messages\nshould make use of the same translations. I.e. we recently made these\nparameterized.\n\n> +\t}\n> +\n>  \tif (exclude_arg)\n>  \t\treturn exclude_existing(exclude_existing_arg);\n>  \n> diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh\n> index 9252a581ab..b79e114c1e 100755\n> --- a/t/t1403-show-ref.sh\n> +++ b/t/t1403-show-ref.sh\n> @@ -196,4 +196,25 @@ test_expect_success 'show-ref --verify with dangling ref' '\n>  \t)\n>  '\n>  \n> +test_expect_success 'show-ref --count limits relevant output' '\n> +\tgit show-ref --heads --count=1 >out &&\n> +\ttest_line_count = 1 out\n> +'\n> +\n> +test_expect_success 'show-ref --count rejects invalid input' '\n> +\ttest_must_fail git show-ref --count=-1 2>err &&\n> +\tgrep \"invalid ..count argument: (.-1. < 0)\" err\n\nThe use of .. here seems odd...\n\n> +'\n> +\n> +test_expect_success 'show-ref --count incompatible with --verify' '\n> +\ttest_must_fail git show-ref --count=1 --verify HEAD 2>err &&\n> +\tgrep \"..count is incompatible with ..verify\" err\n\n...i.e. this looks like a way to avoid \"--\" at the beginning, but then\nwhy use it in the middle of the regex?\n\n> +test_expect_success 'show-ref --count incompatible with --exclude-existing' '\n> +\techo \"refs/heads/main\" >in &&\n> +\ttest_must_fail git show-ref --count=1 --exclude-existing <in 2>err &&\n> +\tgrep \"..count is incompatible with ..exclude.existing\" err\n\nSeems like you could avoid it entirely by escaping it, e.g. \"[-]-\" at\nthe beginning, or in this case I think \"test_expect_code 129\" would be\nmore than sufficient, we use that to test \"had usage spewed at us\"\nelsewhere.\n"},{"id":"456773","messageId":"220607.86mteovpg2.gmgdl@evledraar.gmail.com","threadId":"57962","inReplyTo":"Yp6GRccc5UElNrZb@nand.local","subject":"Re: [PATCH 0/2] builtin/show-ref.c: support `--count` for limiting output","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-07T08:18:56Z","receivedAt":"2022-06-07T08:38:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jun 06 2022, Taylor Blau wrote:\n\n> On Mon, Jun 06, 2022 at 03:52:19PM -0700, Junio C Hamano wrote:\n>> Taylor Blau <me@ttaylorr.com> writes:\n>>\n>> > This short patch series adds support for a new `--count` argument for limiting\n>> > the output of `show-ref` (à-la the `for-each-ref` option by the same name).\n>>\n>> It makes me wonder why we limit this to show-ref.\n>>\n>>     $ git --pipe-to-head-N=3 any-command args...\n>>\n>> IOW, having to add an option like this feels absurd.\n>\n> I don't disagree. But `--pipe-to-head-N=3` feels like too broad a\n> stroke. This series at least imitates `for-each-ref`'s `--count`\n> option, which makes it feel acceptable to me (if not a little silly).\n\nYeah, although I do think it's worthwhile to think about where certain\nUX decisions are leading us, i.e. the logical conclusion here is to have\nevery command that emits >1 lines support --count, which as your patch\nhere shows needs special support, and even in your case you haven't\nimplemented it in a way that's compatible with all existing options.\n\nB.t.w. why would a --count for --verify not just by supported have these\nbe equivalent:\n\n    # same\n    git tag --count=3 --verify <name>\n    git tag --verify <name> | head -n 3\n\n>> > This is useful in contexts where a caller wants to avoid enumerating more\n>> > references than necessary (e.g., they only care whether a tag exists, but not\n>> > how many or what they are called) but doesn't have control of the output stream\n>> > (e.g., they are in Ruby and can't pipe the output to `head -n 1`).\n>>\n>> Are you saying that Ruby is incapable of run a command line like\n>>\n>>    av[0] = \"sh\"\n>>    av[1] = \"-c\"\n>>    av[2] = \"git show-ref blah | head -n 1\"\n>>    av[3] = NULL\n>\n> No, Ruby is perfectly capable of doing that. But it involves an extra\n> process (two, if `head` isn't a shell builtin) [...]\n\nMaybe this really is a limitation of ruby, or maybe I'm missing\nsomething, but doesn't it support just opening a process without \"sh -c\"\nand piping the output to your current process, as this perl command\nwhich makes use of execve() will do:\n\n    $ perl -Mautodie=:all -wE '\n        my $i = 0; my $lim = shift;\n        open my $fh, \"-|\", qw(git show-ref master);\n        while (<$fh>) {\n            last if $i++ >= $lim;\n            print \"$.: $_\";\n        };' 10\n\nSome quick searching for docs online suggests that Ruby's Open3 and/or\nProcess.spawn might be the equivalent.\n\nNote that if you replace that qw(git show-ref master) with e.g.:\n\n    \"git show-ref master | tail -n 20\"\n\nThat you'll get 3x execve(), one sh -c, another for \"git\" and another\nfor \"tail\", but in the first case you'll only get the execve().\n\nIsn't that something that would make this workaround unnecessary? Well,\nmaybe not because...\n\n> [...]and the additional\n> overhead of creating a pipe and passing data through it instead of\n> writing directly to stdout.\n\nIt wouldn't take care of this part, but I'm struggling to think of cases\nwhere you'd be running this in the context of github.com and not already\nneed to capture the output of the command. I.e. surely you're already\npiping stdout/stderr into your program, no?\n"},{"id":"456816","messageId":"73a973fa-cb44-b92e-e8c0-b68e14b4bd34@github.com","threadId":"57962","inReplyTo":"xmqqk09t4cun.fsf@gitster.g","subject":"Re: [PATCH 0/2] builtin/show-ref.c: support `--count` for limiting output","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-06-07T19:41:14Z","receivedAt":"2022-06-08T00:27:46Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/6/2022 7:00 PM, Junio C Hamano wrote:\n> Taylor Blau <me@ttaylorr.com> writes:\n> \n>> If you're strongly opposed to having `show-ref` match `for-each-ref`'s\n>> `--count` option, I won't be too sad. But I'm not in a huge rush to\n>> replace this series with `git --pipe-to-head-N=<n>` either, FWIW.\n> \n> Heh, to me \"git --pipe-to-head-N=<n>\" smells equally absurd, too ;-)\n\nI wonder if we could teach Git to skip an extra process if we see\n\n\tgit -c \"core.pager=head -n=<N>\" ...\n\n?\n\nThanks,\n-Stolee\n"},{"id":"456825","messageId":"Yp+/eXNg4tjiCn5a@nand.local","threadId":"57962","inReplyTo":"220607.86r140vqc4.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 2/2] builtin/show-ref.c: limit output with `--count`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-06-07T21:13:29Z","receivedAt":"2022-06-08T00:33:20Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jun 07, 2022 at 10:07:32AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n> On Mon, Jun 06 2022, Taylor Blau wrote:\n>\n> > diff --git a/Documentation/git-show-ref.txt b/Documentation/git-show-ref.txt\n> > index ab4d271925..28256c04dd 100644\n> > --- a/Documentation/git-show-ref.txt\n> > +++ b/Documentation/git-show-ref.txt\n> > @@ -10,7 +10,7 @@ SYNOPSIS\n> >  [verse]\n> >  'git show-ref' [-q|--quiet] [--verify] [--head] [-d|--dereference]\n> >  \t     [-s|--hash[=<n>]] [--abbrev[=<n>]] [--tags]\n> > -\t     [--heads] [--] [<pattern>...]\n> > +\t     [--heads] [--count=<n>] [--] [<pattern>...]\n>\n> In addition to what Junio noted, the SYNOPSIS is now inaccurate per your\n> documentation. I.e. if this option is incompatible with --verify and\n> --exclude-existing we should use \"|\" to indicate that, e.g.:\n>\n> \t[ [--verify] [--exclude-existing] | --count=<n> ]\n\nGood catch. Should this be squashed into the first example in the\nSYNOPSIS, the second, or a new one?\n\n> > +\tif (max_count) {\n> > +\t\tint compatible = 0;\n> > +\n> > +\t\tif (max_count < 0)\n> > +\t\t\terror(_(\"invalid --count argument: (`%d' < 0)\"),\n> > +\t\t\t      max_count);\n> > +\t\telse if (verify)\n> > +\t\t\terror(_(\"--count is incompatible with %s\"), \"--verify\");\n> > +\t\telse if (exclude_arg)\n> > +\t\t\terror(_(\"--count is incompatible with %s\"),\n> > +\t\t\t      \"--exclude-existing\");\n> > +\t\telse\n> > +\t\t\tcompatible = 1;\n> > +\n> > +\t\tif (!compatible)\n> > +\t\t\tusage_with_options(show_ref_usage, show_ref_options);\n>\n> Instead of this \"int compatible\" and if/else-if\" just use usage_msg_optf().\n>\n> That or die_for_incompatible_opt4(), at least the new _() messages\n> should make use of the same translations. I.e. we recently made these\n> parameterized.\n\nGood catch again. I wasn't aware of usage_msg_optf(), but it's exactly\nwhat I'm looking for here. It does mean that we'd only print one warning\nat a time, but I think that's a fair tradeoff, and unlikely to matter in\npractice anyways.\n\nAnd I must have dropped the parameterized msgids on the floor when\npreparing this patch, since I definitely have it locally. Oops, fixed.\n\n> > +\t}\n> > +\n> >  \tif (exclude_arg)\n> >  \t\treturn exclude_existing(exclude_existing_arg);\n> >\n> > diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh\n> > index 9252a581ab..b79e114c1e 100755\n> > --- a/t/t1403-show-ref.sh\n> > +++ b/t/t1403-show-ref.sh\n> > @@ -196,4 +196,25 @@ test_expect_success 'show-ref --verify with dangling ref' '\n> >  \t)\n> >  '\n> >\n> > +test_expect_success 'show-ref --count limits relevant output' '\n> > +\tgit show-ref --heads --count=1 >out &&\n> > +\ttest_line_count = 1 out\n> > +'\n> > +\n> > +test_expect_success 'show-ref --count rejects invalid input' '\n> > +\ttest_must_fail git show-ref --count=-1 2>err &&\n> > +\tgrep \"invalid ..count argument: (.-1. < 0)\" err\n>\n> The use of .. here seems odd...\n>\n> > +'\n> > +\n> > +test_expect_success 'show-ref --count incompatible with --verify' '\n> > +\ttest_must_fail git show-ref --count=1 --verify HEAD 2>err &&\n> > +\tgrep \"..count is incompatible with ..verify\" err\n>\n> ...i.e. this looks like a way to avoid \"--\" at the beginning, but then\n> why use it in the middle of the regex?\n\nMuscle memory ;).\n\n> > +test_expect_success 'show-ref --count incompatible with --exclude-existing' '\n> > +\techo \"refs/heads/main\" >in &&\n> > +\ttest_must_fail git show-ref --count=1 --exclude-existing <in 2>err &&\n> > +\tgrep \"..count is incompatible with ..exclude.existing\" err\n>\n> Seems like you could avoid it entirely by escaping it, e.g. \"[-]-\" at\n> the beginning, or in this case I think \"test_expect_code 129\" would be\n> more than sufficient, we use that to test \"had usage spewed at us\"\n> elsewhere.\n\nI like having the extra test to ensure the error we got made sense, but\nI agree either would work. I modified the grep expressions to replace\nleading \".\"'s with \"[-]\", and \".\"'s in the middle of the expression with\n\"-\".\n\nThanks,\nTaylor\n"},{"id":"456828","messageId":"Yp+9Wk6VH1BR9T9c@nand.local","threadId":"57962","inReplyTo":"220607.86mteovpg2.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 0/2] builtin/show-ref.c: support `--count` for limiting output","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-06-07T21:04:26Z","receivedAt":"2022-06-08T00:33:40Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jun 07, 2022 at 10:18:56AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n> On Mon, Jun 06 2022, Taylor Blau wrote:\n>\n> > On Mon, Jun 06, 2022 at 03:52:19PM -0700, Junio C Hamano wrote:\n> >> Taylor Blau <me@ttaylorr.com> writes:\n> >>\n> >> > This short patch series adds support for a new `--count` argument for limiting\n> >> > the output of `show-ref` (à-la the `for-each-ref` option by the same name).\n> >>\n> >> It makes me wonder why we limit this to show-ref.\n> >>\n> >>     $ git --pipe-to-head-N=3 any-command args...\n> >>\n> >> IOW, having to add an option like this feels absurd.\n> >\n> > I don't disagree. But `--pipe-to-head-N=3` feels like too broad a\n> > stroke. This series at least imitates `for-each-ref`'s `--count`\n> > option, which makes it feel acceptable to me (if not a little silly).\n>\n> Yeah, although I do think it's worthwhile to think about where certain\n> UX decisions are leading us, i.e. the logical conclusion here is to have\n> every command that emits >1 lines support --count, which as your patch\n> here shows needs special support, and even in your case you haven't\n> implemented it in a way that's compatible with all existing options.\n\nTo be clear, I don't think adding `--count` to every command is a good\nidea. But it exists in `for-each-ref`, and not in `show-ref`, and this\nseries rectifies that gap in functionality. Perhaps `for-each-ref`\nshouldn't have `--count`, but it does, and has since that command's\ninception.\n\n> B.t.w. why would a --count for --verify not just by supported have these\n> be equivalent:\n>\n>     # same\n>     git tag --count=3 --verify <name>\n>     git tag --verify <name> | head -n 3\n\n(I'm not sure if you meant \"git tag\" here versus \"git show-ref\", but\neither way), `show-ref` in `--verify` mode outputs one line of output\nper line of input, so a caller can easily limit the output by limiting\nthe input.\n\n> >> > This is useful in contexts where a caller wants to avoid enumerating more\n> >> > references than necessary (e.g., they only care whether a tag exists, but not\n> >> > how many or what they are called) but doesn't have control of the output stream\n> >> > (e.g., they are in Ruby and can't pipe the output to `head -n 1`).\n> >>\n> >> Are you saying that Ruby is incapable of run a command line like\n> >>\n> >>    av[0] = \"sh\"\n> >>    av[1] = \"-c\"\n> >>    av[2] = \"git show-ref blah | head -n 1\"\n> >>    av[3] = NULL\n> >\n> > No, Ruby is perfectly capable of doing that. But it involves an extra\n> > process (two, if `head` isn't a shell builtin) [...]\n>\n> Maybe this really is a limitation of ruby, or maybe I'm missing\n> something, but doesn't it support just opening a process without \"sh -c\"\n> and piping the output to your current process, as this perl command\n> which makes use of execve() will do:\n>\n>     $ perl -Mautodie=:all -wE '\n>         my $i = 0; my $lim = shift;\n>         open my $fh, \"-|\", qw(git show-ref master);\n>         while (<$fh>) {\n>             last if $i++ >= $lim;\n>             print \"$.: $_\";\n>         };' 10\n>\n> Some quick searching for docs online suggests that Ruby's Open3 and/or\n> Process.spawn might be the equivalent.\n\nTo be clear, Ruby _does_ support something similar to what you\ndemonstrated in Perl above, it just isn't easily accessible to our\ncurrent infrastructure for spawning Git commands.\n\n> Isn't that something that would make this workaround unnecessary? Well,\n> maybe not because...\n>\n> > [...]and the additional\n> > overhead of creating a pipe and passing data through it instead of\n> > writing directly to stdout.\n>\n> It wouldn't take care of this part, but I'm struggling to think of cases\n> where you'd be running this in the context of github.com and not already\n> need to capture the output of the command. I.e. surely you're already\n> piping stdout/stderr into your program, no?\n\nRight, there's already a pipe in place to capture the output, but here\nI'm talking about an _additional_ pipe to feed `show-ref` first through\nto `head` and _then_ back out to the buffer in the calling Ruby program.\n\nThanks,\nTaylor\n"},{"id":"456853","messageId":"220607.86y1y8tb1o.gmgdl@evledraar.gmail.com","threadId":"57962","inReplyTo":"Yp+/eXNg4tjiCn5a@nand.local","subject":"Re: [PATCH 2/2] builtin/show-ref.c: limit output with `--count`","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-07T21:31:28Z","receivedAt":"2022-06-08T03:36:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jun 07 2022, Taylor Blau wrote:\n\n> On Tue, Jun 07, 2022 at 10:07:32AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>>\n>> On Mon, Jun 06 2022, Taylor Blau wrote:\n>>\n>> > diff --git a/Documentation/git-show-ref.txt b/Documentation/git-show-ref.txt\n>> > index ab4d271925..28256c04dd 100644\n>> > --- a/Documentation/git-show-ref.txt\n>> > +++ b/Documentation/git-show-ref.txt\n>> > @@ -10,7 +10,7 @@ SYNOPSIS\n>> >  [verse]\n>> >  'git show-ref' [-q|--quiet] [--verify] [--head] [-d|--dereference]\n>> >  \t     [-s|--hash[=<n>]] [--abbrev[=<n>]] [--tags]\n>> > -\t     [--heads] [--] [<pattern>...]\n>> > +\t     [--heads] [--count=<n>] [--] [<pattern>...]\n>>\n>> In addition to what Junio noted, the SYNOPSIS is now inaccurate per your\n>> documentation. I.e. if this option is incompatible with --verify and\n>> --exclude-existing we should use \"|\" to indicate that, e.g.:\n>>\n>> \t[ [--verify] [--exclude-existing] | --count=<n> ]\n>\n> Good catch. Should this be squashed into the first example in the\n> SYNOPSIS, the second, or a new one?\n\nPersonally I really don't care if the end-state is good :)\n\n>> > +\tif (max_count) {\n>> > +\t\tint compatible = 0;\n>> > +\n>> > +\t\tif (max_count < 0)\n>> > +\t\t\terror(_(\"invalid --count argument: (`%d' < 0)\"),\n>> > +\t\t\t      max_count);\n>> > +\t\telse if (verify)\n>> > +\t\t\terror(_(\"--count is incompatible with %s\"), \"--verify\");\n>> > +\t\telse if (exclude_arg)\n>> > +\t\t\terror(_(\"--count is incompatible with %s\"),\n>> > +\t\t\t      \"--exclude-existing\");\n>> > +\t\telse\n>> > +\t\t\tcompatible = 1;\n>> > +\n>> > +\t\tif (!compatible)\n>> > +\t\t\tusage_with_options(show_ref_usage, show_ref_options);\n>>\n>> Instead of this \"int compatible\" and if/else-if\" just use usage_msg_optf().\n>>\n>> That or die_for_incompatible_opt4(), at least the new _() messages\n>> should make use of the same translations. I.e. we recently made these\n>> parameterized.\n>\n> Good catch again. I wasn't aware of usage_msg_optf(), but it's exactly\n> what I'm looking for here. It does mean that we'd only print one warning\n> at a time, but I think that's a fair tradeoff, and unlikely to matter in\n> practice anyways.\n\nYeah, I think that should be OK. We do that in other cases.\n\n> And I must have dropped the parameterized msgids on the floor when\n> preparing this patch, since I definitely have it locally. Oops, fixed.\n\n*nod*\n\n>> > +\t}\n>> > +\n>> >  \tif (exclude_arg)\n>> >  \t\treturn exclude_existing(exclude_existing_arg);\n>> >\n>> > diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh\n>> > index 9252a581ab..b79e114c1e 100755\n>> > --- a/t/t1403-show-ref.sh\n>> > +++ b/t/t1403-show-ref.sh\n>> > @@ -196,4 +196,25 @@ test_expect_success 'show-ref --verify with dangling ref' '\n>> >  \t)\n>> >  '\n>> >\n>> > +test_expect_success 'show-ref --count limits relevant output' '\n>> > +\tgit show-ref --heads --count=1 >out &&\n>> > +\ttest_line_count = 1 out\n>> > +'\n>> > +\n>> > +test_expect_success 'show-ref --count rejects invalid input' '\n>> > +\ttest_must_fail git show-ref --count=-1 2>err &&\n>> > +\tgrep \"invalid ..count argument: (.-1. < 0)\" err\n>>\n>> The use of .. here seems odd...\n>>\n>> > +'\n>> > +\n>> > +test_expect_success 'show-ref --count incompatible with --verify' '\n>> > +\ttest_must_fail git show-ref --count=1 --verify HEAD 2>err &&\n>> > +\tgrep \"..count is incompatible with ..verify\" err\n>>\n>> ...i.e. this looks like a way to avoid \"--\" at the beginning, but then\n>> why use it in the middle of the regex?\n>\n> Muscle memory ;).\n>\n>> > +test_expect_success 'show-ref --count incompatible with --exclude-existing' '\n>> > +\techo \"refs/heads/main\" >in &&\n>> > +\ttest_must_fail git show-ref --count=1 --exclude-existing <in 2>err &&\n>> > +\tgrep \"..count is incompatible with ..exclude.existing\" err\n>>\n>> Seems like you could avoid it entirely by escaping it, e.g. \"[-]-\" at\n>> the beginning, or in this case I think \"test_expect_code 129\" would be\n>> more than sufficient, we use that to test \"had usage spewed at us\"\n>> elsewhere.\n>\n> I like having the extra test to ensure the error we got made sense, but\n> I agree either would work. I modified the grep expressions to replace\n> leading \".\"'s with \"[-]\", and \".\"'s in the middle of the expression with\n> \"-\".\n\nYeah, fair enough, thanks!\n"},{"id":"456872","messageId":"xmqq35gfrv9o.fsf@gitster.g","threadId":"57962","inReplyTo":"220607.86y1y8tb1o.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 2/2] builtin/show-ref.c: limit output with `--count`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-08T16:10:27Z","receivedAt":"2022-06-08T16:10:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> On Tue, Jun 07 2022, Taylor Blau wrote:\n>\n>> On Tue, Jun 07, 2022 at 10:07:32AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>>>\n>>> On Mon, Jun 06 2022, Taylor Blau wrote:\n>>>\n>>> > diff --git a/Documentation/git-show-ref.txt b/Documentation/git-show-ref.txt\n>>> > index ab4d271925..28256c04dd 100644\n>>> > --- a/Documentation/git-show-ref.txt\n>>> > +++ b/Documentation/git-show-ref.txt\n>>> > @@ -10,7 +10,7 @@ SYNOPSIS\n>>> >  [verse]\n>>> >  'git show-ref' [-q|--quiet] [--verify] [--head] [-d|--dereference]\n>>> >  \t     [-s|--hash[=<n>]] [--abbrev[=<n>]] [--tags]\n>>> > -\t     [--heads] [--] [<pattern>...]\n>>> > +\t     [--heads] [--count=<n>] [--] [<pattern>...]\n>>>\n>>> In addition to what Junio noted, the SYNOPSIS is now inaccurate per your\n>>> documentation. I.e. if this option is incompatible with --verify and\n>>> --exclude-existing we should use \"|\" to indicate that, e.g.:\n>>>\n>>> \t[ [--verify] [--exclude-existing] | --count=<n> ]\n>>\n>> Good catch. Should this be squashed into the first example in the\n>> SYNOPSIS, the second, or a new one?\n>\n> Personally I really don't care if the end-state is good :)\n\nHeh.  I actually do think that the proposed documentation is\ncorrect; the implementation that excludes these two options\nfrom the \"count\" feature is buggy.\n"}]}