Re: [PATCH] cherry-pick -x: add newline before pick note

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Mar 8, 2011, 22:08 UTC
Message-ID: <20110308220843.GA27156@elie>
In-Reply-To: <loom.20110308T134920-72@post.gmane.org>

(please do not cull the CC list) Hi Oswald,

Oswald Buddenhagen wrote:

> so while everybody is apparently thinking about totally over-engineering
> things as much as possible, could we please have this patch applied so we
> have a solution for the time being?

I am not convinced that this patch makes a positive change in general. Starting from a message

	Foo the bar

	Make some excellent improvement to the frobnicator.

	Signed-off-by: A U Thor <author@example.com>

a person passing on the patch might write

	Foo the bar
[...]
	Signed-off-by: A U Thor <author@example.com>
	[committer@example.com: avoid multiple return points]
	Signed-off-by: C O Mitter <committer@example.com>

Similarly, when cherry-picking from permanent history, it can make sense to write

	Foo the bar
[...]
	Signed-off-by: A U Thor <author@example.com>
	(cherry picked from commit 78a8b989a76c8798a9898c98a98c98a98ca)
	Signed-off-by: C O Mitter <committer@example.com>

In both cases, it's just another hop in the life of a patch and not something that seems to deserve emphasis with extra whitespace.

> i really hate to tell my coworkers that
> they have to amend the cherry-picks just to make them comply with git's
> own guidelines for well-formed commit messages (and thus have them pass
> our pre-receive hook).

I assume you are referring to one-line commit messages becoming two-line? Here's something rough to start.

diff --git a/builtin/revert.c b/builtin/revert.c
index dc1b702..343e7e7 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -198,6 +198,28 @@ static void add_message_to_msg(struct strbuf *msgbuf, const char *message)
 	strbuf_addstr(msgbuf, p);
 }
 
+static void add_blank_line_if_oneline(struct strbuf *msgbuf)
+{
+	const char *newline = memchr(msgbuf->buf, '\n', msgbuf->len);
+
+	if (!newline) {	/* No newline at end of message. */
+		strbuf_addch(msgbuf, '\n');
+		newline = msgbuf->buf + msgbuf->len - 1;
+	}
+
+	/*
+	 * If the change description consists of a single line,
+	 * add a blank line separating the title from the new
+	 * message body (the "(cherry picked from" line).
+	 *
+	 * NEEDSWORK: it would be better to reuse the append_signoff
+	 * logic.
+	 */
+	newline = memchr(newline, '\n', msgbuf-> buf + msgbuf->len - newline);
+	if (!newline)
+		strbuf_addch(msgbuf, '\n');
+}
+
 static void set_author_ident_env(const char *message)
 {
 	const char *p = message;
@@ -499,6 +521,7 @@ static int do_pick_commit(void)
 		next_label = msg.label;
 		set_author_ident_env(msg.message);
 		add_message_to_msg(&msgbuf, msg.message);
+		add_blank_line_if_oneline(&msgbuf);
 		if (no_replay) {
 			strbuf_addstr(&msgbuf, "(cherry picked from commit ");
 			strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));