From: Phillip Wood <phillip.wood123@gmail.com>
Date: Thu, 29 May 2025 16:01:03 GMT
Subject: Re: [PATCH v6 4/5] builtin/stash: provide a way to export stashes to a ref
Message-ID: <9429a84c-d5e0-45df-8fd2-6dc92cf39d9e@gmail.com>
In-Reply-To: <20250522185524.18398-6-sandals@crustytoothpaste.net>

Hi brian

This is looking pretty nice, I don't have much to add to Junio's comments.

On 22/05/2025 19:55, brian m. carlson wrote:
> [...]
> +out:
> +	strbuf_release(&msg);
> +	repo_unuse_commit_buffer(r, this, buffer);
> +	free_commit_list(parents);

I found this confusing when I was reading the caller as I couldn't see 
where the list of parent commits was being free()d. I think it would be 
easier to follow if this function did not take ownership of the list.

> +	/*
> +	 * 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 = (ssize_t)items.nr - 1; i >= 0; i--) {

This cast is pretty horrible. ssize_t is not guaranteed to be the same 
width as size_t (see [1] for a bug fix on NonStop caused by this). I 
think we'd normally write this as

	for (size_t i = items.nr; i > 0; i--) {
		const struct object_id *oid = &items[i - 1];

[1] c14e5a1a501 (transport-helper: use xread instead of read, 2019-01-03)

> +		struct commit_list *parents = NULL;
> +		struct commit_list **next = &parents;
> +		struct object_id out;
> +		const struct object_id *oid = items.oid + i;
> [...]
> +static int export_stash(int argc,
> +			const char **argv,
> +			const char *prefix,
> +			struct repository *repo)
> +{
> +	const char *ref = NULL;
> +	enum export_action action = ACTION_NONE;
> +	struct option options[] = {
> +		OPT_CMDMODE(0, "print", &action,
> +			    N_("print the object ID instead of writing it to a ref"),
> +			    ACTION_PRINT),
> +		OPT_STRING(0, "to-ref", &ref, "ref",
> +			    N_("save the data to the given ref")),
> +		OPT_END()
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, options,
> +			     git_stash_export_usage,
> +			     PARSE_OPT_KEEP_DASHDASH);
> +
> +	if (ref && action == ACTION_NONE)
> +		action = ACTION_TO_REF;
> +
> +	if (action == ACTION_NONE)
> +		return error(_("exactly one of --print and --to-ref is required"));
I don't think this protects against 'git stash export --print --to-ref'. 
It is a bit odd to have a single option marked with OPT_CMDMODE() it 
might be worth changing that to OPT_SET_INT().

Best Wishes

Phillip