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

Currently, cherry-pick -x sticks the pick note immediately after the
existing commit message. This

* is bad for commits with 1 line subject (it makes a 2 line subject)
* is different from git-svn, e.g., which leaves an empty line before.

Make cherry-pick always insert an empty line before the pick note.

Reported-by: Martin Svensson <redacted>
Signed-off-by: Michael J Gruber <redacted>
---
 builtin/revert.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 57b51e4..9251257 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c

@@ -485,7 +485,7 @@ static int do_pick_commit(void)
 		set_author_ident_env(msg.message);
 		add_message_to_msg(&msgbuf, msg.message);
 		if (no_replay) {
-			strbuf_addstr(&msgbuf, "(cherry picked from commit ");
+			strbuf_addstr(&msgbuf, "\n(cherry picked from commit ");
 			strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
 			strbuf_addstr(&msgbuf, ")\n");
 		}

-- 
1.7.3.2.193.g78bbb

On Tue, Nov 16, 2010 at 04:11:17PM +0100, Michael J Gruber wrote:

Currently, cherry-pick -x sticks the pick note immediately after the
existing commit message. This

* is bad for commits with 1 line subject (it makes a 2 line subject)
* is different from git-svn, e.g., which leaves an empty line before.

Make cherry-pick always insert an empty line before the pick note.

Hmm. Should this respect pseudo-header blocks at the end? E.g., if I
have:

  message subject

  Message body.

  Signed-off-by: Jeff King [off-list ref]

shouldn't it result in:

  message subject

  Message body.

  (cherry picked from commit ...)

  Signed-off-by: Jeff King [off-list ref]

?

Even better, I wonder if it should actually be:

  message subject

  Message body.

  Signed-off-by: Jeff King [off-list ref]
  Cherry-picked-from: ...

And then you could actually sign off the cherry-pick separately, too, if
you wanted, by adding a line _below_ the cherry-picked-from. I have no
idea if people are trying to grep for "cherry picked from commit...",
which my proposal would break.

Note that none of this is introduced by your patch. The current output
for this case is terribly ugly. But I thought I would mention it, as my
third version means we _do_ want the current behavior in some cases
(i.e., when there is already a pseudo-header block).

-Peff

Using cherry-pick -x -s to backport a public commit results in
an unsightly gap in the sign-off chain:

	Reported-by: Jarek Poplawski [off-list ref]
	Tested-by: Jarek Poplawski [off-list ref]
	Signed-off-by: Frederic Weisbecker [off-list ref]
	Cc: Jeff Mahoney [off-list ref]
	Cc: All since 2.6.32 [off-list ref]
	Signed-off-by: Andrew Morton [off-list ref]
	Signed-off-by: Linus Torvalds [off-list ref]
	(cherry picked from commit 9d8117e72bf453dd9d85e0cd322ce4a0f8bccbc0)

	Signed-off-by: Back Porter [off-list ref]

The cherry-pick is a step in the line of a patch like any other,
so one might prefer to lose the extra newline.

	...
	Signed-off-by: Linus Torvalds [off-list ref]
	(cherry picked from commit 9d8117e72bf453dd9d85e0cd322ce4a0f8bccbc0)
	Signed-off-by: Back Porter [off-list ref]

This commit teaches "git commit --signoff", and thus cherry-pick -s,
to do exactly that.  It works by treating the "(cherry picked" line as
just another line in the signoff chain, except as the first line (that
last exception is to avoid false positives).

Signed-off-by: Jonathan Nieder <redacted>
---
Jeff King wrote:

Even better, I wonder if it should actually be:

  message subject

  Message body.

  Signed-off-by: Jeff King [off-list ref]
  Cherry-picked-from: ...

Here's something like that.  I use "git cherry-pick -x -s" to
backport patches from a public upstream.  Now you can, too.

Ideally inline notes like

	 [akpm@linux-foundation.org: coding-style fixes]

also ought to be tolerated.

 builtin/commit.c               |   20 +++++++
 t/t3510-cherry-pick-message.sh |  112 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 132 insertions(+), 0 deletions(-)
 create mode 100755 t/t3510-cherry-pick-message.sh

diff --git a/builtin/commit.c b/builtin/commit.c
index 66fdd22..71dd52b 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -528,6 +528,8 @@ static int ends_rfc2822_footer(struct strbuf *sb)
 		i++;
 
 	for (; i < len; i = k) {
+		static const char cherry_pick[] = "(cherry picked from commit ";
+
 		for (k = i; k < len && buf[k] != '\n'; k++)
 			; /* do nothing */
 		k++;

@@ -535,6 +537,20 @@ static int ends_rfc2822_footer(struct strbuf *sb)
 		if ((buf[k] == ' ' || buf[k] == '\t') && !first)
 			continue;
 
+		if (!first && buf[k] == '(' && k + strlen(cherry_pick) < len) {
+			/* Might be a cherry-pick notice. */
+			const char *p = buf + k;
+			if (!memcmp(p, cherry_pick, strlen(cherry_pick))) {
+				p = memchr(buf + k, '\n', len - k);
+				if (!p)
+					return 0;
+				if (p + 1 == buf + len)
+					return 1;
+				k = p - buf;
+				continue;
+			}
+		}
+
 		first = 0;
 
 		for (j = 0; i + j < len; j++) {

@@ -625,6 +641,10 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		for (i = sb.len - 1; i > 0 && sb.buf[i - 1] != '\n'; i--)
 			; /* do nothing */
 		if (prefixcmp(sb.buf + i, sob.buf)) {
+			/*
+			 * Only insert an extra newline if the previous line
+			 * is not part of a Signed-off-by:/Acked-by:/etc chain.
+			 */
 			if (!i || !ends_rfc2822_footer(&sb))
 				strbuf_addch(&sb, '\n');
 			strbuf_addbuf(&sb, &sob);

diff --git a/t/t3510-cherry-pick-message.sh b/t/t3510-cherry-pick-message.sh
new file mode 100755
index 0000000..83e50a3
--- /dev/null
+++ b/t/t3510-cherry-pick-message.sh

@@ -0,0 +1,112 @@
+#!/bin/sh
+
+test_description='tests for the log messages cherry-pick produces
+
+  ----
+  +      cherry-pick of branch
+     +   signoff
+    +    basic
+  ++     mainline
+  +++    initial
+'
+. ./test-lib.sh
+
+prepare_commit () {
+	git checkout initial &&
+	test_commit "$1" &&
+	test_tick &&
+	git commit --amend --allow-empty-message -F "$1.message" &&
+	git tag -d "$1" &&
+	git tag "$1"
+}
+
+test_cmp_message () {
+	expect=$1 &&
+	shift &&
+	git log -1 --pretty=format:%B "$@" >actual &&
+	test_cmp "$expect" actual
+}
+
+cat >basic.message <<\EOF
+ A branch
+
+Here comes the lovely description of a change to pick up.
+Contributions come from many people:
+EOF
+
+{
+	cat basic.message
+	cat <<-\EOF
+
+	Signed-off-by: Foo <foo@example.com>
+	Signed-off-by: Bar <bar@example.com>
+	Tested-by: Baz <baz@example.com>
+	EOF
+} >signoff.message
+
+test_expect_success 'setup' '
+	test_commit initial &&
+	prepare_commit basic &&
+	prepare_commit signoff
+'
+
+test_expect_success 'cherry-pick preserves message' '
+	cat basic.message >expect &&
+	git checkout initial &&
+	git cherry-pick basic &&
+	test_cmp_message expect
+'
+
+test_expect_success 'cherry-pick -s adds signoff' '
+	{
+		cat basic.message &&
+		echo &&
+		echo "Signed-off-by: C O Mitter <committer@example.com>"
+	} >expect &&
+	git checkout initial &&
+	git cherry-pick -s basic &&
+	test_cmp_message expect HEAD
+'
+
+test_expect_success 'cherry-pick -s integrates into existing signoff chain' '
+	{
+		cat signoff.message &&
+		echo "Signed-off-by: C O Mitter <committer@example.com>"
+	} >expect &&
+	git checkout initial &&
+	git cherry-pick -s signoff &&
+	test_cmp_message expect HEAD
+'
+
+test_expect_success 'cherry-pick -x adds old commit id' '
+	{
+		cat basic.message &&
+		echo "(cherry picked from commit $(git rev-parse basic^0))"
+	} >expect &&
+	git checkout initial &&
+	git cherry-pick -x basic &&
+	test_cmp_message expect HEAD
+'
+
+test_expect_success 'cherry-pick -x integrates into signoff chain' '
+	{
+		cat signoff.message &&
+		echo "(cherry picked from commit $(git rev-parse signoff^0))"
+	} >expect &&
+	git checkout initial &&
+	git cherry-pick -x signoff &&
+	test_cmp_message expect HEAD
+'
+
+test_expect_success 'cherry-pick -x -s' '
+	{
+		cat signoff.message &&
+		echo "(cherry picked from commit $(git rev-parse signoff^0))"
+		echo "Signed-off-by: C O Mitter <committer@example.com>"
+	} >expect &&
+	git checkout initial &&
+	git cherry-pick -x -s signoff &&
+	test_cmp_message expect HEAD
+'
+
+test_done

-- 
1.7.2.3.551.g13682.dirty

Jonathan Nieder wrote:

	(cherry picked from commit 9d8117e72bf453dd9d85e0cd322ce4a0f8bccbc0)

	Signed-off-by: Back Porter [off-list ref]

The cherry-pick is a step in the line of a patch like any other,
so one might prefer to lose the extra newline.

Sigh.  s/line/life/

[...]

Signed-off-by: Jonathan Nieder <redacted>

Let's kick off the reviews.

Better to share this string with builtin/revert.c, no?

What would happen when "(cherry picked ..." gets translated?
Should only the current language's version be tolerated in
the commit footer, or is there something more generic to
match for that could take care of wording changes automatically?

Maybe simpler:

	p = memchr(...
	if (!p)
		return 0;
	i = p - buf;

to reuse the termination condition in the sign-off parser.

Presumably the main loop could use memchr() instead of open-coding
it as well.

On Tue, Nov 16, 2010 at 2:30 PM, Jeff King [off-list ref] wrote:

shouldn't it result in:

 message subject

 Message body.

 (cherry picked from commit ...)

 Signed-off-by: Jeff King [off-list ref]

+1.

Even better, I wonder if it should actually be:

 message subject

 Message body.

 Signed-off-by: Jeff King [off-list ref]
 Cherry-picked-from: ...

+2.

And then you could actually sign off the cherry-pick separately, too, if
you wanted, by adding a line _below_ the cherry-picked-from. I have no
idea if people are trying to grep for "cherry picked from commit...",
which my proposal would break.

I can fix my regex easily enough, but I'd also be happy to have this
use some other switch than -x.

BTW, I notice that cherry-pick also misbehaves if the original commit
message doesn't end in a newline. I'm not sure whether that's a
cherry-pick bug for not checking that case, or whether it's a commit
bug for not ensuring a newline terminates the commit message.

j.

Michael J Gruber <git <at> drmicha.warpmail.net> writes:

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

regards

(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 [off-list ref]

a person passing on the patch might write

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

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

	Foo the bar
[...]
	Signed-off-by: A U Thor [off-list ref]
	(cherry picked from commit 78a8b989a76c8798a9898c98a98c98a98ca)
	Signed-off-by: C O Mitter [off-list ref]

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

-- 

On Tue, Mar 08, 2011 at 04:08:43PM -0600, Jonathan Nieder wrote:

(please do not cull the CC list)

i posted via the gmane webform ...

Oswald Buddenhagen wrote:

quoted

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

I assume you are referring to one-line commit messages becoming
two-line?

yes

Here's something rough to start.

i did a much simpler patch in that vein as well, but scrapped it again -
totally overengineered. the idea is to optimize the simple case -
cherry-pick -x, push. everything else needs amends anyway.

regards

Hi again.

Oswald Buddenhagen wrote:

i posted via the gmane webform ...

(See <http://thread.gmane.org/gmane.comp.version-control.git/154490>.
It's obnoxious that there is not an easier way, I agree.)

i did a much simpler patch in that vein as well, but scrapped it again -
totally overengineered. the idea is to optimize the simple case -
cherry-pick -x, push. everything else needs amends anyway.

It's been a vague wish of mine for a while to fix "cherry-pick -x -s".
I currently use it without amends and tolerate the blank line between
the "cherry picked" and the sign-off.

Maybe you can convince people that wanting no extra blank line between
an existing sign-off and the "cherry picked from" line is
overengineering and worth regressing.  I am not a fanatic about it ---
I just thought it was worth mentioning that this proposed change would
not be seen as positive by everyone.

Put another way, I don't find the two words "totally overengineered"
very convincing here.  For what it's worth.

Cheers,
Jonathan