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

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

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

> +static int check_stash_topology(struct repository *r, struct commit *stash)
> +{
> +	struct commit *p1, *p2, *p3 = NULL;
> +
> +	/* stash must have two or three parents */
> +	if (!stash->parents || !stash->parents->next ||
> +			(stash->parents->next->next && stash->parents->next->next->next))
> +		return -1;
> +	p1 = stash->parents->item;
> +	p2 = stash->parents->next->item;
> +	if (stash->parents->next->next)
> +		p3 = stash->parents->next->next->item;
> +	if (repo_parse_commit(r, p1) || repo_parse_commit(r, p2) ||
> +			(p3 && repo_parse_commit(r, p3)))
> +		return -1;
> +	/* p2 must have a single parent, p3 must have no parents */
> +	if (!p2->parents || p2->parents->next || (p3 && p3->parents))
> +		return -1;
> +	if (repo_parse_commit(r, p2->parents->item))
> +		return -1;
> +	/* p2^1 must equal p1 */
> +	if (!oideq(&p1->object.oid, &p2->parents->item->object.oid))
> +		return -1;
> +
> +	return 0;
> +}

This is much more elaborate than the existing "does this commit look
like a stash entry" test done elsewhere, isn't it?  Very nicely done.

> +	this = lookup_commit_reference(r, oid);
> +	buffer = repo_get_commit_buffer(r, this, &bufsize);
> +	orig_author = find_commit_header(buffer, "author", &author_len);
> +	orig_committer = find_commit_header(buffer, "committer", &committer_len);
> +
> +	if (!orig_author || !orig_committer) {
> +		ret = error(_("cannot parse commit %s"), oid_to_hex(oid));
> +		goto out;
> +	}
> +
> +	if (split_ident_line(&id, orig_author, author_len) < 0 ||
> +	    split_ident_line(&id, orig_committer, committer_len) < 0) {
> +		ret = error(_("invalid author or committer for %s"), oid_to_hex(oid));
> +		goto out;
> +	}
> +
> +	p = strstr(buffer, "\n\n");
> +	strbuf_addstr(&msg, "git stash: ");
> +
> +	if (p)
> +		strbuf_add(&msg, p + 2, bufsize - (p + 2 - buffer));
> +	strbuf_complete_line(&msg);
> +
> +	author = xmemdupz(orig_author, author_len);
> +	committer = xmemdupz(orig_committer, committer_len);
> +
> +	if (commit_tree_extended(msg.buf, msg.len,
> +				 r->hash_algo->empty_tree, parents,
> +				 out, author, committer,
> +				 NULL, NULL)) {
> +		ret = error(_("could not write commit"));
> +		goto out;
> +	}
> +out:
> +	strbuf_release(&msg);
> +	repo_unuse_commit_buffer(r, this, buffer);
> +	free_commit_list(parents);
> +	free(author);
> +	free(committer);
> +	return ret;
> +}

Nice.

> +	if (argc) {
> +		/*
> +		 * Find each specified stash, and load data into the array.
> +		 */
> +		for (i = 0; i < argc; i++) {
> +			struct object_id oid;
> +			struct commit *stash;
> +
> +			if (parse_stash_revision(&revision, argv[i], 1) ||
> +			    get_oid_with_context(r, revision.buf,
> +						 GET_OID_QUIETLY | GET_OID_GENTLY,
> +						 &oid, &unused)) {
> +				res = error(_("unable to find stash entry %s"), argv[i]);
> +				goto out;
> +			}
> +
> +			stash = lookup_commit_reference(r, &oid);
> +			if (!stash || check_stash_topology(r, stash)) {
> +				res = error(_("%s does not look like a stash commit"),
> +						revision.buf);
> +				goto out;
> +			}
> +			oid_array_append(&items, &oid);
> +		}
> +	} else {
> +		/*
> +		 * Walk the reflog, finding each stash entry, and load data into the
> +		 * array.
> +		 */
> +		struct complete_reflogs *crl = read_complete_reflog("refs/stash");
> +		for (i = 0; i < crl->nr; i++) {
> +			/*
> +			 * Iterate from the latest item in the reflog to the
> +			 * earliest.  crl->items starts from the earliest first.
> +			 */
> +			struct reflog_info *item = crl->items + (crl->nr - i - 1);
> +			struct commit *stash;
> +
> +			stash = lookup_commit_reference(r, &item->noid);
> +			if (!stash || check_stash_topology(r, stash)) {
> +				res = error(_("%s does not look like a stash commit"),
> +						revision.buf);
> +				goto out;
> +			}
> +			oid_array_append(&items, &item->noid);
> +		}
> +		free_complete_reflog(crl);
> +	}

Hmph.  As we work with the commit objects in the above part already,
would it still make sense to use an oid array?  That would force us
to look up the same set of commits using the object names again in
the later loop.  I would have thought we might use commit_list or
something to accumulate commits, and then consume in the loop below.

Not a huge deal, though.  But I somehow find it more natural.  

In addition, we would not have to worry about the ssize_t cast and
other complications if we used commit_list to hold these incoming
stash entries.