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

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

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

> @@ -104,6 +109,7 @@ static struct strbuf stash_index_path = STRBUF_INIT;
>   * b_commit is set to the base commit
>   * i_commit is set to the commit containing the index tree
>   * u_commit is set to the commit containing the untracked files tree
> + * c_commit is set to the first parent (chain commit) when importing and is otherwise unset
>   * w_tree is set to the working tree
>   * b_tree is set to the base tree
>   * i_tree is set to the index tree
> @@ -114,6 +120,7 @@ struct stash_info {
>  	struct object_id b_commit;
>  	struct object_id i_commit;
>  	struct object_id u_commit;
> +	struct object_id c_commit;
>  	struct object_id w_tree;
>  	struct object_id b_tree;
>  	struct object_id i_tree;

With the redesign that an exported chain is a series of two-parent
merges, where the first parent is used to string them together in a
single strand of pearls and the second parent is the stash entry,
the above change becomes totally unnecessary, right?  The import
side will be doing a first-parent walk of the export, pushing the
second parent into reflog of refs/stash---we may want sanity check
these second parents with assert_stash_like(), but there is no need
to re-synthesize the stash entries anymore, which would simplify the
implementation quite a bit, right?
		
Namely:

> +static int do_import_stash(const char *rev)
> +{
> +	struct object_id oid;
> +	size_t nitems = 0, nalloc = 0;
> +	struct stash_info *items = NULL;
> +	int res = 0;
> +
> +	if (get_oid(rev, &oid))
> +		return error(_("not a valid revision: %s"), rev);
> +
> +	/*
> +	 * Walk the commit history, finding each stash entry, and load data into
> +	 * the array.
> +	 */
> +	for (size_t i = 0;; i++, nitems++) {
> +		int ret;
> +
> +		if (nalloc <= i) {
> +			size_t new = nalloc * 3 / 2 + 5;
> +			items = xrealloc(items, new * sizeof(*items));
> +			nalloc = new;
> +		}
> +		memset(&items[i], 0, sizeof(*items));
> +		/* We want this to be quiet because it might not exist. */
> +		ret = get_stash_info_for_import(&items[i], &oid);

The new helper function is not necessary; we can use vanilla
get_stash_info() on the second parent to get the same information,
and we do not really need to keep it in core.  We can sanity check
the shape of the imported stash entry right away and discard
everything except for the commit object name.

> +	/*
> +	 * Now, walk each entry, adding it to the stash as a normal stash
> +	 * commit.
> +	 */
> +	for (ssize_t i = nitems - 1; i >= 0; i--) {
> +		struct commit_list *parents = NULL;
> +		struct commit_list **next = &parents;
> +		struct object_id out;
> +		char *msg;
> +
> +		next = commit_list_append(lookup_commit_reference(the_repository, &items[i].b_commit), next);
> +		next = commit_list_append(lookup_commit_reference(the_repository, &items[i].i_commit), next);
> +		if (items[i].has_u)
> +			next = commit_list_append(lookup_commit_reference(the_repository,
> +								   &items[i].u_commit),
> +						  next);
> +
> +		msg = write_commit_with_parents(&out, &items[i], parents);

And this part becomes completely unnecessary if we reuse what the
origin repository already had, which directly can be ...

> +		if (!msg) {
> +			res = -1;
> +			goto out;
> +		}
> +		if (do_store_stash(&out, msg, 1)) {

... fed to this call.