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
