[PATCH] Unify appending signoff in format-patch, commit and sequencer

[PATCH] Unify appending signoff in format-patch, commit and sequencer

[Nguyễn Thái Ngọc Duy]

There are two implementations of append_signoff in log-tree.c and
sequencer.c, which do more or less the same thing. This patch

 - teaches sequencer.c's append_signoff() not to append the signoff if
   it's already there but not at the bottom

 - removes log-tree.c's

 - make sure "Signed-off-by: foo" in the middle of a line is not
   counted as a sign off

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Interestingly this patch triggers the fault that it fixes.
 I was surprised that there was no blank line before my S-o-b
 and thought I broke something. It turns out I used unmodified
 format-patch and it mistook the S-o-b quote as true S-o-b line.
 The modified one puts the blank line back.

 builtin/log.c | 13 +--------
 log-tree.c    | 92 ++++-------------------------------------------------------
 revision.h    |  2 +-
 sequencer.c   | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++----
 4 files changed, 88 insertions(+), 105 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index e7b7db1..bb48344 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1058,7 +1058,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	struct commit *origin = NULL, *head = NULL;
 	const char *in_reply_to = NULL;
 	struct patch_ids ids;
-	char *add_signoff = NULL;
 	struct strbuf buf = STRBUF_INIT;
 	int use_patch_format = 0;
 	int quiet = 0;
@@ -1154,16 +1153,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			     PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN |
 			     PARSE_OPT_KEEP_DASHDASH);
 
-	if (do_signoff) {
-		const char *committer;
-		const char *endpos;
-		committer = git_committer_info(IDENT_STRICT);
-		endpos = strchr(committer, '>');
-		if (!endpos)
-			die(_("bogus committer info %s"), committer);
-		add_signoff = xmemdupz(committer, endpos - committer + 1);
-	}
-
 	for (i = 0; i < extra_hdr.nr; i++) {
 		strbuf_addstr(&buf, extra_hdr.items[i].string);
 		strbuf_addch(&buf, '\n');
@@ -1354,7 +1343,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		total++;
 		start_number--;
 	}
-	rev.add_signoff = add_signoff;
+	rev.add_signoff = do_signoff;
 	while (0 <= --nr) {
 		int shown;
 		commit = list[nr];
diff --git a/log-tree.c b/log-tree.c
index c894930..18cf006 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -9,6 +9,7 @@
 #include "string-list.h"
 #include "color.h"
 #include "gpg-interface.h"
+#include "sequencer.h"
 
 struct decoration name_decoration = { "object names" };
 
@@ -206,89 +207,6 @@ void show_decorations(struct rev_info *opt, struct commit *commit)
 	putchar(')');
 }
 
-/*
- * Search for "^[-A-Za-z]+: [^@]+@" pattern. It usually matches
- * Signed-off-by: and Acked-by: lines.
- */
-static int detect_any_signoff(char *letter, int size)
-{
-	char *cp;
-	int seen_colon = 0;
-	int seen_at = 0;
-	int seen_name = 0;
-	int seen_head = 0;
-
-	cp = letter + size;
-	while (letter <= --cp && *cp == '\n')
-		continue;
-
-	while (letter <= cp) {
-		char ch = *cp--;
-		if (ch == '\n')
-			break;
-
-		if (!seen_at) {
-			if (ch == '@')
-				seen_at = 1;
-			continue;
-		}
-		if (!seen_colon) {
-			if (ch == '@')
-				return 0;
-			else if (ch == ':')
-				seen_colon = 1;
-			else
-				seen_name = 1;
-			continue;
-		}
-		if (('A' <= ch && ch <= 'Z') ||
-		    ('a' <= ch && ch <= 'z') ||
-		    ch == '-') {
-			seen_head = 1;
-			continue;
-		}
-		/* no empty last line doesn't match */
-		return 0;
-	}
-	return seen_head && seen_name;
-}
-
-static void append_signoff(struct strbuf *sb, const char *signoff)
-{
-	static const char signed_off_by[] = "Signed-off-by: ";
-	size_t signoff_len = strlen(signoff);
-	int has_signoff = 0;
-	char *cp;
-
-	cp = sb->buf;
-
-	/* First see if we already have the sign-off by the signer */
-	while ((cp = strstr(cp, signed_off_by))) {
-
-		has_signoff = 1;
-
-		cp += strlen(signed_off_by);
-		if (cp + signoff_len >= sb->buf + sb->len)
-			break;
-		if (strncmp(cp, signoff, signoff_len))
-			continue;
-		if (!isspace(cp[signoff_len]))
-			continue;
-		/* we already have him */
-		return;
-	}
-
-	if (!has_signoff)
-		has_signoff = detect_any_signoff(sb->buf, sb->len);
-
-	if (!has_signoff)
-		strbuf_addch(sb, '\n');
-
-	strbuf_addstr(sb, signed_off_by);
-	strbuf_add(sb, signoff, signoff_len);
-	strbuf_addch(sb, '\n');
-}
-
 static unsigned int digits_in_number(unsigned int number)
 {
 	unsigned int i = 10, result = 1;
@@ -651,8 +569,10 @@ void show_log(struct rev_info *opt)
 	/*
 	 * And then the pretty-printed message itself
 	 */
-	if (ctx.need_8bit_cte >= 0)
-		ctx.need_8bit_cte = has_non_ascii(opt->add_signoff);
+	if (ctx.need_8bit_cte >= 0 && opt->add_signoff)
+		ctx.need_8bit_cte =
+			has_non_ascii(fmt_name(getenv("GIT_COMMITTER_NAME"),
+					       getenv("GIT_COMMITTER_EMAIL")));
 	ctx.date_mode = opt->date_mode;
 	ctx.date_mode_explicit = opt->date_mode_explicit;
 	ctx.abbrev = opt->diffopt.abbrev;
@@ -663,7 +583,7 @@ void show_log(struct rev_info *opt)
 	pretty_print_commit(&ctx, commit, &msgbuf);
 
 	if (opt->add_signoff)
-		append_signoff(&msgbuf, opt->add_signoff);
+		append_signoff(&msgbuf, 0);
 	if (opt->show_log_size) {
 		printf("log size %i\n", (int)msgbuf.len);
 		graph_show_oneline(opt->graph);
diff --git a/revision.h b/revision.h
index a95bd0b..af35325 100644
--- a/revision.h
+++ b/revision.h
@@ -136,7 +136,7 @@ struct rev_info {
 	int		numbered_files;
 	char		*message_id;
 	struct string_list *ref_message_ids;
-	const char	*add_signoff;
+	int              add_signoff;
 	const char	*extra_headers;
 	const char	*log_reencode;
 	const char	*subject_prefix;
diff --git a/sequencer.c b/sequencer.c
index be0cb8b..4eb59e4 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1058,21 +1058,95 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
 	return 1;
 }
 
+/*
+ * Search for "^[-A-Za-z]+: [^@]+@" pattern. It usually matches
+ * Signed-off-by: and Acked-by: lines.
+ */
+static int detect_any_signoff(char *letter, int size)
+{
+	char *cp;
+	int seen_colon = 0;
+	int seen_at = 0;
+	int seen_name = 0;
+	int seen_head = 0;
+
+	cp = letter + size;
+	while (letter <= --cp && *cp == '\n')
+		continue;
+
+	while (letter <= cp) {
+		char ch = *cp--;
+		if (ch == '\n')
+			break;
+
+		if (!seen_at) {
+			if (ch == '@')
+				seen_at = 1;
+			continue;
+		}
+		if (!seen_colon) {
+			if (ch == '@')
+				return 0;
+			else if (ch == ':')
+				seen_colon = 1;
+			else
+				seen_name = 1;
+			continue;
+		}
+		if (('A' <= ch && ch <= 'Z') ||
+		    ('a' <= ch && ch <= 'z') ||
+		    ch == '-') {
+			seen_head = 1;
+			continue;
+		}
+		/* no empty last line doesn't match */
+		return 0;
+	}
+	return seen_head && seen_name;
+}
+
 void append_signoff(struct strbuf *msgbuf, int ignore_footer)
 {
 	struct strbuf sob = STRBUF_INIT;
-	int i;
+	const char *cp;
+	int i, has_signoff = 0;
+	int signoff_header_len = strlen(sign_off_header);
 
 	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);
+
+	/* First see if we already have the sign-off by the signer */
+	cp = msgbuf->buf;
+	while ((cp = strstr(cp, sign_off_header)) &&
+	       cp + signoff_header_len < msgbuf->buf + msgbuf->len - ignore_footer) {
+
+		if (cp > msgbuf->buf && cp[-1] != '\n' && cp[-1] != '\r') {
+			/*
+			 * Signed-off-by: found in the middle of the
+			 * commit message is not really a sign off
+			 */
+			cp += signoff_header_len;
+			continue;
+		}
+		has_signoff = 1;
+
+	       if (cp + sob.len >= msgbuf->buf + msgbuf->len)
+		       break;
+	       if (!strncmp(cp, sob.buf, sob.len) && isspace(cp[sob.len]))
+		       return;	/* we already have him */
+	       cp += signoff_header_len;
 	}
+
+	if (!has_signoff)
+		has_signoff = detect_any_signoff(msgbuf->buf,
+						 msgbuf->len - ignore_footer);
+
+	if (!i || (!has_signoff && !ends_rfc2822_footer(msgbuf, ignore_footer)))
+		strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, "\n", 1);
+	strbuf_addch(&sob, '\n');
+	strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, sob.buf, sob.len);
 	strbuf_release(&sob);
 }
-- 
1.8.0.151.g12dbe03

Re: [PATCH] Unify appending signoff in format-patch, commit and sequencer

[Brandon Casey]

On Thu, Nov 15, 2012 at 4:32 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> There are two implementations of append_signoff in log-tree.c and
> sequencer.c, which do more or less the same thing. This patch
>
>  - teaches sequencer.c's append_signoff() not to append the signoff if
>    it's already there but not at the bottom
>
>  - removes log-tree.c's
>
>  - make sure "Signed-off-by: foo" in the middle of a line is not
>    counted as a sign off
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  Interestingly this patch triggers the fault that it fixes.
>  I was surprised that there was no blank line before my S-o-b
>  and thought I broke something. It turns out I used unmodified
>  format-patch and it mistook the S-o-b quote as true S-o-b line.
>  The modified one puts the blank line back.

Heh, yeah I noticed this bug yesterday when I submitted my changes
affecting the same area of code (sequence.c).  Glad I didn't waste too
much time fixing it.

Have you looked at this:

   http://thread.gmane.org/gmane.comp.version-control.git/209781

That series doesn't duplicate your work, but the two series's should
be resolved.

-Brandon

Re: [PATCH] Unify appending signoff in format-patch, commit and sequencer

[Nguyen Thai Ngoc Duy]

On Fri, Nov 16, 2012 at 3:42 AM, Brandon Casey <drafnel@gmail.com> wrote:
>>  Interestingly this patch triggers the fault that it fixes.
>>  I was surprised that there was no blank line before my S-o-b
>>  and thought I broke something. It turns out I used unmodified
>>  format-patch and it mistook the S-o-b quote as true S-o-b line.
>>  The modified one puts the blank line back.
>
> Heh, yeah I noticed this bug yesterday when I submitted my changes
> affecting the same area of code (sequence.c).  Glad I didn't waste too
> much time fixing it.
>
> Have you looked at this:
>
>    http://thread.gmane.org/gmane.comp.version-control.git/209781
>
> That series doesn't duplicate your work, but the two series's should
> be resolved.

Thanks I'm watching that discussion now and probably will rebase on
top of yours once it's finalized. I actually wrote this patch a while
ago, just waiting for nd/builtin-to-libgit to graduate because of some
linking issues this patch causes. I can wait a bit longer until yours
graduates.
-- 
Duy