Re: [PATCH v5 4/4] builtin/stash: provide a way to import stashes from a ref

[Junio C Hamano <gitster@pobox.com>]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> +static int do_import_stash(struct repository *r, const char *rev)
> +{
> +	struct object_id chain;
> +	struct oid_array items = OID_ARRAY_INIT;
> +	int res = 0;
> +	int i;
> +	const char *buffer = NULL;
> +	struct commit *this = NULL;
> +	char *msg = NULL;
> +
> +	if (repo_get_oid(r, rev, &chain))
> +		return error(_("not a valid revision: %s"), rev);
> +
> +	/*
> +	 * Walk the commit history, finding each stash entry, and load data into
> +	 * the array.
> +	 */
> +	for (i = 0;; i++) {
> +		struct object_id tree, oid;
> +		char revision[GIT_MAX_HEXSZ + 1];
> +
> +		oid_to_hex_r(revision, &chain);
> +
> +		if (get_oidf(&tree, "%s:", revision) ||
> +		    !oideq(&tree, r->hash_algo->empty_tree)) {
> +			res = error(_("%s is not a valid exported stash commit"), revision);
> +			goto out;
> +		}
> +		if (get_oidf(&chain, "%s^1", revision) ||
> +		    get_oidf(&oid, "%s^2", revision))
> +			break;

This is to stop at the sentinel commit at the end.  Don't we want to
make sure that it actually has the expected shape of the sentinel?

IOW, how robust do we try to be against being fed a random mergy
commit history (e.g., our 'master') and mistakenly adding nonsense
stash entries as refs/stash@{<n>}?

> +	/*
> +	 * Now, walk each entry, adding it to the stash as a normal stash
> +	 * commit.
> +	 */
> +	for (i = items.nr - 1; i >= 0; i--) {
> +		unsigned long bufsize;
> +		const char *p;
> +		const struct object_id *oid = items.oid + i;
> +
> +		this = lookup_commit_reference(r, oid);
> +		buffer = repo_get_commit_buffer(r, this, &bufsize);
> +		if (!buffer) {
> +			res = error(_("cannot read commit buffer for %s"), oid_to_hex(oid));
> +			goto out;
> +		}
> +
> +		p = strstr(buffer, "\n\n");
> +		if (!p) {
> +			res = error(_("cannot parse commit %s"), oid_to_hex(oid));
> +			goto out;
> +		}
> +
> +		p += 2;
> +		msg = xmemdupz(p, bufsize - (p - buffer));

Here, we could check "git stash: " string to make sure that it is as
expected in an exported stash we previously made, and abort, just
like the above "cannot parse" case.

> +		repo_unuse_commit_buffer(r, this, buffer);
> +		buffer = NULL;
> +
> +		if (do_store_stash(oid, msg, 1)) {

Should we be making sure that the object named by "oid" does look
like a stash, like what is done in builtin/stash.c:get_stash_info(),
or is assert_stash_like() called from do_store_stash() sufficient?