| Message ID | 20230330112133.4437-3-oystwa@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | branch, for-each-ref: add option to omit empty lines | expand |
Øystein Walle <oystwa@gmail.com> writes: > diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt > index 6da899c629..0f4fa98b18 100644 > --- a/Documentation/git-for-each-ref.txt > +++ b/Documentation/git-for-each-ref.txt > @@ -93,6 +93,11 @@ OPTIONS > --ignore-case:: > Sorting and filtering refs are case insensitive. > > +-n:: > +--omit-empty-lines:: > + Do not print a newline after formatted refs where the format expands > + to the empty string. While I can see the utility of the new feature, it is unclear if its merit is so clear that it deserves a short-and-sweet single letter option from the get go. Especially, don't we want to give this to "git branch" and "git tag" in their listing modes for consistency, but it means stealing "-n" also from them. > @@ -757,7 +761,13 @@ int cmd_branch(int argc, const char **argv, const char *prefix) > } > > if (list) > + { Move that opening brace at the end of the previous line, i.e. - if (list) + if (list) { > + if (omit_empty_lines && !format.format) { > + error("--omit-empty-lines without --format does not make sense"); > + usage_with_options(builtin_branch_usage, options); > + } Does it not make sense? With the default format, it may happen that there will be no empty line so there is nothing to omit, but I do not see a strong reason to forbid the request like this. > setup_auto_pager("branch", 1); > + } > > if (delete) { > if (!argc) > diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c > index 6f62f40d12..349c4d4ef8 100644 > --- a/builtin/for-each-ref.c > +++ b/builtin/for-each-ref.c > @@ -19,7 +19,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix) > int i; > struct ref_sorting *sorting; > struct string_list sorting_options = STRING_LIST_INIT_DUP; > - int maxcount = 0, icase = 0; > + int maxcount = 0, icase = 0, omit_empty_lines = 0; > struct ref_array array; > struct ref_filter filter; > struct ref_format format = REF_FORMAT_INIT; > @@ -35,6 +35,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix) > N_("quote placeholders suitably for python"), QUOTE_PYTHON), > OPT_BIT(0 , "tcl", &format.quote_style, > N_("quote placeholders suitably for Tcl"), QUOTE_TCL), > + OPT_BOOL('n' , "omit-empty-lines", &omit_empty_lines, > + N_("do not output a newline after empty formatted refs")), > > OPT_GROUP(""), > OPT_INTEGER( 0 , "count", &maxcount, N_("show only <n> matched refs")), > @@ -55,8 +57,6 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix) > memset(&array, 0, sizeof(array)); > memset(&filter, 0, sizeof(filter)); > > - format.format = "%(objectname) %(objecttype)\t%(refname)"; > - > git_config(git_default_config, NULL); > parse_options(argc, argv, prefix, opts, for_each_ref_usage, 0); This smells fishy. We establish the hardcoded built-in default, let the config machinery override, and then finally let command line options to further override. You may be able to reach the same end result by leaving the value unset, fill with the configured value, override with the command line, and then if the value is still unset, fall back to a hardcoded built-in default, but I do not see why such a change logically belongs to a patch to add "--omit-empty" feature. > @@ -68,6 +68,12 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix) > error("more than one quoting style?"); > usage_with_options(for_each_ref_usage, opts); > } > + if (omit_empty_lines && !format.format) { > + error("--omit-empty-lines without --format does not make sense"); > + usage_with_options(for_each_ref_usage, opts); > + } I wouldn't do this, for the same reason as for "git branch". > + if (!format.format) > + format.format = "%(objectname) %(objecttype)\t%(refname)"; This is the other half of the earlier change I called "fishy". It may be benign, but it is distracting, especially when done without explanation, in a change to add a feature that is not related. > @@ -88,7 +94,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix) > if (format_ref_array_item(array.items[i], &format, &output, &err)) > die("%s", err.buf); > fwrite(output.buf, 1, output.len, stdout); > - putchar('\n'); > + if (!omit_empty_lines || output.len > 0) > + putchar('\n'); > } OK, but two points. * do not compare output.len with 0; it is sufficient to just write if (!omit_empty || output.len) * now we care if output is empty anyway, perhaps we can optimize out fwrite() too, perhaps with something like if (output.len || !omit_empty) printf("%.*s\n", output.len, output.buf); perhaps? I am not sure about the latter, but we tend to use "%.*s" liberally when we could use fwrite() in our codebase for brevity, so ... > +test_expect_success 'git branch --format --omit-empty-lines' ' > + cat >expect <<-\EOF && > + Refname is (HEAD detached from fromtag) > + Refname is refs/heads/ambiguous > + Refname is refs/heads/branch-one > + Refname is refs/heads/branch-two > + EOF > + echo >>expect && > + cat >>expect <<-\EOF && > + Refname is refs/heads/ref-to-branch > + Refname is refs/heads/ref-to-remote > + EOF It is hard to see that there is an empty line expected when the expectation is prepared like this. Why not something like cat >expect.full <<-\EOF && one two four (three is missing) EOF sed -e "/^$/d" expect.full >expect.stripped && git branch $args >actual && test_cmp expect.full actual && git branch --omit-empty $args >actual && test_cmp expect.stripped actual && that highlights the fact that there is a missing line for one expectation, and that the only difference in two expectations is the lack of empty line(s)? > diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh > index c466fd989f..eec9d45513 100755 > --- a/t/t6300-for-each-ref.sh > +++ b/t/t6300-for-each-ref.sh > @@ -1374,6 +1374,14 @@ test_expect_success 'for-each-ref --ignore-case ignores case' ' > test_cmp expect actual > ' > > +test_expect_success 'for-each-ref --omit-empty-lines works' ' > + git for-each-ref --format="%(refname)" > actual && > + test_line_count -gt 1 actual && The next test depends on that a branch 'main' exists, so perhaps that should be tested here, at least? And then if there is no other branches and tags, we cannot tell if seeing only the 'main' branch is due to --omit-empty correctly working, or due to the repository having only that branch, so it is also good to check if there is branches or tags other than 'main' in the output. > + git for-each-ref --format="%(if:equals=refs/heads/main)%(refname)%(then)%(refname)%(end)" --omit-empty-lines > actual && > + echo refs/heads/main > expect && > + test_cmp expect actual > +' By the way, lose SP between redirection operator '>' and its target, i.e. write them like so: echo refs/heads/main >expect This feature makes %(if)...%(else)...%(end) construct complete and is a very good addition. Thanks for working on it.
Øystein Walle <oystwa@gmail.com> writes: > The logic is more or less duplicated in branch.c and for-each-ref.c > which I don't like. However I couldn't really find a "central" place to > put it. Imo. it's definitely not a property of the format or the filter, > so struct ref_format and struct ref_filter are no good. I think the division of labor is very much in line with how the ref-filter API is currently laid out. Enumerating is done calling filter_refs(), and result is returned in an array, which the caller adds whatever frills around its elements to show them in the output. The "adding frills" is aided by calling format_ref_array_item(), but the API does not care how the formatting result is used.
On Thu, Mar 30, 2023 at 08:54:35AM -0700, Junio C Hamano wrote: > * now we care if output is empty anyway, perhaps we can optimize > out fwrite() too, perhaps with something like > > if (output.len || !omit_empty) > printf("%.*s\n", output.len, output.buf); > > perhaps? > > I am not sure about the latter, but we tend to use "%.*s" liberally > when we could use fwrite() in our codebase for brevity, so ... I think it would be a mistake here, as you can use "%00" in the format to include a NUL in the output. (The rest of your review seemed quite sensible to me, and I like the idea of the omit-empty option in general). -Peff
Jeff King <peff@peff.net> writes: > On Thu, Mar 30, 2023 at 08:54:35AM -0700, Junio C Hamano wrote: > >> * now we care if output is empty anyway, perhaps we can optimize >> out fwrite() too, perhaps with something like >> >> if (output.len || !omit_empty) >> printf("%.*s\n", output.len, output.buf); >> >> perhaps? >> >> I am not sure about the latter, but we tend to use "%.*s" liberally >> when we could use fwrite() in our codebase for brevity, so ... > > I think it would be a mistake here, as you can use "%00" in the format > to include a NUL in the output. Good point. Thanks for catching it. > > (The rest of your review seemed quite sensible to me, and I like the > idea of the omit-empty option in general). > > -Peff
Hi Junio, On Thu, 30 Mar 2023 at 17:54, Junio C Hamano <gitster@pobox.com> wrote: > While I can see the utility of the new feature, it is unclear if its > merit is so clear that it deserves a short-and-sweet single letter > option from the get go. Especially, don't we want to give this to > "git branch" and "git tag" in their listing modes for consistency, > but it means stealing "-n" also from them. I had this in mind, but I wanted to add a short option because "--omit-empty-lines" is already longer than "| sed '/^$/d' |" :D Which is the workaround I've used in the past. I see that later in your reply you write "--omit-empty". (Perhaps instinctively?) The "line" part is already implied so I'd be equally happy with "--omit-empty". I realize that the parser already allows this implicitly, but the full name of the option should be spelt out in the docs, I assume. > Move that opening brace at the end of the previous line, i.e. > > - if (list) > + if (list) { Of course, my bad. Old habits and so on. But I may not need to change this at all in the first place because... > > + if (omit_empty_lines && !format.format) { > > + error("--omit-empty-lines without --format does not make sense"); > > + usage_with_options(builtin_branch_usage, options); > > + } > > Does it not make sense? With the default format, it may happen that > there will be no empty line so there is nothing to omit, but I do > not see a strong reason to forbid the request like this. ... it's perfectly fine by me to allow --omit-empty when the user has not specified their own format. I added this merely as guidance for the user. For example, Git will bail out with a similar message if the user tries to unshallow a repository that is already complete, which I assume is technically not a problem. > > setup_auto_pager("branch", 1); > > + } > > > > if (delete) { > > if (!argc) > > diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c > > index 6f62f40d12..349c4d4ef8 100644 > > --- a/builtin/for-each-ref.c > > +++ b/builtin/for-each-ref.c > > @@ -19,7 +19,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix) > > int i; > > struct ref_sorting *sorting; > > struct string_list sorting_options = STRING_LIST_INIT_DUP; > > - int maxcount = 0, icase = 0; > > + int maxcount = 0, icase = 0, omit_empty_lines = 0; > > struct ref_array array; > > struct ref_filter filter; > > struct ref_format format = REF_FORMAT_INIT; > > @@ -35,6 +35,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix) > > N_("quote placeholders suitably for python"), QUOTE_PYTHON), > > OPT_BIT(0 , "tcl", &format.quote_style, > > N_("quote placeholders suitably for Tcl"), QUOTE_TCL), > > + OPT_BOOL('n' , "omit-empty-lines", &omit_empty_lines, > > + N_("do not output a newline after empty formatted refs")), > > > > OPT_GROUP(""), > > OPT_INTEGER( 0 , "count", &maxcount, N_("show only <n> matched refs")), > > @@ -55,8 +57,6 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix) > > memset(&array, 0, sizeof(array)); > > memset(&filter, 0, sizeof(filter)); > > > > - format.format = "%(objectname) %(objecttype)\t%(refname)"; > > - > > git_config(git_default_config, NULL); > > parse_options(argc, argv, prefix, opts, for_each_ref_usage, 0); > > This smells fishy. We establish the hardcoded built-in default, let > the config machinery override, and then finally let command line > options to further override. You may be able to reach the same end > result by leaving the value unset, fill with the configured value, > override with the command line, and then if the value is still > unset, fall back to a hardcoded built-in default, but I do not see > why such a change logically belongs to a patch to add "--omit-empty" > feature. This is what I did. I moved the assignment of the default value of format.format to after parse_options() so that I could use its (lack of) value to determine whether --format had been specified by the user, instead of e.g. "int format_was_given = 0;". But then I of course have to check whether parse_options() already has assigned it before maybe assigning the default value. But I only did all of that to be able to die with the error. If we just allow --omit-empty then all of this can be left alone, making the code, and the patch, simpler. > > @@ -68,6 +68,12 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix) > > error("more than one quoting style?"); > > usage_with_options(for_each_ref_usage, opts); > > } > > + if (omit_empty_lines && !format.format) { > > + error("--omit-empty-lines without --format does not make sense"); > > + usage_with_options(for_each_ref_usage, opts); > > + } > > I wouldn't do this, for the same reason as for "git branch". > > > + if (!format.format) > > + format.format = "%(objectname) %(objecttype)\t%(refname)"; > > This is the other half of the earlier change I called "fishy". It > may be benign, but it is distracting, especially when done without > explanation, in a change to add a feature that is not related. It *was* related, because of the error I wanted to provide. But maybe it isn't anymore :P > * do not compare output.len with 0; it is sufficient to just write > > if (!omit_empty || output.len) > Sure, will change. Peff addressed your second point already. But perhaps move fwrite() (or whatever other printing function) inside the if() too? > It is hard to see that there is an empty line expected when the > expectation is prepared like this. Why not something like > > cat >expect.full <<-\EOF && > one > two > > four (three is missing) > EOF > sed -e "/^$/d" expect.full >expect.stripped && > > git branch $args >actual && > test_cmp expect.full actual && > > git branch --omit-empty $args >actual && > test_cmp expect.stripped actual && > > that highlights the fact that there is a missing line for one > expectation, and that the only difference in two expectations is the > lack of empty line(s)? I wholeheartedly agree. I only wrote it this way because actually having an empty line there lead to a whitespace error in the patch. But that was because I mistakenly assumed that empty lines in an indented heredoc also had to be prefixed by a TAB and I didn't investigate. Will fix. > > diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh > > index c466fd989f..eec9d45513 100755 > > --- a/t/t6300-for-each-ref.sh > > +++ b/t/t6300-for-each-ref.sh > > @@ -1374,6 +1374,14 @@ test_expect_success 'for-each-ref --ignore-case ignores case' ' > > test_cmp expect actual > > ' > > > > +test_expect_success 'for-each-ref --omit-empty-lines works' ' > > + git for-each-ref --format="%(refname)" > actual && > > + test_line_count -gt 1 actual && > > The next test depends on that a branch 'main' exists, so perhaps > that should be tested here, at least? And then if there is no other > branches and tags, we cannot tell if seeing only the 'main' branch > is due to --omit-empty correctly working, or due to the repository > having only that branch, so it is also good to check if there is > branches or tags other than 'main' in the output. In general I find it very hard to write meaningful tests because of stuff like this this. In my (admittedly very limited) experience there are usually big dependencies on prior tests in a particular test case, and I just assumed that was okay. I often find it hard to discover what the state of the repository is at the point in the test script where I want to add a test, or modify an existing one. In this particular case I know from the test case right above that main exists. What is not immediately obvious is that at least one other ref exists. But if that ever changes then at least 'test_line_count -gt 1 actual' will fail. > By the way, lose SP between redirection operator '>' and its target, > i.e. write them like so: > > echo refs/heads/main >expect Will fix. > This feature makes %(if)...%(else)...%(end) construct complete and > is a very good addition. > > Thanks for working on it. > Thanks for the review! Øsse
Øystein Walle <oystwa@gmail.com> writes: >> > + if (omit_empty_lines && !format.format) { >> > + error("--omit-empty-lines without --format does not make sense"); >> > + usage_with_options(builtin_branch_usage, options); >> > + } >> >> Does it not make sense? With the default format, it may happen that >> there will be no empty line so there is nothing to omit, but I do >> not see a strong reason to forbid the request like this. > > ... it's perfectly fine by me to allow --omit-empty when the user has > not specified their own format. I added this merely as guidance for the > user. For example, Git will bail out with a similar message if the user > tries to unshallow a repository that is already complete, which I assume > is technically not a problem. It is not just technically a problem but from the end user's point of view a misguided message. If the end result is in the shape of desired state after the command completes, there shouldn't be an error() to stop the user. Informational "the history is now fully complete---by the way, it was already so before I started working" may be OK. It probably should be fixed, instead of being modelled after to spread the mistake to new features, like this patch does. Thanks.
Hi Øystein On 30/03/2023 12:21, Øystein Walle wrote: > If the given format string expands to the empty string a newline is > still printed it. This makes using the output linewise more tedious. For > example, git update-ref --stdin does not accept empty lines. > > Add options to branch and for-each-ref to not print these empty lines. > The default behavior remains the same. Do the empty lines in the output serve any useful purpose? If not then it might be better just to suppress them unconditionally rather than adding a new command line option. Best Wishes Phillip > Signed-off-by: Øystein Walle <oystwa@gmail.com> > --- > > The logic is more or less duplicated in branch.c and for-each-ref.c > which I don't like. However I couldn't really find a "central" place to > put it. Imo. it's definitely not a property of the format or the filter, > so struct ref_format and struct ref_filter are no good. > > I also started working on a patch to make update-ref --stdin accept > empty lines. But that seems to be a much more deliberate decision, with > tests to verify it and all. So I stopped pursuing that. > > Documentation/git-branch.txt | 5 +++++ > Documentation/git-for-each-ref.txt | 5 +++++ > builtin/branch.c | 12 +++++++++++- > builtin/for-each-ref.c | 15 +++++++++++---- > t/t3203-branch-output.sh | 26 ++++++++++++++++++++++++++ > t/t6300-for-each-ref.sh | 8 ++++++++ > 6 files changed, 66 insertions(+), 5 deletions(-) > > diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt > index d382ac69f7..4d53133ce3 100644 > --- a/Documentation/git-branch.txt > +++ b/Documentation/git-branch.txt > @@ -156,6 +156,11 @@ in another worktree linked to the same repository. > --ignore-case:: > Sorting and filtering branches are case insensitive. > > +-n:: > +--omit-empty-lines:: > + Do not print a newline after formatted refs where the format expands > + to the empty string. > + > --column[=<options>]:: > --no-column:: > Display branch listing in columns. See configuration variable > diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt > index 6da899c629..0f4fa98b18 100644 > --- a/Documentation/git-for-each-ref.txt > +++ b/Documentation/git-for-each-ref.txt > @@ -93,6 +93,11 @@ OPTIONS > --ignore-case:: > Sorting and filtering refs are case insensitive. > > +-n:: > +--omit-empty-lines:: > + Do not print a newline after formatted refs where the format expands > + to the empty string. > + > FIELD NAMES > ----------- > > diff --git a/builtin/branch.c b/builtin/branch.c > index f63fd45edb..1bbb36b442 100644 > --- a/builtin/branch.c > +++ b/builtin/branch.c > @@ -41,6 +41,7 @@ static const char *head; > static struct object_id head_oid; > static int recurse_submodules = 0; > static int submodule_propagate_branches = 0; > +static int omit_empty_lines = 0; > > static int branch_use_color = -1; > static char branch_colors[][COLOR_MAXLEN] = { > @@ -461,7 +462,8 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin > string_list_append(output, out.buf); > } else { > fwrite(out.buf, 1, out.len, stdout); > - putchar('\n'); > + if (!omit_empty_lines || out.len > 0) > + putchar('\n'); > } > } > > @@ -670,6 +672,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix) > OPT_BIT('D', NULL, &delete, N_("delete branch (even if not merged)"), 2), > OPT_BIT('m', "move", &rename, N_("move/rename a branch and its reflog"), 1), > OPT_BIT('M', NULL, &rename, N_("move/rename a branch, even if target exists"), 2), > + OPT_BOOL('n' , "omit-empty-lines", &omit_empty_lines, > + N_("do not output a newline after empty formatted refs")), > OPT_BIT('c', "copy", ©, N_("copy a branch and its reflog"), 1), > OPT_BIT('C', NULL, ©, N_("copy a branch, even if target exists"), 2), > OPT_BOOL('l', "list", &list, N_("list branch names")), > @@ -757,7 +761,13 @@ int cmd_branch(int argc, const char **argv, const char *prefix) > } > > if (list) > + { > + if (omit_empty_lines && !format.format) { > + error("--omit-empty-lines without --format does not make sense"); > + usage_with_options(builtin_branch_usage, options); > + } > setup_auto_pager("branch", 1); > + } > > if (delete) { > if (!argc) > diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c > index 6f62f40d12..349c4d4ef8 100644 > --- a/builtin/for-each-ref.c > +++ b/builtin/for-each-ref.c > @@ -19,7 +19,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix) > int i; > struct ref_sorting *sorting; > struct string_list sorting_options = STRING_LIST_INIT_DUP; > - int maxcount = 0, icase = 0; > + int maxcount = 0, icase = 0, omit_empty_lines = 0; > struct ref_array array; > struct ref_filter filter; > struct ref_format format = REF_FORMAT_INIT; > @@ -35,6 +35,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix) > N_("quote placeholders suitably for python"), QUOTE_PYTHON), > OPT_BIT(0 , "tcl", &format.quote_style, > N_("quote placeholders suitably for Tcl"), QUOTE_TCL), > + OPT_BOOL('n' , "omit-empty-lines", &omit_empty_lines, > + N_("do not output a newline after empty formatted refs")), > > OPT_GROUP(""), > OPT_INTEGER( 0 , "count", &maxcount, N_("show only <n> matched refs")), > @@ -55,8 +57,6 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix) > memset(&array, 0, sizeof(array)); > memset(&filter, 0, sizeof(filter)); > > - format.format = "%(objectname) %(objecttype)\t%(refname)"; > - > git_config(git_default_config, NULL); > > parse_options(argc, argv, prefix, opts, for_each_ref_usage, 0); > @@ -68,6 +68,12 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix) > error("more than one quoting style?"); > usage_with_options(for_each_ref_usage, opts); > } > + if (omit_empty_lines && !format.format) { > + error("--omit-empty-lines without --format does not make sense"); > + usage_with_options(for_each_ref_usage, opts); > + } > + if (!format.format) > + format.format = "%(objectname) %(objecttype)\t%(refname)"; > if (verify_ref_format(&format)) > usage_with_options(for_each_ref_usage, opts); > > @@ -88,7 +94,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix) > if (format_ref_array_item(array.items[i], &format, &output, &err)) > die("%s", err.buf); > fwrite(output.buf, 1, output.len, stdout); > - putchar('\n'); > + if (!omit_empty_lines || output.len > 0) > + putchar('\n'); > } > > strbuf_release(&err); > diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh > index d34d77f893..26bf0819ea 100755 > --- a/t/t3203-branch-output.sh > +++ b/t/t3203-branch-output.sh > @@ -341,6 +341,32 @@ test_expect_success 'git branch with --format=%(rest) must fail' ' > test_must_fail git branch --format="%(rest)" >actual > ' > > +test_expect_success 'git branch --format --omit-empty-lines' ' > + cat >expect <<-\EOF && > + Refname is (HEAD detached from fromtag) > + Refname is refs/heads/ambiguous > + Refname is refs/heads/branch-one > + Refname is refs/heads/branch-two > + EOF > + echo >>expect && > + cat >>expect <<-\EOF && > + Refname is refs/heads/ref-to-branch > + Refname is refs/heads/ref-to-remote > + EOF > + git branch --format="%(if:notequals=refs/heads/main)%(refname)%(then)Refname is %(refname)%(end)" >actual && > + test_cmp expect actual && > + cat >expect <<-\EOF && > + Refname is (HEAD detached from fromtag) > + Refname is refs/heads/ambiguous > + Refname is refs/heads/branch-one > + Refname is refs/heads/branch-two > + Refname is refs/heads/ref-to-branch > + Refname is refs/heads/ref-to-remote > + EOF > + git branch --omit-empty-lines --format="%(if:notequals=refs/heads/main)%(refname)%(then)Refname is %(refname)%(end)" >actual && > + test_cmp expect actual > +' > + > test_expect_success 'worktree colors correct' ' > cat >expect <<-EOF && > * <GREEN>(HEAD detached from fromtag)<RESET> > diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh > index c466fd989f..eec9d45513 100755 > --- a/t/t6300-for-each-ref.sh > +++ b/t/t6300-for-each-ref.sh > @@ -1374,6 +1374,14 @@ test_expect_success 'for-each-ref --ignore-case ignores case' ' > test_cmp expect actual > ' > > +test_expect_success 'for-each-ref --omit-empty-lines works' ' > + git for-each-ref --format="%(refname)" > actual && > + test_line_count -gt 1 actual && > + git for-each-ref --format="%(if:equals=refs/heads/main)%(refname)%(then)%(refname)%(end)" --omit-empty-lines > actual && > + echo refs/heads/main > expect && > + test_cmp expect actual > +' > + > test_expect_success 'for-each-ref --ignore-case works on multiple sort keys' ' > # name refs numerically to avoid case-insensitive filesystem conflicts > nr=0 &&
Phillip Wood <phillip.wood123@gmail.com> writes: > Do the empty lines in the output serve any useful purpose? If not then > it might be better just to suppress them unconditionally rather than > adding a new command line option. It's a nice egg of columbus. It however theoretically can break an existing use case where the user correlates the output with a list of refs they externally prepared (e.g. "for-each-ref --format... a b c" shows "A", "", and "C", and the user knows "b" produced ""). I do not know how likely such users complain, though, and if there is nobody who relies on the current behaviour, surely "unconditionally omit" is a very tempting approach to take. Thanks.
On Fri, 31 Mar 2023 at 19:17, Junio C Hamano <gitster@pobox.com> wrote: > > Phillip Wood <phillip.wood123@gmail.com> writes: > > > Do the empty lines in the output serve any useful purpose? If not then > > it might be better just to suppress them unconditionally rather than > > adding a new command line option. > > It's a nice egg of columbus. > > It however theoretically can break an existing use case where the > user correlates the output with a list of refs they externally > prepared (e.g. "for-each-ref --format... a b c" shows "A", "", and > "C", and the user knows "b" produced ""). I do not know how likely > such users complain, though, and if there is nobody who relies on > the current behaviour, surely "unconditionally omit" is a very > tempting approach to take. > > Thanks. I actually instinctively expected for-each-ref to suppress empty lines, at least by default. I don't see a good reason for them, except for something along the lines of what you said. We can of course make it a config option along with the flag, then after some time flip the default, and perhaps ultimately remove the config option again. Perhaps in a v3 if there is enough interest; will send a v2 shortly. But I must admit I am not very motivated to follow that up down the line. Øsse
On Thu, Apr 06, 2023 at 06:55:52PM +0200, Øystein Walle wrote: > > It however theoretically can break an existing use case where the > > user correlates the output with a list of refs they externally > > prepared (e.g. "for-each-ref --format... a b c" shows "A", "", and > > "C", and the user knows "b" produced ""). I do not know how likely > > such users complain, though, and if there is nobody who relies on > > the current behaviour, surely "unconditionally omit" is a very > > tempting approach to take. > > > > Thanks. > > I actually instinctively expected for-each-ref to suppress empty lines, at > least by default. I don't see a good reason for them, except for something > along the lines of what you said. > > We can of course make it a config option along with the flag, then after some > time flip the default, and perhaps ultimately remove the config option again. > Perhaps in a v3 if there is enough interest; will send a v2 shortly. But I > must admit I am not very motivated to follow that up down the line. It might be enough to flip the default unconditionally (no config), but I think we may still want "--no-omit-empty-lines" as an escape hatch. I dunno. Maybe that is somehow choosing the worst of both worlds. -Peff
Øystein Walle <oystwa@gmail.com> writes: >> It however theoretically can break an existing use case where the >> user correlates the output with a list of refs they externally >> prepared (e.g. "for-each-ref --format... a b c" shows "A", "", and >> "C", and the user knows "b" produced ""). I do not know how likely >> such users complain, though, and if there is nobody who relies on >> the current behaviour, surely "unconditionally omit" is a very >> tempting approach to take. >> >> Thanks. > > I actually instinctively expected for-each-ref to suppress empty lines, at > least by default. I don't see a good reason for them, except for something > along the lines of what you said. That makes two of us ;-) > We can of course make it a config option along with the flag, then after some > time flip the default, and perhaps ultimately remove the config option again. Yeah, but this v2 is not starting with purely a new feature without breaking anybody, so we can stop here for now, and once there is enough interest to go through the deprecation dance, we can do that as a separate series later. Thanks.
Jeff King <peff@peff.net> writes: > It might be enough to flip the default unconditionally (no config), but > I think we may still want "--no-omit-empty-lines" as an escape hatch. I > dunno. Maybe that is somehow choosing the worst of both worlds. It is very tempting, indeed. We can add the escape hatch and flip the default, and only when somebody complains, come back and say "oh, sorry, we didn't know anybody used it" and flip the default back, perhaps? This is a totally unrelated tangent, but it is a bit unfortunate that with our parse-options API, it is not trivial to - mark that "--keep-empty-lines" and "--omit-empty-lines" toggle the same underlying Boolean variable, - accept "--no-keep" and "--no-omit" as obvious synonyms for "--omit" and "--keep", - have "git foo -h" listing to show "--keep" and "--omit" together, - omit these "--no-foo" variants from "git foo -h" listing. by the way.
On Thu, Apr 06, 2023 at 11:20:03AM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > It might be enough to flip the default unconditionally (no config), but > > I think we may still want "--no-omit-empty-lines" as an escape hatch. I > > dunno. Maybe that is somehow choosing the worst of both worlds. > > It is very tempting, indeed. We can add the escape hatch and flip > the default, and only when somebody complains, come back and say > "oh, sorry, we didn't know anybody used it" and flip the default > back, perhaps? I don't think flipping back after such an incident is a good idea, as it just creates more confusion. But if the option exists, then at least you can say "oh, sorry; you can still do what you want by passing this option", rather than "oh, sorry; there's no way to get what you want". But either way, the first step before flipping any defaults is adding an option, which is what this patch is doing, so I am all for it. :) > This is a totally unrelated tangent, but it is a bit unfortunate > that with our parse-options API, it is not trivial to > > - mark that "--keep-empty-lines" and "--omit-empty-lines" toggle > the same underlying Boolean variable, > > - accept "--no-keep" and "--no-omit" as obvious synonyms for > "--omit" and "--keep", > > - have "git foo -h" listing to show "--keep" and "--omit" together, > > - omit these "--no-foo" variants from "git foo -h" listing. > > by the way. Yeah, "--no-" is special in our parser in a way that "--keep" and "--omit" aren't. It might be possible to make this pattern easier to support. OTOH, perhaps it is a sign that we are straying too far from existing patterns. It is not just parse-options.c, but also users themselves, who benefit from consistency. -Peff
diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt index d382ac69f7..4d53133ce3 100644 --- a/Documentation/git-branch.txt +++ b/Documentation/git-branch.txt @@ -156,6 +156,11 @@ in another worktree linked to the same repository. --ignore-case:: Sorting and filtering branches are case insensitive. +-n:: +--omit-empty-lines:: + Do not print a newline after formatted refs where the format expands + to the empty string. + --column[=<options>]:: --no-column:: Display branch listing in columns. See configuration variable diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index 6da899c629..0f4fa98b18 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -93,6 +93,11 @@ OPTIONS --ignore-case:: Sorting and filtering refs are case insensitive. +-n:: +--omit-empty-lines:: + Do not print a newline after formatted refs where the format expands + to the empty string. + FIELD NAMES ----------- diff --git a/builtin/branch.c b/builtin/branch.c index f63fd45edb..1bbb36b442 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -41,6 +41,7 @@ static const char *head; static struct object_id head_oid; static int recurse_submodules = 0; static int submodule_propagate_branches = 0; +static int omit_empty_lines = 0; static int branch_use_color = -1; static char branch_colors[][COLOR_MAXLEN] = { @@ -461,7 +462,8 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin string_list_append(output, out.buf); } else { fwrite(out.buf, 1, out.len, stdout); - putchar('\n'); + if (!omit_empty_lines || out.len > 0) + putchar('\n'); } } @@ -670,6 +672,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix) OPT_BIT('D', NULL, &delete, N_("delete branch (even if not merged)"), 2), OPT_BIT('m', "move", &rename, N_("move/rename a branch and its reflog"), 1), OPT_BIT('M', NULL, &rename, N_("move/rename a branch, even if target exists"), 2), + OPT_BOOL('n' , "omit-empty-lines", &omit_empty_lines, + N_("do not output a newline after empty formatted refs")), OPT_BIT('c', "copy", ©, N_("copy a branch and its reflog"), 1), OPT_BIT('C', NULL, ©, N_("copy a branch, even if target exists"), 2), OPT_BOOL('l', "list", &list, N_("list branch names")), @@ -757,7 +761,13 @@ int cmd_branch(int argc, const char **argv, const char *prefix) } if (list) + { + if (omit_empty_lines && !format.format) { + error("--omit-empty-lines without --format does not make sense"); + usage_with_options(builtin_branch_usage, options); + } setup_auto_pager("branch", 1); + } if (delete) { if (!argc) diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c index 6f62f40d12..349c4d4ef8 100644 --- a/builtin/for-each-ref.c +++ b/builtin/for-each-ref.c @@ -19,7 +19,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix) int i; struct ref_sorting *sorting; struct string_list sorting_options = STRING_LIST_INIT_DUP; - int maxcount = 0, icase = 0; + int maxcount = 0, icase = 0, omit_empty_lines = 0; struct ref_array array; struct ref_filter filter; struct ref_format format = REF_FORMAT_INIT; @@ -35,6 +35,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix) N_("quote placeholders suitably for python"), QUOTE_PYTHON), OPT_BIT(0 , "tcl", &format.quote_style, N_("quote placeholders suitably for Tcl"), QUOTE_TCL), + OPT_BOOL('n' , "omit-empty-lines", &omit_empty_lines, + N_("do not output a newline after empty formatted refs")), OPT_GROUP(""), OPT_INTEGER( 0 , "count", &maxcount, N_("show only <n> matched refs")), @@ -55,8 +57,6 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix) memset(&array, 0, sizeof(array)); memset(&filter, 0, sizeof(filter)); - format.format = "%(objectname) %(objecttype)\t%(refname)"; - git_config(git_default_config, NULL); parse_options(argc, argv, prefix, opts, for_each_ref_usage, 0); @@ -68,6 +68,12 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix) error("more than one quoting style?"); usage_with_options(for_each_ref_usage, opts); } + if (omit_empty_lines && !format.format) { + error("--omit-empty-lines without --format does not make sense"); + usage_with_options(for_each_ref_usage, opts); + } + if (!format.format) + format.format = "%(objectname) %(objecttype)\t%(refname)"; if (verify_ref_format(&format)) usage_with_options(for_each_ref_usage, opts); @@ -88,7 +94,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix) if (format_ref_array_item(array.items[i], &format, &output, &err)) die("%s", err.buf); fwrite(output.buf, 1, output.len, stdout); - putchar('\n'); + if (!omit_empty_lines || output.len > 0) + putchar('\n'); } strbuf_release(&err); diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh index d34d77f893..26bf0819ea 100755 --- a/t/t3203-branch-output.sh +++ b/t/t3203-branch-output.sh @@ -341,6 +341,32 @@ test_expect_success 'git branch with --format=%(rest) must fail' ' test_must_fail git branch --format="%(rest)" >actual ' +test_expect_success 'git branch --format --omit-empty-lines' ' + cat >expect <<-\EOF && + Refname is (HEAD detached from fromtag) + Refname is refs/heads/ambiguous + Refname is refs/heads/branch-one + Refname is refs/heads/branch-two + EOF + echo >>expect && + cat >>expect <<-\EOF && + Refname is refs/heads/ref-to-branch + Refname is refs/heads/ref-to-remote + EOF + git branch --format="%(if:notequals=refs/heads/main)%(refname)%(then)Refname is %(refname)%(end)" >actual && + test_cmp expect actual && + cat >expect <<-\EOF && + Refname is (HEAD detached from fromtag) + Refname is refs/heads/ambiguous + Refname is refs/heads/branch-one + Refname is refs/heads/branch-two + Refname is refs/heads/ref-to-branch + Refname is refs/heads/ref-to-remote + EOF + git branch --omit-empty-lines --format="%(if:notequals=refs/heads/main)%(refname)%(then)Refname is %(refname)%(end)" >actual && + test_cmp expect actual +' + test_expect_success 'worktree colors correct' ' cat >expect <<-EOF && * <GREEN>(HEAD detached from fromtag)<RESET> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index c466fd989f..eec9d45513 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -1374,6 +1374,14 @@ test_expect_success 'for-each-ref --ignore-case ignores case' ' test_cmp expect actual ' +test_expect_success 'for-each-ref --omit-empty-lines works' ' + git for-each-ref --format="%(refname)" > actual && + test_line_count -gt 1 actual && + git for-each-ref --format="%(if:equals=refs/heads/main)%(refname)%(then)%(refname)%(end)" --omit-empty-lines > actual && + echo refs/heads/main > expect && + test_cmp expect actual +' + test_expect_success 'for-each-ref --ignore-case works on multiple sort keys' ' # name refs numerically to avoid case-insensitive filesystem conflicts nr=0 &&
If the given format string expands to the empty string a newline is still printed it. This makes using the output linewise more tedious. For example, git update-ref --stdin does not accept empty lines. Add options to branch and for-each-ref to not print these empty lines. The default behavior remains the same. Signed-off-by: Øystein Walle <oystwa@gmail.com> --- The logic is more or less duplicated in branch.c and for-each-ref.c which I don't like. However I couldn't really find a "central" place to put it. Imo. it's definitely not a property of the format or the filter, so struct ref_format and struct ref_filter are no good. I also started working on a patch to make update-ref --stdin accept empty lines. But that seems to be a much more deliberate decision, with tests to verify it and all. So I stopped pursuing that. Documentation/git-branch.txt | 5 +++++ Documentation/git-for-each-ref.txt | 5 +++++ builtin/branch.c | 12 +++++++++++- builtin/for-each-ref.c | 15 +++++++++++---- t/t3203-branch-output.sh | 26 ++++++++++++++++++++++++++ t/t6300-for-each-ref.sh | 8 ++++++++ 6 files changed, 66 insertions(+), 5 deletions(-)