{"thread":{"id":"45496","subject":"[PATCH v2 2/2] sub-process: refactor the filter process code into a reusable module","startedAt":"2017-03-24T15:27:56Z","lastAt":"2017-03-30T17:01:57Z","messageCount":15,"participants":["Ben Peart","Torsten Bögershausen","Jonathan Tan"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"315255","messageId":"20170324152726.14632-4-benpeart@microsoft.com","threadId":"45496","inReplyTo":"20170324152726.14632-1-benpeart@microsoft.com","subject":"[PATCH v2 2/2] sub-process: refactor the filter process code into a reusable module","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-03-24T15:27:23Z","receivedAt":"2017-03-24T15:27:56Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"Refactor the filter.<driver>.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.<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 |  54 ++++++++++\n Makefile                                    |   1 +\n convert.c                                   | 160 ++++++----------------------\n sub-process.c                               | 117 ++++++++++++++++++++\n sub-process.h                               |  46 ++++++++\n 5 files changed, 248 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\ndiff --git a/Documentation/technical/api-sub-process.txt b/Documentation/technical/api-sub-process.txt\nnew file mode 100644\nindex 0000000000..eb5005aa72\n--- /dev/null\n+++ b/Documentation/technical/api-sub-process.txt\n@@ -0,0 +1,54 @@\n+sub-process API\n+===============\n+\n+The sub-process API makes it possible to run background sub-processes\n+that should run until the git command exits and communicate with it\n+through stdin and stdout.  This reduces the overhead of having to fork\n+a new process each time it needs to be communicated with.\n+\n+The sub-processes are kept in a hashmap by command name and looked up\n+via the subprocess_find_entry function.  If an existing instance can not\n+be found then a new process should be created and started.  When the\n+parent git command terminates, all sub-processes are also terminated.\n+\n+This API is based on the run-command API.\n+\n+Data structures\n+---------------\n+\n+* `struct subprocess_entry`\n+\n+The sub-process structure.  Members should not be accessed directly.\n+\n+Types\n+-----\n+\n+'int(*subprocess_start_fn)(struct subprocess_entry *entry)'::\n+\n+\tUser-supplied function to initialize the sub-process.  This is\n+\ttypically used to negoiate the interface version and capabilities.\n+\n+\n+Functions\n+---------\n+\n+`subprocess_start`::\n+\n+\tStart a subprocess and add it to the subprocess hashmap.\n+\n+`subprocess_stop`::\n+\n+\tKill a subprocess and remove it from the subprocess hashmap.\n+\n+`subprocess_find_entry`::\n+\n+\tFind a subprocess in the subprocess hashmap.\n+\n+`subprocess_get_child_process`::\n+\n+\tGet the underlying `struct child_process` from a subprocess.\n+\n+`subprocess_read_status`::\n+\n+\tHelper function to read packets looking for the last \"status=<foo>\"\n+\tkey/value pair.\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/convert.c b/convert.c\nindex 8d652bf27c..565a1d0d8c 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+static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)\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-{\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_writel(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_writel(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,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-\tread_multi_file_filter_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@@ -746,7 +642,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-\tread_multi_file_filter_status(process->out, &filter_status);\n+\terr = subprocess_read_status(process->out, &filter_status);\n+\tif (err)\n+\t\tgoto done;\n+\n \terr = strcmp(filter_status.buf, \"success\");\n \n done:\n@@ -768,7 +667,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);\ndiff --git a/sub-process.c b/sub-process.c\nnew file mode 100644\nindex 0000000000..5a06a9fd1e\n--- /dev/null\n+++ b/sub-process.c\n@@ -0,0 +1,117 @@\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+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\tlen = packet_read_line_gently(fd, NULL, &line);\n+\t\tif ((len == -1) || !line)\n+\t\t\tbreak;\n+\t\tpair = strbuf_split_str(line, '=', 2);\n+\t\tif (pair[0] && pair[0]->len && pair[1]) {\n+\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+\treturn len == -1 ? len : 0;\n+}\ndiff --git a/sub-process.h b/sub-process.h\nnew file mode 100644\nindex 0000000000..d1492f476d\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+int subprocess_read_status(int fd, struct strbuf *status);\n+\n+#endif\n-- \n2.12.0.gvfs.1.43.g876ba2a\n\n"},{"id":"315256","messageId":"20170324152726.14632-3-benpeart@microsoft.com","threadId":"45496","inReplyTo":"20170324152726.14632-1-benpeart@microsoft.com","subject":"[PATCH v2 1/4] t7800: remove whitespace before redirect","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-03-24T15:27:22Z","receivedAt":"2017-03-24T15:27:58Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"From: David Aguilar <davvid@gmail.com>\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Ben Peart <benpeart@microsoft.com>\n---\n t/t7800-difftool.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 25241f4096..e1ec292718 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -428,7 +428,7 @@ run_dir_diff_test 'difftool --dir-diff branch from subdirectory' '\n \t\tgit difftool --dir-diff $symlinks --extcmd ls branch >output &&\n \t\t# \"sub\" must only exist in \"right\"\n \t\t# \"file\" and \"file2\" must be listed in both \"left\" and \"right\"\n-\t\tgrep sub output > sub-output &&\n+\t\tgrep sub output >sub-output &&\n \t\ttest_line_count = 1 sub-output &&\n \t\tgrep file\"$\" output >file-output &&\n \t\ttest_line_count = 2 file-output &&\n-- \n2.12.0.gvfs.1.43.g876ba2a\n\n"},{"id":"315257","messageId":"20170324152726.14632-1-benpeart@microsoft.com","threadId":"45496","inReplyTo":null,"subject":"[PATCH v2 0/2] Refactor the filter process code into a reusable module","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-03-24T15:27:20Z","receivedAt":"2017-03-24T15:28:02Z","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 need the ability to have a background\nprocess running that can accept multiple commands thus avoiding the\noverhead 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.\n\nBen Peart (2):\n  pkt-line: add packet_writel() and packet_read_line_gently()\n  sub-process: refactor the filter process code into a reusable module\n\n Documentation/technical/api-sub-process.txt |  54 ++++++++++\n Makefile                                    |   1 +\n convert.c                                   | 160 ++++++----------------------\n pkt-line.c                                  |  31 ++++++\n pkt-line.h                                  |  11 ++\n sub-process.c                               | 117 ++++++++++++++++++++\n sub-process.h                               |  46 ++++++++\n 7 files changed, 290 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.43.g876ba2a\n\n"},{"id":"315258","messageId":"20170324152726.14632-5-benpeart@microsoft.com","threadId":"45496","inReplyTo":"20170324152726.14632-1-benpeart@microsoft.com","subject":"[PATCH v2 2/4] t7800: cleanup cruft left behind by tests","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-03-24T15:27:24Z","receivedAt":"2017-03-24T15:28:04Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"From: David Aguilar <davvid@gmail.com>\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Ben Peart <benpeart@microsoft.com>\n---\n t/t7800-difftool.sh | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex e1ec292718..e0e65df8de 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -591,6 +591,7 @@ test_expect_success 'difftool --no-symlinks detects conflict ' '\n '\n \n test_expect_success 'difftool properly honors gitlink and core.worktree' '\n+\ttest_when_finished rm -rf submod/ule &&\n \tgit submodule add ./. submod/ule &&\n \ttest_config -C submod/ule diff.tool checktrees &&\n \ttest_config -C submod/ule difftool.checktrees.cmd '\\''\n@@ -600,11 +601,13 @@ test_expect_success 'difftool properly honors gitlink and core.worktree' '\n \t\tcd submod/ule &&\n \t\techo good >expect &&\n \t\tgit difftool --tool=checktrees --dir-diff HEAD~ >actual &&\n-\t\ttest_cmp expect actual\n+\t\ttest_cmp expect actual &&\n+\t\trm -f expect actual\n \t)\n '\n \n test_expect_success SYMLINKS 'difftool --dir-diff symlinked directories' '\n+\ttest_when_finished git reset --hard &&\n \tgit init dirlinks &&\n \t(\n \t\tcd dirlinks &&\n-- \n2.12.0.gvfs.1.43.g876ba2a\n\n"},{"id":"315259","messageId":"20170324152726.14632-6-benpeart@microsoft.com","threadId":"45496","inReplyTo":"20170324152726.14632-1-benpeart@microsoft.com","subject":"[PATCH v2 3/4] difftool: handle modified symlinks in dir-diff mode","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-03-24T15:27:25Z","receivedAt":"2017-03-24T15:28:12Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"From: David Aguilar <davvid@gmail.com>\n\nDetect the null object ID for symlinks in dir-diff so that difftool can\ndetect when symlinks are modified in the worktree.\n\nPreviously, a null symlink object ID would crash difftool.\nHandle null object IDs as unknown content that must be read from\nthe worktree.\n\nHelped-by: Johannes Schindelin <johannes.schindelin@gmx.de>\nSigned-off-by: David Aguilar <davvid@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Ben Peart <benpeart@microsoft.com>\n---\n builtin/difftool.c  | 51 ++++++++++++++++++++++++++++++++++++++++-----\n t/t7800-difftool.sh | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 106 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex d13350ce83..25e54ad3ed 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -254,6 +254,49 @@ static int ensure_leading_directories(char *path)\n \t}\n }\n \n+/*\n+ * Unconditional writing of a plain regular file is what\n+ * \"git difftool --dir-diff\" wants to do for symlinks.  We are preparing two\n+ * temporary directories to be fed to a Git-unaware tool that knows how to\n+ * show a diff of two directories (e.g. \"diff -r A B\").\n+ *\n+ * Because the tool is Git-unaware, if a symbolic link appears in either of\n+ * these temporary directories, it will try to dereference and show the\n+ * difference of the target of the symbolic link, which is not what we want,\n+ * as the goal of the dir-diff mode is to produce an output that is logically\n+ * equivalent to what \"git diff\" produces.\n+ *\n+ * Most importantly, we want to get textual comparison of the result of the\n+ * readlink(2).  get_symlink() provides that---it returns the contents of\n+ * the symlink that gets written to a regular file to force the external tool\n+ * to compare the readlink(2) result as text, even on a filesystem that is\n+ * capable of doing a symbolic link.\n+ */\n+static char *get_symlink(const struct object_id *oid, const char *path)\n+{\n+\tchar *data;\n+\tif (is_null_oid(oid)) {\n+\t\t/* The symlink is unknown to Git so read from the filesystem */\n+\t\tstruct strbuf link = STRBUF_INIT;\n+\t\tif (has_symlinks) {\n+\t\t\tif (strbuf_readlink(&link, path, strlen(path)))\n+\t\t\t\tdie(_(\"could not read symlink %s\"), path);\n+\t\t} else if (strbuf_read_file(&link, path, 128))\n+\t\t\tdie(_(\"could not read symlink file %s\"), path);\n+\n+\t\tdata = strbuf_detach(&link, NULL);\n+\t} else {\n+\t\tenum object_type type;\n+\t\tunsigned long size;\n+\t\tdata = read_sha1_file(oid->hash, &type, &size);\n+\t\tif (!data)\n+\t\t\tdie(_(\"could not read object %s for symlink %s\"),\n+\t\t\t\toid_to_hex(oid), path);\n+\t}\n+\n+\treturn data;\n+}\n+\n static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\t\tint argc, const char **argv)\n {\n@@ -270,8 +313,6 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \tstruct hashmap working_tree_dups, submodules, symlinks2;\n \tstruct hashmap_iter iter;\n \tstruct pair_entry *entry;\n-\tenum object_type type;\n-\tunsigned long size;\n \tstruct index_state wtindex;\n \tstruct checkout lstate, rstate;\n \tint rc, flags = RUN_GIT_CMD, err = 0;\n@@ -377,13 +418,13 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\t}\n \n \t\tif (S_ISLNK(lmode)) {\n-\t\t\tchar *content = read_sha1_file(loid.hash, &type, &size);\n+\t\t\tchar *content = get_symlink(&loid, src_path);\n \t\t\tadd_left_or_right(&symlinks2, src_path, content, 0);\n \t\t\tfree(content);\n \t\t}\n \n \t\tif (S_ISLNK(rmode)) {\n-\t\t\tchar *content = read_sha1_file(roid.hash, &type, &size);\n+\t\t\tchar *content = get_symlink(&roid, dst_path);\n \t\t\tadd_left_or_right(&symlinks2, dst_path, content, 1);\n \t\t\tfree(content);\n \t\t}\n@@ -397,7 +438,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\t\t\treturn error(\"could not write '%s'\", src_path);\n \t\t}\n \n-\t\tif (rmode) {\n+\t\tif (rmode && !S_ISLNK(rmode)) {\n \t\t\tstruct working_tree_entry *entry;\n \n \t\t\t/* Avoid duplicate working_tree entries */\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex e0e65df8de..0e7f30db2d 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -626,4 +626,64 @@ test_expect_success SYMLINKS 'difftool --dir-diff symlinked directories' '\n \t)\n '\n \n+test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '\n+\ttest_when_finished git reset --hard &&\n+\ttouch b &&\n+\tln -s b c &&\n+\tgit add b c &&\n+\ttest_tick &&\n+\tgit commit -m initial &&\n+\ttouch d &&\n+\trm c &&\n+\tln -s d c &&\n+\tcat >expect <<-EOF &&\n+\t\tb\n+\t\tc\n+\n+\t\tc\n+\tEOF\n+\tgit difftool --symlinks --dir-diff --extcmd ls >output &&\n+\tgrep -v ^/ output >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tgit difftool --no-symlinks --dir-diff --extcmd ls >output &&\n+\tgrep -v ^/ output >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# The left side contains symlink \"c\" that points to \"b\"\n+\ttest_config difftool.cat.cmd \"cat \\$LOCAL/c\" &&\n+\tprintf \"%s\\n\" b >expect &&\n+\n+\tgit difftool --symlinks --dir-diff --tool cat >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tgit difftool --symlinks --no-symlinks --dir-diff --tool cat >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# The right side contains symlink \"c\" that points to \"d\"\n+\ttest_config difftool.cat.cmd \"cat \\$REMOTE/c\" &&\n+\tprintf \"%s\\n\" d >expect &&\n+\n+\tgit difftool --symlinks --dir-diff --tool cat >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tgit difftool --no-symlinks --dir-diff --tool cat >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# Deleted symlinks\n+\trm -f c &&\n+\tcat >expect <<-EOF &&\n+\t\tb\n+\t\tc\n+\n+\tEOF\n+\tgit difftool --symlinks --dir-diff --extcmd ls >output &&\n+\tgrep -v ^/ output >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tgit difftool --no-symlinks --dir-diff --extcmd ls >output &&\n+\tgrep -v ^/ output >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.12.0.gvfs.1.43.g876ba2a\n\n"},{"id":"315260","messageId":"20170324152726.14632-7-benpeart@microsoft.com","threadId":"45496","inReplyTo":"20170324152726.14632-1-benpeart@microsoft.com","subject":"[PATCH v2 4/4] Git 2.12.1","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-03-24T15:27:26Z","receivedAt":"2017-03-24T15:28:13Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"From: Junio C Hamano <gitster@pobox.com>\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Ben Peart <benpeart@microsoft.com>\n---\n Documentation/git.txt | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex aa895da4a5..25560f69ff 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -44,9 +44,10 @@ unreleased) version of Git, that is available from the 'master'\n branch of the `git.git` repository.\n Documentation for older releases are available here:\n \n-* link:v2.12.0/git.html[documentation for release 2.12.0]\n+* link:v2.12.1/git.html[documentation for release 2.12.1]\n \n * release notes for\n+  link:RelNotes/2.12.1.txt[2.12.1].\n   link:RelNotes/2.12.0.txt[2.12].\n \n * link:v2.11.1/git.html[documentation for release 2.11.1]\n-- \n2.12.0.gvfs.1.43.g876ba2a\n\n"},{"id":"315261","messageId":"20170324152726.14632-2-benpeart@microsoft.com","threadId":"45496","inReplyTo":"20170324152726.14632-1-benpeart@microsoft.com","subject":"[PATCH v2 1/2] pkt-line: add packet_writel() and packet_read_line_gently()","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-03-24T15:27:21Z","receivedAt":"2017-03-24T15:28:22Z","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(). Add packet_read_line_gently() to\nenable reading a line without dying on EOF.\n\nSigned-off-by: Ben Peart <benpeart@microsoft.com>\n---\n pkt-line.c | 31 +++++++++++++++++++++++++++++++\n pkt-line.h | 11 +++++++++++\n 2 files changed, 42 insertions(+)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex d4b6bfe076..2788aa1af6 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -171,6 +171,25 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)\n \treturn status;\n }\n \n+int packet_writel(int fd, const char *line, ...)\n+{\n+\tva_list args;\n+\tint err;\n+\tva_start(args, line);\n+\tfor (;;) {\n+\t\tif (!line)\n+\t\t\tbreak;\n+\t\tif (strlen(line) > LARGE_PACKET_DATA_MAX)\n+\t\t\treturn -1;\n+\t\terr = packet_write_fmt_gently(fd, \"%s\\n\", line);\n+\t\tif (err)\n+\t\t\treturn err;\n+\t\tline = va_arg(args, const char*);\n+\t}\n+\tva_end(args);\n+\treturn packet_flush_gently(fd);\n+}\n+\n static int packet_write_gently(const int fd_out, const char *buf, size_t size)\n {\n \tstatic char packet_write_buffer[LARGE_PACKET_MAX];\n@@ -323,6 +342,18 @@ char *packet_read_line(int fd, int *len_p)\n \treturn packet_read_line_generic(fd, NULL, NULL, len_p);\n }\n \n+int packet_read_line_gently(int fd, int *dst_len, char** dst_line)\n+{\n+\tint len = packet_read(fd, NULL, NULL,\n+\t\tpacket_buffer, sizeof(packet_buffer),\n+\t\tPACKET_READ_CHOMP_NEWLINE|PACKET_READ_GENTLE_ON_EOF);\n+\tif (dst_len)\n+\t\t*dst_len = len;\n+\tif (dst_line)\n+\t\t*dst_line = (len > 0) ? packet_buffer : NULL;\n+\treturn len;\n+}\n+\n char *packet_read_line_buf(char **src, size_t *src_len, int *dst_len)\n {\n \treturn packet_read_line_generic(-1, src, src_len, dst_len);\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 18eac64830..cb3eda9695 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -25,6 +25,7 @@ void packet_buf_flush(struct strbuf *buf);\n void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n int packet_flush_gently(int fd);\n int packet_write_fmt_gently(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n+int packet_writel(int fd, const char *line, ...);\n int write_packetized_from_fd(int fd_in, int fd_out);\n int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);\n \n@@ -74,6 +75,16 @@ int packet_read(int fd, char **src_buffer, size_t *src_len, char\n char *packet_read_line(int fd, int *size);\n \n /*\n+ * Convenience wrapper for packet_read that sets the PACKET_READ_GENTLE_ON_EOF\n+ * and CHOMP_NEWLINE options. The return value specifies the number of bytes\n+ * read into the buffer or -1 on truncated input. if the *dst_line parameter\n+ * is not NULL it will return NULL for a flush packet and otherwise points to\n+ * a static buffer (that may be overwritten by subsequent calls). If the size\n+ * parameter is not NULL, the length of the packet is written to it.\n+ */\n+int packet_read_line_gently(int fd, int *size, char** dst_line);\n+\n+/*\n  * Same as packet_read_line, but read from a buf rather than a descriptor;\n  * see packet_read for details on how src_* is used.\n  */\n-- \n2.12.0.gvfs.1.43.g876ba2a\n\n"},{"id":"315268","messageId":"BL2PR03MB32312344027A7C07573AC85F43E0@BL2PR03MB323.namprd03.prod.outlook.com","threadId":"45496","inReplyTo":"20170324152726.14632-3-benpeart@microsoft.com","subject":"RE: [PATCH v2 1/4] t7800: remove whitespace before redirect","fromName":"Ben Peart","fromEmail":"ben.peart@microsoft.com","sentAt":"2017-03-24T16:21:55Z","receivedAt":"2017-03-24T16:22:16Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"Please disregard.  I apologize - somehow my format-patch/send-email went awry. \n\nThanks,\n\nBen\n\n> -----Original Message-----\n> From: Ben Peart [mailto:peartben@gmail.com]\n> Sent: Friday, March 24, 2017 11:27 AM\n> To: git@vger.kernel.org\n> Cc: Ben Peart <Ben.Peart@microsoft.com>; christian.couder@gmail.com;\n> larsxschneider@gmail.com; David Aguilar <davvid@gmail.com>; Junio C\n> Hamano <gitster@pobox.com>\n> Subject: [PATCH v2 1/4] t7800: remove whitespace before redirect\n> \n> From: David Aguilar <davvid@gmail.com>\n> \n> Signed-off-by: David Aguilar <davvid@gmail.com>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> Signed-off-by: Ben Peart <benpeart@microsoft.com>\n> ---\n>  t/t7800-difftool.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh index\n> 25241f4096..e1ec292718 100755\n> --- a/t/t7800-difftool.sh\n> +++ b/t/t7800-difftool.sh\n> @@ -428,7 +428,7 @@ run_dir_diff_test 'difftool --dir-diff branch from\n> subdirectory' '\n>  \t\tgit difftool --dir-diff $symlinks --extcmd ls branch >output &&\n>  \t\t# \"sub\" must only exist in \"right\"\n>  \t\t# \"file\" and \"file2\" must be listed in both \"left\" and \"right\"\n> -\t\tgrep sub output > sub-output &&\n> +\t\tgrep sub output >sub-output &&\n>  \t\ttest_line_count = 1 sub-output &&\n>  \t\tgrep file\"$\" output >file-output &&\n>  \t\ttest_line_count = 2 file-output &&\n> --\n> 2.12.0.gvfs.1.43.g876ba2a\n\n"},{"id":"315374","messageId":"7a2c73c8-bb09-12ec-dd8d-99c5363f9bb4@web.de","threadId":"45496","inReplyTo":"20170324152726.14632-2-benpeart@microsoft.com","subject":"Re: [PATCH v2 1/2] pkt-line: add packet_writel() and packet_read_line_gently()","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2017-03-25T05:47:09Z","receivedAt":"2017-03-25T05:47:22Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2017-03-24 16:27, Ben Peart wrote:\n> Add packet_writel() which writes multiple lines in a single call and\n> then calls packet_flush_gently(). Add packet_read_line_gently() to\n> enable reading a line without dying on EOF.\n> \n> Signed-off-by: Ben Peart <benpeart@microsoft.com>\n> ---\n>  pkt-line.c | 31 +++++++++++++++++++++++++++++++\n>  pkt-line.h | 11 +++++++++++\n>  2 files changed, 42 insertions(+)\n> \n> diff --git a/pkt-line.c b/pkt-line.c\n> index d4b6bfe076..2788aa1af6 100644\n> --- a/pkt-line.c\n> +++ b/pkt-line.c\n> @@ -171,6 +171,25 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)\n>  \treturn status;\n>  }\n>  \n> +int packet_writel(int fd, const char *line, ...)\nThe name packet_writel is hard to distinguish from packet_write.\nWould packet_write_lines make more sense ?\n\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> +\nI don't think that this va_start() is needed, even if it works.\n\nint packet_write_line(int fd, const char *lines[])\n|\n\tconst char *line = *lines;\n\tint err;\n\twhile (line) {\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\tlines++;\n\t\tline = *lines;\n\t}\n\treturn packet_flush_gently(fd);\n]\n\n\n"},{"id":"315524","messageId":"c8f85b4a-e69e-76ab-9a8d-66857968fb4d@google.com","threadId":"45496","inReplyTo":"20170324152726.14632-4-benpeart@microsoft.com","subject":"Re: [PATCH v2 2/2] sub-process: refactor the filter process code into a reusable module","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-03-27T18:59:35Z","receivedAt":"2017-03-27T19:00:56Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 03/24/2017 08:27 AM, Ben Peart wrote:\n> Refactor the filter.<driver>.process code into a separate sub-process\n> module that can be used to reduce the cost of starting up a sub-process\n> for multiple commands.  It does this by keeping the external process\n> running and processing all commands by communicating over standard input\n> and standard output using the packet format (pkt-line) based protocol.\n> Full documentation is in Documentation/technical/api-sub-process.txt.\n\nThanks - this looks like something useful to have.\n\nWhen you create a \"struct subprocess_entry\" to be entered into the \nsystem, it is not a true \"struct subprocess_entry\" - it is a \"struct \nsubprocess_entry\" plus some extra variables at the end. Since the \nsub-process hashmap is keyed solely on the command, what happens if \nanother component uses the same trick (but with different extra \nvariables) when using a sub-process with the same command?\n\nI can think of at least two ways to solve this: (i) each component can \nhave its own sub-process hashmap, or (ii) add a component key to the \nhashmap. (i) seems more elegant to me, but I'm not sure what the code \nwill look like.\n\nAlso, I saw some minor code improvements possible (e.g. using \n\"starts_with\" when you're checking for the \"status=<foo>\" line) but I'll \ncomment on those and look into the code more thoroughly once the \nquestions in this e-mail are resolved.\n\n> diff --git a/sub-process.h b/sub-process.h\n> new file mode 100644\n> index 0000000000..d1492f476d\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\nI notice from the documentation (and from \"subprocess_get_child_process\" \nbelow) that this is meant to be opaque, but I think this can be \nnon-opaque (like \"run-command\").\n\nAlso, I would prefer adding a \"util\" pointer here instead of using it as \nan embedded struct. There is no clue here that it is embeddable or meant \nto be embedded.\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\nI'm not sure if it is useful to take a callback here - I think the \ncaller of this function can just run whatever it wants after a \nsuccessful subprocess_start.\n\nAlternatively, if you add the \"util\" pointer (as I described above), \nthen it makes sense to add a subprocess_get_or_start() function (and now \nit makes sense to take the callback). This way, the data structure will \nown, create, and destroy all the \"struct subprocess_entry\" that it \nneeds, creating a nice separation of concerns.\n\n> +\n> +void subprocess_stop(struct subprocess_entry *entry);\n\n(continued from above) And it would be clear that this would free \n\"entry\", for example.\n\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> +int subprocess_read_status(int fd, struct strbuf *status);\n> +\n> +#endif\n>\n"},{"id":"315541","messageId":"BL2PR03MB3233616BE57BB7D911B1AAEF4330@BL2PR03MB323.namprd03.prod.outlook.com","threadId":"45496","inReplyTo":"7a2c73c8-bb09-12ec-dd8d-99c5363f9bb4@web.de","subject":"RE: [PATCH v2 1/2] pkt-line: add packet_writel() and packet_read_line_gently()","fromName":"Ben Peart","fromEmail":"ben.peart@microsoft.com","sentAt":"2017-03-27T22:19:40Z","receivedAt":"2017-03-27T22:34:59Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"> -----Original Message-----\n> From: Torsten Bögershausen [mailto:tboegi@web.de]\n> Sent: Saturday, March 25, 2017 1:47 AM\n> To: Ben Peart <peartben@gmail.com>; git@vger.kernel.org\n> Cc: Ben Peart <Ben.Peart@microsoft.com>; christian.couder@gmail.com;\n> larsxschneider@gmail.com\n> Subject: Re: [PATCH v2 1/2] pkt-line: add packet_writel() and\n> packet_read_line_gently()\n> \n> On 2017-03-24 16:27, Ben Peart wrote:\n> > Add packet_writel() which writes multiple lines in a single call and\n> > then calls packet_flush_gently(). Add packet_read_line_gently() to\n> > enable reading a line without dying on EOF.\n> >\n> > Signed-off-by: Ben Peart <benpeart@microsoft.com>\n> > ---\n> >  pkt-line.c | 31 +++++++++++++++++++++++++++++++  pkt-line.h | 11\n> > +++++++++++\n> >  2 files changed, 42 insertions(+)\n> >\n> > diff --git a/pkt-line.c b/pkt-line.c\n> > index d4b6bfe076..2788aa1af6 100644\n> > --- a/pkt-line.c\n> > +++ b/pkt-line.c\n> > @@ -171,6 +171,25 @@ int packet_write_fmt_gently(int fd, const char\n> *fmt, ...)\n> >  \treturn status;\n> >  }\n> >\n> > +int packet_writel(int fd, const char *line, ...)\n> The name packet_writel is hard to distinguish from packet_write.\n> Would packet_write_lines make more sense ?\n> \n\nJust goes to prove that there are two hard things in computer science: cache invalidation, naming things, and off-by-one errors. :)  The feedback on V1 was:\n\n\"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 something 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 tempted to suggest\n\n\tpacket_writel(int fd, const char *line, ...);\"\n\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> I don't think that this va_start() is needed, even if it works.\n> \n> int packet_write_line(int fd, const char *lines[])\n> |\n> \tconst char *line = *lines;\n> \tint err;\n> \twhile (line) {\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\tlines++;\n> \t\tline = *lines;\n> \t}\n> \treturn packet_flush_gently(fd);\n> ]\n> \n\nThis is a helper function to simply the common pattern of writing out a variable number of lines followed by a flush.  The usage of the function as currently implemented is:\n\npacket_writel(fd, \"line one\", \"line two\", \"line n\");\n\nwhich requires the use of variable number of arguments.  With your proposal that convenience is lost as you have to create an array of strings and pass that instead.  The usage just isn't as simple as the current model.\n\n"},{"id":"315547","messageId":"BL2PR03MB3232F7FE70707532B5FB178F4330@BL2PR03MB323.namprd03.prod.outlook.com","threadId":"45496","inReplyTo":"c8f85b4a-e69e-76ab-9a8d-66857968fb4d@google.com","subject":"RE: [PATCH v2 2/2] sub-process: refactor the filter process code into a reusable module","fromName":"Ben Peart","fromEmail":"ben.peart@microsoft.com","sentAt":"2017-03-27T23:54:36Z","receivedAt":"2017-03-27T23:54:59Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"> -----Original Message-----\n> From: Jonathan Tan [mailto:jonathantanmy@google.com]\n> Sent: Monday, March 27, 2017 3:00 PM\n> To: Ben Peart <peartben@gmail.com>; git@vger.kernel.org\n> Cc: Ben Peart <Ben.Peart@microsoft.com>\n> Subject: Re: [PATCH v2 2/2] sub-process: refactor the filter process code into\n> a reusable module\n> \n> On 03/24/2017 08:27 AM, Ben Peart wrote:\n> > Refactor the filter.<driver>.process code into a separate sub-process\n> > module that can be used to reduce the cost of starting up a\n> > sub-process for multiple commands.  It does this by keeping the\n> > external process running and processing all commands by communicating\n> > over standard input and standard output using the packet format (pkt-line)\n> based protocol.\n> > Full documentation is in Documentation/technical/api-sub-process.txt.\n> \n> Thanks - this looks like something useful to have.\n\nThanks for the review and feedback.\n\n> \n> When you create a \"struct subprocess_entry\" to be entered into the system,\n> it is not a true \"struct subprocess_entry\" - it is a \"struct subprocess_entry\"\n> plus some extra variables at the end. Since the sub-process hashmap is\n> keyed solely on the command, what happens if another component uses the\n> same trick (but with different extra\n> variables) when using a sub-process with the same command?\n\nHaving the command be the unique key is sufficient because it gets executed as a process by run_command and there can't be multiple different processes by the same name. \n\n> \n> I can think of at least two ways to solve this: (i) each component can have its\n> own sub-process hashmap, or (ii) add a component key to the hashmap. (i)\n> seems more elegant to me, but I'm not sure what the code will look like.\n> \n> Also, I saw some minor code improvements possible (e.g. using \"starts_with\"\n> when you're checking for the \"status=<foo>\" line) but I'll comment on those\n> and look into the code more thoroughly once the questions in this e-mail are\n> resolved.\n> \n> > diff --git a/sub-process.h b/sub-process.h new file mode 100644 index\n> > 0000000000..d1492f476d\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> I notice from the documentation (and from \"subprocess_get_child_process\"\n> below) that this is meant to be opaque, but I think this can be non-opaque\n> (like \"run-command\").\n> \n> Also, I would prefer adding a \"util\" pointer here instead of using it as an\n> embedded struct. There is no clue here that it is embeddable or meant to be\n> embedded.\n> \n\nThe structure is intentionally opaque to provide the benefits of encapsulation.  Obviously, the \"C\" language doesn't provide any enforcement of that design principal but we do what we can.  \n\nThe embedded struct is following the same design pattern as elsewhere in git (hashmap for example) simply for consistency.\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> I'm not sure if it is useful to take a callback here - I think the caller of this\n> function can just run whatever it wants after a successful subprocess_start.\n\nThe purpose of doing the subprocess specific initialization via a callback is so that if it encounters an error (for example, it can't negotiate a common interface version) the subprocess_start function can detect that and ensure the hashmap does not contain the invalid/unusable subprocess. \n\n> \n> Alternatively, if you add the \"util\" pointer (as I described above), then it\n> makes sense to add a subprocess_get_or_start() function (and now it makes\n> sense to take the callback). This way, the data structure will own, create, and\n> destroy all the \"struct subprocess_entry\" that it needs, creating a nice\n> separation of concerns.\n> \n> > +\n> > +void subprocess_stop(struct subprocess_entry *entry);\n> \n> (continued from above) And it would be clear that this would free\n> \"entry\", for example.\n> \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> > +int subprocess_read_status(int fd, struct strbuf *status);\n> > +\n> > +#endif\n> >\n"},{"id":"315773","messageId":"a5e259bd-d994-6ddb-9dae-43531ee2e875@web.de","threadId":"45496","inReplyTo":"BL2PR03MB3233616BE57BB7D911B1AAEF4330@BL2PR03MB323.namprd03.prod.outlook.com","subject":"Re: [PATCH v2 1/2] pkt-line: add packet_writel() and packet_read_line_gently()","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2017-03-30T14:04:09Z","receivedAt":"2017-03-30T14:03:28Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"\n[snip]\n>> Would packet_write_lines make more sense ?\n>>\n>\n> Just goes to prove that there are two hard things in computer science: cache invalidation, naming things, and off-by-one errors. :)  The feedback on V1 was:\n>\n> \"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 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 suggest\n>\n> \tpacket_writel(int fd, const char *line, ...);\"\n>\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>> I don't think that this va_start() is needed, even if it works.\n>>\n>> int packet_write_line(int fd, const char *lines[])\n>> |\n>> \tconst char *line = *lines;\n>> \tint err;\n>> \twhile (line) {\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\tlines++;\n>> \t\tline = *lines;\n>> \t}\n>> \treturn packet_flush_gently(fd);\n>> ]\n>>\n>\n> This is a helper function to simply the common pattern of writing out a variable number of lines followed by a flush.  The usage of the function as currently implemented is:\n>\n> packet_writel(fd, \"line one\", \"line two\", \"line n\");\n\n\nDoes this work ?\nI would have expected\npacket_writel(fd, \"line one\", \"line two\", \"line n\"), NULL;\n\n>\n> which requires the use of variable number of arguments.  With your proposal that convenience is lost as you have to create an array of strings and pass that instead.  The usage just isn't as simple as the current model.\n>\nWhat is wrong with\n\nint packet_write_fmt_gently(int fd, const char *fmt, ...)\nand we use it like this:\nif packet_write_fmt_gently(fd, \"%s%s%s\", \"line one\", \"line two\", \"line n\")\n\n\n\n(Or do we need another one like)\nint packet_write_fmt_gently_flush(int fd, const char *fmt, ...)\n\nSorry if I am lost here\n"},{"id":"315789","messageId":"BL2PR03MB3237F4B369C0E00168D7291F4340@BL2PR03MB323.namprd03.prod.outlook.com","threadId":"45496","inReplyTo":"a5e259bd-d994-6ddb-9dae-43531ee2e875@web.de","subject":"RE: [PATCH v2 1/2] pkt-line: add packet_writel() and packet_read_line_gently()","fromName":"Ben Peart","fromEmail":"ben.peart@microsoft.com","sentAt":"2017-03-30T16:01:51Z","receivedAt":"2017-03-30T16:02:00Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"> From: Torsten Bögershausen [mailto:tboegi@web.de]\n> \n> \n> Does this work ?\n> I would have expected\n> packet_writel(fd, \"line one\", \"line two\", \"line n\"), NULL;\n> \n\nNo, that's actually not valid C syntax.\n\n> >\n> > which requires the use of variable number of arguments.  With your\n> proposal that convenience is lost as you have to create an array of strings\n> and pass that instead.  The usage just isn't as simple as the current model.\n> >\n> What is wrong with\n> \n> int packet_write_fmt_gently(int fd, const char *fmt, ...) and we use it like\n> this:\n> if packet_write_fmt_gently(fd, \"%s%s%s\", \"line one\", \"line two\", \"line n\")\n> \n\nPackets are not just strings; see pkt-line.h for more details but basically they are a length packet, followed by the data (in this particular case a string).  The packet_writel function is a convenience function to write out a variable number of packetized strings followed by a flush packet.  You're sample above would simply concatenate the three strings and then write a single packet.  A very different outcome. :)\n\n> \n> \n> (Or do we need another one like)\n> int packet_write_fmt_gently_flush(int fd, const char *fmt, ...)\n> \n> Sorry if I am lost here\n"},{"id":"315792","messageId":"d8bb326c-6aec-9379-a5a6-051fa72f8d77@web.de","threadId":"45496","inReplyTo":"BL2PR03MB3237F4B369C0E00168D7291F4340@BL2PR03MB323.namprd03.prod.outlook.com","subject":"Re: [PATCH v2 1/2] pkt-line: add packet_writel() and packet_read_line_gently()","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2017-03-30T17:01:42Z","receivedAt":"2017-03-30T17:01:57Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 30.03.17 18:01, Ben Peart wrote:\n>> From: Torsten Bögershausen [mailto:tboegi@web.de]\n>>\n>>\n>> Does this work ?\n>> I would have expected\n>> packet_writel(fd, \"line one\", \"line two\", \"line n\"), NULL; \nTypo.\nShould have been:\npacket_writel(fd, \"line one\", \"line two\", \"line n\", NULL);\n>>\n> \n> No, that's actually not valid C syntax.\n> \n>>>\n>>> which requires the use of variable number of arguments.  With your\n>> proposal that convenience is lost as you have to create an array of strings\n>> and pass that instead.  The usage just isn't as simple as the current model.\n>>>\n>> What is wrong with\n>>\n>> int packet_write_fmt_gently(int fd, const char *fmt, ...) and we use it like\n>> this:\n>> if packet_write_fmt_gently(fd, \"%s%s%s\", \"line one\", \"line two\", \"line n\")\n>>\n> \n> Packets are not just strings; see pkt-line.h for more details-\n>  but basically they are a length packet, followed by the data (in this particular case a string). \n> The packet_writel function is a convenience function to write out a variable number of\n> packetized strings followed by a flush packet.\n> You're sample above would simply concatenate the three strings and then write a single packet. \n> A very different outcome. :)\n\nGot it.\n"}]}