Re: [PATCH v3 2/5] stash: convert apply to builtin

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mar 27, 2018, 16:02 UTC
Message-ID: <nycvar.QRO.7.76.6.1803271744370.77@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz>
In-Reply-To: <20180327054432.26419-3-joel@teichroeb.net>

Hi Joel,

On Mon, 26 Mar 2018, Joel Teichroeb wrote:

Show 14 quoted lines

> Add a bulitin helper for performing stash commands. Converting
> all at once proved hard to review, so starting with just apply
> let conversion get started without the other command being
> finished.
> 
> The helper is being implemented as a drop in replacement for
> stash so that when it is complete it can simply be renamed and
> the shell script deleted.
> 
> Delete the contents of the apply_stash shell function and replace
> it with a call to stash--helper apply until pop is also
> converted.
> 
> Signed-off-by: Joel Teichroeb <joel@teichroeb.net>

Makes sense.

I need a couple of adjustments before it compiles on Windows:

Show 18 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, 0, NULL) || reset_tree(&c_tree, 0, 0))

When applied on top of current `master`, I need to replace the &c_tree by c_tree.hash.

Likewise...

Show 21 quoted lines

> +		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 strbuf out = STRBUF_INIT;
> +
> +			if (diff_tree_binary(&out, &info->w_commit)) {
> +				strbuf_release(&out);
> +				return -1;
> +			}
> +
> +			ret = apply_cached(&out);
> +			strbuf_release(&out);
> +			if (ret)
> +				return -1;
> +
> +			discard_cache();
> +			read_cache();
> +			if (write_cache_as_tree(&index_tree, 0, NULL))

... &index_tree -> index_tree.hash.

These are probably changed to use object_id's already in `pu`, I guess.

I also need this change:

> [...]
> +
> +	index_file = get_index_file();
> +	xsnprintf(stash_index_path, PATH_MAX, "%s.stash.%d", index_file, pid);

Since `pid_t` is `unsigned long long` on Windows, I changed the %d" to %"PRIuMAX and cast `pid` to `(uintmax_t)`.

With those changes, the entire patch series compiles here.

BTW t3903 runs in 13m30s here with this patch series, 14m30s otherwise. That might not seem like much, until you realize that t3903 *still* performs a metric ton of Unix shell scripting outside of `git stash` (and that is the reason for the slowness).

Ciao, Dscho