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

Hi brian

On 09/05/2025 00:44, brian m. carlson wrote:

This loop could use some improvement - it does not use the loop variable, it accepts any commit with an empty tree as a valid exported stash, it is pretty lax about checking that the commits in the chain have either zero or two parents, it does not check if the second parent looks like a stash and it is forever converting between strings and object_ids. I think it would be better to loop over commits rather than object ids then you can walk the history using the pointers to the parent commits. Something like

	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 (;;) {
		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;
		}
		if (!this->parents)
			break;
		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;
		}
		/* TODO:
		 *  - store commits, not objects
		 */
		oid_array_append(&items, &this->parents->next->item->object.oid);
		this = this->parents->item;
	}

I've appended a fixup commit below that applies on top of your patch. The fixups for this patch and the previous one can be fetched with

   git fetch https://github.com/phillipwood/git.git bc/stash-import-export-fixups

if you want to use them.

Best Wishes

Phillip

---- >8 ----
From: Phillip Wood <phillip.wood@dunelm.org.uk>
Subject: [PATCH] fixup! builtin/stash: provide a way to import stashes from a
  ref

Strengthen the checks on imported commits to ensure that the chain of imported stashes consists of commits with exactly two parents where the first parent is either the root commit or another imported stash commit and the second parent looks like a stash commit.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
  builtin/stash.c | 43 +++++++++++++++++++++++++++++++------------
  1 file changed, 31 insertions(+), 12 deletions(-)