{"thread":{"id":"47164","subject":"[PATCH 0/2] Convert some stash functionality to a builtin","startedAt":"2017-11-10T23:13:25Z","lastAt":"2017-11-13T21:12:41Z","messageCount":6,"participants":["Joel Teichroeb","Thomas Gummerer","Martin Ågren"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"332264","messageId":"20171110231314.30711-1-joel@teichroeb.net","threadId":"47164","inReplyTo":null,"subject":"[PATCH 0/2] Convert some stash functionality to a builtin","fromName":"Joel Teichroeb","fromEmail":"joel@teichroeb.net","sentAt":"2017-11-10T23:13:12Z","receivedAt":"2017-11-10T23:13:25Z","isPatch":true,"sender":{"key":"joel@teichroeb.net","avatar":"https://avatars.githubusercontent.com/u/240865?v=4"},"body":"I've been working on converting all of git stash to be a\nbuiltin, however it's hard to get it all working at once with\nlimited time, so I've moved around half of it to a new\nstash--helper builtin and called these functions from the shell\nscript. Once this is stabalized, it should be easier to convert\nthe rest of the commands one at a time without breaking\nanything.\n\nI've sent most of this code before, but that was targetting a\nfull replacement of stash. The code is overall the same, but\nwith some code review changes and updates for internal api\nchanges.\n\nJoel Teichroeb (2):\n  merge: close the index lock when not writing the new index\n  stash: implement builtin stash helper\n\n Makefile                |   1 +\n builtin.h               |   1 +\n builtin/stash--helper.c | 516 ++++++++++++++++++++++++++++++++++++++++++++++++\n git-stash.sh            | 134 +------------\n git.c                   |   1 +\n merge-recursive.c       |   9 +-\n 6 files changed, 532 insertions(+), 130 deletions(-)\n create mode 100644 builtin/stash--helper.c\n\n-- \n2.15.0\n\n"},{"id":"332265","messageId":"20171110231314.30711-2-joel@teichroeb.net","threadId":"47164","inReplyTo":"20171110231314.30711-1-joel@teichroeb.net","subject":"[PATCH 1/2] merge: close the index lock when not writing the new index","fromName":"Joel Teichroeb","fromEmail":"joel@teichroeb.net","sentAt":"2017-11-10T23:13:13Z","receivedAt":"2017-11-10T23:13:28Z","isPatch":true,"sender":{"key":"joel@teichroeb.net","avatar":"https://avatars.githubusercontent.com/u/240865?v=4"},"body":"If the merge does not have anything to do, it does not unlock the index,\ncausing any further index operations to fail. Thus, always unlock the index\nregardless of outcome.\n\nSigned-off-by: Joel Teichroeb <joel@teichroeb.net>\n---\n merge-recursive.c | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 2ca8444c6..225ff3fb5 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -2184,9 +2184,12 @@ int merge_recursive_generic(struct merge_options *o,\n \tif (clean < 0)\n \t\treturn clean;\n \n-\tif (active_cache_changed &&\n-\t    write_locked_index(&the_index, &lock, COMMIT_LOCK))\n-\t\treturn err(o, _(\"Unable to write index.\"));\n+\tif (active_cache_changed) {\n+\t\tif (write_locked_index(&the_index, &lock, COMMIT_LOCK))\n+\t\t\treturn err(o, _(\"Unable to write index.\"));\n+\t} else {\n+\t\trollback_lock_file(&lock);\n+\t}\n \n \treturn clean ? 0 : 1;\n }\n-- \n2.15.0\n\n"},{"id":"332266","messageId":"20171110231314.30711-3-joel@teichroeb.net","threadId":"47164","inReplyTo":"20171110231314.30711-1-joel@teichroeb.net","subject":"[PATCH 2/2] stash: implement builtin stash helper","fromName":"Joel Teichroeb","fromEmail":"joel@teichroeb.net","sentAt":"2017-11-10T23:13:14Z","receivedAt":"2017-11-10T23:13:34Z","isPatch":true,"sender":{"key":"joel@teichroeb.net","avatar":"https://avatars.githubusercontent.com/u/240865?v=4"},"body":"Start moving stash functions over to builtin c code and call\nthem in the shell script, instead of converting it all at\nonce.\n\nSigned-off-by: Joel Teichroeb <joel@teichroeb.net>\n---\n Makefile                |   1 +\n builtin.h               |   1 +\n builtin/stash--helper.c | 516 ++++++++++++++++++++++++++++++++++++++++++++++++\n git-stash.sh            | 134 +------------\n git.c                   |   1 +\n 5 files changed, 526 insertions(+), 127 deletions(-)\n create mode 100644 builtin/stash--helper.c\n\ndiff --git a/Makefile b/Makefile\nindex ee9d5eb11..3a9bd4d57 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1000,6 +1000,7 @@ BUILTIN_OBJS += builtin/send-pack.o\n BUILTIN_OBJS += builtin/shortlog.o\n BUILTIN_OBJS += builtin/show-branch.o\n BUILTIN_OBJS += builtin/show-ref.o\n+BUILTIN_OBJS += builtin/stash--helper.o\n BUILTIN_OBJS += builtin/stripspace.o\n BUILTIN_OBJS += builtin/submodule--helper.o\n BUILTIN_OBJS += builtin/symbolic-ref.o\ndiff --git a/builtin.h b/builtin.h\nindex 42378f3aa..a14fd85b0 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -219,6 +219,7 @@ extern int cmd_shortlog(int argc, const char **argv, const char *prefix);\n extern int cmd_show(int argc, const char **argv, const char *prefix);\n extern int cmd_show_branch(int argc, const char **argv, const char *prefix);\n extern int cmd_status(int argc, const char **argv, const char *prefix);\n+extern int cmd_stash__helper(int argc, const char **argv, const char *prefix);\n extern int cmd_stripspace(int argc, const char **argv, const char *prefix);\n extern int cmd_submodule__helper(int argc, const char **argv, const char *prefix);\n extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);\ndiff --git a/builtin/stash--helper.c b/builtin/stash--helper.c\nnew file mode 100644\nindex 000000000..c8cb667fe\n--- /dev/null\n+++ b/builtin/stash--helper.c\n@@ -0,0 +1,516 @@\n+#include \"builtin.h\"\n+#include \"config.h\"\n+#include \"parse-options.h\"\n+#include \"refs.h\"\n+#include \"lockfile.h\"\n+#include \"cache-tree.h\"\n+#include \"unpack-trees.h\"\n+#include \"merge-recursive.h\"\n+#include \"argv-array.h\"\n+#include \"run-command.h\"\n+\n+static const char * const git_stash_usage[] = {\n+\tN_(\"git stash--helper drop [-q|--quiet] [<stash>]\"),\n+\tN_(\"git stash--helper pop [--index] [-q|--quiet] [<stash>]\"),\n+\tN_(\"git stash--helper apply [--index] [-q|--quiet] [<stash>]\"),\n+\tN_(\"git stash--helper branch <branchname> [<stash>]\"),\n+\tN_(\"git stash--helper clear\"),\n+\tNULL\n+};\n+\n+static const char * const git_stash_drop_usage[] = {\n+\tN_(\"git stash--helper drop [-q|--quiet] [<stash>]\"),\n+\tNULL\n+};\n+\n+static const char * const git_stash_pop_usage[] = {\n+\tN_(\"git stash--helper pop [--index] [-q|--quiet] [<stash>]\"),\n+\tNULL\n+};\n+\n+static const char * const git_stash_apply_usage[] = {\n+\tN_(\"git stash--helper apply [--index] [-q|--quiet] [<stash>]\"),\n+\tNULL\n+};\n+\n+static const char * const git_stash_branch_usage[] = {\n+\tN_(\"git stash--helper branch <branchname> [<stash>]\"),\n+\tNULL\n+};\n+\n+static const char * const git_stash_clear_usage[] = {\n+\tN_(\"git stash--helper clear\"),\n+\tNULL\n+};\n+\n+static const char *ref_stash = \"refs/stash\";\n+static int quiet;\n+static char stash_index_path[PATH_MAX];\n+\n+struct stash_info {\n+\tstruct object_id w_commit;\n+\tstruct object_id b_commit;\n+\tstruct object_id i_commit;\n+\tstruct object_id u_commit;\n+\tstruct object_id w_tree;\n+\tstruct object_id b_tree;\n+\tstruct object_id i_tree;\n+\tstruct object_id u_tree;\n+\tconst char *message;\n+\tconst char *revision;\n+\tint is_stash_ref;\n+\tint has_u;\n+\tconst char *patch;\n+};\n+\n+static int get_stash_info(struct stash_info *info, const char *commit)\n+{\n+\tstruct strbuf w_commit_rev = STRBUF_INIT;\n+\tstruct strbuf b_commit_rev = STRBUF_INIT;\n+\tstruct strbuf w_tree_rev = STRBUF_INIT;\n+\tstruct strbuf b_tree_rev = STRBUF_INIT;\n+\tstruct strbuf i_tree_rev = STRBUF_INIT;\n+\tstruct strbuf u_tree_rev = STRBUF_INIT;\n+\tstruct strbuf commit_buf = STRBUF_INIT;\n+\tstruct strbuf symbolic = STRBUF_INIT;\n+\tstruct strbuf out = STRBUF_INIT;\n+\tint ret;\n+\tconst char *revision = commit;\n+\tchar *end_of_rev;\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tinfo->is_stash_ref = 0;\n+\n+\tif (commit == NULL) {\n+\t\tstrbuf_addf(&commit_buf, \"%s@{0}\", ref_stash);\n+\t\trevision = commit_buf.buf;\n+\t} else if (strspn(commit, \"0123456789\") == strlen(commit)) {\n+\t\tstrbuf_addf(&commit_buf, \"%s@{%s}\", ref_stash, commit);\n+\t\trevision = commit_buf.buf;\n+\t}\n+\tinfo->revision = revision;\n+\n+\tstrbuf_addf(&w_commit_rev, \"%s\", revision);\n+\tstrbuf_addf(&b_commit_rev, \"%s^1\", revision);\n+\tstrbuf_addf(&w_tree_rev, \"%s:\", revision);\n+\tstrbuf_addf(&b_tree_rev, \"%s^1:\", revision);\n+\tstrbuf_addf(&i_tree_rev, \"%s^2:\", revision);\n+\n+\tret = !get_oid(w_commit_rev.buf, &info->w_commit) &&\n+\t\t!get_oid(b_commit_rev.buf, &info->b_commit) &&\n+\t\t!get_oid(w_tree_rev.buf, &info->w_tree) &&\n+\t\t!get_oid(b_tree_rev.buf, &info->b_tree) &&\n+\t\t!get_oid(i_tree_rev.buf, &info->i_tree);\n+\n+\tstrbuf_release(&w_commit_rev);\n+\tstrbuf_release(&b_commit_rev);\n+\tstrbuf_release(&w_tree_rev);\n+\tstrbuf_release(&b_tree_rev);\n+\tstrbuf_release(&i_tree_rev);\n+\n+\tif (!ret)\n+\t\treturn error(_(\"%s is not a valid reference\"), revision);\n+\n+\tstrbuf_addf(&u_tree_rev, \"%s^3:\", revision);\n+\n+\tinfo->has_u = !get_oid(u_tree_rev.buf, &info->u_tree);\n+\n+\tstrbuf_release(&u_tree_rev);\n+\n+\tend_of_rev = strchrnul(revision, '@');\n+\tstrbuf_add(&symbolic, revision, end_of_rev - revision);\n+\tcp.git_cmd = 1;\n+\targv_array_pushl(&cp.args, \"rev-parse\", \"--symbolic-full-name\", NULL);\n+\targv_array_pushf(&cp.args, \"%s\", symbolic.buf);\n+\tstrbuf_release(&symbolic);\n+\tpipe_command(&cp, NULL, 0, &out, 0, NULL, 0);\n+\n+\tif (out.len - 1 == strlen(ref_stash))\n+\t\tinfo->is_stash_ref = !strncmp(out.buf, ref_stash, out.len - 1);\n+\tstrbuf_release(&out);\n+\n+\treturn 0;\n+}\n+\n+static int do_clear_stash(void)\n+{\n+\tstruct object_id obj;\n+\tif (get_oid(ref_stash, &obj))\n+\t\treturn 0;\n+\n+\treturn delete_ref(NULL, ref_stash, &obj, 0);\n+}\n+\n+static int clear_stash(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct option options[] = {\n+\t\tOPT_END()\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, options, git_stash_clear_usage, PARSE_OPT_STOP_AT_NON_OPTION);\n+\n+\tif (argc != 0)\n+\t\treturn error(_(\"git stash--helper clear with parameters is unimplemented\"));\n+\n+\treturn do_clear_stash();\n+}\n+\n+static int reset_tree(struct object_id i_tree, int update, int reset)\n+{\n+\tstruct unpack_trees_options opts;\n+\tint nr_trees = 1;\n+\tstruct tree_desc t[MAX_UNPACK_TREES];\n+\tstruct tree *tree;\n+\tstruct lock_file lock_file = LOCK_INIT;\n+\n+\tread_cache_preload(NULL);\n+\tif (refresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL))\n+\t\treturn -1;\n+\n+\thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n+\n+\tmemset(&opts, 0, sizeof(opts));\n+\n+\ttree = parse_tree_indirect(&i_tree);\n+\tif (parse_tree(tree))\n+\t\treturn -1;\n+\n+\tinit_tree_desc(t, tree->buffer, tree->size);\n+\n+\topts.head_idx = 1;\n+\topts.src_index = &the_index;\n+\topts.dst_index = &the_index;\n+\topts.merge = 1;\n+\topts.reset = reset;\n+\topts.update = update;\n+\topts.fn = oneway_merge;\n+\n+\tif (unpack_trees(nr_trees, t, &opts))\n+\t\treturn -1;\n+\n+\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK)) {\n+\t\terror(_(\"unable to write new index file\"));\n+\t\treturn -1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n+static int do_apply_stash(const char *prefix, struct stash_info *info, int index)\n+{\n+\tstruct merge_options o;\n+\tstruct object_id c_tree;\n+\tstruct object_id index_tree;\n+\tconst struct object_id *bases[1];\n+\tint bases_count = 1;\n+\tstruct commit *result;\n+\tint ret;\n+\n+\tread_cache_preload(NULL);\n+\tif (refresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL))\n+\t\treturn -1;\n+\n+\tif (write_cache_as_tree(c_tree.hash, 0, NULL))\n+\t\treturn -1;\n+\n+\tif (reset_tree(c_tree, 0, 0))\n+\t\treturn -1;\n+\n+\tif (index) {\n+\t\tif (!hashcmp(info->b_tree.hash, info->i_tree.hash) || !hashcmp(c_tree.hash, info->i_tree.hash)) {\n+\t\t\tindex = 0;\n+\t\t} else {\n+\t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\t\t\tstruct strbuf out = STRBUF_INIT;\n+\t\t\tstruct argv_array args = ARGV_ARRAY_INIT;\n+\t\t\tcp.git_cmd = 1;\n+\t\t\targv_array_pushl(&cp.args, \"diff-tree\", \"--binary\", NULL);\n+\t\t\targv_array_pushf(&cp.args, \"%s^2^..%s^2\", sha1_to_hex(info->w_commit.hash), sha1_to_hex(info->w_commit.hash));\n+\t\t\tif (pipe_command(&cp, NULL, 0, &out, 0, NULL, 0))\n+\t\t\t\treturn -1;\n+\n+\t\t\tchild_process_init(&cp);\n+\t\t\tcp.git_cmd = 1;\n+\t\t\targv_array_pushl(&cp.args, \"apply\", \"--cached\", NULL);\n+\t\t\tif (pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0))\n+\t\t\t\treturn -1;\n+\n+\t\t\tstrbuf_release(&out);\n+\t\t\tdiscard_cache();\n+\t\t\tread_cache();\n+\t\t\tif (write_cache_as_tree(index_tree.hash, 0, NULL))\n+\t\t\t\treturn -1;\n+\n+\t\t\targv_array_push(&args, \"reset\");\n+\t\t\tcmd_reset(args.argc, args.argv, prefix);\n+\t\t}\n+\t}\n+\n+\tif (info->has_u) {\n+\t\tstruct argv_array args = ARGV_ARRAY_INIT;\n+\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\t\tconst char *index_file = get_index_file();\n+\n+\t\targv_array_push(&args, \"read-tree\");\n+\t\targv_array_push(&args, sha1_to_hex(info->u_tree.hash));\n+\t\targv_array_pushf(&args, \"--index-output=%s\", stash_index_path);\n+\n+\t\tcp.git_cmd = 1;\n+\t\targv_array_pushl(&cp.args, \"checkout-index\", \"--all\", NULL);\n+\t\targv_array_pushf(&cp.env_array, \"GIT_INDEX_FILE=%s\", stash_index_path);\n+\n+\t\tif (cmd_read_tree(args.argc, args.argv, prefix) ||\n+\t\t\t\trun_command(&cp)) {\n+\t\t\treturn error(_(\"Could not restore untracked files from stash\"));\n+\t\t}\n+\t\tset_alternate_index_output(index_file);\n+\t}\n+\n+\tinit_merge_options(&o);\n+\n+\to.branch1 = \"Updated upstream\";\n+\to.branch2 = \"Stashed changes\";\n+\n+\tif (!hashcmp(info->b_tree.hash, c_tree.hash))\n+\t\to.branch1 = \"Version stash was based on\";\n+\n+\tif (quiet)\n+\t\to.verbosity = 0;\n+\n+\tif (o.verbosity >= 3)\n+\t\tprintf_ln(_(\"Merging %s with %s\"), o.branch1, o.branch2);\n+\n+\tbases[0] = &info->b_tree;\n+\n+\tret = merge_recursive_generic(&o, &c_tree, &info->w_tree, bases_count, bases, &result);\n+\tif (ret != 0) {\n+\t\tstruct argv_array args = ARGV_ARRAY_INIT;\n+\t\targv_array_push(&args, \"rerere\");\n+\t\tcmd_rerere(args.argc, args.argv, prefix);\n+\n+\t\tif (index)\n+\t\t\tprintf_ln(_(\"Index was not unstashed.\"));\n+\n+\t\treturn ret;\n+\t}\n+\n+\tif (index) {\n+\t\tif (reset_tree(index_tree, 0, 0))\n+\t\t\treturn -1;\n+\t} else {\n+\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\t\tstruct strbuf out = STRBUF_INIT;\n+\t\tcp.git_cmd = 1;\n+\t\targv_array_pushl(&cp.args, \"diff-index\", \"--cached\", \"--name-only\", \"--diff-filter=A\", NULL);\n+\t\targv_array_push(&cp.args, sha1_to_hex(c_tree.hash));\n+\t\tret = pipe_command(&cp, NULL, 0, &out, 0, NULL, 0);\n+\t\tif (ret)\n+\t\t\treturn -1;\n+\n+\t\tif (reset_tree(c_tree, 0, 1))\n+\t\t\treturn -1;\n+\n+\t\tchild_process_init(&cp);\n+\t\tcp.git_cmd = 1;\n+\t\targv_array_pushl(&cp.args, \"update-index\", \"--add\", \"--stdin\", NULL);\n+\t\tret = pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0);\n+\t\tif (ret)\n+\t\t\treturn -1;\n+\n+\t\tstrbuf_release(&out);\n+\t\tdiscard_cache();\n+\t\tread_cache();\n+\t}\n+\n+\tif (!quiet) {\n+\t\tstruct argv_array args = ARGV_ARRAY_INIT;\n+\t\targv_array_push(&args, \"status\");\n+\t\tcmd_status(args.argc, args.argv, prefix);\n+\t}\n+\n+\treturn 0;\n+}\n+\n+static int apply_stash(int argc, const char **argv, const char *prefix)\n+{\n+\tconst char *commit = NULL;\n+\tint index = 0;\n+\tstruct stash_info info;\n+\tstruct option options[] = {\n+\t\tOPT__QUIET(&quiet, N_(\"be quiet, only report errors\")),\n+\t\tOPT_BOOL(0, \"index\", &index,\n+\t\t\tN_(\"attempt to ininstate the index\")),\n+\t\tOPT_END()\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, options,\n+\t\t\tgit_stash_apply_usage, 0);\n+\n+\tif (argc == 1) {\n+\t\tcommit = argv[0];\n+\t}\n+\n+\tif (get_stash_info(&info, commit))\n+\t\treturn -1;\n+\n+\n+\treturn do_apply_stash(prefix, &info, index);\n+}\n+\n+static int do_drop_stash(const char *prefix, struct stash_info *info)\n+{\n+\tstruct argv_array args = ARGV_ARRAY_INIT;\n+\tint ret;\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\n+\targv_array_pushl(&args, \"reflog\", \"delete\", \"--updateref\", \"--rewrite\", NULL);\n+\targv_array_push(&args, info->revision);\n+\tret = cmd_reflog(args.argc, args.argv, prefix);\n+\tif (!ret) {\n+\t\tif (!quiet) {\n+\t\t\tprintf(_(\"Dropped %s (%s)\\n\"), info->revision, sha1_to_hex(info->w_commit.hash));\n+\t\t}\n+\t} else {\n+\t\treturn error(_(\"%s: Could not drop stash entry\"), info->revision);\n+\t}\n+\n+\tcp.git_cmd = 1;\n+\t/* Even though --quiet is specified, rev-parse still outputs the hash */\n+\tcp.no_stdout = 1;\n+\targv_array_pushl(&cp.args, \"rev-parse\", \"--verify\", \"--quiet\", NULL);\n+\targv_array_pushf(&cp.args, \"%s@{0}\", ref_stash);\n+\tret = run_command(&cp);\n+\n+\tif (ret)\n+\t\tdo_clear_stash();\n+\n+\treturn 0;\n+}\n+\n+static int drop_stash(int argc, const char **argv, const char *prefix)\n+{\n+\tconst char *commit = NULL;\n+\tstruct stash_info info;\n+\tstruct option options[] = {\n+\t\tOPT__QUIET(&quiet, N_(\"be quiet, only report errors\")),\n+\t\tOPT_END()\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, options,\n+\t\t\tgit_stash_drop_usage, 0);\n+\n+\tif (argc == 1)\n+\t\tcommit = argv[0];\n+\n+\tif (get_stash_info(&info, commit))\n+\t\treturn -1;\n+\n+\tif (!info.is_stash_ref)\n+\t\treturn error(_(\"'%s' is not a stash reference\"), commit);\n+\n+\treturn do_drop_stash(prefix, &info);\n+}\n+\n+static int pop_stash(int argc, const char **argv, const char *prefix)\n+{\n+\tint index = 0;\n+\tconst char *commit = NULL;\n+\tstruct stash_info info;\n+\tstruct option options[] = {\n+\t\tOPT__QUIET(&quiet, N_(\"be quiet, only report errors\")),\n+\t\tOPT_BOOL(0, \"index\", &index,\n+\t\t\tN_(\"attempt to ininstate the index\")),\n+\t\tOPT_END()\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, options,\n+\t\t\tgit_stash_pop_usage, 0);\n+\n+\tif (argc == 1)\n+\t\tcommit = argv[0];\n+\n+\tif (get_stash_info(&info, commit))\n+\t\treturn -1;\n+\n+\tif (!info.is_stash_ref)\n+\t\treturn error(_(\"'%s' is not a stash reference\"), commit);\n+\n+\tif (do_apply_stash(prefix, &info, index))\n+\t\treturn -1;\n+\n+\treturn do_drop_stash(prefix, &info);\n+}\n+\n+static int branch_stash(int argc, const char **argv, const char *prefix)\n+{\n+\tconst char *commit = NULL, *branch = NULL;\n+\tint ret;\n+\tstruct argv_array args = ARGV_ARRAY_INIT;\n+\tstruct stash_info info;\n+\tstruct option options[] = {\n+\t\tOPT_END()\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, options,\n+\t\t\tgit_stash_branch_usage, 0);\n+\n+\tif (argc != 0) {\n+\t\tbranch = argv[0];\n+\t\tif (argc == 2)\n+\t\t\tcommit = argv[1];\n+\t}\n+\n+\tif (get_stash_info(&info, commit))\n+\t\treturn -1;\n+\n+\targv_array_pushl(&args, \"checkout\", \"-b\", NULL);\n+\targv_array_push(&args, branch);\n+\targv_array_push(&args, sha1_to_hex(info.b_commit.hash));\n+\tret = cmd_checkout(args.argc, args.argv, prefix);\n+\tif (ret)\n+\t\treturn -1;\n+\n+\tret = do_apply_stash(prefix, &info, 1);\n+\tif (!ret && info.is_stash_ref)\n+\t\tret = do_drop_stash(prefix, &info);\n+\n+\treturn ret;\n+}\n+\n+int cmd_stash__helper(int argc, const char **argv, const char *prefix)\n+{\n+\tint result = 0;\n+\tpid_t pid = getpid();\n+\tconst char *index_file;\n+\n+\tstruct option options[] = {\n+\t\tOPT_END()\n+\t};\n+\n+\tgit_config(git_default_config, NULL);\n+\n+\targc = parse_options(argc, argv, prefix, options, git_stash_usage,\n+\t\tPARSE_OPT_KEEP_UNKNOWN|PARSE_OPT_KEEP_DASHDASH);\n+\n+\tindex_file = get_index_file();\n+\txsnprintf(stash_index_path, PATH_MAX, \"%s.stash.%d\", index_file, pid);\n+\n+\tif (argc < 1)\n+\t\tusage_with_options(git_stash_usage, options);\n+\telse if (!strcmp(argv[0], \"apply\"))\n+\t\tresult = apply_stash(argc, argv, prefix);\n+\telse if (!strcmp(argv[0], \"clear\"))\n+\t\tresult = clear_stash(argc, argv, prefix);\n+\telse if (!strcmp(argv[0], \"drop\"))\n+\t\tresult = drop_stash(argc, argv, prefix);\n+\telse if (!strcmp(argv[0], \"pop\"))\n+\t\tresult = pop_stash(argc, argv, prefix);\n+\telse if (!strcmp(argv[0], \"branch\"))\n+\t\tresult = branch_stash(argc, argv, prefix);\n+\telse {\n+\t\terror(_(\"unknown subcommand: %s\"), argv[0]);\n+\t\tusage_with_options(git_stash_usage, options);\n+\t\tresult = 1;\n+\t}\n+\n+\treturn result;\n+}\ndiff --git a/git-stash.sh b/git-stash.sh\nindex 4b7495144..ed2c57027 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -535,129 +535,6 @@ assert_stash_like() {\n \t}\n }\n \n-is_stash_ref() {\n-\tis_stash_like \"$@\" && test -n \"$IS_STASH_REF\"\n-}\n-\n-assert_stash_ref() {\n-\tis_stash_ref \"$@\" || {\n-\t\targs=\"$*\"\n-\t\tdie \"$(eval_gettext \"'\\$args' is not a stash reference\")\"\n-\t}\n-}\n-\n-apply_stash () {\n-\n-\tassert_stash_like \"$@\"\n-\n-\tgit update-index -q --refresh || die \"$(gettext \"unable to refresh index\")\"\n-\n-\t# current index state\n-\tc_tree=$(git write-tree) ||\n-\t\tdie \"$(gettext \"Cannot apply a stash in the middle of a merge\")\"\n-\n-\tunstashed_index_tree=\n-\tif test -n \"$INDEX_OPTION\" && test \"$b_tree\" != \"$i_tree\" &&\n-\t\t\ttest \"$c_tree\" != \"$i_tree\"\n-\tthen\n-\t\tgit diff-tree --binary $s^2^..$s^2 | git apply --cached\n-\t\ttest $? -ne 0 &&\n-\t\t\tdie \"$(gettext \"Conflicts in index. Try without --index.\")\"\n-\t\tunstashed_index_tree=$(git write-tree) ||\n-\t\t\tdie \"$(gettext \"Could not save index tree\")\"\n-\t\tgit reset\n-\tfi\n-\n-\tif test -n \"$u_tree\"\n-\tthen\n-\t\tGIT_INDEX_FILE=\"$TMPindex\" git read-tree \"$u_tree\" &&\n-\t\tGIT_INDEX_FILE=\"$TMPindex\" git checkout-index --all &&\n-\t\trm -f \"$TMPindex\" ||\n-\t\tdie \"$(gettext \"Could not restore untracked files from stash entry\")\"\n-\tfi\n-\n-\teval \"\n-\t\tGITHEAD_$w_tree='Stashed changes' &&\n-\t\tGITHEAD_$c_tree='Updated upstream' &&\n-\t\tGITHEAD_$b_tree='Version stash was based on' &&\n-\t\texport GITHEAD_$w_tree GITHEAD_$c_tree GITHEAD_$b_tree\n-\t\"\n-\n-\tif test -n \"$GIT_QUIET\"\n-\tthen\n-\t\tGIT_MERGE_VERBOSITY=0 && export GIT_MERGE_VERBOSITY\n-\tfi\n-\tif git merge-recursive $b_tree -- $c_tree $w_tree\n-\tthen\n-\t\t# No conflict\n-\t\tif test -n \"$unstashed_index_tree\"\n-\t\tthen\n-\t\t\tgit read-tree \"$unstashed_index_tree\"\n-\t\telse\n-\t\t\ta=\"$TMP-added\" &&\n-\t\t\tgit diff-index --cached --name-only --diff-filter=A $c_tree >\"$a\" &&\n-\t\t\tgit read-tree --reset $c_tree &&\n-\t\t\tgit update-index --add --stdin <\"$a\" ||\n-\t\t\t\tdie \"$(gettext \"Cannot unstage modified files\")\"\n-\t\t\trm -f \"$a\"\n-\t\tfi\n-\t\tsquelch=\n-\t\tif test -n \"$GIT_QUIET\"\n-\t\tthen\n-\t\t\tsquelch='>/dev/null 2>&1'\n-\t\tfi\n-\t\t(cd \"$START_DIR\" && eval \"git status $squelch\") || :\n-\telse\n-\t\t# Merge conflict; keep the exit status from merge-recursive\n-\t\tstatus=$?\n-\t\tgit rerere\n-\t\tif test -n \"$INDEX_OPTION\"\n-\t\tthen\n-\t\t\tgettextln \"Index was not unstashed.\" >&2\n-\t\tfi\n-\t\texit $status\n-\tfi\n-}\n-\n-pop_stash() {\n-\tassert_stash_ref \"$@\"\n-\n-\tif apply_stash \"$@\"\n-\tthen\n-\t\tdrop_stash \"$@\"\n-\telse\n-\t\tstatus=$?\n-\t\tsay \"$(gettext \"The stash entry is kept in case you need it again.\")\"\n-\t\texit $status\n-\tfi\n-}\n-\n-drop_stash () {\n-\tassert_stash_ref \"$@\"\n-\n-\tgit reflog delete --updateref --rewrite \"${REV}\" &&\n-\t\tsay \"$(eval_gettext \"Dropped \\${REV} (\\$s)\")\" ||\n-\t\tdie \"$(eval_gettext \"\\${REV}: Could not drop stash entry\")\"\n-\n-\t# clear_stash if we just dropped the last stash entry\n-\tgit rev-parse --verify --quiet \"$ref_stash@{0}\" >/dev/null ||\n-\tclear_stash\n-}\n-\n-apply_to_branch () {\n-\ttest -n \"$1\" || die \"$(gettext \"No branch name specified\")\"\n-\tbranch=$1\n-\tshift 1\n-\n-\tset -- --index \"$@\"\n-\tassert_stash_like \"$@\"\n-\n-\tgit checkout -b $branch $REV^ &&\n-\tapply_stash \"$@\" && {\n-\t\ttest -z \"$IS_STASH_REF\" || drop_stash \"$@\"\n-\t}\n-}\n-\n test \"$1\" = \"-p\" && set \"push\" \"$@\"\n \n PARSE_CACHE='--not-parsed'\n@@ -694,7 +571,8 @@ push)\n \t;;\n apply)\n \tshift\n-\tapply_stash \"$@\"\n+\tcd \"$START_DIR\"\n+\tgit stash--helper apply \"$@\"\n \t;;\n clear)\n \tshift\n@@ -710,15 +588,17 @@ store)\n \t;;\n drop)\n \tshift\n-\tdrop_stash \"$@\"\n+\tgit stash--helper drop \"$@\"\n \t;;\n pop)\n \tshift\n-\tpop_stash \"$@\"\n+\tcd \"$START_DIR\"\n+\tgit stash--helper pop \"$@\"\n \t;;\n branch)\n \tshift\n-\tapply_to_branch \"$@\"\n+\tcd \"$START_DIR\"\n+\tgit stash--helper branch \"$@\"\n \t;;\n *)\n \tcase $# in\ndiff --git a/git.c b/git.c\nindex 9e96dd409..868727dc3 100644\n--- a/git.c\n+++ b/git.c\n@@ -466,6 +466,7 @@ static struct cmd_struct commands[] = {\n \t{ \"show-branch\", cmd_show_branch, RUN_SETUP },\n \t{ \"show-ref\", cmd_show_ref, RUN_SETUP },\n \t{ \"stage\", cmd_add, RUN_SETUP | NEED_WORK_TREE },\n+\t{ \"stash--helper\", cmd_stash__helper, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"status\", cmd_status, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"stripspace\", cmd_stripspace },\n \t{ \"submodule--helper\", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX},\n-- \n2.15.0\n\n"},{"id":"332341","messageId":"20171112152741.GA15767@hank.wfi.bloomberg.com","threadId":"47164","inReplyTo":"20171110231314.30711-3-joel@teichroeb.net","subject":"Re: [PATCH 2/2] stash: implement builtin stash helper","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2017-11-12T15:27:41Z","receivedAt":"2017-11-12T15:26:26Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 11/10, Joel Teichroeb wrote:\n> Start moving stash functions over to builtin c code and call\n> them in the shell script, instead of converting it all at\n> once.\n> \n> Signed-off-by: Joel Teichroeb <joel@teichroeb.net>\n> ---\n\nThanks for working on this!  I like the approach of converting this\none command at a time.  I think it would be even better if this was a\nseries of patches, converting stash one command at a time, instead of\nadding multiple multiple commands to stash helper in one patch.  This\nwould make reviewing the patches a bit easier.\n\nFor example this could look like:\n[2/5] stash: convert apply to builtin\n[3/5] stash: convert branch to builtin\n[4/5] stash: convert drop to builtin\n[5/5] stash: convert pop to builtin\n\nSome comments from me below.\n\n>  Makefile                |   1 +\n>  builtin.h               |   1 +\n>  builtin/stash--helper.c | 516 ++++++++++++++++++++++++++++++++++++++++++++++++\n>  git-stash.sh            | 134 +------------\n>  git.c                   |   1 +\n>  5 files changed, 526 insertions(+), 127 deletions(-)\n>  create mode 100644 builtin/stash--helper.c\n> \n> diff --git a/Makefile b/Makefile\n> index ee9d5eb11..3a9bd4d57 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1000,6 +1000,7 @@ BUILTIN_OBJS += builtin/send-pack.o\n>  BUILTIN_OBJS += builtin/shortlog.o\n>  BUILTIN_OBJS += builtin/show-branch.o\n>  BUILTIN_OBJS += builtin/show-ref.o\n> +BUILTIN_OBJS += builtin/stash--helper.o\n>  BUILTIN_OBJS += builtin/stripspace.o\n>  BUILTIN_OBJS += builtin/submodule--helper.o\n>  BUILTIN_OBJS += builtin/symbolic-ref.o\n> diff --git a/builtin.h b/builtin.h\n> index 42378f3aa..a14fd85b0 100644\n> --- a/builtin.h\n> +++ b/builtin.h\n> @@ -219,6 +219,7 @@ extern int cmd_shortlog(int argc, const char **argv, const char *prefix);\n>  extern int cmd_show(int argc, const char **argv, const char *prefix);\n>  extern int cmd_show_branch(int argc, const char **argv, const char *prefix);\n>  extern int cmd_status(int argc, const char **argv, const char *prefix);\n> +extern int cmd_stash__helper(int argc, const char **argv, const char *prefix);\n>  extern int cmd_stripspace(int argc, const char **argv, const char *prefix);\n>  extern int cmd_submodule__helper(int argc, const char **argv, const char *prefix);\n>  extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);\n> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c\n> new file mode 100644\n> index 000000000..c8cb667fe\n> --- /dev/null\n> +++ b/builtin/stash--helper.c\n> @@ -0,0 +1,516 @@\n> +#include \"builtin.h\"\n> +#include \"config.h\"\n> +#include \"parse-options.h\"\n> +#include \"refs.h\"\n> +#include \"lockfile.h\"\n> +#include \"cache-tree.h\"\n> +#include \"unpack-trees.h\"\n> +#include \"merge-recursive.h\"\n> +#include \"argv-array.h\"\n> +#include \"run-command.h\"\n> +\n> +static const char * const git_stash_usage[] = {\n\ns/stash/&_helper/ maybe?  This is how we call it in the other helpers\nas well.\n\n> +\tN_(\"git stash--helper drop [-q|--quiet] [<stash>]\"),\n> +\tN_(\"git stash--helper pop [--index] [-q|--quiet] [<stash>]\"),\n> +\tN_(\"git stash--helper apply [--index] [-q|--quiet] [<stash>]\"),\n> +\tN_(\"git stash--helper branch <branchname> [<stash>]\"),\n> +\tN_(\"git stash--helper clear\"),\n> +\tNULL\n> +};\n> +\n> +static const char * const git_stash_drop_usage[] = {\n> +\tN_(\"git stash--helper drop [-q|--quiet] [<stash>]\"),\n> +\tNULL\n> +};\n> +\n> +static const char * const git_stash_pop_usage[] = {\n> +\tN_(\"git stash--helper pop [--index] [-q|--quiet] [<stash>]\"),\n> +\tNULL\n> +};\n> +\n> +static const char * const git_stash_apply_usage[] = {\n> +\tN_(\"git stash--helper apply [--index] [-q|--quiet] [<stash>]\"),\n> +\tNULL\n> +};\n> +\n> +static const char * const git_stash_branch_usage[] = {\n> +\tN_(\"git stash--helper branch <branchname> [<stash>]\"),\n> +\tNULL\n> +};\n> +\n> +static const char * const git_stash_clear_usage[] = {\n> +\tN_(\"git stash--helper clear\"),\n> +\tNULL\n> +};\n> +\n> +static const char *ref_stash = \"refs/stash\";\n> +static int quiet;\n> +static char stash_index_path[PATH_MAX];\n> +\n> +struct stash_info {\n> +\tstruct object_id w_commit;\n> +\tstruct object_id b_commit;\n> +\tstruct object_id i_commit;\n> +\tstruct object_id u_commit;\n> +\tstruct object_id w_tree;\n> +\tstruct object_id b_tree;\n> +\tstruct object_id i_tree;\n> +\tstruct object_id u_tree;\n> +\tconst char *message;\n> +\tconst char *revision;\n> +\tint is_stash_ref;\n> +\tint has_u;\n> +\tconst char *patch;\n> +};\n> +\n> +static int get_stash_info(struct stash_info *info, const char *commit)\n> +{\n> +\tstruct strbuf w_commit_rev = STRBUF_INIT;\n> +\tstruct strbuf b_commit_rev = STRBUF_INIT;\n> +\tstruct strbuf w_tree_rev = STRBUF_INIT;\n> +\tstruct strbuf b_tree_rev = STRBUF_INIT;\n> +\tstruct strbuf i_tree_rev = STRBUF_INIT;\n> +\tstruct strbuf u_tree_rev = STRBUF_INIT;\n> +\tstruct strbuf commit_buf = STRBUF_INIT;\n> +\tstruct strbuf symbolic = STRBUF_INIT;\n> +\tstruct strbuf out = STRBUF_INIT;\n> +\tint ret;\n> +\tconst char *revision = commit;\n> +\tchar *end_of_rev;\n> +\tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\tinfo->is_stash_ref = 0;\n> +\n> +\tif (commit == NULL) {\n> +\t\tstrbuf_addf(&commit_buf, \"%s@{0}\", ref_stash);\n> +\t\trevision = commit_buf.buf;\n> +\t} else if (strspn(commit, \"0123456789\") == strlen(commit)) {\n> +\t\tstrbuf_addf(&commit_buf, \"%s@{%s}\", ref_stash, commit);\n> +\t\trevision = commit_buf.buf;\n> +\t}\n> +\tinfo->revision = revision;\n> +\n> +\tstrbuf_addf(&w_commit_rev, \"%s\", revision);\n> +\tstrbuf_addf(&b_commit_rev, \"%s^1\", revision);\n> +\tstrbuf_addf(&w_tree_rev, \"%s:\", revision);\n> +\tstrbuf_addf(&b_tree_rev, \"%s^1:\", revision);\n> +\tstrbuf_addf(&i_tree_rev, \"%s^2:\", revision);\n> +\n> +\tret = !get_oid(w_commit_rev.buf, &info->w_commit) &&\n> +\t\t!get_oid(b_commit_rev.buf, &info->b_commit) &&\n> +\t\t!get_oid(w_tree_rev.buf, &info->w_tree) &&\n> +\t\t!get_oid(b_tree_rev.buf, &info->b_tree) &&\n> +\t\t!get_oid(i_tree_rev.buf, &info->i_tree);\n> +\n> +\tstrbuf_release(&w_commit_rev);\n> +\tstrbuf_release(&b_commit_rev);\n> +\tstrbuf_release(&w_tree_rev);\n> +\tstrbuf_release(&b_tree_rev);\n> +\tstrbuf_release(&i_tree_rev);\n> +\n> +\tif (!ret)\n> +\t\treturn error(_(\"%s is not a valid reference\"), revision);\n> +\n> +\tstrbuf_addf(&u_tree_rev, \"%s^3:\", revision);\n> +\n> +\tinfo->has_u = !get_oid(u_tree_rev.buf, &info->u_tree);\n> +\n> +\tstrbuf_release(&u_tree_rev);\n> +\n> +\tend_of_rev = strchrnul(revision, '@');\n> +\tstrbuf_add(&symbolic, revision, end_of_rev - revision);\n> +\tcp.git_cmd = 1;\n> +\targv_array_pushl(&cp.args, \"rev-parse\", \"--symbolic-full-name\", NULL);\n> +\targv_array_pushf(&cp.args, \"%s\", symbolic.buf);\n> +\tstrbuf_release(&symbolic);\n> +\tpipe_command(&cp, NULL, 0, &out, 0, NULL, 0);\n> +\n> +\tif (out.len - 1 == strlen(ref_stash))\n> +\t\tinfo->is_stash_ref = !strncmp(out.buf, ref_stash, out.len - 1);\n> +\tstrbuf_release(&out);\n> +\n> +\treturn 0;\n> +}\n> +\n> +static int do_clear_stash(void)\n> +{\n> +\tstruct object_id obj;\n> +\tif (get_oid(ref_stash, &obj))\n> +\t\treturn 0;\n> +\n> +\treturn delete_ref(NULL, ref_stash, &obj, 0);\n> +}\n> +\n> +static int clear_stash(int argc, const char **argv, const char *prefix)\n> +{\n> +\tstruct option options[] = {\n> +\t\tOPT_END()\n> +\t};\n> +\n> +\targc = parse_options(argc, argv, prefix, options, git_stash_clear_usage, PARSE_OPT_STOP_AT_NON_OPTION);\n> +\n> +\tif (argc != 0)\n> +\t\treturn error(_(\"git stash--helper clear with parameters is unimplemented\"));\n> +\n> +\treturn do_clear_stash();\n> +}\n> +\n> +static int reset_tree(struct object_id i_tree, int update, int reset)\n> +{\n> +\tstruct unpack_trees_options opts;\n> +\tint nr_trees = 1;\n> +\tstruct tree_desc t[MAX_UNPACK_TREES];\n> +\tstruct tree *tree;\n> +\tstruct lock_file lock_file = LOCK_INIT;\n> +\n> +\tread_cache_preload(NULL);\n> +\tif (refresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL))\n> +\t\treturn -1;\n> +\n> +\thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n> +\n> +\tmemset(&opts, 0, sizeof(opts));\n> +\n> +\ttree = parse_tree_indirect(&i_tree);\n> +\tif (parse_tree(tree))\n> +\t\treturn -1;\n> +\n> +\tinit_tree_desc(t, tree->buffer, tree->size);\n> +\n> +\topts.head_idx = 1;\n> +\topts.src_index = &the_index;\n> +\topts.dst_index = &the_index;\n> +\topts.merge = 1;\n> +\topts.reset = reset;\n> +\topts.update = update;\n> +\topts.fn = oneway_merge;\n> +\n> +\tif (unpack_trees(nr_trees, t, &opts))\n> +\t\treturn -1;\n> +\n> +\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK)) {\n> +\t\terror(_(\"unable to write new index file\"));\n> +\t\treturn -1;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +static int do_apply_stash(const char *prefix, struct stash_info *info, int index)\n> +{\n> +\tstruct merge_options o;\n> +\tstruct object_id c_tree;\n> +\tstruct object_id index_tree;\n> +\tconst struct object_id *bases[1];\n> +\tint bases_count = 1;\n> +\tstruct commit *result;\n> +\tint ret;\n> +\n> +\tread_cache_preload(NULL);\n> +\tif (refresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL))\n\nIs there any reason not to use the 'refresh_cache()' macro here?  I\nsee quite a few other places that use '&the_index' directly instead of\nusing the '_cache' macros.  I think it would be nicer to use the\n'_cache()' macros for these things unless there's a reason to do\notherwise.\n\n> +\t\treturn -1;\n> +\n> +\tif (write_cache_as_tree(c_tree.hash, 0, NULL))\n> +\t\treturn -1;\n\nWe're loosing the error message here (and in some other places below\nas well).  Instead of just returning -1 here, you could use 'return\nerror(_(\"Cannot apply a stash in the middle of a merge\"))'\n\n> +\n> +\tif (reset_tree(c_tree, 0, 0))\n> +\t\treturn -1;\n> +\n> +\tif (index) {\n> +\t\tif (!hashcmp(info->b_tree.hash, info->i_tree.hash) || !hashcmp(c_tree.hash, info->i_tree.hash)) {\n\nWe're trying to get away from using 'hashcmp' directly, as that's\nstill specific to SHA1.  As all the oids that are compared above are\n'struct object_id's, you can just replace the 'hashcmp' with 'oidcmp'.\n\n> +\t\t\tindex = 0;\n\nIn the original 'apply_stash()' code we keep this parameter around,\nand don't reset it.  I think resetting it might loose us the \"Index\nwas not unstashed.\" error message in some case, although I didn't try\nto actually reproduce that.\n\n> +\t\t} else {\n> +\t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\t\t\tstruct strbuf out = STRBUF_INIT;\n> +\t\t\tstruct argv_array args = ARGV_ARRAY_INIT;\n> +\t\t\tcp.git_cmd = 1;\n> +\t\t\targv_array_pushl(&cp.args, \"diff-tree\", \"--binary\", NULL);\n> +\t\t\targv_array_pushf(&cp.args, \"%s^2^..%s^2\", sha1_to_hex(info->w_commit.hash), sha1_to_hex(info->w_commit.hash));\n> +\t\t\tif (pipe_command(&cp, NULL, 0, &out, 0, NULL, 0))\n> +\t\t\t\treturn -1;\n> +\n> +\t\t\tchild_process_init(&cp);\n> +\t\t\tcp.git_cmd = 1;\n> +\t\t\targv_array_pushl(&cp.args, \"apply\", \"--cached\", NULL);\n> +\t\t\tif (pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0))\n> +\t\t\t\treturn -1;\n> +\n> +\t\t\tstrbuf_release(&out);\n> +\t\t\tdiscard_cache();\n> +\t\t\tread_cache();\n> +\t\t\tif (write_cache_as_tree(index_tree.hash, 0, NULL))\n> +\t\t\t\treturn -1;\n> +\n> +\t\t\targv_array_push(&args, \"reset\");\n> +\t\t\tcmd_reset(args.argc, args.argv, prefix);\n> +\t\t}\n> +\t}\n> +\n> +\tif (info->has_u) {\n> +\t\tstruct argv_array args = ARGV_ARRAY_INIT;\n> +\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\t\tconst char *index_file = get_index_file();\n> +\n> +\t\targv_array_push(&args, \"read-tree\");\n> +\t\targv_array_push(&args, sha1_to_hex(info->u_tree.hash));\n> +\t\targv_array_pushf(&args, \"--index-output=%s\", stash_index_path);\n> +\n> +\t\tcp.git_cmd = 1;\n> +\t\targv_array_pushl(&cp.args, \"checkout-index\", \"--all\", NULL);\n> +\t\targv_array_pushf(&cp.env_array, \"GIT_INDEX_FILE=%s\", stash_index_path);\n\nWe're writing to to the temporary index file here, but I don't think\nwe ever delete it.  After the calls here succeeded, the temporary\nindex should be removed again from the file system.\n\n> +\n> +\t\tif (cmd_read_tree(args.argc, args.argv, prefix) ||\n> +\t\t\t\trun_command(&cp)) {\n> +\t\t\treturn error(_(\"Could not restore untracked files from stash\"));\n> +\t\t}\n> +\t\tset_alternate_index_output(index_file);\n> +\t}\n> +\n> +\tinit_merge_options(&o);\n> +\n> +\to.branch1 = \"Updated upstream\";\n> +\to.branch2 = \"Stashed changes\";\n> +\n> +\tif (!hashcmp(info->b_tree.hash, c_tree.hash))\n> +\t\to.branch1 = \"Version stash was based on\";\n> +\n> +\tif (quiet)\n> +\t\to.verbosity = 0;\n> +\n> +\tif (o.verbosity >= 3)\n> +\t\tprintf_ln(_(\"Merging %s with %s\"), o.branch1, o.branch2);\n> +\n> +\tbases[0] = &info->b_tree;\n> +\n> +\tret = merge_recursive_generic(&o, &c_tree, &info->w_tree, bases_count, bases, &result);\n> +\tif (ret != 0) {\n> +\t\tstruct argv_array args = ARGV_ARRAY_INIT;\n> +\t\targv_array_push(&args, \"rerere\");\n> +\t\tcmd_rerere(args.argc, args.argv, prefix);\n> +\n> +\t\tif (index)\n> +\t\t\tprintf_ln(_(\"Index was not unstashed.\"));\n> +\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tif (index) {\n> +\t\tif (reset_tree(index_tree, 0, 0))\n> +\t\t\treturn -1;\n> +\t} else {\n> +\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\t\tstruct strbuf out = STRBUF_INIT;\n> +\t\tcp.git_cmd = 1;\n> +\t\targv_array_pushl(&cp.args, \"diff-index\", \"--cached\", \"--name-only\", \"--diff-filter=A\", NULL);\n> +\t\targv_array_push(&cp.args, sha1_to_hex(c_tree.hash));\n> +\t\tret = pipe_command(&cp, NULL, 0, &out, 0, NULL, 0);\n> +\t\tif (ret)\n> +\t\t\treturn -1;\n> +\n> +\t\tif (reset_tree(c_tree, 0, 1))\n> +\t\t\treturn -1;\n> +\n> +\t\tchild_process_init(&cp);\n> +\t\tcp.git_cmd = 1;\n> +\t\targv_array_pushl(&cp.args, \"update-index\", \"--add\", \"--stdin\", NULL);\n> +\t\tret = pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0);\n> +\t\tif (ret)\n> +\t\t\treturn -1;\n> +\n> +\t\tstrbuf_release(&out);\n> +\t\tdiscard_cache();\n> +\t\tread_cache();\n\nDo we need to refresh the index here?  I don't think we need it\nanymore after this operation, so just discarding it should be enough.\nAnyone who wants to do something with the index after this function\ncan just call 'read_cache()' again, but at least we won't have to pay\nthe price of reading the index if we just want to apply the stash and\nthen git exits.\n\n> +\t}\n> +\n> +\tif (!quiet) {\n> +\t\tstruct argv_array args = ARGV_ARRAY_INIT;\n> +\t\targv_array_push(&args, \"status\");\n> +\t\tcmd_status(args.argc, args.argv, prefix);\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +static int apply_stash(int argc, const char **argv, const char *prefix)\n> +{\n> +\tconst char *commit = NULL;\n> +\tint index = 0;\n> +\tstruct stash_info info;\n> +\tstruct option options[] = {\n> +\t\tOPT__QUIET(&quiet, N_(\"be quiet, only report errors\")),\n> +\t\tOPT_BOOL(0, \"index\", &index,\n> +\t\t\tN_(\"attempt to ininstate the index\")),\n> +\t\tOPT_END()\n> +\t};\n> +\n> +\targc = parse_options(argc, argv, prefix, options,\n> +\t\t\tgit_stash_apply_usage, 0);\n> +\n> +\tif (argc == 1) {\n> +\t\tcommit = argv[0];\n> +\t}\n> +\n> +\tif (get_stash_info(&info, commit))\n> +\t\treturn -1;\n> +\n> +\n> +\treturn do_apply_stash(prefix, &info, index);\n> +}\n> +\n> +static int do_drop_stash(const char *prefix, struct stash_info *info)\n> +{\n> +\tstruct argv_array args = ARGV_ARRAY_INIT;\n> +\tint ret;\n> +\tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\n> +\targv_array_pushl(&args, \"reflog\", \"delete\", \"--updateref\", \"--rewrite\", NULL);\n> +\targv_array_push(&args, info->revision);\n> +\tret = cmd_reflog(args.argc, args.argv, prefix);\n> +\tif (!ret) {\n> +\t\tif (!quiet) {\n> +\t\t\tprintf(_(\"Dropped %s (%s)\\n\"), info->revision, sha1_to_hex(info->w_commit.hash));\n> +\t\t}\n> +\t} else {\n> +\t\treturn error(_(\"%s: Could not drop stash entry\"), info->revision);\n> +\t}\n> +\n> +\tcp.git_cmd = 1;\n> +\t/* Even though --quiet is specified, rev-parse still outputs the hash */\n> +\tcp.no_stdout = 1;\n> +\targv_array_pushl(&cp.args, \"rev-parse\", \"--verify\", \"--quiet\", NULL);\n> +\targv_array_pushf(&cp.args, \"%s@{0}\", ref_stash);\n> +\tret = run_command(&cp);\n> +\n> +\tif (ret)\n> +\t\tdo_clear_stash();\n> +\n> +\treturn 0;\n> +}\n> +\n> +static int drop_stash(int argc, const char **argv, const char *prefix)\n> +{\n> +\tconst char *commit = NULL;\n> +\tstruct stash_info info;\n> +\tstruct option options[] = {\n> +\t\tOPT__QUIET(&quiet, N_(\"be quiet, only report errors\")),\n> +\t\tOPT_END()\n> +\t};\n> +\n> +\targc = parse_options(argc, argv, prefix, options,\n> +\t\t\tgit_stash_drop_usage, 0);\n> +\n> +\tif (argc == 1)\n> +\t\tcommit = argv[0];\n> +\n> +\tif (get_stash_info(&info, commit))\n> +\t\treturn -1;\n> +\n> +\tif (!info.is_stash_ref)\n> +\t\treturn error(_(\"'%s' is not a stash reference\"), commit);\n> +\n> +\treturn do_drop_stash(prefix, &info);\n> +}\n> +\n> +static int pop_stash(int argc, const char **argv, const char *prefix)\n> +{\n> +\tint index = 0;\n> +\tconst char *commit = NULL;\n> +\tstruct stash_info info;\n> +\tstruct option options] = {\n> +\t\tOPT__QUIET(&quiet, N_(\"be quiet, only report errors\")),\n> +\t\tOPT_BOOL(0, \"index\", &index,\n> +\t\t\tN_(\"attempt to ininstate the index\")),\n> +\t\tOPT_END()\n> +\t};\n> +\n> +\targc = parse_options(argc, argv, prefix, options,\n> +\t\t\tgit_stash_pop_usage, 0);\n> +\n> +\tif (argc == 1)\n> +\t\tcommit = argv[0];\n> +\n> +\tif (get_stash_info(&info, commit))\n> +\t\treturn -1;\n> +\n> +\tif (!info.is_stash_ref)\n> +\t\treturn error(_(\"'%s' is not a stash reference\"), commit);\n> +\n> +\tif (do_apply_stash(prefix, &info, index))\n> +\t\treturn -1;\n> +\n> +\treturn do_drop_stash(prefix, &info);\n> +}\n> +\n> +static int branch_stash(int argc, const char **argv, const char *prefix)\n> +{\n> +\tconst char *commit = NULL, *branch = NULL;\n> +\tint ret;\n> +\tstruct argv_array args = ARGV_ARRAY_INIT;\n> +\tstruct stash_info info;\n> +\tstruct option options[] = {\n> +\t\tOPT_END()\n> +\t};\n> +\n> +\targc = parse_options(argc, argv, prefix, options,\n> +\t\t\tgit_stash_branch_usage, 0);\n> +\n> +\tif (argc != 0) {\n> +\t\tbranch = argv[0];\n> +\t\tif (argc == 2)\n> +\t\t\tcommit = argv[1];\n> +\t}\n> +\n> +\tif (get_stash_info(&info, commit))\n> +\t\treturn -1;\n> +\n> +\targv_array_pushl(&args, \"checkout\", \"-b\", NULL);\n> +\targv_array_push(&args, branch);\n> +\targv_array_push(&args, sha1_to_hex(info.b_commit.hash));\n> +\tret = cmd_checkout(args.argc, args.argv, prefix);\n> +\tif (ret)\n> +\t\treturn -1;\n> +\n> +\tret = do_apply_stash(prefix, &info, 1);\n> +\tif (!ret && info.is_stash_ref)\n> +\t\tret = do_drop_stash(prefix, &info);\n> +\n> +\treturn ret;\n> +}\n> +\n> +int cmd_stash__helper(int argc, const char **argv, const char *prefix)\n> +{\n> +\tint result = 0;\n> +\tpid_t pid = getpid();\n> +\tconst char *index_file;\n> +\n> +\tstruct option options[] = {\n> +\t\tOPT_END()\n> +\t};\n> +\n> +\tgit_config(git_default_config, NULL);\n> +\n> +\targc = parse_options(argc, argv, prefix, options, git_stash_usage,\n> +\t\tPARSE_OPT_KEEP_UNKNOWN|PARSE_OPT_KEEP_DASHDASH);\n> +\n> +\tindex_file = get_index_file();\n> +\txsnprintf(stash_index_path, PATH_MAX, \"%s.stash.%d\", index_file, pid);\n> +\n> +\tif (argc < 1)\n> +\t\tusage_with_options(git_stash_usage, options);\n> +\telse if (!strcmp(argv[0], \"apply\"))\n> +\t\tresult = apply_stash(argc, argv, prefix);\n> +\telse if (!strcmp(argv[0], \"clear\"))\n> +\t\tresult = clear_stash(argc, argv, prefix);\n> +\telse if (!strcmp(argv[0], \"drop\"))\n> +\t\tresult = drop_stash(argc, argv, prefix);\n> +\telse if (!strcmp(argv[0], \"pop\"))\n> +\t\tresult = pop_stash(argc, argv, prefix);\n> +\telse if (!strcmp(argv[0], \"branch\"))\n> +\t\tresult = branch_stash(argc, argv, prefix);\n> +\telse {\n> +\t\terror(_(\"unknown subcommand: %s\"), argv[0]);\n> +\t\tusage_with_options(git_stash_usage, options);\n> +\t\tresult = 1;\n> +\t}\n> +\n> +\treturn result;\n> +}\n> diff --git a/git-stash.sh b/git-stash.sh\n> index 4b7495144..ed2c57027 100755\n> --- a/git-stash.sh\n> +++ b/git-stash.sh\n> @@ -535,129 +535,6 @@ assert_stash_like() {\n>  \t}\n>  }\n>  \n> -is_stash_ref() {\n> -\tis_stash_like \"$@\" && test -n \"$IS_STASH_REF\"\n> -}\n> -\n> -assert_stash_ref() {\n> -\tis_stash_ref \"$@\" || {\n> -\t\targs=\"$*\"\n> -\t\tdie \"$(eval_gettext \"'\\$args' is not a stash reference\")\"\n> -\t}\n> -}\n> -\n> -apply_stash () {\n> -\n> -\tassert_stash_like \"$@\"\n> -\n> -\tgit update-index -q --refresh || die \"$(gettext \"unable to refresh index\")\"\n> -\n> -\t# current index state\n> -\tc_tree=$(git write-tree) ||\n> -\t\tdie \"$(gettext \"Cannot apply a stash in the middle of a merge\")\"\n> -\n> -\tunstashed_index_tree=\n> -\tif test -n \"$INDEX_OPTION\" && test \"$b_tree\" != \"$i_tree\" &&\n> -\t\t\ttest \"$c_tree\" != \"$i_tree\"\n> -\tthen\n> -\t\tgit diff-tree --binary $s^2^..$s^2 | git apply --cached\n> -\t\ttest $? -ne 0 &&\n> -\t\t\tdie \"$(gettext \"Conflicts in index. Try without --index.\")\"\n> -\t\tunstashed_index_tree=$(git write-tree) ||\n> -\t\t\tdie \"$(gettext \"Could not save index tree\")\"\n> -\t\tgit reset\n> -\tfi\n> -\n> -\tif test -n \"$u_tree\"\n> -\tthen\n> -\t\tGIT_INDEX_FILE=\"$TMPindex\" git read-tree \"$u_tree\" &&\n> -\t\tGIT_INDEX_FILE=\"$TMPindex\" git checkout-index --all &&\n> -\t\trm -f \"$TMPindex\" ||\n> -\t\tdie \"$(gettext \"Could not restore untracked files from stash entry\")\"\n> -\tfi\n> -\n> -\teval \"\n> -\t\tGITHEAD_$w_tree='Stashed changes' &&\n> -\t\tGITHEAD_$c_tree='Updated upstream' &&\n> -\t\tGITHEAD_$b_tree='Version stash was based on' &&\n> -\t\texport GITHEAD_$w_tree GITHEAD_$c_tree GITHEAD_$b_tree\n> -\t\"\n> -\n> -\tif test -n \"$GIT_QUIET\"\n> -\tthen\n> -\t\tGIT_MERGE_VERBOSITY=0 && export GIT_MERGE_VERBOSITY\n> -\tfi\n> -\tif git merge-recursive $b_tree -- $c_tree $w_tree\n> -\tthen\n> -\t\t# No conflict\n> -\t\tif test -n \"$unstashed_index_tree\"\n> -\t\tthen\n> -\t\t\tgit read-tree \"$unstashed_index_tree\"\n> -\t\telse\n> -\t\t\ta=\"$TMP-added\" &&\n> -\t\t\tgit diff-index --cached --name-only --diff-filter=A $c_tree >\"$a\" &&\n> -\t\t\tgit read-tree --reset $c_tree &&\n> -\t\t\tgit update-index --add --stdin <\"$a\" ||\n> -\t\t\t\tdie \"$(gettext \"Cannot unstage modified files\")\"\n> -\t\t\trm -f \"$a\"\n> -\t\tfi\n> -\t\tsquelch=\n> -\t\tif test -n \"$GIT_QUIET\"\n> -\t\tthen\n> -\t\t\tsquelch='>/dev/null 2>&1'\n> -\t\tfi\n> -\t\t(cd \"$START_DIR\" && eval \"git status $squelch\") || :\n> -\telse\n> -\t\t# Merge conflict; keep the exit status from merge-recursive\n> -\t\tstatus=$?\n> -\t\tgit rerere\n> -\t\tif test -n \"$INDEX_OPTION\"\n> -\t\tthen\n> -\t\t\tgettextln \"Index was not unstashed.\" >&2\n> -\t\tfi\n> -\t\texit $status\n> -\tfi\n> -}\n> -\n> -pop_stash() {\n> -\tassert_stash_ref \"$@\"\n> -\n> -\tif apply_stash \"$@\"\n> -\tthen\n> -\t\tdrop_stash \"$@\"\n> -\telse\n> -\t\tstatus=$?\n> -\t\tsay \"$(gettext \"The stash entry is kept in case you need it again.\")\"\n> -\t\texit $status\n> -\tfi\n> -}\n> -\n> -drop_stash () {\n> -\tassert_stash_ref \"$@\"\n> -\n> -\tgit reflog delete --updateref --rewrite \"${REV}\" &&\n> -\t\tsay \"$(eval_gettext \"Dropped \\${REV} (\\$s)\")\" ||\n> -\t\tdie \"$(eval_gettext \"\\${REV}: Could not drop stash entry\")\"\n> -\n> -\t# clear_stash if we just dropped the last stash entry\n> -\tgit rev-parse --verify --quiet \"$ref_stash@{0}\" >/dev/null ||\n> -\tclear_stash\n> -}\n> -\n> -apply_to_branch () {\n> -\ttest -n \"$1\" || die \"$(gettext \"No branch name specified\")\"\n> -\tbranch=$1\n> -\tshift 1\n> -\n> -\tset -- --index \"$@\"\n> -\tassert_stash_like \"$@\"\n> -\n> -\tgit checkout -b $branch $REV^ &&\n> -\tapply_stash \"$@\" && {\n> -\t\ttest -z \"$IS_STASH_REF\" || drop_stash \"$@\"\n> -\t}\n> -}\n> -\n>  test \"$1\" = \"-p\" && set \"push\" \"$@\"\n>  \n>  PARSE_CACHE='--not-parsed'\n> @@ -694,7 +571,8 @@ push)\n>  \t;;\n>  apply)\n>  \tshift\n> -\tapply_stash \"$@\"\n> +\tcd \"$START_DIR\"\n> +\tgit stash--helper apply \"$@\"\n>  \t;;\n>  clear)\n>  \tshift\n> @@ -710,15 +588,17 @@ store)\n>  \t;;\n>  drop)\n>  \tshift\n> -\tdrop_stash \"$@\"\n> +\tgit stash--helper drop \"$@\"\n>  \t;;\n>  pop)\n>  \tshift\n> -\tpop_stash \"$@\"\n> +\tcd \"$START_DIR\"\n> +\tgit stash--helper pop \"$@\"\n>  \t;;\n>  branch)\n>  \tshift\n> -\tapply_to_branch \"$@\"\n> +\tcd \"$START_DIR\"\n> +\tgit stash--helper branch \"$@\"\n>  \t;;\n>  *)\n>  \tcase $# in\n> diff --git a/git.c b/git.c\n> index 9e96dd409..868727dc3 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -466,6 +466,7 @@ static struct cmd_struct commands[] = {\n>  \t{ \"show-branch\", cmd_show_branch, RUN_SETUP },\n>  \t{ \"show-ref\", cmd_show_ref, RUN_SETUP },\n>  \t{ \"stage\", cmd_add, RUN_SETUP | NEED_WORK_TREE },\n> +\t{ \"stash--helper\", cmd_stash__helper, RUN_SETUP | NEED_WORK_TREE },\n>  \t{ \"status\", cmd_status, RUN_SETUP | NEED_WORK_TREE },\n>  \t{ \"stripspace\", cmd_stripspace },\n>  \t{ \"submodule--helper\", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX},\n> -- \n> 2.15.0\n> \n"},{"id":"332350","messageId":"CA+CzEk_0XQoncz6sW3dQPCdxRCgo=BFHxJWr-MQSLxkVPJz8eg@mail.gmail.com","threadId":"47164","inReplyTo":"20171112152741.GA15767@hank.wfi.bloomberg.com","subject":"Re: [PATCH 2/2] stash: implement builtin stash helper","fromName":"Joel Teichroeb","fromEmail":"joel@teichroeb.net","sentAt":"2017-11-12T17:21:52Z","receivedAt":"2017-11-12T17:22:18Z","isPatch":true,"sender":{"key":"joel@teichroeb.net","avatar":"https://avatars.githubusercontent.com/u/240865?v=4"},"body":"Thanks for your comments! I've incorporated them all for the next patch set.\n"},{"id":"332448","messageId":"CAN0heSrFdGQ9469ugQwmjP5ieoxb5NCR3Kpz6ZQ4J5TyarwQ6w@mail.gmail.com","threadId":"47164","inReplyTo":"20171110231314.30711-2-joel@teichroeb.net","subject":"Re: [PATCH 1/2] merge: close the index lock when not writing the new index","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-11-13T21:12:35Z","receivedAt":"2017-11-13T21:12:41Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 11 November 2017 at 00:13, Joel Teichroeb <joel@teichroeb.net> wrote:\n> If the merge does not have anything to do, it does not unlock the index,\n> causing any further index operations to fail. Thus, always unlock the index\n> regardless of outcome.\n\n>         if (clean < 0)\n>                 return clean;\n\nDo we need to roll back the lock also if `clean` is negative? The\ncurrent callers are built-ins which will error out, but future callers\nmight be caught off guard by this.\n\n> -       if (active_cache_changed &&\n> -           write_locked_index(&the_index, &lock, COMMIT_LOCK))\n> -               return err(o, _(\"Unable to write index.\"));\n> +       if (active_cache_changed) {\n> +               if (write_locked_index(&the_index, &lock, COMMIT_LOCK))\n> +                       return err(o, _(\"Unable to write index.\"));\n> +       } else {\n> +               rollback_lock_file(&lock);\n> +       }\n>\n>         return clean ? 0 : 1;\n>  }\n\nLooks correct. A simpler change which would still match the commit\nmessage would be to unconditionally call `rollback_lock_file()` just\nbefore returning. That would perhaps be slightly more future-proof,\nsince it will always leave the lock unlocked, even if the if-else grows\nmore complicated.\n\nWell, \"always\" modulo returning early and forgetting to roll back the\nlock. ;-) Looking at existing code, it's not obvious which way we should\nprefer. Just a thought.\n\nThanks for spotting this. I was poking around here recently, but failed\nto notice this lax lock-handling.\n\nMartin\n"}]}