[PATCH] Pretty-format: Ability to add newline after non-empty string

[PATCH] Pretty-format: Ability to add newline after non-empty string

[matni403]

From: Mats Nilsson <matni403@gmail.com>

This allows for expansion of %*x to %x followed by a LF if
and only if %x is non-empty.

Signed-off-by: Mats Nilsson <matni403@gmail.com>
---
 Documentation/pretty-formats.txt |  4 ++++
 pretty.c                         | 11 +++++++++--
 t/t6006-rev-list-format.sh       |  5 +++++
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index de6953108c..cddd21e27e 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -236,6 +236,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 '*' (star) after '%' of a placeholder, a line-feed
+is added 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 0ab45d10d7..fedea05acc 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1457,7 +1457,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]) {
@@ -1470,6 +1471,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;
 	}
@@ -1492,6 +1496,8 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */
 			strbuf_insert(sb, orig_len, "\n", 1);
 		else if (magic == ADD_SP_BEFORE_NON_EMPTY)
 			strbuf_insert(sb, orig_len, " ", 1);
+		else if (magic == ADD_LF_AFTER_NON_EMPTY)
+			strbuf_addstr(sb, "\n");
 	}
 	return consumed + 1;
 }
@@ -1501,7 +1507,8 @@ 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) {
diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index da113d975b..e333ed91d3 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh
@@ -445,6 +445,11 @@ test_expect_success 'add SP before non-empty (2)' '
 	test $(wc -w <actual) = 6
 '
 
+test_expect_success 'add LF after non-empty' '
+	git show -s --pretty=format:"%s%*sThanks" HEAD^^ >actual &&
+	test_line_count = 2 actual
+'
+
 test_expect_success '--abbrev' '
 	echo SHORT SHORT SHORT >expect2 &&
 	echo LONG LONG LONG >expect3 &&
-- 
2.14.1.windows.1

Re: [PATCH] Pretty-format: Ability to add newline after non-empty string

[Junio C Hamano]

matni403@gmail.com writes:

> Subject: Re: [PATCH] Pretty-format: Ability to add newline after non-empty string

Downcasing 'P' and 'A' would make this fit better when it appears in
the "git shortlog --no-merges" output, I think.  Or perhaps

	[PATCH] pretty: allow to add LF only after non-empty string

or something may fit even better.

> diff --git a/pretty.c b/pretty.c
> index 0ab45d10d7..fedea05acc 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1457,7 +1457,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

Appending at the end in this case does not make less sense than
inserting at the right place in the middle.  Noticing that earlier
ones are all about LF, perhaps

	NO_MAGIC
	ADD_LF_BEFORE_NON_EMPTY
	ADD_LF_AFTER_NON_EMPTY
	DEL_LF_BEFORE_EMPTY
	ADD_SP_BEFORE_NON_EMPTY

may make more sense?  

An obvious question that would come to the reader's mind would be if
we also want ADD_SP_AFTER and DEL_SP_BEFORE for completeness, once
the list is ordered in a more logical way like we see above.

I am not suggesting to add these two right now, but we still must
think about them before adding this new one, because it would affect
the choice of the letter used for this new one.  It is irresponsible
to say "I do not need it, so somebody else can add them later",
without making sure that somebody else can later choose sensible
letters that make the parallel between those two they are adding
with the existing ones.

We have '+' and '-' as a matching pair that adds or deletes a LF
before the expansion, and we are adding '*' to that group to make
the repertoire <add before non-empty, del before empty, add after
non-empty>.  On the SP side, we currently only use ' ', whose
counterpart on the LF side is '+'.  Can we easily find counterparts
for '-' and this new '*' for the SP side, when we want to add "add
after non-empty" and "delete before empty", or would it become
easier if we chose something other than '*', and if so what letter?

> @@ -1501,7 +1507,8 @@ 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) {
> diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
> index da113d975b..e333ed91d3 100755
> --- a/t/t6006-rev-list-format.sh
> +++ b/t/t6006-rev-list-format.sh
> @@ -445,6 +445,11 @@ test_expect_success 'add SP before non-empty (2)' '
>  	test $(wc -w <actual) = 6
>  '
>  
> +test_expect_success 'add LF after non-empty' '
> +	git show -s --pretty=format:"%s%*sThanks" HEAD^^ >actual &&

Here the subject is expanded, LF is added, and then the subject is
expanded _again_ before 'Thanks'.  That does not sound like a
realistic example that shows off the situation in which this new
feature becomes useful.

> +	test_line_count = 2 actual
> +'
> +
>  test_expect_success '--abbrev' '
>  	echo SHORT SHORT SHORT >expect2 &&
>  	echo LONG LONG LONG >expect3 &&

Thanks.

Re: [PATCH] Pretty-format: Ability to add newline after non-empty string

[Mats Nilsson]

Den tors 7 feb. 2019 kl 18:29 skrev Junio C Hamano <gitster@pobox.com>:
>
> matni403@gmail.com writes:
>
> > Subject: Re: [PATCH] Pretty-format: Ability to add newline after non-empty string
>
> Downcasing 'P' and 'A' would make this fit better when it appears in
> the "git shortlog --no-merges" output, I think.  Or perhaps
>
>         [PATCH] pretty: allow to add LF only after non-empty string
>
> or something may fit even better.
>

Will fix this is the updated patch!

> > diff --git a/pretty.c b/pretty.c
> > index 0ab45d10d7..fedea05acc 100644
> > --- a/pretty.c
> > +++ b/pretty.c
> > @@ -1457,7 +1457,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
>
> Appending at the end in this case does not make less sense than
> inserting at the right place in the middle.  Noticing that earlier
> ones are all about LF, perhaps
>
>         NO_MAGIC
>         ADD_LF_BEFORE_NON_EMPTY
>         ADD_LF_AFTER_NON_EMPTY
>         DEL_LF_BEFORE_EMPTY
>         ADD_SP_BEFORE_NON_EMPTY
>
> may make more sense?
>

I though a bit about the placement, my reasoning was to have the
"before"-magic and then the "after"-magic, but it is an easy change.

> An obvious question that would come to the reader's mind would be if
> we also want ADD_SP_AFTER and DEL_SP_BEFORE for completeness, once
> the list is ordered in a more logical way like we see above.
>
> I am not suggesting to add these two right now, but we still must
> think about them before adding this new one, because it would affect
> the choice of the letter used for this new one.  It is irresponsible
> to say "I do not need it, so somebody else can add them later",
> without making sure that somebody else can later choose sensible
> letters that make the parallel between those two they are adding
> with the existing ones.
>

I think I could pretty easily implement those two as well while I am
doing this, the code is all there already.

> We have '+' and '-' as a matching pair that adds or deletes a LF
> before the expansion, and we are adding '*' to that group to make
> the repertoire <add before non-empty, del before empty, add after
> non-empty>.  On the SP side, we currently only use ' ', whose
> counterpart on the LF side is '+'.  Can we easily find counterparts
> for '-' and this new '*' for the SP side, when we want to add "add
> after non-empty" and "delete before empty", or would it become
> easier if we chose something other than '*', and if so what letter?
>

I don't have a good answer to this. Maybe , and . can be used as a pair?
I'm all ears regarding the choice of letter

> > @@ -1501,7 +1507,8 @@ 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) {
> > diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
> > index da113d975b..e333ed91d3 100755
> > --- a/t/t6006-rev-list-format.sh
> > +++ b/t/t6006-rev-list-format.sh
> > @@ -445,6 +445,11 @@ test_expect_success 'add SP before non-empty (2)' '
> >       test $(wc -w <actual) = 6
> >  '
> >
> > +test_expect_success 'add LF after non-empty' '
> > +     git show -s --pretty=format:"%s%*sThanks" HEAD^^ >actual &&
>
> Here the subject is expanded, LF is added, and then the subject is
> expanded _again_ before 'Thanks'.  That does not sound like a
> realistic example that shows off the situation in which this new
> feature becomes useful.
>

My problem, and why I wanted to add this feature comes from when I googled
and found the following stackoverflow question which sums up my problem as well:
https://stackoverflow.com/questions/34829747/git-log-pretty-format-newline-after-placeholder-if-non-empty
This is the similar to what I wanted for my log, if %d is non-empty,
add a newline after the placeholder.
But I didn't know have to write a test which would simulate this since
all the tests around the previous LFs
just did git log, which would make the refs move around?

The easiest case I could think of was the to make sure a placeholder
added a newline after expansion.
Though it worked on my machine, there must be some problem with it
since the build failed.
https://dev.azure.com/gitgitgadget/git/_build/results?buildId=1082
https://travis-ci.org/mnil/git/builds/489689952
I'm not sure how to debug it when it works on my local setup...

> > +     test_line_count = 2 actual
> > +'
> > +
> >  test_expect_success '--abbrev' '
> >       echo SHORT SHORT SHORT >expect2 &&
> >       echo LONG LONG LONG >expect3 &&
>
> Thanks.

Kind Regards