[PATCH 0/2] branch, for-each-ref: add option to omit empty lines

[PATCH 0/2] branch, for-each-ref: add option to omit empty lines

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

[PATCH 1/2] ref-filter: remove unused ref_format member

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

[PATCH 2/2] branch, for-each-ref: add option to omit empty lines

[Ø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", &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 &&
-- 
2.34.1

Re: [PATCH 1/2] ref-filter: remove unused ref_format member

[Junio C Hamano]

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

Re: [PATCH 1/2] ref-filter: remove unused ref_format member

[Junio C Hamano]

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.

Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines

[Junio C Hamano]

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

Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines

[Junio C Hamano]

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

Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines

[Jeff King]

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

Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines

[Junio C Hamano]

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

Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines

[Øystein Walle]

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

Re: [PATCH 1/2] ref-filter: remove unused ref_format member

[Øystein Walle]

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

Re: [PATCH 1/2] ref-filter: remove unused ref_format member

[ZheNing Hu]

Ø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

Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines

[Junio C Hamano]

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

Re: [PATCH 1/2] ref-filter: remove unused ref_format member

[Junio C Hamano]

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

Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines

[Phillip Wood]

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

Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines

[Junio C Hamano]

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.

Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines

[Øystein Walle]

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

[PATCH v2 0/2] branch, for-each-ref: add option to omit empty lines

[Øystein Walle]

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

[PATCH v2 1/2] ref-filter: remove unused ref_format member

[Øystein Walle]

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

[PATCH v2 2/2] branch, for-each-ref: add option to omit empty lines

[Ø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>
---
 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", &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")),
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

Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines

[Jeff King]

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

Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines

[Junio C Hamano]

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

Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines

[Junio C Hamano]

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.

Re: [PATCH v2 0/2] branch, for-each-ref: add option to omit empty lines

[Junio C Hamano]

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

[PATCH v3] branch, for-each-ref, tag: add option to omit empty lines

[Ø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>
---
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", &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")),
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

Re: [PATCH v3] branch, for-each-ref, tag: add option to omit empty lines

[Junio C Hamano]

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

Re: [PATCH 2/2] branch, for-each-ref: add option to omit empty lines

[Jeff King]

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

Re: [PATCH v3] branch, for-each-ref, tag: add option to omit empty lines

[Andrei Rybak]

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

Re: [PATCH v3] branch, for-each-ref, tag: add option to omit empty lines

[Øystein Walle]

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

Re: [PATCH v3] branch, for-each-ref, tag: add option to omit empty lines

[Junio C Hamano]

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