{"thread":{"id":"59498","subject":"[PATCH 0/2] branch, for-each-ref: add option to omit empty lines","startedAt":"2023-03-30T11:21:52Z","lastAt":"2023-04-13T15:14:11Z","messageCount":30,"participants":["Øystein Walle","Junio C Hamano","Jeff King","ZheNing Hu","Phillip Wood","Andrei Rybak"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"474452","messageId":"20230330112133.4437-1-oystwa@gmail.com","threadId":"59498","inReplyTo":null,"subject":"[PATCH 0/2] branch, for-each-ref: add option to omit empty lines","fromName":"Øystein Walle","fromEmail":"oystwa@gmail.com","sentAt":"2023-03-30T11:21:31Z","receivedAt":"2023-03-30T11:21:52Z","isPatch":true,"sender":{"key":"oystwa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/794585?v=4"},"body":"These two patches are independent of eachother. One is just a very small\ncleanup that I discovered while working on the other. Some thoughts\nafter the --- line in each patch.\n\nØystein Walle (2):\n ref-filter: remove unused ref_format member\n branch, for-each-ref: add option to omit empty lines\n\n Documentation/git-branch.txt | 5 +++++\n Documentation/git-for-each-ref.txt | 5 +++++\n ref-filter.h | 1 -\n builtin/branch.c | 12 +++++++++++-\n builtin/for-each-ref.c | 15 +++++++++++----\n ref-filter.c | 1 -\n t/t3203-branch-output.sh | 26 ++++++++++++++++++++++++++\n t/t6300-for-each-ref.sh | 8 ++++++++\n 8 files changed, 66 insertions(+), 7 deletions(-)\n\n-- \n2.34.1\n\n"},{"id":"474453","messageId":"20230330112133.4437-2-oystwa@gmail.com","threadId":"59498","inReplyTo":"20230330112133.4437-1-oystwa@gmail.com","subject":"[PATCH 1/2] ref-filter: remove unused ref_format member","fromName":"Øystein Walle","fromEmail":"oystwa@gmail.com","sentAt":"2023-03-30T11:21:32Z","receivedAt":"2023-03-30T11:21:53Z","isPatch":true,"sender":{"key":"oystwa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/794585?v=4"},"body":"use_rest was added in b9dee075eb (ref-filter: add %(rest) atom,\n2021-07-26) but was never used. As far as I can tell it was used in a\nlater patch that was submitted to the mailing list but never applied.\n\nSigned-off-by: Øystein Walle \n---\nWould be nice to have a link to the email thread here, but I don't know\nhow.\n\n ref-filter.h | 1 -\n ref-filter.c | 1 -\n 2 files changed, 2 deletions(-)\n\ndiff --git a/ref-filter.h b/ref-filter.h\nindex aa0eea4ecf..0f4183233a 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -75,7 +75,6 @@ struct ref_format {\n \tconst char *format;\n \tconst char *rest;\n \tint quote_style;\n-\tint use_rest;\n \tint use_color;\n \n \t/* Internal state to ref-filter */\ndiff --git a/ref-filter.c b/ref-filter.c\nindex ed802778da..20e0a72f24 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -596,7 +596,6 @@ static int rest_atom_parser(struct ref_format *format,\n {\n \tif (arg)\n \t\treturn err_no_arg(err, \"rest\");\n-\tformat->use_rest = 1;\n \treturn 0;\n }\n \n-- \n2.34.1\n\n"},{"id":"474454","messageId":"20230330112133.4437-3-oystwa@gmail.com","threadId":"59498","inReplyTo":"20230330112133.4437-1-oystwa@gmail.com","subject":"[PATCH 2/2] branch, for-each-ref: add option to omit empty lines","fromName":"Øystein Walle","fromEmail":"oystwa@gmail.com","sentAt":"2023-03-30T11:21:33Z","receivedAt":"2023-03-30T11:21:54Z","isPatch":true,"sender":{"key":"oystwa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/794585?v=4"},"body":"If the given format string expands to the empty string a newline is\nstill printed it. This makes using the output linewise more tedious. For\nexample, git update-ref --stdin does not accept empty lines.\n\nAdd options to branch and for-each-ref to not print these empty lines.\nThe default behavior remains the same.\n\nSigned-off-by: Øystein Walle \n---\n\nThe logic is more or less duplicated in branch.c and for-each-ref.c\nwhich I don't like. However I couldn't really find a \"central\" place to\nput it. Imo. it's definitely not a property of the format or the filter,\nso struct ref_format and struct ref_filter are no good.\n\nI also started working on a patch to make update-ref --stdin accept\nempty lines. But that seems to be a much more deliberate decision, with\ntests to verify it and all. So I stopped pursuing that.\n\n Documentation/git-branch.txt | 5 +++++\n Documentation/git-for-each-ref.txt | 5 +++++\n builtin/branch.c | 12 +++++++++++-\n builtin/for-each-ref.c | 15 +++++++++++----\n t/t3203-branch-output.sh | 26 ++++++++++++++++++++++++++\n t/t6300-for-each-ref.sh | 8 ++++++++\n 6 files changed, 66 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt\nindex d382ac69f7..4d53133ce3 100644\n--- a/Documentation/git-branch.txt\n+++ b/Documentation/git-branch.txt\n@@ -156,6 +156,11 @@ in another worktree linked to the same repository.\n --ignore-case::\n \tSorting and filtering branches are case insensitive.\n \n+-n::\n+--omit-empty-lines::\n+\tDo not print a newline after formatted refs where the format expands\n+\tto the empty string.\n+\n --column[=]::\n --no-column::\n \tDisplay branch listing in columns. See configuration variable\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 6da899c629..0f4fa98b18 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -93,6 +93,11 @@ OPTIONS\n --ignore-case::\n \tSorting and filtering refs are case insensitive.\n \n+-n::\n+--omit-empty-lines::\n+\tDo not print a newline after formatted refs where the format expands\n+\tto the empty string.\n+\n FIELD NAMES\n -----------\n \ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex f63fd45edb..1bbb36b442 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -41,6 +41,7 @@ static const char *head;\n static struct object_id head_oid;\n static int recurse_submodules = 0;\n static int submodule_propagate_branches = 0;\n+static int omit_empty_lines = 0;\n \n static int branch_use_color = -1;\n static char branch_colors[][COLOR_MAXLEN] = {\n@@ -461,7 +462,8 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n \t\t\tstring_list_append(output, out.buf);\n \t\t} else {\n \t\t\tfwrite(out.buf, 1, out.len, stdout);\n-\t\t\tputchar('\\n');\n+\t\t\tif (!omit_empty_lines || out.len > 0)\n+\t\t\t\tputchar('\\n');\n \t\t}\n \t}\n \n@@ -670,6 +672,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tOPT_BIT('D', NULL, &delete, N_(\"delete branch (even if not merged)\"), 2),\n \t\tOPT_BIT('m', \"move\", &rename, N_(\"move/rename a branch and its reflog\"), 1),\n \t\tOPT_BIT('M', NULL, &rename, N_(\"move/rename a branch, even if target exists\"), 2),\n+\t\tOPT_BOOL('n' , \"omit-empty-lines\", &omit_empty_lines,\n+\t\t\tN_(\"do not output a newline after empty formatted refs\")),\n \t\tOPT_BIT('c', \"copy\", ©, N_(\"copy a branch and its reflog\"), 1),\n \t\tOPT_BIT('C', NULL, ©, N_(\"copy a branch, even if target exists\"), 2),\n \t\tOPT_BOOL('l', \"list\", &list, N_(\"list branch names\")),\n@@ -757,7 +761,13 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (list)\n+\t{\n+\t\tif (omit_empty_lines && !format.format) {\n+\t\t\terror(\"--omit-empty-lines without --format does not make sense\");\n+\t\t\tusage_with_options(builtin_branch_usage, options);\n+\t\t}\n \t\tsetup_auto_pager(\"branch\", 1);\n+\t}\n \n \tif (delete) {\n \t\tif (!argc)\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 6f62f40d12..349c4d4ef8 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -19,7 +19,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \tint i;\n \tstruct ref_sorting *sorting;\n \tstruct string_list sorting_options = STRING_LIST_INIT_DUP;\n-\tint maxcount = 0, icase = 0;\n+\tint maxcount = 0, icase = 0, omit_empty_lines = 0;\n \tstruct ref_array array;\n \tstruct ref_filter filter;\n \tstruct ref_format format = REF_FORMAT_INIT;\n@@ -35,6 +35,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \t\t\tN_(\"quote placeholders suitably for python\"), QUOTE_PYTHON),\n \t\tOPT_BIT(0 , \"tcl\", &format.quote_style,\n \t\t\tN_(\"quote placeholders suitably for Tcl\"), QUOTE_TCL),\n+\t\tOPT_BOOL('n' , \"omit-empty-lines\", &omit_empty_lines,\n+\t\t\tN_(\"do not output a newline after empty formatted refs\")),\n \n \t\tOPT_GROUP(\"\"),\n \t\tOPT_INTEGER( 0 , \"count\", &maxcount, N_(\"show only matched refs\")),\n@@ -55,8 +57,6 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \tmemset(&array, 0, sizeof(array));\n \tmemset(&filter, 0, sizeof(filter));\n \n-\tformat.format = \"%(objectname) %(objecttype)\\t%(refname)\";\n-\n \tgit_config(git_default_config, NULL);\n \n \tparse_options(argc, argv, prefix, opts, for_each_ref_usage, 0);\n@@ -68,6 +68,12 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \t\terror(\"more than one quoting style?\");\n \t\tusage_with_options(for_each_ref_usage, opts);\n \t}\n+\tif (omit_empty_lines && !format.format) {\n+\t\terror(\"--omit-empty-lines without --format does not make sense\");\n+\t\tusage_with_options(for_each_ref_usage, opts);\n+\t}\n+\tif (!format.format)\n+\t\tformat.format = \"%(objectname) %(objecttype)\\t%(refname)\";\n \tif (verify_ref_format(&format))\n \t\tusage_with_options(for_each_ref_usage, opts);\n \n@@ -88,7 +94,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \t\tif (format_ref_array_item(array.items[i], &format, &output, &err))\n \t\t\tdie(\"%s\", err.buf);\n \t\tfwrite(output.buf, 1, output.len, stdout);\n-\t\tputchar('\\n');\n+\t\tif (!omit_empty_lines || output.len > 0)\n+\t\t\tputchar('\\n');\n \t}\n \n \tstrbuf_release(&err);\ndiff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh\nindex d34d77f893..26bf0819ea 100755\n--- a/t/t3203-branch-output.sh\n+++ b/t/t3203-branch-output.sh\n@@ -341,6 +341,32 @@ test_expect_success 'git branch with --format=%(rest) must fail' '\n \ttest_must_fail git branch --format=\"%(rest)\" >actual\n '\n \n+test_expect_success 'git branch --format --omit-empty-lines' '\n+\tcat >expect <<-\\EOF &&\n+\tRefname is (HEAD detached from fromtag)\n+\tRefname is refs/heads/ambiguous\n+\tRefname is refs/heads/branch-one\n+\tRefname is refs/heads/branch-two\n+\tEOF\n+\techo >>expect &&\n+\tcat >>expect <<-\\EOF &&\n+\tRefname is refs/heads/ref-to-branch\n+\tRefname is refs/heads/ref-to-remote\n+\tEOF\n+\tgit branch --format=\"%(if:notequals=refs/heads/main)%(refname)%(then)Refname is %(refname)%(end)\" >actual &&\n+\ttest_cmp expect actual &&\n+\tcat >expect <<-\\EOF &&\n+\tRefname is (HEAD detached from fromtag)\n+\tRefname is refs/heads/ambiguous\n+\tRefname is refs/heads/branch-one\n+\tRefname is refs/heads/branch-two\n+\tRefname is refs/heads/ref-to-branch\n+\tRefname is refs/heads/ref-to-remote\n+\tEOF\n+\tgit branch --omit-empty-lines --format=\"%(if:notequals=refs/heads/main)%(refname)%(then)Refname is %(refname)%(end)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'worktree colors correct' '\n \tcat >expect <<-EOF &&\n \t* (HEAD detached from fromtag)\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex c466fd989f..eec9d45513 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -1374,6 +1374,14 @@ test_expect_success 'for-each-ref --ignore-case ignores case' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'for-each-ref --omit-empty-lines works' '\n+\tgit for-each-ref --format=\"%(refname)\" > actual &&\n+\ttest_line_count -gt 1 actual &&\n+\tgit for-each-ref --format=\"%(if:equals=refs/heads/main)%(refname)%(then)%(refname)%(end)\" --omit-empty-lines > actual &&\n+\techo refs/heads/main > expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'for-each-ref --ignore-case works on multiple sort keys' '\n \t# name refs numerically to avoid case-insensitive filesystem conflicts\n \tnr=0 &&\n-- \n2.34.1\n\n"},{"id":"474461","messageId":"xmqqo7oa2rjs.fsf@gitster.g","threadId":"59498","inReplyTo":"20230330112133.4437-2-oystwa@gmail.com","subject":"Re: [PATCH 1/2] ref-filter: remove unused ref_format member","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-30T15:21:43Z","receivedAt":"2023-03-30T15:23:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Øystein Walle writes:\n\n> use_rest was added in b9dee075eb (ref-filter: add %(rest) atom,\n> 2021-07-26) but was never used. As far as I can tell it was used in a\n> later patch that was submitted to the mailing list but never applied.\n>\n> Signed-off-by: Øystein Walle \n> ---\n> Would be nice to have a link to the email thread here, but I don't know\n> how.\n\n\nHere is a link to the patch that led to that commit you cited:\n\nhttps://lore.kernel.org/git/207cc5129649e767036d8467ea7c884c3f664cc7.1627270010.git.gitgitgadget@gmail.com/\n\nIt indeed is cumbersome to add, as the Message-Ids for patches from\nGitGitGadget tend to be ultra long.\n\nBut b9dee075eb was the last one in the 5-patch series; I do\nnot see any \"later patch there in the thread.\n\n> ref-filter.h | 1 -\n> ref-filter.c | 1 -\n> 2 files changed, 2 deletions(-)\n>\n> diff --git a/ref-filter.h b/ref-filter.h\n> index aa0eea4ecf..0f4183233a 100644\n> --- a/ref-filter.h\n> +++ b/ref-filter.h\n> @@ -75,7 +75,6 @@ struct ref_format {\n> \tconst char *format;\n> \tconst char *rest;\n> \tint quote_style;\n> -\tint use_rest;\n> \tint use_color;\n> \n> \t/* Internal state to ref-filter */\n> diff --git a/ref-filter.c b/ref-filter.c\n> index ed802778da..20e0a72f24 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -596,7 +596,6 @@ static int rest_atom_parser(struct ref_format *format,\n> {\n> \tif (arg)\n> \t\treturn err_no_arg(err, \"rest\");\n> -\tformat->use_rest = 1;\n> \treturn 0;\n> }\n"},{"id":"474462","messageId":"xmqqjzyy2rdl.fsf@gitster.g","threadId":"59498","inReplyTo":"xmqqo7oa2rjs.fsf@gitster.g","subject":"Re: [PATCH 1/2] ref-filter: remove unused ref_format member","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-30T15:25:26Z","receivedAt":"2023-03-30T15:26:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Øystein Walle writes:\n>\n>> use_rest was added in b9dee075eb (ref-filter: add %(rest) atom,\n>> 2021-07-26) but was never used. As far as I can tell it was used in a\n>> later patch that was submitted to the mailing list but never applied.\n>>\n>> Signed-off-by: Øystein Walle \n>> ---\n>> Would be nice to have a link to the email thread here, but I don't know\n>> how.\n>\n>\n> Here is a link to the patch that led to that commit you cited:\n>\n> https://lore.kernel.org/git/207cc5129649e767036d8467ea7c884c3f664cc7.1627270010.git.gitgitgadget@gmail.com/\n>\n> It indeed is cumbersome to add, as the Message-Ids for patches from\n> GitGitGadget tend to be ultra long.\n>\n> But b9dee075eb was the last one in the 5-patch series; I do\n> not see any \"later patch there in the thread.\n\nI think there was a follow-up RFC series that was written to use the\nvalue of the member, cf.\n\nhttps://lore.kernel.org/git/9c5fddf6885875ccd3ce3f047bb938c77d9bbca2.1628842990.git.gitgitgadget@gmail.com/\n\nbut it seems there was no review on the series.\n"},{"id":"474465","messageId":"xmqqilei1bgk.fsf@gitster.g","threadId":"59498","inReplyTo":"20230330112133.4437-3-oystwa@gmail.com","subject":"Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-30T15:54:35Z","receivedAt":"2023-03-30T15:54:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Øystein Walle writes:\n\n> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\n> index 6da899c629..0f4fa98b18 100644\n> --- a/Documentation/git-for-each-ref.txt\n> +++ b/Documentation/git-for-each-ref.txt\n> @@ -93,6 +93,11 @@ OPTIONS\n> --ignore-case::\n> \tSorting and filtering refs are case insensitive.\n> \n> +-n::\n> +--omit-empty-lines::\n> +\tDo not print a newline after formatted refs where the format expands\n> +\tto the empty string.\n\nWhile I can see the utility of the new feature, it is unclear if its\nmerit is so clear that it deserves a short-and-sweet single letter\noption from the get go. Especially, don't we want to give this to\n\"git branch\" and \"git tag\" in their listing modes for consistency,\nbut it means stealing \"-n\" also from them.\n\n> @@ -757,7 +761,13 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n> \t}\n> \n> \tif (list)\n> +\t{\n\nMove that opening brace at the end of the previous line, i.e.\n\n-\tif (list)\n+\tif (list) {\n\n> +\t\tif (omit_empty_lines && !format.format) {\n> +\t\t\terror(\"--omit-empty-lines without --format does not make sense\");\n> +\t\t\tusage_with_options(builtin_branch_usage, options);\n> +\t\t}\n\nDoes it not make sense? With the default format, it may happen that\nthere will be no empty line so there is nothing to omit, but I do\nnot see a strong reason to forbid the request like this.\n\n> \t\tsetup_auto_pager(\"branch\", 1);\n> +\t}\n> \n> \tif (delete) {\n> \t\tif (!argc)\n> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\n> index 6f62f40d12..349c4d4ef8 100644\n> --- a/builtin/for-each-ref.c\n> +++ b/builtin/for-each-ref.c\n> @@ -19,7 +19,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n> \tint i;\n> \tstruct ref_sorting *sorting;\n> \tstruct string_list sorting_options = STRING_LIST_INIT_DUP;\n> -\tint maxcount = 0, icase = 0;\n> +\tint maxcount = 0, icase = 0, omit_empty_lines = 0;\n> \tstruct ref_array array;\n> \tstruct ref_filter filter;\n> \tstruct ref_format format = REF_FORMAT_INIT;\n> @@ -35,6 +35,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n> \t\t\tN_(\"quote placeholders suitably for python\"), QUOTE_PYTHON),\n> \t\tOPT_BIT(0 , \"tcl\", &format.quote_style,\n> \t\t\tN_(\"quote placeholders suitably for Tcl\"), QUOTE_TCL),\n> +\t\tOPT_BOOL('n' , \"omit-empty-lines\", &omit_empty_lines,\n> +\t\t\tN_(\"do not output a newline after empty formatted refs\")),\n> \n> \t\tOPT_GROUP(\"\"),\n> \t\tOPT_INTEGER( 0 , \"count\", &maxcount, N_(\"show only matched refs\")),\n> @@ -55,8 +57,6 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n> \tmemset(&array, 0, sizeof(array));\n> \tmemset(&filter, 0, sizeof(filter));\n> \n> -\tformat.format = \"%(objectname) %(objecttype)\\t%(refname)\";\n> -\n> \tgit_config(git_default_config, NULL);\n> \tparse_options(argc, argv, prefix, opts, for_each_ref_usage, 0);\n\nThis smells fishy. We establish the hardcoded built-in default, let\nthe config machinery override, and then finally let command line\noptions to further override. You may be able to reach the same end\nresult by leaving the value unset, fill with the configured value,\noverride with the command line, and then if the value is still\nunset, fall back to a hardcoded built-in default, but I do not see\nwhy such a change logically belongs to a patch to add \"--omit-empty\"\nfeature.\n\n> @@ -68,6 +68,12 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n> \t\terror(\"more than one quoting style?\");\n> \t\tusage_with_options(for_each_ref_usage, opts);\n> \t}\n> +\tif (omit_empty_lines && !format.format) {\n> +\t\terror(\"--omit-empty-lines without --format does not make sense\");\n> +\t\tusage_with_options(for_each_ref_usage, opts);\n> +\t}\n\nI wouldn't do this, for the same reason as for \"git branch\".\n\n> +\tif (!format.format)\n> +\t\tformat.format = \"%(objectname) %(objecttype)\\t%(refname)\";\n\nThis is the other half of the earlier change I called \"fishy\". It\nmay be benign, but it is distracting, especially when done without\nexplanation, in a change to add a feature that is not related.\n\n> @@ -88,7 +94,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n> \t\tif (format_ref_array_item(array.items[i], &format, &output, &err))\n> \t\t\tdie(\"%s\", err.buf);\n> \t\tfwrite(output.buf, 1, output.len, stdout);\n> -\t\tputchar('\\n');\n> +\t\tif (!omit_empty_lines || output.len > 0)\n> +\t\t\tputchar('\\n');\n> \t}\n\nOK, but two points.\n\n * do not compare output.len with 0; it is sufficient to just write\n\n\tif (!omit_empty || output.len)\n\n * now we care if output is empty anyway, perhaps we can optimize\n out fwrite() too, perhaps with something like\n\n\tif (output.len || !omit_empty)\n\t\tprintf(\"%.*s\\n\", output.len, output.buf);\n\n perhaps?\n\nI am not sure about the latter, but we tend to use \"%.*s\" liberally\nwhen we could use fwrite() in our codebase for brevity, so ...\n\n> +test_expect_success 'git branch --format --omit-empty-lines' '\n> +\tcat >expect <<-\\EOF &&\n> +\tRefname is (HEAD detached from fromtag)\n> +\tRefname is refs/heads/ambiguous\n> +\tRefname is refs/heads/branch-one\n> +\tRefname is refs/heads/branch-two\n> +\tEOF\n> +\techo >>expect &&\n> +\tcat >>expect <<-\\EOF &&\n> +\tRefname is refs/heads/ref-to-branch\n> +\tRefname is refs/heads/ref-to-remote\n> +\tEOF\n\nIt is hard to see that there is an empty line expected when the\nexpectation is prepared like this. Why not something like\n\n\tcat >expect.full <<-\\EOF &&\n\tone\n\ttwo\n\n\tfour (three is missing)\n\tEOF\n\tsed -e \"/^$/d\" expect.full >expect.stripped &&\n\n\tgit branch $args >actual &&\n\ttest_cmp expect.full actual &&\n\n\tgit branch --omit-empty $args >actual &&\n\ttest_cmp expect.stripped actual &&\n\nthat highlights the fact that there is a missing line for one\nexpectation, and that the only difference in two expectations is the\nlack of empty line(s)?\n\n> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\n> index c466fd989f..eec9d45513 100755\n> --- a/t/t6300-for-each-ref.sh\n> +++ b/t/t6300-for-each-ref.sh\n> @@ -1374,6 +1374,14 @@ test_expect_success 'for-each-ref --ignore-case ignores case' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'for-each-ref --omit-empty-lines works' '\n> +\tgit for-each-ref --format=\"%(refname)\" > actual &&\n> +\ttest_line_count -gt 1 actual &&\n\nThe next test depends on that a branch 'main' exists, so perhaps\nthat should be tested here, at least? And then if there is no other\nbranches and tags, we cannot tell if seeing only the 'main' branch\nis due to --omit-empty correctly working, or due to the repository\nhaving only that branch, so it is also good to check if there is\nbranches or tags other than 'main' in the output.\n\n> +\tgit for-each-ref --format=\"%(if:equals=refs/heads/main)%(refname)%(then)%(refname)%(end)\" --omit-empty-lines > actual &&\n> +\techo refs/heads/main > expect &&\n> +\ttest_cmp expect actual\n> +'\n\nBy the way, lose SP between redirection operator '>' and its target,\ni.e. write them like so:\n\n\techo refs/heads/main >expect\n\nThis feature makes %(if)...%(else)...%(end) construct complete and\nis a very good addition.\n\nThanks for working on it.\n\n"},{"id":"474468","messageId":"xmqq355m17g0.fsf@gitster.g","threadId":"59498","inReplyTo":"20230330112133.4437-3-oystwa@gmail.com","subject":"Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-30T17:21:19Z","receivedAt":"2023-03-30T17:21:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Øystein Walle writes:\n\n> The logic is more or less duplicated in branch.c and for-each-ref.c\n> which I don't like. However I couldn't really find a \"central\" place to\n> put it. Imo. it's definitely not a property of the format or the filter,\n> so struct ref_format and struct ref_filter are no good.\n\nI think the division of labor is very much in line with how the\nref-filter API is currently laid out. Enumerating is done calling\nfilter_refs(), and result is returned in an array, which the caller\nadds whatever frills around its elements to show them in the output.\nThe \"adding frills\" is aided by calling format_ref_array_item(), but\nthe API does not care how the formatting result is used.\n\n"},{"id":"474475","messageId":"20230330182502.GB3286761@coredump.intra.peff.net","threadId":"59498","inReplyTo":"xmqqilei1bgk.fsf@gitster.g","subject":"Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-30T18:25:02Z","receivedAt":"2023-03-30T18:25:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 30, 2023 at 08:54:35AM -0700, Junio C Hamano wrote:\n\n> * now we care if output is empty anyway, perhaps we can optimize\n> out fwrite() too, perhaps with something like\n> \n> \tif (output.len || !omit_empty)\n> \t\tprintf(\"%.*s\\n\", output.len, output.buf);\n> \n> perhaps?\n> \n> I am not sure about the latter, but we tend to use \"%.*s\" liberally\n> when we could use fwrite() in our codebase for brevity, so ...\n\nI think it would be a mistake here, as you can use \"%00\" in the format\nto include a NUL in the output.\n\n(The rest of your review seemed quite sensible to me, and I like the\nidea of the omit-empty option in general).\n\n-Peff\n"},{"id":"474477","messageId":"xmqq4jq2jcij.fsf@gitster.g","threadId":"59498","inReplyTo":"20230330182502.GB3286761@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-30T18:54:28Z","receivedAt":"2023-03-30T18:54:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Mar 30, 2023 at 08:54:35AM -0700, Junio C Hamano wrote:\n>\n>> * now we care if output is empty anyway, perhaps we can optimize\n>> out fwrite() too, perhaps with something like\n>> \n>> \tif (output.len || !omit_empty)\n>> \t\tprintf(\"%.*s\\n\", output.len, output.buf);\n>> \n>> perhaps?\n>> \n>> I am not sure about the latter, but we tend to use \"%.*s\" liberally\n>> when we could use fwrite() in our codebase for brevity, so ...\n>\n> I think it would be a mistake here, as you can use \"%00\" in the format\n> to include a NUL in the output.\n\nGood point. Thanks for catching it.\n\n>\n> (The rest of your review seemed quite sensible to me, and I like the\n> idea of the omit-empty option in general).\n>\n> -Peff\n"},{"id":"474498","messageId":"20230331083213.12013-1-oystwa@gmail.com","threadId":"59498","inReplyTo":"xmqqilei1bgk.fsf@gitster.g","subject":"Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines","fromName":"Øystein Walle","fromEmail":"oystwa@gmail.com","sentAt":"2023-03-31T08:32:13Z","receivedAt":"2023-03-31T08:32:51Z","isPatch":true,"sender":{"key":"oystwa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/794585?v=4"},"body":"Hi Junio,\n\nOn Thu, 30 Mar 2023 at 17:54, Junio C Hamano wrote:\n\n> While I can see the utility of the new feature, it is unclear if its\n> merit is so clear that it deserves a short-and-sweet single letter\n> option from the get go. Especially, don't we want to give this to\n> \"git branch\" and \"git tag\" in their listing modes for consistency,\n> but it means stealing \"-n\" also from them.\n\nI had this in mind, but I wanted to add a short option because\n\"--omit-empty-lines\" is already longer than \"| sed '/^$/d' |\" :D Which\nis the workaround I've used in the past. I see that later in your reply\nyou write \"--omit-empty\". (Perhaps instinctively?) The \"line\" part is\nalready implied so I'd be equally happy with \"--omit-empty\". I realize\nthat the parser already allows this implicitly, but the full name of the\noption should be spelt out in the docs, I assume.\n\n> Move that opening brace at the end of the previous line, i.e.\n>\n> - if (list)\n> + if (list) {\n\nOf course, my bad. Old habits and so on. But I may not need to change\nthis at all in the first place because...\n\n> > + if (omit_empty_lines && !format.format) {\n> > + error(\"--omit-empty-lines without --format does not make sense\");\n> > + usage_with_options(builtin_branch_usage, options);\n> > + }\n>\n> Does it not make sense? With the default format, it may happen that\n> there will be no empty line so there is nothing to omit, but I do\n> not see a strong reason to forbid the request like this.\n\n... it's perfectly fine by me to allow --omit-empty when the user has\nnot specified their own format. I added this merely as guidance for the\nuser. For example, Git will bail out with a similar message if the user\ntries to unshallow a repository that is already complete, which I assume\nis technically not a problem.\n\n> > setup_auto_pager(\"branch\", 1);\n> > + }\n> >\n> > if (delete) {\n> > if (!argc)\n> > diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\n> > index 6f62f40d12..349c4d4ef8 100644\n> > --- a/builtin/for-each-ref.c\n> > +++ b/builtin/for-each-ref.c\n> > @@ -19,7 +19,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n> > int i;\n> > struct ref_sorting *sorting;\n> > struct string_list sorting_options = STRING_LIST_INIT_DUP;\n> > - int maxcount = 0, icase = 0;\n> > + int maxcount = 0, icase = 0, omit_empty_lines = 0;\n> > struct ref_array array;\n> > struct ref_filter filter;\n> > struct ref_format format = REF_FORMAT_INIT;\n> > @@ -35,6 +35,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n> > N_(\"quote placeholders suitably for python\"), QUOTE_PYTHON),\n> > OPT_BIT(0 , \"tcl\", &format.quote_style,\n> > N_(\"quote placeholders suitably for Tcl\"), QUOTE_TCL),\n> > + OPT_BOOL('n' , \"omit-empty-lines\", &omit_empty_lines,\n> > + N_(\"do not output a newline after empty formatted refs\")),\n> >\n> > OPT_GROUP(\"\"),\n> > OPT_INTEGER( 0 , \"count\", &maxcount, N_(\"show only matched refs\")),\n> > @@ -55,8 +57,6 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n> > memset(&array, 0, sizeof(array));\n> > memset(&filter, 0, sizeof(filter));\n> >\n> > - format.format = \"%(objectname) %(objecttype)\\t%(refname)\";\n> > -\n> > git_config(git_default_config, NULL);\n> > parse_options(argc, argv, prefix, opts, for_each_ref_usage, 0);\n>\n> This smells fishy. We establish the hardcoded built-in default, let\n> the config machinery override, and then finally let command line\n> options to further override. You may be able to reach the same end\n> result by leaving the value unset, fill with the configured value,\n> override with the command line, and then if the value is still\n> unset, fall back to a hardcoded built-in default, but I do not see\n> why such a change logically belongs to a patch to add \"--omit-empty\"\n> feature.\n\nThis is what I did. I moved the assignment of the default value of\nformat.format to after parse_options() so that I could use its (lack of)\nvalue to determine whether --format had been specified by the user,\ninstead of e.g. \"int format_was_given = 0;\". But then I of course have\nto check whether parse_options() already has assigned it before maybe\nassigning the default value.\n\nBut I only did all of that to be able to die with the error. If we just\nallow --omit-empty then all of this can be left alone, making the code,\nand the patch, simpler.\n\n> > @@ -68,6 +68,12 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n> > error(\"more than one quoting style?\");\n> > usage_with_options(for_each_ref_usage, opts);\n> > }\n> > + if (omit_empty_lines && !format.format) {\n> > + error(\"--omit-empty-lines without --format does not make sense\");\n> > + usage_with_options(for_each_ref_usage, opts);\n> > + }\n>\n> I wouldn't do this, for the same reason as for \"git branch\".\n>\n> > + if (!format.format)\n> > + format.format = \"%(objectname) %(objecttype)\\t%(refname)\";\n>\n> This is the other half of the earlier change I called \"fishy\". It\n> may be benign, but it is distracting, especially when done without\n> explanation, in a change to add a feature that is not related.\n\nIt *was* related, because of the error I wanted to provide. But maybe it\nisn't anymore :P\n\n> * do not compare output.len with 0; it is sufficient to just write\n>\n> if (!omit_empty || output.len)\n>\n\nSure, will change. Peff addressed your second point already. But perhaps\nmove fwrite() (or whatever other printing function) inside the if() too?\n\n> It is hard to see that there is an empty line expected when the\n> expectation is prepared like this. Why not something like\n>\n> cat >expect.full <<-\\EOF &&\n> one\n> two\n>\n> four (three is missing)\n> EOF\n> sed -e \"/^$/d\" expect.full >expect.stripped &&\n>\n> git branch $args >actual &&\n> test_cmp expect.full actual &&\n>\n> git branch --omit-empty $args >actual &&\n> test_cmp expect.stripped actual &&\n>\n> that highlights the fact that there is a missing line for one\n> expectation, and that the only difference in two expectations is the\n> lack of empty line(s)?\n\nI wholeheartedly agree. I only wrote it this way because actually having\nan empty line there lead to a whitespace error in the patch. But that\nwas because I mistakenly assumed that empty lines in an indented heredoc\nalso had to be prefixed by a TAB and I didn't investigate. Will fix.\n\n> > diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\n> > index c466fd989f..eec9d45513 100755\n> > --- a/t/t6300-for-each-ref.sh\n> > +++ b/t/t6300-for-each-ref.sh\n> > @@ -1374,6 +1374,14 @@ test_expect_success 'for-each-ref --ignore-case ignores case' '\n> > test_cmp expect actual\n> > '\n> >\n> > +test_expect_success 'for-each-ref --omit-empty-lines works' '\n> > + git for-each-ref --format=\"%(refname)\" > actual &&\n> > + test_line_count -gt 1 actual &&\n>\n> The next test depends on that a branch 'main' exists, so perhaps\n> that should be tested here, at least? And then if there is no other\n> branches and tags, we cannot tell if seeing only the 'main' branch\n> is due to --omit-empty correctly working, or due to the repository\n> having only that branch, so it is also good to check if there is\n> branches or tags other than 'main' in the output.\n\nIn general I find it very hard to write meaningful tests because of\nstuff like this this. In my (admittedly very limited) experience there\nare usually big dependencies on prior tests in a particular test case,\nand I just assumed that was okay. I often find it hard to discover what\nthe state of the repository is at the point in the test script where I\nwant to add a test, or modify an existing one.\n\nIn this particular case I know from the test case right above that main\nexists. What is not immediately obvious is that at least one other ref\nexists. But if that ever changes then at least 'test_line_count -gt 1\nactual' will fail.\n\n> By the way, lose SP between redirection operator '>' and its target,\n> i.e. write them like so:\n>\n> echo refs/heads/main >expect\n\nWill fix.\n\n> This feature makes %(if)...%(else)...%(end) construct complete and\n> is a very good addition.\n>\n> Thanks for working on it.\n>\n\nThanks for the review!\n\nØsse\n"},{"id":"474500","messageId":"20230331103708.18945-1-oystwa@gmail.com","threadId":"59498","inReplyTo":"xmqqjzyy2rdl.fsf@gitster.g","subject":"Re: [PATCH 1/2] ref-filter: remove unused ref_format member","fromName":"Øystein Walle","fromEmail":"oystwa@gmail.com","sentAt":"2023-03-31T10:37:08Z","receivedAt":"2023-03-31T10:37:40Z","isPatch":true,"sender":{"key":"oystwa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/794585?v=4"},"body":"On Thu, 30 Mar 2023 at 17:25, Junio C Hamano wrote:\n>\n> Junio C Hamano writes:\n>\n> > Øystein Walle writes:\n> >\n> >> use_rest was added in b9dee075eb (ref-filter: add %(rest) atom,\n> >> 2021-07-26) but was never used. As far as I can tell it was used in a\n> >> later patch that was submitted to the mailing list but never applied.\n> >>\n> >> Signed-off-by: Øystein Walle \n> >> ---\n> >> Would be nice to have a link to the email thread here, but I don't know\n> >> how.\n> >\n> >\n> > Here is a link to the patch that led to that commit you cited:\n> >\n> > https://lore.kernel.org/git/207cc5129649e767036d8467ea7c884c3f664cc7.1627270010.git.gitgitgadget@gmail.com/\n> >\n> > It indeed is cumbersome to add, as the Message-Ids for patches from\n> > GitGitGadget tend to be ultra long.\n> >\n> > But b9dee075eb was the last one in the 5-patch series; I do\n> > not see any \"later patch there in the thread.\n>\n> I think there was a follow-up RFC series that was written to use the\n> value of the member, cf.\n>\n> https://lore.kernel.org/git/9c5fddf6885875ccd3ce3f047bb938c77d9bbca2.1628842990.git.gitgitgadget@gmail.com/\n>\n> but it seems there was no review on the series.\n\nThe follow-up series you link to seems to be a superset of the first series,\nwhich is what confused me. This is why I thought perhaps a subset of the latter\nseries was accepted. But I see now that the dates match that of the first\nseries, and you even applied it very soon after. Strange choice to include the\nfirst five patches in the follow-up series, then...\n\nLooked through the git.git log and see that it's not uncommon to reference\npatches from lore.kernel.org, so I can do the same. Perhaps in the \"footnote\nstyle\" to make it easier to digest. That is, if we want to apply this in the\nfirst place... It is a very minor cleanup of something that does no harm. On\nthe other hand this particlar line of development seems abandoned.\n\nØsse\n"},{"id":"474501","messageId":"CAOLTT8SAo7rJ2NP4wTsSSJ18-qBV42ZhFHF6pRwRqHwhKaQUvA@mail.gmail.com","threadId":"59498","inReplyTo":"20230331103708.18945-1-oystwa@gmail.com","subject":"Re: [PATCH 1/2] ref-filter: remove unused ref_format member","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2023-03-31T10:57:42Z","receivedAt":"2023-03-31T11:00:27Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Øystein Walle 于2023年3月31日周五 18:39写道：\n>\n> On Thu, 30 Mar 2023 at 17:25, Junio C Hamano wrote:\n> >\n> > Junio C Hamano writes:\n> >\n> > > Øystein Walle writes:\n> > >\n> > >> use_rest was added in b9dee075eb (ref-filter: add %(rest) atom,\n> > >> 2021-07-26) but was never used. As far as I can tell it was used in a\n> > >> later patch that was submitted to the mailing list but never applied.\n> > >>\n> > >> Signed-off-by: Øystein Walle \n> > >> ---\n> > >> Would be nice to have a link to the email thread here, but I don't know\n> > >> how.\n> > >\n> > >\n> > > Here is a link to the patch that led to that commit you cited:\n> > >\n> > > https://lore.kernel.org/git/207cc5129649e767036d8467ea7c884c3f664cc7.1627270010.git.gitgitgadget@gmail.com/\n> > >\n> > > It indeed is cumbersome to add, as the Message-Ids for patches from\n> > > GitGitGadget tend to be ultra long.\n> > >\n> > > But b9dee075eb was the last one in the 5-patch series; I do\n> > > not see any \"later patch there in the thread.\n> >\n> > I think there was a follow-up RFC series that was written to use the\n> > value of the member, cf.\n> >\n> > https://lore.kernel.org/git/9c5fddf6885875ccd3ce3f047bb938c77d9bbca2.1628842990.git.gitgitgadget@gmail.com/\n> >\n> > but it seems there was no review on the series.\n>\n> The follow-up series you link to seems to be a superset of the first series,\n> which is what confused me. This is why I thought perhaps a subset of the latter\n> series was accepted. But I see now that the dates match that of the first\n> series, and you even applied it very soon after. Strange choice to include the\n> first five patches in the follow-up series, then...\n>\n> Looked through the git.git log and see that it's not uncommon to reference\n> patches from lore.kernel.org, so I can do the same. Perhaps in the \"footnote\n> style\" to make it easier to digest. That is, if we want to apply this in the\n> first place... It is a very minor cleanup of something that does no harm. On\n> the other hand this particlar line of development seems abandoned.\n>\n\nYes. Originally, I hoped to make all the atoms of cat-file --format compatible\nwith ref-filter, and then make cat-file --format able to use the\ninterface of ref-filter\nBut due to some performance issues, this route is now deprecated. This little\n%(rest) is no longer useful.\n\n> Øsse\n\nZheNing\n"},{"id":"474509","messageId":"xmqq355kj4mf.fsf@gitster.g","threadId":"59498","inReplyTo":"20230331083213.12013-1-oystwa@gmail.com","subject":"Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-31T15:57:12Z","receivedAt":"2023-03-31T15:57:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Øystein Walle writes:\n\n>> > + if (omit_empty_lines && !format.format) {\n>> > + error(\"--omit-empty-lines without --format does not make sense\");\n>> > + usage_with_options(builtin_branch_usage, options);\n>> > + }\n>>\n>> Does it not make sense? With the default format, it may happen that\n>> there will be no empty line so there is nothing to omit, but I do\n>> not see a strong reason to forbid the request like this.\n>\n> ... it's perfectly fine by me to allow --omit-empty when the user has\n> not specified their own format. I added this merely as guidance for the\n> user. For example, Git will bail out with a similar message if the user\n> tries to unshallow a repository that is already complete, which I assume\n> is technically not a problem.\n\nIt is not just technically a problem but from the end user's point\nof view a misguided message. If the end result is in the shape of\ndesired state after the command completes, there shouldn't be an\nerror() to stop the user. Informational \"the history is now fully\ncomplete---by the way, it was already so before I started working\"\nmay be OK. It probably should be fixed, instead of being modelled\nafter to spread the mistake to new features, like this patch does.\n\nThanks.\n"},{"id":"474512","messageId":"xmqq4jq0hp1i.fsf@gitster.g","threadId":"59498","inReplyTo":"20230331103708.18945-1-oystwa@gmail.com","subject":"Re: [PATCH 1/2] ref-filter: remove unused ref_format member","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-31T16:19:05Z","receivedAt":"2023-03-31T16:32:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Øystein Walle writes:\n\n> The follow-up series you link to seems to be a superset of the first series,\n> which is what confused me. This is why I thought perhaps a subset of the latter\n> series was accepted. But I see now that the dates match that of the first\n> series, and you even applied it very soon after. Strange choice to include the\n> first five patches in the follow-up series, then...\n\nIt probably happened because even by then the previous round v4 was\nnot in 'next' when the later iteration was prepared, and then the\ntopic perhaps died at around the time GSoC of the year finished. As\nlong as the earlier and less ambitious attempt turns out to give us\na net positive benefit, these early steps may still advance through\n'next' and down to a release.\n"},{"id":"474514","messageId":"44e7ac0f-2fd9-fd01-89da-a8d036d2e400@dunelm.org.uk","threadId":"59498","inReplyTo":"20230330112133.4437-3-oystwa@gmail.com","subject":"Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-03-31T16:33:37Z","receivedAt":"2023-03-31T16:37:42Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Øystein\n\nOn 30/03/2023 12:21, Øystein Walle wrote:\n> If the given format string expands to the empty string a newline is\n> still printed it. This makes using the output linewise more tedious. For\n> example, git update-ref --stdin does not accept empty lines.\n> \n> Add options to branch and for-each-ref to not print these empty lines.\n> The default behavior remains the same.\n\nDo the empty lines in the output serve any useful purpose? If not then \nit might be better just to suppress them unconditionally rather than \nadding a new command line option.\n\nBest Wishes\n\nPhillip\n\n> Signed-off-by: Øystein Walle \n> ---\n> \n> The logic is more or less duplicated in branch.c and for-each-ref.c\n> which I don't like. However I couldn't really find a \"central\" place to\n> put it. Imo. it's definitely not a property of the format or the filter,\n> so struct ref_format and struct ref_filter are no good.\n> \n> I also started working on a patch to make update-ref --stdin accept\n> empty lines. But that seems to be a much more deliberate decision, with\n> tests to verify it and all. So I stopped pursuing that.\n> \n> Documentation/git-branch.txt | 5 +++++\n> Documentation/git-for-each-ref.txt | 5 +++++\n> builtin/branch.c | 12 +++++++++++-\n> builtin/for-each-ref.c | 15 +++++++++++----\n> t/t3203-branch-output.sh | 26 ++++++++++++++++++++++++++\n> t/t6300-for-each-ref.sh | 8 ++++++++\n> 6 files changed, 66 insertions(+), 5 deletions(-)\n> \n> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt\n> index d382ac69f7..4d53133ce3 100644\n> --- a/Documentation/git-branch.txt\n> +++ b/Documentation/git-branch.txt\n> @@ -156,6 +156,11 @@ in another worktree linked to the same repository.\n> --ignore-case::\n> \tSorting and filtering branches are case insensitive.\n> \n> +-n::\n> +--omit-empty-lines::\n> +\tDo not print a newline after formatted refs where the format expands\n> +\tto the empty string.\n> +\n> --column[=]::\n> --no-column::\n> \tDisplay branch listing in columns. See configuration variable\n> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\n> index 6da899c629..0f4fa98b18 100644\n> --- a/Documentation/git-for-each-ref.txt\n> +++ b/Documentation/git-for-each-ref.txt\n> @@ -93,6 +93,11 @@ OPTIONS\n> --ignore-case::\n> \tSorting and filtering refs are case insensitive.\n> \n> +-n::\n> +--omit-empty-lines::\n> +\tDo not print a newline after formatted refs where the format expands\n> +\tto the empty string.\n> +\n> FIELD NAMES\n> -----------\n> \n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index f63fd45edb..1bbb36b442 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -41,6 +41,7 @@ static const char *head;\n> static struct object_id head_oid;\n> static int recurse_submodules = 0;\n> static int submodule_propagate_branches = 0;\n> +static int omit_empty_lines = 0;\n> \n> static int branch_use_color = -1;\n> static char branch_colors[][COLOR_MAXLEN] = {\n> @@ -461,7 +462,8 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n> \t\t\tstring_list_append(output, out.buf);\n> \t\t} else {\n> \t\t\tfwrite(out.buf, 1, out.len, stdout);\n> -\t\t\tputchar('\\n');\n> +\t\t\tif (!omit_empty_lines || out.len > 0)\n> +\t\t\t\tputchar('\\n');\n> \t\t}\n> \t}\n> \n> @@ -670,6 +672,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n> \t\tOPT_BIT('D', NULL, &delete, N_(\"delete branch (even if not merged)\"), 2),\n> \t\tOPT_BIT('m', \"move\", &rename, N_(\"move/rename a branch and its reflog\"), 1),\n> \t\tOPT_BIT('M', NULL, &rename, N_(\"move/rename a branch, even if target exists\"), 2),\n> +\t\tOPT_BOOL('n' , \"omit-empty-lines\", &omit_empty_lines,\n> +\t\t\tN_(\"do not output a newline after empty formatted refs\")),\n> \t\tOPT_BIT('c', \"copy\", ©, N_(\"copy a branch and its reflog\"), 1),\n> \t\tOPT_BIT('C', NULL, ©, N_(\"copy a branch, even if target exists\"), 2),\n> \t\tOPT_BOOL('l', \"list\", &list, N_(\"list branch names\")),\n> @@ -757,7 +761,13 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n> \t}\n> \n> \tif (list)\n> +\t{\n> +\t\tif (omit_empty_lines && !format.format) {\n> +\t\t\terror(\"--omit-empty-lines without --format does not make sense\");\n> +\t\t\tusage_with_options(builtin_branch_usage, options);\n> +\t\t}\n> \t\tsetup_auto_pager(\"branch\", 1);\n> +\t}\n> \n> \tif (delete) {\n> \t\tif (!argc)\n> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\n> index 6f62f40d12..349c4d4ef8 100644\n> --- a/builtin/for-each-ref.c\n> +++ b/builtin/for-each-ref.c\n> @@ -19,7 +19,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n> \tint i;\n> \tstruct ref_sorting *sorting;\n> \tstruct string_list sorting_options = STRING_LIST_INIT_DUP;\n> -\tint maxcount = 0, icase = 0;\n> +\tint maxcount = 0, icase = 0, omit_empty_lines = 0;\n> \tstruct ref_array array;\n> \tstruct ref_filter filter;\n> \tstruct ref_format format = REF_FORMAT_INIT;\n> @@ -35,6 +35,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n> \t\t\tN_(\"quote placeholders suitably for python\"), QUOTE_PYTHON),\n> \t\tOPT_BIT(0 , \"tcl\", &format.quote_style,\n> \t\t\tN_(\"quote placeholders suitably for Tcl\"), QUOTE_TCL),\n> +\t\tOPT_BOOL('n' , \"omit-empty-lines\", &omit_empty_lines,\n> +\t\t\tN_(\"do not output a newline after empty formatted refs\")),\n> \n> \t\tOPT_GROUP(\"\"),\n> \t\tOPT_INTEGER( 0 , \"count\", &maxcount, N_(\"show only matched refs\")),\n> @@ -55,8 +57,6 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n> \tmemset(&array, 0, sizeof(array));\n> \tmemset(&filter, 0, sizeof(filter));\n> \n> -\tformat.format = \"%(objectname) %(objecttype)\\t%(refname)\";\n> -\n> \tgit_config(git_default_config, NULL);\n> \n> \tparse_options(argc, argv, prefix, opts, for_each_ref_usage, 0);\n> @@ -68,6 +68,12 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n> \t\terror(\"more than one quoting style?\");\n> \t\tusage_with_options(for_each_ref_usage, opts);\n> \t}\n> +\tif (omit_empty_lines && !format.format) {\n> +\t\terror(\"--omit-empty-lines without --format does not make sense\");\n> +\t\tusage_with_options(for_each_ref_usage, opts);\n> +\t}\n> +\tif (!format.format)\n> +\t\tformat.format = \"%(objectname) %(objecttype)\\t%(refname)\";\n> \tif (verify_ref_format(&format))\n> \t\tusage_with_options(for_each_ref_usage, opts);\n> \n> @@ -88,7 +94,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n> \t\tif (format_ref_array_item(array.items[i], &format, &output, &err))\n> \t\t\tdie(\"%s\", err.buf);\n> \t\tfwrite(output.buf, 1, output.len, stdout);\n> -\t\tputchar('\\n');\n> +\t\tif (!omit_empty_lines || output.len > 0)\n> +\t\t\tputchar('\\n');\n> \t}\n> \n> \tstrbuf_release(&err);\n> diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh\n> index d34d77f893..26bf0819ea 100755\n> --- a/t/t3203-branch-output.sh\n> +++ b/t/t3203-branch-output.sh\n> @@ -341,6 +341,32 @@ test_expect_success 'git branch with --format=%(rest) must fail' '\n> \ttest_must_fail git branch --format=\"%(rest)\" >actual\n> '\n> \n> +test_expect_success 'git branch --format --omit-empty-lines' '\n> +\tcat >expect <<-\\EOF &&\n> +\tRefname is (HEAD detached from fromtag)\n> +\tRefname is refs/heads/ambiguous\n> +\tRefname is refs/heads/branch-one\n> +\tRefname is refs/heads/branch-two\n> +\tEOF\n> +\techo >>expect &&\n> +\tcat >>expect <<-\\EOF &&\n> +\tRefname is refs/heads/ref-to-branch\n> +\tRefname is refs/heads/ref-to-remote\n> +\tEOF\n> +\tgit branch --format=\"%(if:notequals=refs/heads/main)%(refname)%(then)Refname is %(refname)%(end)\" >actual &&\n> +\ttest_cmp expect actual &&\n> +\tcat >expect <<-\\EOF &&\n> +\tRefname is (HEAD detached from fromtag)\n> +\tRefname is refs/heads/ambiguous\n> +\tRefname is refs/heads/branch-one\n> +\tRefname is refs/heads/branch-two\n> +\tRefname is refs/heads/ref-to-branch\n> +\tRefname is refs/heads/ref-to-remote\n> +\tEOF\n> +\tgit branch --omit-empty-lines --format=\"%(if:notequals=refs/heads/main)%(refname)%(then)Refname is %(refname)%(end)\" >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_expect_success 'worktree colors correct' '\n> \tcat >expect <<-EOF &&\n> \t* (HEAD detached from fromtag)\n> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\n> index c466fd989f..eec9d45513 100755\n> --- a/t/t6300-for-each-ref.sh\n> +++ b/t/t6300-for-each-ref.sh\n> @@ -1374,6 +1374,14 @@ test_expect_success 'for-each-ref --ignore-case ignores case' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'for-each-ref --omit-empty-lines works' '\n> +\tgit for-each-ref --format=\"%(refname)\" > actual &&\n> +\ttest_line_count -gt 1 actual &&\n> +\tgit for-each-ref --format=\"%(if:equals=refs/heads/main)%(refname)%(then)%(refname)%(end)\" --omit-empty-lines > actual &&\n> +\techo refs/heads/main > expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_expect_success 'for-each-ref --ignore-case works on multiple sort keys' '\n> \t# name refs numerically to avoid case-insensitive filesystem conflicts\n> \tnr=0 &&\n"},{"id":"474517","messageId":"xmqqjzywg7sg.fsf@gitster.g","threadId":"59498","inReplyTo":"44e7ac0f-2fd9-fd01-89da-a8d036d2e400@dunelm.org.uk","subject":"Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-31T17:17:03Z","receivedAt":"2023-03-31T17:17:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> Do the empty lines in the output serve any useful purpose? If not then\n> it might be better just to suppress them unconditionally rather than\n> adding a new command line option.\n\nIt's a nice egg of columbus.\n\nIt however theoretically can break an existing use case where the\nuser correlates the output with a list of refs they externally\nprepared (e.g. \"for-each-ref --format... a b c\" shows \"A\", \"\", and\n\"C\", and the user knows \"b\" produced \"\"). I do not know how likely\nsuch users complain, though, and if there is nobody who relies on\nthe current behaviour, surely \"unconditionally omit\" is a very\ntempting approach to take.\n\nThanks.\n\n\n\n"},{"id":"474940","messageId":"CAFaJEqtxNa+fuuKzkKPLkF3qdYwZUj+tMKXB3u2ok6H008vjHA@mail.gmail.com","threadId":"59498","inReplyTo":"xmqqjzywg7sg.fsf@gitster.g","subject":"Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines","fromName":"Øystein Walle","fromEmail":"oystwa@gmail.com","sentAt":"2023-04-06T16:55:52Z","receivedAt":"2023-04-06T16:56:35Z","isPatch":true,"sender":{"key":"oystwa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/794585?v=4"},"body":"On Fri, 31 Mar 2023 at 19:17, Junio C Hamano wrote:\n>\n> Phillip Wood writes:\n>\n> > Do the empty lines in the output serve any useful purpose? If not then\n> > it might be better just to suppress them unconditionally rather than\n> > adding a new command line option.\n>\n> It's a nice egg of columbus.\n>\n> It however theoretically can break an existing use case where the\n> user correlates the output with a list of refs they externally\n> prepared (e.g. \"for-each-ref --format... a b c\" shows \"A\", \"\", and\n> \"C\", and the user knows \"b\" produced \"\"). I do not know how likely\n> such users complain, though, and if there is nobody who relies on\n> the current behaviour, surely \"unconditionally omit\" is a very\n> tempting approach to take.\n>\n> Thanks.\n\nI actually instinctively expected for-each-ref to suppress empty lines, at\nleast by default. I don't see a good reason for them, except for something\nalong the lines of what you said.\n\nWe can of course make it a config option along with the flag, then after some\ntime flip the default, and perhaps ultimately remove the config option again.\nPerhaps in a v3 if there is enough interest; will send a v2 shortly. But I\nmust admit I am not very motivated to follow that up down the line.\n\nØsse\n"},{"id":"474941","messageId":"20230406170837.10060-1-oystwa@gmail.com","threadId":"59498","inReplyTo":"xmqq4jq0hp1i.fsf@gitster.g","subject":"[PATCH v2 0/2] branch, for-each-ref: add option to omit empty lines","fromName":"Øystein Walle","fromEmail":"oystwa@gmail.com","sentAt":"2023-04-06T17:08:35Z","receivedAt":"2023-04-06T17:09:00Z","isPatch":true,"sender":{"key":"oystwa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/794585?v=4"},"body":"Changes since v1:\n\n - Improved commit message in 1/2,\n - Stopped considering flag combinations as an error, which greatly\n simplified the logic,\n - Improved weird test file generation,\n - Removed short option and renamed long option,\n - Changed conditions to check the string length before the flag, which\n imo. reads better.\n\nØystein Walle (2):\n ref-filter: remove unused ref_format member\n branch, for-each-ref: add option to omit empty lines\n\n Documentation/git-branch.txt | 4 ++++\n Documentation/git-for-each-ref.txt | 4 ++++\n ref-filter.h | 1 -\n builtin/branch.c | 6 +++++-\n builtin/for-each-ref.c | 7 +++++--\n ref-filter.c | 1 -\n t/t3203-branch-output.sh | 24 ++++++++++++++++++++++++\n t/t6300-for-each-ref.sh | 8 ++++++++\n 8 files changed, 50 insertions(+), 5 deletions(-)\n\n-- \n2.20.1\n\n"},{"id":"474942","messageId":"20230406170837.10060-2-oystwa@gmail.com","threadId":"59498","inReplyTo":"20230406170837.10060-1-oystwa@gmail.com","subject":"[PATCH v2 1/2] ref-filter: remove unused ref_format member","fromName":"Øystein Walle","fromEmail":"oystwa@gmail.com","sentAt":"2023-04-06T17:08:36Z","receivedAt":"2023-04-06T17:09:01Z","isPatch":true,"sender":{"key":"oystwa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/794585?v=4"},"body":"use_rest was added in b9dee075eb (ref-filter: add %(rest) atom,\n2021-07-26) but was never used. A follow-up patch series[1] that used\nthis member was submitted, but ultimately the development was abandonded\ndue to performance problems.\n\n[1]: https://lore.kernel.org/git/9c5fddf6885875ccd3ce3f047bb938c77d9bbca2.1628842990.git.gitgitgadget@gmail.com/\n\nSigned-off-by: Øystein Walle \n---\n ref-filter.h | 1 -\n ref-filter.c | 1 -\n 2 files changed, 2 deletions(-)\n\ndiff --git a/ref-filter.h b/ref-filter.h\nindex daa6d02017..e3eea5e3ad 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -75,7 +75,6 @@ struct ref_format {\n \tconst char *format;\n \tconst char *rest;\n \tint quote_style;\n-\tint use_rest;\n \tint use_color;\n \n \t/* Internal state to ref-filter */\ndiff --git a/ref-filter.c b/ref-filter.c\nindex ed802778da..20e0a72f24 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -596,7 +596,6 @@ static int rest_atom_parser(struct ref_format *format,\n {\n \tif (arg)\n \t\treturn err_no_arg(err, \"rest\");\n-\tformat->use_rest = 1;\n \treturn 0;\n }\n \n-- \n2.20.1\n\n"},{"id":"474943","messageId":"20230406170837.10060-3-oystwa@gmail.com","threadId":"59498","inReplyTo":"20230406170837.10060-1-oystwa@gmail.com","subject":"[PATCH v2 2/2] branch, for-each-ref: add option to omit empty lines","fromName":"Øystein Walle","fromEmail":"oystwa@gmail.com","sentAt":"2023-04-06T17:08:37Z","receivedAt":"2023-04-06T17:09:02Z","isPatch":true,"sender":{"key":"oystwa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/794585?v=4"},"body":"If the given format string expands to the empty string a newline is\nstill printed it. This makes using the output linewise more tedious. For\nexample, git update-ref --stdin does not accept empty lines.\n\nAdd options to branch and for-each-ref to not print these empty lines.\nThe default behavior remains the same.\n\nSigned-off-by: Øystein Walle \n---\n Documentation/git-branch.txt | 4 ++++\n Documentation/git-for-each-ref.txt | 4 ++++\n builtin/branch.c | 6 +++++-\n builtin/for-each-ref.c | 7 +++++--\n t/t3203-branch-output.sh | 24 ++++++++++++++++++++++++\n t/t6300-for-each-ref.sh | 8 ++++++++\n 6 files changed, 50 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt\nindex d382ac69f7..d207da9101 100644\n--- a/Documentation/git-branch.txt\n+++ b/Documentation/git-branch.txt\n@@ -156,6 +156,10 @@ in another worktree linked to the same repository.\n --ignore-case::\n \tSorting and filtering branches are case insensitive.\n \n+--omit-empty::\n+\tDo not print a newline after formatted refs where the format expands\n+\tto the empty string.\n+\n --column[=]::\n --no-column::\n \tDisplay branch listing in columns. See configuration variable\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 6da899c629..af790bfa4e 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -93,6 +93,10 @@ OPTIONS\n --ignore-case::\n \tSorting and filtering refs are case insensitive.\n \n+--omit-empty::\n+\tDo not print a newline after formatted refs where the format expands\n+\tto the empty string.\n+\n FIELD NAMES\n -----------\n \ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex f63fd45edb..b47fef51fb 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -41,6 +41,7 @@ static const char *head;\n static struct object_id head_oid;\n static int recurse_submodules = 0;\n static int submodule_propagate_branches = 0;\n+static int omit_empty = 0;\n \n static int branch_use_color = -1;\n static char branch_colors[][COLOR_MAXLEN] = {\n@@ -461,7 +462,8 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n \t\t\tstring_list_append(output, out.buf);\n \t\t} else {\n \t\t\tfwrite(out.buf, 1, out.len, stdout);\n-\t\t\tputchar('\\n');\n+\t\t\tif (out.len || !omit_empty)\n+\t\t\t\tputchar('\\n');\n \t\t}\n \t}\n \n@@ -670,6 +672,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tOPT_BIT('D', NULL, &delete, N_(\"delete branch (even if not merged)\"), 2),\n \t\tOPT_BIT('m', \"move\", &rename, N_(\"move/rename a branch and its reflog\"), 1),\n \t\tOPT_BIT('M', NULL, &rename, N_(\"move/rename a branch, even if target exists\"), 2),\n+\t\tOPT_BOOL(0, \"omit-empty\", &omit_empty,\n+\t\t\tN_(\"do not output a newline after empty formatted refs\")),\n \t\tOPT_BIT('c', \"copy\", ©, N_(\"copy a branch and its reflog\"), 1),\n \t\tOPT_BIT('C', NULL, ©, N_(\"copy a branch, even if target exists\"), 2),\n \t\tOPT_BOOL('l', \"list\", &list, N_(\"list branch names\")),\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 6f62f40d12..1fc5130481 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -19,7 +19,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \tint i;\n \tstruct ref_sorting *sorting;\n \tstruct string_list sorting_options = STRING_LIST_INIT_DUP;\n-\tint maxcount = 0, icase = 0;\n+\tint maxcount = 0, icase = 0, omit_empty = 0;\n \tstruct ref_array array;\n \tstruct ref_filter filter;\n \tstruct ref_format format = REF_FORMAT_INIT;\n@@ -35,6 +35,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \t\t\tN_(\"quote placeholders suitably for python\"), QUOTE_PYTHON),\n \t\tOPT_BIT(0 , \"tcl\", &format.quote_style,\n \t\t\tN_(\"quote placeholders suitably for Tcl\"), QUOTE_TCL),\n+\t\tOPT_BOOL(0, \"omit-empty\", &omit_empty,\n+\t\t\tN_(\"do not output a newline after empty formatted refs\")),\n \n \t\tOPT_GROUP(\"\"),\n \t\tOPT_INTEGER( 0 , \"count\", &maxcount, N_(\"show only matched refs\")),\n@@ -88,7 +90,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \t\tif (format_ref_array_item(array.items[i], &format, &output, &err))\n \t\t\tdie(\"%s\", err.buf);\n \t\tfwrite(output.buf, 1, output.len, stdout);\n-\t\tputchar('\\n');\n+\t\tif (output.len || !omit_empty)\n+\t\t\tputchar('\\n');\n \t}\n \n \tstrbuf_release(&err);\ndiff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh\nindex d34d77f893..c06906d83e 100755\n--- a/t/t3203-branch-output.sh\n+++ b/t/t3203-branch-output.sh\n@@ -341,6 +341,30 @@ test_expect_success 'git branch with --format=%(rest) must fail' '\n \ttest_must_fail git branch --format=\"%(rest)\" >actual\n '\n \n+test_expect_success 'git branch --format --omit-empty' '\n+\tcat >expect <<-\\EOF &&\n+\tRefname is (HEAD detached from fromtag)\n+\tRefname is refs/heads/ambiguous\n+\tRefname is refs/heads/branch-one\n+\tRefname is refs/heads/branch-two\n+\n+\tRefname is refs/heads/ref-to-branch\n+\tRefname is refs/heads/ref-to-remote\n+\tEOF\n+\tgit branch --format=\"%(if:notequals=refs/heads/main)%(refname)%(then)Refname is %(refname)%(end)\" >actual &&\n+\ttest_cmp expect actual &&\n+\tcat >expect <<-\\EOF &&\n+\tRefname is (HEAD detached from fromtag)\n+\tRefname is refs/heads/ambiguous\n+\tRefname is refs/heads/branch-one\n+\tRefname is refs/heads/branch-two\n+\tRefname is refs/heads/ref-to-branch\n+\tRefname is refs/heads/ref-to-remote\n+\tEOF\n+\tgit branch --omit-empty --format=\"%(if:notequals=refs/heads/main)%(refname)%(then)Refname is %(refname)%(end)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'worktree colors correct' '\n \tcat >expect <<-EOF &&\n \t* (HEAD detached from fromtag)\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex c466fd989f..d4ccc22d99 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -1374,6 +1374,14 @@ test_expect_success 'for-each-ref --ignore-case ignores case' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'for-each-ref --omit-empty works' '\n+\tgit for-each-ref --format=\"%(refname)\" >actual &&\n+\ttest_line_count -gt 1 actual &&\n+\tgit for-each-ref --format=\"%(if:equals=refs/heads/main)%(refname)%(then)%(refname)%(end)\" --omit-empty >actual &&\n+\techo refs/heads/main >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'for-each-ref --ignore-case works on multiple sort keys' '\n \t# name refs numerically to avoid case-insensitive filesystem conflicts\n \tnr=0 &&\n-- \n2.20.1\n\n"},{"id":"474944","messageId":"20230406171203.GB2709660@coredump.intra.peff.net","threadId":"59498","inReplyTo":"CAFaJEqtxNa+fuuKzkKPLkF3qdYwZUj+tMKXB3u2ok6H008vjHA@mail.gmail.com","subject":"Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-06T17:12:03Z","receivedAt":"2023-04-06T17:12:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 06, 2023 at 06:55:52PM +0200, Øystein Walle wrote:\n\n> > It however theoretically can break an existing use case where the\n> > user correlates the output with a list of refs they externally\n> > prepared (e.g. \"for-each-ref --format... a b c\" shows \"A\", \"\", and\n> > \"C\", and the user knows \"b\" produced \"\"). I do not know how likely\n> > such users complain, though, and if there is nobody who relies on\n> > the current behaviour, surely \"unconditionally omit\" is a very\n> > tempting approach to take.\n> >\n> > Thanks.\n> \n> I actually instinctively expected for-each-ref to suppress empty lines, at\n> least by default. I don't see a good reason for them, except for something\n> along the lines of what you said.\n> \n> We can of course make it a config option along with the flag, then after some\n> time flip the default, and perhaps ultimately remove the config option again.\n> Perhaps in a v3 if there is enough interest; will send a v2 shortly. But I\n> must admit I am not very motivated to follow that up down the line.\n\nIt might be enough to flip the default unconditionally (no config), but\nI think we may still want \"--no-omit-empty-lines\" as an escape hatch. I\ndunno. Maybe that is somehow choosing the worst of both worlds.\n\n-Peff\n"},{"id":"474945","messageId":"xmqqv8i8q3zj.fsf@gitster.g","threadId":"59498","inReplyTo":"CAFaJEqtxNa+fuuKzkKPLkF3qdYwZUj+tMKXB3u2ok6H008vjHA@mail.gmail.com","subject":"Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-06T18:07:12Z","receivedAt":"2023-04-06T18:07:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Øystein Walle writes:\n\n>> It however theoretically can break an existing use case where the\n>> user correlates the output with a list of refs they externally\n>> prepared (e.g. \"for-each-ref --format... a b c\" shows \"A\", \"\", and\n>> \"C\", and the user knows \"b\" produced \"\"). I do not know how likely\n>> such users complain, though, and if there is nobody who relies on\n>> the current behaviour, surely \"unconditionally omit\" is a very\n>> tempting approach to take.\n>>\n>> Thanks.\n>\n> I actually instinctively expected for-each-ref to suppress empty lines, at\n> least by default. I don't see a good reason for them, except for something\n> along the lines of what you said.\n\nThat makes two of us ;-)\n\n> We can of course make it a config option along with the flag, then after some\n> time flip the default, and perhaps ultimately remove the config option again.\n\nYeah, but this v2 is not starting with purely a new feature without\nbreaking anybody, so we can stop here for now, and once there is\nenough interest to go through the deprecation dance, we can do that\nas a separate series later.\n\nThanks.\n"},{"id":"474946","messageId":"xmqqo7o0q3e4.fsf@gitster.g","threadId":"59498","inReplyTo":"20230406171203.GB2709660@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-06T18:20:03Z","receivedAt":"2023-04-06T18:20:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> It might be enough to flip the default unconditionally (no config), but\n> I think we may still want \"--no-omit-empty-lines\" as an escape hatch. I\n> dunno. Maybe that is somehow choosing the worst of both worlds.\n\nIt is very tempting, indeed. We can add the escape hatch and flip\nthe default, and only when somebody complains, come back and say\n\"oh, sorry, we didn't know anybody used it\" and flip the default\nback, perhaps?\n\nThis is a totally unrelated tangent, but it is a bit unfortunate\nthat with our parse-options API, it is not trivial to\n\n - mark that \"--keep-empty-lines\" and \"--omit-empty-lines\" toggle\n the same underlying Boolean variable,\n\n - accept \"--no-keep\" and \"--no-omit\" as obvious synonyms for\n \"--omit\" and \"--keep\", \n\n - have \"git foo -h\" listing to show \"--keep\" and \"--omit\" together,\n\n - omit these \"--no-foo\" variants from \"git foo -h\" listing.\n\nby the way.\n\n"},{"id":"474947","messageId":"xmqqjzyoq35x.fsf@gitster.g","threadId":"59498","inReplyTo":"20230406170837.10060-1-oystwa@gmail.com","subject":"Re: [PATCH v2 0/2] branch, for-each-ref: add option to omit empty lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-06T18:24:58Z","receivedAt":"2023-04-06T18:25:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Øystein Walle writes:\n\n> Øystein Walle (2):\n> ref-filter: remove unused ref_format member\n> branch, for-each-ref: add option to omit empty lines\n>\n> Documentation/git-branch.txt | 4 ++++\n> Documentation/git-for-each-ref.txt | 4 ++++\n> ref-filter.h | 1 -\n> builtin/branch.c | 6 +++++-\n> builtin/for-each-ref.c | 7 +++++--\n> ref-filter.c | 1 -\n> t/t3203-branch-output.sh | 24 ++++++++++++++++++++++++\n> t/t6300-for-each-ref.sh | 8 ++++++++\n> 8 files changed, 50 insertions(+), 5 deletions(-)\n\nI have always thought that the listing mode of \"branch\" and \"tag\"\nsubcommands is a mere syntax sugar around \"for-each-ref\", and the\nabove leaves me puzzled. Did we decide not to add the same for \"git\ntag\" in the listing mode during the review of the previous round (if\nwe did, I do not recall the discussion), or would it be just the\nmatter of adding another 6-line patch to builtin/tag.c?\n\nThanks.\n\n\n\n"},{"id":"475014","messageId":"20230407175316.6404-1-oystwa@gmail.com","threadId":"59498","inReplyTo":"xmqqjzyoq35x.fsf@gitster.g","subject":"[PATCH v3] branch, for-each-ref, tag: add option to omit empty lines","fromName":"Øystein Walle","fromEmail":"oystwa@gmail.com","sentAt":"2023-04-07T17:53:16Z","receivedAt":"2023-04-07T17:53:40Z","isPatch":true,"sender":{"key":"oystwa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/794585?v=4"},"body":"If the given format string expands to the empty string a newline is\nstill printed it. This makes using the output linewise more tedious. For\nexample, git update-ref --stdin does not accept empty lines.\n\nAdd options to branch and for-each-ref to not print these empty lines.\nThe default behavior remains the same.\n\nSigned-off-by: Øystein Walle \n---\nDang, you're right. But yes, it was a near-identical patch to\nbuiltin/tag.c. Along with a test, of course.\n\nI see you already applied the first of these patches so in this\niteration there's only one. \n\n Documentation/git-branch.txt | 4 ++++\n Documentation/git-for-each-ref.txt | 4 ++++\n Documentation/git-tag.txt | 4 ++++\n builtin/branch.c | 6 +++++-\n builtin/for-each-ref.c | 7 +++++--\n builtin/tag.c | 6 +++++-\n t/t3203-branch-output.sh | 24 ++++++++++++++++++++++++\n t/t6300-for-each-ref.sh | 8 ++++++++\n t/t7004-tag.sh | 16 ++++++++++++++++\n 9 files changed, 75 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt\nindex d382ac69f7..d207da9101 100644\n--- a/Documentation/git-branch.txt\n+++ b/Documentation/git-branch.txt\n@@ -156,6 +156,10 @@ in another worktree linked to the same repository.\n --ignore-case::\n \tSorting and filtering branches are case insensitive.\n \n+--omit-empty::\n+\tDo not print a newline after formatted refs where the format expands\n+\tto the empty string.\n+\n --column[=]::\n --no-column::\n \tDisplay branch listing in columns. See configuration variable\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 0713e49b49..1e215d4e73 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -98,6 +98,10 @@ OPTIONS\n --ignore-case::\n \tSorting and filtering refs are case insensitive.\n \n+--omit-empty::\n+\tDo not print a newline after formatted refs where the format expands\n+\tto the empty string.\n+\n FIELD NAMES\n -----------\n \ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex fdc72b5875..7f61c1edb3 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -131,6 +131,10 @@ options for details.\n --ignore-case::\n \tSorting and filtering tags are case insensitive.\n \n+--omit-empty::\n+\tDo not print a newline after formatted refs where the format expands\n+\tto the empty string.\n+\n --column[=]::\n --no-column::\n \tDisplay tag listing in columns. See configuration variable\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 6413a016c5..98d5fa2c8e 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -44,6 +44,7 @@ static const char *head;\n static struct object_id head_oid;\n static int recurse_submodules = 0;\n static int submodule_propagate_branches = 0;\n+static int omit_empty = 0;\n \n static int branch_use_color = -1;\n static char branch_colors[][COLOR_MAXLEN] = {\n@@ -466,7 +467,8 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n \t\t\tstring_list_append(output, out.buf);\n \t\t} else {\n \t\t\tfwrite(out.buf, 1, out.len, stdout);\n-\t\t\tputchar('\\n');\n+\t\t\tif (out.len || !omit_empty)\n+\t\t\t\tputchar('\\n');\n \t\t}\n \t}\n \n@@ -675,6 +677,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tOPT_BIT('D', NULL, &delete, N_(\"delete branch (even if not merged)\"), 2),\n \t\tOPT_BIT('m', \"move\", &rename, N_(\"move/rename a branch and its reflog\"), 1),\n \t\tOPT_BIT('M', NULL, &rename, N_(\"move/rename a branch, even if target exists\"), 2),\n+\t\tOPT_BOOL(0, \"omit-empty\", &omit_empty,\n+\t\t\tN_(\"do not output a newline after empty formatted refs\")),\n \t\tOPT_BIT('c', \"copy\", ©, N_(\"copy a branch and its reflog\"), 1),\n \t\tOPT_BIT('C', NULL, ©, N_(\"copy a branch, even if target exists\"), 2),\n \t\tOPT_BOOL('l', \"list\", &list, N_(\"list branch names\")),\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 0bdc49a6e1..695fc8f4a5 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -22,7 +22,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \tint i;\n \tstruct ref_sorting *sorting;\n \tstruct string_list sorting_options = STRING_LIST_INIT_DUP;\n-\tint maxcount = 0, icase = 0;\n+\tint maxcount = 0, icase = 0, omit_empty = 0;\n \tstruct ref_array array;\n \tstruct ref_filter filter;\n \tstruct ref_format format = REF_FORMAT_INIT;\n@@ -40,6 +40,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \t\t\tN_(\"quote placeholders suitably for python\"), QUOTE_PYTHON),\n \t\tOPT_BIT(0 , \"tcl\", &format.quote_style,\n \t\t\tN_(\"quote placeholders suitably for Tcl\"), QUOTE_TCL),\n+\t\tOPT_BOOL(0, \"omit-empty\", &omit_empty,\n+\t\t\tN_(\"do not output a newline after empty formatted refs\")),\n \n \t\tOPT_GROUP(\"\"),\n \t\tOPT_INTEGER( 0 , \"count\", &maxcount, N_(\"show only matched refs\")),\n@@ -112,7 +114,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \t\tif (format_ref_array_item(array.items[i], &format, &output, &err))\n \t\t\tdie(\"%s\", err.buf);\n \t\tfwrite(output.buf, 1, output.len, stdout);\n-\t\tputchar('\\n');\n+\t\tif (output.len || !omit_empty)\n+\t\t\tputchar('\\n');\n \t}\n \n \tstrbuf_release(&err);\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 782bb3aa2f..ab5f5c74f4 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -41,6 +41,7 @@ static const char * const git_tag_usage[] = {\n static unsigned int colopts;\n static int force_sign_annotate;\n static int config_sign_tag = -1; /* unspecified */\n+static int omit_empty = 0;\n \n static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,\n \t\t struct ref_format *format)\n@@ -79,7 +80,8 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,\n \t\tif (format_ref_array_item(array.items[i], format, &output, &err))\n \t\t\tdie(\"%s\", err.buf);\n \t\tfwrite(output.buf, 1, output.len, stdout);\n-\t\tputchar('\\n');\n+\t\tif (output.len || !omit_empty)\n+\t\t\tputchar('\\n');\n \t}\n \n \tstrbuf_release(&err);\n@@ -474,6 +476,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tOPT_WITHOUT(&filter.no_commit, N_(\"print only tags that don't contain the commit\")),\n \t\tOPT_MERGED(&filter, N_(\"print only tags that are merged\")),\n \t\tOPT_NO_MERGED(&filter, N_(\"print only tags that are not merged\")),\n+\t\tOPT_BOOL(0, \"omit-empty\", &omit_empty,\n+\t\t\tN_(\"do not output a newline after empty formatted refs\")),\n \t\tOPT_REF_SORT(&sorting_options),\n \t\t{\n \t\t\tOPTION_CALLBACK, 0, \"points-at\", &filter.points_at, N_(\"object\"),\ndiff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh\nindex 1c0f7ea24e..93f8295339 100755\n--- a/t/t3203-branch-output.sh\n+++ b/t/t3203-branch-output.sh\n@@ -355,6 +355,30 @@ test_expect_success 'git branch with --format=%(rest) must fail' '\n \ttest_must_fail git branch --format=\"%(rest)\" >actual\n '\n \n+test_expect_success 'git branch --format --omit-empty' '\n+\tcat >expect <<-\\EOF &&\n+\tRefname is (HEAD detached from fromtag)\n+\tRefname is refs/heads/ambiguous\n+\tRefname is refs/heads/branch-one\n+\tRefname is refs/heads/branch-two\n+\n+\tRefname is refs/heads/ref-to-branch\n+\tRefname is refs/heads/ref-to-remote\n+\tEOF\n+\tgit branch --format=\"%(if:notequals=refs/heads/main)%(refname)%(then)Refname is %(refname)%(end)\" >actual &&\n+\ttest_cmp expect actual &&\n+\tcat >expect <<-\\EOF &&\n+\tRefname is (HEAD detached from fromtag)\n+\tRefname is refs/heads/ambiguous\n+\tRefname is refs/heads/branch-one\n+\tRefname is refs/heads/branch-two\n+\tRefname is refs/heads/ref-to-branch\n+\tRefname is refs/heads/ref-to-remote\n+\tEOF\n+\tgit branch --omit-empty --format=\"%(if:notequals=refs/heads/main)%(refname)%(then)Refname is %(refname)%(end)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'worktree colors correct' '\n \tcat >expect <<-EOF &&\n \t* (HEAD detached from fromtag)\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 6614469d2d..5c00607608 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -1374,6 +1374,14 @@ test_expect_success 'for-each-ref --ignore-case ignores case' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'for-each-ref --omit-empty works' '\n+\tgit for-each-ref --format=\"%(refname)\" >actual &&\n+\ttest_line_count -gt 1 actual &&\n+\tgit for-each-ref --format=\"%(if:equals=refs/heads/main)%(refname)%(then)%(refname)%(end)\" --omit-empty >actual &&\n+\techo refs/heads/main >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'for-each-ref --ignore-case works on multiple sort keys' '\n \t# name refs numerically to avoid case-insensitive filesystem conflicts\n \tnr=0 &&\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 32b312fa80..0fe6ba93a2 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -2046,6 +2046,22 @@ test_expect_success '--format should list tags as per format given' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--format --omit-empty works' '\n+\tcat >expect <<-\\EOF &&\n+\trefname : refs/tags/v1.0\n+\n+\trefname : refs/tags/v1.1.3\n+\tEOF\n+\tgit tag -l --format=\"%(if:notequals=refs/tags/v1.0.1)%(refname)%(then)refname : %(refname)%(end)\" \"v1*\" >actual &&\n+\ttest_cmp expect actual &&\n+\tcat >expect <<-\\EOF &&\n+\trefname : refs/tags/v1.0\n+\trefname : refs/tags/v1.1.3\n+\tEOF\n+\tgit tag -l --omit-empty --format=\"%(if:notequals=refs/tags/v1.0.1)%(refname)%(then)refname : %(refname)%(end)\" \"v1*\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'git tag -l with --format=\"%(rest)\" must fail' '\n \ttest_must_fail git tag -l --format=\"%(rest)\" \"v1*\"\n '\n-- \n2.20.1\n\n"},{"id":"475015","messageId":"xmqqile7jzq2.fsf@gitster.g","threadId":"59498","inReplyTo":"20230407175316.6404-1-oystwa@gmail.com","subject":"Re: [PATCH v3] branch, for-each-ref, tag: add option to omit empty lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-07T18:48:05Z","receivedAt":"2023-04-07T18:49:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Øystein Walle writes:\n\n> If the given format string expands to the empty string a newline is\n> still printed it. This makes using the output linewise more tedious. For\n> example, git update-ref --stdin does not accept empty lines.\n>\n> Add options to branch and for-each-ref to not print these empty lines.\n> The default behavior remains the same.\n>\n> Signed-off-by: Øystein Walle \n> ---\n> Dang, you're right. But yes, it was a near-identical patch to\n> builtin/tag.c. Along with a test, of course.\n\nThere are small nits like \"do not explicitly initialize statics to\n0\", which may not be big enough to warrant a reroll. Other than\nthat, looking good.\n\nThanks, will replace.\n"},{"id":"475078","messageId":"20230410195644.GA104097@coredump.intra.peff.net","threadId":"59498","inReplyTo":"xmqqo7o0q3e4.fsf@gitster.g","subject":"Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-10T19:56:44Z","receivedAt":"2023-04-10T19:56:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 06, 2023 at 11:20:03AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > It might be enough to flip the default unconditionally (no config), but\n> > I think we may still want \"--no-omit-empty-lines\" as an escape hatch. I\n> > dunno. Maybe that is somehow choosing the worst of both worlds.\n> \n> It is very tempting, indeed. We can add the escape hatch and flip\n> the default, and only when somebody complains, come back and say\n> \"oh, sorry, we didn't know anybody used it\" and flip the default\n> back, perhaps?\n\nI don't think flipping back after such an incident is a good idea, as it\njust creates more confusion. But if the option exists, then at least you\ncan say \"oh, sorry; you can still do what you want by passing this\noption\", rather than \"oh, sorry; there's no way to get what you want\".\n\nBut either way, the first step before flipping any defaults is adding an\noption, which is what this patch is doing, so I am all for it. :)\n\n> This is a totally unrelated tangent, but it is a bit unfortunate\n> that with our parse-options API, it is not trivial to\n> \n> - mark that \"--keep-empty-lines\" and \"--omit-empty-lines\" toggle\n> the same underlying Boolean variable,\n> \n> - accept \"--no-keep\" and \"--no-omit\" as obvious synonyms for\n> \"--omit\" and \"--keep\", \n> \n> - have \"git foo -h\" listing to show \"--keep\" and \"--omit\" together,\n> \n> - omit these \"--no-foo\" variants from \"git foo -h\" listing.\n> \n> by the way.\n\nYeah, \"--no-\" is special in our parser in a way that \"--keep\" and\n\"--omit\" aren't. It might be possible to make this pattern easier to\nsupport. OTOH, perhaps it is a sign that we are straying too far from\nexisting patterns. It is not just parse-options.c, but also users\nthemselves, who benefit from consistency.\n\n-Peff\n"},{"id":"475258","messageId":"a8b34639-60fb-8a23-d1d9-1ef4410a2ba4@gmail.com","threadId":"59498","inReplyTo":"20230407175316.6404-1-oystwa@gmail.com","subject":"Re: [PATCH v3] branch, for-each-ref, tag: add option to omit empty lines","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-12T23:44:23Z","receivedAt":"2023-04-12T23:44:30Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"A couple of drive-by nitpicks about the commit message:\n\nOn 07/04/2023 19:53, Øystein Walle wrote:\n> Subject: [PATCH v3] branch, for-each-ref, tag: add option to omit empty lines\n> \n> If the given format string expands to the empty string a newline is\n> still printed it. This makes using the output linewise more tedious. For\n\nIt seems that a word is missing in the first sentence. Perhaps,\n\n s/printed it/printed for it/\n\n?\n\n> example, git update-ref --stdin does not accept empty lines.\n> \n> Add options to branch and for-each-ref to not print these empty lines.\n\n\"git tag\" is mentioned in the subject line, but not here.\n\n> The default behavior remains the same.\n> \n> Signed-off-by: Øystein Walle \n> ---\n> Dang, you're right. But yes, it was a near-identical patch to\n> builtin/tag.c. Along with a test, of course.\n> \n> I see you already applied the first of these patches so in this\n> iteration there's only one.\n> \n> Documentation/git-branch.txt | 4 ++++\n> Documentation/git-for-each-ref.txt | 4 ++++\n> Documentation/git-tag.txt | 4 ++++\n> builtin/branch.c | 6 +++++-\n> builtin/for-each-ref.c | 7 +++++--\n> builtin/tag.c | 6 +++++-\n> t/t3203-branch-output.sh | 24 ++++++++++++++++++++++++\n> t/t6300-for-each-ref.sh | 8 ++++++++\n> t/t7004-tag.sh | 16 ++++++++++++++++\n> 9 files changed, 75 insertions(+), 4 deletions(-)\n> \n"},{"id":"475266","messageId":"CAFaJEqtWg-6wvLNQy1DVSquVOu==E67gN7QYw-sAUf9fufOngw@mail.gmail.com","threadId":"59498","inReplyTo":"a8b34639-60fb-8a23-d1d9-1ef4410a2ba4@gmail.com","subject":"Re: [PATCH v3] branch, for-each-ref, tag: add option to omit empty lines","fromName":"Øystein Walle","fromEmail":"oystwa@gmail.com","sentAt":"2023-04-13T07:17:02Z","receivedAt":"2023-04-13T07:17:45Z","isPatch":true,"sender":{"key":"oystwa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/794585?v=4"},"body":"Hi Andrei,\n\nOn Thu, 13 Apr 2023 at 01:44, Andrei Rybak wrote:\n\n> It seems that a word is missing in the first sentence. Perhaps,\n>\n> s/printed it/printed for it/\n>\n> ?\n\nSort of... I think I meant s/printed it/printed/ :-)\n\n> \"git tag\" is mentioned in the subject line, but not here.\n\nIt should definitely be added, yes. Junio, should I resend or will you touch up\nthe message? Not sure what the proper procedure is since it's already in seen.\n\nThanks.\n\nØsse\n"},{"id":"475308","messageId":"xmqqbkjrzufg.fsf@gitster.g","threadId":"59498","inReplyTo":"CAFaJEqtWg-6wvLNQy1DVSquVOu==E67gN7QYw-sAUf9fufOngw@mail.gmail.com","subject":"Re: [PATCH v3] branch, for-each-ref, tag: add option to omit empty lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-13T15:13:55Z","receivedAt":"2023-04-13T15:14:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Øystein Walle writes:\n\n> Sort of... I think I meant s/printed it/printed/ :-)\n>\n>> \"git tag\" is mentioned in the subject line, but not here.\n>\n> It should definitely be added, yes. Junio, should I resend or will you touch up\n> the message? Not sure what the proper procedure is since it's already in seen.\n\nAs I write in \"What's cooking\", being in 'seen' does not count all\nthat much and we can freely replace them and erace the trace of\n\"past mistakes\", until the series hits 'next'. For small changes\nlike this, telling me to touch them up, as long as the necessary\nchanges are obvious, is just fine. Sending out v4 is also fine for\na series of any size.\n\nI just locally amended the commit log message.\n\nThanks.\n\n1: e0053ad012 ! 1: aabfdc9514 branch, for-each-ref, tag: add option to omit empty lines\n @@ Metadata\n ## Commit message ##\n branch, for-each-ref, tag: add option to omit empty lines\n \n - If the given format string expands to the empty string a newline is\n - still printed it. This makes using the output linewise more tedious. For\n + If the given format string expands to the empty string, a newline is\n + still printed. This makes using the output linewise more tedious. For\n example, git update-ref --stdin does not accept empty lines.\n \n - Add options to branch and for-each-ref to not print these empty lines.\n - The default behavior remains the same.\n + Add options to \"git branch\", \"git for-each-ref\", and \"git tag\" to\n + not print these empty lines. The default behavior remains the same.\n \n Signed-off-by: Øystein Walle \n Signed-off-by: Junio C Hamano \n"}]}