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

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

[Harry Jeffery]

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

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

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

[Junio C Hamano]

Harry Jeffery <harry@exec64.co.uk> writes:

> 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"?

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

[Harry Jeffery]

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.

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

[Junio C Hamano]

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

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

[Jeff King]

On Tue, Sep 09, 2014 at 12:37:48PM -0700, Junio C Hamano wrote:

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

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

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

[Harry Jeffery]

On 09/09/14 22:45, Jeff King wrote:
> 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.

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

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

[Jeff King]

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.

> 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

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

[Junio C Hamano]

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.

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

[Jeff King]

On Wed, Sep 10, 2014 at 10:19:21AM -0700, Junio C Hamano wrote:

> 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

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

[Junio C Hamano]

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.