[PATCH 1/3] pretty: factor out skip_empty_lines()

The patch after the next one will use it.

Signed-off-by: Rene Scharfe <redacted>
---
 pretty.c |   24 +++++++++++++++---------
 1 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/pretty.c b/pretty.c
index f6ff312..c43497b 100644
--- a/pretty.c
+++ b/pretty.c

@@ -181,6 +181,20 @@ static int is_empty_line(const char *line, int *len_p)
 	return !len;
 }
 
+static const char *skip_empty_lines(const char *msg)
+{
+	for (;;) {
+		int linelen = get_one_line(msg);
+		int ll = linelen;
+		if (!linelen)
+			break;
+		if (!is_empty_line(msg, &ll))
+			break;
+		msg += linelen;
+	}
+	return msg;
+}
+
 static void add_merge_info(enum cmit_fmt fmt, struct strbuf *sb,
 			const struct commit *commit, int abbrev)
 {

@@ -850,15 +864,7 @@ void pretty_print_commit(enum cmit_fmt fmt, const struct commit *commit,
 	}
 
 	/* Skip excess blank lines at the beginning of body, if any... */
-	for (;;) {
-		int linelen = get_one_line(msg);
-		int ll = linelen;
-		if (!linelen)
-			break;
-		if (!is_empty_line(msg, &ll))
-			break;
-		msg += linelen;
-	}
+	msg = skip_empty_lines(msg);
 
 	/* These formats treat the title line specially. */
 	if (fmt == CMIT_FMT_ONELINE || fmt == CMIT_FMT_EMAIL)

-- 
1.6.1

The next patch will use it.

In the version that was factored out, we can't rely on the len of the
struct strbuf to find out if a line separator needs to be added, as
it might already contain something.  Add a guard variable ("first")
instead.

Signed-off-by: Rene Scharfe <redacted>
---
 pretty.c |   42 ++++++++++++++++++++++++------------------
 1 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/pretty.c b/pretty.c
index c43497b..632abc5 100644
--- a/pretty.c
+++ b/pretty.c

@@ -495,6 +495,28 @@ static void parse_commit_header(struct format_commit_context *context)
 	context->commit_header_parsed = 1;
 }
 
+static const char *format_subject(struct strbuf *sb, const char *msg,
+				  const char *line_separator)
+{
+	int first = 1;
+
+	for (;;) {
+		const char *line = msg;
+		int linelen = get_one_line(line);
+
+		msg += linelen;
+		if (!linelen || is_empty_line(line, &linelen))
+			break;
+
+		strbuf_grow(sb, linelen + 2);
+		if (!first)
+			strbuf_addstr(sb, line_separator);
+		strbuf_add(sb, line, linelen);
+		first = 0;
+	}
+	return msg;
+}
+
 static void format_decoration(struct strbuf *sb, const struct commit *commit)
 {
 	struct name_decoration *d;

@@ -718,27 +740,11 @@ void pp_title_line(enum cmit_fmt fmt,
 		   const char *encoding,
 		   int need_8bit_cte)
 {
+	const char *line_separator = (fmt == CMIT_FMT_EMAIL) ? "\n " : " ";
 	struct strbuf title;
 
 	strbuf_init(&title, 80);
-
-	for (;;) {
-		const char *line = *msg_p;
-		int linelen = get_one_line(line);
-
-		*msg_p += linelen;
-		if (!linelen || is_empty_line(line, &linelen))
-			break;
-
-		strbuf_grow(&title, linelen + 2);
-		if (title.len) {
-			if (fmt == CMIT_FMT_EMAIL) {
-				strbuf_addch(&title, '\n');
-			}
-			strbuf_addch(&title, ' ');
-		}
-		strbuf_add(&title, line, linelen);
-	}
+	*msg_p = format_subject(&title, *msg_p, line_separator);
 
 	strbuf_grow(sb, title.len + 1024);
 	if (subject) {

-- 
1.6.1

git log --pretty=format:%s (and tformat:) used to display the first
line of the subject, unlike the other --pretty options, which would
construct a subject line from all lines of the first paragraph of
the commit message.

For consistency and increased code reuse, change format: to do the
same as the other options.

Before:
	$ git log --pretty=oneline v1.6.1 | md5sum
	7c0896d2a94fc3315a0372b9b3373a8f  -
	$ git log --pretty=tformat:"%H %s" v1.6.1 | md5sum
	298903b1c065002e15daa5329213c51f  -

After:
	$ git log --pretty=tformat:"%H %s" v1.6.1 | md5sum
	7c0896d2a94fc3315a0372b9b3373a8f  -
	$ git log --pretty=oneline v1.6.1 | md5sum
	7c0896d2a94fc3315a0372b9b3373a8f  -

Signed-off-by: Rene Scharfe <redacted>
---
 pretty.c |   53 ++++++++++++++++++++++++++++++++++-------------------
 1 files changed, 34 insertions(+), 19 deletions(-)

diff --git a/pretty.c b/pretty.c
index 632abc5..343dca5 100644
--- a/pretty.c
+++ b/pretty.c

@@ -424,13 +424,15 @@ struct chunk {
 struct format_commit_context {
 	const struct commit *commit;
 	enum date_mode dmode;
+	unsigned commit_header_parsed:1;
+	unsigned commit_message_parsed:1;
 
 	/* These offsets are relative to the start of the commit message. */
-	int commit_header_parsed;
-	struct chunk subject;
 	struct chunk author;
 	struct chunk committer;
 	struct chunk encoding;
+	size_t message_off;
+	size_t subject_off;
 	size_t body_off;
 
 	/* The following ones are relative to the result struct strbuf. */

@@ -460,23 +462,14 @@ static void parse_commit_header(struct format_commit_context *context)
 {
 	const char *msg = context->commit->buffer;
 	int i;
-	enum { HEADER, SUBJECT, BODY } state;
 
-	for (i = 0, state = HEADER; msg[i] && state < BODY; i++) {
+	for (i = 0; msg[i]; i++) {
 		int eol;
 		for (eol = i; msg[eol] && msg[eol] != '\n'; eol++)
 			; /* do nothing */
 
-		if (state == SUBJECT) {
-			context->subject.off = i;
-			context->subject.len = eol - i;
-			i = eol;
-		}
 		if (i == eol) {
-			state++;
-			/* strip empty lines */
-			while (msg[eol] == '\n' && msg[eol + 1] == '\n')
-				eol++;
+			break;
 		} else if (!prefixcmp(msg + i, "author ")) {
 			context->author.off = i + 7;
 			context->author.len = eol - i - 7;

@@ -488,10 +481,8 @@ static void parse_commit_header(struct format_commit_context *context)
 			context->encoding.len = eol - i - 9;
 		}
 		i = eol;
-		if (!msg[i])
-			break;
 	}
-	context->body_off = i;
+	context->message_off = i;
 	context->commit_header_parsed = 1;
 }
 

@@ -508,6 +499,8 @@ static const char *format_subject(struct strbuf *sb, const char *msg,
 		if (!linelen || is_empty_line(line, &linelen))
 			break;
 
+		if (!sb)
+			continue;
 		strbuf_grow(sb, linelen + 2);
 		if (!first)
 			strbuf_addstr(sb, line_separator);

@@ -517,6 +510,21 @@ static const char *format_subject(struct strbuf *sb, const char *msg,
 	return msg;
 }
 
+static void parse_commit_message(struct format_commit_context *c)
+{
+	const char *msg = c->commit->buffer + c->message_off;
+	const char *start = c->commit->buffer;
+
+	msg = skip_empty_lines(msg);
+	c->subject_off = msg - start;
+
+	msg = format_subject(NULL, msg, NULL);
+	msg = skip_empty_lines(msg);
+	c->body_off = msg - start;
+
+	c->commit_message_parsed = 1;
+}
+
 static void format_decoration(struct strbuf *sb, const struct commit *commit)
 {
 	struct name_decoration *d;

@@ -636,9 +644,6 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 		parse_commit_header(c);
 
 	switch (placeholder[0]) {
-	case 's':	/* subject */
-		strbuf_add(sb, msg + c->subject.off, c->subject.len);
-		return 1;
 	case 'a':	/* author ... */
 		return format_person_part(sb, placeholder[1],
 				   msg + c->author.off, c->author.len,

@@ -650,6 +655,16 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 	case 'e':	/* encoding */
 		strbuf_add(sb, msg + c->encoding.off, c->encoding.len);
 		return 1;
+	}
+
+	/* Now we need to parse the commit message. */
+	if (!c->commit_message_parsed)
+		parse_commit_message(c);
+
+	switch (placeholder[0]) {
+	case 's':	/* subject */
+		format_subject(sb, msg + c->subject_off, " ");
+		return 1;
 	case 'b':	/* body */
 		strbuf_addstr(sb, msg + c->body_off);
 		return 1;

-- 
1.6.1

Hello,

I hope I got the In-Reply-To right, the message was sent before my
subscription to this list.

René Scharfe, 27.12.2008:

git log --pretty=format:%s (and tformat:) used to display the first
line of the subject, unlike the other --pretty options, which would
construct a subject line from all lines of the first paragraph of
the commit message.

And that can be very annoying when working on repositories with a
history not formatted after the recommendations of git (empty line after
the summary). Then the output of commands like
"git log --pretty=oneline", "git format-patch" and "git show-branch"
is unreadable.

Sorry, I haven't tested your patch set, what is the output of
git-shortlog like now?

For consistency and increased code reuse, change format: to do the
same as the other options.

What's wrong with using the first line instead of the first paragraph
for the other pretty options and for cases where only a small subject
line is desired? A sentence would be broken into a small uncomplete part
for the subject for example. What else?

Markus

Quoting Markus Heidelberg [off-list ref]:

quoted

For consistency and increased code reuse, change format: to do the
same as the other options.

What's wrong with using the first line instead of the first paragraph
for the other pretty options and for cases where only a small subject
line is desired?

There were too many issues. The mail archive tells us that there were considerable discussions and thoughts behind the current behavior:

	http://thread.gmane.org/gmane.comp.version-control.git/49694/focus=49922
	http://thread.gmane.org/gmane.comp.version-control.git/46341/focus=46818
	http://thread.gmane.org/gmane.comp.version-control.git/54020/focus=54132
	http://thread.gmane.org/gmane.comp.version-control.git/21949/focus=21987

The last message mentions a bug in "git rebase" that internally uses "git am" when replaying the changes. It was corrected later with:

	http://thread.gmane.org/gmane.linux.ports.sh.devel/3833/focus=79735

Reading the discussion in the messages in this thread will help understanding the issues, after understanding why "the first line" was problematic, as discussed in the other threads.

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

Markus Heidelberg schrieb:

quoted

git log --pretty=format:%s (and tformat:) used to display the first
line of the subject, unlike the other --pretty options, which would
construct a subject line from all lines of the first paragraph of
the commit message.

And that can be very annoying when working on repositories with a
history not formatted after the recommendations of git (empty line after
the summary). Then the output of commands like
"git log --pretty=oneline", "git format-patch" and "git show-branch"
is unreadable.

Sorry, I haven't tested your patch set, what is the output of
git-shortlog like now?

It's unchanged since it has it's own commit message parser.

quoted

For consistency and increased code reuse, change format: to do the
same as the other options.

What's wrong with using the first line instead of the first paragraph
for the other pretty options and for cases where only a small subject
line is desired? A sentence would be broken into a small uncomplete part
for the subject for example. What else?

I'm inching towards the goal of being able to replicate all of the other
--pretty flavours with format: (or tformat:) strings.

--pretty=oneline used to print only the first line, but was changed to
the way it's now in 4234a761 more than a year ago.  In my mind,
"subject" is defined by "what oneline is printing", so I think changing
the format: placeholder %s to show the whole first paragraph is a bug fix.

If you actually like the old behaviour then perhaps a new placeholder
(%S ?) should be introduced instead?

René

On Sun, Dec 28, 2008 at 09:09:09AM +0900, Nanako Shiraishi [off-list ref] wrote:

The last message mentions a bug in "git rebase" that internally uses
"git am" when replaying the changes. It was corrected later with:

	http://thread.gmane.org/gmane.linux.ports.sh.devel/3833/focus=79735

Reading the discussion in the messages in this thread will help
understanding the issues, after understanding why "the first line" was
problematic, as discussed in the other threads.

Actually in that thread I asked if a patch that would make this
configurable would be ever accepted (so I would work on it), but I got
no answer. In the meantime I just learned to live with this, partially
because the vim highlight script highlights anything written to the
second line as red. ;-)

Nanako Shiraishi, 28.12.2008:

Quoting Markus Heidelberg [off-list ref]:

quoted

quoted

For consistency and increased code reuse, change format: to do the
same as the other options.

What's wrong with using the first line instead of the first paragraph
for the other pretty options and for cases where only a small subject
line is desired?

There were too many issues. The mail archive tells us that there were
considerable discussions and thoughts behind the current behavior:

Many thanks for all the pointers. I thought there were reasons, but this
thread just made me ask.

Markus

René Scharfe, 28.12.2008:

Markus Heidelberg schrieb:

quoted

quoted

For consistency and increased code reuse, change format: to do the
same as the other options.

What's wrong with using the first line instead of the first paragraph
for the other pretty options and for cases where only a small subject
line is desired? A sentence would be broken into a small uncomplete part
for the subject for example. What else?

I'm inching towards the goal of being able to replicate all of the other
--pretty flavours with format: (or tformat:) strings.

--pretty=oneline used to print only the first line, but was changed to
the way it's now in 4234a761 more than a year ago.  In my mind,
"subject" is defined by "what oneline is printing", so I think changing
the format: placeholder %s to show the whole first paragraph is a bug fix.

Yes, now I think so, too. I had also noticed the unexpectedly different
behaviour once.

If you actually like the old behaviour then perhaps a new placeholder
(%S ?) should be introduced instead?

I don't need it. I rather asked regarding the old behaviour for oneline
and so, but reading that there are strong intentions for their
behaviour, it's fine.

Markus

René Scharfe schrieb:

Markus Heidelberg schrieb:

quoted

Sorry, I haven't tested your patch set, what is the output of
git-shortlog like now?

It's unchanged since it has it's own commit message parser.

... which displays the first line of the commit message, unlike
--pretty=oneline.  Here's a quick fix.  I probably won't have time
to come up with something prettier this year.

René


 builtin-shortlog.c |    9 ++++++---
 pretty.c           |    4 ++--
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/builtin-shortlog.c b/builtin-shortlog.c
index d03f14f..e492906 100644
--- a/builtin-shortlog.c
+++ b/builtin-shortlog.c

@@ -29,6 +29,9 @@ static int compare_by_number(const void *a1, const void *a2)
 		return -1;
 }
 
+const char *format_subject(struct strbuf *sb, const char *msg,
+			   const char *line_separator);
+
 static void insert_one_record(struct shortlog *log,
 			      const char *author,
 			      const char *oneline)

@@ -41,6 +44,7 @@ static void insert_one_record(struct shortlog *log,
 	size_t len;
 	const char *eol;
 	const char *boemail, *eoemail;
+	struct strbuf subject = STRBUF_INIT;
 
 	boemail = strchr(author, '<');
 	if (!boemail)

@@ -89,9 +93,8 @@ static void insert_one_record(struct shortlog *log,
 	while (*oneline && isspace(*oneline) && *oneline != '\n')
 		oneline++;
 	len = eol - oneline;
-	while (len && isspace(oneline[len-1]))
-		len--;
-	buffer = xmemdupz(oneline, len);
+	format_subject(&subject, oneline, " ");
+	buffer = strbuf_detach(&subject, NULL);
 
 	if (dot3) {
 		int dot3len = strlen(dot3);

diff --git a/pretty.c b/pretty.c
index 343dca5..421d9c5 100644
--- a/pretty.c
+++ b/pretty.c

@@ -486,8 +486,8 @@ static void parse_commit_header(struct format_commit_context *context)
 	context->commit_header_parsed = 1;
 }
 
-static const char *format_subject(struct strbuf *sb, const char *msg,
-				  const char *line_separator)
+const char *format_subject(struct strbuf *sb, const char *msg,
+			   const char *line_separator)
 {
 	int first = 1;
 

René Scharfe [off-list ref] writes:

quoted

It's unchanged since it has it's own commit message parser.

... which displays the first line of the commit message, unlike
--pretty=oneline.  Here's a quick fix.  I probably won't have time
to come up with something prettier this year.

I think the code is pretty enough ;-)  I just need to come up with a patch
description and forge your Sign-off.

Thanks.

The commit message parser of git shortlog used to treat only the first
non-empty line of the commit message as the subject.  Other log commands
(e.g. --pretty=oneline) show the whole first paragraph instead (unwrapped
into a single line).

For consistency, this patch borrows format_subject() from pretty.c to
make shortlog do the same.

Signed-off-by: Rene Scharfe <redacted>
---
 builtin-shortlog.c |    9 ++++++---
 pretty.c           |    4 ++--
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/builtin-shortlog.c b/builtin-shortlog.c
index d03f14f..e492906 100644
--- a/builtin-shortlog.c
+++ b/builtin-shortlog.c

@@ -29,6 +29,9 @@ static int compare_by_number(const void *a1, const void *a2)
 		return -1;
 }
 
+const char *format_subject(struct strbuf *sb, const char *msg,
+			   const char *line_separator);
+
 static void insert_one_record(struct shortlog *log,
 			      const char *author,
 			      const char *oneline)

@@ -41,6 +44,7 @@ static void insert_one_record(struct shortlog *log,
 	size_t len;
 	const char *eol;
 	const char *boemail, *eoemail;
+	struct strbuf subject = STRBUF_INIT;
 
 	boemail = strchr(author, '<');
 	if (!boemail)

@@ -89,9 +93,8 @@ static void insert_one_record(struct shortlog *log,
 	while (*oneline && isspace(*oneline) && *oneline != '\n')
 		oneline++;
 	len = eol - oneline;
-	while (len && isspace(oneline[len-1]))
-		len--;
-	buffer = xmemdupz(oneline, len);
+	format_subject(&subject, oneline, " ");
+	buffer = strbuf_detach(&subject, NULL);
 
 	if (dot3) {
 		int dot3len = strlen(dot3);

diff --git a/pretty.c b/pretty.c
index 343dca5..421d9c5 100644
--- a/pretty.c
+++ b/pretty.c

@@ -486,8 +486,8 @@ static void parse_commit_header(struct format_commit_context *context)
 	context->commit_header_parsed = 1;
 }
 
-static const char *format_subject(struct strbuf *sb, const char *msg,
-				  const char *line_separator)
+const char *format_subject(struct strbuf *sb, const char *msg,
+			   const char *line_separator)
 {
 	int first = 1;
 

-- 
1.6.1

René Scharfe [off-list ref] writes:

The commit message parser of git shortlog used to treat only the first
non-empty line of the commit message as the subject.  Other log commands
(e.g. --pretty=oneline) show the whole first paragraph instead (unwrapped
into a single line).

For consistency, this patch borrows format_subject() from pretty.c to
make shortlog do the same.

Thanks; will queue.