{"thread":{"id":"45878","subject":"[PATCH v7 00/10] refactor the filter process code into a reusable module","startedAt":"2017-05-05T15:28:32Z","lastAt":"2017-05-15T04:02:32Z","messageCount":16,"participants":["Ben Peart","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":7,"patchTotal":10},"messages":[{"id":"318846","messageId":"20170505152802.6724-1-benpeart@microsoft.com","threadId":"45878","inReplyTo":null,"subject":"[PATCH v7 00/10] refactor the filter process code into a reusable module","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-05-05T15:27:52Z","receivedAt":"2017-05-05T15:28:32Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"Changes from V6 include:\n\nconvert: remove erroneous tests for errno == EPIPE\n - split into separate patch to fix a preexisting bug discovered in the review process\n\npkt-line: Update packet_read_line() to test for len > 0\n - split into separate patch to deal with errors that return negative lengths\n\npkt-line: add packet_read_line_gently()\n - update documentation to clarify return values\n - update white space in function definition\n\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 (10):\n convert: remove erroneous tests for errno == EPIPE\n pkt-line: fix packet_read_line() to handle len < 0 errors\n pkt-line: add packet_read_line_gently()\n convert: move packet_write_line() 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 | 59 ++++++++++\n Makefile | 1 +\n convert.c | 161 ++++++----------------------\n pkt-line.c | 33 +++++-\n pkt-line.h | 12 +++\n sub-process.c | 106 ++++++++++++++++++\n sub-process.h | 49 +++++++++\n 7 files changed, 292 insertions(+), 129 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.2.gvfs.2.20.g3624a68d62.dirty\n\n"},{"id":"318847","messageId":"20170505152802.6724-2-benpeart@microsoft.com","threadId":"45878","inReplyTo":"20170505152802.6724-1-benpeart@microsoft.com","subject":"[PATCH v7 01/10] convert: remove erroneous tests for errno == EPIPE","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-05-05T15:27:53Z","receivedAt":"2017-05-05T15:28:41Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"start_multi_file_filter() and apply_multi_file_filter() currently test\nfor errno == EPIPE but treating EPIPE as an error is already happening\nfrom one of the packet_write() functions.\n\nSigned-off-by: Ben Peart \nFound/Fixed-by: Jeff King \nAcked-by: Lars Schneider \n---\n convert.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 4e17e45ed2..bdd528086f 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -661,7 +661,7 @@ 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) {\n \t\terror(\"initialization for external filter '%s' failed\", cmd);\n \t\tkill_multi_file_filter(hashmap, entry);\n \t\treturn NULL;\n@@ -752,7 +752,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n done:\n \tsigchain_pop(SIGPIPE);\n \n-\tif (err || errno == EPIPE) {\n+\tif (err) {\n \t\tif (!strcmp(filter_status.buf, \"error\")) {\n \t\t\t/* The filter signaled a problem with the file. */\n \t\t} else if (!strcmp(filter_status.buf, \"abort\")) {\n-- \n2.12.2.gvfs.2.20.g3624a68d62.dirty\n\n"},{"id":"318848","messageId":"20170505152802.6724-3-benpeart@microsoft.com","threadId":"45878","inReplyTo":"20170505152802.6724-1-benpeart@microsoft.com","subject":"[PATCH v7 02/10] pkt-line: fix packet_read_line() to handle len < 0 errors","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-05-05T15:27:54Z","receivedAt":"2017-05-05T15:28:43Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"Update packet_read_line() to test for len > 0 to avoid potential bug\nif read functions return lengths less than zero to indicate errors.\n\nSigned-off-by: Ben Peart \nFound/Fixed-by: Lars Schneider \n---\n pkt-line.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex d4b6bfe076..6f05b1a4a8 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -315,7 +315,7 @@ static char *packet_read_line_generic(int fd,\n \t\t\t PACKET_READ_CHOMP_NEWLINE);\n \tif (dst_len)\n \t\t*dst_len = len;\n-\treturn len ? packet_buffer : NULL;\n+\treturn (len > 0) ? packet_buffer : NULL;\n }\n \n char *packet_read_line(int fd, int *len_p)\n-- \n2.12.2.gvfs.2.20.g3624a68d62.dirty\n\n"},{"id":"318849","messageId":"20170505152802.6724-4-benpeart@microsoft.com","threadId":"45878","inReplyTo":"20170505152802.6724-1-benpeart@microsoft.com","subject":"[PATCH v7 03/10] pkt-line: add packet_read_line_gently()","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-05-05T15:27:55Z","receivedAt":"2017-05-05T15:28:47Z","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 | 11 +++++++++++\n 2 files changed, 23 insertions(+)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 6f05b1a4a8..7db9119573 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\t\t packet_buffer, sizeof(packet_buffer),\n+\t\t\t PACKET_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..66ef610fc4 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -74,6 +74,17 @@ 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 or when the number of\n+ * bytes copied is zero and otherwise points to a static buffer (that may be\n+ * overwritten by subsequent calls). If the size parameter is not NULL, the\n+ * 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.2.gvfs.2.20.g3624a68d62.dirty\n\n"},{"id":"318850","messageId":"20170505152802.6724-5-benpeart@microsoft.com","threadId":"45878","inReplyTo":"20170505152802.6724-1-benpeart@microsoft.com","subject":"[PATCH v7 04/10] convert: move packet_write_line() into pkt-line as packet_writel()","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-05-05T15:27:56Z","receivedAt":"2017-05-05T15:28:50Z","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 bdd528086f..1f18d947b9 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 7db9119573..9d845ecc3c 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 66ef610fc4..b2965869ad 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.2.gvfs.2.20.g3624a68d62.dirty\n\n"},{"id":"318851","messageId":"20170505152802.6724-6-benpeart@microsoft.com","threadId":"45878","inReplyTo":"20170505152802.6724-1-benpeart@microsoft.com","subject":"[PATCH v7 05/10] convert: split start_multi_file_filter() into two separate functions","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-05-05T15:27:57Z","receivedAt":"2017-05-05T15:28:52Z","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 | 58 ++++++++++++++++++++++++++++++++++------------------------\n 1 file changed, 34 insertions(+), 24 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 1f18d947b9..4e1d018577 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,6 +621,37 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons\n done:\n \tsigchain_pop(SIGPIPE);\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-- \n2.12.2.gvfs.2.20.g3624a68d62.dirty\n\n"},{"id":"318852","messageId":"20170505152802.6724-8-benpeart@microsoft.com","threadId":"45878","inReplyTo":"20170505152802.6724-1-benpeart@microsoft.com","subject":"[PATCH v7 07/10] convert: Update generic functions to only use generic data structures","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-05-05T15:27:59Z","receivedAt":"2017-05-05T15:28:54Z","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 | 41 +++++++++++++++++++++++------------------\n 1 file changed, 23 insertions(+), 18 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 5876218347..7d05dcb4aa 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -556,7 +556,6 @@ static void kill_multi_file_filter(struct hashmap *hashmap, struct subprocess_en\n \tfinish_command(&entry->process);\n \n \thashmap_remove(hashmap, entry, NULL);\n-\tfree(entry);\n }\n \n static void stop_multi_file_filter(struct child_process *process)\n@@ -570,14 +569,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@@ -629,17 +629,16 @@ 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+typedef int(*subprocess_start_fn)(struct subprocess_entry *entry);\n+int start_multi_file_filter(struct hashmap *hashmap, 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+\tentry->cmd = cmd;\n+\tprocess = &entry->process;\n \n \tchild_process_init(process);\n \tprocess->argv = argv;\n@@ -649,22 +648,23 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons\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(hashmap, &entry->subprocess);\n-\t\treturn NULL;\n+\t\tkill_multi_file_filter(hashmap, entry);\n+\t\treturn err;\n \t}\n \n \thashmap_add(hashmap, 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@@ -689,9 +689,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_process_map, 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(&cmd_process_map, &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@@ -765,6 +769,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n \t\t\t */\n \t\t\terror(\"external filter '%s' failed\", cmd);\n \t\t\tkill_multi_file_filter(&cmd_process_map, &entry->subprocess);\n+\t\t\tfree(entry);\n \t\t}\n \t} else {\n \t\tstrbuf_swap(dst, &nbuf);\n-- \n2.12.2.gvfs.2.20.g3624a68d62.dirty\n\n"},{"id":"318853","messageId":"20170505152802.6724-10-benpeart@microsoft.com","threadId":"45878","inReplyTo":"20170505152802.6724-1-benpeart@microsoft.com","subject":"[PATCH v7 09/10] sub-process: move sub-process functions into separate files","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-05-05T15:28:01Z","receivedAt":"2017-05-05T15:28:56Z","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 | 59 ++++++++++++++++\n Makefile | 1 +\n convert.c | 104 +---------------------------\n sub-process.c | 102 +++++++++++++++++++++++++++\n sub-process.h | 49 +++++++++++++\n 5 files changed, 212 insertions(+), 103 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..793508cf3e\n--- /dev/null\n+++ b/Documentation/technical/api-sub-process.txt\n@@ -0,0 +1,59 @@\n+sub-process API\n+===============\n+\n+The sub-process API makes it possible to run background sub-processes\n+for the entire lifetime of a Git invocation. If Git needs to communicate\n+with an external process multiple times, then this can reduces the process\n+invocation overhead. Git and the sub-process communicate through stdin and\n+stdout.\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 negotiate the interface version and capabilities.\n+\n+\n+Functions\n+---------\n+\n+`cmd2process_cmp`::\n+\n+\tFunction to test two subprocess hashmap entries for equality.\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 47827aa129..3239dcbf0a 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -832,6 +832,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 84c4ff8a01..acf51416d1 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,12 +497,6 @@ 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@@ -510,65 +505,6 @@ struct cmd2process {\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(struct hashmap *hashmap, const char *cmd)\n-{\n-\tstruct subprocess_entry key;\n-\n-\thashmap_entry_init(&key, strhash(cmd));\n-\tkey.cmd = cmd;\n-\treturn hashmap_get(hashmap, &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 hashmap *hashmap, 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(hashmap, 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 static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)\n {\n \tint err;\n@@ -629,44 +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 hashmap *hashmap, 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-\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(hashmap, entry);\n-\t\treturn err;\n-\t}\n-\n-\thashmap_add(hashmap, 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..536b60cced\n--- /dev/null\n+++ b/sub-process.c\n@@ -0,0 +1,102 @@\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+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(struct hashmap *hashmap, const char *cmd)\n+{\n+\tstruct subprocess_entry key;\n+\n+\thashmap_entry_init(&key, strhash(cmd));\n+\tkey.cmd = cmd;\n+\treturn hashmap_get(hashmap, &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 hashmap *hashmap, 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(hashmap, 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 hashmap *hashmap, 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+\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(hashmap, entry);\n+\t\treturn err;\n+\t}\n+\n+\thashmap_add(hashmap, entry);\n+\treturn 0;\n+}\ndiff --git a/sub-process.h b/sub-process.h\nnew file mode 100644\nindex 0000000000..a88e782bfc\n--- /dev/null\n+++ b/sub-process.h\n@@ -0,0 +1,49 @@\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+int cmd2process_cmp(const struct subprocess_entry *e1,\n+\tconst struct subprocess_entry *e2, const void *unused);\n+\n+typedef int(*subprocess_start_fn)(struct subprocess_entry *entry);\n+int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, const char *cmd,\n+\t\tsubprocess_start_fn startfn);\n+\n+void subprocess_stop(struct hashmap *hashmap, struct subprocess_entry *entry);\n+\n+struct subprocess_entry *subprocess_find_entry(struct hashmap *hashmap, 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.2.gvfs.2.20.g3624a68d62.dirty\n\n"},{"id":"318854","messageId":"20170505152802.6724-11-benpeart@microsoft.com","threadId":"45878","inReplyTo":"20170505152802.6724-1-benpeart@microsoft.com","subject":"[PATCH v7 10/10] convert: Update subprocess_read_status to not die on EOF","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-05-05T15:28:02Z","receivedAt":"2017-05-05T15:28:58Z","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 acf51416d1..85238e4976 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -635,7 +635,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@@ -644,7 +647,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 536b60cced..92f8aea70a 100644\n--- a/sub-process.c\n+++ b/sub-process.c\n@@ -21,13 +21,15 @@ struct subprocess_entry *subprocess_find_entry(struct hashmap *hashmap, const ch\n \treturn hashmap_get(hashmap, &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 < 0) || !line)\n \t\t\tbreak;\n \t\tpair = strbuf_split_str(line, '=', 2);\n \t\tif (pair[0] && pair[0]->len && pair[1]) {\n@@ -39,6 +41,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 < 0) ? len : 0;\n }\n \n void subprocess_stop(struct hashmap *hashmap, struct subprocess_entry *entry)\ndiff --git a/sub-process.h b/sub-process.h\nindex a88e782bfc..7d451e1cde 100644\n--- a/sub-process.h\n+++ b/sub-process.h\n@@ -44,6 +44,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.2.gvfs.2.20.g3624a68d62.dirty\n\n"},{"id":"318855","messageId":"20170505152802.6724-9-benpeart@microsoft.com","threadId":"45878","inReplyTo":"20170505152802.6724-1-benpeart@microsoft.com","subject":"[PATCH v7 08/10] convert: rename reusable sub-process functions","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-05-05T15:28:00Z","receivedAt":"2017-05-05T15:29:02Z","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 | 40 ++++++++++++++++++++--------------------\n 1 file changed, 20 insertions(+), 20 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 7d05dcb4aa..84c4ff8a01 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,7 +517,7 @@ 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(struct hashmap *hashmap, const char *cmd)\n+static struct subprocess_entry *subprocess_find_entry(struct hashmap *hashmap, const char *cmd)\n {\n \tstruct subprocess_entry key;\n \n@@ -526,7 +526,7 @@ static struct subprocess_entry *find_multi_file_filter_entry(struct hashmap *has\n \treturn hashmap_get(hashmap, &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@@ -546,7 +546,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 subprocess_entry *entry)\n+static void subprocess_stop(struct hashmap *hashmap, struct subprocess_entry *entry)\n {\n \tif (!entry)\n \t\treturn;\n@@ -558,10 +558,10 @@ static void kill_multi_file_filter(struct hashmap *hashmap, struct subprocess_en\n \thashmap_remove(hashmap, entry, NULL);\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@@ -630,7 +630,7 @@ 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 hashmap *hashmap, struct subprocess_entry *entry, const char *cmd,\n+int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, const char *cmd,\n \tsubprocess_start_fn startfn)\n {\n \tint err;\n@@ -646,11 +646,11 @@ int start_multi_file_filter(struct hashmap *hashmap, struct subprocess_entry *en\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@@ -658,8 +658,8 @@ int start_multi_file_filter(struct hashmap *hashmap, struct subprocess_entry *en\n \n \terr = startfn(entry);\n \tif (err) {\n-\t\terror(\"initialization for external filter '%s' failed\", cmd);\n-\t\tkill_multi_file_filter(hashmap, entry);\n+\t\terror(\"initialization for subprocess '%s' failed\", cmd);\n+\t\tsubprocess_stop(hashmap, entry);\n \t\treturn err;\n \t}\n \n@@ -678,12 +678,12 @@ 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+\tif (!subprocess_map_initialized) {\n+\t\tsubprocess_map_initialized = 1;\n+\t\thashmap_init(&subprocess_map, (hashmap_cmp_fn) cmd2process_cmp, 0);\n \t\tentry = NULL;\n \t} else {\n-\t\tentry = (struct cmd2process *)find_multi_file_filter_entry(&cmd_process_map, cmd);\n+\t\tentry = (struct cmd2process *)subprocess_find_entry(&subprocess_map, cmd);\n \t}\n \n \tfflush(NULL);\n@@ -692,7 +692,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(&cmd_process_map, &entry->subprocess, cmd, start_multi_file_filter_fn)) {\n+\t\tif (subprocess_start(&subprocess_map, &entry->subprocess, cmd, start_multi_file_filter_fn)) {\n \t\t\tfree(entry);\n \t\t\treturn 0;\n \t\t}\n@@ -737,7 +737,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@@ -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 \n done:\n@@ -768,7 +768,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->subprocess);\n+\t\t\tsubprocess_stop(&subprocess_map, &entry->subprocess);\n \t\t\tfree(entry);\n \t\t}\n \t} else {\n-- \n2.12.2.gvfs.2.20.g3624a68d62.dirty\n\n"},{"id":"318856","messageId":"20170505152802.6724-7-benpeart@microsoft.com","threadId":"45878","inReplyTo":"20170505152802.6724-1-benpeart@microsoft.com","subject":"[PATCH v7 06/10] convert: Separate generic structures and variables from the filter specific ones","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-05-05T15:27:58Z","receivedAt":"2017-05-05T15:29:06Z","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.\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 | 35 ++++++++++++++++++++---------------\n 1 file changed, 20 insertions(+), 15 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 4e1d018577..5876218347 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -496,26 +496,31 @@ 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(struct hashmap *hashmap, const char *cmd)\n {\n-\tstruct cmd2process key;\n+\tstruct subprocess_entry key;\n+\n \thashmap_entry_init(&key, strhash(cmd));\n \tkey.cmd = cmd;\n \treturn hashmap_get(hashmap, &key, NULL);\n@@ -541,7 +546,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 hashmap *hashmap, struct subprocess_entry *entry)\n {\n \tif (!entry)\n \t\treturn;\n@@ -571,8 +576,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@@ -632,9 +637,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@@ -654,7 +659,7 @@ 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(hashmap, &entry->subprocess);\n \t\treturn NULL;\n \t}\n \n@@ -678,7 +683,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\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\tentry = (struct cmd2process *)find_multi_file_filter_entry(&cmd_process_map, cmd);\n \t}\n \n \tfflush(NULL);\n@@ -688,7 +693,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\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@@ -759,7 +764,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(&cmd_process_map, &entry->subprocess);\n \t\t}\n \t} else {\n \t\tstrbuf_swap(dst, &nbuf);\n-- \n2.12.2.gvfs.2.20.g3624a68d62.dirty\n\n"},{"id":"319037","messageId":"xmqqfuggaz3d.fsf@gitster.mtv.corp.google.com","threadId":"45878","inReplyTo":"20170505152802.6724-1-benpeart@microsoft.com","subject":"Re: [PATCH v7 00/10] refactor the filter process code into a reusable module","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-08T01:58:46Z","receivedAt":"2017-05-08T01:58:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Peart writes:\n\n> Changes from V6 include:\n>\n> convert: remove erroneous tests for errno == EPIPE\n> - split into separate patch to fix a preexisting bug discovered in the review process\n\nThanks.\n\n\n> pkt-line: Update packet_read_line() to test for len > 0\n> - split into separate patch to deal with errors that return negative lengths\n>\n> pkt-line: add packet_read_line_gently()\n> - update documentation to clarify return values\n> - update white space in function definition\n>\n\nI also see some style fixes applied to a few patches. Thanks for\npaying attention to details.\n\nWill queue; during the pre-release freeze, new things would move\nslowly, but let's see if we have more comments from others and then\nmerge it to 'next' soon after the 2.13 final.\n\nThanks.\n"},{"id":"319603","messageId":"20170513090457.s6gmqjdyrj4osmck@sigill.intra.peff.net","threadId":"45878","inReplyTo":"20170505152802.6724-5-benpeart@microsoft.com","subject":"Re: [PATCH v7 04/10] convert: move packet_write_line() into pkt-line as packet_writel()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-13T09:04:58Z","receivedAt":"2017-05-13T09:05:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 05, 2017 at 11:27:56AM -0400, Ben Peart wrote:\n\n> +int packet_writel(int fd, const char *line, ...);\n\nThis isn't a new problem, but I noticed that this function should\nprobably get annotated to describe its interface.\n\nJunio, can you pick up the patch below on top of Ben's series (or I'd be\nfine if it were squashed into this patch)?\n\n-- >8 --\nSubject: [PATCH] pkt-line: annotate packet_writel with LAST_ARG_MUST_BE_NULL\n\npacket_writel() takes a variable-sized list and reads to\nthe first NULL. Let's let the compiler know so that it can\nhelp us catch mistakes in the callers.\n\nThis should have been annotated similarly when it was a\nstatic function, but it's doubly important now that the\nfunction is available to the whole code-base.\n\nSigned-off-by: Jeff King \n---\n pkt-line.h | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/pkt-line.h b/pkt-line.h\nindex b2965869a..450183b64 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+LAST_ARG_MUST_BE_NULL\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-- \n2.13.0.452.g0afc8e12b\n\n"},{"id":"319604","messageId":"20170513090740.xldv4td3omhg3gxc@sigill.intra.peff.net","threadId":"45878","inReplyTo":"20170505152802.6724-10-benpeart@microsoft.com","subject":"Re: [PATCH v7 09/10] sub-process: move sub-process functions into separate files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-13T09:07:40Z","receivedAt":"2017-05-13T09:07:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 05, 2017 at 11:28:01AM -0400, Ben Peart wrote:\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\nThis isn't a new issue with your series, but the SIGPIPEs here seem odd.\nI don't think you can get SIGPIPE from closing descriptors.\n\nI suspect this is a hold-over from when Lars' original design, where we\nactually sent an \"exit\" message to the filter. It's not hurting\nanything, and I don't think it's worth holding up your series for. But\nwhen you are working in this area further, it might be worth cleaning\nup.\n\n-Peff\n"},{"id":"319605","messageId":"20170513091637.zebzyi7ggidybzak@sigill.intra.peff.net","threadId":"45878","inReplyTo":"xmqqfuggaz3d.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v7 00/10] refactor the filter process code into a reusable module","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-13T09:16:37Z","receivedAt":"2017-05-13T09:16:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 08, 2017 at 10:58:46AM +0900, Junio C Hamano wrote:\n\n> Will queue; during the pre-release freeze, new things would move\n> slowly, but let's see if we have more comments from others and then\n> merge it to 'next' soon after the 2.13 final.\n\nI gave it a fresh read-through. I had a few comments, but nothing that I\nthink should hold up moving these patches to 'next'.\n\nI do find the convert/sub-process split a little funny. Most of the\nprotocol bits from convert didn't get moved over, and you could use a\nsub-process that spoke any arbitrary protocol. The odd man out is\nsubprocess_read_status(), which assumes not only pktlines, but also\na \"status=foo\" key/value pair inside them.\n\nI have a feeling that funny split is because this is a preliminary\nrefactoring step that will have more features built on it, and the end\nresult may be more consistent. We _could_ hold these back and pass\njudgement once we see more. But they're a reasonable-sized chunk that's\nalready been through 7 rounds of review. It makes sense to me to\ngraduate them at least to 'next' and let the next steps use them as a\nstable base.\n\n-Peff\n"},{"id":"319753","messageId":"xmqqbmquwywe.fsf@gitster.mtv.corp.google.com","threadId":"45878","inReplyTo":"20170513090457.s6gmqjdyrj4osmck@sigill.intra.peff.net","subject":"Re: [PATCH v7 04/10] convert: move packet_write_line() into pkt-line as packet_writel()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-15T04:02:25Z","receivedAt":"2017-05-15T04:02:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This isn't a new problem, but I noticed that this function should\n> probably get annotated to describe its interface.\n>\n> Junio, can you pick up the patch below on top of Ben's series (or I'd be\n> fine if it were squashed into this patch)?\n\nSurely. Thanks for a careful review.\n\n>\n> -- >8 --\n> Subject: [PATCH] pkt-line: annotate packet_writel with LAST_ARG_MUST_BE_NULL\n>\n> packet_writel() takes a variable-sized list and reads to\n> the first NULL. Let's let the compiler know so that it can\n> help us catch mistakes in the callers.\n>\n> This should have been annotated similarly when it was a\n> static function, but it's doubly important now that the\n> function is available to the whole code-base.\n>\n> Signed-off-by: Jeff King \n> ---\n> pkt-line.h | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/pkt-line.h b/pkt-line.h\n> index b2965869a..450183b64 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> +LAST_ARG_MUST_BE_NULL\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"}]}