threads / patch / 20995

patchgit-log --format: Add %B tag with %B(x) option

Subject: [PATCH] git-log --format: Add %B tag with %B(x) option

## tl;dr

18 messages between Sep 17, 2009 and Oct 10, 2009. Diffs are folded; open one to read it.

replies: 17people: 4as markdown or json

Johannes Gilger· Sep 17, 2009, 22:47 UTC · lore

Since one can simply use spaces to indent any other --pretty field we should have an option to do that with the body too.

Also the %B flag strips the trailing newlines, to enable more compact display.

Signed-off-by: Johannes Gilger <heipei@hackvalue.de>
---
Hey list,

in my never-ending quest to beautify my personal log output I just whipped this up. Maybe you like it too or at least can tell me what's wrong with it ;)

Please CC me as I'm not on the list anymore (but keep up through Gmane though).
 Documentation/pretty-formats.txt |    2 ++
 pretty.c                         |   13 +++++++++++++
 2 files changed, 15 insertions(+), 0 deletions(-)
Show changes to 2 files +15 −0

Documentation/pretty-formats.txt, pretty.c

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 2a845b1..c04f118 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -123,6 +123,8 @@ The placeholders are:
 - '%s': subject
 - '%f': sanitized subject line, suitable for a filename
 - '%b': body
+- '%B': body without trailing newline
+- '%B(x)': body indented with x spaces
 - '%Cred': switch color to red
 - '%Cgreen': switch color to green
 - '%Cblue': switch color to blue
diff --git a/pretty.c b/pretty.c
index f5983f8..6d530e1 100644
--- a/pretty.c
+++ b/pretty.c
@@ -735,6 +735,19 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 	case 'b':	/* body */
 		strbuf_addstr(sb, msg + c->body_off);
 		return 1;
+	case 'B':
+		if (placeholder[1] == '(') {
+			const char *body = msg + c->body_off;
+			const char *end = strchr(placeholder + 2, ')');
+			if(!end)
+				return 0;
+			pp_remainder(CMIT_FMT_MEDIUM, &body, sb, atoi(placeholder + 2));
+			strbuf_rtrim(sb);
+			return end - placeholder + 1;
+		}
+		strbuf_addstr(sb, msg + c->body_off);
+		strbuf_rtrim(sb);
+		return 1;
 	}
 	return 0;	/* unknown placeholder */
 }

-- 
1.6.5.rc1.20.geb7d9
Junio C Hamano· Sep 17, 2009, 23:27 UTC · re: Johannes Gilger · lore

Re: [PATCH] git-log --format: Add %B tag with %B(x) option

Johannes Gilger <heipei@hackvalue.de> writes:
Show 10 quoted lines
> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> index 2a845b1..c04f118 100644
> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -123,6 +123,8 @@ The placeholders are:
>  - '%s': subject
>  - '%f': sanitized subject line, suitable for a filename
>  - '%b': body
> +- '%B': body without trailing newline
> +- '%B(x)': body indented with x spaces
First the design issues.

Because this will set a precedent for possible future formatting features that take optional parameters, we need to pick the syntax carefully not only for this feature but for the later ones that we haven't invented yet.

Let's say that pair of parentheses is a good choice and if later features want to take more than one, it would be reasonable for them to use a comma-separated list, e.g. %Q(1,2,3).

I wonder if it is reasonable to invoke print_wrapped_text(), not just limit this feature to indenting.

Now, let's look at the implementation.
Show 14 quoted lines
> diff --git a/pretty.c b/pretty.c
> index f5983f8..6d530e1 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -735,6 +735,19 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
>  	case 'b':	/* body */
>  		strbuf_addstr(sb, msg + c->body_off);
>  		return 1;
> +	case 'B':
> +		if (placeholder[1] == '(') {
> +			const char *body = msg + c->body_off;
> +			const char *end = strchr(placeholder + 2, ')');
> +			if(!end)
> +				return 0;
Style: "if (!end)"

I'd sleep better if the syntax checking is done as a separate phase way before this in the codepath.

> +			pp_remainder(CMIT_FMT_MEDIUM, &body, sb, atoi(placeholder + 2));
What happens when atoi() fails, or %B(12Q) was given?
We tend to use strto[u]l when parsing integers and check for errors.
Johannes Gilger· Sep 18, 2009, 18:00 UTC · re: Junio C Hamano · lore

[PATCHv2] git-log --format: Add %B tag with %B(x) option

Since one can simply use spaces to indent any other --pretty field we should have an option to do that with the body too.

Also the %B flag strips the trailing newlines, to enable more compact display.

Signed-off-by: Johannes Gilger <heipei@hackvalue.de>
---
Hi again,

I followed Junio's implementation-advice. Since we have two placeholders which take () arguments I put the scanning for those at the top, to avoid repetition. I used strtol in place of atoi and I also had to to add a check for ident > 0 since the indent determines the growth-size of the buffer, and negative values produced nasty stuff (obviously).

As for general extendability: The current code deals with %B(42) as well as %B(42,23[,...]), so even old versions could be used with "new" pretty-formats. %B(c5) is simply no ident, while %B(5c) is 5 spaces indent. Don't know if this is unwanted behaviour, but that's what strtol gives us.

Dscho sent me a pointer to a patch [1], which not only adds indent but also rewrapping. But since this is my second patch and Dscho's patch depended on two other patches I didn't want to get in over my head by making his patches a prerequisite.

A last word on future formats: We can use (x,y,z) easily, another thing one might think of (or at least I do) is using an %an[20] syntax, returning only the first 20 chars of %an, so one can make onelined outputs nicely column-aligned for fields like the author.

Greetings, Jojo

[1] - http://repo.or.cz/w/git/dscho.git?a=commit;h=ad48dfca58169c35e227e135638b4970fe4dc9a5
 Documentation/pretty-formats.txt |    2 ++
 pretty.c                         |   25 ++++++++++++++++++++-----
 2 files changed, 22 insertions(+), 5 deletions(-)
Show changes to 2 files +22 −5

Documentation/pretty-formats.txt, pretty.c

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 2a845b1..533bc5e 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -123,6 +123,8 @@ The placeholders are:
 - '%s': subject
 - '%f': sanitized subject line, suitable for a filename
 - '%b': body
+- '%B': body without trailing newline
+- '%B(x)': body indented by x spaces
 - '%Cred': switch color to red
 - '%Cgreen': switch color to green
 - '%Cblue': switch color to blue
diff --git a/pretty.c b/pretty.c
index f5983f8..7b88827 100644
--- a/pretty.c
+++ b/pretty.c
@@ -605,13 +605,17 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 	int h1, h2;
 
 	/* these are independent of the commit */
+
+	const char *body = msg + c->body_off;
+	const char *end = NULL;
+	/* check if we have arguments to the placeholder */
+	if (placeholder[1] == '(')
+		end = strchr(placeholder + 2, ')');
+
 	switch (placeholder[0]) {
 	case 'C':
-		if (placeholder[1] == '(') {
-			const char *end = strchr(placeholder + 2, ')');
+		if (end) {
 			char color[COLOR_MAXLEN];
-			if (!end)
-				return 0;
 			color_parse_mem(placeholder + 2,
 					end - (placeholder + 2),
 					"--pretty format", color);
@@ -733,7 +737,16 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 		format_sanitized_subject(sb, msg + c->subject_off);
 		return 1;
 	case 'b':	/* body */
-		strbuf_addstr(sb, msg + c->body_off);
+		strbuf_addstr(sb, body);
+		return 1;
+	case 'B':	/* body without trailing newline */
+		if (end) {
+			pp_remainder(CMIT_FMT_MEDIUM, &body, sb, strtol(placeholder + 2, NULL, 10));
+			strbuf_rtrim(sb);
+			return end - placeholder + 1;
+		}
+		strbuf_addstr(sb, body);
+		strbuf_rtrim(sb);
 		return 1;
 	}
 	return 0;	/* unknown placeholder */
@@ -875,6 +888,8 @@ void pp_remainder(enum cmit_fmt fmt,
 		}
 		first = 0;
 
+		if (indent < 0)
+			indent = 0;
 		strbuf_grow(sb, linelen + indent + 20);
 		if (indent) {
 			memset(sb->buf + sb->len, ' ', indent);

-- 
1.6.5.rc1.20.geb7d9
Junio C Hamano· Sep 18, 2009, 19:12 UTC · re: Johannes Gilger · lore

Re: [PATCHv2] git-log --format: Add %B tag with %B(x) option

Johannes Gilger <heipei@hackvalue.de> writes:
> %B(c5) is simply no ident, while %B(5c) is 5 spaces indent. Don't know if this 
> is unwanted behaviour, but that's what strtol gives us.
Not really.

"strto[u]l(arg, &endp, 10)" parses arg as a decimal and moves "char *endp" to point at where the number ended, so you can tell things like:

 - return value of 0 with (arg == endp) being "an empty input, the user
   did not necessarily meant zero"; or
 - (*endp != '\0') being "some garbage after the number".
Johannes Gilger· Sep 19, 2009, 09:58 UTC · re: Junio C Hamano · lore

[PATCHv3] git-log --format: Add %B tag with %B(x) option

Since one can simply use spaces to indent any other --pretty field we should have an option to do that with the body too.

Also the %B flag strips the trailing newlines, to enable more compact display.

Signed-off-by: Johannes Gilger <heipei@hackvalue.de>
---
Changes to PATCHv2:
- Make %B() strict: Only nonnegative integers are allowed between the brackets, 
  everything else yields the placemark itself as output to indicate a wrong 
  argument. This also goes for an empty argument.
 Documentation/pretty-formats.txt |    2 ++
 pretty.c                         |   29 ++++++++++++++++++++++++-----
 2 files changed, 26 insertions(+), 5 deletions(-)
Show changes to 2 files +26 −5

Documentation/pretty-formats.txt, pretty.c

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 2a845b1..533bc5e 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -123,6 +123,8 @@ The placeholders are:
 - '%s': subject
 - '%f': sanitized subject line, suitable for a filename
 - '%b': body
+- '%B': body without trailing newline
+- '%B(x)': body indented by x spaces
 - '%Cred': switch color to red
 - '%Cgreen': switch color to green
 - '%Cblue': switch color to blue
diff --git a/pretty.c b/pretty.c
index f5983f8..8970378 100644
--- a/pretty.c
+++ b/pretty.c
@@ -605,13 +605,17 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 	int h1, h2;
 
 	/* these are independent of the commit */
+
+	const char *body = msg + c->body_off;
+	const char *end = NULL;
+	/* check if we have arguments to the placeholder */
+	if (placeholder[1] == '(')
+		end = strchr(placeholder + 2, ')');
+
 	switch (placeholder[0]) {
 	case 'C':
-		if (placeholder[1] == '(') {
-			const char *end = strchr(placeholder + 2, ')');
+		if (end) {
 			char color[COLOR_MAXLEN];
-			if (!end)
-				return 0;
 			color_parse_mem(placeholder + 2,
 					end - (placeholder + 2),
 					"--pretty format", color);
@@ -733,7 +737,20 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 		format_sanitized_subject(sb, msg + c->subject_off);
 		return 1;
 	case 'b':	/* body */
-		strbuf_addstr(sb, msg + c->body_off);
+		strbuf_addstr(sb, body);
+		return 1;
+	case 'B':	/* body without trailing newline */
+		if (end) {
+			char *endp = NULL;
+			int indent = strtol(placeholder + 2, &endp, 10);
+			if (placeholder + 2 == endp || *endp != ')' || indent < 0)
+				return 0;
+			pp_remainder(CMIT_FMT_MEDIUM, &body, sb, indent);
+			strbuf_rtrim(sb);
+			return end - placeholder + 1;
+		}
+		strbuf_addstr(sb, body);
+		strbuf_rtrim(sb);
 		return 1;
 	}
 	return 0;	/* unknown placeholder */
@@ -875,6 +892,8 @@ void pp_remainder(enum cmit_fmt fmt,
 		}
 		first = 0;
 
+		if (indent < 0)
+			indent = 0;
 		strbuf_grow(sb, linelen + indent + 20);
 		if (indent) {
 			memset(sb->buf + sb->len, ' ', indent);

-- 
1.6.5.rc1.20.geb7d9
Junio C Hamano· Sep 22, 2009, 19:41 UTC · re: Johannes Gilger · lore

Re: [PATCHv3] git-log --format: Add %B tag with %B(x) option

Johannes Gilger <heipei@hackvalue.de> writes:
Show 49 quoted lines
> Since one can simply use spaces to indent any other --pretty field we
> should have an option to do that with the body too.
>
> Also the %B flag strips the trailing newlines, to enable more compact
> display.
>
> Signed-off-by: Johannes Gilger <heipei@hackvalue.de>
> ---
> Changes to PATCHv2:
> - Make %B() strict: Only nonnegative integers are allowed between the brackets, 
>   everything else yields the placemark itself as output to indicate a wrong 
>   argument. This also goes for an empty argument.
>
>  Documentation/pretty-formats.txt |    2 ++
>  pretty.c                         |   29 ++++++++++++++++++++++++-----
>  2 files changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> index 2a845b1..533bc5e 100644
> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -733,7 +737,20 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
>  		format_sanitized_subject(sb, msg + c->subject_off);
>  		return 1;
>  	case 'b':	/* body */
> -		strbuf_addstr(sb, msg + c->body_off);
> +		strbuf_addstr(sb, body);
> +		return 1;
> +	case 'B':	/* body without trailing newline */
> +		if (end) {
> +			char *endp = NULL;
> +			int indent = strtol(placeholder + 2, &endp, 10);
> +			if (placeholder + 2 == endp || *endp != ')' || indent < 0)
> +				return 0;
> +			pp_remainder(CMIT_FMT_MEDIUM, &body, sb, indent);
> +			strbuf_rtrim(sb);
> +			return end - placeholder + 1;
> +		}
> +		strbuf_addstr(sb, body);
> +		strbuf_rtrim(sb);
>  		return 1;
>  	}
>  	return 0;	/* unknown placeholder */
> @@ -875,6 +892,8 @@ void pp_remainder(enum cmit_fmt fmt,
>  		}
>  		first = 0;
>  
> +		if (indent < 0)
> +			indent = 0;

I'd move this check to the caller; other than that and some other small style issues I think this round is Ok.

Show 5 quoted lines
>  		strbuf_grow(sb, linelen + indent + 20);
>  		if (indent) {
>  			memset(sb->buf + sb->len, ' ', indent);
> -- 
> 1.6.5.rc1.20.geb7d9
Johannes Gilger· Sep 22, 2009, 21:30 UTC · re: Junio C Hamano · lore

[PATCHv4] git-log --format: Add %B tag with %B(n) option

Since one can simply use spaces to indent any other --pretty field we should have an option to do that with the body too.

Also the %B flag strips the trailing newlines, to enable more compact display.

Signed-off-by: Johannes Gilger <heipei@hackvalue.de>
---
Hey,

I moved the indent >= 0 check to the caller. Also changed the documentation, n should indicate more strongly that n is supposed to be a natural number.

You mentioned small style issues but I'm not sure what you meant. One thing that could be made more compact is calling pp_remainder(CMIT_FMT_MEDIUM, &body, sb, indent < 0 ? 0 : indent); and thereby saving two extra lines. I saw this at a lot of other spaces in git.git, but saw no specific guideline in CodingGuidelines.

Yet another option is aborting for negative indent values, issuing return 0;

Greetings, Jojo

 Documentation/pretty-formats.txt |    2 ++
 pretty.c                         |   29 ++++++++++++++++++++++++-----
 2 files changed, 26 insertions(+), 5 deletions(-)
Show changes to 2 files +26 −5

Documentation/pretty-formats.txt, pretty.c

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 2a845b1..ca694c9 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -123,6 +123,8 @@ The placeholders are:
 - '%s': subject
 - '%f': sanitized subject line, suitable for a filename
 - '%b': body
+- '%B': body without trailing newline
+- '%B(n)': %B indented by n spaces
 - '%Cred': switch color to red
 - '%Cgreen': switch color to green
 - '%Cblue': switch color to blue
diff --git a/pretty.c b/pretty.c
index f5983f8..dafa8e0 100644
--- a/pretty.c
+++ b/pretty.c
@@ -605,13 +605,17 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 	int h1, h2;
 
 	/* these are independent of the commit */
+
+	const char *body = msg + c->body_off;
+	const char *end = NULL;
+	/* check if we have arguments to the placeholder */
+	if (placeholder[1] == '(')
+		end = strchr(placeholder + 2, ')');
+
 	switch (placeholder[0]) {
 	case 'C':
-		if (placeholder[1] == '(') {
-			const char *end = strchr(placeholder + 2, ')');
+		if (end) {
 			char color[COLOR_MAXLEN];
-			if (!end)
-				return 0;
 			color_parse_mem(placeholder + 2,
 					end - (placeholder + 2),
 					"--pretty format", color);
@@ -733,7 +737,22 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 		format_sanitized_subject(sb, msg + c->subject_off);
 		return 1;
 	case 'b':	/* body */
-		strbuf_addstr(sb, msg + c->body_off);
+		strbuf_addstr(sb, body);
+		return 1;
+	case 'B':	/* body without trailing newline */
+		if (end) {
+			char *endp = NULL;
+			int indent = strtol(placeholder + 2, &endp, 10);
+			if (placeholder + 2 == endp || *endp != ')')
+				return 0;
+			if (indent < 0)
+				indent = 0;
+			pp_remainder(CMIT_FMT_MEDIUM, &body, sb, indent);
+			strbuf_rtrim(sb);
+			return end - placeholder + 1;
+		}
+		strbuf_addstr(sb, body);
+		strbuf_rtrim(sb);
 		return 1;
 	}
 	return 0;	/* unknown placeholder */

-- 
1.6.5.rc1.38.g1fbd3
Johannes Schindelin· Sep 23, 2009, 20:34 UTC · re: Johannes Gilger · lore

[PATCH 0/3] Add a pretty format to rewrapping/indenting commit messages

I needed that a long time ago, so I implemented it and let it rut in my personal tree.

Maybe somebody else wants to benefit from my work, too.
Johannes Schindelin (3):
  print_wrapped_text(): allow hard newlines
  Add strbuf_add_wrapped_text() to utf8.[ch]
  Add "%w" to pretty formats, which rewraps the commit message
 Documentation/pretty-formats.txt |    1 +
 pretty.c                         |   27 ++++++++++++++++++++++
 utf8.c                           |   45 ++++++++++++++++++++++++++++++-------
 utf8.h                           |    2 +
 4 files changed, 66 insertions(+), 9 deletions(-)
Johannes Schindelin· Sep 23, 2009, 20:34 UTC · re: Johannes Schindelin · lore

[PATCH 1/3] print_wrapped_text(): allow hard newlines

print_wrapped_text() will insert its own newlines. Up until now, if the text passed to it contained newlines, they would not be handled properly (the wrapping got confused after that).

The strategy is to replace a single new-line with a space, but keep double new-lines so that already-wrapped text with empty lines between paragraphs will be handled properly.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 utf8.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)
Show changes to utf8.c +14 −2
diff --git a/utf8.c b/utf8.c
index db706ac..589876b 100644
--- a/utf8.c
+++ b/utf8.c
@@ -310,6 +310,8 @@ int print_wrapped_text(const char *text, int indent, int indent2, int width)
 		if (!c || isspace(c)) {
 			if (w < width || !space) {
 				const char *start = bol;
+				if (!c && text == start)
+					return w;
 				if (space)
 					start = space;
 				else
@@ -317,13 +319,23 @@ int print_wrapped_text(const char *text, int indent, int indent2, int width)
 				fwrite(start, text - start, 1, stdout);
 				if (!c)
 					return w;
-				else if (c == '\t')
-					w |= 0x07;
 				space = text;
+				if (c == '\t')
+					w |= 0x07;
+				else if (c == '\n') {
+					space++;
+					if (*space == '\n') {
+						putchar('\n');
+						goto new_line;
+					}
+					else
+						putchar(' ');
+				}
 				w++;
 				text++;
 			}
 			else {
+new_line:
 				putchar('\n');
 				text = bol = space + isspace(*space);
 				space = NULL;

-- 
1.6.4.297.gcb4cc
Johannes Schindelin· Sep 23, 2009, 20:34 UTC · re: Johannes Schindelin · lore

[PATCH 2/3] Add strbuf_add_wrapped_text() to utf8.[ch]

The newly added function can rewrap text according to a given first-line indent, other-indent and text width.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 utf8.c |   33 ++++++++++++++++++++++++---------
 utf8.h |    2 ++
 2 files changed, 26 insertions(+), 9 deletions(-)
Show changes to 2 files +26 −9

utf8.c, utf8.h

diff --git a/utf8.c b/utf8.c
index 589876b..7383a70 100644
--- a/utf8.c
+++ b/utf8.c
@@ -1,4 +1,5 @@
 #include "git-compat-util.h"
+#include "strbuf.h"
 #include "utf8.h"
 
 /* This code is originally from http://www.cl.cam.ac.uk/~mgk25/ucs/ */
@@ -279,14 +280,22 @@ int is_utf8(const char *text)
 	return 1;
 }
 
-static void print_spaces(int count)
+static inline void strbuf_write(struct strbuf *sb, const char *buf, int len)
+{
+	if (sb)
+		strbuf_insert(sb, sb->len, buf, len);
+	else
+		fwrite(buf, len, 1, stdout);
+}
+
+static void print_spaces(struct strbuf *buf, int count)
 {
 	static const char s[] = "                    ";
 	while (count >= sizeof(s)) {
-		fwrite(s, sizeof(s) - 1, 1, stdout);
+		strbuf_write(buf, s, sizeof(s) - 1);
 		count -= sizeof(s) - 1;
 	}
-	fwrite(s, count, 1, stdout);
+	strbuf_write(buf, s, count);
 }
 
 /*
@@ -295,7 +304,8 @@ static void print_spaces(int count)
  * If indent is negative, assume that already -indent columns have been
  * consumed (and no extra indent is necessary for the first line).
  */
-int print_wrapped_text(const char *text, int indent, int indent2, int width)
+int strbuf_add_wrapped_text(struct strbuf *buf,
+		const char *text, int indent, int indent2, int width)
 {
 	int w = indent, assume_utf8 = is_utf8(text);
 	const char *bol = text, *space = NULL;
@@ -315,8 +325,8 @@ int print_wrapped_text(const char *text, int indent, int indent2, int width)
 				if (space)
 					start = space;
 				else
-					print_spaces(indent);
-				fwrite(start, text - start, 1, stdout);
+					print_spaces(buf, indent);
+				strbuf_write(buf, start, text - start);
 				if (!c)
 					return w;
 				space = text;
@@ -325,18 +335,18 @@ int print_wrapped_text(const char *text, int indent, int indent2, int width)
 				else if (c == '\n') {
 					space++;
 					if (*space == '\n') {
-						putchar('\n');
+						strbuf_write(buf, "\n", 1);
 						goto new_line;
 					}
 					else
-						putchar(' ');
+						strbuf_write(buf, " ", 1);
 				}
 				w++;
 				text++;
 			}
 			else {
 new_line:
-				putchar('\n');
+				strbuf_write(buf, "\n", 1);
 				text = bol = space + isspace(*space);
 				space = NULL;
 				w = indent = indent2;
@@ -352,6 +362,11 @@ new_line:
 	}
 }
 
+int print_wrapped_text(const char *text, int indent, int indent2, int width)
+{
+	return strbuf_add_wrapped_text(NULL, text, indent, indent2, width);
+}
+
 int is_encoding_utf8(const char *name)
 {
 	if (!name)
diff --git a/utf8.h b/utf8.h
index 2f1b14f..ae30ae4 100644
--- a/utf8.h
+++ b/utf8.h
@@ -10,6 +10,8 @@ int is_utf8(const char *text);
 int is_encoding_utf8(const char *name);
 
 int print_wrapped_text(const char *text, int indent, int indent2, int len);
+int strbuf_add_wrapped_text(struct strbuf *buf,
+		const char *text, int indent, int indent2, int width);
 
 #ifndef NO_ICONV
 char *reencode_string(const char *in, const char *out_encoding, const char *in_encoding);

-- 
1.6.4.297.gcb4cc
Johannes Schindelin· Sep 23, 2009, 20:34 UTC · re: Johannes Schindelin · lore

[PATCH 3/3] Add "%w" to pretty formats, which rewraps the commit message

Some repositories contain commit messages that are insanely long. And not always is it an option to rewrite the commits with rewrapped commit messages; just think of cvsimports or some such.

Now here is a remedy.

With "--pretty=format:%w(8,6,70)" you will get the commit messages reformatted to width 70 where the first line has indent 8 and the subsequent lines have indent 6.

The following command will output something similar to plain "git log --color", except that the commit bodies will be rewrapped to fit inside 80 columns:

	git log --pretty=format:'%C(yellow)commit %H%C(reset)
	Author: %an <%ae>
	Date: %ad
	%w(4,4,80)'
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/pretty-formats.txt |    1 +
 pretty.c                         |   27 +++++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 0 deletions(-)
Show changes to 2 files +28 −0

Documentation/pretty-formats.txt, pretty.c

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 2a845b1..d727995 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -123,6 +123,7 @@ The placeholders are:
 - '%s': subject
 - '%f': sanitized subject line, suitable for a filename
 - '%b': body
+- '%w(indent,indent2,width)': rewrapped subject and body
 - '%Cred': switch color to red
 - '%Cgreen': switch color to green
 - '%Cblue': switch color to blue
diff --git a/pretty.c b/pretty.c
index f5983f8..639469e 100644
--- a/pretty.c
+++ b/pretty.c
@@ -595,6 +595,30 @@ static void format_decoration(struct strbuf *sb, const struct commit *commit)
 		strbuf_addch(sb, ')');
 }
 
+static int rewrap_text(struct strbuf *sb, const char *params, const char *text)
+{
+	int indent = 4, indent2 = 4, width = 80, pacmanned = 1;
+
+	if (params[0] == '(') {
+		char *p;
+
+		indent = strtoul(params + 1, &p, 0);
+		if (*p == ',') {
+			indent2 = strtoul(p + 1, &p, 0);
+			if (*p == ',')
+				width = strtoul(p + 1, &p, 0);
+		}
+		if (*p != ')') {
+			error ("Invalid parameters: %.*s",
+					(int)(p - params), params);
+			return 0;
+		}
+		pacmanned = p + 1 - params;
+	}
+	strbuf_add_wrapped_text(sb, text, indent, indent2, width);
+	return pacmanned;
+}
+
 static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
                                void *context)
 {
@@ -735,6 +759,9 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 	case 'b':	/* body */
 		strbuf_addstr(sb, msg + c->body_off);
 		return 1;
+	case 'w':	/* re-wrapped body */
+		return rewrap_text(sb, placeholder + 1,
+				msg + c->subject_off) + 1;
 	}
 	return 0;	/* unknown placeholder */
 }

-- 
1.6.4.297.gcb4cc
Johannes Gilger· Sep 23, 2009, 21:00 UTC · re: Johannes Schindelin · lore

Re: [PATCH 3/3] Add "%w" to pretty formats, which rewraps the commit message

On 23/09/09 22:34, Johannes Schindelin wrote:
> With "--pretty=format:%w(8,6,70)" you will get the commit messages
> reformatted to width 70 where the first line has indent 8 and the
> subsequent lines have indent 6.
Hey Johannes,

you sent me your patches earlier (and I really liked the idea and could use it too, especially with svn-mindset people). One thing that bothers me about the %w flag is that is includes subject and body, when we already have atoms for both of these flags. So having a subject(x,y) and body(x,y) tag (where x is indent and y is textwidth to be rewrapped) would be nicer and more in the spirit of the existing format options imho.

Having said that I also have to acknowledge a clear advantage of your patch, which is that one doesn't need to clear trailing newlines when the subject is < wrapwidth and the body is empty (and one used %s(x,y)%n%n%b(x,y) as a format-tag). With my %B, %B(n) patch which is on pu (and which you should probably consider in case it gets into next ;) I do this by calling strbuf_rtrim after adding the body.

Greetings, Jojo

-- 
Johannes Gilger <heipei@hackvalue.de>
http://heipei.net
GPG-Key: 0x42F6DE81
GPG-Fingerprint: BB49 F967 775E BB52 3A81  882C 58EE B178 42F6 DE81
Junio C Hamano· Sep 23, 2009, 23:19 UTC · re: Johannes Gilger · lore

Re: [PATCH 3/3] Add "%w" to pretty formats, which rewraps the commit message

Johannes Gilger <heipei@hackvalue.de> writes:
Show 6 quoted lines
> ... One thing that bothers 
> me about the %w flag is that is includes subject and body, when we 
> already have atoms for both of these flags. So having a subject(x,y) and 
> body(x,y) tag (where x is indent and y is textwidth to be rewrapped) 
> would be nicer and more in the spirit of the existing format options 
> imho.

I do not particularly like this %w() either, and would prefer to see an equivalent solution using combination of %S(i,j,w) and %B(i,j,w). Your %B(n) can be extended to do the same as Johannes's wrapping variant when given three parameters and you can trivially do the same for %s to produce %S(n) and %S(i,j,w).

One issue %w() sidesteps is handing of single liner commit log messages (this is not a new issue your %B(n) introduces). "%s%n%b" will give us the original message only when the log has some contents in addition to the single-line summary. Otherwise we will get an extra blank line.

Perhaps we could extend the pretty-printer so that it understands %+x notation, which expands to %n%x when %x expands to a non-empty result, and otherwise it expands to empty, as a generic extension applicable to any format specifier 'x'. If we have such a notation, "%s%+b", would be a reasonable way to say "give us the original commit log message here", and we won't need %w(i,j,w) -- we can instead say %S(i,j,w)%+B(i,j,w), or %s%+B(i,j,w) depending on what you want.

Junio C Hamano· Oct 5, 2009, 06:25 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/3] Add "%w" to pretty formats, which rewraps the commit message

Junio C Hamano <gitster@pobox.com> writes:
Show 12 quoted lines
> One issue %w() sidesteps is handing of single liner commit log messages
> (this is not a new issue your %B(n) introduces).  "%s%n%b" will give us
> the original message only when the log has some contents in addition to
> the single-line summary.  Otherwise we will get an extra blank line.
>
> Perhaps we could extend the pretty-printer so that it understands %+x
> notation, which expands to %n%x when %x expands to a non-empty result, and
> otherwise it expands to empty, as a generic extension applicable to any
> format specifier 'x'.  If we have such a notation, "%s%+b", would be a
> reasonable way to say "give us the original commit log message here", and
> we won't need %w(i,j,w) -- we can instead say %S(i,j,w)%+B(i,j,w), or
> %s%+B(i,j,w) depending on what you want.

This teaches the machinery to add a separator LF before any non-empty expansion of '%x' if you ask '%+x', and also removes LF if '%x' expands to an empty string if you ask '%-x', for any supported expansion placeholder 'x' it supports.

With the first two patches Dscho posted (and then polished in his tree), it shouldn't be too hard to update your %B(n) to %B(i,j,w) and also add %S(i,j,w) in a similar way, to allow people to say %S(i,j,w)%+B(i,j,w) instead of (or in addition to) his %w(i,j,w).

 pretty.c                   |   42 ++++++++++++++++++++++++++++++++++++++++--
 t/t6006-rev-list-format.sh |   22 ++++++++++++++++++++++
 2 files changed, 62 insertions(+), 2 deletions(-)
Show changes to 2 files +62 −2

pretty.c, t/t6006-rev-list-format.sh

diff --git a/pretty.c b/pretty.c
index f5983f8..081feb6 100644
--- a/pretty.c
+++ b/pretty.c
@@ -595,8 +595,8 @@ static void format_decoration(struct strbuf *sb, const struct commit *commit)
 		strbuf_addch(sb, ')');
 }
 
-static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
-                               void *context)
+static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
+				void *context)
 {
 	struct format_commit_context *c = context;
 	const struct commit *commit = c->commit;
@@ -739,6 +739,44 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 	return 0;	/* unknown placeholder */
 }
 
+static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
+				 void *context)
+{
+	int consumed;
+	size_t orig_len;
+	enum {
+		NO_MAGIC,
+		ADD_LF_BEFORE_NON_EMPTY,
+		DEL_LF_BEFORE_EMPTY,
+	} magic = NO_MAGIC;
+
+	switch (placeholder[0]) {
+	case '-':
+		magic = DEL_LF_BEFORE_EMPTY;
+		break;
+	case '+':
+		magic = ADD_LF_BEFORE_NON_EMPTY;
+		break;
+	default:
+		break;
+	}
+	if (magic != NO_MAGIC)
+		placeholder++;
+
+	orig_len = sb->len;
+	consumed = format_commit_one(sb, placeholder, context);
+	if (magic == NO_MAGIC)
+		return consumed;
+
+	if ((orig_len == sb->len) && magic == DEL_LF_BEFORE_EMPTY) {
+		while (sb->len && sb->buf[sb->len - 1] == '\n')
+			strbuf_setlen(sb, sb->len - 1);
+	} else if ((orig_len != sb->len) && magic == ADD_LF_BEFORE_NON_EMPTY) {
+		strbuf_insert(sb, orig_len, "\n", 1);
+	}
+	return consumed + 1;
+}
+
 void format_commit_message(const struct commit *commit,
 			   const void *format, struct strbuf *sb,
 			   enum date_mode dmode)
diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index 59d1f62..18a77a7 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh
@@ -162,4 +162,26 @@ test_expect_success 'empty email' '
 	}
 '
 
+test_expect_success 'del LF before empty (1)' '
+	git show -s --pretty=format:"%s%n%-b%nThanks%n" HEAD^^ >actual &&
+	test $(wc -l <actual) = 2
+'
+
+test_expect_success 'del LF before empty (2)' '
+	git show -s --pretty=format:"%s%n%-b%nThanks%n" HEAD >actual &&
+	test $(wc -l <actual) = 6 &&
+	grep "^$" actual
+'
+
+test_expect_success 'add LF before non-empty (1)' '
+	git show -s --pretty=format:"%s%+b%nThanks%n" HEAD^^ >actual &&
+	test $(wc -l <actual) = 2
+'
+
+test_expect_success 'add LF before non-empty (2)' '
+	git show -s --pretty=format:"%s%+b%nThanks%n" HEAD >actual &&
+	test $(wc -l <actual) = 6 &&
+	grep "^$" actual
+'
+
 test_done
.
Linus Torvalds· Sep 24, 2009, 00:00 UTC · re: Johannes Schindelin · lore

Re: [PATCH 1/3] print_wrapped_text(): allow hard newlines

On Wed, 23 Sep 2009, Johannes Schindelin wrote:
Show 8 quoted lines
> 
> print_wrapped_text() will insert its own newlines. Up until now, if the
> text passed to it contained newlines, they would not be handled properly
> (the wrapping got confused after that).
> 
> The strategy is to replace a single new-line with a space, but keep double
> new-lines so that already-wrapped text with empty lines between paragraphs
> will be handled properly.

May I suggest doing this _only_ if the newline is followed by an alphanumeric characer?

If the thing is indented ("newline + space") or quoted ("newline + ">" or whatever) then reflowing it is likely wrong and will result in an unholy mess.

			Linus
Johannes Schindelin· Sep 24, 2009, 00:19 UTC · re: Linus Torvalds · lore

Re: [PATCH 1/3] print_wrapped_text(): allow hard newlines

Hi,
On Wed, 23 Sep 2009, Linus Torvalds wrote:
Show 16 quoted lines
> On Wed, 23 Sep 2009, Johannes Schindelin wrote:
> > 
> > print_wrapped_text() will insert its own newlines. Up until now, if the
> > text passed to it contained newlines, they would not be handled properly
> > (the wrapping got confused after that).
> > 
> > The strategy is to replace a single new-line with a space, but keep double
> > new-lines so that already-wrapped text with empty lines between paragraphs
> > will be handled properly.
> 
> May I suggest doing this _only_ if the newline is followed by an 
> alphanumeric characer?
> 
> If the thing is indented ("newline + space") or quoted ("newline + ">" or 
> whatever) then reflowing it is likely wrong and will result in an unholy 
> mess.

It seems Junio is dead-set on ignoring my patches, so I will probably just keep the patch as-is in my tree for the time being. The other Johannes' patch is too limited for me to use, so I _will_ keep my patch, but I have way too little time to fight an uphill battle to get my patch in. And since it stays a private patch (just like my strbuf_vaddf() patch) I will have to refrain from putting more work and time into it than I already have.

Sorry, Dscho

Johannes Schindelin· Sep 25, 2009, 08:21 UTC · re: Linus Torvalds · lore

Re: [PATCH 1/3] print_wrapped_text(): allow hard newlines

Hi,
On Wed, 23 Sep 2009, Linus Torvalds wrote:
Show 16 quoted lines
> On Wed, 23 Sep 2009, Johannes Schindelin wrote:
> > 
> > print_wrapped_text() will insert its own newlines. Up until now, if 
> > the text passed to it contained newlines, they would not be handled 
> > properly (the wrapping got confused after that).
> > 
> > The strategy is to replace a single new-line with a space, but keep 
> > double new-lines so that already-wrapped text with empty lines between 
> > paragraphs will be handled properly.
> 
> May I suggest doing this _only_ if the newline is followed by an 
> alphanumeric characer?
> 
> If the thing is indented ("newline + space") or quoted ("newline + ">" 
> or whatever) then reflowing it is likely wrong and will result in an 
> unholy mess.

After further consideration, I decided to heed your advice; it is an obvious improvement when comparing the output with and without the isalnum().

So I updated my log-rewrap branch:
http://repo.or.cz/w/git/dscho.git?a=shortlog;h=refs/heads/log-rewrap

Thanks, Dscho

Junio C Hamano· Oct 10, 2009, 00:57 UTC · re: Johannes Gilger · lore

Re: [PATCHv4] git-log --format: Add %B tag with %B(n) option

Johannes Gilger <heipei@hackvalue.de> writes:
Show 11 quoted lines
> diff --git a/pretty.c b/pretty.c
> index f5983f8..dafa8e0 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -605,13 +605,17 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
>  	int h1, h2;
>  
>  	/* these are independent of the commit */
> +
> +	const char *body = msg + c->body_off;
> +	const char *end = NULL;

Unfortunately, c->body_off is not valid until you make a call to parse_commit_message(). Obviously, body is used only after such a call is made in the original, so I fixed this initialization into an explicit assignment after the call.

← back to recent threads