{"thread":{"id":"51821","subject":"[RFC PATCH 0/1] for-each-ref: do not output empty lines","startedAt":"2019-09-10T05:16:40Z","lastAt":"2019-09-10T16:35:43Z","messageCount":5,"participants":["Eric Freese","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"382113","messageId":"20190910051705.2765-1-ericdfreese@gmail.com","threadId":"51821","inReplyTo":null,"subject":"[RFC PATCH 0/1] for-each-ref: do not output empty lines","fromName":"Eric Freese","fromEmail":"ericdfreese@gmail.com","sentAt":"2019-09-10T05:17:04Z","receivedAt":"2019-09-10T05:16:40Z","isPatch":true,"sender":{"key":"ericdfreese@gmail.com","avatar":"https://gravatar.com/avatar/9a57c69e58170c5f4b54051c9df2c2fdb51ac99e53f847868317ba042302bfda?d=mp&s=160"},"body":"Hi,\n\nThis is a follow-up patch based on conversation from the thread: \"[RFC\nPATCH 1/1] for-each-ref: add '--no-symbolic' option\"\n\nI had proposed a `--no-symbolic` option to git-for-each-ref that would\nfilter out symbolic refs from the output. It was discovered that the\nbehavior was already possible using an `%(if)` atom, but with the small\ncaveat that empty lines would be output for each non-symbolic ref.\n\nThis patch changes the output behavior of git-for-each-ref such that it\nonly prints lines for refs for which the format string expands to a\nnon-empty string.\n\nIf this is deemed to be too disruptive of a change, a new command option\ncould be added to opt in to the new behavior.\n\nCheers\n\nEric Freese (1):\n for-each-ref: do not output empty lines\n\n ref-filter.c | 3 ++-\n t/t6300-for-each-ref.sh | 4 ++--\n 2 files changed, 4 insertions(+), 3 deletions(-)\n\n-- \n2.23.0\n\n"},{"id":"382114","messageId":"20190910051705.2765-2-ericdfreese@gmail.com","threadId":"51821","inReplyTo":"20190910051705.2765-1-ericdfreese@gmail.com","subject":"[RFC PATCH 1/1] for-each-ref: do not output empty lines","fromName":"Eric Freese","fromEmail":"ericdfreese@gmail.com","sentAt":"2019-09-10T05:17:05Z","receivedAt":"2019-09-10T05:17:12Z","isPatch":true,"sender":{"key":"ericdfreese@gmail.com","avatar":"https://gravatar.com/avatar/9a57c69e58170c5f4b54051c9df2c2fdb51ac99e53f847868317ba042302bfda?d=mp&s=160"},"body":"If the format string expands to an empty string for a given ref, do not\nprint the empty line.\n\nThis is helpful when wanting to print only certain kinds of refs that\nyou can't already filter for.\n\nFor example, to exclude symbolic refs, use format string:\n \"%(if)%(symref)%(then)%(else)%(refname)%(end)\"\n\nOr to include only symbolic refs, use:\n \"%(if)%(symref)%(then)%(refname)%(end)\"\n\nSigned-off-by: Eric Freese \n---\n ref-filter.c | 3 ++-\n t/t6300-for-each-ref.sh | 4 ++--\n 2 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 7338cfc671..b5b3c4ddf6 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2395,9 +2395,10 @@ void show_ref_array_item(struct ref_array_item *info,\n \tif (format_ref_array_item(info, format, &final_buf, &error_buf))\n \t\tdie(\"%s\", error_buf.buf);\n \tfwrite(final_buf.buf, 1, final_buf.len, stdout);\n+\tif (final_buf.len)\n+\t\tputchar('\\n');\n \tstrbuf_release(&error_buf);\n \tstrbuf_release(&final_buf);\n-\tputchar('\\n');\n }\n \n void pretty_print_ref(const char *name, const struct object_id *oid,\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 9c910ce746..1f070e63e2 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -42,7 +42,7 @@ test_atom() {\n \t\t sym) ref=refs/heads/sym ;;\n \t\t *) ref=$1 ;;\n \tesac\n-\tprintf '%s\\n' \"$3\" >expected\n+\t{ test -n \"$3\" && printf '%s\\n' \"$3\"; } >expected\n \ttest_expect_${4:-success} $PREREQ \"basic atom: $1 $2\" \"\n \t\tgit for-each-ref --format='%($2)' $ref >actual &&\n \t\tsanitize_pgp actual.clean &&\n@@ -313,7 +313,7 @@ test_expect_success 'Check format of strftime-local date fields' '\n '\n \n test_expect_success 'exercise strftime with odd fields' '\n-\techo >expected &&\n+\t>expected &&\n \tgit for-each-ref --format=\"%(authordate:format:)\" refs/heads >actual &&\n \ttest_cmp expected actual &&\n \tlong=\"long format -- $ZERO_OID$ZERO_OID$ZERO_OID$ZERO_OID$ZERO_OID$ZERO_OID$ZERO_OID\" &&\n-- \n2.23.0\n\n"},{"id":"382115","messageId":"xmqqpnk8heow.fsf@gitster-ct.c.googlers.com","threadId":"51821","inReplyTo":"20190910051705.2765-2-ericdfreese@gmail.com","subject":"Re: [RFC PATCH 1/1] for-each-ref: do not output empty lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-10T06:02:07Z","receivedAt":"2019-09-10T06:02:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Freese writes:\n\n> If the format string expands to an empty string for a given ref, do not\n> print the empty line.\n>\n> This is helpful when wanting to print only certain kinds of refs that\n> you can't already filter for.\n\nWe tend to prefer stating the reason why we want to do so first and\nthen give a command to the codebase to \"become like so\". Here is to\nillustrate how you would do it:\n\n The custom format specifier \"--format=\" can be used to\n tell the for-each-ref command to say nothing for certain kind of\n refs, e.g.\n\n --format=\"%(if)%(symref)%(then)%(else)%(refname)%(end)\"\n\n may be used to show the refname only for refs that are not\n symbolic refs. Except that the command still would show one\n blank line per each symbolic ref, which is fairly useless.\n\n Introduce the `--omit-empty-lines` option to squelch these\n useless lines from the output.\n\n\n> @@ -2395,9 +2395,10 @@ void show_ref_array_item(struct ref_array_item *info,\n> \tif (format_ref_array_item(info, format, &final_buf, &error_buf))\n> \t\tdie(\"%s\", error_buf.buf);\n> \tfwrite(final_buf.buf, 1, final_buf.len, stdout);\n> +\tif (final_buf.len)\n> +\t\tputchar('\\n');\n\nWhile we are introducing a conditional, let's drop the useless\nfwrite of 0-byte while we are at it [*1*], i.e.\n\n\tif (final_buf.len && !omit_empty_lines) {\n\t\tfwrite(final_buf.buf, 1, final_buf.len, stdout);\n\t\tputchar('\\n');\n\t}\n\nThanks.\n\n\n[Footnote]\n\n*1* \"While we are at it\", the existing code tempts me to drop fwrite\n and replace it with something along the lines of...\n\n\tprintf(\"%*s\\n\", count, buf)\n\n but I refrained from doing so. An enhancement patch like this\n is not a place to \"improve\" existing code (which should be done\n as a separate patch).\n"},{"id":"382116","messageId":"xmqqlfuwhe46.fsf@gitster-ct.c.googlers.com","threadId":"51821","inReplyTo":"xmqqpnk8heow.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH 1/1] for-each-ref: do not output empty lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-10T06:14:33Z","receivedAt":"2019-09-10T06:14:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> \tfwrite(final_buf.buf, 1, final_buf.len, stdout);\n>> +\tif (final_buf.len)\n>> +\t\tputchar('\\n');\n>\n> While we are introducing a conditional, let's drop the useless\n> fwrite of 0-byte while we are at it [*1*], i.e.\n>\n> \tif (final_buf.len && !omit_empty_lines) {\n\nOf course, that's a typo for \"||\"; if it is not empty, we'd emit no\nmatter what, and if omit_empty is not given, we'd emit whether it is\nempty or not.\n\n> \t\tfwrite(final_buf.buf, 1, final_buf.len, stdout);\n> \t\tputchar('\\n');\n> \t}\n\n\n\n>\n> Thanks.\n>\n>\n> [Footnote]\n>\n> *1* \"While we are at it\", the existing code tempts me to drop fwrite\n> and replace it with something along the lines of...\n>\n> \tprintf(\"%*s\\n\", count, buf)\n>\n> but I refrained from doing so. An enhancement patch like this\n> is not a place to \"improve\" existing code (which should be done\n> as a separate patch).\n"},{"id":"382137","messageId":"20190910163541.GA13873@sigill.intra.peff.net","threadId":"51821","inReplyTo":"xmqqpnk8heow.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH 1/1] for-each-ref: do not output empty lines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-10T16:35:42Z","receivedAt":"2019-09-10T16:35:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 09, 2019 at 11:02:07PM -0700, Junio C Hamano wrote:\n\n> Eric Freese writes:\n> \n> > If the format string expands to an empty string for a given ref, do not\n> > print the empty line.\n> >\n> > This is helpful when wanting to print only certain kinds of refs that\n> > you can't already filter for.\n> \n> We tend to prefer stating the reason why we want to do so first and\n> then give a command to the codebase to \"become like so\". Here is to\n> illustrate how you would do it:\n> \n> The custom format specifier \"--format=\" can be used to\n> tell the for-each-ref command to say nothing for certain kind of\n> refs, e.g.\n> \n> --format=\"%(if)%(symref)%(then)%(else)%(refname)%(end)\"\n> \n> may be used to show the refname only for refs that are not\n> symbolic refs. Except that the command still would show one\n> blank line per each symbolic ref, which is fairly useless.\n> \n> Introduce the `--omit-empty-lines` option to squelch these\n> useless lines from the output.\n\nYour proposed commit message (and the suggested code below) have a\ncommand-line option, but Eric's patch does it by default. Which do we\nwant? :)\n\nI'm inclined to say that this new behavior should be the default; even\nthough it's technically a compatibility change, I find it hard to\nimagine people would see it as a regression.\n\nBut we could also introduce \"--omit-empty-lines\", and then flip the\ndefault. That gives an escape hatch of \"--no-omit-empty-lines\", at the\nminor cost of having to carry an option that we assume nobody would ever\nuse.\n\n-Peff\n"}]}