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
