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

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

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

> We don't clear the stash first and instead add the specified stashes to
> the top of the stash.  This is because users may want to export just a
> few stashes, such as to share a small amount of work in progress with a
> colleague, and it would be undesirable for the receiving user to lose
> all of their data.  For users who do want to replace the stash, it's
> easy to do to: simply run "git stash clear" first.

Very sensible.

> +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;
> +	ssize_t i;
> +	const char *buffer = NULL;
> +	unsigned long bufsize;
> +	struct commit *this = NULL;
> +	char *msg = NULL;
> +
> +	if (repo_get_oid(r, rev, &chain))
> +		return error(_("not a valid revision: %s"), rev);
> +
> +	this = lookup_commit_reference(r, &chain);
> +	if (!this)
> +		return error(_("not a commit: %s"), rev);
> +
> +	/*
> +	 * Walk the commit history, finding each stash entry, and load data into
> +	 * the array.
> +	 */
> +	for (;;) {
> +		const char *author, *committer;
> +		size_t author_len, committer_len;
> +		const char *p;
> +		const char *expected = "git stash <git@stash> 1000684800 +0000";

Makes me wonder if we can somehow share this with [4/5] where we
establish the author and committer ident (timestamp spelled in a
different form).  Three strings that has to stay in sync is not a
huge deal, though.

> +		const char *prefix = "git stash: ";
> +		struct commit *stash;
> +		struct tree *tree = repo_get_commit_tree(r, this);
> +
> +		if (!tree ||
> +		    !oideq(&tree->object.oid, r->hash_algo->empty_tree) ||
> +		    (this->parents &&
> +		     (!this->parents->next || this->parents->next->next))) {
> +			res = error(_("%s is not a valid exported stash commit"),
> +					oid_to_hex(&this->object.oid));
> +			goto out;
> +		}

OK.  "buffer" is initialized to NULL before entering this loop, and
cleared to NULL at the end of this loop when "this" is moved, so
jumping to "out:" would not confuse the call to unuse_buffer().

> +		buffer = repo_get_commit_buffer(r, this, &bufsize);

And "buffer" becomes non-NULL and holds data associated with "this";
any "goto out" in the error path would do the right thing.  Very
nicely designed loop.

> +		if (!this->parents) {
> +			/*
> +			 * We don't have any parents.  Make sure this is our
> +			 * root commit.
> +			 */
> +			author = find_commit_header(buffer, "author", &author_len);
> +			committer = find_commit_header(buffer, "committer", &committer_len);
> +
> +			if (!author || !committer) {
> +				error(_("cannot parse commit %s"), oid_to_hex(&this->object.oid));
> +				goto out;
> +			}
> +
> +			if (author_len != strlen(expected) ||
> +			    committer_len != strlen(expected) ||
> +			    memcmp(author, expected, author_len) ||
> +			    memcmp(committer, expected, committer_len)) {
> +				res = error(_("found root commit %s with invalid data"), oid_to_hex(&this->object.oid));
> +				goto out;
> +			}
> +			break;
> +		}

OK.

> +		p = strstr(buffer, "\n\n");
> +		if (!p) {
> +			res = error(_("cannot parse commit %s"), oid_to_hex(&this->object.oid));
> +			goto out;
> +		}
> +
> +		p += 2;
> +		if (((size_t)(bufsize - (p - buffer)) < strlen(prefix)) ||
> +		    memcmp(prefix, p, strlen(prefix))) {
> +			res = error(_("found stash commit %s with unexpected prefix"), oid_to_hex(&this->object.oid));
> +			goto out;
> +		}

I'd call that "without expected prefix" (the difference being "we
expected 'git stash:' prefix but you have 'git stosh:' prefix" vs
"your commit title is 'hello world'"), but OK.  The important thing
is that we are being careful not to get confused.

It is unfortunate historical practice to measure almost everything
in "unsigned long" and bufsize here is one of them, but do we need
that cast?  We know strlen(prefix) is a small known constant
integer, we know p is not smaller than buffer, we know (p-buffer) is
not larger than bufsize.

> +		stash = this->parents->next->item;
> +
> +		if (repo_parse_commit(r, this->parents->item) ||
> +		    repo_parse_commit(r, stash)) {
> +			res = error(_("cannot parse parents of commit: %s"),
> +					oid_to_hex(&this->object.oid));
> +			goto out;
> +		}
> +
> +		if (check_stash_topology(r, stash)) {
> +			res = error(_("%s does not look like a stash commit"),
> +					oid_to_hex(&stash->object.oid));
> +			goto out;
> +		}
> +
> +		repo_unuse_commit_buffer(r, this, buffer);
> +		buffer = NULL;
> +		oid_array_append(&items, &stash->object.oid);
> +		this = this->parents->item;
> +	}
> +
> +	/*
> +	 * Now, walk each entry, adding it to the stash as a normal stash
> +	 * commit.
> +	 */
> +	for (i = (ssize_t)items.nr - 1; i >= 0; i--) {
> +		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));
> +		repo_unuse_commit_buffer(r, this, buffer);
> +		buffer = NULL;
> +
> +		if (do_store_stash(oid, msg, 1)) {
> +			res = error(_("cannot save the stash for %s"), oid_to_hex(oid));
> +			goto out;
> +		}
> +		FREE_AND_NULL(msg);
> +	}
> +out:
> +	if (this && buffer)
> +		repo_unuse_commit_buffer(r, this, buffer);
> +	oid_array_clear(&items);
> +	free(msg);
> +
> +	return res;
> +}

OK.  Again, I suspect that commit_list may be a more natural thing
to use to accumulate the stash entries, but that is not a huge deal.