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

From: Junio C Hamano <gitster@pobox.com>
Date: May 9, 2025, 19:31 UTC
Message-ID: <xmqqecwx1szk.fsf@gitster.g>
In-Reply-To: <20250508234458.3665894-4-sandals@crustytoothpaste.net>

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

Show 21 quoted lines

> +static int do_export_stash(struct repository *r,
> +			   const char *ref,
> +			   int argc,
> +			   const char **argv)
> +{
> +...
> +	if (argc) {
> +		/*
> +		 * Find each specified stash, and load data into the array.
> +		 */
> +		for (i = 0; i < argc; i++) {
> +			struct object_id oid;
> +			if (parse_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;
> +			}
> +			oid_array_append(&items, &oid);
> +		}

Grabbing individual reflog entry given on the command line looks trivial, but ...

Show 17 quoted lines

> +	} else {
> +		/*
> +		 * Walk the reflog, finding each stash entry, and load data into the
> +		 * array.
> +		 */
> +		for (i = 0;; i++) {
> +			char buf[32];
> +			struct object_id oid;
> +
> +			snprintf(buf, sizeof(buf), "%d", i);
> +			if (parse_revision(&revision, buf, 1) ||
> +			    get_oid_with_context(r, revision.buf,
> +						 GET_OID_QUIETLY | GET_OID_GENTLY,
> +						 &oid, &unused))
> +				break;
> +			oid_array_append(&items, &oid);
> +		}

... have you considered reusing reflog-walk.c:read_complete_reflog() as a helper function?

Doing so of would be more efficient than going from int to string back to int to call read_ref_at() and iterate over the same reflog entries with refs_for_each_reflog_ent().

Show 15 quoted lines

> +	}
> +
> +	/*
> +	 * Now, create a set of commits identical to the regular stash commits,
> +	 * but where their first parents form a chain to our original empty
> +	 * base commit.
> +	 */
> +	for (i = items.nr - 1; i >= 0; i--) {
> +		struct commit_list *parents = NULL;
> +		struct commit_list **next = &parents;
> +		struct object_id out;
> +		const struct object_id *oid = items.oid + i;
> +
> +		next = commit_list_append(prev, next);
> +		next = commit_list_append(lookup_commit_reference(r, oid), next);

The individual-reflog-entry mode above was fairly strict in that a list of reflog entries with even one unreadable commit caused the whole command to fail, but reflog-walk mode assumed that a failure to read an entry must always be due to reflog entries running out due to the index incremented to a large enough number. I suspect get_oid_with_context() can give you oid obtained out of a reflog entry without actually parsing the object or checking if it exists.

Should we be a bit more defensinve here in lookup_commit_reference() call, which would silently throw a NULL back at us if the commit cannot be found without complaining?