{"thread":{"id":"48132","subject":"[PATCH 0/4] Convert some stash functionality to a builtin","startedAt":"2018-03-24T17:37:35Z","lastAt":"2018-03-28T03:31:03Z","messageCount":25,"participants":["Joel Teichroeb","Christian Couder","Eric Sunshine","Thomas Gummerer","Ævar Arnfjörð Bjarmason","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"342826","messageId":"20180324173707.17699-1-joel@teichroeb.net","threadId":"48132","inReplyTo":null,"subject":"[PATCH 0/4] Convert some stash functionality to a builtin","fromName":"Joel Teichroeb","fromEmail":"joel@teichroeb.net","sentAt":"2018-03-24T17:37:03Z","receivedAt":"2018-03-24T17:37:35Z","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\nSince there seems to be interest from GSOC students who want to\nwork on converting builtins, I figured I should finish what I\nhave that works now so they could build on top of it.\n\nJoel Teichroeb (4):\n  stash: convert apply to builtin\n  stash: convert branch to builtin\n  stash: convert drop and clear to builtin\n  stash: convert pop to builtin\n\n .gitignore              |   1 +\n Makefile                |   1 +\n builtin.h               |   1 +\n builtin/stash--helper.c | 514 ++++++++++++++++++++++++++++++++++++++++++++++++\n git-stash.sh            |  13 +-\n git.c                   |   1 +\n 6 files changed, 526 insertions(+), 5 deletions(-)\n create mode 100644 builtin/stash--helper.c\n\n-- \n2.16.2\n\n"},{"id":"342827","messageId":"20180324173707.17699-2-joel@teichroeb.net","threadId":"48132","inReplyTo":"20180324173707.17699-1-joel@teichroeb.net","subject":"[PATCH 1/4] stash: convert apply to builtin","fromName":"Joel Teichroeb","fromEmail":"joel@teichroeb.net","sentAt":"2018-03-24T17:37:04Z","receivedAt":"2018-03-24T17:37:41Z","isPatch":true,"sender":{"key":"joel@teichroeb.net","avatar":"https://avatars.githubusercontent.com/u/240865?v=4"},"body":"---\n .gitignore              |   1 +\n Makefile                |   1 +\n builtin.h               |   1 +\n builtin/stash--helper.c | 339 ++++++++++++++++++++++++++++++++++++++++++++++++\n git-stash.sh            |   3 +-\n git.c                   |   1 +\n 6 files changed, 345 insertions(+), 1 deletion(-)\n create mode 100644 builtin/stash--helper.c\n\ndiff --git a/.gitignore b/.gitignore\nindex 833ef3b0b7..296d5f376d 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -152,6 +152,7 @@\n /git-show-ref\n /git-stage\n /git-stash\n+/git-stash--helper\n /git-status\n /git-stripspace\n /git-submodule\ndiff --git a/Makefile b/Makefile\nindex a1d8775adb..8ca361c57a 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1020,6 +1020,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 42378f3aa4..a14fd85b0e 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 0000000000..e9a9574f40\n--- /dev/null\n+++ b/builtin/stash--helper.c\n@@ -0,0 +1,339 @@\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+#include \"dir.h\"\n+\n+static const char * const git_stash_helper_usage[] = {\n+\tN_(\"git stash--helper apply [--index] [-q|--quiet] [<stash>]\"),\n+\tNULL\n+};\n+\n+static const char * const git_stash_helper_apply_usage[] = {\n+\tN_(\"git stash--helper apply [--index] [-q|--quiet] [<stash>]\"),\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 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_cache(REFRESH_QUIET))\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+\tint has_index = index;\n+\n+\tread_cache_preload(NULL);\n+\tif (refresh_cache(REFRESH_QUIET))\n+\t\treturn -1;\n+\n+\tif (write_cache_as_tree(c_tree.hash, 0, NULL) || reset_tree(c_tree, 0, 0))\n+\t\treturn error(_(\"Cannot apply a stash in the middle of a merge\"));\n+\n+\tif (index) {\n+\t\tif (!oidcmp(&info->b_tree, &info->i_tree) || !oidcmp(&c_tree, &info->i_tree)) {\n+\t\t\thas_index = 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 child_process cp = CHILD_PROCESS_INIT;\n+\t\tstruct child_process cp2 = CHILD_PROCESS_INIT;\n+\t\tint res;\n+\n+\t\tcp.git_cmd = 1;\n+\t\targv_array_push(&cp.args, \"read-tree\");\n+\t\targv_array_push(&cp.args, sha1_to_hex(info->u_tree.hash));\n+\t\targv_array_pushf(&cp.env_array, \"GIT_INDEX_FILE=%s\", stash_index_path);\n+\n+\t\tcp2.git_cmd = 1;\n+\t\targv_array_pushl(&cp2.args, \"checkout-index\", \"--all\", NULL);\n+\t\targv_array_pushf(&cp2.env_array, \"GIT_INDEX_FILE=%s\", stash_index_path);\n+\n+\t\tres = run_command(&cp) || run_command(&cp2);\n+\t\tremove_path(stash_index_path);\n+\t\tif (res)\n+\t\t\treturn error(_(\"Could not restore untracked files from stash\"));\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 (has_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}\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_helper_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+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_helper_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_helper_usage, options);\n+\telse if (!strcmp(argv[0], \"apply\"))\n+\t\tresult = apply_stash(argc, argv, prefix);\n+\telse {\n+\t\terror(_(\"unknown subcommand: %s\"), argv[0]);\n+\t\tusage_with_options(git_stash_helper_usage, options);\n+\t\tresult = 1;\n+\t}\n+\n+\treturn result;\n+}\ndiff --git a/git-stash.sh b/git-stash.sh\nindex fc8f8ae640..92c084eb17 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -711,7 +711,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\ndiff --git a/git.c b/git.c\nindex ceaa58ef40..6ffe6364ac 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.16.2\n\n"},{"id":"342828","messageId":"20180324173707.17699-3-joel@teichroeb.net","threadId":"48132","inReplyTo":"20180324173707.17699-1-joel@teichroeb.net","subject":"[PATCH 2/4] stash: convert branch to builtin","fromName":"Joel Teichroeb","fromEmail":"joel@teichroeb.net","sentAt":"2018-03-24T17:37:05Z","receivedAt":"2018-03-24T17:37:44Z","isPatch":true,"sender":{"key":"joel@teichroeb.net","avatar":"https://avatars.githubusercontent.com/u/240865?v=4"},"body":"---\n builtin/stash--helper.c | 44 ++++++++++++++++++++++++++++++++++++++++++++\n git-stash.sh            |  3 ++-\n 2 files changed, 46 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/stash--helper.c b/builtin/stash--helper.c\nindex e9a9574f40..18c4aba665 100644\n--- a/builtin/stash--helper.c\n+++ b/builtin/stash--helper.c\n@@ -12,6 +12,7 @@\n \n static const char * const git_stash_helper_usage[] = {\n \tN_(\"git stash--helper apply [--index] [-q|--quiet] [<stash>]\"),\n+\tN_(\"git stash--helper branch <branchname> [<stash>]\"),\n \tNULL\n };\n \n@@ -20,6 +21,11 @@ static const char * const git_stash_helper_apply_usage[] = {\n \tNULL\n };\n \n+static const char * const git_stash_helper_branch_usage[] = {\n+\tN_(\"git stash--helper branch <branchname> [<stash>]\"),\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@@ -307,6 +313,42 @@ static int apply_stash(int argc, const char **argv, const char *prefix)\n \treturn do_apply_stash(prefix, &info, index);\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_helper_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@@ -329,6 +371,8 @@ int cmd_stash__helper(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(git_stash_helper_usage, options);\n \telse if (!strcmp(argv[0], \"apply\"))\n \t\tresult = apply_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_helper_usage, options);\ndiff --git a/git-stash.sh b/git-stash.sh\nindex 92c084eb17..360643ad4e 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -736,7 +736,8 @@ 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-- \n2.16.2\n\n"},{"id":"342829","messageId":"20180324173707.17699-4-joel@teichroeb.net","threadId":"48132","inReplyTo":"20180324173707.17699-1-joel@teichroeb.net","subject":"[PATCH 3/4] stash: convert drop and clear to builtin","fromName":"Joel Teichroeb","fromEmail":"joel@teichroeb.net","sentAt":"2018-03-24T17:37:06Z","receivedAt":"2018-03-24T17:37:48Z","isPatch":true,"sender":{"key":"joel@teichroeb.net","avatar":"https://avatars.githubusercontent.com/u/240865?v=4"},"body":"---\n builtin/stash--helper.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++\n git-stash.sh            |  4 +--\n 2 files changed, 95 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/stash--helper.c b/builtin/stash--helper.c\nindex 18c4aba665..1598b82ac2 100644\n--- a/builtin/stash--helper.c\n+++ b/builtin/stash--helper.c\n@@ -11,8 +11,15 @@\n #include \"dir.h\"\n \n static const char * const git_stash_helper_usage[] = {\n+\tN_(\"git stash--helper drop [-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_helper_drop_usage[] = {\n+\tN_(\"git stash--helper drop [-q|--quiet] [<stash>]\"),\n \tNULL\n };\n \n@@ -26,6 +33,11 @@ static const char * const git_stash_helper_branch_usage[] = {\n \tNULL\n };\n \n+static const char * const git_stash_helper_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@@ -114,6 +126,29 @@ static int get_stash_info(struct stash_info *info, const char *commit)\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_helper_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@@ -313,6 +348,60 @@ static int apply_stash(int argc, const char **argv, const char *prefix)\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_helper_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 branch_stash(int argc, const char **argv, const char *prefix)\n {\n \tconst char *commit = NULL, *branch = NULL;\n@@ -371,6 +460,10 @@ int cmd_stash__helper(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(git_stash_helper_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], \"branch\"))\n \t\tresult = branch_stash(argc, argv, prefix);\n \telse {\ndiff --git a/git-stash.sh b/git-stash.sh\nindex 360643ad4e..54d0a6c21f 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -716,7 +716,7 @@ apply)\n \t;;\n clear)\n \tshift\n-\tclear_stash \"$@\"\n+\tgit stash--helper clear \"$@\"\n \t;;\n create)\n \tshift\n@@ -728,7 +728,7 @@ store)\n \t;;\n drop)\n \tshift\n-\tdrop_stash \"$@\"\n+\tgit stash--helper drop \"$@\"\n \t;;\n pop)\n \tshift\n-- \n2.16.2\n\n"},{"id":"342830","messageId":"20180324173707.17699-5-joel@teichroeb.net","threadId":"48132","inReplyTo":"20180324173707.17699-1-joel@teichroeb.net","subject":"[PATCH 4/4] stash: convert pop to builtin","fromName":"Joel Teichroeb","fromEmail":"joel@teichroeb.net","sentAt":"2018-03-24T17:37:07Z","receivedAt":"2018-03-24T17:37:51Z","isPatch":true,"sender":{"key":"joel@teichroeb.net","avatar":"https://avatars.githubusercontent.com/u/240865?v=4"},"body":"---\n builtin/stash--helper.c | 38 ++++++++++++++++++++++++++++++++++++++\n git-stash.sh            |  3 ++-\n 2 files changed, 40 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/stash--helper.c b/builtin/stash--helper.c\nindex 1598b82ac2..b912f84c97 100644\n--- a/builtin/stash--helper.c\n+++ b/builtin/stash--helper.c\n@@ -12,6 +12,7 @@\n \n static const char * const git_stash_helper_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@@ -23,6 +24,11 @@ static const char * const git_stash_helper_drop_usage[] = {\n \tNULL\n };\n \n+static const char * const git_stash_helper_pop_usage[] = {\n+\tN_(\"git stash--helper pop [--index] [-q|--quiet] [<stash>]\"),\n+\tNULL\n+};\n+\n static const char * const git_stash_helper_apply_usage[] = {\n \tN_(\"git stash--helper apply [--index] [-q|--quiet] [<stash>]\"),\n \tNULL\n@@ -402,6 +408,36 @@ static int drop_stash(int argc, const char **argv, const char *prefix)\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_helper_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@@ -464,6 +500,8 @@ int cmd_stash__helper(int argc, const char **argv, const char *prefix)\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 {\ndiff --git a/git-stash.sh b/git-stash.sh\nindex 54d0a6c21f..d595bbaf64 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -732,7 +732,8 @@ 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-- \n2.16.2\n\n"},{"id":"342832","messageId":"CAP8UFD37DgfQ63pPrN2CBH0VjTUuc-N4fq-34bTuyQTH0fR9mA@mail.gmail.com","threadId":"48132","inReplyTo":"20180324173707.17699-2-joel@teichroeb.net","subject":"Re: [PATCH 1/4] stash: convert apply to builtin","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-03-24T18:19:42Z","receivedAt":"2018-03-24T18:19:50Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"> +       if (unpack_trees(nr_trees, t, &opts))\n> +               return -1;\n> +\n> +       if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK)) {\n> +               error(_(\"unable to write new index file\"));\n> +               return -1;\n\nMaybe: return error(...);\n\n> +       }\n> +\n> +       return 0;\n> +}\n\n[...]\n\n> +       argc = parse_options(argc, argv, prefix, options,\n> +                       git_stash_helper_apply_usage, 0);\n> +\n> +       if (argc == 1) {\n> +               commit = argv[0];\n> +       }\n\nThe brackets are not needed.\n\n> +       if (get_stash_info(&info, commit))\n> +               return -1;\n> +\n> +\n\nSpurious new line.\n\n> +       return do_apply_stash(prefix, &info, index);\n> +}\n"},{"id":"342833","messageId":"CAP8UFD2yudVPNya8sTTd5UUq7Doxp2VqSf+aCecJPHE_c1VoqA@mail.gmail.com","threadId":"48132","inReplyTo":"20180324173707.17699-4-joel@teichroeb.net","subject":"Re: [PATCH 3/4] stash: convert drop and clear to builtin","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-03-24T18:22:56Z","receivedAt":"2018-03-24T18:23:03Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"> +       argv_array_pushl(&args, \"reflog\", \"delete\", \"--updateref\", \"--rewrite\", NULL);\n> +       argv_array_push(&args, info->revision);\n> +       ret = cmd_reflog(args.argc, args.argv, prefix);\n> +       if (!ret) {\n> +               if (!quiet) {\n> +                       printf(_(\"Dropped %s (%s)\\n\"), info->revision, sha1_to_hex(info->w_commit.hash));\n> +               }\n\nThe brackets are not needed.\n\n> +       } else {\n> +               return error(_(\"%s: Could not drop stash entry\"), info->revision);\n> +       }\n"},{"id":"342866","messageId":"CAPig+cSkQLSvOroB0bLLLBAXy9UBDN+s=i97COtNDpO0FbLJkg@mail.gmail.com","threadId":"48132","inReplyTo":"20180324173707.17699-2-joel@teichroeb.net","subject":"Re: [PATCH 1/4] stash: convert apply to builtin","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-25T06:40:43Z","receivedAt":"2018-03-25T06:40:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Mar 24, 2018 at 1:37 PM, Joel Teichroeb <joel@teichroeb.net> wrote:\n> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c\n> @@ -0,0 +1,339 @@\n> +static int get_stash_info(struct stash_info *info, const char *commit)\n> +{\n> +       struct strbuf w_commit_rev = STRBUF_INIT;\n> +       struct strbuf b_commit_rev = STRBUF_INIT;\n> +       struct strbuf w_tree_rev = STRBUF_INIT;\n> +       struct strbuf b_tree_rev = STRBUF_INIT;\n> +       struct strbuf i_tree_rev = STRBUF_INIT;\n> +       struct strbuf u_tree_rev = STRBUF_INIT;\n> +       struct strbuf commit_buf = STRBUF_INIT;\n> +       struct strbuf symbolic = STRBUF_INIT;\n> +       struct strbuf out = STRBUF_INIT;\n\n'commit_buf' is being leaked. All the others seem to be covered (even\nin the case of early 'return').\n\n> +       if (commit == NULL) {\n> +               strbuf_addf(&commit_buf, \"%s@{0}\", ref_stash);\n> +               revision = commit_buf.buf;\n> +       } else if (strspn(commit, \"0123456789\") == strlen(commit)) {\n> +               strbuf_addf(&commit_buf, \"%s@{%s}\", ref_stash, commit);\n> +               revision = commit_buf.buf;\n> +       }\n> +static int do_apply_stash(const char *prefix, struct stash_info *info, int index)\n> +{\n> +       if (index) {\n> +               if (!oidcmp(&info->b_tree, &info->i_tree) || !oidcmp(&c_tree, &info->i_tree)) {\n> +                       has_index = 0;\n> +               } else {\n> +                       struct child_process cp = CHILD_PROCESS_INIT;\n> +                       struct strbuf out = STRBUF_INIT;\n> +                       struct argv_array args = ARGV_ARRAY_INIT;\n> +                       cp.git_cmd = 1;\n> +                       argv_array_pushl(&cp.args, \"diff-tree\", \"--binary\", NULL);\n> +                       argv_array_pushf(&cp.args, \"%s^2^..%s^2\", sha1_to_hex(info->w_commit.hash), sha1_to_hex(info->w_commit.hash));\n> +                       if (pipe_command(&cp, NULL, 0, &out, 0, NULL, 0))\n> +                               return -1;\n\nLeaking 'out'?\n\n> +\n> +                       child_process_init(&cp);\n> +                       cp.git_cmd = 1;\n> +                       argv_array_pushl(&cp.args, \"apply\", \"--cached\", NULL);\n> +                       if (pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0))\n> +                               return -1;\n\nLeaking 'out'.\n\n> +\n> +                       strbuf_release(&out);\n> +                       discard_cache();\n> +                       read_cache();\n> +                       if (write_cache_as_tree(index_tree.hash, 0, NULL))\n> +                               return -1;\n> +\n> +                       argv_array_push(&args, \"reset\");\n> +                       cmd_reset(args.argc, args.argv, prefix);\n> +               }\n> +       }\n> +       if (has_index) {\n> +               if (reset_tree(index_tree, 0, 0))\n> +                       return -1;\n> +       } else {\n> +               struct child_process cp = CHILD_PROCESS_INIT;\n> +               struct strbuf out = STRBUF_INIT;\n> +               cp.git_cmd = 1;\n> +               argv_array_pushl(&cp.args, \"diff-index\", \"--cached\", \"--name-only\", \"--diff-filter=A\", NULL);\n> +               argv_array_push(&cp.args, sha1_to_hex(c_tree.hash));\n> +               ret = pipe_command(&cp, NULL, 0, &out, 0, NULL, 0);\n> +               if (ret)\n> +                       return -1;\n> +\n> +               if (reset_tree(c_tree, 0, 1))\n> +                       return -1;\n\nLeaking 'out' at these two 'return's?\n\n> +               child_process_init(&cp);\n> +               cp.git_cmd = 1;\n> +               argv_array_pushl(&cp.args, \"update-index\", \"--add\", \"--stdin\", NULL);\n> +               ret = pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0);\n> +               if (ret)\n> +                       return -1;\n\nAnd here.\n\n> +\n> +               strbuf_release(&out);\n> +               discard_cache();\n> +       }\n> +\n> +       if (!quiet) {\n> +               struct argv_array args = ARGV_ARRAY_INIT;\n> +               argv_array_push(&args, \"status\");\n> +               cmd_status(args.argc, args.argv, prefix);\n> +       }\n> +\n> +       return 0;\n> +}\n> +\n> +static int apply_stash(int argc, const char **argv, const char *prefix)\n> +{\n> +       const char *commit = NULL;\n> +       int index = 0;\n> +       struct stash_info info;\n> +       struct option options[] = {\n> +               OPT__QUIET(&quiet, N_(\"be quiet, only report errors\")),\n> +               OPT_BOOL(0, \"index\", &index,\n> +                       N_(\"attempt to ininstate the index\")),\n\n\"ininstate\"??\n\n> +               OPT_END()\n> +       };\n> +\n> +       argc = parse_options(argc, argv, prefix, options,\n> +                       git_stash_helper_apply_usage, 0);\n> +\n> +       if (argc == 1) {\n> +               commit = argv[0];\n> +       }\n> +\n> +       if (get_stash_info(&info, commit))\n> +               return -1;\n> +\n> +\n> +       return do_apply_stash(prefix, &info, index);\n> +}\n"},{"id":"342867","messageId":"CAPig+cRyoKZN9osXXXuqTVXn27twLy--BXsHE1jLqKqXJ6DwAA@mail.gmail.com","threadId":"48132","inReplyTo":"20180324173707.17699-3-joel@teichroeb.net","subject":"Re: [PATCH 2/4] stash: convert branch to builtin","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-25T06:44:52Z","receivedAt":"2018-03-25T06:44:57Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Mar 24, 2018 at 1:37 PM, Joel Teichroeb <joel@teichroeb.net> wrote:\n> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c\n> @@ -307,6 +313,42 @@ static int apply_stash(int argc, const char **argv, const char *prefix)\n> +static int branch_stash(int argc, const char **argv, const char *prefix)\n> +{\n> +       const char *commit = NULL, *branch = NULL;\n> +\n> +       argc = parse_options(argc, argv, prefix, options,\n> +                       git_stash_helper_branch_usage, 0);\n> +\n> +       if (argc != 0) {\n> +               branch = argv[0];\n> +               if (argc == 2)\n> +                       commit = argv[1];\n> +       }\n\nThis seems fragile. What happens if there are three args?\n\n> +       if (get_stash_info(&info, commit))\n> +               return -1;\n> +\n> +       argv_array_pushl(&args, \"checkout\", \"-b\", NULL);\n> +       argv_array_push(&args, branch);\n> +       argv_array_push(&args, sha1_to_hex(info.b_commit.hash));\n> +       ret = cmd_checkout(args.argc, args.argv, prefix);\n> +       if (ret)\n> +               return -1;\n> +\n> +       ret = do_apply_stash(prefix, &info, 1);\n> +       if (!ret && info.is_stash_ref)\n> +               ret = do_drop_stash(prefix, &info);\n> +\n> +       return ret;\n> +}\n"},{"id":"342868","messageId":"CAPig+cSC93bEoUZBtg3b+U_=3O+D2T1-0x-mH2LykyMsM2SROg@mail.gmail.com","threadId":"48132","inReplyTo":"20180324173707.17699-4-joel@teichroeb.net","subject":"Re: [PATCH 3/4] stash: convert drop and clear to builtin","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-25T06:49:24Z","receivedAt":"2018-03-25T06:49:29Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Mar 24, 2018 at 1:37 PM, Joel Teichroeb <joel@teichroeb.net> wrote:\n> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c\n> @@ -313,6 +348,60 @@ static int apply_stash(int argc, const char **argv, const char *prefix)\n> +static int drop_stash(int argc, const char **argv, const char *prefix)\n> +{\n> +       const char *commit = NULL;\n> +       struct stash_info info;\n> +       struct option options[] = {\n> +               OPT__QUIET(&quiet, N_(\"be quiet, only report errors\")),\n> +               OPT_END()\n> +       };\n> +\n> +       argc = parse_options(argc, argv, prefix, options,\n> +                       git_stash_helper_drop_usage, 0);\n> +\n> +       if (argc == 1)\n> +               commit = argv[0];\n\nSeems fragile. What if there are two arguments?\n\n> +       if (get_stash_info(&info, commit))\n> +               return -1;\n> +\n> +       if (!info.is_stash_ref)\n> +               return error(_(\"'%s' is not a stash reference\"), commit);\n> +\n> +       return do_drop_stash(prefix, &info);\n> +}\n"},{"id":"342869","messageId":"CAPig+cQ7XdEMyL=mT4RGfnsz-fjs6RxG=ztGC3oLprxKUBnJoA@mail.gmail.com","threadId":"48132","inReplyTo":"20180324173707.17699-5-joel@teichroeb.net","subject":"Re: [PATCH 4/4] stash: convert pop to builtin","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-25T06:51:28Z","receivedAt":"2018-03-25T06:51:33Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Mar 24, 2018 at 1:37 PM, Joel Teichroeb <joel@teichroeb.net> wrote:\n> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c\n> @@ -402,6 +408,36 @@ static int drop_stash(int argc, const char **argv, const char *prefix)\n> +static int pop_stash(int argc, const char **argv, const char *prefix)\n> +{\n> +       int index = 0;\n> +       const char *commit = NULL;\n> +       struct stash_info info;\n> +       struct option options[] = {\n> +               OPT__QUIET(&quiet, N_(\"be quiet, only report errors\")),\n> +               OPT_BOOL(0, \"index\", &index,\n> +                       N_(\"attempt to ininstate the index\")),\n\n\"ininstate\"??\n\n> +               OPT_END()\n> +       };\n> +\n> +       argc = parse_options(argc, argv, prefix, options,\n> +                       git_stash_helper_pop_usage, 0);\n> +\n> +       if (argc == 1)\n> +               commit = argv[0];\n\nSeems fragile. What if there are two arguments?\n\n> +       if (get_stash_info(&info, commit))\n> +               return -1;\n> +\n> +       if (!info.is_stash_ref)\n> +               return error(_(\"'%s' is not a stash reference\"), commit);\n> +\n> +       if (do_apply_stash(prefix, &info, index))\n> +               return -1;\n> +\n> +       return do_drop_stash(prefix, &info);\n> +}\n"},{"id":"342875","messageId":"CAP8UFD3Qxt2YMqTtHwU8n7EDvD66QjGSywRQoxJDnncv7=2BUg@mail.gmail.com","threadId":"48132","inReplyTo":"20180324173707.17699-2-joel@teichroeb.net","subject":"Re: [PATCH 1/4] stash: convert apply to builtin","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-03-25T08:09:35Z","receivedAt":"2018-03-25T08:09:46Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sat, Mar 24, 2018 at 6:37 PM, Joel Teichroeb <joel@teichroeb.net> wrote:\n> diff --git a/git-stash.sh b/git-stash.sh\n> index fc8f8ae640..92c084eb17 100755\n> --- a/git-stash.sh\n> +++ b/git-stash.sh\n> @@ -711,7 +711,8 @@ push)\n>         ;;\n>  apply)\n>         shift\n> -       apply_stash \"$@\"\n> +       cd \"$START_DIR\"\n> +       git stash--helper apply \"$@\"\n>         ;;\n>  clear)\n>         shift\n\nIt seems to me that the apply_stash() shell function is also used in\npop_stash() and in apply_to_branch(). Can the new helper be used there\ntoo instead of apply_stash()? And then could apply_stash() be remove?\n"},{"id":"342876","messageId":"CAP8UFD0Rmyiom0pEY6u7OVzCZbs9rg3VjEsW6Rs8S6m40uKrPw@mail.gmail.com","threadId":"48132","inReplyTo":"20180324173707.17699-3-joel@teichroeb.net","subject":"Re: [PATCH 2/4] stash: convert branch to builtin","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-03-25T08:22:58Z","receivedAt":"2018-03-25T08:23:08Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sat, Mar 24, 2018 at 6:37 PM, Joel Teichroeb <joel@teichroeb.net> wrote:\n> diff --git a/git-stash.sh b/git-stash.sh\n> index 92c084eb17..360643ad4e 100755\n> --- a/git-stash.sh\n> +++ b/git-stash.sh\n> @@ -736,7 +736,8 @@ pop)\n>         ;;\n>  branch)\n>         shift\n> -       apply_to_branch \"$@\"\n> +       cd \"$START_DIR\"\n> +       git stash--helper branch \"$@\"\n>         ;;\n>  *)\n>         case $# in\n\nCan the apply_to_branch() shell function be removed from git-stash.sh?\n"},{"id":"342878","messageId":"CAP8UFD1f=VM7VLWGPOzn5PSwHr_m2PohP0gOCM6wk7FdoZ3-Yg@mail.gmail.com","threadId":"48132","inReplyTo":"CAPig+cSkQLSvOroB0bLLLBAXy9UBDN+s=i97COtNDpO0FbLJkg@mail.gmail.com","subject":"Re: [PATCH 1/4] stash: convert apply to builtin","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-03-25T09:27:41Z","receivedAt":"2018-03-25T09:27:49Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sun, Mar 25, 2018 at 8:40 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Sat, Mar 24, 2018 at 1:37 PM, Joel Teichroeb <joel@teichroeb.net> wrote:\n\n>> +static int do_apply_stash(const char *prefix, struct stash_info *info, int index)\n>> +{\n>> +       if (index) {\n>> +               if (!oidcmp(&info->b_tree, &info->i_tree) || !oidcmp(&c_tree, &info->i_tree)) {\n>> +                       has_index = 0;\n>> +               } else {\n>> +                       struct child_process cp = CHILD_PROCESS_INIT;\n>> +                       struct strbuf out = STRBUF_INIT;\n>> +                       struct argv_array args = ARGV_ARRAY_INIT;\n>> +                       cp.git_cmd = 1;\n>> +                       argv_array_pushl(&cp.args, \"diff-tree\", \"--binary\", NULL);\n>> +                       argv_array_pushf(&cp.args, \"%s^2^..%s^2\", sha1_to_hex(info->w_commit.hash), sha1_to_hex(info->w_commit.hash));\n>> +                       if (pipe_command(&cp, NULL, 0, &out, 0, NULL, 0))\n>> +                               return -1;\n>\n> Leaking 'out'?\n>\n>> +\n>> +                       child_process_init(&cp);\n>> +                       cp.git_cmd = 1;\n>> +                       argv_array_pushl(&cp.args, \"apply\", \"--cached\", NULL);\n>> +                       if (pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0))\n>> +                               return -1;\n>\n> Leaking 'out'.\n\nIt might be a good idea to have small functions encapsulating the\nforks of git commands. For example:\n\nstatic int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)\n{\n    struct child_process cp = CHILD_PROCESS_INIT;\n    const char *w_commit_hex = oid_to_hex(w_commit);\n\n    cp.git_cmd = 1;\n    argv_array_pushl(&cp.args, \"diff-tree\", \"--binary\", NULL);\n    argv_array_pushf(&cp.args, \"%s^2^..%s^2\", w_commit_hex, w_commit_hex);\n\n    return pipe_command(&cp, NULL, 0, out, 0, NULL, 0);\n}\n\nstatic int apply_cached(struct strbuf *out)\n{\n    struct child_process cp = CHILD_PROCESS_INIT;\n\n    cp.git_cmd = 1;\n    argv_array_pushl(&cp.args, \"apply\", \"--cached\", NULL);\n    return pipe_command(&cp, out->buf, out->len, NULL, 0, NULL, 0);\n}\n\nThis could help find leaks and maybe later make it easier to call\nlibified code instead of forking a git command.\n"},{"id":"342898","messageId":"20180325164300.GA10909@hank","threadId":"48132","inReplyTo":"20180324173707.17699-2-joel@teichroeb.net","subject":"Re: [PATCH 1/4] stash: convert apply to builtin","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2018-03-25T16:43:00Z","receivedAt":"2018-03-25T16:39:42Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 03/24, Joel Teichroeb wrote:\n> ---\n\nMissing sign-off?  I saw it's missing in the other patches as well. \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> +\tint has_index = index;\n> +\n> +\tread_cache_preload(NULL);\n> +\tif (refresh_cache(REFRESH_QUIET))\n> +\t\treturn -1;\n> +\n> +\tif (write_cache_as_tree(c_tree.hash, 0, NULL) || reset_tree(c_tree, 0, 0))\n> +\t\treturn error(_(\"Cannot apply a stash in the middle of a merge\"));\n> +\n> +\tif (index) {\n> +\t\tif (!oidcmp(&info->b_tree, &info->i_tree) || !oidcmp(&c_tree, &info->i_tree)) {\n> +\t\t\thas_index = 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 child_process cp = CHILD_PROCESS_INIT;\n> +\t\tstruct child_process cp2 = CHILD_PROCESS_INIT;\n> +\t\tint res;\n> +\n> +\t\tcp.git_cmd = 1;\n> +\t\targv_array_push(&cp.args, \"read-tree\");\n> +\t\targv_array_push(&cp.args, sha1_to_hex(info->u_tree.hash));\n> +\t\targv_array_pushf(&cp.env_array, \"GIT_INDEX_FILE=%s\", stash_index_path);\n> +\n> +\t\tcp2.git_cmd = 1;\n> +\t\targv_array_pushl(&cp2.args, \"checkout-index\", \"--all\", NULL);\n> +\t\targv_array_pushf(&cp2.env_array, \"GIT_INDEX_FILE=%s\", stash_index_path);\n> +\n> +\t\tres = run_command(&cp) || run_command(&cp2);\n> +\t\tremove_path(stash_index_path);\n> +\t\tif (res)\n> +\t\t\treturn error(_(\"Could not restore untracked files from stash\"));\n\nA minor change in behaviour here is that we are removing the temporary\nindex file unconditionally here, while we would previously only remove\nit if both 'read-tree' and 'checkout-index' would succeed.\n\nI don't think that's a bad thing, we probably don't want users to try\nand use that index file in any way, and I doubt that's part of anyones\nworkflow, so I think cleaning it up makes sense.\n\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> +\t\tif (index)\n> +\t\t\tprintf_ln(_(\"Index was not unstashed.\"));\n\nMinor nit:  I think the above should be 'fprintf_ln(stderr, ...)' to\nmatch what we currently have.\n\n> +\n> +\t\treturn ret;\n> +\t}\n> +\n> [...]\n"},{"id":"342904","messageId":"CA+CzEk9QpmHK_TSBwQfEedNqrcVSBp3xY7bdv1YA_KxePiFeXw@mail.gmail.com","threadId":"48132","inReplyTo":"CAP8UFD3Qxt2YMqTtHwU8n7EDvD66QjGSywRQoxJDnncv7=2BUg@mail.gmail.com","subject":"Re: [PATCH 1/4] stash: convert apply to builtin","fromName":"Joel Teichroeb","fromEmail":"joel@teichroeb.net","sentAt":"2018-03-25T16:51:55Z","receivedAt":"2018-03-25T16:52:21Z","isPatch":true,"sender":{"key":"joel@teichroeb.net","avatar":"https://avatars.githubusercontent.com/u/240865?v=4"},"body":"On Sun, Mar 25, 2018 at 1:09 AM, Christian Couder\n<christian.couder@gmail.com> wrote:\n> It seems to me that the apply_stash() shell function is also used in\n> pop_stash() and in apply_to_branch(). Can the new helper be used there\n> too instead of apply_stash()? And then could apply_stash() be remove?\n\nI wasn't really sure if I should remove code from the .sh file as it\nseems in the past the old .sh files have been kept around as examples.\nHas that been done for previous conversions?\n"},{"id":"342907","messageId":"20180325170236.GB10909@hank","threadId":"48132","inReplyTo":"20180324173707.17699-3-joel@teichroeb.net","subject":"Re: [PATCH 2/4] stash: convert branch to builtin","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2018-03-25T17:02:36Z","receivedAt":"2018-03-25T16:59:18Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 03/24, Joel Teichroeb wrote:\n> ---\n>  builtin/stash--helper.c | 44 ++++++++++++++++++++++++++++++++++++++++++++\n>  git-stash.sh            |  3 ++-\n>  2 files changed, 46 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c\n> index e9a9574f40..18c4aba665 100644\n> --- a/builtin/stash--helper.c\n> +++ b/builtin/stash--helper.c\n> @@ -12,6 +12,7 @@\n>  \n>  static const char * const git_stash_helper_usage[] = {\n>  \tN_(\"git stash--helper apply [--index] [-q|--quiet] [<stash>]\"),\n> +\tN_(\"git stash--helper branch <branchname> [<stash>]\"),\n>  \tNULL\n>  };\n>  \n> @@ -20,6 +21,11 @@ static const char * const git_stash_helper_apply_usage[] = {\n>  \tNULL\n>  };\n>  \n> +static const char * const git_stash_helper_branch_usage[] = {\n> +\tN_(\"git stash--helper branch <branchname> [<stash>]\"),\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> @@ -307,6 +313,42 @@ static int apply_stash(int argc, const char **argv, const char *prefix)\n>  \treturn do_apply_stash(prefix, &info, index);\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_helper_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\nI see this is supposed to do something similar to what\n'assert_stash_like' was doing.  However we never end up die'ing with\n\"... is not a a stash-like commit\" here from what I can see.  I think\nI can see where this is coming from, and I missed it when reading over\n1/4 here.  I'll go back and comment there, where I think we're going\nslightly wrong.\n\nEither way while I tripped over the 'get_stash_info' call here, I\nthink it's the right thing to do.\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'do_drop_stash' is only defined in the next patch.  Maybe maybe 2/4\nand 3/4 need to swap places?\n\nAll patches should compile individually, and all tests should pass for\neach patch, so we maintain bisectability of the codebase.\n\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> @@ -329,6 +371,8 @@ int cmd_stash__helper(int argc, const char **argv, const char *prefix)\n>  \t\tusage_with_options(git_stash_helper_usage, options);\n>  \telse if (!strcmp(argv[0], \"apply\"))\n>  \t\tresult = apply_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_helper_usage, options);\n> diff --git a/git-stash.sh b/git-stash.sh\n> index 92c084eb17..360643ad4e 100755\n> --- a/git-stash.sh\n> +++ b/git-stash.sh\n> @@ -736,7 +736,8 @@ 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> -- \n> 2.16.2\n> \n"},{"id":"342911","messageId":"20180325172309.GC10909@hank","threadId":"48132","inReplyTo":"20180324173707.17699-2-joel@teichroeb.net","subject":"Re: [PATCH 1/4] stash: convert apply to builtin","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2018-03-25T17:23:09Z","receivedAt":"2018-03-25T17:19:52Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 03/24, Joel Teichroeb wrote:\n> ---\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\nBefore setting up the revisions here, as is done below, we used to\ncheck if a stash even exists, if no commit was given.  So in a\nrepository with no stashes we would die with \"No stash entries found\",\nwhile now we die with \"error: refs/stash@{0} is not a valid\nreference\".  I think the error message we had previously was slightly\nnicer, and we should try to keep it.\n\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\nWe used to distinguish between \"not a valid reference\" and \"not a\nstash-like commit\" here.  I think just doing the first 'get_oid'\nbefore the others, and returning the error if that fails, and then\ndoing the rest and returning the \"not a stash-like commit\" if one of\nthe other 'get_oid' calls fails would work, although I did not test it.\n\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> [...]\n"},{"id":"342913","messageId":"20180325173635.GD10909@hank","threadId":"48132","inReplyTo":"20180324173707.17699-5-joel@teichroeb.net","subject":"Re: [PATCH 4/4] stash: convert pop to builtin","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2018-03-25T17:36:35Z","receivedAt":"2018-03-25T17:33:17Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 03/24, Joel Teichroeb wrote:\n> ---\n>  builtin/stash--helper.c | 38 ++++++++++++++++++++++++++++++++++++++\n>  git-stash.sh            |  3 ++-\n>  2 files changed, 40 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c\n> index 1598b82ac2..b912f84c97 100644\n> --- a/builtin/stash--helper.c\n> +++ b/builtin/stash--helper.c\n> @@ -12,6 +12,7 @@\n>  \n>  static const char * const git_stash_helper_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> @@ -23,6 +24,11 @@ static const char * const git_stash_helper_drop_usage[] = {\n>  \tNULL\n>  };\n>  \n> +static const char * const git_stash_helper_pop_usage[] = {\n> +\tN_(\"git stash--helper pop [--index] [-q|--quiet] [<stash>]\"),\n> +\tNULL\n> +};\n> +\n>  static const char * const git_stash_helper_apply_usage[] = {\n>  \tN_(\"git stash--helper apply [--index] [-q|--quiet] [<stash>]\"),\n>  \tNULL\n> @@ -402,6 +408,36 @@ static int drop_stash(int argc, const char **argv, const char *prefix)\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_helper_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\nThe pattern above appears twice now, is it worth factoring it out into\na separate function, similar to 'assert_stash_ref'?\n\n> +\n> +\tif (do_apply_stash(prefix, &info, index))\n> +\t\treturn -1;\n\nIf we fail, currently we print \"The stash entry is kept in case you\nneed it again\", which we are loosing here.  I think that's useful\noutput in case the 'apply' command fails, especially in the case of a\nmerge conflict, where I think the 'apply' will fail as well, and the\nuser may be confused whether/why the stash is not dropped.\n\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> @@ -464,6 +500,8 @@ int cmd_stash__helper(int argc, const char **argv, const char *prefix)\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> diff --git a/git-stash.sh b/git-stash.sh\n> index 54d0a6c21f..d595bbaf64 100755\n> --- a/git-stash.sh\n> +++ b/git-stash.sh\n> @@ -732,7 +732,8 @@ 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> -- \n> 2.16.2\n> \n"},{"id":"342914","messageId":"20180325173916.GE10909@hank","threadId":"48132","inReplyTo":"20180324173707.17699-1-joel@teichroeb.net","subject":"Re: [PATCH 0/4] Convert some stash functionality to a builtin","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2018-03-25T17:39:16Z","receivedAt":"2018-03-25T17:35:59Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 03/24, Joel Teichroeb wrote:\n> I've been working on converting all of git stash to be a\n> builtin, however it's hard to get it all working at once with\n> limited time, so I've moved around half of it to a new\n> stash--helper builtin and called these functions from the shell\n> script. Once this is stabalized, it should be easier to convert\n> the rest of the commands one at a time without breaking\n> anything.\n> \n> I've sent most of this code before, but that was targetting a\n> full replacement of stash. The code is overall the same, but\n> with some code review changes and updates for internal api\n> changes.\n\nThanks for splitting this up into multiple patches, I found that much\nmore pleasant to review, and thanks for your continued work on this :)\n\n> Since there seems to be interest from GSOC students who want to\n> work on converting builtins, I figured I should finish what I\n> have that works now so they could build on top of it.\n> \n> Joel Teichroeb (4):\n>   stash: convert apply to builtin\n>   stash: convert branch to builtin\n>   stash: convert drop and clear to builtin\n>   stash: convert pop to builtin\n> \n>  .gitignore              |   1 +\n>  Makefile                |   1 +\n>  builtin.h               |   1 +\n>  builtin/stash--helper.c | 514 ++++++++++++++++++++++++++++++++++++++++++++++++\n>  git-stash.sh            |  13 +-\n>  git.c                   |   1 +\n>  6 files changed, 526 insertions(+), 5 deletions(-)\n>  create mode 100644 builtin/stash--helper.c\n> \n> -- \n> 2.16.2\n> \n"},{"id":"342936","messageId":"CAP8UFD04LF68LONP3hxpjc4oQokSab4HYvTkazeBq8STzN6E-A@mail.gmail.com","threadId":"48132","inReplyTo":"CA+CzEk9QpmHK_TSBwQfEedNqrcVSBp3xY7bdv1YA_KxePiFeXw@mail.gmail.com","subject":"Re: [PATCH 1/4] stash: convert apply to builtin","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-03-25T19:58:59Z","receivedAt":"2018-03-25T19:59:05Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sun, Mar 25, 2018 at 6:51 PM, Joel Teichroeb <joel@teichroeb.net> wrote:\n> On Sun, Mar 25, 2018 at 1:09 AM, Christian Couder\n> <christian.couder@gmail.com> wrote:\n>> It seems to me that the apply_stash() shell function is also used in\n>> pop_stash() and in apply_to_branch(). Can the new helper be used there\n>> too instead of apply_stash()? And then could apply_stash() be remove?\n>\n> I wasn't really sure if I should remove code from the .sh file as it\n> seems in the past the old .sh files have been kept around as examples.\n\nYeah, some original shell scripts that have been converted are kept in\ncontrib/examples/, but the shell code has still been removed from the\n.sh files when they were being converted.\n\n> Has that been done for previous conversions?\n\nI don't think there were some cases when the shell code was not\nremoved. I haven't looked at all the conversions in details though.\n"},{"id":"342943","messageId":"878tafyito.fsf@evledraar.gmail.com","threadId":"48132","inReplyTo":"20180325204653.1470-1-avarab@gmail.com","subject":"[PATCH] Remove contrib/examples/*","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-03-25T20:57:23Z","receivedAt":"2018-03-25T20:57:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Mar 25 2018, Ævar Arnfjörð Bjarmason wrote:\n\n> There were some side discussions at Git Merge this year about how we\n> should just update the README to tell users they can dig these up from\n> the history if the need them, do that.\n>\n> Looking at the \"git log\" for this directory we get quite a bit more\n> patch churn than we should here, mainly from things fixing various\n> tree-wide issues.\n>\n> There's also confusion on the list occasionally about how these should\n> be treated, \"Re: [PATCH 1/4] stash: convert apply to\n> builtin\" (<CA+CzEk9QpmHK_TSBwQfEedNqrcVSBp3xY7bdv1YA_KxePiFeXw@mail.gmail.com>)\n> being the latest example of that.\n\nThe people on CC got this, but it seems the git ML rejected the message\nas it's too big. The abbreviated patches is here quoted inline, and at:\nhttps://github.com/avar/git/commit/cc578c81c2cb2999b1a0b73954610bd74951c37b\n\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>\n> On Sun, Mar 25, 2018 at 6:51 PM, Joel Teichroeb <joel@teichroeb.net> wrote:\n>> On Sun, Mar 25, 2018 at 1:09 AM, Christian Couder\n>> <christian.couder@gmail.com> wrote:\n>>> It seems to me that the apply_stash() shell function is also used in\n>>> pop_stash() and in apply_to_branch(). Can the new helper be used there\n>>> too instead of apply_stash()? And then could apply_stash() be remove?\n>>\n>> I wasn't really sure if I should remove code from the .sh file as it\n>> seems in the past the old .sh files have been kept around as examples.\n>> Has that been done for previous conversions?\n>\n> I was skimming this patch and it seemed to me like it would be more\n> readable if the *.sh code was removed in the same change (if\n> possible). It's easier to review like that.\n>\n> Also, we should just stop maintainign contrib/examples/*.\n>\n>  contrib/examples/README                |  23 +-\n>  contrib/examples/builtin-fetch--tool.c | 575 ---------------\n>  contrib/examples/git-am.sh             | 975 ------------------------\n>  contrib/examples/git-checkout.sh       | 302 --------\n>  contrib/examples/git-clean.sh          | 118 ---\n>  contrib/examples/git-clone.sh          | 525 -------------\n>  contrib/examples/git-commit.sh         | 639 ----------------\n>  contrib/examples/git-difftool.perl     | 481 ------------\n>  contrib/examples/git-fetch.sh          | 379 ----------\n>  contrib/examples/git-gc.sh             |  37 -\n>  contrib/examples/git-log.sh            |  15 -\n>  contrib/examples/git-ls-remote.sh      | 142 ----\n>  contrib/examples/git-merge-ours.sh     |  14 -\n>  contrib/examples/git-merge.sh          | 620 ----------------\n>  contrib/examples/git-notes.sh          | 121 ---\n>  contrib/examples/git-pull.sh           | 381 ----------\n>  contrib/examples/git-remote.perl       | 474 ------------\n>  contrib/examples/git-repack.sh         | 194 -----\n>  contrib/examples/git-rerere.perl       | 284 -------\n>  contrib/examples/git-reset.sh          | 106 ---\n>  contrib/examples/git-resolve.sh        | 112 ---\n>  contrib/examples/git-revert.sh         | 207 ------\n>  contrib/examples/git-svnimport.perl    | 976 -------------------------\n>  contrib/examples/git-svnimport.txt     | 179 -----\n>  contrib/examples/git-tag.sh            | 205 ------\n>  contrib/examples/git-verify-tag.sh     |  45 --\n>  contrib/examples/git-whatchanged.sh    |  28 -\n>  27 files changed, 20 insertions(+), 8137 deletions(-)\n>  delete mode 100644 contrib/examples/builtin-fetch--tool.c\n>  delete mode 100755 contrib/examples/git-am.sh\n>  delete mode 100755 contrib/examples/git-checkout.sh\n>  delete mode 100755 contrib/examples/git-clean.sh\n>  delete mode 100755 contrib/examples/git-clone.sh\n>  delete mode 100755 contrib/examples/git-commit.sh\n>  delete mode 100755 contrib/examples/git-difftool.perl\n>  delete mode 100755 contrib/examples/git-fetch.sh\n>  delete mode 100755 contrib/examples/git-gc.sh\n>  delete mode 100755 contrib/examples/git-log.sh\n>  delete mode 100755 contrib/examples/git-ls-remote.sh\n>  delete mode 100755 contrib/examples/git-merge-ours.sh\n>  delete mode 100755 contrib/examples/git-merge.sh\n>  delete mode 100755 contrib/examples/git-notes.sh\n>  delete mode 100755 contrib/examples/git-pull.sh\n>  delete mode 100755 contrib/examples/git-remote.perl\n>  delete mode 100755 contrib/examples/git-repack.sh\n>  delete mode 100755 contrib/examples/git-rerere.perl\n>  delete mode 100755 contrib/examples/git-reset.sh\n>  delete mode 100755 contrib/examples/git-resolve.sh\n>  delete mode 100755 contrib/examples/git-revert.sh\n>  delete mode 100755 contrib/examples/git-svnimport.perl\n>  delete mode 100644 contrib/examples/git-svnimport.txt\n>  delete mode 100755 contrib/examples/git-tag.sh\n>  delete mode 100755 contrib/examples/git-verify-tag.sh\n>  delete mode 100755 contrib/examples/git-whatchanged.sh\n>\n> diff --git a/contrib/examples/README b/contrib/examples/README\n> index 6946f3dd2a..18bc60b021 100644\n> --- a/contrib/examples/README\n> +++ b/contrib/examples/README\n> @@ -1,3 +1,20 @@\n> -These are original scripted implementations, kept primarily for their\n> -reference value to any aspiring plumbing users who want to learn how\n> -pieces can be fit together.\n> +This directory used to contain scripted implementations of builtins\n> +that have since been rewritten in C.\n> +\n> +They have now been removed, but can be retrieved from an older commit\n> +that removed them from this directory.\n> +\n> +They're interesting for their reference value to any aspiring plumbing\n> +users who want to learn how pieces can be fit together, but in many\n> +cases have drifted enough from the actual implementations Git uses to\n> +be instructive.\n> +\n> +Other things that can be useful:\n> +\n> + * Some commands such as git-gc wrap other commands, and what they're\n> +   doing behind the scenes can be seen by running them under\n> +   GIT_TRACE=1\n> +\n> + * Doing `git log` on paths matching '*--helper.c' will show\n> +   incremental effort in the direction of moving existing shell\n> +   scripts to C.\n> [...]\n\nThe rest of this patch is deleting everything in contrib/examples/ that\nisn't the README.\n"},{"id":"342965","messageId":"20180326060135.GB7594@sigill.intra.peff.net","threadId":"48132","inReplyTo":"878tafyito.fsf@evledraar.gmail.com","subject":"Re: [PATCH] Remove contrib/examples/*","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-26T06:01:35Z","receivedAt":"2018-03-26T06:01:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 25, 2018 at 08:46:53PM +0000, Ævar Arnfjörð Bjarmason wrote:\n\n> There were some side discussions at Git Merge this year about how we\n> should just update the README to tell users they can dig these up from\n> the history if the need them, do that.\n> \n> Looking at the \"git log\" for this directory we get quite a bit more\n> patch churn than we should here, mainly from things fixing various\n> tree-wide issues.\n> \n> There's also confusion on the list occasionally about how these should\n> be treated, \"Re: [PATCH 1/4] stash: convert apply to\n> builtin\" (<CA+CzEk9QpmHK_TSBwQfEedNqrcVSBp3xY7bdv1YA_KxePiFeXw@mail.gmail.com>)\n> being the latest example of that.\n\nI'm in favor of this. I don't think I've ever come across this directory\nand _not_ been annoyed (because it was the result of a grep and was just\ncluttering the results). And I think your README change leaves a nice\nsignpost for people who might be digging around for plumbing examples.\n\n> The people on CC got this, but it seems the git ML rejected the message\n> as it's too big. The abbreviated patches is here quoted inline, and at:\n> https://github.com/avar/git/commit/cc578c81c2cb2999b1a0b73954610bd74951c37b\n\nI was going to suggest re-sending with \"-D\", but it looks like \"git\napply\" will not apply such a patch (even though it could in theory\nrealize that the current blob matches the preimage sha1 and it would be\nsafe to remove it).\n\n-Peff\n"},{"id":"343061","messageId":"xmqqpo3qbll7.fsf@gitster-ct.c.googlers.com","threadId":"48132","inReplyTo":"20180325204653.1470-1-avarab@gmail.com","subject":"Re: [PATCH] Remove contrib/examples/*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-26T20:58:28Z","receivedAt":"2018-03-26T20:58:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> + * Doing `git log` on paths matching '*--helper.c' will show\n> +   incremental effort in the direction of moving existing shell\n> +   scripts to C.\n\nIt may be benefitial to remind readers of \"--full-diff\", e.g.\n\n    $ git log --full-diff --stat -p -- \"${foo}--helper.c\"\n\nhere.\n"},{"id":"343201","messageId":"CA+CzEk_vRc2AQ+Cxn66TmqbYjDcMsgy0-QXLsJwKRLE70nip_A@mail.gmail.com","threadId":"48132","inReplyTo":"20180325164300.GA10909@hank","subject":"Re: [PATCH 1/4] stash: convert apply to builtin","fromName":"Joel Teichroeb","fromEmail":"joel@teichroeb.net","sentAt":"2018-03-28T03:30:36Z","receivedAt":"2018-03-28T03:31:03Z","isPatch":true,"sender":{"key":"joel@teichroeb.net","avatar":"https://avatars.githubusercontent.com/u/240865?v=4"},"body":"On Sun, Mar 25, 2018 at 9:43 AM, Thomas Gummerer <t.gummerer@gmail.com> wrote:\n> On 03/24, Joel Teichroeb wrote:\n>> ---\n>\n> Missing sign-off?  I saw it's missing in the other patches as well.\n>\n\nThanks! I always forget to add a sign-off.\n\n>> [...]\n>> +\n>> +     if (info->has_u) {\n>> +             struct child_process cp = CHILD_PROCESS_INIT;\n>> +             struct child_process cp2 = CHILD_PROCESS_INIT;\n>> +             int res;\n>> +\n>> +             cp.git_cmd = 1;\n>> +             argv_array_push(&cp.args, \"read-tree\");\n>> +             argv_array_push(&cp.args, sha1_to_hex(info->u_tree.hash));\n>> +             argv_array_pushf(&cp.env_array, \"GIT_INDEX_FILE=%s\", stash_index_path);\n>> +\n>> +             cp2.git_cmd = 1;\n>> +             argv_array_pushl(&cp2.args, \"checkout-index\", \"--all\", NULL);\n>> +             argv_array_pushf(&cp2.env_array, \"GIT_INDEX_FILE=%s\", stash_index_path);\n>> +\n>> +             res = run_command(&cp) || run_command(&cp2);\n>> +             remove_path(stash_index_path);\n>> +             if (res)\n>> +                     return error(_(\"Could not restore untracked files from stash\"));\n>\n> A minor change in behaviour here is that we are removing the temporary\n> index file unconditionally here, while we would previously only remove\n> it if both 'read-tree' and 'checkout-index' would succeed.\n>\n> I don't think that's a bad thing, we probably don't want users to try\n> and use that index file in any way, and I doubt that's part of anyones\n> workflow, so I think cleaning it up makes sense.\n>\n\nI'm not sure about that. The shell script has a trap near the start in\norder to clean up the temp index, unless I'm understanding the shell\nscript incorrectly.\n"}]}