{"thread":{"id":"45698","subject":"[PATCHv3] rebase: pass --[no-]signoff option to git am","startedAt":"2017-04-14T22:57:27Z","lastAt":"2017-04-15T10:05:28Z","messageCount":4,"participants":["Giuseppe Bilotta","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"316867","messageId":"20170414225713.29710-1-giuseppe.bilotta@gmail.com","threadId":"45698","inReplyTo":null,"subject":"[PATCHv3] rebase: pass --[no-]signoff option to git am","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2017-04-14T22:57:13Z","receivedAt":"2017-04-14T22:57:27Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"This makes it easy to sign off a whole patchset before submission.\n\nTo make things work, we also fix a design issue in git-am that made it\nignore the signoff option during rebase (specifically, signoff was\nhandled in parse_mail(), but not in parse_mail_rebasing()).\n\nThis is trivially fixed by moving the conditional addition of the\nsignoff from parse_mail() to the caller (am_run()), after either of the\nparse_mail*() functions were called.\n\nSigned-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>\n---\n Documentation/git-rebase.txt | 5 +++++\n builtin/am.c                 | 6 +++---\n git-rebase.sh                | 3 ++-\n 3 files changed, 10 insertions(+), 4 deletions(-)\n\nAs suggested by Ævar, it's [no-]signoff, not [no]-signoff.\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 67d48e6883..e6f0b93337 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -385,6 +385,11 @@ have the long commit hash prepended to the format.\n \tRecreate merge commits instead of flattening the history by replaying\n \tcommits a merge commit introduces. Merge conflict resolutions or manual\n \tamendments to merge commits are not preserved.\n+\n+--signoff::\n+\tThis flag is passed to 'git am' to sign off all the rebased\n+\tcommits (see linkgit:git-am[1]).\n+\n +\n This uses the `--interactive` machinery internally, but combining it\n with the `--interactive` option explicitly is generally not a good\ndiff --git a/builtin/am.c b/builtin/am.c\nindex f7a7a971fb..d072027b5a 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1321,9 +1321,6 @@ static int parse_mail(struct am_state *state, const char *mail)\n \tstrbuf_addbuf(&msg, &mi.log_message);\n \tstrbuf_stripspace(&msg, 0);\n \n-\tif (state->signoff)\n-\t\tam_signoff(&msg);\n-\n \tassert(!state->author_name);\n \tstate->author_name = strbuf_detach(&author_name, NULL);\n \n@@ -1848,6 +1845,9 @@ static void am_run(struct am_state *state, int resume)\n \t\t\tif (skip)\n \t\t\t\tgoto next; /* mail should be skipped */\n \n+\t\t\tif (state->signoff)\n+\t\t\t\tam_append_signoff(state);\n+\n \t\t\twrite_author_script(state);\n \t\t\twrite_commit_msg(state);\n \t\t}\ndiff --git a/git-rebase.sh b/git-rebase.sh\nindex 48d7c5ded4..cce72d8494 100755\n--- a/git-rebase.sh\n+++ b/git-rebase.sh\n@@ -34,6 +34,7 @@ root!              rebase all reachable commits up to the root(s)\n autosquash         move commits that begin with squash!/fixup! under -i\n committer-date-is-author-date! passed to 'git am'\n ignore-date!       passed to 'git am'\n+[no-]signoff!      passed to 'git am'\n whitespace=!       passed to 'git apply'\n ignore-whitespace! passed to 'git apply'\n C=!                passed to 'git apply'\n@@ -321,7 +322,7 @@ do\n \t--ignore-whitespace)\n \t\tgit_am_opt=\"$git_am_opt $1\"\n \t\t;;\n-\t--committer-date-is-author-date|--ignore-date)\n+\t--committer-date-is-author-date|--ignore-date|--signoff|--no-signoff)\n \t\tgit_am_opt=\"$git_am_opt $1\"\n \t\tforce_rebase=t\n \t\t;;\n-- \n2.12.2.765.g845dc5dc05\n\n"},{"id":"316875","messageId":"xmqqefwum3mh.fsf@gitster.mtv.corp.google.com","threadId":"45698","inReplyTo":"20170414225713.29710-1-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv3] rebase: pass --[no-]signoff option to git am","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-15T09:17:26Z","receivedAt":"2017-04-15T09:17:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:\n\n> This makes it easy to sign off a whole patchset before submission.\n>\n> To make things work, we also fix a design issue in git-am that made it\n> ignore the signoff option during rebase (specifically, signoff was\n> handled in parse_mail(), but not in parse_mail_rebasing()).\n\nI doubt that the above implementation detail in the code is \"a\ndesign issue\"; it is a logical consequence of a design whose\n\"rebase\" never passes \"--signoff\" down to underlying \"am\", so it is\nunderstandable that whoever wants to pass \"--signoff\" thru during\nthe rebase needs to update the implementation, but I do not think it\nis fair to call that \"an issue\".\n\n>  Documentation/git-rebase.txt | 5 +++++\n>  builtin/am.c                 | 6 +++---\n>  git-rebase.sh                | 3 ++-\n>  3 files changed, 10 insertions(+), 4 deletions(-)\n\nWe need new tests for \"git rebase --signoff\" that makes sure this\nworks as expected and only when it should.\n\n> diff --git a/builtin/am.c b/builtin/am.c\n> index f7a7a971fb..d072027b5a 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -1321,9 +1321,6 @@ static int parse_mail(struct am_state *state, const char *mail)\n>  \tstrbuf_addbuf(&msg, &mi.log_message);\n>  \tstrbuf_stripspace(&msg, 0);\n>  \n> -\tif (state->signoff)\n> -\t\tam_signoff(&msg);\n> -\n>  \tassert(!state->author_name);\n>  \tstate->author_name = strbuf_detach(&author_name, NULL);\n>  \n> @@ -1848,6 +1845,9 @@ static void am_run(struct am_state *state, int resume)\n>  \t\t\tif (skip)\n>  \t\t\t\tgoto next; /* mail should be skipped */\n>  \n> +\t\t\tif (state->signoff)\n> +\t\t\t\tam_append_signoff(state);\n> +\n>  \t\t\twrite_author_script(state);\n>  \t\t\twrite_commit_msg(state);\n>  \t\t}\n\nThis removes the last direct caller to am_signoff().  It may be\nworth considering to remove the function and move its body to its\nonly internal caller am_append_signoff().\n"},{"id":"316876","messageId":"CAOxFTcwDrYvg5Nf1w9SfmM=Nt7XYsJPhKSYkJzMC0123EY94Aw@mail.gmail.com","threadId":"45698","inReplyTo":"xmqqefwum3mh.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3] rebase: pass --[no-]signoff option to git am","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2017-04-15T09:36:44Z","receivedAt":"2017-04-15T09:37:10Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Sat, Apr 15, 2017 at 11:17 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:\n>\n>> This makes it easy to sign off a whole patchset before submission.\n>>\n>> To make things work, we also fix a design issue in git-am that made it\n>> ignore the signoff option during rebase (specifically, signoff was\n>> handled in parse_mail(), but not in parse_mail_rebasing()).\n>\n> I doubt that the above implementation detail in the code is \"a\n> design issue\"; it is a logical consequence of a design whose\n> \"rebase\" never passes \"--signoff\" down to underlying \"am\", so it is\n> understandable that whoever wants to pass \"--signoff\" thru during\n> the rebase needs to update the implementation, but I do not think it\n> is fair to call that \"an issue\".\n\nGood point. It's an issue now that we want to be able to pass signoff,\nbut when the split was introduced it most definitely wasn't 8-)\n\n>>  Documentation/git-rebase.txt | 5 +++++\n>>  builtin/am.c                 | 6 +++---\n>>  git-rebase.sh                | 3 ++-\n>>  3 files changed, 10 insertions(+), 4 deletions(-)\n>\n> We need new tests for \"git rebase --signoff\" that makes sure this\n> works as expected and only when it should.\n\nWould the norm in this case be to introduce the test in the same\ncommit, or in a previous commit (as in: this is the feature we want to\nimplement, it obviously doesn't work now, but the next commit will fix\nthat), or in a subsequent one?\n\n>> diff --git a/builtin/am.c b/builtin/am.c\n>> index f7a7a971fb..d072027b5a 100644\n>> --- a/builtin/am.c\n>> +++ b/builtin/am.c\n>> @@ -1321,9 +1321,6 @@ static int parse_mail(struct am_state *state, const char *mail)\n>>       strbuf_addbuf(&msg, &mi.log_message);\n>>       strbuf_stripspace(&msg, 0);\n>>\n>> -     if (state->signoff)\n>> -             am_signoff(&msg);\n>> -\n>>       assert(!state->author_name);\n>>       state->author_name = strbuf_detach(&author_name, NULL);\n>>\n>> @@ -1848,6 +1845,9 @@ static void am_run(struct am_state *state, int resume)\n>>                       if (skip)\n>>                               goto next; /* mail should be skipped */\n>>\n>> +                     if (state->signoff)\n>> +                             am_append_signoff(state);\n>> +\n>>                       write_author_script(state);\n>>                       write_commit_msg(state);\n>>               }\n>\n> This removes the last direct caller to am_signoff().  It may be\n> worth considering to remove the function and move its body to its\n> only internal caller am_append_signoff().\n\nGood point. It becomes a bit bigger change though, so I'll probably\nsplit it off in a separate commit now.\n\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"316878","messageId":"xmqqa87im1hf.fsf@gitster.mtv.corp.google.com","threadId":"45698","inReplyTo":"CAOxFTcwDrYvg5Nf1w9SfmM=Nt7XYsJPhKSYkJzMC0123EY94Aw@mail.gmail.com","subject":"Re: [PATCHv3] rebase: pass --[no-]signoff option to git am","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-15T10:03:40Z","receivedAt":"2017-04-15T10:05:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:\n\n>> We need new tests for \"git rebase --signoff\" that makes sure this\n>> works as expected and only when it should.\n>\n> Would the norm in this case be to introduce the test in the same\n> commit, or in a previous commit (as in: this is the feature we want to\n> implement, it obviously doesn't work now, but the next commit will fix\n> that), or in a subsequent one?\n\nFor a new feature (especially with this small implementation), it is\nbest to have the test in the same commit.  \n\nWe often use the \"start with expect_failure, update the code while\nflipping _failure to _success\" pattern but that is primarily\nsuitable for bugfixes.\n"}]}