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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi brian

On 09/05/2025 00:44, brian m. carlson wrote:
> 
> @@ -1962,6 +1971,99 @@ static int write_commit_with_parents(struct repository *r,
>   	return ret;
>   }
>   
> +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;
> +		oid_array_append(&items, &oid);
> +	}

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(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index 02cf344ed9..7d3a8c03a0 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -2011,25 +2011,44 @@ static int do_import_stash(struct repository *r, const char *rev)
  	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 (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);
+	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 (get_oidf(&chain, "%s^1", revision) ||
-		    get_oidf(&oid, "%s^2", revision))
+		if (!this->parents)
  			break;
-		oid_array_append(&items, &oid);
+		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;
  	}
  
  	/*
-- 
2.49.0.300.g813f75aecee