threads / patch / 16876

patch, 3 partspretty: factor out skip_empty_lines()

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

## tl;dr

15 messages between Dec 27, 2008 and Jan 7, 2009. Diffs are folded; open one to read it.

replies: 14people: 5as markdown or json

René Scharfe· Dec 27, 2008, 00:32 UTC · lore
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(-)
Show changes to pretty.c +15 −9
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
René Scharfe· Dec 27, 2008, 00:39 UTC · re: René Scharfe · lore

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

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(-)
Show changes to pretty.c +24 −18
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
René Scharfe· Dec 27, 2008, 00:49 UTC · re: René Scharfe · lore

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

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(-)
Show changes to pretty.c +34 −19
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
Markus Heidelberg· Dec 27, 2008, 23:24 UTC · re: René Scharfe · lore

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

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
Nanako Shiraishi· Dec 28, 2008, 00:09 UTC · re: Markus Heidelberg · lore

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

Quoting Markus Heidelberg <markus.heidelberg@web.de>:
Show 6 quoted lines
>> 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/
Miklos Vajna· Dec 28, 2008, 01:44 UTC · re: Nanako Shiraishi · lore

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

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

Markus Heidelberg· Dec 28, 2008, 14:08 UTC · re: Nanako Shiraishi · lore

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

Nanako Shiraishi, 28.12.2008:
Show 11 quoted lines
> 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
René Scharfe· Dec 28, 2008, 00:12 UTC · re: Markus Heidelberg · lore

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

Markus Heidelberg schrieb:
Show 13 quoted lines
>> 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.
Show 7 quoted lines
>> 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é
Markus Heidelberg· Dec 28, 2008, 14:16 UTC · re: René Scharfe · lore

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

René Scharfe, 28.12.2008:
Show 16 quoted lines
> 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
René Scharfe· Dec 29, 2008, 22:15 UTC · re: René Scharfe · lore

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

René Scharfe schrieb:
Show 5 quoted lines
> 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(-)
Show changes to 2 files +8 −5

builtin-shortlog.c, pretty.c

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;
 
Junio C Hamano· Jan 4, 2009, 10:01 UTC · re: René Scharfe · lore

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

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
Show 5 quoted lines
>> 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.
René Scharfe· Jan 6, 2009, 20:41 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to 2 files +8 −5

builtin-shortlog.c, pretty.c

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
Junio C Hamano· Jan 7, 2009, 21:19 UTC · re: René Scharfe · lore

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

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
Show 7 quoted lines
> 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.
Junio C Hamano· Dec 28, 2008, 00:13 UTC · re: Markus Heidelberg · lore

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

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

Markus Heidelberg· Dec 28, 2008, 14:26 UTC · re: Junio C Hamano · lore

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

Junio C Hamano, 28.12.2008:
Show 19 quoted lines
> 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

← back to recent threads