{"thread":{"id":"46140","subject":"[PATCH v4 0/5] Implement git stash as a builtin command","startedAt":"2017-06-08T00:57:05Z","lastAt":"2017-06-27T14:53:20Z","messageCount":26,"participants":["Joel Teichroeb","Thomas Gummerer","Junio C Hamano","Johannes Schindelin","Jeff King","Matthieu Moy"],"isPatch":true,"patchVersion":4,"patchTotal":5},"messages":[{"id":"321722","messageId":"20170608005535.13080-1-joel@teichroeb.net","threadId":"46140","inReplyTo":null,"subject":"[PATCH v4 0/5] Implement git stash as a builtin command","fromName":"Joel Teichroeb","fromEmail":"joel@teichroeb.net","sentAt":"2017-06-08T00:55:30Z","receivedAt":"2017-06-08T00:57:05Z","isPatch":true,"sender":{"key":"joel@teichroeb.net","avatar":"https://avatars.githubusercontent.com/u/240865?v=4"},"body":"I've rewritten git stash as a builtin c command. All tests pass,\nand I've added two new tests. Test coverage is around 95% with the\nonly things missing coverage being error handlers.\n\nChanges since v3:\n * Fixed formatting issues\n * Fixed a bug with stash branch and added a new test for it\n * Fixed review comments\n\nOutstanding issue:\n * Not all argv array memory is cleaned up\n\nJoel Teichroeb (5):\n stash: add test for stash create with no files\n stash: Add a test for when apply fails during stash branch\n stash: add test for stashing in a detached state\n merge: close the index lock when not writing the new index\n stash: implement builtin stash\n\n Makefile | 2 +-\n builtin.h | 1 +\n builtin/stash.c | 1224 +++++++++++++++++++++++++\n git-stash.sh => contrib/examples/git-stash.sh | 0\n git.c | 1 +\n merge-recursive.c | 9 +-\n t/t3903-stash.sh | 34 +\n 7 files changed, 1267 insertions(+), 4 deletions(-)\n create mode 100644 builtin/stash.c\n rename git-stash.sh => contrib/examples/git-stash.sh (100%)\n\n-- \n2.13.0\n\n"},{"id":"321723","messageId":"20170608005535.13080-2-joel@teichroeb.net","threadId":"46140","inReplyTo":"20170608005535.13080-1-joel@teichroeb.net","subject":"[PATCH v4 1/5] stash: add test for stash create with no files","fromName":"Joel Teichroeb","fromEmail":"joel@teichroeb.net","sentAt":"2017-06-08T00:55:31Z","receivedAt":"2017-06-08T00:57:10Z","isPatch":true,"sender":{"key":"joel@teichroeb.net","avatar":"https://avatars.githubusercontent.com/u/240865?v=4"},"body":"Ensure the command gives the correct return code\n\nSigned-off-by: Joel Teichroeb \n---\n t/t3903-stash.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 3b4bed5c9a..cc923e6335 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -444,6 +444,14 @@ test_expect_failure 'stash file to directory' '\n \ttest foo = \"$(cat file/file)\"\n '\n \n+test_expect_success 'stash create - no changes' '\n+\tgit stash clear &&\n+\ttest_when_finished \"git reset --hard HEAD\" &&\n+\tgit reset --hard &&\n+\tgit stash create >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n test_expect_success 'stash branch - no stashes on stack, stash-like argument' '\n \tgit stash clear &&\n \ttest_when_finished \"git reset --hard HEAD\" &&\n-- \n2.13.0\n\n"},{"id":"321724","messageId":"20170608005535.13080-3-joel@teichroeb.net","threadId":"46140","inReplyTo":"20170608005535.13080-1-joel@teichroeb.net","subject":"[PATCH v4 2/5] stash: Add a test for when apply fails during stash branch","fromName":"Joel Teichroeb","fromEmail":"joel@teichroeb.net","sentAt":"2017-06-08T00:55:32Z","receivedAt":"2017-06-08T00:57:16Z","isPatch":true,"sender":{"key":"joel@teichroeb.net","avatar":"https://avatars.githubusercontent.com/u/240865?v=4"},"body":"If the return value of merge recurisve is not checked, the stash could end\nup being dropped even though it was not applied properly\n\nSigned-off-by: Joel Teichroeb \n---\n t/t3903-stash.sh | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex cc923e6335..5399fb05ca 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -656,6 +656,20 @@ test_expect_success 'stash branch should not drop the stash if the branch exists\n \tgit rev-parse stash@{0} --\n '\n \n+test_expect_success 'stash branch should not drop the stash if the apply fails' '\n+\tgit stash clear &&\n+\tgit reset HEAD~1 --hard &&\n+\techo foo >file &&\n+\tgit add file &&\n+\tgit commit -m initial &&\n+\techo bar >file &&\n+\tgit stash &&\n+\techo baz >file &&\n+\ttest_when_finished \"git checkout master\" &&\n+\ttest_must_fail git stash branch new_branch stash@{0} &&\n+\tgit rev-parse stash@{0} --\n+'\n+\n test_expect_success 'stash apply shows status same as git status (relative to current directory)' '\n \tgit stash clear &&\n \techo 1 >subdir/subfile1 &&\n-- \n2.13.0\n\n"},{"id":"321725","messageId":"20170608005535.13080-4-joel@teichroeb.net","threadId":"46140","inReplyTo":"20170608005535.13080-1-joel@teichroeb.net","subject":"[PATCH v4 3/5] stash: add test for stashing in a detached state","fromName":"Joel Teichroeb","fromEmail":"joel@teichroeb.net","sentAt":"2017-06-08T00:55:33Z","receivedAt":"2017-06-08T00:57:21Z","isPatch":true,"sender":{"key":"joel@teichroeb.net","avatar":"https://avatars.githubusercontent.com/u/240865?v=4"},"body":"Signed-off-by: Joel Teichroeb \n---\n t/t3903-stash.sh | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 5399fb05ca..ce4c8fe3d6 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -822,6 +822,18 @@ test_expect_success 'create with multiple arguments for the message' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'create in a detached state' '\n+\ttest_when_finished \"git checkout master\" &&\n+\tgit checkout HEAD~1 &&\n+\t>foo &&\n+\tgit add foo &&\n+\tSTASH_ID=$(git stash create) &&\n+\tHEAD_ID=$(git rev-parse --short HEAD) &&\n+\techo \"WIP on (no branch): ${HEAD_ID} initial\" >expect &&\n+\tgit show --pretty=%s -s ${STASH_ID} >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'stash -- stashes and restores the file' '\n \t>foo &&\n \t>bar &&\n-- \n2.13.0\n\n"},{"id":"321726","messageId":"20170608005535.13080-5-joel@teichroeb.net","threadId":"46140","inReplyTo":"20170608005535.13080-1-joel@teichroeb.net","subject":"[PATCH v4 4/5] merge: close the index lock when not writing the new index","fromName":"Joel Teichroeb","fromEmail":"joel@teichroeb.net","sentAt":"2017-06-08T00:55:34Z","receivedAt":"2017-06-08T00:57:27Z","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 \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 ae5238d82c..16bb5512ef 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -2145,9 +2145,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.13.0\n\n"},{"id":"321727","messageId":"20170608005535.13080-6-joel@teichroeb.net","threadId":"46140","inReplyTo":"20170608005535.13080-1-joel@teichroeb.net","subject":"[PATCH v4 5/5] stash: implement builtin stash","fromName":"Joel Teichroeb","fromEmail":"joel@teichroeb.net","sentAt":"2017-06-08T00:55:35Z","receivedAt":"2017-06-08T00:57:35Z","isPatch":true,"sender":{"key":"joel@teichroeb.net","avatar":"https://avatars.githubusercontent.com/u/240865?v=4"},"body":"Implement all git stash functionality as a builtin command\n\nSigned-off-by: Joel Teichroeb \n---\n Makefile | 2 +-\n builtin.h | 1 +\n builtin/stash.c | 1224 +++++++++++++++++++++++++\n git-stash.sh => contrib/examples/git-stash.sh | 0\n git.c | 1 +\n 5 files changed, 1227 insertions(+), 1 deletion(-)\n create mode 100644 builtin/stash.c\n rename git-stash.sh => contrib/examples/git-stash.sh (100%)\n\ndiff --git a/Makefile b/Makefile\nindex 7c621f7f76..3364d87630 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -525,7 +525,6 @@ SCRIPT_SH += git-quiltimport.sh\n SCRIPT_SH += git-rebase.sh\n SCRIPT_SH += git-remote-testgit.sh\n SCRIPT_SH += git-request-pull.sh\n-SCRIPT_SH += git-stash.sh\n SCRIPT_SH += git-submodule.sh\n SCRIPT_SH += git-web--browse.sh\n \n@@ -965,6 +964,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.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 498ac80d07..fa59481420 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -119,6 +119,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(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.c b/builtin/stash.c\nnew file mode 100644\nindex 0000000000..a9680f2909\n--- /dev/null\n+++ b/builtin/stash.c\n@@ -0,0 +1,1224 @@\n+#include \"builtin.h\"\n+#include \"parse-options.h\"\n+#include \"refs.h\"\n+#include \"tree.h\"\n+#include \"lockfile.h\"\n+#include \"object.h\"\n+#include \"tree-walk.h\"\n+#include \"cache-tree.h\"\n+#include \"unpack-trees.h\"\n+#include \"diff.h\"\n+#include \"revision.h\"\n+#include \"commit.h\"\n+#include \"diffcore.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 list []\"),\n+\tN_(\"git stash show []\"),\n+\tN_(\"git stash drop [-q|--quiet] []\"),\n+\tN_(\"git stash ( pop | apply ) [--index] [-q|--quiet] []\"),\n+\tN_(\"git stash branch []\"),\n+\tN_(\"git stash [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]\"),\n+\tN_(\" [-u|--include-untracked] [-a|--all] []]\"),\n+\tN_(\"git stash clear\"),\n+\tN_(\"git stash create []\"),\n+\tN_(\"git stash store [-m|--message ] [-q|--quiet] \"),\n+\tNULL\n+};\n+\n+static const char * const git_stash_list_usage[] = {\n+\tN_(\"git stash list []\"),\n+\tNULL\n+};\n+\n+static const char * const git_stash_show_usage[] = {\n+\tN_(\"git stash show []\"),\n+\tNULL\n+};\n+\n+static const char * const git_stash_drop_usage[] = {\n+\tN_(\"git stash drop [-q|--quiet] []\"),\n+\tNULL\n+};\n+\n+static const char * const git_stash_pop_usage[] = {\n+\tN_(\"git stash pop [--index] [-q|--quiet] []\"),\n+\tNULL\n+};\n+\n+static const char * const git_stash_apply_usage[] = {\n+\tN_(\"git stash apply [--index] [-q|--quiet] []\"),\n+\tNULL\n+};\n+\n+static const char * const git_stash_branch_usage[] = {\n+\tN_(\"git stash branch []\"),\n+\tNULL\n+};\n+\n+static const char * const git_stash_save_usage[] = {\n+\tN_(\"git stash [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]\"),\n+\tN_(\" [-u|--include-untracked] [-a|--all] []]\"),\n+\tNULL\n+};\n+\n+static const char * const git_stash_clear_usage[] = {\n+\tN_(\"git stash clear\"),\n+\tNULL\n+};\n+\n+static const char * const git_stash_create_usage[] = {\n+\tN_(\"git stash create []\"),\n+\tNULL\n+};\n+\n+static const char * const git_stash_store_usage[] = {\n+\tN_(\"git stash store [-m|--message ] [-q|--quiet] \"),\n+\tNULL\n+};\n+\n+static const char *ref_stash = \"refs/stash\";\n+static int quiet = 0;\n+static struct lock_file lock_file;\n+static char stash_index_path[64];\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 untracked_files(struct strbuf *out, int include_untracked,\n+\t\tint include_ignored, const char **argv)\n+{\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tcp.git_cmd = 1;\n+\targv_array_pushl(&cp.args, \"ls-files\", \"-o\", \"-z\", NULL);\n+\tif (include_untracked && !include_ignored)\n+\t\targv_array_push(&cp.args, \"--exclude-standard\");\n+\targv_array_push(&cp.args, \"--\");\n+\tif (argv)\n+\t\targv_array_pushv(&cp.args, argv);\n+\treturn pipe_command(&cp, NULL, 0, out, 0, NULL, 0);\n+}\n+\n+static int check_no_changes(const char *prefix, int include_untracked,\n+\t\tint include_ignored, const char **argv)\n+{\n+\tstruct argv_array args1 = ARGV_ARRAY_INIT;\n+\tstruct argv_array args2 = ARGV_ARRAY_INIT;\n+\tstruct strbuf out = STRBUF_INIT;\n+\tint ret;\n+\n+\targv_array_pushl(&args1, \"diff-index\", \"--quiet\", \"--cached\", \"HEAD\",\n+\t\t\"--ignore-submodules\", \"--\", NULL);\n+\tif (argv)\n+\t\targv_array_pushv(&args1, argv);\n+\n+\targv_array_pushl(&args2, \"diff-files\", \"--quiet\", \"--ignore-submodules\",\n+\t\t\"--\", NULL);\n+\tif (argv)\n+\t\targv_array_pushv(&args2, argv);\n+\n+\tif (include_untracked)\n+\t\tuntracked_files(&out, include_untracked, include_ignored, argv);\n+\n+\tret = cmd_diff_index(args1.argc, args1.argv, prefix) == 0 &&\n+\t\t\tcmd_diff_files(args2.argc, args2.argv, prefix) == 0 &&\n+\t\t\t(!include_untracked || out.len == 0);\n+\tstrbuf_release(&out);\n+\treturn ret;\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 (strlen(commit) < 3) {\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+\tstrbuf_addf(&u_tree_rev, \"%s^3:\", revision);\n+\n+\tret = (get_sha1(w_commit_rev.buf, info->w_commit.hash) == 0 &&\n+\t\tget_sha1(b_commit_rev.buf, info->b_commit.hash) == 0 &&\n+\t\tget_sha1(w_tree_rev.buf, info->w_tree.hash) == 0 &&\n+\t\tget_sha1(b_tree_rev.buf, info->b_tree.hash) == 0 &&\n+\t\tget_sha1(i_tree_rev.buf, info->i_tree.hash) == 0);\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+\tinfo->has_u = get_sha1(u_tree_rev.buf, info->u_tree.hash) == 0;\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) == 0;\n+\tstrbuf_release(&out);\n+\n+\treturn !ret;\n+}\n+\n+static void stash_create_callback(struct diff_queue_struct *q,\n+\t\t\t\tstruct diff_options *opt, void *cbdata)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < q->nr; i++) {\n+\t\tstruct diff_filepair *p = q->queue[i];\n+\t\tconst char *path = p->one->path;\n+\t\tstruct stat st;\n+\t\tremove_file_from_index(&the_index, path);\n+\t\tif (!lstat(path, &st))\n+\t\t\tadd_to_index(&the_index, path, &st, 0);\n+\t}\n+}\n+\n+/*\n+ * Untracked files are stored by themselves in a parentless commit, for\n+ * ease of unpacking later.\n+ */\n+static int save_untracked(struct stash_info *info, const char *message,\n+\t\tint include_untracked, int include_ignored, const char **argv)\n+{\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tstruct strbuf out = STRBUF_INIT;\n+\tstruct object_id orig_tree;\n+\tint ret;\n+\tconst char *index_file = get_index_file();\n+\n+\tset_alternate_index_output(stash_index_path);\n+\tuntracked_files(&out, include_untracked, include_ignored, argv);\n+\n+\tcp.git_cmd = 1;\n+\targv_array_pushl(&cp.args, \"update-index\", \"-z\", \"--add\", \"--remove\",\n+\t\t\"--stdin\", NULL);\n+\targv_array_pushf(&cp.env_array, \"GIT_INDEX_FILE=%s\", stash_index_path);\n+\n+\tif (pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0)) {\n+\t\tstrbuf_release(&out);\n+\t\treturn 1;\n+\t}\n+\n+\tstrbuf_reset(&out);\n+\n+\tdiscard_cache();\n+\tread_cache_from(stash_index_path);\n+\n+\twrite_index_as_tree(orig_tree.hash, &the_index, stash_index_path, 0,NULL);\n+\tdiscard_cache();\n+\n+\tread_cache_from(stash_index_path);\n+\n+\twrite_cache_as_tree(info->u_tree.hash, 0, NULL);\n+\tstrbuf_addf(&out, \"untracked files on %s\", message);\n+\n+\tret = commit_tree(out.buf, out.len, info->u_tree.hash, NULL,\n+\t\t\tinfo->u_commit.hash, NULL, NULL);\n+\tstrbuf_release(&out);\n+\tif (ret)\n+\t\treturn 1;\n+\n+\tset_alternate_index_output(index_file);\n+\tdiscard_cache();\n+\tread_cache();\n+\n+\treturn 0;\n+}\n+\n+static int save_working_tree(struct stash_info *info, const char *prefix,\n+\t\tconst char **argv)\n+{\n+\tstruct object_id orig_tree;\n+\tstruct rev_info rev;\n+\tint nr_trees = 1;\n+\tstruct tree_desc t[MAX_UNPACK_TREES];\n+\tstruct tree *tree;\n+\tstruct unpack_trees_options opts;\n+\tstruct object *obj;\n+\n+\tdiscard_cache();\n+\ttree = parse_tree_indirect(&info->i_tree);\n+\tprime_cache_tree(&the_index, tree);\n+\twrite_index_as_tree(orig_tree.hash, &the_index, stash_index_path, 0, NULL);\n+\tdiscard_cache();\n+\n+\tread_cache_from(stash_index_path);\n+\n+\tmemset(&opts, 0, sizeof(opts));\n+\n+\tparse_tree(tree);\n+\n+\topts.head_idx = 1;\n+\topts.src_index = &the_index;\n+\topts.dst_index = &the_index;\n+\topts.merge = 1;\n+\topts.fn = oneway_merge;\n+\n+\tinit_tree_desc(t, tree->buffer, tree->size);\n+\n+\tif (unpack_trees(nr_trees, t, &opts))\n+\t\treturn 1;\n+\n+\tinit_revisions(&rev, prefix);\n+\tsetup_revisions(0, NULL, &rev, NULL);\n+\trev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;\n+\trev.diffopt.format_callback = stash_create_callback;\n+\tDIFF_OPT_SET(&rev.diffopt, EXIT_WITH_STATUS);\n+\n+\tparse_pathspec(&rev.prune_data, 0, 0, prefix, argv);\n+\n+\tdiff_setup_done(&rev.diffopt);\n+\tobj = parse_object(&info->b_commit);\n+\tadd_pending_object(&rev, obj, \"\");\n+\tif (run_diff_index(&rev, 0))\n+\t\treturn 1;\n+\n+\tif (write_cache_as_tree(info->w_tree.hash, 0, NULL))\n+\t\treturn 1;\n+\n+\tdiscard_cache();\n+\tread_cache();\n+\n+\treturn 0;\n+}\n+\n+static int patch_working_tree(struct stash_info *info, const char *prefix,\n+\t\tconst char **argv)\n+{\n+\tstruct argv_array args = ARGV_ARRAY_INIT;\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tstruct strbuf out = STRBUF_INIT;\n+\tsize_t unused;\n+\tconst char *index_file = get_index_file();\n+\n+\targv_array_pushl(&args, \"read-tree\", \"HEAD\", NULL);\n+\targv_array_pushf(&args, \"--index-output=%s\", stash_index_path);\n+\tcmd_read_tree(args.argc, args.argv, prefix);\n+\n+\tcp.git_cmd = 1;\n+\targv_array_pushl(&cp.args, \"add--interactive\", \"--patch=stash\", \"--\", NULL);\n+\targv_array_pushf(&cp.env_array, \"GIT_INDEX_FILE=%s\", stash_index_path);\n+\tif (run_command(&cp))\n+\t\treturn 1;\n+\n+\tdiscard_cache();\n+\tread_cache_from(stash_index_path);\n+\n+\tif (write_cache_as_tree(info->w_tree.hash, 0, NULL))\n+\t\treturn 1;\n+\n+\tchild_process_init(&cp);\n+\tcp.git_cmd = 1;\n+\targv_array_pushl(&cp.args, \"diff-tree\", \"-p\", \"HEAD\", NULL);\n+\targv_array_push(&cp.args, sha1_to_hex(info->w_tree.hash));\n+\targv_array_push(&cp.args, \"--\");\n+\tif (pipe_command(&cp, NULL, 0, &out, 0, NULL, 0) || out.len == 0)\n+\t\treturn 1;\n+\n+\tinfo->patch = strbuf_detach(&out, &unused);\n+\n+\tset_alternate_index_output(index_file);\n+\tdiscard_cache();\n+\tread_cache();\n+\n+\treturn 0;\n+}\n+\n+static int do_create_stash(struct stash_info *info, const char *prefix,\n+\t\tconst char *message, int include_untracked, int include_ignored,\n+\t\tint patch, const char **argv)\n+{\n+\tstruct object_id curr_head;\n+\tchar *branch_path = NULL;\n+\tconst char *branch_name = NULL;\n+\tstruct commit_list *parents = NULL;\n+\tstruct strbuf out_message = STRBUF_INIT;\n+\tstruct strbuf out = STRBUF_INIT;\n+\tstruct pretty_print_context ctx = {0};\n+\n+\tstruct commit *c = NULL;\n+\tconst char *hash;\n+\n+\tread_cache_preload(NULL);\n+\trefresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL);\n+\tif (check_no_changes(prefix, include_untracked, include_ignored, argv))\n+\t\treturn 1;\n+\n+\tif (get_sha1_tree(\"HEAD\", info->b_commit.hash))\n+\t\treturn error(_(\"You do not have the initial commit yet\"));\n+\n+\tbranch_path = resolve_refdup(\"HEAD\", 0, curr_head.hash, NULL);\n+\n+\tif (branch_path == NULL || strcmp(branch_path, \"HEAD\") == 0)\n+\t\tbranch_name = \"(no branch)\";\n+\telse\n+\t\tskip_prefix(branch_path, \"refs/heads/\", &branch_name);\n+\n+\tc = lookup_commit(&info->b_commit);\n+\n+\tctx.output_encoding = get_log_output_encoding();\n+\tctx.abbrev = 1;\n+\tctx.fmt = CMIT_FMT_ONELINE;\n+\thash = find_unique_abbrev(c->object.oid.hash, DEFAULT_ABBREV);\n+\n+\tstrbuf_addf(&out_message, \"%s: %s \", branch_name, hash);\n+\n+\tpretty_print_commit(&ctx, c, &out_message);\n+\n+\tstrbuf_addf(&out, \"index on %s\\n\", out_message.buf);\n+\n+\tcommit_list_insert(lookup_commit(&info->b_commit), &parents);\n+\n+\tif (write_cache_as_tree(info->i_tree.hash, 0, NULL))\n+\t\treturn error(_(\"git write-tree failed to write a tree\"));\n+\n+\tif (commit_tree(out.buf, out.len, info->i_tree.hash, parents, info->i_commit.hash, NULL, NULL))\n+\t\treturn error(_(\"Cannot save the current index state\"));\n+\n+\tstrbuf_reset(&out);\n+\n+\tif (include_untracked) {\n+\t\tif (save_untracked(info, out_message.buf, include_untracked, include_ignored, argv))\n+\t\t\treturn error(_(\"Cannot save the untracked files\"));\n+\t}\n+\n+\tif (patch) {\n+\t\tif (patch_working_tree(info, prefix, argv))\n+\t\t\treturn error(_(\"Cannot save the current worktree state\"));\n+\t} else {\n+\t\tif (save_working_tree(info, prefix, argv))\n+\t\t\treturn error(_(\"Cannot save the current worktree state\"));\n+\t}\n+\tparents = NULL;\n+\n+\tif (include_untracked)\n+\t\tcommit_list_insert(lookup_commit(&info->u_commit), &parents);\n+\n+\tcommit_list_insert(lookup_commit(&info->i_commit), &parents);\n+\tcommit_list_insert(lookup_commit(&info->b_commit), &parents);\n+\n+\tif (message != NULL && strlen(message) != 0)\n+\t\tstrbuf_addf(&out, \"On %s: %s\\n\", branch_name, message);\n+\telse\n+\t\tstrbuf_addf(&out, \"WIP on %s\\n\", out_message.buf);\n+\n+\tif (commit_tree(out.buf, out.len, info->w_tree.hash, parents, info->w_commit.hash, NULL, NULL))\n+\t\treturn error(_(\"Cannot record working tree state\"));\n+\n+\tinfo->message = out.buf;\n+\n+\tstrbuf_release(&out_message);\n+\tfree(branch_path);\n+\n+\treturn 0;\n+}\n+\n+static int create_stash(int argc, const char **argv, const char *prefix)\n+{\n+\tint include_untracked = 0;\n+\tconst char *message = NULL;\n+\tstruct stash_info info;\n+\tint ret;\n+\tstruct strbuf out = STRBUF_INIT;\n+\tstruct option options[] = {\n+\t\tOPT_BOOL('u', \"include-untracked\", &include_untracked,\n+\t\t\tN_(\"stash untracked filed\")),\n+\t\tOPT_STRING('m', \"message\", &message, N_(\"message\"),\n+\t\t\tN_(\"stash commit message\")),\n+\t\tOPT_END()\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, options,\n+\t\t\tgit_stash_create_usage, 0);\n+\n+\tif (argc != 0) {\n+\t\tint i;\n+\t\tfor (i = 0; i < argc; ++i) {\n+\t\t\tif (i != 0) {\n+\t\t\t\tstrbuf_addf(&out, \" \");\n+\t\t\t}\n+\t\t\tstrbuf_addf(&out, \"%s\", argv[i]);\n+\t\t}\n+\t\tmessage = out.buf;\n+\t}\n+\n+\tret = do_create_stash(&info, prefix, message, include_untracked, 0, 0, NULL);\n+\n+\tstrbuf_release(&out);\n+\n+\tif (ret)\n+\t\treturn 0;\n+\n+\tprintf(\"%s\\n\", sha1_to_hex(info.w_commit.hash));\n+\treturn 0;\n+}\n+\n+static int do_store_stash(const char *prefix, int quiet, const char *message,\n+\t\tstruct object_id commit)\n+{\n+\tint ret;\n+\tret = update_ref(message, ref_stash, commit.hash, NULL,\n+\t\t\tREF_FORCE_CREATE_REFLOG, UPDATE_REFS_DIE_ON_ERR);\n+\n+\tif (ret && !quiet)\n+\t\treturn error(_(\"Cannot update %s with %s\"), ref_stash, sha1_to_hex(commit.hash));\n+\n+\treturn ret;\n+}\n+\n+static int store_stash(int argc, const char **argv, const char *prefix)\n+{\n+\tconst char *message = \"Create via \\\"git stash store\\\".\";\n+\tconst char *commit = NULL;\n+\tstruct object_id obj;\n+\tstruct option options[] = {\n+\t\tOPT_STRING('m', \"message\", &message, N_(\"message\"),\n+\t\t\tN_(\"stash commit message\")),\n+\t\tOPT__QUIET(&quiet, N_(\"be quiet, only report errors\")),\n+\t\tOPT_END()\n+\t};\n+\targc = parse_options(argc, argv, prefix, options, git_stash_store_usage, 0);\n+\n+\tif (argc != 1)\n+\t\treturn error(_(\"\\\"git stash store\\\" requires one argument\"));\n+\n+\tcommit = argv[0];\n+\n+\tif (get_sha1(commit, obj.hash)) {\n+\t\tfprintf_ln(stderr, _(\"fatal: %s: not a valid SHA1\"), commit);\n+\t\tfprintf_ln(stderr, _(\"cannot update %s with %s\"), ref_stash, commit);\n+\t\treturn 1;\n+\t}\n+\n+\treturn do_store_stash(prefix, quiet, message, obj);\n+}\n+\n+static int do_clear_stash(void)\n+{\n+\tstruct object_id obj;\n+\tif (get_sha1(ref_stash, obj.hash))\n+\t\treturn 0;\n+\n+\treturn delete_ref(NULL, ref_stash, obj.hash, 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 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+\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_push_stash(const char *prefix, const char *message,\n+\tint keep_index, int include_untracked, int include_ignored, int patch,\n+\tconst char **argv)\n+{\n+\tint res;\n+\tstruct stash_info info;\n+\n+\tif (patch && include_untracked)\n+\t\treturn error(_(\"can't use --patch and --include-untracked or --all at the same time\"));\n+\n+\tif (!include_untracked) {\n+\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\n+\t\tcp.git_cmd = 1;\n+\t\tcp.no_stdout = 1;\n+\t\targv_array_pushl(&cp.args, \"ls-files\", \"--error-unmatch\", \"--\", NULL);\n+\t\tif (argv)\n+\t\t\targv_array_pushv(&cp.args, argv);\n+\t\tres = run_command(&cp);\n+\t\tif (res)\n+\t\t\treturn 1;\n+\t}\n+\n+\tread_cache_preload(NULL);\n+\trefresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL);\n+\tif (check_no_changes(prefix, include_untracked, include_ignored, argv)) {\n+\t\tprintf_ln(_(\"No local changes to save\"));\n+\t\treturn 0;\n+\t}\n+\n+\tif (!reflog_exists(ref_stash)) {\n+\t\tif (do_clear_stash())\n+\t\t\treturn error(_(\"Cannot initialize stash\"));\n+\t}\n+\n+\tif (do_create_stash(&info, prefix, message, include_untracked, include_ignored, patch, argv))\n+\t\treturn 1;\n+\tres = do_store_stash(prefix, 1, info.message, info.w_commit);\n+\n+\tif (res == 0 && !quiet)\n+\t\tprintf(_(\"Saved working directory and index state %s\"), info.message);\n+\n+\tif (!patch) {\n+\t\tif (argv && *argv) {\n+\t\t\tstruct argv_array args = ARGV_ARRAY_INIT;\n+\t\t\tstruct argv_array args2 = ARGV_ARRAY_INIT;\n+\t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\t\t\tstruct strbuf out = STRBUF_INIT;\n+\t\t\targv_array_pushl(&args, \"reset\", \"--quiet\", \"--\", NULL);\n+\t\t\targv_array_pushv(&args, argv);\n+\t\t\tcmd_reset(args.argc, args.argv, prefix);\n+\n+\t\t\tcp.git_cmd = 1;\n+\t\t\targv_array_pushl(&cp.args, \"ls-files\", \"-z\", \"--modified\", \"--\",\n+\t\t\t\tNULL);\n+\t\t\targv_array_pushv(&cp.args, argv);\n+\t\t\tpipe_command(&cp, NULL, 0, &out, 0, NULL, 0);\n+\n+\t\t\tchild_process_init(&cp);\n+\t\t\tcp.git_cmd = 1;\n+\t\t\targv_array_pushl(&cp.args, \"checkout-index\", \"-z\", \"--force\",\n+\t\t\t\t\"--stdin\", NULL);\n+\t\t\tpipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0);\n+\t\t\tstrbuf_release(&out);\n+\n+\t\t\targv_array_pushl(&args2, \"clean\", \"--force\", \"-d\", \"--quiet\", \"--\",\n+\t\t\t\tNULL);\n+\t\t\targv_array_pushv(&args2, argv);\n+\t\t\tcmd_clean(args2.argc, args2.argv, prefix);\n+\t\t} else {\n+\t\t\tstruct argv_array args = ARGV_ARRAY_INIT;\n+\t\t\targv_array_pushl(&args, \"reset\", \"--hard\", \"--quiet\", NULL);\n+\t\t\tcmd_reset(args.argc, args.argv, prefix);\n+\t\t}\n+\n+\t\tif (include_untracked) {\n+\t\t\tstruct argv_array args = ARGV_ARRAY_INIT;\n+\t\t\targv_array_pushl(&args, \"clean\", \"--force\", \"--quiet\", \"-d\", NULL);\n+\t\t\tif (include_ignored)\n+\t\t\t\targv_array_push(&args, \"-x\");\n+\t\t\targv_array_push(&args, \"--\");\n+\t\t\tif (argv)\n+\t\t\t\targv_array_pushv(&args, argv);\n+\t\t\tcmd_clean(args.argc, args.argv, prefix);\n+\t\t}\n+\n+\t\tif (keep_index) {\n+\t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\t\t\tstruct strbuf out = STRBUF_INIT;\n+\n+\t\t\treset_tree(info.i_tree, 0, 1);\n+\n+\t\t\tcp.git_cmd = 1;\n+\t\t\targv_array_pushl(&cp.args, \"ls-files\", \"-z\", \"--modified\", \"--\",\n+\t\t\t\tNULL);\n+\t\t\targv_array_pushv(&cp.args, argv);\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, \"checkout-index\", \"-z\", \"--force\",\n+\t\t\t\t\"--stdin\", NULL);\n+\t\t\tif (pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0))\n+\t\t\t\treturn 1;\n+\t\t\tstrbuf_release(&out);\n+\t\t}\n+\t} else {\n+\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\t\tcp.git_cmd = 1;\n+\t\targv_array_pushl(&cp.args, \"apply\", \"-R\", NULL);\n+\t\tif (pipe_command(&cp, info.patch, strlen(info.patch), NULL, 0, NULL, 0))\n+\t\t\treturn error(_(\"Cannot remove worktree changes\"));\n+\n+\t\tif (!keep_index) {\n+\t\t\tstruct argv_array args = ARGV_ARRAY_INIT;\n+\t\t\targv_array_pushl(&args, \"reset\", \"--quiet\", \"--\", NULL);\n+\t\t\tif (argv)\n+\t\t\t\targv_array_pushv(&args, argv);\n+\t\t\tcmd_reset(args.argc, args.argv, prefix);\n+\t\t}\n+\t}\n+\n+\treturn 0;\n+}\n+\n+static int push_stash(int argc, const char **argv, const char *prefix)\n+{\n+\tconst char *message = NULL;\n+\tint include_untracked = 0;\n+\tint include_ignored = 0;\n+\tint patch = 0;\n+\tint keep_index_set = -1;\n+\tint keep_index = 0;\n+\tstruct option options[] = {\n+\t\tOPT_BOOL('u', \"include-untracked\", &include_untracked,\n+\t\t\tN_(\"stash untracked filed\")),\n+\t\tOPT_BOOL('a', \"all\", &include_ignored,\n+\t\t\tN_(\"stash ignored untracked files\")),\n+\t\tOPT_BOOL('k', \"keep-index\", &keep_index_set,\n+\t\t\tN_(\"restore the index after applying the stash\")),\n+\t\tOPT__QUIET(&quiet, N_(\"be quiet, only report errors\")),\n+\t\tOPT_STRING('m', \"message\", &message, N_(\"message\"),\n+\t\t\tN_(\"stash commit message\")),\n+\t\tOPT_BOOL('p', \"patch\", &patch,\n+\t\t\tN_(\"edit current diff and apply\")),\n+\t\tOPT_END()\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, options,\n+\t\t\t\tgit_stash_save_usage, PARSE_OPT_STOP_AT_NON_OPTION);\n+\n+\tif (include_ignored)\n+\t\tinclude_untracked = 1;\n+\n+\tif (keep_index_set != -1)\n+\t\tkeep_index = keep_index_set;\n+\telse if (patch)\n+\t\tkeep_index = 1;\n+\n+\treturn do_push_stash(prefix, message, keep_index, include_untracked, include_ignored, patch, argv);\n+}\n+\n+static int save_stash(int argc, const char **argv, const char *prefix)\n+{\n+\tconst char *message = NULL;\n+\tint include_untracked = 0;\n+\tint include_ignored = 0;\n+\tint patch = 0;\n+\tint keep_index_set = -1;\n+\tint keep_index = 0;\n+\tint ret;\n+\tstruct strbuf out = STRBUF_INIT;\n+\tstruct option options[] = {\n+\t\tOPT_BOOL('u', \"include-untracked\", &include_untracked,\n+\t\t\tN_(\"stash untracked filed\")),\n+\t\tOPT_BOOL('a', \"all\", &include_ignored,\n+\t\t\tN_(\"stash ignored untracked files\")),\n+\t\tOPT_BOOL('k', \"keep-index\", &keep_index_set,\n+\t\t\tN_(\"restore the index after applying the stash\")),\n+\t\tOPT__QUIET(&quiet, N_(\"be quiet, only report errors\")),\n+\t\tOPT_STRING('m', \"message\", &message, N_(\"message\"),\n+\t\t\tN_(\"stash commit message\")),\n+\t\tOPT_BOOL('p', \"patch\", &patch,\n+\t\t\tN_(\"edit current diff and apply\")),\n+\t\tOPT_END()\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, options,\n+\t\t\tgit_stash_save_usage, PARSE_OPT_STOP_AT_NON_OPTION);\n+\n+\tif (include_ignored)\n+\t\tinclude_untracked = 1;\n+\n+\tif (keep_index_set != -1)\n+\t\tkeep_index = keep_index_set;\n+\telse if (patch)\n+\t\tkeep_index = 1;\n+\n+\tif (argc != 0) {\n+\t\tint i;\n+\t\tfor (i = 0; i < argc; ++i) {\n+\t\t\tif (i != 0)\n+\t\t\t\tstrbuf_addf(&out, \" \");\n+\t\t\tstrbuf_addf(&out, \"%s\", argv[i]);\n+\t\t}\n+\t\tmessage = out.buf;\n+\t}\n+\n+\tret = do_push_stash(prefix, message, keep_index, include_untracked,\n+\t\t\tinclude_ignored, patch, NULL);\n+\tstrbuf_release(&out);\n+\treturn ret;\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 (index) {\n+\t\tif (hashcmp(info->b_tree.hash, info->i_tree.hash) == 0 || hashcmp(c_tree.hash, info->i_tree.hash) == 0) {\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) == 0)\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\tret = reset_tree(index_tree, 0, 0);\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\tret = reset_tree(c_tree, 0, 1);\n+\t\tif (ret)\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+\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 == 0) {\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 list_stash(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct option options[] = {\n+\t\tOPT_END()\n+\t};\n+\n+\tstruct object_id obj;\n+\tstruct argv_array args = ARGV_ARRAY_INIT;\n+\tint ret;\n+\n+\targc = parse_options(argc, argv, prefix, options,\n+\t\t\tgit_stash_list_usage, PARSE_OPT_KEEP_UNKNOWN);\n+\n+\tif (get_sha1(ref_stash, obj.hash))\n+\t\treturn 0;\n+\n+\targv_array_pushl(&args, \"log\", \"--format=%gd: %gs\", \"-g\", \"--first-parent\", \"-m\", NULL);\n+\targv_array_pushv(&args, argv);\n+\targv_array_push(&args, ref_stash);\n+\tret = cmd_log(args.argc, args.argv, prefix);\n+\tif (ret)\n+\t\treturn 1;\n+\n+\treturn 0;\n+}\n+\n+static int show_stash(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct argv_array args = ARGV_ARRAY_INIT;\n+\tstruct stash_info info;\n+\tconst char *commit = NULL;\n+\tint numstat = 0;\n+\tint patch = 0;\n+\tint ret;\n+\n+\tstruct option options[] = {\n+\t\tOPT_BOOL(0, \"numstat\", &numstat,\n+\t\t\tN_(\"Shows number of added and deleted lines in decimal notation\")),\n+\t\tOPT_BOOL('p', \"patch\", &patch,\n+\t\t\tN_(\"Generate patch\")),\n+\t\tOPT_END()\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, options,\n+\t\t\tgit_stash_show_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+\targv_array_push(&args, \"diff\");\n+\tif (numstat)\n+\t\targv_array_push(&args, \"--numstat\");\n+\telse if (patch)\n+\t\targv_array_push(&args, \"-p\");\n+\telse\n+\t\targv_array_push(&args, \"--stat\");\n+\n+\targv_array_push(&args, sha1_to_hex(info.b_commit.hash));\n+\targv_array_push(&args, sha1_to_hex(info.w_commit.hash));\n+\tret = cmd_diff(args.argc, args.argv, prefix);\n+\treturn ret;\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 == 0 && info.is_stash_ref)\n+\t\tret = do_drop_stash(prefix, &info);\n+\n+\treturn ret;\n+}\n+\n+int cmd_stash(int argc, const char **argv, const char *prefix)\n+{\n+\tint result = 0;\n+\tpid_t pid = getpid();\n+\n+\tstruct option options[] = {\n+\t\tOPT_END()\n+\t};\n+\n+\tgit_config(git_default_config, NULL);\n+\n+\txsnprintf(stash_index_path, 64, \".git/index.stash.%d\", pid);\n+\n+\targc = parse_options(argc, argv, prefix, options, git_stash_usage,\n+\t\tPARSE_OPT_KEEP_UNKNOWN|PARSE_OPT_KEEP_DASHDASH);\n+\n+\tif (argc < 1) {\n+\t\tresult = do_push_stash(NULL, prefix, 0, 0, 0, 0, NULL);\n+\t} else if (!strcmp(argv[0], \"list\"))\n+\t\tresult = list_stash(argc, argv, prefix);\n+\telse if (!strcmp(argv[0], \"show\"))\n+\t\tresult = show_stash(argc, argv, prefix);\n+\telse if (!strcmp(argv[0], \"save\"))\n+\t\tresult = save_stash(argc, argv, prefix);\n+\telse if (!strcmp(argv[0], \"push\"))\n+\t\tresult = push_stash(argc, argv, prefix);\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], \"create\"))\n+\t\tresult = create_stash(argc, argv, prefix);\n+\telse if (!strcmp(argv[0], \"store\"))\n+\t\tresult = store_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\tif (argv[0][0] == '-') {\n+\t\t\tstruct argv_array args = ARGV_ARRAY_INIT;\n+\t\t\targv_array_push(&args, \"push\");\n+\t\t\targv_array_pushv(&args, argv);\n+\t\t\tresult = push_stash(args.argc, args.argv, prefix);\n+\t\t\tif (!result)\n+\t\t\t\tprintf_ln(_(\"To restore them type \\\"git stash apply\\\"\"));\n+\t\t} else {\n+\t\t\terror(_(\"unknown subcommand: %s\"), argv[0]);\n+\t\t\tresult = 1;\n+\t\t}\n+\t}\n+\n+\treturn result;\n+}\ndiff --git a/git-stash.sh b/contrib/examples/git-stash.sh\nsimilarity index 100%\nrename from git-stash.sh\nrename to contrib/examples/git-stash.sh\ndiff --git a/git.c b/git.c\nindex 8ff44f081d..4531011cdc 100644\n--- a/git.c\n+++ b/git.c\n@@ -491,6 +491,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\", cmd_stash, 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.13.0\n\n"},{"id":"321938","messageId":"b67c04ae-a7f4-bd53-6b96-6482e6b83356@teichroeb.net","threadId":"46140","inReplyTo":"20170608005535.13080-1-joel@teichroeb.net","subject":"Re: [PATCH v4 0/5] Implement git stash as a builtin command","fromName":"Joel Teichroeb","fromEmail":"joel@teichroeb.net","sentAt":"2017-06-11T17:40:11Z","receivedAt":"2017-06-11T17:40:18Z","isPatch":true,"sender":{"key":"joel@teichroeb.net","avatar":"https://avatars.githubusercontent.com/u/240865?v=4"},"body":"I haven't seen any response. Would it be possible for anyone to review?\n\nThanks,\nJoel\n\nOn 6/7/2017 5:55 PM, Joel Teichroeb wrote:\n> I've rewritten git stash as a builtin c command. All tests pass,\n> and I've added two new tests. Test coverage is around 95% with the\n> only things missing coverage being error handlers.\n>\n> Changes since v3:\n> * Fixed formatting issues\n> * Fixed a bug with stash branch and added a new test for it\n> * Fixed review comments\n>\n> Outstanding issue:\n> * Not all argv array memory is cleaned up\n>\n> Joel Teichroeb (5):\n> stash: add test for stash create with no files\n> stash: Add a test for when apply fails during stash branch\n> stash: add test for stashing in a detached state\n> merge: close the index lock when not writing the new index\n> stash: implement builtin stash\n>\n> Makefile | 2 +-\n> builtin.h | 1 +\n> builtin/stash.c | 1224 +++++++++++++++++++++++++\n> git-stash.sh => contrib/examples/git-stash.sh | 0\n> git.c | 1 +\n> merge-recursive.c | 9 +-\n> t/t3903-stash.sh | 34 +\n> 7 files changed, 1267 insertions(+), 4 deletions(-)\n> create mode 100644 builtin/stash.c\n> rename git-stash.sh => contrib/examples/git-stash.sh (100%)\n>\n\n"},{"id":"321942","messageId":"20170611212739.GA7737@hank","threadId":"46140","inReplyTo":"20170608005535.13080-6-joel@teichroeb.net","subject":"Re: [PATCH v4 5/5] stash: implement builtin stash","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2017-06-11T21:27:39Z","receivedAt":"2017-06-11T21:27:43Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 06/07, Joel Teichroeb wrote:\n> Implement all git stash functionality as a builtin command\n> \n> Signed-off-by: Joel Teichroeb \n> ---\n\nThanks for working on this. A few comments from me below. Mainly on\nstash push, as that's what I'm most familiar with, and all I had time\nfor today. Hope it helps :)\n\n> Makefile | 2 +-\n> builtin.h | 1 +\n> builtin/stash.c | 1224 +++++++++++++++++++++++++\n> git-stash.sh => contrib/examples/git-stash.sh | 0\n> git.c | 1 +\n> 5 files changed, 1227 insertions(+), 1 deletion(-)\n> create mode 100644 builtin/stash.c\n> rename git-stash.sh => contrib/examples/git-stash.sh (100%)\n\n[...]\n\n> \n> +\n> +static const char * const git_stash_usage[] = {\n> +\tN_(\"git stash list []\"),\n> +\tN_(\"git stash show []\"),\n> +\tN_(\"git stash drop [-q|--quiet] []\"),\n> +\tN_(\"git stash ( pop | apply ) [--index] [-q|--quiet] []\"),\n> +\tN_(\"git stash branch []\"),\n> +\tN_(\"git stash [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]\"),\n> +\tN_(\" [-u|--include-untracked] [-a|--all] []]\"),\n\nThis is missing the newly introduced push command.\n\n> +\tN_(\"git stash clear\"),\n> +\tN_(\"git stash create []\"),\n> +\tN_(\"git stash store [-m|--message ] [-q|--quiet] \"),\n\ncreate and store are not currently advertised in the usage. I think\nthis is intentional, because those commands are intended to be used\nonly in scripts. I don't have a particularly strong opinion on\nwhether they should be added or not, but if we do add them I think we\nshould do so consciously in a separate commit, instead of adding them\non in this commit.\n\n> +\tNULL\n> +};\n> +\n> +static const char * const git_stash_list_usage[] = {\n> +\tN_(\"git stash list []\"),\n> +\tNULL\n> +};\n> +\n> +static const char * const git_stash_show_usage[] = {\n> +\tN_(\"git stash show []\"),\n> +\tNULL\n> +};\n> +\n> +static const char * const git_stash_drop_usage[] = {\n> +\tN_(\"git stash drop [-q|--quiet] []\"),\n> +\tNULL\n> +};\n> +\n> +static const char * const git_stash_pop_usage[] = {\n> +\tN_(\"git stash pop [--index] [-q|--quiet] []\"),\n> +\tNULL\n> +};\n> +\n> +static const char * const git_stash_apply_usage[] = {\n> +\tN_(\"git stash apply [--index] [-q|--quiet] []\"),\n> +\tNULL\n> +};\n> +\n> +static const char * const git_stash_branch_usage[] = {\n> +\tN_(\"git stash branch []\"),\n> +\tNULL\n> +};\n> +\n> +static const char * const git_stash_save_usage[] = {\n> +\tN_(\"git stash [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]\"),\n> +\tN_(\" [-u|--include-untracked] [-a|--all] []]\"),\n> +\tNULL\n> +};\n> +\n> +static const char * const git_stash_clear_usage[] = {\n> +\tN_(\"git stash clear\"),\n> +\tNULL\n> +};\n> +\n> +static const char * const git_stash_create_usage[] = {\n> +\tN_(\"git stash create []\"),\n> +\tNULL\n> +};\n> +\n> +static const char * const git_stash_store_usage[] = {\n> +\tN_(\"git stash store [-m|--message ] [-q|--quiet] \"),\n> +\tNULL\n> +};\n> +\n\n[...]\n\n> +\n> +static int do_push_stash(const char *prefix, const char *message,\n> +\tint keep_index, int include_untracked, int include_ignored, int patch,\n> +\tconst char **argv)\n\nargv here is a list of pathspecs. I think this would be a bit easier\nto follow if the argument was called \"pathspecs\". \n\n> +{\n> +\tint res;\n> +\tstruct stash_info info;\n> +\n> +\tif (patch && include_untracked)\n> +\t\treturn error(_(\"can't use --patch and --include-untracked or --all at the same time\"));\n> +\n> +\tif (!include_untracked) {\n> +\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\n> +\t\tcp.git_cmd = 1;\n> +\t\tcp.no_stdout = 1;\n> +\t\targv_array_pushl(&cp.args, \"ls-files\", \"--error-unmatch\", \"--\", NULL);\n> +\t\tif (argv)\n> +\t\t\targv_array_pushv(&cp.args, argv);\n> +\t\tres = run_command(&cp);\n> +\t\tif (res)\n> +\t\t\treturn 1;\n> +\t}\n> +\n> +\tread_cache_preload(NULL);\n> +\trefresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL);\n> +\tif (check_no_changes(prefix, include_untracked, include_ignored, argv)) {\n> +\t\tprintf_ln(_(\"No local changes to save\"));\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tif (!reflog_exists(ref_stash)) {\n> +\t\tif (do_clear_stash())\n> +\t\t\treturn error(_(\"Cannot initialize stash\"));\n> +\t}\n> +\n> +\tif (do_create_stash(&info, prefix, message, include_untracked, include_ignored, patch, argv))\n> +\t\treturn 1;\n> +\tres = do_store_stash(prefix, 1, info.message, info.w_commit);\n> +\n> +\tif (res == 0 && !quiet)\n\nSometimes the function is used directly in the if, and sometimes the\nres variable is used. I think it would be nicer to consistently use\none or the other. My preference would be to always use the functions\ndirectly, as res is not used anywhere other than the if.\n\nAlso I think we prefer using (!res) instead of (res == 0) for checking\nreturn values.\n\n> +\t\tprintf(_(\"Saved working directory and index state %s\"), info.message);\n> +\n> +\tif (!patch) {\n> +\t\tif (argv && *argv) {\n> +\t\t\tstruct argv_array args = ARGV_ARRAY_INIT;\n> +\t\t\tstruct argv_array args2 = ARGV_ARRAY_INIT;\n> +\t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\t\t\tstruct strbuf out = STRBUF_INIT;\n> +\t\t\targv_array_pushl(&args, \"reset\", \"--quiet\", \"--\", NULL);\n> +\t\t\targv_array_pushv(&args, argv);\n> +\t\t\tcmd_reset(args.argc, args.argv, prefix);\n> +\n> +\t\t\tcp.git_cmd = 1;\n> +\t\t\targv_array_pushl(&cp.args, \"ls-files\", \"-z\", \"--modified\", \"--\",\n> +\t\t\t\tNULL);\n> +\t\t\targv_array_pushv(&cp.args, argv);\n> +\t\t\tpipe_command(&cp, NULL, 0, &out, 0, NULL, 0);\n> +\n> +\t\t\tchild_process_init(&cp);\n> +\t\t\tcp.git_cmd = 1;\n> +\t\t\targv_array_pushl(&cp.args, \"checkout-index\", \"-z\", \"--force\",\n> +\t\t\t\t\"--stdin\", NULL);\n> +\t\t\tpipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0);\n> +\t\t\tstrbuf_release(&out);\n> +\n> +\t\t\targv_array_pushl(&args2, \"clean\", \"--force\", \"-d\", \"--quiet\", \"--\",\n> +\t\t\t\tNULL);\n> +\t\t\targv_array_pushv(&args2, argv);\n> +\t\t\tcmd_clean(args2.argc, args2.argv, prefix);\n> +\t\t} else {\n> +\t\t\tstruct argv_array args = ARGV_ARRAY_INIT;\n> +\t\t\targv_array_pushl(&args, \"reset\", \"--hard\", \"--quiet\", NULL);\n> +\t\t\tcmd_reset(args.argc, args.argv, prefix);\n> +\t\t}\n> +\n> +\t\tif (include_untracked) {\n> +\t\t\tstruct argv_array args = ARGV_ARRAY_INIT;\n> +\t\t\targv_array_pushl(&args, \"clean\", \"--force\", \"--quiet\", \"-d\", NULL);\n> +\t\t\tif (include_ignored)\n> +\t\t\t\targv_array_push(&args, \"-x\");\n> +\t\t\targv_array_push(&args, \"--\");\n> +\t\t\tif (argv)\n> +\t\t\t\targv_array_pushv(&args, argv);\n> +\t\t\tcmd_clean(args.argc, args.argv, prefix);\n> +\t\t}\n> +\n> +\t\tif (keep_index) {\n> +\t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\t\t\tstruct strbuf out = STRBUF_INIT;\n> +\n> +\t\t\treset_tree(info.i_tree, 0, 1);\n> +\n> +\t\t\tcp.git_cmd = 1;\n> +\t\t\targv_array_pushl(&cp.args, \"ls-files\", \"-z\", \"--modified\", \"--\",\n> +\t\t\t\tNULL);\n> +\t\t\targv_array_pushv(&cp.args, argv);\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, \"checkout-index\", \"-z\", \"--force\",\n> +\t\t\t\t\"--stdin\", NULL);\n> +\t\t\tif (pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0))\n> +\t\t\t\treturn 1;\n> +\t\t\tstrbuf_release(&out);\n> +\t\t}\n> +\t} else {\n> +\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\t\tcp.git_cmd = 1;\n> +\t\targv_array_pushl(&cp.args, \"apply\", \"-R\", NULL);\n> +\t\tif (pipe_command(&cp, info.patch, strlen(info.patch), NULL, 0, NULL, 0))\n> +\t\t\treturn error(_(\"Cannot remove worktree changes\"));\n> +\n> +\t\tif (!keep_index) {\n> +\t\t\tstruct argv_array args = ARGV_ARRAY_INIT;\n> +\t\t\targv_array_pushl(&args, \"reset\", \"--quiet\", \"--\", NULL);\n> +\t\t\tif (argv)\n> +\t\t\t\targv_array_pushv(&args, argv);\n> +\t\t\tcmd_reset(args.argc, args.argv, prefix);\n> +\t\t}\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +static int push_stash(int argc, const char **argv, const char *prefix)\n> +{\n> +\tconst char *message = NULL;\n> +\tint include_untracked = 0;\n> +\tint include_ignored = 0;\n> +\tint patch = 0;\n> +\tint keep_index_set = -1;\n> +\tint keep_index = 0;\n> +\tstruct option options[] = {\n> +\t\tOPT_BOOL('u', \"include-untracked\", &include_untracked,\n> +\t\t\tN_(\"stash untracked filed\")),\n> +\t\tOPT_BOOL('a', \"all\", &include_ignored,\n> +\t\t\tN_(\"stash ignored untracked files\")),\n> +\t\tOPT_BOOL('k', \"keep-index\", &keep_index_set,\n> +\t\t\tN_(\"restore the index after applying the stash\")),\n> +\t\tOPT__QUIET(&quiet, N_(\"be quiet, only report errors\")),\n> +\t\tOPT_STRING('m', \"message\", &message, N_(\"message\"),\n> +\t\t\tN_(\"stash commit message\")),\n> +\t\tOPT_BOOL('p', \"patch\", &patch,\n> +\t\t\tN_(\"edit current diff and apply\")),\n> +\t\tOPT_END()\n> +\t};\n> +\n> +\targc = parse_options(argc, argv, prefix, options,\n> +\t\t\t\tgit_stash_save_usage, PARSE_OPT_STOP_AT_NON_OPTION);\n\n\"git_stash_save_usage\" is slightly different from the usage for\npush, which we should display here. We probably should introduce\n\"git_stash_push_usage\" for this.\n\n> +\tif (include_ignored)\n> +\t\tinclude_untracked = 1;\n> +\n> +\tif (keep_index_set != -1)\n> +\t\tkeep_index = keep_index_set;\n> +\telse if (patch)\n> +\t\tkeep_index = 1;\n> +\n> +\treturn do_push_stash(prefix, message, keep_index, include_untracked, include_ignored, patch, argv);\n> +}\n> +\n\n[...]\n\n> +\n> +int cmd_stash(int argc, const char **argv, const char *prefix)\n> +{\n> +\tint result = 0;\n> +\tpid_t pid = getpid();\n> +\n> +\tstruct option options[] = {\n> +\t\tOPT_END()\n> +\t};\n> +\n> +\tgit_config(git_default_config, NULL);\n> +\n> +\txsnprintf(stash_index_path, 64, \".git/index.stash.%d\", pid);\n> +\n> +\targc = parse_options(argc, argv, prefix, options, git_stash_usage,\n> +\t\tPARSE_OPT_KEEP_UNKNOWN|PARSE_OPT_KEEP_DASHDASH);\n> +\n> +\tif (argc < 1) {\n> +\t\tresult = do_push_stash(NULL, prefix, 0, 0, 0, 0, NULL);\n> +\t} else if (!strcmp(argv[0], \"list\"))\n> +\t\tresult = list_stash(argc, argv, prefix);\n> +\telse if (!strcmp(argv[0], \"show\"))\n> +\t\tresult = show_stash(argc, argv, prefix);\n> +\telse if (!strcmp(argv[0], \"save\"))\n> +\t\tresult = save_stash(argc, argv, prefix);\n> +\telse if (!strcmp(argv[0], \"push\"))\n> +\t\tresult = push_stash(argc, argv, prefix);\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], \"create\"))\n> +\t\tresult = create_stash(argc, argv, prefix);\n> +\telse if (!strcmp(argv[0], \"store\"))\n> +\t\tresult = store_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\tif (argv[0][0] == '-') {\n> +\t\t\tstruct argv_array args = ARGV_ARRAY_INIT;\n> +\t\t\targv_array_push(&args, \"push\");\n> +\t\t\targv_array_pushv(&args, argv);\n> +\t\t\tresult = push_stash(args.argc, args.argv, prefix);\n\nThis is a bit of a change in behaviour to what we currently have.\n\nThe rules we decided on are as follows:\n\n - \"git stash -p\" is an alias for \"git stash push -p\".\n - \"git stash\" with only option arguments is an alias for \"git stash\n push\" with those same arguments. non-option arguments can be\n specified after a \"--\" for disambiguation.\n\nThe above makes \"git stash -*\" a alias for \"git stash push -*\". This\nwould result in a change of behaviour, for example in the case where\nsomeone would use \"git stash -this is a test-\". In that case the\ncurrent behaviour is to create a stash with the message \"-this is a\ntest-\", while the above would end up making git stash error out. The\ndiscussion on how we came up with those rules can be found at\nhttp://public-inbox.org/git/20170206161432.zvpsqegjspaa2l5l@sigill.intra.peff.net/. \n\n> +\t\t\tif (!result)\n> +\t\t\t\tprintf_ln(_(\"To restore them type \\\"git stash apply\\\"\"));\n\nIn the shell script this is only displayed when the stash_push in the\ncase where git stash is invoked with no arguments, not in the push\ncase if I read this correctly. So the two lines above should go in\nthe (argc < 1) case I think.\n\n> +\t\t} else {\n> +\t\t\terror(_(\"unknown subcommand: %s\"), argv[0]);\n\nCurrently we're displaying the whole usage string in this case. I\nthink we should keep doing that.\n\n> +\t\t\tresult = 1;\n> +\t\t}\n> +\t}\n> +\n> +\treturn result;\n> +}\n> diff --git a/git-stash.sh b/contrib/examples/git-stash.sh\n> similarity index 100%\n> rename from git-stash.sh\n> rename to contrib/examples/git-stash.sh\n> diff --git a/git.c b/git.c\n> index 8ff44f081d..4531011cdc 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -491,6 +491,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\", cmd_stash, 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.13.0\n> \n"},{"id":"322131","messageId":"xmqqk24f65wo.fsf@gitster.mtv.corp.google.com","threadId":"46140","inReplyTo":"20170608005535.13080-2-joel@teichroeb.net","subject":"Re: [PATCH v4 1/5] stash: add test for stash create with no files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-13T19:31:35Z","receivedAt":"2017-06-13T19:31:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joel Teichroeb writes:\n\n> Ensure the command gives the correct return code\n\nOK. When you know what the correct return code is, we'd prefer to\nsee it spelled out, i.e.\n\n Ensure that the command succeeds.\n\nOr did you mean that the command outputs nothing?\n\nThe test itself looks obviously correct ;-)\n\n> Signed-off-by: Joel Teichroeb \n> ---\n> t/t3903-stash.sh | 8 ++++++++\n> 1 file changed, 8 insertions(+)\n>\n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> index 3b4bed5c9a..cc923e6335 100755\n> --- a/t/t3903-stash.sh\n> +++ b/t/t3903-stash.sh\n> @@ -444,6 +444,14 @@ test_expect_failure 'stash file to directory' '\n> \ttest foo = \"$(cat file/file)\"\n> '\n> \n> +test_expect_success 'stash create - no changes' '\n> +\tgit stash clear &&\n> +\ttest_when_finished \"git reset --hard HEAD\" &&\n> +\tgit reset --hard &&\n> +\tgit stash create >actual &&\n> +\ttest_must_be_empty actual\n> +'\n> +\n> test_expect_success 'stash branch - no stashes on stack, stash-like argument' '\n> \tgit stash clear &&\n> \ttest_when_finished \"git reset --hard HEAD\" &&\n"},{"id":"322132","messageId":"xmqqefun65h0.fsf@gitster.mtv.corp.google.com","threadId":"46140","inReplyTo":"20170608005535.13080-3-joel@teichroeb.net","subject":"Re: [PATCH v4 2/5] stash: Add a test for when apply fails during stash branch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-13T19:40:59Z","receivedAt":"2017-06-13T19:41:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joel Teichroeb writes:\n\n> If the return value of merge recurisve is not checked, the stash could end\n> up being dropped even though it was not applied properly\n\ns/recurisve/recursive/\n\n> Signed-off-by: Joel Teichroeb \n> ---\n> t/t3903-stash.sh | 14 ++++++++++++++\n> 1 file changed, 14 insertions(+)\n>\n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> index cc923e6335..5399fb05ca 100755\n> --- a/t/t3903-stash.sh\n> +++ b/t/t3903-stash.sh\n> @@ -656,6 +656,20 @@ test_expect_success 'stash branch should not drop the stash if the branch exists\n> \tgit rev-parse stash@{0} --\n> '\n> \n> +test_expect_success 'stash branch should not drop the stash if the apply fails' '\n> +\tgit stash clear &&\n> +\tgit reset HEAD~1 --hard &&\n> +\techo foo >file &&\n> +\tgit add file &&\n> +\tgit commit -m initial &&\n\nIt's not quite intuitive to call a non-root commit \"initial\" ;-)\n\n> +\techo bar >file &&\n> +\tgit stash &&\n> +\techo baz >file &&\n\nOK, so 'file' has 'foo' in HEAD, 'bar' in the stash@{0}.\n\n> +\ttest_when_finished \"git checkout master\" &&\n> +\ttest_must_fail git stash branch new_branch stash@{0} &&\n\nHmph. Do we blindly checkout new_branch out of stash@{0}^1 and\nunstash, but because 'file' in the working tree is dirty, we fail to\napply the stash and stop?\n\nThis sounds like a bug to me. Shouldn't we be staying on 'master',\nand fail without even creating 'new_branch', when this happens?\n\nIn any case we should be testing what branch we are on after this\nstep. What branch should we be on after \"git stash branch\" fails?\n\n> +\tgit rev-parse stash@{0} --\n> +'\n> +\n> test_expect_success 'stash apply shows status same as git status (relative to current directory)' '\n> \tgit stash clear &&\n> \techo 1 >subdir/subfile1 &&\n"},{"id":"322133","messageId":"xmqqa85b65a8.fsf@gitster.mtv.corp.google.com","threadId":"46140","inReplyTo":"20170608005535.13080-4-joel@teichroeb.net","subject":"Re: [PATCH v4 3/5] stash: add test for stashing in a detached state","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-13T19:45:03Z","receivedAt":"2017-06-13T19:45:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joel Teichroeb writes:\n\n> Signed-off-by: Joel Teichroeb \n> ---\n> t/t3903-stash.sh | 12 ++++++++++++\n> 1 file changed, 12 insertions(+)\n>\n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> index 5399fb05ca..ce4c8fe3d6 100755\n> --- a/t/t3903-stash.sh\n> +++ b/t/t3903-stash.sh\n> @@ -822,6 +822,18 @@ test_expect_success 'create with multiple arguments for the message' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'create in a detached state' '\n> +\ttest_when_finished \"git checkout master\" &&\n> +\tgit checkout HEAD~1 &&\n> +\t>foo &&\n> +\tgit add foo &&\n> +\tSTASH_ID=$(git stash create) &&\n> +\tHEAD_ID=$(git rev-parse --short HEAD) &&\n> +\techo \"WIP on (no branch): ${HEAD_ID} initial\" >expect &&\n> +\tgit show --pretty=%s -s ${STASH_ID} >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nHmph. Is the title automatically given to the stash the\nonly/primary thing that is of interest to us in this test? I think\nwe care more about that we record the right thing in the resulting\nstash and also after creating the stash the working tree and the\nindex becomes clean. Shouldn't we be testing that?\n\nIf \"git stash create\" fails to make the working tree and the index\nclean, then \"git checkout master\" run by when-finished will carry\nthe local modifications with us, which probably is not what you\nmeant. You'd need \"reset --hard\" there, too, perhaps?\n\n> test_expect_success 'stash -- stashes and restores the file' '\n> \t>foo &&\n> \t>bar &&\n"},{"id":"322134","messageId":"xmqq60fz6565.fsf@gitster.mtv.corp.google.com","threadId":"46140","inReplyTo":"20170608005535.13080-5-joel@teichroeb.net","subject":"Re: [PATCH v4 4/5] merge: close the index lock when not writing the new index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-13T19:47:30Z","receivedAt":"2017-06-13T19:47:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joel Teichroeb writes:\n\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> Signed-off-by: Joel Teichroeb \n> ---\n\nThis one makes sense. \n\nSo far, nobody who calls this function performs further index\nmanipulations and letting the atexit handlers automatically release\nthe lock was sufficient. This allows new callers to do more work on\nthe index after a merge finishes.\n\n\n> merge-recursive.c | 9 ++++++---\n> 1 file changed, 6 insertions(+), 3 deletions(-)\n>\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index ae5238d82c..16bb5512ef 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -2145,9 +2145,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"},{"id":"322135","messageId":"CA+CzEk8U6P58OqruPkP1HePFurNWjgf=Q-h=Hu57zoHpDeenmA@mail.gmail.com","threadId":"46140","inReplyTo":"xmqqa85b65a8.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 3/5] stash: add test for stashing in a detached state","fromName":"Joel Teichroeb","fromEmail":"joel@teichroeb.net","sentAt":"2017-06-13T19:48:54Z","receivedAt":"2017-06-13T19:49:21Z","isPatch":true,"sender":{"key":"joel@teichroeb.net","avatar":"https://avatars.githubusercontent.com/u/240865?v=4"},"body":"On Tue, Jun 13, 2017 at 12:45 PM, Junio C Hamano wrote:\n> Joel Teichroeb writes:\n>\n>> Signed-off-by: Joel Teichroeb \n>> ---\n>> t/t3903-stash.sh | 12 ++++++++++++\n>> 1 file changed, 12 insertions(+)\n>>\n>> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n>> index 5399fb05ca..ce4c8fe3d6 100755\n>> --- a/t/t3903-stash.sh\n>> +++ b/t/t3903-stash.sh\n>> @@ -822,6 +822,18 @@ test_expect_success 'create with multiple arguments for the message' '\n>> test_cmp expect actual\n>> '\n>>\n>> +test_expect_success 'create in a detached state' '\n>> + test_when_finished \"git checkout master\" &&\n>> + git checkout HEAD~1 &&\n>> + >foo &&\n>> + git add foo &&\n>> + STASH_ID=$(git stash create) &&\n>> + HEAD_ID=$(git rev-parse --short HEAD) &&\n>> + echo \"WIP on (no branch): ${HEAD_ID} initial\" >expect &&\n>> + git show --pretty=%s -s ${STASH_ID} >actual &&\n>> + test_cmp expect actual\n>> +'\n>\n> Hmph. Is the title automatically given to the stash the\n> only/primary thing that is of interest to us in this test? I think\n> we care more about that we record the right thing in the resulting\n> stash and also after creating the stash the working tree and the\n> index becomes clean. Shouldn't we be testing that?\n\nIn this case, the title is really what I wanted to test. There are\nother tests already to make sure that stash create works, but there\nwere no tests to ensure that a stash was created with the correct\ntitle when not on a branch. That being said though, I'll add more\nvalidation as more validation is always better.\n\n>\n> If \"git stash create\" fails to make the working tree and the index\n> clean, then \"git checkout master\" run by when-finished will carry\n> the local modifications with us, which probably is not what you\n> meant. You'd need \"reset --hard\" there, too, perhaps?\n\nAgreed.\n\n>\n>> test_expect_success 'stash -- stashes and restores the file' '\n>> >foo &&\n>> >bar &&\n"},{"id":"322136","messageId":"CA+CzEk8QiSu4heMDsx7XC729UEPNm6hdZfs9W5uwoJBvuLWr+w@mail.gmail.com","threadId":"46140","inReplyTo":"xmqqefun65h0.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 2/5] stash: Add a test for when apply fails during stash branch","fromName":"Joel Teichroeb","fromEmail":"joel@teichroeb.net","sentAt":"2017-06-13T19:54:37Z","receivedAt":"2017-06-13T19:55:03Z","isPatch":true,"sender":{"key":"joel@teichroeb.net","avatar":"https://avatars.githubusercontent.com/u/240865?v=4"},"body":"On Tue, Jun 13, 2017 at 12:40 PM, Junio C Hamano wrote:\n> Joel Teichroeb writes:\n>\n>> If the return value of merge recurisve is not checked, the stash could end\n>> up being dropped even though it was not applied properly\n>\n> s/recurisve/recursive/\n>\n>> Signed-off-by: Joel Teichroeb \n>> ---\n>> t/t3903-stash.sh | 14 ++++++++++++++\n>> 1 file changed, 14 insertions(+)\n>>\n>> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n>> index cc923e6335..5399fb05ca 100755\n>> --- a/t/t3903-stash.sh\n>> +++ b/t/t3903-stash.sh\n>> @@ -656,6 +656,20 @@ test_expect_success 'stash branch should not drop the stash if the branch exists\n>> git rev-parse stash@{0} --\n>> '\n>>\n>> +test_expect_success 'stash branch should not drop the stash if the apply fails' '\n>> + git stash clear &&\n>> + git reset HEAD~1 --hard &&\n>> + echo foo >file &&\n>> + git add file &&\n>> + git commit -m initial &&\n>\n> It's not quite intuitive to call a non-root commit \"initial\" ;-)\n>\n>> + echo bar >file &&\n>> + git stash &&\n>> + echo baz >file &&\n>\n> OK, so 'file' has 'foo' in HEAD, 'bar' in the stash@{0}.\n>\n>> + test_when_finished \"git checkout master\" &&\n>> + test_must_fail git stash branch new_branch stash@{0} &&\n>\n> Hmph. Do we blindly checkout new_branch out of stash@{0}^1 and\n> unstash, but because 'file' in the working tree is dirty, we fail to\n> apply the stash and stop?\n>\n> This sounds like a bug to me. Shouldn't we be staying on 'master',\n> and fail without even creating 'new_branch', when this happens?\n\nGood point. The existing behavior is to create new_branch and check it\nout. I'm not sure what the correct state should be then. Create\nnew_branch, checkout new_branch, fail to apply, checkout master?\nShould it then delete new_branch? Is there a way instead to test\napplying the stash before creating the branch without actually\napplying it? Something like putting merge_recursive into some kind of\ndry-run mode?\n\n>\n> In any case we should be testing what branch we are on after this\n> step. What branch should we be on after \"git stash branch\" fails?\n>\n>> + git rev-parse stash@{0} --\n>> +'\n>> +\n>> test_expect_success 'stash apply shows status same as git status (relative to current directory)' '\n>> git stash clear &&\n>> echo 1 >subdir/subfile1 &&\n"},{"id":"322138","messageId":"xmqq1sqn61w6.fsf@gitster.mtv.corp.google.com","threadId":"46140","inReplyTo":"CA+CzEk8U6P58OqruPkP1HePFurNWjgf=Q-h=Hu57zoHpDeenmA@mail.gmail.com","subject":"Re: [PATCH v4 3/5] stash: add test for stashing in a detached state","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-13T20:58:17Z","receivedAt":"2017-06-13T20:58:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joel Teichroeb writes:\n\n>>> +test_expect_success 'create in a detached state' '\n>>> + test_when_finished \"git checkout master\" &&\n>>> + git checkout HEAD~1 &&\n>>> + >foo &&\n>>> + git add foo &&\n>>> + STASH_ID=$(git stash create) &&\n>>> + HEAD_ID=$(git rev-parse --short HEAD) &&\n>>> + echo \"WIP on (no branch): ${HEAD_ID} initial\" >expect &&\n>>> + git show --pretty=%s -s ${STASH_ID} >actual &&\n>>> + test_cmp expect actual\n>>> +'\n>>\n>> Hmph. Is the title automatically given to the stash the\n>> only/primary thing that is of interest to us in this test? I think\n>> we care more about that we record the right thing in the resulting\n>> stash and also after creating the stash the working tree and the\n>> index becomes clean. Shouldn't we be testing that?\n>\n> In this case, the title is really what I wanted to test. There are\n> other tests already to make sure that stash create works, but there\n> were no tests to ensure that a stash was created with the correct\n> title when not on a branch.\n\nAh, OK.\n\nThanks.\n"},{"id":"322465","messageId":"xmqqbmpnyklk.fsf@gitster.mtv.corp.google.com","threadId":"46140","inReplyTo":"20170608005535.13080-6-joel@teichroeb.net","subject":"Re: [PATCH v4 5/5] stash: implement builtin stash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-16T16:15:51Z","receivedAt":"2017-06-16T16:16:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joel Teichroeb writes:\n\n> diff --git a/builtin/stash.c b/builtin/stash.c\n> new file mode 100644\n> index 0000000000..a9680f2909\n> --- /dev/null\n> +++ b/builtin/stash.c\n> ...\n> +static const char *ref_stash = \"refs/stash\";\n> +static int quiet = 0;\n\nLet BSS take care of zero-initialization, i.e. drop \" = 0\".\n\n> +static int untracked_files(struct strbuf *out, int include_untracked,\n> +\t\tint include_ignored, const char **argv)\n> +{\n> +\tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\tcp.git_cmd = 1;\n> +\targv_array_pushl(&cp.args, \"ls-files\", \"-o\", \"-z\", NULL);\n> +\tif (include_untracked && !include_ignored)\n> +\t\targv_array_push(&cp.args, \"--exclude-standard\");\n> +\targv_array_push(&cp.args, \"--\");\n> +\tif (argv)\n> +\t\targv_array_pushv(&cp.args, argv);\n> +\treturn pipe_command(&cp, NULL, 0, out, 0, NULL, 0);\n> +}\n\nSeeing that include_untracked and include_ignored always come in a\npair throughout the program, I wondered if it may be better to use\na single \"unsigned include\" with two bits\n\n #define INCLUDE_UNTRACKED 01\n #define INCLUDE_IGNORED 02\n\nto pass around. As long as we envision that we will not gain other\nkind of \"do we include X?\" in the future, what your patch does is\nfine, I would say.\n\n> +static int check_no_changes(const char *prefix, int include_untracked,\n> +\t\tint include_ignored, const char **argv)\n> +{\n> +\tstruct argv_array args1 = ARGV_ARRAY_INIT;\n> +\tstruct argv_array args2 = ARGV_ARRAY_INIT;\n> +\tstruct strbuf out = STRBUF_INIT;\n> +\tint ret;\n> +\n> +\targv_array_pushl(&args1, \"diff-index\", \"--quiet\", \"--cached\", \"HEAD\",\n> +\t\t\"--ignore-submodules\", \"--\", NULL);\n> +\tif (argv)\n> +\t\targv_array_pushv(&args1, argv);\n> +\n> +\targv_array_pushl(&args2, \"diff-files\", \"--quiet\", \"--ignore-submodules\",\n> +\t\t\"--\", NULL);\n> +\tif (argv)\n> +\t\targv_array_pushv(&args2, argv);\n> +\n> +\tif (include_untracked)\n> +\t\tuntracked_files(&out, include_untracked, include_ignored, argv);\n> +\n> +\tret = cmd_diff_index(args1.argc, args1.argv, prefix) == 0 &&\n> +\t\t\tcmd_diff_files(args2.argc, args2.argv, prefix) == 0 &&\n> +\t\t\t(!include_untracked || out.len == 0);\n\nWhen diff_index() finds there are modified paths, you do not have to\ncall diff_files() or untracked_files() at all (and you do not even\nhave to set-up args2). Doesn't the above leak args.argv[] when &&\nshort circuits?\n\n This is a tangent, but it is somewhat unusual to call cmd_foo()\n as a subroutine. I think cmd_diff_*() are written reasonably\n well to allow them to be called in this way safely, and there\n are a few existing commands that already do so, so it may be OK.\n\n> +\tstrbuf_release(&out);\n> +\treturn ret;\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 (strlen(commit) < 3) {\n\nThis is a bit sloppy (the original is even sloppier but it is in\nshell, so it is more excusable ;-). This code thinks that anything\nwith @{} must be longer than 3 because it has to have @{},\nand that is where the strlen() comes from, I think, but the magic\nnumber 3 appears without explanation here.\n\nWhat the code actually needs to do is to see if the stash entry\nspecification came in \"commit\" (which by the way is a bit misnamed\nparameter) is a bare number and use refs/stash@{} only\nin that case, I think. strspn() might be useful.\n\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> +\tstrbuf_addf(&u_tree_rev, \"%s^3:\", revision);\n> +\n> +\tret = (get_sha1(w_commit_rev.buf, info->w_commit.hash) == 0 &&\n> +\t\tget_sha1(b_commit_rev.buf, info->b_commit.hash) == 0 &&\n> +\t\tget_sha1(w_tree_rev.buf, info->w_tree.hash) == 0 &&\n> +\t\tget_sha1(b_tree_rev.buf, info->b_tree.hash) == 0 &&\n> +\t\tget_sha1(i_tree_rev.buf, info->i_tree.hash) == 0);\n\nIt's more conventional to check for errors with !get_sha1(params),\nnot a long-hand comparision with 0.\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 are leaking u_tree_rev.buf upon early return.\n\n> +\tinfo->has_u = get_sha1(u_tree_rev.buf, info->u_tree.hash) == 0;\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) == 0;\n\nFavor !strncmp(params) over comparision with 0.\n\nStyle: Have SP around both sides of binary operator \"-\".\n\n> +\tstrbuf_release(&out);\n> +\treturn !ret;\n\nHmph, where did we last assign ret in this code? Didn't we check\nthat value and returned error already, which means we know what !ret\nis when the control reaches here?\n\n> +}\n\nI have to move to another building, so I'll stop here for now, but\nwill continue later.\n\nThanks.\n\n"},{"id":"322498","messageId":"xmqqvanvv9be.fsf@gitster.mtv.corp.google.com","threadId":"46140","inReplyTo":"20170608005535.13080-6-joel@teichroeb.net","subject":"Re: [PATCH v4 5/5] stash: implement builtin stash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-16T22:47:49Z","receivedAt":"2017-06-16T22:47:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joel Teichroeb writes:\n\n> +static void stash_create_callback(struct diff_queue_struct *q,\n> +\t\t\t\tstruct diff_options *opt, void *cbdata)\n> +{\n> +\tint i;\n> +\n> +\tfor (i = 0; i < q->nr; i++) {\n> +\t\tstruct diff_filepair *p = q->queue[i];\n> +\t\tconst char *path = p->one->path;\n> +\t\tstruct stat st;\n\nThe order is somewhat ugly. Move \"struct stat st;\" that does not\nhave any initialization at the beginning.\n\n> +\t\tremove_file_from_index(&the_index, path);\n> +\t\tif (!lstat(path, &st))\n> +\t\t\tadd_to_index(&the_index, path, &st, 0);\n> +\t}\n> +}\n\nSo this will be called with list of paths that are different from\nthe working tree and the index, and adds all the paths the index\nknows about to the index from the working tree? Sounds OK, but I am\nnot sure if that is \"stash_create_callback()\". Surely it is _part_\nof creating a stash, but it would be better to name it to reflect\nwhich part of creating a stash this helper is about. I think this\nis about recording the working tree state, so I would have expected\n\"record\" and/or \"working_tree\" in its name.\n\n> +/*\n> + * Untracked files are stored by themselves in a parentless commit, for\n> + * ease of unpacking later.\n> + */\n> +static int save_untracked(struct stash_info *info, const char *message,\n> +\t\tint include_untracked, int include_ignored, const char **argv)\n> +{\n> +\tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\tstruct strbuf out = STRBUF_INIT;\n> +\tstruct object_id orig_tree;\n> +\tint ret;\n> +\tconst char *index_file = get_index_file();\n> +\n> +\tset_alternate_index_output(stash_index_path);\n> +\tuntracked_files(&out, include_untracked, include_ignored, argv);\n> +\n> +\tcp.git_cmd = 1;\n> +\targv_array_pushl(&cp.args, \"update-index\", \"-z\", \"--add\", \"--remove\",\n> +\t\t\"--stdin\", NULL);\n> +\targv_array_pushf(&cp.env_array, \"GIT_INDEX_FILE=%s\", stash_index_path);\n> +\n> +\tif (pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0)) {\n> +\t\tstrbuf_release(&out);\n> +\t\treturn 1;\n> +\t}\n> +\n\nOK, that's a very straight-forward way of doing this, and as we do\nnot care too much about performance in this initial conversion to C,\nit is even sensible. In a later update after this patch lands, you\nmay want to use dir.c's fill_directory() API to find the untracked\nfiles and add them yourself internally, without running ls-files (in\nuntracked_files()) or update-index (here) as subprocesses, but that\nis in the future. Let's get this round finished.\n\n> +\tstrbuf_reset(&out);\n> +\n> +\tdiscard_cache();\n> +\tread_cache_from(stash_index_path);\n> +\n> +\twrite_index_as_tree(orig_tree.hash, &the_index, stash_index_path, 0,NULL);\n\nSP before \"NULL\".\n\n> +\tdiscard_cache();\n> +\n> +\tread_cache_from(stash_index_path);\n\nHmph, what did anybody change in the on-disk stash_index (or\ncontents in the_index) since you read_cache_from()?\n\n> +\twrite_cache_as_tree(info->u_tree.hash, 0, NULL);\n\nThen you write exactly the same index contents again, this time to\ninfo->u_tree here. I am not sure why you need to do this twice, and\nI do not see how orig_tree.hash you wrote earlier is used?\n\n> +\tstrbuf_addf(&out, \"untracked files on %s\", message);\n> +\n> +\tret = commit_tree(out.buf, out.len, info->u_tree.hash, NULL,\n> +\t\t\tinfo->u_commit.hash, NULL, NULL);\n> +\tstrbuf_release(&out);\n> +\tif (ret)\n> +\t\treturn 1;\n> +\n> +\tset_alternate_index_output(index_file);\n> +\tdiscard_cache();\n> +\tread_cache();\n> +\n> +\treturn 0;\n> +}\n\nOK, except for minor nits, this seems to correctly replicate what\nu_commit=$(...) does in create_stash shell function in the original.\n\n> +static int save_working_tree(struct stash_info *info, const char *prefix,\n> +\t\tconst char **argv)\n> +{\n> +\tstruct object_id orig_tree;\n> +\tstruct rev_info rev;\n> +\tint nr_trees = 1;\n> +\tstruct tree_desc t[MAX_UNPACK_TREES];\n> +\tstruct tree *tree;\n> +\tstruct unpack_trees_options opts;\n> +\tstruct object *obj;\n> +\n> +\tdiscard_cache();\n> +\ttree = parse_tree_indirect(&info->i_tree);\n> +\tprime_cache_tree(&the_index, tree);\n> +\twrite_index_as_tree(orig_tree.hash, &the_index, stash_index_path, 0, NULL);\n> +\tdiscard_cache();\n\nHmph, the caller of this function did read_cache(), refresh_index(),\nand write_cache_as_tree(), and the result is in info->i_tree.\nThe above sequence discards, reads that tree into the index and\nwrites the same tree again. Which seems like a huge no-op. IIUC,\nthe write_cache_as_tree() the caller already did should have already \nprimed the cache-tree structure, too. These five lines are puzzling.\n\n> +\tread_cache_from(stash_index_path);\n\nHmph, it is unclear who wrote what state to this $TMPindex from this\ncodeflow. Do you really want to read from there? I am guessing\nthat this part corresponds to w_tree=$( ... ) in create_stash shell\nfunction, which does \"read-tree --index-output=$TMPindex -m $i_tree\"\nstarting from the real $GIT_DIR/index and the call to unpack_tree()\nthat follows here is that \"read-tree\".\n\nA one-way \"read-tree -m\" is purely a performance measure and the\nresulting index will have the entries in $i_tree no matter what\nindex contents you start from, so you may not have seen an incorrect\nresult per-se, but I suspect that you do not want to be reading from\n$TMPindex here. Puzzled...\n\n> +\n> +\tmemset(&opts, 0, sizeof(opts));\n> +\n> +\tparse_tree(tree);\n> +\n> +\topts.head_idx = 1;\n> +\topts.src_index = &the_index;\n> +\topts.dst_index = &the_index;\n> +\topts.merge = 1;\n> +\topts.fn = oneway_merge;\n> +\n> +\tinit_tree_desc(t, tree->buffer, tree->size);\n> +\n> +\tif (unpack_trees(nr_trees, t, &opts))\n> +\t\treturn 1;\n> +\n> +\tinit_revisions(&rev, prefix);\n> +\tsetup_revisions(0, NULL, &rev, NULL);\n> +\trev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;\n> +\trev.diffopt.format_callback = stash_create_callback;\n> +\tDIFF_OPT_SET(&rev.diffopt, EXIT_WITH_STATUS);\n> +\n> +\tparse_pathspec(&rev.prune_data, 0, 0, prefix, argv);\n> +\n> +\tdiff_setup_done(&rev.diffopt);\n> +\tobj = parse_object(&info->b_commit);\n> +\tadd_pending_object(&rev, obj, \"\");\n> +\tif (run_diff_index(&rev, 0))\n> +\t\treturn 1;\n> +\n> +\tif (write_cache_as_tree(info->w_tree.hash, 0, NULL))\n> +\t\treturn 1;\n> +\n> +\tdiscard_cache();\n> +\tread_cache();\n> +\n> +\treturn 0;\n> +}\n\nThis part otherwise looks like a correct way to grab changes to the\nworking tree into w_tree.\n\nAgain, I need to stop here for now. Will continue later.\n\n\n"},{"id":"322561","messageId":"alpine.DEB.2.21.1.1706191516350.57822@virtualbox","threadId":"46140","inReplyTo":"xmqqvanvv9be.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 5/5] stash: implement builtin stash","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-19T13:16:48Z","receivedAt":"2017-06-19T13:18:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 16 Jun 2017, Junio C Hamano wrote:\n\n> Joel Teichroeb writes:\n> \n> > +static void stash_create_callback(struct diff_queue_struct *q,\n> > +\t\t\t\tstruct diff_options *opt, void *cbdata)\n> > +{\n> > +\tint i;\n> > +\n> > +\tfor (i = 0; i < q->nr; i++) {\n> > +\t\tstruct diff_filepair *p = q->queue[i];\n> > +\t\tconst char *path = p->one->path;\n> > +\t\tstruct stat st;\n> \n> The order is somewhat ugly. Move \"struct stat st;\" that does not\n> have any initialization at the beginning.\n\nLet's not call it \"ugly\". You may find it ugly, but maybe you may want to\navoid contributors feeling judged negatively, either.\n\nInstead, let's say that it is preferred in Git's source code to declare\nuninitialized variables first, and then declare variables which are\ninitialized at the same time.\n\nThis convention, however, would need to be documented in CodingGuidelines\nfirst. We do not want to make contributors feel dumb now, do we?\n\nIn this particular case, I also wonder whether it is worth the time to\npoint out an unwritten (and not always obeyed) rule. The variable block is\nsmall enough that it does not matter much in which order the variables are\ndeclared.\n\nHowever, trying to be very strict even in such a small matter may well\ncost us contributors (and it is dubious whether the most critical parts of\nour technical debt has anything to do with small code style issues similar\nto this one). It's not like our bar of entry to new contributors is very\nlow, exactly...\n\nAnd if you disagree with this assessment, you should point out the same\nissues in literally all of my patches, as I always put initialized\nvariables first, uninitialized last.\n\n> > +\tstrbuf_reset(&out);\n> > +\n> > +\tdiscard_cache();\n> > +\tread_cache_from(stash_index_path);\n> > +\n> > +\twrite_index_as_tree(orig_tree.hash, &the_index, stash_index_path, 0,NULL);\n> \n> SP before \"NULL\".\n\nIf only we had automated source code formatting, saving us from these\ndistractions during patch review.\n\nThe rest of the review, modulo all the \"Hmpf\"s, seems helpful enough that\nI will try to find time to review the next iteration of this patch series\n(with a fresh mind, as I only skimmed the previous iteration) instead of\nadding my comments here.\n\nCiao,\nDscho\n"},{"id":"322562","messageId":"20170619132025.z42iuqqqskynu64u@sigill.intra.peff.net","threadId":"46140","inReplyTo":"alpine.DEB.2.21.1.1706191516350.57822@virtualbox","subject":"Re: [PATCH v4 5/5] stash: implement builtin stash","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-19T13:20:26Z","receivedAt":"2017-06-19T13:20:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 19, 2017 at 03:16:48PM +0200, Johannes Schindelin wrote:\n\n> And if you disagree with this assessment, you should point out the same\n> issues in literally all of my patches, as I always put initialized\n> variables first, uninitialized last.\n\nYeah, I am scratching my head here. If we do have a convention for\nordering, I'd have thought it is that (and I'd probably put statics even\nabove initialized variables).\n\n-Peff\n"},{"id":"322640","messageId":"CA+CzEk8+B71RoMeiZukfST-e6Ry+BijkNzHBusHycq2nhh2sPw@mail.gmail.com","threadId":"46140","inReplyTo":"xmqqvanvv9be.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 5/5] stash: implement builtin stash","fromName":"Joel Teichroeb","fromEmail":"joel@teichroeb.net","sentAt":"2017-06-20T02:12:20Z","receivedAt":"2017-06-20T02:12:47Z","isPatch":true,"sender":{"key":"joel@teichroeb.net","avatar":"https://avatars.githubusercontent.com/u/240865?v=4"},"body":"On Fri, Jun 16, 2017 at 3:47 PM, Junio C Hamano wrote:\n> Joel Teichroeb writes:\n>> +/*\n>> + * Untracked files are stored by themselves in a parentless commit, for\n>> + * ease of unpacking later.\n>> + */\n>> +static int save_untracked(struct stash_info *info, const char *message,\n>> + int include_untracked, int include_ignored, const char **argv)\n>> +{\n>> + struct child_process cp = CHILD_PROCESS_INIT;\n>> + struct strbuf out = STRBUF_INIT;\n>> + struct object_id orig_tree;\n>> + int ret;\n>> + const char *index_file = get_index_file();\n>> +\n>> + set_alternate_index_output(stash_index_path);\n>> + untracked_files(&out, include_untracked, include_ignored, argv);\n>> +\n>> + cp.git_cmd = 1;\n>> + argv_array_pushl(&cp.args, \"update-index\", \"-z\", \"--add\", \"--remove\",\n>> + \"--stdin\", NULL);\n>> + argv_array_pushf(&cp.env_array, \"GIT_INDEX_FILE=%s\", stash_index_path);\n>> +\n>> + if (pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0)) {\n>> + strbuf_release(&out);\n>> + return 1;\n>> + }\n>> +\n>\n> OK, that's a very straight-forward way of doing this, and as we do\n> not care too much about performance in this initial conversion to C,\n> it is even sensible. In a later update after this patch lands, you\n> may want to use dir.c's fill_directory() API to find the untracked\n> files and add them yourself internally, without running ls-files (in\n> untracked_files()) or update-index (here) as subprocesses, but that\n> is in the future. Let's get this round finished.\n>\n>> + strbuf_reset(&out);\n>> +\n>> + discard_cache();\n>> + read_cache_from(stash_index_path);\n>> +\n>> + write_index_as_tree(orig_tree.hash, &the_index, stash_index_path, 0,NULL);\n>\n> SP before \"NULL\".\n>\n>> + discard_cache();\n>> +\n>> + read_cache_from(stash_index_path);\n>\n> Hmph, what did anybody change in the on-disk stash_index (or\n> contents in the_index) since you read_cache_from()?\n>\n>> + write_cache_as_tree(info->u_tree.hash, 0, NULL);\n>\n> Then you write exactly the same index contents again, this time to\n> info->u_tree here. I am not sure why you need to do this twice, and\n> I do not see how orig_tree.hash you wrote earlier is used?\n>\n\nI'm not sure I understand what's happening here either. When I was\nwriting this, it was essentially a lot of trial and error in order to\nget the index handling correct. Getting rid of any single one of these\nlines makes the test fail. At some point I'd like to redo all the\nindex handling parts here, as I think I can do without an additional\nindex, but I'd need to make sure the error handling is perfect first.\n"},{"id":"322642","messageId":"CA+CzEk9i8H2BAUrL854WJELCTa-O1ONMWa0uOcTsW=WxnB_22Q@mail.gmail.com","threadId":"46140","inReplyTo":"20170611212739.GA7737@hank","subject":"Re: [PATCH v4 5/5] stash: implement builtin stash","fromName":"Joel Teichroeb","fromEmail":"joel@teichroeb.net","sentAt":"2017-06-20T02:37:22Z","receivedAt":"2017-06-20T02:37:49Z","isPatch":true,"sender":{"key":"joel@teichroeb.net","avatar":"https://avatars.githubusercontent.com/u/240865?v=4"},"body":"On Sun, Jun 11, 2017 at 2:27 PM, Thomas Gummerer wrote:\n>> +\n>> +int cmd_stash(int argc, const char **argv, const char *prefix)\n>> +{\n>> + int result = 0;\n>> + pid_t pid = getpid();\n>> +\n>> + struct option options[] = {\n>> + OPT_END()\n>> + };\n>> +\n>> + git_config(git_default_config, NULL);\n>> +\n>> + xsnprintf(stash_index_path, 64, \".git/index.stash.%d\", pid);\n>> +\n>> + argc = parse_options(argc, argv, prefix, options, git_stash_usage,\n>> + PARSE_OPT_KEEP_UNKNOWN|PARSE_OPT_KEEP_DASHDASH);\n>> +\n>> + if (argc < 1) {\n>> + result = do_push_stash(NULL, prefix, 0, 0, 0, 0, NULL);\n>> + } else if (!strcmp(argv[0], \"list\"))\n>> + result = list_stash(argc, argv, prefix);\n>> + else if (!strcmp(argv[0], \"show\"))\n>> + result = show_stash(argc, argv, prefix);\n>> + else if (!strcmp(argv[0], \"save\"))\n>> + result = save_stash(argc, argv, prefix);\n>> + else if (!strcmp(argv[0], \"push\"))\n>> + result = push_stash(argc, argv, prefix);\n>> + else if (!strcmp(argv[0], \"apply\"))\n>> + result = apply_stash(argc, argv, prefix);\n>> + else if (!strcmp(argv[0], \"clear\"))\n>> + result = clear_stash(argc, argv, prefix);\n>> + else if (!strcmp(argv[0], \"create\"))\n>> + result = create_stash(argc, argv, prefix);\n>> + else if (!strcmp(argv[0], \"store\"))\n>> + result = store_stash(argc, argv, prefix);\n>> + else if (!strcmp(argv[0], \"drop\"))\n>> + result = drop_stash(argc, argv, prefix);\n>> + else if (!strcmp(argv[0], \"pop\"))\n>> + result = pop_stash(argc, argv, prefix);\n>> + else if (!strcmp(argv[0], \"branch\"))\n>> + result = branch_stash(argc, argv, prefix);\n>> + else {\n>> + if (argv[0][0] == '-') {\n>> + struct argv_array args = ARGV_ARRAY_INIT;\n>> + argv_array_push(&args, \"push\");\n>> + argv_array_pushv(&args, argv);\n>> + result = push_stash(args.argc, args.argv, prefix);\n>\n> This is a bit of a change in behaviour to what we currently have.\n>\n> The rules we decided on are as follows:\n>\n> - \"git stash -p\" is an alias for \"git stash push -p\".\n> - \"git stash\" with only option arguments is an alias for \"git stash\n> push\" with those same arguments. non-option arguments can be\n> specified after a \"--\" for disambiguation.\n>\n> The above makes \"git stash -*\" a alias for \"git stash push -*\". This\n> would result in a change of behaviour, for example in the case where\n> someone would use \"git stash -this is a test-\". In that case the\n> current behaviour is to create a stash with the message \"-this is a\n> test-\", while the above would end up making git stash error out. The\n> discussion on how we came up with those rules can be found at\n> http://public-inbox.org/git/20170206161432.zvpsqegjspaa2l5l@sigill.intra.peff.net/.\n\nI don't really like the \"argv[0][0] == '-'\" logic, but it doesn't seem\nto have the flaw you pointed out:\n$ ./git stash -this is a test-\nerror: unknown switch `t'\nusage: git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n[...]\n\nI'm not sure this is the best possible error message, but it's just as\nuseful as the message from the old version.\n\n>\n>> + if (!result)\n>> + printf_ln(_(\"To restore them type \\\"git stash apply\\\"\"));\n>\n> In the shell script this is only displayed when the stash_push in the\n> case where git stash is invoked with no arguments, not in the push\n> case if I read this correctly. So the two lines above should go in\n> the (argc < 1) case I think.\n\nI think it's correct as is. One of the tests checks for this string to\nbe output, and if I move the line, the test fails.\n\n\n\nI agreed with all the other points you raised, and they will be fixed\nin my next revision.\n"},{"id":"322934","messageId":"xmqqk244kl3c.fsf@gitster.mtv.corp.google.com","threadId":"46140","inReplyTo":"20170608005535.13080-6-joel@teichroeb.net","subject":"Re: [PATCH v4 5/5] stash: implement builtin stash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-22T17:07:03Z","receivedAt":"2017-06-22T17:07:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joel Teichroeb writes:\n\n> +static int patch_working_tree(struct stash_info *info, const char *prefix,\n> +\t\tconst char **argv)\n> +{\n> +\tstruct argv_array args = ARGV_ARRAY_INIT;\n> +\tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\tstruct strbuf out = STRBUF_INIT;\n> +\tsize_t unused;\n> +\tconst char *index_file = get_index_file();\n> +\n> +\targv_array_pushl(&args, \"read-tree\", \"HEAD\", NULL);\n> +\targv_array_pushf(&args, \"--index-output=%s\", stash_index_path);\n> +\tcmd_read_tree(args.argc, args.argv, prefix);\n\nI do not think if cmd_read_tree() is prepared to be called like\nthis, and even if it happens to be OK, I do not think we should rely\non it. \n\nIn general, cmd_foo() that implements subcommand \"foo\" expects only\nto be called from main(), and expects that the calling main() will\nexit with its return status. This has implications that you, who\nabuse a cmd_foo() function as if it is a reusable helper function,\nneed to watch out for. For example, cmd_foo() may use static global\nvariables in builtin/foo.c that are initialized in a certain way\nbefore it starts, so calling cmd_foo() twice may not work correctly\n(the first invocation may change these variables and they won't be\nreset when it returns). cmd_foo() can and do leave resources\nunreclaimed, because it expects the calling main() to exit\nimmediately, leaving descriptors it creates open or chunks of memory\nit allocates unfreed. cmd_foo() also can and do die() without\nreturning the control to the caller (this last item does not make\nmuch difference to this particular codepath, as you'd end up dying\nsoon if this read-tree fails anyway, but in general you'd want to\ngive a more specific error message when it happens, i.e. instead of\nan error message cmd_read_tree() internally gives, you want to say\n\"Cannot save the current worktree state\").\n\n> +\n> +\tcp.git_cmd = 1;\n> +\targv_array_pushl(&cp.args, \"add--interactive\", \"--patch=stash\", \"--\", NULL);\n> +\targv_array_pushf(&cp.env_array, \"GIT_INDEX_FILE=%s\", stash_index_path);\n> +\tif (run_command(&cp))\n> +\t\treturn 1;\n\nUnlike the above direct call to cmd_read_tree(), the way this\ninvokes \"git add--interactive\" is kosher. By returning non-zero (by\nthe way, the prevailing convention in this codebase is to return\nnegative for an error), you give a chance to the caller of this\nhelper function to say \"Cannot save the current worktree state\".\n\n> +\tdiscard_cache();\n> +\tread_cache_from(stash_index_path);\n> +\n> +\tif (write_cache_as_tree(info->w_tree.hash, 0, NULL))\n> +\t\treturn 1;\n\nOK.\n\n> +\tchild_process_init(&cp);\n> +\tcp.git_cmd = 1;\n> +\targv_array_pushl(&cp.args, \"diff-tree\", \"-p\", \"HEAD\", NULL);\n> +\targv_array_push(&cp.args, sha1_to_hex(info->w_tree.hash));\n> +\targv_array_push(&cp.args, \"--\");\n> +\tif (pipe_command(&cp, NULL, 0, &out, 0, NULL, 0) || out.len == 0)\n> +\t\treturn 1;\n\nThis \"diff-tree\" call is also reasonable. Instead of getting the\npatch text into a temporary file (which is what the original did),\nwe slurp it into a strbuf \"out\", and pass it out to the caller by\nstoring in info->patch. OK.\n\n> +\tinfo->patch = strbuf_detach(&out, &unused);\n\nYou can pass NULL instead of a throw-away variable &unused.\n\n> +\n> +\tset_alternate_index_output(index_file);\n> +\tdiscard_cache();\n> +\tread_cache();\n> +\n> +\treturn 0;\n> +}\n\nSo this looks fairly faithful rewrite to C of a half of the\ncreate_stash shell function (we already reviewed the other half done\nin your save_working_tree() function).\n\n> +static int do_create_stash(struct stash_info *info, const char *prefix,\n> +\t\tconst char *message, int include_untracked, int include_ignored,\n> +\t\tint patch, const char **argv)\n> +{\n> +\tstruct object_id curr_head;\n> +\tchar *branch_path = NULL;\n> +\tconst char *branch_name = NULL;\n> +\tstruct commit_list *parents = NULL;\n> +\tstruct strbuf out_message = STRBUF_INIT;\n> +\tstruct strbuf out = STRBUF_INIT;\n> +\tstruct pretty_print_context ctx = {0};\n> +\n> +\tstruct commit *c = NULL;\n> +\tconst char *hash;\n> +\n> +\tread_cache_preload(NULL);\n> +\trefresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL);\n> +\tif (check_no_changes(prefix, include_untracked, include_ignored, argv))\n> +\t\treturn 1;\n\nWe find there is nothing to stash, so we tell the caller that fact\nby returning 1. The caller can tell that this is a \"different kind\nof success\" and is not an error by checking the sign of the return\nvalue.\n\n> +\tif (get_sha1_tree(\"HEAD\", info->b_commit.hash))\n> +\t\treturn error(_(\"You do not have the initial commit yet\"));\n\nAnd this is an error from the caller's point of view (error()\nreturns -1).\n\n> +\tbranch_path = resolve_refdup(\"HEAD\", 0, curr_head.hash, NULL);\n> +\n> +\tif (branch_path == NULL || strcmp(branch_path, \"HEAD\") == 0)\n> +\t\tbranch_name = \"(no branch)\";\n> +\telse\n> +\t\tskip_prefix(branch_path, \"refs/heads/\", &branch_name);\n> +\n> +\tc = lookup_commit(&info->b_commit);\n> +\n> +\tctx.output_encoding = get_log_output_encoding();\n> +\tctx.abbrev = 1;\n> +\tctx.fmt = CMIT_FMT_ONELINE;\n> +\thash = find_unique_abbrev(c->object.oid.hash, DEFAULT_ABBREV);\n> +\n> +\tstrbuf_addf(&out_message, \"%s: %s \", branch_name, hash);\n> +\n> +\tpretty_print_commit(&ctx, c, &out_message);\n> +\n> +\tstrbuf_addf(&out, \"index on %s\\n\", out_message.buf);\n\nOK, the above roughly correspond to \"# state of the base commit\"\npart of the original. This message is created with\n\n\tmsg=$(printf '%s: %s' \"$branch\" \"$head\")\n\nand your \"branch_name\" is computed to be the same as $branch, and\n$head in the original is head=$(git rev-list --oneline -n 1 HEAD--)\nwhich is your \"hash\" with the oneline. The whole $msg corresponds\nto your \"out_message.buf\". Looks correct.\n\n> +\tcommit_list_insert(lookup_commit(&info->b_commit), &parents);\n> +\n> +\tif (write_cache_as_tree(info->i_tree.hash, 0, NULL))\n> +\t\treturn error(_(\"git write-tree failed to write a tree\"));\n\nShouldn't the user also see \"Cannot save the current index state\"\nmessage in this case? A failure by write-tree is an implementation\ndetail and the latter is what matters more to the end user.\n\n> +\tif (commit_tree(out.buf, out.len, info->i_tree.hash, parents, info->i_commit.hash, NULL, NULL))\n> +\t\treturn error(_(\"Cannot save the current index state\"));\n> +\n> +\tstrbuf_reset(&out);\n> +\n> +\tif (include_untracked) {\n> +\t\tif (save_untracked(info, out_message.buf, include_untracked, include_ignored, argv))\n> +\t\t\treturn error(_(\"Cannot save the untracked files\"));\n> +\t}\n> +\n> +\tif (patch) {\n> +\t\tif (patch_working_tree(info, prefix, argv))\n> +\t\t\treturn error(_(\"Cannot save the current worktree state\"));\n> +\t} else {\n> +\t\tif (save_working_tree(info, prefix, argv))\n> +\t\t\treturn error(_(\"Cannot save the current worktree state\"));\n> +\t}\n> +\tparents = NULL;\n\nThe elements on the parents list here are leaked here, by early\nreturns we see above and also at the end of this function.\n\n> +\tif (include_untracked)\n> +\t\tcommit_list_insert(lookup_commit(&info->u_commit), &parents);\n> +\n> +\tcommit_list_insert(lookup_commit(&info->i_commit), &parents);\n> +\tcommit_list_insert(lookup_commit(&info->b_commit), &parents);\n> +\n> +\tif (message != NULL && strlen(message) != 0)\n> +\t\tstrbuf_addf(&out, \"On %s: %s\\n\", branch_name, message);\n> +\telse\n> +\t\tstrbuf_addf(&out, \"WIP on %s\\n\", out_message.buf);\n> +\n> +\tif (commit_tree(out.buf, out.len, info->w_tree.hash, parents, info->w_commit.hash, NULL, NULL))\n> +\t\treturn error(_(\"Cannot record working tree state\"));\n> +\n> +\tinfo->message = out.buf;\n> +\n> +\tstrbuf_release(&out_message);\n> +\tfree(branch_path);\n> +\n> +\treturn 0;\n> +}\n\n> +static int create_stash(int argc, const char **argv, const char *prefix)\n> +{\n> +\tint include_untracked = 0;\n> +\tconst char *message = NULL;\n> +\tstruct stash_info info;\n> +\tint ret;\n> +\tstruct strbuf out = STRBUF_INIT;\n> +\tstruct option options[] = {\n> +\t\tOPT_BOOL('u', \"include-untracked\", &include_untracked,\n> +\t\t\tN_(\"stash untracked filed\")),\n> +\t\tOPT_STRING('m', \"message\", &message, N_(\"message\"),\n> +\t\t\tN_(\"stash commit message\")),\n> +\t\tOPT_END()\n> +\t};\n> +\n> +\targc = parse_options(argc, argv, prefix, options,\n> +\t\t\tgit_stash_create_usage, 0);\n> +\n> +\tif (argc != 0) {\n> +\t\tint i;\n> +\t\tfor (i = 0; i < argc; ++i) {\n> +\t\t\tif (i != 0) {\n> +\t\t\t\tstrbuf_addf(&out, \" \");\n> +\t\t\t}\n\nStyle tips:\n Do not enclose a single statement inside a block. \n We prefer \"if (i)\" over \"if (i != 0)\".\n We prefer \"if (!i)\" over \"if (i == 0)\".\n We prefer \"i++\" over \"++i\", UNLESS you use the value before increment.\n\n> +\t\t\tstrbuf_addf(&out, \"%s\", argv[i]);\n> +\t\t}\n> +\t\tmessage = out.buf;\n> +\t}\n> +\n> +\tret = do_create_stash(&info, prefix, message, include_untracked, 0, 0, NULL);\n> +\n> +\tstrbuf_release(&out);\n\nI do not see a need for \"message\" variable; you can just pass\nout.buf to do_create_stash().\n\n> +\n> +\tif (ret)\n> +\t\treturn 0;\n> +\n> +\tprintf(\"%s\\n\", sha1_to_hex(info.w_commit.hash));\n> +\treturn 0;\n> +}\n\n"},{"id":"322935","messageId":"xmqqefuckkcj.fsf@gitster.mtv.corp.google.com","threadId":"46140","inReplyTo":"CA+CzEk8+B71RoMeiZukfST-e6Ry+BijkNzHBusHycq2nhh2sPw@mail.gmail.com","subject":"Re: [PATCH v4 5/5] stash: implement builtin stash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-22T17:23:08Z","receivedAt":"2017-06-22T17:23:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joel Teichroeb writes:\n\n> On Fri, Jun 16, 2017 at 3:47 PM, Junio C Hamano wrote:\n> ...\n>> Then you write exactly the same index contents again, this time to\n>> info->u_tree here. I am not sure why you need to do this twice, and\n>> I do not see how orig_tree.hash you wrote earlier is used?\n>\n> I'm not sure I understand what's happening here either. When I was\n> writing this, it was essentially a lot of trial and error in order to\n> get the index handling correct....\n\nThanks for being honest. I agree that we do not want to say \"we do\nnot yet know the exact mechanism how X happens, but X does happen\"\nfor any value of X (in this case \"the code happens to do the same\nthing as the original\"). In biology or physics experiments, that\nmay be how science advances, but it is different when it comes for\nus to explain our own code ;-). After all, its our creation.\n\nI haven't followed the big picture in your codepath, but if you had\nsomething like this, I can see how you need a seemingly unneeded\nreading of the index:\n\n function A\n\tdiscard and read index\n\tdo A's thing\n\n function B\n\tdiscard and read index\n\tdo B's thing\n\n function C\n\tdiscard and read index\n\tif (some condition)\n\t\tdo things that involves smudging the index\n\t\tcall A\n\telse\n\t\tcall B\n\n function D\n\tread index\n\tif (some other condition)\n\t\tcall A\n\telse\n\t\tdo things that involves smudging the index\n\t\tcall B\n\nThat is, when the division of labor for preparing the in-core index\nis not very well defined between the caller and the callee. When\nfunction C calls function B, the index is unnecessarily discarded\nand read at the beginning of function B, but if you remove it\nwithout changing anything else, the call to it from function D would\nbreak. One way to fix it would be to make the two helpers work from\nthe given in-core index, iow, make their callers responsible for\npreparing the in-core index to desired state, i.e.\n\n function A\n\tdo A's thing\n\n function B\n\tdo B's thing\n\n function C\n\tdiscard and read index\n\tif (some condition)\n\t\tdo things that involves smudging the index\n\t\tdiscard and read index\n\t\tcall A\n\telse\n\t\tcall B\n\n function D\n\tread index\n\tif (some other condition)\n\t\tcall A\n\telse\n\t\tdo things that involves smudging the index\n discard and read index\n\t\tcall B\n\nAgain, I didn't follow the big picture callpath in your patch, so\nthe above may not be why your extra read-index calls are needed, and\nI do not know which of your functions correspond to A, B, C and D in\nthe above illustration. But I think you get the idea.\n"},{"id":"323249","messageId":"20170625210909.GB7737@hank","threadId":"46140","inReplyTo":"CA+CzEk9i8H2BAUrL854WJELCTa-O1ONMWa0uOcTsW=WxnB_22Q@mail.gmail.com","subject":"Re: [PATCH v4 5/5] stash: implement builtin stash","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2017-06-25T21:09:09Z","receivedAt":"2017-06-25T21:08:56Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 06/19, Joel Teichroeb wrote:\n> On Sun, Jun 11, 2017 at 2:27 PM, Thomas Gummerer wrote:\n> >> +\n> >> +int cmd_stash(int argc, const char **argv, const char *prefix)\n> >> +{\n> >> + int result = 0;\n> >> + pid_t pid = getpid();\n> >> +\n> >> + struct option options[] = {\n> >> + OPT_END()\n> >> + };\n> >> +\n> >> + git_config(git_default_config, NULL);\n> >> +\n> >> + xsnprintf(stash_index_path, 64, \".git/index.stash.%d\", pid);\n> >> +\n> >> + argc = parse_options(argc, argv, prefix, options, git_stash_usage,\n> >> + PARSE_OPT_KEEP_UNKNOWN|PARSE_OPT_KEEP_DASHDASH);\n> >> +\n> >> + if (argc < 1) {\n> >> + result = do_push_stash(NULL, prefix, 0, 0, 0, 0, NULL);\n> >> + } else if (!strcmp(argv[0], \"list\"))\n> >> + result = list_stash(argc, argv, prefix);\n> >> + else if (!strcmp(argv[0], \"show\"))\n> >> + result = show_stash(argc, argv, prefix);\n> >> + else if (!strcmp(argv[0], \"save\"))\n> >> + result = save_stash(argc, argv, prefix);\n> >> + else if (!strcmp(argv[0], \"push\"))\n> >> + result = push_stash(argc, argv, prefix);\n> >> + else if (!strcmp(argv[0], \"apply\"))\n> >> + result = apply_stash(argc, argv, prefix);\n> >> + else if (!strcmp(argv[0], \"clear\"))\n> >> + result = clear_stash(argc, argv, prefix);\n> >> + else if (!strcmp(argv[0], \"create\"))\n> >> + result = create_stash(argc, argv, prefix);\n> >> + else if (!strcmp(argv[0], \"store\"))\n> >> + result = store_stash(argc, argv, prefix);\n> >> + else if (!strcmp(argv[0], \"drop\"))\n> >> + result = drop_stash(argc, argv, prefix);\n> >> + else if (!strcmp(argv[0], \"pop\"))\n> >> + result = pop_stash(argc, argv, prefix);\n> >> + else if (!strcmp(argv[0], \"branch\"))\n> >> + result = branch_stash(argc, argv, prefix);\n> >> + else {\n> >> + if (argv[0][0] == '-') {\n> >> + struct argv_array args = ARGV_ARRAY_INIT;\n> >> + argv_array_push(&args, \"push\");\n> >> + argv_array_pushv(&args, argv);\n> >> + result = push_stash(args.argc, args.argv, prefix);\n> >\n> > This is a bit of a change in behaviour to what we currently have.\n> >\n> > The rules we decided on are as follows:\n> >\n> > - \"git stash -p\" is an alias for \"git stash push -p\".\n> > - \"git stash\" with only option arguments is an alias for \"git stash\n> > push\" with those same arguments. non-option arguments can be\n> > specified after a \"--\" for disambiguation.\n> >\n> > The above makes \"git stash -*\" a alias for \"git stash push -*\". This\n> > would result in a change of behaviour, for example in the case where\n> > someone would use \"git stash -this is a test-\". In that case the\n> > current behaviour is to create a stash with the message \"-this is a\n> > test-\", while the above would end up making git stash error out. The\n> > discussion on how we came up with those rules can be found at\n> > http://public-inbox.org/git/20170206161432.zvpsqegjspaa2l5l@sigill.intra.peff.net/.\n> \n> I don't really like the \"argv[0][0] == '-'\" logic, but it doesn't seem\n> to have the flaw you pointed out:\n> $ ./git stash -this is a test-\n> error: unknown switch `t'\n> usage: git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n> [...]\n\nI just went through the thread again, to remind myself why we did it\nthis way. The example I had above was the wrong example, sorry. The\nmessage at [1] explains it better. Essentially by implementing the\nrules I mentioned we wanted to avoid the potential confusion of what\ndoes 'git stash -q drop' mean. Before the rewrite this fails and\nshows the usage. After the rewrite this would try to stash everything\nmatching the pathspec drop, which might be confusing for users. \n\n[1]: http://public-inbox.org/git/20170213214521.pkjesijdlus36tnp@sigill.intra.peff.net/\n\n> I'm not sure this is the best possible error message, but it's just as\n> useful as the message from the old version.\n> \n> >\n> >> + if (!result)\n> >> + printf_ln(_(\"To restore them type \\\"git stash apply\\\"\"));\n> >\n> > In the shell script this is only displayed when the stash_push in the\n> > case where git stash is invoked with no arguments, not in the push\n> > case if I read this correctly. So the two lines above should go in\n> > the (argc < 1) case I think.\n> \n> I think it's correct as is. One of the tests checks for this string to\n> be output, and if I move the line, the test fails.\n\nRight, that test that fails only when the \"To restore...\" string is\nprinted to stdout. So moving the \"printf_ln()\" to the line you did\nonly makes sure it's not printed there. Reading the code again and\ntrying to trigger this print in the shell script stash makes me think\nthis is not even possible to trigger there anymore.\n\nAfter the line\n\ntest -n \"$seen_non_option\" || set \"push\" \"$@\"\n\nit's not possible that $# is 0 anymore, so this will never be\nprinted. From a quick look at the history it seems like it wasn't\npossible to trigger that codepath for a while. If I'm reading things\ncorrectly 3c2eb80fe3 (\"stash: simplify defaulting to \"save\" and reject\nunknown options\", 2009-08-18) seems to have introduced the small\nchange in behaviour. As I don't think anyone has complained since\nthen, I'd just leave it as is, which makes git stash with no options a\nlittle less verbose. [Adding Matthieu to cc as author of the above\nmentioned commit]\n\n> I agreed with all the other points you raised, and they will be fixed\n> in my next revision.\n\nThanks!\n"},{"id":"323257","messageId":"vpqshini3r6.fsf@anie.imag.fr","threadId":"46140","inReplyTo":"20170625210909.GB7737@hank","subject":"Re: [PATCH v4 5/5] stash: implement builtin stash","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2017-06-26T07:53:33Z","receivedAt":"2017-06-26T08:02:42Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Thomas Gummerer writes:\n\n> After the line\n>\n> test -n \"$seen_non_option\" || set \"push\" \"$@\"\n>\n> it's not possible that $# is 0 anymore, so this will never be\n> printed. From a quick look at the history it seems like it wasn't\n> possible to trigger that codepath for a while. If I'm reading things\n> correctly 3c2eb80fe3 (\"stash: simplify defaulting to \"save\" and reject\n> unknown options\", 2009-08-18) seems to have introduced the small\n> change in behaviour.\n\nIndeed. That wasn't on purpose, but I seem to have turned this\n\n\tcase $# in\n\t0)\n\t\tpush_stash &&\n\t\tsay \"$(gettext \"(To restore them type \\\"git stash apply\\\")\")\"\n\t\t;;\n\ninto dead code.\n\n> As I don't think anyone has complained since then, I'd just leave it\n> as is, which makes git stash with no options a little less verbose.\n\nI agree it's OK to keep is as-is, but the original logic (give a bit\nmore advice when \"stash push\" was DWIM-ed) made sense too, so it can\nmake sense to re-activate it while porting to C.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"323335","messageId":"CALgYhfPQXNZYv758+8ZB32aFLpNXNerfOeYt0QLHDWFZ-FQwDA@mail.gmail.com","threadId":"46140","inReplyTo":"vpqshini3r6.fsf@anie.imag.fr","subject":"Re: [PATCH v4 5/5] stash: implement builtin stash","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2017-06-27T14:53:01Z","receivedAt":"2017-06-27T14:53:20Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On Mon, Jun 26, 2017 at 8:53 AM Matthieu Moy\n wrote:\n>\n> Thomas Gummerer writes:\n>\n> > After the line\n> >\n> > test -n \"$seen_non_option\" || set \"push\" \"$@\"\n> >\n> > it's not possible that $# is 0 anymore, so this will never be\n> > printed. From a quick look at the history it seems like it wasn't\n> > possible to trigger that codepath for a while. If I'm reading things\n> > correctly 3c2eb80fe3 (\"stash: simplify defaulting to \"save\" and reject\n> > unknown options\", 2009-08-18) seems to have introduced the small\n> > change in behaviour.\n>\n> Indeed. That wasn't on purpose, but I seem to have turned this\n>\n> case $# in\n> 0)\n> push_stash &&\n> say \"$(gettext \"(To restore them type \\\"git stash apply\\\")\")\"\n> ;;\n>\n> into dead code.\n>\n> > As I don't think anyone has complained since then, I'd just leave it\n> > as is, which makes git stash with no options a little less verbose.\n>\n> I agree it's OK to keep is as-is, but the original logic (give a bit\n> more advice when \"stash push\" was DWIM-ed) made sense too, so it can\n> make sense to re-activate it while porting to C.\n\nI'd be happy either way. If we decide to restore the original behaviour, I\nthink it should be done in a separate patch, as the test case will need\nsome adjustments. That way we can keep this patch purely as the\nconversion step, which makes it a bit easier to review.\n\n> --\n> Matthieu Moy\n> http://www-verimag.imag.fr/~moy/\n"}]}