{"thread":{"id":"46062","subject":"[PATCH v3 0/4] Implement git stash as a builtin command","startedAt":"2017-05-28T16:57:27Z","lastAt":"2017-10-23T18:35:46Z","messageCount":25,"participants":["Joel Teichroeb","Ævar Arnfjörð Bjarmason","Christian Couder","Jeff King","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":3,"patchTotal":4},"messages":[{"id":"320891","messageId":"20170528165642.14699-1-joel@teichroeb.net","threadId":"46062","inReplyTo":null,"subject":"[PATCH v3 0/4] Implement git stash as a builtin command","fromName":"Joel Teichroeb","fromEmail":"joel@teichroeb.net","sentAt":"2017-05-28T16:56:38Z","receivedAt":"2017-05-28T16:57:27Z","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\nJoel Teichroeb (4):\n stash: add test for stash create with no files\n stash: add test for stashing in a detached state\n 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/add.c | 3 +-\n builtin/mv.c | 8 +-\n builtin/rm.c | 3 +-\n builtin/stash.c | 1280 +++++++++++++++++++++++++\n git-stash.sh => contrib/examples/git-stash.sh | 0\n git.c | 1 +\n merge-recursive.c | 8 +-\n t/t3903-stash.sh | 19 +\n 10 files changed, 1316 insertions(+), 9 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":"320892","messageId":"20170528165642.14699-2-joel@teichroeb.net","threadId":"46062","inReplyTo":"20170528165642.14699-1-joel@teichroeb.net","subject":"[PATCH v3 1/4] stash: add test for stash create with no files","fromName":"Joel Teichroeb","fromEmail":"joel@teichroeb.net","sentAt":"2017-05-28T16:56:39Z","receivedAt":"2017-05-28T16:57:32Z","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..aaae221304 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 $(cat actual | wc -l) -eq 0\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":"320893","messageId":"20170528165642.14699-3-joel@teichroeb.net","threadId":"46062","inReplyTo":"20170528165642.14699-1-joel@teichroeb.net","subject":"[PATCH v3 2/4] stash: add test for stashing in a detached state","fromName":"Joel Teichroeb","fromEmail":"joel@teichroeb.net","sentAt":"2017-05-28T16:56:40Z","receivedAt":"2017-05-28T16:57:36Z","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 | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex aaae221304..b86851ef46 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -808,6 +808,17 @@ 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+\techo \"WIP on (no branch): 47d5e0e 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":"320894","messageId":"20170528165642.14699-4-joel@teichroeb.net","threadId":"46062","inReplyTo":"20170528165642.14699-1-joel@teichroeb.net","subject":"[PATCH v3 3/4] close the index lock when not writing the new index","fromName":"Joel Teichroeb","fromEmail":"joel@teichroeb.net","sentAt":"2017-05-28T16:56:41Z","receivedAt":"2017-05-28T16:57:38Z","isPatch":true,"sender":{"key":"joel@teichroeb.net","avatar":"https://avatars.githubusercontent.com/u/240865?v=4"},"body":"Signed-off-by: Joel Teichroeb \n---\n builtin/add.c | 3 ++-\n builtin/mv.c | 8 +++++---\n builtin/rm.c | 3 ++-\n merge-recursive.c | 8 +++++---\n 4 files changed, 14 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 9f53f020d0..6b04eb2c71 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -461,7 +461,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \tif (active_cache_changed) {\n \t\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\t\tdie(_(\"Unable to write new index file\"));\n-\t}\n+\t} else\n+\t\trollback_lock_file(&lock_file);\n \n \treturn exit_status;\n }\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex 61d20037ad..ccf21de17f 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -293,9 +293,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \tif (gitmodules_modified)\n \t\tstage_updated_gitmodules();\n \n-\tif (active_cache_changed &&\n-\t write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n-\t\tdie(_(\"Unable to write new index file\"));\n+\tif (active_cache_changed) {\n+\t\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n+\t\t\tdie(_(\"Unable to write new index file\"));\n+\t} else\n+\t\trollback_lock_file(&lock_file);\n \n \treturn 0;\n }\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex fb79dcab18..4c7a91888b 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -389,7 +389,8 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \tif (active_cache_changed) {\n \t\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\t\tdie(_(\"Unable to write new index file\"));\n-\t}\n+\t} else\n+\t\trollback_lock_file(&lock_file);\n \n \treturn 0;\n }\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 62decd51cc..db841c0d38 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -2145,9 +2145,11 @@ 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 \n \treturn clean ? 0 : 1;\n }\n-- \n2.13.0\n\n"},{"id":"320895","messageId":"20170528165642.14699-5-joel@teichroeb.net","threadId":"46062","inReplyTo":"20170528165642.14699-1-joel@teichroeb.net","subject":"[PATCH v3 4/4] stash: implement builtin stash","fromName":"Joel Teichroeb","fromEmail":"joel@teichroeb.net","sentAt":"2017-05-28T16:56:42Z","receivedAt":"2017-05-28T16:57:46Z","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 | 1280 +++++++++++++++++++++++++\n git-stash.sh => contrib/examples/git-stash.sh | 0\n git.c | 1 +\n 5 files changed, 1283 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 e35542e631..4af4ac41c7 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -523,7 +523,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@@ -961,6 +960,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 9e4a89816d..16e8a437d4 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -121,6 +121,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..bf36ff8f9b\n--- /dev/null\n+++ b/builtin/stash.c\n@@ -0,0 +1,1280 @@\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 *REV;\n+\tint is_stash_ref;\n+\tint has_u;\n+\tconst char *patch;\n+};\n+\n+int untracked_files(struct strbuf *out, int include_untracked,\n+\t\tconst char **argv)\n+{\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tcp.git_cmd = 1;\n+\targv_array_push(&cp.args, \"ls-files\");\n+\targv_array_push(&cp.args, \"-o\");\n+\targv_array_push(&cp.args, \"-z\");\n+\tif (include_untracked != 2)\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\tconst char **argv)\n+{\n+\tstruct argv_array args1;\n+\tstruct argv_array args2;\n+\tstruct strbuf out = STRBUF_INIT;\n+\n+\targv_array_init(&args1);\n+\targv_array_push(&args1, \"diff-index\");\n+\targv_array_push(&args1, \"--quiet\");\n+\targv_array_push(&args1, \"--cached\");\n+\targv_array_push(&args1, \"HEAD\");\n+\targv_array_push(&args1, \"--ignore-submodules\");\n+\targv_array_push(&args1, \"--\");\n+\tif (argv)\n+\t\targv_array_pushv(&args1, argv);\n+\targv_array_init(&args2);\n+\targv_array_push(&args2, \"diff-files\");\n+\targv_array_push(&args2, \"--quiet\");\n+\targv_array_push(&args2, \"--ignore-submodules\");\n+\targv_array_push(&args2, \"--\");\n+\tif (argv)\n+\t\targv_array_pushv(&args2, argv);\n+\tif (include_untracked)\n+\t\tuntracked_files(&out, include_untracked, argv);\n+\treturn 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+}\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 i_commit_rev = STRBUF_INIT;\n+\tstruct strbuf u_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+\tstruct object_context unused;\n+\tchar *str;\n+\tint ret;\n+\tconst char *REV = commit;\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\tREV = commit_buf.buf;\n+\t} else if (strlen(commit) < 3) {\n+\t\tstrbuf_addf(&commit_buf, \"%s@{%s}\", ref_stash, commit);\n+\t\tREV = commit_buf.buf;\n+\t}\n+\tinfo->REV = REV;\n+\n+\tstrbuf_addf(&w_commit_rev, \"%s\", REV);\n+\tstrbuf_addf(&b_commit_rev, \"%s^1\", REV);\n+\tstrbuf_addf(&i_commit_rev, \"%s^2\", REV);\n+\tstrbuf_addf(&u_commit_rev, \"%s^3\", REV);\n+\tstrbuf_addf(&w_tree_rev, \"%s:\", REV);\n+\tstrbuf_addf(&b_tree_rev, \"%s^1:\", REV);\n+\tstrbuf_addf(&i_tree_rev, \"%s^2:\", REV);\n+\tstrbuf_addf(&u_tree_rev, \"%s^3:\", REV);\n+\n+\n+\tret = (\n+\t\tget_sha1_with_context(w_commit_rev.buf, 0, info->w_commit.hash, &unused) == 0 &&\n+\t\tget_sha1_with_context(b_commit_rev.buf, 0, info->b_commit.hash, &unused) == 0 &&\n+\t\tget_sha1_with_context(i_commit_rev.buf, 0, info->i_commit.hash, &unused) == 0 &&\n+\t\tget_sha1_with_context(w_tree_rev.buf, 0, info->w_tree.hash, &unused) == 0 &&\n+\t\tget_sha1_with_context(b_tree_rev.buf, 0, info->b_tree.hash, &unused) == 0 &&\n+\t\tget_sha1_with_context(i_tree_rev.buf, 0, info->i_tree.hash, &unused) == 0);\n+\n+\tif (!ret) {\n+\t\tfprintf_ln(stderr, _(\"%s is not a valid reference\"), REV);\n+\t\treturn 1;\n+\t}\n+\n+\n+\tinfo->has_u = get_sha1_with_context(u_commit_rev.buf, 0, info->u_commit.hash, &unused) == 0 &&\n+\t\tget_sha1_with_context(u_tree_rev.buf, 0, info->u_tree.hash, &unused) == 0;\n+\n+\n+\t/* TODO: Improve this logic */\n+\tstrbuf_addf(&symbolic, \"%s\", REV);\n+\tstr = strstr(symbolic.buf, \"@\");\n+\tif (str)\n+\t\tstr[0] = 0;\n+\tcp.git_cmd = 1;\n+\targv_array_push(&cp.args, \"rev-parse\");\n+\targv_array_push(&cp.args, \"--symbolic-full-name\");\n+\targv_array_pushf(&cp.args, \"%s\", symbolic.buf);\n+\tpipe_command(&cp, NULL, 0, &out, 0, NULL, 0);\n+\tout.buf[out.len-1] = 0;\n+\n+\tinfo->is_stash_ref = strcmp(out.buf, ref_stash) == 0;\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+\n+\t}\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, struct strbuf *out,\n+\t\tint include_untracked, const char **argv)\n+{\n+\tstruct child_process cp2 = CHILD_PROCESS_INIT;\n+\tstruct strbuf out3 = STRBUF_INIT;\n+\tstruct strbuf out4 = STRBUF_INIT;\n+\tstruct object_id orig_tree;\n+\n+\tset_alternate_index_output(stash_index_path);\n+\tuntracked_files(&out4, include_untracked, argv);\n+\n+\tcp2.git_cmd = 1;\n+\targv_array_push(&cp2.args, \"update-index\");\n+\targv_array_push(&cp2.args, \"-z\");\n+\targv_array_push(&cp2.args, \"--add\");\n+\targv_array_push(&cp2.args, \"--remove\");\n+\targv_array_push(&cp2.args, \"--stdin\");\n+\targv_array_pushf(&cp2.env_array, \"GIT_INDEX_FILE=%s\", stash_index_path);\n+\n+\tif (pipe_command(&cp2, out4.buf, out4.len, NULL, 0, NULL, 0))\n+\t\treturn 1;\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(&out3, \"untracked files on %s\", out->buf);\n+\n+\tif (commit_tree(out3.buf, out3.len, info->u_tree.hash, NULL, info->u_commit.hash, NULL, NULL))\n+\t\treturn 1;\n+\n+\tset_alternate_index_output(\".git/index\");\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.hash);\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.hash);\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+\tconst char *stash_index_path = \".git/foocache2\";\n+\tstruct argv_array args;\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tstruct child_process cp2 = CHILD_PROCESS_INIT;\n+\tstruct strbuf out = STRBUF_INIT;\n+\n+\targv_array_init(&args);\n+\targv_array_push(&args, \"read-tree\");\n+\targv_array_push(&args, \"HEAD\");\n+\targv_array_pushf(&args, \"--index-output=%s\", stash_index_path);\n+\tcmd_read_tree(args.argc, args.argv, prefix);\n+\n+\tcp2.git_cmd = 1;\n+\targv_array_push(&cp2.args, \"add--interactive\");\n+\targv_array_push(&cp2.args, \"--patch=stash\");\n+\targv_array_push(&cp2.args, \"--\");\n+\targv_array_pushf(&cp2.env_array, \"GIT_INDEX_FILE=%s\", stash_index_path);\n+\tif (run_command(&cp2))\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+\tcp.git_cmd = 1;\n+\targv_array_push(&cp.args, \"diff-tree\");\n+\targv_array_push(&cp.args, \"-p\");\n+\targv_array_push(&cp.args, \"HEAD\");\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 = out.buf;\n+\n+\tset_alternate_index_output(\".git/index\");\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+\tconst char *message, int include_untracked, int 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 = STRBUF_INIT;\n+\tstruct strbuf out3 = STRBUF_INIT;\n+\tstruct pretty_print_context ctx = {0};\n+\n+\tstruct commit *c = NULL;\n+\tconst char *hash;\n+\tstruct strbuf out2 = STRBUF_INIT;\n+\n+\tread_cache_preload(NULL);\n+\trefresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL);\n+\tif (check_no_changes(prefix, include_untracked, argv))\n+\t\treturn 1;\n+\n+\tif (get_sha1_tree(\"HEAD\", info->b_commit.hash))\n+\t\tdie(_(\"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.hash);\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, \"%s: %s \", branch_name, hash);\n+\n+\tpretty_print_commit(&ctx, c, &out);\n+\n+\tstrbuf_addf(&out3, \"index on %s\\n\", out.buf);\n+\n+\tcommit_list_insert(lookup_commit(info->b_commit.hash), &parents);\n+\n+\tif (write_cache_as_tree(info->i_tree.hash, 0, NULL))\n+\t\tdie(_(\"git write-tree failed to write a tree\"));\n+\n+\tif (commit_tree(out3.buf, out3.len, info->i_tree.hash, parents, info->i_commit.hash, NULL, NULL))\n+\t\tdie(_(\"Cannot save the current index state\"));\n+\n+\n+\tif (include_untracked) {\n+\t\tif (save_untracked(info, &out, include_untracked, argv))\n+\t\t\tdie(_(\"Cannot save the untracked files\"));\n+\t}\n+\n+\n+\tif (patch) {\n+\t\tif (patch_working_tree(info, prefix, argv))\n+\t\t\tdie(_(\"Cannot save the current worktree state\"));\n+\t} else {\n+\t\tif (save_working_tree(info, prefix, argv))\n+\t\t\tdie(_(\"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.hash), &parents);\n+\t}\n+\n+\tcommit_list_insert(lookup_commit(info->i_commit.hash), &parents);\n+\tcommit_list_insert(lookup_commit(info->b_commit.hash), &parents);\n+\n+\tif (message != NULL && strlen(message) != 0)\n+\t\tstrbuf_addf(&out2, \"On %s: %s\\n\", branch_name, message);\n+\telse\n+\t\tstrbuf_addf(&out2, \"WIP on %s\\n\", out.buf);\n+\n+\tif (commit_tree(out2.buf, out2.len, info->w_tree.hash, parents, info->w_commit.hash, NULL, NULL))\n+\t\tdie(_(\"Cannot record working tree state\"));\n+\n+\tinfo->message = out2.buf;\n+\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+\tstruct option options[] = {\n+\t\tOPT_BOOL('u', \"include-untracked\", &include_untracked,\n+\t\t\t N_(\"stash untracked filed\")),\n+\t\tOPT_STRING('m', \"message\", &message, N_(\"message\"),\n+\t\t\t N_(\"stash commit message\")),\n+\t\tOPT_END()\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, options,\n+\t\t\t\t git_stash_create_usage, 0);\n+\n+\tif (argc != 0) {\n+\t\tstruct strbuf out = STRBUF_INIT;\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+\tif (do_create_stash(&info, prefix, message, include_untracked, 0, NULL))\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\tdie(_(\"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 = NULL;\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\t N_(\"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,\n+\t\t\t\t git_stash_store_usage, 0);\n+\n+\tif (message == NULL)\n+\t\tmessage = \"Create via \\\"git stash store\\\".\";\n+\n+\tif (argc != 1)\n+\t\tdie(_(\"\\\"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+\tstruct object_context unused;\n+\tif (get_sha1_with_context(ref_stash, 0, obj.hash, &unused))\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,\n+\t\t\t\t git_stash_clear_usage, PARSE_OPT_STOP_AT_NON_OPTION);\n+\n+\tif (argc != 0)\n+\t\tdie(_(\"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.hash);\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+\t\tint keep_index, int include_untracked, int patch, const char **argv)\n+{\n+\tint result;\n+\tstruct stash_info info;\n+\n+\tif (patch && include_untracked) {\n+\t\tfprintf_ln(stderr, _(\"can't use --patch and --include-untracked or --all at the same time\"));\n+\t\treturn 1;\n+\t}\n+\n+\tif (!include_untracked) {\n+\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\t\tstruct strbuf out = STRBUF_INIT;\n+\n+\t\tcp.git_cmd = 1;\n+\t\targv_array_push(&cp.args, \"ls-files\");\n+\t\targv_array_push(&cp.args, \"--error-unmatch\");\n+\t\targv_array_push(&cp.args, \"--\");\n+\t\tif (argv)\n+\t\t\targv_array_pushv(&cp.args, argv);\n+\t\tresult = pipe_command(&cp, NULL, 0, &out, 0, NULL, 0);\n+\t\tif (result)\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, argv)) {\n+\t\tprintf(_(\"No local changes to save\\n\"));\n+\t\treturn 0;\n+\t}\n+\n+\tif (!reflog_exists(ref_stash)) {\n+\t\tif (do_clear_stash())\n+\t\t\tdie(_(\"Cannot initialize stash\"));\n+\t}\n+\n+\n+\tdo_create_stash(&info, prefix, message, include_untracked, patch, argv);\n+\tresult = do_store_stash(prefix, 1, info.message, info.w_commit);\n+\n+\tif (result == 0 && !quiet) {\n+\t\tprintf(_(\"Saved working directory and index state %s\"), info.message);\n+\t}\n+\n+\tif (!patch) {\n+\t\tif (argv && *argv) {\n+\t\t\tstruct argv_array args;\n+\t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\t\t\tstruct child_process cp2 = CHILD_PROCESS_INIT;\n+\t\t\tstruct strbuf out = STRBUF_INIT;\n+\t\t\targv_array_init(&args);\n+\t\t\targv_array_push(&args, \"reset\");\n+\t\t\targv_array_push(&args, \"--quiet\");\n+\t\t\targv_array_push(&args, \"--\");\n+\t\t\targv_array_pushv(&args, argv);\n+\t\t\tcmd_reset(args.argc, args.argv, prefix);\n+\n+\n+\t\t\tcp.git_cmd = 1;\n+\t\t\targv_array_push(&cp.args, \"ls-files\");\n+\t\t\targv_array_push(&cp.args, \"-z\");\n+\t\t\targv_array_push(&cp.args, \"--modified\");\n+\t\t\targv_array_push(&cp.args, \"--\");\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\tcp2.git_cmd = 1;\n+\t\t\targv_array_push(&cp2.args, \"checkout-index\");\n+\t\t\targv_array_push(&cp2.args, \"-z\");\n+\t\t\targv_array_push(&cp2.args, \"--force\");\n+\t\t\targv_array_push(&cp2.args, \"--stdin\");\n+\t\t\tpipe_command(&cp2, out.buf, out.len, NULL, 0, NULL, 0);\n+\n+\t\t\targv_array_init(&args);\n+\t\t\targv_array_push(&args, \"clean\");\n+\t\t\targv_array_push(&args, \"--force\");\n+\t\t\targv_array_push(&args, \"-d\");\n+\t\t\targv_array_push(&args, \"--quiet\");\n+\t\t\targv_array_push(&args, \"--\");\n+\t\t\targv_array_pushv(&args, argv);\n+\t\t\tcmd_clean(args.argc, args.argv, prefix);\n+\t\t} else {\n+\t\t\tstruct argv_array args;\n+\t\t\targv_array_init(&args);\n+\t\t\targv_array_push(&args, \"reset\");\n+\t\t\targv_array_push(&args, \"--hard\");\n+\t\t\targv_array_push(&args, \"--quiet\");\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;\n+\t\t\targv_array_init(&args);\n+\t\t\targv_array_push(&args, \"clean\");\n+\t\t\targv_array_push(&args, \"--force\");\n+\t\t\targv_array_push(&args, \"--quiet\");\n+\t\t\targv_array_push(&args, \"-d\");\n+\t\t\tif (include_untracked == 2)\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 child_process cp2 = 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_push(&cp.args, \"ls-files\");\n+\t\t\targv_array_push(&cp.args, \"-z\");\n+\t\t\targv_array_push(&cp.args, \"--modified\");\n+\t\t\targv_array_push(&cp.args, \"--\");\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\tcp2.git_cmd = 1;\n+\t\t\targv_array_push(&cp2.args, \"checkout-index\");\n+\t\t\targv_array_push(&cp2.args, \"-z\");\n+\t\t\targv_array_push(&cp2.args, \"--force\");\n+\t\t\targv_array_push(&cp2.args, \"--stdin\");\n+\t\t\tpipe_command(&cp2, out.buf, out.len, NULL, 0, NULL, 0);\n+\t\t}\n+\t} else {\n+\t\tstruct child_process cp2 = CHILD_PROCESS_INIT;\n+\t\tcp2.git_cmd = 1;\n+\t\targv_array_push(&cp2.args, \"apply\");\n+\t\targv_array_push(&cp2.args, \"-R\");\n+\t\tif (pipe_command(&cp2, info.patch, strlen(info.patch), NULL, 0, NULL, 0))\n+\t\t\tdie(_(\"Cannot remove worktree changes\"));\n+\n+\t\tif (!keep_index) {\n+\t\t\tstruct argv_array args;\n+\t\t\targv_array_init(&args);\n+\t\t\targv_array_push(&args, \"reset\");\n+\t\t\targv_array_push(&args, \"--quiet\");\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_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 all = 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\t N_(\"stash untracked filed\")),\n+\t\tOPT_BOOL('a', \"all\", &all,\n+\t\t\t N_(\"stash ignored untracked files\")),\n+\t\tOPT_BOOL('k', \"keep-index\", &keep_index_set,\n+\t\t\t N_(\"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\t N_(\"stash commit message\")),\n+\t\tOPT_BOOL('p', \"patch\", &patch,\n+\t\t\t N_(\"edit current diff and apply\")),\n+\t\tOPT_END()\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, options,\n+\t\t\t\t git_stash_save_usage, PARSE_OPT_STOP_AT_NON_OPTION);\n+\n+\tif (all) {\n+\t\tinclude_untracked = 2;\n+\t}\n+\n+\tif (keep_index_set != -1) {\n+\t\tkeep_index = keep_index_set;\n+\t} else if (patch) {\n+\t\tkeep_index = 1;\n+\t}\n+\n+\treturn do_push_stash(prefix, message, keep_index, include_untracked, 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 all = 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\t N_(\"stash untracked filed\")),\n+\t\tOPT_BOOL('a', \"all\", &all,\n+\t\t\t N_(\"stash ignored untracked files\")),\n+\t\tOPT_BOOL('k', \"keep-index\", &keep_index_set,\n+\t\t\t N_(\"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\t N_(\"stash commit message\")),\n+\t\tOPT_BOOL('p', \"patch\", &patch,\n+\t\t\t N_(\"edit current diff and apply\")),\n+\t\tOPT_END()\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, options,\n+\t\t\t\t git_stash_save_usage, PARSE_OPT_STOP_AT_NON_OPTION);\n+\n+\tif (all) {\n+\t\tinclude_untracked = 2;\n+\t}\n+\n+\tif (keep_index_set != -1) {\n+\t\tkeep_index = keep_index_set;\n+\t} else if (patch) {\n+\t\tkeep_index = 1;\n+\t}\n+\n+\tif (argc != 0) {\n+\t\tstruct strbuf out = STRBUF_INIT;\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+\treturn do_push_stash(prefix, message, keep_index, include_untracked, patch, NULL);\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 child_process cp2 = CHILD_PROCESS_INIT;\n+\t\t\tstruct strbuf out = STRBUF_INIT;\n+\t\t\tstruct argv_array args;\n+\t\t\tcp.git_cmd = 1;\n+\t\t\targv_array_push(&cp.args, \"diff-tree\");\n+\t\t\targv_array_push(&cp.args, \"--binary\");\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\tpipe_command(&cp, NULL, 0, &out, 0, NULL, 0);\n+\n+\t\t\tcp2.git_cmd = 1;\n+\t\t\targv_array_push(&cp2.args, \"apply\");\n+\t\t\targv_array_push(&cp2.args, \"--cached\");\n+\t\t\tpipe_command(&cp2, out.buf, out.len, NULL, 0, NULL, 0);\n+\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_init(&args);\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;\n+\t\tstruct child_process cp2 = CHILD_PROCESS_INIT;\n+\n+\t\targv_array_init(&args);\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+\n+\t\tcp2.git_cmd = 1;\n+\t\targv_array_push(&cp2.args, \"checkout-index\");\n+\t\targv_array_push(&cp2.args, \"--all\");\n+\t\targv_array_pushf(&cp2.env_array, \"GIT_INDEX_FILE=%s\", stash_index_path);\n+\n+\t\tif (cmd_read_tree(args.argc, args.argv, prefix) ||\n+\t\t\trun_command(&cp2)) {\n+\t\t\tdie(_(\"Could not restore untracked files from stash\"));\n+\t\t}\n+\t\tset_alternate_index_output(\".git/index\");\n+\t}\n+\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(_(\"Merging %s with %s\\n\"), 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;\n+\t\targv_array_init(&args);\n+\t\targv_array_push(&args, \"rerere\");\n+\t\tcmd_rerere(args.argc, args.argv, prefix);\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 child_process cp2 = CHILD_PROCESS_INIT;\n+\t\tstruct strbuf out = STRBUF_INIT;\n+\t\tcp.git_cmd = 1;\n+\t\targv_array_push(&cp.args, \"diff-index\");\n+\t\targv_array_push(&cp.args, \"--cached\");\n+\t\targv_array_push(&cp.args, \"--name-only\");\n+\t\targv_array_push(&cp.args, \"--diff-filter=A\");\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+\n+\t\tret = reset_tree(c_tree, 0, 1);\n+\n+\t\tcp2.git_cmd = 1;\n+\t\targv_array_push(&cp2.args, \"update-index\");\n+\t\targv_array_push(&cp2.args, \"--add\");\n+\t\targv_array_push(&cp2.args, \"--stdin\");\n+\t\tret = pipe_command(&cp2, out.buf, out.len, NULL, 0, NULL, 0);\n+\t\tdiscard_cache();\n+\t\tread_cache();\n+\t}\n+\n+\tif (!quiet) {\n+\t\tstruct argv_array args;\n+\t\targv_array_init(&args);\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\t N_(\"attempt to ininstate the index\")),\n+\t\tOPT_END()\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, options,\n+\t\t\t\t git_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;\n+\tint ret;\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\n+\targv_array_init(&args);\n+\targv_array_push(&args, \"reflog\");\n+\targv_array_push(&args, \"delete\");\n+\targv_array_push(&args, \"--updateref\");\n+\targv_array_push(&args, \"--rewrite\");\n+\targv_array_push(&args, info->REV);\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->REV, sha1_to_hex(info->w_commit.hash));\n+\t\t}\n+\t} else {\n+\t\tdie(_(\"%s: Could not drop stash entry\"), info->REV);\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_init(&cp.args);\n+\targv_array_push(&cp.args, \"rev-parse\");\n+\targv_array_push(&cp.args, \"--verify\");\n+\targv_array_push(&cp.args, \"--quiet\");\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\t\t git_stash_drop_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+\tif (!info.is_stash_ref) {\n+\t\tfprintf_ln(stderr, _(\"'%s' is not a stash reference\"), commit);\n+\t\treturn 1;\n+\t}\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 object_context unused;\n+\tstruct argv_array args;\n+\n+\targc = parse_options(argc, argv, prefix, options,\n+\t\t\t\t git_stash_list_usage, PARSE_OPT_KEEP_UNKNOWN);\n+\n+\tif (get_sha1_with_context(ref_stash, 0, obj.hash, &unused))\n+\t\treturn 0;\n+\n+\targv_array_init(&args);\n+\targv_array_push(&args, \"log\");\n+\targv_array_push(&args, \"--format=%gd: %gs\");\n+\targv_array_push(&args, \"-g\");\n+\targv_array_push(&args, \"--first-parent\");\n+\targv_array_push(&args, \"-m\");\n+\targv_array_pushv(&args, argv);\n+\targv_array_push(&args, ref_stash);\n+\tif (cmd_log(args.argc, args.argv, prefix))\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;\n+\tstruct stash_info info;\n+\tconst char *commit = NULL;\n+\tint numstat = 0;\n+\tint patch = 0;\n+\n+\tstruct option options[] = {\n+\t\tOPT_BOOL(0, \"numstat\", &numstat,\n+\t\t\t N_(\"Shows number of added and deleted lines in decimal notation\")),\n+\t\tOPT_BOOL('p', \"patch\", &patch,\n+\t\t\t N_(\"Generate patch\")),\n+\t\tOPT_END()\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, options,\n+\t\t\t\t git_stash_show_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+\targv_array_init(&args);\n+\targv_array_push(&args, \"diff\");\n+\tif (numstat) {\n+\t\targv_array_push(&args, \"--numstat\");\n+\t} else if (patch) {\n+\t\targv_array_push(&args, \"-p\");\n+\t} else {\n+\t\targv_array_push(&args, \"--stat\");\n+\t}\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+\treturn cmd_diff(args.argc, args.argv, prefix);\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\t N_(\"attempt to ininstate the index\")),\n+\t\tOPT_END()\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, options,\n+\t\t\t\t git_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\tfprintf_ln(stderr, _(\"'%s' is not a stash reference\"), commit);\n+\t\treturn 1;\n+\t}\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;\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\t\t git_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\t}\n+\t}\n+\n+\tif (get_stash_info(&info, commit))\n+\t\treturn 1;\n+\n+\targv_array_init(&args);\n+\targv_array_push(&args, \"checkout\");\n+\targv_array_push(&args, \"-b\");\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+\n+\tret = do_apply_stash(prefix, &info, 1);\n+\tif (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, 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;\n+\t\t\targv_array_init(&args);\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":"320898","messageId":"CACBZZX7vsz9fRg6bggMETqfcK_WWFijQ1+YbLcfFauqa1s1KpQ@mail.gmail.com","threadId":"46062","inReplyTo":"20170528165642.14699-2-joel@teichroeb.net","subject":"Re: [PATCH v3 1/4] stash: add test for stash create with no files","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-28T17:45:16Z","receivedAt":"2017-05-28T17:45:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb wrote:\n> Ensure the command gives the correct return code\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..aaae221304 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> test foo = \"$(cat file/file)\"\n> '\n>\n> +test_expect_success 'stash create - no changes' '\n> + git stash clear &&\n> + test_when_finished \"git reset --hard HEAD\" &&\n> + git reset --hard &&\n> + git stash create > actual &&\n> + test $(cat actual | wc -l) -eq 0\n\nLooks fine like this, I don't think there's any actual portability\nconcern (as with some wrappers), but FWIW you can do this more briefly\nwith the test framework as:\n\n test_line_count = 0 actual\n\nAlthough I wonder in this case whether you don't actually mean:\n\n [...]>actual &&\n ! test -s actual\n\nI.e. isn't the test that there should be no output, not that there\nshouldn't be a \\n there? Or alternatively:\n\n test $(cat actual | wc -c) -eq 0\n\n> +'\n> +\n> test_expect_success 'stash branch - no stashes on stack, stash-like argument' '\n> git stash clear &&\n> test_when_finished \"git reset --hard HEAD\" &&\n> --\n> 2.13.0\n>\n"},{"id":"320899","messageId":"CACBZZX7AirkQBfF3RneMkTVugy=pDNkH71gvVS1jpc7eFYFicg@mail.gmail.com","threadId":"46062","inReplyTo":"20170528165642.14699-4-joel@teichroeb.net","subject":"Re: [PATCH v3 3/4] close the index lock when not writing the new index","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-28T17:46:48Z","receivedAt":"2017-05-28T17:47:14Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb wrote:\n> Signed-off-by: Joel Teichroeb \n> ---\n> builtin/add.c | 3 ++-\n> builtin/mv.c | 8 +++++---\n> builtin/rm.c | 3 ++-\n> merge-recursive.c | 8 +++++---\n> 4 files changed, 14 insertions(+), 8 deletions(-)\n>\n> diff --git a/builtin/add.c b/builtin/add.c\n> index 9f53f020d0..6b04eb2c71 100644\n> --- a/builtin/add.c\n> +++ b/builtin/add.c\n> @@ -461,7 +461,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n> if (active_cache_changed) {\n> if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n> die(_(\"Unable to write new index file\"));\n> - }\n> + } else\n> + rollback_lock_file(&lock_file);\n\nFrom Documentation/CodingGuidelines: \"When there are multiple arms to\na conditional and some of them[...]\".\n\n>\n> return exit_status;\n> }\n> diff --git a/builtin/mv.c b/builtin/mv.c\n> index 61d20037ad..ccf21de17f 100644\n> --- a/builtin/mv.c\n> +++ b/builtin/mv.c\n> @@ -293,9 +293,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n> if (gitmodules_modified)\n> stage_updated_gitmodules();\n>\n> - if (active_cache_changed &&\n> - write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n> - die(_(\"Unable to write new index file\"));\n> + if (active_cache_changed) {\n> + if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n> + die(_(\"Unable to write new index file\"));\n> + } else\n> + rollback_lock_file(&lock_file);\n>\n> return 0;\n> }\n> diff --git a/builtin/rm.c b/builtin/rm.c\n> index fb79dcab18..4c7a91888b 100644\n> --- a/builtin/rm.c\n> +++ b/builtin/rm.c\n> @@ -389,7 +389,8 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n> if (active_cache_changed) {\n> if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n> die(_(\"Unable to write new index file\"));\n> - }\n> + } else\n> + rollback_lock_file(&lock_file);\n>\n> return 0;\n> }\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index 62decd51cc..db841c0d38 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -2145,9 +2145,11 @@ int merge_recursive_generic(struct merge_options *o,\n> if (clean < 0)\n> return clean;\n>\n> - if (active_cache_changed &&\n> - write_locked_index(&the_index, lock, COMMIT_LOCK))\n> - return err(o, _(\"Unable to write index.\"));\n> + if (active_cache_changed) {\n> + if (write_locked_index(&the_index, lock, COMMIT_LOCK))\n> + return err(o, _(\"Unable to write index.\"));\n> + } else\n> + rollback_lock_file(lock);\n>\n> return clean ? 0 : 1;\n> }\n> --\n> 2.13.0\n>\n"},{"id":"320900","messageId":"CAP8UFD26oR2Y4hTYmZLBeKA8MTVe8ZTG3pJxfS9Xv8JUExbcKA@mail.gmail.com","threadId":"46062","inReplyTo":"20170528165642.14699-5-joel@teichroeb.net","subject":"Re: [PATCH v3 4/4] stash: implement builtin stash","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-05-28T17:56:28Z","receivedAt":"2017-05-28T17:56:35Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb wrote:\n\n[...]\n\n> +int untracked_files(struct strbuf *out, int include_untracked,\n> + const char **argv)\n> +{\n> + struct child_process cp = CHILD_PROCESS_INIT;\n> + cp.git_cmd = 1;\n> + argv_array_push(&cp.args, \"ls-files\");\n> + argv_array_push(&cp.args, \"-o\");\n> + argv_array_push(&cp.args, \"-z\");\n\nYou might want to use argv_array_pushl(), for example:\n\n argv_array_pushl(&cp.args, \"ls-files\", \"-o\", \"-z\", NULL);\n\n> + if (include_untracked != 2)\n> + argv_array_push(&cp.args, \"--exclude-standard\");\n> + argv_array_push(&cp.args, \"--\");\n> + if (argv)\n> + argv_array_pushv(&cp.args, argv);\n> + return pipe_command(&cp, NULL, 0, out, 0, NULL, 0);\n> +}\n> +\n> +static int check_no_changes(const char *prefix, int include_untracked,\n> + const char **argv)\n> +{\n> + struct argv_array args1;\n> + struct argv_array args2;\n> + struct strbuf out = STRBUF_INIT;\n> +\n> + argv_array_init(&args1);\n> + argv_array_push(&args1, \"diff-index\");\n> + argv_array_push(&args1, \"--quiet\");\n> + argv_array_push(&args1, \"--cached\");\n> + argv_array_push(&args1, \"HEAD\");\n> + argv_array_push(&args1, \"--ignore-submodules\");\n> + argv_array_push(&args1, \"--\");\n\nHere and in other places also you could use argv_array_pushl().\n\n> + if (argv)\n> + argv_array_pushv(&args1, argv);\n> + argv_array_init(&args2);\n> + argv_array_push(&args2, \"diff-files\");\n> + argv_array_push(&args2, \"--quiet\");\n> + argv_array_push(&args2, \"--ignore-submodules\");\n> + argv_array_push(&args2, \"--\");\n> + if (argv)\n> + argv_array_pushv(&args2, argv);\n> + if (include_untracked)\n> + untracked_files(&out, include_untracked, argv);\n> + return cmd_diff_index(args1.argc, args1.argv, prefix) == 0 &&\n> + cmd_diff_files(args2.argc, args2.argv, prefix) == 0 &&\n> + (!include_untracked || out.len == 0);\n> +}\n> +\n> +static int get_stash_info(struct stash_info *info, const char *commit)\n> +{\n> + struct strbuf w_commit_rev = STRBUF_INIT;\n> + struct strbuf b_commit_rev = STRBUF_INIT;\n> + struct strbuf i_commit_rev = STRBUF_INIT;\n> + struct strbuf u_commit_rev = STRBUF_INIT;\n> + struct strbuf w_tree_rev = STRBUF_INIT;\n> + struct strbuf b_tree_rev = STRBUF_INIT;\n> + struct strbuf i_tree_rev = STRBUF_INIT;\n> + struct strbuf u_tree_rev = STRBUF_INIT;\n> + struct strbuf commit_buf = STRBUF_INIT;\n> + struct strbuf symbolic = STRBUF_INIT;\n> + struct strbuf out = STRBUF_INIT;\n> + struct object_context unused;\n> + char *str;\n> + int ret;\n> + const char *REV = commit;\n\nWe use lower case variable names.\n\n> + struct child_process cp = CHILD_PROCESS_INIT;\n> + info->is_stash_ref = 0;\n> +\n> + if (commit == NULL) {\n> + strbuf_addf(&commit_buf, \"%s@{0}\", ref_stash);\n> + REV = commit_buf.buf;\n> + } else if (strlen(commit) < 3) {\n> + strbuf_addf(&commit_buf, \"%s@{%s}\", ref_stash, commit);\n> + REV = commit_buf.buf;\n> + }\n> + info->REV = REV;\n\nAlso the \"REV\" member of struct stash_info could be lower cased.\n\n> + strbuf_addf(&w_commit_rev, \"%s\", REV);\n> + strbuf_addf(&b_commit_rev, \"%s^1\", REV);\n> + strbuf_addf(&i_commit_rev, \"%s^2\", REV);\n> + strbuf_addf(&u_commit_rev, \"%s^3\", REV);\n> + strbuf_addf(&w_tree_rev, \"%s:\", REV);\n> + strbuf_addf(&b_tree_rev, \"%s^1:\", REV);\n> + strbuf_addf(&i_tree_rev, \"%s^2:\", REV);\n> + strbuf_addf(&u_tree_rev, \"%s^3:\", REV);\n> +\n> +\n\nSpurious new line above.\n\n> + ret = (\n> + get_sha1_with_context(w_commit_rev.buf, 0, info->w_commit.hash, &unused) == 0 &&\n> + get_sha1_with_context(b_commit_rev.buf, 0, info->b_commit.hash, &unused) == 0 &&\n> + get_sha1_with_context(i_commit_rev.buf, 0, info->i_commit.hash, &unused) == 0 &&\n> + get_sha1_with_context(w_tree_rev.buf, 0, info->w_tree.hash, &unused) == 0 &&\n> + get_sha1_with_context(b_tree_rev.buf, 0, info->b_tree.hash, &unused) == 0 &&\n> + get_sha1_with_context(i_tree_rev.buf, 0, info->i_tree.hash, &unused) == 0);\n> +\n> + if (!ret) {\n> + fprintf_ln(stderr, _(\"%s is not a valid reference\"), REV);\n> + return 1;\n\nMaybe use \"return error(_(\"%s is not a valid reference\"), REV);\"\n\n> + }\n> +\n> +\n\nSpurious new lines above.\n\n> +\n> +static void stash_create_callback(struct diff_queue_struct *q,\n> + struct diff_options *opt, void *cbdata)\n> +{\n> + int i;\n> +\n> + for (i = 0; i < q->nr; i++) {\n> + struct diff_filepair *p = q->queue[i];\n> + const char *path = p->one->path;\n> + struct stat st;\n> + remove_file_from_index(&the_index, path);\n> + if (!lstat(path, &st))\n> + add_to_index(&the_index, path, &st, 0);\n> +\n\nSpurious new line above.\n\n> + }\n> +}\n> +\n> +/* Untracked files are stored by themselves in a parentless commit, for\n> + * ease of unpacking later.\n> + */\n\nThis comment should rather be like:\n\n/*\n * Untracked files are stored by themselves in a parentless commit, for\n * ease of unpacking later.\n */\n\n> +static int save_untracked(struct stash_info *info, struct strbuf *out,\n> + int include_untracked, const char **argv)\n> +{\n> + struct child_process cp2 = CHILD_PROCESS_INIT;\n> + struct strbuf out3 = STRBUF_INIT;\n> + struct strbuf out4 = STRBUF_INIT;\n\nPlease try to find more meaningful names for such variables.\n\n> + struct object_id orig_tree;\n> +\n> + set_alternate_index_output(stash_index_path);\n> + untracked_files(&out4, include_untracked, argv);\n> +\n> + cp2.git_cmd = 1;\n> + argv_array_push(&cp2.args, \"update-index\");\n> + argv_array_push(&cp2.args, \"-z\");\n> + argv_array_push(&cp2.args, \"--add\");\n> + argv_array_push(&cp2.args, \"--remove\");\n> + argv_array_push(&cp2.args, \"--stdin\");\n> + argv_array_pushf(&cp2.env_array, \"GIT_INDEX_FILE=%s\", stash_index_path);\n> +\n> + if (pipe_command(&cp2, out4.buf, out4.len, NULL, 0, NULL, 0))\n> + return 1;\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> + discard_cache();\n> +\n> + read_cache_from(stash_index_path);\n> +\n> + write_cache_as_tree(info->u_tree.hash, 0, NULL);\n> + strbuf_addf(&out3, \"untracked files on %s\", out->buf);\n> +\n> + if (commit_tree(out3.buf, out3.len, info->u_tree.hash, NULL, info->u_commit.hash, NULL, NULL))\n> + return 1;\n> +\n> + set_alternate_index_output(\".git/index\");\n\nIsn't there a variable, a function or a constant that could be used\ninstead of the hard coded \".git/index\"?\n\n> + discard_cache();\n> + read_cache();\n> +\n> + return 0;\n> +}\n> +\n> +static int save_working_tree(struct stash_info *info, const char *prefix,\n> + const char **argv)\n> +{\n> + struct object_id orig_tree;\n> + struct rev_info rev;\n> + int nr_trees = 1;\n> + struct tree_desc t[MAX_UNPACK_TREES];\n> + struct tree *tree;\n> + struct unpack_trees_options opts;\n> + struct object *obj;\n> +\n> + discard_cache();\n> + tree = parse_tree_indirect(info->i_tree.hash);\n> + prime_cache_tree(&the_index, tree);\n> + write_index_as_tree(orig_tree.hash, &the_index, stash_index_path, 0, NULL);\n> + discard_cache();\n> +\n> + read_cache_from(stash_index_path);\n> +\n> + memset(&opts, 0, sizeof(opts));\n> +\n> + parse_tree(tree);\n> +\n> + opts.head_idx = 1;\n> + opts.src_index = &the_index;\n> + opts.dst_index = &the_index;\n> + opts.merge = 1;\n> + opts.fn = oneway_merge;\n> +\n> + init_tree_desc(t, tree->buffer, tree->size);\n> +\n> + if (unpack_trees(nr_trees, t, &opts))\n> + return 1;\n\nIn general I think we tend to return -1 instead of 1 in case of errors in C.\n\n[...]\n\n> +static int do_create_stash(struct stash_info *info, const char *prefix,\n> + const char *message, int include_untracked, int patch, const char **argv)\n> +{\n> + struct object_id curr_head;\n> + char *branch_path = NULL;\n> + const char *branch_name = NULL;\n> + struct commit_list *parents = NULL;\n> + struct strbuf out = STRBUF_INIT;\n> + struct strbuf out3 = STRBUF_INIT;\n> + struct pretty_print_context ctx = {0};\n> +\n> + struct commit *c = NULL;\n> + const char *hash;\n> + struct strbuf out2 = STRBUF_INIT;\n> +\n> + read_cache_preload(NULL);\n> + refresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL);\n> + if (check_no_changes(prefix, include_untracked, argv))\n> + return 1;\n> +\n> + if (get_sha1_tree(\"HEAD\", info->b_commit.hash))\n> + die(_(\"You do not have the initial commit yet\"));\n> +\n> + branch_path = resolve_refdup(\"HEAD\", 0, curr_head.hash, NULL);\n> +\n> + if (branch_path == NULL || strcmp(branch_path, \"HEAD\") == 0)\n> + branch_name = \"(no branch)\";\n> + else\n> + skip_prefix(branch_path, \"refs/heads/\", &branch_name);\n> +\n> + c = lookup_commit(info->b_commit.hash);\n> +\n> + ctx.output_encoding = get_log_output_encoding();\n> + ctx.abbrev = 1;\n> + ctx.fmt = CMIT_FMT_ONELINE;\n> + hash = find_unique_abbrev(c->object.oid.hash, DEFAULT_ABBREV);\n> +\n> + strbuf_addf(&out, \"%s: %s \", branch_name, hash);\n> +\n> + pretty_print_commit(&ctx, c, &out);\n> +\n> + strbuf_addf(&out3, \"index on %s\\n\", out.buf);\n> +\n> + commit_list_insert(lookup_commit(info->b_commit.hash), &parents);\n> +\n> + if (write_cache_as_tree(info->i_tree.hash, 0, NULL))\n> + die(_(\"git write-tree failed to write a tree\"));\n> +\n> + if (commit_tree(out3.buf, out3.len, info->i_tree.hash, parents, info->i_commit.hash, NULL, NULL))\n> + die(_(\"Cannot save the current index state\"));\n> +\n> +\n> + if (include_untracked) {\n> + if (save_untracked(info, &out, include_untracked, argv))\n> + die(_(\"Cannot save the untracked files\"));\n> + }\n> +\n> +\n\nSpurious new line above.\n\n> + if (patch) {\n> + if (patch_working_tree(info, prefix, argv))\n> + die(_(\"Cannot save the current worktree state\"));\n> + } else {\n> + if (save_working_tree(info, prefix, argv))\n> + die(_(\"Cannot save the current worktree state\"));\n> + }\n> + parents = NULL;\n> +\n> + if (include_untracked) {\n> + commit_list_insert(lookup_commit(info->u_commit.hash), &parents);\n> + }\n\nBraces can be removed above.\n\n> + commit_list_insert(lookup_commit(info->i_commit.hash), &parents);\n> + commit_list_insert(lookup_commit(info->b_commit.hash), &parents);\n> +\n> + if (message != NULL && strlen(message) != 0)\n> + strbuf_addf(&out2, \"On %s: %s\\n\", branch_name, message);\n> + else\n> + strbuf_addf(&out2, \"WIP on %s\\n\", out.buf);\n> +\n> + if (commit_tree(out2.buf, out2.len, info->w_tree.hash, parents, info->w_commit.hash, NULL, NULL))\n> + die(_(\"Cannot record working tree state\"));\n> +\n> + info->message = out2.buf;\n> +\n> + free(branch_path);\n> +\n> + return 0;\n> +}\n> +\n> +static int create_stash(int argc, const char **argv, const char *prefix)\n> +{\n> + int include_untracked = 0;\n> + const char *message = NULL;\n> + struct stash_info info;\n> + struct option options[] = {\n> + OPT_BOOL('u', \"include-untracked\", &include_untracked,\n> + N_(\"stash untracked filed\")),\n> + OPT_STRING('m', \"message\", &message, N_(\"message\"),\n> + N_(\"stash commit message\")),\n> + OPT_END()\n> + };\n> +\n> + argc = parse_options(argc, argv, prefix, options,\n> + git_stash_create_usage, 0);\n> +\n> + if (argc != 0) {\n> + struct strbuf out = STRBUF_INIT;\n> + int i;\n> + for (i = 0; i < argc; ++i) {\n> + if (i != 0) {\n> + strbuf_addf(&out, \" \");\n> + }\n\nBraces can be removed.\n\n[...]\n\n> +static int store_stash(int argc, const char **argv, const char *prefix)\n> +{\n> + const char *message = NULL;\n> + const char *commit = NULL;\n> + struct object_id obj;\n> + struct option options[] = {\n> + OPT_STRING('m', \"message\", &message, N_(\"message\"),\n> + N_(\"stash commit message\")),\n> + OPT__QUIET(&quiet, N_(\"be quiet, only report errors\")),\n> + OPT_END()\n> + };\n> + argc = parse_options(argc, argv, prefix, options,\n> + git_stash_store_usage, 0);\n> +\n> + if (message == NULL)\n> + message = \"Create via \\\"git stash store\\\".\";\n\nMaybe you could have initialized \"message\" when you declared the variable above.\n\n> + if (argc != 1)\n> + die(_(\"\\\"git stash store\\\" requires one argument\"));\n> +\n> + commit = argv[0];\n> +\n> + if (get_sha1(commit, obj.hash)) {\n> + fprintf_ln(stderr, _(\"fatal: %s: not a valid SHA1\"), commit);\n> + fprintf_ln(stderr, _(\"cannot update %s with %s\"), ref_stash, commit);\n> + return 1;\n\nMaybe use error() or warning().\n\n> + }\n\n[...]\n\n> +static int do_push_stash(const char *prefix, const char *message,\n> + int keep_index, int include_untracked, int patch, const char **argv)\n> +{\n> + int result;\n> + struct stash_info info;\n> +\n> + if (patch && include_untracked) {\n> + fprintf_ln(stderr, _(\"can't use --patch and --include-untracked or --all at the same time\"));\n> + return 1;\n\nerror()?\n\n> + }\n> +\n> + if (!include_untracked) {\n> + struct child_process cp = CHILD_PROCESS_INIT;\n> + struct strbuf out = STRBUF_INIT;\n> +\n> + cp.git_cmd = 1;\n> + argv_array_push(&cp.args, \"ls-files\");\n> + argv_array_push(&cp.args, \"--error-unmatch\");\n> + argv_array_push(&cp.args, \"--\");\n> + if (argv)\n> + argv_array_pushv(&cp.args, argv);\n> + result = pipe_command(&cp, NULL, 0, &out, 0, NULL, 0);\n> + if (result)\n> + return 1;\n> + }\n> +\n> + read_cache_preload(NULL);\n> + refresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL);\n> + if (check_no_changes(prefix, include_untracked, argv)) {\n> + printf(_(\"No local changes to save\\n\"));\n> + return 0;\n> + }\n> +\n> + if (!reflog_exists(ref_stash)) {\n> + if (do_clear_stash())\n> + die(_(\"Cannot initialize stash\"));\n> + }\n> +\n> +\n\nSpurious new line.\n\n> + do_create_stash(&info, prefix, message, include_untracked, patch, argv);\n> + result = do_store_stash(prefix, 1, info.message, info.w_commit);\n> +\n> + if (result == 0 && !quiet) {\n> + printf(_(\"Saved working directory and index state %s\"), info.message);\n> + }\n\nBraces can be removed.\n\nI stopped skimming here.\n\nThanks,\nChristian.\n"},{"id":"320901","messageId":"CACBZZX4anGGe1i-jtasPbfRbye-OXQMyWO5T1oFdJajnS=d1rA@mail.gmail.com","threadId":"46062","inReplyTo":"20170528165642.14699-3-joel@teichroeb.net","subject":"Re: [PATCH v3 2/4] stash: add test for stashing in a detached state","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-28T17:57:48Z","receivedAt":"2017-05-28T17:58:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb wrote:\n> Signed-off-by: Joel Teichroeb \n> ---\n> t/t3903-stash.sh | 11 +++++++++++\n> 1 file changed, 11 insertions(+)\n>\n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> index aaae221304..b86851ef46 100755\n> --- a/t/t3903-stash.sh\n> +++ b/t/t3903-stash.sh\n> @@ -808,6 +808,17 @@ 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> + echo \"WIP on (no branch): 47d5e0e initial\" >expect &&\n> + git show --pretty=%s -s ${STASH_ID} >actual &&\n> + test_cmp expect actual\n\nI thought this needed test_i18ncmp, turns out it didn't, neither the\noriginal stash code nor your replacement translates this. That's fine,\njust a note to other reviewers...\n\n> +'\n> +\n> test_expect_success 'stash -- stashes and restores the file' '\n> >foo &&\n> >bar &&\n> --\n> 2.13.0\n>\n"},{"id":"320902","messageId":"CACBZZX5cxkV4C42OwJ077b1XMHCk+uTCbdHV9Ez5EVStR4DWxg@mail.gmail.com","threadId":"46062","inReplyTo":"20170528165642.14699-5-joel@teichroeb.net","subject":"Re: [PATCH v3 4/4] stash: implement builtin stash","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-28T18:26:27Z","receivedAt":"2017-05-28T18:27:04Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb wrote:\n> Implement all git stash functionality as a builtin command\n\nFirst thanks for working on this, it's great. Applied it locally,\npasses all tests for me. A couple of comments Christian didn't cover\n\n> + info->has_u = get_sha1_with_context(u_commit_rev.buf, 0, info->u_commit.hash, &unused) == 0 &&\n> + get_sha1_with_context(u_tree_rev.buf, 0, info->u_tree.hash, &unused) == 0;\n> +\n> +\n> + /* TODO: Improve this logic */\n> + strbuf_addf(&symbolic, \"%s\", REV);\n> + str = strstr(symbolic.buf, \"@\");\n\nCould you elaborate on how this should be improved?\n\n\n> +static int patch_working_tree(struct stash_info *info, const char *prefix,\n> + const char **argv)\n> +{\n> + const char *stash_index_path = \".git/foocache2\";\n\nThis foocache path isn't created by the shell code, if it's a new\nthing that's needed (and I haven't followed this code in detail, don'n\nknow what it's for) shouldn't we give it a more descriptive name so\nthat if git crashes it's obvious what it is?\n\n> + const char *message = NULL;\n> + const char *commit = NULL;\n> + struct object_id obj;\n> + struct option options[] = {\n> + OPT_STRING('m', \"message\", &message, N_(\"message\"),\n> + N_(\"stash commit message\")),\n> + OPT__QUIET(&quiet, N_(\"be quiet, only report errors\")),\n> + OPT_END()\n> + };\n> + argc = parse_options(argc, argv, prefix, options,\n> + git_stash_store_usage, 0);\n\nNit: In general in this patch the 2nd line of parse_options doesn't\nalign with a tabwidth of 8. Ditto for indenting function arguments\n(e.g. for untracked_files).\n"},{"id":"320903","messageId":"CA+CzEk8Xx4nh=Bh533DR=Xo_BjQsxAoYNbx3=O=rtWr8+v4xsQ@mail.gmail.com","threadId":"46062","inReplyTo":"CACBZZX5cxkV4C42OwJ077b1XMHCk+uTCbdHV9Ez5EVStR4DWxg@mail.gmail.com","subject":"Re: [PATCH v3 4/4] stash: implement builtin stash","fromName":"Joel Teichroeb","fromEmail":"joel@teichroeb.net","sentAt":"2017-05-28T18:31:48Z","receivedAt":"2017-05-28T18:32:13Z","isPatch":true,"sender":{"key":"joel@teichroeb.net","avatar":"https://avatars.githubusercontent.com/u/240865?v=4"},"body":"On Sun, May 28, 2017 at 11:26 AM, Ævar Arnfjörð Bjarmason\n wrote:\n> On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb wrote:\n>> Implement all git stash functionality as a builtin command\n>\n> First thanks for working on this, it's great. Applied it locally,\n> passes all tests for me. A couple of comments Christian didn't cover\n>\n>> + info->has_u = get_sha1_with_context(u_commit_rev.buf, 0, info->u_commit.hash, &unused) == 0 &&\n>> + get_sha1_with_context(u_tree_rev.buf, 0, info->u_tree.hash, &unused) == 0;\n>> +\n>> +\n>> + /* TODO: Improve this logic */\n>> + strbuf_addf(&symbolic, \"%s\", REV);\n>> + str = strstr(symbolic.buf, \"@\");\n>\n> Could you elaborate on how this should be improved?\n>\n\nI just figured there would be a builtin function that could help here,\nbut hadn't had the chance to look into it. It's something easy to do\nin bash, but more complicated in C.\n\n>\n>> +static int patch_working_tree(struct stash_info *info, const char *prefix,\n>> + const char **argv)\n>> +{\n>> + const char *stash_index_path = \".git/foocache2\";\n>\n> This foocache path isn't created by the shell code, if it's a new\n> thing that's needed (and I haven't followed this code in detail, don'n\n> know what it's for) shouldn't we give it a more descriptive name so\n> that if git crashes it's obvious what it is?\n>\n\nOpps, I had cleaned that part up locally, but I forgot to push it when\nswitching computers. It'll be better in the next patch set.\n\n>> + const char *message = NULL;\n>> + const char *commit = NULL;\n>> + struct object_id obj;\n>> + struct option options[] = {\n>> + OPT_STRING('m', \"message\", &message, N_(\"message\"),\n>> + N_(\"stash commit message\")),\n>> + OPT__QUIET(&quiet, N_(\"be quiet, only report errors\")),\n>> + OPT_END()\n>> + };\n>> + argc = parse_options(argc, argv, prefix, options,\n>> + git_stash_store_usage, 0);\n>\n> Nit: In general in this patch the 2nd line of parse_options doesn't\n> align with a tabwidth of 8. Ditto for indenting function arguments\n> (e.g. for untracked_files).\n\nI'll fix my tab width. Forgot that long lines would change, haha.\n"},{"id":"320904","messageId":"CACBZZX6uXnE+BTfsiLNF2OT3Dsr-J99uUFEwcu-qK45OrU+1hQ@mail.gmail.com","threadId":"46062","inReplyTo":"20170528165642.14699-5-joel@teichroeb.net","subject":"Re: [PATCH v3 4/4] stash: implement builtin stash","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-28T18:51:07Z","receivedAt":"2017-05-28T18:51:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb wrote:\n> Implement all git stash functionality as a builtin command\n>\n> Signed-off-by: Joel Teichroeb \n> ---\n\nGeneral note on this that I missed in my first E-Mail, you have ~20\ncalls to argv_array_init() but none to argv_array_clear(). So you're\nleaking memory, and it obscures potential other issues with valgrind.\n\nA lot of that's easy to solve, but sometimes requires a temporary\nvariable since the code is now returning directly, e.g:\n\n@@ -1091,6 +1094,7 @@ static int list_stash(int argc, const char\n**argv, const char *prefix)\n struct object_id obj;\n struct object_context unused;\n struct argv_array args;\n+ int ret = 0;\n\n argc = parse_options(argc, argv, prefix, options,\n git_stash_list_usage, PARSE_OPT_KEEP_UNKNOWN);\n@@ -1107,9 +1111,9 @@ static int list_stash(int argc, const char\n**argv, const char *prefix)\n argv_array_pushv(&args, argv);\n argv_array_push(&args, ref_stash);\n if (cmd_log(args.argc, args.argv, prefix))\n- return 1;\n-\n- return 0;\n+ ret = 1;\n+ argv_array_clear(&args);\n+ return ret;\n }\n\nBut more generally this goes a long way to resolving the issue where\nyou have variables like out1, out2 or cp1, cp2 etc. which Christian\npointed out. I.e. you're not freeing/clearing strbufs either, instead\njust creating new ones that also aren't freed, or not clearing\nchild_process structs, e.g. this on top allows you to re-use the same\nvariable and stops leaking memory:\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex bf36ff8f9b..4e7344501a 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -729,7 +729,6 @@ static int do_push_stash(const char *prefix, const\nchar *message,\n\n if (keep_index) {\n struct child_process cp = CHILD_PROCESS_INIT;\n- struct child_process cp2 = CHILD_PROCESS_INIT;\n struct strbuf out = STRBUF_INIT;\n\n reset_tree(info.i_tree, 0, 1);\n@@ -741,13 +740,18 @@ static int do_push_stash(const char *prefix,\nconst char *message,\n argv_array_push(&cp.args, \"--\");\n argv_array_pushv(&cp.args, argv);\n pipe_command(&cp, NULL, 0, &out, 0, NULL, 0);\n+ argv_array_clear(&cp.args);\n+ child_process_clear(&cp);\n\n- cp2.git_cmd = 1;\n- argv_array_push(&cp2.args, \"checkout-index\");\n- argv_array_push(&cp2.args, \"-z\");\n- argv_array_push(&cp2.args, \"--force\");\n- argv_array_push(&cp2.args, \"--stdin\");\n- pipe_command(&cp2, out.buf, out.len, NULL, 0, NULL, 0);\n+ child_process_init(&cp);\n+ cp.git_cmd = 1;\n+ argv_array_push(&cp.args, \"checkout-index\");\n+ argv_array_push(&cp.args, \"-z\");\n+ argv_array_push(&cp.args, \"--force\");\n+ argv_array_push(&cp.args, \"--stdin\");\n+ pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0);\n+ argv_array_clear(&cp.args);\n+ child_process_clear(&cp);\n }\n } else {\n struct child_process cp2 = CHILD_PROCESS_INIT;\n"},{"id":"320905","messageId":"CACBZZX5wA57+9t+wzYc-iMpvns-zm1XAdzKszUk7PF2vkoxeMQ@mail.gmail.com","threadId":"46062","inReplyTo":"20170528165642.14699-1-joel@teichroeb.net","subject":"Re: [PATCH v3 0/4] Implement git stash as a builtin command","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-28T19:08:34Z","receivedAt":"2017-05-28T19:09:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sun, May 28, 2017 at 6:56 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\nWorth noting, with your patches the best of 3 run of all the stash tests:\n\n$ for i in {1..3}; do (time prove -j1 t*-stash*.sh) 2>&1 | grep ^real;\ndone |awk '{print $2}' | sort -n | head -n 1\n0m3.293s\n\nWithout:\n\n$ for i in {1..3}; do (time prove -j1 t*-stash*.sh) 2>&1 | grep ^real;\ndone |awk '{print $2}' | sort -n | head -n 1\n0m7.619s\n\nOf course some individual invocations are much faster than that, this\nincludes all the shell overhead of the tests themselves.\n"},{"id":"320906","messageId":"20170528192149.dgzibu334n5ja57e@sigill.intra.peff.net","threadId":"46062","inReplyTo":"CACBZZX6uXnE+BTfsiLNF2OT3Dsr-J99uUFEwcu-qK45OrU+1hQ@mail.gmail.com","subject":"Re: [PATCH v3 4/4] stash: implement builtin stash","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-28T19:21:50Z","receivedAt":"2017-05-28T19:22:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 28, 2017 at 08:51:07PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb wrote:\n> > Implement all git stash functionality as a builtin command\n> >\n> > Signed-off-by: Joel Teichroeb \n> > ---\n> \n> General note on this that I missed in my first E-Mail, you have ~20\n> calls to argv_array_init() but none to argv_array_clear(). So you're\n> leaking memory, and it obscures potential other issues with valgrind.\n\nI haven't looked carefully at the patches, but note that the argv array\nin a child_process is handled automatically by start/finish_command.\n\nSo:\n\n> @@ -1107,9 +1111,9 @@ static int list_stash(int argc, const char\n> **argv, const char *prefix)\n> argv_array_pushv(&args, argv);\n> argv_array_push(&args, ref_stash);\n> if (cmd_log(args.argc, args.argv, prefix))\n> - return 1;\n> -\n> - return 0;\n> + ret = 1;\n> + argv_array_clear(&args);\n> + return ret;\n> }\n\nThis one does need a clear, because it's calling an internal function.\nBut...\n\n> @@ -741,13 +740,18 @@ static int do_push_stash(const char *prefix,\n> const char *message,\n> argv_array_push(&cp.args, \"--\");\n> argv_array_pushv(&cp.args, argv);\n> pipe_command(&cp, NULL, 0, &out, 0, NULL, 0);\n> + argv_array_clear(&cp.args);\n\nThis one does not, because the array will have been cleared by calling\npipe_command (because it does the start/finish block itself; and yes,\nbefore you go checking, start_command() clears it even when it returns\nerror).\n\n> + child_process_clear(&cp);\n\nThis should not be necessary for the same reason.\n\n> - cp2.git_cmd = 1;\n> - argv_array_push(&cp2.args, \"checkout-index\");\n> - argv_array_push(&cp2.args, \"-z\");\n> - argv_array_push(&cp2.args, \"--force\");\n> - argv_array_push(&cp2.args, \"--stdin\");\n> - pipe_command(&cp2, out.buf, out.len, NULL, 0, NULL, 0);\n> + child_process_init(&cp);\n> + cp.git_cmd = 1;\n> + argv_array_push(&cp.args, \"checkout-index\");\n> + argv_array_push(&cp.args, \"-z\");\n> + argv_array_push(&cp.args, \"--force\");\n> + argv_array_push(&cp.args, \"--stdin\");\n> + pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0);\n> + argv_array_clear(&cp.args);\n> + child_process_clear(&cp);\n\nAnd ditto here.\n\nI'd also encourage using CHILD_PROCESS_INIT and ARGV_ARRAY_INIT constant\ninitializers instead of their function-call counterparts, as that\nmatches our usual style.\n\n-Peff\n"},{"id":"320907","messageId":"20170528192632.3iei3hxfggqkfrfh@sigill.intra.peff.net","threadId":"46062","inReplyTo":"CA+CzEk8Xx4nh=Bh533DR=Xo_BjQsxAoYNbx3=O=rtWr8+v4xsQ@mail.gmail.com","subject":"Re: [PATCH v3 4/4] stash: implement builtin stash","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-28T19:26:32Z","receivedAt":"2017-05-28T19:26:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 28, 2017 at 11:31:48AM -0700, Joel Teichroeb wrote:\n\n> >> + /* TODO: Improve this logic */\n> >> + strbuf_addf(&symbolic, \"%s\", REV);\n> >> + str = strstr(symbolic.buf, \"@\");\n> >\n> > Could you elaborate on how this should be improved?\n> \n> I just figured there would be a builtin function that could help here,\n> but hadn't had the chance to look into it. It's something easy to do\n> in bash, but more complicated in C.\n\nThere's no strbuf function for \"truncate at this character\". But:\n\n - you can use strchr for a single-character match, which is more\n efficient; i.e.:\n\n str = strchr(symbolic.buf, '@');\n\n - instead of inserting a '\\0' into the strbuf, use strbuf_setlen(),\n which also updates the symbolic.len; i.e.:\n\n strbuf_setlen(&symbolic, str - symbolic.buf);\n\n - it looks like you copy into the strbuf just to truncate, so you\n could actually get the final size before inserting into the strbuf\n using strchrnul. Like:\n\n end_of_rev = strchrnul(REV, '@');\n strbuf_add(&symbolic, REV, end_of_rev - REV);\n\n-Peff\n"},{"id":"320929","messageId":"xmqqwp906ugr.fsf@gitster.mtv.corp.google.com","threadId":"46062","inReplyTo":"CACBZZX7vsz9fRg6bggMETqfcK_WWFijQ1+YbLcfFauqa1s1KpQ@mail.gmail.com","subject":"Re: [PATCH v3 1/4] stash: add test for stash create with no files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-29T06:35:48Z","receivedAt":"2017-05-29T06:35:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n>> + git stash create > actual &&\n>> + test $(cat actual | wc -l) -eq 0\n> ...\n> Although I wonder in this case whether you don't actually mean:\n>\n> [...]>actual &&\n> ! test -s actual\n>\n> I.e. isn't the test that there should be no output, not that there\n> shouldn't be a \\n there?\n\nGood suggestion. There is \"test_must_be_empty\" you may want to use.\n\nAlso, we do not leave SP between the redirection operator and the\nfilename. Your example \">actual\" is good; the original goes against\nour style.\n\nThanks.\n"},{"id":"320930","messageId":"xmqqshjo6u6s.fsf@gitster.mtv.corp.google.com","threadId":"46062","inReplyTo":"20170528165642.14699-3-joel@teichroeb.net","subject":"Re: [PATCH v3 2/4] stash: add test for stashing in a detached state","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-29T06:41:47Z","receivedAt":"2017-05-29T06:41:53Z","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 | 11 +++++++++++\n> 1 file changed, 11 insertions(+)\n>\n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> index aaae221304..b86851ef46 100755\n> --- a/t/t3903-stash.sh\n> +++ b/t/t3903-stash.sh\n> @@ -808,6 +808,17 @@ 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> +\techo \"WIP on (no branch): 47d5e0e initial\" >expect &&\n\nIs it easy to generate this (especially 47d5e0e) at runtime? I know\nthat earlier tests in this script are brittle and will break when\nobject names change by using test vectors that hardcode object names\na bit too much, but if we can avoid making it worse, let's try to do\nso for future developers who will have to do the work of adjusting\nthe tests for possible changes to the object name hashing algorithm.\n\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"},{"id":"320931","messageId":"xmqqo9uc6tzq.fsf@gitster.mtv.corp.google.com","threadId":"46062","inReplyTo":"20170528165642.14699-4-joel@teichroeb.net","subject":"Re: [PATCH v3 3/4] close the index lock when not writing the new index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-29T06:46:01Z","receivedAt":"2017-05-29T06:46:08Z","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\nThe title says what the patch does; it does not explain why it is a\ngood change. Lockfiles will be closed automatically when we exit\nanyway, so one can argue that the current code is good.\n\nIf you are planning to add more code to these \"missing\" else clauses\nin future steps in this series, this change makes sort-of sense. If\nthat is the case, please say that in your log message.\n\n> builtin/add.c | 3 ++-\n> builtin/mv.c | 8 +++++---\n> builtin/rm.c | 3 ++-\n> merge-recursive.c | 8 +++++---\n> 4 files changed, 14 insertions(+), 8 deletions(-)\n>\n> diff --git a/builtin/add.c b/builtin/add.c\n> index 9f53f020d0..6b04eb2c71 100644\n> --- a/builtin/add.c\n> +++ b/builtin/add.c\n> @@ -461,7 +461,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n> \tif (active_cache_changed) {\n> \t\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n> \t\t\tdie(_(\"Unable to write new index file\"));\n> -\t}\n> +\t} else\n> +\t\trollback_lock_file(&lock_file);\n\nI think Ævar already pointed out the style issue, i.e.\n\n\tif (a-c-c) {\n\t\tif (w-l-i())\n\t\t\tdie(...);\n\t} else {\n\t\trollback_lock_file(&lock_file);\n\t}\n\nThe same for all other hunks in this patch.\n\nThanks.\n"},{"id":"320932","messageId":"xmqqinkk6skg.fsf@gitster.mtv.corp.google.com","threadId":"46062","inReplyTo":"20170528165642.14699-5-joel@teichroeb.net","subject":"Re: [PATCH v3 4/4] stash: implement builtin stash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-29T07:16:47Z","receivedAt":"2017-05-29T07:16:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joel Teichroeb writes:\n\n> +int untracked_files(struct strbuf *out, int include_untracked,\n> +\t\tconst char **argv)\n\nDoes this need to be public? \n\nFor a caller that wants to learn if there is _any_ untracked file,\nhaving a strbuf that holds all output is overkill. For a caller\nthat wants to learn what are the untracked paths, having a strbuf\nthat it needs to parse is not all that helpful. Only for a caller\nthat does an unusual thing, namely, just grab the output and throw\nit at another command as input, without checking what the output was\nitself at all, would benefit.\n\nSo the interface to this function doesn't look like a very good one\nif this wants to be a public helper. Perhaps mark it as \"static\"?\n\n> +{\n> +\tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\tcp.git_cmd = 1;\n> +\targv_array_push(&cp.args, \"ls-files\");\n> +\targv_array_push(&cp.args, \"-o\");\n> +\targv_array_push(&cp.args, \"-z\");\n> +\tif (include_untracked != 2)\n> +\t\targv_array_push(&cp.args, \"--exclude-standard\");\n\nThis magic number \"2\" will be hard to grok by future readers.\n\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\tconst char **argv)\n> +{\n> +\tstruct argv_array args1;\n> +\tstruct argv_array args2;\n> +\tstruct strbuf out = STRBUF_INIT;\n> +\n> +\targv_array_init(&args1);\n> +\targv_array_push(&args1, \"diff-index\");\n> +\targv_array_push(&args1, \"--quiet\");\n> +\targv_array_push(&args1, \"--cached\");\n> +\targv_array_push(&args1, \"HEAD\");\n> +\targv_array_push(&args1, \"--ignore-submodules\");\n> +\targv_array_push(&args1, \"--\");\n> +\tif (argv)\n> +\t\targv_array_pushv(&args1, argv);\n> +\targv_array_init(&args2);\n> +\targv_array_push(&args2, \"diff-files\");\n> +\targv_array_push(&args2, \"--quiet\");\n> +\targv_array_push(&args2, \"--ignore-submodules\");\n> +\targv_array_push(&args2, \"--\");\n> +\tif (argv)\n> +\t\targv_array_pushv(&args2, argv);\n\n> +\tif (include_untracked)\n> +\t\tuntracked_files(&out, include_untracked, argv);\n> +\treturn 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> +}\n\nPossible leak of out.buf here.\n\n> +\n> +static int get_stash_info(struct stash_info *info, const char *commit)\n> +{\n\nJudging from the callers of get_stash_info(), nobody passes a\n\"commit\" as parameter; as far as they are concerned, they are\ndealing with the name of one item in the stash (stash-id?). They do\nnot care the fact that the item happens to be implemented as a\ncommit object.\n\nPerhaps rename \"commit\" (parameter) to \"stash_id\" or something.\n\n> +\tstruct strbuf w_commit_rev = STRBUF_INIT;\n> +\tstruct strbuf b_commit_rev = STRBUF_INIT;\n> +\tstruct strbuf i_commit_rev = STRBUF_INIT;\n> +\tstruct strbuf u_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> +\tstruct object_context unused;\n> +\tchar *str;\n> +\tint ret;\n> +\tconst char *REV = commit;\n\nVariables and field names that are all caps become misleading.\nAvoid them.\n\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\tREV = commit_buf.buf;\n> +\t} else if (strlen(commit) < 3) {\n> +\t\tstrbuf_addf(&commit_buf, \"%s@{%s}\", ref_stash, commit);\n> +\t\tREV = commit_buf.buf;\n> +\t}\n> +\tinfo->REV = REV;\n> +\n> +\tstrbuf_addf(&w_commit_rev, \"%s\", REV);\n> +\tstrbuf_addf(&b_commit_rev, \"%s^1\", REV);\n> +\tstrbuf_addf(&i_commit_rev, \"%s^2\", REV);\n> +\tstrbuf_addf(&u_commit_rev, \"%s^3\", REV);\n> +\tstrbuf_addf(&w_tree_rev, \"%s:\", REV);\n> +\tstrbuf_addf(&b_tree_rev, \"%s^1:\", REV);\n> +\tstrbuf_addf(&i_tree_rev, \"%s^2:\", REV);\n> +\tstrbuf_addf(&u_tree_rev, \"%s^3:\", REV);\n> +\n> +\n> +\tret = (\n> +\t\tget_sha1_with_context(w_commit_rev.buf, 0, info->w_commit.hash, &unused) == 0 &&\n> +\t\tget_sha1_with_context(b_commit_rev.buf, 0, info->b_commit.hash, &unused) == 0 &&\n> +\t\tget_sha1_with_context(i_commit_rev.buf, 0, info->i_commit.hash, &unused) == 0 &&\n> +\t\tget_sha1_with_context(w_tree_rev.buf, 0, info->w_tree.hash, &unused) == 0 &&\n> +\t\tget_sha1_with_context(b_tree_rev.buf, 0, info->b_tree.hash, &unused) == 0 &&\n> +\t\tget_sha1_with_context(i_tree_rev.buf, 0, info->i_tree.hash, &unused) == 0);\n\nHmph. What's the reason to use get_sha1_with_context() if you\ndeclare the context is unused? Wouldn't plain-vanilla get_sha1()\nwork better?\n\nHaving to take both commit and tree of these sounds somewhat\ncumbersome. Do we have to know all of them and do we use all of\nthem (not a complaint, just asking because I find it somewhat\nsurprising)?\n\n> +\n> +\tif (!ret) {\n> +\t\tfprintf_ln(stderr, _(\"%s is not a valid reference\"), REV);\n> +\t\treturn 1;\n> +\t}\n> +\n> +\n> +\tinfo->has_u = get_sha1_with_context(u_commit_rev.buf, 0, info->u_commit.hash, &unused) == 0 &&\n> +\t\tget_sha1_with_context(u_tree_rev.buf, 0, info->u_tree.hash, &unused) == 0;\n\nDitto.\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> +\tstruct option options[] = {\n> +\t\tOPT_BOOL('u', \"include-untracked\", &include_untracked,\n> +\t\t\t N_(\"stash untracked filed\")),\n> +\t\tOPT_STRING('m', \"message\", &message, N_(\"message\"),\n> +\t\t\t N_(\"stash commit message\")),\n> +\t\tOPT_END()\n> +\t};\n\nOPT_BOOL(), unlike its now deprecated and removed predecessor OPT_BOOLEAN(),\ndoes not count up, so the value of include_untracked is lmited to\neither 0 or 1. So this is not the source of mysterious magic number\n\"2\" I noticed above.\n\nI'll stop here for now. \n\nThanks for working on this.\n\n"},{"id":"320959","messageId":"CA+CzEk8NoGhDB6nX6RRL7J-KenoJBE7rVfskJfCQn_iCSnA4nA@mail.gmail.com","threadId":"46062","inReplyTo":"20170528192149.dgzibu334n5ja57e@sigill.intra.peff.net","subject":"Re: [PATCH v3 4/4] stash: implement builtin stash","fromName":"Joel Teichroeb","fromEmail":"joel@teichroeb.net","sentAt":"2017-05-29T18:18:25Z","receivedAt":"2017-05-29T18:18:51Z","isPatch":true,"sender":{"key":"joel@teichroeb.net","avatar":"https://avatars.githubusercontent.com/u/240865?v=4"},"body":"Once I have all those leaks fixed, is there a way to make sure I'm not\nmissing any? I tried using valgrind with leak-check enabled, but there\nare too many leaks from other git commands.\n\nJoel\n"},{"id":"320960","messageId":"CACBZZX4VP2GZSxw5MqBY=w1EhN64=75MFnjd0CYvPTvoVdFWQw@mail.gmail.com","threadId":"46062","inReplyTo":"CA+CzEk8NoGhDB6nX6RRL7J-KenoJBE7rVfskJfCQn_iCSnA4nA@mail.gmail.com","subject":"Re: [PATCH v3 4/4] stash: implement builtin stash","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-29T18:26:20Z","receivedAt":"2017-05-29T18:26:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Mon, May 29, 2017 at 8:18 PM, Joel Teichroeb wrote:\n> Once I have all those leaks fixed, is there a way to make sure I'm not\n> missing any? I tried using valgrind with leak-check enabled, but there\n> are too many leaks from other git commands.\n\nI just used:\n\n valgrind --leak-check=full ./git-stash list\n\nAnd then skimmed things that mentioned stash.c in the pager. There\nmight be some better way to do this (e.g. instrument the test suite to\nrun valgrind for all commands and summarize that somehow), but I don't\nknow how to do that offhand...\n"},{"id":"321241","messageId":"CA+CzEk9uV5Oqa=NMhd1bjnXP-vjUqVUMH9KSzW6ehi67hqU2+Q@mail.gmail.com","threadId":"46062","inReplyTo":"CACBZZX4VP2GZSxw5MqBY=w1EhN64=75MFnjd0CYvPTvoVdFWQw@mail.gmail.com","subject":"Re: [PATCH v3 4/4] stash: implement builtin stash","fromName":"Joel Teichroeb","fromEmail":"joel@teichroeb.net","sentAt":"2017-06-01T03:29:43Z","receivedAt":"2017-06-01T03:30:09Z","isPatch":true,"sender":{"key":"joel@teichroeb.net","avatar":"https://avatars.githubusercontent.com/u/240865?v=4"},"body":"I'm running into a lot of trouble using argv_array_clear. It seems\nthat some of the builtin git cmd functions move the parameters around,\nand write new pointers to argv. There's three options I have now, and\nI'm not sure which is the best one.\n\n1. Fix all the builtin cmd functions that I use to not mess around with argv\n2. Stop using the builtin cmd functions, and use child processes exclusively\n3. Don't worry about clearing the memory used for these function calls.\n\nIt looks like the rest of the code generally does #3.\n\nAny advice here would be appreciated.\n"},{"id":"321242","messageId":"20170601040717.63ajkcgr7ihhvgmz@sigill.intra.peff.net","threadId":"46062","inReplyTo":"CA+CzEk9uV5Oqa=NMhd1bjnXP-vjUqVUMH9KSzW6ehi67hqU2+Q@mail.gmail.com","subject":"Re: [PATCH v3 4/4] stash: implement builtin stash","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-01T04:07:17Z","receivedAt":"2017-06-01T04:07:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 31, 2017 at 08:29:43PM -0700, Joel Teichroeb wrote:\n\n> I'm running into a lot of trouble using argv_array_clear. It seems\n> that some of the builtin git cmd functions move the parameters around,\n> and write new pointers to argv. There's three options I have now, and\n> I'm not sure which is the best one.\n\nHrm. It's normal for parsing to reorder the parameters (e.g., shifting\nnon-options to the front), but that should still allow a clear at the\nend. New pointers would definitely cause a problem, though. I don't know\nof any cases where we do that, but on the other hand I wouldn't be too\nsurprised to find that the revision.c options parser does some nasty\ntricks.\n\nDo you have a specific example? I'd be curious to see if we can just fix\nthe parser to be less surprising (i.e., your (1) below).\n\n> 1. Fix all the builtin cmd functions that I use to not mess around with argv\n\nIf it's just one or two spots, this might be viable.\n\n> 2. Stop using the builtin cmd functions, and use child processes exclusively\n\nThat might not be the worst thing in the world for a first cut at a\nshell to C transition, because it eliminates a whole class of possible\nproblems. But it really just side-steps the problem, as we'd want to\neventually deal with it and reduce the process count.\n\n> 3. Don't worry about clearing the memory used for these function calls.\n\nThat might be do-able, as long as the leaks are O(1) for a program run\n(and not say, a leak per commit). At the very least we should mark\nthose spots with a \"NEEDSWORK\" comment and an explanation of the issue\nso that your work in finding them isn't wasted.\n\n> It looks like the rest of the code generally does #3.\n\nIt looks like we don't actually pass argv arrays to setup_revisions()\nall that often. The three I see are:\n\n - bisect_rev_setup(), which is a known leak. This is trickier, though,\n because we actually pass the initialized rev_info out of the\n function, and the memory needs to last until we're done with the\n traversal\n\n - http-push, which does seem to free the memory\n\n - stat_tracking_info(), which does seem to free\n\nI could well believe there are places where we leak, though, especially\nfor top-level functions that exit the program when they're done.\n\nA fourth option is to massage the argv array into something that can be\nmassaged by the callee, and retain the original array for freeing. I.e.,\nsomething like:\n\n struct argv_array argv = ARGV_ARRAY_INIT;\n const char **massaged;\n\n argv_array_pushl(&argv, ...whatever...);\n\n ALLOC_ARRAY(massaged, argc);\n COPY_ARRAY(massaged, argv, argc);\n\n setup_revisions(argv.argc, massaged, &revs, NULL);\n\n /*\n * No clue what's in \"massaged\" now, as setup_revisions() may have\n * reordered things, added new elements, deleted some, etc. But we\n * don't have to care because any pointers we need to free are still\n * in the original argv struct, and we should be safe to free the\n * massaged array itself.\n */\n free(massaged);\n argv_array_clear(&argv);\n\nThat's pretty horrible, though. If setup_revisions() is requiring us to\ndo that, I'd really prefer to look into fixing it.\n\n-Peff\n"},{"id":"330830","messageId":"alpine.DEB.2.21.1.1710231308200.6482@virtualbox","threadId":"46062","inReplyTo":"20170528165642.14699-1-joel@teichroeb.net","subject":"Re: [PATCH v3 0/4] Implement git stash as a builtin command","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-10-23T11:09:11Z","receivedAt":"2017-10-23T11:09:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Joel,\n\nOn Sun, 28 May 2017, Joel Teichroeb wrote:\n\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\nI am embarrassed to say that I never found the time to have a detailed\nlook at this, and it has been 5 months since you sent this. Sorry about\nthat.\n\nDo you have an easy-to-fetch branch with this somewhere?\n\nThanks,\nDscho\n"},{"id":"330847","messageId":"CA+CzEk8RrE0AiaaNxmyio=FO+noR_qRLOgzUotJSdqOpJn4Q3Q@mail.gmail.com","threadId":"46062","inReplyTo":"alpine.DEB.2.21.1.1710231308200.6482@virtualbox","subject":"Re: [PATCH v3 0/4] Implement git stash as a builtin command","fromName":"Joel Teichroeb","fromEmail":"joel@teichroeb.net","sentAt":"2017-10-23T18:35:20Z","receivedAt":"2017-10-23T18:35:46Z","isPatch":true,"sender":{"key":"joel@teichroeb.net","avatar":"https://avatars.githubusercontent.com/u/240865?v=4"},"body":"Hi Johannes,\n\nI fixed all the tests at that time, but another one was added that I\ncould not figure out how to fix. I was planning to break the commit up\ninto smaller parts and only convert one stash command at a time\ncalling them still from the shell script. I haven't had time to do\nthat yet though.\n\nMy latest change is here:\nhttps://github.com/klusark/git/tree/rfc-c-stash\n\nThe test commits have already been merged, so just the last two are\nneeded. I haven't been keeping history of my changes, although I have\nsent it to the mailing list around 4 times.\n\nJoel\n\nOn Mon, Oct 23, 2017 at 4:09 AM, Johannes Schindelin\n wrote:\n> Hi Joel,\n>\n> On Sun, 28 May 2017, Joel Teichroeb wrote:\n>\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> I am embarrassed to say that I never found the time to have a detailed\n> look at this, and it has been 5 months since you sent this. Sorry about\n> that.\n>\n> Do you have an easy-to-fetch branch with this somewhere?\n>\n> Thanks,\n> Dscho\n"}]}