Web lists-archives.com

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




Hi Joel,

On Wed, 4 Apr 2018, Joel Teichroeb wrote:

> Add the drop and clear commands to the builtin helper. These two
> are each simple, but are being added together as they are quite
> related.

Makes sense.

> We have to unfortunately keep the drop and clear functions in the
> shell script as functions are called with parameters internally
> that are not valid when the commands are called externally. Once
> pop is converted they can both be removed.

Excellent explanation.

> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
> index 9d00a9547d..520cd746c4 100644
> --- a/builtin/stash--helper.c
> +++ b/builtin/stash--helper.c
> @@ -134,6 +146,29 @@ static int get_stash_info(struct stash_info *info, int argc, const char **argv)
>  	return 0;
>  }
>  
> +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);
> +}
> +
> +static int clear_stash(int argc, const char **argv, const char *prefix)
> +{
> +	struct option options[] = {
> +		OPT_END()
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, options, git_stash_helper_clear_usage, PARSE_OPT_STOP_AT_NON_OPTION);
> +
> +	if (argc != 0)
> +		return error(_("git stash--helper clear with parameters is unimplemented"));
> +
> +	return do_clear_stash();
> +}
> +
>  static int reset_tree(struct object_id *i_tree, int update, int reset)
>  {
>  	struct unpack_trees_options opts;
> @@ -399,6 +434,74 @@ static int apply_stash(int argc, const char **argv, const char *prefix)
>  	return ret;
>  }
>  
> +static int do_drop_stash(const char *prefix, struct stash_info *info)
> +{
> +	struct argv_array args = ARGV_ARRAY_INIT;
> +	int ret;
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +
> +	/* reflog does not provide a simple function for deleting refs. One will
> +	 * need to be added to avoid implementing too much reflog code here
> +	 */
> +	argv_array_pushl(&args, "reflog", "delete", "--updateref", "--rewrite", NULL);
> +	argv_array_push(&args, info->revision.buf);
> +	ret = cmd_reflog(args.argc, args.argv, prefix);
> +	if (!ret) {
> +		if (!quiet)
> +			printf(_("Dropped %s (%s)\n"), info->revision.buf, oid_to_hex(&info->w_commit));

This is a correct conversion. In the future, we will want to print this to
stderr, though.

> +	} else {
> +		return error(_("%s: Could not drop stash entry"), info->revision.buf);
> +	}
> +
> +	/* This could easily be replaced by get_oid, but currently it will throw a
> +	 * fatal error when a reflog is empty, which we can not recover from
> +	 */
> +	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);

Since we already have `grab_oid()` (or `get_oidf()` if you heed my
suggestion), we could use that here right away.

> +
> +	if (ret)
> +		do_clear_stash();

I was a bit puzzled why we clear the entire stash when we only wanted to
drop one? Going back to the shell script, there is this helpful comment:

	# clear_stash if we just dropped the last stash entry

Can you please add that comment here, too? It really helps readers, such
as myself...

> +
> +	return 0;
> +}
> +
> +static int assert_stash_ref(struct stash_info *info)
> +{
> +	if (!info->is_stash_ref)
> +		return error(_("'%s' is not a stash reference"), info->revision.buf);
> +
> +	return 0;
> +}

I am a bit curious why that did not appear in the previous patch, as
`apply_stash()` in the shell script already called `assert_stash_ref()`...

The rest of the patch looks pretty good to me! Thank you.

Ciao,
Dscho