[PATCH] builtin-commit.c: Not add duplicate S-o-b when commit

[PATCH] builtin-commit.c: Not add duplicate S-o-b when commit

[Jiang Xin]

Scan the whole rfc2822 footer for duplicate S-o-b, not just the last
line of the commit message.

A commit may have multiple S-o-bs, or other tags, such as:

    some commit log...

    Signed-off-by: C O Mitter <committer@example.com>
    Reported-by: R E Porter <reporter@example.com>

Because the S-o-b is not located at the last line in the above commit,
when the above commit is amended by the original committer, a
duplicated S-o-b may appended by accident. New commit log may looks
like:

    some commit log...

    Signed-off-by: C O Mitter <committer@example.com>
    Reported-by: R E Porter <reporter@example.com>
    Signed-off-by: C O Mitter <committer@example.com>

This hack scans the whole rfc2822 footer for duplicate S-o-b, and only
append a S-o-b when necessary. Also add testcases in 't/t7502-commit.sh'
for this.

Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
---
 builtin/commit.c  | 28 ++++++++++++++++++++++++----
 t/t7502-commit.sh | 19 +++++++++++++++++++
 2 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 20cef..1a3da 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -704,15 +704,35 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	if (signoff) {
 		struct strbuf sob = STRBUF_INIT;
 		int i;
+		int hit_footer = 0;
+		int hit_sob = 0;
 
 		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))
+		for (i = sb.len - 1; i > 0; i--) {
+			if (hit_footer && sb.buf[i] == '\n') {
+				hit_footer = 2;
+				i += 2;
+				break;
+			}
+			hit_footer = (sb.buf[i] == '\n');
+		}
+		hit_footer = (2 == hit_footer);
+		if (hit_footer) {
+			while (i < sb.len)
+			{
+				if (!prefixcmp(sb.buf + i, sob.buf)) {
+					hit_sob = 1;
+					break;
+				}
+				while (i < sb.len && sb.buf[i++] != '\n')
+					; /* do nothing */
+			}
+		}
+		if (!hit_sob) {
+			if (!hit_footer || !ends_rfc2822_footer(&sb))
 				strbuf_addch(&sb, '\n');
 			strbuf_addbuf(&sb, &sob);
 		}
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 18145..8198f 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -336,7 +336,26 @@ test_expect_success 'A single-liner subject with a token plus colon is not a foo
 	git commit -s -m "hello: kitty" --allow-empty &&
 	git cat-file commit HEAD | sed -e "1,/^$/d" >actual &&
 	test_line_count = 3 actual
+'
+
+cat > expect << EOF
+Footer-like: commit log
+
+Signed-off-by: C O Mitter <committer@example.com>
+EOF
+
+test_expect_success 'S-o-b after footer-like commit message' '
+	head -1 expect | git commit -s --allow-empty -F - &&
+	git cat-file commit HEAD | sed "1,/^\$/d" > output &&
+	test_cmp expect output
+'
+
+echo "Reported-by: R E Porter <reporter@example.com>" >> expect
 
+test_expect_success 'no duplicate S-o-b when signoff' '
+	cat expect | git commit -s --allow-empty -F - &&
+	git cat-file commit HEAD | sed "1,/^\$/d" > output &&
+	test_cmp expect output
 '
 
 cat >.git/FAKE_EDITOR <<EOF
-- 
1.7.12.rc0.28.g8ecd8a5.dirty

Re: [PATCH] builtin-commit.c: Not add duplicate S-o-b when commit

[Junio C Hamano]

Jiang Xin <worldhello.net@gmail.com> writes:

> Scan the whole rfc2822 footer for duplicate S-o-b, not just the last
> line of the commit message.
>
> A commit may have multiple S-o-bs, or other tags, such as:
>
>     some commit log...
>
>     Signed-off-by: C O Mitter <committer@example.com>
>     Reported-by: R E Porter <reporter@example.com>
>
> Because the S-o-b is not located at the last line in the above commit,
> when the above commit is amended by the original committer, a
> duplicated S-o-b may appended by accident. New commit log may looks
> like:
>
>     some commit log...
>
>     Signed-off-by: C O Mitter <committer@example.com>
>     Reported-by: R E Porter <reporter@example.com>
>     Signed-off-by: C O Mitter <committer@example.com>
>

After stating the observation like the above, please make it a habit
to say "which is bad because...", if you think it is a bad behaviour
and the patch is about fixing it.

Because a chain of S-o-b is used to record the flow of a patch, it
is entirely normal if developer A writes the patch (she signs it
off), reviewer B picks it up and sends it back with a minor fix-up
to the list, and developer A again picks it up from the list and
forwards it to the uplevel maintainer, in which case you may see
S-o-b by A, then B (it may be S-o-b or something else,
e.g. Reviewed-by) and then S-o-b by A again.

The above observation is correct (a commit log may look like so),
but your untold conclusion (it is a bad thing because there are
S-o-b from the same person twice) is not necessarily correct.

Re: [PATCH] builtin-commit.c: Not add duplicate S-o-b when commit

[Jiang Xin]

2012/7/26 Junio C Hamano <gitster@pobox.com>:
> After stating the observation like the above, please make it a habit
> to say "which is bad because...", if you think it is a bad behaviour
> and the patch is about fixing it.

Indead before I start, I examine git-commit and git-am, and find
the behaviours of the two commands are different.

"git commit -s" checks the last line of the footer, while "git -am"
checks the last S-o-b. E.g. original commit X:

    commit log...

    Signed-off-by: A
    Signed-off-by: B
    Reported-by: C

When user B amend the commit, the amended commit Y looks like:

    commit log...

    Signed-off-by: A
    Signed-off-by: B
    Reported-by: C
    Signed-off-by: B

While if the original commit X send to user B by patch, and
user B run command "git am -s", the commit would be:

    Signed-off-by: A
    Signed-off-by: B
    Reported-by: C

So I guess duplicate S-o-b is not intentional.

I use an alias for commit:

    git config --global alias.ci "commit -s"

And will encounter duplicate S-o-b issues frequently, especially
format-patch/send-email workflow.

-- 
Jiang Xin

Re: [PATCH] builtin-commit.c: Not add duplicate S-o-b when commit

[Junio C Hamano]

Jiang Xin <worldhello.net@gmail.com> writes:

> 2012/7/26 Junio C Hamano <gitster@pobox.com>:
>> After stating the observation like the above, please make it a habit
>> to say "which is bad because...", if you think it is a bad behaviour
>> and the patch is about fixing it.
>
> Indead before I start, I examine git-commit and git-am, and find
> the behaviours of the two commands are different.
>
> "git commit -s" checks the last line of the footer, while "git -am"
> checks the last S-o-b.

I think "git am -s" (which I think you meant) is wrong, then.

> E.g. original commit X:
>
>     commit log...
>
>     Signed-off-by: A
>     Signed-off-by: B
>     Reported-by: C

The order in this "original" is already wrong, isn't it, though?
Didn't the change result by first C reporting an issue, fixes done
by A and forwarded by B?


> When user B amend the commit, the amended commit Y looks like:
>
>     commit log...
>
>     Signed-off-by: A
>     Signed-off-by: B
>     Reported-by: C
>     Signed-off-by: B
>
> While if the original commit X send to user B by patch, and
> user B run command "git am -s", the commit would be:
>
>     Signed-off-by: A
>     Signed-off-by: B
>     Reported-by: C
>
> So I guess duplicate S-o-b is not intentional.

I think the two commands are doing randomly different things on
garbage input.  The order in the input (i.e. your "original") does
not make sense.  C is not the person who handled the patch the last.

If you start from

	Reported-by: X
	S-o-b: A
	Reviewed-by: Y
        S-o-b: B

i.e. the last person who touched this patch is B, tweaking the patch
and amending a commit will add S-o-b for the person who amends, iff
that person is not B, which is what you usually want.

> I use an alias for commit:
>
>     git config --global alias.ci "commit -s"
>
> And will encounter duplicate S-o-b issues frequently, especially
> format-patch/send-email workflow.

Re: [PATCH] builtin-commit.c: Not add duplicate S-o-b when commit

[Jiang Xin]

2012/7/27 Junio C Hamano <gitster@pobox.com>:
>>     Signed-off-by: A
>>     Signed-off-by: B
>>     Reported-by: C
>>
>> So I guess duplicate S-o-b is not intentional.
>
> I think the two commands are doing randomly different things on
> garbage input.  The order in the input (i.e. your "original") does
> not make sense.  C is not the person who handled the patch the last.

I am clear, thanks. Please reword the commit 3c58c1a in branch
"jx/i18n-1.7.11". Change the tags from:

    Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
    Reported-by: Vincent van Ravesteijn <vfr@lyx.org>
    Reviewed-by: Stefano Lattarini <stefano.lattarini@gmail.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

to

    Reported-by: Vincent van Ravesteijn <vfr@lyx.org>
    Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
    Reviewed-by: Stefano Lattarini <stefano.lattarini@gmail.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>


-- 
Jiang Xin