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
