threads / patch / 35861

patchfor-each-ref: add option to omit newlines

Subject: [PATCH] for-each-ref: add option to omit newlines

## tl;dr

7 messages between Feb 13, 2014 and Feb 14, 2014. Diffs are folded; open one to read it.

replies: 6people: 2as markdown or json

Øystein Walle· Feb 13, 2014, 18:00 UTC · lore

Even when having specified a format string for-each-ref still prints a newline after printing each ref according to the format. This breaks splitting the output on newlines, for example.

Signed-off-by: Øystein Walle <oystwa@gmail.com>
---
I was somewhat surprised by this behaviour; I expected to be in full control of
the output but I could not get rid of the newlines.

As far as I'm *personally* concerned the line "putchar('\n');" could just as well be removed completely and an '\n' appended to the default format string. But since this command (and this behaviour) as been around since 2006 I assume that it's in Git's best interest to not change the default behaviour. Hence the introduction of this option. Although I was taken aback by this behaviour, is it patch-worthy? The fix isn't very pretty.

On to the patch itself: I contemplated putting '\n' in the default format and removing it if -n was given, which would get rid of the need to pass an exta argument to show_ref(). But that means we would need to *insert it* when a format is given and -n is not...

I've changed how the default format is assigned so that we can check if it's NULL after option parsing to spit out an error. Alternatively we could just allow it.

 Documentation/git-for-each-ref.txt |  3 +++
 builtin/for-each-ref.c             | 20 +++++++++++++++-----
 2 files changed, 18 insertions(+), 5 deletions(-)
Show changes to 2 files +18 −5

Documentation/git-for-each-ref.txt, builtin/for-each-ref.c

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 4240875..805914a 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -62,6 +62,9 @@ OPTIONS
 	the specified host language.  This is meant to produce
 	a scriptlet that can directly be `eval`ed.
 
+-n::
+--no-newlines::
+	Do not print a newline after printing each formatted ref.
 
 FIELD NAMES
 -----------
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 51798b4..32d6b12 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -998,7 +998,8 @@ static void emit(const char *cp, const char *ep)
 	}
 }
 
-static void show_ref(struct refinfo *info, const char *format, int quote_style)
+static void show_ref(struct refinfo *info, const char *format, int quote_style,
+		     int no_newlines)
 {
 	const char *cp, *sp, *ep;
 
@@ -1023,7 +1024,8 @@ static void show_ref(struct refinfo *info, const char *format, int quote_style)
 		resetv.s = color;
 		print_value(&resetv, quote_style);
 	}
-	putchar('\n');
+	if (!no_newlines)
+		putchar('\n');
 }
 
 static struct ref_sort *default_sort(void)
@@ -1067,9 +1069,9 @@ static char const * const for_each_ref_usage[] = {
 int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 {
 	int i, num_refs;
-	const char *format = "%(objectname) %(objecttype)\t%(refname)";
+	const char *format = NULL;
 	struct ref_sort *sort = NULL, **sort_tail = &sort;
-	int maxcount = 0, quote_style = 0;
+	int maxcount = 0, quote_style = 0, no_newlines = 0;
 	struct refinfo **refs;
 	struct grab_ref_cbdata cbdata;
 
@@ -1082,6 +1084,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",  &quote_style,
 			N_("quote placeholders suitably for tcl"), QUOTE_TCL),
+		OPT_BOOL('n' , "no-newlines",  &no_newlines,
+			N_("do not output a newline after each ref")),
 
 		OPT_GROUP(""),
 		OPT_INTEGER( 0 , "count", &maxcount, N_("show only <n> matched refs")),
@@ -1100,6 +1104,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 (no_newlines && !format) {
+		error("--no-newlines without --format does not make sense");
+		usage_with_options(for_each_ref_usage, opts);
+	}
+	if (!format)
+		format = "%(objectname) %(objecttype)\t%(refname)";
 	if (verify_format(format))
 		usage_with_options(for_each_ref_usage, opts);
 
@@ -1120,6 +1130,6 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 	if (!maxcount || num_refs < maxcount)
 		maxcount = num_refs;
 	for (i = 0; i < maxcount; i++)
-		show_ref(refs[i], format, quote_style);
+		show_ref(refs[i], format, quote_style, no_newlines);
 	return 0;
 }

-- 
1.8.5
Junio C Hamano· Feb 13, 2014, 20:10 UTC · re: Øystein Walle · lore

Re: [PATCH] for-each-ref: add option to omit newlines

Øystein Walle <oystwa@gmail.com> writes:
> On to the patch itself: I contemplated putting '\n' in the default format and
> removing it if -n was given, which would get rid of the need to pass an exta
> argument to show_ref(). But that means we would need to *insert it* when a
> format is given and -n is not...

I would rather see us go in the direction to add "-z" output option, which is what everybody else that produces NUL terminated entries in our suite of subcommands does.

IOW, something along with this line (untested).
 builtin/for-each-ref.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
Show changes to builtin/for-each-ref.c +5 −1
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 51798b4..2c8cac8 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -94,6 +94,7 @@ static const char **used_atom;
 static cmp_type *used_atom_type;
 static int used_atom_cnt, need_tagged, need_symref;
 static int need_color_reset_at_eol;
+static int line_termination = '\n';
 
 /*
  * Used to parse format string and sort specifiers
@@ -1023,7 +1024,7 @@ static void show_ref(struct refinfo *info, const char *format, int quote_style)
 		resetv.s = color;
 		print_value(&resetv, quote_style);
 	}
-	putchar('\n');
+	putchar(line_termination);
 }
 
 static struct ref_sort *default_sort(void)
@@ -1088,6 +1089,9 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 		OPT_STRING(  0 , "format", &format, N_("format"), N_("format to use for the output")),
 		OPT_CALLBACK(0 , "sort", sort_tail, N_("key"),
 			    N_("field name to sort on"), &opt_parse_sort),
+		OPT_SET_INT('z', NULL, &line_termination,
+			    N_("Use NUL instead of LF to end each output records"),
+			    '\0'),
 		OPT_END(),
 	};
Øystein Walle· Feb 14, 2014, 08:11 UTC · re: Junio C Hamano · lore

Re: [PATCH] for-each-ref: add option to omit newlines

Junio C Hamano <gitster <at> pobox.com> writes:
Show 5 quoted lines
> 
> I would rather see us go in the direction to add "-z" output option,
> which is what everybody else that produces NUL terminated entries in
> our suite of subcommands does.
>

I agree that -z would help in this case and I very much appreciate that option when using diff --name-only, ls-files, etc.

However, when specifying a format string it's just a matter of ending the format string in '%00' and you're good to go. But then you get the null byte *and* a newline. And with your proposal there would be no way of saying you want neither.

I expected the output to be formatted according to a (repetition of) the format string, not some variation of it that I couldn't opt out of. But I see that git-log also shows this behavior and already has a -z option, so I guess that's fairly ingrained.

Maybe it's just me? In that case I've no problem with throwing in the towel.

Øsse
Junio C Hamano· Feb 14, 2014, 16:28 UTC · re: Øystein Walle · lore

Re: [PATCH] for-each-ref: add option to omit newlines

Øystein Walle <oystwa@gmail.com> writes:
> However, when specifying a format string it's just a matter of ending
> the format string in '%00' and you're good to go. But then you get the
> null byte *and* a newline. And with your proposal there would be no way
> of saying you want neither.

I very well understand that. All other commands that support "-z" to give you NUL terminated output do not consider that a downside. Why should for-each-ref be special?

Øystein Walle· Feb 14, 2014, 23:03 UTC · re: Junio C Hamano · lore

Re: [PATCH] for-each-ref: add option to omit newlines

Junio C Hamano <gitster <at> pobox.com> writes:
Show 5 quoted lines
> 
> I very well understand that.  All other commands that support "-z"
> to give you NUL terminated output do not consider that a downside.
> Why should for-each-ref be special?
>

After I discovered log also has this there is nothing special about for-each-ref any longer, so my patch as-is would only make things less consistent. What is "special" is that they give you the option of supplying a format string.

ls-files, diff and others print a specific list of items (paths, shas, ...) and there's no question about how they are presented other than the delimiter between each item, to which a selection of either a newline or a null byte is plenty.

With log, for-each-ref and rev-list (any others?) that sort of breaks down. With the format string you're given the power to make the command print basically anything you like, however you like; no longer only a question of mere delimiters. It only makes sense then (to me, at least) that the command does not meddle with the format the user has chosen.

Maybe it's all subjective... I'm okay with just leaving things as they are. There are ways around it.

Junio C Hamano· Feb 14, 2014, 23:24 UTC · re: Øystein Walle · lore

Re: [PATCH] for-each-ref: add option to omit newlines

Øystein Walle <oystwa@gmail.com> writes:
> Maybe it's all subjective... I'm okay with just leaving things as they
> are.

Lack of "-z" in for-each-ref can be called an inconsistency that already exists you may want to fix in any case.

As an extension to that, I would not be fundamentally against a new option, e.g. "--terminiator=7", that causes us to use putchar(7) instead of putchar('\n') or putchar('\0') to terminate each records. At that point, "-z" becomes a synonym for "--terminator=0".

And "--terminator=''" might even be a natural extension to that option to cause us not to call any putchar() there.

If we were to do that, we should do them for all commands that let you use "-z", not just for-each-ref, for consistency reasons, I would think.

← back to recent threads