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

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

[René Scharfe]

The patch after the next one will use it.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 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

[PATCH 2/3] pretty: factor out format_subject()

[René Scharfe]

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 <rene.scharfe@lsrfire.ath.cx>
---
 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

[PATCH 3/3] pretty: support multiline subjects with format:

[René Scharfe]

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 <rene.scharfe@lsrfire.ath.cx>
---
 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

Re: [PATCH 3/3] pretty: support multiline subjects with format:

[Markus Heidelberg]

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

Re: [PATCH 3/3] pretty: support multiline subjects with format:

[René Scharfe]

Markus Heidelberg schrieb:
>> 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.

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

Re: [PATCH 3/3] pretty: support multiline subjects with format:

[Markus Heidelberg]

René Scharfe, 28.12.2008:
> Markus Heidelberg schrieb:
> >> 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

Re: [PATCH 3/3] pretty: support multiline subjects with format:

[René Scharfe]

René Scharfe schrieb:
> Markus Heidelberg schrieb:
>> 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;
 

Re: [PATCH 3/3] pretty: support multiline subjects with format:

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

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

[PATCH 4/3] shortlog: handle multi-line subjects like log --pretty=oneline et. al. do

[René Scharfe]

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 <rene.scharfe@lsrfire.ath.cx>
---
 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

Re: [PATCH 4/3] shortlog: handle multi-line subjects like log --pretty=oneline et. al. do

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> 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.

Re: [PATCH 3/3] pretty: support multiline subjects with format:

[Junio C Hamano]

Markus Heidelberg <markus.heidelberg@web.de> writes:

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

The "first line" actually was what we used to do originally.

git-native commits did not have problem with this behaviour, but it caused
countless complaints from people looking at the history converted from
other cultures.

The behaviour was later fixed to avoid information loss when a commit from
a different culture begins the log with a long sentence, choped at
mid-sentence, to continue to the second line.

Please check the list archive for the details (sorry, I am bit too lazy to
repeat the argument myself).

Re: [PATCH 3/3] pretty: support multiline subjects with format:

[Markus Heidelberg]

Junio C Hamano, 28.12.2008:
> Markus Heidelberg <markus.heidelberg@web.de> writes:
> 
> > 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?
> 
> The "first line" actually was what we used to do originally.
> 
> git-native commits did not have problem with this behaviour, but it caused
> countless complaints from people looking at the history converted from
> other cultures.
> 
> The behaviour was later fixed to avoid information loss when a commit from
> a different culture begins the log with a long sentence, choped at
> mid-sentence, to continue to the second line.
> 
> Please check the list archive for the details (sorry, I am bit too lazy to
> repeat the argument myself).

I don't call this lazy. Normally I look myself and don't want others to
spend time on searching for me. Reading that there already were
discussions about this and that the current behaviour is thoughtful is
enough for me to look for details myself. Thanks.

Markus

Re: [PATCH 3/3] pretty: support multiline subjects with format:

[Nanako Shiraishi]

Quoting Markus Heidelberg <markus.heidelberg@web.de>:

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

Re: [PATCH 3/3] pretty: support multiline subjects with format:

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 807 bytes --]

On Sun, Dec 28, 2008 at 09:09:09AM +0900, Nanako Shiraishi <nanako3@lavabit.com> 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. ;-)

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 3/3] pretty: support multiline subjects with format:

[Markus Heidelberg]

Nanako Shiraishi, 28.12.2008:
> Quoting Markus Heidelberg <markus.heidelberg@web.de>:
> 
> >> 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