{"thread":{"id":"41279","subject":"[PATCH 2/2] stash: use \"stash--helper\"","startedAt":"2016-01-28T20:36:05Z","lastAt":"2016-02-01T23:40:11Z","messageCount":14,"participants":["Matthias Asshauer","Stefan Beller","Matthias Aßhauer","Junio C Hamano","Roberto Tyley","Thomas Gummerer","Michael Blume"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"277003","messageId":"0000015289f33e85-713596a1-2718-4c3a-bf3c-4a0f1048d401-000000@eu-west-1.amazonses.com","threadId":"41279","inReplyTo":"0000015289f33df4-d0095101-cfc0-4c41-b1e7-6137105b93fb-000000@eu-west-1.amazonses.com","subject":"[PATCH 2/2] stash: use \"stash--helper\"","fromName":"Matthias Asshauer","fromEmail":"mha1993@live.de","sentAt":"2016-01-28T20:36:05Z","receivedAt":"2016-01-28T20:36:05Z","isPatch":true,"sender":{"key":"mha1993@live.de","avatar":"https://avatars.githubusercontent.com/u/6178234?v=4"},"body":"From: Matthias Aßhauer <mha1993@live.de>\n\nUse the new \"git stash--helper\" builtin. It should be faster than the old shell code and is a first step to move\nmore shell code to C.\n\nSigned-off-by: Matthias Aßhauer <mha1993@live.de>\n---\n git-stash.sh | 10 +---------\n 1 file changed, 1 insertion(+), 9 deletions(-)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex c7c65e2..973c77b 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -112,15 +112,7 @@ create_stash () {\n \tthen\n \n \t\t# state of the working tree\n-\t\tw_tree=$( (\n-\t\t\tgit read-tree --index-output=\"$TMPindex\" -m $i_tree &&\n-\t\t\tGIT_INDEX_FILE=\"$TMPindex\" &&\n-\t\t\texport GIT_INDEX_FILE &&\n-\t\t\tgit diff --name-only -z HEAD -- >\"$TMP-stagenames\" &&\n-\t\t\tgit update-index -z --add --remove --stdin <\"$TMP-stagenames\" &&\n-\t\t\tgit write-tree &&\n-\t\t\trm -f \"$TMPindex\"\n-\t\t) ) ||\n+\t\tw_tree=$(git stash--helper --non-patch \"$TMPindex\" $i_tree) ||\n \t\t\tdie \"$(gettext \"Cannot save the current worktree state\")\"\n \n \telse\n\n--\nhttps://github.com/git/git/pull/191\n"},{"id":"277004","messageId":"0000015289f33df4-d0095101-cfc0-4c41-b1e7-6137105b93fb-000000@eu-west-1.amazonses.com","threadId":"41279","inReplyTo":"BLU436-SMTP27D65F59A444FA678FFD8AA5DA0@phx.gbl","subject":"[PATCH 1/2] stash--helper: implement \"git stash--helper\"","fromName":"Matthias Asshauer","fromEmail":"mha1993@live.de","sentAt":"2016-01-28T20:36:05Z","receivedAt":"2016-01-28T20:36:05Z","isPatch":true,"sender":{"key":"mha1993@live.de","avatar":"https://avatars.githubusercontent.com/u/6178234?v=4"},"body":"From: Matthias Aßhauer <mha1993@live.de>\n\nThis patch implements a new \"git stash--helper\" builtin plumbing\ncommand that will be used to migrate \"git-stash.sh\" to C.\n\nWe start by implementing only the \"--non-patch\" option that will\nhandle the core part of the non-patch stashing.\n\nSigned-off-by: Matthias Aßhauer <mha1993@live.de>\n---\n Makefile                |  2 ++\n builtin.h               |  1 +\n builtin/stash--helper.c | 13 ++++++++++\n git.c                   |  1 +\n stash.c                 | 65 +++++++++++++++++++++++++++++++++++++++++++++++++\n stash.h                 | 11 +++++++++\n 6 files changed, 93 insertions(+)\n create mode 100644 builtin/stash--helper.c\n create mode 100644 stash.c\n create mode 100644 stash.h\n\ndiff --git a/Makefile b/Makefile\nindex fc2f1ab..88c07ea 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -792,6 +792,7 @@ LIB_OBJS += shallow.o\n LIB_OBJS += sideband.o\n LIB_OBJS += sigchain.o\n LIB_OBJS += split-index.o\n+LIB_OBJS += stash.o\n LIB_OBJS += strbuf.o\n LIB_OBJS += streaming.o\n LIB_OBJS += string-list.o\n@@ -913,6 +914,7 @@ BUILTIN_OBJS += builtin/send-pack.o\n BUILTIN_OBJS += builtin/shortlog.o\n BUILTIN_OBJS += builtin/show-branch.o\n BUILTIN_OBJS += builtin/show-ref.o\n+BUILTIN_OBJS += builtin/stash--helper.o\n BUILTIN_OBJS += builtin/stripspace.o\n BUILTIN_OBJS += builtin/submodule--helper.o\n BUILTIN_OBJS += builtin/symbolic-ref.o\ndiff --git a/builtin.h b/builtin.h\nindex 6b95006..f1c8b39 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -118,6 +118,7 @@ extern int cmd_send_pack(int argc, const char **argv, const char *prefix);\n 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_stash__helper(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_stripspace(int argc, const char **argv, const char *prefix);\n extern int cmd_submodule__helper(int argc, const char **argv, const char *prefix);\ndiff --git a/builtin/stash--helper.c b/builtin/stash--helper.c\nnew file mode 100644\nindex 0000000..542e782\n--- /dev/null\n+++ b/builtin/stash--helper.c\n@@ -0,0 +1,13 @@\n+#include \"../stash.h\"\n+#include <string.h>\n+\n+static const char builtin_stash__helper_usage[] = {\n+\t\"Usage: git stash--helper --non-patch <tmp_indexfile> <i_tree>\"\n+};\n+\n+int cmd_stash__helper(int argc, const char **argv, const char *prefix)\n+{\n+\tif (argc == 4 && !strcmp(\"--non-patch\", argv[1]))\n+\t\treturn stash_non_patch(argv[2], argv[3], prefix);\n+\tusage(builtin_stash__helper_usage);\n+}\ndiff --git a/git.c b/git.c\nindex da278c3..9829ee8 100644\n--- a/git.c\n+++ b/git.c\n@@ -470,6 +470,7 @@ static struct cmd_struct commands[] = {\n \t{ \"show-branch\", cmd_show_branch, RUN_SETUP },\n \t{ \"show-ref\", cmd_show_ref, RUN_SETUP },\n \t{ \"stage\", cmd_add, RUN_SETUP | NEED_WORK_TREE },\n+\t{ \"stash--helper\", cmd_stash__helper, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"status\", cmd_status, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"stripspace\", cmd_stripspace },\n \t{ \"submodule--helper\", cmd_submodule__helper, RUN_SETUP },\ndiff --git a/stash.c b/stash.c\nnew file mode 100644\nindex 0000000..c3d6e67\n--- /dev/null\n+++ b/stash.c\n@@ -0,0 +1,65 @@\n+#include \"stash.h\"\n+#include \"strbuf.h\"\n+\n+static int prepare_update_index_argv(struct argv_array *args,\n+\tstruct strbuf *buf)\n+{\n+\tstruct strbuf **bufs, **b;\n+\n+\tbufs = strbuf_split(buf, '\\0');\n+\tfor (b = bufs; *b; b++)\n+\t\targv_array_pushf(args, \"%s\", (*b)->buf);\n+\targv_array_push(args, \"--\");\n+\tstrbuf_list_free(bufs);\n+\n+\treturn 0;\n+}\n+\n+int stash_non_patch(const char *tmp_indexfile, const char *i_tree,\n+\tconst char *prefix)\n+{\n+\tint result;\n+\tstruct child_process read_tree = CHILD_PROCESS_INIT;\n+\tstruct child_process diff = CHILD_PROCESS_INIT;\n+\tstruct child_process update_index = CHILD_PROCESS_INIT;\n+\tstruct child_process write_tree = CHILD_PROCESS_INIT;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\targv_array_push(&read_tree.args, \"read-tree\");\n+\targv_array_pushf(&read_tree.args, \"--index-output=%s\", tmp_indexfile);\n+\targv_array_pushl(&read_tree.args, \"-m\", i_tree, NULL);\n+\n+\targv_array_pushl(&diff.args, \"diff\", \"--name-only\", \"-z\", \"HEAD\", \"--\",\n+\t\tNULL);\n+\n+\targv_array_pushl(&update_index.args, \"update-index\", \"--add\",\n+\t\t\"--remove\", NULL);\n+\n+\targv_array_push(&write_tree.args, \"write-tree\");\n+\n+\tread_tree.env =\n+\t\tdiff.env =\n+\t\tupdate_index.env =\n+\t\twrite_tree.env = prefix;\n+\n+\tread_tree.use_shell =\n+\t\tdiff.use_shell =\n+\t\tupdate_index.use_shell =\n+\t\twrite_tree.use_shell = 1;\n+\n+\tread_tree.git_cmd =\n+\t\tdiff.git_cmd =\n+\t\tupdate_index.git_cmd =\n+\t\twrite_tree.git_cmd = 1;\n+\n+\tresult = run_command(&read_tree) ||\n+\t\tsetenv(\"GIT_INDEX_FILE\", tmp_indexfile, 1) ||\n+\t\tcapture_command(&diff, &buf, 0) ||\n+\t\tprepare_update_index_argv(&update_index.args, &buf) ||\n+\t\trun_command(&update_index) ||\n+\t\trun_command(&write_tree) ||\n+\t\tremove(tmp_indexfile);\n+\n+\tstrbuf_release(&buf);\n+\treturn result;\n+}\ndiff --git a/stash.h b/stash.h\nnew file mode 100644\nindex 0000000..0880456\n--- /dev/null\n+++ b/stash.h\n@@ -0,0 +1,11 @@\n+#ifndef STASH_H\n+#define STASH_H\n+\n+#include \"git-compat-util.h\"\n+#include \"gettext.h\"\n+#include \"run-command.h\"\n+\n+extern int stash_non_patch(const char *tmp_indexfile, const char *i_tree,\n+\tconst char *prefix);\n+\n+#endif /* STASH_H */\n\n--\nhttps://github.com/git/git/pull/191\n"},{"id":"277005","messageId":"CAGZ79kaPQP+-LpW8ExM2wmfftW4_oa7tB5XdfsdC8XHwH4aFOA@mail.gmail.com","threadId":"41279","inReplyTo":"0000015289f33e85-713596a1-2718-4c3a-bf3c-4a0f1048d401-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH 2/2] stash: use \"stash--helper\"","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-01-28T20:59:41Z","receivedAt":"2016-01-28T20:59:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jan 28, 2016 at 12:36 PM, Matthias Asshauer <mha1993@live.de> wrote:\n> From: Matthias Aßhauer <mha1993@live.de>\n>\n> Use the new \"git stash--helper\" builtin. It should be faster than the old shell code and is a first step to move\n> more shell code to C.\n\nYou had some good measurements in the coverletter, which is not going to be\nrecorded in the projects history. This part however would be part of the commit.\nSo you could move the speed improvements here (as well as the other reasoning)\non why this is a good idea. :)\n\n>\n> Signed-off-by: Matthias Aßhauer <mha1993@live.de>\n> ---\n>  git-stash.sh | 10 +---------\n>  1 file changed, 1 insertion(+), 9 deletions(-)\n>\n> diff --git a/git-stash.sh b/git-stash.sh\n> index c7c65e2..973c77b 100755\n> --- a/git-stash.sh\n> +++ b/git-stash.sh\n> @@ -112,15 +112,7 @@ create_stash () {\n>         then\n>\n>                 # state of the working tree\n> -               w_tree=$( (\n> -                       git read-tree --index-output=\"$TMPindex\" -m $i_tree &&\n> -                       GIT_INDEX_FILE=\"$TMPindex\" &&\n> -                       export GIT_INDEX_FILE &&\n> -                       git diff --name-only -z HEAD -- >\"$TMP-stagenames\" &&\n> -                       git update-index -z --add --remove --stdin <\"$TMP-stagenames\" &&\n> -                       git write-tree &&\n> -                       rm -f \"$TMPindex\"\n> -               ) ) ||\n> +               w_tree=$(git stash--helper --non-patch \"$TMPindex\" $i_tree) ||\n>                         die \"$(gettext \"Cannot save the current worktree state\")\"\n>\n>         else\n>\n> --\n> https://github.com/git/git/pull/191\n\nOh I see you're using the pull-request to email translator, cool!\n\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"277008","messageId":"BLU436-SMTP572EDBE67B8D37ECADD616A5DA0@phx.gbl","threadId":"41279","inReplyTo":"CAGZ79kaPQP+-LpW8ExM2wmfftW4_oa7tB5XdfsdC8XHwH4aFOA@mail.gmail.com","subject":"AW: [PATCH 2/2] stash: use \"stash--helper\"","fromName":"Matthias Aßhauer","fromEmail":"mha1993@live.de","sentAt":"2016-01-28T21:25:25Z","receivedAt":"2016-01-28T21:25:25Z","isPatch":true,"sender":{"key":"mha1993@live.de","avatar":"https://avatars.githubusercontent.com/u/6178234?v=4"},"body":"> You had some good measurements in the coverletter, which is not going to be recorded in the projects history. This part however would be part of the commit.\n> So you could move the speed improvements here (as well as the other reasoning) on why this is a good idea. :)\n\nI considered that, but I thought it would inflate the size of the commit message quite a bit and represents a  pretty temporary information as I'm planning to port more code. Any further progression on this would make the old meassurements kind of obsolete IMHO. I decided to move it to the coverletter, because it is only valid information if you consider both commits. If the general opinion on here is that I should add it to the commit message though, I'll gladly update it.\n\n>> https://github.com/git/git/pull/191\n>\n> Oh I see you're using the pull-request to email translator, cool! \n\nYes, I did. It definitly makes things easier if you are not used to mailing lists, but it was also a bit of a kerfuffle. I tried to start working on coverletter support, but I couldn't get it to accept the amazon SES credentials I provided. I ended up manually submiting the coverletter. It also didn't like my name.\n\nThank you for your quick feedback. \n"},{"id":"277012","messageId":"CAGZ79kYVRY+6zFnHe8LPp2E_W_gAs--Vog-HoqXW-Do_WgHGXw@mail.gmail.com","threadId":"41279","inReplyTo":"BLU436-SMTP572EDBE67B8D37ECADD616A5DA0@phx.gbl","subject":"Re: [PATCH 2/2] stash: use \"stash--helper\"","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-01-28T21:41:23Z","receivedAt":"2016-01-28T21:41:23Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jan 28, 2016 at 1:25 PM, Matthias Aßhauer <mha1993@live.de> wrote:\n>> You had some good measurements in the coverletter, which is not going to be recorded in the projects history. This part however would be part of the commit.\n>> So you could move the speed improvements here (as well as the other reasoning) on why this is a good idea. :)\n>\n> I considered that, but I thought it would inflate the size of the commit message quite a bit and represents a  pretty temporary information as I'm planning to port more code.\n\nNo worries about too large commit messages. ;) See\ndcd1742e56ebb944c4ff62346da4548e1e3be675 as an example for commit\nmessage per code raio what Jeff usually produces. :)\n\n> Any further progression on this would make the old meassurements kind of obsolete IMHO.\n\nWell it records that this specific step was beneficial, too, on the\nplatforms you measured on. If it turns out to there is a regression\nafter you rewrote lots of code, it is still traceable that this commit\nwas done in good faith.\n\n> I decided to move it to the coverletter, because it is only valid information if you consider both commits. If the general opinion on here is that I should add it to the commit message though, I'll gladly update it.\n\nHeh, true. However you enable the speedup in the second patch. If you\nwere to apply only the first (add the helper), you'd not see the\ndifference, so maybe it's worth adding it to the second commit\nmessage.\n\n>\n>>> https://github.com/git/git/pull/191\n>>\n>> Oh I see you're using the pull-request to email translator, cool!\n>\n> Yes, I did. It definitly makes things easier if you are not used to mailing lists, but it was also a bit of a kerfuffle. I tried to start working on coverletter support, but I couldn't get it to accept the amazon SES credentials I provided. I ended up manually submiting the coverletter. It also didn't like my name.\n\nNot sure if Roberto, the creator of that tool, follows the mailing\nlist.  I cc'd him.\n\n>\n> Thank you for your quick feedback.\n>\n"},{"id":"277018","messageId":"xmqqr3h1l2x8.fsf@gitster.mtv.corp.google.com","threadId":"41279","inReplyTo":"0000015289f33df4-d0095101-cfc0-4c41-b1e7-6137105b93fb-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH 1/2] stash--helper: implement \"git stash--helper\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-28T23:06:27Z","receivedAt":"2016-01-28T23:06:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthias Asshauer <mha1993@live.de> writes:\n\n> From: Matthias Aßhauer <mha1993@live.de>\n>\n> This patch implements a new \"git stash--helper\" builtin plumbing\n> command that will be used to migrate \"git-stash.sh\" to C.\n>\n> We start by implementing only the \"--non-patch\" option that will\n> handle the core part of the non-patch stashing.\n>\n> Signed-off-by: Matthias Aßhauer <mha1993@live.de>\n> ---\n>  Makefile                |  2 ++\n>  builtin.h               |  1 +\n>  builtin/stash--helper.c | 13 ++++++++++\n>  git.c                   |  1 +\n>  stash.c                 | 65 +++++++++++++++++++++++++++++++++++++++++++++++++\n>  stash.h                 | 11 +++++++++\n\nHmph, why not have everything inside builtin/stash--helper.c?  I do\nnot quite see a point of having the other two \"library-ish\" looking\nfiles.\n\nAlso I personally feel that it would be easier to review when\nthese two patches are squashed into one.  I had to go back and forth\nwhile reading the \"non-patch\" C function to see what logic from the\nscripted version it is trying to replace.\n\n> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c\n> new file mode 100644\n> index 0000000..542e782\n> --- /dev/null\n> +++ b/builtin/stash--helper.c\n> @@ -0,0 +1,13 @@\n> +#include \"../stash.h\"\n> +#include <string.h>\n> +\n> +static const char builtin_stash__helper_usage[] = {\n> +\t\"Usage: git stash--helper --non-patch <tmp_indexfile> <i_tree>\"\n> +};\n> +\n> +int cmd_stash__helper(int argc, const char **argv, const char *prefix)\n> +{\n> +\tif (argc == 4 && !strcmp(\"--non-patch\", argv[1]))\n> +\t\treturn stash_non_patch(argv[2], argv[3], prefix);\n> +\tusage(builtin_stash__helper_usage);\n> +}\n\nThis is meant to replace this sequence:\n\n\tgit read-tree --index-output=\"$TMPindex\" -m $i_tree &&\n\tGIT_INDEX_FILE=\"$TMPindex\" &&\n\texport GIT_INDEX_FILE &&\n\tgit diff --name-only -z HEAD -- >\"$TMP-stagenames\" &&\n\tgit update-index -z --add --remove --stdin <\"$TMP-stagenames\" &&\n\tgit write-tree &&\n\trm -f \"$TMPindex\"\n\nAnd outside of this section of the script, $TMPindex is never looked\nat after this part finishes (which is obvious as the last thing the\nsection does is to remove it).  As you are rewriting this whole\nsection in C, you should wonder if you can do it without using a\ntemporary file in the filesystem at all.\n\n> diff --git a/stash.c b/stash.c\n> new file mode 100644\n> index 0000000..c3d6e67\n> --- /dev/null\n> +++ b/stash.c\n> @@ -0,0 +1,65 @@\n> +#include \"stash.h\"\n> +#include \"strbuf.h\"\n> +\n> +static int prepare_update_index_argv(struct argv_array *args,\n> +\tstruct strbuf *buf)\n> +{\n> +\tstruct strbuf **bufs, **b;\n> +\n> +\tbufs = strbuf_split(buf, '\\0');\n> +\tfor (b = bufs; *b; b++)\n> +\t\targv_array_pushf(args, \"%s\", (*b)->buf);\n> +\targv_array_push(args, \"--\");\n> +\tstrbuf_list_free(bufs);\n> +\n> +\treturn 0;\n> +}\n> +\n> +int stash_non_patch(const char *tmp_indexfile, const char *i_tree,\n> +\tconst char *prefix)\n> +{\n> +\tint result;\n> +\tstruct child_process read_tree = CHILD_PROCESS_INIT;\n> +\tstruct child_process diff = CHILD_PROCESS_INIT;\n> +\tstruct child_process update_index = CHILD_PROCESS_INIT;\n> +\tstruct child_process write_tree = CHILD_PROCESS_INIT;\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\n> +\targv_array_push(&read_tree.args, \"read-tree\");\n> +\targv_array_pushf(&read_tree.args, \"--index-output=%s\", tmp_indexfile);\n> +\targv_array_pushl(&read_tree.args, \"-m\", i_tree, NULL);\n> +\n> +\targv_array_pushl(&diff.args, \"diff\", \"--name-only\", \"-z\", \"HEAD\", \"--\",\n> +\t\tNULL);\n> +\n> +\targv_array_pushl(&update_index.args, \"update-index\", \"--add\",\n> +\t\t\"--remove\", NULL);\n> +\n> +\targv_array_push(&write_tree.args, \"write-tree\");\n\nHonestly, I had high hopes after seeing the \"we are rewriting it in\nC\" but I am not enthused after seeing this.  I was hoping that the\nrewritten version would do this all in-core, by calling these\nfunctions that we already have:\n\n * read_index() to read the current index into the_index;\n\n * unpack_trees() to overlay the contents of i_tree to the contents\n   of the index;\n\n * run_diff_index() to make the comparison between the result of the\n   above and HEAD to collect the paths that are different (you'd use\n   DIFF_FORMAT_CALLBACK mechanism to collect paths---see wt-status.c\n   for existing code that already does this for hints);\n\n * add_to_index() to add or remove paths you found in the previous\n   step to the in-core index;\n\n * write_cache_as_tree() to write out the resulting index of the\n   above sequence of calls to a new tree object;\n\n * sha1_to_hex() to convert that resulting tree object name to hex\n   format;\n\n * puts() to output the result.\n\n\nActually, because i_tree is coming from $(git write-tree) that was\ndone earlier on the current index, the unpack_trees() step should\nnot even be necessary.\n\nThe first three lines of the scripted version:\n\n\tgit read-tree --index-output=\"$TMPindex\" -m $i_tree &&\n\tGIT_INDEX_FILE=\"$TMPindex\" &&\n\texport GIT_INDEX_FILE &&\n\nare creating a new file $TMPindex on the filesystem while preserving\nthe cached stat info when it can, which is a glorified version of:\n\n\tcp -p $GIT_INDEX_FILE $TMPindex\n\nIn fact, versions of \"git stash\" before 3ba2e865 (stash: copy the\nindex using --index-output instead of cp -p, 2011-03-16) simply\ndid a \"cp -p\".\n\nA C rewrite that works all in-core does not even need to write out a\ntemporary; it can just read the current index and do various things\nup to writing the contents of the in-core index as a tree, and the\nresult would be correct as long as you do not forget *NOT* to write\nthe in-core index out to $GIT_INDEX_FILE.\n"},{"id":"277020","messageId":"CAFY1edZGvdmESLdax1ErTdgyj+A7B+K9zKHsmF0Qb6d_XEk_mA@mail.gmail.com","threadId":"41279","inReplyTo":"CAGZ79kYVRY+6zFnHe8LPp2E_W_gAs--Vog-HoqXW-Do_WgHGXw@mail.gmail.com","subject":"Re: [PATCH 2/2] stash: use \"stash--helper\"","fromName":"Roberto Tyley","fromEmail":"roberto.tyley@gmail.com","sentAt":"2016-01-28T23:28:42Z","receivedAt":"2016-01-28T23:28:42Z","isPatch":true,"sender":{"key":"roberto.tyley@gmail.com","avatar":"https://avatars.githubusercontent.com/u/52038?v=4"},"body":"On 28 January 2016 at 21:41, Stefan Beller <sbeller@google.com> wrote:\n> On Thu, Jan 28, 2016 at 1:25 PM, Matthias Aßhauer <mha1993@live.de> wrote:\n>>>> https://github.com/git/git/pull/191\n>>>\n>>> Oh I see you're using the pull-request to email translator, cool!\n\nYay!\n\n>> Yes, I did. It definitly makes things easier if you are not used to mailing lists, but it was also a bit of a kerfuffle. I tried to start working on coverletter support, but I couldn't get it to accept the amazon SES credentials I provided. I ended up manually submiting the coverletter. It also didn't like my name.\n\nApologies for that - https://github.com/rtyley/submitgit/pull/26 has\njust been deployed, which should resolve the encoding for non-US ASCII\ncharacters - if you feel like submitting another patch, and want to\nput the eszett back into your GitHub account display name, I'd be\ninterested to know how that goes.\n\n> Not sure if Roberto, the creator of that tool, follows the mailing\n> list.  I cc'd him.\n\nI don't closely follow the mailing list, so thanks for the cc!\n\nRoberto\n"},{"id":"277047","messageId":"20160129112152.GO7100@hank","threadId":"41279","inReplyTo":"CAGZ79kaPQP+-LpW8ExM2wmfftW4_oa7tB5XdfsdC8XHwH4aFOA@mail.gmail.com","subject":"Re: [PATCH 2/2] stash: use \"stash--helper\"","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-01-29T11:21:52Z","receivedAt":"2016-01-29T11:21:52Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 01/28, Stefan Beller wrote:\n> On Thu, Jan 28, 2016 at 12:36 PM, Matthias Asshauer <mha1993@live.de> wrote:\n> > From: Matthias Aßhauer <mha1993@live.de>\n> >\n> > Use the new \"git stash--helper\" builtin. It should be faster than the old shell code and is a first step to move\n> > more shell code to C.\n>\n> You had some good measurements in the coverletter, which is not going to be\n> recorded in the projects history. This part however would be part of the commit.\n> So you could move the speed improvements here (as well as the other reasoning)\n> on why this is a good idea. :)\n\nIn addition it would be nice to add a performance test in t/perf,\nespecially since it seems further improvements are planned.  That will\nmake it easy for everyone to reproduce the performance numbers for\ndifferent use-cases.\n\nMatthias, feel free to squash the following (or something similar) in\nwhen you re-roll.\n\ndiff --git a/t/perf/p3000-stash.sh b/t/perf/p3000-stash.sh\nnew file mode 100755\nindex 0000000..e6e1153\n--- /dev/null\n+++ b/t/perf/p3000-stash.sh\n@@ -0,0 +1,20 @@\n+#!/bin/sh\n+\n+test_description=\"Test performance of git stash\"\n+\n+. ./perf-lib.sh\n+\n+test_perf_default_repo\n+\n+file=$(git ls-files | tail -n 30 | head -1)\n+\n+test_expect_success \"prepare repository\" \"\n+\techo x >$file\n+\"\n+\n+test_perf \"stash/stash pop\" \"\n+\tgit stash &&\n+\tgit stash pop\n+\"\n+\n+test_done\n"},{"id":"277056","messageId":"xmqq8u38jkua.fsf@gitster.mtv.corp.google.com","threadId":"41279","inReplyTo":"20160129112152.GO7100@hank","subject":"Re: [PATCH 2/2] stash: use \"stash--helper\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-29T18:34:37Z","receivedAt":"2016-01-29T18:34:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer <t.gummerer@gmail.com> writes:\n\n> Matthias, feel free to squash the following (or something similar) in\n> when you re-roll.\n>\n> diff --git a/t/perf/p3000-stash.sh b/t/perf/p3000-stash.sh\n> new file mode 100755\n> index 0000000..e6e1153\n> --- /dev/null\n> +++ b/t/perf/p3000-stash.sh\n> @@ -0,0 +1,20 @@\n> +#!/bin/sh\n> +\n> +test_description=\"Test performance of git stash\"\n> +\n> +. ./perf-lib.sh\n> +\n> +test_perf_default_repo\n> +\n> +file=$(git ls-files | tail -n 30 | head -1)\n\nIf you use \"tail -n 30\" not \"tail -30\", which is good manners, you\nwould want to be consistent and say \"head -n 1\".\n\n> +\n> +test_expect_success \"prepare repository\" \"\n> +\techo x >$file\n> +\"\n> +\n> +test_perf \"stash/stash pop\" \"\n> +\tgit stash &&\n> +\tgit stash pop\n> +\"\n> +\n> +test_done\n"},{"id":"277061","messageId":"BLU436-SMTP10996033F3EBFE2E8639F96A5DB0@phx.gbl","threadId":"41279","inReplyTo":"xmqqr3h1l2x8.fsf@gitster.mtv.corp.google.com","subject":"AW: [PATCH 1/2] stash--helper: implement \"git stash--helper\"","fromName":"Matthias Aßhauer","fromEmail":"mha1993@live.de","sentAt":"2016-01-29T19:32:45Z","receivedAt":"2016-01-29T19:32:45Z","isPatch":true,"sender":{"key":"mha1993@live.de","avatar":"https://avatars.githubusercontent.com/u/6178234?v=4"},"body":"> Hmph, why not have everything inside builtin/stash--helper.c?  I do not quite see a point of having the other two \"library-ish\" looking files.\n> \n> Also I personally feel that it would be easier to review when these two patches are squashed into one.  I had to go back and forth while reading the \"non-patch\" C function to see what logic from the scripted version it is trying to replace.\n\nI can certainly do that.\n\n> This is meant to replace this sequence:\n>\n>\tgit read-tree --index-output=\"$TMPindex\" -m $i_tree &&\n>\tGIT_INDEX_FILE=\"$TMPindex\" &&\n>\texport GIT_INDEX_FILE &&\n>\tgit diff --name-only -z HEAD -- >\"$TMP-stagenames\" &&\n>\tgit update-index -z --add --remove --stdin <\"$TMP-stagenames\" &&\n>\tgit write-tree &&\n>\trm -f \"$TMPindex\"\n>\n> And outside of this section of the script, $TMPindex is never looked at after this part finishes (which is obvious as the last thing the section does is to remove it).  As you are rewriting this whole section in C, you should wonder if you can do it without using a temporary file in the filesystem at all.\n\nI assumed the path to $TMPindex was still available in GIT_INDEX_FILE after this, as it gets exported, but I guess the surrounding $() imply a subshell.\n\n> Honestly, I had high hopes after seeing the \"we are rewriting it in C\" but I am not enthused after seeing this.  I was hoping that the rewritten version would do this all in-core, by calling these functions that we already have:\n\nThese functions might be obvious to you, but I'm new to git's source code, so I worked with the things I found documented and those that were brought to my attention by Johannes Schindelin. The run-command API is documented and seemed to be the \"official\" method of calling any git commands from within native git  code. On the other hand, the documentation for the in-core index API boils down to \"TODO: document this\". This lead me to believe I did this the intended way and just calling random functions that sound  similar to the  original command may be frowned upon at best.\n\n> A C rewrite that works all in-core does not even need to write out a temporary; it can just read the current index and do various things up to writing the contents of the in-core index as a tree, and the result would be correct as long as you do not forget *NOT* to write the in-core index out to $GIT_INDEX_FILE.\n\nI'll be working on a v2 that incorporates the feedback from you, Thomas Gummerer  and Stefan Beller then. Further feedback is of course welcome.\n\n-----Ursprüngliche Nachricht-----\nVon: Junio C Hamano [mailto:gitster@pobox.com] \nGesendet: Freitag, 29. Januar 2016 00:06\nAn: Matthias Asshauer <mha1993@live.de>\nCc: git@vger.kernel.org\nBetreff: Re: [PATCH 1/2] stash--helper: implement \"git stash--helper\"\n\nMatthias Asshauer <mha1993@live.de> writes:\n\n> From: Matthias Aßhauer <mha1993@live.de>\n>\n> This patch implements a new \"git stash--helper\" builtin plumbing \n> command that will be used to migrate \"git-stash.sh\" to C.\n>\n> We start by implementing only the \"--non-patch\" option that will \n> handle the core part of the non-patch stashing.\n>\n> Signed-off-by: Matthias Aßhauer <mha1993@live.de>\n> ---\n>  Makefile                |  2 ++\n>  builtin.h               |  1 +\n>  builtin/stash--helper.c | 13 ++++++++++\n>  git.c                   |  1 +\n>  stash.c                 | 65 +++++++++++++++++++++++++++++++++++++++++++++++++\n>  stash.h                 | 11 +++++++++\n\nHmph, why not have everything inside builtin/stash--helper.c?  I do not quite see a point of having the other two \"library-ish\" looking files.\n\nAlso I personally feel that it would be easier to review when these two patches are squashed into one.  I had to go back and forth while reading the \"non-patch\" C function to see what logic from the scripted version it is trying to replace.\n\n> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c new \n> file mode 100644 index 0000000..542e782\n> --- /dev/null\n> +++ b/builtin/stash--helper.c\n> @@ -0,0 +1,13 @@\n> +#include \"../stash.h\"\n> +#include <string.h>\n> +\n> +static const char builtin_stash__helper_usage[] = {\n> +\t\"Usage: git stash--helper --non-patch <tmp_indexfile> <i_tree>\"\n> +};\n> +\n> +int cmd_stash__helper(int argc, const char **argv, const char \n> +*prefix) {\n> +\tif (argc == 4 && !strcmp(\"--non-patch\", argv[1]))\n> +\t\treturn stash_non_patch(argv[2], argv[3], prefix);\n> +\tusage(builtin_stash__helper_usage);\n> +}\n\nThis is meant to replace this sequence:\n\n\tgit read-tree --index-output=\"$TMPindex\" -m $i_tree &&\n\tGIT_INDEX_FILE=\"$TMPindex\" &&\n\texport GIT_INDEX_FILE &&\n\tgit diff --name-only -z HEAD -- >\"$TMP-stagenames\" &&\n\tgit update-index -z --add --remove --stdin <\"$TMP-stagenames\" &&\n\tgit write-tree &&\n\trm -f \"$TMPindex\"\n\nAnd outside of this section of the script, $TMPindex is never looked at after this part finishes (which is obvious as the last thing the section does is to remove it).  As you are rewriting this whole section in C, you should wonder if you can do it without using a temporary file in the filesystem at all.\n\n> diff --git a/stash.c b/stash.c\n> new file mode 100644\n> index 0000000..c3d6e67\n> --- /dev/null\n> +++ b/stash.c\n> @@ -0,0 +1,65 @@\n> +#include \"stash.h\"\n> +#include \"strbuf.h\"\n> +\n> +static int prepare_update_index_argv(struct argv_array *args,\n> +\tstruct strbuf *buf)\n> +{\n> +\tstruct strbuf **bufs, **b;\n> +\n> +\tbufs = strbuf_split(buf, '\\0');\n> +\tfor (b = bufs; *b; b++)\n> +\t\targv_array_pushf(args, \"%s\", (*b)->buf);\n> +\targv_array_push(args, \"--\");\n> +\tstrbuf_list_free(bufs);\n> +\n> +\treturn 0;\n> +}\n> +\n> +int stash_non_patch(const char *tmp_indexfile, const char *i_tree,\n> +\tconst char *prefix)\n> +{\n> +\tint result;\n> +\tstruct child_process read_tree = CHILD_PROCESS_INIT;\n> +\tstruct child_process diff = CHILD_PROCESS_INIT;\n> +\tstruct child_process update_index = CHILD_PROCESS_INIT;\n> +\tstruct child_process write_tree = CHILD_PROCESS_INIT;\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\n> +\targv_array_push(&read_tree.args, \"read-tree\");\n> +\targv_array_pushf(&read_tree.args, \"--index-output=%s\", tmp_indexfile);\n> +\targv_array_pushl(&read_tree.args, \"-m\", i_tree, NULL);\n> +\n> +\targv_array_pushl(&diff.args, \"diff\", \"--name-only\", \"-z\", \"HEAD\", \"--\",\n> +\t\tNULL);\n> +\n> +\targv_array_pushl(&update_index.args, \"update-index\", \"--add\",\n> +\t\t\"--remove\", NULL);\n> +\n> +\targv_array_push(&write_tree.args, \"write-tree\");\n\nHonestly, I had high hopes after seeing the \"we are rewriting it in C\" but I am not enthused after seeing this.  I was hoping that the rewritten version would do this all in-core, by calling these functions that we already have:\n\n * read_index() to read the current index into the_index;\n\n * unpack_trees() to overlay the contents of i_tree to the contents\n   of the index;\n\n * run_diff_index() to make the comparison between the result of the\n   above and HEAD to collect the paths that are different (you'd use\n   DIFF_FORMAT_CALLBACK mechanism to collect paths---see wt-status.c\n   for existing code that already does this for hints);\n\n * add_to_index() to add or remove paths you found in the previous\n   step to the in-core index;\n\n * write_cache_as_tree() to write out the resulting index of the\n   above sequence of calls to a new tree object;\n\n * sha1_to_hex() to convert that resulting tree object name to hex\n   format;\n\n * puts() to output the result.\n\n\nActually, because i_tree is coming from $(git write-tree) that was done earlier on the current index, the unpack_trees() step should not even be necessary.\n\nThe first three lines of the scripted version:\n\n\tgit read-tree --index-output=\"$TMPindex\" -m $i_tree &&\n\tGIT_INDEX_FILE=\"$TMPindex\" &&\n\texport GIT_INDEX_FILE &&\n\nare creating a new file $TMPindex on the filesystem while preserving the cached stat info when it can, which is a glorified version of:\n\n\tcp -p $GIT_INDEX_FILE $TMPindex\n\nIn fact, versions of \"git stash\" before 3ba2e865 (stash: copy the index using --index-output instead of cp -p, 2011-03-16) simply did a \"cp -p\".\n\nA C rewrite that works all in-core does not even need to write out a temporary; it can just read the current index and do various things up to writing the contents of the in-core index as a tree, and the result would be correct as long as you do not forget *NOT* to write the in-core index out to $GIT_INDEX_FILE.\n"},{"id":"277062","messageId":"BLU436-SMTP17832CE801BA2CA0636A82FA5DB0@phx.gbl","threadId":"41279","inReplyTo":"CAFY1edZGvdmESLdax1ErTdgyj+A7B+K9zKHsmF0Qb6d_XEk_mA@mail.gmail.com","subject":"AW: [PATCH 2/2] stash: use \"stash--helper\"","fromName":"Matthias Aßhauer","fromEmail":"mha1993@live.de","sentAt":"2016-01-29T19:37:26Z","receivedAt":"2016-01-29T19:37:26Z","isPatch":true,"sender":{"key":"mha1993@live.de","avatar":"https://avatars.githubusercontent.com/u/6178234?v=4"},"body":">>> Yes, I did. It definitly makes things easier if you are not used to mailing lists, but it was also a bit of a kerfuffle. I tried to start working on coverletter support, but I couldn't get it to accept the amazon SES credentials I provided. I ended up manually submiting the coverletter. It also didn't like my name.\n\n> Apologies for that - https://github.com/rtyley/submitgit/pull/26 has just been deployed, which should resolve the encoding for non-US ASCII characters - if you feel like submitting another patch, and want to put the eszett back into your GitHub account display name, I'd be interested to know how that goes.\n\nYou don't need to apologise. I knew the tool was WIP and had seen the Isuue before Iattempted to submit this. I will try out the patched version when I submit v2 of this.\n"},{"id":"277063","messageId":"xmqqlh78ximf.fsf@gitster.mtv.corp.google.com","threadId":"41279","inReplyTo":"BLU436-SMTP10996033F3EBFE2E8639F96A5DB0@phx.gbl","subject":"Re: AW: [PATCH 1/2] stash--helper: implement \"git stash--helper\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-29T19:58:48Z","receivedAt":"2016-01-29T19:58:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthias Aßhauer <mha1993@live.de> writes:\n\n[administrivia: please wrap your lines to reasonable lengths]\n\n>> Honestly, I had high hopes after seeing the \"we are rewriting it\n>> in C\" but I am not enthused after seeing this.  I was hoping that\n>> the rewritten version would do this all in-core, by calling these\n>> functions that we already have:\n>\n> These functions might be obvious to you, but I'm new to git's\n> source code, ...\n\nAhh, I didn't realize I was talking with somebody unfamiliar with\nthe codebase.  Apologies.\n\nNevertheless, the list of functions I gave are a good starting\npoint; they are widely used building blocks in the codebase.\n\n> I'll be working on a v2 that incorporates the feedback from you,\n> Thomas Gummerer and Stefan Beller then. Further feedback is of\n> course welcome.\n\nThanks.\n"},{"id":"277195","messageId":"CAO2U3QhvibfEexCUuDJyj=4P+bebnrQhMaVq3VrgNBLbiTDNaA@mail.gmail.com","threadId":"41279","inReplyTo":"xmqqlh78ximf.fsf@gitster.mtv.corp.google.com","subject":"Re: AW: [PATCH 1/2] stash--helper: implement \"git stash--helper\"","fromName":"Michael Blume","fromEmail":"blume.mike@gmail.com","sentAt":"2016-02-01T23:36:27Z","receivedAt":"2016-02-01T23:36:27Z","isPatch":true,"sender":{"key":"blume.mike@gmail.com","avatar":"https://gravatar.com/avatar/1a7b440e1d942425ff4098ac7fc15b86b30cecaa56e1692a7ef8b5939ba25ea7?d=mp&s=160"},"body":"On Fri, Jan 29, 2016 at 11:58 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Matthias Aßhauer <mha1993@live.de> writes:\n>\n> [administrivia: please wrap your lines to reasonable lengths]\n>\n>>> Honestly, I had high hopes after seeing the \"we are rewriting it\n>>> in C\" but I am not enthused after seeing this.  I was hoping that\n>>> the rewritten version would do this all in-core, by calling these\n>>> functions that we already have:\n>>\n>> These functions might be obvious to you, but I'm new to git's\n>> source code, ...\n>\n> Ahh, I didn't realize I was talking with somebody unfamiliar with\n> the codebase.  Apologies.\n>\n> Nevertheless, the list of functions I gave are a good starting\n> point; they are widely used building blocks in the codebase.\n>\n>> I'll be working on a v2 that incorporates the feedback from you,\n>> Thomas Gummerer and Stefan Beller then. Further feedback is of\n>> course welcome.\n>\n> Thanks.\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n\nMaybe this isn't important given that it looks like the patch is going\nto be rewritten, but I have\n\nstash.c:43:18: warning: incompatible pointer types assigning to 'const\nchar *const *' from 'const char *'; take the address with &\n[-Wincompatible-pointer-types]\n                write_tree.env = prefix;\n"},{"id":"277197","messageId":"xmqqa8nkt2xw.fsf@gitster.mtv.corp.google.com","threadId":"41279","inReplyTo":"CAO2U3QhvibfEexCUuDJyj=4P+bebnrQhMaVq3VrgNBLbiTDNaA@mail.gmail.com","subject":"Re: AW: [PATCH 1/2] stash--helper: implement \"git stash--helper\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-01T23:40:11Z","receivedAt":"2016-02-01T23:40:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Blume <blume.mike@gmail.com> writes:\n\n> Maybe this isn't important given that it looks like the patch is going\n> to be rewritten, but I have\n>\n> stash.c:43:18: warning: incompatible pointer types assigning to 'const\n> char *const *' from 'const char *'; take the address with &\n> [-Wincompatible-pointer-types]\n>                 write_tree.env = prefix;\n\nThe way posted patch tries to use the .env field when using the\nrun-command API is totally bogus and this compilation error is a\nmanifestation of that.\n\nBut the good news is that this should become irrelevant when the\npatch is done by using internal calls ;-).\n"}]}