{"thread":{"id":"10095","subject":"[PATCH 2/5] Use start_command() in git_connect() instead of explicit fork/exec.","startedAt":"2007-09-30T20:09:56Z","lastAt":"2007-10-04T20:11:58Z","messageCount":30,"participants":["Johannes Sixt","Junio C Hamano","Johannes Schindelin","David Kastrup"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"54424","messageId":"1191183001-5368-1-git-send-email-johannes.sixt@telecom.at","threadId":"10095","inReplyTo":null,"subject":"[PATCH 0/5] fork/exec removal series","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-09-30T20:09:56Z","receivedAt":"2007-09-30T20:09:56Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Here is a series of patches that removes a number fork/exec pairs.\nThey are replaced by delegating to start_command/finish_command/run_command.\nYou can regard this as the beginning of the MinGW port integration.\n\nThere still remain a few forks, which fall into these categories:\n\n- They are in tools or code that are not (yet) ported to MinGW.[*]\n\n- The fork()s are not followed by exec(). These need a different\n  implementation. I am thinking of a start_coroutine()/finish_coroutine()\n  API that is implemented with threads in MinGW. (Suggestions of a better\n  as well as implementations are welcome.)\n\n- The upload-pack case calls for an entirely different solution.\n\n[*] Just this weekend Mike Pape ported git-daemon, but I didn't find the\ntime to have it integrated in this series - if that were possible at all.\n\nPatch 2 depends on Patch 1; otherwise, there are no dependencies.\nThe series goes on top of next (it touches str_buf stuff in connect.c).\n\n builtin-archive.c    |    8 +--\n builtin-fetch-pack.c |   63 ++++++++-----------------\n cache.h              |    4 +-\n connect.c            |  126 ++++++++++++++++++++++++--------------------------\n convert.c            |   36 ++++----------\n diff.c               |   38 +--------------\n peek-remote.c        |    8 +--\n send-pack.c          |    8 +--\n transport.c          |    9 +---\n 9 files changed, 106 insertions(+), 194 deletions(-)\n\n-- Hannes\n"},{"id":"54428","messageId":"1191183001-5368-2-git-send-email-johannes.sixt@telecom.at","threadId":"10095","inReplyTo":"1191183001-5368-1-git-send-email-johannes.sixt@telecom.at","subject":"[PATCH 1/5] Change git_connect() to return a struct child_process instead of a pid_t.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-09-30T20:09:57Z","receivedAt":"2007-09-30T20:09:57Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"This prepares the API of git_connect() and finish_connect() to operate on\na struct child_process. Currently, we just use that object as a placeholder\nfor the pid that we used to return. A follow-up patch will change the\nimplementation of git_connect() and finish_connect() to make full use\nof the object.\n\nSigned-off-by: Johannes Sixt <johannes.sixt@telecom.at>\n---\n builtin-archive.c    |    8 +++-----\n builtin-fetch-pack.c |    8 +++-----\n cache.h              |    4 ++--\n connect.c            |   28 +++++++++++++++-------------\n peek-remote.c        |    8 +++-----\n send-pack.c          |    8 +++-----\n transport.c          |    9 ++-------\n 7 files changed, 31 insertions(+), 42 deletions(-)\n\ndiff --git a/builtin-archive.c b/builtin-archive.c\nindex 04385de..76db6cf 100644\n--- a/builtin-archive.c\n+++ b/builtin-archive.c\n@@ -30,7 +30,7 @@ static int run_remote_archiver(const char *remote, int argc,\n {\n \tchar *url, buf[LARGE_PACKET_MAX];\n \tint fd[2], i, len, rv;\n-\tpid_t pid;\n+\tstruct child_process *chld;\n \tconst char *exec = \"git-upload-archive\";\n \tint exec_at = 0;\n \n@@ -46,9 +46,7 @@ static int run_remote_archiver(const char *remote, int argc,\n \t}\n \n \turl = xstrdup(remote);\n-\tpid = git_connect(fd, url, exec, 0);\n-\tif (pid < 0)\n-\t\treturn pid;\n+\tchld = git_connect(fd, url, exec, 0);\n \n \tfor (i = 1; i < argc; i++) {\n \t\tif (i == exec_at)\n@@ -76,7 +74,7 @@ static int run_remote_archiver(const char *remote, int argc,\n \trv = recv_sideband(\"archive\", fd[0], 1, 2);\n \tclose(fd[0]);\n \tclose(fd[1]);\n-\trv |= finish_connect(pid);\n+\trv |= finish_connect(chld);\n \n \treturn !!rv;\n }\ndiff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c\nindex 8f25d50..19c144d 100644\n--- a/builtin-fetch-pack.c\n+++ b/builtin-fetch-pack.c\n@@ -762,7 +762,7 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,\n {\n \tint i, ret;\n \tint fd[2];\n-\tpid_t pid;\n+\tstruct child_process *chld;\n \tstruct ref *ref;\n \tstruct stat st;\n \n@@ -773,16 +773,14 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,\n \t\t\tst.st_mtime = 0;\n \t}\n \n-\tpid = git_connect(fd, (char *)dest, uploadpack,\n+\tchld = git_connect(fd, (char *)dest, uploadpack,\n                           args.verbose ? CONNECT_VERBOSE : 0);\n-\tif (pid < 0)\n-\t\treturn NULL;\n \tif (heads && nr_heads)\n \t\tnr_heads = remove_duplicates(nr_heads, heads);\n \tref = do_fetch_pack(fd, nr_heads, heads, pack_lockfile);\n \tclose(fd[0]);\n \tclose(fd[1]);\n-\tret = finish_connect(pid);\n+\tret = finish_connect(chld);\n \n \tif (!ret && nr_heads) {\n \t\t/* If the heads to pull were given, we should have\ndiff --git a/cache.h b/cache.h\nindex 27485d3..32ce8a7 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -503,8 +503,8 @@ struct ref {\n #define REF_TAGS\t(1u << 2)\n \n #define CONNECT_VERBOSE       (1u << 0)\n-extern pid_t git_connect(int fd[2], char *url, const char *prog, int flags);\n-extern int finish_connect(pid_t pid);\n+extern struct child_process *git_connect(int fd[2], char *url, const char *prog, int flags);\n+extern int finish_connect(struct child_process *chld);\n extern int path_match(const char *path, int nr, char **match);\n extern int get_ack(int fd, unsigned char *result_sha1);\n extern struct ref **get_remote_heads(int in, struct ref **list, int nr_match, char **match, unsigned int flags);\ndiff --git a/connect.c b/connect.c\nindex 3d5c4ab..f6dcab9 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -468,21 +468,22 @@ char *get_port(char *host)\n }\n \n /*\n- * This returns 0 if the transport protocol does not need fork(2),\n+ * This returns NULL if the transport protocol does not need fork(2),\n  * or a process id if it does.  Once done, finish the connection\n  * with finish_connect() with the value returned from this function\n- * (it is safe to call finish_connect() with 0 to support the former\n+ * (it is safe to call finish_connect() with NULL to support the former\n  * case).\n  *\n- * Does not return a negative value on error; it just dies.\n+ * If it returns, the connect is successful; it just dies on errors.\n  */\n-pid_t git_connect(int fd[2], char *url, const char *prog, int flags)\n+struct child_process *git_connect(int fd[2], char *url,\n+\t\t\t\t  const char *prog, int flags)\n {\n \tchar *host, *path = url;\n \tchar *end;\n \tint c;\n \tint pipefd[2][2];\n-\tpid_t pid;\n+\tstruct child_process *chld;\n \tenum protocol protocol = PROTO_LOCAL;\n \tint free_path = 0;\n \tchar *port = NULL;\n@@ -568,15 +569,16 @@ pid_t git_connect(int fd[2], char *url, const char *prog, int flags)\n \t\tfree(target_host);\n \t\tif (free_path)\n \t\t\tfree(path);\n-\t\treturn 0;\n+\t\treturn NULL;\n \t}\n \n \tif (pipe(pipefd[0]) < 0 || pipe(pipefd[1]) < 0)\n \t\tdie(\"unable to create pipe pair for communication\");\n-\tpid = fork();\n-\tif (pid < 0)\n+\tchld = xcalloc(1, sizeof(*chld));\n+\tchld->pid = fork();\n+\tif (chld->pid < 0)\n \t\tdie(\"unable to fork\");\n-\tif (!pid) {\n+\tif (!chld->pid) {\n \t\tstruct strbuf cmd;\n \n \t\tstrbuf_init(&cmd, MAX_CMD_LEN);\n@@ -625,15 +627,15 @@ pid_t git_connect(int fd[2], char *url, const char *prog, int flags)\n \tclose(pipefd[1][0]);\n \tif (free_path)\n \t\tfree(path);\n-\treturn pid;\n+\treturn chld;\n }\n \n-int finish_connect(pid_t pid)\n+int finish_connect(struct child_process *chld)\n {\n-\tif (pid == 0)\n+\tif (chld == NULL)\n \t\treturn 0;\n \n-\twhile (waitpid(pid, NULL, 0) < 0) {\n+\twhile (waitpid(chld->pid, NULL, 0) < 0) {\n \t\tif (errno != EINTR)\n \t\t\treturn -1;\n \t}\ndiff --git a/peek-remote.c b/peek-remote.c\nindex ceb7871..59cc8eb 100644\n--- a/peek-remote.c\n+++ b/peek-remote.c\n@@ -25,7 +25,7 @@ int main(int argc, char **argv)\n \tint i, ret;\n \tchar *dest = NULL;\n \tint fd[2];\n-\tpid_t pid;\n+\tstruct child_process *chld;\n \tint nongit = 0;\n \tunsigned flags = 0;\n \n@@ -64,12 +64,10 @@ int main(int argc, char **argv)\n \tif (!dest || i != argc - 1)\n \t\tusage(peek_remote_usage);\n \n-\tpid = git_connect(fd, dest, uploadpack, 0);\n-\tif (pid < 0)\n-\t\treturn 1;\n+\tchld = git_connect(fd, dest, uploadpack, 0);\n \tret = peek_remote(fd, flags);\n \tclose(fd[0]);\n \tclose(fd[1]);\n-\tret |= finish_connect(pid);\n+\tret |= finish_connect(chld);\n \treturn !!ret;\n }\ndiff --git a/send-pack.c b/send-pack.c\nindex 4533d1b..e9257fd 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -362,7 +362,7 @@ int main(int argc, char **argv)\n \tchar *dest = NULL;\n \tchar **heads = NULL;\n \tint fd[2], ret;\n-\tpid_t pid;\n+\tstruct child_process *chld;\n \tchar *remote_name = NULL;\n \tstruct remote *remote = NULL;\n \n@@ -426,12 +426,10 @@ int main(int argc, char **argv)\n \t\t}\n \t}\n \n-\tpid = git_connect(fd, dest, receivepack, verbose ? CONNECT_VERBOSE : 0);\n-\tif (pid < 0)\n-\t\treturn 1;\n+\tchld = git_connect(fd, dest, receivepack, verbose ? CONNECT_VERBOSE : 0);\n \tret = send_pack(fd[0], fd[1], remote, nr_heads, heads);\n \tclose(fd[0]);\n \tclose(fd[1]);\n-\tret |= finish_connect(pid);\n+\tret |= finish_connect(chld);\n \treturn !!ret;\n }\ndiff --git a/transport.c b/transport.c\nindex 3475cca..54ed9bc 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -279,18 +279,13 @@ static struct ref *get_refs_via_connect(const struct transport *transport)\n \tstruct git_transport_data *data = transport->data;\n \tstruct ref *refs;\n \tint fd[2];\n-\tpid_t pid;\n \tchar *dest = xstrdup(transport->url);\n-\n-\tpid = git_connect(fd, dest, data->uploadpack, 0);\n-\n-\tif (pid < 0)\n-\t\tdie(\"Failed to connect to \\\"%s\\\"\", transport->url);\n+\tstruct child_process *chld = git_connect(fd, dest, data->uploadpack, 0);\n \n \tget_remote_heads(fd[0], &refs, 0, NULL, 0);\n \tpacket_flush(fd[1]);\n \n-\tfinish_connect(pid);\n+\tfinish_connect(chld);\n \n \tfree(dest);\n \n-- \n1.5.3.3.1134.gee562\n"},{"id":"54423","messageId":"1191183001-5368-3-git-send-email-johannes.sixt@telecom.at","threadId":"10095","inReplyTo":"1191183001-5368-2-git-send-email-johannes.sixt@telecom.at","subject":"[PATCH 2/5] Use start_command() in git_connect() instead of explicit fork/exec.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-09-30T20:09:58Z","receivedAt":"2007-09-30T20:09:58Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"The child process handling is delegated to start_command() and\nfinish_command().\n\nSigned-off-by: Johannes Sixt <johannes.sixt@telecom.at>\n---\n connect.c |  104 ++++++++++++++++++++++++++++--------------------------------\n 1 files changed, 49 insertions(+), 55 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex f6dcab9..29fd431 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -482,11 +482,12 @@ struct child_process *git_connect(int fd[2], char *url,\n \tchar *host, *path = url;\n \tchar *end;\n \tint c;\n-\tint pipefd[2][2];\n \tstruct child_process *chld;\n \tenum protocol protocol = PROTO_LOCAL;\n \tint free_path = 0;\n \tchar *port = NULL;\n+\tconst char **arg;\n+\tstruct strbuf cmd;\n \n \t/* Without this we cannot rely on waitpid() to tell\n \t * what happened to our children.\n@@ -572,72 +573,65 @@ struct child_process *git_connect(int fd[2], char *url,\n \t\treturn NULL;\n \t}\n \n-\tif (pipe(pipefd[0]) < 0 || pipe(pipefd[1]) < 0)\n-\t\tdie(\"unable to create pipe pair for communication\");\n \tchld = xcalloc(1, sizeof(*chld));\n-\tchld->pid = fork();\n-\tif (chld->pid < 0)\n-\t\tdie(\"unable to fork\");\n-\tif (!chld->pid) {\n-\t\tstruct strbuf cmd;\n-\n-\t\tstrbuf_init(&cmd, MAX_CMD_LEN);\n-\t\tstrbuf_addstr(&cmd, prog);\n-\t\tstrbuf_addch(&cmd, ' ');\n-\t\tsq_quote_buf(&cmd, path);\n-\t\tif (cmd.len >= MAX_CMD_LEN)\n-\t\t\tdie(\"command line too long\");\n-\n-\t\tdup2(pipefd[1][0], 0);\n-\t\tdup2(pipefd[0][1], 1);\n-\t\tclose(pipefd[0][0]);\n-\t\tclose(pipefd[0][1]);\n-\t\tclose(pipefd[1][0]);\n-\t\tclose(pipefd[1][1]);\n-\t\tif (protocol == PROTO_SSH) {\n-\t\t\tconst char *ssh, *ssh_basename;\n-\t\t\tssh = getenv(\"GIT_SSH\");\n-\t\t\tif (!ssh) ssh = \"ssh\";\n-\t\t\tssh_basename = strrchr(ssh, '/');\n-\t\t\tif (!ssh_basename)\n-\t\t\t\tssh_basename = ssh;\n-\t\t\telse\n-\t\t\t\tssh_basename++;\n \n-\t\t\tif (!port)\n-\t\t\t\texeclp(ssh, ssh_basename, host, cmd.buf, NULL);\n-\t\t\telse\n-\t\t\t\texeclp(ssh, ssh_basename, \"-p\", port, host,\n-\t\t\t\t       cmd.buf, NULL);\n+\tstrbuf_init(&cmd, MAX_CMD_LEN);\n+\tstrbuf_addstr(&cmd, prog);\n+\tstrbuf_addch(&cmd, ' ');\n+\tsq_quote_buf(&cmd, path);\n+\tif (cmd.len >= MAX_CMD_LEN)\n+\t\tdie(\"command line too long\");\n+\n+\tchld->in = chld->out = -1;\n+\tchld->argv = arg = xcalloc(6, sizeof(*arg));\n+\tif (protocol == PROTO_SSH) {\n+\t\tconst char *ssh = getenv(\"GIT_SSH\");\n+\t\tif (!ssh) ssh = \"ssh\";\n+\n+\t\t*arg++ = ssh;\n+\t\tif (port) {\n+\t\t\t*arg++ = \"-p\";\n+\t\t\t*arg++ = port;\n \t\t}\n-\t\telse {\n-\t\t\tunsetenv(ALTERNATE_DB_ENVIRONMENT);\n-\t\t\tunsetenv(DB_ENVIRONMENT);\n-\t\t\tunsetenv(GIT_DIR_ENVIRONMENT);\n-\t\t\tunsetenv(GIT_WORK_TREE_ENVIRONMENT);\n-\t\t\tunsetenv(GRAFT_ENVIRONMENT);\n-\t\t\tunsetenv(INDEX_ENVIRONMENT);\n-\t\t\texeclp(\"sh\", \"sh\", \"-c\", cmd.buf, NULL);\n-\t\t}\n-\t\tdie(\"exec failed\");\n+\t\t*arg++ = host;\n+\t}\n+\telse {\n+\t\t/* remove these from the environment */\n+\t\tconst char *env[] = {\n+\t\t\tALTERNATE_DB_ENVIRONMENT,\n+\t\t\tDB_ENVIRONMENT,\n+\t\t\tGIT_DIR_ENVIRONMENT,\n+\t\t\tGIT_WORK_TREE_ENVIRONMENT,\n+\t\t\tGRAFT_ENVIRONMENT,\n+\t\t\tINDEX_ENVIRONMENT,\n+\t\t\tNULL\n+\t\t};\n+\t\tchld->env = env;\n+\t\t*arg++ = \"sh\";\n+\t\t*arg++ = \"-c\";\n \t}\n-\tfd[0] = pipefd[0][0];\n-\tfd[1] = pipefd[1][1];\n-\tclose(pipefd[0][1]);\n-\tclose(pipefd[1][0]);\n+\t*arg++ = cmd.buf;\n+\t*arg = NULL;\n+\n+\tif (start_command(chld))\n+\t\tdie(\"unable to fork\");\n+\n+\tfd[0] = chld->out; /* read from child's stdout */\n+\tfd[1] = chld->in;  /* write to child's stdin */\n \tif (free_path)\n \t\tfree(path);\n+\tstrbuf_release(&cmd);\n \treturn chld;\n }\n \n int finish_connect(struct child_process *chld)\n {\n+\tint code;\n \tif (chld == NULL)\n \t\treturn 0;\n \n-\twhile (waitpid(chld->pid, NULL, 0) < 0) {\n-\t\tif (errno != EINTR)\n-\t\t\treturn -1;\n-\t}\n-\treturn 0;\n+\tcode = finish_command(chld);\n+\tfree(chld->argv);\n+\tfree(chld);\n+\treturn code;\n }\n-- \n1.5.3.3.1134.gee562\n"},{"id":"54426","messageId":"1191183001-5368-4-git-send-email-johannes.sixt@telecom.at","threadId":"10095","inReplyTo":"1191183001-5368-3-git-send-email-johannes.sixt@telecom.at","subject":"[PATCH 3/5] Use start_command() to run the filter instead of explicit fork/exec.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-09-30T20:09:59Z","receivedAt":"2007-09-30T20:09:59Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>\n---\n convert.c |   36 ++++++++++--------------------------\n 1 files changed, 10 insertions(+), 26 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 0d5e909..560679d 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -196,40 +196,24 @@ static int filter_buffer(const char *path, const char *src,\n \t/*\n \t * Spawn cmd and feed the buffer contents through its stdin.\n \t */\n-\tstruct child_process child_process;\n-\tint pipe_feed[2];\n+\tstruct child_process chld;\n \tint write_err, status;\n+\tconst char *argv[] = { \"sh\", \"-c\", cmd, NULL };\n \n-\tmemset(&child_process, 0, sizeof(child_process));\n+\tmemset(&chld, 0, sizeof(chld));\n+\tchld.argv = argv;\n+\tchld.in = -1;\n \n-\tif (pipe(pipe_feed) < 0) {\n-\t\terror(\"cannot create pipe to run external filter %s\", cmd);\n-\t\treturn 1;\n-\t}\n+\tif (start_command(&chld))\n+\t\treturn error(\"cannot fork to run external filter %s\", cmd);\n \n-\tchild_process.pid = fork();\n-\tif (child_process.pid < 0) {\n-\t\terror(\"cannot fork to run external filter %s\", cmd);\n-\t\tclose(pipe_feed[0]);\n-\t\tclose(pipe_feed[1]);\n-\t\treturn 1;\n-\t}\n-\tif (!child_process.pid) {\n-\t\tdup2(pipe_feed[0], 0);\n-\t\tclose(pipe_feed[0]);\n-\t\tclose(pipe_feed[1]);\n-\t\texeclp(\"sh\", \"sh\", \"-c\", cmd, NULL);\n-\t\treturn 1;\n-\t}\n-\tclose(pipe_feed[0]);\n-\n-\twrite_err = (write_in_full(pipe_feed[1], src, size) < 0);\n-\tif (close(pipe_feed[1]))\n+\twrite_err = (write_in_full(chld.in, src, size) < 0);\n+\tif (close(chld.in))\n \t\twrite_err = 1;\n \tif (write_err)\n \t\terror(\"cannot feed the input to external filter %s\", cmd);\n \n-\tstatus = finish_command(&child_process);\n+\tstatus = finish_command(&chld);\n \tif (status)\n \t\terror(\"external filter %s failed %d\", cmd, -status);\n \treturn (write_err || status);\n-- \n1.5.3.3.1134.gee562\n"},{"id":"54425","messageId":"1191183001-5368-5-git-send-email-johannes.sixt@telecom.at","threadId":"10095","inReplyTo":"1191183001-5368-4-git-send-email-johannes.sixt@telecom.at","subject":"[PATCH 4/5] Use run_command() to spawn external diff programs instead of fork/exec.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-09-30T20:10:00Z","receivedAt":"2007-09-30T20:10:00Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>\n---\n diff.c |   38 +++-----------------------------------\n 1 files changed, 3 insertions(+), 35 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 0bd7e24..29dfa82 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -9,6 +9,7 @@\n #include \"xdiff-interface.h\"\n #include \"color.h\"\n #include \"attr.h\"\n+#include \"run-command.h\"\n \n #ifdef NO_FAST_WORKING_DIRECTORY\n #define FAST_WORKING_DIRECTORY 0\n@@ -1748,40 +1749,6 @@ static void remove_tempfile_on_signal(int signo)\n \traise(signo);\n }\n \n-static int spawn_prog(const char *pgm, const char **arg)\n-{\n-\tpid_t pid;\n-\tint status;\n-\n-\tfflush(NULL);\n-\tpid = fork();\n-\tif (pid < 0)\n-\t\tdie(\"unable to fork\");\n-\tif (!pid) {\n-\t\texecvp(pgm, (char *const*) arg);\n-\t\texit(255);\n-\t}\n-\n-\twhile (waitpid(pid, &status, 0) < 0) {\n-\t\tif (errno == EINTR)\n-\t\t\tcontinue;\n-\t\treturn -1;\n-\t}\n-\n-\t/* Earlier we did not check the exit status because\n-\t * diff exits non-zero if files are different, and\n-\t * we are not interested in knowing that.  It was a\n-\t * mistake which made it harder to quit a diff-*\n-\t * session that uses the git-apply-patch-script as\n-\t * the GIT_EXTERNAL_DIFF.  A custom GIT_EXTERNAL_DIFF\n-\t * should also exit non-zero only when it wants to\n-\t * abort the entire diff-* session.\n-\t */\n-\tif (WIFEXITED(status) && !WEXITSTATUS(status))\n-\t\treturn 0;\n-\treturn -1;\n-}\n-\n /* An external diff command takes:\n  *\n  * diff-cmd name infile1 infile1-sha1 infile1-mode \\\n@@ -1834,7 +1801,8 @@ static void run_external_diff(const char *pgm,\n \t\t*arg++ = name;\n \t}\n \t*arg = NULL;\n-\tretval = spawn_prog(pgm, spawn_arg);\n+\tfflush(NULL);\n+\tretval = run_command_v_opt(spawn_arg, 0);\n \tremove_tempfile();\n \tif (retval) {\n \t\tfprintf(stderr, \"external diff died, stopping at %s.\\n\", name);\n-- \n1.5.3.3.1134.gee562\n"},{"id":"54427","messageId":"1191183001-5368-6-git-send-email-johannes.sixt@telecom.at","threadId":"10095","inReplyTo":"1191183001-5368-5-git-send-email-johannes.sixt@telecom.at","subject":"[PATCH 5/5] Use start_comand() in builtin-fetch-pack.c instead of explicit fork/exec.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-09-30T20:10:01Z","receivedAt":"2007-09-30T20:10:01Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>\n---\n builtin-fetch-pack.c |   55 ++++++++++++++-----------------------------------\n 1 files changed, 16 insertions(+), 39 deletions(-)\n\ndiff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c\nindex 19c144d..d0eca2d 100644\n--- a/builtin-fetch-pack.c\n+++ b/builtin-fetch-pack.c\n@@ -7,6 +7,7 @@\n #include \"pack.h\"\n #include \"sideband.h\"\n #include \"fetch-pack.h\"\n+#include \"run-command.h\"\n \n static int transfer_unpack_limit = -1;\n static int fetch_unpack_limit = -1;\n@@ -499,11 +500,13 @@ static int get_pack(int xd[2], char **pack_lockfile)\n \tchar hdr_arg[256];\n \tconst char **av;\n \tint do_keep = args.keep_pack;\n-\tint keep_pipe[2];\n+\tstruct child_process cmd;\n \n \tside_pid = setup_sideband(fd, xd);\n \n-\tav = argv;\n+\tmemset(&cmd, 0, sizeof(cmd));\n+\tcmd.argv = av = argv;\n+\n \t*hdr_arg = 0;\n \tif (!args.keep_pack && unpack_limit) {\n \t\tstruct pack_header header;\n@@ -519,8 +522,8 @@ static int get_pack(int xd[2], char **pack_lockfile)\n \t}\n \n \tif (do_keep) {\n-\t\tif (pack_lockfile && pipe(keep_pipe))\n-\t\t\tdie(\"fetch-pack: pipe setup failure: %s\", strerror(errno));\n+\t\tif (pack_lockfile)\n+\t\t\tcmd.out = -1;\n \t\t*av++ = \"index-pack\";\n \t\t*av++ = \"--stdin\";\n \t\tif (!args.quiet && !args.no_progress)\n@@ -544,43 +547,17 @@ static int get_pack(int xd[2], char **pack_lockfile)\n \t\t*av++ = hdr_arg;\n \t*av++ = NULL;\n \n-\tpid = fork();\n-\tif (pid < 0)\n+\tcmd.in = fd[0];\n+\tcmd.git_cmd = 1;\n+\tif (start_command(&cmd))\n \t\tdie(\"fetch-pack: unable to fork off %s\", argv[0]);\n-\tif (!pid) {\n-\t\tdup2(fd[0], 0);\n-\t\tif (do_keep && pack_lockfile) {\n-\t\t\tdup2(keep_pipe[1], 1);\n-\t\t\tclose(keep_pipe[0]);\n-\t\t\tclose(keep_pipe[1]);\n-\t\t}\n-\t\tclose(fd[0]);\n-\t\tclose(fd[1]);\n-\t\texecv_git_cmd(argv);\n-\t\tdie(\"%s exec failed\", argv[0]);\n-\t}\n-\tclose(fd[0]);\n \tclose(fd[1]);\n-\tif (do_keep && pack_lockfile) {\n-\t\tclose(keep_pipe[1]);\n-\t\t*pack_lockfile = index_pack_lockfile(keep_pipe[0]);\n-\t\tclose(keep_pipe[0]);\n-\t}\n-\twhile (waitpid(pid, &status, 0) < 0) {\n-\t\tif (errno != EINTR)\n-\t\t\tdie(\"waiting for %s: %s\", argv[0], strerror(errno));\n-\t}\n-\tif (WIFEXITED(status)) {\n-\t\tint code = WEXITSTATUS(status);\n-\t\tif (code)\n-\t\t\tdie(\"%s died with error code %d\", argv[0], code);\n-\t\treturn 0;\n-\t}\n-\tif (WIFSIGNALED(status)) {\n-\t\tint sig = WTERMSIG(status);\n-\t\tdie(\"%s died of signal %d\", argv[0], sig);\n-\t}\n-\tdie(\"%s died of unnatural causes %d\", argv[0], status);\n+\tif (do_keep && pack_lockfile)\n+\t\t*pack_lockfile = index_pack_lockfile(cmd.out);\n+\n+\tif (finish_command(&cmd))\n+\t\tdie(\"%s failed\", argv[0]);\n+\treturn 0;\n }\n \n static struct ref *do_fetch_pack(int fd[2],\n-- \n1.5.3.3.1134.gee562\n"},{"id":"54429","messageId":"7vbqbjhpmf.fsf@gitster.siamese.dyndns.org","threadId":"10095","inReplyTo":"1191183001-5368-1-git-send-email-johannes.sixt@telecom.at","subject":"Re: [PATCH 0/5] fork/exec removal series","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-09-30T20:20:24Z","receivedAt":"2007-09-30T20:20:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <johannes.sixt@telecom.at> writes:\n\n> Here is a series of patches that removes a number fork/exec pairs.\n> They are replaced by delegating to start_command/finish_command/run_command.\n> You can regard this as the beginning of the MinGW port integration.\n\nYay!\n\n> Patch 2 depends on Patch 1; otherwise, there are no dependencies.\n> The series goes on top of next (it touches str_buf stuff in connect.c).\n\nThanks for an advance warning.\n\nI'll keep the series in my mailbox (promise) and queue it for\n'pu' for now (but only if I have time), as I intend to merge the\nstrbuf stuff to 'master' soon.\n"},{"id":"54437","messageId":"7vbqbjg9zz.fsf@gitster.siamese.dyndns.org","threadId":"10095","inReplyTo":"1191183001-5368-2-git-send-email-johannes.sixt@telecom.at","subject":"Re: [PATCH 1/5] Change git_connect() to return a struct child_process instead of a pid_t.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-09-30T20:43:12Z","receivedAt":"2007-09-30T20:43:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <johannes.sixt@telecom.at> writes:\n\n> This prepares the API of git_connect() and finish_connect() to operate on\n> a struct child_process. Currently, we just use that object as a placeholder\n> for the pid that we used to return. A follow-up patch will change the\n> implementation of git_connect() and finish_connect() to make full use\n> of the object.\n\nGood description, except removal of checks for negative return\nof the calling sites raised my eyebrow and had me spend a few\nmore minutes to review than necessary (see below).\n\n> diff --git a/builtin-archive.c b/builtin-archive.c\n> index 04385de..76db6cf 100644\n> --- a/builtin-archive.c\n> +++ b/builtin-archive.c\n> @@ -30,7 +30,7 @@ static int run_remote_archiver(const char *remote, int argc,\n>  {\n>  \tchar *url, buf[LARGE_PACKET_MAX];\n>  \tint fd[2], i, len, rv;\n> -\tpid_t pid;\n> +\tstruct child_process *chld;\n\nIs \"child\" a reserved keyword or something that we need to avoid\nits use as an identifier?\n\n>  \tconst char *exec = \"git-upload-archive\";\n>  \tint exec_at = 0;\n>  \n> @@ -46,9 +46,7 @@ static int run_remote_archiver(const char *remote, int argc,\n>  \t}\n>  \n>  \turl = xstrdup(remote);\n> -\tpid = git_connect(fd, url, exec, 0);\n> -\tif (pid < 0)\n> -\t\treturn pid;\n> +\tchld = git_connect(fd, url, exec, 0);\n>  \n\nInteresting.  This and other callers of git_connect() were\nprepared to see git_connect() to report errors with negative PID\nbut the callee simply died --- so this change is not regressing\nby removing an early error return.  It would be better to have\nsomething like this in the commit log message:\n\n\tOld code had early-return-on-error checks at the calling\n\tsites of git_connect(), but the callee simply died on\n\terrors without returning negative values.  This patch\n\tremoves such bogosity.\n\n> diff --git a/connect.c b/connect.c\n> index 3d5c4ab..f6dcab9 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -468,21 +468,22 @@ char *get_port(char *host)\n>  }\n>  \n>  /*\n> - * This returns 0 if the transport protocol does not need fork(2),\n> + * This returns NULL if the transport protocol does not need fork(2),\n>   * or a process id if it does.  Once done, finish the connection\n\nIt does not return a process ID anymore, so this comment needs\nto be updated.  Instead, you now return a struct child_process\nthat is newly allocated, and needs to be deallocated somehow.\nAt the end of finish_connect() might be a good place to do so.\n\n>   * with finish_connect() with the value returned from this function\n> - * (it is safe to call finish_connect() with 0 to support the former\n> + * (it is safe to call finish_connect() with NULL to support the former\n>   * case).\n>   *\n> - * Does not return a negative value on error; it just dies.\n> + * If it returns, the connect is successful; it just dies on errors.\n>   */\n> -pid_t git_connect(int fd[2], char *url, const char *prog, int flags)\n> +struct child_process *git_connect(int fd[2], char *url,\n> +\t\t\t\t  const char *prog, int flags)\n>  {\n> ...\n> -int finish_connect(pid_t pid)\n> +int finish_connect(struct child_process *chld)\n>  {\n> -\tif (pid == 0)\n> +\tif (chld == NULL)\n>  \t\treturn 0;\n>  \n> -\twhile (waitpid(pid, NULL, 0) < 0) {\n> +\twhile (waitpid(chld->pid, NULL, 0) < 0) {\n>  \t\tif (errno != EINTR)\n>  \t\t\treturn -1;\n>  \t}\n\nBut it seems you don't, leaking the struct.  I see this is fixed\nin the next patch in the series, but it would be nicer to have\nthe free from the very beginning.\n"},{"id":"54433","messageId":"Pine.LNX.4.64.0709302158110.28395@racer.site","threadId":"10095","inReplyTo":"1191183001-5368-2-git-send-email-johannes.sixt@telecom.at","subject":"Re: [PATCH 1/5] Change git_connect() to return a struct child_process instead of a pid_t.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-09-30T21:04:08Z","receivedAt":"2007-09-30T21:04:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 30 Sep 2007, Johannes Sixt wrote:\n\n> -\tpid_t pid;\n> +\tstruct child_process *chld;\n\nWhy not call it \"child\"?  It's only one letter more, and much less \nconfusing.\n\n> -\tpid = git_connect(fd, url, exec, 0);\n> -\tif (pid < 0)\n> -\t\treturn pid;\n> +\tchld = git_connect(fd, url, exec, 0);\n\nchld never gets free()d.  (I'm was about to suggest doing that in \nfinish_command(), but I'm not quite sure what this would break.)\n\n> @@ -773,16 +773,14 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,\n>  \t\t\tst.st_mtime = 0;\n>  \t}\n>  \n> -\tpid = git_connect(fd, (char *)dest, uploadpack,\n> +\tchld = git_connect(fd, (char *)dest, uploadpack,\n>                            args.verbose ? CONNECT_VERBOSE : 0);\n> -\tif (pid < 0)\n> -\t\treturn NULL;\n>  \tif (heads && nr_heads)\n>  \t\tnr_heads = remove_duplicates(nr_heads, heads);\n>  \tref = do_fetch_pack(fd, nr_heads, heads, pack_lockfile);\n\nIn general, I would not lightly do away with the \"return NULL\" on error, \nsince it is really hard to assess what do_fetch_pack() or packet_write() \ndo (or don't) when git_connect() failed.\n\n> -\twhile (waitpid(pid, NULL, 0) < 0) {\n> +\twhile (waitpid(chld->pid, NULL, 0) < 0) {\n\nShouldn't this be converted to finish_command() already?\n\nCiao,\nDscho\n"},{"id":"54434","messageId":"Pine.LNX.4.64.0709302206330.28395@racer.site","threadId":"10095","inReplyTo":"1191183001-5368-4-git-send-email-johannes.sixt@telecom.at","subject":"Re: [PATCH 3/5] Use start_command() to run the filter instead of explicit fork/exec.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-09-30T21:07:01Z","receivedAt":"2007-09-30T21:07:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 30 Sep 2007, Johannes Sixt wrote:\n\n> -\tstruct child_process child_process;\n> +\tstruct child_process chld;\n\nWhy?  This patch is less readable because of that rename.\n\nCiao,\nDscho\n"},{"id":"54435","messageId":"Pine.LNX.4.64.0709302210260.28395@racer.site","threadId":"10095","inReplyTo":"1191183001-5368-5-git-send-email-johannes.sixt@telecom.at","subject":"Re: [PATCH 4/5] Use run_command() to spawn external diff programs instead of fork/exec.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-09-30T21:10:41Z","receivedAt":"2007-09-30T21:10:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 30 Sep 2007, Johannes Sixt wrote:\n\n>  diff.c |   38 +++-----------------------------------\n>  1 files changed, 3 insertions(+), 35 deletions(-)\n\n_Nice_!\n\nCiao,\nDscho\n"},{"id":"54436","messageId":"Pine.LNX.4.64.0709302212160.28395@racer.site","threadId":"10095","inReplyTo":"1191183001-5368-1-git-send-email-johannes.sixt@telecom.at","subject":"Re: [PATCH 0/5] fork/exec removal series","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-09-30T21:14:38Z","receivedAt":"2007-09-30T21:14:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 30 Sep 2007, Johannes Sixt wrote:\n\n> You can regard this as the beginning of the MinGW port integration.\n\nThank you very much!  This effort cannot be praised enough.\n\n> There still remain a few forks, which fall into these categories:\n> \n> - They are in tools or code that are not (yet) ported to MinGW.[*]\n\nThe nice thing about the integration effort: It does not need to be done \nin one go.\n\n> - The fork()s are not followed by exec(). These need a different\n>   implementation. I am thinking of a start_coroutine()/finish_coroutine()\n>   API that is implemented with threads in MinGW. (Suggestions of a better\n>   as well as implementations are welcome.)\n\nIs there more than the case I introduced with shallow clones?\n\nThanks,\nDscho\n"},{"id":"54442","messageId":"200709302334.37129.johannes.sixt@telecom.at","threadId":"10095","inReplyTo":"Pine.LNX.4.64.0709302212160.28395@racer.site","subject":"Re: [PATCH 0/5] fork/exec removal series","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-09-30T21:34:36Z","receivedAt":"2007-09-30T21:34:36Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Sunday 30 September 2007 23:14, Johannes Schindelin wrote:\n> On Sun, 30 Sep 2007, Johannes Sixt wrote:\n> > - The fork()s are not followed by exec(). These need a different\n> >   implementation. I am thinking of a start_coroutine()/finish_coroutine()\n> >   API that is implemented with threads in MinGW. (Suggestions of a better\n> >   as well as implementations are welcome.)\n>\n> Is there more than the case I introduced with shallow clones?\n\nI don't know which one you are refering to.\n\nThese cases I hope to be able to treat as \"coroutine\":\n\n- sideband demultiplexer in builtin-fetch-pack.c\n- internal rev-list in upload-pack\n- the two-way pipe handling in convert.c and builtin-upload-archive.c\n\nThere are probably more in daemon.c and imap-send.c.\n\nBTW, the convert.c case (apply_filter) is most interesting for me, since I \nhave a real-world use-case for a clean-filter.\n\n-- Hannes\n"},{"id":"54444","messageId":"200709302340.17644.johannes.sixt@telecom.at","threadId":"10095","inReplyTo":"7vbqbjg9zz.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/5] Change git_connect() to return a struct child_process instead of a pid_t.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-09-30T21:40:17Z","receivedAt":"2007-09-30T21:40:17Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Sunday 30 September 2007 22:43, Junio C Hamano wrote:\n> Johannes Sixt <johannes.sixt@telecom.at> writes:\n> > +\tstruct child_process *chld;\n>\n> Is \"child\" a reserved keyword or something that we need to avoid\n> its use as an identifier?\n\nAllow me to call it \"conn\": We are using this as a \"connection object\".\n\nWill integrate your and Dscho's suggestions and resend when the strbuf series \nappears in master.\n\n-- Hannes\n"},{"id":"54446","messageId":"Pine.LNX.4.64.0709302242100.28395@racer.site","threadId":"10095","inReplyTo":"200709302334.37129.johannes.sixt@telecom.at","subject":"Re: [PATCH 0/5] fork/exec removal series","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-09-30T21:43:21Z","receivedAt":"2007-09-30T21:43:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 30 Sep 2007, Johannes Sixt wrote:\n\n> On Sunday 30 September 2007 23:14, Johannes Schindelin wrote:\n>\n> > Is there more than the case I introduced with shallow clones?\n> \n> I don't know which one you are refering to.\n\nThe rev-list one in upload-pack.\n\n> These cases I hope to be able to treat as \"coroutine\":\n> \n> - sideband demultiplexer in builtin-fetch-pack.c\n> - internal rev-list in upload-pack\n> - the two-way pipe handling in convert.c and builtin-upload-archive.c\n> \n> There are probably more in daemon.c and imap-send.c.\n> \n> BTW, the convert.c case (apply_filter) is most interesting for me, since I \n> have a real-world use-case for a clean-filter.\n\nCalling it coroutine is interesting... But yes, I agree that these three \ncases cannot be handled otherwise.\n\nCiao,\nDscho\n"},{"id":"54452","messageId":"47009CC7.70300@viscovery.net","threadId":"10095","inReplyTo":"Pine.LNX.4.64.0709302242100.28395@racer.site","subject":"Re: [PATCH 0/5] fork/exec removal series","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2007-10-01T07:07:51Z","receivedAt":"2007-10-01T07:07:51Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Johannes Schindelin schrieb:\n> On Sun, 30 Sep 2007, Johannes Sixt wrote:\n>> These cases I hope to be able to treat as \"coroutine\":\n>>\n>> - sideband demultiplexer in builtin-fetch-pack.c\n>> - internal rev-list in upload-pack\n>> - the two-way pipe handling in convert.c and builtin-upload-archive.c\n>>\n>> There are probably more in daemon.c and imap-send.c.\n>>\n>> BTW, the convert.c case (apply_filter) is most interesting for me, since I \n>> have a real-world use-case for a clean-filter.\n> \n> Calling it coroutine is interesting... But yes, I agree that these three \n> cases cannot be handled otherwise.\n\nSuggestions for a better name are appreciated!\n\n-- Hannes\n"},{"id":"54453","messageId":"4700A067.3010004@viscovery.net","threadId":"10095","inReplyTo":"7vbqbjg9zz.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/5] Change git_connect() to return a struct child_process instead of a pid_t.","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2007-10-01T07:23:19Z","receivedAt":"2007-10-01T07:23:19Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Junio C Hamano schrieb:\n> Johannes Sixt <johannes.sixt@telecom.at> writes:\n> \n>> This prepares the API of git_connect() and finish_connect() to operate on\n>> a struct child_process. Currently, we just use that object as a placeholder\n>> for the pid that we used to return. A follow-up patch will change the\n>> implementation of git_connect() and finish_connect() to make full use\n>> of the object.\n> \n> Good description, except removal of checks for negative return\n> of the calling sites raised my eyebrow and had me spend a few\n> more minutes to review than necessary (see below).\n\nI've thought about this issue a bit more.\n\nLetting git_connect() die on error unconditionally is poison for any \nlibification efforts. So here's a plan:\n\n1. Let git_connect() return a struct child_process even for the\n    non-forking case. This way a return value of NULL can uniquely\n    identify a failure.\n\n2. Keep the error checks in the callers (adjust to test for NULL).\n\n3. Change the die() calls to return failure.\n\n4. Note that the int fd[2] parameter to git_connect() is really an\n    output: Remove it and use .in and .out of the returned struct\n    child_process instead.\n\nAnd maybe:\n\n5. Reuse somehow the struct child_process that git_proxy_connect()\n    already fills in.\n\nSince my patch doesn't do (1), it can't do (2), i.e. keep the error checks - \nthey must be removed because no unique failure value exists. So I could \ncomplete (1) in a new version of this patch, in order to also do (2). What \nis your preference?\n\n-- Hannes\n\nPS: I've postponed the completion of this plan - in favor of the MinGW port \nintegration - because it only helps libification.\n"},{"id":"54459","messageId":"7vtzpbrzye.fsf@gitster.siamese.dyndns.org","threadId":"10095","inReplyTo":"4700A067.3010004@viscovery.net","subject":"Re: [PATCH 1/5] Change git_connect() to return a struct child_process instead of a pid_t.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-10-01T08:39:21Z","receivedAt":"2007-10-01T08:39:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j.sixt@viscovery.net> writes:\n\n> Junio C Hamano schrieb:\n>> Johannes Sixt <johannes.sixt@telecom.at> writes:\n>>\n>>> This prepares the API of git_connect() and finish_connect() to operate on\n>>> a struct child_process. Currently, we just use that object as a placeholder\n>>> for the pid that we used to return. A follow-up patch will change the\n>>> implementation of git_connect() and finish_connect() to make full use\n>>> of the object.\n>>\n>> Good description, except removal of checks for negative return\n>> of the calling sites raised my eyebrow and had me spend a few\n>> more minutes to review than necessary (see below).\n>\n> I've thought about this issue a bit more.\n>\n> Letting git_connect() die on error unconditionally is poison for any\n> libification efforts. So here's a plan:\n>\n> 1. Let git_connect() return a struct child_process even for the\n>    non-forking case. This way a return value of NULL can uniquely\n>    identify a failure.\n> ...\n> Since my patch doesn't do (1), it can't do (2), i.e. keep the error\n> checks -\n> they must be removed because no unique failure value exists. So I\n> could complete (1) in a new version of this patch, in order to also do\n> (2). What is your preference?\n\nIn any case, I'd rather first have one that hides fork/exec\nbehind child_process first without changing the call to die() in\ngit_connect() in this round.  I am still in \"post feature\nrelease clean-up\" mood ;-)\n\nAs to error indication, it somehow does not feel right to return\nsomething called \"child _process_\" structure when we want to\ntell the caller that there is no process to wait for in the\nno-error case, although the fact that we can use .in/.out fd in\nthe structure when we _do_ have child process is attractive.\n\nAs an alternative, we could keep the \"NULL return means there\nwas no need to fork\" semantics of git_connect(), and instead add\n\"int *status_ret\" parameter for the caller to check.\n"},{"id":"54460","messageId":"4700B8FC.70704@viscovery.net","threadId":"10095","inReplyTo":"7vtzpbrzye.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/5] Change git_connect() to return a struct child_process instead of a pid_t.","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2007-10-01T09:08:12Z","receivedAt":"2007-10-01T09:08:12Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Junio C Hamano schrieb:\n> Johannes Sixt <j.sixt@viscovery.net> writes:\n>> Letting git_connect() die on error unconditionally is poison for any\n>> libification efforts. So here's a plan:\n>>\n>> 1. Let git_connect() return a struct child_process even for the\n>>    non-forking case. This way a return value of NULL can uniquely\n>>    identify a failure.\n>> ...\n>> Since my patch doesn't do (1), it can't do (2), i.e. keep the error\n>> checks -\n>> they must be removed because no unique failure value exists. So I\n>> could complete (1) in a new version of this patch, in order to also do\n>> (2). What is your preference?\n> \n> In any case, I'd rather first have one that hides fork/exec\n> behind child_process first without changing the call to die() in\n> git_connect() in this round.  I am still in \"post feature\n> release clean-up\" mood ;-)\n\nSure: The die()s are converted in a later step.\n\nMy problem is that if I don't wrap the non-fork connections somehow in this \nfirst round, I *must* remove the error checks because there is no unique \nfailure return value anymore.\n\n> As to error indication, it somehow does not feel right to return\n> something called \"child _process_\" structure when we want to\n> tell the caller that there is no process to wait for in the\n> no-error case, although the fact that we can use .in/.out fd in\n> the structure when we _do_ have child process is attractive.\n\nDid you mean: \"even if we don't have a child process\"?\n\nHow about a typedef if you dislike the name?\n\n> As an alternative, we could keep the \"NULL return means there\n> was no need to fork\" semantics of git_connect(), and instead add\n> \"int *status_ret\" parameter for the caller to check.\n\nSeriously? Add an *out* parameter when we can get rid of one and have a \nreturn value, too?\n\n-- Hannes\n"},{"id":"54462","messageId":"86odfjb1vw.fsf@lola.quinscape.zz","threadId":"10095","inReplyTo":"47009CC7.70300@viscovery.net","subject":"Re: [PATCH 0/5] fork/exec removal series","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2007-10-01T09:49:39Z","receivedAt":"2007-10-01T09:49:39Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Johannes Sixt <j.sixt@viscovery.net> writes:\n\n> Johannes Schindelin schrieb:\n>> On Sun, 30 Sep 2007, Johannes Sixt wrote:\n>>> These cases I hope to be able to treat as \"coroutine\":\n>>>\n>>> - sideband demultiplexer in builtin-fetch-pack.c\n>>> - internal rev-list in upload-pack\n>>> - the two-way pipe handling in convert.c and builtin-upload-archive.c\n>>>\n>>> There are probably more in daemon.c and imap-send.c.\n>>>\n>>> BTW, the convert.c case (apply_filter) is most interesting for me,\n>>> since I have a real-world use-case for a clean-filter.\n>>\n>> Calling it coroutine is interesting... But yes, I agree that these\n>> three cases cannot be handled otherwise.\n>\n> Suggestions for a better name are appreciated!\n\nI think coroutine is commonly used as the name for _synchronous_\ncontext switches aka message passing.  Basically the same as\nsubroutine calls, except that the called subroutine has its own\ndynamic context (instruction pointer, call stack, control flow) that\ngets activated and suspended.\n\nIf there is parallelism implied, \"thread\" is the more appropriate\nname.\n\n-- \nDavid Kastrup\n"},{"id":"54574","messageId":"7v641pmmgs.fsf@gitster.siamese.dyndns.org","threadId":"10095","inReplyTo":"4700B8FC.70704@viscovery.net","subject":"Re: [PATCH 1/5] Change git_connect() to return a struct child_process instead of a pid_t.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-10-02T17:54:11Z","receivedAt":"2007-10-02T17:54:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j.sixt@viscovery.net> writes:\n\n> Junio C Hamano schrieb:\n> ...\n>> As to error indication, it somehow does not feel right to return\n>> something called \"child _process_\" structure when we want to\n>> tell the caller that there is no process to wait for in the\n>> no-error case, although the fact that we can use .in/.out fd in\n>> the structure when we _do_ have child process is attractive.\n>\n> Did you mean: \"even if we don't have a child process\"?\n>\n> How about a typedef if you dislike the name?\n>\n>> As an alternative, we could keep the \"NULL return means there\n>> was no need to fork\" semantics of git_connect(), and instead add\n>> \"int *status_ret\" parameter for the caller to check.\n>\n> Seriously? Add an *out* parameter when we can get rid of one and have\n> a return value, too?\n\nAh, I somehow got confused and thought that the caller decides\nnot to do the waitpid business at the end of the connection, but\nas everybody calls finish_connect() with what it got from\ngit_connect(), as long as the fake \"child_process\" structure\nrecords something to let finish_connect() know that it should\nnot waitpid() on the process, all is well.\n\nIt might make sense to teach finish_command() that a magic value\nof (cmd->pid == 0) means there is no process to wait for and\nthis \"child_process\" structure is only about the in/out stream\nto the other side.\n"},{"id":"54743","messageId":"1191442180-15905-1-git-send-email-johannes.sixt@telecom.at","threadId":"10095","inReplyTo":"200709302340.17644.johannes.sixt@telecom.at","subject":"[PATCH 0/5, resend] fork/exec removal series","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-10-03T20:09:35Z","receivedAt":"2007-10-03T20:09:35Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Here is a series of patches that removes a number fork/exec pairs.\nThey are replaced by delegating to start_command/finish_command/run_command.\n\nI've addressed all issues that were raised. However, concerning whether\ngit_connect() shoud return NULL or not, I decided to stay with my earlier\napproach to return NULL for non-forking connections. Consequently, I had to\nremove all error checks because there is now no unique value that can\nindicate failure. The motivation is that the alternate approach (to always\nreturn non-NULL for any successful connection and NULL for failure) would\nbenefit *only* libification; but *if* that ever happens, a major audit\n(and possibly) rewrite in the callers has to be done anyway because\ncurrently we are juggling, leaking, and double-closing file descriptors\nlike mad - in error paths and normal paths.\n\nPatch 2 depends on Patch 1; otherwise, there are no dependencies.\nThe series goes on top of 'master'. Note that 'next' has an additional\ncall of git_connect() in the new transport.c.\n\n builtin-archive.c |    8 +--\n cache.h           |    4 +-\n connect.c         |  128 +++++++++++++++++++++++++---------------------------\n convert.c         |   30 +++----------\n diff.c            |   38 +--------------\n fetch-pack.c      |   43 +++++------------\n peek-remote.c     |    8 +--\n send-pack.c       |    8 +--\n 8 files changed, 96 insertions(+), 171 deletions(-)\n\n-- Hannes\n"},{"id":"54744","messageId":"1191442180-15905-2-git-send-email-johannes.sixt@telecom.at","threadId":"10095","inReplyTo":"1191442180-15905-1-git-send-email-johannes.sixt@telecom.at","subject":"[PATCH 1/5] Change git_connect() to return a struct child_process instead of a pid_t.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-10-03T20:09:36Z","receivedAt":"2007-10-03T20:09:36Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"This prepares the API of git_connect() and finish_connect() to operate on\na struct child_process. Currently, we just use that object as a placeholder\nfor the pid that we used to return. A follow-up patch will change the\nimplementation of git_connect() and finish_connect() to make full use\nof the object.\n\nOld code had early-return-on-error checks at the calling sites of\ngit_connect(), but since git_connect() dies on errors anyway, these checks\nwere removed.\n\nSigned-off-by: Johannes Sixt <johannes.sixt@telecom.at>\n---\n\tI don't replace the waitpid() in finish_connect() by\n\tfinish_command() because (1) that's not the purpose of this patch\n\tand (2) there is no corresponding start_command(), yet.\n\n\t-- Hannes\n\n builtin-archive.c |    8 +++-----\n cache.h           |    4 ++--\n connect.c         |   31 +++++++++++++++++--------------\n fetch-pack.c      |    8 +++-----\n peek-remote.c     |    8 +++-----\n send-pack.c       |    8 +++-----\n 6 files changed, 31 insertions(+), 36 deletions(-)\n\ndiff --git a/builtin-archive.c b/builtin-archive.c\nindex 04385de..76f8d3d 100644\n--- a/builtin-archive.c\n+++ b/builtin-archive.c\n@@ -30,7 +30,7 @@ static int run_remote_archiver(const char *remote, int argc,\n {\n \tchar *url, buf[LARGE_PACKET_MAX];\n \tint fd[2], i, len, rv;\n-\tpid_t pid;\n+\tstruct child_process *conn;\n \tconst char *exec = \"git-upload-archive\";\n \tint exec_at = 0;\n \n@@ -46,9 +46,7 @@ static int run_remote_archiver(const char *remote, int argc,\n \t}\n \n \turl = xstrdup(remote);\n-\tpid = git_connect(fd, url, exec, 0);\n-\tif (pid < 0)\n-\t\treturn pid;\n+\tconn = git_connect(fd, url, exec, 0);\n \n \tfor (i = 1; i < argc; i++) {\n \t\tif (i == exec_at)\n@@ -76,7 +74,7 @@ static int run_remote_archiver(const char *remote, int argc,\n \trv = recv_sideband(\"archive\", fd[0], 1, 2);\n \tclose(fd[0]);\n \tclose(fd[1]);\n-\trv |= finish_connect(pid);\n+\trv |= finish_connect(conn);\n \n \treturn !!rv;\n }\ndiff --git a/cache.h b/cache.h\nindex e0abcd6..548fb52 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -502,8 +502,8 @@ struct ref {\n #define REF_TAGS\t(1u << 2)\n \n #define CONNECT_VERBOSE       (1u << 0)\n-extern pid_t git_connect(int fd[2], char *url, const char *prog, int flags);\n-extern int finish_connect(pid_t pid);\n+extern struct child_process *git_connect(int fd[2], char *url, const char *prog, int flags);\n+extern int finish_connect(struct child_process *conn);\n extern int path_match(const char *path, int nr, char **match);\n extern int get_ack(int fd, unsigned char *result_sha1);\n extern struct ref **get_remote_heads(int in, struct ref **list, int nr_match, char **match, unsigned int flags);\ndiff --git a/connect.c b/connect.c\nindex 06d279e..458a502 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -468,21 +468,22 @@ char *get_port(char *host)\n }\n \n /*\n- * This returns 0 if the transport protocol does not need fork(2),\n- * or a process id if it does.  Once done, finish the connection\n+ * This returns NULL if the transport protocol does not need fork(2), or a\n+ * struct child_process object if it does.  Once done, finish the connection\n  * with finish_connect() with the value returned from this function\n- * (it is safe to call finish_connect() with 0 to support the former\n+ * (it is safe to call finish_connect() with NULL to support the former\n  * case).\n  *\n- * Does not return a negative value on error; it just dies.\n+ * If it returns, the connect is successful; it just dies on errors.\n  */\n-pid_t git_connect(int fd[2], char *url, const char *prog, int flags)\n+struct child_process *git_connect(int fd[2], char *url,\n+\t\t\t\t  const char *prog, int flags)\n {\n \tchar *host, *path = url;\n \tchar *end;\n \tint c;\n \tint pipefd[2][2];\n-\tpid_t pid;\n+\tstruct child_process *conn;\n \tenum protocol protocol = PROTO_LOCAL;\n \tint free_path = 0;\n \tchar *port = NULL;\n@@ -568,15 +569,16 @@ pid_t git_connect(int fd[2], char *url, const char *prog, int flags)\n \t\tfree(target_host);\n \t\tif (free_path)\n \t\t\tfree(path);\n-\t\treturn 0;\n+\t\treturn NULL;\n \t}\n \n \tif (pipe(pipefd[0]) < 0 || pipe(pipefd[1]) < 0)\n \t\tdie(\"unable to create pipe pair for communication\");\n-\tpid = fork();\n-\tif (pid < 0)\n+\tconn = xcalloc(1, sizeof(*conn));\n+\tconn->pid = fork();\n+\tif (conn->pid < 0)\n \t\tdie(\"unable to fork\");\n-\tif (!pid) {\n+\tif (!conn->pid) {\n \t\tstruct strbuf cmd;\n \n \t\tstrbuf_init(&cmd, MAX_CMD_LEN);\n@@ -625,17 +627,18 @@ pid_t git_connect(int fd[2], char *url, const char *prog, int flags)\n \tclose(pipefd[1][0]);\n \tif (free_path)\n \t\tfree(path);\n-\treturn pid;\n+\treturn conn;\n }\n \n-int finish_connect(pid_t pid)\n+int finish_connect(struct child_process *conn)\n {\n-\tif (pid == 0)\n+\tif (conn == NULL)\n \t\treturn 0;\n \n-\twhile (waitpid(pid, NULL, 0) < 0) {\n+\twhile (waitpid(conn->pid, NULL, 0) < 0) {\n \t\tif (errno != EINTR)\n \t\t\treturn -1;\n \t}\n+\tfree(conn);\n \treturn 0;\n }\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 9c81305..d06b5ec 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -668,7 +668,7 @@ int main(int argc, char **argv)\n \tint i, ret, nr_heads;\n \tchar *dest = NULL, **heads;\n \tint fd[2];\n-\tpid_t pid;\n+\tstruct child_process *conn;\n \tstruct stat st;\n \n \tsetup_git_directory();\n@@ -733,15 +733,13 @@ int main(int argc, char **argv)\n \t}\n \tif (!dest)\n \t\tusage(fetch_pack_usage);\n-\tpid = git_connect(fd, dest, uploadpack, verbose ? CONNECT_VERBOSE : 0);\n-\tif (pid < 0)\n-\t\treturn 1;\n+\tconn = git_connect(fd, dest, uploadpack, verbose ? CONNECT_VERBOSE : 0);\n \tif (heads && nr_heads)\n \t\tnr_heads = remove_duplicates(nr_heads, heads);\n \tret = fetch_pack(fd, nr_heads, heads);\n \tclose(fd[0]);\n \tclose(fd[1]);\n-\tret |= finish_connect(pid);\n+\tret |= finish_connect(conn);\n \n \tif (!ret && nr_heads) {\n \t\t/* If the heads to pull were given, we should have\ndiff --git a/peek-remote.c b/peek-remote.c\nindex ceb7871..8d20f7c 100644\n--- a/peek-remote.c\n+++ b/peek-remote.c\n@@ -25,7 +25,7 @@ int main(int argc, char **argv)\n \tint i, ret;\n \tchar *dest = NULL;\n \tint fd[2];\n-\tpid_t pid;\n+\tstruct child_process *conn;\n \tint nongit = 0;\n \tunsigned flags = 0;\n \n@@ -64,12 +64,10 @@ int main(int argc, char **argv)\n \tif (!dest || i != argc - 1)\n \t\tusage(peek_remote_usage);\n \n-\tpid = git_connect(fd, dest, uploadpack, 0);\n-\tif (pid < 0)\n-\t\treturn 1;\n+\tconn = git_connect(fd, dest, uploadpack, 0);\n \tret = peek_remote(fd, flags);\n \tclose(fd[0]);\n \tclose(fd[1]);\n-\tret |= finish_connect(pid);\n+\tret |= finish_connect(conn);\n \treturn !!ret;\n }\ndiff --git a/send-pack.c b/send-pack.c\nindex f74e66a..f4c7bbf 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -362,7 +362,7 @@ int main(int argc, char **argv)\n \tchar *dest = NULL;\n \tchar **heads = NULL;\n \tint fd[2], ret;\n-\tpid_t pid;\n+\tstruct child_process *conn;\n \tchar *remote_name = NULL;\n \tstruct remote *remote = NULL;\n \n@@ -426,12 +426,10 @@ int main(int argc, char **argv)\n \t\t}\n \t}\n \n-\tpid = git_connect(fd, dest, receivepack, verbose ? CONNECT_VERBOSE : 0);\n-\tif (pid < 0)\n-\t\treturn 1;\n+\tconn = git_connect(fd, dest, receivepack, verbose ? CONNECT_VERBOSE : 0);\n \tret = send_pack(fd[0], fd[1], remote, nr_heads, heads);\n \tclose(fd[0]);\n \tclose(fd[1]);\n-\tret |= finish_connect(pid);\n+\tret |= finish_connect(conn);\n \treturn !!ret;\n }\n-- \n1.5.3.3.1134.gee562\n"},{"id":"54747","messageId":"1191442180-15905-3-git-send-email-johannes.sixt@telecom.at","threadId":"10095","inReplyTo":"1191442180-15905-2-git-send-email-johannes.sixt@telecom.at","subject":"[PATCH 2/5] Use start_command() in git_connect() instead of explicit fork/exec.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-10-03T20:09:37Z","receivedAt":"2007-10-03T20:09:37Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"The child process handling is delegated to start_command() and\nfinish_command().\n\nSigned-off-by: Johannes Sixt <johannes.sixt@telecom.at>\n---\n connect.c |  103 ++++++++++++++++++++++++++++--------------------------------\n 1 files changed, 48 insertions(+), 55 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 458a502..bb55d19 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -482,11 +482,12 @@ struct child_process *git_connect(int fd[2], char *url,\n \tchar *host, *path = url;\n \tchar *end;\n \tint c;\n-\tint pipefd[2][2];\n \tstruct child_process *conn;\n \tenum protocol protocol = PROTO_LOCAL;\n \tint free_path = 0;\n \tchar *port = NULL;\n+\tconst char **arg;\n+\tstruct strbuf cmd;\n \n \t/* Without this we cannot rely on waitpid() to tell\n \t * what happened to our children.\n@@ -572,73 +573,65 @@ struct child_process *git_connect(int fd[2], char *url,\n \t\treturn NULL;\n \t}\n \n-\tif (pipe(pipefd[0]) < 0 || pipe(pipefd[1]) < 0)\n-\t\tdie(\"unable to create pipe pair for communication\");\n \tconn = xcalloc(1, sizeof(*conn));\n-\tconn->pid = fork();\n-\tif (conn->pid < 0)\n-\t\tdie(\"unable to fork\");\n-\tif (!conn->pid) {\n-\t\tstruct strbuf cmd;\n-\n-\t\tstrbuf_init(&cmd, MAX_CMD_LEN);\n-\t\tstrbuf_addstr(&cmd, prog);\n-\t\tstrbuf_addch(&cmd, ' ');\n-\t\tsq_quote_buf(&cmd, path);\n-\t\tif (cmd.len >= MAX_CMD_LEN)\n-\t\t\tdie(\"command line too long\");\n-\n-\t\tdup2(pipefd[1][0], 0);\n-\t\tdup2(pipefd[0][1], 1);\n-\t\tclose(pipefd[0][0]);\n-\t\tclose(pipefd[0][1]);\n-\t\tclose(pipefd[1][0]);\n-\t\tclose(pipefd[1][1]);\n-\t\tif (protocol == PROTO_SSH) {\n-\t\t\tconst char *ssh, *ssh_basename;\n-\t\t\tssh = getenv(\"GIT_SSH\");\n-\t\t\tif (!ssh) ssh = \"ssh\";\n-\t\t\tssh_basename = strrchr(ssh, '/');\n-\t\t\tif (!ssh_basename)\n-\t\t\t\tssh_basename = ssh;\n-\t\t\telse\n-\t\t\t\tssh_basename++;\n \n-\t\t\tif (!port)\n-\t\t\t\texeclp(ssh, ssh_basename, host, cmd.buf, NULL);\n-\t\t\telse\n-\t\t\t\texeclp(ssh, ssh_basename, \"-p\", port, host,\n-\t\t\t\t       cmd.buf, NULL);\n+\tstrbuf_init(&cmd, MAX_CMD_LEN);\n+\tstrbuf_addstr(&cmd, prog);\n+\tstrbuf_addch(&cmd, ' ');\n+\tsq_quote_buf(&cmd, path);\n+\tif (cmd.len >= MAX_CMD_LEN)\n+\t\tdie(\"command line too long\");\n+\n+\tconn->in = conn->out = -1;\n+\tconn->argv = arg = xcalloc(6, sizeof(*arg));\n+\tif (protocol == PROTO_SSH) {\n+\t\tconst char *ssh = getenv(\"GIT_SSH\");\n+\t\tif (!ssh) ssh = \"ssh\";\n+\n+\t\t*arg++ = ssh;\n+\t\tif (port) {\n+\t\t\t*arg++ = \"-p\";\n+\t\t\t*arg++ = port;\n \t\t}\n-\t\telse {\n-\t\t\tunsetenv(ALTERNATE_DB_ENVIRONMENT);\n-\t\t\tunsetenv(DB_ENVIRONMENT);\n-\t\t\tunsetenv(GIT_DIR_ENVIRONMENT);\n-\t\t\tunsetenv(GIT_WORK_TREE_ENVIRONMENT);\n-\t\t\tunsetenv(GRAFT_ENVIRONMENT);\n-\t\t\tunsetenv(INDEX_ENVIRONMENT);\n-\t\t\texeclp(\"sh\", \"sh\", \"-c\", cmd.buf, NULL);\n-\t\t}\n-\t\tdie(\"exec failed\");\n+\t\t*arg++ = host;\n+\t}\n+\telse {\n+\t\t/* remove these from the environment */\n+\t\tconst char *env[] = {\n+\t\t\tALTERNATE_DB_ENVIRONMENT,\n+\t\t\tDB_ENVIRONMENT,\n+\t\t\tGIT_DIR_ENVIRONMENT,\n+\t\t\tGIT_WORK_TREE_ENVIRONMENT,\n+\t\t\tGRAFT_ENVIRONMENT,\n+\t\t\tINDEX_ENVIRONMENT,\n+\t\t\tNULL\n+\t\t};\n+\t\tconn->env = env;\n+\t\t*arg++ = \"sh\";\n+\t\t*arg++ = \"-c\";\n \t}\n-\tfd[0] = pipefd[0][0];\n-\tfd[1] = pipefd[1][1];\n-\tclose(pipefd[0][1]);\n-\tclose(pipefd[1][0]);\n+\t*arg++ = cmd.buf;\n+\t*arg = NULL;\n+\n+\tif (start_command(conn))\n+\t\tdie(\"unable to fork\");\n+\n+\tfd[0] = conn->out; /* read from child's stdout */\n+\tfd[1] = conn->in;  /* write to child's stdin */\n \tif (free_path)\n \t\tfree(path);\n+\tstrbuf_release(&cmd);\n \treturn conn;\n }\n \n int finish_connect(struct child_process *conn)\n {\n+\tint code;\n \tif (conn == NULL)\n \t\treturn 0;\n \n-\twhile (waitpid(conn->pid, NULL, 0) < 0) {\n-\t\tif (errno != EINTR)\n-\t\t\treturn -1;\n-\t}\n+\tcode = finish_command(conn);\n+\tfree(conn->argv);\n \tfree(conn);\n-\treturn 0;\n+\treturn code;\n }\n-- \n1.5.3.3.1134.gee562\n"},{"id":"54746","messageId":"1191442180-15905-4-git-send-email-johannes.sixt@telecom.at","threadId":"10095","inReplyTo":"1191442180-15905-3-git-send-email-johannes.sixt@telecom.at","subject":"[PATCH 3/5] Use start_command() to run the filter instead of explicit fork/exec.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-10-03T20:09:38Z","receivedAt":"2007-10-03T20:09:38Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>\n---\n convert.c |   30 +++++++-----------------------\n 1 files changed, 7 insertions(+), 23 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 0d5e909..6a8c806 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -197,34 +197,18 @@ static int filter_buffer(const char *path, const char *src,\n \t * Spawn cmd and feed the buffer contents through its stdin.\n \t */\n \tstruct child_process child_process;\n-\tint pipe_feed[2];\n \tint write_err, status;\n+\tconst char *argv[] = { \"sh\", \"-c\", cmd, NULL };\n \n \tmemset(&child_process, 0, sizeof(child_process));\n+\tchild_process.argv = argv;\n+\tchild_process.in = -1;\n \n-\tif (pipe(pipe_feed) < 0) {\n-\t\terror(\"cannot create pipe to run external filter %s\", cmd);\n-\t\treturn 1;\n-\t}\n-\n-\tchild_process.pid = fork();\n-\tif (child_process.pid < 0) {\n-\t\terror(\"cannot fork to run external filter %s\", cmd);\n-\t\tclose(pipe_feed[0]);\n-\t\tclose(pipe_feed[1]);\n-\t\treturn 1;\n-\t}\n-\tif (!child_process.pid) {\n-\t\tdup2(pipe_feed[0], 0);\n-\t\tclose(pipe_feed[0]);\n-\t\tclose(pipe_feed[1]);\n-\t\texeclp(\"sh\", \"sh\", \"-c\", cmd, NULL);\n-\t\treturn 1;\n-\t}\n-\tclose(pipe_feed[0]);\n+\tif (start_command(&child_process))\n+\t\treturn error(\"cannot fork to run external filter %s\", cmd);\n \n-\twrite_err = (write_in_full(pipe_feed[1], src, size) < 0);\n-\tif (close(pipe_feed[1]))\n+\twrite_err = (write_in_full(child_process.in, src, size) < 0);\n+\tif (close(child_process.in))\n \t\twrite_err = 1;\n \tif (write_err)\n \t\terror(\"cannot feed the input to external filter %s\", cmd);\n-- \n1.5.3.3.1134.gee562\n"},{"id":"54745","messageId":"1191442180-15905-5-git-send-email-johannes.sixt@telecom.at","threadId":"10095","inReplyTo":"1191442180-15905-4-git-send-email-johannes.sixt@telecom.at","subject":"[PATCH 4/5] Use run_command() to spawn external diff programs instead of fork/exec.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-10-03T20:09:39Z","receivedAt":"2007-10-03T20:09:39Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>\n---\n diff.c |   38 +++-----------------------------------\n 1 files changed, 3 insertions(+), 35 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 6648e01..30b7544 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -9,6 +9,7 @@\n #include \"xdiff-interface.h\"\n #include \"color.h\"\n #include \"attr.h\"\n+#include \"run-command.h\"\n \n #ifdef NO_FAST_WORKING_DIRECTORY\n #define FAST_WORKING_DIRECTORY 0\n@@ -1748,40 +1749,6 @@ static void remove_tempfile_on_signal(int signo)\n \traise(signo);\n }\n \n-static int spawn_prog(const char *pgm, const char **arg)\n-{\n-\tpid_t pid;\n-\tint status;\n-\n-\tfflush(NULL);\n-\tpid = fork();\n-\tif (pid < 0)\n-\t\tdie(\"unable to fork\");\n-\tif (!pid) {\n-\t\texecvp(pgm, (char *const*) arg);\n-\t\texit(255);\n-\t}\n-\n-\twhile (waitpid(pid, &status, 0) < 0) {\n-\t\tif (errno == EINTR)\n-\t\t\tcontinue;\n-\t\treturn -1;\n-\t}\n-\n-\t/* Earlier we did not check the exit status because\n-\t * diff exits non-zero if files are different, and\n-\t * we are not interested in knowing that.  It was a\n-\t * mistake which made it harder to quit a diff-*\n-\t * session that uses the git-apply-patch-script as\n-\t * the GIT_EXTERNAL_DIFF.  A custom GIT_EXTERNAL_DIFF\n-\t * should also exit non-zero only when it wants to\n-\t * abort the entire diff-* session.\n-\t */\n-\tif (WIFEXITED(status) && !WEXITSTATUS(status))\n-\t\treturn 0;\n-\treturn -1;\n-}\n-\n /* An external diff command takes:\n  *\n  * diff-cmd name infile1 infile1-sha1 infile1-mode \\\n@@ -1834,7 +1801,8 @@ static void run_external_diff(const char *pgm,\n \t\t*arg++ = name;\n \t}\n \t*arg = NULL;\n-\tretval = spawn_prog(pgm, spawn_arg);\n+\tfflush(NULL);\n+\tretval = run_command_v_opt(spawn_arg, 0);\n \tremove_tempfile();\n \tif (retval) {\n \t\tfprintf(stderr, \"external diff died, stopping at %s.\\n\", name);\n-- \n1.5.3.3.1134.gee562\n"},{"id":"54748","messageId":"1191442180-15905-6-git-send-email-johannes.sixt@telecom.at","threadId":"10095","inReplyTo":"1191442180-15905-5-git-send-email-johannes.sixt@telecom.at","subject":"[PATCH 5/5] Use start_comand() in builtin-fetch-pack.c instead of explicit fork/exec.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-10-03T20:09:40Z","receivedAt":"2007-10-03T20:09:40Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>\n---\n fetch-pack.c |   35 ++++++++++-------------------------\n 1 files changed, 10 insertions(+), 25 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex d06b5ec..80268e1 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -6,6 +6,7 @@\n #include \"exec_cmd.h\"\n #include \"pack.h\"\n #include \"sideband.h\"\n+#include \"run-command.h\"\n \n static int keep_pack;\n static int transfer_unpack_limit = -1;\n@@ -502,9 +503,12 @@ static int get_pack(int xd[2])\n \tchar hdr_arg[256];\n \tconst char **av;\n \tint do_keep = keep_pack;\n+\tstruct child_process cmd;\n \n \tside_pid = setup_sideband(fd, xd);\n \n+\tmemset(&cmd, 0, sizeof(cmd));\n+\tcmd.argv = argv;\n \tav = argv;\n \t*hdr_arg = 0;\n \tif (unpack_limit) {\n@@ -544,33 +548,14 @@ static int get_pack(int xd[2])\n \t\t*av++ = hdr_arg;\n \t*av++ = NULL;\n \n-\tpid = fork();\n-\tif (pid < 0)\n+\tcmd.in = fd[0];\n+\tcmd.git_cmd = 1;\n+\tif (start_command(&cmd))\n \t\tdie(\"fetch-pack: unable to fork off %s\", argv[0]);\n-\tif (!pid) {\n-\t\tdup2(fd[0], 0);\n-\t\tclose(fd[0]);\n-\t\tclose(fd[1]);\n-\t\texecv_git_cmd(argv);\n-\t\tdie(\"%s exec failed\", argv[0]);\n-\t}\n-\tclose(fd[0]);\n \tclose(fd[1]);\n-\twhile (waitpid(pid, &status, 0) < 0) {\n-\t\tif (errno != EINTR)\n-\t\t\tdie(\"waiting for %s: %s\", argv[0], strerror(errno));\n-\t}\n-\tif (WIFEXITED(status)) {\n-\t\tint code = WEXITSTATUS(status);\n-\t\tif (code)\n-\t\t\tdie(\"%s died with error code %d\", argv[0], code);\n-\t\treturn 0;\n-\t}\n-\tif (WIFSIGNALED(status)) {\n-\t\tint sig = WTERMSIG(status);\n-\t\tdie(\"%s died of signal %d\", argv[0], sig);\n-\t}\n-\tdie(\"%s died of unnatural causes %d\", argv[0], status);\n+\tif (finish_command(&cmd))\n+\t\tdie(\"%s failed\", argv[0]);\n+\treturn 0;\n }\n \n static int fetch_pack(int fd[2], int nr_match, char **match)\n-- \n1.5.3.3.1134.gee562\n"},{"id":"54804","messageId":"7v8x6jb6of.fsf@gitster.siamese.dyndns.org","threadId":"10095","inReplyTo":"1191442180-15905-6-git-send-email-johannes.sixt@telecom.at","subject":"Re: [PATCH 5/5] Use start_comand() in builtin-fetch-pack.c instead of explicit fork/exec.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-10-04T08:55:12Z","receivedAt":"2007-10-04T08:55:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <johannes.sixt@telecom.at> writes:\n\n> Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>\n> ---\n>  fetch-pack.c |   35 ++++++++++-------------------------\n>  1 files changed, 10 insertions(+), 25 deletions(-)\n>\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index d06b5ec..80268e1 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -502,9 +503,12 @@ static int get_pack(int xd[2])\n>  \tchar hdr_arg[256];\n>  \tconst char **av;\n>  \tint do_keep = keep_pack;\n> +\tstruct child_process cmd;\n>  \n>  \tside_pid = setup_sideband(fd, xd);\n>  \n> +\tmemset(&cmd, 0, sizeof(cmd));\n> +\tcmd.argv = argv;\n>  \tav = argv;\n>  \t*hdr_arg = 0;\n>  \tif (unpack_limit) {\n\nYour patch to this function makes status and pid unused\nvariables, which I've fixed up locally.\n\nThe tip of your topic is currently queued to the tip of 'pu';\nthere were quite severe merge conflicts (textual conflicts in\nbuiltin-fetch-pack.c, and adjustment to transport.c for\nsemantics change was also needed), so I ended up doing an evil\nmerge there, which I am not very happy about.\n\nI suspect the evil-merge's changes to builtin-fetch-pack to\nhandle the connection to the index-pack process may be quite\nbusted, but I ran out of time.  Please check if the result makes\nsense, Ok?\n\nI think Daniel and Shawn's git-fetch-in-C should graduate\n'master' before this series.  If you can re-send the series\nrebased on 2b5a06edca8f7237aad6464b349b79772024d2a2 (Restore\ndefault verbosity for http fetches.), it would be much\nappreciated.\n"},{"id":"54806","messageId":"4704B0EF.3040207@viscovery.net","threadId":"10095","inReplyTo":"7v8x6jb6of.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 5/5] Use start_comand() in builtin-fetch-pack.c instead of explicit fork/exec.","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2007-10-04T09:22:55Z","receivedAt":"2007-10-04T09:22:55Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Junio C Hamano schrieb:\n> I think Daniel and Shawn's git-fetch-in-C should graduate\n> 'master' before this series.  If you can re-send the series\n> rebased on 2b5a06edca8f7237aad6464b349b79772024d2a2 (Restore\n> default verbosity for http fetches.), it would be much\n> appreciated.\n\nSure, will do this evening.\n\n-- Hannes\n"},{"id":"54868","messageId":"200710042211.58363.johannes.sixt@telecom.at","threadId":"10095","inReplyTo":"4704B0EF.3040207@viscovery.net","subject":"Re: [PATCH 5/5] Use start_comand() in builtin-fetch-pack.c instead of explicit fork/exec.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-10-04T20:11:58Z","receivedAt":"2007-10-04T20:11:58Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Thursday 04 October 2007 11:22, Johannes Sixt wrote:\n> Junio C Hamano schrieb:\n> > I think Daniel and Shawn's git-fetch-in-C should graduate\n> > 'master' before this series.  If you can re-send the series\n> > rebased on 2b5a06edca8f7237aad6464b349b79772024d2a2 (Restore\n> > default verbosity for http fetches.), it would be much\n> > appreciated.\n>\n> Sure, will do this evening.\n\nI've difficulties with this rebase: 2b5a06 does not compile for me due to \n7155b727c9 (Introduce remove_dir_recursively()), which needs strbuf, but that \nis not merged into this branch. I can rebase on its parent (bundle transport: \nfix an alloc_ref() call) or 17df81ff (Merge branch 'db/fetch-pack' into \nnext). I'd prefer the latter since it also has strbuf.\n\n-- Hannes\n"}]}