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

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Apr 07, 2016 at 10:08:37AM -0700, Junio C Hamano wrote:
> "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.

Hmm - splitting it out was easy for development since I could verify all
tests pass.  But if you do want the optimization, then sure, I'll have
to squash it in.

> >  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?
> 
> > +
> > +	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);