Hi Paul,

On Fri, 31 Aug 2018, Paul-Sebastian Ungureanu wrote:

Not worth sending another iteration, but if you end up doing so, I found
an alternative paradigm more useful: instead of having a Boolean to
indicate whether you need to free, have a `char *to_free` that is
initialized to `NULL`, then unconditionally release that:

	char *to_free = NULL;

	if (!stash_msg)
		stash_msg = to_free = xstrdup("Created via \"git stash store\".");

	[...]

	free(to_free);
	return ret;

This works because `free(NULL)` is defined as a no-op.

+	struct object_id obj;
+
+	if (!stash_msg) {
+		need_to_free = 1;
+		stash_msg  = xstrdup("Created via \"git stash store\".");
+	}
+
+	ret = get_oid(w_commit, &obj);

Is `get_oid()` non-quiet? If so, we might need to shut it up when the
`quiet` variable is non-zero. If it is quiet, we should probably mention
something here when `quiet` is zero and `ret` indicates an error with
`get_oid()`.

+	if (!ret) {
+		ret = update_ref(stash_msg, ref_stash, &obj, NULL,
+				 REF_FORCE_CREATE_REFLOG,
+				 quiet ? UPDATE_REFS_QUIET_ON_ERR :
+				 UPDATE_REFS_MSG_ON_ERR);
+	}
+	if (ret && !quiet)
+		fprintf_ln(stderr, _("Cannot update %s with %s"),
+			   ref_stash, w_commit);
+	if (need_to_free)
+		free((char *) stash_msg);

Okay, so all we need `stash_msg` for is the `update_ref()` call? And the
fall-back message is constant. So how about

	if (!stash_msg)
		stash_msg = "Created via \"git stash store\".";

? No need to allocate/release memory at all...

+	return ret;
+}
+
+static int store_stash(int argc, const char **argv, const char *prefix)
+{
+	const char *stash_msg = NULL;
+	struct option options[] = {
+		OPT__QUIET(&quiet, N_("be quiet, only report errors")),
+		OPT_STRING('m', "message", &stash_msg, "message", N_("stash message")),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, prefix, options,
+			     git_stash_helper_store_usage,
+			     PARSE_OPT_KEEP_UNKNOWN);
+
+	if (argc != 1) {
+		fprintf_ln(stderr, _("\"git stash store\" requires one <commit> argument"));
+		return -1;
+	}
+
+	return do_store_stash(argv[0], stash_msg, quiet);
+}

I guess `store_stash()` and `do_store_stash()` are separate functions to
discern between the higher-level function that parses the command-line,
and the lower-level function that is already libified?

If so:

1. I like it. Your code is already so much better prepared to serve as a proper
   internal API than even some Git old-timers'.

2. It might make sense to move the `get_oid()` call to `store_stash()`, as
   it is also parsing: it is parsing the plain-text representation of the
   revision into the internal representation (object ID).

Neither of these suggestions are blockers, though.

Thanks,
Dscho