Re: [PATCH/WIP v3 07/31] am: extract patch, message and authorship with git-mailinfo

[Junio C Hamano <gitster@pobox.com>]

Paul Tan <pyokagan@gmail.com> writes:

> OK, I'll try that out. Looks like this now:
>
> static char *read_shell_var(FILE *fp, const char *key)
> {
> ...
>     str = sq_dequote(sb.buf);
>     if (!str)
>         return NULL;

You are unlikely to get !str, but if it does, you leak sb here,
don't you?

>     return strbuf_detach(&sb, NULL);

This call is OK; if you passed the &length to detach, you're likely
to get a wrong result, though ;-)

sq_dequote() is one of the older parts of the API set and its "we
know it cannot do anything but shrink, so we'd do it in-place"
attitude, which may be vastly useful in practice, is now showing
some impedance mismatch with newer parts of the API like strbuf.

>>> +/**
>>> + * Saves state->author_name, state->author_email and state->author_date in
>>> + * `filename` as an "author script", which is the format used by git-am.sh.
>>> + */
>>> +static void write_author_script(const struct am_state *state)
>>> +{
>>> +     static const char fmt[] = "GIT_AUTHOR_NAME=%s\n"
>>> +             "GIT_AUTHOR_EMAIL=%s\n"
>>> +             "GIT_AUTHOR_DATE=%s\n";
>>> +     struct strbuf author_name = STRBUF_INIT;
>>> +     struct strbuf author_email = STRBUF_INIT;
>>> +     struct strbuf author_date = STRBUF_INIT;
>>> +
>>> +     sq_quote_buf(&author_name, state->author_name.buf);
>>> +     sq_quote_buf(&author_email, state->author_email.buf);
>>> +     sq_quote_buf(&author_date, state->author_date.buf);
>>
>> The `sq_quote_buf()` function does not call
>> strbuf_reset()`. Therefore you could just use a single strbuf to
>> construct the entire three lines and then write that out.

Yup.  "quote" appends to the output, so you could do this:

	add(&out, "GIT_AUTHOR_NAME=");
        quote(&out, state->author_name);
        add(&out, "\"\nGIT_AUTHOR_EMAIL=");
        quote(&out, state->author_email);
        ...

I am not sure if that is easier to read than what you have, though.

>> Again, if
>> you follow my suggestion to keep a "scratch pad" strbuf in am_state,
>> you could reuse that.

Don't do "scratch pad" in a structure that is passed around to
various people.  Somebody may be tempted to use the scratch pad
while he has the control, but as soon as he becomes complex enough
to require calling some helper functions, the ownership rules of the
scratch pad will become cumbersome to manage and understandable only
by the person who originally wrote the codepath.