{"thread":{"id":"10267","subject":"[PATCH 0/14] fork/exec removal series","startedAt":"2007-10-13T20:06:10Z","lastAt":"2007-10-14T17:14:11Z","messageCount":30,"participants":["Johannes Sixt","Johannes Schindelin","Shawn O. Pearce","Pierre Habouzit","Andreas Ericsson"],"isPatch":true,"patchVersion":1,"patchTotal":14},"messages":[{"id":"55626","messageId":"1192305984-22594-1-git-send-email-johannes.sixt@telecom.at","threadId":"10267","inReplyTo":null,"subject":"[PATCH 0/14] fork/exec removal series","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-10-13T20:06:10Z","receivedAt":"2007-10-13T20:06:10Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Junio,\n\nhere is a series of patches that removes a number fork/exec pairs.\n\nThe series consists of 2 parts:\n\n- The first half replaces a number of fork/exec pairs by start_command/\n finish_command or run_command.\n\n- The second half introduces a new framework that runs a function\n asynchronously. New functions start_async and finish_async are implemented\n similarly to start_command and run_command. They are used to replace\n occurrences of fork() that does not exec() in the child. Such code\n could in principle be run in a thread, and on MinGW port we will go this\n route, but on Posix we stay with fork().\n\nThe series can be applied on top of 2b5a06e (Restore default verbosity for\nhttp fetches), as you reqested, but that commit does not compile, so I\ndeveloped it on 90446a00 (bundle transport: fix an alloc_ref() call),\nwhich is a few commits earlier.\n\nThere will be some strbuf related merge conflicts once you merge this into\nmaster.\n\n builtin-archive.c | 8 +-\n builtin-fetch-pack.c | 101 +++++++++----------------\n cache.h | 4 +-\n connect.c | 131 ++++++++++++++++-----------------\n convert.c | 87 ++++++++--------------\n diff.c | 38 +---------\n peek-remote.c | 8 +-\n run-command.c | 79 +++++++++++++++++---\n run-command.h | 24 ++++++\n send-pack.c | 8 +-\n t/t0021-conversion.sh | 7 ++-\n transport.c | 9 +--\n upload-pack.c | 199 ++++++++++++++++++++++---------------------------\n 13 files changed, 334 insertions(+), 369 deletions(-)\n\n-- Hannes\n"},{"id":"55629","messageId":"1192305984-22594-2-git-send-email-johannes.sixt@telecom.at","threadId":"10267","inReplyTo":"1192305984-22594-1-git-send-email-johannes.sixt@telecom.at","subject":"[PATCH 01/14] 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-13T20:06:11Z","receivedAt":"2007-10-13T20:06:11Z","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 \n---\n builtin-archive.c | 8 +++-----\n builtin-fetch-pack.c | 8 +++-----\n cache.h | 4 ++--\n connect.c | 31 +++++++++++++++++--------------\n peek-remote.c | 8 +++-----\n send-pack.c | 8 +++-----\n transport.c | 9 ++-------\n 7 files changed, 33 insertions(+), 43 deletions(-)\n\ndiff --git a/builtin-archive.c b/builtin-archive.c\nindex a90c65c..346444b 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/builtin-fetch-pack.c b/builtin-fetch-pack.c\nindex 8f25d50..f56592f 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 *conn;\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+\tconn = 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(conn);\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 16bdbb2..c428f87 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 aee78ff..b156f92 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -470,21 +470,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@@ -570,15 +571,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\tchar command[MAX_CMD_LEN];\n \t\tchar *posn = command;\n \t\tint size = MAX_CMD_LEN;\n@@ -630,17 +632,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/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 4533d1b..b562953 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 }\ndiff --git a/transport.c b/transport.c\nindex 3475cca..5d723b0 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 *conn = 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(conn);\n \n \tfree(dest);\n \n-- \n1.5.3.3.1134.gee562\n"},{"id":"55627","messageId":"1192305984-22594-3-git-send-email-johannes.sixt@telecom.at","threadId":"10267","inReplyTo":"1192305984-22594-2-git-send-email-johannes.sixt@telecom.at","subject":"[PATCH 02/14] Use start_command() in git_connect() instead of explicit fork/exec.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-10-13T20:06:12Z","receivedAt":"2007-10-13T20:06:12Z","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 \n---\n connect.c | 108 ++++++++++++++++++++++++++++--------------------------------\n 1 files changed, 50 insertions(+), 58 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex b156f92..21597bf 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -484,11 +484,15 @@ 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+\tchar command[MAX_CMD_LEN];\n+\tchar *posn = command;\n+\tint size = MAX_CMD_LEN;\n+\tint of = 0;\n \n \t/* Without this we cannot rely on waitpid() to tell\n \t * what happened to our children.\n@@ -574,62 +578,51 @@ 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\tchar command[MAX_CMD_LEN];\n-\t\tchar *posn = command;\n-\t\tint size = MAX_CMD_LEN;\n-\t\tint of = 0;\n-\n-\t\tof |= add_to_string(&posn, &size, prog, 0);\n-\t\tof |= add_to_string(&posn, &size, \" \", 0);\n-\t\tof |= add_to_string(&posn, &size, path, 1);\n-\n-\t\tif (of)\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, command, NULL);\n-\t\t\telse\n-\t\t\t\texeclp(ssh, ssh_basename, \"-p\", port, host,\n-\t\t\t\t command, NULL);\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\", command, NULL);\n+\tof |= add_to_string(&posn, &size, prog, 0);\n+\tof |= add_to_string(&posn, &size, \" \", 0);\n+\tof |= add_to_string(&posn, &size, path, 1);\n+\n+\tif (of)\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\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++ = command;\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 \treturn conn;\n@@ -637,13 +630,12 @@ struct child_process *git_connect(int fd[2], char *url,\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":"55628","messageId":"1192305984-22594-4-git-send-email-johannes.sixt@telecom.at","threadId":"10267","inReplyTo":"1192305984-22594-3-git-send-email-johannes.sixt@telecom.at","subject":"[PATCH 03/14] Use start_command() to run content filters instead of explicit fork/exec.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-10-13T20:06:13Z","receivedAt":"2007-10-13T20:06:13Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"The previous code already used finish_command() to wait for the process\nto terminate, but did not use start_command() to run it.\n\nSigned-off-by: Johannes Sixt \n---\n convert.c | 30 +++++++-----------------------\n 1 files changed, 7 insertions(+), 23 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex d77c8eb..6d64994 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -208,34 +208,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":"55630","messageId":"1192305984-22594-5-git-send-email-johannes.sixt@telecom.at","threadId":"10267","inReplyTo":"1192305984-22594-4-git-send-email-johannes.sixt@telecom.at","subject":"[PATCH 04/14] Use run_command() to spawn external diff programs instead of fork/exec.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-10-13T20:06:14Z","receivedAt":"2007-10-13T20:06:14Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Signed-off-by: Johannes Sixt \n---\n diff.c | 38 +++-----------------------------------\n 1 files changed, 3 insertions(+), 35 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 0ee9ea1..d03dc6a 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@@ -1791,40 +1792,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@@ -1877,7 +1844,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":"55631","messageId":"1192305984-22594-6-git-send-email-johannes.sixt@telecom.at","threadId":"10267","inReplyTo":"1192305984-22594-5-git-send-email-johannes.sixt@telecom.at","subject":"[PATCH 05/14] Use start_comand() in builtin-fetch-pack.c instead of explicit fork/exec.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-10-13T20:06:15Z","receivedAt":"2007-10-13T20:06:15Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Signed-off-by: Johannes Sixt \n---\n builtin-fetch-pack.c | 56 ++++++++++++++-----------------------------------\n 1 files changed, 16 insertions(+), 40 deletions(-)\n\ndiff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c\nindex f56592f..871b704 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@@ -491,18 +492,19 @@ static pid_t setup_sideband(int fd[2], int xd[2])\n \n static int get_pack(int xd[2], char **pack_lockfile)\n {\n-\tint status;\n-\tpid_t pid, side_pid;\n+\tpid_t side_pid;\n \tint fd[2];\n \tconst char *argv[20];\n \tchar keep_arg[256];\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+\tmemset(&cmd, 0, sizeof(cmd));\n+\tcmd.argv = argv;\n \tav = argv;\n \t*hdr_arg = 0;\n \tif (!args.keep_pack && unpack_limit) {\n@@ -519,8 +521,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 +546,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":"55638","messageId":"1192305984-22594-7-git-send-email-johannes.sixt@telecom.at","threadId":"10267","inReplyTo":"1192305984-22594-6-git-send-email-johannes.sixt@telecom.at","subject":"[PATCH 06/14] Have start_command() create a pipe to read the stderr of the child.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-10-13T20:06:16Z","receivedAt":"2007-10-13T20:06:16Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"This adds another stanza that allocates a pipe that is connected to the\nchild's stderr and that the caller can read from. In order to request this\npipe, the caller sets cmd->err to -1.\n\nThe implementation is not exactly modeled after the stdout case: For stdout\nthe caller can supply an existing file descriptor, but this facility is\nnowhere needed in the stderr case. Additionally, the caller is required to\nclose cmd->err.\n\nSigned-off-by: Johannes Sixt \n---\n run-command.c | 26 ++++++++++++++++++++++++--\n run-command.h | 1 +\n 2 files changed, 25 insertions(+), 2 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 7e779d3..d00c03b 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -17,8 +17,8 @@ static inline void dup_devnull(int to)\n \n int start_command(struct child_process *cmd)\n {\n-\tint need_in, need_out;\n-\tint fdin[2], fdout[2];\n+\tint need_in, need_out, need_err;\n+\tint fdin[2], fdout[2], fderr[2];\n \n \tneed_in = !cmd->no_stdin && cmd->in < 0;\n \tif (need_in) {\n@@ -41,12 +41,26 @@ int start_command(struct child_process *cmd)\n \t\tcmd->close_out = 1;\n \t}\n \n+\tneed_err = cmd->err < 0;\n+\tif (need_err) {\n+\t\tif (pipe(fderr) < 0) {\n+\t\t\tif (need_in)\n+\t\t\t\tclose_pair(fdin);\n+\t\t\tif (need_out)\n+\t\t\t\tclose_pair(fdout);\n+\t\t\treturn -ERR_RUN_COMMAND_PIPE;\n+\t\t}\n+\t\tcmd->err = fderr[0];\n+\t}\n+\n \tcmd->pid = fork();\n \tif (cmd->pid < 0) {\n \t\tif (need_in)\n \t\t\tclose_pair(fdin);\n \t\tif (need_out)\n \t\t\tclose_pair(fdout);\n+\t\tif (need_err)\n+\t\t\tclose_pair(fderr);\n \t\treturn -ERR_RUN_COMMAND_FORK;\n \t}\n \n@@ -73,6 +87,11 @@ int start_command(struct child_process *cmd)\n \t\t\tclose(cmd->out);\n \t\t}\n \n+\t\tif (need_err) {\n+\t\t\tdup2(fderr[1], 2);\n+\t\t\tclose_pair(fderr);\n+\t\t}\n+\n \t\tif (cmd->dir && chdir(cmd->dir))\n \t\t\tdie(\"exec %s: cd to %s failed (%s)\", cmd->argv[0],\n \t\t\t cmd->dir, strerror(errno));\n@@ -102,6 +121,9 @@ int start_command(struct child_process *cmd)\n \telse if (cmd->out > 1)\n \t\tclose(cmd->out);\n \n+\tif (need_err)\n+\t\tclose(fderr[1]);\n+\n \treturn 0;\n }\n \ndiff --git a/run-command.h b/run-command.h\nindex 7958eb1..35b9fb6 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -16,6 +16,7 @@ struct child_process {\n \tpid_t pid;\n \tint in;\n \tint out;\n+\tint err;\n \tconst char *dir;\n \tconst char *const *env;\n \tunsigned close_in:1;\n-- \n1.5.3.3.1134.gee562\n"},{"id":"55635","messageId":"1192305984-22594-8-git-send-email-johannes.sixt@telecom.at","threadId":"10267","inReplyTo":"1192305984-22594-7-git-send-email-johannes.sixt@telecom.at","subject":"[PATCH 07/14] upload-pack: Use start_command() to run pack-objects in create_pack_file().","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-10-13T20:06:17Z","receivedAt":"2007-10-13T20:06:17Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"This gets rid of an explicit fork/exec.\n\nSince upload-pack has to coordinate two processes (rev-list and\npack-objects), we cannot use the normal finish_command(), but have to\nmonitor the processes explicitly. Hence, the waitpid() call remains.\n\nSigned-off-by: Johannes Sixt \n---\n upload-pack.c | 105 ++++++++++++++++++++++++---------------------------------\n 1 files changed, 44 insertions(+), 61 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex fe96ef1..ef2894a 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -9,6 +9,7 @@\n #include \"diff.h\"\n #include \"revision.h\"\n #include \"list-objects.h\"\n+#include \"run-command.h\"\n \n static const char upload_pack_usage[] = \"git-upload-pack [--strict] [--timeout=nn] \";\n \n@@ -98,16 +99,18 @@ static void show_edge(struct commit *commit)\n \n static void create_pack_file(void)\n {\n-\t/* Pipes between rev-list to pack-objects, pack-objects to us\n-\t * and pack-objects error stream for progress bar.\n+\t/* Pipe from rev-list to pack-objects\n \t */\n-\tint lp_pipe[2], pu_pipe[2], pe_pipe[2];\n-\tpid_t pid_rev_list, pid_pack_objects;\n+\tint lp_pipe[2];\n+\tpid_t pid_rev_list;\n+\tstruct child_process pack_objects;\n \tint create_full_pack = (nr_our_refs == want_obj.nr && !have_obj.nr);\n \tchar data[8193], progress[128];\n \tchar abort_msg[] = \"aborting due to possible repository \"\n \t\t\"corruption on the remote side.\";\n \tint buffered = -1;\n+\tconst char *argv[10];\n+\tint arg = 0;\n \n \tif (pipe(lp_pipe) < 0)\n \t\tdie(\"git-upload-pack: unable to create pipe\");\n@@ -154,52 +157,32 @@ static void create_pack_file(void)\n \t\texit(0);\n \t}\n \n-\tif (pipe(pu_pipe) < 0)\n-\t\tdie(\"git-upload-pack: unable to create pipe\");\n-\tif (pipe(pe_pipe) < 0)\n-\t\tdie(\"git-upload-pack: unable to create pipe\");\n-\tpid_pack_objects = fork();\n-\tif (pid_pack_objects < 0) {\n+\t/* close this so that it is not inherited by pack_objects */\n+\tclose(lp_pipe[1]);\n+\n+\targv[arg++] = \"pack-objects\";\n+\targv[arg++] = \"--stdout\";\n+\tif (!no_progress)\n+\t\targv[arg++] = \"--progress\";\n+\tif (use_ofs_delta)\n+\t\targv[arg++] = \"--delta-base-offset\";\n+\targv[arg++] = NULL;\n+\n+\tmemset(&pack_objects, 0, sizeof(pack_objects));\n+\tpack_objects.in = lp_pipe[0];\t/* start_command closes it */\n+\tpack_objects.out = -1;\n+\tpack_objects.err = -1;\n+\tpack_objects.git_cmd = 1;\n+\tpack_objects.argv = argv;\n+\tif (start_command(&pack_objects)) {\n \t\t/* daemon sets things up to ignore TERM */\n \t\tkill(pid_rev_list, SIGKILL);\n \t\tdie(\"git-upload-pack: unable to fork git-pack-objects\");\n \t}\n-\tif (!pid_pack_objects) {\n-\t\tconst char *argv[10];\n-\t\tint i = 0;\n-\n-\t\tdup2(lp_pipe[0], 0);\n-\t\tdup2(pu_pipe[1], 1);\n-\t\tdup2(pe_pipe[1], 2);\n-\n-\t\tclose(lp_pipe[0]);\n-\t\tclose(lp_pipe[1]);\n-\t\tclose(pu_pipe[0]);\n-\t\tclose(pu_pipe[1]);\n-\t\tclose(pe_pipe[0]);\n-\t\tclose(pe_pipe[1]);\n-\n-\t\targv[i++] = \"pack-objects\";\n-\t\targv[i++] = \"--stdout\";\n-\t\tif (!no_progress)\n-\t\t\targv[i++] = \"--progress\";\n-\t\tif (use_ofs_delta)\n-\t\t\targv[i++] = \"--delta-base-offset\";\n-\t\targv[i++] = NULL;\n-\n-\t\texecv_git_cmd(argv);\n-\t\tkill(pid_rev_list, SIGKILL);\n-\t\tdie(\"git-upload-pack: unable to exec git-pack-objects\");\n-\t}\n-\n-\tclose(lp_pipe[0]);\n-\tclose(lp_pipe[1]);\n \n-\t/* We read from pe_pipe[0] to capture stderr output for\n-\t * progress bar, and pu_pipe[0] to capture the pack data.\n+\t/* We read from pack_objects.err to capture stderr output for\n+\t * progress bar, and pack_objects.out to capture the pack data.\n \t */\n-\tclose(pe_pipe[1]);\n-\tclose(pu_pipe[1]);\n \n \twhile (1) {\n \t\tconst char *who;\n@@ -214,14 +197,14 @@ static void create_pack_file(void)\n \t\tpollsize = 0;\n \t\tpe = pu = -1;\n \n-\t\tif (0 <= pu_pipe[0]) {\n-\t\t\tpfd[pollsize].fd = pu_pipe[0];\n+\t\tif (0 <= pack_objects.out) {\n+\t\t\tpfd[pollsize].fd = pack_objects.out;\n \t\t\tpfd[pollsize].events = POLLIN;\n \t\t\tpu = pollsize;\n \t\t\tpollsize++;\n \t\t}\n-\t\tif (0 <= pe_pipe[0]) {\n-\t\t\tpfd[pollsize].fd = pe_pipe[0];\n+\t\tif (0 <= pack_objects.err) {\n+\t\t\tpfd[pollsize].fd = pack_objects.err;\n \t\t\tpfd[pollsize].events = POLLIN;\n \t\t\tpe = pollsize;\n \t\t\tpollsize++;\n@@ -254,13 +237,13 @@ static void create_pack_file(void)\n \t\t\t\t\t*cp++ = buffered;\n \t\t\t\t\toutsz++;\n \t\t\t\t}\n-\t\t\t\tsz = xread(pu_pipe[0], cp,\n+\t\t\t\tsz = xread(pack_objects.out, cp,\n \t\t\t\t\t sizeof(data) - outsz);\n \t\t\t\tif (0 < sz)\n \t\t\t\t\t\t;\n \t\t\t\telse if (sz == 0) {\n-\t\t\t\t\tclose(pu_pipe[0]);\n-\t\t\t\t\tpu_pipe[0] = -1;\n+\t\t\t\t\tclose(pack_objects.out);\n+\t\t\t\t\tpack_objects.out = -1;\n \t\t\t\t}\n \t\t\t\telse\n \t\t\t\t\tgoto fail;\n@@ -279,13 +262,13 @@ static void create_pack_file(void)\n \t\t\t\t/* Status ready; we ship that in the side-band\n \t\t\t\t * or dump to the standard error.\n \t\t\t\t */\n-\t\t\t\tsz = xread(pe_pipe[0], progress,\n+\t\t\t\tsz = xread(pack_objects.err, progress,\n \t\t\t\t\t sizeof(progress));\n \t\t\t\tif (0 < sz)\n \t\t\t\t\tsend_client_data(2, progress, sz);\n \t\t\t\telse if (sz == 0) {\n-\t\t\t\t\tclose(pe_pipe[0]);\n-\t\t\t\t\tpe_pipe[0] = -1;\n+\t\t\t\t\tclose(pack_objects.err);\n+\t\t\t\t\tpack_objects.err = -1;\n \t\t\t\t}\n \t\t\t\telse\n \t\t\t\t\tgoto fail;\n@@ -293,12 +276,12 @@ static void create_pack_file(void)\n \t\t}\n \n \t\t/* See if the children are still there */\n-\t\tif (pid_rev_list || pid_pack_objects) {\n+\t\tif (pid_rev_list || pack_objects.pid) {\n \t\t\tpid = waitpid(-1, &status, WNOHANG);\n \t\t\tif (!pid)\n \t\t\t\tcontinue;\n \t\t\twho = ((pid == pid_rev_list) ? \"git-rev-list\" :\n-\t\t\t (pid == pid_pack_objects) ? \"git-pack-objects\" :\n+\t\t\t (pid == pack_objects.pid) ? \"git-pack-objects\" :\n \t\t\t NULL);\n \t\t\tif (!who) {\n \t\t\t\tif (pid < 0) {\n@@ -317,9 +300,9 @@ static void create_pack_file(void)\n \t\t\t}\n \t\t\tif (pid == pid_rev_list)\n \t\t\t\tpid_rev_list = 0;\n-\t\t\tif (pid == pid_pack_objects)\n-\t\t\t\tpid_pack_objects = 0;\n-\t\t\tif (pid_rev_list || pid_pack_objects)\n+\t\t\tif (pid == pack_objects.pid)\n+\t\t\t\tpack_objects.pid = 0;\n+\t\t\tif (pid_rev_list || pack_objects.pid)\n \t\t\t\tcontinue;\n \t\t}\n \n@@ -340,8 +323,8 @@ static void create_pack_file(void)\n \t\treturn;\n \t}\n fail:\n-\tif (pid_pack_objects)\n-\t\tkill(pid_pack_objects, SIGKILL);\n+\tif (pack_objects.pid)\n+\t\tkill(pack_objects.pid, SIGKILL);\n \tif (pid_rev_list)\n \t\tkill(pid_rev_list, SIGKILL);\n \tsend_client_data(3, abort_msg, sizeof(abort_msg));\n-- \n1.5.3.3.1134.gee562\n"},{"id":"55639","messageId":"1192305984-22594-9-git-send-email-johannes.sixt@telecom.at","threadId":"10267","inReplyTo":"1192305984-22594-8-git-send-email-johannes.sixt@telecom.at","subject":"[PATCH 08/14] Add infrastructure to run a function asynchronously.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-10-13T20:06:18Z","receivedAt":"2007-10-13T20:06:18Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"This adds start_async() and finish_async(), which runs a function\nasynchronously. Communication with the caller happens only via pipes.\nFor this reason, this implementation forks off a child process that runs\nthe function.\n\nSigned-off-by: Johannes Sixt \n---\n run-command.c | 53 +++++++++++++++++++++++++++++++++++++++++++++--------\n run-command.h | 23 +++++++++++++++++++++++\n 2 files changed, 68 insertions(+), 8 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex d00c03b..111d584 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -127,16 +127,11 @@ int start_command(struct child_process *cmd)\n \treturn 0;\n }\n \n-int finish_command(struct child_process *cmd)\n+static int wait_or_whine(pid_t pid)\n {\n-\tif (cmd->close_in)\n-\t\tclose(cmd->in);\n-\tif (cmd->close_out)\n-\t\tclose(cmd->out);\n-\n \tfor (;;) {\n \t\tint status, code;\n-\t\tpid_t waiting = waitpid(cmd->pid, &status, 0);\n+\t\tpid_t waiting = waitpid(pid, &status, 0);\n \n \t\tif (waiting < 0) {\n \t\t\tif (errno == EINTR)\n@@ -144,7 +139,7 @@ int finish_command(struct child_process *cmd)\n \t\t\terror(\"waitpid failed (%s)\", strerror(errno));\n \t\t\treturn -ERR_RUN_COMMAND_WAITPID;\n \t\t}\n-\t\tif (waiting != cmd->pid)\n+\t\tif (waiting != pid)\n \t\t\treturn -ERR_RUN_COMMAND_WAITPID_WRONG_PID;\n \t\tif (WIFSIGNALED(status))\n \t\t\treturn -ERR_RUN_COMMAND_WAITPID_SIGNAL;\n@@ -158,6 +153,15 @@ int finish_command(struct child_process *cmd)\n \t}\n }\n \n+int finish_command(struct child_process *cmd)\n+{\n+\tif (cmd->close_in)\n+\t\tclose(cmd->in);\n+\tif (cmd->close_out)\n+\t\tclose(cmd->out);\n+\treturn wait_or_whine(cmd->pid);\n+}\n+\n int run_command(struct child_process *cmd)\n {\n \tint code = start_command(cmd);\n@@ -200,3 +204,36 @@ int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const\n \tcmd.env = env;\n \treturn run_command(&cmd);\n }\n+\n+int start_async(struct async *async)\n+{\n+\tint pipe_out[2];\n+\n+\tif (pipe(pipe_out) < 0) {\n+\t\treturn error(\"cannot create pipe: %s\", strerror(errno));\n+\t}\n+\n+\tasync->pid = fork();\n+\tif (async->pid < 0) {\n+\t\terror(\"fork (async) failed: %s\", strerror(errno));\n+\t\tclose(pipe_out[0]);\n+\t\tclose(pipe_out[1]);\n+\t\treturn -1;\n+\t}\n+\tif (!async->pid) {\n+\t\tclose(pipe_out[0]);\n+\t\texit(!!async->proc(pipe_out[1], async->data));\n+\t}\n+\tasync->out = pipe_out[0];\n+\tclose(pipe_out[1]);\n+\treturn 0;\n+}\n+\n+int finish_async(struct async *async)\n+{\n+\tint ret = 0;\n+\n+\tif (wait_or_whine(async->pid))\n+\t\tret = error(\"waitpid (async) failed\");\n+\treturn ret;\n+}\ndiff --git a/run-command.h b/run-command.h\nindex 35b9fb6..c5fd2fc 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -43,4 +43,27 @@ int run_command_v_opt_cd(const char **argv, int opt, const char *dir);\n */\n int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const char *const *env);\n \n+/*\n+ * The purpose of the following functions is to feed a pipe by running\n+ * a function asynchronously and providing output that the call reads\n+ * in a different pipe.\n+ *\n+ * It is expected that no synchronization and mutual exclusion between\n+ * the caller and the feed function is necessary so that the function\n+ * can run in a thread without interfering with the caller.\n+ */\n+struct async {\n+\t/*\n+\t * proc writes to fd and closes it;\n+\t * returns 0 on success, non-zero on failure\n+\t */\n+\tint (*proc)(int fd, void *data);\n+\tvoid *data;\n+\tint out;\t/* caller reads from this */\n+\tpid_t pid;\n+};\n+\n+int start_async(struct async *async);\n+int finish_async(struct async *async);\n+\n #endif\n-- \n1.5.3.3.1134.gee562\n"},{"id":"55637","messageId":"1192305984-22594-10-git-send-email-johannes.sixt@telecom.at","threadId":"10267","inReplyTo":"1192305984-22594-9-git-send-email-johannes.sixt@telecom.at","subject":"[PATCH 09/14] Use the asyncronous function infrastructure in builtin-fetch-pack.c.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-10-13T20:06:19Z","receivedAt":"2007-10-13T20:06:19Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"We run the sideband demultiplexer in an asynchronous function.\n\nNote that earlier there was a check in the child process that closed\nxd[1] only if it was different from xd[0]; this test is no longer needed\nbecause git_connect() always returns two different file descriptors\n(see ec587fde0a76780931c7ac32474c8c000aa45134).\n\nSigned-off-by: Johannes Sixt \n---\n builtin-fetch-pack.c | 39 ++++++++++++++++++---------------------\n 1 files changed, 18 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c\nindex 871b704..51d8a32 100644\n--- a/builtin-fetch-pack.c\n+++ b/builtin-fetch-pack.c\n@@ -457,42 +457,37 @@ static int everything_local(struct ref **refs, int nr_match, char **match)\n \treturn retval;\n }\n \n-static pid_t setup_sideband(int fd[2], int xd[2])\n+static int sideband_demux(int fd, void *data)\n {\n-\tpid_t side_pid;\n+\tint *xd = data;\n \n+\tclose(xd[1]);\n+\treturn recv_sideband(\"fetch-pack\", xd[0], fd, 2);\n+}\n+\n+static void setup_sideband(int fd[2], int xd[2], struct async *demux)\n+{\n \tif (!use_sideband) {\n \t\tfd[0] = xd[0];\n \t\tfd[1] = xd[1];\n-\t\treturn 0;\n+\t\treturn;\n \t}\n \t/* xd[] is talking with upload-pack; subprocess reads from\n \t * xd[0], spits out band#2 to stderr, and feeds us band#1\n-\t * through our fd[0].\n+\t * through demux->out.\n \t */\n-\tif (pipe(fd) < 0)\n-\t\tdie(\"fetch-pack: unable to set up pipe\");\n-\tside_pid = fork();\n-\tif (side_pid < 0)\n+\tdemux->proc = sideband_demux;\n+\tdemux->data = xd;\n+\tif (start_async(demux))\n \t\tdie(\"fetch-pack: unable to fork off sideband demultiplexer\");\n-\tif (!side_pid) {\n-\t\t/* subprocess */\n-\t\tclose(fd[0]);\n-\t\tif (xd[0] != xd[1])\n-\t\t\tclose(xd[1]);\n-\t\tif (recv_sideband(\"fetch-pack\", xd[0], fd[1], 2))\n-\t\t\texit(1);\n-\t\texit(0);\n-\t}\n \tclose(xd[0]);\n-\tclose(fd[1]);\n+\tfd[0] = demux->out;\n \tfd[1] = xd[1];\n-\treturn side_pid;\n }\n \n static int get_pack(int xd[2], char **pack_lockfile)\n {\n-\tpid_t side_pid;\n+\tstruct async demux;\n \tint fd[2];\n \tconst char *argv[20];\n \tchar keep_arg[256];\n@@ -501,7 +496,7 @@ static int get_pack(int xd[2], char **pack_lockfile)\n \tint do_keep = args.keep_pack;\n \tstruct child_process cmd;\n \n-\tside_pid = setup_sideband(fd, xd);\n+\tsetup_sideband(fd, xd, &demux);\n \n \tmemset(&cmd, 0, sizeof(cmd));\n \tcmd.argv = argv;\n@@ -556,6 +551,8 @@ static int get_pack(int xd[2], char **pack_lockfile)\n \n \tif (finish_command(&cmd))\n \t\tdie(\"%s failed\", argv[0]);\n+\tif (use_sideband && finish_async(&demux))\n+\t\tdie(\"error in sideband demultiplexer\");\n \treturn 0;\n }\n \n-- \n1.5.3.3.1134.gee562\n"},{"id":"55636","messageId":"1192305984-22594-11-git-send-email-johannes.sixt@telecom.at","threadId":"10267","inReplyTo":"1192305984-22594-10-git-send-email-johannes.sixt@telecom.at","subject":"[PATCH 10/14] upload-pack: Move the revision walker into a separate function.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-10-13T20:06:20Z","receivedAt":"2007-10-13T20:06:20Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"While this is mostly a cleanup and makes a long function shorter, it later\nalso allows us to use start_async() with this function.\n\nSigned-off-by: Johannes Sixt \n---\n upload-pack.c | 70 ++++++++++++++++++++++++++++++--------------------------\n 1 files changed, 37 insertions(+), 33 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex ef2894a..c5aa0ea 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -97,6 +97,42 @@ static void show_edge(struct commit *commit)\n \tfprintf(pack_pipe, \"-%s\\n\", sha1_to_hex(commit->object.sha1));\n }\n \n+static void do_rev_list(int create_full_pack)\n+{\n+\tint i;\n+\tstruct rev_info revs;\n+\n+\tif (create_full_pack)\n+\t\tuse_thin_pack = 0; /* no point doing it */\n+\tinit_revisions(&revs, NULL);\n+\trevs.tag_objects = 1;\n+\trevs.tree_objects = 1;\n+\trevs.blob_objects = 1;\n+\tif (use_thin_pack)\n+\t\trevs.edge_hint = 1;\n+\n+\tif (create_full_pack) {\n+\t\tconst char *args[] = {\"rev-list\", \"--all\", NULL};\n+\t\tsetup_revisions(2, args, &revs, NULL);\n+\t} else {\n+\t\tfor (i = 0; i < want_obj.nr; i++) {\n+\t\t\tstruct object *o = want_obj.objects[i].item;\n+\t\t\t/* why??? */\n+\t\t\to->flags &= ~UNINTERESTING;\n+\t\t\tadd_pending_object(&revs, o, NULL);\n+\t\t}\n+\t\tfor (i = 0; i < have_obj.nr; i++) {\n+\t\t\tstruct object *o = have_obj.objects[i].item;\n+\t\t\to->flags |= UNINTERESTING;\n+\t\t\tadd_pending_object(&revs, o, NULL);\n+\t\t}\n+\t\tsetup_revisions(0, NULL, &revs, NULL);\n+\t}\n+\tprepare_revision_walk(&revs);\n+\tmark_edges_uninteresting(revs.commits, &revs, show_edge);\n+\ttraverse_commit_list(&revs, show_commit, show_object);\n+}\n+\n static void create_pack_file(void)\n {\n \t/* Pipe from rev-list to pack-objects\n@@ -119,41 +155,9 @@ static void create_pack_file(void)\n \t\tdie(\"git-upload-pack: unable to fork git-rev-list\");\n \n \tif (!pid_rev_list) {\n-\t\tint i;\n-\t\tstruct rev_info revs;\n-\n \t\tclose(lp_pipe[0]);\n \t\tpack_pipe = fdopen(lp_pipe[1], \"w\");\n-\n-\t\tif (create_full_pack)\n-\t\t\tuse_thin_pack = 0; /* no point doing it */\n-\t\tinit_revisions(&revs, NULL);\n-\t\trevs.tag_objects = 1;\n-\t\trevs.tree_objects = 1;\n-\t\trevs.blob_objects = 1;\n-\t\tif (use_thin_pack)\n-\t\t\trevs.edge_hint = 1;\n-\n-\t\tif (create_full_pack) {\n-\t\t\tconst char *args[] = {\"rev-list\", \"--all\", NULL};\n-\t\t\tsetup_revisions(2, args, &revs, NULL);\n-\t\t} else {\n-\t\t\tfor (i = 0; i < want_obj.nr; i++) {\n-\t\t\t\tstruct object *o = want_obj.objects[i].item;\n-\t\t\t\t/* why??? */\n-\t\t\t\to->flags &= ~UNINTERESTING;\n-\t\t\t\tadd_pending_object(&revs, o, NULL);\n-\t\t\t}\n-\t\t\tfor (i = 0; i < have_obj.nr; i++) {\n-\t\t\t\tstruct object *o = have_obj.objects[i].item;\n-\t\t\t\to->flags |= UNINTERESTING;\n-\t\t\t\tadd_pending_object(&revs, o, NULL);\n-\t\t\t}\n-\t\t\tsetup_revisions(0, NULL, &revs, NULL);\n-\t\t}\n-\t\tprepare_revision_walk(&revs);\n-\t\tmark_edges_uninteresting(revs.commits, &revs, show_edge);\n-\t\ttraverse_commit_list(&revs, show_commit, show_object);\n+\t\tdo_rev_list(create_full_pack);\n \t\texit(0);\n \t}\n \n-- \n1.5.3.3.1134.gee562\n"},{"id":"55634","messageId":"1192305984-22594-12-git-send-email-johannes.sixt@telecom.at","threadId":"10267","inReplyTo":"1192305984-22594-11-git-send-email-johannes.sixt@telecom.at","subject":"[PATCH 11/14] upload-pack: Run rev-list in an asynchronous function.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-10-13T20:06:21Z","receivedAt":"2007-10-13T20:06:21Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"This gets rid of an explicit fork().\n\nSince upload-pack has to coordinate two processes (rev-list and\npack-objects), we cannot use the normal finish_async(), but have to monitor\nthe process explicitly. Hence, there are no changes at this front.\n\nSigned-off-by: Johannes Sixt \n---\n upload-pack.c | 46 ++++++++++++++++++----------------------------\n 1 files changed, 18 insertions(+), 28 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex c5aa0ea..6799468 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -97,11 +97,12 @@ static void show_edge(struct commit *commit)\n \tfprintf(pack_pipe, \"-%s\\n\", sha1_to_hex(commit->object.sha1));\n }\n \n-static void do_rev_list(int create_full_pack)\n+static int do_rev_list(int fd, void *create_full_pack)\n {\n \tint i;\n \tstruct rev_info revs;\n \n+\tpack_pipe = fdopen(fd, \"w\");\n \tif (create_full_pack)\n \t\tuse_thin_pack = 0; /* no point doing it */\n \tinit_revisions(&revs, NULL);\n@@ -131,14 +132,12 @@ static void do_rev_list(int create_full_pack)\n \tprepare_revision_walk(&revs);\n \tmark_edges_uninteresting(revs.commits, &revs, show_edge);\n \ttraverse_commit_list(&revs, show_commit, show_object);\n+\treturn 0;\n }\n \n static void create_pack_file(void)\n {\n-\t/* Pipe from rev-list to pack-objects\n-\t */\n-\tint lp_pipe[2];\n-\tpid_t pid_rev_list;\n+\tstruct async rev_list;\n \tstruct child_process pack_objects;\n \tint create_full_pack = (nr_our_refs == want_obj.nr && !have_obj.nr);\n \tchar data[8193], progress[128];\n@@ -148,22 +147,12 @@ static void create_pack_file(void)\n \tconst char *argv[10];\n \tint arg = 0;\n \n-\tif (pipe(lp_pipe) < 0)\n-\t\tdie(\"git-upload-pack: unable to create pipe\");\n-\tpid_rev_list = fork();\n-\tif (pid_rev_list < 0)\n+\trev_list.proc = do_rev_list;\n+\t/* .data is just a boolean: any non-NULL value will do */\n+\trev_list.data = create_full_pack ? &rev_list : NULL;\n+\tif (start_async(&rev_list))\n \t\tdie(\"git-upload-pack: unable to fork git-rev-list\");\n \n-\tif (!pid_rev_list) {\n-\t\tclose(lp_pipe[0]);\n-\t\tpack_pipe = fdopen(lp_pipe[1], \"w\");\n-\t\tdo_rev_list(create_full_pack);\n-\t\texit(0);\n-\t}\n-\n-\t/* close this so that it is not inherited by pack_objects */\n-\tclose(lp_pipe[1]);\n-\n \targv[arg++] = \"pack-objects\";\n \targv[arg++] = \"--stdout\";\n \tif (!no_progress)\n@@ -173,14 +162,15 @@ static void create_pack_file(void)\n \targv[arg++] = NULL;\n \n \tmemset(&pack_objects, 0, sizeof(pack_objects));\n-\tpack_objects.in = lp_pipe[0];\t/* start_command closes it */\n+\tpack_objects.in = rev_list.out;\t/* start_command closes it */\n \tpack_objects.out = -1;\n \tpack_objects.err = -1;\n \tpack_objects.git_cmd = 1;\n \tpack_objects.argv = argv;\n+\n \tif (start_command(&pack_objects)) {\n \t\t/* daemon sets things up to ignore TERM */\n-\t\tkill(pid_rev_list, SIGKILL);\n+\t\tkill(rev_list.pid, SIGKILL);\n \t\tdie(\"git-upload-pack: unable to fork git-pack-objects\");\n \t}\n \n@@ -280,11 +270,11 @@ static void create_pack_file(void)\n \t\t}\n \n \t\t/* See if the children are still there */\n-\t\tif (pid_rev_list || pack_objects.pid) {\n+\t\tif (rev_list.pid || pack_objects.pid) {\n \t\t\tpid = waitpid(-1, &status, WNOHANG);\n \t\t\tif (!pid)\n \t\t\t\tcontinue;\n-\t\t\twho = ((pid == pid_rev_list) ? \"git-rev-list\" :\n+\t\t\twho = ((pid == rev_list.pid) ? \"git-rev-list\" :\n \t\t\t (pid == pack_objects.pid) ? \"git-pack-objects\" :\n \t\t\t NULL);\n \t\t\tif (!who) {\n@@ -302,11 +292,11 @@ static void create_pack_file(void)\n \t\t\t\t who);\n \t\t\t\tgoto fail;\n \t\t\t}\n-\t\t\tif (pid == pid_rev_list)\n-\t\t\t\tpid_rev_list = 0;\n+\t\t\tif (pid == rev_list.pid)\n+\t\t\t\trev_list.pid = 0;\n \t\t\tif (pid == pack_objects.pid)\n \t\t\t\tpack_objects.pid = 0;\n-\t\t\tif (pid_rev_list || pack_objects.pid)\n+\t\t\tif (rev_list.pid || pack_objects.pid)\n \t\t\t\tcontinue;\n \t\t}\n \n@@ -329,8 +319,8 @@ static void create_pack_file(void)\n fail:\n \tif (pack_objects.pid)\n \t\tkill(pack_objects.pid, SIGKILL);\n-\tif (pid_rev_list)\n-\t\tkill(pid_rev_list, SIGKILL);\n+\tif (rev_list.pid)\n+\t\tkill(rev_list.pid, SIGKILL);\n \tsend_client_data(3, abort_msg, sizeof(abort_msg));\n \tdie(\"git-upload-pack: %s\", abort_msg);\n }\n-- \n1.5.3.3.1134.gee562\n"},{"id":"55640","messageId":"1192305984-22594-13-git-send-email-johannes.sixt@telecom.at","threadId":"10267","inReplyTo":"1192305984-22594-12-git-send-email-johannes.sixt@telecom.at","subject":"[PATCH 12/14] t0021-conversion.sh: Test that the clean filter really cleans content.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-10-13T20:06:22Z","receivedAt":"2007-10-13T20:06:22Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"This test uses a rot13 filter, which is its own inverse. It tested only\nthat the content was the same as the original after both the 'clean' and\nthe 'smudge' filter were applied. This way it would not detect whether\nany filter was run at all. Hence, here we add another test that checks\nthat the repository contained content that was processed by the 'clean'\nfilter.\n\nSigned-off-by: Johannes Sixt \n---\n t/t0021-conversion.sh | 7 ++++++-\n 1 files changed, 6 insertions(+), 1 deletions(-)\n\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex a839f4e..cb86029 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -42,7 +42,12 @@ test_expect_success check '\n \tgit diff --raw --exit-code :test :test.i &&\n \tid=$(git rev-parse --verify :test) &&\n \tembedded=$(sed -ne \"$script\" test.i) &&\n-\ttest \"z$id\" = \"z$embedded\"\n+\ttest \"z$id\" = \"z$embedded\" &&\n+\n+\tgit cat-file blob :test.t > test.r &&\n+\n+\t./rot13.sh < test.o > test.t &&\n+\tcmp test.r test.t\n '\n \n # If an expanded ident ever gets into the repository, we want to make sure that\n-- \n1.5.3.3.1134.gee562\n"},{"id":"55632","messageId":"1192305984-22594-14-git-send-email-johannes.sixt@telecom.at","threadId":"10267","inReplyTo":"1192305984-22594-13-git-send-email-johannes.sixt@telecom.at","subject":"[PATCH 13/14] Avoid a dup2(2) in apply_filter() - start_command() can do it for us.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-10-13T20:06:23Z","receivedAt":"2007-10-13T20:06:23Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"When apply_filter() runs the external (clean or smudge) filter program, it\nneeds to pass the writable end of a pipe as its stdout. For this purpose,\nit used to dup2(2) the file descriptor explicitly to stdout. Now we use\nthe facilities of start_command() to do it for us.\n\nFurthermore, the path argument of a subordinate function, filter_buffer(),\nwas not used, so here we replace it to pass the fd instead.\n\nSigned-off-by: Johannes Sixt \n---\n convert.c | 7 +++----\n 1 files changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 6d64994..c870817 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -201,7 +201,7 @@ static char *crlf_to_worktree(const char *path, const char *src, unsigned long *\n \treturn buffer;\n }\n \n-static int filter_buffer(const char *path, const char *src,\n+static int filter_buffer(int fd, const char *src,\n \t\t\t unsigned long size, const char *cmd)\n {\n \t/*\n@@ -214,6 +214,7 @@ static int filter_buffer(const char *path, const char *src,\n \tmemset(&child_process, 0, sizeof(child_process));\n \tchild_process.argv = argv;\n \tchild_process.in = -1;\n+\tchild_process.out = fd;\n \n \tif (start_command(&child_process))\n \t\treturn error(\"cannot fork to run external filter %s\", cmd);\n@@ -265,10 +266,8 @@ static char *apply_filter(const char *path, const char *src,\n \t\treturn NULL;\n \t}\n \tif (!child_process.pid) {\n-\t\tdup2(pipe_feed[1], 1);\n \t\tclose(pipe_feed[0]);\n-\t\tclose(pipe_feed[1]);\n-\t\texit(filter_buffer(path, src, *sizep, cmd));\n+\t\texit(filter_buffer(pipe_feed[1], src, *sizep, cmd));\n \t}\n \tclose(pipe_feed[1]);\n \n-- \n1.5.3.3.1134.gee562\n"},{"id":"55633","messageId":"1192305984-22594-15-git-send-email-johannes.sixt@telecom.at","threadId":"10267","inReplyTo":"1192305984-22594-14-git-send-email-johannes.sixt@telecom.at","subject":"[PATCH 14/14] Use the asyncronous function infrastructure to run the content filter.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-10-13T20:06:24Z","receivedAt":"2007-10-13T20:06:24Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Signed-off-by: Johannes Sixt \n---\n convert.c | 60 +++++++++++++++++++++++++++---------------------------------\n 1 files changed, 27 insertions(+), 33 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex c870817..10161a0 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -201,15 +201,21 @@ static char *crlf_to_worktree(const char *path, const char *src, unsigned long *\n \treturn buffer;\n }\n \n-static int filter_buffer(int fd, const char *src,\n-\t\t\t unsigned long size, const char *cmd)\n+struct filter_params {\n+\tconst char *src;\n+\tunsigned long size;\n+\tconst char *cmd;\n+};\n+\n+static int filter_buffer(int fd, void *data)\n {\n \t/*\n \t * Spawn cmd and feed the buffer contents through its stdin.\n \t */\n \tstruct child_process child_process;\n+\tstruct filter_params *params = (struct filter_params *)data;\n \tint write_err, status;\n-\tconst char *argv[] = { \"sh\", \"-c\", cmd, NULL };\n+\tconst char *argv[] = { \"sh\", \"-c\", params->cmd, NULL };\n \n \tmemset(&child_process, 0, sizeof(child_process));\n \tchild_process.argv = argv;\n@@ -217,17 +223,17 @@ static int filter_buffer(int fd, const char *src,\n \tchild_process.out = fd;\n \n \tif (start_command(&child_process))\n-\t\treturn error(\"cannot fork to run external filter %s\", cmd);\n+\t\treturn error(\"cannot fork to run external filter %s\", params->cmd);\n \n-\twrite_err = (write_in_full(child_process.in, src, size) < 0);\n+\twrite_err = (write_in_full(child_process.in, params->src, params->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+\t\terror(\"cannot feed the input to external filter %s\", params->cmd);\n \n \tstatus = finish_command(&child_process);\n \tif (status)\n-\t\terror(\"external filter %s failed %d\", cmd, -status);\n+\t\terror(\"external filter %s failed\", params->cmd);\n \treturn (write_err || status);\n }\n \n@@ -241,42 +247,31 @@ static char *apply_filter(const char *path, const char *src,\n \t * (child --> cmd) --> us\n \t */\n \tconst int SLOP = 4096;\n-\tint pipe_feed[2];\n-\tint status;\n \tchar *dst;\n \tunsigned long dstsize, dstalloc;\n-\tstruct child_process child_process;\n+\tstruct async async;\n+\tstruct filter_params params;\n \n \tif (!cmd)\n \t\treturn NULL;\n \n-\tmemset(&child_process, 0, sizeof(child_process));\n-\n-\tif (pipe(pipe_feed) < 0) {\n-\t\terror(\"cannot create pipe to run external filter %s\", cmd);\n-\t\treturn NULL;\n-\t}\n+\tmemset(&async, 0, sizeof(async));\n+\tasync.proc = filter_buffer;\n+\tasync.data = ¶ms;\n+\tparams.src = src;\n+\tparams.size = *sizep;\n+\tparams.cmd = cmd;\n \n \tfflush(NULL);\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 NULL;\n-\t}\n-\tif (!child_process.pid) {\n-\t\tclose(pipe_feed[0]);\n-\t\texit(filter_buffer(pipe_feed[1], src, *sizep, cmd));\n-\t}\n-\tclose(pipe_feed[1]);\n+\tif (start_async(&async))\n+\t\treturn 0;\t/* error was already reported */\n \n \tdstalloc = *sizep;\n \tdst = xmalloc(dstalloc);\n \tdstsize = 0;\n \n \twhile (1) {\n-\t\tssize_t numread = xread(pipe_feed[0], dst + dstsize,\n+\t\tssize_t numread = xread(async.out, dst + dstsize,\n \t\t\t\t\tdstalloc - dstsize);\n \n \t\tif (numread <= 0) {\n@@ -293,15 +288,14 @@ static char *apply_filter(const char *path, const char *src,\n \t\t\tdst = xrealloc(dst, dstalloc);\n \t\t}\n \t}\n-\tif (close(pipe_feed[0])) {\n+\tif (close(async.out)) {\n \t\terror(\"read from external filter %s failed\", cmd);\n \t\tfree(dst);\n \t\tdst = NULL;\n \t}\n \n-\tstatus = finish_command(&child_process);\n-\tif (status) {\n-\t\terror(\"external filter %s failed %d\", cmd, -status);\n+\tif (finish_async(&async)) {\n+\t\terror(\"external filter %s failed\", cmd);\n \t\tfree(dst);\n \t\tdst = NULL;\n \t}\n-- \n1.5.3.3.1134.gee562\n"},{"id":"55657","messageId":"Pine.LNX.4.64.0710140156100.25221@racer.site","threadId":"10267","inReplyTo":"1192305984-22594-2-git-send-email-johannes.sixt@telecom.at","subject":"Re: [PATCH 01/14] Change git_connect() to return a struct child_process instead of a pid_t.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-10-14T00:57:28Z","receivedAt":"2007-10-14T00:57:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 13 Oct 2007, Johannes Sixt wrote:\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\nJust for completeness' sake: could you do a free(conn); before return -1;?\n\nThanks,\nDscho\n"},{"id":"55661","messageId":"20071014021149.GO27899@spearce.org","threadId":"10267","inReplyTo":"1192305984-22594-1-git-send-email-johannes.sixt@telecom.at","subject":"Re: [PATCH 0/14] fork/exec removal series","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-10-14T02:11:49Z","receivedAt":"2007-10-14T02:11:49Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Johannes Sixt wrote:\n> here is a series of patches that removes a number fork/exec pairs.\n...\n> The series consists of 2 parts:\n> \n> - The first half replaces a number of fork/exec pairs by start_command/\n> finish_command or run_command.\n> \n> - The second half introduces a new framework that runs a function\n> asynchronously. New functions start_async and finish_async are implemented\n> similarly to start_command and run_command. They are used to replace\n> occurrences of fork() that does not exec() in the child. Such code\n> could in principle be run in a thread, and on MinGW port we will go this\n> route, but on Posix we stay with fork().\n\nThis series looks pretty good to me. I like seeing huge blocks\ngo away only to be replaced with the simple API offered by\nrun-command.h. Makes the result much easier to follow.\n\nThe async interface is also quite simple. Unfortunately there\nis some risk with the canonical fork() implementation in that the\nasync routine might attempt to alter global data that the parent\nis also using, and folks on a good UNIX that is using the fork()\nimplementation will not even notice as they are in totally separated\naddress spaces. But you'll see it in MSYS Git.\n\nSince builtin-pack-objects now accepts (limited) pthread support,\nperhaps this should be implemented in terms of pthread support\nwhen pthreads are available? Most Linux/BSD/Mac OS X systems do\nhave pthreads these days and that's the majority of git users and\ndevelopers. This would make it more likely that bugs in this sort\nof code would be detected early. Just a thought.\n \n> 13 files changed, 334 insertions(+), 369 deletions(-)\n\nHard to argue with that final state. You killed 35 lines and\nalso made Git easier to port to \"that OS unfortunately named after\ntransparent glass thingies\".\n\n-- \nShawn.\n"},{"id":"55662","messageId":"Pine.LNX.4.64.0710140348550.25221@racer.site","threadId":"10267","inReplyTo":"20071014021149.GO27899@spearce.org","subject":"Re: [PATCH 0/14] fork/exec removal series","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-10-14T02:50:33Z","receivedAt":"2007-10-14T02:50:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 13 Oct 2007, Shawn O. Pearce wrote:\n\n> Since builtin-pack-objects now accepts (limited) pthread support, \n> perhaps this should be implemented in terms of pthread support when \n> pthreads are available?\n\nFalling back to fork() when no pthreads are available? Yes, that makes \nsense.\n\nIt might also (marginally) speed up operations, since the switches between \nthreads are cheaper than those between processes, right?\n\nCiao,\nDscho\n"},{"id":"55663","messageId":"20071014025857.GQ27899@spearce.org","threadId":"10267","inReplyTo":"Pine.LNX.4.64.0710140348550.25221@racer.site","subject":"Re: [PATCH 0/14] fork/exec removal series","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-10-14T02:58:57Z","receivedAt":"2007-10-14T02:58:57Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Johannes Schindelin wrote:\n> On Sat, 13 Oct 2007, Shawn O. Pearce wrote:\n> \n> > Since builtin-pack-objects now accepts (limited) pthread support, \n> > perhaps this should be implemented in terms of pthread support when \n> > pthreads are available?\n> \n> Falling back to fork() when no pthreads are available? Yes, that makes \n> sense.\n> \n> It might also (marginally) speed up operations, since the switches between \n> threads are cheaper than those between processes, right?\n\nUsually. If we have a large virtual address space (say due to\nopening a bunch of packfiles and reading commits out of them into\nstruct commit* thingies) and the OS does a giant copy of the page\ntables during fork() then the pthread creation should be a heck of\na lot cheaper.\n\nBut we most definately *must* continue to support fork() for the\nasync functions. Its the most common interface available on one\nof our biggest platforms (UNIX).\n\n-- \nShawn.\n"},{"id":"55664","messageId":"Pine.LNX.4.64.0710140404480.25221@racer.site","threadId":"10267","inReplyTo":"1192305984-22594-15-git-send-email-johannes.sixt@telecom.at","subject":"Re: [PATCH 14/14] Use the asyncronous function infrastructure to run the content filter.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-10-14T03:07:18Z","receivedAt":"2007-10-14T03:07:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 13 Oct 2007, Johannes Sixt wrote:\n\n> \tstatus = finish_command(&child_process);\n> \tif (status)\n> -\t\terror(\"external filter %s failed %d\", cmd, -status);\n> +\t\terror(\"external filter %s failed\", params->cmd);\n\nDid you mean to remove the status from the output (it should probably read \n\"(exit status %d)\" instead of just \"%d\", but an exit status can help \nidentify problems, right?\n\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 NULL;\n> -\t}\n> -\tif (!child_process.pid) {\n> -\t\tclose(pipe_feed[0]);\n> -\t\texit(filter_buffer(pipe_feed[1], src, *sizep, cmd));\n> -\t}\n> -\tclose(pipe_feed[1]);\n> +\tif (start_async(&async))\n> +\t\treturn 0;\t/* error was already reported */\n\nPlease write \"return NULL;\"\n\nThanks,\nDscho\n"},{"id":"55669","messageId":"20071014071239.GB1198@artemis.corp","threadId":"10267","inReplyTo":"20071014025857.GQ27899@spearce.org","subject":"Re: [PATCH 0/14] fork/exec removal series","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-10-14T07:12:39Z","receivedAt":"2007-10-14T07:12:39Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Sun, Oct 14, 2007 at 02:58:57AM +0000, Shawn O. Pearce wrote:\n> Johannes Schindelin wrote:\n> > On Sat, 13 Oct 2007, Shawn O. Pearce wrote:\n> > \n> > > Since builtin-pack-objects now accepts (limited) pthread support, \n> > > perhaps this should be implemented in terms of pthread support when \n> > > pthreads are available?\n> > \n> > Falling back to fork() when no pthreads are available? Yes, that makes \n> > sense.\n> > \n> > It might also (marginally) speed up operations, since the switches between \n> > threads are cheaper than those between processes, right?\n> \n> Usually. If we have a large virtual address space (say due to\n> opening a bunch of packfiles and reading commits out of them into\n> struct commit* thingies) and the OS does a giant copy of the page\n> tables during fork() then the pthread creation should be a heck of\n> a lot cheaper.\n> \n> But we most definately *must* continue to support fork() for the\n> async functions. Its the most common interface available on one\n> of our biggest platforms (UNIX).\n\n Yeah that, and the fact that many of the git modules aren't\nthread-safe (some modules have static buffers strbuf's or caching\nvariables) and should be used with care.\n\n The trivial way is to add a __thread keyword to make them TLS\nvariables, though, it's not really a step in the direction of\nportability, and last time I looked at it, mingw didn't had TLS support,\nnot sure if msys has. Though, if Msys has, it's worth using, and we\ncould require that targets using the fancy pthread thingy should also\nhave some fancy TLS, or use fork().\n\n Portability for such issues, would be to use pthread_key_* and\npthread_{get,set}specific, and those are a hell of a sucky API.\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"55670","messageId":"20071014071751.GC1198@artemis.corp","threadId":"10267","inReplyTo":"20071014071239.GB1198@artemis.corp","subject":"Re: [PATCH 0/14] fork/exec removal series","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-10-14T07:17:51Z","receivedAt":"2007-10-14T07:17:51Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On dim, oct 14, 2007 at 07:12:39 +0000, Pierre Habouzit wrote:\n> The trivial way is to add a __thread keyword to make them TLS\n> variables, though, it's not really a step in the direction of\n> portability, and last time I looked at it, mingw didn't had TLS support,\n> not sure if msys has. Though, if Msys has, it's worth using, and we\n\n Okay forget it, mingw and msys are one and the same *g*.\n So well, maybe threading isn't such a so great idea :/\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"55671","messageId":"20071014072849.GD1198@artemis.corp","threadId":"10267","inReplyTo":"20071014071751.GC1198@artemis.corp","subject":"Re: [PATCH 0/14] fork/exec removal series","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-10-14T07:28:49Z","receivedAt":"2007-10-14T07:28:49Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On dim, oct 14, 2007 at 07:17:51 +0000, Pierre Habouzit wrote:\n> On dim, oct 14, 2007 at 07:12:39 +0000, Pierre Habouzit wrote:\n> > The trivial way is to add a __thread keyword to make them TLS\n> > variables, though, it's not really a step in the direction of\n> > portability, and last time I looked at it, mingw didn't had TLS support,\n> > not sure if msys has. Though, if Msys has, it's worth using, and we\n> \n> Okay forget it, mingw and msys are one and the same *g*.\n> So well, maybe threading isn't such a so great idea :/\n\n And again last time I checked it was still a mingw 3.x in debian, now\nthat it's 4.2.1 it seems to support __thread (but not\n__declspec(thread)) and their changelog seems to confirm that fact [0].\n\n So the question holds again, do we require pthread-using targets to\nsupport TLS ? It feels sane and right to me, but …\n\n\n [0] http://sourceforge.net/project/shownotes.php?release_id=532062\n [...]\n * The __thread keyword is honoured.\n [...]\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"55686","messageId":"4711DD20.8070600@op5.se","threadId":"10267","inReplyTo":"20071014072849.GD1198@artemis.corp","subject":"Re: [PATCH 0/14] fork/exec removal series","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2007-10-14T09:10:56Z","receivedAt":"2007-10-14T09:10:56Z","isPatch":true,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"Pierre Habouzit wrote:\n> On dim, oct 14, 2007 at 07:17:51 +0000, Pierre Habouzit wrote:\n>> On dim, oct 14, 2007 at 07:12:39 +0000, Pierre Habouzit wrote:\n>>> The trivial way is to add a __thread keyword to make them TLS\n>>> variables, though, it's not really a step in the direction of\n>>> portability, and last time I looked at it, mingw didn't had TLS support,\n>>> not sure if msys has. Though, if Msys has, it's worth using, and we\n>> Okay forget it, mingw and msys are one and the same *g*.\n>> So well, maybe threading isn't such a so great idea :/\n> \n> And again last time I checked it was still a mingw 3.x in debian, now\n> that it's 4.2.1 it seems to support __thread (but not\n> __declspec(thread)) and their changelog seems to confirm that fact [0].\n> \n> So the question holds again, do we require pthread-using targets to\n> support TLS ? It feels sane and right to me, but …\n> \n\nTo me it's a sane place to start. As time goes by and people on non-TLS\ncapable systems come along that need the functionality (or the speedup;\nfork() is expensive on some systems), they can probably implement it\nthemselves or at least give voice to the fact that they need it.\n\n-- \nAndreas Ericsson andreas.ericsson@op5.se\nOP5 AB www.op5.se\nTel: +46 8-230225 Fax: +46 8-230231\n"},{"id":"55689","messageId":"200710141139.31410.johannes.sixt@telecom.at","threadId":"10267","inReplyTo":"Pine.LNX.4.64.0710140404480.25221@racer.site","subject":"Re: [PATCH 14/14] Use the asyncronous function infrastructure to run the content filter.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-10-14T09:39:31Z","receivedAt":"2007-10-14T09:39:31Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Sunday 14 October 2007 05:07, Johannes Schindelin wrote:\n> Hi,\n>\n> On Sat, 13 Oct 2007, Johannes Sixt wrote:\n> > \tstatus = finish_command(&child_process);\n> > \tif (status)\n> > -\t\terror(\"external filter %s failed %d\", cmd, -status);\n> > +\t\terror(\"external filter %s failed\", params->cmd);\n>\n> Did you mean to remove the status from the output (it should probably read\n> \"(exit status %d)\" instead of just \"%d\", but an exit status can help\n> identify problems, right?\n\nOops, that looks like an artefact. Will correct.\n\n> > +\tif (start_async(&async))\n> > +\t\treturn 0;\t/* error was already reported */\n>\n> Please write \"return NULL;\"\n\nWill do.\n\n-- Hannes\n"},{"id":"55690","messageId":"200710141140.10597.johannes.sixt@telecom.at","threadId":"10267","inReplyTo":"Pine.LNX.4.64.0710140156100.25221@racer.site","subject":"Re: [PATCH 01/14] 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-14T09:40:10Z","receivedAt":"2007-10-14T09:40:10Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Sunday 14 October 2007 02:57, Johannes Schindelin wrote:\n> Hi,\n>\n> On Sat, 13 Oct 2007, Johannes Sixt wrote:\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>\n> Just for completeness' sake: could you do a free(conn); before return -1;?\n\nI know. But the loop is going away with the next patch, so I didn't bother. \nCan you live with that?\n\n-- Hannes\n"},{"id":"55739","messageId":"200710141908.45779.johannes.sixt@telecom.at","threadId":"10267","inReplyTo":"1192305984-22594-9-git-send-email-johannes.sixt@telecom.at","subject":"[PATCH amend 08/14] Add infrastructure to run a function asynchronously.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-10-14T17:08:45Z","receivedAt":"2007-10-14T17:08:45Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"This adds start_async() and finish_async(), which runs a function\nasynchronously. Communication with the caller happens only via pipes.\nFor this reason, this implementation forks off a child process that runs\nthe function.\n\nSigned-off-by: Johannes Sixt \n---\n\nThis is the same as submitted previously except it uses close_pair()\nto close the pipe in an error patch and improves the comments in\nrun-command.h.\n\n-- Hannes\n\n run-command.c | 52 ++++++++++++++++++++++++++++++++++++++++++++--------\n run-command.h | 22 ++++++++++++++++++++++\n 2 files changed, 66 insertions(+), 8 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex d00c03b..db02f75 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -127,16 +127,11 @@ int start_command(struct child_process *cmd)\n \treturn 0;\n }\n \n-int finish_command(struct child_process *cmd)\n+static int wait_or_whine(pid_t pid)\n {\n-\tif (cmd->close_in)\n-\t\tclose(cmd->in);\n-\tif (cmd->close_out)\n-\t\tclose(cmd->out);\n-\n \tfor (;;) {\n \t\tint status, code;\n-\t\tpid_t waiting = waitpid(cmd->pid, &status, 0);\n+\t\tpid_t waiting = waitpid(pid, &status, 0);\n \n \t\tif (waiting < 0) {\n \t\t\tif (errno == EINTR)\n@@ -144,7 +139,7 @@ int finish_command(struct child_process *cmd)\n \t\t\terror(\"waitpid failed (%s)\", strerror(errno));\n \t\t\treturn -ERR_RUN_COMMAND_WAITPID;\n \t\t}\n-\t\tif (waiting != cmd->pid)\n+\t\tif (waiting != pid)\n \t\t\treturn -ERR_RUN_COMMAND_WAITPID_WRONG_PID;\n \t\tif (WIFSIGNALED(status))\n \t\t\treturn -ERR_RUN_COMMAND_WAITPID_SIGNAL;\n@@ -158,6 +153,15 @@ int finish_command(struct child_process *cmd)\n \t}\n }\n \n+int finish_command(struct child_process *cmd)\n+{\n+\tif (cmd->close_in)\n+\t\tclose(cmd->in);\n+\tif (cmd->close_out)\n+\t\tclose(cmd->out);\n+\treturn wait_or_whine(cmd->pid);\n+}\n+\n int run_command(struct child_process *cmd)\n {\n \tint code = start_command(cmd);\n@@ -200,3 +204,35 @@ int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const\n \tcmd.env = env;\n \treturn run_command(&cmd);\n }\n+\n+int start_async(struct async *async)\n+{\n+\tint pipe_out[2];\n+\n+\tif (pipe(pipe_out) < 0) {\n+\t\treturn error(\"cannot create pipe: %s\", strerror(errno));\n+\t}\n+\n+\tasync->pid = fork();\n+\tif (async->pid < 0) {\n+\t\terror(\"fork (async) failed: %s\", strerror(errno));\n+\t\tclose_pair(pipe_out);\n+\t\treturn -1;\n+\t}\n+\tif (!async->pid) {\n+\t\tclose(pipe_out[0]);\n+\t\texit(!!async->proc(pipe_out[1], async->data));\n+\t}\n+\tasync->out = pipe_out[0];\n+\tclose(pipe_out[1]);\n+\treturn 0;\n+}\n+\n+int finish_async(struct async *async)\n+{\n+\tint ret = 0;\n+\n+\tif (wait_or_whine(async->pid))\n+\t\tret = error(\"waitpid (async) failed\");\n+\treturn ret;\n+}\ndiff --git a/run-command.h b/run-command.h\nindex 35b9fb6..94e1e9d 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -43,4 +43,26 @@ int run_command_v_opt_cd(const char **argv, int opt, const char *dir);\n */\n int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const char *const *env);\n \n+/*\n+ * The purpose of the following functions is to feed a pipe by running\n+ * a function asynchronously and providing output that the caller reads.\n+ *\n+ * It is expected that no synchronization and mutual exclusion between\n+ * the caller and the feed function is necessary so that the function\n+ * can run in a thread without interfering with the caller.\n+ */\n+struct async {\n+\t/*\n+\t * proc writes to fd and closes it;\n+\t * returns 0 on success, non-zero on failure\n+\t */\n+\tint (*proc)(int fd, void *data);\n+\tvoid *data;\n+\tint out;\t/* caller reads from here and closes it */\n+\tpid_t pid;\n+};\n+\n+int start_async(struct async *async);\n+int finish_async(struct async *async);\n+\n #endif\n-- \n1.5.3.2.141.g48f10\n"},{"id":"55740","messageId":"Pine.LNX.4.64.0710141808300.25221@racer.site","threadId":"10267","inReplyTo":"20071014071239.GB1198@artemis.corp","subject":"Re: [PATCH 0/14] fork/exec removal series","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-10-14T17:09:34Z","receivedAt":"2007-10-14T17:09:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 14 Oct 2007, Pierre Habouzit wrote:\n\n> On Sun, Oct 14, 2007 at 02:58:57AM +0000, Shawn O. Pearce wrote:\n>\n> > But we most definately *must* continue to support fork() for the async \n> > functions. Its the most common interface available on one of our \n> > biggest platforms (UNIX).\n> \n> Yeah that, and the fact that many of the git modules aren't thread-safe \n> (some modules have static buffers strbuf's or caching variables) and \n> should be used with care.\n\nI think that was exactly the point of Shawn: expose bugs on Unix that \nwould otherwise only show in msysGit.\n\nCiao,\nDscho\n"},{"id":"55741","messageId":"Pine.LNX.4.64.0710141810020.25221@racer.site","threadId":"10267","inReplyTo":"200710141140.10597.johannes.sixt@telecom.at","subject":"Re: [PATCH 01/14] Change git_connect() to return a struct child_process instead of a pid_t.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-10-14T17:10:22Z","receivedAt":"2007-10-14T17:10:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 14 Oct 2007, Johannes Sixt wrote:\n\n> On Sunday 14 October 2007 02:57, Johannes Schindelin wrote:\n> >\n> > On Sat, 13 Oct 2007, Johannes Sixt wrote:\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> >\n> > Just for completeness' sake: could you do a free(conn); before return \n> > -1;?\n> \n> I know. But the loop is going away with the next patch, so I didn't \n> bother. Can you live with that?\n\nIt'll be hard, but I'll try ;-)\n\nCiao,\nDscho\n"},{"id":"55743","messageId":"200710141914.11775.johannes.sixt@telecom.at","threadId":"10267","inReplyTo":"Pine.LNX.4.64.0710140404480.25221@racer.site","subject":"[PATCH amend 14/14] Use the asyncronous function infrastructure to run the content filter.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-10-14T17:14:11Z","receivedAt":"2007-10-14T17:14:11Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Signed-off-by: Johannes Sixt \n---\nOn Sunday 14 October 2007 05:07, Johannes Schindelin wrote:\n> On Sat, 13 Oct 2007, Johannes Sixt wrote:\n> > \tstatus = finish_command(&child_process);\n> > \tif (status)\n> > -\t\terror(\"external filter %s failed %d\", cmd, -status);\n> > +\t\terror(\"external filter %s failed\", params->cmd);\n>\n> Did you mean to remove the status from the output (it should probably read\n> \"(exit status %d)\" instead of just \"%d\", but an exit status can help\n> identify problems, right?\n\nI didn't mean to change the error message here.\n\n> > +\tif (start_async(&async))\n> > +\t\treturn 0;\t/* error was already reported */\n>\n> Please write \"return NULL;\"\n\nThis patch now does just that.\n\n-- Hannes\n\n convert.c | 60 +++++++++++++++++++++++++++---------------------------------\n 1 files changed, 27 insertions(+), 33 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex c870817..ac04157 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -201,15 +201,21 @@ static char *crlf_to_worktree(const char *path, const char *src, unsigned long *\n \treturn buffer;\n }\n \n-static int filter_buffer(int fd, const char *src,\n-\t\t\t unsigned long size, const char *cmd)\n+struct filter_params {\n+\tconst char *src;\n+\tunsigned long size;\n+\tconst char *cmd;\n+};\n+\n+static int filter_buffer(int fd, void *data)\n {\n \t/*\n \t * Spawn cmd and feed the buffer contents through its stdin.\n \t */\n \tstruct child_process child_process;\n+\tstruct filter_params *params = (struct filter_params *)data;\n \tint write_err, status;\n-\tconst char *argv[] = { \"sh\", \"-c\", cmd, NULL };\n+\tconst char *argv[] = { \"sh\", \"-c\", params->cmd, NULL };\n \n \tmemset(&child_process, 0, sizeof(child_process));\n \tchild_process.argv = argv;\n@@ -217,17 +223,17 @@ static int filter_buffer(int fd, const char *src,\n \tchild_process.out = fd;\n \n \tif (start_command(&child_process))\n-\t\treturn error(\"cannot fork to run external filter %s\", cmd);\n+\t\treturn error(\"cannot fork to run external filter %s\", params->cmd);\n \n-\twrite_err = (write_in_full(child_process.in, src, size) < 0);\n+\twrite_err = (write_in_full(child_process.in, params->src, params->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+\t\terror(\"cannot feed the input to external filter %s\", params->cmd);\n \n \tstatus = finish_command(&child_process);\n \tif (status)\n-\t\terror(\"external filter %s failed %d\", cmd, -status);\n+\t\terror(\"external filter %s failed %d\", params->cmd, -status);\n \treturn (write_err || status);\n }\n \n@@ -241,42 +247,31 @@ static char *apply_filter(const char *path, const char *src,\n \t * (child --> cmd) --> us\n \t */\n \tconst int SLOP = 4096;\n-\tint pipe_feed[2];\n-\tint status;\n \tchar *dst;\n \tunsigned long dstsize, dstalloc;\n-\tstruct child_process child_process;\n+\tstruct async async;\n+\tstruct filter_params params;\n \n \tif (!cmd)\n \t\treturn NULL;\n \n-\tmemset(&child_process, 0, sizeof(child_process));\n-\n-\tif (pipe(pipe_feed) < 0) {\n-\t\terror(\"cannot create pipe to run external filter %s\", cmd);\n-\t\treturn NULL;\n-\t}\n+\tmemset(&async, 0, sizeof(async));\n+\tasync.proc = filter_buffer;\n+\tasync.data = ¶ms;\n+\tparams.src = src;\n+\tparams.size = *sizep;\n+\tparams.cmd = cmd;\n \n \tfflush(NULL);\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 NULL;\n-\t}\n-\tif (!child_process.pid) {\n-\t\tclose(pipe_feed[0]);\n-\t\texit(filter_buffer(pipe_feed[1], src, *sizep, cmd));\n-\t}\n-\tclose(pipe_feed[1]);\n+\tif (start_async(&async))\n+\t\treturn NULL;\t/* error was already reported */\n \n \tdstalloc = *sizep;\n \tdst = xmalloc(dstalloc);\n \tdstsize = 0;\n \n \twhile (1) {\n-\t\tssize_t numread = xread(pipe_feed[0], dst + dstsize,\n+\t\tssize_t numread = xread(async.out, dst + dstsize,\n \t\t\t\t\tdstalloc - dstsize);\n \n \t\tif (numread <= 0) {\n@@ -293,15 +288,14 @@ static char *apply_filter(const char *path, const char *src,\n \t\t\tdst = xrealloc(dst, dstalloc);\n \t\t}\n \t}\n-\tif (close(pipe_feed[0])) {\n+\tif (close(async.out)) {\n \t\terror(\"read from external filter %s failed\", cmd);\n \t\tfree(dst);\n \t\tdst = NULL;\n \t}\n \n-\tstatus = finish_command(&child_process);\n-\tif (status) {\n-\t\terror(\"external filter %s failed %d\", cmd, -status);\n+\tif (finish_async(&async)) {\n+\t\terror(\"external filter %s failed\", cmd);\n \t\tfree(dst);\n \t\tdst = NULL;\n \t}\n-- \n1.5.3.2.141.g48f10\n"}]}