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

["brian m. carlson" <sandals@crustytoothpaste.net>]

[-- Attachment #1: Type: text/plain, Size: 3169 bytes --]

On 2025-05-09 at 19:54:12, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > +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;
> 
> This is to stop at the sentinel commit at the end.  Don't we want to
> make sure that it actually has the expected shape of the sentinel?
> 
> IOW, how robust do we try to be against being fed a random mergy
> commit history (e.g., our 'master') and mistakenly adding nonsense
> stash entries as refs/stash@{<n>}?

I'll try to make this more robust.  The advantage of the current
approach is that we don't have to parse the buffer, but once we need to
do that, we might as well do the entire thing.

> > +	/*
> > +	 * Now, walk each entry, adding it to the stash as a normal stash
> > +	 * commit.
> > +	 */
> > +	for (i = items.nr - 1; i >= 0; i--) {
> > +		unsigned long bufsize;
> > +		const char *p;
> > +		const struct object_id *oid = items.oid + i;
> > +
> > +		this = lookup_commit_reference(r, oid);
> > +		buffer = repo_get_commit_buffer(r, this, &bufsize);
> > +		if (!buffer) {
> > +			res = error(_("cannot read commit buffer for %s"), oid_to_hex(oid));
> > +			goto out;
> > +		}
> > +
> > +		p = strstr(buffer, "\n\n");
> > +		if (!p) {
> > +			res = error(_("cannot parse commit %s"), oid_to_hex(oid));
> > +			goto out;
> > +		}
> > +
> > +		p += 2;
> > +		msg = xmemdupz(p, bufsize - (p - buffer));
> 
> Here, we could check "git stash: " string to make sure that it is as
> expected in an exported stash we previously made, and abort, just
> like the above "cannot parse" case.

This part doesn't have the prefix.  That's the part above, but I'll add
the check there.  This part instead is the actual stash commit and we're
extracting the message to put into the reflog.

> Should we be making sure that the object named by "oid" does look
> like a stash, like what is done in builtin/stash.c:get_stash_info(),
> or is assert_stash_like() called from do_store_stash() sufficient?

I think `assert_stash_like` should be fine.  I don't have any better
checking to do than that, since we don't have any more checks to do.
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 325 bytes --]