Re: [PATCH v7 3/4] builtin/stash: provide a way to export stashes to a ref
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Jun 5, 2025, 09:25 UTC
- 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:
Show 18 quoted lines
> > 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.
Show 11 quoted lines
>
> +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