Hi,
Thanks for handling this.
On Sun, Sep 6, 2015 at 12:56 PM, Junio C Hamano <gits...@pobox.com> wrote:
> Linus noticed that the recently reimplementated "git am -s" defines
s/reimplementated/reimplemented/ ?
> the trailer block too rigidly, resulting an unnecessary blank line
s/resulting an/resulting in an/ ?
> 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 <origi...@auth.or>
> [rv: tweaked frotz and nitfol]
> Signed-off-by: Re Viewer <r...@ew.er>
> Signed-off-by: Other Reviewer <ot...@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
s/extra blank/extra blank line/ ?
> 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 <torva...@linux-foundation.org>
> Signed-off-by: Junio C Hamano <gits...@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)
> +{
Hmm, okay, but now we have two similarly named functions am_signoff()
and am_append_signoff() which both do nearly similar things, the only
difference being am_signoff() operates on a strbuf while
am_append_signoff() operates on the "msg" char* field in the am_state,
which seems a bit iffy to me. I wonder if the logic could be
implemented in am_append_signoff() instead so we have only one
function?
> + 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")));
Maybe use git_committer_info() here?
> + if (mine.len < sb->len &&
> + !strcmp(mine.buf, sb->buf + sb->len - mine.len))
Perhaps use ends_with()?
> + 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);
Thanks again,
Paul
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
