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

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

[Johannes Gilger]

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

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

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

[Junio C Hamano]

Johannes Gilger <heipei@hackvalue.de> writes:

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

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

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

[Johannes Gilger]

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

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

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

[Junio C Hamano]

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

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

[Johannes Gilger]

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

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

[Junio C Hamano]

Johannes Gilger <heipei@hackvalue.de> writes:

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

>  		strbuf_grow(sb, linelen + indent + 20);
>  		if (indent) {
>  			memset(sb->buf + sb->len, ' ', indent);
> -- 
> 1.6.5.rc1.20.geb7d9

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

[Johannes Gilger]

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

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

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

[Johannes Schindelin]

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

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

[Johannes Schindelin]

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

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

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

[Johannes Schindelin]

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

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

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

[Johannes Schindelin]

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

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

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

[Johannes Gilger]

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

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

[Junio C Hamano]

Johannes Gilger <heipei@hackvalue.de> writes:

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

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

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

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
.

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

[Linus Torvalds]

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.

			Linus

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

[Johannes Schindelin]

Hi,

On Wed, 23 Sep 2009, Linus Torvalds wrote:

> 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

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

[Johannes Schindelin]

Hi,

On Wed, 23 Sep 2009, Linus Torvalds wrote:

> 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

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

[Junio C Hamano]

Johannes Gilger <heipei@hackvalue.de> writes:

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