Re: [PATCH v5 3/5] stash: convert drop and clear to builtin
From: Johannes Schindelin <hidden>
Date: 2018-04-06 15:40:00
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.
quoted hunk ↗ jump to hunk
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