Re: [PATCH 18/18] revert: Introduce --continue to continue the operation

[Jonathan Nieder <jrnieder@gmail.com>]

Ramkumar Ramachandra wrote:
>> Ramkumar Ramachandra wrote:

>>> during commit time.  One glitch to note is that the "--signoff" option
>>> specified at cherry-pick invocation time is not reflected in the
>>> commit message provided by CHERRY_PICK_HEAD
[...]
> The previous discussion thread for your reference [1] -- I thought we
> agreed that it was a glitch.

Ok.  I thought it had come up before and that there was not wide
agreement but probably I imagined it.  How about the patch below
(needs commit message, tests)?

Anyway, this is simple program logic that would be easy to change, not
a short-lived fault like the term "glitch" usually refers to.

> Neither of us can find any traces of it
> being intentional, and the commit that introduced this option doesn't
> say this: v1.5.6-rc0~83^2 (Allow cherry-pick (and revert) to add
> signoff line, 2008-04-26).

At the time, -x was not propagated to "git commit", either.  I guess
it was not considered notable that -s worked the same way, as in
"Of course 'git commit -c $original_commit' is going to discard
MERGE_MSG."

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/commit.c |   62 +---------------------------------------------
 builtin/revert.c |    5 ++-
 commit.c         |   71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 commit.h         |    3 ++
 4 files changed, 79 insertions(+), 62 deletions(-)

diff --git c/builtin/commit.c i/builtin/commit.c
index e1af9b19..55ae8294 100644
--- c/builtin/commit.c
+++ i/builtin/commit.c
@@ -524,8 +524,6 @@ static int is_a_merge(const unsigned char *sha1)
 	return !!(commit->parents && commit->parents->next);
 }
 
-static const char sign_off_header[] = "Signed-off-by: ";
-
 static void determine_author_info(struct strbuf *author_ident)
 {
 	char *name, *email, *date;
@@ -574,47 +572,6 @@ static void determine_author_info(struct strbuf *author_ident)
 					      IDENT_ERROR_ON_NO_NAME));
 }
 
-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, '>');
@@ -734,23 +691,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)
+		add_signoff(&sb);
 
 	if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)
 		die_errno(_("could not write commit template"));
diff --git c/builtin/revert.c i/builtin/revert.c
index 1f27c633..79ed9883 100644
--- c/builtin/revert.c
+++ i/builtin/revert.c
@@ -369,8 +369,6 @@ static int run_git_commit(const char *defmsg)
 
 	args[i++] = "commit";
 	args[i++] = "-n";
-	if (signoff)
-		args[i++] = "-s";
 	if (!edit) {
 		args[i++] = "-F";
 		args[i++] = defmsg;
@@ -485,6 +483,9 @@ static int do_pick_commit(void)
 			write_cherry_pick_head();
 	}
 
+	if (signoff)
+		add_signoff(&msgbuf);
+
 	if (!strategy || !strcmp(strategy, "recursive") || action == REVERT) {
 		res = do_recursive_merge(base, next, base_label, next_label,
 					 head, &msgbuf);
diff --git c/commit.c i/commit.c
index ac337c7d..5022db6e 100644
--- c/commit.c
+++ i/commit.c
@@ -10,6 +10,7 @@
 int save_commit_buffer = 1;
 
 const char *commit_type = "commit";
+const char sign_off_header[] = "Signed-off-by: ";
 
 static struct commit *check_commit(struct object *obj,
 				   const unsigned char *sha1,
@@ -348,6 +349,76 @@ int find_commit_subject(const char *commit_buffer, const char **subject)
 	return eol - p;
 }
 
+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 add_signoff(struct strbuf *sb)
+{
+	struct strbuf sob = STRBUF_INIT;
+	const char *p;
+
+	strbuf_addstr(&sob, sign_off_header);
+	strbuf_addstr(&sob, fmt_name(getenv("GIT_COMMITTER_NAME"),
+				     getenv("GIT_COMMITTER_EMAIL")));
+	strbuf_addch(&sob, '\n');
+
+	/* final line */
+	p = memrchr(sb->buf, '\n', sb->len ? sb->len - 1 : 0);
+	if (!p)
+		p = sb->buf;
+	else
+		p++;
+
+	/*
+	 * If the line does not already contain our sign-off,
+	 * add it.
+	 */
+	if (prefixcmp(p, sob.buf)) {
+		if (p == sb->buf || !ends_rfc2822_footer(sb))
+			strbuf_addch(sb, '\n');
+		strbuf_addbuf(sb, &sob);
+	}
+	strbuf_release(&sob);
+}
+
 struct commit_list *commit_list_insert(struct commit *item, struct commit_list **list_p)
 {
 	struct commit_list *new_list = xmalloc(sizeof(struct commit_list));
diff --git c/commit.h i/commit.h
index a2d571b9..3d198afb 100644
--- c/commit.h
+++ i/commit.h
@@ -23,6 +23,7 @@ struct commit {
 
 extern int save_commit_buffer;
 extern const char *commit_type;
+extern const char sign_off_header[];
 
 /* While we can decorate any object with a name, it's only used for commits.. */
 extern struct decoration name_decoration;
@@ -44,6 +45,8 @@ int parse_commit(struct commit *item);
 /* Find beginning and length of commit subject. */
 int find_commit_subject(const char *commit_buffer, const char **subject);
 
+void add_signoff(struct strbuf *sb);
+
 struct commit_list *commit_list_insert(struct commit *item,
 					struct commit_list **list);
 unsigned commit_list_count(const struct commit_list *l);
--