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

Harry Jeffery [off-list ref] writes:

On 09/09/14 20:15, Junio C Hamano wrote:

quoted

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

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

Harry Jeffery [off-list ref] writes:

quoted

On 09/09/14 20:15, Junio C Hamano wrote:

quoted

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;

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.

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