Re: [PATCH 3/4] builtin/am: read mailinfo from file

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 7, 2016, 17:08 UTC
Message-ID: <xmqqinzt1h4a.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1460042563-32741-4-git-send-email-mst@redhat.com>

"Michael S. Tsirkin" <mst@redhat.com> writes:

> Slightly slower, but will allow easy additional processing on it.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---

I haven't read 4/4 yet, but can guess from what this patch does that the next step would let others futz with the contents of the message that is on disk (i.e. what mailinfo() wrote out, which is identical to what we have in mi.log_message at this point of the codeflow) before you do the new strbuf_read_file().

It probably is better to do this as part of 4/4; it is easier to understand why this is a good and necessary thing to do. An obvious improvement is to omit this extra "read back from the filesystem" when we won't be making any interpret-trailer calls (i.e. no -t option from the command line), but if we stop at this step 3/4, then we'd end up wasting cycles without having any benefit.

Show 24 quoted lines

>  builtin/am.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/am.c b/builtin/am.c
> index d003939..4180b04 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -1246,6 +1246,7 @@ static int parse_mail(struct am_state *state, const char *mail)
>  	FILE *fp;
>  	struct strbuf sb = STRBUF_INIT;
>  	struct strbuf msg = STRBUF_INIT;
> +	struct strbuf log_msg = STRBUF_INIT;
>  	struct strbuf author_name = STRBUF_INIT;
>  	struct strbuf author_date = STRBUF_INIT;
>  	struct strbuf author_email = STRBUF_INIT;
> @@ -1330,7 +1331,12 @@ static int parse_mail(struct am_state *state, const char *mail)
>  	}
>  
>  	strbuf_addstr(&msg, "\n\n");
> -	strbuf_addbuf(&msg, &mi.log_message);
> +
> +	if (strbuf_read_file(&log_msg,  am_path(state, "msg"), 0) < 0) {
> +		die_errno(_("could not read '%s'"), am_path(state, "msg"));
> +	}

I do not think these {} serve any purpose; drop them?

Show 13 quoted lines

> +
> +	strbuf_addbuf(&msg, &log_msg);
>  	strbuf_stripspace(&msg, 0);
>  
>  	if (state->signoff)
> @@ -1349,6 +1355,7 @@ static int parse_mail(struct am_state *state, const char *mail)
>  	state->msg = strbuf_detach(&msg, &state->msg_len);
>  
>  finish:
> +	strbuf_release(&log_msg);
>  	strbuf_release(&msg);
>  	strbuf_release(&author_date);
>  	strbuf_release(&author_email);