{"thread":{"id":"45466","subject":"[PATCH v1 1/3] pkt-line: add packet_write_list_gently()","startedAt":"2017-03-22T17:00:38Z","lastAt":"2017-03-27T22:21:21Z","messageCount":12,"participants":["Ben Peart","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"314916","messageId":"20170322165220.5660-2-benpeart@microsoft.com","threadId":"45466","inReplyTo":"20170322165220.5660-1-benpeart@microsoft.com","subject":"[PATCH v1 1/3] pkt-line: add packet_write_list_gently()","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-03-22T16:52:18Z","receivedAt":"2017-03-22T17:00:38Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"Add packet_write_list_gently() which writes multiple lines in a single\ncall and then calls packet_flush_gently(). This is used later in this\npatch series.\n\nSigned-off-by: Ben Peart <benpeart@microsoft.com>\n---\n pkt-line.c | 19 +++++++++++++++++++\n pkt-line.h |  1 +\n 2 files changed, 20 insertions(+)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex d4b6bfe076..fccdac1352 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_write_list_gently(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 18eac64830..3674d04509 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_write_list_gently(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.gvfs.1.42.g0b7328eac2\n\n"},{"id":"314917","messageId":"20170322165220.5660-4-benpeart@microsoft.com","threadId":"45466","inReplyTo":"20170322165220.5660-1-benpeart@microsoft.com","subject":"[PATCH v1 3/3] convert: use new sub-process module for filter processes","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-03-22T16:52:20Z","receivedAt":"2017-03-22T17:02:08Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"The filter.<driver>.process code was refactored into a separate\n“sub-process” module to facilitate reuse in future patch series. This\ncentralizes the logic in a single place to simplify maintenance. As a\nside benefit, it also improves the readability of convert.c.\n\nSigned-off-by: Ben Peart <benpeart@microsoft.com>\n---\n convert.c | 154 ++++++++++----------------------------------------------------\n 1 file changed, 24 insertions(+), 130 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 8d652bf27c..ca8e0d26c0 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@@ -497,132 +498,30 @@ static int apply_single_file_filter(const char *path, const char *src, size_t le\n #define CAP_SMUDGE   (1u<<1)\n \n struct cmd2process {\n-\tstruct hashmap_entry ent; /* must be the first member! */\n+\tstruct subprocess_entry subprocess;\n \tunsigned int supported_capabilities;\n-\tconst char *cmd;\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-{\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-{\n-\tstruct cmd2process key;\n-\thashmap_entry_init(&key, strhash(cmd));\n-\tkey.cmd = cmd;\n-\treturn hashmap_get(hashmap, &key, NULL);\n-}\n-\n-static int packet_write_list(int fd, const char *line, ...)\n+static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)\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-\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=<foo>\" 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 kill_multi_file_filter(struct hashmap *hashmap, struct cmd2process *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-\tfree(entry);\n-}\n-\n-static void stop_multi_file_filter(struct child_process *process)\n-{\n-\tsigchain_push(SIGPIPE, SIG_IGN);\n-\t/* Closing the pipe signals the filter 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 struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, const char *cmd)\n-{\n-\tint err;\n-\tstruct cmd2process *entry;\n+\tstruct cmd2process *entry = (struct cmd2process *)subprocess;\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+\tprocess = subprocess_get_child_process(&entry->subprocess);\n \n \tsigchain_push(SIGPIPE, SIG_IGN);\n \n-\terr = packet_write_list(process->in, \"git-filter-client\", \"version=2\", NULL);\n+\terr = packet_write_list_gently(process->in, \"git-filter-client\", \"version=2\", NULL);\n \tif (err)\n \t\tgoto done;\n \n \terr = strcmp(packet_read_line(process->out, NULL), \"git-filter-server\");\n \tif (err) {\n-\t\terror(\"external filter '%s' does not support filter protocol version 2\", cmd);\n+\t\terror(\"external filter '%s' does not support filter protocol version 2\", subprocess->cmd);\n \t\tgoto done;\n \t}\n \terr = strcmp(packet_read_line(process->out, NULL), \"version=2\");\n@@ -632,7 +531,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_write_list_gently(process->in, \"capability=clean\", \"capability=smudge\", NULL);\n \n \tfor (;;) {\n \t\tcap_buf = packet_read_line(process->out, NULL);\n@@ -651,7 +550,7 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons\n \t\t} else {\n \t\t\twarning(\n \t\t\t\t\"external filter '%s' requested unsupported filter capability '%s'\",\n-\t\t\t\tcmd, cap_name\n+\t\t\t\tsubprocess->cmd, cap_name\n \t\t\t);\n \t\t}\n \n@@ -661,14 +560,10 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons\n done:\n \tsigchain_pop(SIGPIPE);\n \n-\tif (err || errno == EPIPE) {\n-\t\terror(\"initialization for external filter '%s' failed\", cmd);\n-\t\tkill_multi_file_filter(hashmap, entry);\n-\t\treturn NULL;\n-\t}\n+\tif (err || errno == EPIPE)\n+\t\terr = err ? err : errno;\n \n-\thashmap_add(hashmap, entry);\n-\treturn entry;\n+\treturn err;\n }\n \n static int apply_multi_file_filter(const char *path, const char *src, size_t len,\n@@ -682,22 +577,20 @@ 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 *)subprocess_find_entry(cmd);\n \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 (subprocess_start(&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->process;\n+\tprocess = subprocess_get_child_process(&entry->subprocess);\n \n \tif (!(wanted_capability & entry->supported_capabilities))\n \t\treturn 0;\n@@ -737,7 +630,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 +639,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 +661,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(&cmd_process_map, 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.gvfs.1.42.g0b7328eac2\n\n"},{"id":"314918","messageId":"20170322165220.5660-3-benpeart@microsoft.com","threadId":"45466","inReplyTo":"20170322165220.5660-1-benpeart@microsoft.com","subject":"[PATCH v1 2/3] sub-process: refactor the filter process code into a reusable module","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-03-22T16:52:19Z","receivedAt":"2017-03-22T17:02:16Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"Create a new sub-process module that can be used to reduce the cost of\nstarting up a sub-process for multiple commands.  It does this by\nkeeping the external process running and processing all commands by\ncommunicating over standard input and standard output using the packet\nformat (pkt-line) based protocol.  Full documentation is contained in\nDocumentation/technical/api-sub-process.txt.\n\nThis code is refactored from:\n\n\tCommit edcc85814c (\"convert: add filter.<driver>.process option\", 2016-10-16)\n\tkeeps the external process running and processes all commands\n\nSigned-off-by: Ben Peart <benpeart@microsoft.com>\n---\n Documentation/technical/api-sub-process.txt |  55 ++++++++++++++\n Makefile                                    |   1 +\n sub-process.c                               | 113 ++++++++++++++++++++++++++++\n sub-process.h                               |  46 +++++++++++\n 4 files changed, 215 insertions(+)\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..8471875611\n--- /dev/null\n+++ b/Documentation/technical/api-sub-process.txt\n@@ -0,0 +1,55 @@\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=<foo>\"\n+\tkey/value pair.\n+\ndiff --git a/Makefile b/Makefile\nindex a5a11e721a..8afe733092 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -830,6 +830,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/sub-process.c b/sub-process.c\nnew file mode 100644\nindex 0000000000..02050b6867\n--- /dev/null\n+++ b/sub-process.c\n@@ -0,0 +1,113 @@\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 name2process_cmp(const struct subprocess_entry *e1,\n+\t\tconst struct subprocess_entry *e2, const void *unused)\n+{\n+\treturn strcmp(e1->cmd, e2->cmd);\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 filter 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+\t\tsubprocess_start_fn startfn)\n+{\n+\tint err;\n+\tconst char *argv[] = { cmd, NULL };\n+\n+\tif (!subprocess_map_initialized) {\n+\t\thashmap_init(&subprocess_map, (hashmap_cmp_fn)name2process_cmp, 0);\n+\t\tsubprocess_map_initialized = 1;\n+\t}\n+\n+\tentry->cmd = cmd;\n+\n+\tchild_process_init(&entry->process);\n+\tentry->process.argv = argv;\n+\tentry->process.use_shell = 1;\n+\tentry->process.in = -1;\n+\tentry->process.out = -1;\n+\tentry->process.clean_on_exit = 1;\n+\tentry->process.clean_on_exit_handler = subprocess_exit_handler;\n+\n+\terr = start_command(&entry->process);\n+\tif (err) {\n+\t\terror(\"cannot fork to run sub-process '%s'\", entry->cmd);\n+\t\treturn err;\n+\t}\n+\n+\terr = startfn(entry);\n+\tif (err) {\n+\t\terror(\"initialization for sub-process '%s' failed\", entry->cmd);\n+\t\tsubprocess_stop(entry);\n+\t\treturn err;\n+\t}\n+\n+\thashmap_entry_init(entry, strhash(entry->cmd));\n+\thashmap_add(&subprocess_map, entry);\n+\n+\treturn 0;\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+struct subprocess_entry *subprocess_find_entry(const char *cmd)\n+{\n+\tstruct subprocess_entry key;\n+\n+\tif (!subprocess_map_initialized) {\n+\t\thashmap_init(&subprocess_map, (hashmap_cmp_fn)name2process_cmp, 0);\n+\t\tsubprocess_map_initialized = 1;\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=<foo>\" 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+}\ndiff --git a/sub-process.h b/sub-process.h\nnew file mode 100644\nindex 0000000000..235f1e5fa3\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+\tstruct child_process process;\n+\tconst char *cmd;\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=<foo>\"\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.gvfs.1.42.g0b7328eac2\n\n"},{"id":"314920","messageId":"20170322165220.5660-1-benpeart@microsoft.com","threadId":"45466","inReplyTo":null,"subject":"[PATCH v1 0/3] Add support for downloading blobs on demand","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-03-22T16:52:17Z","receivedAt":"2017-03-22T17:03:06Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"We have a couple of patch series we’re working on (ObjectDB/Read-Object,\nWatchman integration) where we could use the ability to have a\nbackground process running that can accept multiple commands thus\navoiding the overhead of spawning a new process for every command.\n\nThe ability to do this was added in:\n\n\tCommit edcc85814c (\"convert: add filter.<driver>.process option\", 2016-10-16)\n\tkeeps the external process running and processes all commands\n\nbut it is integrated into the convert code.  This patch series takes the\ncode from convert.c and refactors it into a separate “sub-process”\nmodule so that we can centralize and reuse this logic in other areas.\nOnce the code was refactored into sub-process, convert.c was updated to\nuse the new module.\n\nBen Peart (3):\n  pkt-line: add packet_write_list_gently()\n  sub-process: refactor the filter process code into a reusable module\n  convert: use new sub-process module for filter processes\n\n Documentation/technical/api-sub-process.txt |  55 ++++++++++\n Makefile                                    |   1 +\n convert.c                                   | 154 +++++-----------------------\n pkt-line.c                                  |  19 ++++\n pkt-line.h                                  |   1 +\n sub-process.c                               | 113 ++++++++++++++++++++\n sub-process.h                               |  46 +++++++++\n 7 files changed, 259 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.gvfs.1.42.g0b7328eac2\n\n"},{"id":"314961","messageId":"xmqqefxpjc5b.fsf@gitster.mtv.corp.google.com","threadId":"45466","inReplyTo":"20170322165220.5660-2-benpeart@microsoft.com","subject":"Re: [PATCH v1 1/3] pkt-line: add packet_write_list_gently()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-22T20:21:20Z","receivedAt":"2017-03-22T20:21:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Peart <peartben@gmail.com> writes:\n\n> Add packet_write_list_gently() which writes multiple lines in a single\n> call and then calls packet_flush_gently(). This is used later in this\n> patch series.\n\nI can see how it would be convenient to have a function like this.\nI'd name it without _gently(), though.  We call something _gently()\nwhen we initially only have a function that dies hard on error and\nlater want to have a variant that returns an error for the caller to\nhandle.  You are starting without a dying variant (which is probably\na preferable way to structure the API).\n\nAlso I am hesitant to take a function that does not take any \"list\"\ntype argument and yet calls itself \"write_list\".  IOW, I'd expect\nsomething like these\n\n\twrite_list(int fd, const char **lines);\n\twrite_list(int fd, struct string_list *lines);\n\ngiven that name, and not \"varargs, each of which is a line\".  I am\ntempted to suggest\n\n\tpacket_writel(int fd, const char *line, ...);\n\nnoticing similarity with execl(), but perhaps others may be able to\ncome up with better names.\n\n> Signed-off-by: Ben Peart <benpeart@microsoft.com>\n> ---\n>  pkt-line.c | 19 +++++++++++++++++++\n>  pkt-line.h |  1 +\n>  2 files changed, 20 insertions(+)\n>\n> diff --git a/pkt-line.c b/pkt-line.c\n> index d4b6bfe076..fccdac1352 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_write_list_gently(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 18eac64830..3674d04509 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_write_list_gently(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"},{"id":"315039","messageId":"xmqqk27gfrga.fsf@gitster.mtv.corp.google.com","threadId":"45466","inReplyTo":"20170322165220.5660-3-benpeart@microsoft.com","subject":"Re: [PATCH v1 2/3] sub-process: refactor the filter process code into a reusable module","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-23T06:16:37Z","receivedAt":"2017-03-23T07:33:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Peart <peartben@gmail.com> writes:\n\n> This code is refactored from:\n>\n> \tCommit edcc85814c (\"convert: add filter.<driver>.process option\", 2016-10-16)\n> \tkeeps the external process running and processes all commands\n\nI am afraid that this organization of the patch series is making it\nharder than necessary to review, by duplicating the same code first\n_WITH_ renaming of symbols, etc., in this step and then updaing the\noriginal callers while removing the now-stale original callee\nimplementation in a separate next step.\n\nWould it perhaps make it clearer to understand what is going on if\nyou instead started to update the code in convert.c in place to make\nit more suitable fro reuse as the patch title advertises, and then\nmove the resulting cleaned-up code to a separate file, I wonder.\n\n"},{"id":"315240","messageId":"BL2PR03MB32362E9532BFD2C895C1B73F43E0@BL2PR03MB323.namprd03.prod.outlook.com","threadId":"45466","inReplyTo":"xmqqefxpjc5b.fsf@gitster.mtv.corp.google.com","subject":"RE: [PATCH v1 1/3] pkt-line: add packet_write_list_gently()","fromName":"Ben Peart","fromEmail":"ben.peart@microsoft.com","sentAt":"2017-03-24T12:34:25Z","receivedAt":"2017-03-24T12:34:41Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"> -----Original Message-----\n> From: Junio C Hamano [mailto:gitster@pobox.com]\n> Sent: Wednesday, March 22, 2017 4:21 PM\n> To: Ben Peart <peartben@gmail.com>\n> Cc: git@vger.kernel.org; Ben Peart <Ben.Peart@microsoft.com>;\n> christian.couder@gmail.com; larsxschneider@gmail.com\n> Subject: Re: [PATCH v1 1/3] pkt-line: add packet_write_list_gently()\n> \n> Ben Peart <peartben@gmail.com> writes:\n> \n> > Add packet_write_list_gently() which writes multiple lines in a single\n> > call and then calls packet_flush_gently(). This is used later in this\n> > patch series.\n> \n> I can see how it would be convenient to have a function like this.\n> I'd name it without _gently(), though.  We call something _gently() when we\n> initially only have a function that dies hard on error and later want to have a\n> variant that returns an error for the caller to handle.  You are starting\n> without a dying variant (which is probably a preferable way to structure the\n> API).\n> \n> Also I am hesitant to take a function that does not take any \"list\"\n> type argument and yet calls itself \"write_list\".  IOW, I'd expect something like\n> these\n> \n> \twrite_list(int fd, const char **lines);\n> \twrite_list(int fd, struct string_list *lines);\n> \n> given that name, and not \"varargs, each of which is a line\".  I am tempted to\n> suggest\n> \n> \tpacket_writel(int fd, const char *line, ...);\n> \n> noticing similarity with execl(), but perhaps others may be able to come up\n> with better names.\n\nGiven there haven't been any better names suggested, I'll go ahead and update it to be packet_writel.\n\n> \n> > Signed-off-by: Ben Peart <benpeart@microsoft.com>\n> > ---\n> >  pkt-line.c | 19 +++++++++++++++++++\n> >  pkt-line.h |  1 +\n> >  2 files changed, 20 insertions(+)\n> >\n> > diff --git a/pkt-line.c b/pkt-line.c\n> > index d4b6bfe076..fccdac1352 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\n> *fmt, ...)\n> >  \treturn status;\n> >  }\n> >\n> > +int packet_write_list_gently(int fd, const char *line, ...) {\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,\n> > size_t size)  {\n> >  \tstatic char packet_write_buffer[LARGE_PACKET_MAX];\n> > diff --git a/pkt-line.h b/pkt-line.h\n> > index 18eac64830..3674d04509 100644\n> > --- a/pkt-line.h\n> > +++ b/pkt-line.h\n> > @@ -25,6 +25,7 @@ void packet_buf_flush(struct strbuf *buf);  void\n> > packet_buf_write(struct strbuf *buf, const char *fmt, ...)\n> > __attribute__((format (printf, 2, 3)));  int packet_flush_gently(int\n> > fd);  int packet_write_fmt_gently(int fd, const char *fmt, ...)\n> > __attribute__((format (printf, 2, 3)));\n> > +int packet_write_list_gently(int fd, const char *line, ...);\n> >  int write_packetized_from_fd(int fd_in, int fd_out);  int\n> > write_packetized_from_buf(const char *src_in, size_t len, int fd_out);\n"},{"id":"315243","messageId":"BL2PR03MB32308D0F2BB1C7D6F04141BF43E0@BL2PR03MB323.namprd03.prod.outlook.com","threadId":"45466","inReplyTo":"xmqqk27gfrga.fsf@gitster.mtv.corp.google.com","subject":"RE: [PATCH v1 2/3] sub-process: refactor the filter process code into a reusable module","fromName":"Ben Peart","fromEmail":"ben.peart@microsoft.com","sentAt":"2017-03-24T12:39:07Z","receivedAt":"2017-03-24T12:39:15Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"> -----Original Message-----\n> From: Junio C Hamano [mailto:gitster@pobox.com]\n> Sent: Thursday, March 23, 2017 2:17 AM\n> To: Ben Peart <peartben@gmail.com>\n> Cc: git@vger.kernel.org; Ben Peart <Ben.Peart@microsoft.com>;\n> christian.couder@gmail.com; larsxschneider@gmail.com\n> Subject: Re: [PATCH v1 2/3] sub-process: refactor the filter process code into\n> a reusable module\n> \n> Ben Peart <peartben@gmail.com> writes:\n> \n> > This code is refactored from:\n> >\n> > \tCommit edcc85814c (\"convert: add filter.<driver>.process option\",\n> 2016-10-16)\n> > \tkeeps the external process running and processes all commands\n> \n> I am afraid that this organization of the patch series is making it harder than\n> necessary to review, by duplicating the same code first _WITH_ renaming of\n> symbols, etc., in this step and then updaing the original callers while\n> removing the now-stale original callee implementation in a separate next\n> step.\n> \n> Would it perhaps make it clearer to understand what is going on if you\n> instead started to update the code in convert.c in place to make it more\n> suitable fro reuse as the patch title advertises, and then move the resulting\n> cleaned-up code to a separate file, I wonder.\n\nI'm not entirely sure what you're asking for here but I think the\nchallenge may be that by splitting the refactoring into two separate\ncommits, it's hard to see the before and after when looking at the\ncommit in isolation.  By splitting them, it's more \"a bunch of new code\"\nfollowed by \"using new code\" than it is a refactoring of existing code.\n\nHow about I squash the last two patches together so that its more\napparent that it's just a refactoring of existing code with the before\nand after in a single patch?\n"},{"id":"315267","messageId":"xmqqpoh68xld.fsf@gitster.mtv.corp.google.com","threadId":"45466","inReplyTo":"BL2PR03MB32308D0F2BB1C7D6F04141BF43E0@BL2PR03MB323.namprd03.prod.outlook.com","subject":"Re: [PATCH v1 2/3] sub-process: refactor the filter process code into a reusable module","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-24T16:10:22Z","receivedAt":"2017-03-24T16:17:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Peart <Ben.Peart@microsoft.com> writes:\n\n> How about I squash the last two patches together so that its more\n> apparent that it's just a refactoring of existing code with the before\n> and after in a single patch?\n\nI do not think making a pair of patches, each already does too much,\ninto one patch would make things easier to chew.  It would make it\neven harder to understand.\n\nI offhand do not know how many patches the ideal split of this\nseries should be, but I know it shouldn't be one.\n\nMore likely that it should be N+1, and I know the last one should be\n\"Now all data structures and variables have been renamed, all helper\nfunctions have been renamed and generalized for reuse while the\noriginal code these helper functions came from have been updated to\ncall them, we can move them from convert.c to sub-process.[ch];\nlet's create these two files\".  This last step will be moving lines\nfrom an old file to two new files, almost without any modification\n(of course, \"#ifndef SUB_PROCESS_H\", \"#include sub-process.h\", etc.\nwould be new lines so this will not be strictnly pure movement, but\nall readers of this message are intelligent enough to understand\nwhat I mean).\n\nWhat would the other N patches before the last step should contain,\nthen?\n\nFor example, your name2process_cmp() is just a renamed version of\nthe original.  Some of its callers may also just straight rename.\nThese \"only renaming, doing nothing else\" changes can go into a\nsingle large patch and as long as you mark as such, the review\nburden can be lessened.  It would be a \"boring mechanical\" part of\nthe whole thing.\n\nOn the other hand, your subprocess_start() shares quite a lot with\nthe original start_multi_file_filter() but the latter does some more\nthan the former (because the latter is more specific to the need of\nconvert.c).  \n\nA patch series must be structured to make it easier to review such\nchanges, because they are the likely places where mistakes happen\n(both in implementation and in the helper API design).  To get from\nthe original start_multi_file_filter() to a new version that calls\nsubprocess_start(), such a patch would _MOVE_ bunch of lines from\nthe old function to the new function, the new function may acquire\nits own unique new lines, the old function would lose these moved\nlines but instead call the new function, etc.  You can call it as \"I\nrefactored subprocess_start() out of start_multi_file_filter() and\nupdated the latter to call the former.\" and that would be a single\nlogical unit of the change.  To make patches easier to understand,\nthese logical unit would want to be reasonably small.\n\nAnd for something of sub-process.[ch]'s size, I suspect that it\nwould have more than 1 such logical unit to be independently\nrefactored, so in total, I would suspect the series would become\n\n    1 (boring mechanical part) +\n    2 or more (refactoring) +\n    1 (final movement)\n\ni.e. 4 or more patches?\n"},{"id":"315276","messageId":"xmqqd1d68ukr.fsf@gitster.mtv.corp.google.com","threadId":"45466","inReplyTo":"xmqqpoh68xld.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1 2/3] sub-process: refactor the filter process code into a reusable module","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-24T17:15:32Z","receivedAt":"2017-03-24T17:15:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> ...\n> And for something of sub-process.[ch]'s size, I suspect that it\n> would have more than 1 such logical unit to be independently\n> refactored, so in total, I would suspect the series would become\n>\n>     1 (boring mechanical part) +\n>     2 or more (refactoring) +\n>     1 (final movement)\n>\n> i.e. 4 or more patches?\n\nTo avoid confusion (although readers may not require), even though I\nexplained \"boring mechanical part\" first and \"refactoring\", that was\npurely for explanation.  \n\nIn practice, I would expect that it would be easier to both do and\nreview if refactoring is done with the original name.  \n\nA function that will keep its name in the final result (e.g.\nstart_multi_file_filter()) will call a new and more generic helper\nfunction.  The new helper may start using the new name from the\nget-go (e.g. subprocess_start()), but the data types it shares with\nthe original part of the code (e.g. 'struct cmd2process') may still\nbe using the original name.\n\nAnd after completing \"2 or more\" refactoring would be a good place\nto do the remaining \"boring mechanical rename\".  IOW, the count\nabove could be\n\n     2 or more (refactoring) +\n     1 (boring mechanical part) +\n     1 (final movement)\n\nand I didn't mean to say that you need to rename first.  What we\nwant is \"if you need to have a single large patch that cannot be\nsplit, see if you can make it purely mechanical.\", as a single large\npatch that is _not_ mechanical conversion is the worst kind of patch\nfor reviewers.\n"},{"id":"315384","messageId":"CACsJy8DyqxUCXievDON7=D9gNv=mWm=0HZ_t4yj6=xddUp5CaA@mail.gmail.com","threadId":"45466","inReplyTo":"20170322165220.5660-1-benpeart@microsoft.com","subject":"Re: [PATCH v1 0/3] Add support for downloading blobs on demand","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-03-25T11:59:02Z","receivedAt":"2017-03-25T11:59:37Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Mar 22, 2017 at 11:52 PM, Ben Peart <peartben@gmail.com> wrote:\n> We have a couple of patch series we’re working on (ObjectDB/Read-Object,\n> Watchman integration)\n\nOops, sorry. I should be reworking the index-helper series for\nwatchman support, but I haven't time for it. Yes I'm also eyeing Lars\ncode as the communication channel for any external helper (which in\nturn can talk to watchman or whatever). I guess I'll let you do it\nthen.\n-- \nDuy\n"},{"id":"315540","messageId":"BL2PR03MB3237E6E03677DAFB22F58FEF4330@BL2PR03MB323.namprd03.prod.outlook.com","threadId":"45466","inReplyTo":"xmqqd1d68ukr.fsf@gitster.mtv.corp.google.com","subject":"RE: [PATCH v1 2/3] sub-process: refactor the filter process code into a reusable module","fromName":"Ben Peart","fromEmail":"ben.peart@microsoft.com","sentAt":"2017-03-27T22:04:50Z","receivedAt":"2017-03-27T22:21:21Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"> Junio C Hamano <gitster@pobox.com> writes:\n> \n> \n> To avoid confusion (although readers may not require), even though I\n> explained \"boring mechanical part\" first and \"refactoring\", that was purely\n> for explanation.\n> \n> In practice, I would expect that it would be easier to both do and review if\n> refactoring is done with the original name.\n> \n> A function that will keep its name in the final result (e.g.\n> start_multi_file_filter()) will call a new and more generic helper function.  The\n> new helper may start using the new name from the get-go (e.g.\n> subprocess_start()), but the data types it shares with the original part of the\n> code (e.g. 'struct cmd2process') may still be using the original name.\n> \n> And after completing \"2 or more\" refactoring would be a good place to do\n> the remaining \"boring mechanical rename\".  IOW, the count above could be\n> \n>      2 or more (refactoring) +\n>      1 (boring mechanical part) +\n>      1 (final movement)\n> \n> and I didn't mean to say that you need to rename first.  What we want is \"if\n> you need to have a single large patch that cannot be split, see if you can\n> make it purely mechanical.\", as a single large patch that is _not_ mechanical\n> conversion is the worst kind of patch for reviewers.\n\nThanks, I think I better understand what you are looking for in a patch series.  In short, any non trivial refactoring should take place within the same file using 1 or more patches to keep each patch as simple as possible.  Any large or cross file refactoring should be made as boring/mechanical as possible. This is to make it easier to see any complex changes within a single format patch section and avoid having to look between two file patches to ensure the refactoring didn't unintentionally change behavior.\n\nI'll throw out my current refactoring and do it again attempting to follow these guidelines as soon as I can find the time ($DAYJOB tends to take priority over my open source work).\n\n"}]}