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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 16, 2017, 16:15 UTC
Message-ID: <xmqqbmpnyklk.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170608005535.13080-6-joel@teichroeb.net>

Joel Teichroeb <joel@teichroeb.net> writes:

Show 8 quoted lines

> 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".

Show 13 quoted lines

> +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.

Show 24 quoted lines

> +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.

Show 25 quoted lines

> +	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.

Show 17 quoted lines

> +		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.

Show 8 quoted lines

> +	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.

Show 14 quoted lines

> +	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.