Re: [PATCH v4 5/5] stash: implement builtin stash

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

Joel Teichroeb <joel@teichroeb.net> writes:

> diff --git a/builtin/stash.c b/builtin/stash.c
> new file mode 100644
> index 0000000000..a9680f2909
> --- /dev/null
> +++ b/builtin/stash.c
> ...
> +static const char *ref_stash = "refs/stash";
> +static int quiet = 0;

Let BSS take care of zero-initialization, i.e. drop " = 0".

> +static int untracked_files(struct strbuf *out, int include_untracked,
> +		int include_ignored, const char **argv)
> +{
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	cp.git_cmd = 1;
> +	argv_array_pushl(&cp.args, "ls-files", "-o", "-z", NULL);
> +	if (include_untracked && !include_ignored)
> +		argv_array_push(&cp.args, "--exclude-standard");
> +	argv_array_push(&cp.args, "--");
> +	if (argv)
> +		argv_array_pushv(&cp.args, argv);
> +	return pipe_command(&cp, NULL, 0, out, 0, NULL, 0);
> +}

Seeing that include_untracked and include_ignored always come in a
pair throughout the program, I wondered if it may be better to use
a single "unsigned include" with two bits

    #define INCLUDE_UNTRACKED 01
    #define INCLUDE_IGNORED 02

to pass around.  As long as we envision that we will not gain other
kind of "do we include X?" in the future, what your patch does is
fine, I would say.

> +static int check_no_changes(const char *prefix, int include_untracked,
> +		int include_ignored, const char **argv)
> +{
> +	struct argv_array args1 = ARGV_ARRAY_INIT;
> +	struct argv_array args2 = ARGV_ARRAY_INIT;
> +	struct strbuf out = STRBUF_INIT;
> +	int ret;
> +
> +	argv_array_pushl(&args1, "diff-index", "--quiet", "--cached", "HEAD",
> +		"--ignore-submodules", "--", NULL);
> +	if (argv)
> +		argv_array_pushv(&args1, argv);
> +
> +	argv_array_pushl(&args2, "diff-files", "--quiet", "--ignore-submodules",
> +		"--", NULL);
> +	if (argv)
> +		argv_array_pushv(&args2, argv);
> +
> +	if (include_untracked)
> +		untracked_files(&out, include_untracked, include_ignored, argv);
> +
> +	ret = cmd_diff_index(args1.argc, args1.argv, prefix) == 0 &&
> +			cmd_diff_files(args2.argc, args2.argv, prefix) == 0 &&
> +			(!include_untracked || out.len == 0);

When diff_index() finds there are modified paths, you do not have to
call diff_files() or untracked_files() at all (and you do not even
have to set-up args2).  Doesn't the above leak args.argv[] when &&
short circuits?

    This is a tangent, but it is somewhat unusual to call cmd_foo()
    as a subroutine.  I think cmd_diff_*() are written reasonably
    well to allow them to be called in this way safely, and there
    are a few existing commands that already do so, so it may be OK.

> +	strbuf_release(&out);
> +	return ret;
> +}
> +
> +static int get_stash_info(struct stash_info *info, const char *commit)
> +{
> +	struct strbuf w_commit_rev = STRBUF_INIT;
> +	struct strbuf b_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;
> +	int ret;
> +	const char *revision = commit;
> +	char *end_of_rev;
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	info->is_stash_ref = 0;
> +
> +	if (commit == NULL) {
> +		strbuf_addf(&commit_buf, "%s@{0}", ref_stash);
> +		revision = commit_buf.buf;
> +	} else if (strlen(commit) < 3) {

This is a bit sloppy (the original is even sloppier but it is in
shell, so it is more excusable ;-).  This code thinks that anything
with @{<num>} must be longer than 3 because it has to have @{},
and that is where the strlen() comes from, I think, but the magic
number 3 appears without explanation here.

What the code actually needs to do is to see if the stash entry
specification came in "commit" (which by the way is a bit misnamed
parameter) is a bare number and use refs/stash@{<that number>} only
in that case, I think.  strspn() might be useful.

> +		strbuf_addf(&commit_buf, "%s@{%s}", ref_stash, commit);
> +		revision = commit_buf.buf;
> +	}
> +	info->revision = revision;
> +
> +	strbuf_addf(&w_commit_rev, "%s", revision);
> +	strbuf_addf(&b_commit_rev, "%s^1", revision);
> +	strbuf_addf(&w_tree_rev, "%s:", revision);
> +	strbuf_addf(&b_tree_rev, "%s^1:", revision);
> +	strbuf_addf(&i_tree_rev, "%s^2:", revision);
> +	strbuf_addf(&u_tree_rev, "%s^3:", revision);
> +
> +	ret = (get_sha1(w_commit_rev.buf, info->w_commit.hash) == 0 &&
> +		get_sha1(b_commit_rev.buf, info->b_commit.hash) == 0 &&
> +		get_sha1(w_tree_rev.buf, info->w_tree.hash) == 0 &&
> +		get_sha1(b_tree_rev.buf, info->b_tree.hash) == 0 &&
> +		get_sha1(i_tree_rev.buf, info->i_tree.hash) == 0);

It's more conventional to check for errors with !get_sha1(params),
not a long-hand comparision with 0.

> +	strbuf_release(&w_commit_rev);
> +	strbuf_release(&b_commit_rev);
> +	strbuf_release(&w_tree_rev);
> +	strbuf_release(&b_tree_rev);
> +	strbuf_release(&i_tree_rev);
> +
> +	if (!ret)
> +		return error(_("%s is not a valid reference"), revision);

We are leaking u_tree_rev.buf upon early return.

> +	info->has_u = get_sha1(u_tree_rev.buf, info->u_tree.hash) == 0;
> +
> +	strbuf_release(&u_tree_rev);
> +
> +	end_of_rev = strchrnul(revision, '@');
> +	strbuf_add(&symbolic, revision, end_of_rev - revision);
> +	cp.git_cmd = 1;
> +	argv_array_pushl(&cp.args, "rev-parse", "--symbolic-full-name", NULL);
> +	argv_array_pushf(&cp.args, "%s", symbolic.buf);
> +	strbuf_release(&symbolic);
> +	pipe_command(&cp, NULL, 0, &out, 0, NULL, 0);
> +
> +	if (out.len-1 == strlen(ref_stash))
> +		info->is_stash_ref = strncmp(out.buf, ref_stash, out.len-1) == 0;

Favor !strncmp(params) over comparision with 0.

Style: Have SP around both sides of binary operator "-".

> +	strbuf_release(&out);
> +	return !ret;

Hmph, where did we last assign ret in this code?  Didn't we check
that value and returned error already, which means we know what !ret
is when the control reaches here?

> +}

I have to move to another building, so I'll stop here for now, but
will continue later.

Thanks.