Re: [PATCH] am: match --signoff to the original scripted version

> between the existing sign-offs and his new sign-off.  An e-mail
> submission sent to Linus in real life ends with mixture of sign-offs
> and commentaries, e.g.
>
>         title here
>
>         message here
>
>         Signed-off-by: Original Author <original@auth.or>
>         [rv: tweaked frotz and nitfol]
>         Signed-off-by: Re Viewer <rv@ew.er>
>         Signed-off-by: Other Reviewer <other@rev.ewer>
>         ---
>         patch here
>
> Because the reimplementation reused append_signoff() helper that is
> used by other codepaths, which is unaware that people intermix such
> comments with their sign-offs in the trailer block, such a message
> was judged to end with a non-trailer, resulting in an extra blank

> before adding a new sign-off.
>
> The original scripted version of "git am" used a lot looser
> definition, i.e. "if and only if there is no line that begins with
> Signed-off-by:, add a blank line before adding a new sign-off".  For
> the upcoming release, stop using the append_signoff() in "git am"
> and reimplement the looser definition used by the scripted version
> to use only in "git am" to fix this regression in "am" while
> avoiding new regressions to other users of append_signoff().
>
> In the longer term, we should look into loosening append_signoff()
> so that other codepaths that add a new sign-off behave the same way
> as "git am -s", but that is a task for post-release.
>
> Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  builtin/am.c  | 31 +++++++++++++++++++++++++++++--
>  t/t4150-am.sh | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 77 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/am.c b/builtin/am.c
> index 634f7a7..e7828e5 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -1191,6 +1191,33 @@ static void NORETURN die_user_resolve(const struct am_state *state)
>         exit(128);
>  }
>
> +static void am_signoff(struct strbuf *sb)
> +{

> +       char *cp;
> +       struct strbuf mine = STRBUF_INIT;
> +
> +       /* Does it end with our own sign-off? */
> +       strbuf_addf(&mine, "\n%s%s\n",
> +                   sign_off_header,
> +                   fmt_name(getenv("GIT_COMMITTER_NAME"),
> +                            getenv("GIT_COMMITTER_EMAIL")));

> +               goto exit; /* no need to duplicate */
> +
> +       /* Does it have any Signed-off-by: in the text */
> +       for (cp = sb->buf;
> +            cp && *cp && (cp = strstr(cp, sign_off_header)) != NULL;
> +            cp = strchr(cp, '\n')) {
> +               if (sb->buf == cp || cp[-1] == '\n')
> +                       break;
> +       }
> +
> +       strbuf_addstr(sb, mine.buf + !!cp);
> +exit:
> +       strbuf_release(&mine);
> +}
> +
>  /**
>   * Appends signoff to the "msg" field of the am_state.
>   */
> @@ -1199,7 +1226,7 @@ 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);
> -       append_signoff(&sb, 0, 0);
> +       am_signoff(&sb);
>         state->msg = strbuf_detach(&sb, &state->msg_len);
>  }
>
> @@ -1303,7 +1330,7 @@ static int parse_mail(struct am_state *state, const char *mail)
>         stripspace(&msg, 0);
>
>         if (state->signoff)
> -               append_signoff(&msg, 0, 0);
> +               am_signoff(&msg);
>
>         assert(!state->author_name);
>         state->author_name = strbuf_detach(&author_name, NULL);