From: Phillip Wood <phillip.wood123@gmail.com>
Date: Thu, 05 Jun 2025 09:25:42 GMT
Subject: Re: [PATCH v7 3/4] builtin/stash: provide a way to export stashes to a ref
Message-ID: <127d9d03-e94e-4928-9c6d-07a5396ca325@gmail.com>
In-Reply-To: <20250601223225.464076-4-sandals@crustytoothpaste.net>

Hi brian

There are a couple of points outstanding from my last review. The first 
point below about the option handling definitely needs fixing, the other 
is more of a matter of personal taste.

On 01/06/2025 23:32, brian m. carlson wrote:
> 
> diff --git a/Documentation/git-stash.adoc b/Documentation/git-stash.adoc
> index 1a5177f498..e8efd43ba4 100644
> --- a/Documentation/git-stash.adoc
> +++ b/Documentation/git-stash.adoc
> @@ -23,6 +23,7 @@ SYNOPSIS
>   'git stash' clear
>   'git stash' create [<message>]
>   'git stash' store [(-m | --message) <message>] [-q | --quiet] <commit>
> +'git stash' export (--print | --to-ref <ref>) [<stash>...]
>   
>   DESCRIPTION
>   -----------
> @@ -154,6 +155,12 @@ store::
>   	reflog.  This is intended to be useful for scripts.  It is
>   	probably not the command you want to use; see "push" above.
>   
> +export ( --print | --to-ref <ref> ) [<stash>...]::

--print and --to-ref are documented as mutually exclusive but that is 
not implemented in the code.

     git stash export --print --to-ref refs/exported-stash

creates refs/exported-stash but does not print the object id. We should 
either ensure they are actually mutually exclusive or just allow both. I 
don't have a strong preference either way - I'm not sure it is 
particularly useful for the user to be able to specify both but it 
doesn't do any harm so long as we honor both options.

>   
> +static int write_commit_with_parents(struct repository *r,
> +				     struct object_id *out,
> +				     const struct object_id *oid,
> +				     struct commit_list *parents)
> +{
> [...]
> +out:
> +	strbuf_release(&msg);
> +	repo_unuse_commit_buffer(r, this, buffer);
> +	free_commit_list(parents);

I think taking ownership of function parameters like this makes the code 
harder to reason about because C has no way for the function prototype 
to signal to the reader that the ownership is transferred. It would be 
easier to see that the list of parent commits was not leaked if it was 
freed by the caller.

Best Wishes

Phillip