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:

> +	/* 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...

> +/**
> + * 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:

> +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.