[PATCH] cherry-pick: don't forget -s on failure

[PATCH] cherry-pick: don't forget -s on failure

[Miklos Vajna]

In case 'git cherry-pick -s <commit>' failed, the user had to use 'git
commit -s' (i.e. state the -s option again), which is easy to forget
about.  Instead, write the signed-off-by line early, so plain 'git
commit' will have the same result.

Signed-off-by: Miklos Vajna <vmiklos@suse.cz>
---

Hi,

See
http://article.gmane.org/gmane.comp.documentfoundation.libreoffice.devel/36103
for motivation. :-)

Given that I needed sign_off_header / ends_rfc2822_footer and some code
from builtin/commit.c in sequencer.c, I moved them there, let me know if
there is a place better than sequencer.c for them.

Thanks,

Miklos

 builtin/commit.c                |   63 ++------------------------------------
 sequencer.c                     |   65 +++++++++++++++++++++++++++++++++++++++
 sequencer.h                     |    4 ++
 t/t3507-cherry-pick-conflict.sh |    6 +++
 4 files changed, 78 insertions(+), 60 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 778cf16..7d0df9a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -28,6 +28,7 @@
 #include "submodule.h"
 #include "gpg-interface.h"
 #include "column.h"
+#include "sequencer.h"
 
 static const char * const builtin_commit_usage[] = {
 	N_("git commit [options] [--] <filepattern>..."),
@@ -466,8 +467,6 @@ static int is_a_merge(const struct commit *current_head)
 	return !!(current_head->parents && current_head->parents->next);
 }
 
-static const char sign_off_header[] = "Signed-off-by: ";
-
 static void export_one(const char *var, const char *s, const char *e, int hack)
 {
 	struct strbuf buf = STRBUF_INIT;
@@ -552,47 +551,6 @@ static void determine_author_info(struct strbuf *author_ident)
 	}
 }
 
-static int ends_rfc2822_footer(struct strbuf *sb)
-{
-	int ch;
-	int hit = 0;
-	int i, j, k;
-	int len = sb->len;
-	int first = 1;
-	const char *buf = sb->buf;
-
-	for (i = len - 1; i > 0; i--) {
-		if (hit && buf[i] == '\n')
-			break;
-		hit = (buf[i] == '\n');
-	}
-
-	while (i < len - 1 && buf[i] == '\n')
-		i++;
-
-	for (; i < len; i = k) {
-		for (k = i; k < len && buf[k] != '\n'; k++)
-			; /* do nothing */
-		k++;
-
-		if ((buf[k] == ' ' || buf[k] == '\t') && !first)
-			continue;
-
-		first = 0;
-
-		for (j = 0; i + j < len; j++) {
-			ch = buf[i + j];
-			if (ch == ':')
-				break;
-			if (isalnum(ch) ||
-			    (ch == '-'))
-				continue;
-			return 0;
-		}
-	}
-	return 1;
-}
-
 static char *cut_ident_timestamp_part(char *string)
 {
 	char *ket = strrchr(string, '>');
@@ -716,23 +674,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	if (clean_message_contents)
 		stripspace(&sb, 0);
 
-	if (signoff) {
-		struct strbuf sob = STRBUF_INIT;
-		int i;
-
-		strbuf_addstr(&sob, sign_off_header);
-		strbuf_addstr(&sob, fmt_name(getenv("GIT_COMMITTER_NAME"),
-					     getenv("GIT_COMMITTER_EMAIL")));
-		strbuf_addch(&sob, '\n');
-		for (i = sb.len - 1; i > 0 && sb.buf[i - 1] != '\n'; i--)
-			; /* do nothing */
-		if (prefixcmp(sb.buf + i, sob.buf)) {
-			if (!i || !ends_rfc2822_footer(&sb))
-				strbuf_addch(&sb, '\n');
-			strbuf_addbuf(&sb, &sob);
-		}
-		strbuf_release(&sob);
-	}
+	if (signoff)
+		append_signoff(&sb);
 
 	if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)
 		die_errno(_("could not write commit template"));
diff --git a/sequencer.c b/sequencer.c
index f86f116..402dcd6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -17,6 +17,8 @@
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
+const char sign_off_header[] = "Signed-off-by: ";
+
 void remove_sequencer_state(void)
 {
 	struct strbuf seq_dir = STRBUF_INIT;
@@ -249,6 +251,9 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 		}
 	}
 
+	if (opts->signoff)
+		append_signoff(msgbuf);
+
 	return !clean;
 }
 
@@ -1011,3 +1016,63 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 	save_opts(opts);
 	return pick_commits(todo_list, opts);
 }
+
+static int ends_rfc2822_footer(struct strbuf *sb)
+{
+	int ch;
+	int hit = 0;
+	int i, j, k;
+	int len = sb->len;
+	int first = 1;
+	const char *buf = sb->buf;
+
+	for (i = len - 1; i > 0; i--) {
+		if (hit && buf[i] == '\n')
+			break;
+		hit = (buf[i] == '\n');
+	}
+
+	while (i < len - 1 && buf[i] == '\n')
+		i++;
+
+	for (; i < len; i = k) {
+		for (k = i; k < len && buf[k] != '\n'; k++)
+			; /* do nothing */
+		k++;
+
+		if ((buf[k] == ' ' || buf[k] == '\t') && !first)
+			continue;
+
+		first = 0;
+
+		for (j = 0; i + j < len; j++) {
+			ch = buf[i + j];
+			if (ch == ':')
+				break;
+			if (isalnum(ch) ||
+			    (ch == '-'))
+				continue;
+			return 0;
+		}
+	}
+	return 1;
+}
+
+void append_signoff(struct strbuf *msgbuf)
+{
+	struct strbuf sob = STRBUF_INIT;
+	int i;
+
+	strbuf_addstr(&sob, sign_off_header);
+	strbuf_addstr(&sob, fmt_name(getenv("GIT_COMMITTER_NAME"),
+				getenv("GIT_COMMITTER_EMAIL")));
+	strbuf_addch(&sob, '\n');
+	for (i = msgbuf->len - 1; i > 0 && msgbuf->buf[i - 1] != '\n'; i--)
+		; /* do nothing */
+	if (prefixcmp(msgbuf->buf + i, sob.buf)) {
+		if (!i || !ends_rfc2822_footer(msgbuf))
+			strbuf_addch(msgbuf, '\n');
+		strbuf_addbuf(msgbuf, &sob);
+	}
+	strbuf_release(&sob);
+}
diff --git a/sequencer.h b/sequencer.h
index d849420..440b0c9 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -49,4 +49,8 @@ extern void remove_sequencer_state(void);
 
 int sequencer_pick_revisions(struct replay_opts *opts);
 
+extern const char sign_off_header[];
+
+void append_signoff(struct strbuf *msgbuf);
+
 #endif
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index 0c81b3c..eb52f1e 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -340,4 +340,10 @@ test_expect_success 'revert conflict, diff3 -m style' '
 	test_cmp expected actual
 '
 
+test_expect_success 'failed cherry-pick does not forget -s' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick -s picked &&
+	test_i18ngrep -e "Signed-off-by" .git/MERGE_MSG
+'
+
 test_done
-- 
1.7.7

Re: [PATCH] cherry-pick: don't forget -s on failure

[Junio C Hamano]

Miklos Vajna <vmiklos@suse.cz> writes:

> In case 'git cherry-pick -s <commit>' failed, the user had to use 'git
> commit -s' (i.e. state the -s option again), which is easy to forget
> about.  Instead, write the signed-off-by line early, so plain 'git
> commit' will have the same result.
>
> Signed-off-by: Miklos Vajna <vmiklos@suse.cz>
> ---
>
> Hi,
>
> See
> http://article.gmane.org/gmane.comp.documentfoundation.libreoffice.devel/36103
> for motivation. :-)
>
> Given that I needed sign_off_header / ends_rfc2822_footer and some code
> from builtin/commit.c in sequencer.c, I moved them there, let me know if
> there is a place better than sequencer.c for them.

I think we had a separate topic around cherry-pick that needs the
footer thing accessible from cherry-pick recently ($gmane/204755).

I think the code movement in this patch is a good one.

Thanks.

>
> Thanks,
>
> Miklos
>
>  builtin/commit.c                |   63 ++------------------------------------
>  sequencer.c                     |   65 +++++++++++++++++++++++++++++++++++++++
>  sequencer.h                     |    4 ++
>  t/t3507-cherry-pick-conflict.sh |    6 +++
>  4 files changed, 78 insertions(+), 60 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 778cf16..7d0df9a 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -28,6 +28,7 @@
>  #include "submodule.h"
>  #include "gpg-interface.h"
>  #include "column.h"
> +#include "sequencer.h"
>  
>  static const char * const builtin_commit_usage[] = {
>  	N_("git commit [options] [--] <filepattern>..."),
> @@ -466,8 +467,6 @@ static int is_a_merge(const struct commit *current_head)
>  	return !!(current_head->parents && current_head->parents->next);
>  }
>  
> -static const char sign_off_header[] = "Signed-off-by: ";
> -
>  static void export_one(const char *var, const char *s, const char *e, int hack)
>  {
>  	struct strbuf buf = STRBUF_INIT;
> @@ -552,47 +551,6 @@ static void determine_author_info(struct strbuf *author_ident)
>  	}
>  }
>  
> -static int ends_rfc2822_footer(struct strbuf *sb)
> -{
> -	int ch;
> -	int hit = 0;
> -	int i, j, k;
> -	int len = sb->len;
> -	int first = 1;
> -	const char *buf = sb->buf;
> -
> -	for (i = len - 1; i > 0; i--) {
> -		if (hit && buf[i] == '\n')
> -			break;
> -		hit = (buf[i] == '\n');
> -	}
> -
> -	while (i < len - 1 && buf[i] == '\n')
> -		i++;
> -
> -	for (; i < len; i = k) {
> -		for (k = i; k < len && buf[k] != '\n'; k++)
> -			; /* do nothing */
> -		k++;
> -
> -		if ((buf[k] == ' ' || buf[k] == '\t') && !first)
> -			continue;
> -
> -		first = 0;
> -
> -		for (j = 0; i + j < len; j++) {
> -			ch = buf[i + j];
> -			if (ch == ':')
> -				break;
> -			if (isalnum(ch) ||
> -			    (ch == '-'))
> -				continue;
> -			return 0;
> -		}
> -	}
> -	return 1;
> -}
> -
>  static char *cut_ident_timestamp_part(char *string)
>  {
>  	char *ket = strrchr(string, '>');
> @@ -716,23 +674,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  	if (clean_message_contents)
>  		stripspace(&sb, 0);
>  
> -	if (signoff) {
> -		struct strbuf sob = STRBUF_INIT;
> -		int i;
> -
> -		strbuf_addstr(&sob, sign_off_header);
> -		strbuf_addstr(&sob, fmt_name(getenv("GIT_COMMITTER_NAME"),
> -					     getenv("GIT_COMMITTER_EMAIL")));
> -		strbuf_addch(&sob, '\n');
> -		for (i = sb.len - 1; i > 0 && sb.buf[i - 1] != '\n'; i--)
> -			; /* do nothing */
> -		if (prefixcmp(sb.buf + i, sob.buf)) {
> -			if (!i || !ends_rfc2822_footer(&sb))
> -				strbuf_addch(&sb, '\n');
> -			strbuf_addbuf(&sb, &sob);
> -		}
> -		strbuf_release(&sob);
> -	}
> +	if (signoff)
> +		append_signoff(&sb);
>  
>  	if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)
>  		die_errno(_("could not write commit template"));
> diff --git a/sequencer.c b/sequencer.c
> index f86f116..402dcd6 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -17,6 +17,8 @@
>  
>  #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
>  
> +const char sign_off_header[] = "Signed-off-by: ";
> +
>  void remove_sequencer_state(void)
>  {
>  	struct strbuf seq_dir = STRBUF_INIT;
> @@ -249,6 +251,9 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
>  		}
>  	}
>  
> +	if (opts->signoff)
> +		append_signoff(msgbuf);
> +
>  	return !clean;
>  }
>  
> @@ -1011,3 +1016,63 @@ int sequencer_pick_revisions(struct replay_opts *opts)
>  	save_opts(opts);
>  	return pick_commits(todo_list, opts);
>  }
> +
> +static int ends_rfc2822_footer(struct strbuf *sb)
> +{
> +	int ch;
> +	int hit = 0;
> +	int i, j, k;
> +	int len = sb->len;
> +	int first = 1;
> +	const char *buf = sb->buf;
> +
> +	for (i = len - 1; i > 0; i--) {
> +		if (hit && buf[i] == '\n')
> +			break;
> +		hit = (buf[i] == '\n');
> +	}
> +
> +	while (i < len - 1 && buf[i] == '\n')
> +		i++;
> +
> +	for (; i < len; i = k) {
> +		for (k = i; k < len && buf[k] != '\n'; k++)
> +			; /* do nothing */
> +		k++;
> +
> +		if ((buf[k] == ' ' || buf[k] == '\t') && !first)
> +			continue;
> +
> +		first = 0;
> +
> +		for (j = 0; i + j < len; j++) {
> +			ch = buf[i + j];
> +			if (ch == ':')
> +				break;
> +			if (isalnum(ch) ||
> +			    (ch == '-'))
> +				continue;
> +			return 0;
> +		}
> +	}
> +	return 1;
> +}
> +
> +void append_signoff(struct strbuf *msgbuf)
> +{
> +	struct strbuf sob = STRBUF_INIT;
> +	int i;
> +
> +	strbuf_addstr(&sob, sign_off_header);
> +	strbuf_addstr(&sob, fmt_name(getenv("GIT_COMMITTER_NAME"),
> +				getenv("GIT_COMMITTER_EMAIL")));
> +	strbuf_addch(&sob, '\n');
> +	for (i = msgbuf->len - 1; i > 0 && msgbuf->buf[i - 1] != '\n'; i--)
> +		; /* do nothing */
> +	if (prefixcmp(msgbuf->buf + i, sob.buf)) {
> +		if (!i || !ends_rfc2822_footer(msgbuf))
> +			strbuf_addch(msgbuf, '\n');
> +		strbuf_addbuf(msgbuf, &sob);
> +	}
> +	strbuf_release(&sob);
> +}
> diff --git a/sequencer.h b/sequencer.h
> index d849420..440b0c9 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -49,4 +49,8 @@ extern void remove_sequencer_state(void);
>  
>  int sequencer_pick_revisions(struct replay_opts *opts);
>  
> +extern const char sign_off_header[];
> +
> +void append_signoff(struct strbuf *msgbuf);
> +
>  #endif
> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
> index 0c81b3c..eb52f1e 100755
> --- a/t/t3507-cherry-pick-conflict.sh
> +++ b/t/t3507-cherry-pick-conflict.sh
> @@ -340,4 +340,10 @@ test_expect_success 'revert conflict, diff3 -m style' '
>  	test_cmp expected actual
>  '
>  
> +test_expect_success 'failed cherry-pick does not forget -s' '
> +	pristine_detach initial &&
> +	test_must_fail git cherry-pick -s picked &&
> +	test_i18ngrep -e "Signed-off-by" .git/MERGE_MSG
> +'
> +
>  test_done

Re: [PATCH] cherry-pick: don't forget -s on failure

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I think we had a separate topic around cherry-pick that needs the
> footer thing accessible from cherry-pick recently ($gmane/204755).
>
> I think the code movement in this patch is a good one.
>
> Thanks.

Having said that, the behaviour after this patch is applied is not
quite right.

A typical .git/MERGE_MSG that is left after "cherry-pick" gives the
control back to you asking for help, with your patch that adds the
sign-off at the end, would look like this:


    cherry-pick: don't forget -s on failure

    In case 'git cherry-pick -s <commit>' failed, the user had to use 'git
    commit -s' (i.e. state the -s option again), which is easy to forget
    about.  Instead, write the signed-off-by line early, so plain 'git
    commit' will have the same result.

    Signed-off-by: Miklos Vajna <vmiklos@suse.cz>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

    Conflicts:
            builtin/commit.c

    Signed-off-by: Junio C Hamano <gitster@pobox.com>


Notice two issues?

 - The additional S-o-b should come immediately after the existing
   block of footers.

 - And the last entry in the existing footer block is already mine,
   so there shouldn't have been a new and duplicated one added.


I am not sure how reusable the moved function is without
enhancements for your purpose.  The logic to identify the footer
needs to be enhanced so that an "end" pointer to point at the byte
before the caller added "Conflicts: " can be given, and pretend as
if it is the end of the buffer, unlike in the fresh commit case
where it can consider the real end of the buffer as such.

Or something like that.

[PATCH v2] cherry-pick: don't forget -s on failure

[Miklos Vajna]

In case 'git cherry-pick -s <commit>' failed, the user had to use 'git
commit -s' (i.e. state the -s option again), which is easy to forget
about.  Instead, write the signed-off-by line early, so plain 'git
commit' will have the same result.

Also update 'git commit -s', so that in case there is already a relevant
Signed-off-by line before the Conflicts: line, it won't add one more at
the end of the message.

Signed-off-by: Miklos Vajna <vmiklos@suse.cz>
---

On Wed, Sep 12, 2012 at 03:45:10PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
>  - The additional S-o-b should come immediately after the existing
>    block of footers.

This was trivial to fix.

>  - And the last entry in the existing footer block is already mine,
>    so there shouldn't have been a new and duplicated one added.
> 
> 
> I am not sure how reusable the moved function is without
> enhancements for your purpose.  The logic to identify the footer
> needs to be enhanced so that an "end" pointer to point at the byte
> before the caller added "Conflicts: " can be given, and pretend as
> if it is the end of the buffer, unlike in the fresh commit case
> where it can consider the real end of the buffer as such.

Below is what I came up with. It simply ignores anything after the 
Conclicts: line, when checking for the last Signed-off-by line.

An other thing: I forgot to run 'make test' for the initial patch, it 
seems t3510-cherry-pick-sequence.sh has 3 tests that basically ensures 
the opposite of what my patch does. Given that there are already 
testcases for the new behaviour, can they be just removed? For now, I 
just disabled them.

 builtin/commit.c                |   79 +++++++++++---------------------------
 sequencer.c                     |   65 ++++++++++++++++++++++++++++++++
 sequencer.h                     |    4 ++
 t/t3507-cherry-pick-conflict.sh |   14 +++++++
 t/t3510-cherry-pick-sequence.sh |    6 +-
 t/test-lib-functions.sh         |    8 +++-
 6 files changed, 116 insertions(+), 60 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 778cf16..4d50484 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -28,6 +28,7 @@
 #include "submodule.h"
 #include "gpg-interface.h"
 #include "column.h"
+#include "sequencer.h"
 
 static const char * const builtin_commit_usage[] = {
 	N_("git commit [options] [--] <filepattern>..."),
@@ -466,8 +467,6 @@ static int is_a_merge(const struct commit *current_head)
 	return !!(current_head->parents && current_head->parents->next);
 }
 
-static const char sign_off_header[] = "Signed-off-by: ";
-
 static void export_one(const char *var, const char *s, const char *e, int hack)
 {
 	struct strbuf buf = STRBUF_INIT;
@@ -552,47 +551,6 @@ static void determine_author_info(struct strbuf *author_ident)
 	}
 }
 
-static int ends_rfc2822_footer(struct strbuf *sb)
-{
-	int ch;
-	int hit = 0;
-	int i, j, k;
-	int len = sb->len;
-	int first = 1;
-	const char *buf = sb->buf;
-
-	for (i = len - 1; i > 0; i--) {
-		if (hit && buf[i] == '\n')
-			break;
-		hit = (buf[i] == '\n');
-	}
-
-	while (i < len - 1 && buf[i] == '\n')
-		i++;
-
-	for (; i < len; i = k) {
-		for (k = i; k < len && buf[k] != '\n'; k++)
-			; /* do nothing */
-		k++;
-
-		if ((buf[k] == ' ' || buf[k] == '\t') && !first)
-			continue;
-
-		first = 0;
-
-		for (j = 0; i + j < len; j++) {
-			ch = buf[i + j];
-			if (ch == ':')
-				break;
-			if (isalnum(ch) ||
-			    (ch == '-'))
-				continue;
-			return 0;
-		}
-	}
-	return 1;
-}
-
 static char *cut_ident_timestamp_part(char *string)
 {
 	char *ket = strrchr(string, '>');
@@ -717,21 +675,30 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		stripspace(&sb, 0);
 
 	if (signoff) {
-		struct strbuf sob = STRBUF_INIT;
-		int i;
+		/*
+		 * See if we have a Conflicts: block at the end. If yes, count
+		 * its size, so we can ignore it.
+		 */
+		int ignore_footer = 0;
+		int i, eol, previous = 0;
+		const char *nl;
 
-		strbuf_addstr(&sob, sign_off_header);
-		strbuf_addstr(&sob, fmt_name(getenv("GIT_COMMITTER_NAME"),
-					     getenv("GIT_COMMITTER_EMAIL")));
-		strbuf_addch(&sob, '\n');
-		for (i = sb.len - 1; i > 0 && sb.buf[i - 1] != '\n'; i--)
-			; /* do nothing */
-		if (prefixcmp(sb.buf + i, sob.buf)) {
-			if (!i || !ends_rfc2822_footer(&sb))
-				strbuf_addch(&sb, '\n');
-			strbuf_addbuf(&sb, &sob);
+		for (i = 0; i < sb.len; i++) {
+			nl = memchr(sb.buf + i, '\n', sb.len - i);
+			if (nl)
+				eol = nl - sb.buf;
+			else
+				eol = sb.len;
+			if (!prefixcmp(sb.buf + previous, "\nConflicts:\n")) {
+				ignore_footer = sb.len - previous;
+				break;
+			}
+			while (i < eol)
+				i++;
+			previous = eol;
 		}
-		strbuf_release(&sob);
+
+		append_signoff(&sb, ignore_footer);
 	}
 
 	if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)
diff --git a/sequencer.c b/sequencer.c
index f86f116..e691b08 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -17,6 +17,8 @@
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
+const char sign_off_header[] = "Signed-off-by: ";
+
 void remove_sequencer_state(void)
 {
 	struct strbuf seq_dir = STRBUF_INIT;
@@ -233,6 +235,9 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 		die(_("%s: Unable to write new index file"), action_name(opts));
 	rollback_lock_file(&index_lock);
 
+	if (opts->signoff)
+		append_signoff(msgbuf, 0);
+
 	if (!clean) {
 		int i;
 		strbuf_addstr(msgbuf, "\nConflicts:\n");
@@ -1011,3 +1016,63 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 	save_opts(opts);
 	return pick_commits(todo_list, opts);
 }
+
+static int ends_rfc2822_footer(struct strbuf *sb)
+{
+	int ch;
+	int hit = 0;
+	int i, j, k;
+	int len = sb->len;
+	int first = 1;
+	const char *buf = sb->buf;
+
+	for (i = len - 1; i > 0; i--) {
+		if (hit && buf[i] == '\n')
+			break;
+		hit = (buf[i] == '\n');
+	}
+
+	while (i < len - 1 && buf[i] == '\n')
+		i++;
+
+	for (; i < len; i = k) {
+		for (k = i; k < len && buf[k] != '\n'; k++)
+			; /* do nothing */
+		k++;
+
+		if ((buf[k] == ' ' || buf[k] == '\t') && !first)
+			continue;
+
+		first = 0;
+
+		for (j = 0; i + j < len; j++) {
+			ch = buf[i + j];
+			if (ch == ':')
+				break;
+			if (isalnum(ch) ||
+			    (ch == '-'))
+				continue;
+			return 0;
+		}
+	}
+	return 1;
+}
+
+void append_signoff(struct strbuf *msgbuf, int ignore_footer)
+{
+	struct strbuf sob = STRBUF_INIT;
+	int i;
+
+	strbuf_addstr(&sob, sign_off_header);
+	strbuf_addstr(&sob, fmt_name(getenv("GIT_COMMITTER_NAME"),
+				getenv("GIT_COMMITTER_EMAIL")));
+	strbuf_addch(&sob, '\n');
+	for (i = msgbuf->len - 1 - ignore_footer; i > 0 && msgbuf->buf[i - 1] != '\n'; i--)
+		; /* do nothing */
+	if (prefixcmp(msgbuf->buf + i, sob.buf)) {
+		if (!i || !ends_rfc2822_footer(msgbuf))
+			strbuf_addch(msgbuf, '\n');
+		strbuf_addbuf(msgbuf, &sob);
+	}
+	strbuf_release(&sob);
+}
diff --git a/sequencer.h b/sequencer.h
index d849420..60287b8 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -49,4 +49,8 @@ extern void remove_sequencer_state(void);
 
 int sequencer_pick_revisions(struct replay_opts *opts);
 
+extern const char sign_off_header[];
+
+void append_signoff(struct strbuf *msgbuf, int ignore_footer);
+
 #endif
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index 0c81b3c..b67756f 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -30,6 +30,7 @@ test_expect_success setup '
 	test_commit initial foo a &&
 	test_commit base foo b &&
 	test_commit picked foo c &&
+	test_commit --signoff picked-signed foo d &&
 	git config advice.detachedhead false
 
 '
@@ -340,4 +341,17 @@ test_expect_success 'revert conflict, diff3 -m style' '
 	test_cmp expected actual
 '
 
+test_expect_success 'failed cherry-pick does not forget -s' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick -s picked &&
+	test_i18ngrep -e "Signed-off-by" .git/MERGE_MSG
+'
+
+test_expect_success 'commit after failed cherry-pick does not add duplicated -s' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick -s picked-signed &&
+	git commit -a -s &&
+	test $(git show -s |grep -c "Signed-off-by") = 1
+'
+
 test_done
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index f4e6450..b5fb527 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -410,7 +410,7 @@ test_expect_success '--continue respects -x in first commit in multi-pick' '
 	grep "cherry picked from.*$picked" msg
 '
 
-test_expect_success '--signoff is not automatically propagated to resolved conflict' '
+test_expect_failure '--signoff is automatically propagated to resolved conflict' '
 	pristine_detach initial &&
 	test_expect_code 1 git cherry-pick --signoff base..anotherpick &&
 	echo "c" >foo &&
@@ -428,7 +428,7 @@ test_expect_success '--signoff is not automatically propagated to resolved confl
 	grep "Signed-off-by:" anotherpick_msg
 '
 
-test_expect_success '--signoff dropped for implicit commit of resolution, multi-pick case' '
+test_expect_failure '--signoff dropped for implicit commit of resolution, multi-pick case' '
 	pristine_detach initial &&
 	test_must_fail git cherry-pick -s picked anotherpick &&
 	echo c >foo &&
@@ -441,7 +441,7 @@ test_expect_success '--signoff dropped for implicit commit of resolution, multi-
 	! grep Signed-off-by: msg
 '
 
-test_expect_success 'sign-off needs to be reaffirmed after conflict resolution, single-pick case' '
+test_expect_failure 'sign-off needs to be reaffirmed after conflict resolution, single-pick case' '
 	pristine_detach initial &&
 	test_must_fail git cherry-pick -s picked &&
 	echo c >foo &&
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 9bc57d2..bbb7b7d 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -149,6 +149,12 @@ test_commit () {
 		notick=yes
 		shift
 	fi &&
+	signoff= &&
+	if test "z$1" = "z--signoff"
+	then
+		signoff="$1"
+		shift
+	fi &&
 	file=${2:-"$1.t"} &&
 	echo "${3-$1}" > "$file" &&
 	git add "$file" &&
@@ -156,7 +162,7 @@ test_commit () {
 	then
 		test_tick
 	fi &&
-	git commit -m "$1" &&
+	git commit $signoff -m "$1" &&
 	git tag "$1"
 }
 
-- 
1.7.7

Re: [PATCH v2] cherry-pick: don't forget -s on failure

[Junio C Hamano]

Miklos Vajna <vmiklos@suse.cz> writes:

> In case 'git cherry-pick -s <commit>' failed, the user had to use 'git
> commit -s' (i.e. state the -s option again), which is easy to forget
> about.  Instead, write the signed-off-by line early, so plain 'git
> commit' will have the same result.
>
> Also update 'git commit -s', so that in case there is already a relevant
> Signed-off-by line before the Conflicts: line, it won't add one more at
> the end of the message.
>
> Signed-off-by: Miklos Vajna <vmiklos@suse.cz>
> ---
>
> On Wed, Sep 12, 2012 at 03:45:10PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
>>  - The additional S-o-b should come immediately after the existing
>>    block of footers.
>
> This was trivial to fix.

Indeed.  Just inserting before starting to add "Oh, there were
conflicts, and add the info on them" before doing it at the end is
all it takes.  Simple and straightforward---I like it.

> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -149,6 +149,12 @@ test_commit () {
>  		notick=yes
>  		shift
>  	fi &&
> +	signoff= &&
> +	if test "z$1" = "z--signoff"
> +	then
> +		signoff="$1"
> +		shift
> +	fi &&
>  	file=${2:-"$1.t"} &&
>  	echo "${3-$1}" > "$file" &&
>  	git add "$file" &&

This is somewhat iffy.  Shouldn't "test_commit --signoff --notick" work?

> @@ -156,7 +162,7 @@ test_commit () {
>  	then
>  		test_tick
>  	fi &&
> -	git commit -m "$1" &&
> +	git commit $signoff -m "$1" &&
>  	git tag "$1"
>  }

Thanks.

Re: [PATCH v2] cherry-pick: don't forget -s on failure

[Junio C Hamano]

Miklos Vajna <vmiklos@suse.cz> writes:

> +void append_signoff(struct strbuf *msgbuf, int ignore_footer)
> +{
> +	struct strbuf sob = STRBUF_INIT;
> +	int i;
> +
> +	strbuf_addstr(&sob, sign_off_header);
> +	strbuf_addstr(&sob, fmt_name(getenv("GIT_COMMITTER_NAME"),
> +				getenv("GIT_COMMITTER_EMAIL")));
> +	strbuf_addch(&sob, '\n');
> +	for (i = msgbuf->len - 1 - ignore_footer; i > 0 && msgbuf->buf[i - 1] != '\n'; i--)
> +		; /* do nothing */
> +	if (prefixcmp(msgbuf->buf + i, sob.buf)) {
> +		if (!i || !ends_rfc2822_footer(msgbuf))
> +			strbuf_addch(msgbuf, '\n');
> +		strbuf_addbuf(msgbuf, &sob);
> +	}
> +	strbuf_release(&sob);
> +}

Hrm, what is this thing trying to do?  It does start scanning from
the end (ignoring the "Conflicts:" thing) to see who the last person
that signed it off was, but once it decides that it needs to add a
new sign-off, it still adds it at the very end anyway.

[PATCH v3] cherry-pick: don't forget -s on failure

[Miklos Vajna]

In case 'git cherry-pick -s <commit>' failed, the user had to use 'git
commit -s' (i.e. state the -s option again), which is easy to forget
about.  Instead, write the signed-off-by line early, so plain 'git
commit' will have the same result.

Also update 'git commit -s', so that in case there is already a relevant
Signed-off-by line before the Conflicts: line, it won't add one more at
the end of the message. If there is no such line, then add it before the
the Conflicts: line.

Signed-off-by: Miklos Vajna <vmiklos@suse.cz>
---

> This is somewhat iffy.  Shouldn't "test_commit --signoff --notick"
> work?

Indeed, fixed now.

> Hrm, what is this thing trying to do?  It does start scanning from
> the end (ignoring the "Conflicts:" thing) to see who the last person
> that signed it off was, but once it decides that it needs to add a
> new sign-off, it still adds it at the very end anyway.

Ah, I did not handle that, as the original git commit -s didn't do it 
either -- and originally I just wanted to touch git cherry-pick. 
Implemented now.

 builtin/commit.c                |   79 +++++++++++---------------------------
 sequencer.c                     |   72 +++++++++++++++++++++++++++++++++++
 sequencer.h                     |    4 ++
 t/t3507-cherry-pick-conflict.sh |   32 ++++++++++++++++
 t/t3510-cherry-pick-sequence.sh |    6 +-
 t/test-lib-functions.sh         |   20 +++++++--
 6 files changed, 149 insertions(+), 64 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 778cf16..4d50484 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -28,6 +28,7 @@
 #include "submodule.h"
 #include "gpg-interface.h"
 #include "column.h"
+#include "sequencer.h"
 
 static const char * const builtin_commit_usage[] = {
 	N_("git commit [options] [--] <filepattern>..."),
@@ -466,8 +467,6 @@ static int is_a_merge(const struct commit *current_head)
 	return !!(current_head->parents && current_head->parents->next);
 }
 
-static const char sign_off_header[] = "Signed-off-by: ";
-
 static void export_one(const char *var, const char *s, const char *e, int hack)
 {
 	struct strbuf buf = STRBUF_INIT;
@@ -552,47 +551,6 @@ static void determine_author_info(struct strbuf *author_ident)
 	}
 }
 
-static int ends_rfc2822_footer(struct strbuf *sb)
-{
-	int ch;
-	int hit = 0;
-	int i, j, k;
-	int len = sb->len;
-	int first = 1;
-	const char *buf = sb->buf;
-
-	for (i = len - 1; i > 0; i--) {
-		if (hit && buf[i] == '\n')
-			break;
-		hit = (buf[i] == '\n');
-	}
-
-	while (i < len - 1 && buf[i] == '\n')
-		i++;
-
-	for (; i < len; i = k) {
-		for (k = i; k < len && buf[k] != '\n'; k++)
-			; /* do nothing */
-		k++;
-
-		if ((buf[k] == ' ' || buf[k] == '\t') && !first)
-			continue;
-
-		first = 0;
-
-		for (j = 0; i + j < len; j++) {
-			ch = buf[i + j];
-			if (ch == ':')
-				break;
-			if (isalnum(ch) ||
-			    (ch == '-'))
-				continue;
-			return 0;
-		}
-	}
-	return 1;
-}
-
 static char *cut_ident_timestamp_part(char *string)
 {
 	char *ket = strrchr(string, '>');
@@ -717,21 +675,30 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		stripspace(&sb, 0);
 
 	if (signoff) {
-		struct strbuf sob = STRBUF_INIT;
-		int i;
+		/*
+		 * See if we have a Conflicts: block at the end. If yes, count
+		 * its size, so we can ignore it.
+		 */
+		int ignore_footer = 0;
+		int i, eol, previous = 0;
+		const char *nl;
 
-		strbuf_addstr(&sob, sign_off_header);
-		strbuf_addstr(&sob, fmt_name(getenv("GIT_COMMITTER_NAME"),
-					     getenv("GIT_COMMITTER_EMAIL")));
-		strbuf_addch(&sob, '\n');
-		for (i = sb.len - 1; i > 0 && sb.buf[i - 1] != '\n'; i--)
-			; /* do nothing */
-		if (prefixcmp(sb.buf + i, sob.buf)) {
-			if (!i || !ends_rfc2822_footer(&sb))
-				strbuf_addch(&sb, '\n');
-			strbuf_addbuf(&sb, &sob);
+		for (i = 0; i < sb.len; i++) {
+			nl = memchr(sb.buf + i, '\n', sb.len - i);
+			if (nl)
+				eol = nl - sb.buf;
+			else
+				eol = sb.len;
+			if (!prefixcmp(sb.buf + previous, "\nConflicts:\n")) {
+				ignore_footer = sb.len - previous;
+				break;
+			}
+			while (i < eol)
+				i++;
+			previous = eol;
 		}
-		strbuf_release(&sob);
+
+		append_signoff(&sb, ignore_footer);
 	}
 
 	if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)
diff --git a/sequencer.c b/sequencer.c
index f86f116..4420807 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -17,6 +17,8 @@
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
+const char sign_off_header[] = "Signed-off-by: ";
+
 void remove_sequencer_state(void)
 {
 	struct strbuf seq_dir = STRBUF_INIT;
@@ -233,6 +235,9 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 		die(_("%s: Unable to write new index file"), action_name(opts));
 	rollback_lock_file(&index_lock);
 
+	if (opts->signoff)
+		append_signoff(msgbuf, 0);
+
 	if (!clean) {
 		int i;
 		strbuf_addstr(msgbuf, "\nConflicts:\n");
@@ -1011,3 +1016,70 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 	save_opts(opts);
 	return pick_commits(todo_list, opts);
 }
+
+static int ends_rfc2822_footer(struct strbuf *sb)
+{
+	int ch;
+	int hit = 0;
+	int i, j, k;
+	int len = sb->len;
+	int first = 1;
+	const char *buf = sb->buf;
+
+	for (i = len - 1; i > 0; i--) {
+		if (hit && buf[i] == '\n')
+			break;
+		hit = (buf[i] == '\n');
+	}
+
+	while (i < len - 1 && buf[i] == '\n')
+		i++;
+
+	for (; i < len; i = k) {
+		for (k = i; k < len && buf[k] != '\n'; k++)
+			; /* do nothing */
+		k++;
+
+		if ((buf[k] == ' ' || buf[k] == '\t') && !first)
+			continue;
+
+		first = 0;
+
+		for (j = 0; i + j < len; j++) {
+			ch = buf[i + j];
+			if (ch == ':')
+				break;
+			if (isalnum(ch) ||
+			    (ch == '-'))
+				continue;
+			return 0;
+		}
+	}
+	return 1;
+}
+
+void append_signoff(struct strbuf *msgbuf, int ignore_footer)
+{
+	struct strbuf sob = STRBUF_INIT;
+	int i;
+
+	strbuf_addstr(&sob, sign_off_header);
+	strbuf_addstr(&sob, fmt_name(getenv("GIT_COMMITTER_NAME"),
+				getenv("GIT_COMMITTER_EMAIL")));
+	strbuf_addch(&sob, '\n');
+	for (i = msgbuf->len - 1 - ignore_footer; i > 0 && msgbuf->buf[i - 1] != '\n'; i--)
+		; /* do nothing */
+	struct strbuf footer = STRBUF_INIT;
+	if (ignore_footer > 0) {
+		strbuf_addstr(&footer, msgbuf->buf + msgbuf->len - ignore_footer);
+		strbuf_setlen(msgbuf, msgbuf->len - ignore_footer);
+	}
+	if (prefixcmp(msgbuf->buf + i, sob.buf)) {
+		if (!i || !ends_rfc2822_footer(msgbuf))
+			strbuf_addch(msgbuf, '\n');
+		strbuf_addbuf(msgbuf, &sob);
+	}
+	strbuf_release(&sob);
+	strbuf_addbuf(msgbuf, &footer);
+	strbuf_release(&footer);
+}
diff --git a/sequencer.h b/sequencer.h
index d849420..60287b8 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -49,4 +49,8 @@ extern void remove_sequencer_state(void);
 
 int sequencer_pick_revisions(struct replay_opts *opts);
 
+extern const char sign_off_header[];
+
+void append_signoff(struct strbuf *msgbuf, int ignore_footer);
+
 #endif
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index 0c81b3c..c82f721 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -30,6 +30,7 @@ test_expect_success setup '
 	test_commit initial foo a &&
 	test_commit base foo b &&
 	test_commit picked foo c &&
+	test_commit --signoff picked-signed foo d &&
 	git config advice.detachedhead false
 
 '
@@ -340,4 +341,35 @@ test_expect_success 'revert conflict, diff3 -m style' '
 	test_cmp expected actual
 '
 
+test_expect_success 'failed cherry-pick does not forget -s' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick -s picked &&
+	test_i18ngrep -e "Signed-off-by" .git/MERGE_MSG
+'
+
+test_expect_success 'commit after failed cherry-pick does not add duplicated -s' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick -s picked-signed &&
+	git commit -a -s &&
+	test $(git show -s |grep -c "Signed-off-by") = 1
+'
+
+test_expect_success 'commit after failed cherry-pick adds -s at the right place' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick picked &&
+	git commit -a -s &&
+	pwd &&
+	cat <<EOF > expected &&
+picked
+
+Signed-off-by: C O Mitter <committer@example.com>
+
+Conflicts:
+	foo
+EOF
+
+	git show -s --pretty=format:%B > actual &&
+	test_cmp expected actual
+'
+
 test_done
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index f4e6450..b5fb527 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -410,7 +410,7 @@ test_expect_success '--continue respects -x in first commit in multi-pick' '
 	grep "cherry picked from.*$picked" msg
 '
 
-test_expect_success '--signoff is not automatically propagated to resolved conflict' '
+test_expect_failure '--signoff is automatically propagated to resolved conflict' '
 	pristine_detach initial &&
 	test_expect_code 1 git cherry-pick --signoff base..anotherpick &&
 	echo "c" >foo &&
@@ -428,7 +428,7 @@ test_expect_success '--signoff is not automatically propagated to resolved confl
 	grep "Signed-off-by:" anotherpick_msg
 '
 
-test_expect_success '--signoff dropped for implicit commit of resolution, multi-pick case' '
+test_expect_failure '--signoff dropped for implicit commit of resolution, multi-pick case' '
 	pristine_detach initial &&
 	test_must_fail git cherry-pick -s picked anotherpick &&
 	echo c >foo &&
@@ -441,7 +441,7 @@ test_expect_success '--signoff dropped for implicit commit of resolution, multi-
 	! grep Signed-off-by: msg
 '
 
-test_expect_success 'sign-off needs to be reaffirmed after conflict resolution, single-pick case' '
+test_expect_failure 'sign-off needs to be reaffirmed after conflict resolution, single-pick case' '
 	pristine_detach initial &&
 	test_must_fail git cherry-pick -s picked &&
 	echo c >foo &&
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 9bc57d2..0802da5 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -144,11 +144,21 @@ test_pause () {
 
 test_commit () {
 	notick= &&
-	if test "z$1" = "z--notick"
-	then
-		notick=yes
+	signoff= &&
+	while test $# != 0; do
+		case "$1" in
+		--notick)
+			notick=yes
+			;;
+		--signoff)
+			signoff="$1"
+			;;
+		*)
+			break
+			;;
+		esac
 		shift
-	fi &&
+	done &&
 	file=${2:-"$1.t"} &&
 	echo "${3-$1}" > "$file" &&
 	git add "$file" &&
@@ -156,7 +166,7 @@ test_commit () {
 	then
 		test_tick
 	fi &&
-	git commit -m "$1" &&
+	git commit $signoff -m "$1" &&
 	git tag "$1"
 }
 
-- 
1.7.7

Re: [PATCH v3] cherry-pick: don't forget -s on failure

[Junio C Hamano]

Miklos Vajna <vmiklos@suse.cz> writes:

> +void append_signoff(struct strbuf *msgbuf, int ignore_footer)
> +{
> +	struct strbuf sob = STRBUF_INIT;
> +	int i;
> +
> +	strbuf_addstr(&sob, sign_off_header);
> +	strbuf_addstr(&sob, fmt_name(getenv("GIT_COMMITTER_NAME"),
> +				getenv("GIT_COMMITTER_EMAIL")));
> +	strbuf_addch(&sob, '\n');
> +	for (i = msgbuf->len - 1 - ignore_footer; i > 0 && msgbuf->buf[i - 1] != '\n'; i--)
> +		; /* do nothing */
> +	struct strbuf footer = STRBUF_INIT;
> +	if (ignore_footer > 0) {
> +		strbuf_addstr(&footer, msgbuf->buf + msgbuf->len - ignore_footer);
> +		strbuf_setlen(msgbuf, msgbuf->len - ignore_footer);
> +	}

That's decl-after-stmt.

I would have expected that you can just do strbuf_splice() to add
the &sob into &msgbuf with the original code structure, without a
substantial rewrite of the function like this.  Perhaps I am missing
something?

[PATCH v4] cherry-pick: don't forget -s on failure

[Miklos Vajna]

In case 'git cherry-pick -s <commit>' failed, the user had to use 'git
commit -s' (i.e. state the -s option again), which is easy to forget
about.  Instead, write the signed-off-by line early, so plain 'git
commit' will have the same result.

Also update 'git commit -s', so that in case there is already a relevant
Signed-off-by line before the Conflicts: line, it won't add one more at
the end of the message. If there is no such line, then add it before the
the Conflicts: line.

Signed-off-by: Miklos Vajna <vmiklos@suse.cz>
---

On Thu, Sep 13, 2012 at 02:13:46PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> That's decl-after-stmt.

Sorry, added -Wdeclaration-after-statement to CFLAGS now.

> I would have expected that you can just do strbuf_splice() to add
> the &sob into &msgbuf with the original code structure, without a
> substantial rewrite of the function like this.  Perhaps I am missing
> something?

I forgot about strbuf_splice(). ;-) Here is a version with it -- it's 
indeed shorter, even if ends_rfc2822_footer() now has to be aware of a 
possible footer.

 builtin/commit.c                |   79 +++++++++++---------------------------
 sequencer.c                     |   65 ++++++++++++++++++++++++++++++++
 sequencer.h                     |    4 ++
 t/t3507-cherry-pick-conflict.sh |   32 ++++++++++++++++
 t/t3510-cherry-pick-sequence.sh |    6 +-
 t/test-lib-functions.sh         |   20 +++++++--
 6 files changed, 142 insertions(+), 64 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 778cf16..4d50484 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -28,6 +28,7 @@
 #include "submodule.h"
 #include "gpg-interface.h"
 #include "column.h"
+#include "sequencer.h"
 
 static const char * const builtin_commit_usage[] = {
 	N_("git commit [options] [--] <filepattern>..."),
@@ -466,8 +467,6 @@ static int is_a_merge(const struct commit *current_head)
 	return !!(current_head->parents && current_head->parents->next);
 }
 
-static const char sign_off_header[] = "Signed-off-by: ";
-
 static void export_one(const char *var, const char *s, const char *e, int hack)
 {
 	struct strbuf buf = STRBUF_INIT;
@@ -552,47 +551,6 @@ static void determine_author_info(struct strbuf *author_ident)
 	}
 }
 
-static int ends_rfc2822_footer(struct strbuf *sb)
-{
-	int ch;
-	int hit = 0;
-	int i, j, k;
-	int len = sb->len;
-	int first = 1;
-	const char *buf = sb->buf;
-
-	for (i = len - 1; i > 0; i--) {
-		if (hit && buf[i] == '\n')
-			break;
-		hit = (buf[i] == '\n');
-	}
-
-	while (i < len - 1 && buf[i] == '\n')
-		i++;
-
-	for (; i < len; i = k) {
-		for (k = i; k < len && buf[k] != '\n'; k++)
-			; /* do nothing */
-		k++;
-
-		if ((buf[k] == ' ' || buf[k] == '\t') && !first)
-			continue;
-
-		first = 0;
-
-		for (j = 0; i + j < len; j++) {
-			ch = buf[i + j];
-			if (ch == ':')
-				break;
-			if (isalnum(ch) ||
-			    (ch == '-'))
-				continue;
-			return 0;
-		}
-	}
-	return 1;
-}
-
 static char *cut_ident_timestamp_part(char *string)
 {
 	char *ket = strrchr(string, '>');
@@ -717,21 +675,30 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		stripspace(&sb, 0);
 
 	if (signoff) {
-		struct strbuf sob = STRBUF_INIT;
-		int i;
+		/*
+		 * See if we have a Conflicts: block at the end. If yes, count
+		 * its size, so we can ignore it.
+		 */
+		int ignore_footer = 0;
+		int i, eol, previous = 0;
+		const char *nl;
 
-		strbuf_addstr(&sob, sign_off_header);
-		strbuf_addstr(&sob, fmt_name(getenv("GIT_COMMITTER_NAME"),
-					     getenv("GIT_COMMITTER_EMAIL")));
-		strbuf_addch(&sob, '\n');
-		for (i = sb.len - 1; i > 0 && sb.buf[i - 1] != '\n'; i--)
-			; /* do nothing */
-		if (prefixcmp(sb.buf + i, sob.buf)) {
-			if (!i || !ends_rfc2822_footer(&sb))
-				strbuf_addch(&sb, '\n');
-			strbuf_addbuf(&sb, &sob);
+		for (i = 0; i < sb.len; i++) {
+			nl = memchr(sb.buf + i, '\n', sb.len - i);
+			if (nl)
+				eol = nl - sb.buf;
+			else
+				eol = sb.len;
+			if (!prefixcmp(sb.buf + previous, "\nConflicts:\n")) {
+				ignore_footer = sb.len - previous;
+				break;
+			}
+			while (i < eol)
+				i++;
+			previous = eol;
 		}
-		strbuf_release(&sob);
+
+		append_signoff(&sb, ignore_footer);
 	}
 
 	if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)
diff --git a/sequencer.c b/sequencer.c
index f86f116..dbef5ce 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -17,6 +17,8 @@
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
+const char sign_off_header[] = "Signed-off-by: ";
+
 void remove_sequencer_state(void)
 {
 	struct strbuf seq_dir = STRBUF_INIT;
@@ -233,6 +235,9 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 		die(_("%s: Unable to write new index file"), action_name(opts));
 	rollback_lock_file(&index_lock);
 
+	if (opts->signoff)
+		append_signoff(msgbuf, 0);
+
 	if (!clean) {
 		int i;
 		strbuf_addstr(msgbuf, "\nConflicts:\n");
@@ -1011,3 +1016,63 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 	save_opts(opts);
 	return pick_commits(todo_list, opts);
 }
+
+static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
+{
+	int ch;
+	int hit = 0;
+	int i, j, k;
+	int len = sb->len - ignore_footer;
+	int first = 1;
+	const char *buf = sb->buf;
+
+	for (i = len - 1; i > 0; i--) {
+		if (hit && buf[i] == '\n')
+			break;
+		hit = (buf[i] == '\n');
+	}
+
+	while (i < len - 1 && buf[i] == '\n')
+		i++;
+
+	for (; i < len; i = k) {
+		for (k = i; k < len && buf[k] != '\n'; k++)
+			; /* do nothing */
+		k++;
+
+		if ((buf[k] == ' ' || buf[k] == '\t') && !first)
+			continue;
+
+		first = 0;
+
+		for (j = 0; i + j < len; j++) {
+			ch = buf[i + j];
+			if (ch == ':')
+				break;
+			if (isalnum(ch) ||
+			    (ch == '-'))
+				continue;
+			return 0;
+		}
+	}
+	return 1;
+}
+
+void append_signoff(struct strbuf *msgbuf, int ignore_footer)
+{
+	struct strbuf sob = STRBUF_INIT;
+	int i;
+
+	strbuf_addstr(&sob, sign_off_header);
+	strbuf_addstr(&sob, fmt_name(getenv("GIT_COMMITTER_NAME"),
+				getenv("GIT_COMMITTER_EMAIL")));
+	strbuf_addch(&sob, '\n');
+	for (i = msgbuf->len - 1 - ignore_footer; i > 0 && msgbuf->buf[i - 1] != '\n'; i--)
+		; /* do nothing */
+	if (prefixcmp(msgbuf->buf + i, sob.buf)) {
+		if (!i || !ends_rfc2822_footer(msgbuf, ignore_footer))
+			strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, "\n", 1);
+		strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, sob.buf, sob.len);
+	}
+	strbuf_release(&sob);
+}
diff --git a/sequencer.h b/sequencer.h
index d849420..60287b8 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -49,4 +49,8 @@ extern void remove_sequencer_state(void);
 
 int sequencer_pick_revisions(struct replay_opts *opts);
 
+extern const char sign_off_header[];
+
+void append_signoff(struct strbuf *msgbuf, int ignore_footer);
+
 #endif
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index 0c81b3c..c82f721 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -30,6 +30,7 @@ test_expect_success setup '
 	test_commit initial foo a &&
 	test_commit base foo b &&
 	test_commit picked foo c &&
+	test_commit --signoff picked-signed foo d &&
 	git config advice.detachedhead false
 
 '
@@ -340,4 +341,35 @@ test_expect_success 'revert conflict, diff3 -m style' '
 	test_cmp expected actual
 '
 
+test_expect_success 'failed cherry-pick does not forget -s' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick -s picked &&
+	test_i18ngrep -e "Signed-off-by" .git/MERGE_MSG
+'
+
+test_expect_success 'commit after failed cherry-pick does not add duplicated -s' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick -s picked-signed &&
+	git commit -a -s &&
+	test $(git show -s |grep -c "Signed-off-by") = 1
+'
+
+test_expect_success 'commit after failed cherry-pick adds -s at the right place' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick picked &&
+	git commit -a -s &&
+	pwd &&
+	cat <<EOF > expected &&
+picked
+
+Signed-off-by: C O Mitter <committer@example.com>
+
+Conflicts:
+	foo
+EOF
+
+	git show -s --pretty=format:%B > actual &&
+	test_cmp expected actual
+'
+
 test_done
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index f4e6450..b5fb527 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -410,7 +410,7 @@ test_expect_success '--continue respects -x in first commit in multi-pick' '
 	grep "cherry picked from.*$picked" msg
 '
 
-test_expect_success '--signoff is not automatically propagated to resolved conflict' '
+test_expect_failure '--signoff is automatically propagated to resolved conflict' '
 	pristine_detach initial &&
 	test_expect_code 1 git cherry-pick --signoff base..anotherpick &&
 	echo "c" >foo &&
@@ -428,7 +428,7 @@ test_expect_success '--signoff is not automatically propagated to resolved confl
 	grep "Signed-off-by:" anotherpick_msg
 '
 
-test_expect_success '--signoff dropped for implicit commit of resolution, multi-pick case' '
+test_expect_failure '--signoff dropped for implicit commit of resolution, multi-pick case' '
 	pristine_detach initial &&
 	test_must_fail git cherry-pick -s picked anotherpick &&
 	echo c >foo &&
@@ -441,7 +441,7 @@ test_expect_success '--signoff dropped for implicit commit of resolution, multi-
 	! grep Signed-off-by: msg
 '
 
-test_expect_success 'sign-off needs to be reaffirmed after conflict resolution, single-pick case' '
+test_expect_failure 'sign-off needs to be reaffirmed after conflict resolution, single-pick case' '
 	pristine_detach initial &&
 	test_must_fail git cherry-pick -s picked &&
 	echo c >foo &&
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 9bc57d2..0802da5 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -144,11 +144,21 @@ test_pause () {
 
 test_commit () {
 	notick= &&
-	if test "z$1" = "z--notick"
-	then
-		notick=yes
+	signoff= &&
+	while test $# != 0; do
+		case "$1" in
+		--notick)
+			notick=yes
+			;;
+		--signoff)
+			signoff="$1"
+			;;
+		*)
+			break
+			;;
+		esac
 		shift
-	fi &&
+	done &&
 	file=${2:-"$1.t"} &&
 	echo "${3-$1}" > "$file" &&
 	git add "$file" &&
@@ -156,7 +166,7 @@ test_commit () {
 	then
 		test_tick
 	fi &&
-	git commit -m "$1" &&
+	git commit $signoff -m "$1" &&
 	git tag "$1"
 }
 
-- 
1.7.7