threads / patch / 45702

patch, 3 partsrebase --signoff

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

## tl;dr

10 messages between Apr 15, 2017 and Apr 18, 2017. Diffs are folded; open one to read it.

replies: 9people: 2as markdown or json

Giuseppe Bilotta· Apr 15, 2017, 14:41 UTC · lore

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
Giuseppe Bilotta· Apr 15, 2017, 14:41 UTC · re: Giuseppe Bilotta · lore

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

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(-)
Show changes to builtin/am.c +3 −3
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
Giuseppe Bilotta· Apr 15, 2017, 14:41 UTC · re: Giuseppe Bilotta · lore

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

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(-)
Show changes to builtin/am.c +15 −18
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
Giuseppe Bilotta· Apr 15, 2017, 14:41 UTC · re: Giuseppe Bilotta · lore

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

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
Show changes to 3 files +53 −1

Documentation/git-rebase.txt, git-rebase.sh, 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
Giuseppe Bilotta· Apr 15, 2017, 17:45 UTC · re: Giuseppe Bilotta · lore

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

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 ... ?

Junio C Hamano· Apr 17, 2017, 04:17 UTC · re: Giuseppe Bilotta · lore

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

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
Show 9 quoted lines
> 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.
Junio C Hamano· Apr 17, 2017, 07:12 UTC · re: Giuseppe Bilotta · lore

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

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?
Giuseppe Bilotta· Apr 17, 2017, 14:12 UTC · re: Junio C Hamano · lore

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

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
Junio C Hamano· Apr 18, 2017, 00:14 UTC · re: Giuseppe Bilotta · lore

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

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 ;-)
Giuseppe Bilotta· Apr 18, 2017, 08:40 UTC · re: Junio C Hamano · lore

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

On Tue, Apr 18, 2017 at 2:14 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 12 quoted lines
> 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

← back to recent threads