Re: [PATCH v4 3/5] stash: convert drop and clear to builtin

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 30, 2018, 16:17 UTC
Message-ID: <xmqqin9d4jy2.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180328222129.22192-4-joel@teichroeb.net>

Joel Teichroeb <joel@teichroeb.net> writes:

Show 7 quoted lines

> +static int do_clear_stash(void)
> +{
> +	struct object_id obj;
> +	if (get_oid(ref_stash, &obj))
> +		return 0;
> +	return delete_ref(NULL, ref_stash, &obj, 0);
> +}

Here you see if the "refs/stash" is there, and learn what its current value is, using get_oid(), so that you can call delete_ref() with it.

Show 11 quoted lines

> +static int do_drop_stash(const char *prefix, struct stash_info *info)
> +{
> +	...
> +	cp.git_cmd = 1;
> +	/* Even though --quiet is specified, rev-parse still outputs the hash */
> +	cp.no_stdout = 1;
> +	argv_array_pushl(&cp.args, "rev-parse", "--verify", "--quiet", NULL);
> +	argv_array_pushf(&cp.args, "%s@{0}", ref_stash);
> +	ret = run_command(&cp);
> +	if (ret)
> +		do_clear_stash();

Here you call out to rev-parse as an external process. Isn't doing the same get_oid() sufficient?

Not limited to the above examples, the conversion in this series feels somewhat unbalanced---doing easy things like this by forking an external process and then doing a relatively heavyweight thing like merge operation (in 2/5) in-process feels the other way around.

Thanks.