[PATCH 0/3] rebase --signoff

[PATCH 0/3] rebase --signoff

[Giuseppe Bilotta]

Allow signing off a whole patchset by rebasing it with the --signoff
option, which is simply passed through to git am.

Compared to previous incarnations, I've split the am massaging to
separate commits (for cleanliness and easier reverts if needed),
and introduced a test case for both --signoff and its negation.

Giuseppe Bilotta (3):
  builtin/am: obey --signoff also when --rebasing
  builtin/am: fold am_signoff() into am_append_signoff()
  rebase: pass --[no-]signoff option to git am

 Documentation/git-rebase.txt |  5 +++++
 builtin/am.c                 | 39 +++++++++++++++++--------------------
 git-rebase.sh                |  3 ++-
 t/t3428-rebase-signoff.sh    | 46 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 71 insertions(+), 22 deletions(-)
 create mode 100755 t/t3428-rebase-signoff.sh

-- 
2.12.2.765.g2bf946761b

[PATCH 1/3] builtin/am: obey --signoff also when --rebasing

[Giuseppe Bilotta]

Signoff is handled in parse_mail(), but not in parse_mail_rebasing(),
since the latter is only used when git-rebase calls git-am with the
--rebasing option, and --signoff is never passed in this case.

In order to introduce (in the upcoming commits) support for `git-rebase
--signoff`, we must make gi-am obey it also in the rebase case. This is
trivially fixed by moving the conditional addition of the signoff from
parse_mail() to the caller am_run(), after either of the parse_mail*()
functions were called.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 builtin/am.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index f7a7a971fb..d072027b5a 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1321,9 +1321,6 @@ static int parse_mail(struct am_state *state, const char *mail)
 	strbuf_addbuf(&msg, &mi.log_message);
 	strbuf_stripspace(&msg, 0);
 
-	if (state->signoff)
-		am_signoff(&msg);
-
 	assert(!state->author_name);
 	state->author_name = strbuf_detach(&author_name, NULL);
 
@@ -1848,6 +1845,9 @@ static void am_run(struct am_state *state, int resume)
 			if (skip)
 				goto next; /* mail should be skipped */
 
+			if (state->signoff)
+				am_append_signoff(state);
+
 			write_author_script(state);
 			write_commit_msg(state);
 		}
-- 
2.12.2.765.g2bf946761b

[PATCH 2/3] builtin/am: fold am_signoff() into am_append_signoff()

[Giuseppe Bilotta]

There are no more direct calls to am_signoff(), so we can fold its
logic  in am_append_signoff().

(This is done in a separate commit rather than in the previous one, to
make it easier to revert this specific change if additional calls are
ever introduced.)

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 builtin/am.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index d072027b5a..b29f885e41 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1181,42 +1181,39 @@ static void NORETURN die_user_resolve(const struct am_state *state)
 	exit(128);
 }
 
-static void am_signoff(struct strbuf *sb)
+/**
+ * Appends signoff to the "msg" field of the am_state.
+ */
+static void am_append_signoff(struct am_state *state)
 {
 	char *cp;
 	struct strbuf mine = STRBUF_INIT;
+	struct strbuf sb = STRBUF_INIT;
 
-	/* Does it end with our own sign-off? */
+	strbuf_attach(&sb, state->msg, state->msg_len, state->msg_len);
+
+	/* our sign-off */
 	strbuf_addf(&mine, "\n%s%s\n",
 		    sign_off_header,
 		    fmt_name(getenv("GIT_COMMITTER_NAME"),
 			     getenv("GIT_COMMITTER_EMAIL")));
-	if (mine.len < sb->len &&
-	    !strcmp(mine.buf, sb->buf + sb->len - mine.len))
+
+	/* Does sb end with it already? */
+	if (mine.len < sb.len &&
+	    !strcmp(mine.buf, sb.buf + sb.len - mine.len))
 		goto exit; /* no need to duplicate */
 
 	/* Does it have any Signed-off-by: in the text */
-	for (cp = sb->buf;
+	for (cp = sb.buf;
 	     cp && *cp && (cp = strstr(cp, sign_off_header)) != NULL;
 	     cp = strchr(cp, '\n')) {
-		if (sb->buf == cp || cp[-1] == '\n')
+		if (sb.buf == cp || cp[-1] == '\n')
 			break;
 	}
 
-	strbuf_addstr(sb, mine.buf + !!cp);
+	strbuf_addstr(&sb, mine.buf + !!cp);
 exit:
 	strbuf_release(&mine);
-}
-
-/**
- * Appends signoff to the "msg" field of the am_state.
- */
-static void am_append_signoff(struct am_state *state)
-{
-	struct strbuf sb = STRBUF_INIT;
-
-	strbuf_attach(&sb, state->msg, state->msg_len, state->msg_len);
-	am_signoff(&sb);
 	state->msg = strbuf_detach(&sb, &state->msg_len);
 }
 
-- 
2.12.2.765.g2bf946761b

[PATCH 3/3] rebase: pass --[no-]signoff option to git am

[Giuseppe Bilotta]

This makes it easy to sign off a whole patchset before submission.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 Documentation/git-rebase.txt |  5 +++++
 git-rebase.sh                |  3 ++-
 t/t3428-rebase-signoff.sh    | 46 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 53 insertions(+), 1 deletion(-)
 create mode 100755 t/t3428-rebase-signoff.sh

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 67d48e6883..e6f0b93337 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -385,6 +385,11 @@ have the long commit hash prepended to the format.
 	Recreate merge commits instead of flattening the history by replaying
 	commits a merge commit introduces. Merge conflict resolutions or manual
 	amendments to merge commits are not preserved.
+
+--signoff::
+	This flag is passed to 'git am' to sign off all the rebased
+	commits (see linkgit:git-am[1]).
+
 +
 This uses the `--interactive` machinery internally, but combining it
 with the `--interactive` option explicitly is generally not a good
diff --git a/git-rebase.sh b/git-rebase.sh
index 48d7c5ded4..6889fd19f3 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -34,6 +34,7 @@ root!              rebase all reachable commits up to the root(s)
 autosquash         move commits that begin with squash!/fixup! under -i
 committer-date-is-author-date! passed to 'git am'
 ignore-date!       passed to 'git am'
+signoff!           passed to 'git am'
 whitespace=!       passed to 'git apply'
 ignore-whitespace! passed to 'git apply'
 C=!                passed to 'git apply'
@@ -321,7 +322,7 @@ do
 	--ignore-whitespace)
 		git_am_opt="$git_am_opt $1"
 		;;
-	--committer-date-is-author-date|--ignore-date)
+	--committer-date-is-author-date|--ignore-date|--signoff|--no-signoff)
 		git_am_opt="$git_am_opt $1"
 		force_rebase=t
 		;;
diff --git a/t/t3428-rebase-signoff.sh b/t/t3428-rebase-signoff.sh
new file mode 100755
index 0000000000..2afb564701
--- /dev/null
+++ b/t/t3428-rebase-signoff.sh
@@ -0,0 +1,46 @@
+#!/bin/sh
+
+test_description='git rebase --signoff
+
+This test runs git rebase --signoff and make sure that it works.
+'
+
+. ./test-lib.sh
+
+# A simple file to commit
+cat >file <<EOF
+a
+EOF
+
+# Expected commit message after rebase --signoff
+cat >expected-signed <<EOF
+first
+
+Signed-off-by: $(git var GIT_COMMITTER_IDENT | sed -e "s/>.*/>/")
+EOF
+
+# Expected commit message after rebase without --signoff (or with --no-signoff)
+cat >expected-unsigned <<EOF
+first
+EOF
+
+
+# We configure an alias to do the rebase --signoff so that
+# on the next subtest we can show that --no-signoff overrides the alias
+test_expect_success 'rebase --signoff adds a sign-off line' '
+	git commit --allow-empty -m "Initial empty commit" &&
+	git add file && git commit -m first &&
+	git config alias.rbs "rebase --signoff" &&
+	git rbs HEAD^ &&
+	git cat-file commit HEAD | sed -e "1,/^\$/d" > actual &&
+	test_cmp expected-signed actual
+'
+
+test_expect_success 'rebase --no-signoff does not add a sign-off line' '
+	git commit --amend -m "first" &&
+	git rbs --no-signoff HEAD^ &&
+	git cat-file commit HEAD | sed -e "1,/^\$/d" > actual &&
+	test_cmp expected-unsigned actual
+'
+
+test_done
-- 
2.12.2.765.g2bf946761b

Re: [PATCH 3/3] rebase: pass --[no-]signoff option to git am

[Giuseppe Bilotta]

Damnit! I just realized that I forgot to amend before the format-patch:

On Sat, Apr 15, 2017 at 4:41 PM, Giuseppe Bilotta
<giuseppe.bilotta@gmail.com> wrote:

> +signoff!           passed to 'git am'

This should be without the ! or --no-signoff is not accepted. Do I
need to resend or ... ?

Re: [PATCH 3/3] rebase: pass --[no-]signoff option to git am

[Junio C Hamano]

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

> Damnit! I just realized that I forgot to amend before the format-patch:
>
> On Sat, Apr 15, 2017 at 4:41 PM, Giuseppe Bilotta
> <giuseppe.bilotta@gmail.com> wrote:
>
>> +signoff!           passed to 'git am'
>
> This should be without the ! or --no-signoff is not accepted. Do I
> need to resend or ... ?

Resend or send something that can be "git apply"ed, which would
reduce the chance of mistakes.  Let the maintainer _TYPE_ as little
as possible, or typoes will sneak in.

Thanks.

Re: [PATCH 0/3] rebase --signoff

[Junio C Hamano]

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

> Allow signing off a whole patchset by rebasing it with the --signoff
> option, which is simply passed through to git am.

>  Documentation/git-rebase.txt |  5 +++++
>  builtin/am.c                 | 39 +++++++++++++++++--------------------
>  git-rebase.sh                |  3 ++-
>  t/t3428-rebase-signoff.sh    | 46 ++++++++++++++++++++++++++++++++++++++++++++

Two questions.

 - Is it better to add a brand new test script than adding new tests
   to existing scripts that test "git rebase"?

 - How does this interact with "git rebase -i" and other modes of
   operation?

Re: [PATCH 0/3] rebase --signoff

[Giuseppe Bilotta]

On Mon, Apr 17, 2017 at 9:12 AM, Junio C Hamano <gitster@pobox.com> wrote:

> Two questions.
>
>  - Is it better to add a brand new test script than adding new tests
>    to existing scripts that test "git rebase"?

Since this is a completely (in some sense) new feature, I felt it was
appropriate to put all --signoff-related tests in their own file. So,
if the need arises to put more tests concerning the interaction of
signoff with other stuff, this new test file can be extended.

>  - How does this interact with "git rebase -i" and other modes of
>    operation?

A better question would maybe be how do we want this to interact? For
example, with -i: do we want -i --signoff to just sign off everything?
Or do we want a new -i command (o, signoff) to signoff only individual
commits on request? When preserving merges, do we want to sign-off
merge-commits too? I'm not entirely sure what the best policy would
be.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH 0/3] rebase --signoff

[Junio C Hamano]

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

>>  - How does this interact with "git rebase -i" and other modes of
>>    operation?
>
> A better question would maybe be how do we want this to interact?

If "git rebase -i/-m --signoff" will not do anything (which I
suspect is what we have here), we at least would want it to be
documented, or the combination be made to error out, I would think.

A better question can wait until that happens ;-)

Re: [PATCH 0/3] rebase --signoff

[Giuseppe Bilotta]

On Tue, Apr 18, 2017 at 2:14 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>
>>>  - How does this interact with "git rebase -i" and other modes of
>>>    operation?
>>
>> A better question would maybe be how do we want this to interact?
>
> If "git rebase -i/-m --signoff" will not do anything (which I
> suspect is what we have here), we at least would want it to be
> documented, or the combination be made to error out, I would think.
>
> A better question can wait until that happens ;-)

I've been looking into adding signoff support to the rest of
git-rebase, but the thing is far less trivial to do than I initially
imagined, since the interactive part is split across a number of
sections and files, including C helpers and the sequencer itself. It
can _probably_ be done, but building tests for all corner cases is
quite the daunting task. I think that for the moment I'll resubmit
with the Documentation fix to declare it non-interactive only, and
then leave the extended for later.

-- 
Giuseppe "Oblomov" Bilotta