patch, 2 partsbranch, for-each-ref: add option to omit empty lines

> 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;
>  }

> Ø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.

>
> 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.

>
> 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.
>

> 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...

> Ø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(-)

> 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.

> 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(-)
> 

> 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.

> 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.

> @@ -757,7 +761,13 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  	}
>  
>  	if (list)
> +	{

>  		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);
> +	}

> @@ -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');
>  	}

> +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

> 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 &&

>  * 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 ...

> 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

> 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.

> > +             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.

> 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.

> This feature makes %(if)...%(else)...%(end) construct complete and
> is a very good addition.
>
> Thanks for working on it.
>

>> > +             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.

> 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", &copy, N_("copy a branch and its reflog"), 1),
>   		OPT_BIT('C', NULL, &copy, 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.

> > 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.

> 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.

>> 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.