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

From: Paul Tan <pyokagan@gmail.com>
Date: Jun 26, 2015, 08:11 UTC
Message-ID: <CACRoPnSyu6or8nkMQ0yS_KcLwumJVNv3pfSFGcSDyk4-P_VZ_A@mail.gmail.com>
In-Reply-To: <72c470f3a6890dfcb66f1439383d2278@www.dscho.org>

On Thu, Jun 25, 2015 at 12:36 AM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

Show 44 quoted lines

> On 2015-06-18 13:25, Paul Tan wrote:
>> diff --git a/builtin/am.c b/builtin/am.c
>> index 7b97ea8..d6434e4 100644
>> --- a/builtin/am.c
>> +++ b/builtin/am.c
>> @@ -94,6 +126,105 @@ static int read_state_file(struct strbuf *sb,
>> const char *file, size_t hint, int
>>  }
>>
>>  /**
>> + * Reads a KEY=VALUE shell variable assignment from fp, and returns the VALUE
>> + * in `value`. VALUE must be a quoted string, and the KEY must match `key`.
>> + * Returns 0 on success, -1 on failure.
>> + *
>> + * This is used by read_author_script() to read the GIT_AUTHOR_* variables from
>> + * the author-script.
>> + */
>> +static int read_shell_var(struct strbuf *value, FILE *fp, const char *key)
>> +{
>> +     struct strbuf sb = STRBUF_INIT;
>> +     char *str;
>> +
>> +     if (strbuf_getline(&sb, fp, '\n'))
>> +             return -1;
>> +
>> +     if (!skip_prefix(sb.buf, key, (const char **)&str))
>> +             return -1;
>> +
>> +     if (!skip_prefix(str, "=", (const char **)&str))
>> +             return -1;
>> +
>> +     str = sq_dequote(str);
>> +     if (!str)
>> +             return -1;
>> +
>> +     strbuf_reset(value);
>> +     strbuf_addstr(value, str);
>> +
>> +     strbuf_release(&sb);
>> +
>> +     return 0;
>> +}
>
> How about using `strbuf_remove()` and keeping `str` as `const char *`?

OK, I'll try that out. Looks like this now:

static char *read_shell_var(FILE *fp, const char *key)
{
    struct strbuf sb = STRBUF_INIT;
    const char *str;

    if (strbuf_getline(&sb, fp, '\n'))
        return NULL;

    if (!skip_prefix(sb.buf, key, &str))
        return NULL;

    if (!skip_prefix(str, "=", &str))
        return NULL;

    strbuf_remove(&sb, 0, str - sb.buf);

    str = sq_dequote(sb.buf);
    if (!str)
        return NULL;

    return strbuf_detach(&sb, NULL);
}

> I also think we can fold it into the `read_author_script()` function and make it more resilient with regards to the order of the variables. Something like this:
> [...]

Hmm, I think we should be very strict about parsing the author-script file though. As explained in read_author_script(), git-am.sh evals the author-script, which we can't in C. I would much rather we barf at the first sign that the author-script is not what we expect, rather than attempt to parse it as much as possible, but end up with the wrong results as compared to git-am.sh.

Besides, currently git-am.sh will always write the author-script with the order of GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL and GIT_AUTHOR_DATE. If the order is wrong, I would think it means that something is messing with the author-script, and it would be better if we bail out immediately, instead of potentially doing something wrong.

Show 18 quoted lines

>> +/**
>> + * 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. Again, if you follow my suggestion to keep a "scratch pad" strbuf in am_state, you could reuse that.

Right, makes sense. I've implemented it on my end.

Thanks, Paul