# [PATCH 0/3] rebase --signoff 10 messages from 2017-04-15 to 2017-04-18. Participants: Giuseppe Bilotta, Junio C Hamano. Thread: https://gitlist.dev/t/45702 ## Giuseppe Bilotta, 2017-04-15 14:41 Subject: [PATCH 0/3] rebase --signoff Message-ID: <20170415144103.11986-1-giuseppe.bilotta@gmail.com> URL: https://gitlist.dev/e/20170415144103.11986-1-giuseppe.bilotta%40gmail.com ``` 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, 2017-04-15 14:41 Subject: [PATCH 1/3] builtin/am: obey --signoff also when --rebasing Message-ID: <20170415144103.11986-2-giuseppe.bilotta@gmail.com> URL: https://gitlist.dev/e/20170415144103.11986-2-giuseppe.bilotta%40gmail.com In-Reply-To: <20170415144103.11986-1-giuseppe.bilotta@gmail.com> ``` 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 --- 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 ``` ## Giuseppe Bilotta, 2017-04-15 14:41 Subject: [PATCH 2/3] builtin/am: fold am_signoff() into am_append_signoff() Message-ID: <20170415144103.11986-3-giuseppe.bilotta@gmail.com> URL: https://gitlist.dev/e/20170415144103.11986-3-giuseppe.bilotta%40gmail.com In-Reply-To: <20170415144103.11986-1-giuseppe.bilotta@gmail.com> ``` 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 --- 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 ``` ## Giuseppe Bilotta, 2017-04-15 14:41 Subject: [PATCH 3/3] rebase: pass --[no-]signoff option to git am Message-ID: <20170415144103.11986-4-giuseppe.bilotta@gmail.com> URL: https://gitlist.dev/e/20170415144103.11986-4-giuseppe.bilotta%40gmail.com In-Reply-To: <20170415144103.11986-1-giuseppe.bilotta@gmail.com> ``` This makes it easy to sign off a whole patchset before submission. Signed-off-by: Giuseppe Bilotta --- 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 <expected-signed <.*/>/") +EOF + +# Expected commit message after rebase without --signoff (or with --no-signoff) +cat >expected-unsigned < 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, 2017-04-15 17:45 Subject: Re: [PATCH 3/3] rebase: pass --[no-]signoff option to git am Message-ID: URL: https://gitlist.dev/e/CAOxFTczZS6aDGzDVLskQGObtDOeLL_i3-8jJdh8OrQFXVGbOHQ%40mail.gmail.com In-Reply-To: <20170415144103.11986-4-giuseppe.bilotta@gmail.com> ``` Damnit! I just realized that I forgot to amend before the format-patch: On Sat, Apr 15, 2017 at 4:41 PM, Giuseppe Bilotta 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, 2017-04-17 04:17 Subject: Re: [PATCH 3/3] rebase: pass --[no-]signoff option to git am Message-ID: URL: https://gitlist.dev/e/xmqqa87fk6r4.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Giuseppe Bilotta 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 > 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, 2017-04-17 07:12 Subject: Re: [PATCH 0/3] rebase --signoff Message-ID: URL: https://gitlist.dev/e/xmqqshl7ik21.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20170415144103.11986-1-giuseppe.bilotta@gmail.com> ``` Giuseppe Bilotta 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, 2017-04-17 14:12 Subject: Re: [PATCH 0/3] rebase --signoff Message-ID: URL: https://gitlist.dev/e/CAOxFTczhfvzhrSiCj7SgLXbO3hrBW_QaDVZMpOqrij_hCJyCzg%40mail.gmail.com In-Reply-To: ``` On Mon, Apr 17, 2017 at 9:12 AM, Junio C Hamano 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, 2017-04-18 00:14 Subject: Re: [PATCH 0/3] rebase --signoff Message-ID: URL: https://gitlist.dev/e/xmqqinm2inc2.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Giuseppe Bilotta 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, 2017-04-18 08:40 Subject: Re: [PATCH 0/3] rebase --signoff Message-ID: URL: https://gitlist.dev/e/CAOxFTcyv_o8Gbjj-R0g6eK-i1QDZUA9okwq%3D7Yb4iW3n%3DkF-mQ%40mail.gmail.com In-Reply-To: ``` On Tue, Apr 18, 2017 at 2:14 AM, Junio C Hamano wrote: > Giuseppe Bilotta 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 ```