{"thread":{"id":"42129","subject":"[PATCH 80/83] run-command: make dup_devnull() non static","startedAt":"2016-04-24T13:39:46Z","lastAt":"2016-05-09T17:40:18Z","messageCount":24,"participants":["Christian Couder","Johannes Schindelin","Eric Sunshine","Junio C Hamano","Johannes Sixt","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":83},"messages":[{"id":"284325","messageId":"1461505189-16234-1-git-send-email-chriscool@tuxfamily.org","threadId":"42129","inReplyTo":null,"subject":"[PATCH 80/83] run-command: make dup_devnull() non static","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-04-24T13:39:46Z","receivedAt":"2016-04-24T13:39:46Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n run-command.c | 2 +-\n run-command.h | 6 ++++++\n 2 files changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 8c7115a..29d2bda 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -85,7 +85,7 @@ static inline void close_pair(int fd[2])\n }\n \n #ifndef GIT_WINDOWS_NATIVE\n-static inline void dup_devnull(int to)\n+void dup_devnull(int to)\n {\n \tint fd = open(\"/dev/null\", O_RDWR);\n \tif (fd < 0)\ndiff --git a/run-command.h b/run-command.h\nindex de1727e..3aa244a 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -200,4 +200,10 @@ int run_processes_parallel(int n,\n \t\t\t   task_finished_fn,\n \t\t\t   void *pp_cb);\n \n+/**\n+ * Misc helper functions\n+ */\n+\n+void dup_devnull(int to);\n+\n #endif\n-- \n2.8.1.300.g5fed0c0\n"},{"id":"284326","messageId":"1461505189-16234-2-git-send-email-chriscool@tuxfamily.org","threadId":"42129","inReplyTo":"1461505189-16234-1-git-send-email-chriscool@tuxfamily.org","subject":"[PATCH 81/83] apply: roll back index in case of error","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-04-24T13:39:47Z","receivedAt":"2016-04-24T13:39:47Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n apply.c | 20 ++++++++++++++++----\n 1 file changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 86e0d20..7cee834 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -4718,8 +4718,11 @@ int apply_all_patches(struct apply_state *state,\n \n \t\tif (!strcmp(arg, \"-\")) {\n \t\t\tres = apply_patch(state, 0, \"<stdin>\", options);\n-\t\t\tif (res < 0)\n+\t\t\tif (res < 0) {\n+\t\t\t\tif (state->lock_file)\n+\t\t\t\t\trollback_lock_file(state->lock_file);\n \t\t\t\treturn -1;\n+\t\t\t}\n \t\t\terrs |= res;\n \t\t\tread_stdin = 0;\n \t\t\tcontinue;\n@@ -4734,16 +4737,22 @@ int apply_all_patches(struct apply_state *state,\n \t\tread_stdin = 0;\n \t\tset_default_whitespace_mode(state);\n \t\tres = apply_patch(state, fd, arg, options);\n-\t\tif (res < 0)\n+\t\tif (res < 0) {\n+\t\t\tif (state->lock_file)\n+\t\t\t\trollback_lock_file(state->lock_file);\n \t\t\treturn -1;\n+\t\t}\n \t\terrs |= res;\n \t\tclose(fd);\n \t}\n \tset_default_whitespace_mode(state);\n \tif (read_stdin) {\n \t\tres = apply_patch(state, 0, \"<stdin>\", options);\n-\t\tif (res < 0)\n+\t\tif (res < 0) {\n+\t\t\tif (state->lock_file)\n+\t\t\t\trollback_lock_file(state->lock_file);\n \t\t\treturn -1;\n+\t\t}\n \t\terrs |= res;\n \t}\n \n@@ -4757,11 +4766,14 @@ int apply_all_patches(struct apply_state *state,\n \t\t\t\t   squelched),\n \t\t\t\tsquelched);\n \t\t}\n-\t\tif (state->ws_error_action == die_on_ws_error)\n+\t\tif (state->ws_error_action == die_on_ws_error) {\n+\t\t\tif (state->lock_file)\n+\t\t\t\trollback_lock_file(state->lock_file);\n \t\t\treturn error(Q_(\"%d line adds whitespace errors.\",\n \t\t\t\t\t\"%d lines add whitespace errors.\",\n \t\t\t\t\tstate->whitespace_error),\n \t\t\t\t     state->whitespace_error);\n+\t\t}\n \t\tif (state->applied_after_fixing_ws && state->apply)\n \t\t\twarning(\"%d line%s applied after\"\n \t\t\t\t\" fixing whitespace errors.\",\n-- \n2.8.1.300.g5fed0c0\n"},{"id":"284327","messageId":"1461505189-16234-3-git-send-email-chriscool@tuxfamily.org","threadId":"42129","inReplyTo":"1461505189-16234-1-git-send-email-chriscool@tuxfamily.org","subject":"[PATCH 82/83] environment: add set_index_file()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-04-24T13:39:48Z","receivedAt":"2016-04-24T13:39:48Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n cache.h       | 1 +\n environment.c | 5 +++++\n 2 files changed, 6 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex 2711048..7f36aa3 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -461,6 +461,7 @@ extern int is_inside_work_tree(void);\n extern const char *get_git_dir(void);\n extern const char *get_git_common_dir(void);\n extern char *get_object_directory(void);\n+extern void set_index_file(char *index_file);\n extern char *get_index_file(void);\n extern char *get_graft_file(void);\n extern int set_git_dir(const char *path);\ndiff --git a/environment.c b/environment.c\nindex 57acb2f..5a57822 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -290,6 +290,11 @@ int odb_pack_keep(char *name, size_t namesz, const unsigned char *sha1)\n \treturn open(name, O_RDWR|O_CREAT|O_EXCL, 0600);\n }\n \n+void set_index_file(char *index_file)\n+{\n+\tgit_index_file = index_file;\n+}\n+\n char *get_index_file(void)\n {\n \tif (!git_index_file)\n-- \n2.8.1.300.g5fed0c0\n"},{"id":"284328","messageId":"1461505189-16234-4-git-send-email-chriscool@tuxfamily.org","threadId":"42129","inReplyTo":"1461505189-16234-1-git-send-email-chriscool@tuxfamily.org","subject":"[PATCH 83/83] builtin/am: use apply api in run_apply()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-04-24T13:39:49Z","receivedAt":"2016-04-24T13:39:49Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This replaces run_apply() implementation with a new one that\nuses the apply api that has been previously prepared in\napply.c and apply.h.\n\nThis shoud improve performance a lot in certain cases.\n\nAs the previous implementation was creating a new `git apply`\nprocess to apply each patch, it could be slow on systems like\nWindows where it is costly to create new processes.\n\nAlso the new `git apply` process had to read the index from\ndisk, and when the process was done the calling process\ndiscarded its own index and read back from disk the new\nindex that had been created by the `git apply` process.\n\nThis could be very inefficient with big repositories that\nhave big index files, especially when the system decided\nthat it was a good idea to run the `git apply` processes on\na different processor core.\n\nAlso eliminating index reads enables further performance\nimprovements by using:\n\n`git update-index --split-index`\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n builtin/am.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++++-----------\n 1 file changed, 85 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex d003939..85a77d7 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -28,6 +28,7 @@\n #include \"rerere.h\"\n #include \"prompt.h\"\n #include \"mailinfo.h\"\n+#include \"apply.h\"\n \n /**\n  * Returns 1 if the file is empty or does not exist, 0 otherwise.\n@@ -1522,39 +1523,105 @@ static int parse_mail_rebase(struct am_state *state, const char *mail)\n  */\n static int run_apply(const struct am_state *state, const char *index_file)\n {\n-\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\n-\tcp.git_cmd = 1;\n-\n-\tif (index_file)\n-\t\targv_array_pushf(&cp.env_array, \"GIT_INDEX_FILE=%s\", index_file);\n+\tstruct argv_array apply_paths = ARGV_ARRAY_INIT;\n+\tstruct argv_array apply_opts = ARGV_ARRAY_INIT;\n+\tstruct apply_state apply_state;\n+\tint save_stdout_fd, save_stderr_fd;\n+\tint res, opts_left;\n+\tchar *save_index_file;\n+\n+\tstruct option am_apply_options[] = {\n+\t\t{ OPTION_CALLBACK, 0, \"whitespace\", &apply_state, N_(\"action\"),\n+\t\t\tN_(\"detect new or modified lines that have whitespace errors\"),\n+\t\t\t0, apply_option_parse_whitespace },\n+\t\t{ OPTION_CALLBACK, 0, \"ignore-space-change\", &apply_state, NULL,\n+\t\t\tN_(\"ignore changes in whitespace when finding context\"),\n+\t\t\tPARSE_OPT_NOARG, apply_option_parse_space_change },\n+\t\t{ OPTION_CALLBACK, 0, \"ignore-whitespace\", &apply_state, NULL,\n+\t\t\tN_(\"ignore changes in whitespace when finding context\"),\n+\t\t\tPARSE_OPT_NOARG, apply_option_parse_space_change },\n+\t\t{ OPTION_CALLBACK, 0, \"directory\", &apply_state, N_(\"root\"),\n+\t\t\tN_(\"prepend <root> to all filenames\"),\n+\t\t\t0, apply_option_parse_directory },\n+\t\t{ OPTION_CALLBACK, 0, \"exclude\", &apply_state, N_(\"path\"),\n+\t\t\tN_(\"don't apply changes matching the given path\"),\n+\t\t\t0, apply_option_parse_exclude },\n+\t\t{ OPTION_CALLBACK, 0, \"include\", &apply_state, N_(\"path\"),\n+\t\t\tN_(\"apply changes matching the given path\"),\n+\t\t\t0, apply_option_parse_include },\n+\t\tOPT_INTEGER('C', NULL, &apply_state.p_context,\n+\t\t\t\tN_(\"ensure at least <n> lines of context match\")),\n+\t\t{ OPTION_CALLBACK, 'p', NULL, &apply_state, N_(\"num\"),\n+\t\t\tN_(\"remove <num> leading slashes from traditional diff paths\"),\n+\t\t\t0, apply_option_parse_p },\n+\t\tOPT_BOOL(0, \"reject\", &apply_state.apply_with_reject,\n+\t\t\tN_(\"leave the rejected hunks in corresponding *.rej files\")),\n+\t\tOPT_END()\n+\t};\n \n \t/*\n \t * If we are allowed to fall back on 3-way merge, don't give false\n \t * errors during the initial attempt.\n \t */\n+\n \tif (state->threeway && !index_file) {\n-\t\tcp.no_stdout = 1;\n-\t\tcp.no_stderr = 1;\n+\t\tsave_stdout_fd = dup(1);\n+\t\tdup_devnull(1);\n+\t\tsave_stderr_fd = dup(2);\n+\t\tdup_devnull(2);\n \t}\n \n-\targv_array_push(&cp.args, \"apply\");\n+\tif (index_file) {\n+\t\tsave_index_file = get_index_file();\n+\t\tset_index_file((char *)index_file);\n+\t}\n \n-\targv_array_pushv(&cp.args, state->git_apply_opts.argv);\n+\tif (init_apply_state(&apply_state, NULL))\n+\t\tdie(\"init_apply_state() failed\");\n+\n+\targv_array_push(&apply_opts, \"apply\");\n+\targv_array_pushv(&apply_opts, state->git_apply_opts.argv);\n+\n+\topts_left = parse_options(apply_opts.argc, apply_opts.argv,\n+\t\t\t\t  NULL, am_apply_options, NULL, 0);\n+\n+\tif (opts_left != 0)\n+\t\tdie(\"unknown option passed thru to git apply\");\n \n \tif (index_file)\n-\t\targv_array_push(&cp.args, \"--cached\");\n+\t\tapply_state.cached = 1;\n \telse\n-\t\targv_array_push(&cp.args, \"--index\");\n+\t\tapply_state.check_index = 1;\n \n-\targv_array_push(&cp.args, am_path(state, \"patch\"));\n+\tif (check_apply_state(&apply_state, 0))\n+\t\tdie(\"check_apply_state() failed\");\n \n-\tif (run_command(&cp))\n-\t\treturn -1;\n+\targv_array_push(&apply_paths, am_path(state, \"patch\"));\n \n-\t/* Reload index as git-apply will have modified it. */\n-\tdiscard_cache();\n-\tread_cache_from(index_file ? index_file : get_index_file());\n+\tres = apply_all_patches(&apply_state, apply_paths.argc, apply_paths.argv, 0);\n+\n+\t/* Restore stdout and stderr */\n+\tif (state->threeway && !index_file) {\n+\t\tdup2(save_stdout_fd, 1);\n+\t\tclose(save_stdout_fd);\n+\t\tdup2(save_stderr_fd, 2);\n+\t\tclose(save_stderr_fd);\n+\t}\n+\n+\tif (index_file)\n+\t\tset_index_file(save_index_file);\n+\n+\targv_array_clear(&apply_paths);\n+\targv_array_clear(&apply_opts);\n+\n+\tif (res)\n+\t\treturn res;\n+\n+\tif (index_file) {\n+\t\t/* Reload index as apply_all_patches() will have modified it. */\n+\t\tdiscard_cache();\n+\t\tread_cache_from(index_file);\n+\t}\n \n \treturn 0;\n }\n-- \n2.8.1.300.g5fed0c0\n"},{"id":"284410","messageId":"alpine.DEB.2.20.1604251701180.2896@virtualbox","threadId":"42129","inReplyTo":"1461505189-16234-4-git-send-email-chriscool@tuxfamily.org","subject":"Re: [PATCH 83/83] builtin/am: use apply api in run_apply()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-04-25T15:03:36Z","receivedAt":"2016-04-25T15:03:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Chris,\n\nOn Sun, 24 Apr 2016, Christian Couder wrote:\n\n> [...]\n>  \t/*\n>  \t * If we are allowed to fall back on 3-way merge, don't give false\n>  \t * errors during the initial attempt.\n>  \t */\n> +\n>  \tif (state->threeway && !index_file) {\n> -\t\tcp.no_stdout = 1;\n> -\t\tcp.no_stderr = 1;\n> +\t\tsave_stdout_fd = dup(1);\n> +\t\tdup_devnull(1);\n> +\t\tsave_stderr_fd = dup(2);\n> +\t\tdup_devnull(2);\n\nI wonder. It should be possible to teach the apply function to be quiet by\ndefault, yes? That would be more elegant than dup()ing back and forth.\n\nCiao,\nDscho\n"},{"id":"284411","messageId":"alpine.DEB.2.20.1604251704300.2896@virtualbox","threadId":"42129","inReplyTo":"1461505189-16234-1-git-send-email-chriscool@tuxfamily.org","subject":"Re: [PATCH 80/83] run-command: make dup_devnull() non static","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-04-25T15:05:09Z","receivedAt":"2016-04-25T15:05:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Chris,\n\nOn Sun, 24 Apr 2016, Christian Couder wrote:\n\n> diff --git a/run-command.c b/run-command.c\n> index 8c7115a..29d2bda 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -85,7 +85,7 @@ static inline void close_pair(int fd[2])\n>  }\n>  \n>  #ifndef GIT_WINDOWS_NATIVE\n> -static inline void dup_devnull(int to)\n> +void dup_devnull(int to)\n>  {\n\nThe #ifndef GIT_WINDOWS_NATIVE rings very, very loud alarm bells.\n\nCiao,\nDscho\n"},{"id":"284431","messageId":"alpine.DEB.2.20.1604251802480.2896@virtualbox","threadId":"42129","inReplyTo":"1461505189-16234-2-git-send-email-chriscool@tuxfamily.org","subject":"Re: [PATCH 81/83] apply: roll back index in case of error","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-04-25T16:06:03Z","receivedAt":"2016-04-25T16:06:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Chris,\n\nOn Sun, 24 Apr 2016, Christian Couder wrote:\n\n> @@ -4734,16 +4737,22 @@ int apply_all_patches(struct apply_state *state,\n>  \t\tread_stdin = 0;\n>  \t\tset_default_whitespace_mode(state);\n>  \t\tres = apply_patch(state, fd, arg, options);\n> -\t\tif (res < 0)\n> +\t\tif (res < 0) {\n> +\t\t\tif (state->lock_file)\n> +\t\t\t\trollback_lock_file(state->lock_file);\n>  \t\t\treturn -1;\n> +\t\t}\n>  \t\terrs |= res;\n>  \t\tclose(fd);\n\nIn case of error, this leaves fd open, which in the end will prevent the\n\"patch\" file, and hence the \"rebase-apply/\" directory from being removed\non Windows. This triggered a failure of t4014 here (and possibly more, but\nit took me quite a while to track this down, what with builtin/am.c's\nam_destroy() not bothering at all to check the return value of\nremove_dir_recursively(), resulting in the error to be caught only much,\nmuch later).\n\nCould you please review all open()/close() and fopen()/fclose() calls in\nyour patch series, to make sure that there are no mistakes? A passing test\nsuite does not really make me confident here, as our code coverage is not\nquite 100%.\n\nThanks,\nDscho\n"},{"id":"285144","messageId":"alpine.DEB.2.20.1605020906400.9313@virtualbox","threadId":"42129","inReplyTo":"alpine.DEB.2.20.1604251802480.2896@virtualbox","subject":"Re: [PATCH 81/83] apply: roll back index in case of error","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-05-02T07:07:01Z","receivedAt":"2016-05-02T07:07:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\napparently this mail never made it to the list???\n\nOn Mon, 25 Apr 2016, Johannes Schindelin wrote:\n\n> Hi Chris,\n> \n> On Sun, 24 Apr 2016, Christian Couder wrote:\n> \n> > @@ -4734,16 +4737,22 @@ int apply_all_patches(struct apply_state *state,\n> >  \t\tread_stdin = 0;\n> >  \t\tset_default_whitespace_mode(state);\n> >  \t\tres = apply_patch(state, fd, arg, options);\n> > -\t\tif (res < 0)\n> > +\t\tif (res < 0) {\n> > +\t\t\tif (state->lock_file)\n> > +\t\t\t\trollback_lock_file(state->lock_file);\n> >  \t\t\treturn -1;\n> > +\t\t}\n> >  \t\terrs |= res;\n> >  \t\tclose(fd);\n> \n> In case of error, this leaves fd open, which in the end will prevent the\n> \"patch\" file, and hence the \"rebase-apply/\" directory from being removed\n> on Windows. This triggered a failure of t4014 here (and possibly more, but\n> it took me quite a while to track this down, what with builtin/am.c's\n> am_destroy() not bothering at all to check the return value of\n> remove_dir_recursively(), resulting in the error to be caught only much,\n> much later).\n> \n> Could you please review all open()/close() and fopen()/fclose() calls in\n> your patch series, to make sure that there are no mistakes? A passing test\n> suite does not really make me confident here, as our code coverage is not\n> quite 100%.\n> \n> Thanks,\n> Dscho\n> \n"},{"id":"285147","messageId":"CAPig+cTovkSPi3W2-46fVLOdiYMCCwUkBNKys0C40dJwZ_dL-g@mail.gmail.com","threadId":"42129","inReplyTo":"alpine.DEB.2.20.1605020906400.9313@virtualbox","subject":"Re: [PATCH 81/83] apply: roll back index in case of error","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-02T07:18:37Z","receivedAt":"2016-05-02T07:18:37Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, May 2, 2016 at 3:07 AM, Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n> apparently this mail never made it to the list???\n\nIt made it to the list[1].\n\n[1]: http://git.661346.n2.nabble.com/PATCH-80-83-run-command-make-dup-devnull-non-static-tp7654127p7654233.html\n\n> On Mon, 25 Apr 2016, Johannes Schindelin wrote:\n>\n>> Hi Chris,\n>>\n>> On Sun, 24 Apr 2016, Christian Couder wrote:\n>>\n>> > @@ -4734,16 +4737,22 @@ int apply_all_patches(struct apply_state *state,\n>> >             read_stdin = 0;\n>> >             set_default_whitespace_mode(state);\n>> >             res = apply_patch(state, fd, arg, options);\n>> > -           if (res < 0)\n>> > +           if (res < 0) {\n>> > +                   if (state->lock_file)\n>> > +                           rollback_lock_file(state->lock_file);\n>> >                     return -1;\n>> > +           }\n>> >             errs |= res;\n>> >             close(fd);\n>>\n>> In case of error, this leaves fd open, which in the end will prevent the\n>> \"patch\" file, and hence the \"rebase-apply/\" directory from being removed\n>> on Windows. This triggered a failure of t4014 here (and possibly more, but\n>> it took me quite a while to track this down, what with builtin/am.c's\n>> am_destroy() not bothering at all to check the return value of\n>> remove_dir_recursively(), resulting in the error to be caught only much,\n>> much later).\n>>\n>> Could you please review all open()/close() and fopen()/fclose() calls in\n>> your patch series, to make sure that there are no mistakes? A passing test\n>> suite does not really make me confident here, as our code coverage is not\n>> quite 100%.\n>>\n>> Thanks,\n>> Dscho\n"},{"id":"285318","messageId":"CAP8UFD0tGruUTrk5aJ7oGM9jj1HzpM2afqEsJfKyuDB+CWc=fQ@mail.gmail.com","threadId":"42129","inReplyTo":"alpine.DEB.2.20.1604251802480.2896@virtualbox","subject":"Re: [PATCH 81/83] apply: roll back index in case of error","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-05-03T12:57:52Z","receivedAt":"2016-05-03T12:57:52Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi Dscho,\n\nOn Mon, Apr 25, 2016 at 6:06 PM, Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n> Hi Chris,\n>\n> On Sun, 24 Apr 2016, Christian Couder wrote:\n>\n>> @@ -4734,16 +4737,22 @@ int apply_all_patches(struct apply_state *state,\n>>               read_stdin = 0;\n>>               set_default_whitespace_mode(state);\n>>               res = apply_patch(state, fd, arg, options);\n>> -             if (res < 0)\n>> +             if (res < 0) {\n>> +                     if (state->lock_file)\n>> +                             rollback_lock_file(state->lock_file);\n>>                       return -1;\n>> +             }\n>>               errs |= res;\n>>               close(fd);\n>\n> In case of error, this leaves fd open, which in the end will prevent the\n> \"patch\" file, and hence the \"rebase-apply/\" directory from being removed\n> on Windows. This triggered a failure of t4014 here (and possibly more, but\n> it took me quite a while to track this down, what with builtin/am.c's\n> am_destroy() not bothering at all to check the return value of\n> remove_dir_recursively(), resulting in the error to be caught only much,\n> much later).\n\nSorry about that and thanks for tracking down the source of the test failure.\nI fixed this by moving the \"close(fd)\" call just after the \"apply_patch()\" call.\n\n> Could you please review all open()/close() and fopen()/fclose() calls in\n> your patch series, to make sure that there are no mistakes? A passing test\n> suite does not really make me confident here, as our code coverage is not\n> quite 100%.\n\nOk, I will have another look at the 2 other places where there are\nopen()/close() or fopen()/fclose() calls.\n\nThanks,\nChristian.\n"},{"id":"285324","messageId":"xmqqh9efp30n.fsf@gitster.mtv.corp.google.com","threadId":"42129","inReplyTo":"1461505189-16234-3-git-send-email-chriscool@tuxfamily.org","subject":"Re: [PATCH 82/83] environment: add set_index_file()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-03T15:36:24Z","receivedAt":"2016-05-03T15:36:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> +void set_index_file(char *index_file)\n> +{\n> +\tgit_index_file = index_file;\n> +}\n\nWhat's the rationale for this change, and more importantly, the\nownership rule for the string?  When you call this function, does\nthe caller still own that piece of memory?  When you call this\ntwice, where does the old value go and who is responsible for\ntaking care of not leaking it?\n\nCannot guess any of the above with no proposed log message (that\ncomment applies most of your patches in this series).\n"},{"id":"285462","messageId":"CAP8UFD1kfjU58cey-T9g3Nwo1RM6DN6MrgxyJQL8Ad53a+3DmQ@mail.gmail.com","threadId":"42129","inReplyTo":"xmqqh9efp30n.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 82/83] environment: add set_index_file()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-05-04T11:50:18Z","receivedAt":"2016-05-04T11:50:18Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, May 3, 2016 at 5:36 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Christian Couder <christian.couder@gmail.com> writes:\n>\n>> +void set_index_file(char *index_file)\n>> +{\n>> +     git_index_file = index_file;\n>> +}\n>\n> What's the rationale for this change, and more importantly, the\n> ownership rule for the string?  When you call this function, does\n> the caller still own that piece of memory?  When you call this\n> twice, where does the old value go and who is responsible for\n> taking care of not leaking it?\n>\n> Cannot guess any of the above with no proposed log message (that\n> comment applies most of your patches in this series).\n\nYeah, I agree that I could have been more verbose on this one, and\nsome other ones too.\n\nThe reason for this is that run_apply() in builtin/am.c has a \"const\nchar *index_file\" argument.\nThe current version of run_apply() does:\n\n        if (index_file)\n                argv_array_pushf(&cp.env_array, \"GIT_INDEX_FILE=%s\",\nindex_file);\n\nto pass that parameter to the `git apply` process that it launches.\n\nI couldn't find a good way other than doing:\n\n    if (index_file) {\n        save_index_file = get_index_file();\n        set_index_file((char *)index_file);\n    }\n\n    /* Call libified apply functions */\n    ...\n\n    if (index_file)\n        set_index_file(save_index_file);\n\nto do the equivalent of what was done previously.\n\nSo I guess I could have written something like the following in the\ncommit message of the commit that introduces set_index_file():\n\n    Introduce set_index_file() to be able to temporarily change the index file.\n\n    It should be used like:\n\n        /* Save current index file */\n        old_index_file = get_index_file();\n        set_index_file((char *)tmp_index_file);\n\n        /* Do stuff that will use tmp_index_file as the index file */\n        ...\n\n        /* When finished reset the index file */\n        set_index_file(old_index_file);\n\nMaybe I could also add a comment just before the function.\n"},{"id":"285556","messageId":"CAP8UFD2f1L3YKNBA4zP1bmGe8BQitxZp-aDsGP5UTDv06LbLBg@mail.gmail.com","threadId":"42129","inReplyTo":"alpine.DEB.2.20.1604251704300.2896@virtualbox","subject":"Re: [PATCH 80/83] run-command: make dup_devnull() non static","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-05-05T09:50:29Z","receivedAt":"2016-05-05T09:50:29Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Apr 25, 2016 at 5:05 PM, Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n> Hi Chris,\n>\n> On Sun, 24 Apr 2016, Christian Couder wrote:\n>\n>> diff --git a/run-command.c b/run-command.c\n>> index 8c7115a..29d2bda 100644\n>> --- a/run-command.c\n>> +++ b/run-command.c\n>> @@ -85,7 +85,7 @@ static inline void close_pair(int fd[2])\n>>  }\n>>\n>>  #ifndef GIT_WINDOWS_NATIVE\n>> -static inline void dup_devnull(int to)\n>> +void dup_devnull(int to)\n>>  {\n>\n> The #ifndef GIT_WINDOWS_NATIVE rings very, very loud alarm bells.\n\nYeah, but I must say that I don't know what I should do about this.\nDo you have a suggestion? Should I try to implement the same function\nfor Windows?\n"},{"id":"285565","messageId":"CAP8UFD22kGGH9W62Mzoc2h+fbtGRP5vGofAauAhdBP8R5bja_Q@mail.gmail.com","threadId":"42129","inReplyTo":"alpine.DEB.2.20.1604251701180.2896@virtualbox","subject":"Re: [PATCH 83/83] builtin/am: use apply api in run_apply()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-05-05T10:04:23Z","receivedAt":"2016-05-05T10:04:23Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi Dscho,\n\nOn Mon, Apr 25, 2016 at 5:03 PM, Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n> Hi Chris,\n>\n> On Sun, 24 Apr 2016, Christian Couder wrote:\n>\n>> [...]\n>>       /*\n>>        * If we are allowed to fall back on 3-way merge, don't give false\n>>        * errors during the initial attempt.\n>>        */\n>> +\n>>       if (state->threeway && !index_file) {\n>> -             cp.no_stdout = 1;\n>> -             cp.no_stderr = 1;\n>> +             save_stdout_fd = dup(1);\n>> +             dup_devnull(1);\n>> +             save_stderr_fd = dup(2);\n>> +             dup_devnull(2);\n>\n> I wonder. It should be possible to teach the apply function to be quiet by\n> default, yes? That would be more elegant than dup()ing back and forth.\n\nYes, it could be possible, but it could mean many changes not only in\nthe apply functions, but in possibly many other places as well. I\ndidn't check, but for example if an apply function calls a function\nfrom another part of git and this function uses error(...) in case of\nerror, I would have to change this function too.\n\nI could also introduce a hack like a global variable that would tell\nerror() to shut up, but I am not sure that would be more elegant.\n\nThanks,\nChristian.\n"},{"id":"285596","messageId":"572BA80E.3000309@kdbg.org","threadId":"42129","inReplyTo":"CAP8UFD2f1L3YKNBA4zP1bmGe8BQitxZp-aDsGP5UTDv06LbLBg@mail.gmail.com","subject":"Re: [PATCH 80/83] run-command: make dup_devnull() non static","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-05-05T20:07:42Z","receivedAt":"2016-05-05T20:07:42Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 05.05.2016 um 11:50 schrieb Christian Couder:\n> On Mon, Apr 25, 2016 at 5:05 PM, Johannes Schindelin\n> <Johannes.Schindelin@gmx.de> wrote:\n>> Hi Chris,\n>>\n>> On Sun, 24 Apr 2016, Christian Couder wrote:\n>>\n>>> diff --git a/run-command.c b/run-command.c\n>>> index 8c7115a..29d2bda 100644\n>>> --- a/run-command.c\n>>> +++ b/run-command.c\n>>> @@ -85,7 +85,7 @@ static inline void close_pair(int fd[2])\n>>>   }\n>>>\n>>>   #ifndef GIT_WINDOWS_NATIVE\n>>> -static inline void dup_devnull(int to)\n>>> +void dup_devnull(int to)\n>>>   {\n>>\n>> The #ifndef GIT_WINDOWS_NATIVE rings very, very loud alarm bells.\n>\n> Yeah, but I must say that I don't know what I should do about this.\n> Do you have a suggestion? Should I try to implement the same function\n> for Windows?\n\nNo, just remove the #ifndef brackets. There is already code in \ncompat/mingw.c that treats the file name \"/dev/null\" specially.\n\n-- Hannes\n"},{"id":"285703","messageId":"CAP8UFD35L8P2YJbbvg_-n9Nc61xMs+_2qBJwBxFzXeF-jA4WwQ@mail.gmail.com","threadId":"42129","inReplyTo":"572BA80E.3000309@kdbg.org","subject":"Re: [PATCH 80/83] run-command: make dup_devnull() non static","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-05-06T13:56:58Z","receivedAt":"2016-05-06T13:56:58Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, May 5, 2016 at 10:07 PM, Johannes Sixt <j6t@kdbg.org> wrote:\n> Am 05.05.2016 um 11:50 schrieb Christian Couder:\n>>\n>> On Mon, Apr 25, 2016 at 5:05 PM, Johannes Schindelin\n>> <Johannes.Schindelin@gmx.de> wrote:\n>>>\n>>> Hi Chris,\n>>>\n>>> On Sun, 24 Apr 2016, Christian Couder wrote:\n>>>\n>>>> diff --git a/run-command.c b/run-command.c\n>>>> index 8c7115a..29d2bda 100644\n>>>> --- a/run-command.c\n>>>> +++ b/run-command.c\n>>>> @@ -85,7 +85,7 @@ static inline void close_pair(int fd[2])\n>>>>   }\n>>>>\n>>>>   #ifndef GIT_WINDOWS_NATIVE\n>>>> -static inline void dup_devnull(int to)\n>>>> +void dup_devnull(int to)\n>>>>   {\n>>>\n>>>\n>>> The #ifndef GIT_WINDOWS_NATIVE rings very, very loud alarm bells.\n>>\n>>\n>> Yeah, but I must say that I don't know what I should do about this.\n>> Do you have a suggestion? Should I try to implement the same function\n>> for Windows?\n>\n> No, just remove the #ifndef brackets. There is already code in\n> compat/mingw.c that treats the file name \"/dev/null\" specially.\n\nOk, I will do that in the same patch though the \"#ifndef\nGIT_WINDOWS_NATIVE\" was already there before.\n\nThanks,\nChristian.\n"},{"id":"285713","messageId":"alpine.DEB.2.20.1605061733170.2963@virtualbox","threadId":"42129","inReplyTo":"CAP8UFD35L8P2YJbbvg_-n9Nc61xMs+_2qBJwBxFzXeF-jA4WwQ@mail.gmail.com","subject":"Re: [PATCH 80/83] run-command: make dup_devnull() non static","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-05-06T15:34:33Z","receivedAt":"2016-05-06T15:34:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Chris,\n\nOn Fri, 6 May 2016, Christian Couder wrote:\n\n> On Thu, May 5, 2016 at 10:07 PM, Johannes Sixt <j6t@kdbg.org> wrote:\n> > Am 05.05.2016 um 11:50 schrieb Christian Couder:\n> >>\n> >> On Mon, Apr 25, 2016 at 5:05 PM, Johannes Schindelin\n> >> <Johannes.Schindelin@gmx.de> wrote:\n> >>>\n> >>> Hi Chris,\n> >>>\n> >>> On Sun, 24 Apr 2016, Christian Couder wrote:\n> >>>\n> >>>> diff --git a/run-command.c b/run-command.c\n> >>>> index 8c7115a..29d2bda 100644\n> >>>> --- a/run-command.c\n> >>>> +++ b/run-command.c\n> >>>> @@ -85,7 +85,7 @@ static inline void close_pair(int fd[2])\n> >>>>   }\n> >>>>\n> >>>>   #ifndef GIT_WINDOWS_NATIVE\n> >>>> -static inline void dup_devnull(int to)\n> >>>> +void dup_devnull(int to)\n> >>>>   {\n> >>>\n> >>>\n> >>> The #ifndef GIT_WINDOWS_NATIVE rings very, very loud alarm bells.\n> >>\n> >>\n> >> Yeah, but I must say that I don't know what I should do about this.\n> >> Do you have a suggestion? Should I try to implement the same function\n> >> for Windows?\n\nNo, you should change the code that requires that ugly dup()ing so that it\ncan be configured to shut up.\n\n> > No, just remove the #ifndef brackets. There is already code in\n> > compat/mingw.c that treats the file name \"/dev/null\" specially.\n> \n> Ok, I will do that in the same patch though the \"#ifndef\n> GIT_WINDOWS_NATIVE\" was already there before.\n\nThe idea was that compat/mingw.c is *really* only for the MINGW version,\nnot for the MSVC version.\n\nCiao,\nDscho\n"},{"id":"285812","messageId":"CAP8UFD0ht+ozCMguGwBc19dd9ViXG4tTE0mdaOQ+JVXhSOk02Q@mail.gmail.com","threadId":"42129","inReplyTo":"alpine.DEB.2.20.1605061733170.2963@virtualbox","subject":"Re: [PATCH 80/83] run-command: make dup_devnull() non static","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-05-07T10:12:55Z","receivedAt":"2016-05-07T10:12:55Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi Dscho,\n\nOn Fri, May 6, 2016 at 5:34 PM, Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n> Hi Chris,\n>\n> On Fri, 6 May 2016, Christian Couder wrote:\n>\n>> On Thu, May 5, 2016 at 10:07 PM, Johannes Sixt <j6t@kdbg.org> wrote:\n>> > Am 05.05.2016 um 11:50 schrieb Christian Couder:\n>> >>\n>> >> On Mon, Apr 25, 2016 at 5:05 PM, Johannes Schindelin\n>> >> <Johannes.Schindelin@gmx.de> wrote:\n>> >>>\n>> >>> Hi Chris,\n>> >>>\n>> >>> On Sun, 24 Apr 2016, Christian Couder wrote:\n>> >>>\n>> >>>> diff --git a/run-command.c b/run-command.c\n>> >>>> index 8c7115a..29d2bda 100644\n>> >>>> --- a/run-command.c\n>> >>>> +++ b/run-command.c\n>> >>>> @@ -85,7 +85,7 @@ static inline void close_pair(int fd[2])\n>> >>>>   }\n>> >>>>\n>> >>>>   #ifndef GIT_WINDOWS_NATIVE\n>> >>>> -static inline void dup_devnull(int to)\n>> >>>> +void dup_devnull(int to)\n>> >>>>   {\n>> >>>\n>> >>>\n>> >>> The #ifndef GIT_WINDOWS_NATIVE rings very, very loud alarm bells.\n>> >>\n>> >>\n>> >> Yeah, but I must say that I don't know what I should do about this.\n>> >> Do you have a suggestion? Should I try to implement the same function\n>> >> for Windows?\n>\n> No, you should change the code that requires that ugly dup()ing so that it\n> can be configured to shut up.\n\nAfter taking a look, it looks like a routine that does nothing could\nbe passed to set_error_routine() and that could do part of the trick.\n\nThis part might not be too ugly, but it would anyway be more complex,\nless close to what the code is doing now and more error prone, as one\nalso need to make sure that for example no warning() or\nfprintf(stderr, ...) are called and nothing is printed on stdout.\n\nBy the way I took a look and there are 11 calls to fprintf(stderr,\n...) and 10 calls to warning() in different places in builtin/apply.c.\nThere might also be such calls in functions outside builtin/apply.c\nthat are called by the functions in builtin/apply.c.\n\nSo I'd much rather keep doing what I am doing now. If you or someone\nelse want to contribute patches on top of the series to do it in\nanother way, maybe they might be integrated at the same time by Junio,\nso that the whole thing would appear in the same release and there\nwould be no feature discrepancy between Windows and the other\nplatforms, and you wouldn't need to implement anything special for\nWindows.\n\nBut anyway, even though I don't know much about Windows, I think if\nyou have some code already in compat/mingw.c to handle redirections,\nit might be easier and safer overall to just implement the\nredirections in Windows.\n\nThanks,\nChristian.\n"},{"id":"285814","messageId":"alpine.DEB.2.20.1605071409490.2963@virtualbox","threadId":"42129","inReplyTo":"CAP8UFD0ht+ozCMguGwBc19dd9ViXG4tTE0mdaOQ+JVXhSOk02Q@mail.gmail.com","subject":"Re: [PATCH 80/83] run-command: make dup_devnull() non static","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-05-07T12:13:24Z","receivedAt":"2016-05-07T12:13:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Chris,\n\nOn Sat, 7 May 2016, Christian Couder wrote:\n\n> On Fri, May 6, 2016 at 5:34 PM, Johannes Schindelin\n> <Johannes.Schindelin@gmx.de> wrote:\n> >\n> > On Fri, 6 May 2016, Christian Couder wrote:\n> >\n> >> On Thu, May 5, 2016 at 10:07 PM, Johannes Sixt <j6t@kdbg.org> wrote:\n> >> > Am 05.05.2016 um 11:50 schrieb Christian Couder:\n> >> >>\n> >> >> On Mon, Apr 25, 2016 at 5:05 PM, Johannes Schindelin\n> >> >> <Johannes.Schindelin@gmx.de> wrote:\n> >> >>>\n> >> >>> Hi Chris,\n> >> >>>\n> >> >>> On Sun, 24 Apr 2016, Christian Couder wrote:\n> >> >>>\n> >> >>>> diff --git a/run-command.c b/run-command.c\n> >> >>>> index 8c7115a..29d2bda 100644\n> >> >>>> --- a/run-command.c\n> >> >>>> +++ b/run-command.c\n> >> >>>> @@ -85,7 +85,7 @@ static inline void close_pair(int fd[2])\n> >> >>>>   }\n> >> >>>>\n> >> >>>>   #ifndef GIT_WINDOWS_NATIVE\n> >> >>>> -static inline void dup_devnull(int to)\n> >> >>>> +void dup_devnull(int to)\n> >> >>>>   {\n> >> >>>\n> >> >>>\n> >> >>> The #ifndef GIT_WINDOWS_NATIVE rings very, very loud alarm bells.\n> >> >>\n> >> >>\n> >> >> Yeah, but I must say that I don't know what I should do about this.\n> >> >> Do you have a suggestion? Should I try to implement the same function\n> >> >> for Windows?\n> >\n> > No, you should change the code that requires that ugly dup()ing so that it\n> > can be configured to shut up.\n> \n> After taking a look, it looks like a routine that does nothing could\n> be passed to set_error_routine() and that could do part of the trick.\n> \n> This part might not be too ugly, but it would anyway be more complex,\n> less close to what the code is doing now and more error prone, as one\n> also need to make sure that for example no warning() or\n> fprintf(stderr, ...) are called and nothing is printed on stdout.\n\nI am afraid that you *have* to do that, though, if you truly want to\nlibify the code.\n\nOf course you can go with really ugly workarounds instead. Something like\na global flag that die() and error() and warning() respect. It would\nincur some technical debt, but it would make your life easier in the short\nrun.\n\nBoth the real solution and the workaround would be better than the current\nversion of the patches that dup() back and forth, just to avoid addressing\nthe real problem.\n\nCiao,\nDscho\n"},{"id":"285820","messageId":"CAP8UFD2wFZgOzmadm7X2988RyDGve92TsoMHPf2S5Ydyhc=Nyg@mail.gmail.com","threadId":"42129","inReplyTo":"alpine.DEB.2.20.1605071409490.2963@virtualbox","subject":"Re: [PATCH 80/83] run-command: make dup_devnull() non static","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-05-07T13:46:58Z","receivedAt":"2016-05-07T13:46:58Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi Dscho,\n\nOn Sat, May 7, 2016 at 2:13 PM, Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n> Hi Chris,\n>\n> On Sat, 7 May 2016, Christian Couder wrote:\n>\n>> On Fri, May 6, 2016 at 5:34 PM, Johannes Schindelin\n>> <Johannes.Schindelin@gmx.de> wrote:\n>> >\n>> > No, you should change the code that requires that ugly dup()ing so that it\n>> > can be configured to shut up.\n>>\n>> After taking a look, it looks like a routine that does nothing could\n>> be passed to set_error_routine() and that could do part of the trick.\n>>\n>> This part might not be too ugly, but it would anyway be more complex,\n>> less close to what the code is doing now and more error prone, as one\n>> also need to make sure that for example no warning() or\n>> fprintf(stderr, ...) are called and nothing is printed on stdout.\n>\n> I am afraid that you *have* to do that, though, if you truly want to\n> libify the code.\n>\n> Of course you can go with really ugly workarounds instead. Something like\n> a global flag that die() and error() and warning() respect. It would\n> incur some technical debt, but it would make your life easier in the short\n> run.\n>\n> Both the real solution and the workaround would be better than the current\n> version of the patches that dup() back and forth, just to avoid addressing\n> the real problem.\n\nThe code that is now in master in builtin/am.c does:\n\n    if (state->threeway && !index_file) {\n        cp.no_stdout = 1;\n        cp.no_stderr = 1;\n    }\n\nand in run-command.c there is already:\n\n        if (cmd->no_stdout)\n            dup_devnull(1);\n        [...]\n        if (cmd->no_stderr)\n            dup_devnull(2);\n\nfor Linux and the following for Windows:\n\n    if (cmd->no_stderr)\n        fherr = open(\"/dev/null\", O_RDWR);\n        [...]\n    if (cmd->no_stdout)\n        fhout = open(\"/dev/null\", O_RDWR);\n\nso the current code is already using dup_devnull() for Linux that you\ndon't want me to use, and it looks like there is already a simple way\nto do that on Windows.\n\nSo what's the problem? Isn't it just that you don't want a\ndup_devnull() for Windows that would be a few lines long?\n\nYou keep saying that what is done in this patch is \"ugly\" or that\nthere is a \"real problem\", but frankly I don't see why. Could you\nexplain exactly why?\nBecause the more I look at it, the more it looks to me like the\nsolution that is the simplest (even for Windows), the safest and the\nclosest to what the current code is doing.\n\nThanks,\nChristian.\n"},{"id":"285832","messageId":"alpine.DEB.2.20.1605080811380.4092@virtualbox","threadId":"42129","inReplyTo":"CAP8UFD2wFZgOzmadm7X2988RyDGve92TsoMHPf2S5Ydyhc=Nyg@mail.gmail.com","subject":"Re: [PATCH 80/83] run-command: make dup_devnull() non static","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-05-08T06:33:26Z","receivedAt":"2016-05-08T06:33:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Chris,\n\nOn Sat, 7 May 2016, Christian Couder wrote:\n\n> On Sat, May 7, 2016 at 2:13 PM, Johannes Schindelin\n> <Johannes.Schindelin@gmx.de> wrote:\n> >\n> > On Sat, 7 May 2016, Christian Couder wrote:\n> >\n> >> On Fri, May 6, 2016 at 5:34 PM, Johannes Schindelin\n> >> <Johannes.Schindelin@gmx.de> wrote:\n> >> >\n> >> > No, you should change the code that requires that ugly dup()ing so\n> >> > that it can be configured to shut up.\n> >>\n> >> After taking a look, it looks like a routine that does nothing could\n> >> be passed to set_error_routine() and that could do part of the trick.\n> >>\n> >> This part might not be too ugly, but it would anyway be more complex,\n> >> less close to what the code is doing now and more error prone, as one\n> >> also need to make sure that for example no warning() or\n> >> fprintf(stderr, ...) are called and nothing is printed on stdout.\n> >\n> > I am afraid that you *have* to do that, though, if you truly want to\n> > libify the code.\n> >\n> > Of course you can go with really ugly workarounds instead. Something\n> > like a global flag that die() and error() and warning() respect. It\n> > would incur some technical debt, but it would make your life easier in\n> > the short run.\n> >\n> > Both the real solution and the workaround would be better than the\n> > current version of the patches that dup() back and forth, just to\n> > avoid addressing the real problem.\n> \n> The code that is now in master in builtin/am.c does:\n> \n>     if (state->threeway && !index_file) {\n>         cp.no_stdout = 1;\n>         cp.no_stderr = 1;\n>     }\n> \n> and in run-command.c there is already:\n> \n>         if (cmd->no_stdout)\n>             dup_devnull(1);\n>         [...]\n>         if (cmd->no_stderr)\n>             dup_devnull(2);\n\nOf course it does that. Because there is no other way, that's why: you\ncannot change the code that is spawned by start_command().\n\n> for Linux and the following for Windows:\n> \n>     if (cmd->no_stderr)\n>         fherr = open(\"/dev/null\", O_RDWR);\n>         [...]\n>     if (cmd->no_stdout)\n>         fhout = open(\"/dev/null\", O_RDWR);\n\nAnd it is very well contained on Windows. No other callers. The code\nis limited to run_command.c.\n\n> so the current code is already using dup_devnull() for Linux that you\n> don't want me to use, and it looks like there is already a simple way\n> to do that on Windows.\n\nThe difference between the code in master and what your patches try to do\nis that in the latter case, you want to dup() just for a while, to shut up\na code path that is not only known very well, but our very own code that\nis easily changed, only to dup() it *back* in the end.\n\nThe claim is that this libifies the procedure. But it makes the code\nreally nasty for use as a library: if this is run in a thread (and you\nknow that we are going to have to do this in the near future, for\nperformance reasons), it will completely mess up all the other threads\nbecause it messes with the global file descriptors.\n\nAnd that is why it is ugly: it incurs an enormous technical debt that will\nmake code changes substantially more complicated down the road.\n\nIn essence, you save yourself a little time by sloppily dup()ing back and\nforth. At the expense of making the life much harder for the developer who\nneeds to use your code as a library function.\n\nAnd actually, hiding even fatal errors might be an ugly side effect of the\ncurrent implementation, an unfortunate implementation detail, really, not\nsomething we want to preserve when libifying the code. And actually,\ndup()ing the *caller's* stdout is not exactly preserving the current\nbehavior that dup()s the *called process'* stdout.\n\nSo yes, I find the proposed patch inelegant.\n\nIf others are okay with this, I will shut up. But I have to point out that\nit is ugly code, plain and simple, that silences an entire global file\ndescriptor, just temporarily, only to avoid a careful set of patches that\nintroduces a silent mode to the library functions that need to be called,\nwhich might even facilitate other libifying efforts.\n\nI hope you do not take this as a personal attack. It is not intended as\nsuch. It is intended to help end up with the best possible code quality.\n\nCiao,\nJohannes\n"},{"id":"285881","messageId":"CACsJy8DUcLWQ6AwyLE+CF+TxZtqDLS=82peizSf4TrZ3q=qjJg@mail.gmail.com","threadId":"42129","inReplyTo":"alpine.DEB.2.20.1605080811380.4092@virtualbox","subject":"Re: [PATCH 80/83] run-command: make dup_devnull() non static","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-05-08T10:15:53Z","receivedAt":"2016-05-08T10:15:53Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, May 8, 2016 at 1:33 PM, Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n> The claim is that this libifies the procedure. But it makes the code\n> really nasty for use as a library: if this is run in a thread (and you\n> know that we are going to have to do this in the near future, for\n> performance reasons), it will completely mess up all the other threads\n> because it messes with the global file descriptors.\n\nI vote one step at a time, leave multi-thread support for future.\nThere's a lot more shared state than file descriptors anyway, at least\nthere are object db and index access and probably a couple of hidden\nstatic variables somewhere. And I'm not sure if multi-thread really\nhelps here. Are we really CPU-bound? If object inflation causes that\n(wild guess), can we just inflate ahead in some separate process and\npass the result back?\n-- \nDuy\n"},{"id":"285921","messageId":"alpine.DEB.2.20.1605091700060.4092@virtualbox","threadId":"42129","inReplyTo":"CACsJy8DUcLWQ6AwyLE+CF+TxZtqDLS=82peizSf4TrZ3q=qjJg@mail.gmail.com","subject":"Re: [PATCH 80/83] run-command: make dup_devnull() non static","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-05-09T15:05:08Z","receivedAt":"2016-05-09T15:05:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Duy,\n\nOn Sun, 8 May 2016, Duy Nguyen wrote:\n\n> On Sun, May 8, 2016 at 1:33 PM, Johannes Schindelin\n> <Johannes.Schindelin@gmx.de> wrote:\n> > The claim is that this libifies the procedure. But it makes the code\n> > really nasty for use as a library: if this is run in a thread (and you\n> > know that we are going to have to do this in the near future, for\n> > performance reasons), it will completely mess up all the other threads\n> > because it messes with the global file descriptors.\n> \n> I vote one step at a time, leave multi-thread support for future.\n\nOh, but I never said that we have to do that now!\n\nAll I said was that using this dup() dance instead of truly libifying the\nfunctions would slam the door almost shut for future multi-threading.\nWhich is a strong hint in my book that we should *not* do that dup()\ndance, but fix our code by introducing a silent mode.\n\n> There's a lot more shared state than file descriptors anyway, at least\n> there are object db and index access and probably a couple of hidden\n> static variables somewhere.\n\nSure. And do we change those shared states temporarily in our functions?\nNo, we don't. The object db is not made temporarily inaccessible while a\ncertain function runs. The index access is not temporarily disabled while\na certain function runs.\n\nAnd this is what that dup() dance does: it disables *all* output, not only\nfrom the current thread.\n\n> And I'm not sure if multi-thread really helps here. Are we really\n> CPU-bound? If object inflation causes that (wild guess), can we just\n> inflate ahead in some separate process and pass the result back?\n\nAgain. I did *not* suggest to introduce multi-threading. I was making a\ncase for *avoiding* that ugly dup() to /dev/null and then dup() it back to\nthe original state. That would just ask for unintended side effects.\n\nCiao,\nDscho\n"},{"id":"285947","messageId":"xmqq4ma7w2nx.fsf@gitster.mtv.corp.google.com","threadId":"42129","inReplyTo":"CACsJy8DUcLWQ6AwyLE+CF+TxZtqDLS=82peizSf4TrZ3q=qjJg@mail.gmail.com","subject":"Re: [PATCH 80/83] run-command: make dup_devnull() non static","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-09T17:40:18Z","receivedAt":"2016-05-09T17:40:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> I vote one step at a time, leave multi-thread support for future.\n> There's a lot more shared state than file descriptors anyway, at least\n> there are object db and index access and probably a couple of hidden\n> static variables somewhere. And I'm not sure if multi-thread really\n> helps here. Are we really CPU-bound? If object inflation causes that\n> (wild guess), can we just inflate ahead in some separate process and\n> pass the result back?\n\nI do not particularly care about multi-thread issues, but I have to\nagree with Dscho that the updated code that claims to be \"libified\"\nthat futz with the file descriptors like the way this patch does is\nnot a proper libification.\n\nUnfortunately, the anticipated caller of this code that does \"this\nmay fail and it is OK because it is merely one of the attempts, so\nlet's not show the errors\" is not something we call only when we are\nfalling back, so \"why not do this rare codepath via the usual\nrun_command() interface to spawn 'apply' as a separate process?\",\nwhich would be the most sensible \"one step at a time\" suggestion if\nit were the case, would not apply.\n\nAs you will be passing the apply state structure throughout the\ncallchain, would it be a viable and reasonable endgame state to have\na strbuf in it that accumulates the errors?  That is, instead of\ndup()ing the standard error stream out, you would accumulate the\nerrors for a caller that asks their errors not directly sent to\nthe standard error stream, so that it can choose to either show it\nat the end, or ignore it altogether.\n\nHow far can you go with just set-error-routine?  Are there things,\nother than the file descriptors, that you need to futz with in order\nto covert that \"we'd fallback, so this early round must be silent\"\ncodepath?\n"}]}