Re: [PATCH 1/4] stash: convert apply to builtin

From: Thomas Gummerer <t.gummerer@gmail.com>
Date: Mar 25, 2018, 16:43 UTC
Message-ID: <20180325164300.GA10909@hank>
In-Reply-To: <20180324173707.17699-2-joel@teichroeb.net>

On 03/24, Joel Teichroeb wrote:

> ---

Missing sign-off?  I saw it's missing in the other patches as well.

Show 67 quoted lines

> [...]
> +static int do_apply_stash(const char *prefix, struct stash_info *info, int index)
> +{
> +	struct merge_options o;
> +	struct object_id c_tree;
> +	struct object_id index_tree;
> +	const struct object_id *bases[1];
> +	int bases_count = 1;
> +	struct commit *result;
> +	int ret;
> +	int has_index = index;
> +
> +	read_cache_preload(NULL);
> +	if (refresh_cache(REFRESH_QUIET))
> +		return -1;
> +
> +	if (write_cache_as_tree(c_tree.hash, 0, NULL) || reset_tree(c_tree, 0, 0))
> +		return error(_("Cannot apply a stash in the middle of a merge"));
> +
> +	if (index) {
> +		if (!oidcmp(&info->b_tree, &info->i_tree) || !oidcmp(&c_tree, &info->i_tree)) {
> +			has_index = 0;
> +		} else {
> +			struct child_process cp = CHILD_PROCESS_INIT;
> +			struct strbuf out = STRBUF_INIT;
> +			struct argv_array args = ARGV_ARRAY_INIT;
> +			cp.git_cmd = 1;
> +			argv_array_pushl(&cp.args, "diff-tree", "--binary", NULL);
> +			argv_array_pushf(&cp.args, "%s^2^..%s^2", sha1_to_hex(info->w_commit.hash), sha1_to_hex(info->w_commit.hash));
> +			if (pipe_command(&cp, NULL, 0, &out, 0, NULL, 0))
> +				return -1;
> +
> +			child_process_init(&cp);
> +			cp.git_cmd = 1;
> +			argv_array_pushl(&cp.args, "apply", "--cached", NULL);
> +			if (pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0))
> +				return -1;
> +
> +			strbuf_release(&out);
> +			discard_cache();
> +			read_cache();
> +			if (write_cache_as_tree(index_tree.hash, 0, NULL))
> +				return -1;
> +
> +			argv_array_push(&args, "reset");
> +			cmd_reset(args.argc, args.argv, prefix);
> +		}
> +	}
> +
> +	if (info->has_u) {
> +		struct child_process cp = CHILD_PROCESS_INIT;
> +		struct child_process cp2 = CHILD_PROCESS_INIT;
> +		int res;
> +
> +		cp.git_cmd = 1;
> +		argv_array_push(&cp.args, "read-tree");
> +		argv_array_push(&cp.args, sha1_to_hex(info->u_tree.hash));
> +		argv_array_pushf(&cp.env_array, "GIT_INDEX_FILE=%s", stash_index_path);
> +
> +		cp2.git_cmd = 1;
> +		argv_array_pushl(&cp2.args, "checkout-index", "--all", NULL);
> +		argv_array_pushf(&cp2.env_array, "GIT_INDEX_FILE=%s", stash_index_path);
> +
> +		res = run_command(&cp) || run_command(&cp2);
> +		remove_path(stash_index_path);
> +		if (res)
> +			return error(_("Could not restore untracked files from stash"));

A minor change in behaviour here is that we are removing the temporary index file unconditionally here, while we would previously only remove it if both 'read-tree' and 'checkout-index' would succeed.

I don't think that's a bad thing, we probably don't want users to try and use that index file in any way, and I doubt that's part of anyones workflow, so I think cleaning it up makes sense.

Show 25 quoted lines

> +	}
> +
> +	init_merge_options(&o);
> +
> +	o.branch1 = "Updated upstream";
> +	o.branch2 = "Stashed changes";
> +
> +	if (!hashcmp(info->b_tree.hash, c_tree.hash))
> +		o.branch1 = "Version stash was based on";
> +
> +	if (quiet)
> +		o.verbosity = 0;
> +
> +	if (o.verbosity >= 3)
> +		printf_ln(_("Merging %s with %s"), o.branch1, o.branch2);
> +
> +	bases[0] = &info->b_tree;
> +
> +	ret = merge_recursive_generic(&o, &c_tree, &info->w_tree, bases_count, bases, &result);
> +	if (ret != 0) {
> +		struct argv_array args = ARGV_ARRAY_INIT;
> +		argv_array_push(&args, "rerere");
> +		cmd_rerere(args.argc, args.argv, prefix);
> +		if (index)
> +			printf_ln(_("Index was not unstashed."));

Minor nit: I think the above should be 'fprintf_ln(stderr, ...)' to match what we currently have.

Show 5 quoted lines

> +
> +		return ret;
> +	}
> +
> [...]