threads / patch / 37526

patchpretty-format: add append line-feed format specifier

Subject: [PATCH] pretty-format: add append line-feed format specifier

## tl;dr

10 messages between Sep 9, 2014 and Sep 12, 2014. Diffs are folded; open one to read it.

replies: 9people: 3as markdown or json

Harry Jeffery· Sep 9, 2014, 18:09 UTC · lore

Add a new format prefix `_` that causes a line-feed to be inserted immediately after an expansion if the expansion expands to a non-empty string. This is useful for when you would like a line for an expansion to be prepended, but only when the expansion expands to a non empty string, such as inserting a '%_d' expansion before a commit to show any refs pointing towards it.

Signed-off-by: Harry Jeffery <harry@exec64.co.uk>
---
  Documentation/pretty-formats.txt |  4 ++++
  pretty.c                         | 10 ++++++++--
  2 files changed, 12 insertions(+), 2 deletions(-)
Show changes to 2 files +12 −2

Documentation/pretty-formats.txt, pretty.c

diff --git a/Documentation/pretty-formats.txt 
b/Documentation/pretty-formats.txt
index 85d6353..842cd17 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -197,6 +197,10 @@ If you add a ` ` (space) after '%' of a 
placeholder, a space
  is inserted immediately before the expansion if and only if the
  placeholder expands to a non-empty string.

+If you add a `_` (underscore) after '%' of a placeholder, a line-feed
+is inserted immediately after the expansion if and only if the
+placeholder expands to a non-empty string.
+
  * 'tformat:'
  +
  The 'tformat:' format works exactly like 'format:', except that it
diff --git a/pretty.c b/pretty.c
index 44b9f64..ddb930d 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1416,7 +1416,8 @@ static size_t format_commit_item(struct strbuf 
*sb, /* in UTF-8 */
  		NO_MAGIC,
  		ADD_LF_BEFORE_NON_EMPTY,
  		DEL_LF_BEFORE_EMPTY,
-		ADD_SP_BEFORE_NON_EMPTY
+		ADD_SP_BEFORE_NON_EMPTY,
+		ADD_LF_AFTER_NON_EMPTY
  	} magic = NO_MAGIC;

  	switch (placeholder[0]) {
@@ -1429,6 +1430,9 @@ static size_t format_commit_item(struct strbuf 
*sb, /* in UTF-8 */
  	case ' ':
  		magic = ADD_SP_BEFORE_NON_EMPTY;
  		break;
+	case '_':
+		magic = ADD_LF_AFTER_NON_EMPTY;
+		break;
  	default:
  		break;
  	}
@@ -1449,6 +1453,8 @@ static size_t format_commit_item(struct strbuf 
*sb, /* in UTF-8 */
  	} else if (orig_len != sb->len) {
  		if (magic == ADD_LF_BEFORE_NON_EMPTY)
  			strbuf_insert(sb, orig_len, "\n", 1);
+		else if (magic == ADD_LF_AFTER_NON_EMPTY)
+			strbuf_addch(sb, '\n');
  		else if (magic == ADD_SP_BEFORE_NON_EMPTY)
  			strbuf_insert(sb, orig_len, " ", 1);
  	}
@@ -1460,7 +1466,7 @@ static size_t userformat_want_item(struct strbuf 
*sb, const char *placeholder,
  {
  	struct userformat_want *w = context;

-	if (*placeholder == '+' || *placeholder == '-' || *placeholder == ' ')
+	if (*placeholder == '+' || *placeholder == '-' || *placeholder == ' ' 
|| *placeholder == '_')
  		placeholder++;

  	switch (*placeholder) {

-- 
2.1.0
Junio C Hamano· Sep 9, 2014, 19:15 UTC · re: Harry Jeffery · lore

Re: [PATCH] pretty-format: add append line-feed format specifier

Harry Jeffery <harry@exec64.co.uk> writes:
Show 6 quoted lines
> Add a new format prefix `_` that causes a line-feed to be inserted
> immediately after an expansion if the expansion expands to a non-empty
> string. This is useful for when you would like a line for an expansion
> to be prepended, but only when the expansion expands to a non empty
> string, such as inserting a '%_d' expansion before a commit to show any
> refs pointing towards it.
Is this different from "%n%-d"?
Harry Jeffery· Sep 9, 2014, 19:30 UTC · re: Junio C Hamano · lore

Re: [PATCH] pretty-format: add append line-feed format specifier

On 09/09/14 20:15, Junio C Hamano wrote:
> Is this different from "%n%-d"?
>
Yes. "%n%-d" will place the newline before the expansion, not after.

log --decorate --pretty=format:"%n%-d%h\\ %t\\ [%cn]\\ %s" ---

  (HEAD, upstream/master, master)85f0837 c29da1d [Junio C Hamano] Start 
the post-2.1 cycle
  f655651 4027a43 [Junio C Hamano] Merge branch 'rs/strbuf-getcwd'
  51eeaea 1f4970c [Junio C Hamano] Merge branch 'ta/pretty-parse-config'
  4740891 8961621 [Junio C Hamano] Merge branch 'bc/archive-pax-header-mode'
---
log --decorate --pretty=format:"%_d%%h\\ %t\\ [%cn]\\ %s"
---
  (HEAD, upstream/master, master)
85f0837 c29da1d [Junio C Hamano] Start the post-2.1 cycle
f655651 4027a43 [Junio C Hamano] Merge branch 'rs/strbuf-getcwd'
51eeaea 1f4970c [Junio C Hamano] Merge branch 'ta/pretty-parse-config'
4740891 8961621 [Junio C Hamano] Merge branch 'bc/archive-pax-header-mode'
---

The latter is the output I've been trying to accomplish, and as far as I can tell, this patch is the only way to achieve it.

Well, you can do "%d%n" but that will put a blank line before every commit that doesn't have a ref.

Junio C Hamano· Sep 9, 2014, 19:37 UTC · re: Harry Jeffery · lore

Re: [PATCH] pretty-format: add append line-feed format specifier

Harry Jeffery <harry@exec64.co.uk> writes:
Show 5 quoted lines
> On 09/09/14 20:15, Junio C Hamano wrote:
>> Is this different from "%n%-d"?
>>
>
> Yes. "%n%-d" will place the newline before the expansion, not after.

Maybe "%[-+ ]" needs to be rethought, instead of making things worse by turning it into "%[-_+ ]", as the next person who comes would want to add space after the expansion and would need to find yet another letter like you did with '_'.

Jeff King· Sep 9, 2014, 21:45 UTC · re: Junio C Hamano · lore

Re: [PATCH] pretty-format: add append line-feed format specifier

On Tue, Sep 09, 2014 at 12:37:48PM -0700, Junio C Hamano wrote:
Show 12 quoted lines
> Harry Jeffery <harry@exec64.co.uk> writes:
> 
> > On 09/09/14 20:15, Junio C Hamano wrote:
> >> Is this different from "%n%-d"?
> >>
> >
> > Yes. "%n%-d" will place the newline before the expansion, not after.
> 
> Maybe "%[-+ ]" needs to be rethought, instead of making things worse
> by turning it into "%[-_+ ]", as the next person who comes would
> want to add space after the expansion and would need to find yet
> another letter like you did with '_'.

Yeah, that was my thought on reading the initial patch, too. Why limit ourselves to newlines and spaces. I'd much rather have full conditional expansion, like "${foo:+prefix $foo suffix}" in the shell.

Something like the patch below might work, but I didn't test it very thoroughly (and note the comments, which might need dealing with). Maybe it would make a sensible base for Harry to build on if he wants to pursue this.

With it, you can do:
  git log --format='%h %s%if(%d,%n  Decoration:%d)' origin
to get:
  85f0837 Start the post-2.1 cycle
    Decoration: (origin/master, origin/HEAD, github/foo)
  f655651 Merge branch 'rs/strbuf-getcwd'
  51eeaea Merge branch 'ta/pretty-parse-config'
  4740891 Merge branch 'bc/archive-pax-header-mode'
  0e28161 Merge branch 'pr/remotes-in-hashmap'
  44ceb79 Merge branch 'jk/pretty-empty-format'
  56f214e Merge branch 'ta/config-set'
  e8e4ce7 Merge branch 'rs/init-no-duplicate-real-path'
  1d8a6f6 Merge branch 'mm/config-edit-global'
  c518279 Merge branch 'jc/reopen-lock-file'
  96db324 Merge git://github.com/git-l10n/git-po
    Decoration: (origin/maint)

You could also make "%d" more flexible with it. We unconditionally include the " (...)" wrapper when expanding it. But assuming we introduced a "%D" that is _just_ the decoration names, you could do:

  %if(%D, (%D))
to get the same effect with much more flexibility.
---
Show changes to pretty.c +36 −0
diff --git a/pretty.c b/pretty.c
index fe34ddc..96cd512 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1398,6 +1398,42 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */
 		ADD_SP_BEFORE_NON_EMPTY
 	} magic = NO_MAGIC;
 
+	if (starts_with(placeholder, "if(")) {
+		const char *cond_beg, *cond_end;
+		const char *data_beg, *data_end;
+		char *buf;
+		struct strbuf scratch = STRBUF_INIT;
+
+		/* can't handle commas in conditions; allow backslash-escaping? */
+		cond_beg = cond_end = placeholder + 3;
+		for (cond_end = cond_beg; *cond_end != ','; cond_end++)
+			if (!*cond_end)
+				return 0;
+
+		/* ditto for nested parentheses; backslash escaping? */
+		data_beg = cond_end + 1;
+		for (data_end = data_beg; *data_end != ')'; data_end++)
+			if (!*data_end)
+				return 0;
+
+		/*
+		 * Should teach formatters to return size only without
+		 * actually writing to scratch buffer?
+		 */
+		buf = xmemdupz(cond_beg, cond_end - cond_beg);
+		strbuf_expand(&scratch, buf, format_commit_item, context);
+		free(buf);
+
+		if (scratch.len) {
+			buf = xmemdupz(data_beg, data_end - data_beg);
+			strbuf_expand(sb, buf, format_commit_item, context);
+			free(buf);
+		}
+		strbuf_release(&scratch);
+
+		return data_end - placeholder + 1;
+	}
+
 	switch (placeholder[0]) {
 	case '-':
 		magic = DEL_LF_BEFORE_EMPTY;
Harry Jeffery· Sep 9, 2014, 22:17 UTC · re: Jeff King · lore

Re: [PATCH] pretty-format: add append line-feed format specifier

On 09/09/14 22:45, Jeff King wrote:
Show 8 quoted lines
> Yeah, that was my thought on reading the initial patch, too. Why limit
> ourselves to newlines and spaces. I'd much rather have full conditional
> expansion, like "${foo:+prefix $foo suffix}" in the shell.
>
> Something like the patch below might work, but I didn't test it very
> thoroughly (and note the comments, which might need dealing with). Maybe
> it would make a sensible base for Harry to build on if he wants to
> pursue this.

I definitely prefer your more general solution to my bare-minimum-to-scratch-itch patch. I'd certainly be willing to take your patch and expand upon it (pun unintended) once Junio has weighed in on your suggestions.

Show 7 quoted lines
> You could also make "%d" more flexible with it. We unconditionally
> include the " (...)" wrapper when expanding it. But assuming we
> introduced a "%D" that is _just_ the decoration names, you could do:
>
>    %if(%D, (%D))
>
> to get the same effect with much more flexibility.

Regardless of what happens with the conditional expansion I think it would definitely be a useful addition to be able to print the decorators without the " (...)" wrapper. I think it's general enough that it'd warrant its own separate patch rather than being part of a patch series for the conditional expansion.

Jeff King· Sep 9, 2014, 22:31 UTC · re: Harry Jeffery · lore

Re: [PATCH] pretty-format: add append line-feed format specifier

On Tue, Sep 09, 2014 at 11:17:20PM +0100, Harry Jeffery wrote:
> I definitely prefer your more general solution to my
> bare-minimum-to-scratch-itch patch. I'd certainly be willing to take your
> patch and expand upon it (pun unintended) once Junio has weighed in on your
> suggestions.

Thanks. I am always happy to see contributors willing to pick up and run with ideas.

It is probably out-of-scope for what you want, but while we are talking about %d, it may be worth considering whether there is something simple we can do to make formatting list-like items more flexible. E.g., even with "%D", you are stuck with the format "foo, bar, baz" for multiple decorations. Some kind of "%join(%d,; )" might work to produce "foo; bar; baz" (or whatever you want). But that may also be crossing the line into insanity, and we would be better to allow some Turing-complete embedded language like lua. For that matter, conditionals might be crossing that insanity line, too.

Show 5 quoted lines
> Regardless of what happens with the conditional expansion I think it would
> definitely be a useful addition to be able to print the decorators without
> the " (...)" wrapper. I think it's general enough that it'd warrant its own
> separate patch rather than being part of a patch series for the conditional
> expansion.
Yeah, I agree it can be separate.
-Peff
Junio C Hamano· Sep 10, 2014, 17:19 UTC · re: Jeff King · lore

Re: [PATCH] pretty-format: add append line-feed format specifier

Jeff King <peff@peff.net> writes:
Show 16 quoted lines
> Something like the patch below might work, but I didn't test it very
> thoroughly (and note the comments, which might need dealing with). Maybe
> it would make a sensible base for Harry to build on if he wants to
> pursue this.
>
> With it, you can do:
>
>   git log --format='%h %s%if(%d,%n  Decoration:%d)' origin
> ...
> You could also make "%d" more flexible with it. We unconditionally
> include the " (...)" wrapper when expanding it. But assuming we
> introduced a "%D" that is _just_ the decoration names, you could do:
>
>   %if(%D, (%D))
>
> to get the same effect with much more flexibility.
Yup.

I do not think we need to go overboard to support nesting and stuff, let alone turing completeness ;-), especially when we are going to test the condition part only for emptyness. Even with this simple patch, I sense that we are near a slipperly slope of wanting to add %unless(%d, ) or %ifelse(%d,%d, \(undefined\)), so I am not 100% convinced yet.

Jeff King· Sep 12, 2014, 04:49 UTC · re: Junio C Hamano · lore

Re: [PATCH] pretty-format: add append line-feed format specifier

On Wed, Sep 10, 2014 at 10:19:21AM -0700, Junio C Hamano wrote:
Show 27 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > Something like the patch below might work, but I didn't test it very
> > thoroughly (and note the comments, which might need dealing with). Maybe
> > it would make a sensible base for Harry to build on if he wants to
> > pursue this.
> >
> > With it, you can do:
> >
> >   git log --format='%h %s%if(%d,%n  Decoration:%d)' origin
> > ...
> > You could also make "%d" more flexible with it. We unconditionally
> > include the " (...)" wrapper when expanding it. But assuming we
> > introduced a "%D" that is _just_ the decoration names, you could do:
> >
> >   %if(%D, (%D))
> >
> > to get the same effect with much more flexibility.
> 
> Yup.
> 
> I do not think we need to go overboard to support nesting and stuff,
> let alone turing completeness ;-), especially when we are going to
> test the condition part only for emptyness.  Even with this simple
> patch, I sense that we are near a slipperly slope of wanting to add
> %unless(%d, ) or %ifelse(%d,%d, \(undefined\)), so I am not 100%
> convinced yet.

What compelled me is the fact that we already started down the slippery slope with %+ and friends. Providing conditionals but then only allowing certain characters seems weirdly limiting. But I guess it is all part of the same slope.

I dunno. I wrote that original set of lua pretty-format patches to try to stop the insanity once and for all. But I realized that I do not actually want to do anything complicated with the output formats, and "--oneline" and a few simple "--format" calls usually are enough. And if I do want more, I pipe it into a perl script (typically using --format to make it simple to parse).

We could also provide the data in some standard structured format like JSON to make the "pipe to your language of choice" option easier on people. But using custom --formats to do so is not that hard, and is way more efficient than dumping all of the data.

-Peff
Junio C Hamano· Sep 12, 2014, 16:36 UTC · re: Jeff King · lore

Re: [PATCH] pretty-format: add append line-feed format specifier

Jeff King <peff@peff.net> writes:
> I dunno. I wrote that original set of lua pretty-format patches to try
> to stop the insanity once and for all. But I realized that I do not
> actually want to do anything complicated with the output formats, and
> "--oneline" and a few simple "--format" calls usually are enough.

Yeah, I share that exactly, and %+ and friends are meant to be on this side of the line between "a few simple" and "complicated". I was not sure which side %if() falls myself, and while I still am not sure, if I were asked to decide which today, I would probably say it is on the "simple" side.

← back to recent threads