Re: [PATCH/WIP v3 07/31] am: extract patch, message and authorship with git-mailinfo
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 18, 2015, 21:10 UTC
- Message-ID
- <xmqqtwu4d8pg.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <1434626743-8552-8-git-send-email-pyokagan@gmail.com>
Paul Tan <pyokagan@gmail.com> writes:
Show 5 quoted lines
> + /* commit message and metadata */ > + struct strbuf author_name; > + struct strbuf author_email; > + struct strbuf author_date; > + struct strbuf msg;
Same comment as "dir" in the earlier patch applies to these. If the fields are read or computed and then kept constant, use a temporary variable that is a strbuf to read/compute the final value, and then detach to a "const char *" field. If they are constantly changing and in-place updates are vital, then they can and should be strbufs, but I do not think that is the case for these.
For example...
Show 16 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);As you use a separate author_name variable here, what gets sq-quoted that is in *state does not have to be strbuf.
The code to read is the same story:
Show 15 quoted lines
> +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;
> +...
> + strbuf_reset(value);
> + strbuf_addstr(value, str);
> +
> + strbuf_release(&sb);
> +
> + return 0;
> +}As you use a separate sb strbuf variable here, there is no need for "value" to be pointing at a strbuf; it could be "char **" that sb's contents is detached into.