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

[Johannes Schindelin <johannes.schindelin@gmx.de>]

Hi Paul,

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 *`? 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:

static int read_author_script(struct am_state *state)
{
	struct strbuf sb = STRBUF_INIT;
	const char *filename = am_path(state, "author-script");
	FILE *fp = fopen(filename, "r");
	if (!fp) {
		if (errno == ENOENT)
			return 0;
		die_errno(_("could not open '%s' for reading"), filename);
	}

	while (!strbuf_getline(&sb, fp, '\n')) {
		char *equal = strchr(sb.buf, '='), **var;

		if (!equal) {
error:
			fclose(fp);
			return -1;
		}
		*equal = '\0';
		if (!strcmp(sb.buf, "GIT_AUTHOR_NAME"))
			var = &state->author_name;
		else if (!strcmp(sb.buf, "GIT_AUTHOR_EMAIL"))
			var = &state->author_email;
		else if (!strcmp(sb.buf, "GIT_AUTHOR_DATE"))
			var = &state->author_date;
		else
			goto error;
		*var = xstrdup(sq_dequote(equal + 1));
	}

	fclose(fp);
	return -1;
}

If you follow my earlier suggestion to keep a strbuf inside the am_state, you could reuse that here, too.

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

That scratch pad could come in handy in a couple of other places in the rest of this patch.

Ciao,
Dscho