Re: [PATCH v4 3/4] builtin/stash: provide a way to export stashes to a ref

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Apr 13, 2022, 15:36 UTC
Message-ID: <220413.86ee21f0g9.gmgdl@evledraar.gmail.com>
In-Reply-To: <20220407215352.3491567-4-sandals@crustytoothpaste.net>

On Thu, Apr 07 2022, brian m. carlson wrote:

Show 16 quoted lines

> +static int write_commit_with_parents(struct object_id *out, const struct object_id *oid, struct commit_list *parents)
> +{
> +	size_t author_len, committer_len;
> +	struct commit *this;
> +	const char *orig_author, *orig_committer;
> +	char *author = NULL, *committer = NULL;
> +	const char *buffer;
> +	unsigned long bufsize;
> +	const char *p;
> +	struct strbuf msg = STRBUF_INIT;
> +	int ret = 0;
> +
> +	this = lookup_commit_reference(the_repository, oid);
> +	buffer = get_commit_buffer(this, &bufsize);
> +	orig_author = find_commit_header(buffer, "author", &author_len);
> +	orig_committer = find_commit_header(buffer, "committer", &committer_len);

In builtin/am.c we also start with this, but follow it with passing the data through split_ident_line(), any reason we're not doing the same thing here, and are there cases wehre that would result in propagating not-sanity-checked-enough data?

Show 10 quoted lines

> +	p = memmem(buffer, bufsize, "\n\n", 2);
> +
> +	if (!orig_author || !orig_committer || !p) {
> +		ret = error(_("cannot parse commit %s"), oid_to_hex(oid));
> +		goto out;
> +	}
> +	/* Jump to message. */
> +	p += 2;
> +	strbuf_addstr(&msg, "git stash: ");
> +	strbuf_add(&msg, p, bufsize - (p - buffer));

And more on those behavior differences, the am.c version seems to not care that we *might* have a \0 in a commit buffer and just uses xstrdup(msg + 2).

But this one looks like it will faithfully carry that \0-containing message forward.

Probably OK, just checking...