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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

Thanks for handling this.

On Sun, Sep 6, 2015 at 12:56 PM, Junio C Hamano <gitster@xxxxxxxxx> 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 <original@xxxxxxx>
>         [rv: tweaked frotz and nitfol]
>         Signed-off-by: Re Viewer <rv@xxxxx>
>         Signed-off-by: Other Reviewer <other@xxxxxxxx>
>         ---
>         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 <torvalds@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
> ---
>  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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]