{"thread":{"id":"46457","subject":"[PATCH] sub-process: refactor handshake to common function","startedAt":"2017-07-24T21:38:21Z","lastAt":"2017-08-07T18:29:38Z","messageCount":22,"participants":["Jonathan Tan","Jonathan Nieder","Ben Peart","Brandon Williams","Junio C Hamano","Lars Schneider"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"325004","messageId":"20170724213810.29831-1-jonathantanmy@google.com","threadId":"46457","inReplyTo":null,"subject":"[PATCH] sub-process: refactor handshake to common function","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-07-24T21:38:10Z","receivedAt":"2017-07-24T21:38:21Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Refactor, into a common function, the version and capability negotiation\ndone when invoking a long-running process as a clean or smudge filter.\nThis will be useful for other Git code that needs to interact similarly\nwith a long-running process.\n\nSigned-off-by: Jonathan Tan \n---\nThis will be useful for anyone implementing functionality like that in\n[1].\n\nIt is unfortunate that the code grew larger because it had to be more\ngeneric, but I think this is better than having (in the future) 2 or\nmore separate handshake functions.\n\nI also don't think that the protocol should be permissive - I think\nthings would be simpler if all protocol errors were fatal errors. As it\nis, most parts are permissive, but packet_read_line() already dies\nanyway upon a malformed packet, so it may not be too drastic a change to\nchange this. For reference, the original protocol comes from [2].\n\n[1] https://public-inbox.org/git/20170714132651.170708-2-benpeart@microsoft.com/\n[2] edcc858 (\"convert: add filter..process option\", 2016-10-17)\n---\n convert.c | 61 ++++-----------------------------\n pkt-line.c | 19 -----------\n pkt-line.h | 2 --\n sub-process.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++\n sub-process.h | 18 ++++++++++\n t/t0021-conversion.sh | 2 +-\n 6 files changed, 120 insertions(+), 76 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex deaf0ba7b..ec8ecc2ea 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -512,62 +512,15 @@ static struct hashmap subprocess_map;\n \n static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)\n {\n-\tint err;\n+\tstatic int versions[] = {2, 0};\n+\tstatic struct subprocess_capability capabilities[] = {\n+\t\t{\"clean\", CAP_CLEAN}, {\"smudge\", CAP_SMUDGE}, {NULL, 0}\n+\t};\n \tstruct cmd2process *entry = (struct cmd2process *)subprocess;\n-\tstruct string_list cap_list = STRING_LIST_INIT_NODUP;\n-\tchar *cap_buf;\n-\tconst char *cap_name;\n-\tstruct child_process *process = &subprocess->process;\n-\tconst char *cmd = subprocess->cmd;\n-\n-\tsigchain_push(SIGPIPE, SIG_IGN);\n-\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\tgoto done;\n-\t}\n-\terr = strcmp(packet_read_line(process->out, NULL), \"version=2\");\n-\tif (err)\n-\t\tgoto done;\n-\terr = packet_read_line(process->out, NULL) != NULL;\n-\tif (err)\n-\t\tgoto done;\n-\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-\t\tif (!cap_buf)\n-\t\t\tbreak;\n-\t\tstring_list_split_in_place(&cap_list, cap_buf, '=', 1);\n-\n-\t\tif (cap_list.nr != 2 || strcmp(cap_list.items[0].string, \"capability\"))\n-\t\t\tcontinue;\n-\n-\t\tcap_name = cap_list.items[1].string;\n-\t\tif (!strcmp(cap_name, \"clean\")) {\n-\t\t\tentry->supported_capabilities |= CAP_CLEAN;\n-\t\t} else if (!strcmp(cap_name, \"smudge\")) {\n-\t\t\tentry->supported_capabilities |= CAP_SMUDGE;\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);\n-\t\t}\n-\n-\t\tstring_list_clear(&cap_list, 0);\n-\t}\n-\n-done:\n-\tsigchain_pop(SIGPIPE);\n \n-\treturn err;\n+\treturn subprocess_handshake(subprocess, \"git-filter-\", versions, NULL,\n+\t\t\t\t capabilities,\n+\t\t\t\t &entry->supported_capabilities);\n }\n \n static int apply_multi_file_filter(const char *path, const char *src, size_t len,\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 9d845ecc3..7db911957 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -171,25 +171,6 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)\n \treturn status;\n }\n \n-int packet_writel(int fd, const char *line, ...)\n-{\n-\tva_list args;\n-\tint err;\n-\tva_start(args, line);\n-\tfor (;;) {\n-\t\tif (!line)\n-\t\t\tbreak;\n-\t\tif (strlen(line) > LARGE_PACKET_DATA_MAX)\n-\t\t\treturn -1;\n-\t\terr = packet_write_fmt_gently(fd, \"%s\\n\", line);\n-\t\tif (err)\n-\t\t\treturn err;\n-\t\tline = va_arg(args, const char*);\n-\t}\n-\tva_end(args);\n-\treturn packet_flush_gently(fd);\n-}\n-\n static int packet_write_gently(const int fd_out, const char *buf, size_t size)\n {\n \tstatic char packet_write_buffer[LARGE_PACKET_MAX];\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 450183b64..66ef610fc 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -25,8 +25,6 @@ void packet_buf_flush(struct strbuf *buf);\n void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n int packet_flush_gently(int fd);\n int packet_write_fmt_gently(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n-LAST_ARG_MUST_BE_NULL\n-int packet_writel(int fd, const char *line, ...);\n int write_packetized_from_fd(int fd_in, int fd_out);\n int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);\n \ndiff --git a/sub-process.c b/sub-process.c\nindex a3cfab1a9..1a3f39bdf 100644\n--- a/sub-process.c\n+++ b/sub-process.c\n@@ -105,3 +105,97 @@ int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, co\n \thashmap_add(hashmap, entry);\n \treturn 0;\n }\n+\n+int subprocess_handshake(struct subprocess_entry *entry,\n+\t\t\t const char *welcome_prefix,\n+\t\t\t int *versions,\n+\t\t\t int *chosen_version,\n+\t\t\t struct subprocess_capability *capabilities,\n+\t\t\t unsigned int *supported_capabilities) {\n+\tint version_scratch;\n+\tunsigned int capabilities_scratch;\n+\tstruct child_process *process = &entry->process;\n+\tint i;\n+\tchar *line;\n+\tconst char *p;\n+\n+\tif (!chosen_version)\n+\t\tchosen_version = &version_scratch;\n+\tif (!supported_capabilities)\n+\t\tsupported_capabilities = &capabilities_scratch;\n+\n+\tsigchain_push(SIGPIPE, SIG_IGN);\n+\n+\tif (packet_write_fmt_gently(process->in, \"%sclient\\n\",\n+\t\t\t\t welcome_prefix)) {\n+\t\terror(\"Could not write client identification\");\n+\t\tgoto error;\n+\t}\n+\tfor (i = 0; versions[i]; i++) {\n+\t\tif (packet_write_fmt_gently(process->in, \"version=%d\\n\",\n+\t\t\t\t\t versions[i])) {\n+\t\t\terror(\"Could not write requested version\");\n+\t\t\tgoto error;\n+\t\t}\n+\t}\n+\tif (packet_flush_gently(process->in))\n+\t\tgoto error;\n+\n+\tif (!(line = packet_read_line(process->out, NULL)) ||\n+\t !skip_prefix(line, welcome_prefix, &p) ||\n+\t strcmp(p, \"server\")) {\n+\t\terror(\"Unexpected line '%s', expected %sserver\",\n+\t\t line ? line : \"\", welcome_prefix);\n+\t\tgoto error;\n+\t}\n+\tif (!(line = packet_read_line(process->out, NULL)) ||\n+\t !skip_prefix(line, \"version=\", &p) ||\n+\t strtol_i(p, 10, chosen_version)) {\n+\t\terror(\"Unexpected line '%s', expected version\",\n+\t\t line ? line : \"\");\n+\t\tgoto error;\n+\t}\n+\tfor (i = 0; versions[i]; i++) {\n+\t\tif (versions[i] == *chosen_version)\n+\t\t\tgoto version_found;\n+\t}\n+\terror(\"Version %d not supported\", *chosen_version);\n+\tgoto error;\n+version_found:\n+\tif ((line = packet_read_line(process->out, NULL))) {\n+\t\terror(\"Unexpected line '%s', expected flush\", line);\n+\t\tgoto error;\n+\t}\n+\n+\tfor (i = 0; capabilities[i].name; i++) {\n+\t\tif (packet_write_fmt_gently(process->in, \"capability=%s\\n\",\n+\t\t\t\t\t capabilities[i].name)) {\n+\t\t\terror(\"Could not write requested capability\");\n+\t\t\tgoto error;\n+\t\t}\n+\t}\n+\tif (packet_flush_gently(process->in))\n+\t\tgoto error;\n+\n+\twhile ((line = packet_read_line(process->out, NULL))) {\n+\t\tif (!skip_prefix(line, \"capability=\", &p))\n+\t\t\tcontinue;\n+\n+\t\tfor (i = 0; capabilities[i].name; i++) {\n+\t\t\tif (!strcmp(p, capabilities[i].name)) {\n+\t\t\t\t*supported_capabilities |= capabilities[i].flag;\n+\t\t\t\tgoto capability_found;\n+\t\t\t}\n+\t\t}\n+\t\twarning(\"external filter requested unsupported filter capability '%s'\",\n+\t\t\tp);\n+capability_found:\n+\t\t;\n+\t}\n+\n+\tsigchain_pop(SIGPIPE);\n+\treturn 0;\n+error:\n+\tsigchain_pop(SIGPIPE);\n+\treturn 1;\n+}\ndiff --git a/sub-process.h b/sub-process.h\nindex 96a2cca36..a72e7f7cf 100644\n--- a/sub-process.h\n+++ b/sub-process.h\n@@ -18,6 +18,11 @@ struct subprocess_entry {\n \tstruct child_process process;\n };\n \n+struct subprocess_capability {\n+\tconst char *name;\n+\tunsigned int flag;\n+};\n+\n /* subprocess functions */\n \n extern int cmd2process_cmp(const void *unused_cmp_data,\n@@ -41,6 +46,19 @@ static inline struct child_process *subprocess_get_child_process(\n \treturn &entry->process;\n }\n \n+/*\n+ * Perform the handshake to a long-running process as described in the\n+ * gitattributes documentation using the given requested versions and\n+ * capabilities. The \"versions\" and \"capabilities\" parameters are arrays\n+ * terminated by a 0 or blank struct.\n+ */\n+int subprocess_handshake(struct subprocess_entry *entry,\n+\t\t\t const char *welcome_prefix,\n+\t\t\t int *versions,\n+\t\t\t int *chosen_version,\n+\t\t\t struct subprocess_capability *capabilities,\n+\t\t\t unsigned int *supported_capabilities);\n+\n /*\n * Helper function that will read packets looking for \"status=\"\n * key/value pairs and return the value from the last \"status\" packet\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex 161f56044..d191a7228 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -697,7 +697,7 @@ test_expect_success PERL 'invalid process filter must fail (and not hang!)' '\n \n \t\tcp \"$TEST_ROOT/test.o\" test.r &&\n \t\ttest_must_fail git add . 2>git-stderr.log &&\n-\t\tgrep \"does not support filter protocol version\" git-stderr.log\n+\t\tgrep \"expected git-filter-server\" git-stderr.log\n \t)\n '\n \n-- \n2.14.0.rc0.400.g1c36432dff-goog\n\n"},{"id":"325010","messageId":"20170724222139.GF13924@aiede.mtv.corp.google.com","threadId":"46457","inReplyTo":"20170724213810.29831-1-jonathantanmy@google.com","subject":"Re: [PATCH] sub-process: refactor handshake to common function","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-07-24T22:21:39Z","receivedAt":"2017-07-24T22:21:48Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJonathan Tan wrote:\n\n> Refactor, into a common function, the version and capability negotiation\n> done when invoking a long-running process as a clean or smudge filter.\n> This will be useful for other Git code that needs to interact similarly\n> with a long-running process.\n>\n> Signed-off-by: Jonathan Tan \n> ---\n\nSounds like a sensible thing to do.\n\n[...]\n> --- a/sub-process.h\n> +++ b/sub-process.h\n> @@ -18,6 +18,11 @@ struct subprocess_entry {\n> \tstruct child_process process;\n> };\n> \n> +struct subprocess_capability {\n> +\tconst char *name;\n> +\tunsigned int flag;\n\nWhat does this flag represent? What values can it have? A comment\nmight help.\n\n[...]\n> @@ -41,6 +46,19 @@ static inline struct child_process *subprocess_get_child_process(\n> \treturn &entry->process;\n> }\n> \n> +/*\n> + * Perform the handshake to a long-running process as described in the\n> + * gitattributes documentation using the given requested versions and\n> + * capabilities. The \"versions\" and \"capabilities\" parameters are arrays\n> + * terminated by a 0 or blank struct.\n> + */\n> +int subprocess_handshake(struct subprocess_entry *entry,\n> +\t\t\t const char *welcome_prefix,\n> +\t\t\t int *versions,\n> +\t\t\t int *chosen_version,\n> +\t\t\t struct subprocess_capability *capabilities,\n> +\t\t\t unsigned int *supported_capabilities);\n> +\n\nIs there a more precise pointer within the gitattributes documentation\nthat describes what this does? I searched for \"handshake\" and found\nnothing. Is the \"Long Running Filter Process\" section where I should\nhave started?\n\nThe API docs for this header are currently in\nDocumentation/technical/api-sub-process.txt. Perhaps these docs\nshould also go there, or, even better, the docs in\nDocumentation/technical/ could move to this header in a preparatory\npatch.\n\nAside (not about this patch): why is the subprocess object called\nstruct subprocess_entry? Would it make sense to rename it to struct\nsubprocess?\n\nBack to this function. Is this a function I should only call once,\nwhen first launching a subprocess, or can I call it again later? A\nnote about suggested usage might help.\n\n[...]\n> --- a/t/t0021-conversion.sh\n> +++ b/t/t0021-conversion.sh\n> @@ -697,7 +697,7 @@ test_expect_success PERL 'invalid process filter must fail (and not hang!)' '\n> \n> \t\tcp \"$TEST_ROOT/test.o\" test.r &&\n> \t\ttest_must_fail git add . 2>git-stderr.log &&\n> -\t\tgrep \"does not support filter protocol version\" git-stderr.log\n> +\t\tgrep \"expected git-filter-server\" git-stderr.log\n\nThis error message looks a little less friendly than the old one.\nIs that okay because it is not supposed to happen in practice?\n\n[...]\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -512,62 +512,15 @@ static struct hashmap subprocess_map;\n> \n> static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)\n> {\n> -\tint err;\n> -\tstruct cmd2process *entry = (struct cmd2process *)subprocess;\n[... many lines snipped ...]\n> -\treturn err;\n> +\tstatic int versions[] = {2, 0};\n> +\tstatic struct subprocess_capability capabilities[] = {\n> +\t\t{\"clean\", CAP_CLEAN}, {\"smudge\", CAP_SMUDGE}, {NULL, 0}\n> +\t};\n> +\tstruct cmd2process *entry = (struct cmd2process *)subprocess;\n> +\n> +\treturn subprocess_handshake(subprocess, \"git-filter-\", versions, NULL,\n> +\t\t\t\t capabilities,\n> +\t\t\t\t &entry->supported_capabilities);\n> }\n\nAPI looks nice.\n\n[...]\n> --- a/sub-process.c\n> +++ b/sub-process.c\n> @@ -105,3 +105,97 @@ int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, co\n\nImplementation looks sane from a quick glance.\n\nThanks,\nJonathan\n"},{"id":"325040","messageId":"8abee4ab-de28-9181-6ff2-e6e171b27f58@gmail.com","threadId":"46457","inReplyTo":"20170724213810.29831-1-jonathantanmy@google.com","subject":"Re: [PATCH] sub-process: refactor handshake to common function","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-07-25T14:38:51Z","receivedAt":"2017-07-25T14:39:01Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 7/24/2017 5:38 PM, Jonathan Tan wrote:\n> Refactor, into a common function, the version and capability negotiation\n> done when invoking a long-running process as a clean or smudge filter.\n> This will be useful for other Git code that needs to interact similarly\n> with a long-running process.\n> \n\nChristian Couder has been working on a similar refactoring for the perl \nversions of very similar helper functions. They can be found in the \nfollowing patch series:\n\nhttps://public-inbox.org/git/20170620075523.26961-1-chriscool@tuxfamily.org/\n\nIn particular:\n\n - Patches 02/49 to 08/49 create a Git/Packet.pm module by\n refactoring \"t0021/rot13-filter.pl\". Functions from this new\n module will be used later in test scripts.\n\nSome differences I noticed: this patch does both the version and \ncapability negotiation and it lives in \"sub-process.h/c\" files. In the \nperl script patch series, the version negotiation is in the new \n\"packet.pm\" module but it does not include the capability negotiation.\n\nIt would make sense to me for these to have the same API and usage \nbehaviors. Perhaps pull them together into a single patch series so \nthat we can review and keep them in sync?\n\n> Signed-off-by: Jonathan Tan \n> ---\n> This will be useful for anyone implementing functionality like that in\n> [1].\n> \n> It is unfortunate that the code grew larger because it had to be more\n> generic, but I think this is better than having (in the future) 2 or\n> more separate handshake functions.\n> \n\nI'm always happy to see an effort to refactor common code into reusable \nAPIs. Its a good engineering practice that makes it easier to \nstabilize, extend and maintain the code. The challenge is making sure \nthe common function supports all the requirements of the various callers \nand that the increase in complexity doesn't outweigh the benefit of \ncentralizing the logic.\n\n> I also don't think that the protocol should be permissive - I think\n> things would be simpler if all protocol errors were fatal errors. As it\n> is, most parts are permissive, but packet_read_line() already dies\n> anyway upon a malformed packet, so it may not be too drastic a change to\n> change this. For reference, the original protocol comes from [2].\n> \n> [1] https://public-inbox.org/git/20170714132651.170708-2-benpeart@microsoft.com/\n> [2] edcc858 (\"convert: add filter..process option\", 2016-10-17)\n> ---\n> convert.c | 61 ++++-----------------------------\n> pkt-line.c | 19 -----------\n> pkt-line.h | 2 --\n> sub-process.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++\n> sub-process.h | 18 ++++++++++\n> t/t0021-conversion.sh | 2 +-\n> 6 files changed, 120 insertions(+), 76 deletions(-)\n> \n> diff --git a/convert.c b/convert.c\n> index deaf0ba7b..ec8ecc2ea 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -512,62 +512,15 @@ static struct hashmap subprocess_map;\n> \n> static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)\n> {\n> -\tint err;\n> +\tstatic int versions[] = {2, 0};\n> +\tstatic struct subprocess_capability capabilities[] = {\n> +\t\t{\"clean\", CAP_CLEAN}, {\"smudge\", CAP_SMUDGE}, {NULL, 0}\n> +\t};\n> \tstruct cmd2process *entry = (struct cmd2process *)subprocess;\n> -\tstruct string_list cap_list = STRING_LIST_INIT_NODUP;\n> -\tchar *cap_buf;\n> -\tconst char *cap_name;\n> -\tstruct child_process *process = &subprocess->process;\n> -\tconst char *cmd = subprocess->cmd;\n> -\n> -\tsigchain_push(SIGPIPE, SIG_IGN);\n> -\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\tgoto done;\n> -\t}\n> -\terr = strcmp(packet_read_line(process->out, NULL), \"version=2\");\n> -\tif (err)\n> -\t\tgoto done;\n> -\terr = packet_read_line(process->out, NULL) != NULL;\n> -\tif (err)\n> -\t\tgoto done;\n> -\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> -\t\tif (!cap_buf)\n> -\t\t\tbreak;\n> -\t\tstring_list_split_in_place(&cap_list, cap_buf, '=', 1);\n> -\n> -\t\tif (cap_list.nr != 2 || strcmp(cap_list.items[0].string, \"capability\"))\n> -\t\t\tcontinue;\n> -\n> -\t\tcap_name = cap_list.items[1].string;\n> -\t\tif (!strcmp(cap_name, \"clean\")) {\n> -\t\t\tentry->supported_capabilities |= CAP_CLEAN;\n> -\t\t} else if (!strcmp(cap_name, \"smudge\")) {\n> -\t\t\tentry->supported_capabilities |= CAP_SMUDGE;\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);\n> -\t\t}\n> -\n> -\t\tstring_list_clear(&cap_list, 0);\n> -\t}\n> -\n> -done:\n> -\tsigchain_pop(SIGPIPE);\n> \n> -\treturn err;\n> +\treturn subprocess_handshake(subprocess, \"git-filter-\", versions, NULL,\n> +\t\t\t\t capabilities,\n> +\t\t\t\t &entry->supported_capabilities);\n> }\n> \n> static int apply_multi_file_filter(const char *path, const char *src, size_t len,\n> diff --git a/pkt-line.c b/pkt-line.c\n> index 9d845ecc3..7db911957 100644\n> --- a/pkt-line.c\n> +++ b/pkt-line.c\n> @@ -171,25 +171,6 @@ 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\nWhile I found the writel functions useful, all my use cases were in \nimplementing the client/server version and capability handshake. \nHopefully, all those can be replaced with this new API.\n\nI'm of the opinion that having a helper function that isn't actively \nused just results in dead code that has to be maintained so it makes \nsense to remove it.\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 450183b64..66ef610fc 100644\n> --- a/pkt-line.h\n> +++ b/pkt-line.h\n> @@ -25,8 +25,6 @@ void packet_buf_flush(struct strbuf *buf);\n> void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n> int packet_flush_gently(int fd);\n> int packet_write_fmt_gently(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n> -LAST_ARG_MUST_BE_NULL\n> -int packet_writel(int fd, const char *line, ...);\n> int write_packetized_from_fd(int fd_in, int fd_out);\n> int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);\n> \n> diff --git a/sub-process.c b/sub-process.c\n> index a3cfab1a9..1a3f39bdf 100644\n> --- a/sub-process.c\n> +++ b/sub-process.c\n> @@ -105,3 +105,97 @@ int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, co\n> \thashmap_add(hashmap, entry);\n> \treturn 0;\n> }\n> +\n> +int subprocess_handshake(struct subprocess_entry *entry,\n> +\t\t\t const char *welcome_prefix,\n> +\t\t\t int *versions,\n> +\t\t\t int *chosen_version,\n> +\t\t\t struct subprocess_capability *capabilities,\n> +\t\t\t unsigned int *supported_capabilities) {\n> +\tint version_scratch;\n> +\tunsigned int capabilities_scratch;\n> +\tstruct child_process *process = &entry->process;\n> +\tint i;\n> +\tchar *line;\n> +\tconst char *p;\n> +\n> +\tif (!chosen_version)\n> +\t\tchosen_version = &version_scratch;\n> +\tif (!supported_capabilities)\n> +\t\tsupported_capabilities = &capabilities_scratch;\n> +\n> +\tsigchain_push(SIGPIPE, SIG_IGN);\n> +\n> +\tif (packet_write_fmt_gently(process->in, \"%sclient\\n\",\n> +\t\t\t\t welcome_prefix)) {\n> +\t\terror(\"Could not write client identification\");\n> +\t\tgoto error;\n> +\t}\n> +\tfor (i = 0; versions[i]; i++) {\n> +\t\tif (packet_write_fmt_gently(process->in, \"version=%d\\n\",\n> +\t\t\t\t\t versions[i])) {\n> +\t\t\terror(\"Could not write requested version\");\n> +\t\t\tgoto error;\n> +\t\t}\n> +\t}\n> +\tif (packet_flush_gently(process->in))\n> +\t\tgoto error;\n> +\n> +\tif (!(line = packet_read_line(process->out, NULL)) ||\n> +\t !skip_prefix(line, welcome_prefix, &p) ||\n> +\t strcmp(p, \"server\")) {\n> +\t\terror(\"Unexpected line '%s', expected %sserver\",\n> +\t\t line ? line : \"\", welcome_prefix);\n> +\t\tgoto error;\n> +\t}\n> +\tif (!(line = packet_read_line(process->out, NULL)) ||\n> +\t !skip_prefix(line, \"version=\", &p) ||\n> +\t strtol_i(p, 10, chosen_version)) {\n> +\t\terror(\"Unexpected line '%s', expected version\",\n> +\t\t line ? line : \"\");\n> +\t\tgoto error;\n> +\t}\n> +\tfor (i = 0; versions[i]; i++) {\n> +\t\tif (versions[i] == *chosen_version)\n> +\t\t\tgoto version_found;\n> +\t}\n\nThis doesn't look like it supports negotiating a common version between \nthe client and server. If a client supports version 1, 2, and 3 and the \nserver only supports version 1 and 2, which version and capabilities \nwill be used?\n\n> +\terror(\"Version %d not supported\", *chosen_version);\n> +\tgoto error;\n> +version_found:\n> +\tif ((line = packet_read_line(process->out, NULL))) {\n> +\t\terror(\"Unexpected line '%s', expected flush\", line);\n> +\t\tgoto error;\n> +\t}\n> +\n> +\tfor (i = 0; capabilities[i].name; i++) {\n> +\t\tif (packet_write_fmt_gently(process->in, \"capability=%s\\n\",\n> +\t\t\t\t\t capabilities[i].name)) {\n> +\t\t\terror(\"Could not write requested capability\");\n> +\t\t\tgoto error;\n> +\t\t}\n> +\t}\n> +\tif (packet_flush_gently(process->in))\n> +\t\tgoto error;\n> +\n> +\twhile ((line = packet_read_line(process->out, NULL))) {\n> +\t\tif (!skip_prefix(line, \"capability=\", &p))\n> +\t\t\tcontinue;\n> +\n> +\t\tfor (i = 0; capabilities[i].name; i++) {\n> +\t\t\tif (!strcmp(p, capabilities[i].name)) {\n> +\t\t\t\t*supported_capabilities |= capabilities[i].flag;\n> +\t\t\t\tgoto capability_found;\n> +\t\t\t}\n> +\t\t}\n> +\t\twarning(\"external filter requested unsupported filter capability '%s'\",\n> +\t\t\tp);\n> +capability_found:\n> +\t\t;\n> +\t}\n> +\n> +\tsigchain_pop(SIGPIPE);\n> +\treturn 0;\n> +error:\n> +\tsigchain_pop(SIGPIPE);\n> +\treturn 1;\n> +}\n> diff --git a/sub-process.h b/sub-process.h\n> index 96a2cca36..a72e7f7cf 100644\n> --- a/sub-process.h\n> +++ b/sub-process.h\n> @@ -18,6 +18,11 @@ struct subprocess_entry {\n> \tstruct child_process process;\n> };\n> \n> +struct subprocess_capability {\n> +\tconst char *name;\n> +\tunsigned int flag;\n> +};\n> +\n> /* subprocess functions */\n> \n> extern int cmd2process_cmp(const void *unused_cmp_data,\n> @@ -41,6 +46,19 @@ static inline struct child_process *subprocess_get_child_process(\n> \treturn &entry->process;\n> }\n> \n> +/*\n> + * Perform the handshake to a long-running process as described in the\n> + * gitattributes documentation using the given requested versions and\n> + * capabilities. The \"versions\" and \"capabilities\" parameters are arrays\n> + * terminated by a 0 or blank struct.\n> + */\n> +int subprocess_handshake(struct subprocess_entry *entry,\n> +\t\t\t const char *welcome_prefix,\n> +\t\t\t int *versions,\n> +\t\t\t int *chosen_version,\n> +\t\t\t struct subprocess_capability *capabilities,\n> +\t\t\t unsigned int *supported_capabilities);\n> +\n> /*\n> * Helper function that will read packets looking for \"status=\"\n> * key/value pairs and return the value from the last \"status\" packet\n> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\n> index 161f56044..d191a7228 100755\n> --- a/t/t0021-conversion.sh\n> +++ b/t/t0021-conversion.sh\n> @@ -697,7 +697,7 @@ test_expect_success PERL 'invalid process filter must fail (and not hang!)' '\n> \n> \t\tcp \"$TEST_ROOT/test.o\" test.r &&\n> \t\ttest_must_fail git add . 2>git-stderr.log &&\n> -\t\tgrep \"does not support filter protocol version\" git-stderr.log\n> +\t\tgrep \"expected git-filter-server\" git-stderr.log\n> \t)\n> '\n> \n> \n"},{"id":"325046","messageId":"20170725105317.7c1ea016@twelve2.svl.corp.google.com","threadId":"46457","inReplyTo":"8abee4ab-de28-9181-6ff2-e6e171b27f58@gmail.com","subject":"Re: [PATCH] sub-process: refactor handshake to common function","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-07-25T17:53:17Z","receivedAt":"2017-07-25T17:53:28Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Tue, 25 Jul 2017 10:38:51 -0400\nBen Peart wrote:\n\n> Christian Couder has been working on a similar refactoring for the perl \n> versions of very similar helper functions. They can be found in the \n> following patch series:\n> \n> https://public-inbox.org/git/20170620075523.26961-1-chriscool@tuxfamily.org/\n> \n> In particular:\n> \n> - Patches 02/49 to 08/49 create a Git/Packet.pm module by\n> refactoring \"t0021/rot13-filter.pl\". Functions from this new\n> module will be used later in test scripts.\n> \n> Some differences I noticed: this patch does both the version and \n> capability negotiation and it lives in \"sub-process.h/c\" files. In the \n> perl script patch series, the version negotiation is in the new \n> \"packet.pm\" module but it does not include the capability negotiation.\n> \n> It would make sense to me for these to have the same API and usage \n> behaviors. Perhaps pull them together into a single patch series so \n> that we can review and keep them in sync?\n\nThanks for the pointer. These are different languages and different use\ncases (mine for production, his for tests), though, so I don't think\nconsistency in API and behavior is practical.\n\n> > It is unfortunate that the code grew larger because it had to be more\n> > generic, but I think this is better than having (in the future) 2 or\n> > more separate handshake functions.\n> \n> I'm always happy to see an effort to refactor common code into reusable \n> APIs. Its a good engineering practice that makes it easier to \n> stabilize, extend and maintain the code. The challenge is making sure \n> the common function supports all the requirements of the various callers \n> and that the increase in complexity doesn't outweigh the benefit of \n> centralizing the logic.\n\nAgreed - thanks.\n\n> > +int subprocess_handshake(struct subprocess_entry *entry,\n> > +\t\t\t const char *welcome_prefix,\n> > +\t\t\t int *versions,\n> > +\t\t\t int *chosen_version,\n> > +\t\t\t struct subprocess_capability *capabilities,\n> > +\t\t\t unsigned int *supported_capabilities) {\n> > +\tint version_scratch;\n> > +\tunsigned int capabilities_scratch;\n> > +\tstruct child_process *process = &entry->process;\n> > +\tint i;\n> > +\tchar *line;\n> > +\tconst char *p;\n> > +\n> > +\tif (!chosen_version)\n> > +\t\tchosen_version = &version_scratch;\n> > +\tif (!supported_capabilities)\n> > +\t\tsupported_capabilities = &capabilities_scratch;\n> > +\n> > +\tsigchain_push(SIGPIPE, SIG_IGN);\n> > +\n> > +\tif (packet_write_fmt_gently(process->in, \"%sclient\\n\",\n> > +\t\t\t\t welcome_prefix)) {\n> > +\t\terror(\"Could not write client identification\");\n> > +\t\tgoto error;\n> > +\t}\n> > +\tfor (i = 0; versions[i]; i++) {\n> > +\t\tif (packet_write_fmt_gently(process->in, \"version=%d\\n\",\n> > +\t\t\t\t\t versions[i])) {\n> > +\t\t\terror(\"Could not write requested version\");\n> > +\t\t\tgoto error;\n> > +\t\t}\n> > +\t}\n> > +\tif (packet_flush_gently(process->in))\n> > +\t\tgoto error;\n> > +\n> > +\tif (!(line = packet_read_line(process->out, NULL)) ||\n> > +\t !skip_prefix(line, welcome_prefix, &p) ||\n> > +\t strcmp(p, \"server\")) {\n> > +\t\terror(\"Unexpected line '%s', expected %sserver\",\n> > +\t\t line ? line : \"\", welcome_prefix);\n> > +\t\tgoto error;\n> > +\t}\n> > +\tif (!(line = packet_read_line(process->out, NULL)) ||\n> > +\t !skip_prefix(line, \"version=\", &p) ||\n> > +\t strtol_i(p, 10, chosen_version)) {\n> > +\t\terror(\"Unexpected line '%s', expected version\",\n> > +\t\t line ? line : \"\");\n> > +\t\tgoto error;\n> > +\t}\n> > +\tfor (i = 0; versions[i]; i++) {\n> > +\t\tif (versions[i] == *chosen_version)\n> > +\t\t\tgoto version_found;\n> > +\t}\n> \n> This doesn't look like it supports negotiating a common version between \n> the client and server. If a client supports version 1, 2, and 3 and the \n> server only supports version 1 and 2, which version and capabilities \n> will be used?\n\nIn the protocol, the client sends a list of versions it supports (see\nthe \"for\" loop above), and expects the server to write the single\nversion that the server chooses (see the part where we read one single\nversion line). In your example, the client would write \"version=1\\n\",\n\"version=2\\n\", and \"version=3\\n\", and the server would write\n\"version=2\\n\" (well, it could write \"version=1\\n\" too). So version 2 (or\n1) will be used.\n"},{"id":"325050","messageId":"cover.1501007300.git.jonathantanmy@google.com","threadId":"46457","inReplyTo":"20170724213810.29831-1-jonathantanmy@google.com","subject":"[PATCH v2 0/2] sub-process: refactor handshake to common function","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-07-25T18:29:36Z","receivedAt":"2017-07-25T18:29:48Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Changes from v1:\n - Moved API docs to header file\n - Updated documentation\n - Updated commit message of patch 2 to explain why the error message\n changed\n\nJonathan Tan (2):\n Documentation: migrate sub-process docs to header\n sub-process: refactor handshake to common function\n\n Documentation/technical/api-sub-process.txt | 59 ------------------\n convert.c | 61 +++----------------\n pkt-line.c | 19 ------\n pkt-line.h | 2 -\n sub-process.c | 94 +++++++++++++++++++++++++++++\n sub-process.h | 51 +++++++++++++++-\n t/t0021-conversion.sh | 2 +-\n 7 files changed, 151 insertions(+), 137 deletions(-)\n delete mode 100644 Documentation/technical/api-sub-process.txt\n\n-- \n2.14.0.rc0.400.g1c36432dff-goog\n\n"},{"id":"325051","messageId":"e47344b6e4bce2a038ba62abb158ec720221a96c.1501007300.git.jonathantanmy@google.com","threadId":"46457","inReplyTo":"cover.1501007300.git.jonathantanmy@google.com","subject":"[PATCH v2 2/2] sub-process: refactor handshake to common function","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-07-25T18:29:38Z","receivedAt":"2017-07-25T18:29:55Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Refactor, into a common function, the version and capability negotiation\ndone when invoking a long-running process as a clean or smudge filter.\nThis will be useful for other Git code that needs to interact similarly\nwith a long-running process.\n\nAs you can see in the change to t0021, this commit changes the error\nmessage reported when the long-running process does not introduce itself\nwith the expected \"server\"-terminated line. Originally, the error\nmessage reports that the filter \"does not support filter protocol\nversion 2\", differentiating between the old single-file filter protocol\nand the new multi-file filter protocol - I have updated it to something\nmore generic and useful.\n\nSigned-off-by: Jonathan Tan \n---\n convert.c | 61 ++++-----------------------------\n pkt-line.c | 19 -----------\n pkt-line.h | 2 --\n sub-process.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++\n sub-process.h | 26 ++++++++++++++\n t/t0021-conversion.sh | 2 +-\n 6 files changed, 128 insertions(+), 76 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex deaf0ba7b..ec8ecc2ea 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -512,62 +512,15 @@ static struct hashmap subprocess_map;\n \n static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)\n {\n-\tint err;\n+\tstatic int versions[] = {2, 0};\n+\tstatic struct subprocess_capability capabilities[] = {\n+\t\t{\"clean\", CAP_CLEAN}, {\"smudge\", CAP_SMUDGE}, {NULL, 0}\n+\t};\n \tstruct cmd2process *entry = (struct cmd2process *)subprocess;\n-\tstruct string_list cap_list = STRING_LIST_INIT_NODUP;\n-\tchar *cap_buf;\n-\tconst char *cap_name;\n-\tstruct child_process *process = &subprocess->process;\n-\tconst char *cmd = subprocess->cmd;\n-\n-\tsigchain_push(SIGPIPE, SIG_IGN);\n-\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\tgoto done;\n-\t}\n-\terr = strcmp(packet_read_line(process->out, NULL), \"version=2\");\n-\tif (err)\n-\t\tgoto done;\n-\terr = packet_read_line(process->out, NULL) != NULL;\n-\tif (err)\n-\t\tgoto done;\n-\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-\t\tif (!cap_buf)\n-\t\t\tbreak;\n-\t\tstring_list_split_in_place(&cap_list, cap_buf, '=', 1);\n-\n-\t\tif (cap_list.nr != 2 || strcmp(cap_list.items[0].string, \"capability\"))\n-\t\t\tcontinue;\n-\n-\t\tcap_name = cap_list.items[1].string;\n-\t\tif (!strcmp(cap_name, \"clean\")) {\n-\t\t\tentry->supported_capabilities |= CAP_CLEAN;\n-\t\t} else if (!strcmp(cap_name, \"smudge\")) {\n-\t\t\tentry->supported_capabilities |= CAP_SMUDGE;\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);\n-\t\t}\n-\n-\t\tstring_list_clear(&cap_list, 0);\n-\t}\n-\n-done:\n-\tsigchain_pop(SIGPIPE);\n \n-\treturn err;\n+\treturn subprocess_handshake(subprocess, \"git-filter-\", versions, NULL,\n+\t\t\t\t capabilities,\n+\t\t\t\t &entry->supported_capabilities);\n }\n \n static int apply_multi_file_filter(const char *path, const char *src, size_t len,\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 9d845ecc3..7db911957 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -171,25 +171,6 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)\n \treturn status;\n }\n \n-int packet_writel(int fd, const char *line, ...)\n-{\n-\tva_list args;\n-\tint err;\n-\tva_start(args, line);\n-\tfor (;;) {\n-\t\tif (!line)\n-\t\t\tbreak;\n-\t\tif (strlen(line) > LARGE_PACKET_DATA_MAX)\n-\t\t\treturn -1;\n-\t\terr = packet_write_fmt_gently(fd, \"%s\\n\", line);\n-\t\tif (err)\n-\t\t\treturn err;\n-\t\tline = va_arg(args, const char*);\n-\t}\n-\tva_end(args);\n-\treturn packet_flush_gently(fd);\n-}\n-\n static int packet_write_gently(const int fd_out, const char *buf, size_t size)\n {\n \tstatic char packet_write_buffer[LARGE_PACKET_MAX];\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 450183b64..66ef610fc 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -25,8 +25,6 @@ void packet_buf_flush(struct strbuf *buf);\n void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n int packet_flush_gently(int fd);\n int packet_write_fmt_gently(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n-LAST_ARG_MUST_BE_NULL\n-int packet_writel(int fd, const char *line, ...);\n int write_packetized_from_fd(int fd_in, int fd_out);\n int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);\n \ndiff --git a/sub-process.c b/sub-process.c\nindex a3cfab1a9..1a3f39bdf 100644\n--- a/sub-process.c\n+++ b/sub-process.c\n@@ -105,3 +105,97 @@ int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, co\n \thashmap_add(hashmap, entry);\n \treturn 0;\n }\n+\n+int subprocess_handshake(struct subprocess_entry *entry,\n+\t\t\t const char *welcome_prefix,\n+\t\t\t int *versions,\n+\t\t\t int *chosen_version,\n+\t\t\t struct subprocess_capability *capabilities,\n+\t\t\t unsigned int *supported_capabilities) {\n+\tint version_scratch;\n+\tunsigned int capabilities_scratch;\n+\tstruct child_process *process = &entry->process;\n+\tint i;\n+\tchar *line;\n+\tconst char *p;\n+\n+\tif (!chosen_version)\n+\t\tchosen_version = &version_scratch;\n+\tif (!supported_capabilities)\n+\t\tsupported_capabilities = &capabilities_scratch;\n+\n+\tsigchain_push(SIGPIPE, SIG_IGN);\n+\n+\tif (packet_write_fmt_gently(process->in, \"%sclient\\n\",\n+\t\t\t\t welcome_prefix)) {\n+\t\terror(\"Could not write client identification\");\n+\t\tgoto error;\n+\t}\n+\tfor (i = 0; versions[i]; i++) {\n+\t\tif (packet_write_fmt_gently(process->in, \"version=%d\\n\",\n+\t\t\t\t\t versions[i])) {\n+\t\t\terror(\"Could not write requested version\");\n+\t\t\tgoto error;\n+\t\t}\n+\t}\n+\tif (packet_flush_gently(process->in))\n+\t\tgoto error;\n+\n+\tif (!(line = packet_read_line(process->out, NULL)) ||\n+\t !skip_prefix(line, welcome_prefix, &p) ||\n+\t strcmp(p, \"server\")) {\n+\t\terror(\"Unexpected line '%s', expected %sserver\",\n+\t\t line ? line : \"\", welcome_prefix);\n+\t\tgoto error;\n+\t}\n+\tif (!(line = packet_read_line(process->out, NULL)) ||\n+\t !skip_prefix(line, \"version=\", &p) ||\n+\t strtol_i(p, 10, chosen_version)) {\n+\t\terror(\"Unexpected line '%s', expected version\",\n+\t\t line ? line : \"\");\n+\t\tgoto error;\n+\t}\n+\tfor (i = 0; versions[i]; i++) {\n+\t\tif (versions[i] == *chosen_version)\n+\t\t\tgoto version_found;\n+\t}\n+\terror(\"Version %d not supported\", *chosen_version);\n+\tgoto error;\n+version_found:\n+\tif ((line = packet_read_line(process->out, NULL))) {\n+\t\terror(\"Unexpected line '%s', expected flush\", line);\n+\t\tgoto error;\n+\t}\n+\n+\tfor (i = 0; capabilities[i].name; i++) {\n+\t\tif (packet_write_fmt_gently(process->in, \"capability=%s\\n\",\n+\t\t\t\t\t capabilities[i].name)) {\n+\t\t\terror(\"Could not write requested capability\");\n+\t\t\tgoto error;\n+\t\t}\n+\t}\n+\tif (packet_flush_gently(process->in))\n+\t\tgoto error;\n+\n+\twhile ((line = packet_read_line(process->out, NULL))) {\n+\t\tif (!skip_prefix(line, \"capability=\", &p))\n+\t\t\tcontinue;\n+\n+\t\tfor (i = 0; capabilities[i].name; i++) {\n+\t\t\tif (!strcmp(p, capabilities[i].name)) {\n+\t\t\t\t*supported_capabilities |= capabilities[i].flag;\n+\t\t\t\tgoto capability_found;\n+\t\t\t}\n+\t\t}\n+\t\twarning(\"external filter requested unsupported filter capability '%s'\",\n+\t\t\tp);\n+capability_found:\n+\t\t;\n+\t}\n+\n+\tsigchain_pop(SIGPIPE);\n+\treturn 0;\n+error:\n+\tsigchain_pop(SIGPIPE);\n+\treturn 1;\n+}\ndiff --git a/sub-process.h b/sub-process.h\nindex 9e6975b5e..28863fabc 100644\n--- a/sub-process.h\n+++ b/sub-process.h\n@@ -29,6 +29,16 @@ struct subprocess_entry {\n \tstruct child_process process;\n };\n \n+struct subprocess_capability {\n+\tconst char *name;\n+\n+\t/*\n+\t * subprocess_handshake will \"|=\" this value to supported_capabilities\n+\t * if the server reports that it supports this capability.\n+\t */\n+\tunsigned int flag;\n+};\n+\n /* subprocess functions */\n \n /* Function to test two subprocess hashmap entries for equality. */\n@@ -62,6 +72,22 @@ static inline struct child_process *subprocess_get_child_process(\n \treturn &entry->process;\n }\n \n+/*\n+ * Perform the version and capability negotiation as described in the \"Long\n+ * Running Filter Process\" section of the gitattributes documentation using the\n+ * given requested versions and capabilities. The \"versions\" and \"capabilities\"\n+ * parameters are arrays terminated by a 0 or blank struct.\n+ *\n+ * This function is typically called when a subprocess is started (as part of\n+ * the \"startfn\" passed to subprocess_start).\n+ */\n+int subprocess_handshake(struct subprocess_entry *entry,\n+\t\t\t const char *welcome_prefix,\n+\t\t\t int *versions,\n+\t\t\t int *chosen_version,\n+\t\t\t struct subprocess_capability *capabilities,\n+\t\t\t unsigned int *supported_capabilities);\n+\n /*\n * Helper function that will read packets looking for \"status=\"\n * key/value pairs and return the value from the last \"status\" packet\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex 161f56044..d191a7228 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -697,7 +697,7 @@ test_expect_success PERL 'invalid process filter must fail (and not hang!)' '\n \n \t\tcp \"$TEST_ROOT/test.o\" test.r &&\n \t\ttest_must_fail git add . 2>git-stderr.log &&\n-\t\tgrep \"does not support filter protocol version\" git-stderr.log\n+\t\tgrep \"expected git-filter-server\" git-stderr.log\n \t)\n '\n \n-- \n2.14.0.rc0.400.g1c36432dff-goog\n\n"},{"id":"325052","messageId":"2d97d07a4977048a8147292ac13db48f5202d52f.1501007300.git.jonathantanmy@google.com","threadId":"46457","inReplyTo":"cover.1501007300.git.jonathantanmy@google.com","subject":"[PATCH v2 1/2] Documentation: migrate sub-process docs to header","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-07-25T18:29:37Z","receivedAt":"2017-07-25T18:29:58Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Move the documentation for the sub-process API from a separate txt file\nto its header file.\n\nSigned-off-by: Jonathan Tan \n---\n Documentation/technical/api-sub-process.txt | 59 -----------------------------\n sub-process.h | 25 +++++++++++-\n 2 files changed, 23 insertions(+), 61 deletions(-)\n delete mode 100644 Documentation/technical/api-sub-process.txt\n\ndiff --git a/Documentation/technical/api-sub-process.txt b/Documentation/technical/api-sub-process.txt\ndeleted file mode 100644\nindex 793508cf3..000000000\n--- a/Documentation/technical/api-sub-process.txt\n+++ /dev/null\n@@ -1,59 +0,0 @@\n-sub-process API\n-===============\n-\n-The sub-process API makes it possible to run background sub-processes\n-for the entire lifetime of a Git invocation. If Git needs to communicate\n-with an external process multiple times, then this can reduces the process\n-invocation overhead. Git and the sub-process communicate through stdin and\n-stdout.\n-\n-The sub-processes are kept in a hashmap by command name and looked up\n-via the subprocess_find_entry function. If an existing instance can not\n-be found then a new process should be created and started. When the\n-parent git command terminates, all sub-processes are also terminated.\n-\n-This API is based on the run-command API.\n-\n-Data structures\n----------------\n-\n-* `struct subprocess_entry`\n-\n-The sub-process structure. Members should not be accessed directly.\n-\n-Types\n------\n-\n-'int(*subprocess_start_fn)(struct subprocess_entry *entry)'::\n-\n-\tUser-supplied function to initialize the sub-process. This is\n-\ttypically used to negotiate the interface version and capabilities.\n-\n-\n-Functions\n----------\n-\n-`cmd2process_cmp`::\n-\n-\tFunction to test two subprocess hashmap entries for equality.\n-\n-`subprocess_start`::\n-\n-\tStart a subprocess and add it to the subprocess hashmap.\n-\n-`subprocess_stop`::\n-\n-\tKill a subprocess and remove it from the subprocess hashmap.\n-\n-`subprocess_find_entry`::\n-\n-\tFind a subprocess in the subprocess hashmap.\n-\n-`subprocess_get_child_process`::\n-\n-\tGet the underlying `struct child_process` from a subprocess.\n-\n-`subprocess_read_status`::\n-\n-\tHelper function to read packets looking for the last \"status=\"\n-\tkey/value pair.\ndiff --git a/sub-process.h b/sub-process.h\nindex 96a2cca36..9e6975b5e 100644\n--- a/sub-process.h\n+++ b/sub-process.h\n@@ -6,12 +6,23 @@\n #include \"run-command.h\"\n \n /*\n- * Generic implementation of background process infrastructure.\n- * See: Documentation/technical/api-sub-process.txt\n+ * The sub-process API makes it possible to run background sub-processes\n+ * for the entire lifetime of a Git invocation. If Git needs to communicate\n+ * with an external process multiple times, then this can reduces the process\n+ * invocation overhead. Git and the sub-process communicate through stdin and\n+ * stdout.\n+ *\n+ * The sub-processes are kept in a hashmap by command name and looked up\n+ * via the subprocess_find_entry function. If an existing instance can not\n+ * be found then a new process should be created and started. When the\n+ * parent git command terminates, all sub-processes are also terminated.\n+ * \n+ * This API is based on the run-command API.\n */\n \n /* data structures */\n \n+/* Members should not be accessed directly. */\n struct subprocess_entry {\n \tstruct hashmap_entry ent; /* must be the first member! */\n \tconst char *cmd;\n@@ -20,21 +31,31 @@ struct subprocess_entry {\n \n /* subprocess functions */\n \n+/* Function to test two subprocess hashmap entries for equality. */\n extern int cmd2process_cmp(const void *unused_cmp_data,\n \t\t\t const struct subprocess_entry *e1,\n \t\t\t const struct subprocess_entry *e2,\n \t\t\t const void *unused_keydata);\n \n+/*\n+ * User-supplied function to initialize the sub-process. This is\n+ * typically used to negotiate the interface version and capabilities.\n+ */\n typedef int(*subprocess_start_fn)(struct subprocess_entry *entry);\n+\n+/* Start a subprocess and add it to the subprocess hashmap. */\n int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, const char *cmd,\n \t\tsubprocess_start_fn startfn);\n \n+/* Kill a subprocess and remove it from the subprocess hashmap. */\n void subprocess_stop(struct hashmap *hashmap, struct subprocess_entry *entry);\n \n+/* Find a subprocess in the subprocess hashmap. */\n struct subprocess_entry *subprocess_find_entry(struct hashmap *hashmap, const char *cmd);\n \n /* subprocess helper functions */\n \n+/* Get the underlying `struct child_process` from a subprocess. */\n static inline struct child_process *subprocess_get_child_process(\n \t\tstruct subprocess_entry *entry)\n {\n-- \n2.14.0.rc0.400.g1c36432dff-goog\n\n"},{"id":"325062","messageId":"20170725201858.GJ92874@google.com","threadId":"46457","inReplyTo":"2d97d07a4977048a8147292ac13db48f5202d52f.1501007300.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 1/2] Documentation: migrate sub-process docs to header","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-07-25T20:18:58Z","receivedAt":"2017-07-25T20:19:08Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 07/25, Jonathan Tan wrote:\n> Move the documentation for the sub-process API from a separate txt file\n> to its header file.\n> \n> Signed-off-by: Jonathan Tan \n\nI really like this change!\n\n> ---\n> Documentation/technical/api-sub-process.txt | 59 -----------------------------\n> sub-process.h | 25 +++++++++++-\n> 2 files changed, 23 insertions(+), 61 deletions(-)\n> delete mode 100644 Documentation/technical/api-sub-process.txt\n> \n> diff --git a/Documentation/technical/api-sub-process.txt b/Documentation/technical/api-sub-process.txt\n> deleted file mode 100644\n> index 793508cf3..000000000\n> --- a/Documentation/technical/api-sub-process.txt\n> +++ /dev/null\n> @@ -1,59 +0,0 @@\n> -sub-process API\n> -===============\n> -\n> -The sub-process API makes it possible to run background sub-processes\n> -for the entire lifetime of a Git invocation. If Git needs to communicate\n> -with an external process multiple times, then this can reduces the process\n> -invocation overhead. Git and the sub-process communicate through stdin and\n> -stdout.\n> -\n> -The sub-processes are kept in a hashmap by command name and looked up\n> -via the subprocess_find_entry function. If an existing instance can not\n> -be found then a new process should be created and started. When the\n> -parent git command terminates, all sub-processes are also terminated.\n> -\n> -This API is based on the run-command API.\n> -\n> -Data structures\n> ----------------\n> -\n> -* `struct subprocess_entry`\n> -\n> -The sub-process structure. Members should not be accessed directly.\n> -\n> -Types\n> ------\n> -\n> -'int(*subprocess_start_fn)(struct subprocess_entry *entry)'::\n> -\n> -\tUser-supplied function to initialize the sub-process. This is\n> -\ttypically used to negotiate the interface version and capabilities.\n> -\n> -\n> -Functions\n> ----------\n> -\n> -`cmd2process_cmp`::\n> -\n> -\tFunction to test two subprocess hashmap entries for equality.\n> -\n> -`subprocess_start`::\n> -\n> -\tStart a subprocess and add it to the subprocess hashmap.\n> -\n> -`subprocess_stop`::\n> -\n> -\tKill a subprocess and remove it from the subprocess hashmap.\n> -\n> -`subprocess_find_entry`::\n> -\n> -\tFind a subprocess in the subprocess hashmap.\n> -\n> -`subprocess_get_child_process`::\n> -\n> -\tGet the underlying `struct child_process` from a subprocess.\n> -\n> -`subprocess_read_status`::\n> -\n> -\tHelper function to read packets looking for the last \"status=\"\n> -\tkey/value pair.\n> diff --git a/sub-process.h b/sub-process.h\n> index 96a2cca36..9e6975b5e 100644\n> --- a/sub-process.h\n> +++ b/sub-process.h\n> @@ -6,12 +6,23 @@\n> #include \"run-command.h\"\n> \n> /*\n> - * Generic implementation of background process infrastructure.\n> - * See: Documentation/technical/api-sub-process.txt\n> + * The sub-process API makes it possible to run background sub-processes\n> + * for the entire lifetime of a Git invocation. If Git needs to communicate\n> + * with an external process multiple times, then this can reduces the process\n> + * invocation overhead. Git and the sub-process communicate through stdin and\n> + * stdout.\n> + *\n> + * The sub-processes are kept in a hashmap by command name and looked up\n> + * via the subprocess_find_entry function. If an existing instance can not\n> + * be found then a new process should be created and started. When the\n> + * parent git command terminates, all sub-processes are also terminated.\n> + * \n> + * This API is based on the run-command API.\n> */\n> \n> /* data structures */\n> \n> +/* Members should not be accessed directly. */\n> struct subprocess_entry {\n> \tstruct hashmap_entry ent; /* must be the first member! */\n> \tconst char *cmd;\n> @@ -20,21 +31,31 @@ struct subprocess_entry {\n> \n> /* subprocess functions */\n> \n> +/* Function to test two subprocess hashmap entries for equality. */\n> extern int cmd2process_cmp(const void *unused_cmp_data,\n> \t\t\t const struct subprocess_entry *e1,\n> \t\t\t const struct subprocess_entry *e2,\n> \t\t\t const void *unused_keydata);\n> \n> +/*\n> + * User-supplied function to initialize the sub-process. This is\n> + * typically used to negotiate the interface version and capabilities.\n> + */\n> typedef int(*subprocess_start_fn)(struct subprocess_entry *entry);\n> +\n> +/* Start a subprocess and add it to the subprocess hashmap. */\n> int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, const char *cmd,\n> \t\tsubprocess_start_fn startfn);\n> \n> +/* Kill a subprocess and remove it from the subprocess hashmap. */\n> void subprocess_stop(struct hashmap *hashmap, struct subprocess_entry *entry);\n> \n> +/* Find a subprocess in the subprocess hashmap. */\n> struct subprocess_entry *subprocess_find_entry(struct hashmap *hashmap, const char *cmd);\n> \n> /* subprocess helper functions */\n> \n> +/* Get the underlying `struct child_process` from a subprocess. */\n> static inline struct child_process *subprocess_get_child_process(\n> \t\tstruct subprocess_entry *entry)\n> {\n> -- \n> 2.14.0.rc0.400.g1c36432dff-goog\n> \n\n-- \nBrandon Williams\n"},{"id":"325065","messageId":"20170725202807.GK92874@google.com","threadId":"46457","inReplyTo":"e47344b6e4bce2a038ba62abb158ec720221a96c.1501007300.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 2/2] sub-process: refactor handshake to common function","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-07-25T20:28:07Z","receivedAt":"2017-07-25T20:28:16Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 07/25, Jonathan Tan wrote:\n> Refactor, into a common function, the version and capability negotiation\n> done when invoking a long-running process as a clean or smudge filter.\n> This will be useful for other Git code that needs to interact similarly\n> with a long-running process.\n> \n> As you can see in the change to t0021, this commit changes the error\n> message reported when the long-running process does not introduce itself\n> with the expected \"server\"-terminated line. Originally, the error\n> message reports that the filter \"does not support filter protocol\n> version 2\", differentiating between the old single-file filter protocol\n> and the new multi-file filter protocol - I have updated it to something\n> more generic and useful.\n> \n> Signed-off-by: Jonathan Tan \n> ---\n> convert.c | 61 ++++-----------------------------\n> pkt-line.c | 19 -----------\n> pkt-line.h | 2 --\n> sub-process.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++\n> sub-process.h | 26 ++++++++++++++\n> t/t0021-conversion.sh | 2 +-\n> 6 files changed, 128 insertions(+), 76 deletions(-)\n> \n> diff --git a/convert.c b/convert.c\n> index deaf0ba7b..ec8ecc2ea 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -512,62 +512,15 @@ static struct hashmap subprocess_map;\n> \n> static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)\n> {\n> -\tint err;\n> +\tstatic int versions[] = {2, 0};\n> +\tstatic struct subprocess_capability capabilities[] = {\n> +\t\t{\"clean\", CAP_CLEAN}, {\"smudge\", CAP_SMUDGE}, {NULL, 0}\n> +\t};\n> \tstruct cmd2process *entry = (struct cmd2process *)subprocess;\n> -\tstruct string_list cap_list = STRING_LIST_INIT_NODUP;\n> -\tchar *cap_buf;\n> -\tconst char *cap_name;\n> -\tstruct child_process *process = &subprocess->process;\n> -\tconst char *cmd = subprocess->cmd;\n> -\n> -\tsigchain_push(SIGPIPE, SIG_IGN);\n> -\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\tgoto done;\n> -\t}\n> -\terr = strcmp(packet_read_line(process->out, NULL), \"version=2\");\n> -\tif (err)\n> -\t\tgoto done;\n> -\terr = packet_read_line(process->out, NULL) != NULL;\n> -\tif (err)\n> -\t\tgoto done;\n> -\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> -\t\tif (!cap_buf)\n> -\t\t\tbreak;\n> -\t\tstring_list_split_in_place(&cap_list, cap_buf, '=', 1);\n> -\n> -\t\tif (cap_list.nr != 2 || strcmp(cap_list.items[0].string, \"capability\"))\n> -\t\t\tcontinue;\n> -\n> -\t\tcap_name = cap_list.items[1].string;\n> -\t\tif (!strcmp(cap_name, \"clean\")) {\n> -\t\t\tentry->supported_capabilities |= CAP_CLEAN;\n> -\t\t} else if (!strcmp(cap_name, \"smudge\")) {\n> -\t\t\tentry->supported_capabilities |= CAP_SMUDGE;\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);\n> -\t\t}\n> -\n> -\t\tstring_list_clear(&cap_list, 0);\n> -\t}\n> -\n> -done:\n> -\tsigchain_pop(SIGPIPE);\n> \n> -\treturn err;\n> +\treturn subprocess_handshake(subprocess, \"git-filter-\", versions, NULL,\n> +\t\t\t\t capabilities,\n> +\t\t\t\t &entry->supported_capabilities);\n> }\n> \n> static int apply_multi_file_filter(const char *path, const char *src, size_t len,\n> diff --git a/pkt-line.c b/pkt-line.c\n> index 9d845ecc3..7db911957 100644\n> --- a/pkt-line.c\n> +++ b/pkt-line.c\n> @@ -171,25 +171,6 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)\n> \treturn status;\n> }\n> \n> -int packet_writel(int fd, const char *line, ...)\n> -{\n> -\tva_list args;\n> -\tint err;\n> -\tva_start(args, line);\n> -\tfor (;;) {\n> -\t\tif (!line)\n> -\t\t\tbreak;\n> -\t\tif (strlen(line) > LARGE_PACKET_DATA_MAX)\n> -\t\t\treturn -1;\n> -\t\terr = packet_write_fmt_gently(fd, \"%s\\n\", line);\n> -\t\tif (err)\n> -\t\t\treturn err;\n> -\t\tline = va_arg(args, const char*);\n> -\t}\n> -\tva_end(args);\n> -\treturn packet_flush_gently(fd);\n> -}\n> -\n> static int packet_write_gently(const int fd_out, const char *buf, size_t size)\n> {\n> \tstatic char packet_write_buffer[LARGE_PACKET_MAX];\n> diff --git a/pkt-line.h b/pkt-line.h\n> index 450183b64..66ef610fc 100644\n> --- a/pkt-line.h\n> +++ b/pkt-line.h\n> @@ -25,8 +25,6 @@ void packet_buf_flush(struct strbuf *buf);\n> void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n> int packet_flush_gently(int fd);\n> int packet_write_fmt_gently(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n> -LAST_ARG_MUST_BE_NULL\n> -int packet_writel(int fd, const char *line, ...);\n> int write_packetized_from_fd(int fd_in, int fd_out);\n> int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);\n> \n> diff --git a/sub-process.c b/sub-process.c\n> index a3cfab1a9..1a3f39bdf 100644\n> --- a/sub-process.c\n> +++ b/sub-process.c\n> @@ -105,3 +105,97 @@ int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, co\n> \thashmap_add(hashmap, entry);\n> \treturn 0;\n> }\n> +\n> +int subprocess_handshake(struct subprocess_entry *entry,\n> +\t\t\t const char *welcome_prefix,\n> +\t\t\t int *versions,\n> +\t\t\t int *chosen_version,\n> +\t\t\t struct subprocess_capability *capabilities,\n> +\t\t\t unsigned int *supported_capabilities) {\n> +\tint version_scratch;\n> +\tunsigned int capabilities_scratch;\n> +\tstruct child_process *process = &entry->process;\n> +\tint i;\n> +\tchar *line;\n> +\tconst char *p;\n> +\n> +\tif (!chosen_version)\n> +\t\tchosen_version = &version_scratch;\n> +\tif (!supported_capabilities)\n> +\t\tsupported_capabilities = &capabilities_scratch;\n> +\n> +\tsigchain_push(SIGPIPE, SIG_IGN);\n> +\n> +\tif (packet_write_fmt_gently(process->in, \"%sclient\\n\",\n> +\t\t\t\t welcome_prefix)) {\n> +\t\terror(\"Could not write client identification\");\n> +\t\tgoto error;\n> +\t}\n> +\tfor (i = 0; versions[i]; i++) {\n> +\t\tif (packet_write_fmt_gently(process->in, \"version=%d\\n\",\n> +\t\t\t\t\t versions[i])) {\n> +\t\t\terror(\"Could not write requested version\");\n> +\t\t\tgoto error;\n> +\t\t}\n> +\t}\n> +\tif (packet_flush_gently(process->in))\n> +\t\tgoto error;\n> +\n> +\tif (!(line = packet_read_line(process->out, NULL)) ||\n> +\t !skip_prefix(line, welcome_prefix, &p) ||\n> +\t strcmp(p, \"server\")) {\n> +\t\terror(\"Unexpected line '%s', expected %sserver\",\n> +\t\t line ? line : \"\", welcome_prefix);\n> +\t\tgoto error;\n> +\t}\n> +\tif (!(line = packet_read_line(process->out, NULL)) ||\n> +\t !skip_prefix(line, \"version=\", &p) ||\n> +\t strtol_i(p, 10, chosen_version)) {\n> +\t\terror(\"Unexpected line '%s', expected version\",\n> +\t\t line ? line : \"\");\n> +\t\tgoto error;\n> +\t}\n> +\tfor (i = 0; versions[i]; i++) {\n> +\t\tif (versions[i] == *chosen_version)\n> +\t\t\tgoto version_found;\n> +\t}\n> +\terror(\"Version %d not supported\", *chosen_version);\n> +\tgoto error;\n> +version_found:\n> +\tif ((line = packet_read_line(process->out, NULL))) {\n> +\t\terror(\"Unexpected line '%s', expected flush\", line);\n> +\t\tgoto error;\n> +\t}\n> +\n> +\tfor (i = 0; capabilities[i].name; i++) {\n> +\t\tif (packet_write_fmt_gently(process->in, \"capability=%s\\n\",\n> +\t\t\t\t\t capabilities[i].name)) {\n> +\t\t\terror(\"Could not write requested capability\");\n> +\t\t\tgoto error;\n> +\t\t}\n> +\t}\n> +\tif (packet_flush_gently(process->in))\n> +\t\tgoto error;\n> +\n> +\twhile ((line = packet_read_line(process->out, NULL))) {\n> +\t\tif (!skip_prefix(line, \"capability=\", &p))\n> +\t\t\tcontinue;\n> +\n> +\t\tfor (i = 0; capabilities[i].name; i++) {\n> +\t\t\tif (!strcmp(p, capabilities[i].name)) {\n> +\t\t\t\t*supported_capabilities |= capabilities[i].flag;\n> +\t\t\t\tgoto capability_found;\n> +\t\t\t}\n> +\t\t}\n> +\t\twarning(\"external filter requested unsupported filter capability '%s'\",\n> +\t\t\tp);\n> +capability_found:\n> +\t\t;\n> +\t}\n\nI'm not familiar with this section of code so I can't comment on what\nthe logic is doing. But I think that this whole function can be written\nbetter. Its quite a lengthy function so you may benefit from breaking\nit up into smaller functions to improve readability. My biggest\ncomplaint is the use of goto's to jump around the function, in and out\nof loops and the like. IMO its completely fine to have a 'goto error'\nwhere you can do any necessary cleanup but it just makes things very\nhard to follow when they are structured using gotos to break out of\nloops or jump to other parts of the function as you have here.\n\nIt should be pretty easy to eliminate the two non-essential goto's by\nmaking a helper function to determine if a version is found as well as\nfinding a capability. That way you should be able to restructure the\ncode using if's instead of gotos.\n\n> +\n> +\tsigchain_pop(SIGPIPE);\n> +\treturn 0;\n> +error:\n> +\tsigchain_pop(SIGPIPE);\n> +\treturn 1;\n> +}\n> diff --git a/sub-process.h b/sub-process.h\n> index 9e6975b5e..28863fabc 100644\n> --- a/sub-process.h\n> +++ b/sub-process.h\n> @@ -29,6 +29,16 @@ struct subprocess_entry {\n> \tstruct child_process process;\n> };\n> \n> +struct subprocess_capability {\n> +\tconst char *name;\n> +\n> +\t/*\n> +\t * subprocess_handshake will \"|=\" this value to supported_capabilities\n> +\t * if the server reports that it supports this capability.\n> +\t */\n> +\tunsigned int flag;\n> +};\n> +\n> /* subprocess functions */\n> \n> /* Function to test two subprocess hashmap entries for equality. */\n> @@ -62,6 +72,22 @@ static inline struct child_process *subprocess_get_child_process(\n> \treturn &entry->process;\n> }\n> \n> +/*\n> + * Perform the version and capability negotiation as described in the \"Long\n> + * Running Filter Process\" section of the gitattributes documentation using the\n> + * given requested versions and capabilities. The \"versions\" and \"capabilities\"\n> + * parameters are arrays terminated by a 0 or blank struct.\n> + *\n> + * This function is typically called when a subprocess is started (as part of\n> + * the \"startfn\" passed to subprocess_start).\n> + */\n> +int subprocess_handshake(struct subprocess_entry *entry,\n> +\t\t\t const char *welcome_prefix,\n> +\t\t\t int *versions,\n> +\t\t\t int *chosen_version,\n> +\t\t\t struct subprocess_capability *capabilities,\n> +\t\t\t unsigned int *supported_capabilities);\n> +\n> /*\n> * Helper function that will read packets looking for \"status=\"\n> * key/value pairs and return the value from the last \"status\" packet\n> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\n> index 161f56044..d191a7228 100755\n> --- a/t/t0021-conversion.sh\n> +++ b/t/t0021-conversion.sh\n> @@ -697,7 +697,7 @@ test_expect_success PERL 'invalid process filter must fail (and not hang!)' '\n> \n> \t\tcp \"$TEST_ROOT/test.o\" test.r &&\n> \t\ttest_must_fail git add . 2>git-stderr.log &&\n> -\t\tgrep \"does not support filter protocol version\" git-stderr.log\n> +\t\tgrep \"expected git-filter-server\" git-stderr.log\n> \t)\n> '\n> \n> -- \n> 2.14.0.rc0.400.g1c36432dff-goog\n> \n\n-- \nBrandon Williams\n"},{"id":"325096","messageId":"xmqqinigtar8.fsf@gitster.mtv.corp.google.com","threadId":"46457","inReplyTo":"e47344b6e4bce2a038ba62abb158ec720221a96c.1501007300.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 2/2] sub-process: refactor handshake to common function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-25T22:25:15Z","receivedAt":"2017-07-25T22:25:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Refactor, into a common function, the version and capability negotiation\n> done when invoking a long-running process as a clean or smudge filter.\n> This will be useful for other Git code that needs to interact similarly\n> with a long-running process.\n>\n> As you can see in the change to t0021, this commit changes the error\n> message reported when the long-running process does not introduce itself\n> with the expected \"server\"-terminated line. Originally, the error\n> message reports that the filter \"does not support filter protocol\n> version 2\", differentiating between the old single-file filter protocol\n> and the new multi-file filter protocol - I have updated it to something\n> more generic and useful.\n>\n> Signed-off-by: Jonathan Tan \n\nOverall I like the direction, even though the abstraction the\nresulting code results in seems to me a bit too tightly defined; in\nother words, I cannot be sure that this will be useful enough in a\nmore general context, or make some potential applications feel a bit\ntoo constrained.\n\n> +\tstatic int versions[] = {2, 0};\n> +\tstatic struct subprocess_capability capabilities[] = {\n> +\t\t{\"clean\", CAP_CLEAN}, {\"smudge\", CAP_SMUDGE}, {NULL, 0}\n> +\t};\n> \tstruct cmd2process *entry = (struct cmd2process *)subprocess;\n> ...\n> +\treturn subprocess_handshake(subprocess, \"git-filter-\", versions, NULL,\n> +\t\t\t\t capabilities,\n> +\t\t\t\t &entry->supported_capabilities);\n> }\n\nI would have defined the welcome prefix to lack the final dash,\ni.e. forcing the hardcoded suffixes for clients and servers in any\nprotocol that uses this API to end with \"-client\" and \"-server\",\ni.e. with dash.\n\n> diff --git a/sub-process.c b/sub-process.c\n> index a3cfab1a9..1a3f39bdf 100644\n> --- a/sub-process.c\n> +++ b/sub-process.c\n> @@ -105,3 +105,97 @@ int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, co\n> \thashmap_add(hashmap, entry);\n> \treturn 0;\n> }\n> +\n> +int subprocess_handshake(struct subprocess_entry *entry,\n> +\t\t\t const char *welcome_prefix,\n> +\t\t\t int *versions,\n> +\t\t\t int *chosen_version,\n> +\t\t\t struct subprocess_capability *capabilities,\n> +\t\t\t unsigned int *supported_capabilities) {\n> +\tint version_scratch;\n> +\tunsigned int capabilities_scratch;\n> +\tstruct child_process *process = &entry->process;\n> +\tint i;\n> +\tchar *line;\n> +\tconst char *p;\n> +\n> +\tif (!chosen_version)\n> +\t\tchosen_version = &version_scratch;\n> +\tif (!supported_capabilities)\n> +\t\tsupported_capabilities = &capabilities_scratch;\n> +\n> +\tsigchain_push(SIGPIPE, SIG_IGN);\n> +\n> +\tif (packet_write_fmt_gently(process->in, \"%sclient\\n\",\n> +\t\t\t\t welcome_prefix)) {\n> +\t\terror(\"Could not write client identification\");\n> +\t\tgoto error;\n> +\t}\n> +\tfor (i = 0; versions[i]; i++) {\n> +\t\tif (packet_write_fmt_gently(process->in, \"version=%d\\n\",\n> +\t\t\t\t\t versions[i])) {\n> +\t\t\terror(\"Could not write requested version\");\n> +\t\t\tgoto error;\n> +\t\t}\n> +\t}\n\nThis forces version numbers to be positive integers, which is OK, as\nI do not see it a downside that any potential application cannot use\n\"version=0\".\n\n> +\tif (packet_flush_gently(process->in))\n> +\t\tgoto error;\n> +\n> +\tif (!(line = packet_read_line(process->out, NULL)) ||\n> +\t !skip_prefix(line, welcome_prefix, &p) ||\n> +\t strcmp(p, \"server\")) {\n> +\t\terror(\"Unexpected line '%s', expected %sserver\",\n> +\t\t line ? line : \"\", welcome_prefix);\n> +\t\tgoto error;\n> +\t}\n> +\tif (!(line = packet_read_line(process->out, NULL)) ||\n> +\t !skip_prefix(line, \"version=\", &p) ||\n> +\t strtol_i(p, 10, chosen_version)) {\n> +\t\terror(\"Unexpected line '%s', expected version\",\n> +\t\t line ? line : \"\");\n> +\t\tgoto error;\n> +\t}\n> +\tfor (i = 0; versions[i]; i++) {\n> +\t\tif (versions[i] == *chosen_version)\n> +\t\t\tgoto version_found;\n> +\t}\n> +\terror(\"Version %d not supported\", *chosen_version);\n> +\tgoto error;\n> +version_found:\n\nIt would have been more natural to do\n\n\tfor (i = 0; versions[i]; i++)\n\t\tif (versions[i] == *chosen_version)\n\t\t\tbreak;\n\tif (versions[i]) {\n\t\terror(\"...\");\n\t\tgoto error;\n\t}\n\nwithout \"version_found:\" label. In general, I'd prefer to avoid\njumping to a label in the normal/expected case and reserve \"goto\"\nfor error handling.\n\n> +\tif ((line = packet_read_line(process->out, NULL))) {\n> +\t\terror(\"Unexpected line '%s', expected flush\", line);\n> +\t\tgoto error;\n> +\t}\n> +\n> +\tfor (i = 0; capabilities[i].name; i++) {\n> +\t\tif (packet_write_fmt_gently(process->in, \"capability=%s\\n\",\n> +\t\t\t\t\t capabilities[i].name)) {\n> +\t\t\terror(\"Could not write requested capability\");\n> +\t\t\tgoto error;\n> +\t\t}\n> +\t}\n> +\tif (packet_flush_gently(process->in))\n> +\t\tgoto error;\n> +\n> +\twhile ((line = packet_read_line(process->out, NULL))) {\n> +\t\tif (!skip_prefix(line, \"capability=\", &p))\n> +\t\t\tcontinue;\n> +\n> +\t\tfor (i = 0; capabilities[i].name; i++) {\n> +\t\t\tif (!strcmp(p, capabilities[i].name)) {\n> +\t\t\t\t*supported_capabilities |= capabilities[i].flag;\n> +\t\t\t\tgoto capability_found;\n> +\t\t\t}\n> +\t\t}\n> +\t\twarning(\"external filter requested unsupported filter capability '%s'\",\n> +\t\t\tp);\n> +capability_found:\n> +\t\t;\n\nLikewise.\n\nAlso, this is the reason why I said this might make future\napplications feel a bit too constrained; is the set of fields in the\nsubprocess_capability struct general enough? It can only say \"a\ncapability with this name was found\" with a single bit, so you can\nhave only 32 (or 64) capabilities that are all yes/no. I am not\nsaying that is definitely insufficient (not yet anyway); I am\nwondering if future applications may need to have something like:\n\n\tcapability=buffer-size=64k\n\nwhere \"=64k\" part is not known at this layer but is known by the\nuser of the API.\n\n> +\t}\n> +\n> +\tsigchain_pop(SIGPIPE);\n> +\treturn 0;\n> +error:\n> +\tsigchain_pop(SIGPIPE);\n> +\treturn 1;\n\nI would prepare at the beginning of the function:\n\n\tint retval = -1; /* assume failure */\n\nand rewrite the above to\n\n retval = 0;\n error:\n sigchain_pop(SIGPIPE);\n return retval;\n\nif I were writing this code.\n"},{"id":"325131","messageId":"EB54F0E5-E555-40BF-8AA7-7CB3174FE22C@gmail.com","threadId":"46457","inReplyTo":"20170724213810.29831-1-jonathantanmy@google.com","subject":"Re: [PATCH] sub-process: refactor handshake to common function","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-07-26T16:52:29Z","receivedAt":"2017-07-26T16:52:40Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 24 Jul 2017, at 23:38, Jonathan Tan wrote:\n> \n> Refactor, into a common function, the version and capability negotiation\n> done when invoking a long-running process as a clean or smudge filter.\n> This will be useful for other Git code that needs to interact similarly\n> with a long-running process.\n> \n> Signed-off-by: Jonathan Tan \n> ---\n> This will be useful for anyone implementing functionality like that in\n> [1].\n> \n> It is unfortunate that the code grew larger because it had to be more\n> generic, but I think this is better than having (in the future) 2 or\n> more separate handshake functions.\n> \n> I also don't think that the protocol should be permissive - I think\n> things would be simpler if all protocol errors were fatal errors. As it\n> is, most parts are permissive, but packet_read_line() already dies\n> anyway upon a malformed packet, so it may not be too drastic a change to\n> change this. For reference, the original protocol comes from [2].\n> \n> [1] https://public-inbox.org/git/20170714132651.170708-2-benpeart@microsoft.com/\n> [2] edcc858 (\"convert: add filter..process option\", 2016-10-17)\n\nThanks for this refactoring! That's great!\n\nPlease note that I've recently refactored the capabilities negotiation a bit:\nhttps://github.com/git/git/commit/1514c8edd62d96006cd1de31e906ed5798dd4681\n\nThis change is still cooking in `next`. I am not sure how this should/could\nbe handled but maybe you can use my refactoring as your base?\n\nThanks,\nLars\n\n\n> ---\n> convert.c | 61 ++++-----------------------------\n> pkt-line.c | 19 -----------\n> pkt-line.h | 2 --\n> sub-process.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++\n> sub-process.h | 18 ++++++++++\n> t/t0021-conversion.sh | 2 +-\n> 6 files changed, 120 insertions(+), 76 deletions(-)\n> \n> diff --git a/convert.c b/convert.c\n> index deaf0ba7b..ec8ecc2ea 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -512,62 +512,15 @@ static struct hashmap subprocess_map;\n> \n> static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)\n> {\n> -\tint err;\n> +\tstatic int versions[] = {2, 0};\n> +\tstatic struct subprocess_capability capabilities[] = {\n> +\t\t{\"clean\", CAP_CLEAN}, {\"smudge\", CAP_SMUDGE}, {NULL, 0}\n> +\t};\n> \tstruct cmd2process *entry = (struct cmd2process *)subprocess;\n> -\tstruct string_list cap_list = STRING_LIST_INIT_NODUP;\n> -\tchar *cap_buf;\n> -\tconst char *cap_name;\n> -\tstruct child_process *process = &subprocess->process;\n> -\tconst char *cmd = subprocess->cmd;\n> -\n> -\tsigchain_push(SIGPIPE, SIG_IGN);\n> -\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\tgoto done;\n> -\t}\n> -\terr = strcmp(packet_read_line(process->out, NULL), \"version=2\");\n> -\tif (err)\n> -\t\tgoto done;\n> -\terr = packet_read_line(process->out, NULL) != NULL;\n> -\tif (err)\n> -\t\tgoto done;\n> -\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> -\t\tif (!cap_buf)\n> -\t\t\tbreak;\n> -\t\tstring_list_split_in_place(&cap_list, cap_buf, '=', 1);\n> -\n> -\t\tif (cap_list.nr != 2 || strcmp(cap_list.items[0].string, \"capability\"))\n> -\t\t\tcontinue;\n> -\n> -\t\tcap_name = cap_list.items[1].string;\n> -\t\tif (!strcmp(cap_name, \"clean\")) {\n> -\t\t\tentry->supported_capabilities |= CAP_CLEAN;\n> -\t\t} else if (!strcmp(cap_name, \"smudge\")) {\n> -\t\t\tentry->supported_capabilities |= CAP_SMUDGE;\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);\n> -\t\t}\n> -\n> -\t\tstring_list_clear(&cap_list, 0);\n> -\t}\n> -\n> -done:\n> -\tsigchain_pop(SIGPIPE);\n> \n> -\treturn err;\n> +\treturn subprocess_handshake(subprocess, \"git-filter-\", versions, NULL,\n> +\t\t\t\t capabilities,\n> +\t\t\t\t &entry->supported_capabilities);\n> }\n> \n> static int apply_multi_file_filter(const char *path, const char *src, size_t len,\n> diff --git a/pkt-line.c b/pkt-line.c\n> index 9d845ecc3..7db911957 100644\n> --- a/pkt-line.c\n> +++ b/pkt-line.c\n> @@ -171,25 +171,6 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)\n> \treturn status;\n> }\n> \n> -int packet_writel(int fd, const char *line, ...)\n> -{\n> -\tva_list args;\n> -\tint err;\n> -\tva_start(args, line);\n> -\tfor (;;) {\n> -\t\tif (!line)\n> -\t\t\tbreak;\n> -\t\tif (strlen(line) > LARGE_PACKET_DATA_MAX)\n> -\t\t\treturn -1;\n> -\t\terr = packet_write_fmt_gently(fd, \"%s\\n\", line);\n> -\t\tif (err)\n> -\t\t\treturn err;\n> -\t\tline = va_arg(args, const char*);\n> -\t}\n> -\tva_end(args);\n> -\treturn packet_flush_gently(fd);\n> -}\n> -\n> static int packet_write_gently(const int fd_out, const char *buf, size_t size)\n> {\n> \tstatic char packet_write_buffer[LARGE_PACKET_MAX];\n> diff --git a/pkt-line.h b/pkt-line.h\n> index 450183b64..66ef610fc 100644\n> --- a/pkt-line.h\n> +++ b/pkt-line.h\n> @@ -25,8 +25,6 @@ void packet_buf_flush(struct strbuf *buf);\n> void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n> int packet_flush_gently(int fd);\n> int packet_write_fmt_gently(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n> -LAST_ARG_MUST_BE_NULL\n> -int packet_writel(int fd, const char *line, ...);\n> int write_packetized_from_fd(int fd_in, int fd_out);\n> int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);\n> \n> diff --git a/sub-process.c b/sub-process.c\n> index a3cfab1a9..1a3f39bdf 100644\n> --- a/sub-process.c\n> +++ b/sub-process.c\n> @@ -105,3 +105,97 @@ int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, co\n> \thashmap_add(hashmap, entry);\n> \treturn 0;\n> }\n> +\n> +int subprocess_handshake(struct subprocess_entry *entry,\n> +\t\t\t const char *welcome_prefix,\n> +\t\t\t int *versions,\n> +\t\t\t int *chosen_version,\n> +\t\t\t struct subprocess_capability *capabilities,\n> +\t\t\t unsigned int *supported_capabilities) {\n> +\tint version_scratch;\n> +\tunsigned int capabilities_scratch;\n> +\tstruct child_process *process = &entry->process;\n> +\tint i;\n> +\tchar *line;\n> +\tconst char *p;\n> +\n> +\tif (!chosen_version)\n> +\t\tchosen_version = &version_scratch;\n> +\tif (!supported_capabilities)\n> +\t\tsupported_capabilities = &capabilities_scratch;\n> +\n> +\tsigchain_push(SIGPIPE, SIG_IGN);\n> +\n> +\tif (packet_write_fmt_gently(process->in, \"%sclient\\n\",\n> +\t\t\t\t welcome_prefix)) {\n> +\t\terror(\"Could not write client identification\");\n> +\t\tgoto error;\n> +\t}\n> +\tfor (i = 0; versions[i]; i++) {\n> +\t\tif (packet_write_fmt_gently(process->in, \"version=%d\\n\",\n> +\t\t\t\t\t versions[i])) {\n> +\t\t\terror(\"Could not write requested version\");\n> +\t\t\tgoto error;\n> +\t\t}\n> +\t}\n> +\tif (packet_flush_gently(process->in))\n> +\t\tgoto error;\n> +\n> +\tif (!(line = packet_read_line(process->out, NULL)) ||\n> +\t !skip_prefix(line, welcome_prefix, &p) ||\n> +\t strcmp(p, \"server\")) {\n> +\t\terror(\"Unexpected line '%s', expected %sserver\",\n> +\t\t line ? line : \"\", welcome_prefix);\n> +\t\tgoto error;\n> +\t}\n> +\tif (!(line = packet_read_line(process->out, NULL)) ||\n> +\t !skip_prefix(line, \"version=\", &p) ||\n> +\t strtol_i(p, 10, chosen_version)) {\n> +\t\terror(\"Unexpected line '%s', expected version\",\n> +\t\t line ? line : \"\");\n> +\t\tgoto error;\n> +\t}\n> +\tfor (i = 0; versions[i]; i++) {\n> +\t\tif (versions[i] == *chosen_version)\n> +\t\t\tgoto version_found;\n> +\t}\n> +\terror(\"Version %d not supported\", *chosen_version);\n> +\tgoto error;\n> +version_found:\n> +\tif ((line = packet_read_line(process->out, NULL))) {\n> +\t\terror(\"Unexpected line '%s', expected flush\", line);\n> +\t\tgoto error;\n> +\t}\n> +\n> +\tfor (i = 0; capabilities[i].name; i++) {\n> +\t\tif (packet_write_fmt_gently(process->in, \"capability=%s\\n\",\n> +\t\t\t\t\t capabilities[i].name)) {\n> +\t\t\terror(\"Could not write requested capability\");\n> +\t\t\tgoto error;\n> +\t\t}\n> +\t}\n> +\tif (packet_flush_gently(process->in))\n> +\t\tgoto error;\n> +\n> +\twhile ((line = packet_read_line(process->out, NULL))) {\n> +\t\tif (!skip_prefix(line, \"capability=\", &p))\n> +\t\t\tcontinue;\n> +\n> +\t\tfor (i = 0; capabilities[i].name; i++) {\n> +\t\t\tif (!strcmp(p, capabilities[i].name)) {\n> +\t\t\t\t*supported_capabilities |= capabilities[i].flag;\n> +\t\t\t\tgoto capability_found;\n> +\t\t\t}\n> +\t\t}\n> +\t\twarning(\"external filter requested unsupported filter capability '%s'\",\n> +\t\t\tp);\n> +capability_found:\n> +\t\t;\n> +\t}\n> +\n> +\tsigchain_pop(SIGPIPE);\n> +\treturn 0;\n> +error:\n> +\tsigchain_pop(SIGPIPE);\n> +\treturn 1;\n> +}\n> diff --git a/sub-process.h b/sub-process.h\n> index 96a2cca36..a72e7f7cf 100644\n> --- a/sub-process.h\n> +++ b/sub-process.h\n> @@ -18,6 +18,11 @@ struct subprocess_entry {\n> \tstruct child_process process;\n> };\n> \n> +struct subprocess_capability {\n> +\tconst char *name;\n> +\tunsigned int flag;\n> +};\n> +\n> /* subprocess functions */\n> \n> extern int cmd2process_cmp(const void *unused_cmp_data,\n> @@ -41,6 +46,19 @@ static inline struct child_process *subprocess_get_child_process(\n> \treturn &entry->process;\n> }\n> \n> +/*\n> + * Perform the handshake to a long-running process as described in the\n> + * gitattributes documentation using the given requested versions and\n> + * capabilities. The \"versions\" and \"capabilities\" parameters are arrays\n> + * terminated by a 0 or blank struct.\n> + */\n> +int subprocess_handshake(struct subprocess_entry *entry,\n> +\t\t\t const char *welcome_prefix,\n> +\t\t\t int *versions,\n> +\t\t\t int *chosen_version,\n> +\t\t\t struct subprocess_capability *capabilities,\n> +\t\t\t unsigned int *supported_capabilities);\n> +\n> /*\n> * Helper function that will read packets looking for \"status=\"\n> * key/value pairs and return the value from the last \"status\" packet\n> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\n> index 161f56044..d191a7228 100755\n> --- a/t/t0021-conversion.sh\n> +++ b/t/t0021-conversion.sh\n> @@ -697,7 +697,7 @@ test_expect_success PERL 'invalid process filter must fail (and not hang!)' '\n> \n> \t\tcp \"$TEST_ROOT/test.o\" test.r &&\n> \t\ttest_must_fail git add . 2>git-stderr.log &&\n> -\t\tgrep \"does not support filter protocol version\" git-stderr.log\n> +\t\tgrep \"expected git-filter-server\" git-stderr.log\n> \t)\n> '\n> \n> -- \n> 2.14.0.rc0.400.g1c36432dff-goog\n> \n\n"},{"id":"325135","messageId":"xmqqeft3rrpy.fsf@gitster.mtv.corp.google.com","threadId":"46457","inReplyTo":"EB54F0E5-E555-40BF-8AA7-7CB3174FE22C@gmail.com","subject":"Re: [PATCH] sub-process: refactor handshake to common function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-26T18:14:01Z","receivedAt":"2017-07-26T18:14:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n> Please note that I've recently refactored the capabilities negotiation a bit:\n> https://github.com/git/git/commit/1514c8edd62d96006cd1de31e906ed5798dd4681\n>\n> This change is still cooking in `next`. I am not sure how this should/could\n> be handled but maybe you can use my refactoring as your base?\n\nI think they can play well together as independent topics. The\nknown_caps[] thing you introduced essentially is the same as the\nstruct subprocess_capability capablities[] Jonathan has.\n\nPlease check\n\n $ git show \"pu^{/jt/subprocess-handshake' into pu}\" convert.c\n\nto see if it makes sense.\n\nThanks.\n\n\n\n\n"},{"id":"325136","messageId":"cover.1501092795.git.jonathantanmy@google.com","threadId":"46457","inReplyTo":"20170724213810.29831-1-jonathantanmy@google.com","subject":"[PATCH for NEXT v3 0/2] sub-process: refactor handshake to common function","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-07-26T18:17:27Z","receivedAt":"2017-07-26T18:17:45Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Thanks for all your comments.\n\nThis is now built off \"next\" to include Lars's changes.\n\nAbout whether this is too restrictive (for example, as Junio mentions,\nthis will not allow future capabilities of the form \"key=???\"), I think\nthat this can be upgraded later if necessary. For now, both the filter\ncode and the ODB (and ODB-like) proposals on the mailing list do not\nrequire such a thing, so I have not included that functionality.\n\nChanges from v2:\n - now rebased onto next, to pick up Lars's changes\n - split up into more functions\n - welcome prefix does not include final dash\n - no more gotos in expected cases (or at all)\n\nJonathan Tan (2):\n Documentation: migrate sub-process docs to header\n sub-process: refactor handshake to common function\n\n Documentation/technical/api-sub-process.txt | 59 ----------------\n convert.c | 75 ++------------------\n pkt-line.c | 19 -----\n pkt-line.h | 2 -\n sub-process.c | 103 ++++++++++++++++++++++++++++\n sub-process.h | 51 +++++++++++++-\n t/t0021-conversion.sh | 2 +-\n 7 files changed, 160 insertions(+), 151 deletions(-)\n delete mode 100644 Documentation/technical/api-sub-process.txt\n\n-- \n2.14.0.rc0.400.g1c36432dff-goog\n\n"},{"id":"325137","messageId":"b3700e7b49cbefd99511f31d3e0b2837f48964fb.1501092795.git.jonathantanmy@google.com","threadId":"46457","inReplyTo":"cover.1501092795.git.jonathantanmy@google.com","subject":"[PATCH for NEXT v3 1/2] Documentation: migrate sub-process docs to header","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-07-26T18:17:28Z","receivedAt":"2017-07-26T18:17:48Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Move the documentation for the sub-process API from a separate txt file\nto its header file.\n\nSigned-off-by: Jonathan Tan \n---\n Documentation/technical/api-sub-process.txt | 59 -----------------------------\n sub-process.h | 25 +++++++++++-\n 2 files changed, 23 insertions(+), 61 deletions(-)\n delete mode 100644 Documentation/technical/api-sub-process.txt\n\ndiff --git a/Documentation/technical/api-sub-process.txt b/Documentation/technical/api-sub-process.txt\ndeleted file mode 100644\nindex 793508cf3..000000000\n--- a/Documentation/technical/api-sub-process.txt\n+++ /dev/null\n@@ -1,59 +0,0 @@\n-sub-process API\n-===============\n-\n-The sub-process API makes it possible to run background sub-processes\n-for the entire lifetime of a Git invocation. If Git needs to communicate\n-with an external process multiple times, then this can reduces the process\n-invocation overhead. Git and the sub-process communicate through stdin and\n-stdout.\n-\n-The sub-processes are kept in a hashmap by command name and looked up\n-via the subprocess_find_entry function. If an existing instance can not\n-be found then a new process should be created and started. When the\n-parent git command terminates, all sub-processes are also terminated.\n-\n-This API is based on the run-command API.\n-\n-Data structures\n----------------\n-\n-* `struct subprocess_entry`\n-\n-The sub-process structure. Members should not be accessed directly.\n-\n-Types\n------\n-\n-'int(*subprocess_start_fn)(struct subprocess_entry *entry)'::\n-\n-\tUser-supplied function to initialize the sub-process. This is\n-\ttypically used to negotiate the interface version and capabilities.\n-\n-\n-Functions\n----------\n-\n-`cmd2process_cmp`::\n-\n-\tFunction to test two subprocess hashmap entries for equality.\n-\n-`subprocess_start`::\n-\n-\tStart a subprocess and add it to the subprocess hashmap.\n-\n-`subprocess_stop`::\n-\n-\tKill a subprocess and remove it from the subprocess hashmap.\n-\n-`subprocess_find_entry`::\n-\n-\tFind a subprocess in the subprocess hashmap.\n-\n-`subprocess_get_child_process`::\n-\n-\tGet the underlying `struct child_process` from a subprocess.\n-\n-`subprocess_read_status`::\n-\n-\tHelper function to read packets looking for the last \"status=\"\n-\tkey/value pair.\ndiff --git a/sub-process.h b/sub-process.h\nindex 8cd07a59a..d37c1499a 100644\n--- a/sub-process.h\n+++ b/sub-process.h\n@@ -6,12 +6,23 @@\n #include \"run-command.h\"\n \n /*\n- * Generic implementation of background process infrastructure.\n- * See: Documentation/technical/api-sub-process.txt\n+ * The sub-process API makes it possible to run background sub-processes\n+ * for the entire lifetime of a Git invocation. If Git needs to communicate\n+ * with an external process multiple times, then this can reduces the process\n+ * invocation overhead. Git and the sub-process communicate through stdin and\n+ * stdout.\n+ *\n+ * The sub-processes are kept in a hashmap by command name and looked up\n+ * via the subprocess_find_entry function. If an existing instance can not\n+ * be found then a new process should be created and started. When the\n+ * parent git command terminates, all sub-processes are also terminated.\n+ * \n+ * This API is based on the run-command API.\n */\n \n /* data structures */\n \n+/* Members should not be accessed directly. */\n struct subprocess_entry {\n \tstruct hashmap_entry ent; /* must be the first member! */\n \tconst char *cmd;\n@@ -20,21 +31,31 @@ struct subprocess_entry {\n \n /* subprocess functions */\n \n+/* Function to test two subprocess hashmap entries for equality. */\n extern int cmd2process_cmp(const void *unused_cmp_data,\n \t\t\t const void *e1,\n \t\t\t const void *e2,\n \t\t\t const void *unused_keydata);\n \n+/*\n+ * User-supplied function to initialize the sub-process. This is\n+ * typically used to negotiate the interface version and capabilities.\n+ */\n typedef int(*subprocess_start_fn)(struct subprocess_entry *entry);\n+\n+/* Start a subprocess and add it to the subprocess hashmap. */\n int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, const char *cmd,\n \t\tsubprocess_start_fn startfn);\n \n+/* Kill a subprocess and remove it from the subprocess hashmap. */\n void subprocess_stop(struct hashmap *hashmap, struct subprocess_entry *entry);\n \n+/* Find a subprocess in the subprocess hashmap. */\n struct subprocess_entry *subprocess_find_entry(struct hashmap *hashmap, const char *cmd);\n \n /* subprocess helper functions */\n \n+/* Get the underlying `struct child_process` from a subprocess. */\n static inline struct child_process *subprocess_get_child_process(\n \t\tstruct subprocess_entry *entry)\n {\n-- \n2.14.0.rc0.400.g1c36432dff-goog\n\n"},{"id":"325138","messageId":"6327579311fdb941a11b6d452318777a3c42ee65.1501092795.git.jonathantanmy@google.com","threadId":"46457","inReplyTo":"cover.1501092795.git.jonathantanmy@google.com","subject":"[PATCH for NEXT v3 2/2] sub-process: refactor handshake to common function","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-07-26T18:17:29Z","receivedAt":"2017-07-26T18:17:50Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Refactor, into a common function, the version and capability negotiation\ndone when invoking a long-running process as a clean or smudge filter.\nThis will be useful for other Git code that needs to interact similarly\nwith a long-running process.\n\nAs you can see in the change to t0021, this commit changes the error\nmessage reported when the long-running process does not introduce itself\nwith the expected \"server\"-terminated line. Originally, the error\nmessage reports that the filter \"does not support filter protocol\nversion 2\", differentiating between the old single-file filter protocol\nand the new multi-file filter protocol - I have updated it to something\nmore generic and useful.\n\nSigned-off-by: Jonathan Tan \n---\n convert.c | 75 ++++--------------------------------\n pkt-line.c | 19 ----------\n pkt-line.h | 2 -\n sub-process.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++++++\n sub-process.h | 26 +++++++++++++\n t/t0021-conversion.sh | 2 +-\n 6 files changed, 137 insertions(+), 90 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex dbdbb24e4..1012462e3 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -513,78 +513,17 @@ static struct hashmap subprocess_map;\n \n static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)\n {\n-\tint err, i;\n-\tstruct cmd2process *entry = (struct cmd2process *)subprocess;\n-\tstruct string_list cap_list = STRING_LIST_INIT_NODUP;\n-\tchar *cap_buf;\n-\tconst char *cap_name;\n-\tstruct child_process *process = &subprocess->process;\n-\tconst char *cmd = subprocess->cmd;\n-\n-\tstatic const struct {\n-\t\tconst char *name;\n-\t\tunsigned int cap;\n-\t} known_caps[] = {\n+\tstatic int versions[] = {2, 0};\n+\tstatic struct subprocess_capability capabilities[] = {\n \t\t{ \"clean\", CAP_CLEAN },\n \t\t{ \"smudge\", CAP_SMUDGE },\n \t\t{ \"delay\", CAP_DELAY },\n+\t\t{ NULL, 0 }\n \t};\n-\n-\tsigchain_push(SIGPIPE, SIG_IGN);\n-\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\tgoto done;\n-\t}\n-\terr = strcmp(packet_read_line(process->out, NULL), \"version=2\");\n-\tif (err)\n-\t\tgoto done;\n-\terr = packet_read_line(process->out, NULL) != NULL;\n-\tif (err)\n-\t\tgoto done;\n-\n-\tfor (i = 0; i < ARRAY_SIZE(known_caps); ++i) {\n-\t\terr = packet_write_fmt_gently(\n-\t\t\tprocess->in, \"capability=%s\\n\", known_caps[i].name);\n-\t\tif (err)\n-\t\t\tgoto done;\n-\t}\n-\terr = packet_flush_gently(process->in);\n-\tif (err)\n-\t\tgoto done;\n-\n-\tfor (;;) {\n-\t\tcap_buf = packet_read_line(process->out, NULL);\n-\t\tif (!cap_buf)\n-\t\t\tbreak;\n-\t\tstring_list_split_in_place(&cap_list, cap_buf, '=', 1);\n-\n-\t\tif (cap_list.nr != 2 || strcmp(cap_list.items[0].string, \"capability\"))\n-\t\t\tcontinue;\n-\n-\t\tcap_name = cap_list.items[1].string;\n-\t\ti = ARRAY_SIZE(known_caps) - 1;\n-\t\twhile (i >= 0 && strcmp(cap_name, known_caps[i].name))\n-\t\t\ti--;\n-\n-\t\tif (i >= 0)\n-\t\t\tentry->supported_capabilities |= known_caps[i].cap;\n-\t\telse\n-\t\t\twarning(\"external filter '%s' requested unsupported filter capability '%s'\",\n-\t\t\tcmd, cap_name);\n-\n-\t\tstring_list_clear(&cap_list, 0);\n-\t}\n-\n-done:\n-\tsigchain_pop(SIGPIPE);\n-\n-\treturn err;\n+\tstruct cmd2process *entry = (struct cmd2process *)subprocess;\n+\treturn subprocess_handshake(subprocess, \"git-filter\", versions, NULL,\n+\t\t\t\t capabilities,\n+\t\t\t\t &entry->supported_capabilities);\n }\n \n static void handle_filter_error(const struct strbuf *filter_status,\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 9d845ecc3..7db911957 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -171,25 +171,6 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)\n \treturn status;\n }\n \n-int packet_writel(int fd, const char *line, ...)\n-{\n-\tva_list args;\n-\tint err;\n-\tva_start(args, line);\n-\tfor (;;) {\n-\t\tif (!line)\n-\t\t\tbreak;\n-\t\tif (strlen(line) > LARGE_PACKET_DATA_MAX)\n-\t\t\treturn -1;\n-\t\terr = packet_write_fmt_gently(fd, \"%s\\n\", line);\n-\t\tif (err)\n-\t\t\treturn err;\n-\t\tline = va_arg(args, const char*);\n-\t}\n-\tva_end(args);\n-\treturn packet_flush_gently(fd);\n-}\n-\n static int packet_write_gently(const int fd_out, const char *buf, size_t size)\n {\n \tstatic char packet_write_buffer[LARGE_PACKET_MAX];\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 450183b64..66ef610fc 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -25,8 +25,6 @@ void packet_buf_flush(struct strbuf *buf);\n void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n int packet_flush_gently(int fd);\n int packet_write_fmt_gently(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n-LAST_ARG_MUST_BE_NULL\n-int packet_writel(int fd, const char *line, ...);\n int write_packetized_from_fd(int fd_in, int fd_out);\n int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);\n \ndiff --git a/sub-process.c b/sub-process.c\nindex 6cbffa440..37b4bd0ad 100644\n--- a/sub-process.c\n+++ b/sub-process.c\n@@ -108,3 +108,106 @@ int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, co\n \thashmap_add(hashmap, entry);\n \treturn 0;\n }\n+\n+static int handshake_version(struct child_process *process,\n+\t\t\t const char *welcome_prefix, int *versions,\n+\t\t\t int *chosen_version)\n+{\n+\tint version_scratch;\n+\tint i;\n+\tchar *line;\n+\tconst char *p;\n+\n+\tif (!chosen_version)\n+\t\tchosen_version = &version_scratch;\n+\n+\tif (packet_write_fmt_gently(process->in, \"%s-client\\n\",\n+\t\t\t\t welcome_prefix))\n+\t\treturn error(\"Could not write client identification\");\n+\tfor (i = 0; versions[i]; i++) {\n+\t\tif (packet_write_fmt_gently(process->in, \"version=%d\\n\",\n+\t\t\t\t\t versions[i]))\n+\t\t\treturn error(\"Could not write requested version\");\n+\t}\n+\tif (packet_flush_gently(process->in))\n+\t\treturn error(\"Could not write flush packet\");\n+\n+\tif (!(line = packet_read_line(process->out, NULL)) ||\n+\t !skip_prefix(line, welcome_prefix, &p) ||\n+\t strcmp(p, \"-server\"))\n+\t\treturn error(\"Unexpected line '%s', expected %s-server\",\n+\t\t\t line ? line : \"\", welcome_prefix);\n+\tif (!(line = packet_read_line(process->out, NULL)) ||\n+\t !skip_prefix(line, \"version=\", &p) ||\n+\t strtol_i(p, 10, chosen_version))\n+\t\treturn error(\"Unexpected line '%s', expected version\",\n+\t\t\t line ? line : \"\");\n+\tif ((line = packet_read_line(process->out, NULL)))\n+\t\treturn error(\"Unexpected line '%s', expected flush\", line);\n+\n+\t/* Check to make sure that the version received is supported */\n+\tfor (i = 0; versions[i]; i++) {\n+\t\tif (versions[i] == *chosen_version)\n+\t\t\tbreak;\n+\t}\n+\tif (!versions[i])\n+\t\treturn error(\"Version %d not supported\", *chosen_version);\n+\n+\treturn 0;\n+}\n+\n+static int handshake_capabilities(struct child_process *process,\n+\t\t\t\t struct subprocess_capability *capabilities,\n+\t\t\t\t unsigned int *supported_capabilities)\n+{\n+\tint i;\n+\tchar *line;\n+\n+\tfor (i = 0; capabilities[i].name; i++) {\n+\t\tif (packet_write_fmt_gently(process->in, \"capability=%s\\n\",\n+\t\t\t\t\t capabilities[i].name))\n+\t\t\treturn error(\"Could not write requested capability\");\n+\t}\n+\tif (packet_flush_gently(process->in))\n+\t\treturn error(\"Could not write flush packet\");\n+\n+\twhile ((line = packet_read_line(process->out, NULL))) {\n+\t\tconst char *p;\n+\t\tif (!skip_prefix(line, \"capability=\", &p))\n+\t\t\tcontinue;\n+\n+\t\tfor (i = 0;\n+\t\t capabilities[i].name && strcmp(p, capabilities[i].name);\n+\t\t i++)\n+\t\t\t;\n+\t\tif (capabilities[i].name) {\n+\t\t\tif (supported_capabilities)\n+\t\t\t\t*supported_capabilities |= capabilities[i].flag;\n+\t\t} else {\n+\t\t\twarning(\"external filter requested unsupported filter capability '%s'\",\n+\t\t\t\tp);\n+\t\t}\n+\t}\n+\n+\treturn 0;\n+}\n+\n+int subprocess_handshake(struct subprocess_entry *entry,\n+\t\t\t const char *welcome_prefix,\n+\t\t\t int *versions,\n+\t\t\t int *chosen_version,\n+\t\t\t struct subprocess_capability *capabilities,\n+\t\t\t unsigned int *supported_capabilities) {\n+\tint retval;\n+\tstruct child_process *process = &entry->process;\n+\n+\tsigchain_push(SIGPIPE, SIG_IGN);\n+\n+\tretval = handshake_version(process, welcome_prefix, versions,\n+\t\t\t\t chosen_version) ||\n+\t\t handshake_capabilities(process, capabilities,\n+\t\t\t\t\tsupported_capabilities);\n+\n+\tsigchain_pop(SIGPIPE);\n+\treturn retval;\n+}\ndiff --git a/sub-process.h b/sub-process.h\nindex d37c1499a..6857eb1b5 100644\n--- a/sub-process.h\n+++ b/sub-process.h\n@@ -29,6 +29,16 @@ struct subprocess_entry {\n \tstruct child_process process;\n };\n \n+struct subprocess_capability {\n+\tconst char *name;\n+\n+\t/*\n+\t * subprocess_handshake will \"|=\" this value to supported_capabilities\n+\t * if the server reports that it supports this capability.\n+\t */\n+\tunsigned int flag;\n+};\n+\n /* subprocess functions */\n \n /* Function to test two subprocess hashmap entries for equality. */\n@@ -62,6 +72,22 @@ static inline struct child_process *subprocess_get_child_process(\n \treturn &entry->process;\n }\n \n+/*\n+ * Perform the version and capability negotiation as described in the \"Long\n+ * Running Filter Process\" section of the gitattributes documentation using the\n+ * given requested versions and capabilities. The \"versions\" and \"capabilities\"\n+ * parameters are arrays terminated by a 0 or blank struct.\n+ *\n+ * This function is typically called when a subprocess is started (as part of\n+ * the \"startfn\" passed to subprocess_start).\n+ */\n+int subprocess_handshake(struct subprocess_entry *entry,\n+\t\t\t const char *welcome_prefix,\n+\t\t\t int *versions,\n+\t\t\t int *chosen_version,\n+\t\t\t struct subprocess_capability *capabilities,\n+\t\t\t unsigned int *supported_capabilities);\n+\n /*\n * Helper function that will read packets looking for \"status=\"\n * key/value pairs and return the value from the last \"status\" packet\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex eb3d83744..46f8e583c 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -697,7 +697,7 @@ test_expect_success PERL 'invalid process filter must fail (and not hang!)' '\n \n \t\tcp \"$TEST_ROOT/test.o\" test.r &&\n \t\ttest_must_fail git add . 2>git-stderr.log &&\n-\t\tgrep \"does not support filter protocol version\" git-stderr.log\n+\t\tgrep \"expected git-filter-server\" git-stderr.log\n \t)\n '\n \n-- \n2.14.0.rc0.400.g1c36432dff-goog\n\n"},{"id":"325145","messageId":"xmqqtw1zq8rl.fsf@gitster.mtv.corp.google.com","threadId":"46457","inReplyTo":"cover.1501092795.git.jonathantanmy@google.com","subject":"Re: [PATCH for NEXT v3 0/2] sub-process: refactor handshake to common function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-26T19:48:46Z","receivedAt":"2017-07-26T19:48:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Thanks for all your comments.\n>\n> This is now built off \"next\" to include Lars's changes.\n>\n> About whether this is too restrictive (for example, as Junio mentions,\n> this will not allow future capabilities of the form \"key=???\"), I think\n> that this can be upgraded later if necessary. For now, both the filter\n> code and the ODB (and ODB-like) proposals on the mailing list do not\n> require such a thing, so I have not included that functionality.\n\nHeh, anything can be updated later. The real question is how much\ndamage such an update may incur. If such an update ends up having\nto dismantle large part of what you did, then that is not a real\nconsolation that it \"can be\" updated later.\n\nI think subprocess_handshake() can become the lowest level helper\nthat takes callback function to act on the capability request that\nwas received, and the existing user like start_multi_file_filter_fn()\ncan be served by a thin-wrapper around subprocess_handshake() that\ndefines a standard/built-in callback that checks the capability list\nfor exact match and flips the bit in an unsigned, so it is possible\nto limit the damage when such an enhancement happens.\n\n\n\n>\n> Changes from v2:\n> - now rebased onto next, to pick up Lars's changes\n> - split up into more functions\n> - welcome prefix does not include final dash\n> - no more gotos in expected cases (or at all)\n>\n> Jonathan Tan (2):\n> Documentation: migrate sub-process docs to header\n> sub-process: refactor handshake to common function\n>\n> Documentation/technical/api-sub-process.txt | 59 ----------------\n> convert.c | 75 ++------------------\n> pkt-line.c | 19 -----\n> pkt-line.h | 2 -\n> sub-process.c | 103 ++++++++++++++++++++++++++++\n> sub-process.h | 51 +++++++++++++-\n> t/t0021-conversion.sh | 2 +-\n> 7 files changed, 160 insertions(+), 151 deletions(-)\n> delete mode 100644 Documentation/technical/api-sub-process.txt\n"},{"id":"325248","messageId":"xmqqmv7njjkc.fsf@gitster.mtv.corp.google.com","threadId":"46457","inReplyTo":"cover.1501092795.git.jonathantanmy@google.com","subject":"Re: [PATCH for NEXT v3 0/2] sub-process: refactor handshake to common function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-29T16:26:27Z","receivedAt":"2017-07-29T16:26:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Thanks for all your comments.\n>\n> This is now built off \"next\" to include Lars's changes.\n\nDeveloping on 'next' to ask for comments is OK, but try to minimize\nthe dependencies; otherwise you are setting yourself to be taken\nhostage by many unrelated topics.\n\nI did a merge of ls/filter-branch-delayed into v2.14-rc1 and then\napplied these two on top (with one small style-fix) to see how bad\nits dependencies are---I think that is the only topic you need that\nis still in flight, so it does not look too bad.\n\n"},{"id":"325658","messageId":"323E470B-994B-4AD8-9F30-588C2B97A845@gmail.com","threadId":"46457","inReplyTo":"6327579311fdb941a11b6d452318777a3c42ee65.1501092795.git.jonathantanmy@google.com","subject":"Re: [PATCH for NEXT v3 2/2] sub-process: refactor handshake to common function","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-08-06T19:58:24Z","receivedAt":"2017-08-06T19:58:33Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 26 Jul 2017, at 20:17, Jonathan Tan wrote:\n> \n> Refactor, into a common function, the version and capability negotiation\n> done when invoking a long-running process as a clean or smudge filter.\n> This will be useful for other Git code that needs to interact similarly\n> with a long-running process.\n> \n> As you can see in the change to t0021, this commit changes the error\n> message reported when the long-running process does not introduce itself\n> with the expected \"server\"-terminated line. Originally, the error\n> message reports that the filter \"does not support filter protocol\n> version 2\", differentiating between the old single-file filter protocol\n> and the new multi-file filter protocol - I have updated it to something\n> more generic and useful.\n> \n> Signed-off-by: Jonathan Tan \n> ---\n> convert.c | 75 ++++--------------------------------\n> pkt-line.c | 19 ----------\n> pkt-line.h | 2 -\n> sub-process.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++++++\n> sub-process.h | 26 +++++++++++++\n> t/t0021-conversion.sh | 2 +-\n> 6 files changed, 137 insertions(+), 90 deletions(-)\n> \n> diff --git a/convert.c b/convert.c\n> index dbdbb24e4..1012462e3 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -513,78 +513,17 @@ static struct hashmap subprocess_map;\n> \n> static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)\n> {\n> -\tint err, i;\n> -\tstruct cmd2process *entry = (struct cmd2process *)subprocess;\n> -\tstruct string_list cap_list = STRING_LIST_INIT_NODUP;\n> -\tchar *cap_buf;\n> -\tconst char *cap_name;\n> -\tstruct child_process *process = &subprocess->process;\n> -\tconst char *cmd = subprocess->cmd;\n> -\n> -\tstatic const struct {\n> -\t\tconst char *name;\n> -\t\tunsigned int cap;\n> -\t} known_caps[] = {\n> +\tstatic int versions[] = {2, 0};\n> +\tstatic struct subprocess_capability capabilities[] = {\n> \t\t{ \"clean\", CAP_CLEAN },\n> \t\t{ \"smudge\", CAP_SMUDGE },\n> \t\t{ \"delay\", CAP_DELAY },\n> +\t\t{ NULL, 0 }\n> \t};\n> -\n> -\tsigchain_push(SIGPIPE, SIG_IGN);\n> -\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\tgoto done;\n> -\t}\n> -\terr = strcmp(packet_read_line(process->out, NULL), \"version=2\");\n> -\tif (err)\n> -\t\tgoto done;\n> -\terr = packet_read_line(process->out, NULL) != NULL;\n> -\tif (err)\n> -\t\tgoto done;\n> -\n> -\tfor (i = 0; i < ARRAY_SIZE(known_caps); ++i) {\n> -\t\terr = packet_write_fmt_gently(\n> -\t\t\tprocess->in, \"capability=%s\\n\", known_caps[i].name);\n> -\t\tif (err)\n> -\t\t\tgoto done;\n> -\t}\n> -\terr = packet_flush_gently(process->in);\n> -\tif (err)\n> -\t\tgoto done;\n> -\n> -\tfor (;;) {\n> -\t\tcap_buf = packet_read_line(process->out, NULL);\n> -\t\tif (!cap_buf)\n> -\t\t\tbreak;\n> -\t\tstring_list_split_in_place(&cap_list, cap_buf, '=', 1);\n> -\n> -\t\tif (cap_list.nr != 2 || strcmp(cap_list.items[0].string, \"capability\"))\n> -\t\t\tcontinue;\n> -\n> -\t\tcap_name = cap_list.items[1].string;\n> -\t\ti = ARRAY_SIZE(known_caps) - 1;\n> -\t\twhile (i >= 0 && strcmp(cap_name, known_caps[i].name))\n> -\t\t\ti--;\n> -\n> -\t\tif (i >= 0)\n> -\t\t\tentry->supported_capabilities |= known_caps[i].cap;\n> -\t\telse\n> -\t\t\twarning(\"external filter '%s' requested unsupported filter capability '%s'\",\n> -\t\t\tcmd, cap_name);\n> -\n> -\t\tstring_list_clear(&cap_list, 0);\n> -\t}\n> -\n> -done:\n> -\tsigchain_pop(SIGPIPE);\n> -\n> -\treturn err;\n> +\tstruct cmd2process *entry = (struct cmd2process *)subprocess;\n> +\treturn subprocess_handshake(subprocess, \"git-filter\", versions, NULL,\n> +\t\t\t\t capabilities,\n> +\t\t\t\t &entry->supported_capabilities);\n\nWouldn't it make sense to add `supported_capabilities` to `struct subprocess_entry` ?\n\n\n> }\n> \n> static void handle_filter_error(const struct strbuf *filter_status,\n> diff --git a/pkt-line.c b/pkt-line.c\n> index 9d845ecc3..7db911957 100644\n> --- a/pkt-line.c\n> +++ b/pkt-line.c\n> @@ -171,25 +171,6 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)\n> \treturn status;\n> }\n> \n> -int packet_writel(int fd, const char *line, ...)\n> -{\n> -\tva_list args;\n> -\tint err;\n> -\tva_start(args, line);\n> -\tfor (;;) {\n> -\t\tif (!line)\n> -\t\t\tbreak;\n> -\t\tif (strlen(line) > LARGE_PACKET_DATA_MAX)\n> -\t\t\treturn -1;\n> -\t\terr = packet_write_fmt_gently(fd, \"%s\\n\", line);\n> -\t\tif (err)\n> -\t\t\treturn err;\n> -\t\tline = va_arg(args, const char*);\n> -\t}\n> -\tva_end(args);\n> -\treturn packet_flush_gently(fd);\n> -}\n> -\n> static int packet_write_gently(const int fd_out, const char *buf, size_t size)\n> {\n> \tstatic char packet_write_buffer[LARGE_PACKET_MAX];\n> diff --git a/pkt-line.h b/pkt-line.h\n> index 450183b64..66ef610fc 100644\n> --- a/pkt-line.h\n> +++ b/pkt-line.h\n> @@ -25,8 +25,6 @@ void packet_buf_flush(struct strbuf *buf);\n> void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n> int packet_flush_gently(int fd);\n> int packet_write_fmt_gently(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n> -LAST_ARG_MUST_BE_NULL\n> -int packet_writel(int fd, const char *line, ...);\n> int write_packetized_from_fd(int fd_in, int fd_out);\n> int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);\n> \n> diff --git a/sub-process.c b/sub-process.c\n> index 6cbffa440..37b4bd0ad 100644\n> --- a/sub-process.c\n> +++ b/sub-process.c\n> @@ -108,3 +108,106 @@ int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, co\n> \thashmap_add(hashmap, entry);\n> \treturn 0;\n> }\n> +\n> +static int handshake_version(struct child_process *process,\n> +\t\t\t const char *welcome_prefix, int *versions,\n\nMaybe it would be less ambiguous if we call it `supported_versions` ? \n\n\n> +\t\t\t int *chosen_version)\n> +{\n> +\tint version_scratch;\n> +\tint i;\n> +\tchar *line;\n> +\tconst char *p;\n> +\n> +\tif (!chosen_version)\n> +\t\tchosen_version = &version_scratch;\n\nI am not an C expert but wouldn't 'version_scratch' go out of scope as soon\nas the function returns? Why don't you error here right away?\n\n\n> +\tif (packet_write_fmt_gently(process->in, \"%s-client\\n\",\n> +\t\t\t\t welcome_prefix))\n> +\t\treturn error(\"Could not write client identification\");\n\nNit: Would it make sense to rename `welcome_prefix` to `client_id`?\nAlternatively, could we rename the error messages to \"welcome prefix\"?\n\n\n> +\tfor (i = 0; versions[i]; i++) {\n> +\t\tif (packet_write_fmt_gently(process->in, \"version=%d\\n\",\n> +\t\t\t\t\t versions[i]))\n> +\t\t\treturn error(\"Could not write requested version\");\n\nMaybe: \"Could not write supported versions\"?\n\n\n> +\t}\n> +\tif (packet_flush_gently(process->in))\n> +\t\treturn error(\"Could not write flush packet\");\n\nI feel this error is too generic.\nMaybe: \"Could not finish writing supported versions\"?\n\n\n> +\n> +\tif (!(line = packet_read_line(process->out, NULL)) ||\n> +\t !skip_prefix(line, welcome_prefix, &p) ||\n> +\t strcmp(p, \"-server\"))\n> +\t\treturn error(\"Unexpected line '%s', expected %s-server\",\n> +\t\t\t line ? line : \"\", welcome_prefix);\n> +\tif (!(line = packet_read_line(process->out, NULL)) ||\n> +\t !skip_prefix(line, \"version=\", &p) ||\n> +\t strtol_i(p, 10, chosen_version))\n\nMaybe `strlen(\"version=\")` would be more clear than 10?\n\n\n> +\t\treturn error(\"Unexpected line '%s', expected version\",\n\nMaybe \"... expected version number\" ?\n\n\n> +\t\t\t line ? line : \"\");\n> +\tif ((line = packet_read_line(process->out, NULL)))\n> +\t\treturn error(\"Unexpected line '%s', expected flush\", line);\n> +\n> +\t/* Check to make sure that the version received is supported */\n> +\tfor (i = 0; versions[i]; i++) {\n> +\t\tif (versions[i] == *chosen_version)\n> +\t\t\tbreak;\n> +\t}\n> +\tif (!versions[i])\n> +\t\treturn error(\"Version %d not supported\", *chosen_version);\n> +\n> +\treturn 0;\n> +}\n> +\n> +static int handshake_capabilities(struct child_process *process,\n> +\t\t\t\t struct subprocess_capability *capabilities,\n> +\t\t\t\t unsigned int *supported_capabilities)\n\nI feel the naming could be misleading. I think ...\n`capabilities` is really `supported_capabilities` \nand \n`supported_capabilities` is really `negiotated_capabilties` or `agreed_capabilites`\n\n\n> +{\n> +\tint i;\n> +\tchar *line;\n> +\n> +\tfor (i = 0; capabilities[i].name; i++) {\n> +\t\tif (packet_write_fmt_gently(process->in, \"capability=%s\\n\",\n> +\t\t\t\t\t capabilities[i].name))\n> +\t\t\treturn error(\"Could not write requested capability\");\n\nI think this should be \"Could not write supported capability\", no?\n\n\n> +\t}\n> +\tif (packet_flush_gently(process->in))\n> +\t\treturn error(\"Could not write flush packet\");\n\nMaybe \" \"Could not finish writing supported capability\" ?\n\n\n> +\twhile ((line = packet_read_line(process->out, NULL))) {\n> +\t\tconst char *p;\n> +\t\tif (!skip_prefix(line, \"capability=\", &p))\n> +\t\t\tcontinue;\n\nShouldn't we write an error in this case?\n\n\n> +\t\tfor (i = 0;\n> +\t\t capabilities[i].name && strcmp(p, capabilities[i].name);\n> +\t\t i++)\n> +\t\t\t;\n> +\t\tif (capabilities[i].name) {\n> +\t\t\tif (supported_capabilities)\n> +\t\t\t\t*supported_capabilities |= capabilities[i].flag;\n> +\t\t} else {\n> +\t\t\twarning(\"external filter requested unsupported filter capability '%s'\",\n> +\t\t\t\tp);\n> +\t\t}\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +int subprocess_handshake(struct subprocess_entry *entry,\n> +\t\t\t const char *welcome_prefix,\n> +\t\t\t int *versions,\n> +\t\t\t int *chosen_version,\n> +\t\t\t struct subprocess_capability *capabilities,\n> +\t\t\t unsigned int *supported_capabilities) {\n> +\tint retval;\n> +\tstruct child_process *process = &entry->process;\n> +\n> +\tsigchain_push(SIGPIPE, SIG_IGN);\n> +\n> +\tretval = handshake_version(process, welcome_prefix, versions,\n> +\t\t\t\t chosen_version) ||\n> +\t\t handshake_capabilities(process, capabilities,\n> +\t\t\t\t\tsupported_capabilities);\n> +\n> +\tsigchain_pop(SIGPIPE);\n> +\treturn retval;\n> +}\n> diff --git a/sub-process.h b/sub-process.h\n> index d37c1499a..6857eb1b5 100644\n> --- a/sub-process.h\n> +++ b/sub-process.h\n> @@ -29,6 +29,16 @@ struct subprocess_entry {\n> \tstruct child_process process;\n> };\n> \n> +struct subprocess_capability {\n> +\tconst char *name;\n> +\n> +\t/*\n> +\t * subprocess_handshake will \"|=\" this value to supported_capabilities\n> +\t * if the server reports that it supports this capability.\n> +\t */\n> +\tunsigned int flag;\n> +};\n> +\n> /* subprocess functions */\n> \n> /* Function to test two subprocess hashmap entries for equality. */\n> @@ -62,6 +72,22 @@ static inline struct child_process *subprocess_get_child_process(\n> \treturn &entry->process;\n> }\n> \n> +/*\n> + * Perform the version and capability negotiation as described in the \"Long\n> + * Running Filter Process\" section of the gitattributes documentation using the\n> + * given requested versions and capabilities. The \"versions\" and \"capabilities\"\n> + * parameters are arrays terminated by a 0 or blank struct.\n\nShould we reference the \"gitattributes docs\" if we want to make this generic?\nI thought \"technical/api-sub-process.txt\" would explain this kind of stuff\nand I was surprised that you deleted it in an earlier patch.\n\n\n> + *\n> + * This function is typically called when a subprocess is started (as part of\n> + * the \"startfn\" passed to subprocess_start).\n> + */\n> +int subprocess_handshake(struct subprocess_entry *entry,\n> +\t\t\t const char *welcome_prefix,\n> +\t\t\t int *versions,\n> +\t\t\t int *chosen_version,\n> +\t\t\t struct subprocess_capability *capabilities,\n> +\t\t\t unsigned int *supported_capabilities);\n> +\n> /*\n> * Helper function that will read packets looking for \"status=\"\n> * key/value pairs and return the value from the last \"status\" packet\n> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\n> index eb3d83744..46f8e583c 100755\n> --- a/t/t0021-conversion.sh\n> +++ b/t/t0021-conversion.sh\n> @@ -697,7 +697,7 @@ test_expect_success PERL 'invalid process filter must fail (and not hang!)' '\n> \n> \t\tcp \"$TEST_ROOT/test.o\" test.r &&\n> \t\ttest_must_fail git add . 2>git-stderr.log &&\n> -\t\tgrep \"does not support filter protocol version\" git-stderr.log\n> +\t\tgrep \"expected git-filter-server\" git-stderr.log\n> \t)\n> '\n> \n\nThe generalization of this protocol is nice to see.\n\nThanks for working on it,\nLars\n\n\n\n"},{"id":"325702","messageId":"20170807102136.30b23023@twelve2.svl.corp.google.com","threadId":"46457","inReplyTo":"323E470B-994B-4AD8-9F30-588C2B97A845@gmail.com","subject":"Re: [PATCH for NEXT v3 2/2] sub-process: refactor handshake to common function","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-08-07T17:21:36Z","receivedAt":"2017-08-07T17:21:48Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Sun, 6 Aug 2017 21:58:24 +0200\nLars Schneider wrote:\n\n> > +\tstruct cmd2process *entry = (struct cmd2process *)subprocess;\n> > +\treturn subprocess_handshake(subprocess, \"git-filter\", versions, NULL,\n> > +\t\t\t\t capabilities,\n> > +\t\t\t\t &entry->supported_capabilities);\n> \n> Wouldn't it make sense to add `supported_capabilities` to `struct subprocess_entry` ?\n\nThe members of \"struct subprocess_entry\" are not supposed to be accessed\ndirectly, according to the documentation. If we relaxed that, then we\ncould do this, but before that I think it's better to let the caller\nhandle it.\n\n> > +\n> > +static int handshake_version(struct child_process *process,\n> > +\t\t\t const char *welcome_prefix, int *versions,\n> \n> Maybe it would be less ambiguous if we call it `supported_versions` ? \n\nI thought of that, but I think \"supported_versions\" is actually more\nambiguous, since we don't know if these are versions supported by the\nserver or client or both.\n\n> > +\t\t\t int *chosen_version)\n> > +{\n> > +\tint version_scratch;\n> > +\tint i;\n> > +\tchar *line;\n> > +\tconst char *p;\n> > +\n> > +\tif (!chosen_version)\n> > +\t\tchosen_version = &version_scratch;\n> \n> I am not an C expert but wouldn't 'version_scratch' go out of scope as soon\n> as the function returns? Why don't you error here right away?\n\nIt does, but so does chosen_version. This is meant to allow the caller\nto pass NULL to this function.\n\n> > +\tif (packet_write_fmt_gently(process->in, \"%s-client\\n\",\n> > +\t\t\t\t welcome_prefix))\n> > +\t\treturn error(\"Could not write client identification\");\n> \n> Nit: Would it make sense to rename `welcome_prefix` to `client_id`?\n> Alternatively, could we rename the error messages to \"welcome prefix\"?\n\nI was retaining the existing terminology, but your suggestions seem\nreasonable. This might be best done in another patch once this series\nlands in master, though.\n\n> > +\tfor (i = 0; versions[i]; i++) {\n> > +\t\tif (packet_write_fmt_gently(process->in, \"version=%d\\n\",\n> > +\t\t\t\t\t versions[i]))\n> > +\t\t\treturn error(\"Could not write requested version\");\n> \n> Maybe: \"Could not write supported versions\"?\n\nSame as above - \"supported\" is ambiguous.\n\n> > +\t}\n> > +\tif (packet_flush_gently(process->in))\n> > +\t\treturn error(\"Could not write flush packet\");\n> \n> I feel this error is too generic.\n> Maybe: \"Could not finish writing supported versions\"?\n\nThat's reasonable. This is a rare error, though, and if it does occur, I\nthink this message is more informative. But I'm OK either way.\n\n> > +\n> > +\tif (!(line = packet_read_line(process->out, NULL)) ||\n> > +\t !skip_prefix(line, welcome_prefix, &p) ||\n> > +\t strcmp(p, \"-server\"))\n> > +\t\treturn error(\"Unexpected line '%s', expected %s-server\",\n> > +\t\t\t line ? line : \"\", welcome_prefix);\n> > +\tif (!(line = packet_read_line(process->out, NULL)) ||\n> > +\t !skip_prefix(line, \"version=\", &p) ||\n> > +\t strtol_i(p, 10, chosen_version))\n> \n> Maybe `strlen(\"version=\")` would be more clear than 10?\n\nThe 10 here is the base, not the length. If there's a better way to\nconvert strings to integers, let me know.\n\n> > +\t\treturn error(\"Unexpected line '%s', expected version\",\n> \n> Maybe \"... expected version number\" ?\n\nI'm fine either way.\n\n> > +static int handshake_capabilities(struct child_process *process,\n> > +\t\t\t\t struct subprocess_capability *capabilities,\n> > +\t\t\t\t unsigned int *supported_capabilities)\n> \n> I feel the naming could be misleading. I think ...\n> `capabilities` is really `supported_capabilities` \n> and \n> `supported_capabilities` is really `negiotated_capabilties` or `agreed_capabilites`\n\nThese \"supported capabilities\" are those supported by both the client\n(Git) and the server (the process Git is invoking). I think it's better\nto use this term for the intersection of capabilities, rather than\nexclusively for the client or server.\n\n> > +\tfor (i = 0; capabilities[i].name; i++) {\n> > +\t\tif (packet_write_fmt_gently(process->in, \"capability=%s\\n\",\n> > +\t\t\t\t\t capabilities[i].name))\n> > +\t\t\treturn error(\"Could not write requested capability\");\n> \n> I think this should be \"Could not write supported capability\", no?\n\nSame comment as above.\n\n> > +\t}\n> > +\tif (packet_flush_gently(process->in))\n> > +\t\treturn error(\"Could not write flush packet\");\n> \n> Maybe \" \"Could not finish writing supported capability\" ?\n\nSame comment as the one about writing flush packets above.\n\n> > +\twhile ((line = packet_read_line(process->out, NULL))) {\n> > +\t\tconst char *p;\n> > +\t\tif (!skip_prefix(line, \"capability=\", &p))\n> > +\t\t\tcontinue;\n> \n> Shouldn't we write an error in this case?\n\nI'm preserving the existing behavior.\n\n> > +/*\n> > + * Perform the version and capability negotiation as described in the \"Long\n> > + * Running Filter Process\" section of the gitattributes documentation using the\n> > + * given requested versions and capabilities. The \"versions\" and \"capabilities\"\n> > + * parameters are arrays terminated by a 0 or blank struct.\n> \n> Should we reference the \"gitattributes docs\" if we want to make this generic?\n> I thought \"technical/api-sub-process.txt\" would explain this kind of stuff\n> and I was surprised that you deleted it in an earlier patch.\n\nI think this should be done only when we have two users of this, for\nexample, when a patch like [1] (which does contain the change to move\naway from the gitattributes doc) lands.\n\n[1] https://public-inbox.org/git/eadce97b6a1e80345a2621e71ce187e9e6bc05bf.1501532294.git.jonathantanmy@google.com/\n\n> The generalization of this protocol is nice to see.\n> \n> Thanks for working on it,\n> Lars\n\nThanks for your comments.\n"},{"id":"325707","messageId":"2BEA5CEF-44C9-45DC-A462-579864790EDB@gmail.com","threadId":"46457","inReplyTo":"20170807102136.30b23023@twelve2.svl.corp.google.com","subject":"Re: [PATCH for NEXT v3 2/2] sub-process: refactor handshake to common function","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-08-07T17:51:04Z","receivedAt":"2017-08-07T17:51:16Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 07 Aug 2017, at 19:21, Jonathan Tan wrote:\n> \n> On Sun, 6 Aug 2017 21:58:24 +0200\n> Lars Schneider wrote:\n> \n>>> +\tstruct cmd2process *entry = (struct cmd2process *)subprocess;\n>>> +\treturn subprocess_handshake(subprocess, \"git-filter\", versions, NULL,\n>>> +\t\t\t\t capabilities,\n>>> +\t\t\t\t &entry->supported_capabilities);\n>> \n>> Wouldn't it make sense to add `supported_capabilities` to `struct subprocess_entry` ?\n> \n> The members of \"struct subprocess_entry\" are not supposed to be accessed\n> directly, according to the documentation. If we relaxed that, then we\n> could do this, but before that I think it's better to let the caller\n> handle it.\n\n@Ben: You wrote that \" Members should not be accessed directly.\":\nhttps://github.com/git/git/commit/99605d62e8e7e568035dc953b24b79b3d52f0522#diff-c1655ad5d68943a3dc5bfae8c98466f2R22\nCan you give me a hint why?\n\n@Jonathan: What do you mean by \"it's better to let the caller handle it\"\n\n\n>>> \n>>> +static int handshake_version(struct child_process *process,\n>>> +\t\t\t const char *welcome_prefix, int *versions,\n>> \n>> Maybe it would be less ambiguous if we call it `supported_versions` ? \n> \n> I thought of that, but I think \"supported_versions\" is actually more\n> ambiguous, since we don't know if these are versions supported by the\n> server or client or both.\n\nTrue! Maybe `versions_supported_by_git` to annoy people that hate \nlong variable names ;-)\n\n\n>>> +\t\t\t int *chosen_version)\n>>> +{\n>>> +\tint version_scratch;\n>>> +\tint i;\n>>> +\tchar *line;\n>>> +\tconst char *p;\n>>> +\n>>> +\tif (!chosen_version)\n>>> +\t\tchosen_version = &version_scratch;\n>> \n>> I am not an C expert but wouldn't 'version_scratch' go out of scope as soon\n>> as the function returns? Why don't you error here right away?\n> \n> It does, but so does chosen_version. This is meant to allow the caller\n> to pass NULL to this function.\n\nHm. I think every protocol should be versioned otherwise we could run\ninto trouble in the long run.\n\nTBH I wouldn't support NULL in that case in the first place. If you\nwant to support it then I think we should document it.\n\n\n>>> +\tif (packet_write_fmt_gently(process->in, \"%s-client\\n\",\n>>> +\t\t\t\t welcome_prefix))\n>>> +\t\treturn error(\"Could not write client identification\");\n>> \n>> Nit: Would it make sense to rename `welcome_prefix` to `client_id`?\n>> Alternatively, could we rename the error messages to \"welcome prefix\"?\n> \n> I was retaining the existing terminology, but your suggestions seem\n> reasonable. This might be best done in another patch once this series\n> lands in master, though.\n\nYeah, sorry for my late review :-(\n\n\n>>> +\tfor (i = 0; versions[i]; i++) {\n>>> +\t\tif (packet_write_fmt_gently(process->in, \"version=%d\\n\",\n>>> +\t\t\t\t\t versions[i]))\n>>> +\t\t\treturn error(\"Could not write requested version\");\n>> \n>> Maybe: \"Could not write supported versions\"?\n> \n> Same as above - \"supported\" is ambiguous.\n> \n>>> +\t}\n>>> +\tif (packet_flush_gently(process->in))\n>>> +\t\treturn error(\"Could not write flush packet\");\n>> \n>> I feel this error is too generic.\n>> Maybe: \"Could not finish writing supported versions\"?\n> \n> That's reasonable. This is a rare error, though, and if it does occur, I\n> think this message is more informative. But I'm OK either way.\n\nMy thinking is this: if I see an error then I want to roughly know what\nwent wrong and I want to have a good chance to find the error in the\nsource. The \"Could not write flush packet\" is technically correct but\nit makes it harder to pinpoint the error in the source as we throw\nit in several places.\n\n\n> \n>>> +\n>>> +\tif (!(line = packet_read_line(process->out, NULL)) ||\n>>> +\t !skip_prefix(line, welcome_prefix, &p) ||\n>>> +\t strcmp(p, \"-server\"))\n>>> +\t\treturn error(\"Unexpected line '%s', expected %s-server\",\n>>> +\t\t\t line ? line : \"\", welcome_prefix);\n>>> +\tif (!(line = packet_read_line(process->out, NULL)) ||\n>>> +\t !skip_prefix(line, \"version=\", &p) ||\n>>> +\t strtol_i(p, 10, chosen_version))\n>> \n>> Maybe `strlen(\"version=\")` would be more clear than 10?\n> \n> The 10 here is the base, not the length. If there's a better way to\n> convert strings to integers, let me know.\n\nArgh, of course! Sorry! To my defense: it was late last night :-)\n\n\n> \n>>> +\t\treturn error(\"Unexpected line '%s', expected version\",\n>> \n>> Maybe \"... expected version number\" ?\n> \n> I'm fine either way.\n> \n>>> +static int handshake_capabilities(struct child_process *process,\n>>> +\t\t\t\t struct subprocess_capability *capabilities,\n>>> +\t\t\t\t unsigned int *supported_capabilities)\n>> \n>> I feel the naming could be misleading. I think ...\n>> `capabilities` is really `supported_capabilities` \n>> and \n>> `supported_capabilities` is really `negiotated_capabilties` or `agreed_capabilites`\n> \n> These \"supported capabilities\" are those supported by both the client\n> (Git) and the server (the process Git is invoking). I think it's better\n> to use this term for the intersection of capabilities, rather than\n> exclusively for the client or server.\n> \n>>> +\tfor (i = 0; capabilities[i].name; i++) {\n>>> +\t\tif (packet_write_fmt_gently(process->in, \"capability=%s\\n\",\n>>> +\t\t\t\t\t capabilities[i].name))\n>>> +\t\t\treturn error(\"Could not write requested capability\");\n>> \n>> I think this should be \"Could not write supported capability\", no?\n> \n> Same comment as above.\n> \n>>> +\t}\n>>> +\tif (packet_flush_gently(process->in))\n>>> +\t\treturn error(\"Could not write flush packet\");\n>> \n>> Maybe \" \"Could not finish writing supported capability\" ?\n> \n> Same comment as the one about writing flush packets above.\n> \n>>> +\twhile ((line = packet_read_line(process->out, NULL))) {\n>>> +\t\tconst char *p;\n>>> +\t\tif (!skip_prefix(line, \"capability=\", &p))\n>>> +\t\t\tcontinue;\n>> \n>> Shouldn't we write an error in this case?\n> \n> I'm preserving the existing behavior.\n\nYou're right:\nhttps://github.com/git/git/blob/4384e3cde2ce8ecd194202e171ae16333d241326/convert.c#L549-L550\n\n\n- Lars"},{"id":"325712","messageId":"20170807111708.2d745df2@twelve2.svl.corp.google.com","threadId":"46457","inReplyTo":"2BEA5CEF-44C9-45DC-A462-579864790EDB@gmail.com","subject":"Re: [PATCH for NEXT v3 2/2] sub-process: refactor handshake to common function","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-08-07T18:17:08Z","receivedAt":"2017-08-07T18:17:20Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Mon, 7 Aug 2017 19:51:04 +0200\nLars Schneider wrote:\n\n> \n> > On 07 Aug 2017, at 19:21, Jonathan Tan wrote:\n> > \n> > On Sun, 6 Aug 2017 21:58:24 +0200\n> > Lars Schneider wrote:\n> > \n> >>> +\tstruct cmd2process *entry = (struct cmd2process *)subprocess;\n> >>> +\treturn subprocess_handshake(subprocess, \"git-filter\", versions, NULL,\n> >>> +\t\t\t\t capabilities,\n> >>> +\t\t\t\t &entry->supported_capabilities);\n> >> \n> >> Wouldn't it make sense to add `supported_capabilities` to `struct subprocess_entry` ?\n> > \n> > The members of \"struct subprocess_entry\" are not supposed to be accessed\n> > directly, according to the documentation. If we relaxed that, then we\n> > could do this, but before that I think it's better to let the caller\n> > handle it.\n> \n> @Ben: You wrote that \" Members should not be accessed directly.\":\n> https://github.com/git/git/commit/99605d62e8e7e568035dc953b24b79b3d52f0522#diff-c1655ad5d68943a3dc5bfae8c98466f2R22\n> Can you give me a hint why?\n> \n> @Jonathan: What do you mean by \"it's better to let the caller handle it\"\n\nLet the caller provide their own place to store the capabilities, I\nmean, instead of (say) using a field as you describe and an accessor\nmethod.\n\nI don't feel strongly about this, though.\n\n> > It does, but so does chosen_version. This is meant to allow the caller\n> > to pass NULL to this function.\n> \n> Hm. I think every protocol should be versioned otherwise we could run\n> into trouble in the long run.\n> \n> TBH I wouldn't support NULL in that case in the first place. If you\n> want to support it then I think we should document it.\n\nNote that this NULL is for the chosen version as chosen by the server,\nnot the versions declared as supported by the client.\n\nThe protocol is versioned. Some users (e.g. the filter mechanism) of\nthis subprocess thing would want to pass NULL because they only support\none version and the subprocess thing already ensures that the server\nreport that it supports one of the versions sent.\n"},{"id":"325722","messageId":"142c1757-d366-61fe-ffa8-a398bdfd8c03@gmail.com","threadId":"46457","inReplyTo":"20170807111708.2d745df2@twelve2.svl.corp.google.com","subject":"Re: [PATCH for NEXT v3 2/2] sub-process: refactor handshake to common function","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-08-07T18:29:27Z","receivedAt":"2017-08-07T18:29:38Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 8/7/2017 2:17 PM, Jonathan Tan wrote:\n> On Mon, 7 Aug 2017 19:51:04 +0200\n> Lars Schneider wrote:\n> \n>>\n>>> On 07 Aug 2017, at 19:21, Jonathan Tan wrote:\n>>>\n>>> On Sun, 6 Aug 2017 21:58:24 +0200\n>>> Lars Schneider wrote:\n>>>\n>>>>> +\tstruct cmd2process *entry = (struct cmd2process *)subprocess;\n>>>>> +\treturn subprocess_handshake(subprocess, \"git-filter\", versions, NULL,\n>>>>> +\t\t\t\t capabilities,\n>>>>> +\t\t\t\t &entry->supported_capabilities);\n>>>>\n>>>> Wouldn't it make sense to add `supported_capabilities` to `struct subprocess_entry` ?\n>>>\n>>> The members of \"struct subprocess_entry\" are not supposed to be accessed\n>>> directly, according to the documentation. If we relaxed that, then we\n>>> could do this, but before that I think it's better to let the caller\n>>> handle it.\n>>\n>> @Ben: You wrote that \" Members should not be accessed directly.\":\n>> https://github.com/git/git/commit/99605d62e8e7e568035dc953b24b79b3d52f0522#diff-c1655ad5d68943a3dc5bfae8c98466f2R22\n>> Can you give me a hint why?\n>>\n\nIt's just good object oriented design of providing a layer of \nabstraction between the implementation details and the use of the \nclass/object/API. I was following the model in api-hashmap.txt but \nthere are many other examples of where we don't do this.\n\nPerhaps providing a function that returns the property you want to \naccess (similar to subprocess_get_child_process) would work.\n\n>> @Jonathan: What do you mean by \"it's better to let the caller handle it\"\n> \n> Let the caller provide their own place to store the capabilities, I\n> mean, instead of (say) using a field as you describe and an accessor\n> method.\n> \n> I don't feel strongly about this, though.\n> \n>>> It does, but so does chosen_version. This is meant to allow the caller\n>>> to pass NULL to this function.\n>>\n>> Hm. I think every protocol should be versioned otherwise we could run\n>> into trouble in the long run.\n>>\n>> TBH I wouldn't support NULL in that case in the first place. If you\n>> want to support it then I think we should document it.\n> \n> Note that this NULL is for the chosen version as chosen by the server,\n> not the versions declared as supported by the client.\n> \n> The protocol is versioned. Some users (e.g. the filter mechanism) of\n> this subprocess thing would want to pass NULL because they only support\n> one version and the subprocess thing already ensures that the server\n> report that it supports one of the versions sent.\n> \n"}]}