Re: [PATCH v3 4/4] stash: implement builtin stash

[Junio C Hamano <gitster@pobox.com>]

Joel Teichroeb <joel@teichroeb.net> writes:

> +int untracked_files(struct strbuf *out, int include_untracked,
> +		const char **argv)

Does this need to be public?  

For a caller that wants to learn if there is _any_ untracked file,
having a strbuf that holds all output is overkill.  For a caller
that wants to learn what are the untracked paths, having a strbuf
that it needs to parse is not all that helpful.  Only for a caller
that does an unusual thing, namely, just grab the output and throw
it at another command as input, without checking what the output was
itself at all, would benefit.

So the interface to this function doesn't look like a very good one
if this wants to be a public helper.  Perhaps mark it as "static"?

> +{
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	cp.git_cmd = 1;
> +	argv_array_push(&cp.args, "ls-files");
> +	argv_array_push(&cp.args, "-o");
> +	argv_array_push(&cp.args, "-z");
> +	if (include_untracked != 2)
> +		argv_array_push(&cp.args, "--exclude-standard");

This magic number "2" will be hard to grok by future readers.

> +	argv_array_push(&cp.args, "--");
> +	if (argv)
> +		argv_array_pushv(&cp.args, argv);
> +	return pipe_command(&cp, NULL, 0, out, 0, NULL, 0);
> +}
> +
> +static int check_no_changes(const char *prefix, int include_untracked,
> +		const char **argv)
> +{
> +	struct argv_array args1;
> +	struct argv_array args2;
> +	struct strbuf out = STRBUF_INIT;
> +
> +	argv_array_init(&args1);
> +	argv_array_push(&args1, "diff-index");
> +	argv_array_push(&args1, "--quiet");
> +	argv_array_push(&args1, "--cached");
> +	argv_array_push(&args1, "HEAD");
> +	argv_array_push(&args1, "--ignore-submodules");
> +	argv_array_push(&args1, "--");
> +	if (argv)
> +		argv_array_pushv(&args1, argv);
> +	argv_array_init(&args2);
> +	argv_array_push(&args2, "diff-files");
> +	argv_array_push(&args2, "--quiet");
> +	argv_array_push(&args2, "--ignore-submodules");
> +	argv_array_push(&args2, "--");
> +	if (argv)
> +		argv_array_pushv(&args2, argv);

> +	if (include_untracked)
> +		untracked_files(&out, include_untracked, argv);
> +	return cmd_diff_index(args1.argc, args1.argv, prefix) == 0 &&
> +			cmd_diff_files(args2.argc, args2.argv, prefix) == 0 &&
> +			(!include_untracked || out.len == 0);
> +}

Possible leak of out.buf here.

> +
> +static int get_stash_info(struct stash_info *info, const char *commit)
> +{

Judging from the callers of get_stash_info(), nobody passes a
"commit" as parameter; as far as they are concerned, they are
dealing with the name of one item in the stash (stash-id?).  They do
not care the fact that the item happens to be implemented as a
commit object.

Perhaps rename "commit" (parameter) to "stash_id" or something.

> +	struct strbuf w_commit_rev = STRBUF_INIT;
> +	struct strbuf b_commit_rev = STRBUF_INIT;
> +	struct strbuf i_commit_rev = STRBUF_INIT;
> +	struct strbuf u_commit_rev = STRBUF_INIT;
> +	struct strbuf w_tree_rev = STRBUF_INIT;
> +	struct strbuf b_tree_rev = STRBUF_INIT;
> +	struct strbuf i_tree_rev = STRBUF_INIT;
> +	struct strbuf u_tree_rev = STRBUF_INIT;
> +	struct strbuf commit_buf = STRBUF_INIT;
> +	struct strbuf symbolic = STRBUF_INIT;
> +	struct strbuf out = STRBUF_INIT;
> +	struct object_context unused;
> +	char *str;
> +	int ret;
> +	const char *REV = commit;

Variables and field names that are all caps become misleading.
Avoid them.

> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	info->is_stash_ref = 0;
> +
> +	if (commit == NULL) {
> +		strbuf_addf(&commit_buf, "%s@{0}", ref_stash);
> +		REV = commit_buf.buf;
> +	} else if (strlen(commit) < 3) {
> +		strbuf_addf(&commit_buf, "%s@{%s}", ref_stash, commit);
> +		REV = commit_buf.buf;
> +	}
> +	info->REV = REV;
> +
> +	strbuf_addf(&w_commit_rev, "%s", REV);
> +	strbuf_addf(&b_commit_rev, "%s^1", REV);
> +	strbuf_addf(&i_commit_rev, "%s^2", REV);
> +	strbuf_addf(&u_commit_rev, "%s^3", REV);
> +	strbuf_addf(&w_tree_rev, "%s:", REV);
> +	strbuf_addf(&b_tree_rev, "%s^1:", REV);
> +	strbuf_addf(&i_tree_rev, "%s^2:", REV);
> +	strbuf_addf(&u_tree_rev, "%s^3:", REV);
> +
> +
> +	ret = (
> +		get_sha1_with_context(w_commit_rev.buf, 0, info->w_commit.hash, &unused) == 0 &&
> +		get_sha1_with_context(b_commit_rev.buf, 0, info->b_commit.hash, &unused) == 0 &&
> +		get_sha1_with_context(i_commit_rev.buf, 0, info->i_commit.hash, &unused) == 0 &&
> +		get_sha1_with_context(w_tree_rev.buf, 0, info->w_tree.hash, &unused) == 0 &&
> +		get_sha1_with_context(b_tree_rev.buf, 0, info->b_tree.hash, &unused) == 0 &&
> +		get_sha1_with_context(i_tree_rev.buf, 0, info->i_tree.hash, &unused) == 0);

Hmph.  What's the reason to use get_sha1_with_context() if you
declare the context is unused?  Wouldn't plain-vanilla get_sha1()
work better?

Having to take both commit and tree of these sounds somewhat
cumbersome.  Do we have to know all of them and do we use all of
them (not a complaint, just asking because I find it somewhat
surprising)?

> +
> +	if (!ret) {
> +		fprintf_ln(stderr, _("%s is not a valid reference"), REV);
> +		return 1;
> +	}
> +
> +
> +	info->has_u = get_sha1_with_context(u_commit_rev.buf, 0, info->u_commit.hash, &unused) == 0 &&
> +		get_sha1_with_context(u_tree_rev.buf, 0, info->u_tree.hash, &unused) == 0;

Ditto.

> +static int create_stash(int argc, const char **argv, const char *prefix)
> +{
> +	int include_untracked = 0;
> +	const char *message = NULL;
> +	struct stash_info info;
> +	struct option options[] = {
> +		OPT_BOOL('u', "include-untracked", &include_untracked,
> +			 N_("stash untracked filed")),
> +		OPT_STRING('m', "message", &message, N_("message"),
> +			 N_("stash commit message")),
> +		OPT_END()
> +	};

OPT_BOOL(), unlike its now deprecated and removed predecessor OPT_BOOLEAN(),
does not count up, so the value of include_untracked is lmited to
either 0 or 1.  So this is not the source of mysterious magic number
"2" I noticed above.

I'll stop here for now.  

Thanks for working on this.