threads / patch / 32160

v2, 4 partsformat-patch: update append_signoff prototype

Subject: [PATCH v2 3/4] format-patch: update append_signoff prototype

## tl;dr

9 messages between Nov 22, 2012 and Dec 2, 2012. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

Nguyen Thai Ngoc Duy· Nov 22, 2012, 11:24 UTC · lore

Re: Topics currently in the Stalled category

On Wed, Nov 21, 2012 at 7:05 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 5 quoted lines
> * nd/unify-appending-of-s-o-b (2012-11-15) 1 commit
>  - Unify appending signoff in format-patch, commit and sequencer
>
>  I am not sure if the logic to refrain from adding a sign-off based
>  on the existing run of sign-offs is done correctly in this change.

I'm not sure what's the right logic here. The original code in format-patch has exactly one test just to verify that --signoff does _something_. There's another signoff code in git-am.sh and we should make sure at least the behavior is consistent. I guess I should start over by writing new tests to record the current behavior, then fixes and finally the unification. I think you can drop this for now.

-- 
Duy
Nguyễn Thái Ngọc Duy· Nov 22, 2012, 16:38 UTC · re: Nguyen Thai Ngoc Duy · lore

[PATCH v2 0/4] nd/unify-appending-of-s-o-b

This round cherry-pick and commit only skip S-o-b if the last one is the same while format-patch skips it if the same one appears anywhere in S-o-b list. I don't have strong opinion about which way is more correct. Though if we decide to change format-patch behavior, the series may become a bit different.

Nguyễn Thái Ngọc Duy (4):
  t4014: more tests about appending s-o-b lines
  format-patch: stricter S-o-b detection
  format-patch: update append_signoff prototype
  Unify appending signoff in format-patch, commit and sequencer
 builtin/commit.c        |  10 ++-
 builtin/log.c           |  13 +---
 log-tree.c              |  66 +++++++++++++---
 log-tree.h              |   4 +
 revision.h              |   2 +-
 sequencer.c             |  65 +---------------
 sequencer.h             |   4 -
 t/t4014-format-patch.sh | 199 ++++++++++++++++++++++++++++++++++++++++++++++++
 t/t7501-commit.sh       |   2 +-
 9 files changed, 272 insertions(+), 93 deletions(-)
-- 
1.8.0.4.g5d0415a
Nguyễn Thái Ngọc Duy· Nov 22, 2012, 16:38 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 1/4] t4014: more tests about appending s-o-b lines

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t4014-format-patch.sh | 145 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 145 insertions(+)
Show changes to t/t4014-format-patch.sh +145 −0
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 959aa26..c8d5d29 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -891,4 +891,149 @@ test_expect_success 'format patch ignores color.ui' '
 	test_cmp expect actual
 '
 
+append_signoff()
+{
+	C=`git commit-tree HEAD^^{tree} -p HEAD` &&
+	git format-patch --stdout --signoff ${C}^..${C} |
+		tee append_signoff.patch |
+		sed -n "1,/^---$/p" |
+		grep -n -E "^Subject|Sign|^$"
+}
+
+test_expect_success 'signoff: commit with no body' '
+	append_signoff </dev/null >actual &&
+	cat <<\EOF | sed "s/EOL$//" >expected &&
+4:Subject: [PATCH] EOL
+8:
+9:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'signoff: commit with only subject' '
+	echo subject | append_signoff >actual &&
+	cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+9:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'signoff: commit with only subject that does not end with NL' '
+	echo -n subject | append_signoff >actual &&
+	cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+9:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'signoff: no existing signoffs' '
+	append_signoff <<\EOF >actual &&
+subject
+
+body
+EOF
+	cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+10:
+11:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'signoff: no existing signoffs and no trailing NL' '
+	printf "subject\n\nbody" | append_signoff >actual &&
+	cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+10:
+11:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'signoff: some random signoff' '
+	append_signoff <<\EOF >actual &&
+subject
+
+body
+
+Signed-off-by: my@house
+EOF
+	cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+10:
+11:Signed-off-by: my@house
+12:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'signoff: some random signoff-alike' '
+	append_signoff <<\EOF >actual &&
+subject
+
+body
+Fooled-by-me: my@house
+EOF
+	cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+11:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'signoff: the same signoff at the end' '
+	append_signoff <<\EOF >actual &&
+subject
+
+body
+
+Signed-off-by: C O Mitter <committer@example.com>
+EOF
+	cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+10:
+11:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'signoff: the same signoff at the end, no trailing NL' '
+	printf "subject\n\nSigned-off-by: C O Mitter <committer@example.com>" |
+		append_signoff >actual &&
+	cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+9:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'signoff: the same signoff NOT at the end' '
+	append_signoff <<\EOF >actual &&
+subject
+
+body
+
+Signed-off-by: C O Mitter <committer@example.com>
+Signed-off-by: my@house
+EOF
+	cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+10:
+11:Signed-off-by: C O Mitter <committer@example.com>
+12:Signed-off-by: my@house
+EOF
+	test_cmp expected actual
+'
+
 test_done
-- 
1.8.0.4.g5d0415a
Torsten Bögershausen· Dec 2, 2012, 07:06 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH v2 1/4] t4014: more tests about appending s-o-b lines

On 22.11.12 17:38, Nguyễn Thái Ngọc Duy wrote:
Show 6 quoted lines
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  t/t4014-format-patch.sh | 145 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 145 insertions(+)
> +	echo -n subject | append_signoff >actual &&

"echo -n" is not portable, and we use printf everywhere. I found one "echo -n" in line 996.

Can we squeeze that in, before going to next?
Show changes to t/t4014-format-patch.sh +1 −1
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 6cfad13..f460930 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -993,7 +993,7 @@ EOF
 '
 
 test_expect_success 'signoff: commit with only subject that does not end with NL' '
-       echo -n subject | append_signoff >actual &&
+       printf subject | append_signoff >actual &&
        cat >expected <<\EOF &&
 4:Subject: [PATCH] subject
 8:
Brandon Casey· Dec 2, 2012, 08:03 UTC · re: Torsten Bögershausen · lore

Re: [PATCH v2 1/4] t4014: more tests about appending s-o-b lines

On Sat, Dec 1, 2012 at 11:06 PM, Torsten Bögershausen <tboegi@web.de> wrote:
Show 13 quoted lines
> On 22.11.12 17:38, Nguyễn Thái Ngọc Duy wrote:
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>>  t/t4014-format-patch.sh | 145 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 145 insertions(+)
>> +     echo -n subject | append_signoff >actual &&
>
>
> "echo -n" is not portable, and we use printf everywhere.
> I found one "echo -n" in line  996.
>
> Can we squeeze that in, before going to next?
I got it.  I'll squash it into the next series.
-Brandon
Nguyễn Thái Ngọc Duy· Nov 22, 2012, 16:38 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 2/4] format-patch: stricter S-o-b detection

S-o-b in the middle of a sentence, at the beginning of the sentence but it is just because of text wrapping, or a full paragraph of valid S-o-b in the middle of the message. All these are not S-o-b, but detected as is. Fix them.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 log-tree.c              | 37 +++++++++++++++++++++++++++++++--
 t/t4014-format-patch.sh | 54 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 89 insertions(+), 2 deletions(-)
Show changes to 2 files +89 −2

log-tree.c, t/t4014-format-patch.sh

diff --git a/log-tree.c b/log-tree.c
index c894930..aff7c0a 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -260,14 +260,47 @@ static void append_signoff(struct strbuf *sb, const char *signoff)
 	int has_signoff = 0;
 	char *cp;
 
-	cp = sb->buf;
+	/*
+	 * Only search in the last paragrah, don't be fooled by a
+	 * paragraph full of valid S-o-b in the middle.
+	 */
+	cp = sb->buf + sb->len - 1;
+	while (cp > sb->buf) {
+		if (cp[0] == '\n' && cp[1] == '\n') {
+			cp += 2;
+			break;
+		}
+		cp--;
+	}
 
 	/* First see if we already have the sign-off by the signer */
 	while ((cp = strstr(cp, signed_off_by))) {
+		const char *s = cp;
+		cp += strlen(signed_off_by);
+
+		if (!has_signoff && s > sb->buf) {
+			/*
+			 * S-o-b in the middle of a sentence is not
+			 * really S-o-b
+			 */
+			if (s[-1] != '\n')
+				continue;
+
+			if (s - 1 > sb->buf && s[-2] == '\n')
+				; /* the first S-o-b */
+			else if (!detect_any_signoff(sb->buf, s - sb->buf))
+				/*
+				 * The previous line looks like an
+				 * S-o-b. There's still no guarantee
+				 * that it's an actual S-o-b. For that
+				 * we need to look back until we find
+				 * a blank line, which is too costly.
+				 */
+				continue;
+		}
 
 		has_signoff = 1;
 
-		cp += strlen(signed_off_by);
 		if (cp + signoff_len >= sb->buf + sb->len)
 			break;
 		if (strncmp(cp, signoff, signoff_len))
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index c8d5d29..d0b3c85 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -989,6 +989,60 @@ EOF
 	test_cmp expected actual
 '
 
+test_expect_success 'signoff: not really a signoff' '
+	append_signoff <<\EOF >actual &&
+subject
+
+I want to mention about Signed-off-by: here.
+EOF
+	cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+9:I want to mention about Signed-off-by: here.
+10:
+11:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'signoff: not really a signoff (2)' '
+	append_signoff <<\EOF >actual &&
+subject
+
+My unfortunate
+Signed-off-by: example happens to be wrapped here.
+EOF
+	cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+10:Signed-off-by: example happens to be wrapped here.
+11:
+12:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'signoff: valid S-o-b paragraph in the middle' '
+	append_signoff <<\EOF >actual &&
+subject
+
+Signed-off-by: my@house
+Signed-off-by: your@house
+
+A lot of houses.
+EOF
+	cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+9:Signed-off-by: my@house
+10:Signed-off-by: your@house
+11:
+13:
+14:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+	test_cmp expected actual
+'
+
 test_expect_success 'signoff: the same signoff at the end' '
 	append_signoff <<\EOF >actual &&
 subject
-- 
1.8.0.4.g5d0415a
Nguyễn Thái Ngọc Duy· Nov 22, 2012, 16:38 UTC · re: Nguyễn Thái Ngọc Duy · lore

This is a preparation step for merging with append_signoff from sequencer.c

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/log.c | 13 +------------
 log-tree.c    | 21 +++++++++++++--------
 revision.h    |  2 +-
 3 files changed, 15 insertions(+), 21 deletions(-)
Show changes to 3 files +15 −21

builtin/log.c, log-tree.c, revision.h

diff --git a/builtin/log.c b/builtin/log.c
index 09cf43e..f201070 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1053,7 +1053,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;
@@ -1148,16 +1147,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');
@@ -1348,7 +1337,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 aff7c0a..7e50545 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -253,9 +253,11 @@ static int detect_any_signoff(char *letter, int size)
 	return seen_head && seen_name;
 }
 
-static void append_signoff(struct strbuf *sb, const char *signoff)
+static void append_signoff(struct strbuf *sb, int flags)
 {
-	static const char signed_off_by[] = "Signed-off-by: ";
+	char* signoff = xstrdup(fmt_name(getenv("GIT_COMMITTER_NAME"),
+					 getenv("GIT_COMMITTER_EMAIL")));
+	static const char sign_off_header[] = "Signed-off-by: ";
 	size_t signoff_len = strlen(signoff);
 	int has_signoff = 0;
 	char *cp;
@@ -274,9 +276,9 @@ static void append_signoff(struct strbuf *sb, const char *signoff)
 	}
 
 	/* First see if we already have the sign-off by the signer */
-	while ((cp = strstr(cp, signed_off_by))) {
+	while ((cp = strstr(cp, sign_off_header))) {
 		const char *s = cp;
-		cp += strlen(signed_off_by);
+		cp += strlen(sign_off_header);
 
 		if (!has_signoff && s > sb->buf) {
 			/*
@@ -317,9 +319,10 @@ static void append_signoff(struct strbuf *sb, const char *signoff)
 	if (!has_signoff)
 		strbuf_addch(sb, '\n');
 
-	strbuf_addstr(sb, signed_off_by);
+	strbuf_addstr(sb, sign_off_header);
 	strbuf_add(sb, signoff, signoff_len);
 	strbuf_addch(sb, '\n');
+	free(signoff);
 }
 
 static unsigned int digits_in_number(unsigned int number)
@@ -684,8 +687,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;
@@ -696,7 +701,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;
-- 
1.8.0.4.g5d0415a
Nguyễn Thái Ngọc Duy· Nov 22, 2012, 16:38 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/commit.c  | 10 ++++++++-
 log-tree.c        | 14 ++++++++----
 log-tree.h        |  4 ++++
 sequencer.c       | 65 ++-----------------------------------------------------
 sequencer.h       |  4 ----
 t/t7501-commit.sh |  2 +-
 6 files changed, 26 insertions(+), 73 deletions(-)
Show changes to 6 files +26 −73

builtin/commit.c, log-tree.c, log-tree.h, sequencer.c, sequencer.h, t/t7501-commit.sh

diff --git a/builtin/commit.c b/builtin/commit.c
index a17a5df..6d323d9 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -698,7 +698,15 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 			previous = eol;
 		}
 
-		append_signoff(&sb, ignore_footer);
+		if (ignore_footer) {
+			struct strbuf footer = STRBUF_INIT;
+			strbuf_addstr(&footer, sb.buf + sb.len - ignore_footer);
+			strbuf_setlen(&sb, sb.len - ignore_footer);
+			append_signoff(&sb, SOB_IGNORE_SAME);
+			strbuf_addstr(&sb, footer.buf);
+			strbuf_release(&footer);
+		} else
+			append_signoff(&sb, SOB_IGNORE_SAME);
 	}
 
 	if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)
diff --git a/log-tree.c b/log-tree.c
index 7e50545..e8d31d9 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -12,6 +12,8 @@
 
 struct decoration name_decoration = { "object names" };
 
+const char sign_off_header[] = "Signed-off-by: ";
+
 enum decoration_type {
 	DECORATION_NONE = 0,
 	DECORATION_REF_LOCAL,
@@ -207,7 +209,7 @@ void show_decorations(struct rev_info *opt, struct commit *commit)
 }
 
 /*
- * Search for "^[-A-Za-z]+: [^@]+@" pattern. It usually matches
+ * Search for "^[-A-Za-z0-9]+: [^@]+@" pattern. It usually matches
  * Signed-off-by: and Acked-by: lines.
  */
 static int detect_any_signoff(char *letter, int size)
@@ -243,6 +245,7 @@ static int detect_any_signoff(char *letter, int size)
 		}
 		if (('A' <= ch && ch <= 'Z') ||
 		    ('a' <= ch && ch <= 'z') ||
+		    ('0' <= ch && ch <= '9') ||
 		    ch == '-') {
 			seen_head = 1;
 			continue;
@@ -253,11 +256,10 @@ static int detect_any_signoff(char *letter, int size)
 	return seen_head && seen_name;
 }
 
-static void append_signoff(struct strbuf *sb, int flags)
+void append_signoff(struct strbuf *sb, int flags)
 {
 	char* signoff = xstrdup(fmt_name(getenv("GIT_COMMITTER_NAME"),
 					 getenv("GIT_COMMITTER_EMAIL")));
-	static const char sign_off_header[] = "Signed-off-by: ";
 	size_t signoff_len = strlen(signoff);
 	int has_signoff = 0;
 	char *cp;
@@ -310,7 +312,11 @@ static void append_signoff(struct strbuf *sb, int flags)
 		if (!isspace(cp[signoff_len]))
 			continue;
 		/* we already have him */
-		return;
+		if (flags & SOB_IGNORE_SAME) {
+			if (cp[signoff_len + 1] == '\0')
+				return;
+		} else
+			return;
 	}
 
 	if (!has_signoff)
diff --git a/log-tree.h b/log-tree.h
index f5ac238..739f729 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -24,4 +24,8 @@ void load_ref_decorations(int flags);
 void get_patch_filename(struct commit *commit, const char *subject, int nr,
 			const char *suffix, struct strbuf *buf);
 
+#define SOB_IGNORE_SAME   1
+extern const char sign_off_header[];
+void append_signoff(struct strbuf *msgbuf, int flags);
+
 #endif
diff --git a/sequencer.c b/sequencer.c
index e3723d2..bc02a66 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -14,11 +14,10 @@
 #include "merge-recursive.h"
 #include "refs.h"
 #include "argv-array.h"
+#include "log-tree.h"
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
-const char sign_off_header[] = "Signed-off-by: ";
-
 static void remove_sequencer_state(void)
 {
 	struct strbuf seq_dir = STRBUF_INIT;
@@ -236,7 +235,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 	rollback_lock_file(&index_lock);
 
 	if (opts->signoff)
-		append_signoff(msgbuf, 0);
+		append_signoff(msgbuf, SOB_IGNORE_SAME);
 
 	if (!clean) {
 		int i;
@@ -1016,63 +1015,3 @@ 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 9d57d57..99eb7fa 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -46,8 +46,4 @@ struct replay_opts {
 
 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/t7501-commit.sh b/t/t7501-commit.sh
index 195e747..6dd4580 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -353,7 +353,7 @@ test_expect_success 'signoff gap' '
 
 	echo 3 >positive &&
 	git add positive &&
-	alt="Alt-RFC-822-Header: Value" &&
+	alt="Alt-RFC-822-Header: Va@lue" &&
 	git commit -s -m "welcome
 
 $alt" &&
-- 
1.8.0.4.g5d0415a
Junio C Hamano· Nov 24, 2012, 02:05 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH v2 0/4] nd/unify-appending-of-s-o-b

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
> This round cherry-pick and commit only skip S-o-b if the last one is
> the same...

That sounds sensible. When the user asks us to add sign-off, we should honor it, unless it is known that adding one is absolutely unnecessary, which is "it already exists as the last one". Ignoring all other cases will lose information.

Among the non-merge commits in the kernel history, you see about 500 such commits where the same contributor signs off the patch more than once, recording the flow of the patch; 106a4ee258 seems to be the most recent example.

Thanks.

← back to recent threads