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

Subsystems: the rest

STALE3715d

3 messages, 2 authors, 2016-06-15 · open the first message on its own page

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

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2016-06-15 22:55:17

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 <redacted>
---
 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

From: Brandon Casey <hidden>
Date: 2016-06-15 22:55:18

On Thu, Nov 15, 2012 at 4:32 AM, Nguyễn Thái Ngọc Duy [off-list ref] 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 <redacted>
---
 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

From: Nguyen Thai Ngoc Duy <hidden>
Date: 2016-06-15 22:55:18

On Fri, Nov 16, 2012 at 3:42 AM, Brandon Casey [off-list ref] wrote:
quoted
 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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help