* [PATCH 0/2] branch, for-each-ref: add option to omit empty lines @ 2023-03-30 11:21 Øystein Walle 2023-03-30 11:21 ` [PATCH 1/2] ref-filter: remove unused ref_format member Øystein Walle 2023-03-30 11:21 ` [PATCH 2/2] branch, for-each-ref: " Øystein Walle 0 siblings, 2 replies; 30+ messages in thread From: Øystein Walle @ 2023-03-30 11:21 UTC (permalink / raw) To: git; +Cc: Øystein Walle These two patches are independent of eachother. One is just a very small cleanup that I discovered while working on the other. Some thoughts after the --- line in each patch. Øystein Walle (2): ref-filter: remove unused ref_format member branch, for-each-ref: add option to omit empty lines Documentation/git-branch.txt | 5 +++++ Documentation/git-for-each-ref.txt | 5 +++++ ref-filter.h | 1 - builtin/branch.c | 12 +++++++++++- builtin/for-each-ref.c | 15 +++++++++++---- ref-filter.c | 1 - t/t3203-branch-output.sh | 26 ++++++++++++++++++++++++++ t/t6300-for-each-ref.sh | 8 ++++++++ 8 files changed, 66 insertions(+), 7 deletions(-) -- 2.34.1 ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 1/2] ref-filter: remove unused ref_format member 2023-03-30 11:21 [PATCH 0/2] branch, for-each-ref: add option to omit empty lines Øystein Walle @ 2023-03-30 11:21 ` Øystein Walle 2023-03-30 15:21 ` Junio C Hamano 2023-03-30 11:21 ` [PATCH 2/2] branch, for-each-ref: " Øystein Walle 1 sibling, 1 reply; 30+ messages in thread From: Øystein Walle @ 2023-03-30 11:21 UTC (permalink / raw) To: git; +Cc: Øystein Walle use_rest was added in b9dee075eb (ref-filter: add %(rest) atom, 2021-07-26) but was never used. As far as I can tell it was used in a later patch that was submitted to the mailing list but never applied. Signed-off-by: Øystein Walle <oystwa@gmail.com> --- Would be nice to have a link to the email thread here, but I don't know how. ref-filter.h | 1 - ref-filter.c | 1 - 2 files changed, 2 deletions(-) diff --git a/ref-filter.h b/ref-filter.h index aa0eea4ecf..0f4183233a 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -75,7 +75,6 @@ struct ref_format { const char *format; const char *rest; int quote_style; - int use_rest; int use_color; /* Internal state to ref-filter */ diff --git a/ref-filter.c b/ref-filter.c index ed802778da..20e0a72f24 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -596,7 +596,6 @@ static int rest_atom_parser(struct ref_format *format, { if (arg) return err_no_arg(err, "rest"); - format->use_rest = 1; return 0; } -- 2.34.1 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] ref-filter: remove unused ref_format member 2023-03-30 11:21 ` [PATCH 1/2] ref-filter: remove unused ref_format member Øystein Walle @ 2023-03-30 15:21 ` Junio C Hamano 2023-03-30 15:25 ` Junio C Hamano 0 siblings, 1 reply; 30+ messages in thread From: Junio C Hamano @ 2023-03-30 15:21 UTC (permalink / raw) To: Øystein Walle; +Cc: git Øystein Walle <oystwa@gmail.com> writes: > use_rest was added in b9dee075eb (ref-filter: add %(rest) atom, > 2021-07-26) but was never used. As far as I can tell it was used in a > later patch that was submitted to the mailing list but never applied. > > Signed-off-by: Øystein Walle <oystwa@gmail.com> > --- > Would be nice to have a link to the email thread here, but I don't know > how. Here is a link to the patch that led to that commit you cited: https://lore.kernel.org/git/207cc5129649e767036d8467ea7c884c3f664cc7.1627270010.git.gitgitgadget@gmail.com/ It indeed is cumbersome to add, as the Message-Ids for patches from GitGitGadget tend to be ultra long. But b9dee075eb was the last one in the 5-patch series; I do not see any "later patch there in the thread. > ref-filter.h | 1 - > ref-filter.c | 1 - > 2 files changed, 2 deletions(-) > > diff --git a/ref-filter.h b/ref-filter.h > index aa0eea4ecf..0f4183233a 100644 > --- a/ref-filter.h > +++ b/ref-filter.h > @@ -75,7 +75,6 @@ struct ref_format { > const char *format; > const char *rest; > int quote_style; > - int use_rest; > int use_color; > > /* Internal state to ref-filter */ > diff --git a/ref-filter.c b/ref-filter.c > index ed802778da..20e0a72f24 100644 > --- a/ref-filter.c > +++ b/ref-filter.c > @@ -596,7 +596,6 @@ static int rest_atom_parser(struct ref_format *format, > { > if (arg) > return err_no_arg(err, "rest"); > - format->use_rest = 1; > return 0; > } ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] ref-filter: remove unused ref_format member 2023-03-30 15:21 ` Junio C Hamano @ 2023-03-30 15:25 ` Junio C Hamano 2023-03-31 10:37 ` Øystein Walle 0 siblings, 1 reply; 30+ messages in thread From: Junio C Hamano @ 2023-03-30 15:25 UTC (permalink / raw) To: Øystein Walle; +Cc: git Junio C Hamano <gitster@pobox.com> writes: > Øystein Walle <oystwa@gmail.com> writes: > >> use_rest was added in b9dee075eb (ref-filter: add %(rest) atom, >> 2021-07-26) but was never used. As far as I can tell it was used in a >> later patch that was submitted to the mailing list but never applied. >> >> Signed-off-by: Øystein Walle <oystwa@gmail.com> >> --- >> Would be nice to have a link to the email thread here, but I don't know >> how. > > > Here is a link to the patch that led to that commit you cited: > > https://lore.kernel.org/git/207cc5129649e767036d8467ea7c884c3f664cc7.1627270010.git.gitgitgadget@gmail.com/ > > It indeed is cumbersome to add, as the Message-Ids for patches from > GitGitGadget tend to be ultra long. > > But b9dee075eb was the last one in the 5-patch series; I do > not see any "later patch there in the thread. I think there was a follow-up RFC series that was written to use the value of the member, cf. https://lore.kernel.org/git/9c5fddf6885875ccd3ce3f047bb938c77d9bbca2.1628842990.git.gitgitgadget@gmail.com/ but it seems there was no review on the series. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] ref-filter: remove unused ref_format member 2023-03-30 15:25 ` Junio C Hamano @ 2023-03-31 10:37 ` Øystein Walle 2023-03-31 10:57 ` ZheNing Hu 2023-03-31 16:19 ` Junio C Hamano 0 siblings, 2 replies; 30+ messages in thread From: Øystein Walle @ 2023-03-31 10:37 UTC (permalink / raw) To: gitster; +Cc: git, oystwa On Thu, 30 Mar 2023 at 17:25, Junio C Hamano <gitster@pobox.com> wrote: > > Junio C Hamano <gitster@pobox.com> writes: > > > Øystein Walle <oystwa@gmail.com> writes: > > > >> use_rest was added in b9dee075eb (ref-filter: add %(rest) atom, > >> 2021-07-26) but was never used. As far as I can tell it was used in a > >> later patch that was submitted to the mailing list but never applied. > >> > >> Signed-off-by: Øystein Walle <oystwa@gmail.com> > >> --- > >> Would be nice to have a link to the email thread here, but I don't know > >> how. > > > > > > Here is a link to the patch that led to that commit you cited: > > > > https://lore.kernel.org/git/207cc5129649e767036d8467ea7c884c3f664cc7.1627270010.git.gitgitgadget@gmail.com/ > > > > It indeed is cumbersome to add, as the Message-Ids for patches from > > GitGitGadget tend to be ultra long. > > > > But b9dee075eb was the last one in the 5-patch series; I do > > not see any "later patch there in the thread. > > I think there was a follow-up RFC series that was written to use the > value of the member, cf. > > https://lore.kernel.org/git/9c5fddf6885875ccd3ce3f047bb938c77d9bbca2.1628842990.git.gitgitgadget@gmail.com/ > > but it seems there was no review on the series. The follow-up series you link to seems to be a superset of the first series, which is what confused me. This is why I thought perhaps a subset of the latter series was accepted. But I see now that the dates match that of the first series, and you even applied it very soon after. Strange choice to include the first five patches in the follow-up series, then... Looked through the git.git log and see that it's not uncommon to reference patches from lore.kernel.org, so I can do the same. Perhaps in the "footnote style" to make it easier to digest. That is, if we want to apply this in the first place... It is a very minor cleanup of something that does no harm. On the other hand this particlar line of development seems abandoned. Øsse ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] ref-filter: remove unused ref_format member 2023-03-31 10:37 ` Øystein Walle @ 2023-03-31 10:57 ` ZheNing Hu 2023-03-31 16:19 ` Junio C Hamano 1 sibling, 0 replies; 30+ messages in thread From: ZheNing Hu @ 2023-03-31 10:57 UTC (permalink / raw) To: Øystein Walle; +Cc: gitster, git Øystein Walle <oystwa@gmail.com> 于2023年3月31日周五 18:39写道: > > On Thu, 30 Mar 2023 at 17:25, Junio C Hamano <gitster@pobox.com> wrote: > > > > Junio C Hamano <gitster@pobox.com> writes: > > > > > Øystein Walle <oystwa@gmail.com> writes: > > > > > >> use_rest was added in b9dee075eb (ref-filter: add %(rest) atom, > > >> 2021-07-26) but was never used. As far as I can tell it was used in a > > >> later patch that was submitted to the mailing list but never applied. > > >> > > >> Signed-off-by: Øystein Walle <oystwa@gmail.com> > > >> --- > > >> Would be nice to have a link to the email thread here, but I don't know > > >> how. > > > > > > > > > Here is a link to the patch that led to that commit you cited: > > > > > > https://lore.kernel.org/git/207cc5129649e767036d8467ea7c884c3f664cc7.1627270010.git.gitgitgadget@gmail.com/ > > > > > > It indeed is cumbersome to add, as the Message-Ids for patches from > > > GitGitGadget tend to be ultra long. > > > > > > But b9dee075eb was the last one in the 5-patch series; I do > > > not see any "later patch there in the thread. > > > > I think there was a follow-up RFC series that was written to use the > > value of the member, cf. > > > > https://lore.kernel.org/git/9c5fddf6885875ccd3ce3f047bb938c77d9bbca2.1628842990.git.gitgitgadget@gmail.com/ > > > > but it seems there was no review on the series. > > The follow-up series you link to seems to be a superset of the first series, > which is what confused me. This is why I thought perhaps a subset of the latter > series was accepted. But I see now that the dates match that of the first > series, and you even applied it very soon after. Strange choice to include the > first five patches in the follow-up series, then... > > Looked through the git.git log and see that it's not uncommon to reference > patches from lore.kernel.org, so I can do the same. Perhaps in the "footnote > style" to make it easier to digest. That is, if we want to apply this in the > first place... It is a very minor cleanup of something that does no harm. On > the other hand this particlar line of development seems abandoned. > Yes. Originally, I hoped to make all the atoms of cat-file --format compatible with ref-filter, and then make cat-file --format able to use the interface of ref-filter But due to some performance issues, this route is now deprecated. This little %(rest) is no longer useful. > Øsse ZheNing ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] ref-filter: remove unused ref_format member 2023-03-31 10:37 ` Øystein Walle 2023-03-31 10:57 ` ZheNing Hu @ 2023-03-31 16:19 ` Junio C Hamano 2023-04-06 17:08 ` [PATCH v2 0/2] branch, for-each-ref: add option to omit empty lines Øystein Walle 1 sibling, 1 reply; 30+ messages in thread From: Junio C Hamano @ 2023-03-31 16:19 UTC (permalink / raw) To: Øystein Walle; +Cc: git Øystein Walle <oystwa@gmail.com> writes: > The follow-up series you link to seems to be a superset of the first series, > which is what confused me. This is why I thought perhaps a subset of the latter > series was accepted. But I see now that the dates match that of the first > series, and you even applied it very soon after. Strange choice to include the > first five patches in the follow-up series, then... It probably happened because even by then the previous round v4 was not in 'next' when the later iteration was prepared, and then the topic perhaps died at around the time GSoC of the year finished. As long as the earlier and less ambitious attempt turns out to give us a net positive benefit, these early steps may still advance through 'next' and down to a release. ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 0/2] branch, for-each-ref: add option to omit empty lines 2023-03-31 16:19 ` Junio C Hamano @ 2023-04-06 17:08 ` Øystein Walle 2023-04-06 17:08 ` [PATCH v2 1/2] ref-filter: remove unused ref_format member Øystein Walle ` (2 more replies) 0 siblings, 3 replies; 30+ messages in thread From: Øystein Walle @ 2023-04-06 17:08 UTC (permalink / raw) To: git; +Cc: gitster, phillip.wood123, peff, oystwa Changes since v1: - Improved commit message in 1/2, - Stopped considering flag combinations as an error, which greatly simplified the logic, - Improved weird test file generation, - Removed short option and renamed long option, - Changed conditions to check the string length before the flag, which imo. reads better. Øystein Walle (2): ref-filter: remove unused ref_format member branch, for-each-ref: add option to omit empty lines Documentation/git-branch.txt | 4 ++++ Documentation/git-for-each-ref.txt | 4 ++++ ref-filter.h | 1 - builtin/branch.c | 6 +++++- builtin/for-each-ref.c | 7 +++++-- ref-filter.c | 1 - t/t3203-branch-output.sh | 24 ++++++++++++++++++++++++ t/t6300-for-each-ref.sh | 8 ++++++++ 8 files changed, 50 insertions(+), 5 deletions(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 1/2] ref-filter: remove unused ref_format member 2023-04-06 17:08 ` [PATCH v2 0/2] branch, for-each-ref: add option to omit empty lines Øystein Walle @ 2023-04-06 17:08 ` Øystein Walle 2023-04-06 17:08 ` [PATCH v2 2/2] branch, for-each-ref: add option to omit empty lines Øystein Walle 2023-04-06 18:24 ` [PATCH v2 0/2] " Junio C Hamano 2 siblings, 0 replies; 30+ messages in thread From: Øystein Walle @ 2023-04-06 17:08 UTC (permalink / raw) To: git; +Cc: gitster, phillip.wood123, peff, oystwa use_rest was added in b9dee075eb (ref-filter: add %(rest) atom, 2021-07-26) but was never used. A follow-up patch series[1] that used this member was submitted, but ultimately the development was abandonded due to performance problems. [1]: https://lore.kernel.org/git/9c5fddf6885875ccd3ce3f047bb938c77d9bbca2.1628842990.git.gitgitgadget@gmail.com/ Signed-off-by: Øystein Walle <oystwa@gmail.com> --- ref-filter.h | 1 - ref-filter.c | 1 - 2 files changed, 2 deletions(-) diff --git a/ref-filter.h b/ref-filter.h index daa6d02017..e3eea5e3ad 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -75,7 +75,6 @@ struct ref_format { const char *format; const char *rest; int quote_style; - int use_rest; int use_color; /* Internal state to ref-filter */ diff --git a/ref-filter.c b/ref-filter.c index ed802778da..20e0a72f24 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -596,7 +596,6 @@ static int rest_atom_parser(struct ref_format *format, { if (arg) return err_no_arg(err, "rest"); - format->use_rest = 1; return 0; } -- 2.20.1 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v2 2/2] branch, for-each-ref: add option to omit empty lines 2023-04-06 17:08 ` [PATCH v2 0/2] branch, for-each-ref: add option to omit empty lines Øystein Walle 2023-04-06 17:08 ` [PATCH v2 1/2] ref-filter: remove unused ref_format member Øystein Walle @ 2023-04-06 17:08 ` Øystein Walle 2023-04-06 18:24 ` [PATCH v2 0/2] " Junio C Hamano 2 siblings, 0 replies; 30+ messages in thread From: Øystein Walle @ 2023-04-06 17:08 UTC (permalink / raw) To: git; +Cc: gitster, phillip.wood123, peff, oystwa 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> --- Documentation/git-branch.txt | 4 ++++ Documentation/git-for-each-ref.txt | 4 ++++ builtin/branch.c | 6 +++++- builtin/for-each-ref.c | 7 +++++-- t/t3203-branch-output.sh | 24 ++++++++++++++++++++++++ t/t6300-for-each-ref.sh | 8 ++++++++ 6 files changed, 50 insertions(+), 3 deletions(-) diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt index d382ac69f7..d207da9101 100644 --- a/Documentation/git-branch.txt +++ b/Documentation/git-branch.txt @@ -156,6 +156,10 @@ in another worktree linked to the same repository. --ignore-case:: Sorting and filtering branches are case insensitive. +--omit-empty:: + 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..af790bfa4e 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -93,6 +93,10 @@ OPTIONS --ignore-case:: Sorting and filtering refs are case insensitive. +--omit-empty:: + 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..b47fef51fb 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 = 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 (out.len || !omit_empty) + 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(0, "omit-empty", &omit_empty, + 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")), diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c index 6f62f40d12..1fc5130481 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 = 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(0, "omit-empty", &omit_empty, + N_("do not output a newline after empty formatted refs")), OPT_GROUP(""), OPT_INTEGER( 0 , "count", &maxcount, N_("show only <n> matched refs")), @@ -88,7 +90,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 (output.len || !omit_empty) + putchar('\n'); } strbuf_release(&err); diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh index d34d77f893..c06906d83e 100755 --- a/t/t3203-branch-output.sh +++ b/t/t3203-branch-output.sh @@ -341,6 +341,30 @@ 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' ' + 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 --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 --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..d4ccc22d99 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 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 >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 && -- 2.20.1 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v2 0/2] branch, for-each-ref: add option to omit empty lines 2023-04-06 17:08 ` [PATCH v2 0/2] branch, for-each-ref: add option to omit empty lines Øystein Walle 2023-04-06 17:08 ` [PATCH v2 1/2] ref-filter: remove unused ref_format member Øystein Walle 2023-04-06 17:08 ` [PATCH v2 2/2] branch, for-each-ref: add option to omit empty lines Øystein Walle @ 2023-04-06 18:24 ` Junio C Hamano 2023-04-07 17:53 ` [PATCH v3] branch, for-each-ref, tag: " Øystein Walle 2 siblings, 1 reply; 30+ messages in thread From: Junio C Hamano @ 2023-04-06 18:24 UTC (permalink / raw) To: Øystein Walle; +Cc: git, phillip.wood123, peff Øystein Walle <oystwa@gmail.com> writes: > Øystein Walle (2): > ref-filter: remove unused ref_format member > branch, for-each-ref: add option to omit empty lines > > Documentation/git-branch.txt | 4 ++++ > Documentation/git-for-each-ref.txt | 4 ++++ > ref-filter.h | 1 - > builtin/branch.c | 6 +++++- > builtin/for-each-ref.c | 7 +++++-- > ref-filter.c | 1 - > t/t3203-branch-output.sh | 24 ++++++++++++++++++++++++ > t/t6300-for-each-ref.sh | 8 ++++++++ > 8 files changed, 50 insertions(+), 5 deletions(-) I have always thought that the listing mode of "branch" and "tag" subcommands is a mere syntax sugar around "for-each-ref", and the above leaves me puzzled. Did we decide not to add the same for "git tag" in the listing mode during the review of the previous round (if we did, I do not recall the discussion), or would it be just the matter of adding another 6-line patch to builtin/tag.c? Thanks. ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v3] branch, for-each-ref, tag: add option to omit empty lines 2023-04-06 18:24 ` [PATCH v2 0/2] " Junio C Hamano @ 2023-04-07 17:53 ` Øystein Walle 2023-04-07 18:48 ` Junio C Hamano 2023-04-12 23:44 ` Andrei Rybak 0 siblings, 2 replies; 30+ messages in thread From: Øystein Walle @ 2023-04-07 17:53 UTC (permalink / raw) To: gitster; +Cc: git, oystwa, peff, phillip.wood123 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> --- Dang, you're right. But yes, it was a near-identical patch to builtin/tag.c. Along with a test, of course. I see you already applied the first of these patches so in this iteration there's only one. Documentation/git-branch.txt | 4 ++++ Documentation/git-for-each-ref.txt | 4 ++++ Documentation/git-tag.txt | 4 ++++ builtin/branch.c | 6 +++++- builtin/for-each-ref.c | 7 +++++-- builtin/tag.c | 6 +++++- t/t3203-branch-output.sh | 24 ++++++++++++++++++++++++ t/t6300-for-each-ref.sh | 8 ++++++++ t/t7004-tag.sh | 16 ++++++++++++++++ 9 files changed, 75 insertions(+), 4 deletions(-) diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt index d382ac69f7..d207da9101 100644 --- a/Documentation/git-branch.txt +++ b/Documentation/git-branch.txt @@ -156,6 +156,10 @@ in another worktree linked to the same repository. --ignore-case:: Sorting and filtering branches are case insensitive. +--omit-empty:: + 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 0713e49b49..1e215d4e73 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -98,6 +98,10 @@ OPTIONS --ignore-case:: Sorting and filtering refs are case insensitive. +--omit-empty:: + Do not print a newline after formatted refs where the format expands + to the empty string. + FIELD NAMES ----------- diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt index fdc72b5875..7f61c1edb3 100644 --- a/Documentation/git-tag.txt +++ b/Documentation/git-tag.txt @@ -131,6 +131,10 @@ options for details. --ignore-case:: Sorting and filtering tags are case insensitive. +--omit-empty:: + Do not print a newline after formatted refs where the format expands + to the empty string. + --column[=<options>]:: --no-column:: Display tag listing in columns. See configuration variable diff --git a/builtin/branch.c b/builtin/branch.c index 6413a016c5..98d5fa2c8e 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -44,6 +44,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 = 0; static int branch_use_color = -1; static char branch_colors[][COLOR_MAXLEN] = { @@ -466,7 +467,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 (out.len || !omit_empty) + putchar('\n'); } } @@ -675,6 +677,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(0, "omit-empty", &omit_empty, + 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")), diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c index 0bdc49a6e1..695fc8f4a5 100644 --- a/builtin/for-each-ref.c +++ b/builtin/for-each-ref.c @@ -22,7 +22,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 = 0; struct ref_array array; struct ref_filter filter; struct ref_format format = REF_FORMAT_INIT; @@ -40,6 +40,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(0, "omit-empty", &omit_empty, + N_("do not output a newline after empty formatted refs")), OPT_GROUP(""), OPT_INTEGER( 0 , "count", &maxcount, N_("show only <n> matched refs")), @@ -112,7 +114,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 (output.len || !omit_empty) + putchar('\n'); } strbuf_release(&err); diff --git a/builtin/tag.c b/builtin/tag.c index 782bb3aa2f..ab5f5c74f4 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -41,6 +41,7 @@ static const char * const git_tag_usage[] = { static unsigned int colopts; static int force_sign_annotate; static int config_sign_tag = -1; /* unspecified */ +static int omit_empty = 0; static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, struct ref_format *format) @@ -79,7 +80,8 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, 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 (output.len || !omit_empty) + putchar('\n'); } strbuf_release(&err); @@ -474,6 +476,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix) OPT_WITHOUT(&filter.no_commit, N_("print only tags that don't contain the commit")), OPT_MERGED(&filter, N_("print only tags that are merged")), OPT_NO_MERGED(&filter, N_("print only tags that are not merged")), + OPT_BOOL(0, "omit-empty", &omit_empty, + N_("do not output a newline after empty formatted refs")), OPT_REF_SORT(&sorting_options), { OPTION_CALLBACK, 0, "points-at", &filter.points_at, N_("object"), diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh index 1c0f7ea24e..93f8295339 100755 --- a/t/t3203-branch-output.sh +++ b/t/t3203-branch-output.sh @@ -355,6 +355,30 @@ 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' ' + 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 --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 --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 6614469d2d..5c00607608 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 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 >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 && diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index 32b312fa80..0fe6ba93a2 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -2046,6 +2046,22 @@ test_expect_success '--format should list tags as per format given' ' test_cmp expect actual ' +test_expect_success '--format --omit-empty works' ' + cat >expect <<-\EOF && + refname : refs/tags/v1.0 + + refname : refs/tags/v1.1.3 + EOF + git tag -l --format="%(if:notequals=refs/tags/v1.0.1)%(refname)%(then)refname : %(refname)%(end)" "v1*" >actual && + test_cmp expect actual && + cat >expect <<-\EOF && + refname : refs/tags/v1.0 + refname : refs/tags/v1.1.3 + EOF + git tag -l --omit-empty --format="%(if:notequals=refs/tags/v1.0.1)%(refname)%(then)refname : %(refname)%(end)" "v1*" >actual && + test_cmp expect actual +' + test_expect_success 'git tag -l with --format="%(rest)" must fail' ' test_must_fail git tag -l --format="%(rest)" "v1*" ' -- 2.20.1 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v3] branch, for-each-ref, tag: add option to omit empty lines 2023-04-07 17:53 ` [PATCH v3] branch, for-each-ref, tag: " Øystein Walle @ 2023-04-07 18:48 ` Junio C Hamano 2023-04-12 23:44 ` Andrei Rybak 1 sibling, 0 replies; 30+ messages in thread From: Junio C Hamano @ 2023-04-07 18:48 UTC (permalink / raw) To: Øystein Walle; +Cc: git, peff, phillip.wood123 Øystein Walle <oystwa@gmail.com> writes: > 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> > --- > Dang, you're right. But yes, it was a near-identical patch to > builtin/tag.c. Along with a test, of course. There are small nits like "do not explicitly initialize statics to 0", which may not be big enough to warrant a reroll. Other than that, looking good. Thanks, will replace. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3] branch, for-each-ref, tag: add option to omit empty lines 2023-04-07 17:53 ` [PATCH v3] branch, for-each-ref, tag: " Øystein Walle 2023-04-07 18:48 ` Junio C Hamano @ 2023-04-12 23:44 ` Andrei Rybak 2023-04-13 7:17 ` Øystein Walle 1 sibling, 1 reply; 30+ messages in thread From: Andrei Rybak @ 2023-04-12 23:44 UTC (permalink / raw) To: Øystein Walle, gitster; +Cc: git, peff, phillip.wood123 A couple of drive-by nitpicks about the commit message: On 07/04/2023 19:53, Øystein Walle wrote: > Subject: [PATCH v3] branch, for-each-ref, tag: add option to omit empty lines > > 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 It seems that a word is missing in the first sentence. Perhaps, s/printed it/printed for it/ ? > example, git update-ref --stdin does not accept empty lines. > > Add options to branch and for-each-ref to not print these empty lines. "git tag" is mentioned in the subject line, but not here. > The default behavior remains the same. > > Signed-off-by: Øystein Walle <oystwa@gmail.com> > --- > Dang, you're right. But yes, it was a near-identical patch to > builtin/tag.c. Along with a test, of course. > > I see you already applied the first of these patches so in this > iteration there's only one. > > Documentation/git-branch.txt | 4 ++++ > Documentation/git-for-each-ref.txt | 4 ++++ > Documentation/git-tag.txt | 4 ++++ > builtin/branch.c | 6 +++++- > builtin/for-each-ref.c | 7 +++++-- > builtin/tag.c | 6 +++++- > t/t3203-branch-output.sh | 24 ++++++++++++++++++++++++ > t/t6300-for-each-ref.sh | 8 ++++++++ > t/t7004-tag.sh | 16 ++++++++++++++++ > 9 files changed, 75 insertions(+), 4 deletions(-) > ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3] branch, for-each-ref, tag: add option to omit empty lines 2023-04-12 23:44 ` Andrei Rybak @ 2023-04-13 7:17 ` Øystein Walle 2023-04-13 15:13 ` Junio C Hamano 0 siblings, 1 reply; 30+ messages in thread From: Øystein Walle @ 2023-04-13 7:17 UTC (permalink / raw) To: Andrei Rybak; +Cc: gitster, git, peff, phillip.wood123 Hi Andrei, On Thu, 13 Apr 2023 at 01:44, Andrei Rybak <rybak.a.v@gmail.com> wrote: > It seems that a word is missing in the first sentence. Perhaps, > > s/printed it/printed for it/ > > ? Sort of... I think I meant s/printed it/printed/ :-) > "git tag" is mentioned in the subject line, but not here. It should definitely be added, yes. Junio, should I resend or will you touch up the message? Not sure what the proper procedure is since it's already in seen. Thanks. Øsse ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3] branch, for-each-ref, tag: add option to omit empty lines 2023-04-13 7:17 ` Øystein Walle @ 2023-04-13 15:13 ` Junio C Hamano 0 siblings, 0 replies; 30+ messages in thread From: Junio C Hamano @ 2023-04-13 15:13 UTC (permalink / raw) To: Øystein Walle; +Cc: Andrei Rybak, git, peff, phillip.wood123 Øystein Walle <oystwa@gmail.com> writes: > Sort of... I think I meant s/printed it/printed/ :-) > >> "git tag" is mentioned in the subject line, but not here. > > It should definitely be added, yes. Junio, should I resend or will you touch up > the message? Not sure what the proper procedure is since it's already in seen. As I write in "What's cooking", being in 'seen' does not count all that much and we can freely replace them and erace the trace of "past mistakes", until the series hits 'next'. For small changes like this, telling me to touch them up, as long as the necessary changes are obvious, is just fine. Sending out v4 is also fine for a series of any size. I just locally amended the commit log message. Thanks. 1: e0053ad012 ! 1: aabfdc9514 branch, for-each-ref, tag: add option to omit empty lines @@ Metadata ## Commit message ## branch, for-each-ref, tag: add option to omit empty lines - 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 + If the given format string expands to the empty string, a newline is + still printed. 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. + Add options to "git branch", "git for-each-ref", and "git tag" to + not print these empty lines. The default behavior remains the same. Signed-off-by: Øystein Walle <oystwa@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 2/2] branch, for-each-ref: add option to omit empty lines 2023-03-30 11:21 [PATCH 0/2] branch, for-each-ref: add option to omit empty lines Øystein Walle 2023-03-30 11:21 ` [PATCH 1/2] ref-filter: remove unused ref_format member Øystein Walle @ 2023-03-30 11:21 ` Øystein Walle 2023-03-30 15:54 ` Junio C Hamano ` (2 more replies) 1 sibling, 3 replies; 30+ messages in thread From: Øystein Walle @ 2023-03-30 11:21 UTC (permalink / raw) To: git; +Cc: Øystein Walle 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(-) 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 && -- 2.34.1 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines 2023-03-30 11:21 ` [PATCH 2/2] branch, for-each-ref: " Øystein Walle @ 2023-03-30 15:54 ` Junio C Hamano 2023-03-30 18:25 ` Jeff King 2023-03-31 8:32 ` Øystein Walle 2023-03-30 17:21 ` Junio C Hamano 2023-03-31 16:33 ` Phillip Wood 2 siblings, 2 replies; 30+ messages in thread From: Junio C Hamano @ 2023-03-30 15:54 UTC (permalink / raw) To: Øystein Walle; +Cc: git Ø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. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines 2023-03-30 15:54 ` Junio C Hamano @ 2023-03-30 18:25 ` Jeff King 2023-03-30 18:54 ` Junio C Hamano 2023-03-31 8:32 ` Øystein Walle 1 sibling, 1 reply; 30+ messages in thread From: Jeff King @ 2023-03-30 18:25 UTC (permalink / raw) To: Junio C Hamano; +Cc: Øystein Walle, git 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines 2023-03-30 18:25 ` Jeff King @ 2023-03-30 18:54 ` Junio C Hamano 0 siblings, 0 replies; 30+ messages in thread From: Junio C Hamano @ 2023-03-30 18:54 UTC (permalink / raw) To: Jeff King; +Cc: Øystein Walle, git 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines 2023-03-30 15:54 ` Junio C Hamano 2023-03-30 18:25 ` Jeff King @ 2023-03-31 8:32 ` Øystein Walle 2023-03-31 15:57 ` Junio C Hamano 1 sibling, 1 reply; 30+ messages in thread From: Øystein Walle @ 2023-03-31 8:32 UTC (permalink / raw) To: gitster; +Cc: git, oystwa 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines 2023-03-31 8:32 ` Øystein Walle @ 2023-03-31 15:57 ` Junio C Hamano 0 siblings, 0 replies; 30+ messages in thread From: Junio C Hamano @ 2023-03-31 15:57 UTC (permalink / raw) To: Øystein Walle; +Cc: git Ø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. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines 2023-03-30 11:21 ` [PATCH 2/2] branch, for-each-ref: " Øystein Walle 2023-03-30 15:54 ` Junio C Hamano @ 2023-03-30 17:21 ` Junio C Hamano 2023-03-31 16:33 ` Phillip Wood 2 siblings, 0 replies; 30+ messages in thread From: Junio C Hamano @ 2023-03-30 17:21 UTC (permalink / raw) To: Øystein Walle; +Cc: git Ø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. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines 2023-03-30 11:21 ` [PATCH 2/2] branch, for-each-ref: " Øystein Walle 2023-03-30 15:54 ` Junio C Hamano 2023-03-30 17:21 ` Junio C Hamano @ 2023-03-31 16:33 ` Phillip Wood 2023-03-31 17:17 ` Junio C Hamano 2 siblings, 1 reply; 30+ messages in thread From: Phillip Wood @ 2023-03-31 16:33 UTC (permalink / raw) To: Øystein Walle, git; +Cc: Junio C Hamano, Jeff King 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 && ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines 2023-03-31 16:33 ` Phillip Wood @ 2023-03-31 17:17 ` Junio C Hamano 2023-04-06 16:55 ` Øystein Walle 0 siblings, 1 reply; 30+ messages in thread From: Junio C Hamano @ 2023-03-31 17:17 UTC (permalink / raw) To: Phillip Wood; +Cc: Øystein Walle, git, Jeff King 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. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines 2023-03-31 17:17 ` Junio C Hamano @ 2023-04-06 16:55 ` Øystein Walle 2023-04-06 17:12 ` Jeff King 2023-04-06 18:07 ` Junio C Hamano 0 siblings, 2 replies; 30+ messages in thread From: Øystein Walle @ 2023-04-06 16:55 UTC (permalink / raw) To: Junio C Hamano; +Cc: Phillip Wood, git, Jeff King 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines 2023-04-06 16:55 ` Øystein Walle @ 2023-04-06 17:12 ` Jeff King 2023-04-06 18:20 ` Junio C Hamano 2023-04-06 18:07 ` Junio C Hamano 1 sibling, 1 reply; 30+ messages in thread From: Jeff King @ 2023-04-06 17:12 UTC (permalink / raw) To: Øystein Walle; +Cc: Junio C Hamano, Phillip Wood, git 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines 2023-04-06 17:12 ` Jeff King @ 2023-04-06 18:20 ` Junio C Hamano 2023-04-10 19:56 ` Jeff King 0 siblings, 1 reply; 30+ messages in thread From: Junio C Hamano @ 2023-04-06 18:20 UTC (permalink / raw) To: Jeff King; +Cc: Øystein Walle, Phillip Wood, git 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. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines 2023-04-06 18:20 ` Junio C Hamano @ 2023-04-10 19:56 ` Jeff King 0 siblings, 0 replies; 30+ messages in thread From: Jeff King @ 2023-04-10 19:56 UTC (permalink / raw) To: Junio C Hamano; +Cc: Øystein Walle, Phillip Wood, git 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines 2023-04-06 16:55 ` Øystein Walle 2023-04-06 17:12 ` Jeff King @ 2023-04-06 18:07 ` Junio C Hamano 1 sibling, 0 replies; 30+ messages in thread From: Junio C Hamano @ 2023-04-06 18:07 UTC (permalink / raw) To: Øystein Walle; +Cc: Phillip Wood, git, Jeff King Ø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. ^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2023-04-13 15:14 UTC | newest] Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-03-30 11:21 [PATCH 0/2] branch, for-each-ref: add option to omit empty lines Øystein Walle 2023-03-30 11:21 ` [PATCH 1/2] ref-filter: remove unused ref_format member Øystein Walle 2023-03-30 15:21 ` Junio C Hamano 2023-03-30 15:25 ` Junio C Hamano 2023-03-31 10:37 ` Øystein Walle 2023-03-31 10:57 ` ZheNing Hu 2023-03-31 16:19 ` Junio C Hamano 2023-04-06 17:08 ` [PATCH v2 0/2] branch, for-each-ref: add option to omit empty lines Øystein Walle 2023-04-06 17:08 ` [PATCH v2 1/2] ref-filter: remove unused ref_format member Øystein Walle 2023-04-06 17:08 ` [PATCH v2 2/2] branch, for-each-ref: add option to omit empty lines Øystein Walle 2023-04-06 18:24 ` [PATCH v2 0/2] " Junio C Hamano 2023-04-07 17:53 ` [PATCH v3] branch, for-each-ref, tag: " Øystein Walle 2023-04-07 18:48 ` Junio C Hamano 2023-04-12 23:44 ` Andrei Rybak 2023-04-13 7:17 ` Øystein Walle 2023-04-13 15:13 ` Junio C Hamano 2023-03-30 11:21 ` [PATCH 2/2] branch, for-each-ref: " Øystein Walle 2023-03-30 15:54 ` Junio C Hamano 2023-03-30 18:25 ` Jeff King 2023-03-30 18:54 ` Junio C Hamano 2023-03-31 8:32 ` Øystein Walle 2023-03-31 15:57 ` Junio C Hamano 2023-03-30 17:21 ` Junio C Hamano 2023-03-31 16:33 ` Phillip Wood 2023-03-31 17:17 ` Junio C Hamano 2023-04-06 16:55 ` Øystein Walle 2023-04-06 17:12 ` Jeff King 2023-04-06 18:20 ` Junio C Hamano 2023-04-10 19:56 ` Jeff King 2023-04-06 18:07 ` Junio C Hamano
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.