{"thread":{"id":"45630","subject":"[PATCH v5 0/8] refactor the filter process code into a reusable module","startedAt":"2017-04-07T12:04:27Z","lastAt":"2017-04-20T17:27:55Z","messageCount":27,"participants":["Ben Peart","Lars Schneider","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":5,"patchTotal":8},"messages":[{"id":"316338","messageId":"20170407120354.17736-1-benpeart@microsoft.com","threadId":"45630","inReplyTo":null,"subject":"[PATCH v5 0/8] refactor the filter process code into a reusable module","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-04-07T12:03:46Z","receivedAt":"2017-04-07T12:04:27Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"Hopefully final spin. Updates patch 6 to rename additional instances\nof \"filter\" in comment and error messages.\n\nRefactor the filter..process code into a separate sub-process\nmodule that can be used to reduce the cost of starting up a sub-process\nfor multiple commands. It does this by keeping the external process\nrunning and processing all commands by communicating over standard input\nand standard output using the packet format (pkt-line) based protocol.\nFull documentation is in Documentation/technical/api-sub-process.txt.\n\nThis code is refactored from:\n\n\tCommit edcc85814c (\"convert: add filter..process option\", 2016-10-16)\n\tkeeps the external process running and processes all commands\n\nBen Peart (8):\n pkt-line: add packet_read_line_gently()\n convert: move packet_write_list() into pkt-line as packet_writel()\n convert: Split start_multi_file_filter into two separate functions\n convert: Separate generic structures and variables from the filter\n specific ones\n convert: Update generic functions to only use generic data structures\n convert: rename reusable sub-process functions\n sub-process: move sub-process functions into separate files\n convert: Update subprocess_read_status to not die on EOF\n\n Documentation/technical/api-sub-process.txt | 54 ++++++++++\n Makefile | 1 +\n convert.c | 159 +++++-----------------------\n pkt-line.c | 31 ++++++\n pkt-line.h | 11 ++\n sub-process.c | 120 +++++++++++++++++++++\n sub-process.h | 46 ++++++++\n 7 files changed, 292 insertions(+), 130 deletions(-)\n create mode 100644 Documentation/technical/api-sub-process.txt\n create mode 100644 sub-process.c\n create mode 100644 sub-process.h\n\n-- \n2.12.0.windows.1.31.g1548525701.dirty\n\n"},{"id":"316339","messageId":"20170407120354.17736-2-benpeart@microsoft.com","threadId":"45630","inReplyTo":"20170407120354.17736-1-benpeart@microsoft.com","subject":"[PATCH v5 1/8] pkt-line: add packet_read_line_gently()","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-04-07T12:03:47Z","receivedAt":"2017-04-07T12:04:40Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"Add packet_read_line_gently() to enable reading a line without dying on\nEOF.\n\nSigned-off-by: Ben Peart \n---\n pkt-line.c | 12 ++++++++++++\n pkt-line.h | 10 ++++++++++\n 2 files changed, 22 insertions(+)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex d4b6bfe076..58842544b4 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -323,6 +323,18 @@ char *packet_read_line(int fd, int *len_p)\n \treturn packet_read_line_generic(fd, NULL, NULL, len_p);\n }\n \n+int packet_read_line_gently(int fd, int *dst_len, char** dst_line)\n+{\n+\tint len = packet_read(fd, NULL, NULL,\n+\t\tpacket_buffer, sizeof(packet_buffer),\n+\t\tPACKET_READ_CHOMP_NEWLINE|PACKET_READ_GENTLE_ON_EOF);\n+\tif (dst_len)\n+\t\t*dst_len = len;\n+\tif (dst_line)\n+\t\t*dst_line = (len > 0) ? packet_buffer : NULL;\n+\treturn len;\n+}\n+\n char *packet_read_line_buf(char **src, size_t *src_len, int *dst_len)\n {\n \treturn packet_read_line_generic(-1, src, src_len, dst_len);\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 18eac64830..12b18991f6 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -74,6 +74,16 @@ int packet_read(int fd, char **src_buffer, size_t *src_len, char\n char *packet_read_line(int fd, int *size);\n \n /*\n+ * Convenience wrapper for packet_read that sets the PACKET_READ_GENTLE_ON_EOF\n+ * and CHOMP_NEWLINE options. The return value specifies the number of bytes\n+ * read into the buffer or -1 on truncated input. if the *dst_line parameter\n+ * is not NULL it will return NULL for a flush packet and otherwise points to\n+ * a static buffer (that may be overwritten by subsequent calls). If the size\n+ * parameter is not NULL, the length of the packet is written to it.\n+ */\n+int packet_read_line_gently(int fd, int *size, char** dst_line);\n+\n+/*\n * Same as packet_read_line, but read from a buf rather than a descriptor;\n * see packet_read for details on how src_* is used.\n */\n-- \n2.12.0.windows.1.31.g1548525701.dirty\n\n"},{"id":"316340","messageId":"20170407120354.17736-3-benpeart@microsoft.com","threadId":"45630","inReplyTo":"20170407120354.17736-1-benpeart@microsoft.com","subject":"[PATCH v5 2/8] convert: move packet_write_list() into pkt-line as packet_writel()","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-04-07T12:03:48Z","receivedAt":"2017-04-07T12:04:42Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"Add packet_writel() which writes multiple lines in a single call and\nthen calls packet_flush_gently(). Update convert.c to use the new\npacket_writel() function from pkt-line.\n\nSigned-off-by: Ben Peart \n---\n convert.c | 23 ++---------------------\n pkt-line.c | 19 +++++++++++++++++++\n pkt-line.h | 1 +\n 3 files changed, 22 insertions(+), 21 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 8d652bf27c..793c29ebfd 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -521,25 +521,6 @@ static struct cmd2process *find_multi_file_filter_entry(struct hashmap *hashmap,\n \treturn hashmap_get(hashmap, &key, NULL);\n }\n \n-static int packet_write_list(int fd, const char *line, ...)\n-{\n-\tva_list args;\n-\tint err;\n-\tva_start(args, line);\n-\tfor (;;) {\n-\t\tif (!line)\n-\t\t\tbreak;\n-\t\tif (strlen(line) > LARGE_PACKET_DATA_MAX)\n-\t\t\treturn -1;\n-\t\terr = packet_write_fmt_gently(fd, \"%s\\n\", line);\n-\t\tif (err)\n-\t\t\treturn err;\n-\t\tline = va_arg(args, const char*);\n-\t}\n-\tva_end(args);\n-\treturn packet_flush_gently(fd);\n-}\n-\n static void read_multi_file_filter_status(int fd, struct strbuf *status)\n {\n \tstruct strbuf **pair;\n@@ -616,7 +597,7 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons\n \n \tsigchain_push(SIGPIPE, SIG_IGN);\n \n-\terr = packet_write_list(process->in, \"git-filter-client\", \"version=2\", NULL);\n+\terr = packet_writel(process->in, \"git-filter-client\", \"version=2\", NULL);\n \tif (err)\n \t\tgoto done;\n \n@@ -632,7 +613,7 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons\n \tif (err)\n \t\tgoto done;\n \n-\terr = packet_write_list(process->in, \"capability=clean\", \"capability=smudge\", NULL);\n+\terr = packet_writel(process->in, \"capability=clean\", \"capability=smudge\", NULL);\n \n \tfor (;;) {\n \t\tcap_buf = packet_read_line(process->out, NULL);\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 58842544b4..2788aa1af6 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -171,6 +171,25 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)\n \treturn status;\n }\n \n+int packet_writel(int fd, const char *line, ...)\n+{\n+\tva_list args;\n+\tint err;\n+\tva_start(args, line);\n+\tfor (;;) {\n+\t\tif (!line)\n+\t\t\tbreak;\n+\t\tif (strlen(line) > LARGE_PACKET_DATA_MAX)\n+\t\t\treturn -1;\n+\t\terr = packet_write_fmt_gently(fd, \"%s\\n\", line);\n+\t\tif (err)\n+\t\t\treturn err;\n+\t\tline = va_arg(args, const char*);\n+\t}\n+\tva_end(args);\n+\treturn packet_flush_gently(fd);\n+}\n+\n static int packet_write_gently(const int fd_out, const char *buf, size_t size)\n {\n \tstatic char packet_write_buffer[LARGE_PACKET_MAX];\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 12b18991f6..cb3eda9695 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -25,6 +25,7 @@ void packet_buf_flush(struct strbuf *buf);\n void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n int packet_flush_gently(int fd);\n int packet_write_fmt_gently(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n+int packet_writel(int fd, const char *line, ...);\n int write_packetized_from_fd(int fd_in, int fd_out);\n int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);\n \n-- \n2.12.0.windows.1.31.g1548525701.dirty\n\n"},{"id":"316341","messageId":"20170407120354.17736-4-benpeart@microsoft.com","threadId":"45630","inReplyTo":"20170407120354.17736-1-benpeart@microsoft.com","subject":"[PATCH v5 3/8] convert: Split start_multi_file_filter into two separate functions","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-04-07T12:03:49Z","receivedAt":"2017-04-07T12:04:44Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"To enable future reuse of the filter..process infrastructure,\nsplit start_multi_file_filter into two separate parts.\n\nstart_multi_file_filter will now only contain the generic logic to\nmanage the creation and tracking of the child process in a hashmap.\n\nstart_multi_file_filter_fn is a protocol specific initialization\nfunction that will negotiate the multi-file-filter interface version\nand capabilities.\n\nSigned-off-by: Ben Peart \n---\n convert.c | 63 ++++++++++++++++++++++++++++++++++++++-------------------------\n 1 file changed, 38 insertions(+), 25 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 793c29ebfd..404757eac9 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -565,35 +565,14 @@ static void stop_multi_file_filter(struct child_process *process)\n \tfinish_command(process);\n }\n \n-static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, const char *cmd)\n+static int start_multi_file_filter_fn(struct cmd2process *entry)\n {\n \tint err;\n-\tstruct cmd2process *entry;\n-\tstruct child_process *process;\n-\tconst char *argv[] = { cmd, NULL };\n \tstruct string_list cap_list = STRING_LIST_INIT_NODUP;\n \tchar *cap_buf;\n \tconst char *cap_name;\n-\n-\tentry = xmalloc(sizeof(*entry));\n-\tentry->cmd = cmd;\n-\tentry->supported_capabilities = 0;\n-\tprocess = &entry->process;\n-\n-\tchild_process_init(process);\n-\tprocess->argv = argv;\n-\tprocess->use_shell = 1;\n-\tprocess->in = -1;\n-\tprocess->out = -1;\n-\tprocess->clean_on_exit = 1;\n-\tprocess->clean_on_exit_handler = stop_multi_file_filter;\n-\n-\tif (start_command(process)) {\n-\t\terror(\"cannot fork to run external filter '%s'\", cmd);\n-\t\treturn NULL;\n-\t}\n-\n-\thashmap_entry_init(entry, strhash(cmd));\n+\tstruct child_process *process = &entry->process;\n+\tconst char *cmd = entry->cmd;\n \n \tsigchain_push(SIGPIPE, SIG_IGN);\n \n@@ -642,7 +621,41 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons\n done:\n \tsigchain_pop(SIGPIPE);\n \n-\tif (err || errno == EPIPE) {\n+\tif (err || errno == EPIPE)\n+\t\terr = err ? err : errno;\n+\n+\treturn err;\n+}\n+\n+static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, const char *cmd)\n+{\n+\tint err;\n+\tstruct cmd2process *entry;\n+\tstruct child_process *process;\n+\tconst char *argv[] = { cmd, NULL };\n+\n+\tentry = xmalloc(sizeof(*entry));\n+\tentry->cmd = cmd;\n+\tentry->supported_capabilities = 0;\n+\tprocess = &entry->process;\n+\n+\tchild_process_init(process);\n+\tprocess->argv = argv;\n+\tprocess->use_shell = 1;\n+\tprocess->in = -1;\n+\tprocess->out = -1;\n+\tprocess->clean_on_exit = 1;\n+\tprocess->clean_on_exit_handler = stop_multi_file_filter;\n+\n+\tif (start_command(process)) {\n+\t\terror(\"cannot fork to run external filter '%s'\", cmd);\n+\t\treturn NULL;\n+\t}\n+\n+\thashmap_entry_init(entry, strhash(cmd));\n+\n+\terr = start_multi_file_filter_fn(entry);\n+\tif (err) {\n \t\terror(\"initialization for external filter '%s' failed\", cmd);\n \t\tkill_multi_file_filter(hashmap, entry);\n \t\treturn NULL;\n-- \n2.12.0.windows.1.31.g1548525701.dirty\n\n"},{"id":"316342","messageId":"20170407120354.17736-7-benpeart@microsoft.com","threadId":"45630","inReplyTo":"20170407120354.17736-1-benpeart@microsoft.com","subject":"[PATCH v5 6/8] convert: rename reusable sub-process functions","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-04-07T12:03:52Z","receivedAt":"2017-04-07T12:04:45Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"Do a mechanical rename of the functions that will become the reusable\nsub-process module.\n\nSigned-off-by: Ben Peart \n---\n convert.c | 53 +++++++++++++++++++++++++++--------------------------\n 1 file changed, 27 insertions(+), 26 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 747c0c363b..235a6a5279 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -507,8 +507,8 @@ struct cmd2process {\n \tunsigned int supported_capabilities;\n };\n \n-static int cmd_process_map_initialized;\n-static struct hashmap cmd_process_map;\n+static int subprocess_map_initialized;\n+static struct hashmap subprocess_map;\n \n static int cmd2process_cmp(const struct subprocess_entry *e1,\n \t\t\t const struct subprocess_entry *e2,\n@@ -517,22 +517,22 @@ static int cmd2process_cmp(const struct subprocess_entry *e1,\n \treturn strcmp(e1->cmd, e2->cmd);\n }\n \n-static struct subprocess_entry *find_multi_file_filter_entry(const char *cmd)\n+static struct subprocess_entry *subprocess_find_entry(const char *cmd)\n {\n \tstruct subprocess_entry key;\n \n-\tif (!cmd_process_map_initialized) {\n-\t\tcmd_process_map_initialized = 1;\n-\t\thashmap_init(&cmd_process_map, (hashmap_cmp_fn)cmd2process_cmp, 0);\n+\tif (!subprocess_map_initialized) {\n+\t\tsubprocess_map_initialized = 1;\n+\t\thashmap_init(&subprocess_map, (hashmap_cmp_fn)cmd2process_cmp, 0);\n \t\treturn NULL;\n \t}\n \n \thashmap_entry_init(&key, strhash(cmd));\n \tkey.cmd = cmd;\n-\treturn hashmap_get(&cmd_process_map, &key, NULL);\n+\treturn hashmap_get(&subprocess_map, &key, NULL);\n }\n \n-static void read_multi_file_filter_status(int fd, struct strbuf *status)\n+static void subprocess_read_status(int fd, struct strbuf *status)\n {\n \tstruct strbuf **pair;\n \tchar *line;\n@@ -552,7 +552,7 @@ static void read_multi_file_filter_status(int fd, struct strbuf *status)\n \t}\n }\n \n-static void kill_multi_file_filter(struct subprocess_entry *entry)\n+static void subprocess_stop(struct subprocess_entry *entry)\n {\n \tif (!entry)\n \t\treturn;\n@@ -561,14 +561,14 @@ static void kill_multi_file_filter(struct subprocess_entry *entry)\n \tkill(entry->process.pid, SIGTERM);\n \tfinish_command(&entry->process);\n \n-\thashmap_remove(&cmd_process_map, entry, NULL);\n+\thashmap_remove(&subprocess_map, entry, NULL);\n \tfree(entry);\n }\n \n-static void stop_multi_file_filter(struct child_process *process)\n+static void subprocess_exit_handler(struct child_process *process)\n {\n \tsigchain_push(SIGPIPE, SIG_IGN);\n-\t/* Closing the pipe signals the filter to initiate a shutdown. */\n+\t/* Closing the pipe signals the subprocess to initiate a shutdown. */\n \tclose(process->in);\n \tclose(process->out);\n \tsigchain_pop(SIGPIPE);\n@@ -640,16 +640,16 @@ static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)\n }\n \n typedef int(*subprocess_start_fn)(struct subprocess_entry *entry);\n-int start_multi_file_filter(struct subprocess_entry *entry, const char *cmd,\n+int subprocess_start(struct subprocess_entry *entry, const char *cmd,\n \tsubprocess_start_fn startfn)\n {\n \tint err;\n \tstruct child_process *process;\n \tconst char *argv[] = { cmd, NULL };\n \n-\tif (!cmd_process_map_initialized) {\n-\t\tcmd_process_map_initialized = 1;\n-\t\thashmap_init(&cmd_process_map, (hashmap_cmp_fn)cmd2process_cmp, 0);\n+\tif (!subprocess_map_initialized) {\n+\t\tsubprocess_map_initialized = 1;\n+\t\thashmap_init(&subprocess_map, (hashmap_cmp_fn)cmd2process_cmp, 0);\n \t}\n \n \tentry->cmd = cmd;\n@@ -661,11 +661,11 @@ int start_multi_file_filter(struct subprocess_entry *entry, const char *cmd,\n \tprocess->in = -1;\n \tprocess->out = -1;\n \tprocess->clean_on_exit = 1;\n-\tprocess->clean_on_exit_handler = stop_multi_file_filter;\n+\tprocess->clean_on_exit_handler = subprocess_exit_handler;\n \n \terr = start_command(process);\n \tif (err) {\n-\t\terror(\"cannot fork to run external filter '%s'\", cmd);\n+\t\terror(\"cannot fork to run subprocess '%s'\", cmd);\n \t\treturn err;\n \t}\n \n@@ -673,12 +673,12 @@ int start_multi_file_filter(struct subprocess_entry *entry, const char *cmd,\n \n \terr = startfn(entry);\n \tif (err) {\n-\t\terror(\"initialization for external filter '%s' failed\", cmd);\n-\t\tkill_multi_file_filter(entry);\n+\t\terror(\"initialization for subprocess '%s' failed\", cmd);\n+\t\tsubprocess_stop(entry);\n \t\treturn err;\n \t}\n \n-\thashmap_add(&cmd_process_map, entry);\n+\thashmap_add(&subprocess_map, entry);\n \treturn 0;\n }\n \n@@ -693,7 +693,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n \tstruct strbuf filter_status = STRBUF_INIT;\n \tconst char *filter_type;\n \n-\tentry = (struct cmd2process *)find_multi_file_filter_entry(cmd);\n+\tentry = (struct cmd2process *)subprocess_find_entry(cmd);\n \n \tfflush(NULL);\n \n@@ -701,7 +701,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n \t\tentry = xmalloc(sizeof(*entry));\n \t\tentry->supported_capabilities = 0;\n \n-\t\tif (start_multi_file_filter(&entry->subprocess, cmd, start_multi_file_filter_fn)) {\n+\t\tif (subprocess_start(&entry->subprocess, cmd, start_multi_file_filter_fn)) {\n \t\t\tfree(entry);\n \t\t\treturn 0;\n \t\t}\n@@ -746,7 +746,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n \tif (err)\n \t\tgoto done;\n \n-\tread_multi_file_filter_status(process->out, &filter_status);\n+\tsubprocess_read_status(process->out, &filter_status);\n \terr = strcmp(filter_status.buf, \"success\");\n \tif (err)\n \t\tgoto done;\n@@ -755,7 +755,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n \tif (err)\n \t\tgoto done;\n \n-\tread_multi_file_filter_status(process->out, &filter_status);\n+\tsubprocess_read_status(process->out, &filter_status);\n \terr = strcmp(filter_status.buf, \"success\");\n \n done:\n@@ -777,7 +777,8 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n \t\t\t * Force shutdown and restart if another blob requires filtering.\n \t\t\t */\n \t\t\terror(\"external filter '%s' failed\", cmd);\n-\t\t\tkill_multi_file_filter((struct subprocess_entry *)entry);\n+\t\t\tsubprocess_stop((struct subprocess_entry *)entry);\n+\t\t\tfree(entry);\n \t\t}\n \t} else {\n \t\tstrbuf_swap(dst, &nbuf);\n-- \n2.12.0.windows.1.31.g1548525701.dirty\n\n"},{"id":"316343","messageId":"20170407120354.17736-8-benpeart@microsoft.com","threadId":"45630","inReplyTo":"20170407120354.17736-1-benpeart@microsoft.com","subject":"[PATCH v5 7/8] sub-process: move sub-process functions into separate files","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-04-07T12:03:53Z","receivedAt":"2017-04-07T12:04:47Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"Move the sub-proces functions into sub-process.h/c. Add documentation\nfor the new module in Documentation/technical/api-sub-process.txt\n\nSigned-off-by: Ben Peart \n---\n Documentation/technical/api-sub-process.txt | 54 +++++++++++++\n Makefile | 1 +\n convert.c | 119 +---------------------------\n sub-process.c | 116 +++++++++++++++++++++++++++\n sub-process.h | 46 +++++++++++\n 5 files changed, 218 insertions(+), 118 deletions(-)\n create mode 100644 Documentation/technical/api-sub-process.txt\n create mode 100644 sub-process.c\n create mode 100644 sub-process.h\n\ndiff --git a/Documentation/technical/api-sub-process.txt b/Documentation/technical/api-sub-process.txt\nnew file mode 100644\nindex 0000000000..eb5005aa72\n--- /dev/null\n+++ b/Documentation/technical/api-sub-process.txt\n@@ -0,0 +1,54 @@\n+sub-process API\n+===============\n+\n+The sub-process API makes it possible to run background sub-processes\n+that should run until the git command exits and communicate with it\n+through stdin and stdout. This reduces the overhead of having to fork\n+a new process each time it needs to be communicated with.\n+\n+The sub-processes are kept in a hashmap by command name and looked up\n+via the subprocess_find_entry function. If an existing instance can not\n+be found then a new process should be created and started. When the\n+parent git command terminates, all sub-processes are also terminated.\n+\n+This API is based on the run-command API.\n+\n+Data structures\n+---------------\n+\n+* `struct subprocess_entry`\n+\n+The sub-process structure. Members should not be accessed directly.\n+\n+Types\n+-----\n+\n+'int(*subprocess_start_fn)(struct subprocess_entry *entry)'::\n+\n+\tUser-supplied function to initialize the sub-process. This is\n+\ttypically used to negoiate the interface version and capabilities.\n+\n+\n+Functions\n+---------\n+\n+`subprocess_start`::\n+\n+\tStart a subprocess and add it to the subprocess hashmap.\n+\n+`subprocess_stop`::\n+\n+\tKill a subprocess and remove it from the subprocess hashmap.\n+\n+`subprocess_find_entry`::\n+\n+\tFind a subprocess in the subprocess hashmap.\n+\n+`subprocess_get_child_process`::\n+\n+\tGet the underlying `struct child_process` from a subprocess.\n+\n+`subprocess_read_status`::\n+\n+\tHelper function to read packets looking for the last \"status=\"\n+\tkey/value pair.\ndiff --git a/Makefile b/Makefile\nindex 9f8b35ad41..add945b560 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -838,6 +838,7 @@ LIB_OBJS += streaming.o\n LIB_OBJS += string-list.o\n LIB_OBJS += submodule.o\n LIB_OBJS += submodule-config.o\n+LIB_OBJS += sub-process.o\n LIB_OBJS += symlinks.o\n LIB_OBJS += tag.o\n LIB_OBJS += tempfile.o\ndiff --git a/convert.c b/convert.c\nindex 235a6a5279..baa41da760 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -4,6 +4,7 @@\n #include \"quote.h\"\n #include \"sigchain.h\"\n #include \"pkt-line.h\"\n+#include \"sub-process.h\"\n \n /*\n * convert.c - convert a file when checking it out and checking it in.\n@@ -496,86 +497,11 @@ static int apply_single_file_filter(const char *path, const char *src, size_t le\n #define CAP_CLEAN (1u<<0)\n #define CAP_SMUDGE (1u<<1)\n \n-struct subprocess_entry {\n-\tstruct hashmap_entry ent; /* must be the first member! */\n-\tconst char *cmd;\n-\tstruct child_process process;\n-};\n-\n struct cmd2process {\n \tstruct subprocess_entry subprocess; /* must be the first member! */\n \tunsigned int supported_capabilities;\n };\n \n-static int subprocess_map_initialized;\n-static struct hashmap subprocess_map;\n-\n-static int cmd2process_cmp(const struct subprocess_entry *e1,\n-\t\t\t const struct subprocess_entry *e2,\n-\t\t\t const void *unused)\n-{\n-\treturn strcmp(e1->cmd, e2->cmd);\n-}\n-\n-static struct subprocess_entry *subprocess_find_entry(const char *cmd)\n-{\n-\tstruct subprocess_entry key;\n-\n-\tif (!subprocess_map_initialized) {\n-\t\tsubprocess_map_initialized = 1;\n-\t\thashmap_init(&subprocess_map, (hashmap_cmp_fn)cmd2process_cmp, 0);\n-\t\treturn NULL;\n-\t}\n-\n-\thashmap_entry_init(&key, strhash(cmd));\n-\tkey.cmd = cmd;\n-\treturn hashmap_get(&subprocess_map, &key, NULL);\n-}\n-\n-static void subprocess_read_status(int fd, struct strbuf *status)\n-{\n-\tstruct strbuf **pair;\n-\tchar *line;\n-\tfor (;;) {\n-\t\tline = packet_read_line(fd, NULL);\n-\t\tif (!line)\n-\t\t\tbreak;\n-\t\tpair = strbuf_split_str(line, '=', 2);\n-\t\tif (pair[0] && pair[0]->len && pair[1]) {\n-\t\t\t/* the last \"status=\" line wins */\n-\t\t\tif (!strcmp(pair[0]->buf, \"status=\")) {\n-\t\t\t\tstrbuf_reset(status);\n-\t\t\t\tstrbuf_addbuf(status, pair[1]);\n-\t\t\t}\n-\t\t}\n-\t\tstrbuf_list_free(pair);\n-\t}\n-}\n-\n-static void subprocess_stop(struct subprocess_entry *entry)\n-{\n-\tif (!entry)\n-\t\treturn;\n-\n-\tentry->process.clean_on_exit = 0;\n-\tkill(entry->process.pid, SIGTERM);\n-\tfinish_command(&entry->process);\n-\n-\thashmap_remove(&subprocess_map, entry, NULL);\n-\tfree(entry);\n-}\n-\n-static void subprocess_exit_handler(struct child_process *process)\n-{\n-\tsigchain_push(SIGPIPE, SIG_IGN);\n-\t/* Closing the pipe signals the subprocess to initiate a shutdown. */\n-\tclose(process->in);\n-\tclose(process->out);\n-\tsigchain_pop(SIGPIPE);\n-\t/* Finish command will wait until the shutdown is complete. */\n-\tfinish_command(process);\n-}\n-\n static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)\n {\n \tint err;\n@@ -639,49 +565,6 @@ static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)\n \treturn err;\n }\n \n-typedef int(*subprocess_start_fn)(struct subprocess_entry *entry);\n-int subprocess_start(struct subprocess_entry *entry, const char *cmd,\n-\tsubprocess_start_fn startfn)\n-{\n-\tint err;\n-\tstruct child_process *process;\n-\tconst char *argv[] = { cmd, NULL };\n-\n-\tif (!subprocess_map_initialized) {\n-\t\tsubprocess_map_initialized = 1;\n-\t\thashmap_init(&subprocess_map, (hashmap_cmp_fn)cmd2process_cmp, 0);\n-\t}\n-\n-\tentry->cmd = cmd;\n-\tprocess = &entry->process;\n-\n-\tchild_process_init(process);\n-\tprocess->argv = argv;\n-\tprocess->use_shell = 1;\n-\tprocess->in = -1;\n-\tprocess->out = -1;\n-\tprocess->clean_on_exit = 1;\n-\tprocess->clean_on_exit_handler = subprocess_exit_handler;\n-\n-\terr = start_command(process);\n-\tif (err) {\n-\t\terror(\"cannot fork to run subprocess '%s'\", cmd);\n-\t\treturn err;\n-\t}\n-\n-\thashmap_entry_init(entry, strhash(cmd));\n-\n-\terr = startfn(entry);\n-\tif (err) {\n-\t\terror(\"initialization for subprocess '%s' failed\", cmd);\n-\t\tsubprocess_stop(entry);\n-\t\treturn err;\n-\t}\n-\n-\thashmap_add(&subprocess_map, entry);\n-\treturn 0;\n-}\n-\n static int apply_multi_file_filter(const char *path, const char *src, size_t len,\n \t\t\t\t int fd, struct strbuf *dst, const char *cmd,\n \t\t\t\t const unsigned int wanted_capability)\ndiff --git a/sub-process.c b/sub-process.c\nnew file mode 100644\nindex 0000000000..60bb650012\n--- /dev/null\n+++ b/sub-process.c\n@@ -0,0 +1,116 @@\n+/*\n+ * Generic implementation of background process infrastructure.\n+ */\n+#include \"sub-process.h\"\n+#include \"sigchain.h\"\n+#include \"pkt-line.h\"\n+\n+static int subprocess_map_initialized;\n+static struct hashmap subprocess_map;\n+\n+static int cmd2process_cmp(const struct subprocess_entry *e1,\n+\t\t\t const struct subprocess_entry *e2,\n+\t\t\t const void *unused)\n+{\n+\treturn strcmp(e1->cmd, e2->cmd);\n+}\n+\n+struct subprocess_entry *subprocess_find_entry(const char *cmd)\n+{\n+\tstruct subprocess_entry key;\n+\n+\tif (!subprocess_map_initialized) {\n+\t\tsubprocess_map_initialized = 1;\n+\t\thashmap_init(&subprocess_map, (hashmap_cmp_fn)cmd2process_cmp, 0);\n+\t\treturn NULL;\n+\t}\n+\n+\thashmap_entry_init(&key, strhash(cmd));\n+\tkey.cmd = cmd;\n+\treturn hashmap_get(&subprocess_map, &key, NULL);\n+}\n+\n+void subprocess_read_status(int fd, struct strbuf *status)\n+{\n+\tstruct strbuf **pair;\n+\tchar *line;\n+\tfor (;;) {\n+\t\tline = packet_read_line(fd, NULL);\n+\t\tif (!line)\n+\t\t\tbreak;\n+\t\tpair = strbuf_split_str(line, '=', 2);\n+\t\tif (pair[0] && pair[0]->len && pair[1]) {\n+\t\t\t/* the last \"status=\" line wins */\n+\t\t\tif (!strcmp(pair[0]->buf, \"status=\")) {\n+\t\t\t\tstrbuf_reset(status);\n+\t\t\t\tstrbuf_addbuf(status, pair[1]);\n+\t\t\t}\n+\t\t}\n+\t\tstrbuf_list_free(pair);\n+\t}\n+}\n+\n+void subprocess_stop(struct subprocess_entry *entry)\n+{\n+\tif (!entry)\n+\t\treturn;\n+\n+\tentry->process.clean_on_exit = 0;\n+\tkill(entry->process.pid, SIGTERM);\n+\tfinish_command(&entry->process);\n+\n+\thashmap_remove(&subprocess_map, entry, NULL);\n+}\n+\n+static void subprocess_exit_handler(struct child_process *process)\n+{\n+\tsigchain_push(SIGPIPE, SIG_IGN);\n+\t/* Closing the pipe signals the subprocess to initiate a shutdown. */\n+\tclose(process->in);\n+\tclose(process->out);\n+\tsigchain_pop(SIGPIPE);\n+\t/* Finish command will wait until the shutdown is complete. */\n+\tfinish_command(process);\n+}\n+\n+int subprocess_start(struct subprocess_entry *entry, const char *cmd,\n+\tsubprocess_start_fn startfn)\n+{\n+\tint err;\n+\tstruct child_process *process;\n+\tconst char *argv[] = { cmd, NULL };\n+\n+\tif (!subprocess_map_initialized) {\n+\t\tsubprocess_map_initialized = 1;\n+\t\thashmap_init(&subprocess_map, (hashmap_cmp_fn)cmd2process_cmp, 0);\n+\t}\n+\n+\tentry->cmd = cmd;\n+\tprocess = &entry->process;\n+\n+\tchild_process_init(process);\n+\tprocess->argv = argv;\n+\tprocess->use_shell = 1;\n+\tprocess->in = -1;\n+\tprocess->out = -1;\n+\tprocess->clean_on_exit = 1;\n+\tprocess->clean_on_exit_handler = subprocess_exit_handler;\n+\n+\terr = start_command(process);\n+\tif (err) {\n+\t\terror(\"cannot fork to run subprocess '%s'\", cmd);\n+\t\treturn err;\n+\t}\n+\n+\thashmap_entry_init(entry, strhash(cmd));\n+\n+\terr = startfn(entry);\n+\tif (err) {\n+\t\terror(\"initialization for subprocess '%s' failed\", cmd);\n+\t\tsubprocess_stop(entry);\n+\t\treturn err;\n+\t}\n+\n+\thashmap_add(&subprocess_map, entry);\n+\treturn 0;\n+}\ndiff --git a/sub-process.h b/sub-process.h\nnew file mode 100644\nindex 0000000000..0cf1760a0a\n--- /dev/null\n+++ b/sub-process.h\n@@ -0,0 +1,46 @@\n+#ifndef SUBPROCESS_H\n+#define SUBPROCESS_H\n+\n+#include \"git-compat-util.h\"\n+#include \"hashmap.h\"\n+#include \"run-command.h\"\n+\n+/*\n+ * Generic implementation of background process infrastructure.\n+ * See Documentation/technical/api-background-process.txt.\n+ */\n+\n+ /* data structures */\n+\n+struct subprocess_entry {\n+\tstruct hashmap_entry ent; /* must be the first member! */\n+\tconst char *cmd;\n+\tstruct child_process process;\n+};\n+\n+/* subprocess functions */\n+\n+typedef int(*subprocess_start_fn)(struct subprocess_entry *entry);\n+int subprocess_start(struct subprocess_entry *entry, const char *cmd,\n+\t\tsubprocess_start_fn startfn);\n+\n+void subprocess_stop(struct subprocess_entry *entry);\n+\n+struct subprocess_entry *subprocess_find_entry(const char *cmd);\n+\n+/* subprocess helper functions */\n+\n+static inline struct child_process *subprocess_get_child_process(\n+\t\tstruct subprocess_entry *entry)\n+{\n+\treturn &entry->process;\n+}\n+\n+/*\n+ * Helper function that will read packets looking for \"status=\"\n+ * key/value pairs and return the value from the last \"status\" packet\n+ */\n+\n+void subprocess_read_status(int fd, struct strbuf *status);\n+\n+#endif\n-- \n2.12.0.windows.1.31.g1548525701.dirty\n\n"},{"id":"316344","messageId":"20170407120354.17736-9-benpeart@microsoft.com","threadId":"45630","inReplyTo":"20170407120354.17736-1-benpeart@microsoft.com","subject":"[PATCH v5 8/8] convert: Update subprocess_read_status to not die on EOF","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-04-07T12:03:54Z","receivedAt":"2017-04-07T12:04:49Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"Enable sub-processes to gracefully handle when the process dies by\nupdating subprocess_read_status to return an error on EOF instead of\ndying.\n\nUpdate apply_multi_file_filter to take advantage of the revised\nsubprocess_read_status.\n\nSigned-off-by: Ben Peart \n---\n convert.c | 10 ++++++++--\n sub-process.c | 10 +++++++---\n sub-process.h | 2 +-\n 3 files changed, 16 insertions(+), 6 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex baa41da760..9e181e27ad 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -629,7 +629,10 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n \tif (err)\n \t\tgoto done;\n \n-\tsubprocess_read_status(process->out, &filter_status);\n+\terr = subprocess_read_status(process->out, &filter_status);\n+\tif (err)\n+\t\tgoto done;\n+\n \terr = strcmp(filter_status.buf, \"success\");\n \tif (err)\n \t\tgoto done;\n@@ -638,7 +641,10 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n \tif (err)\n \t\tgoto done;\n \n-\tsubprocess_read_status(process->out, &filter_status);\n+\terr = subprocess_read_status(process->out, &filter_status);\n+\tif (err)\n+\t\tgoto done;\n+\n \terr = strcmp(filter_status.buf, \"success\");\n \n done:\ndiff --git a/sub-process.c b/sub-process.c\nindex 60bb650012..c5057cafcd 100644\n--- a/sub-process.c\n+++ b/sub-process.c\n@@ -30,13 +30,15 @@ struct subprocess_entry *subprocess_find_entry(const char *cmd)\n \treturn hashmap_get(&subprocess_map, &key, NULL);\n }\n \n-void subprocess_read_status(int fd, struct strbuf *status)\n+int subprocess_read_status(int fd, struct strbuf *status)\n {\n \tstruct strbuf **pair;\n \tchar *line;\n+\tint len;\n+\n \tfor (;;) {\n-\t\tline = packet_read_line(fd, NULL);\n-\t\tif (!line)\n+\t\tlen = packet_read_line_gently(fd, NULL, &line);\n+\t\tif ((len == -1) || !line)\n \t\t\tbreak;\n \t\tpair = strbuf_split_str(line, '=', 2);\n \t\tif (pair[0] && pair[0]->len && pair[1]) {\n@@ -48,6 +50,8 @@ void subprocess_read_status(int fd, struct strbuf *status)\n \t\t}\n \t\tstrbuf_list_free(pair);\n \t}\n+\n+\treturn len == -1 ? len : 0;\n }\n \n void subprocess_stop(struct subprocess_entry *entry)\ndiff --git a/sub-process.h b/sub-process.h\nindex 0cf1760a0a..5a1eeeece0 100644\n--- a/sub-process.h\n+++ b/sub-process.h\n@@ -41,6 +41,6 @@ static inline struct child_process *subprocess_get_child_process(\n * key/value pairs and return the value from the last \"status\" packet\n */\n \n-void subprocess_read_status(int fd, struct strbuf *status);\n+int subprocess_read_status(int fd, struct strbuf *status);\n \n #endif\n-- \n2.12.0.windows.1.31.g1548525701.dirty\n\n"},{"id":"316345","messageId":"20170407120354.17736-5-benpeart@microsoft.com","threadId":"45630","inReplyTo":"20170407120354.17736-1-benpeart@microsoft.com","subject":"[PATCH v5 4/8] convert: Separate generic structures and variables from the filter specific ones","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-04-07T12:03:50Z","receivedAt":"2017-04-07T12:04:50Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"To enable future reuse of the filter..process infrastructure,\nsplit the cmd2process structure into two separate parts.\n\nsubprocess_entry will now contain the generic data required to manage\nthe creation and tracking of the child process in a hashmap. Also move\nall knowledge of the hashmap into the generic functions.\n\ncmd2process is a filter protocol specific structure that is used to\ntrack the negotiated capabilities of the filter.\n\nSigned-off-by: Ben Peart \n---\n convert.c | 57 +++++++++++++++++++++++++++++++--------------------------\n 1 file changed, 31 insertions(+), 26 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 404757eac9..f569026511 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -496,29 +496,40 @@ static int apply_single_file_filter(const char *path, const char *src, size_t le\n #define CAP_CLEAN (1u<<0)\n #define CAP_SMUDGE (1u<<1)\n \n-struct cmd2process {\n+struct subprocess_entry {\n \tstruct hashmap_entry ent; /* must be the first member! */\n-\tunsigned int supported_capabilities;\n \tconst char *cmd;\n \tstruct child_process process;\n };\n \n+struct cmd2process {\n+\tstruct subprocess_entry subprocess; /* must be the first member! */\n+\tunsigned int supported_capabilities;\n+};\n+\n static int cmd_process_map_initialized;\n static struct hashmap cmd_process_map;\n \n-static int cmd2process_cmp(const struct cmd2process *e1,\n-\t\t\t const struct cmd2process *e2,\n+static int cmd2process_cmp(const struct subprocess_entry *e1,\n+\t\t\t const struct subprocess_entry *e2,\n \t\t\t const void *unused)\n {\n \treturn strcmp(e1->cmd, e2->cmd);\n }\n \n-static struct cmd2process *find_multi_file_filter_entry(struct hashmap *hashmap, const char *cmd)\n+static struct subprocess_entry *find_multi_file_filter_entry(const char *cmd)\n {\n-\tstruct cmd2process key;\n+\tstruct subprocess_entry key;\n+\n+\tif (!cmd_process_map_initialized) {\n+\t\tcmd_process_map_initialized = 1;\n+\t\thashmap_init(&cmd_process_map, (hashmap_cmp_fn)cmd2process_cmp, 0);\n+\t\treturn NULL;\n+\t}\n+\n \thashmap_entry_init(&key, strhash(cmd));\n \tkey.cmd = cmd;\n-\treturn hashmap_get(hashmap, &key, NULL);\n+\treturn hashmap_get(&cmd_process_map, &key, NULL);\n }\n \n static void read_multi_file_filter_status(int fd, struct strbuf *status)\n@@ -541,7 +552,7 @@ static void read_multi_file_filter_status(int fd, struct strbuf *status)\n \t}\n }\n \n-static void kill_multi_file_filter(struct hashmap *hashmap, struct cmd2process *entry)\n+static void kill_multi_file_filter(struct subprocess_entry *entry)\n {\n \tif (!entry)\n \t\treturn;\n@@ -550,7 +561,7 @@ static void kill_multi_file_filter(struct hashmap *hashmap, struct cmd2process *\n \tkill(entry->process.pid, SIGTERM);\n \tfinish_command(&entry->process);\n \n-\thashmap_remove(hashmap, entry, NULL);\n+\thashmap_remove(&cmd_process_map, entry, NULL);\n \tfree(entry);\n }\n \n@@ -571,8 +582,8 @@ static int start_multi_file_filter_fn(struct cmd2process *entry)\n \tstruct string_list cap_list = STRING_LIST_INIT_NODUP;\n \tchar *cap_buf;\n \tconst char *cap_name;\n-\tstruct child_process *process = &entry->process;\n-\tconst char *cmd = entry->cmd;\n+\tstruct child_process *process = &entry->subprocess.process;\n+\tconst char *cmd = entry->subprocess.cmd;\n \n \tsigchain_push(SIGPIPE, SIG_IGN);\n \n@@ -627,7 +638,7 @@ static int start_multi_file_filter_fn(struct cmd2process *entry)\n \treturn err;\n }\n \n-static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, const char *cmd)\n+static struct cmd2process *start_multi_file_filter(const char *cmd)\n {\n \tint err;\n \tstruct cmd2process *entry;\n@@ -635,9 +646,9 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons\n \tconst char *argv[] = { cmd, NULL };\n \n \tentry = xmalloc(sizeof(*entry));\n-\tentry->cmd = cmd;\n+\tentry->subprocess.cmd = cmd;\n \tentry->supported_capabilities = 0;\n-\tprocess = &entry->process;\n+\tprocess = &entry->subprocess.process;\n \n \tchild_process_init(process);\n \tprocess->argv = argv;\n@@ -657,11 +668,11 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons\n \terr = start_multi_file_filter_fn(entry);\n \tif (err) {\n \t\terror(\"initialization for external filter '%s' failed\", cmd);\n-\t\tkill_multi_file_filter(hashmap, entry);\n+\t\tkill_multi_file_filter(&entry->subprocess);\n \t\treturn NULL;\n \t}\n \n-\thashmap_add(hashmap, entry);\n+\thashmap_add(&cmd_process_map, entry);\n \treturn entry;\n }\n \n@@ -676,22 +687,16 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n \tstruct strbuf filter_status = STRBUF_INIT;\n \tconst char *filter_type;\n \n-\tif (!cmd_process_map_initialized) {\n-\t\tcmd_process_map_initialized = 1;\n-\t\thashmap_init(&cmd_process_map, (hashmap_cmp_fn) cmd2process_cmp, 0);\n-\t\tentry = NULL;\n-\t} else {\n-\t\tentry = find_multi_file_filter_entry(&cmd_process_map, cmd);\n-\t}\n+\tentry = (struct cmd2process *)find_multi_file_filter_entry(cmd);\n \n \tfflush(NULL);\n \n \tif (!entry) {\n-\t\tentry = start_multi_file_filter(&cmd_process_map, cmd);\n+\t\tentry = start_multi_file_filter(cmd);\n \t\tif (!entry)\n \t\t\treturn 0;\n \t}\n-\tprocess = &entry->process;\n+\tprocess = &entry->subprocess.process;\n \n \tif (!(wanted_capability & entry->supported_capabilities))\n \t\treturn 0;\n@@ -762,7 +767,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n \t\t\t * Force shutdown and restart if another blob requires filtering.\n \t\t\t */\n \t\t\terror(\"external filter '%s' failed\", cmd);\n-\t\t\tkill_multi_file_filter(&cmd_process_map, entry);\n+\t\t\tkill_multi_file_filter(&entry->subprocess);\n \t\t}\n \t} else {\n \t\tstrbuf_swap(dst, &nbuf);\n-- \n2.12.0.windows.1.31.g1548525701.dirty\n\n"},{"id":"316346","messageId":"20170407120354.17736-6-benpeart@microsoft.com","threadId":"45630","inReplyTo":"20170407120354.17736-1-benpeart@microsoft.com","subject":"[PATCH v5 5/8] convert: Update generic functions to only use generic data structures","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-04-07T12:03:51Z","receivedAt":"2017-04-07T12:04:52Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"Update all functions that are going to be moved into a reusable module\nso that they only work with the reusable data structures. Move code\nthat is specific to the filter out into the filter specific functions.\n\nSigned-off-by: Ben Peart \n---\n convert.c | 46 ++++++++++++++++++++++++++++------------------\n 1 file changed, 28 insertions(+), 18 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex f569026511..747c0c363b 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -576,14 +576,15 @@ static void stop_multi_file_filter(struct child_process *process)\n \tfinish_command(process);\n }\n \n-static int start_multi_file_filter_fn(struct cmd2process *entry)\n+static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)\n {\n \tint err;\n+\tstruct cmd2process *entry = (struct cmd2process *)subprocess;\n \tstruct string_list cap_list = STRING_LIST_INIT_NODUP;\n \tchar *cap_buf;\n \tconst char *cap_name;\n-\tstruct child_process *process = &entry->subprocess.process;\n-\tconst char *cmd = entry->subprocess.cmd;\n+\tstruct child_process *process = &subprocess->process;\n+\tconst char *cmd = subprocess->cmd;\n \n \tsigchain_push(SIGPIPE, SIG_IGN);\n \n@@ -638,17 +639,21 @@ static int start_multi_file_filter_fn(struct cmd2process *entry)\n \treturn err;\n }\n \n-static struct cmd2process *start_multi_file_filter(const char *cmd)\n+typedef int(*subprocess_start_fn)(struct subprocess_entry *entry);\n+int start_multi_file_filter(struct subprocess_entry *entry, const char *cmd,\n+\tsubprocess_start_fn startfn)\n {\n \tint err;\n-\tstruct cmd2process *entry;\n \tstruct child_process *process;\n \tconst char *argv[] = { cmd, NULL };\n \n-\tentry = xmalloc(sizeof(*entry));\n-\tentry->subprocess.cmd = cmd;\n-\tentry->supported_capabilities = 0;\n-\tprocess = &entry->subprocess.process;\n+\tif (!cmd_process_map_initialized) {\n+\t\tcmd_process_map_initialized = 1;\n+\t\thashmap_init(&cmd_process_map, (hashmap_cmp_fn)cmd2process_cmp, 0);\n+\t}\n+\n+\tentry->cmd = cmd;\n+\tprocess = &entry->process;\n \n \tchild_process_init(process);\n \tprocess->argv = argv;\n@@ -658,22 +663,23 @@ static struct cmd2process *start_multi_file_filter(const char *cmd)\n \tprocess->clean_on_exit = 1;\n \tprocess->clean_on_exit_handler = stop_multi_file_filter;\n \n-\tif (start_command(process)) {\n+\terr = start_command(process);\n+\tif (err) {\n \t\terror(\"cannot fork to run external filter '%s'\", cmd);\n-\t\treturn NULL;\n+\t\treturn err;\n \t}\n \n \thashmap_entry_init(entry, strhash(cmd));\n \n-\terr = start_multi_file_filter_fn(entry);\n+\terr = startfn(entry);\n \tif (err) {\n \t\terror(\"initialization for external filter '%s' failed\", cmd);\n-\t\tkill_multi_file_filter(&entry->subprocess);\n-\t\treturn NULL;\n+\t\tkill_multi_file_filter(entry);\n+\t\treturn err;\n \t}\n \n \thashmap_add(&cmd_process_map, entry);\n-\treturn entry;\n+\treturn 0;\n }\n \n static int apply_multi_file_filter(const char *path, const char *src, size_t len,\n@@ -692,9 +698,13 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n \tfflush(NULL);\n \n \tif (!entry) {\n-\t\tentry = start_multi_file_filter(cmd);\n-\t\tif (!entry)\n+\t\tentry = xmalloc(sizeof(*entry));\n+\t\tentry->supported_capabilities = 0;\n+\n+\t\tif (start_multi_file_filter(&entry->subprocess, cmd, start_multi_file_filter_fn)) {\n+\t\t\tfree(entry);\n \t\t\treturn 0;\n+\t\t}\n \t}\n \tprocess = &entry->subprocess.process;\n \n@@ -767,7 +777,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n \t\t\t * Force shutdown and restart if another blob requires filtering.\n \t\t\t */\n \t\t\terror(\"external filter '%s' failed\", cmd);\n-\t\t\tkill_multi_file_filter(&entry->subprocess);\n+\t\t\tkill_multi_file_filter((struct subprocess_entry *)entry);\n \t\t}\n \t} else {\n \t\tstrbuf_swap(dst, &nbuf);\n-- \n2.12.0.windows.1.31.g1548525701.dirty\n\n"},{"id":"316448","messageId":"81D3FB34-F4B6-4561-9374-77EE7B9E5BA5@gmail.com","threadId":"45630","inReplyTo":"20170407120354.17736-2-benpeart@microsoft.com","subject":"Re: [PATCH v5 1/8] pkt-line: add packet_read_line_gently()","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-04-09T19:34:56Z","receivedAt":"2017-04-09T19:35:06Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 07 Apr 2017, at 14:03, Ben Peart wrote:\n> \n> Add packet_read_line_gently() to enable reading a line without dying on\n> EOF.\n> \n> Signed-off-by: Ben Peart \n> ---\n> pkt-line.c | 12 ++++++++++++\n> pkt-line.h | 10 ++++++++++\n> 2 files changed, 22 insertions(+)\n> \n> diff --git a/pkt-line.c b/pkt-line.c\n> index d4b6bfe076..58842544b4 100644\n> --- a/pkt-line.c\n> +++ b/pkt-line.c\n> @@ -323,6 +323,18 @@ char *packet_read_line(int fd, int *len_p)\n> \treturn packet_read_line_generic(fd, NULL, NULL, len_p);\n> }\n> \n> +int packet_read_line_gently(int fd, int *dst_len, char** dst_line)\n> +{\n> +\tint len = packet_read(fd, NULL, NULL,\n> +\t\tpacket_buffer, sizeof(packet_buffer),\n> +\t\tPACKET_READ_CHOMP_NEWLINE|PACKET_READ_GENTLE_ON_EOF);\n\nReally minor nit: you could align the parameters according to fd\nsimilar to the \"packet_read_line_generic\" code (tab width 8).\n\n> +\tif (dst_len)\n> +\t\t*dst_len = len;\n> +\tif (dst_line)\n> +\t\t*dst_line = (len > 0) ? packet_buffer : NULL;\n\nMinor nit: The explicit check \"len > 0\" is necessary here as len can\nbe \"-1\". The original \"packet_read_line_generic\" just checks for \n\"len\". I think it would be nice if the code would be consistent and both\nwould check \"len > 0\".\n\n> +\treturn len;\n> +}\n> +\n> char *packet_read_line_buf(char **src, size_t *src_len, int *dst_len)\n> {\n> \treturn packet_read_line_generic(-1, src, src_len, dst_len);\n> diff --git a/pkt-line.h b/pkt-line.h\n> index 18eac64830..12b18991f6 100644\n> --- a/pkt-line.h\n> +++ b/pkt-line.h\n> @@ -74,6 +74,16 @@ int packet_read(int fd, char **src_buffer, size_t *src_len, char\n> char *packet_read_line(int fd, int *size);\n> \n> /*\n> + * Convenience wrapper for packet_read that sets the PACKET_READ_GENTLE_ON_EOF\n> + * and CHOMP_NEWLINE options. The return value specifies the number of bytes\n> + * read into the buffer or -1 on truncated input. if the *dst_line parameter\n\ns/if/If/\n\n\n> + * is not NULL it will return NULL for a flush packet and otherwise points to\n\nThis sentences is a bit confusing to me. Maybe:\n\nIf the *dst_line parameter is not NULL, then it will point to a static buffer\n(that may be overwritten by subsequent calls) or it will return NULL for a flush \npacket.\n\n... feel free to completely ignore this as I am no native speaker.\n\n> + * a static buffer (that may be overwritten by subsequent calls). If the size\n> + * parameter is not NULL, the length of the packet is written to it.\n> + */\n> +int packet_read_line_gently(int fd, int *size, char** dst_line);\n> +\n> +/*\n> * Same as packet_read_line, but read from a buf rather than a descriptor;\n> * see packet_read for details on how src_* is used.\n> */\n\nIf you send another round then you could address the minor nits.\nIf not, then this patch as it is looks good to me.\n\nThanks,\nLars"},{"id":"316449","messageId":"F62E46F7-6829-46E3-A890-106964CB632E@gmail.com","threadId":"45630","inReplyTo":"20170407120354.17736-3-benpeart@microsoft.com","subject":"Re: [PATCH v5 2/8] convert: move packet_write_list() into pkt-line as packet_writel()","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-04-09T19:43:02Z","receivedAt":"2017-04-09T19:43:10Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 07 Apr 2017, at 14:03, Ben Peart wrote:\n> \n> Add packet_writel() which writes multiple lines in a single call and\n> then calls packet_flush_gently(). Update convert.c to use the new\n> packet_writel() function from pkt-line.\n> \n> Signed-off-by: Ben Peart \n> ---\n> convert.c | 23 ++---------------------\n> pkt-line.c | 19 +++++++++++++++++++\n> pkt-line.h | 1 +\n> 3 files changed, 22 insertions(+), 21 deletions(-)\n> \n> diff --git a/convert.c b/convert.c\n> index 8d652bf27c..793c29ebfd 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -521,25 +521,6 @@ static struct cmd2process *find_multi_file_filter_entry(struct hashmap *hashmap,\n> \treturn hashmap_get(hashmap, &key, NULL);\n> }\n> \n> -static int packet_write_list(int fd, const char *line, ...)\n> -{\n> -\tva_list args;\n> -\tint err;\n> -\tva_start(args, line);\n> -\tfor (;;) {\n> -\t\tif (!line)\n> -\t\t\tbreak;\n> -\t\tif (strlen(line) > LARGE_PACKET_DATA_MAX)\n> -\t\t\treturn -1;\n> -\t\terr = packet_write_fmt_gently(fd, \"%s\\n\", line);\n> -\t\tif (err)\n> -\t\t\treturn err;\n> -\t\tline = va_arg(args, const char*);\n> -\t}\n> -\tva_end(args);\n> -\treturn packet_flush_gently(fd);\n> -}\n> -\n> static void read_multi_file_filter_status(int fd, struct strbuf *status)\n> {\n> \tstruct strbuf **pair;\n> @@ -616,7 +597,7 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons\n> \n> \tsigchain_push(SIGPIPE, SIG_IGN);\n> \n> -\terr = packet_write_list(process->in, \"git-filter-client\", \"version=2\", NULL);\n> +\terr = packet_writel(process->in, \"git-filter-client\", \"version=2\", NULL);\n> \tif (err)\n> \t\tgoto done;\n> \n> @@ -632,7 +613,7 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons\n> \tif (err)\n> \t\tgoto done;\n> \n> -\terr = packet_write_list(process->in, \"capability=clean\", \"capability=smudge\", NULL);\n> +\terr = packet_writel(process->in, \"capability=clean\", \"capability=smudge\", NULL);\n> \n> \tfor (;;) {\n> \t\tcap_buf = packet_read_line(process->out, NULL);\n> diff --git a/pkt-line.c b/pkt-line.c\n> index 58842544b4..2788aa1af6 100644\n> --- a/pkt-line.c\n> +++ b/pkt-line.c\n> @@ -171,6 +171,25 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)\n> \treturn status;\n> }\n> \n> +int packet_writel(int fd, const char *line, ...)\n> +{\n> +\tva_list args;\n> +\tint err;\n> +\tva_start(args, line);\n> +\tfor (;;) {\n> +\t\tif (!line)\n> +\t\t\tbreak;\n> +\t\tif (strlen(line) > LARGE_PACKET_DATA_MAX)\n> +\t\t\treturn -1;\n> +\t\terr = packet_write_fmt_gently(fd, \"%s\\n\", line);\n> +\t\tif (err)\n> +\t\t\treturn err;\n> +\t\tline = va_arg(args, const char*);\n> +\t}\n> +\tva_end(args);\n> +\treturn packet_flush_gently(fd);\n> +}\n> +\n> static int packet_write_gently(const int fd_out, const char *buf, size_t size)\n> {\n> \tstatic char packet_write_buffer[LARGE_PACKET_MAX];\n> diff --git a/pkt-line.h b/pkt-line.h\n> index 12b18991f6..cb3eda9695 100644\n> --- a/pkt-line.h\n> +++ b/pkt-line.h\n> @@ -25,6 +25,7 @@ void packet_buf_flush(struct strbuf *buf);\n> void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n> int packet_flush_gently(int fd);\n> int packet_write_fmt_gently(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n> +int packet_writel(int fd, const char *line, ...);\n> int write_packetized_from_fd(int fd_in, int fd_out);\n> int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);\n> \n> -- \n> 2.12.0.windows.1.31.g1548525701.dirty\n\nLooks good to me.\n\nThanks,\nLars\n"},{"id":"316450","messageId":"C977E0BA-8386-4B87-9569-A1C3B9E97B31@gmail.com","threadId":"45630","inReplyTo":"20170407120354.17736-4-benpeart@microsoft.com","subject":"Re: [PATCH v5 3/8] convert: Split start_multi_file_filter into two separate functions","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-04-09T19:56:04Z","receivedAt":"2017-04-09T19:56:13Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 07 Apr 2017, at 14:03, Ben Peart wrote:\n> \n> To enable future reuse of the filter..process infrastructure,\n> split start_multi_file_filter into two separate parts.\n> \n> start_multi_file_filter will now only contain the generic logic to\n> manage the creation and tracking of the child process in a hashmap.\n> \n> start_multi_file_filter_fn is a protocol specific initialization\n> function that will negotiate the multi-file-filter interface version\n> and capabilities.\n> \n> Signed-off-by: Ben Peart \n> ---\n> convert.c | 63 ++++++++++++++++++++++++++++++++++++++-------------------------\n> 1 file changed, 38 insertions(+), 25 deletions(-)\n> \n> diff --git a/convert.c b/convert.c\n> index 793c29ebfd..404757eac9 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -565,35 +565,14 @@ static void stop_multi_file_filter(struct child_process *process)\n> \tfinish_command(process);\n> }\n> \n> -static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, const char *cmd)\n> +static int start_multi_file_filter_fn(struct cmd2process *entry)\n> {\n> \tint err;\n> -\tstruct cmd2process *entry;\n> -\tstruct child_process *process;\n> -\tconst char *argv[] = { cmd, NULL };\n> \tstruct string_list cap_list = STRING_LIST_INIT_NODUP;\n> \tchar *cap_buf;\n> \tconst char *cap_name;\n> -\n> -\tentry = xmalloc(sizeof(*entry));\n> -\tentry->cmd = cmd;\n> -\tentry->supported_capabilities = 0;\n> -\tprocess = &entry->process;\n> -\n> -\tchild_process_init(process);\n> -\tprocess->argv = argv;\n> -\tprocess->use_shell = 1;\n> -\tprocess->in = -1;\n> -\tprocess->out = -1;\n> -\tprocess->clean_on_exit = 1;\n> -\tprocess->clean_on_exit_handler = stop_multi_file_filter;\n> -\n> -\tif (start_command(process)) {\n> -\t\terror(\"cannot fork to run external filter '%s'\", cmd);\n> -\t\treturn NULL;\n> -\t}\n> -\n> -\thashmap_entry_init(entry, strhash(cmd));\n> +\tstruct child_process *process = &entry->process;\n> +\tconst char *cmd = entry->cmd;\n> \n> \tsigchain_push(SIGPIPE, SIG_IGN);\n> \n> @@ -642,7 +621,41 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons\n> done:\n> \tsigchain_pop(SIGPIPE);\n> \n> -\tif (err || errno == EPIPE) {\n> +\tif (err || errno == EPIPE)\n> +\t\terr = err ? err : errno;\n\nNice! I should have done this here, too:\nhttps://github.com/git/git/blob/b14f27f91770e0f99f64135348977a0ce1c7993a/convert.c#L755\n\nThis is clearly a bug in my code. I'll send a patch shortly.\n\n\n> +\n> +\treturn err;\n> +}\n> +\n> +static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, const char *cmd)\n> +{\n> +\tint err;\n> +\tstruct cmd2process *entry;\n> +\tstruct child_process *process;\n> +\tconst char *argv[] = { cmd, NULL };\n> +\n> +\tentry = xmalloc(sizeof(*entry));\n> +\tentry->cmd = cmd;\n> +\tentry->supported_capabilities = 0;\n> +\tprocess = &entry->process;\n> +\n> +\tchild_process_init(process);\n> +\tprocess->argv = argv;\n> +\tprocess->use_shell = 1;\n> +\tprocess->in = -1;\n> +\tprocess->out = -1;\n> +\tprocess->clean_on_exit = 1;\n> +\tprocess->clean_on_exit_handler = stop_multi_file_filter;\n> +\n> +\tif (start_command(process)) {\n> +\t\terror(\"cannot fork to run external filter '%s'\", cmd);\n> +\t\treturn NULL;\n> +\t}\n> +\n> +\thashmap_entry_init(entry, strhash(cmd));\n> +\n> +\terr = start_multi_file_filter_fn(entry);\n> +\tif (err) {\n> \t\terror(\"initialization for external filter '%s' failed\", cmd);\n> \t\tkill_multi_file_filter(hashmap, entry);\n> \t\treturn NULL;\n> -- \n> 2.12.0.windows.1.31.g1548525701.dirty\n\nLooks good to me.\n\nThanks,\nLars\n\n"},{"id":"316467","messageId":"48FA4601-0819-4DE2-943A-7A791BA7C583@gmail.com","threadId":"45630","inReplyTo":"20170407120354.17736-5-benpeart@microsoft.com","subject":"Re: [PATCH v5 4/8] convert: Separate generic structures and variables from the filter specific ones","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-04-10T10:18:45Z","receivedAt":"2017-04-10T10:18:53Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 07 Apr 2017, at 14:03, Ben Peart wrote:\n> \n> To enable future reuse of the filter..process infrastructure,\n> split the cmd2process structure into two separate parts.\n> \n> subprocess_entry will now contain the generic data required to manage\n> the creation and tracking of the child process in a hashmap. Also move\n> all knowledge of the hashmap into the generic functions.\n> \n> cmd2process is a filter protocol specific structure that is used to\n> track the negotiated capabilities of the filter.\n> \n> Signed-off-by: Ben Peart \n> ---\n> convert.c | 57 +++++++++++++++++++++++++++++++--------------------------\n> 1 file changed, 31 insertions(+), 26 deletions(-)\n> \n> diff --git a/convert.c b/convert.c\n> index 404757eac9..f569026511 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -496,29 +496,40 @@ static int apply_single_file_filter(const char *path, const char *src, size_t le\n> #define CAP_CLEAN (1u<<0)\n> #define CAP_SMUDGE (1u<<1)\n> \n> -struct cmd2process {\n> +struct subprocess_entry {\n> \tstruct hashmap_entry ent; /* must be the first member! */\n> -\tunsigned int supported_capabilities;\n> \tconst char *cmd;\n> \tstruct child_process process;\n> };\n> \n> +struct cmd2process {\n> +\tstruct subprocess_entry subprocess; /* must be the first member! */\n> +\tunsigned int supported_capabilities;\n> +};\n> +\n> static int cmd_process_map_initialized;\n> static struct hashmap cmd_process_map;\n> \n> -static int cmd2process_cmp(const struct cmd2process *e1,\n> -\t\t\t const struct cmd2process *e2,\n> +static int cmd2process_cmp(const struct subprocess_entry *e1,\n> +\t\t\t const struct subprocess_entry *e2,\n> \t\t\t const void *unused)\n> {\n> \treturn strcmp(e1->cmd, e2->cmd);\n> }\n> \n> -static struct cmd2process *find_multi_file_filter_entry(struct hashmap *hashmap, const char *cmd)\n> +static struct subprocess_entry *find_multi_file_filter_entry(const char *cmd)\n\nI am curious why you removed the hashmap parameter (here and in other pars of this patch). \nI know the parameter is not strictly necessary as the hashmap is a global variable anyways. \nHowever, I think it eases code maintainability in the long run if a function is \"as pure as \npossible\" (IOW does rely on global state as less as possible).\n\nAs I consider this personal preference I think either way is fine.\n\n\n> {\n> -\tstruct cmd2process key;\n> +\tstruct subprocess_entry key;\n> +\n> +\tif (!cmd_process_map_initialized) {\n> +\t\tcmd_process_map_initialized = 1;\n> +\t\thashmap_init(&cmd_process_map, (hashmap_cmp_fn)cmd2process_cmp, 0);\n> +\t\treturn NULL;\n> +\t}\n> +\n\nI am a bit in doubt about this one. If the process map was not initialized yet, then\nI would expect \"find_multi_file_filter_entry\" to return NULL. Creating the hash\nmap as side effect seems a bit unexpected to me. \n\nThe rest of the patch looks good to me.\n\nThanks,\nLars"},{"id":"316482","messageId":"5900F7F1-89D9-433E-A6C3-0AB27C815BE6@gmail.com","threadId":"45630","inReplyTo":"20170407120354.17736-6-benpeart@microsoft.com","subject":"Re: [PATCH v5 5/8] convert: Update generic functions to only use generic data structures","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-04-10T12:05:52Z","receivedAt":"2017-04-10T12:05:59Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 07 Apr 2017, at 14:03, Ben Peart wrote:\n> \n> Update all functions that are going to be moved into a reusable module\n> so that they only work with the reusable data structures. Move code\n> that is specific to the filter out into the filter specific functions.\n> \n> Signed-off-by: Ben Peart \n> ---\n> convert.c | 46 ++++++++++++++++++++++++++++------------------\n> 1 file changed, 28 insertions(+), 18 deletions(-)\n> \n> diff --git a/convert.c b/convert.c\n> index f569026511..747c0c363b 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -576,14 +576,15 @@ static void stop_multi_file_filter(struct child_process *process)\n> \tfinish_command(process);\n> }\n> \n> -static int start_multi_file_filter_fn(struct cmd2process *entry)\n> +static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)\n> {\n> \tint err;\n> +\tstruct cmd2process *entry = (struct cmd2process *)subprocess;\n\nI wonder if a \"subprocess_entry\" should rather have a generic data pointer\nfor any extra information. However, this is probably bikeshedding and I think\nthis solution is equally good.\n\n\n> \tstruct string_list cap_list = STRING_LIST_INIT_NODUP;\n> \tchar *cap_buf;\n> \tconst char *cap_name;\n> -\tstruct child_process *process = &entry->subprocess.process;\n> -\tconst char *cmd = entry->subprocess.cmd;\n> +\tstruct child_process *process = &subprocess->process;\n> +\tconst char *cmd = subprocess->cmd;\n> \n> \tsigchain_push(SIGPIPE, SIG_IGN);\n> \n> @@ -638,17 +639,21 @@ static int start_multi_file_filter_fn(struct cmd2process *entry)\n> \treturn err;\n> }\n\n> ...\n\n> static int apply_multi_file_filter(const char *path, const char *src, size_t len,\n> @@ -692,9 +698,13 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n> \tfflush(NULL);\n> \n> \tif (!entry) {\n> -\t\tentry = start_multi_file_filter(cmd);\n> -\t\tif (!entry)\n> +\t\tentry = xmalloc(sizeof(*entry));\n> +\t\tentry->supported_capabilities = 0;\n\nIf we would use a generic data pointer then we could initialize\nsupported_capabilities in \"start_multi_file_filter_fn\".\n\n\nApart from the bike shedding above this patch looks good to me.\nMinor nit: It breaks t0021 \"invalid process filter must fail (and not hang!)\"\ntest case but I assume this is corrected in a later patch.\n\n\nCheers,\nLars\n\n\n"},{"id":"316483","messageId":"1B2A2436-8BD4-4858-BC90-BCB1617D586B@gmail.com","threadId":"45630","inReplyTo":"20170407120354.17736-7-benpeart@microsoft.com","subject":"Re: [PATCH v5 6/8] convert: rename reusable sub-process functions","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-04-10T12:11:08Z","receivedAt":"2017-04-10T12:11:16Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 07 Apr 2017, at 14:03, Ben Peart wrote:\n> \n> Do a mechanical rename of the functions that will become the reusable\n> sub-process module.\n> \n> Signed-off-by: Ben Peart \n> ---\n> convert.c | 53 +++++++++++++++++++++++++++--------------------------\n> 1 file changed, 27 insertions(+), 26 deletions(-)\n> \n> diff --git a/convert.c b/convert.c\n> index 747c0c363b..235a6a5279 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -507,8 +507,8 @@ struct cmd2process {\n> \tunsigned int supported_capabilities;\n> };\n> \n\n> ...\n\n> \n> -static void kill_multi_file_filter(struct subprocess_entry *entry)\n> +static void subprocess_stop(struct subprocess_entry *entry)\n> {\n> \tif (!entry)\n> \t\treturn;\n> @@ -561,14 +561,14 @@ static void kill_multi_file_filter(struct subprocess_entry *entry)\n> \tkill(entry->process.pid, SIGTERM);\n> \tfinish_command(&entry->process);\n> \n> -\thashmap_remove(&cmd_process_map, entry, NULL);\n> +\thashmap_remove(&subprocess_map, entry, NULL);\n> \tfree(entry);\n> }\n\n> ...\n\n> @@ -777,7 +777,8 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n> \t\t\t * Force shutdown and restart if another blob requires filtering.\n> \t\t\t */\n> \t\t\terror(\"external filter '%s' failed\", cmd);\n> -\t\t\tkill_multi_file_filter((struct subprocess_entry *)entry);\n> +\t\t\tsubprocess_stop((struct subprocess_entry *)entry);\n> +\t\t\tfree(entry);\n\nI think we perform a double \"free\" here and in subprocess_stop(), no?\nEverything else looks good to me!\n\nThanks,\nLars"},{"id":"316484","messageId":"40AA3048-3B05-4950-8981-942DB58B464F@gmail.com","threadId":"45630","inReplyTo":"20170407120354.17736-8-benpeart@microsoft.com","subject":"Re: [PATCH v5 7/8] sub-process: move sub-process functions into separate files","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-04-10T12:41:52Z","receivedAt":"2017-04-10T12:42:01Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 07 Apr 2017, at 14:03, Ben Peart wrote:\n> \n> Move the sub-proces functions into sub-process.h/c. Add documentation\n> for the new module in Documentation/technical/api-sub-process.txt\n> \n> Signed-off-by: Ben Peart \n> ---\n> Documentation/technical/api-sub-process.txt | 54 +++++++++++++\n> Makefile | 1 +\n> convert.c | 119 +---------------------------\n> sub-process.c | 116 +++++++++++++++++++++++++++\n> sub-process.h | 46 +++++++++++\n> 5 files changed, 218 insertions(+), 118 deletions(-)\n> create mode 100644 Documentation/technical/api-sub-process.txt\n> create mode 100644 sub-process.c\n> create mode 100644 sub-process.h\n> \n> diff --git a/Documentation/technical/api-sub-process.txt b/Documentation/technical/api-sub-process.txt\n> new file mode 100644\n> index 0000000000..eb5005aa72\n> --- /dev/null\n> +++ b/Documentation/technical/api-sub-process.txt\n> @@ -0,0 +1,54 @@\n> +sub-process API\n> +===============\n> +\n> +The sub-process API makes it possible to run background sub-processes\n> +that should run until the git command exits and communicate with it\n> +through stdin and stdout. This reduces the overhead of having to fork\n> +a new process each time it needs to be communicated with.\n\nMinor nit, maybe:\nThe sub-process API makes it possible to run background sub-processes\nfor the entire lifetime of a Git invocation. If Git needs to communicate\nwith an external process multiple times, then this can reduces the process\ninvocation overhead. Git and the sub-process communicate through stdin and \nstdout. \n\nFeel free to ignore as this is bike-shedding.\n\n\n> +\n> +The sub-processes are kept in a hashmap by command name and looked up\n> +via the subprocess_find_entry function. If an existing instance can not\n> +be found then a new process should be created and started. When the\n> +parent git command terminates, all sub-processes are also terminated.\n> +\n> +This API is based on the run-command API.\n> +\n> +Data structures\n> +---------------\n> +\n> +* `struct subprocess_entry`\n> +\n> +The sub-process structure. Members should not be accessed directly.\n> +\n> +Types\n> +-----\n> +\n> +'int(*subprocess_start_fn)(struct subprocess_entry *entry)'::\n> +\n> +\tUser-supplied function to initialize the sub-process. This is\n> +\ttypically used to negoiate the interface version and capabilities.\ns/negoiate/negotiate/\n\n\n> +\n> +\n> +Functions\n> +---------\n> +\n> +`subprocess_start`::\n> +\n> +\tStart a subprocess and add it to the subprocess hashmap.\n> +\n> +`subprocess_stop`::\n> +\n> +\tKill a subprocess and remove it from the subprocess hashmap.\n> +\n> +`subprocess_find_entry`::\n> +\n> +\tFind a subprocess in the subprocess hashmap.\n\nAs mentioned in an earlier review, this function also initializes the\nhashmap as (maybe to the user unexpected?) side effect.\n\nhttp://public-inbox.org/git/48FA4601-0819-4DE2-943A-7A791BA7C583@gmail.com/\n\n\n> +\n> +`subprocess_get_child_process`::\n> +\n> +\tGet the underlying `struct child_process` from a subprocess.\n> +\n> +`subprocess_read_status`::\n> +\n> +\tHelper function to read packets looking for the last \"status=\"\n> +\tkey/value pair.\n> diff --git a/Makefile b/Makefile\n> index 9f8b35ad41..add945b560 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -838,6 +838,7 @@ LIB_OBJS += streaming.o\n> LIB_OBJS += string-list.o\n> LIB_OBJS += submodule.o\n> LIB_OBJS += submodule-config.o\n> +LIB_OBJS += sub-process.o\n> LIB_OBJS += symlinks.o\n> LIB_OBJS += tag.o\n> LIB_OBJS += tempfile.o\n> diff --git a/convert.c b/convert.c\n> index 235a6a5279..baa41da760 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -4,6 +4,7 @@\n> #include \"quote.h\"\n> #include \"sigchain.h\"\n> #include \"pkt-line.h\"\n> +#include \"sub-process.h\"\n> \n> /*\n> * convert.c - convert a file when checking it out and checking it in.\n> @@ -496,86 +497,11 @@ static int apply_single_file_filter(const char *path, const char *src, size_t le\n> #define CAP_CLEAN (1u<<0)\n> #define CAP_SMUDGE (1u<<1)\n> \n> -struct subprocess_entry {\n> -\tstruct hashmap_entry ent; /* must be the first member! */\n> -\tconst char *cmd;\n> -\tstruct child_process process;\n> -};\n> -\n> struct cmd2process {\n> \tstruct subprocess_entry subprocess; /* must be the first member! */\n> \tunsigned int supported_capabilities;\n> };\n> \n> -static int subprocess_map_initialized;\n> -static struct hashmap subprocess_map;\n> -\n> -static int cmd2process_cmp(const struct subprocess_entry *e1,\n> -\t\t\t const struct subprocess_entry *e2,\n> -\t\t\t const void *unused)\n> -{\n> -\treturn strcmp(e1->cmd, e2->cmd);\n> -}\n> -\n> -static struct subprocess_entry *subprocess_find_entry(const char *cmd)\n> -{\n> -\tstruct subprocess_entry key;\n> -\n> -\tif (!subprocess_map_initialized) {\n> -\t\tsubprocess_map_initialized = 1;\n> -\t\thashmap_init(&subprocess_map, (hashmap_cmp_fn)cmd2process_cmp, 0);\n> -\t\treturn NULL;\n> -\t}\n> -\n> -\thashmap_entry_init(&key, strhash(cmd));\n> -\tkey.cmd = cmd;\n> -\treturn hashmap_get(&subprocess_map, &key, NULL);\n> -}\n> -\n> -static void subprocess_read_status(int fd, struct strbuf *status)\n> -{\n> -\tstruct strbuf **pair;\n> -\tchar *line;\n> -\tfor (;;) {\n> -\t\tline = packet_read_line(fd, NULL);\n> -\t\tif (!line)\n> -\t\t\tbreak;\n> -\t\tpair = strbuf_split_str(line, '=', 2);\n> -\t\tif (pair[0] && pair[0]->len && pair[1]) {\n> -\t\t\t/* the last \"status=\" line wins */\n> -\t\t\tif (!strcmp(pair[0]->buf, \"status=\")) {\n> -\t\t\t\tstrbuf_reset(status);\n> -\t\t\t\tstrbuf_addbuf(status, pair[1]);\n> -\t\t\t}\n> -\t\t}\n> -\t\tstrbuf_list_free(pair);\n> -\t}\n> -}\n> -\n> -static void subprocess_stop(struct subprocess_entry *entry)\n> -{\n> -\tif (!entry)\n> -\t\treturn;\n> -\n> -\tentry->process.clean_on_exit = 0;\n> -\tkill(entry->process.pid, SIGTERM);\n> -\tfinish_command(&entry->process);\n> -\n> -\thashmap_remove(&subprocess_map, entry, NULL);\n> -\tfree(entry);\n> -}\n> -\n> -static void subprocess_exit_handler(struct child_process *process)\n> -{\n> -\tsigchain_push(SIGPIPE, SIG_IGN);\n> -\t/* Closing the pipe signals the subprocess to initiate a shutdown. */\n> -\tclose(process->in);\n> -\tclose(process->out);\n> -\tsigchain_pop(SIGPIPE);\n> -\t/* Finish command will wait until the shutdown is complete. */\n> -\tfinish_command(process);\n> -}\n> -\n> static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)\n> {\n> \tint err;\n> @@ -639,49 +565,6 @@ static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)\n> \treturn err;\n> }\n> \n> -typedef int(*subprocess_start_fn)(struct subprocess_entry *entry);\n> -int subprocess_start(struct subprocess_entry *entry, const char *cmd,\n> -\tsubprocess_start_fn startfn)\n> -{\n> -\tint err;\n> -\tstruct child_process *process;\n> -\tconst char *argv[] = { cmd, NULL };\n> -\n> -\tif (!subprocess_map_initialized) {\n> -\t\tsubprocess_map_initialized = 1;\n> -\t\thashmap_init(&subprocess_map, (hashmap_cmp_fn)cmd2process_cmp, 0);\n> -\t}\n> -\n> -\tentry->cmd = cmd;\n> -\tprocess = &entry->process;\n> -\n> -\tchild_process_init(process);\n> -\tprocess->argv = argv;\n> -\tprocess->use_shell = 1;\n> -\tprocess->in = -1;\n> -\tprocess->out = -1;\n> -\tprocess->clean_on_exit = 1;\n> -\tprocess->clean_on_exit_handler = subprocess_exit_handler;\n> -\n> -\terr = start_command(process);\n> -\tif (err) {\n> -\t\terror(\"cannot fork to run subprocess '%s'\", cmd);\n> -\t\treturn err;\n> -\t}\n> -\n> -\thashmap_entry_init(entry, strhash(cmd));\n> -\n> -\terr = startfn(entry);\n> -\tif (err) {\n> -\t\terror(\"initialization for subprocess '%s' failed\", cmd);\n> -\t\tsubprocess_stop(entry);\n> -\t\treturn err;\n> -\t}\n> -\n> -\thashmap_add(&subprocess_map, entry);\n> -\treturn 0;\n> -}\n> -\n> static int apply_multi_file_filter(const char *path, const char *src, size_t len,\n> \t\t\t\t int fd, struct strbuf *dst, const char *cmd,\n> \t\t\t\t const unsigned int wanted_capability)\n> diff --git a/sub-process.c b/sub-process.c\n> new file mode 100644\n> index 0000000000..60bb650012\n> --- /dev/null\n> +++ b/sub-process.c\n> @@ -0,0 +1,116 @@\n> +/*\n> + * Generic implementation of background process infrastructure.\n> + */\n> +#include \"sub-process.h\"\n> +#include \"sigchain.h\"\n> +#include \"pkt-line.h\"\n> +\n> +static int subprocess_map_initialized;\n> +static struct hashmap subprocess_map;\n> +\n> +static int cmd2process_cmp(const struct subprocess_entry *e1,\n> +\t\t\t const struct subprocess_entry *e2,\n> +\t\t\t const void *unused)\n> +{\n> +\treturn strcmp(e1->cmd, e2->cmd);\n> +}\n> +\n> +struct subprocess_entry *subprocess_find_entry(const char *cmd)\n> +{\n> +\tstruct subprocess_entry key;\n> +\n> +\tif (!subprocess_map_initialized) {\n> +\t\tsubprocess_map_initialized = 1;\n> +\t\thashmap_init(&subprocess_map, (hashmap_cmp_fn)cmd2process_cmp, 0);\n> +\t\treturn NULL;\n> +\t}\n> +\n> +\thashmap_entry_init(&key, strhash(cmd));\n> +\tkey.cmd = cmd;\n> +\treturn hashmap_get(&subprocess_map, &key, NULL);\n> +}\n> +\n> +void subprocess_read_status(int fd, struct strbuf *status)\n> +{\n> +\tstruct strbuf **pair;\n> +\tchar *line;\n> +\tfor (;;) {\n> +\t\tline = packet_read_line(fd, NULL);\n> +\t\tif (!line)\n> +\t\t\tbreak;\n> +\t\tpair = strbuf_split_str(line, '=', 2);\n> +\t\tif (pair[0] && pair[0]->len && pair[1]) {\n> +\t\t\t/* the last \"status=\" line wins */\n> +\t\t\tif (!strcmp(pair[0]->buf, \"status=\")) {\n> +\t\t\t\tstrbuf_reset(status);\n> +\t\t\t\tstrbuf_addbuf(status, pair[1]);\n> +\t\t\t}\n> +\t\t}\n> +\t\tstrbuf_list_free(pair);\n> +\t}\n> +}\n> +\n> +void subprocess_stop(struct subprocess_entry *entry)\n> +{\n> +\tif (!entry)\n> +\t\treturn;\n> +\n> +\tentry->process.clean_on_exit = 0;\n> +\tkill(entry->process.pid, SIGTERM);\n> +\tfinish_command(&entry->process);\n> +\n> +\thashmap_remove(&subprocess_map, entry, NULL);\n> +}\n> +\n> +static void subprocess_exit_handler(struct child_process *process)\n> +{\n> +\tsigchain_push(SIGPIPE, SIG_IGN);\n> +\t/* Closing the pipe signals the subprocess to initiate a shutdown. */\n> +\tclose(process->in);\n> +\tclose(process->out);\n> +\tsigchain_pop(SIGPIPE);\n> +\t/* Finish command will wait until the shutdown is complete. */\n> +\tfinish_command(process);\n> +}\n> +\n> +int subprocess_start(struct subprocess_entry *entry, const char *cmd,\n> +\tsubprocess_start_fn startfn)\n> +{\n> +\tint err;\n> +\tstruct child_process *process;\n> +\tconst char *argv[] = { cmd, NULL };\n> +\n> +\tif (!subprocess_map_initialized) {\n> +\t\tsubprocess_map_initialized = 1;\n> +\t\thashmap_init(&subprocess_map, (hashmap_cmp_fn)cmd2process_cmp, 0);\n> +\t}\n> +\n> +\tentry->cmd = cmd;\n> +\tprocess = &entry->process;\n> +\n> +\tchild_process_init(process);\n> +\tprocess->argv = argv;\n> +\tprocess->use_shell = 1;\n> +\tprocess->in = -1;\n> +\tprocess->out = -1;\n> +\tprocess->clean_on_exit = 1;\n> +\tprocess->clean_on_exit_handler = subprocess_exit_handler;\n> +\n> +\terr = start_command(process);\n> +\tif (err) {\n> +\t\terror(\"cannot fork to run subprocess '%s'\", cmd);\n> +\t\treturn err;\n> +\t}\n> +\n> +\thashmap_entry_init(entry, strhash(cmd));\n> +\n> +\terr = startfn(entry);\n> +\tif (err) {\n> +\t\terror(\"initialization for subprocess '%s' failed\", cmd);\n> +\t\tsubprocess_stop(entry);\n> +\t\treturn err;\n> +\t}\n> +\n> +\thashmap_add(&subprocess_map, entry);\n> +\treturn 0;\n> +}\n> diff --git a/sub-process.h b/sub-process.h\n> new file mode 100644\n> index 0000000000..0cf1760a0a\n> --- /dev/null\n> +++ b/sub-process.h\n> @@ -0,0 +1,46 @@\n> +#ifndef SUBPROCESS_H\n> +#define SUBPROCESS_H\n> +\n> +#include \"git-compat-util.h\"\n> +#include \"hashmap.h\"\n> +#include \"run-command.h\"\n> +\n> +/*\n> + * Generic implementation of background process infrastructure.\n> + * See Documentation/technical/api-background-process.txt.\n> + */\n> +\n> + /* data structures */\n> +\n> +struct subprocess_entry {\n> +\tstruct hashmap_entry ent; /* must be the first member! */\n> +\tconst char *cmd;\n> +\tstruct child_process process;\n> +};\n> +\n> +/* subprocess functions */\n> +\n> +typedef int(*subprocess_start_fn)(struct subprocess_entry *entry);\n> +int subprocess_start(struct subprocess_entry *entry, const char *cmd,\n> +\t\tsubprocess_start_fn startfn);\n> +\n> +void subprocess_stop(struct subprocess_entry *entry);\n> +\n> +struct subprocess_entry *subprocess_find_entry(const char *cmd);\n> +\n> +/* subprocess helper functions */\n> +\n> +static inline struct child_process *subprocess_get_child_process(\n> +\t\tstruct subprocess_entry *entry)\n> +{\n> +\treturn &entry->process;\n> +}\n> +\n> +/*\n> + * Helper function that will read packets looking for \"status=\"\n> + * key/value pairs and return the value from the last \"status\" packet\n> + */\n> +\n> +void subprocess_read_status(int fd, struct strbuf *status);\n> +\n> +#endif\n> -- \n> 2.12.0.windows.1.31.g1548525701.dirty\n\nLooks all good to me. This patch also fixes the failing t0021 \"invalid \nprocess filter must fail (and not hang!)\" introduced here:\nhttp://public-inbox.org/git/5900F7F1-89D9-433E-A6C3-0AB27C815BE6@gmail.com/\n\nTBH, I don't see why this fixes the test, though.\n\nThanks,\nLars"},{"id":"316485","messageId":"DD629A2F-362D-41F0-9240-F79C319AA76B@gmail.com","threadId":"45630","inReplyTo":"20170407120354.17736-9-benpeart@microsoft.com","subject":"Re: [PATCH v5 8/8] convert: Update subprocess_read_status to not die on EOF","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-04-10T12:48:59Z","receivedAt":"2017-04-10T12:49:06Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 07 Apr 2017, at 14:03, Ben Peart wrote:\n> \n> Enable sub-processes to gracefully handle when the process dies by\n> updating subprocess_read_status to return an error on EOF instead of\n> dying.\n> \n> Update apply_multi_file_filter to take advantage of the revised\n> subprocess_read_status.\n> \n> Signed-off-by: Ben Peart \n> ---\n> convert.c | 10 ++++++++--\n> sub-process.c | 10 +++++++---\n> sub-process.h | 2 +-\n> 3 files changed, 16 insertions(+), 6 deletions(-)\n> \n> diff --git a/convert.c b/convert.c\n> index baa41da760..9e181e27ad 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -629,7 +629,10 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n> \tif (err)\n> \t\tgoto done;\n> \n> -\tsubprocess_read_status(process->out, &filter_status);\n> +\terr = subprocess_read_status(process->out, &filter_status);\n> +\tif (err)\n> +\t\tgoto done;\n> +\n> \terr = strcmp(filter_status.buf, \"success\");\n> \tif (err)\n> \t\tgoto done;\n> @@ -638,7 +641,10 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n> \tif (err)\n> \t\tgoto done;\n> \n> -\tsubprocess_read_status(process->out, &filter_status);\n> +\terr = subprocess_read_status(process->out, &filter_status);\n> +\tif (err)\n> +\t\tgoto done;\n> +\n> \terr = strcmp(filter_status.buf, \"success\");\n> \n> done:\n> diff --git a/sub-process.c b/sub-process.c\n> index 60bb650012..c5057cafcd 100644\n> --- a/sub-process.c\n> +++ b/sub-process.c\n> @@ -30,13 +30,15 @@ struct subprocess_entry *subprocess_find_entry(const char *cmd)\n> \treturn hashmap_get(&subprocess_map, &key, NULL);\n> }\n> \n> -void subprocess_read_status(int fd, struct strbuf *status)\n> +int subprocess_read_status(int fd, struct strbuf *status)\n> {\n> \tstruct strbuf **pair;\n> \tchar *line;\n> +\tint len;\n> +\n> \tfor (;;) {\n> -\t\tline = packet_read_line(fd, NULL);\n> -\t\tif (!line)\n> +\t\tlen = packet_read_line_gently(fd, NULL, &line);\n> +\t\tif ((len == -1) || !line)\n\nMinor nit: Maybe \"if ((len < 0) || !line)\" ?\n\n\n> \t\t\tbreak;\n> \t\tpair = strbuf_split_str(line, '=', 2);\n> \t\tif (pair[0] && pair[0]->len && pair[1]) {\n> @@ -48,6 +50,8 @@ void subprocess_read_status(int fd, struct strbuf *status)\n> \t\t}\n> \t\tstrbuf_list_free(pair);\n> \t}\n> +\n> +\treturn len == -1 ? len : 0;\n\nMinor nit: Maybe \"return len < 0 ? len : 0;\n\n> }\n> \n> void subprocess_stop(struct subprocess_entry *entry)\n> diff --git a/sub-process.h b/sub-process.h\n> index 0cf1760a0a..5a1eeeece0 100644\n> --- a/sub-process.h\n> +++ b/sub-process.h\n> @@ -41,6 +41,6 @@ static inline struct child_process *subprocess_get_child_process(\n> * key/value pairs and return the value from the last \"status\" packet\n> */\n> \n> -void subprocess_read_status(int fd, struct strbuf *status);\n> +int subprocess_read_status(int fd, struct strbuf *status);\n> \n> #endif\n> -- \n> 2.12.0.windows.1.31.g1548525701.dirty\n> \n\nLooks good to me.\n\nThanks,\nLars\n\n"},{"id":"316600","messageId":"20170411161617.fyu5pmzgplscoozz@sigill.intra.peff.net","threadId":"45630","inReplyTo":"20170407120354.17736-4-benpeart@microsoft.com","subject":"Re: [PATCH v5 3/8] convert: Split start_multi_file_filter into two separate functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-11T16:16:17Z","receivedAt":"2017-04-11T16:16:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 07, 2017 at 08:03:49AM -0400, Ben Peart wrote:\n\n> @@ -642,7 +621,41 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons\n> done:\n> \tsigchain_pop(SIGPIPE);\n> \n> -\tif (err || errno == EPIPE) {\n> +\tif (err || errno == EPIPE)\n> +\t\terr = err ? err : errno;\n> +\n> +\treturn err;\n> +}\n\nThis isn't a new problem introduced by your patch, but this use of errno\nseems funny to me. Specifically:\n\n 1. Do we need to save errno before calling sigchain_pop()? It's making\n syscalls (though admittedly they are unlikely to fail).\n\n 2. If err is 0, then nothing failed. Who would have set errno? Aren't\n we reading whatever cruft happened to be in errno before the\n function started?\n\n So I'm confused about what case would trigger on this errno check\n at all.\n\nAlso, I'm not quite sure what the return value of the function is\nsupposed to be; usually we'd use 0 for success and negative for errors.\nBut here we might return a negative value that we got from the packet_*\nfunctions, or we might return EPIPE, which is positive. I don't think\nit's a huge deal because the caller checks \"if (err)\", but perhaps it\nshould be \"-errno\" for consistency.\n\n-Peff\n"},{"id":"316625","messageId":"FD58071A-4DC7-4AB7-8B6B-56B71E715144@gmail.com","threadId":"45630","inReplyTo":"20170411161617.fyu5pmzgplscoozz@sigill.intra.peff.net","subject":"Re: [PATCH v5 3/8] convert: Split start_multi_file_filter into two separate functions","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-04-11T19:29:36Z","receivedAt":"2017-04-11T19:29:46Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 11 Apr 2017, at 18:16, Jeff King wrote:\n> \n> On Fri, Apr 07, 2017 at 08:03:49AM -0400, Ben Peart wrote:\n> \n>> @@ -642,7 +621,41 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons\n>> done:\n>> \tsigchain_pop(SIGPIPE);\n>> \n>> -\tif (err || errno == EPIPE) {\n>> +\tif (err || errno == EPIPE)\n>> +\t\terr = err ? err : errno;\n>> +\n>> +\treturn err;\n>> +}\n> \n> This isn't a new problem introduced by your patch, but this use of errno\n> seems funny to me. Specifically:\n\nI introduced these lines, therefore I try to answer :-)\n\n\n> 1. Do we need to save errno before calling sigchain_pop()? It's making\n> syscalls (though admittedly they are unlikely to fail).\n\nWhat if we add the following right before sigchain_pop() ?\n\n\tif (errno == EPIPE)\n\t\terr = -1;\n\n\n> 2. If err is 0, then nothing failed. Who would have set errno? Aren't\n> we reading whatever cruft happened to be in errno before the\n> function started?\n\nYeah, looks like you're right:\nhttps://www.securecoding.cert.org/confluence/pages/viewpage.action?pageId=6619179\n\nAccording to this article we shouldn't even check *only* for errno. \nAt least we should add\n\terrno = 0;\nat the beginning of the function, no?\n\nThis means we have many areas in Git where we don't handle errno\ncorrectly. E.g. right in convert.c where I stole code from:\nhttps://github.com/git/git/commit/0c4dd67a048b39470b9b95912e4912fecc405a85#diff-7949b716ab0a83e8c422a0d6336f19d6R361\n\nShould that be addressed?\n\n- Lars"},{"id":"316626","messageId":"20170411193709.w5tz3i3vg5mnnhfe@sigill.intra.peff.net","threadId":"45630","inReplyTo":"FD58071A-4DC7-4AB7-8B6B-56B71E715144@gmail.com","subject":"Re: [PATCH v5 3/8] convert: Split start_multi_file_filter into two separate functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-11T19:37:10Z","receivedAt":"2017-04-11T19:37:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 11, 2017 at 09:29:36PM +0200, Lars Schneider wrote:\n\n> > 1. Do we need to save errno before calling sigchain_pop()? It's making\n> > syscalls (though admittedly they are unlikely to fail).\n> \n> What if we add the following right before sigchain_pop() ?\n> \n> \tif (errno == EPIPE)\n> \t\terr = -1;\n\nYes, that would be fine (though again, this runs against point 2 below).\n\n> > 2. If err is 0, then nothing failed. Who would have set errno? Aren't\n> > we reading whatever cruft happened to be in errno before the\n> > function started?\n> \n> Yeah, looks like you're right:\n> https://www.securecoding.cert.org/confluence/pages/viewpage.action?pageId=6619179\n> \n> According to this article we shouldn't even check *only* for errno. \n> At least we should add\n> \terrno = 0;\n> at the beginning of the function, no?\n\nIf you initialize errno to 0 right before a syscall, then yes, you can\ntrust it without checking the return value of the syscall. I wouldn't\ntrust it before calling more complicated functions, though. Not even\nxwrite(), which may see EINTR and keep going (which is OK for checking\nfor EPIPE, but not checking generally for errno values).\n\n> This means we have many areas in Git where we don't handle errno\n> correctly. E.g. right in convert.c where I stole code from:\n> https://github.com/git/git/commit/0c4dd67a048b39470b9b95912e4912fecc405a85#diff-7949b716ab0a83e8c422a0d6336f19d6R361\n> \n> Should that be addressed?\n\nThat one is questionable code, but I don't think it behaves incorrectly.\nAfter the write_in_full() call finishes, then either:\n\n 1. write_err is 0, and conditional is a noop\n\n 2. write_err is non-zero, and errno is relevant\n\nI do think it would be more clear as:\n\n if (write_err && errno == EPIPE)\n\twrite_err = 0;\n\nsimilar to the code right below it.\n\n-Peff\n"},{"id":"316632","messageId":"629B2192-FD64-422E-9361-C182303582DC@gmail.com","threadId":"45630","inReplyTo":"20170411193709.w5tz3i3vg5mnnhfe@sigill.intra.peff.net","subject":"Re: [PATCH v5 3/8] convert: Split start_multi_file_filter into two separate functions","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-04-11T20:01:02Z","receivedAt":"2017-04-11T20:01:11Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 11 Apr 2017, at 21:37, Jeff King wrote:\n> \n> On Tue, Apr 11, 2017 at 09:29:36PM +0200, Lars Schneider wrote:\n> \n>>> 1. Do we need to save errno before calling sigchain_pop()? It's making\n>>> syscalls (though admittedly they are unlikely to fail).\n>> \n>> What if we add the following right before sigchain_pop() ?\n>> \n>> \tif (errno == EPIPE)\n>> \t\terr = -1;\n> \n> Yes, that would be fine (though again, this runs against point 2 below).\n> \n>>> 2. If err is 0, then nothing failed. Who would have set errno? Aren't\n>>> we reading whatever cruft happened to be in errno before the\n>>> function started?\n>> \n>> Yeah, looks like you're right:\n>> https://www.securecoding.cert.org/confluence/pages/viewpage.action?pageId=6619179\n>> \n>> According to this article we shouldn't even check *only* for errno. \n>> At least we should add\n>> \terrno = 0;\n>> at the beginning of the function, no?\n> \n> If you initialize errno to 0 right before a syscall, then yes, you can\n> trust it without checking the return value of the syscall. I wouldn't\n> trust it before calling more complicated functions, though. Not even\n> xwrite(), which may see EINTR and keep going (which is OK for checking\n> for EPIPE, but not checking generally for errno values).\n\nShould we remove all the errno checks here as we don't have any direct \n\"write\" etc syscalls anyways then?\n\n\n- Lars"},{"id":"316633","messageId":"20170411200520.oivytvlzkdu7bfh5@sigill.intra.peff.net","threadId":"45630","inReplyTo":"629B2192-FD64-422E-9361-C182303582DC@gmail.com","subject":"Re: [PATCH v5 3/8] convert: Split start_multi_file_filter into two separate functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-11T20:05:20Z","receivedAt":"2017-04-11T20:05:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 11, 2017 at 10:01:02PM +0200, Lars Schneider wrote:\n\n> > If you initialize errno to 0 right before a syscall, then yes, you can\n> > trust it without checking the return value of the syscall. I wouldn't\n> > trust it before calling more complicated functions, though. Not even\n> > xwrite(), which may see EINTR and keep going (which is OK for checking\n> > for EPIPE, but not checking generally for errno values).\n> \n> Should we remove all the errno checks here as we don't have any direct \n> \"write\" etc syscalls anyways then?\n\nYeah, you should be trusting the return value from the various\nsub-functions. Usually you'd check \"errno == EPIPE\" only when you saw an\nerror return but you want to _ignore_ EPIPE. This is what\nfilter_buffer_or_fd() is doing.\n\nBut the code here is the opposite case. It definitely wants to treat\nEPIPE as an error. But that should be happening already because any\nEPIPE we get would come with an error-return from one of the\npacket_write() functions.\n\nSo I would say that \"err || errno == EPIPE\" here can just become \"err\",\nand ditto in apply_multi_file_filter().\n\n-Peff\n"},{"id":"316982","messageId":"xmqqmvbfk8va.fsf@gitster.mtv.corp.google.com","threadId":"45630","inReplyTo":"48FA4601-0819-4DE2-943A-7A791BA7C583@gmail.com","subject":"Re: [PATCH v5 4/8] convert: Separate generic structures and variables from the filter specific ones","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-17T03:31:37Z","receivedAt":"2017-04-17T03:31:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n>> -static struct cmd2process *find_multi_file_filter_entry(struct hashmap *hashmap, const char *cmd)\n>> +static struct subprocess_entry *find_multi_file_filter_entry(const char *cmd)\n>\n> I am curious why you removed the hashmap parameter (here and in other pars of this patch). \n> I know the parameter is not strictly necessary as the hashmap is a global variable anyways. \n> However, I think it eases code maintainability in the long run if a function is \"as pure as \n> possible\" (IOW does rely on global state as less as possible).\n\nIf the original relied on a global hashmap and this update kept the\ncode like so, I wouldn't mind the end result of this series\n(i.e. rely on it being global). But that is not the case. It is\nmaking the code worse by stopping passing the hashmap through the\ncallchain.\n"},{"id":"317115","messageId":"48448c2c-378d-0d87-2f99-32095326f323@gmail.com","threadId":"45630","inReplyTo":"xmqqmvbfk8va.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v5 4/8] convert: Separate generic structures and variables from the filter specific ones","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-04-18T16:38:37Z","receivedAt":"2017-04-18T16:38:48Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"On 4/16/2017 11:31 PM, Junio C Hamano wrote:\n> Lars Schneider writes:\n>\n>>> -static struct cmd2process *find_multi_file_filter_entry(struct hashmap *hashmap, const char *cmd)\n>>> +static struct subprocess_entry *find_multi_file_filter_entry(const char *cmd)\n>> I am curious why you removed the hashmap parameter (here and in other pars of this patch).\n>> I know the parameter is not strictly necessary as the hashmap is a global variable anyways.\n>> However, I think it eases code maintainability in the long run if a function is \"as pure as\n>> possible\" (IOW does rely on global state as less as possible).\n> If the original relied on a global hashmap and this update kept the\n> code like so, I wouldn't mind the end result of this series\n> (i.e. rely on it being global). But that is not the case. It is\n> making the code worse by stopping passing the hashmap through the\n> callchain.\nI debated this myself but there were a couple of things that pushed me \nto this simpler model. First, it simplified the interface a little as \nyou don't need an explicit init call with state to detect if it has \nalready been initialized and you don't have to pass the hashmap into the \nvarious functions. Since it was already a global, I didn't feel like \nthis made it any worse.\n\nSecond, since the hashmap key is the exact command that was executed if \nyou ever had two hashmaps with the same key, you'd end up with two \ncopies of the same background process running. I couldn't come up with \nany scenario where you would want two copies of the exact same command \nrunning at the same time so again went with the simpler model.\n\nWhile I was typing this, I realized that since the background process is \na versioned interface, if there were two different parts of the code \nusing background processes and if they both wanted to run the same \nbackground process and if they wanted to use different versions of the \ninterface, then you could want two different copies.\n\nThat said, I tend to build for things that are needed now rather than \nthose that might be needed in the future. If I've missed a scenario, \nI'm happy to change the interface.\n"},{"id":"317164","messageId":"xmqqa87ddwbe.fsf@gitster.mtv.corp.google.com","threadId":"45630","inReplyTo":"48448c2c-378d-0d87-2f99-32095326f323@gmail.com","subject":"Re: [PATCH v5 4/8] convert: Separate generic structures and variables from the filter specific ones","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-19T01:23:49Z","receivedAt":"2017-04-19T01:23:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Peart writes:\n\n> On 4/16/2017 11:31 PM, Junio C Hamano wrote:\n>> Lars Schneider writes:\n>>\n>>> However, I think it eases code maintainability in the long run if a function is \"as pure as\n>>> possible\" (IOW does rely on global state as less as possible).\n>>\n>> If the original relied on a global hashmap and this update kept the\n>> code like so, I wouldn't mind the end result of this series\n>> (i.e. rely on it being global). But that is not the case. It is\n>> making the code worse by stopping passing the hashmap through the\n>> callchain.\n>\n> ... Since it was already a global, I didn't feel\n> like this made it any worse.\n\nThe code before your series can easily lose the globals with the\nattached patch, _exactly_ because it is prepared to be reusable by a\nnew caller that supplies its own hashmap by passing it through the\ncallchain. The child-process machinery Lars made for his conversion\nthing, which you are trying to split out to make it reusable, can be\nused by somebody other than apply_multi_file_filter() if you do not\nlose the hashmap; what the new caller needs to do is to supply its\nown hashmap so that they do not interact with the set of processes\nused by Lars's conversion machinery.\n\nIf we want to lose the global _after_ applying this patch 4/8, don't\nwe have to essentially _undo_ 4/8? How can it not be seen as making\nit worse?\n\n\n convert.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 8d652bf27c..ff831e85b8 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -503,9 +503,6 @@ struct cmd2process {\n \tstruct child_process process;\n };\n \n-static int cmd_process_map_initialized;\n-static struct hashmap cmd_process_map;\n-\n static int cmd2process_cmp(const struct cmd2process *e1,\n \t\t\t const struct cmd2process *e2,\n \t\t\t const void *unused)\n@@ -682,6 +679,9 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n \tstruct strbuf filter_status = STRBUF_INIT;\n \tconst char *filter_type;\n \n+static int cmd_process_map_initialized;\n+static struct hashmap cmd_process_map;\n+\n \tif (!cmd_process_map_initialized) {\n \t\tcmd_process_map_initialized = 1;\n \t\thashmap_init(&cmd_process_map, (hashmap_cmp_fn) cmd2process_cmp, 0);\n\n"},{"id":"317358","messageId":"afcdc386-0122-a33e-fa90-4ddbba32ab66@gmail.com","threadId":"45630","inReplyTo":"xmqqa87ddwbe.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v5 4/8] convert: Separate generic structures and variables from the filter specific ones","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-04-20T17:24:45Z","receivedAt":"2017-04-20T17:24:59Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"On 4/18/2017 9:23 PM, Junio C Hamano wrote:\n> Ben Peart writes:\n>\n>> On 4/16/2017 11:31 PM, Junio C Hamano wrote:\n>>> Lars Schneider writes:\n>>>\n>>>> However, I think it eases code maintainability in the long run if a function is \"as pure as\n>>>> possible\" (IOW does rely on global state as less as possible).\n>>> If the original relied on a global hashmap and this update kept the\n>>> code like so, I wouldn't mind the end result of this series\n>>> (i.e. rely on it being global). But that is not the case. It is\n>>> making the code worse by stopping passing the hashmap through the\n>>> callchain.\n>> ... Since it was already a global, I didn't feel\n>> like this made it any worse.\n> The code before your series can easily lose the globals with the\n> attached patch, _exactly_ because it is prepared to be reusable by a\n> new caller that supplies its own hashmap by passing it through the\n> callchain. The child-process machinery Lars made for his conversion\n> thing, which you are trying to split out to make it reusable, can be\n> used by somebody other than apply_multi_file_filter() if you do not\n> lose the hashmap; what the new caller needs to do is to supply its\n> own hashmap so that they do not interact with the set of processes\n> used by Lars's conversion machinery.\n>\n> If we want to lose the global _after_ applying this patch 4/8, don't\n> we have to essentially _undo_ 4/8? How can it not be seen as making\n> it worse?\n\nThat's fine. I'll flip it back in the next spin and enable multiple \npools of sub-processes. We'll still need 4/8 but it will be a smaller \nchange.\n\n>\n> convert.c | 6 +++---\n> 1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/convert.c b/convert.c\n> index 8d652bf27c..ff831e85b8 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -503,9 +503,6 @@ struct cmd2process {\n> \tstruct child_process process;\n> };\n> \n> -static int cmd_process_map_initialized;\n> -static struct hashmap cmd_process_map;\n> -\n> static int cmd2process_cmp(const struct cmd2process *e1,\n> \t\t\t const struct cmd2process *e2,\n> \t\t\t const void *unused)\n> @@ -682,6 +679,9 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n> \tstruct strbuf filter_status = STRBUF_INIT;\n> \tconst char *filter_type;\n> \n> +static int cmd_process_map_initialized;\n> +static struct hashmap cmd_process_map;\n> +\n> \tif (!cmd_process_map_initialized) {\n> \t\tcmd_process_map_initialized = 1;\n> \t\thashmap_init(&cmd_process_map, (hashmap_cmp_fn) cmd2process_cmp, 0);\n>\n\n"},{"id":"317359","messageId":"c8204d07-ce42-6153-61a6-1a612840d337@gmail.com","threadId":"45630","inReplyTo":"20170411200520.oivytvlzkdu7bfh5@sigill.intra.peff.net","subject":"Re: [PATCH v5 3/8] convert: Split start_multi_file_filter into two separate functions","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-04-20T17:27:44Z","receivedAt":"2017-04-20T17:27:55Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"On 4/11/2017 4:05 PM, Jeff King wrote:\n> On Tue, Apr 11, 2017 at 10:01:02PM +0200, Lars Schneider wrote:\n>\n>>> If you initialize errno to 0 right before a syscall, then yes, you can\n>>> trust it without checking the return value of the syscall. I wouldn't\n>>> trust it before calling more complicated functions, though. Not even\n>>> xwrite(), which may see EINTR and keep going (which is OK for checking\n>>> for EPIPE, but not checking generally for errno values).\n>> Should we remove all the errno checks here as we don't have any direct\n>> \"write\" etc syscalls anyways then?\n> Yeah, you should be trusting the return value from the various\n> sub-functions. Usually you'd check \"errno == EPIPE\" only when you saw an\n> error return but you want to _ignore_ EPIPE. This is what\n> filter_buffer_or_fd() is doing.\n>\n> But the code here is the opposite case. It definitely wants to treat\n> EPIPE as an error. But that should be happening already because any\n> EPIPE we get would come with an error-return from one of the\n> packet_write() functions.\n>\n> So I would say that \"err || errno == EPIPE\" here can just become \"err\",\n> and ditto in apply_multi_file_filter().\nI'll update it this way in the next spin of the patch series.\n> -Peff\n\n"}]}