{"thread":{"id":"43044","subject":"Re: [PATCH v5 05/15] pkt-line: add packet_write_gently_fmt()","startedAt":"2016-08-10T18:07:44Z","lastAt":"2016-08-18T14:24:58Z","messageCount":50,"participants":["Lars Schneider","Junio C Hamano","larsxschneider@gmail.com","Jeff King","Stefan Beller","Johannes Schindelin"],"isPatch":true,"patchVersion":5,"patchTotal":15},"messages":[{"id":"293563","messageId":"2B2AC073-95AD-42E9-AD3A-23E8E13C66DE@gmail.com","threadId":"43044","inReplyTo":"xmqqpopg5yqf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v5 05/15] pkt-line: add packet_write_gently_fmt()","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-10T17:53:22Z","receivedAt":"2016-08-10T18:07:44Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 10 Aug 2016, at 19:18, Junio C Hamano wrote:\n> \n> Jeff King writes:\n> \n>> On Wed, Aug 10, 2016 at 03:04:01PM +0200, larsxschneider@gmail.com wrote:\n>> \n>>> +int packet_write_gently_fmt(int fd, const char *fmt, ...)\n>>> +{\n>>> +\tstatic struct strbuf buf = STRBUF_INIT;\n>>> +\tva_list args;\n>>> +\n>>> +\tstrbuf_reset(&buf);\n>>> +\tva_start(args, fmt);\n>>> +\tformat_packet(1, &buf, fmt, args);\n>>> +\tva_end(args);\n>>> +\tpacket_trace(buf.buf + 4, buf.len - 4, 1);\n>>> +\treturn (write_in_full(fd, buf.buf, buf.len) == buf.len ? 0 : -1);\n>>> +}\n>> \n>> Could the end of this function just be:\n>> \n>> return packet_write_gently(fd, buf.buf, buf.len);\n>> \n>> ? I guess we'd prefer to avoid that, because it incurs an extra\n>> memmove() of the data.\n>> \n>> Similarly, I'd think this could share code with the non-gentle form\n>> (which should be able to just call this and die() if returns an error).\n>> Though sometimes the va_list transformation makes that awkward.\n> \n> Yes.\n\nPeff just posted that he tried the shared code idea but the result\nended up ugly.\n\n\n> Also regarding the naming, please have \"_gently\" at the end; that is\n> how all other function families with _gently variant are named, I\n> think.\n\nOK, I will rename them.\n\nThanks,\nLars\n"},{"id":"293568","messageId":"13CB2673-7C7D-4982-9725-27D4091AAD84@gmail.com","threadId":"43044","inReplyTo":"xmqqtwes5ysb.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v5 04/15] pkt-line: add packet_write_gently()","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-10T17:49:48Z","receivedAt":"2016-08-10T18:17:59Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 10 Aug 2016, at 19:17, Junio C Hamano wrote:\n> \n> Jeff King writes:\n> \n>> On Wed, Aug 10, 2016 at 03:36:45PM +0200, Lars Schneider wrote:\n>> \n>>>> So now we have packet_write() and packet_write_gently(), but they differ\n>>>> in more than just whether they are gentle. That seems like a weird\n>>>> interface.\n>>>> \n>>>> Should we either be picking a new name (e.g., packet_write_mem() or\n>>>> something), or migrating packet_write() to packet_write_fmt()?\n>>> \n>>> Done in \"[PATCH v5 08/15] pkt-line: rename packet_write() to packet_write_fmt()\"\n>> \n>> Ah, OK. Generally I'd suggest to reorder things so that each patch looks\n>> like a step forward (and so the early patches become preparatory steps,\n>> and the justification in them is something like \"we're going to add more\n>> write functions, so let's give this a more descriptive name\").\n> \n> I am guilty for saying \"packet_write() should have been similar to\n> write(2)\". We may want to have a time-period during which there is\n> no \"packet_write()\" in the codebase, before we get to that stage.\n> I.e. rename it to packet_write_fmt() to vacate the name and add\n> packet_write_mem(), and then later rename packet_write_mem() to its\n> final name packet_write(), or something like that. The two-step\n> process would reduce the chance of misconversion.\n\nOK. Does this mean I can leave the \"packet_write()\" to \"packet_write_fmt()\"\nrename as is in this series?\n\nThanks,\nLars\n\n"},{"id":"293571","messageId":"xmqq8tw45vtg.fsf@gitster.mtv.corp.google.com","threadId":"43044","inReplyTo":"13CB2673-7C7D-4982-9725-27D4091AAD84@gmail.com","subject":"Re: [PATCH v5 04/15] pkt-line: add packet_write_gently()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-10T18:21:31Z","receivedAt":"2016-08-10T18:21:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n>> On 10 Aug 2016, at 19:17, Junio C Hamano wrote:\n>> \n> OK. Does this mean I can leave the \"packet_write()\" to \"packet_write_fmt()\"\n> rename as is in this series?\n\nI didn't really check what order you are doing things to answer\nthat.\n\nIf the function that is introduced in this step is a version of\npacket_write_fmt() that does its thing only gently, you would want\nto do the rename s/packet_write/packet_write_fmt/ before this step,\nand then add the new function as packet_write_fmt_gently(), I would\nthink.\n"},{"id":"293574","messageId":"20160810130411.12419-15-larsxschneider@gmail.com","threadId":"43044","inReplyTo":"20160810130411.12419-1-larsxschneider@gmail.com","subject":"[PATCH v5 14/15] convert: add filter..process option","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-10T13:04:10Z","receivedAt":"2016-08-10T18:28:29Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\nGit's clean/smudge mechanism invokes an external filter process for every\nsingle blob that is affected by a filter. If Git filters a lot of blobs\nthen the startup time of the external filter processes can become a\nsignificant part of the overall Git execution time.\n\nIn a preliminary performance test this developer used a clean/smudge filter\nwritten in golang to filter 12,000 files. This process took 364s with the\nexisting filter mechanism and 5s with the new mechanism. See details here:\nhttps://github.com/github/git-lfs/pull/1382\n\nThis patch adds the `filter..process` string option which, if used,\nkeeps the external filter process running and processes all blobs with\nthe packet format (pkt-line) based protocol over standard input and standard\noutput described below.\n\nGit starts the filter when it encounters the first file\nthat needs to be cleaned or smudged. After the filter started\nGit sends a welcome message, a list of supported protocol\nversion numbers, and a flush packet. Git expects to read the\nwelcome message and one protocol version number from the\npreviously sent list. Afterwards Git sends a list of supported\ncapabilities and a flush packet. Git expects to read a list of\ndesired capabilities, which must be a subset of the supported\ncapabilities list, and a flush packet as response:\n------------------------\npacket: git> git-filter-client\npacket: git> version=2\npacket: git> version=42\npacket: git> 0000\npacket: git< git-filter-server\npacket: git< version=2\npacket: git> clean=true\npacket: git> smudge=true\npacket: git> not-yet-invented=true\npacket: git> 0000\npacket: git< clean=true\npacket: git< smudge=true\npacket: git< 0000\n------------------------\nSupported filter capabilities in version 2 are \"clean\" and\n\"smudge\".\n\nAfterwards Git sends a list of \"key=value\" pairs terminated with\na flush packet. The list will contain at least the filter command\n(based on the supported capabilities) and the pathname of the file\nto filter relative to the repository root. Right after these packets\nGit sends the content split in zero or more pkt-line packets and a\nflush packet to terminate content.\n------------------------\npacket: git> command=smudge\\n\npacket: git> pathname=path/testfile.dat\\n\npacket: git> 0000\npacket: git> CONTENT\npacket: git> 0000\n------------------------\n\nThe filter is expected to respond with a list of \"key=value\" pairs\nterminated with a flush packet. If the filter does not experience\nproblems then the list must contain a \"success\" status. Right after\nthese packets the filter is expected to send the content in zero\nor more pkt-line packets and a flush packet at the end. Finally, a\nsecond list of \"key=value\" pairs terminated with a flush packet\nis expected. The filter can change the status in the second list.\n------------------------\npacket: git< status=success\\n\npacket: git< 0000\npacket: git< SMUDGED_CONTENT\npacket: git< 0000\npacket: git< 0000 # empty list!\n------------------------\n\nIf the result content is empty then the filter is expected to respond\nwith a success status and an empty list.\n------------------------\npacket: git< status=success\\n\npacket: git< 0000\npacket: git< 0000 # empty content!\npacket: git< 0000 # empty list!\n------------------------\n\nIn case the filter cannot or does not want to process the content,\nit is expected to respond with an \"error\" status. Depending on the\n`filter..required` flag Git will interpret that as error\nbut it will not stop or restart the filter process.\n------------------------\npacket: git< status=error\\n\npacket: git< 0000\n------------------------\n\nIn case the filter cannot or does not want to process the content\nas well as any future content for the lifetime of the Git process,\nit is expected to respond with an \"error-all\" status. Depending on\nthe `filter..required` flag Git will interpret that as error\nbut it will not stop or restart the filter process.\n------------------------\npacket: git< status=error-all\\n\npacket: git< 0000\n------------------------\n\nIf the filter experiences an error during processing, then it can\nsend the status \"error\". Depending on the `filter..required`\nflag Git will interpret that as error but it will not stop or restart\nthe filter process.\n------------------------\npacket: git< status=success\\n\npacket: git< 0000\npacket: git< HALF_WRITTEN_ERRONEOUS_CONTENT\npacket: git< 0000\npacket: git< status=error\\n\npacket: git< 0000\n------------------------\n\nIf the filter dies during the communication or does not adhere to\nthe protocol then Git will stop the filter process and restart it\nwith the next file that needs to be processed.\n\nAfter the filter has processed a blob it is expected to wait for\nthe next \"key=value\" list containing a command. When the Git process\nterminates, it will send a kill signal to the filter in that stage.\n\nIf a `filter..clean` or `filter..smudge` command\nis configured then these commands always take precedence over\na configured `filter..process` command.\n\nHelped-by: Martin-Louis Bright \nReviewed-by: Jakub Narebski \nSigned-off-by: Lars Schneider \n---\n Documentation/gitattributes.txt | 139 ++++++++++++++-\n convert.c | 343 ++++++++++++++++++++++++++++++++----\n t/t0021-conversion.sh | 372 ++++++++++++++++++++++++++++++++++++++++\n t/t0021/rot13-filter.pl | 176 +++++++++++++++++++\n unpack-trees.c | 1 +\n 5 files changed, 1001 insertions(+), 30 deletions(-)\n create mode 100755 t/t0021/rot13-filter.pl\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 8882a3e..6e563a6 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -300,7 +300,13 @@ checkout, when the `smudge` command is specified, the command is\n fed the blob object from its standard input, and its standard\n output is used to update the worktree file. Similarly, the\n `clean` command is used to convert the contents of worktree file\n-upon checkin.\n+upon checkin. By default these commands process only a single\n+blob and terminate. If a long running `process` filter is used\n+in place of `clean` and/or `smudge` filters, then Git can process\n+all blobs with a single filter command invocation for the entire\n+life of a single Git command, for example `git add --all`. See\n+section below for the description of the protocol used to\n+communicate with a `process` filter.\n \n One use of the content filtering is to massage the content into a shape\n that is more convenient for the platform, filesystem, and the user to use.\n@@ -375,6 +381,137 @@ substitution. For example:\n ------------------------\n \n \n+Long Running Filter Process\n+^^^^^^^^^^^^^^^^^^^^^^^^^^^\n+\n+If the filter command (a string value) is defined via\n+`filter..process` then Git can process all blobs with a\n+single filter invocation for the entire life of a single Git\n+command. This is achieved by using the following packet format\n+(pkt-line, see technical/protocol-common.txt) based protocol over\n+standard input and standard output.\n+\n+Git starts the filter when it encounters the first file\n+that needs to be cleaned or smudged. After the filter started\n+Git sends a welcome message, a list of supported protocol\n+version numbers, and a flush packet. Git expects to read the\n+welcome message and one protocol version number from the\n+previously sent list. Afterwards Git sends a list of supported\n+capabilities and a flush packet. Git expects to read a list of\n+desired capabilities, which must be a subset of the supported\n+capabilities list, and a flush packet as response:\n+------------------------\n+packet: git> git-filter-client\n+packet: git> version=2\n+packet: git> version=42\n+packet: git> 0000\n+packet: git< git-filter-server\n+packet: git< version=2\n+packet: git> clean=true\n+packet: git> smudge=true\n+packet: git> not-yet-invented=true\n+packet: git> 0000\n+packet: git< clean=true\n+packet: git< smudge=true\n+packet: git< 0000\n+------------------------\n+Supported filter capabilities in version 2 are \"clean\" and\n+\"smudge\".\n+\n+Afterwards Git sends a list of \"key=value\" pairs terminated with\n+a flush packet. The list will contain at least the filter command\n+(based on the supported capabilities) and the pathname of the file\n+to filter relative to the repository root. Right after these packets\n+Git sends the content split in zero or more pkt-line packets and a\n+flush packet to terminate content.\n+------------------------\n+packet: git> command=smudge\\n\n+packet: git> pathname=path/testfile.dat\\n\n+packet: git> 0000\n+packet: git> CONTENT\n+packet: git> 0000\n+------------------------\n+\n+The filter is expected to respond with a list of \"key=value\" pairs\n+terminated with a flush packet. If the filter does not experience\n+problems then the list must contain a \"success\" status. Right after\n+these packets the filter is expected to send the content in zero\n+or more pkt-line packets and a flush packet at the end. Finally, a\n+second list of \"key=value\" pairs terminated with a flush packet\n+is expected. The filter can change the status in the second list.\n+------------------------\n+packet: git< status=success\\n\n+packet: git< 0000\n+packet: git< SMUDGED_CONTENT\n+packet: git< 0000\n+packet: git< 0000 # empty list!\n+------------------------\n+\n+If the result content is empty then the filter is expected to respond\n+with a success status and an empty list.\n+------------------------\n+packet: git< status=success\\n\n+packet: git< 0000\n+packet: git< 0000 # empty content!\n+packet: git< 0000 # empty list!\n+------------------------\n+\n+In case the filter cannot or does not want to process the content,\n+it is expected to respond with an \"error\" status. Depending on the\n+`filter..required` flag Git will interpret that as error\n+but it will not stop or restart the filter process.\n+------------------------\n+packet: git< status=error\\n\n+packet: git< 0000\n+------------------------\n+\n+In case the filter cannot or does not want to process the content\n+as well as any future content for the lifetime of the Git process,\n+it is expected to respond with an \"error-all\" status. Depending on\n+the `filter..required` flag Git will interpret that as error\n+but it will not stop or restart the filter process.\n+------------------------\n+packet: git< status=error-all\\n\n+packet: git< 0000\n+------------------------\n+\n+If the filter experiences an error during processing, then it can\n+send the status \"error\". Depending on the `filter..required`\n+flag Git will interpret that as error but it will not stop or restart\n+the filter process.\n+------------------------\n+packet: git< status=success\\n\n+packet: git< 0000\n+packet: git< HALF_WRITTEN_ERRONEOUS_CONTENT\n+packet: git< 0000\n+packet: git< status=error\\n\n+packet: git< 0000\n+------------------------\n+\n+If the filter dies during the communication or does not adhere to\n+the protocol then Git will stop the filter process and restart it\n+with the next file that needs to be processed.\n+\n+After the filter has processed a blob it is expected to wait for\n+the next \"key=value\" list containing a command. When the Git process\n+terminates, it will send a kill signal to the filter in that stage.\n+\n+A long running filter demo implementation can be found in\n+`t/t0021/rot13-filter.pl` located in the Git core repository.\n+If you develop your own long running filter process then the\n+`GIT_TRACE_PACKET` environment variables can be very helpful\n+for debugging (see linkgit:git[1]).\n+\n+If a `filter..clean` or `filter..smudge` command\n+is configured then these commands always take precedence over\n+a configured `filter..process` command.\n+\n+Please note that you cannot use an existing `filter..clean`\n+or `filter..smudge` command with `filter..process`\n+because the former two use a different inter process communication\n+protocol than the latter one.\n+\n+\n Interaction between checkin/checkout attributes\n ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n \ndiff --git a/convert.c b/convert.c\nindex bd17340..e421f4a 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -3,6 +3,7 @@\n #include \"run-command.h\"\n #include \"quote.h\"\n #include \"sigchain.h\"\n+#include \"pkt-line.h\"\n \n /*\n * convert.c - convert a file when checking it out and checking it in.\n@@ -427,7 +428,7 @@ static int filter_buffer_or_fd(int in, int out, void *data)\n \treturn (write_err || status);\n }\n \n-static int apply_filter(const char *path, const char *src, size_t len, int fd,\n+static int apply_single_file_filter(const char *path, const char *src, size_t len, int fd,\n struct strbuf *dst, const char *cmd)\n {\n \t/*\n@@ -441,12 +442,6 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,\n \tstruct async async;\n \tstruct filter_params params;\n \n-\tif (!cmd || !*cmd)\n-\t\treturn 0;\n-\n-\tif (!dst)\n-\t\treturn 1;\n-\n \tmemset(&async, 0, sizeof(async));\n \tasync.proc = filter_buffer_or_fd;\n \tasync.data = ¶ms;\n@@ -481,14 +476,311 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,\n \treturn !err;\n }\n \n+#define CAP_CLEAN (1u<<0)\n+#define CAP_SMUDGE (1u<<1)\n+\n+struct cmd2process {\n+\tstruct hashmap_entry ent; /* must be the first member! */\n+\tint supported_capabilities;\n+\tconst char *cmd;\n+\tstruct child_process process;\n+};\n+\n+static int cmd_process_map_initialized;\n+static struct hashmap cmd_process_map;\n+\n+static int cmd2process_cmp(const struct cmd2process *e1,\n+ const struct cmd2process *e2,\n+ const void *unused)\n+{\n+\treturn strcmp(e1->cmd, e2->cmd);\n+}\n+\n+static struct cmd2process *find_multi_file_filter_entry(struct hashmap *hashmap, const char *cmd)\n+{\n+\tstruct cmd2process key;\n+\thashmap_entry_init(&key, strhash(cmd));\n+\tkey.cmd = cmd;\n+\treturn hashmap_get(hashmap, &key, NULL);\n+}\n+\n+static void kill_multi_file_filter(struct hashmap *hashmap, struct cmd2process *entry)\n+{\n+\tif (!entry)\n+\t\treturn;\n+\tsigchain_push(SIGPIPE, SIG_IGN);\n+\t/*\n+\t * We kill the filter most likely because an error happened already.\n+\t * That's why we are not interested in any error code here.\n+\t */\n+\tclose(entry->process.in);\n+\tclose(entry->process.out);\n+\tsigchain_pop(SIGPIPE);\n+\tfinish_command(&entry->process);\n+\thashmap_remove(hashmap, entry, NULL);\n+\tfree(entry);\n+}\n+\n+static int packet_write_list(int fd, const char *line, ...)\n+{\n+\tva_list args;\n+\tint err;\n+\tva_start(args, line);\n+\tfor (;;)\n+\t{\n+\t\tif (!line)\n+\t\t\tbreak;\n+\t\terr = packet_write_gently_fmt(fd, \"%s\", 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 struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, const char *cmd)\n+{\n+\tint err;\n+\tstruct cmd2process *entry;\n+\tstruct child_process *process;\n+\tconst char *argv[] = { cmd, NULL };\n+\tstruct string_list cap_list = STRING_LIST_INIT_NODUP;\n+\tchar *cap_buf;\n+\tconst char *cap_name;\n+\n+\tentry = xmalloc(sizeof(*entry));\n+\thashmap_entry_init(entry, strhash(cmd));\n+\tentry->cmd = cmd;\n+\tentry->supported_capabilities = 0;\n+\tprocess = &entry->process;\n+\n+\tchild_process_init(process);\n+\tprocess->argv = argv;\n+\tprocess->use_shell = 1;\n+\tprocess->in = -1;\n+\tprocess->out = -1;\n+\n+\tif (start_command(process)) {\n+\t\terror(\"cannot fork to run external filter '%s'\", cmd);\n+\t\tkill_multi_file_filter(hashmap, entry);\n+\t\treturn NULL;\n+\t}\n+\n+\tsigchain_push(SIGPIPE, SIG_IGN);\n+\n+\terr = packet_write_list(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 long running filter protocol\", 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+\n+\terr = packet_write_list(process->in, \"clean=true\", \"smudge=true\", NULL);\n+\n+\tfor (;;)\n+\t{\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[1].string, \"true\"))\n+\t\t\tcontinue;\n+\n+\t\tcap_name = cap_list.items[0].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+\tif (err || errno == EPIPE) {\n+\t\terror(\"initialization for external filter '%s' failed\", cmd);\n+\t\tkill_multi_file_filter(hashmap, entry);\n+\t\treturn NULL;\n+\t}\n+\n+\thashmap_add(hashmap, entry);\n+\treturn entry;\n+}\n+\n+static void read_multi_file_filter_values(int fd, struct strbuf *status) {\n+\tstruct strbuf **pair;\n+\tchar *line;\n+\tfor (;;) {\n+\t\tline = packet_read_line(fd, NULL);\n+\t\tif (!line)\n+\t\t\tbreak;\n+\t\tpair = strbuf_split_str(line, '=', 2);\n+\t\tif (pair[0] && pair[0]->len && pair[1]) {\n+\t\t\tif (!strcmp(pair[0]->buf, \"status=\")) {\n+\t\t\t\tstrbuf_reset(status);\n+\t\t\t\tstrbuf_addbuf(status, pair[1]);\n+\t\t\t}\n+\t\t}\n+\t}\n+}\n+\n+static int apply_multi_file_filter(const char *path, const char *src, size_t len,\n+ int fd, struct strbuf *dst, const char *cmd,\n+ const int wanted_capability)\n+{\n+\tint err;\n+\tstruct cmd2process *entry;\n+\tstruct child_process *process;\n+\tstruct stat file_stat;\n+\tstruct strbuf nbuf = STRBUF_INIT;\n+\tstruct strbuf filter_status = STRBUF_INIT;\n+\tchar *filter_type;\n+\n+\tif (!cmd_process_map_initialized) {\n+\t\tcmd_process_map_initialized = 1;\n+\t\thashmap_init(&cmd_process_map, (hashmap_cmp_fn) cmd2process_cmp, 0);\n+\t\tentry = NULL;\n+\t} else {\n+\t\tentry = find_multi_file_filter_entry(&cmd_process_map, cmd);\n+\t}\n+\n+\tfflush(NULL);\n+\n+\tif (!entry) {\n+\t\tentry = start_multi_file_filter(&cmd_process_map, cmd);\n+\t\tif (!entry)\n+\t\t\treturn 0;\n+\t}\n+\tprocess = &entry->process;\n+\n+\tif (!(wanted_capability & entry->supported_capabilities))\n+\t\treturn 0;\n+\n+\tif (CAP_CLEAN & wanted_capability)\n+\t\tfilter_type = \"clean\";\n+\telse if (CAP_SMUDGE & wanted_capability)\n+\t\tfilter_type = \"smudge\";\n+\telse\n+\t\tdie(\"unexpected filter type\");\n+\n+\tif (fd >= 0 && !src) {\n+\t\tif (fstat(fd, &file_stat) == -1)\n+\t\t\treturn 0;\n+\t\tlen = xsize_t(file_stat.st_size);\n+\t}\n+\n+\tsigchain_push(SIGPIPE, SIG_IGN);\n+\n+\terr = packet_write_gently_fmt(process->in, \"command=%s\\n\", filter_type);\n+\tif (err)\n+\t\tgoto done;\n+\n+\terr = packet_write_gently_fmt(process->in, \"pathname=%s\\n\", path);\n+\tif (err)\n+\t\tgoto done;\n+\n+\terr = packet_flush_gently(process->in);\n+\tif (err)\n+\t\tgoto done;\n+\n+\tif (fd >= 0)\n+\t\terr = packet_write_stream_with_flush_from_fd(fd, process->in);\n+\telse\n+\t\terr = packet_write_stream_with_flush_from_buf(src, len, process->in);\n+\tif (err)\n+\t\tgoto done;\n+\n+\tread_multi_file_filter_values(process->out, &filter_status);\n+\terr = strcmp(filter_status.buf, \"success\");\n+\tif (err)\n+\t\tgoto done;\n+\n+\terr = packet_read_till_flush(process->out, &nbuf) < 0;\n+\tif (err)\n+\t\tgoto done;\n+\n+\tread_multi_file_filter_values(process->out, &filter_status);\n+\terr = strcmp(filter_status.buf, \"success\");\n+\n+done:\n+\tsigchain_pop(SIGPIPE);\n+\n+\tif (err || errno == EPIPE) {\n+\t\tif (!strcmp(filter_status.buf, \"error\")) {\n+\t\t\t/*\n+\t\t * The filter signaled a problem with the file.\n+\t\t */\n+\t\t} else if (!strcmp(filter_status.buf, \"error-all\")) {\n+\t\t\t/*\n+\t\t\t * The filter signaled a permanent problem. Don't try to filter\n+\t\t\t * files with the same command for the lifetime of the current\n+\t\t\t * Git process.\n+\t\t\t */\n+\t\t\t entry->supported_capabilities &= ~wanted_capability;\n+\t\t} else {\n+\t\t\t/*\n+\t\t\t * Something went wrong with the protocol filter.\n+\t\t\t * Force shutdown and restart if another blob requires filtering!\n+\t\t\t */\n+\t\t\terror(\"external filter '%s' failed\", cmd);\n+\t\t\tkill_multi_file_filter(&cmd_process_map, entry);\n+\t\t}\n+\t} else {\n+\t\tstrbuf_swap(dst, &nbuf);\n+\t}\n+\tstrbuf_release(&nbuf);\n+\treturn !err;\n+}\n+\n static struct convert_driver {\n \tconst char *name;\n \tstruct convert_driver *next;\n \tconst char *smudge;\n \tconst char *clean;\n+\tconst char *process;\n \tint required;\n } *user_convert, **user_convert_tail;\n \n+static int apply_filter(const char *path, const char *src, size_t len,\n+ int fd, struct strbuf *dst, struct convert_driver *drv,\n+ const int wanted_capability)\n+{\n+\tconst char* cmd = NULL;\n+\n+\tif (!drv)\n+\t\treturn 0;\n+\n+\tif (!dst)\n+\t\treturn 1;\n+\n+\tif ((CAP_CLEAN & wanted_capability) && drv->clean)\n+\t\tcmd = drv->clean;\n+\telse if ((CAP_SMUDGE & wanted_capability) && drv->smudge)\n+\t\tcmd = drv->smudge;\n+\n+\tif (cmd && *cmd)\n+\t\treturn apply_single_file_filter(path, src, len, fd, dst, cmd);\n+\telse if (drv->process && *drv->process)\n+\t\treturn apply_multi_file_filter(path, src, len, fd, dst, drv->process, wanted_capability);\n+\n+\treturn 0;\n+}\n+\n static int read_convert_config(const char *var, const char *value, void *cb)\n {\n \tconst char *key, *name;\n@@ -526,6 +818,10 @@ static int read_convert_config(const char *var, const char *value, void *cb)\n \tif (!strcmp(\"clean\", key))\n \t\treturn git_config_string(&drv->clean, var, value);\n \n+\tif (!strcmp(\"process\", key)) {\n+\t\treturn git_config_string(&drv->process, var, value);\n+\t}\n+\n \tif (!strcmp(\"required\", key)) {\n \t\tdrv->required = git_config_bool(var, value);\n \t\treturn 0;\n@@ -823,7 +1119,7 @@ int would_convert_to_git_filter_fd(const char *path)\n \tif (!ca.drv->required)\n \t\treturn 0;\n \n-\treturn apply_filter(path, NULL, 0, -1, NULL, ca.drv->clean);\n+\treturn apply_filter(path, NULL, 0, -1, NULL, ca.drv, CAP_CLEAN);\n }\n \n const char *get_convert_attr_ascii(const char *path)\n@@ -856,18 +1152,12 @@ int convert_to_git(const char *path, const char *src, size_t len,\n struct strbuf *dst, enum safe_crlf checksafe)\n {\n \tint ret = 0;\n-\tconst char *filter = NULL;\n-\tint required = 0;\n \tstruct conv_attrs ca;\n \n \tconvert_attrs(&ca, path);\n-\tif (ca.drv) {\n-\t\tfilter = ca.drv->clean;\n-\t\trequired = ca.drv->required;\n-\t}\n \n-\tret |= apply_filter(path, src, len, -1, dst, filter);\n-\tif (!ret && required)\n+\tret |= apply_filter(path, src, len, -1, dst, ca.drv, CAP_CLEAN);\n+\tif (!ret && ca.drv && ca.drv->required)\n \t\tdie(\"%s: clean filter '%s' failed\", path, ca.drv->name);\n \n \tif (ret && dst) {\n@@ -889,9 +1179,9 @@ void convert_to_git_filter_fd(const char *path, int fd, struct strbuf *dst,\n \tconvert_attrs(&ca, path);\n \n \tassert(ca.drv);\n-\tassert(ca.drv->clean);\n+\tassert(ca.drv->clean || ca.drv->process);\n \n-\tif (!apply_filter(path, NULL, 0, fd, dst, ca.drv->clean))\n+\tif (!apply_filter(path, NULL, 0, fd, dst, ca.drv, CAP_CLEAN))\n \t\tdie(\"%s: clean filter '%s' failed\", path, ca.drv->name);\n \n \tcrlf_to_git(path, dst->buf, dst->len, dst, ca.crlf_action, checksafe);\n@@ -903,15 +1193,9 @@ static int convert_to_working_tree_internal(const char *path, const char *src,\n \t\t\t\t\t int normalizing)\n {\n \tint ret = 0, ret_filter = 0;\n-\tconst char *filter = NULL;\n-\tint required = 0;\n \tstruct conv_attrs ca;\n \n \tconvert_attrs(&ca, path);\n-\tif (ca.drv) {\n-\t\tfilter = ca.drv->smudge;\n-\t\trequired = ca.drv->required;\n-\t}\n \n \tret |= ident_to_worktree(path, src, len, dst, ca.ident);\n \tif (ret) {\n@@ -920,9 +1204,10 @@ static int convert_to_working_tree_internal(const char *path, const char *src,\n \t}\n \t/*\n \t * CRLF conversion can be skipped if normalizing, unless there\n-\t * is a smudge filter. The filter might expect CRLFs.\n+\t * is a smudge or process filter (even if the process filter doesn't\n+\t * support smudge). The filters might expect CRLFs.\n \t */\n-\tif (filter || !normalizing) {\n+\tif ((ca.drv && (ca.drv->smudge || ca.drv->process)) || !normalizing) {\n \t\tret |= crlf_to_worktree(path, src, len, dst, ca.crlf_action);\n \t\tif (ret) {\n \t\t\tsrc = dst->buf;\n@@ -930,8 +1215,8 @@ static int convert_to_working_tree_internal(const char *path, const char *src,\n \t\t}\n \t}\n \n-\tret_filter = apply_filter(path, src, len, -1, dst, filter);\n-\tif (!ret_filter && required)\n+\tret_filter = apply_filter(path, src, len, -1, dst, ca.drv, CAP_SMUDGE);\n+\tif (!ret_filter && ca.drv && ca.drv->required)\n \t\tdie(\"%s: smudge filter %s failed\", path, ca.drv->name);\n \n \treturn ret | ret_filter;\n@@ -1383,7 +1668,7 @@ struct stream_filter *get_stream_filter(const char *path, const unsigned char *s\n \tstruct stream_filter *filter = NULL;\n \n \tconvert_attrs(&ca, path);\n-\tif (ca.drv && (ca.drv->smudge || ca.drv->clean))\n+\tif (ca.drv && (ca.drv->process || ca.drv->smudge || ca.drv->clean))\n \t\treturn NULL;\n \n \tif (ca.crlf_action == CRLF_AUTO || ca.crlf_action == CRLF_AUTO_CRLF)\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex 34c8eb9..5ba694b 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -42,6 +42,9 @@ test_expect_success setup '\n \trm -f test test.t test.i &&\n \tgit checkout -- test test.t test.i &&\n \n+\techo \"content-test2\" >test2.o &&\n+\techo \"content-test3-subdir\" >test3-subdir.o &&\n+\n \tmkdir generated-test-data &&\n \tfor i in $(test_seq 1 $T0021_LARGE_FILE_SIZE)\n \tdo\n@@ -296,4 +299,373 @@ test_expect_success 'disable filter with empty override' '\n \ttest_must_be_empty err\n '\n \n+check_filter () {\n+\trm -f rot13-filter.log actual.log &&\n+\t\"$@\" 2> git_stderr.log &&\n+\ttest_must_be_empty git_stderr.log &&\n+\tcat >expected.log &&\n+\tsort rot13-filter.log | uniq -c | sed \"s/^[ ]*//\" >actual.log &&\n+\ttest_cmp expected.log actual.log\n+}\n+\n+check_filter_count_clean () {\n+\trm -f rot13-filter.log actual.log &&\n+\t\"$@\" 2> git_stderr.log &&\n+\ttest_must_be_empty git_stderr.log &&\n+\tcat >expected.log &&\n+\tsort rot13-filter.log | uniq -c | sed \"s/^[ ]*//\" |\n+\t\tsed \"s/^\\([0-9]\\) IN: clean/x IN: clean/\" >actual.log &&\n+\ttest_cmp expected.log actual.log\n+}\n+\n+check_filter_ignore_clean () {\n+\trm -f rot13-filter.log actual.log &&\n+\t\"$@\" &&\n+\tcat >expected.log &&\n+\tgrep -v \"IN: clean\" rot13-filter.log >actual.log &&\n+\ttest_cmp expected.log actual.log\n+}\n+\n+check_filter_no_call () {\n+\trm -f rot13-filter.log &&\n+\t\"$@\" 2> git_stderr.log &&\n+\ttest_must_be_empty git_stderr.log &&\n+\ttest_must_be_empty rot13-filter.log\n+}\n+\n+check_rot13 () {\n+\ttest_cmp $1 $2 &&\n+\t./../rot13.sh <$1 >expected &&\n+\tgit cat-file blob :$2 >actual &&\n+\ttest_cmp expected actual\n+}\n+\n+test_expect_success PERL 'required process filter should filter data' '\n+\ttest_config_global filter.protocol.process \"$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge\" &&\n+\ttest_config_global filter.protocol.required true &&\n+\trm -rf repo &&\n+\tmkdir repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit init &&\n+\n+\t\techo \"*.r filter=protocol\" >.gitattributes &&\n+\t\tgit add . &&\n+\t\tgit commit . -m \"test commit\" &&\n+\t\tgit branch empty &&\n+\n+\t\tcp ../test.o test.r &&\n+\t\tcp ../test2.o test2.r &&\n+\t\tmkdir testsubdir &&\n+\t\tcp ../test3-subdir.o testsubdir/test3-subdir.r &&\n+\t\t>test4-empty.r &&\n+\n+\t\tcheck_filter \\\n+\t\t\tgit add . \\\n+\t\t\t\t<<-\\EOF &&\n+\t\t\t\t\t1 IN: clean test.r 57 [OK] -- OUT: 57 [OK]\n+\t\t\t\t\t1 IN: clean test2.r 14 [OK] -- OUT: 14 [OK]\n+\t\t\t\t\t1 IN: clean test4-empty.r 0 [OK] -- OUT: 0 [OK]\n+\t\t\t\t\t1 IN: clean testsubdir/test3-subdir.r 21 [OK] -- OUT: 21 [OK]\n+\t\t\t\t\t1 start\n+\t\t\t\t\t1 wrote filter header\n+\t\t\t\tEOF\n+\n+\t\tcheck_filter_count_clean \\\n+\t\t\tgit commit . -m \"test commit\" \\\n+\t\t\t\t<<-\\EOF &&\n+\t\t\t\t\tx IN: clean test.r 57 [OK] -- OUT: 57 [OK]\n+\t\t\t\t\tx IN: clean test2.r 14 [OK] -- OUT: 14 [OK]\n+\t\t\t\t\tx IN: clean test4-empty.r 0 [OK] -- OUT: 0 [OK]\n+\t\t\t\t\tx IN: clean testsubdir/test3-subdir.r 21 [OK] -- OUT: 21 [OK]\n+\t\t\t\t\t1 start\n+\t\t\t\t\t1 wrote filter header\n+\t\t\t\tEOF\n+\n+\t\trm -f test?.r testsubdir/test3-subdir.r &&\n+\n+\t\tcheck_filter_ignore_clean \\\n+\t\t\tgit checkout . \\\n+\t\t\t\t<<-\\EOF &&\n+\t\t\t\t\tstart\n+\t\t\t\t\twrote filter header\n+\t\t\t\t\tIN: smudge test2.r 14 [OK] -- OUT: 14 [OK]\n+\t\t\t\t\tIN: smudge testsubdir/test3-subdir.r 21 [OK] -- OUT: 21 [OK]\n+\t\t\t\tEOF\n+\n+\t\tcheck_filter_ignore_clean \\\n+\t\t\tgit checkout empty \\\n+\t\t\t\t<<-\\EOF &&\n+\t\t\t\t\tstart\n+\t\t\t\t\twrote filter header\n+\t\t\t\tEOF\n+\n+\t\tcheck_filter_ignore_clean \\\n+\t\t\tgit checkout master \\\n+\t\t\t\t<<-\\EOF &&\n+\t\t\t\t\tstart\n+\t\t\t\t\twrote filter header\n+\t\t\t\t\tIN: smudge test.r 57 [OK] -- OUT: 57 [OK]\n+\t\t\t\t\tIN: smudge test2.r 14 [OK] -- OUT: 14 [OK]\n+\t\t\t\t\tIN: smudge test4-empty.r 0 [OK] -- OUT: 0 [OK]\n+\t\t\t\t\tIN: smudge testsubdir/test3-subdir.r 21 [OK] -- OUT: 21 [OK]\n+\t\t\t\tEOF\n+\n+\t\tcheck_rot13 ../test.o test.r &&\n+\t\tcheck_rot13 ../test2.o test2.r &&\n+\t\tcheck_rot13 ../test3-subdir.o testsubdir/test3-subdir.r\n+\t)\n+'\n+\n+test_expect_success PERL 'required process filter should filter smudge data and one-shot filter should clean' '\n+\ttest_config_global filter.protocol.clean ./../rot13.sh &&\n+\ttest_config_global filter.protocol.process \"$TEST_DIRECTORY/t0021/rot13-filter.pl smudge\" &&\n+\ttest_config_global filter.protocol.required true &&\n+\trm -rf repo &&\n+\tmkdir repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit init &&\n+\n+\t\techo \"*.r filter=protocol\" >.gitattributes &&\n+\t\tgit add . &&\n+\t\tgit commit . -m \"test commit\" &&\n+\t\tgit branch empty &&\n+\n+\t\tcp ../test.o test.r &&\n+\t\tcp ../test2.o test2.r &&\n+\n+\t\tcheck_filter_no_call \\\n+\t\t\tgit add . &&\n+\n+\t\tcheck_filter_no_call \\\n+\t\t\tgit commit . -m \"test commit\" &&\n+\n+\t\trm -f test?.r testsubdir/test3-subdir.r &&\n+\n+\t\tcheck_filter_ignore_clean \\\n+\t\t\tgit checkout . \\\n+\t\t\t\t<<-\\EOF &&\n+\t\t\t\t\tstart\n+\t\t\t\t\twrote filter header\n+\t\t\t\t\tIN: smudge test2.r 14 [OK] -- OUT: 14 [OK]\n+\t\t\t\tEOF\n+\n+\t\tgit checkout empty &&\n+\n+\t\tcheck_filter_ignore_clean \\\n+\t\t\tgit checkout master\\\n+\t\t\t\t<<-\\EOF &&\n+\t\t\t\t\tstart\n+\t\t\t\t\twrote filter header\n+\t\t\t\t\tIN: smudge test.r 57 [OK] -- OUT: 57 [OK]\n+\t\t\t\t\tIN: smudge test2.r 14 [OK] -- OUT: 14 [OK]\n+\t\t\t\tEOF\n+\n+\t\tcheck_rot13 ../test.o test.r &&\n+\t\tcheck_rot13 ../test2.o test2.r\n+\t)\n+'\n+\n+test_expect_success PERL 'required process filter should clean only' '\n+\ttest_config_global filter.protocol.process \"$TEST_DIRECTORY/t0021/rot13-filter.pl clean\" &&\n+\ttest_config_global filter.protocol.required true &&\n+\trm -rf repo &&\n+\tmkdir repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit init &&\n+\n+\t\techo \"*.r filter=protocol\" >.gitattributes &&\n+\t\tgit add . &&\n+\t\tgit commit . -m \"test commit\" &&\n+\t\tgit branch empty &&\n+\n+\t\tcp ../test.o test.r &&\n+\n+\t\tcheck_filter \\\n+\t\t\tgit add . \\\n+\t\t\t\t<<-\\EOF &&\n+\t\t\t\t\t1 IN: clean test.r 57 [OK] -- OUT: 57 [OK]\n+\t\t\t\t\t1 start\n+\t\t\t\t\t1 wrote filter header\n+\t\t\t\tEOF\n+\n+\t\tcheck_filter_count_clean \\\n+\t\t\tgit commit . -m \"test commit\" \\\n+\t\t\t\t<<-\\EOF\n+\t\t\t\t\tx IN: clean test.r 57 [OK] -- OUT: 57 [OK]\n+\t\t\t\t\t1 start\n+\t\t\t\t\t1 wrote filter header\n+\t\t\t\tEOF\n+\t)\n+'\n+\n+test_expect_success PERL 'required process filter should process binary files larger LARGE_PACKET_MAX' '\n+\ttest_config_global filter.protocol.process \"$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge\" &&\n+\ttest_config_global filter.protocol.required true &&\n+\trm -rf repo &&\n+\tmkdir repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit init &&\n+\n+\t\techo \"*.file filter=protocol\" >.gitattributes &&\n+\t\tcat ../generated-test-data/largish.file.rot13 >large.rot13 &&\n+\t\tcat ../generated-test-data/largish.file >large.file &&\n+\t\tcat large.file >large.original &&\n+\n+\t\tgit add large.file .gitattributes &&\n+\t\tgit commit . -m \"test commit\" &&\n+\n+\t\trm -f large.file &&\n+\t\tgit checkout -- large.file &&\n+\t\tgit cat-file blob :large.file >actual &&\n+\t\ttest_cmp large.rot13 actual\n+\t)\n+'\n+\n+test_expect_success PERL 'required process filter should with clean error should fail' '\n+\ttest_config_global filter.protocol.process \"$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge\" &&\n+\ttest_config_global filter.protocol.required true &&\n+\trm -rf repo &&\n+\tmkdir repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit init &&\n+\n+\t\techo \"*.r filter=protocol\" >.gitattributes &&\n+\n+\t\tcp ../test.o test.r &&\n+\t\techo \"this is going to fail\" >clean-write-fail.r &&\n+\t\techo \"content-test3-subdir\" >test3.r &&\n+\n+\t\t# Note: There are three clean paths in convert.c we just test one here.\n+\t\ttest_must_fail git add .\n+\t)\n+'\n+\n+test_expect_success PERL 'process filter should restart after unexpected write failure' '\n+\ttest_config_global filter.protocol.process \"$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge\" &&\n+\trm -rf repo &&\n+\tmkdir repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit init &&\n+\n+\t\techo \"*.r filter=protocol\" >.gitattributes &&\n+\n+\t\tcp ../test.o test.r &&\n+\t\tcp ../test2.o test2.r &&\n+\t\techo \"this is going to fail\" >smudge-write-fail.o &&\n+\t\tcat smudge-write-fail.o >smudge-write-fail.r &&\n+\t\tgit add . &&\n+\t\tgit commit . -m \"test commit\" &&\n+\t\trm -f *.r &&\n+\n+\t\tcheck_filter_ignore_clean \\\n+\t\t\tgit checkout . \\\n+\t\t\t\t<<-\\EOF &&\n+\t\t\t\t\tstart\n+\t\t\t\t\twrote filter header\n+\t\t\t\t\tIN: smudge smudge-write-fail.r 22 [OK] -- OUT: 22 [WRITE FAIL]\n+\t\t\t\t\tstart\n+\t\t\t\t\twrote filter header\n+\t\t\t\t\tIN: smudge test.r 57 [OK] -- OUT: 57 [OK]\n+\t\t\t\t\tIN: smudge test2.r 14 [OK] -- OUT: 14 [OK]\n+\t\t\t\tEOF\n+\n+\t\tcheck_rot13 ../test.o test.r &&\n+\t\tcheck_rot13 ../test2.o test2.r &&\n+\n+\t\t! test_cmp smudge-write-fail.o smudge-write-fail.r && # Smudge failed!\n+\t\t./../rot13.sh expected &&\n+\t\tgit cat-file blob :smudge-write-fail.r >actual &&\n+\t\ttest_cmp expected actual\t\t\t\t\t\t\t # Clean worked!\n+\t)\n+'\n+\n+test_expect_success PERL 'process filter should not restart in case of an error' '\n+\ttest_config_global filter.protocol.process \"$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge\" &&\n+\trm -rf repo &&\n+\tmkdir repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit init &&\n+\n+\t\techo \"*.r filter=protocol\" >.gitattributes &&\n+\n+\t\tcp ../test.o test.r &&\n+\t\tcp ../test2.o test2.r &&\n+\t\techo \"this will cause an error\" >error.o &&\n+\t\tcp error.o error.r &&\n+\t\tgit add . &&\n+\t\tgit commit . -m \"test commit\" &&\n+\t\trm -f *.r &&\n+\n+\t\tcheck_filter_ignore_clean \\\n+\t\t\tgit checkout . \\\n+\t\t\t\t<<-\\EOF &&\n+\t\t\t\t\tstart\n+\t\t\t\t\twrote filter header\n+\t\t\t\t\tIN: smudge error.r 25 [OK] -- OUT: 0 [ERROR]\n+\t\t\t\t\tIN: smudge test.r 57 [OK] -- OUT: 57 [OK]\n+\t\t\t\t\tIN: smudge test2.r 14 [OK] -- OUT: 14 [OK]\n+\t\t\t\tEOF\n+\n+\t\tcheck_rot13 ../test.o test.r &&\n+\t\tcheck_rot13 ../test2.o test2.r &&\n+\t\ttest_cmp error.o error.r\n+\t)\n+'\n+\n+test_expect_success PERL 'process filter should be able to signal an error for all future files' '\n+\ttest_config_global filter.protocol.process \"$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge\" &&\n+\trm -rf repo &&\n+\tmkdir repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit init &&\n+\n+\t\techo \"*.r filter=protocol\" >.gitattributes &&\n+\n+\t\tcp ../test.o test.r &&\n+\t\tcp ../test2.o test2.r &&\n+\t\techo \"error this blob and all future blobs\" >error-all.o &&\n+\t\tcp error-all.o error-all.r &&\n+\t\tgit add . &&\n+\t\tgit commit . -m \"test commit\" &&\n+\t\trm -f *.r &&\n+\n+\t\tcheck_filter_ignore_clean \\\n+\t\t\tgit checkout . \\\n+\t\t\t\t<<-\\EOF &&\n+\t\t\t\t\tstart\n+\t\t\t\t\twrote filter header\n+\t\t\t\t\tIN: smudge error-all.r 37 [OK] -- OUT: 0 [ERROR-ALL]\n+\t\t\t\tEOF\n+\n+\t\ttest_cmp ../test.o test.r &&\n+\t\ttest_cmp ../test2.o test2.r &&\n+\t\ttest_cmp error-all.o error-all.r\n+\t)\n+'\n+\n+test_expect_success PERL 'invalid process filter must fail (and not hang!)' '\n+\ttest_config_global filter.protocol.process cat &&\n+\ttest_config_global filter.protocol.required true &&\n+\trm -rf repo &&\n+\tmkdir repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit init &&\n+\n+\t\techo \"*.r filter=protocol\" >.gitattributes &&\n+\n+\t\tcp ../test.o test.r &&\n+\t\ttest_must_fail git add . 2> git_stderr.log &&\n+\t\tgrep \"not support long running filter protocol\" git_stderr.log\n+\t)\n+'\n+\n test_done\ndiff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl\nnew file mode 100755\nindex 0000000..0e1181d\n--- /dev/null\n+++ b/t/t0021/rot13-filter.pl\n@@ -0,0 +1,176 @@\n+#!/usr/bin/perl\n+#\n+# Example implementation for the Git filter protocol version 2\n+# See Documentation/gitattributes.txt, section \"Filter Protocol\"\n+#\n+# The script takes the list of supported protocol capabilities as\n+# arguments (\"clean\", \"smudge\", etc).\n+#\n+# This implementation supports special test cases:\n+# (1) If data with the pathname \"clean-write-fail.r\" is processed with\n+# a \"clean\" operation then the write operation will die.\n+# (2) If data with the pathname \"smudge-write-fail.r\" is processed with\n+# a \"smudge\" operation then the write operation will die.\n+# (3) If data with the pathname \"error.r\" is processed with any\n+# operation then the filter signals that it cannot or does not want\n+# to process the file.\n+# (4) If data with the pathname \"error-all.r\" is processed with any\n+# operation then the filter signals that it cannot or does not want\n+# to process the file and any file after that is processed with the\n+# same command.\n+#\n+\n+use strict;\n+use warnings;\n+\n+my $MAX_PACKET_CONTENT_SIZE = 65516;\n+my @capabilities = @ARGV;\n+\n+sub rot13 {\n+ my ($str) = @_;\n+ $str =~ y/A-Za-z/N-ZA-Mn-za-m/;\n+ return $str;\n+}\n+\n+sub packet_read {\n+ my $buffer;\n+ my $bytes_read = read STDIN, $buffer, 4;\n+ if ( $bytes_read == 0 ) {\n+\n+ # EOF - Git stopped talking to us!\n+ exit();\n+ }\n+ elsif ( $bytes_read != 4 ) {\n+ die \"invalid packet size '$bytes_read' field\";\n+ }\n+ my $pkt_size = hex($buffer);\n+ if ( $pkt_size == 0 ) {\n+ return ( 1, \"\" );\n+ }\n+ elsif ( $pkt_size > 4 ) {\n+ my $content_size = $pkt_size - 4;\n+ $bytes_read = read STDIN, $buffer, $content_size;\n+ if ( $bytes_read != $content_size ) {\n+ die \"invalid packet ($content_size expected; $bytes_read read)\";\n+ }\n+ return ( 0, $buffer );\n+ }\n+ else {\n+ die \"invalid packet size\";\n+ }\n+}\n+\n+sub packet_write {\n+ my ($packet) = @_;\n+ print STDOUT sprintf( \"%04x\", length($packet) + 4 );\n+ print STDOUT $packet;\n+ STDOUT->flush();\n+}\n+\n+sub packet_flush {\n+ print STDOUT sprintf( \"%04x\", 0 );\n+ STDOUT->flush();\n+}\n+\n+open my $debug, \">>\", \"rot13-filter.log\";\n+print $debug \"start\\n\";\n+$debug->flush();\n+\n+( packet_read() eq ( 0, \"git-filter-client\" ) ) || die \"bad initialization\";\n+( packet_read() eq ( 0, \"version=2\" ) ) || die \"bad version\";\n+( packet_read() eq ( 1, \"\" ) ) || die \"bad version end\";\n+\n+packet_write(\"git-filter-server\\n\");\n+packet_write(\"version=2\\n\");\n+\n+( packet_read() eq ( 0, \"clean=true\" ) ) || die \"bad capability\";\n+( packet_read() eq ( 0, \"smudge=true\" ) ) || die \"bad capability\";\n+( packet_read() eq ( 1, \"\" ) ) || die \"bad capability end\";\n+\n+foreach (@capabilities) {\n+ packet_write( $_ . \"=true\\n\" );\n+}\n+packet_flush();\n+print $debug \"wrote filter header\\n\";\n+$debug->flush();\n+\n+while (1) {\n+ my ($command) = packet_read() =~ /^command=([^=]+)\\n$/;\n+ print $debug \"IN: $command\";\n+ $debug->flush();\n+\n+ my ($pathname) = packet_read() =~ /^pathname=([^=]+)\\n$/;\n+ print $debug \" $pathname\";\n+ $debug->flush();\n+\n+ # Flush\n+ packet_read();\n+\n+ my $input = \"\";\n+ {\n+ binmode(STDIN);\n+ my $buffer;\n+ my $done = 0;\n+ while ( !$done ) {\n+ ( $done, $buffer ) = packet_read();\n+ $input .= $buffer;\n+ }\n+ print $debug \" \" . length($input) . \" [OK] -- \";\n+ $debug->flush();\n+ }\n+\n+ my $output;\n+ if ( $pathname eq \"error.r\" or $pathname eq \"error-all.r\" ) {\n+ $output = \"\";\n+ }\n+ elsif ( $command eq \"clean\" and grep( /^clean$/, @capabilities ) ) {\n+ $output = rot13($input);\n+ }\n+ elsif ( $command eq \"smudge\" and grep( /^smudge$/, @capabilities ) ) {\n+ $output = rot13($input);\n+ }\n+ else {\n+ die \"bad command '$command'\";\n+ }\n+\n+ print $debug \"OUT: \" . length($output) . \" \";\n+ $debug->flush();\n+\n+ if ( $pathname eq \"error.r\" ) {\n+ print $debug \"[ERROR]\\n\";\n+ $debug->flush();\n+ packet_write(\"status=error\\n\");\n+ packet_flush();\n+ }\n+ elsif ( $pathname eq \"error-all.r\" ) {\n+ print $debug \"[ERROR-ALL]\\n\";\n+ $debug->flush();\n+ packet_write(\"status=error-all\\n\");\n+ packet_flush();\n+ }\n+ else {\n+ packet_write(\"status=success\\n\");\n+ packet_flush();\n+\n+ if ( $pathname eq \"${command}-write-fail.r\" ) {\n+ print $debug \"[WRITE FAIL]\\n\";\n+ $debug->flush();\n+ die \"${command} write error\";\n+ }\n+\n+ while ( length($output) > 0 ) {\n+ my $packet = substr( $output, 0, $MAX_PACKET_CONTENT_SIZE );\n+ packet_write($packet);\n+ if ( length($output) > $MAX_PACKET_CONTENT_SIZE ) {\n+ $output = substr( $output, $MAX_PACKET_CONTENT_SIZE );\n+ }\n+ else {\n+ $output = \"\";\n+ }\n+ }\n+ packet_flush();\n+ print $debug \"[OK]\\n\";\n+ $debug->flush();\n+ packet_flush();\n+ }\n+}\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 11c37fb..f6798f8 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -10,6 +10,7 @@\n #include \"attr.h\"\n #include \"split-index.h\"\n #include \"dir.h\"\n+#include \"convert.h\"\n \n /*\n * Error messages expected by scripts out of plumbing commands such as\n-- \n2.9.2\n\n"},{"id":"293575","messageId":"20160810143452.siolzmqwgs3xzcal@sigill.intra.peff.net","threadId":"43044","inReplyTo":"982CD9A4-E155-4DEE-9B47-BCEA4F0C493C@gmail.com","subject":"Re: [PATCH v5 03/15] pkt-line: add `gentle` parameter to format_packet()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-10T14:34:53Z","receivedAt":"2016-08-10T18:35:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 10, 2016 at 03:59:19PM +0200, Lars Schneider wrote:\n\n> > It does still feel a little weird that you cannot tell the difference\n> > between a write() error and bad input. Because you really might want to\n> > do something different between the two. Like:\n> > \n> > #define MAX_FILENAME (PKTLINE_DATA_MAXLEN - strlen(\"filename\"))\n> > \n> > if (filename > MAX_FILENAME) {\n> > \twarning(\"woah, that name is ridiculous; truncating\");\n> > \tret = packet_write_fmt_gently(fd, \"%.*s\", MAX_FILENAME, filename);\n> > } else\n> > \tret = packet_write_fmt_gently(fd, \"%s\", filename);\n> \n> \n> I can do that. However, I wouldn't truncate the filename as this\n> might create a weird outcome. I would just let the filter fail.\n\nYeah, I think that is probably fine (I don't have a real opinion for\nthis particular case, but was mostly just trying to think about whether\nthe pktline interface was suitably flexible).\n\n-Peff\n"},{"id":"293577","messageId":"20160810143321.q7mjirgr5ynml5ff@sigill.intra.peff.net","threadId":"43044","inReplyTo":"9BF0767F-6B10-42D5-9A72-2710A753E26A@gmail.com","subject":"Re: [PATCH v5 02/15] pkt-line: call packet_trace() only if a packet is actually send","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-10T14:33:21Z","receivedAt":"2016-08-10T18:36:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 10, 2016 at 03:51:35PM +0200, Lars Schneider wrote:\n\n> I guess my point is that I stumbled over the un-intutiive format_packet() behavior\n> and I wanted to improve the situation in a way that others don't run into this\n> trap. If you think that is no issue then it would be OK for me if we leave the\n> current behavior as is.\n\nI don't think the behavior as-is is a problem, and it would remain OK as\nlong as no callers are added who format a packet but don't write it (or\ndon't write it in a timely manner).\n\nBut most importantly, if you are going to refactor code, you can't\nregress the existing callers. So even if we did want to change how this\nworked, this patch is not acceptable as-is; it would need to fix up all\nof the callers of packet_buf_write().\n\n-Peff\n"},{"id":"293578","messageId":"20160810131317.a4npld5hf2qk6wiq@sigill.intra.peff.net","threadId":"43044","inReplyTo":"20160810130411.12419-3-larsxschneider@gmail.com","subject":"Re: [PATCH v5 02/15] pkt-line: call packet_trace() only if a packet is actually send","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-10T13:13:17Z","receivedAt":"2016-08-10T18:40:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 10, 2016 at 03:03:58PM +0200, larsxschneider@gmail.com wrote:\n\n> From: Lars Schneider \n> \n> The packet_trace() call is not ideal in format_packet() as we would print\n> a trace when a packet is formatted and (potentially) when the packet is\n> actually send. This was no problem up until now because format_packet()\n> was only used by one function. Fix it by moving the trace call into the\n> function that actually sends the packet.\n\nIt looks like there are two functions: packet_write() and\npacket_buf_write().\n\nYour patch only touches one of them, and it looks like we would fail to\ntrace many packets (e.g., see receive-pack.c:report(), which uses\npacket_buf_write() and then write()s out the result).\n\n-Peff\n\nPS Also, s/send/sent/ in the commit message.\n"},{"id":"293581","messageId":"xmqqvaz84g9y.fsf@gitster.mtv.corp.google.com","threadId":"43044","inReplyTo":"2B2AC073-95AD-42E9-AD3A-23E8E13C66DE@gmail.com","subject":"Re: [PATCH v5 05/15] pkt-line: add packet_write_gently_fmt()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-10T18:42:33Z","receivedAt":"2016-08-10T18:42:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n>>> Similarly, I'd think this could share code with the non-gentle form\n>>> (which should be able to just call this and die() if returns an error).\n>>> Though sometimes the va_list transformation makes that awkward.\n>> \n>> Yes.\n>\n> Peff just posted that he tried the shared code idea but the result\n> ended up ugly.\n\nOK.\n"},{"id":"293582","messageId":"20160810131541.ovpvgwdxjibae5gy@sigill.intra.peff.net","threadId":"43044","inReplyTo":"20160810130411.12419-4-larsxschneider@gmail.com","subject":"Re: [PATCH v5 03/15] pkt-line: add `gentle` parameter to format_packet()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-10T13:15:41Z","receivedAt":"2016-08-10T18:43:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 10, 2016 at 03:03:59PM +0200, larsxschneider@gmail.com wrote:\n\n> From: Lars Schneider \n> \n> format_packet() dies if the caller wants to format a packet larger than\n> LARGE_PACKET_MAX. Certain callers might prefer an error response instead.\n\nI am not sure I agree here. Certainly I see the usefulness of gently\nhandling a failure to write(). But if you are passing in too-large\nbuffers, isn't that a bug in the program?\n\nHow would you recover, except by splitting up the content? That might\nnot be possible depending on how you are using the pkt-lines. And even\nif it is, wouldn't it be simpler to split it up before sending it to\nformat_packet()?\n\n-Peff\n"},{"id":"293584","messageId":"20160810130411.12419-9-larsxschneider@gmail.com","threadId":"43044","inReplyTo":"20160810130411.12419-1-larsxschneider@gmail.com","subject":"[PATCH v5 08/15] pkt-line: rename packet_write() to packet_write_fmt()","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-10T13:04:04Z","receivedAt":"2016-08-10T18:54:43Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\npacket_write() should be called packet_write_fmt() as the string\nparameter can be formatted.\n\nSuggested-by: Junio C Hamano \nSigned-off-by: Lars Schneider \n---\n builtin/archive.c | 4 ++--\n builtin/receive-pack.c | 4 ++--\n builtin/remote-ext.c | 4 ++--\n builtin/upload-archive.c | 4 ++--\n connect.c | 2 +-\n daemon.c | 2 +-\n http-backend.c | 2 +-\n pkt-line.c | 2 +-\n pkt-line.h | 2 +-\n shallow.c | 2 +-\n upload-pack.c | 30 +++++++++++++++---------------\n 11 files changed, 29 insertions(+), 29 deletions(-)\n\ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex a1e3b94..49f4914 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -47,10 +47,10 @@ static int run_remote_archiver(int argc, const char **argv,\n \tif (name_hint) {\n \t\tconst char *format = archive_format_from_filename(name_hint);\n \t\tif (format)\n-\t\t\tpacket_write(fd[1], \"argument --format=%s\\n\", format);\n+\t\t\tpacket_write_fmt(fd[1], \"argument --format=%s\\n\", format);\n \t}\n \tfor (i = 1; i < argc; i++)\n-\t\tpacket_write(fd[1], \"argument %s\\n\", argv[i]);\n+\t\tpacket_write_fmt(fd[1], \"argument %s\\n\", argv[i]);\n \tpacket_flush(fd[1]);\n \n \tbuf = packet_read_line(fd[0], NULL);\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 15c323a..d60a412 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -199,7 +199,7 @@ static int receive_pack_config(const char *var, const char *value, void *cb)\n static void show_ref(const char *path, const unsigned char *sha1)\n {\n \tif (sent_capabilities) {\n-\t\tpacket_write(1, \"%s %s\\n\", sha1_to_hex(sha1), path);\n+\t\tpacket_write_fmt(1, \"%s %s\\n\", sha1_to_hex(sha1), path);\n \t} else {\n \t\tstruct strbuf cap = STRBUF_INIT;\n \n@@ -212,7 +212,7 @@ static void show_ref(const char *path, const unsigned char *sha1)\n \t\tif (push_cert_nonce)\n \t\t\tstrbuf_addf(&cap, \" push-cert=%s\", push_cert_nonce);\n \t\tstrbuf_addf(&cap, \" agent=%s\", git_user_agent_sanitized());\n-\t\tpacket_write(1, \"%s %s%c%s\\n\",\n+\t\tpacket_write_fmt(1, \"%s %s%c%s\\n\",\n \t\t\t sha1_to_hex(sha1), path, 0, cap.buf);\n \t\tstrbuf_release(&cap);\n \t\tsent_capabilities = 1;\ndiff --git a/builtin/remote-ext.c b/builtin/remote-ext.c\nindex 88eb8f9..11b48bf 100644\n--- a/builtin/remote-ext.c\n+++ b/builtin/remote-ext.c\n@@ -128,9 +128,9 @@ static void send_git_request(int stdin_fd, const char *serv, const char *repo,\n \tconst char *vhost)\n {\n \tif (!vhost)\n-\t\tpacket_write(stdin_fd, \"%s %s%c\", serv, repo, 0);\n+\t\tpacket_write_fmt(stdin_fd, \"%s %s%c\", serv, repo, 0);\n \telse\n-\t\tpacket_write(stdin_fd, \"%s %s%chost=%s%c\", serv, repo, 0,\n+\t\tpacket_write_fmt(stdin_fd, \"%s %s%chost=%s%c\", serv, repo, 0,\n \t\t\t vhost, 0);\n }\n \ndiff --git a/builtin/upload-archive.c b/builtin/upload-archive.c\nindex 2caedf1..dc872f6 100644\n--- a/builtin/upload-archive.c\n+++ b/builtin/upload-archive.c\n@@ -88,11 +88,11 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)\n \twriter.git_cmd = 1;\n \tif (start_command(&writer)) {\n \t\tint err = errno;\n-\t\tpacket_write(1, \"NACK unable to spawn subprocess\\n\");\n+\t\tpacket_write_fmt(1, \"NACK unable to spawn subprocess\\n\");\n \t\tdie(\"upload-archive: %s\", strerror(err));\n \t}\n \n-\tpacket_write(1, \"ACK\\n\");\n+\tpacket_write_fmt(1, \"ACK\\n\");\n \tpacket_flush(1);\n \n \twhile (1) {\ndiff --git a/connect.c b/connect.c\nindex 722dc3f..5330d9c 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -730,7 +730,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t * Note: Do not add any other headers here! Doing so\n \t\t * will cause older git-daemon servers to crash.\n \t\t */\n-\t\tpacket_write(fd[1],\n+\t\tpacket_write_fmt(fd[1],\n \t\t\t \"%s %s%chost=%s%c\",\n \t\t\t prog, path, 0,\n \t\t\t target_host, 0);\ndiff --git a/daemon.c b/daemon.c\nindex e647254..2646d0f 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -281,7 +281,7 @@ static int daemon_error(const char *dir, const char *msg)\n {\n \tif (!informative_errors)\n \t\tmsg = \"access denied or repository not exported\";\n-\tpacket_write(1, \"ERR %s: %s\", msg, dir);\n+\tpacket_write_fmt(1, \"ERR %s: %s\", msg, dir);\n \treturn -1;\n }\n \ndiff --git a/http-backend.c b/http-backend.c\nindex 0d59499..aa61c18 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -460,7 +460,7 @@ static void get_info_refs(char *arg)\n \t\thdr_str(content_type, buf.buf);\n \t\tend_headers();\n \n-\t\tpacket_write(1, \"# service=git-%s\\n\", svc->name);\n+\t\tpacket_write_fmt(1, \"# service=git-%s\\n\", svc->name);\n \t\tpacket_flush(1);\n \n \t\targv[0] = svc->name;\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 4b526e1..b98e291 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -135,7 +135,7 @@ static int format_packet(int gentle, struct strbuf *out, const char *fmt, va_lis\n \treturn 0;\n }\n \n-void packet_write(int fd, const char *fmt, ...)\n+void packet_write_fmt(int fd, const char *fmt, ...)\n {\n \tstatic struct strbuf buf = STRBUF_INIT;\n \tva_list args;\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 89063ee..ddd6041 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -20,7 +20,7 @@\n * side can't, we stay with pure read/write interfaces.\n */\n void packet_flush(int fd);\n-void packet_write(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n+void packet_write_fmt(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n 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);\ndiff --git a/shallow.c b/shallow.c\nindex 4d554ca..1826114 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -260,7 +260,7 @@ static int advertise_shallow_grafts_cb(const struct commit_graft *graft, void *c\n {\n \tint fd = *(int *)cb;\n \tif (graft->nr_parent == -1)\n-\t\tpacket_write(fd, \"shallow %s\\n\", oid_to_hex(&graft->oid));\n+\t\tpacket_write_fmt(fd, \"shallow %s\\n\", oid_to_hex(&graft->oid));\n \treturn 0;\n }\n \ndiff --git a/upload-pack.c b/upload-pack.c\nindex d4cc414..8e77b00 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -393,13 +393,13 @@ static int get_common_commits(void)\n \t\t\tif (multi_ack == 2 && got_common\n \t\t\t && !got_other && ok_to_give_up()) {\n \t\t\t\tsent_ready = 1;\n-\t\t\t\tpacket_write(1, \"ACK %s ready\\n\", last_hex);\n+\t\t\t\tpacket_write_fmt(1, \"ACK %s ready\\n\", last_hex);\n \t\t\t}\n \t\t\tif (have_obj.nr == 0 || multi_ack)\n-\t\t\t\tpacket_write(1, \"NAK\\n\");\n+\t\t\t\tpacket_write_fmt(1, \"NAK\\n\");\n \n \t\t\tif (no_done && sent_ready) {\n-\t\t\t\tpacket_write(1, \"ACK %s\\n\", last_hex);\n+\t\t\t\tpacket_write_fmt(1, \"ACK %s\\n\", last_hex);\n \t\t\t\treturn 0;\n \t\t\t}\n \t\t\tif (stateless_rpc)\n@@ -416,20 +416,20 @@ static int get_common_commits(void)\n \t\t\t\t\tconst char *hex = sha1_to_hex(sha1);\n \t\t\t\t\tif (multi_ack == 2) {\n \t\t\t\t\t\tsent_ready = 1;\n-\t\t\t\t\t\tpacket_write(1, \"ACK %s ready\\n\", hex);\n+\t\t\t\t\t\tpacket_write_fmt(1, \"ACK %s ready\\n\", hex);\n \t\t\t\t\t} else\n-\t\t\t\t\t\tpacket_write(1, \"ACK %s continue\\n\", hex);\n+\t\t\t\t\t\tpacket_write_fmt(1, \"ACK %s continue\\n\", hex);\n \t\t\t\t}\n \t\t\t\tbreak;\n \t\t\tdefault:\n \t\t\t\tgot_common = 1;\n \t\t\t\tmemcpy(last_hex, sha1_to_hex(sha1), 41);\n \t\t\t\tif (multi_ack == 2)\n-\t\t\t\t\tpacket_write(1, \"ACK %s common\\n\", last_hex);\n+\t\t\t\t\tpacket_write_fmt(1, \"ACK %s common\\n\", last_hex);\n \t\t\t\telse if (multi_ack)\n-\t\t\t\t\tpacket_write(1, \"ACK %s continue\\n\", last_hex);\n+\t\t\t\t\tpacket_write_fmt(1, \"ACK %s continue\\n\", last_hex);\n \t\t\t\telse if (have_obj.nr == 1)\n-\t\t\t\t\tpacket_write(1, \"ACK %s\\n\", last_hex);\n+\t\t\t\t\tpacket_write_fmt(1, \"ACK %s\\n\", last_hex);\n \t\t\t\tbreak;\n \t\t\t}\n \t\t\tcontinue;\n@@ -437,10 +437,10 @@ static int get_common_commits(void)\n \t\tif (!strcmp(line, \"done\")) {\n \t\t\tif (have_obj.nr > 0) {\n \t\t\t\tif (multi_ack)\n-\t\t\t\t\tpacket_write(1, \"ACK %s\\n\", last_hex);\n+\t\t\t\t\tpacket_write_fmt(1, \"ACK %s\\n\", last_hex);\n \t\t\t\treturn 0;\n \t\t\t}\n-\t\t\tpacket_write(1, \"NAK\\n\");\n+\t\t\tpacket_write_fmt(1, \"NAK\\n\");\n \t\t\treturn -1;\n \t\t}\n \t\tdie(\"git upload-pack: expected SHA1 list, got '%s'\", line);\n@@ -650,7 +650,7 @@ static void receive_needs(void)\n \t\twhile (result) {\n \t\t\tstruct object *object = &result->item->object;\n \t\t\tif (!(object->flags & (CLIENT_SHALLOW|NOT_SHALLOW))) {\n-\t\t\t\tpacket_write(1, \"shallow %s\",\n+\t\t\t\tpacket_write_fmt(1, \"shallow %s\",\n \t\t\t\t\t\toid_to_hex(&object->oid));\n \t\t\t\tregister_shallow(object->oid.hash);\n \t\t\t\tshallow_nr++;\n@@ -662,7 +662,7 @@ static void receive_needs(void)\n \t\t\tstruct object *object = shallows.objects[i].item;\n \t\t\tif (object->flags & NOT_SHALLOW) {\n \t\t\t\tstruct commit_list *parents;\n-\t\t\t\tpacket_write(1, \"unshallow %s\",\n+\t\t\t\tpacket_write_fmt(1, \"unshallow %s\",\n \t\t\t\t\toid_to_hex(&object->oid));\n \t\t\t\tobject->flags &= ~CLIENT_SHALLOW;\n \t\t\t\t/* make sure the real parents are parsed */\n@@ -741,7 +741,7 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \t\tstruct strbuf symref_info = STRBUF_INIT;\n \n \t\tformat_symref_info(&symref_info, cb_data);\n-\t\tpacket_write(1, \"%s %s%c%s%s%s%s%s agent=%s\\n\",\n+\t\tpacket_write_fmt(1, \"%s %s%c%s%s%s%s%s agent=%s\\n\",\n \t\t\t oid_to_hex(oid), refname_nons,\n \t\t\t 0, capabilities,\n \t\t\t (allow_unadvertised_object_request & ALLOW_TIP_SHA1) ?\n@@ -753,11 +753,11 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \t\t\t git_user_agent_sanitized());\n \t\tstrbuf_release(&symref_info);\n \t} else {\n-\t\tpacket_write(1, \"%s %s\\n\", oid_to_hex(oid), refname_nons);\n+\t\tpacket_write_fmt(1, \"%s %s\\n\", oid_to_hex(oid), refname_nons);\n \t}\n \tcapabilities = NULL;\n \tif (!peel_ref(refname, peeled.hash))\n-\t\tpacket_write(1, \"%s %s^{}\\n\", oid_to_hex(&peeled), refname_nons);\n+\t\tpacket_write_fmt(1, \"%s %s^{}\\n\", oid_to_hex(&peeled), refname_nons);\n \treturn 0;\n }\n \n-- \n2.9.2\n\n"},{"id":"293586","messageId":"20160810133017.ovdx3a3ldqj5gl7q@sigill.intra.peff.net","threadId":"43044","inReplyTo":"4111F00D-74A9-449D-BDE9-EE170DA4A403@gmail.com","subject":"Re: [PATCH v5 02/15] pkt-line: call packet_trace() only if a packet is actually send","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-10T13:30:18Z","receivedAt":"2016-08-10T18:57:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 10, 2016 at 03:24:38PM +0200, Lars Schneider wrote:\n\n> > On Wed, Aug 10, 2016 at 03:03:58PM +0200, larsxschneider@gmail.com wrote:\n> > \n> >> From: Lars Schneider \n> >> \n> >> The packet_trace() call is not ideal in format_packet() as we would print\n> >> a trace when a packet is formatted and (potentially) when the packet is\n> >> actually send. This was no problem up until now because format_packet()\n> >> was only used by one function. Fix it by moving the trace call into the\n> >> function that actually sends the packet.\n> > \n> > It looks like there are two functions: packet_write() and\n> > packet_buf_write().\n> \n> I did not call trace in packet_buf_write() because this function does not\n> perform any writes.\n\nYes, but then who is responsible for the trace? The caller?\n\nAnd why is it a bad thing to do it some time other than writing? It is\nif you format and then _don't_ write the packet, but the current callers\nare not doing that.\n\n> > Your patch only touches one of them, and it looks like we would fail to\n> > trace many packets (e.g., see receive-pack.c:report(), which uses\n> > packet_buf_write() and then write()s out the result).\n> \n> I see. But isn't it confusing if packet_buf_write() issues a trace call?\n> If I just call this function then nothing happens at all. Shouldn't the\n> trace call be made in receive-pack.c:report() ? Or shouldn't receive-pack\n> let pkt-line.c perform the write calls?\n\nHow would report() do that without re-parsing each of the packets?\n\n-Peff\n"},{"id":"293587","messageId":"20160810130411.12419-6-larsxschneider@gmail.com","threadId":"43044","inReplyTo":"20160810130411.12419-1-larsxschneider@gmail.com","subject":"[PATCH v5 05/15] pkt-line: add packet_write_gently_fmt()","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-10T13:04:01Z","receivedAt":"2016-08-10T18:57:29Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\npacket_write() would die in case of a write error even though for some callers\nan error would be acceptable. Add packet_write_gently_fmt() which writes a\nformatted pkt-line and returns `0` for success and `-1` for an error.\n\nSigned-off-by: Lars Schneider \n---\n pkt-line.c | 13 +++++++++++++\n pkt-line.h | 1 +\n 2 files changed, 14 insertions(+)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 4f25748..a8207dd 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -142,6 +142,19 @@ void packet_write(int fd, const char *fmt, ...)\n \twrite_or_die(fd, buf.buf, buf.len);\n }\n \n+int packet_write_gently_fmt(int fd, const char *fmt, ...)\n+{\n+\tstatic struct strbuf buf = STRBUF_INIT;\n+\tva_list args;\n+\n+\tstrbuf_reset(&buf);\n+\tva_start(args, fmt);\n+\tformat_packet(1, &buf, fmt, args);\n+\tva_end(args);\n+\tpacket_trace(buf.buf + 4, buf.len - 4, 1);\n+\treturn (write_in_full(fd, buf.buf, buf.len) == buf.len ? 0 : -1);\n+}\n+\n int packet_write_gently(const int fd_out, const char *buf, size_t size)\n {\n \tif (size > PKTLINE_DATA_MAXLEN)\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 88584f1..82676f4 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -23,6 +23,7 @@ void packet_flush(int fd);\n void packet_write(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n 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_write_gently_fmt(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n \n /*\n * Read a packetized line into the buffer, which must be at least size bytes\n-- \n2.9.2\n\n"},{"id":"293590","messageId":"20160810130411.12419-16-larsxschneider@gmail.com","threadId":"43044","inReplyTo":"20160810130411.12419-1-larsxschneider@gmail.com","subject":"[PATCH v5 15/15] read-cache: make sure file handles are not inherited by child processes","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-10T13:04:11Z","receivedAt":"2016-08-10T19:00:24Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\nConsider the case of a file that requires filtering and is present in branch A\nbut not in branch B. If A is the current HEAD and we checkout B then the\nfollowing happens:\n\n1. ce_compare_data() opens the file\n2. index_fd() detects that the file requires to run a clean filter and\n calls index_stream_convert_blob()\n4. index_stream_convert_blob() calls convert_to_git_filter_fd()\n5. convert_to_git_filter_fd() calls apply_filter() which creates a new\n long running filter process (in case it is the first file of this kind\n to be filtered)\n6. The new filter process inherits all file handles. This is the default\n on Linux/OSX and is explicitly defined in the `CreateProcessW` call\n in `mingw.c` on Windows.\n7. ce_compare_data() closes the file\n8. Git unlinks the file as it is not present in B\n\nThe unlink operation does not work on Windows because the filter process has\nstill an open handle to the file. Apparently that is no problem on Linux/OSX.\nProbably because \"[...] the two file descriptors share open file status flags\"\n(see fork(2)).\n\nFix this problem by opening files in read-cache with the `O_CLOEXEC` flag to\nensure that the file descriptor does not remain open in a newly spawned process.\n`O_CLOEXEX` is defined as `O_NOINHERIT` on Windows. A similar fix for temporary\nfile handles was applied on Git for Windows already:\nhttps://github.com/git-for-windows/git/commit/667b8b51ec850c3e1c7d75dee69dc13c29d1f162\n\nSigned-off-by: Lars Schneider \n---\n read-cache.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex db27766..f481dee 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -159,7 +159,7 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)\n static int ce_compare_data(const struct cache_entry *ce, struct stat *st)\n {\n \tint match = -1;\n-\tint fd = open(ce->name, O_RDONLY);\n+\tint fd = open(ce->name, O_RDONLY | O_CLOEXEC);\n \n \tif (fd >= 0) {\n \t\tunsigned char sha1[20];\n-- \n2.9.2\n\n"},{"id":"293593","messageId":"20160810133745.wagccvvf35o3pbwb@sigill.intra.peff.net","threadId":"43044","inReplyTo":"F4D9C42A-5B7F-47B4-B334-704D5F6210F5@gmail.com","subject":"Re: [PATCH v5 03/15] pkt-line: add `gentle` parameter to format_packet()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-10T13:37:46Z","receivedAt":"2016-08-10T19:04:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 10, 2016 at 03:29:26PM +0200, Lars Schneider wrote:\n\n> \n> > On 10 Aug 2016, at 15:15, Jeff King wrote:\n> > \n> > On Wed, Aug 10, 2016 at 03:03:59PM +0200, larsxschneider@gmail.com wrote:\n> > \n> >> From: Lars Schneider \n> >> \n> >> format_packet() dies if the caller wants to format a packet larger than\n> >> LARGE_PACKET_MAX. Certain callers might prefer an error response instead.\n> > \n> > I am not sure I agree here. Certainly I see the usefulness of gently\n> > handling a failure to write(). But if you are passing in too-large\n> > buffers, isn't that a bug in the program?\n> > \n> > How would you recover, except by splitting up the content? That might\n> > not be possible depending on how you are using the pkt-lines. And even\n> > if it is, wouldn't it be simpler to split it up before sending it to\n> > format_packet()?\n> \n> Good argument. I agree - this patch should be dropped.\n\nActually, after reading further, one thought did occur to me. Let's say\nyou are writing to a smudge filter, and one of the header packets you\nsend has the filename in it. So you might do something like:\n\n if (packet_write_fmt_gently(fd, \"filename=%s\", filename) < 0) {\n\tif (filter_required)\n\t\tdie(...);\n\telse\n\t\treturn -1; /* we tried our best; skip smudge */\n }\n\nThe \"recovery\" there is not to try sending again, but rather to give up.\nAnd that is presumably a sane outcome for somebody who tries to checkout\na filename larger than 64K.\n\nIt does still feel a little weird that you cannot tell the difference\nbetween a write() error and bad input. Because you really might want to\ndo something different between the two. Like:\n\n #define MAX_FILENAME (PKTLINE_DATA_MAXLEN - strlen(\"filename\"))\n\n if (filename > MAX_FILENAME) {\n\twarning(\"woah, that name is ridiculous; truncating\");\n\tret = packet_write_fmt_gently(fd, \"%.*s\", MAX_FILENAME, filename);\n } else\n\tret = packet_write_fmt_gently(fd, \"%s\", filename);\n\n-Peff\n"},{"id":"293595","messageId":"20160810130411.12419-1-larsxschneider@gmail.com","threadId":"43044","inReplyTo":"20160803164225.46355-1-larsxschneider@gmail.com/","subject":"[PATCH v5 00/15] Git filter protocol","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-10T13:03:56Z","receivedAt":"2016-08-10T19:07:40Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\nHi all,\n\n## Notable changes since v4\n* drop the shutdown capability\n* add error-all response to signal that the filter does not want to filter anymore\n* extend init sequence to negotiate version number and capabilities\n (plus detect wrongly configured version 1 filters)\n* improve status response format according to Peff's suggestions\n* fix Git for Window support\n\n\n## Patches\n* 01-09: Make pktline ready to be used for filters.\n* 01 - @Junio: I know you told me twice that you don't like this patch but\n please look closely: the patch does not add a new interface to\n pktline. It is a private functions used 03.\n* 08 - This patch changes the pktline interface as it renames packet_write() to\n packet_write_fmt() as suggested by Junio.\n (This patch is not required for the the series and could be dropped)\n* 10-13: Prepare convert code.\n* 14: Main contribution.\n* 15: Proposed fix to make the long running filters work on Windows. See\n the discussion here:\n https://github.com/git-for-windows/git/issues/770#issuecomment-238361829\n (This patch is not required in Git core and could be dropped)\n\n\n## Jakub\n* http://public-inbox.org/git/607c07fe-5b6f-fd67-13e1-705020c267ee%40gmail.com/\n * make Git start the handshake with the filter to detect one-shot filter\n mis configured as process-filter\n * filter driver refusing to filter further\n\n* http://public-inbox.org/git/e8b550ed-1765-764f-49e5-72e5a609d936%40gmail.com/\n * improve commit messages\n\n* http://public-inbox.org/git/0b7d7d96-dfdc-54a4-2c24-2aead6743ae1%40gmail.com/\n * handle sigpipe correctly\n * use cp instead of cat where appropriate\n * explicitly name the test with a binary file (containing \\0)\n\n\n## Junio\n* http://public-inbox.org/git/xmqq8twd8uld.fsf%40gitster.mtv.corp.google.com/\n * shorten variable names\n * do not initialize statics explicitly to 0\n * no // comments\n * replace \"ret\" with \"!error\"\n\n* http://public-inbox.org/git/CAPc5daV3Tke4qHjtpri%3D6QCRaOax_K3uYhpFzRcd271%3DGHj1%2BQ%40mail.gmail.com/\n * rename packet_write() to packet_write_fmt()\n\n\n## Peff\n* http://public-inbox.org/git/20160803224619.bwtbvmslhuicx2qi%40sigill.intra.peff.net/\n * change capabilities format\n\n* http://public-inbox.org/git/20160808150255.2otm3z5fluimpiqw%40sigill.intra.peff.net/\n * change status response format\n\n\n## Eric\n* http://public-inbox.org/git/20160805185559.GB463%40starla/\n * die if larger-than-4GB files are processed on 32-bit\n\n\nThanks a lot for the valuable reviews!\n\nBest,\nLars\n\nYou can find a branch with the source on GitHub here:\nhttps://github.com/larsxschneider/git/tree/protocol-filter/v5\n\nI also rebased the series on top of the latest Git for Windows master:\nhttps://github.com/larsxschneider/git/tree/protocol-filter/v5-win\n\n\nLars Schneider (15):\n pkt-line: extract set_packet_header()\n pkt-line: call packet_trace() only if a packet is actually send\n pkt-line: add `gentle` parameter to format_packet()\n pkt-line: add packet_write_gently()\n pkt-line: add packet_write_gently_fmt()\n pkt-line: add packet_flush_gently()\n pkt-line: add functions to read/write flush terminated packet streams\n pkt-line: rename packet_write() to packet_write_fmt()\n pack-protocol: fix maximum pkt-line size\n convert: quote filter names in error messages\n convert: modernize tests\n convert: generate large test files only once\n convert: make apply_filter() adhere to standard Git error handling\n convert: add filter..process option\n read-cache: make sure file handles are not inherited by child\n processes\n\n Documentation/gitattributes.txt | 139 +++++++-\n Documentation/technical/protocol-common.txt | 6 +-\n builtin/archive.c | 4 +-\n builtin/receive-pack.c | 4 +-\n builtin/remote-ext.c | 4 +-\n builtin/upload-archive.c | 4 +-\n connect.c | 2 +-\n convert.c | 367 ++++++++++++++++++---\n daemon.c | 2 +-\n http-backend.c | 2 +-\n pkt-line.c | 155 ++++++++-\n pkt-line.h | 12 +-\n read-cache.c | 2 +-\n shallow.c | 2 +-\n t/t0021-conversion.sh | 482 +++++++++++++++++++++++++---\n t/t0021/rot13-filter.pl | 176 ++++++++++\n unpack-trees.c | 1 +\n upload-pack.c | 30 +-\n 18 files changed, 1267 insertions(+), 127 deletions(-)\n create mode 100755 t/t0021/rot13-filter.pl\n\n--\n2.9.2\n\n"},{"id":"293596","messageId":"20160810130411.12419-2-larsxschneider@gmail.com","threadId":"43044","inReplyTo":"20160810130411.12419-1-larsxschneider@gmail.com","subject":"[PATCH v5 01/15] pkt-line: extract set_packet_header()","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-10T13:03:57Z","receivedAt":"2016-08-10T19:08:55Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\nset_packet_header() converts an integer to a 4 byte hex string. Make\nthis function locally available so that other pkt-line functions can\nuse it.\n\nSigned-off-by: Lars Schneider \n---\n pkt-line.c | 18 ++++++++++++------\n 1 file changed, 12 insertions(+), 6 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 62fdb37..177dc73 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -97,10 +97,19 @@ void packet_buf_flush(struct strbuf *buf)\n \tstrbuf_add(buf, \"0000\", 4);\n }\n \n-#define hex(a) (hexchar[(a) & 15])\n-static void format_packet(struct strbuf *out, const char *fmt, va_list args)\n+static void set_packet_header(char *buf, const int size)\n {\n \tstatic char hexchar[] = \"0123456789abcdef\";\n+\t#define hex(a) (hexchar[(a) & 15])\n+\tbuf[0] = hex(size >> 12);\n+\tbuf[1] = hex(size >> 8);\n+\tbuf[2] = hex(size >> 4);\n+\tbuf[3] = hex(size);\n+\t#undef hex\n+}\n+\n+static void format_packet(struct strbuf *out, const char *fmt, va_list args)\n+{\n \tsize_t orig_len, n;\n \n \torig_len = out->len;\n@@ -111,10 +120,7 @@ static void format_packet(struct strbuf *out, const char *fmt, va_list args)\n \tif (n > LARGE_PACKET_MAX)\n \t\tdie(\"protocol error: impossibly long line\");\n \n-\tout->buf[orig_len + 0] = hex(n >> 12);\n-\tout->buf[orig_len + 1] = hex(n >> 8);\n-\tout->buf[orig_len + 2] = hex(n >> 4);\n-\tout->buf[orig_len + 3] = hex(n);\n+\tset_packet_header(&out->buf[orig_len], n);\n \tpacket_trace(out->buf + orig_len + 4, n - 4, 1);\n }\n \n-- \n2.9.2\n\n"},{"id":"293597","messageId":"20160810130411.12419-10-larsxschneider@gmail.com","threadId":"43044","inReplyTo":"20160810130411.12419-1-larsxschneider@gmail.com","subject":"[PATCH v5 09/15] pack-protocol: fix maximum pkt-line size","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-10T13:04:05Z","receivedAt":"2016-08-10T19:08:58Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\nAccording to LARGE_PACKET_MAX in pkt-line.h the maximal length of a\npkt-line packet is 65520 bytes. The pkt-line header takes 4 bytes and\ntherefore the pkt-line data component must not exceed 65516 bytes.\n\nSigned-off-by: Lars Schneider \n---\n Documentation/technical/protocol-common.txt | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/technical/protocol-common.txt b/Documentation/technical/protocol-common.txt\nindex bf30167..ecedb34 100644\n--- a/Documentation/technical/protocol-common.txt\n+++ b/Documentation/technical/protocol-common.txt\n@@ -67,9 +67,9 @@ with non-binary data the same whether or not they contain the trailing\n LF (stripping the LF if present, and not complaining when it is\n missing).\n \n-The maximum length of a pkt-line's data component is 65520 bytes.\n-Implementations MUST NOT send pkt-line whose length exceeds 65524\n-(65520 bytes of payload + 4 bytes of length data).\n+The maximum length of a pkt-line's data component is 65516 bytes.\n+Implementations MUST NOT send pkt-line whose length exceeds 65520\n+(65516 bytes of payload + 4 bytes of length data).\n \n Implementations SHOULD NOT send an empty pkt-line (\"0004\").\n \n-- \n2.9.2\n\n"},{"id":"293600","messageId":"20160810134346.6nmf2sudwh56nq76@sigill.intra.peff.net","threadId":"43044","inReplyTo":"20160810130411.12419-6-larsxschneider@gmail.com","subject":"Re: [PATCH v5 05/15] pkt-line: add packet_write_gently_fmt()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-10T13:43:47Z","receivedAt":"2016-08-10T19:10:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 10, 2016 at 03:04:01PM +0200, larsxschneider@gmail.com wrote:\n\n> +int packet_write_gently_fmt(int fd, const char *fmt, ...)\n> +{\n> +\tstatic struct strbuf buf = STRBUF_INIT;\n> +\tva_list args;\n> +\n> +\tstrbuf_reset(&buf);\n> +\tva_start(args, fmt);\n> +\tformat_packet(1, &buf, fmt, args);\n> +\tva_end(args);\n> +\tpacket_trace(buf.buf + 4, buf.len - 4, 1);\n> +\treturn (write_in_full(fd, buf.buf, buf.len) == buf.len ? 0 : -1);\n> +}\n\nCould the end of this function just be:\n\n return packet_write_gently(fd, buf.buf, buf.len);\n\n? I guess we'd prefer to avoid that, because it incurs an extra\nmemmove() of the data.\n\nSimilarly, I'd think this could share code with the non-gentle form\n(which should be able to just call this and die() if returns an error).\nThough sometimes the va_list transformation makes that awkward.\n\n-Peff\n"},{"id":"293601","messageId":"20160810134003.q6mzgkcrwmkxv5fw@sigill.intra.peff.net","threadId":"43044","inReplyTo":"434CB5D7-3FC0-4398-9028-135701121E55@gmail.com","subject":"Re: [PATCH v5 04/15] pkt-line: add packet_write_gently()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-10T13:40:04Z","receivedAt":"2016-08-10T19:11:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 10, 2016 at 03:36:45PM +0200, Lars Schneider wrote:\n\n> > So now we have packet_write() and packet_write_gently(), but they differ\n> > in more than just whether they are gentle. That seems like a weird\n> > interface.\n> > \n> > Should we either be picking a new name (e.g., packet_write_mem() or\n> > something), or migrating packet_write() to packet_write_fmt()?\n> \n> Done in \"[PATCH v5 08/15] pkt-line: rename packet_write() to packet_write_fmt()\"\n\nAh, OK. Generally I'd suggest to reorder things so that each patch looks\nlike a step forward (and so the early patches become preparatory steps,\nand the justification in them is something like \"we're going to add more\nwrite functions, so let's give this a more descriptive name\").\n\n> I agree. In a later patch I am using PKTLINE_DATA_MAXLEN inside pkt-line.c,\n> too. I will change it to your suggestion.\n> \n> For now I would remove PKTLINE_DATA_MAXLEN because it should be an implementation\n> detail of pkt-line.c (plus it is not used by anyone).\n\nSounds reasonable.\n\n-Peff\n"},{"id":"293602","messageId":"20160810130411.12419-7-larsxschneider@gmail.com","threadId":"43044","inReplyTo":"20160810130411.12419-1-larsxschneider@gmail.com","subject":"[PATCH v5 06/15] pkt-line: add packet_flush_gently()","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-10T13:04:02Z","receivedAt":"2016-08-10T19:12:07Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\npacket_flush() would die in case of a write error even though for some callers\nan error would be acceptable. Add packet_flush_gently() which writes a pkt-line\nflush packet and returns `0` for success and `-1` for failure.\n\nSigned-off-by: Lars Schneider \n---\n pkt-line.c | 6 ++++++\n pkt-line.h | 1 +\n 2 files changed, 7 insertions(+)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex a8207dd..e6a0924 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -92,6 +92,12 @@ void packet_flush(int fd)\n \twrite_or_die(fd, \"0000\", 4);\n }\n \n+int packet_flush_gently(int fd)\n+{\n+\tpacket_trace(\"0000\", 4, 1);\n+\treturn (write_in_full(fd, \"0000\", 4) == 4 ? 0 : -1);\n+}\n+\n void packet_buf_flush(struct strbuf *buf)\n {\n \tpacket_trace(\"0000\", 4, 1);\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 82676f4..b6c8bcd 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -23,6 +23,7 @@ void packet_flush(int fd);\n void packet_write(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n 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_gently_fmt(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n \n /*\n-- \n2.9.2\n\n"},{"id":"293605","messageId":"350E338E-BBBA-4FE4-8209-4C05056E2AAD@gmail.com","threadId":"43044","inReplyTo":"xmqq8tw45vtg.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v5 04/15] pkt-line: add packet_write_gently()","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-10T19:15:50Z","receivedAt":"2016-08-10T19:15:59Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 10 Aug 2016, at 20:21, Junio C Hamano wrote:\n> \n> Lars Schneider writes:\n> \n>>> On 10 Aug 2016, at 19:17, Junio C Hamano wrote:\n>>> \n>> OK. Does this mean I can leave the \"packet_write()\" to \"packet_write_fmt()\"\n>> rename as is in this series?\n> \n> I didn't really check what order you are doing things to answer\n> that.\n> \n> If the function that is introduced in this step is a version of\n> packet_write_fmt() that does its thing only gently, you would want\n> to do the rename s/packet_write/packet_write_fmt/ before this step,\n> and then add the new function as packet_write_fmt_gently(), I would\n> think.\n\nOK - will fix. I did it that way because I thought it would be easier\nif we decide to drop the big rename patch.\n\nThanks,\nLars\n\n"},{"id":"293607","messageId":"20160810130411.12419-11-larsxschneider@gmail.com","threadId":"43044","inReplyTo":"20160810130411.12419-1-larsxschneider@gmail.com","subject":"[PATCH v5 10/15] convert: quote filter names in error messages","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-10T13:04:06Z","receivedAt":"2016-08-10T19:20:40Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\nGit filter driver commands with spaces (e.g. `filter.sh foo`) are hard to\nread in error messages. Quote them to improve the readability.\n\nSigned-off-by: Lars Schneider \n---\n convert.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex b1614bf..522e2c5 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -397,7 +397,7 @@ static int filter_buffer_or_fd(int in, int out, void *data)\n \tchild_process.out = out;\n \n \tif (start_command(&child_process))\n-\t\treturn error(\"cannot fork to run external filter %s\", params->cmd);\n+\t\treturn error(\"cannot fork to run external filter '%s'\", params->cmd);\n \n \tsigchain_push(SIGPIPE, SIG_IGN);\n \n@@ -415,13 +415,13 @@ static int filter_buffer_or_fd(int in, int out, void *data)\n \tif (close(child_process.in))\n \t\twrite_err = 1;\n \tif (write_err)\n-\t\terror(\"cannot feed the input to external filter %s\", params->cmd);\n+\t\terror(\"cannot feed the input to external filter '%s'\", params->cmd);\n \n \tsigchain_pop(SIGPIPE);\n \n \tstatus = finish_command(&child_process);\n \tif (status)\n-\t\terror(\"external filter %s failed %d\", params->cmd, status);\n+\t\terror(\"external filter '%s' failed %d\", params->cmd, status);\n \n \tstrbuf_release(&cmd);\n \treturn (write_err || status);\n@@ -462,15 +462,15 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,\n \t\treturn 0;\t/* error was already reported */\n \n \tif (strbuf_read(&nbuf, async.out, len) < 0) {\n-\t\terror(\"read from external filter %s failed\", cmd);\n+\t\terror(\"read from external filter '%s' failed\", cmd);\n \t\tret = 0;\n \t}\n \tif (close(async.out)) {\n-\t\terror(\"read from external filter %s failed\", cmd);\n+\t\terror(\"read from external filter '%s' failed\", cmd);\n \t\tret = 0;\n \t}\n \tif (finish_async(&async)) {\n-\t\terror(\"external filter %s failed\", cmd);\n+\t\terror(\"external filter '%s' failed\", cmd);\n \t\tret = 0;\n \t}\n \n-- \n2.9.2\n\n"},{"id":"293610","messageId":"9BF0767F-6B10-42D5-9A72-2710A753E26A@gmail.com","threadId":"43044","inReplyTo":"20160810133017.ovdx3a3ldqj5gl7q@sigill.intra.peff.net","subject":"Re: [PATCH v5 02/15] pkt-line: call packet_trace() only if a packet is actually send","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-10T13:51:35Z","receivedAt":"2016-08-10T19:22:30Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 10 Aug 2016, at 15:30, Jeff King wrote:\n> \n> On Wed, Aug 10, 2016 at 03:24:38PM +0200, Lars Schneider wrote:\n> \n>>> On Wed, Aug 10, 2016 at 03:03:58PM +0200, larsxschneider@gmail.com wrote:\n>>> \n>>>> From: Lars Schneider \n>>>> \n>>>> The packet_trace() call is not ideal in format_packet() as we would print\n>>>> a trace when a packet is formatted and (potentially) when the packet is\n>>>> actually send. This was no problem up until now because format_packet()\n>>>> was only used by one function. Fix it by moving the trace call into the\n>>>> function that actually sends the packet.\n>>> \n>>> It looks like there are two functions: packet_write() and\n>>> packet_buf_write().\n>> \n>> I did not call trace in packet_buf_write() because this function does not\n>> perform any writes.\n> \n> Yes, but then who is responsible for the trace? The caller?\n\nFrom my point of view the one that issues the write call.\n\n\n> And why is it a bad thing to do it some time other than writing? It is\n> if you format and then _don't_ write the packet, but the current callers\n> are not doing that.\n\nTrue, they don't do that. However, I don't think that is intuitive behavior\nfor future callers. If I call \"format\" and then trace tells me that a packet\nwas sent although it was not (yet).\n\n> \n>>> Your patch only touches one of them, and it looks like we would fail to\n>>> trace many packets (e.g., see receive-pack.c:report(), which uses\n>>> packet_buf_write() and then write()s out the result).\n>> \n>> I see. But isn't it confusing if packet_buf_write() issues a trace call?\n>> If I just call this function then nothing happens at all. Shouldn't the\n>> trace call be made in receive-pack.c:report() ? Or shouldn't receive-pack\n>> let pkt-line.c perform the write calls?\n> \n> How would report() do that without re-parsing each of the packets?\n\nWe could add a function to pkt-line that takes a list of strings and\ngenerates a list of packets with a terminating flush packet out of it.\nThen it sends the packets.\n\nAs a quick compromise we could also introduce a function \"packet_buf_write_with_trace()\"\nthat wraps the \"packet_buf_write()\" and calls trace. However, I can imagine that\nwould be perceived as ugly.\n\nI guess my point is that I stumbled over the un-intutiive format_packet() behavior\nand I wanted to improve the situation in a way that others don't run into this\ntrap. If you think that is no issue then it would be OK for me if we leave the\ncurrent behavior as is.\n\nThanks,\nLars\n\n\n\n\n"},{"id":"293611","messageId":"20160810150139.lpxyrqkr53s5f4sx@sigill.intra.peff.net","threadId":"43044","inReplyTo":"E6D0E830-764D-42E5-9F69-340F718B98B2@gmail.com","subject":"Re: [PATCH v5 05/15] pkt-line: add packet_write_gently_fmt()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-10T15:01:39Z","receivedAt":"2016-08-10T19:22:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 10, 2016 at 04:10:02PM +0200, Lars Schneider wrote:\n\n> \n> > On 10 Aug 2016, at 15:43, Jeff King wrote:\n> > \n> > On Wed, Aug 10, 2016 at 03:04:01PM +0200, larsxschneider@gmail.com wrote:\n> > \n> >> +int packet_write_gently_fmt(int fd, const char *fmt, ...)\n> >> +{\n> >> +\tstatic struct strbuf buf = STRBUF_INIT;\n> >> +\tva_list args;\n> >> +\n> >> +\tstrbuf_reset(&buf);\n> >> +\tva_start(args, fmt);\n> >> +\tformat_packet(1, &buf, fmt, args);\n> >> +\tva_end(args);\n> >> +\tpacket_trace(buf.buf + 4, buf.len - 4, 1);\n> >> +\treturn (write_in_full(fd, buf.buf, buf.len) == buf.len ? 0 : -1);\n> >> +}\n> > \n> > Could the end of this function just be:\n> > \n> > return packet_write_gently(fd, buf.buf, buf.len);\n> > \n> > ? I guess we'd prefer to avoid that, because it incurs an extra\n> > memmove() of the data.\n> \n> I don't think the memmove would be that expensive. However, format_packet()\n> already creates the packet_header and packet_write_gently would do the same\n> again, no?\n\nYeah, I think you would want extra refactoring to have a shared common\nfunction. I took a stab at it, but the result ends up pretty ugly; the\namount of boilerplate exceeds the duplication here (the really nasty\nthing is that format_packet() is hard to split up, because the part you\nwant to switch out is in the middle, but it needs to keep some context\nbetween the start and the end. In a higher level language you'd pass it\na callback to fill in the strbuf in the middle, but in C that just ends\nup horrible).\n\n> > Similarly, I'd think this could share code with the non-gentle form\n> > (which should be able to just call this and die() if returns an error).\n> > Though sometimes the va_list transformation makes that awkward.\n> \n> Yeah, the code duplication annoyed me, too. va_list was the reason I did it\n> that way. Do you think that is something that needs to be addressed in the\n> series?\n\nNo, I don't think it needs to be. It's just a case of making sure that\nthe internals don't grow too crufty and unmanageable for future\nmaintainability.\n\n-Peff\n"},{"id":"293613","messageId":"20160810130411.12419-8-larsxschneider@gmail.com","threadId":"43044","inReplyTo":"20160810130411.12419-1-larsxschneider@gmail.com","subject":"[PATCH v5 07/15] pkt-line: add functions to read/write flush terminated packet streams","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-10T13:04:03Z","receivedAt":"2016-08-10T19:29:48Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\npacket_write_stream_with_flush_from_fd() and\npacket_write_stream_with_flush_from_buf() write a stream of packets. All\ncontent packets use the maximal packet size except for the last one.\nAfter the last content packet a `flush` control packet is written.\n\npacket_read_till_flush() reads arbitary sized packets until it detects\na `flush` packet.\n\nSigned-off-by: Lars Schneider \n---\n pkt-line.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n pkt-line.h | 7 +++++\n 2 files changed, 96 insertions(+)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex e6a0924..4b526e1 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -181,6 +181,45 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...)\n \tva_end(args);\n }\n \n+int packet_write_stream_with_flush_from_fd(int fd_in, int fd_out)\n+{\n+\tint err = 0;\n+\tssize_t bytes_to_write;\n+\twhile (!err) {\n+\t\tbytes_to_write = xread(fd_in, packet_write_buffer, PKTLINE_DATA_MAXLEN);\n+\t\tif (bytes_to_write < 0)\n+\t\t\treturn COPY_READ_ERROR;\n+\t\tif (bytes_to_write == 0)\n+\t\t\tbreak;\n+\t\tif (bytes_to_write > PKTLINE_DATA_MAXLEN)\n+\t\t\treturn COPY_WRITE_ERROR;\n+\t\terr = packet_write_gently(fd_out, packet_write_buffer, bytes_to_write);\n+\t}\n+\tif (!err)\n+\t\terr = packet_flush_gently(fd_out);\n+\treturn err;\n+}\n+\n+int packet_write_stream_with_flush_from_buf(const char *src_in, size_t len, int fd_out)\n+{\n+\tint err = 0;\n+\tsize_t bytes_written = 0;\n+\tsize_t bytes_to_write;\n+\twhile (!err) {\n+\t\tif ((len - bytes_written) > PKTLINE_DATA_MAXLEN)\n+\t\t\tbytes_to_write = PKTLINE_DATA_MAXLEN;\n+\t\telse\n+\t\t\tbytes_to_write = len - bytes_written;\n+\t\tif (bytes_to_write == 0)\n+\t\t\tbreak;\n+\t\terr = packet_write_gently(fd_out, src_in + bytes_written, bytes_to_write);\n+\t\tbytes_written += bytes_to_write;\n+\t}\n+\tif (!err)\n+\t\terr = packet_flush_gently(fd_out);\n+\treturn err;\n+}\n+\n static int get_packet_data(int fd, char **src_buf, size_t *src_size,\n \t\t\t void *dst, unsigned size, int options)\n {\n@@ -290,3 +329,53 @@ char *packet_read_line_buf(char **src, size_t *src_len, int *dst_len)\n {\n \treturn packet_read_line_generic(-1, src, src_len, dst_len);\n }\n+\n+ssize_t packet_read_till_flush(int fd_in, struct strbuf *sb_out)\n+{\n+\tint len, ret;\n+\tint options = PACKET_READ_GENTLE_ON_EOF;\n+\tchar linelen[4];\n+\n+\tsize_t oldlen = sb_out->len;\n+\tsize_t oldalloc = sb_out->alloc;\n+\n+\tfor (;;) {\n+\t\t/* Read packet header */\n+\t\tret = get_packet_data(fd_in, NULL, NULL, linelen, 4, options);\n+\t\tif (ret < 0)\n+\t\t\tgoto done;\n+\t\tlen = packet_length(linelen);\n+\t\tif (len < 0)\n+\t\t\tdie(\"protocol error: bad line length character: %.4s\", linelen);\n+\t\tif (!len) {\n+\t\t\t/* Found a flush packet - Done! */\n+\t\t\tpacket_trace(\"0000\", 4, 0);\n+\t\t\tbreak;\n+\t\t}\n+\t\tlen -= 4;\n+\n+\t\t/* Read packet content */\n+\t\tstrbuf_grow(sb_out, len);\n+\t\tret = get_packet_data(fd_in, NULL, NULL, sb_out->buf + sb_out->len, len, options);\n+\t\tif (ret < 0)\n+\t\t\tgoto done;\n+\n+\t\tif (ret != len) {\n+\t\t\terror(\"protocol error: incomplete read (expected %d, got %d)\", len, ret);\n+\t\t\tgoto done;\n+\t\t}\n+\n+\t\tpacket_trace(sb_out->buf + sb_out->len, len, 0);\n+\t\tsb_out->len += len;\n+\t}\n+\n+done:\n+\tif (ret < 0) {\n+\t\tif (oldalloc == 0)\n+\t\t\tstrbuf_release(sb_out);\n+\t\telse\n+\t\t\tstrbuf_setlen(sb_out, oldlen);\n+\t\treturn ret; /* unexpected EOF */\n+\t}\n+\treturn sb_out->len - oldlen;\n+}\ndiff --git a/pkt-line.h b/pkt-line.h\nindex b6c8bcd..89063ee 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -25,6 +25,8 @@ 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_gently_fmt(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n+int packet_write_stream_with_flush_from_fd(int fd_in, int fd_out);\n+int packet_write_stream_with_flush_from_buf(const char *src_in, size_t len, int fd_out);\n \n /*\n * Read a packetized line into the buffer, which must be at least size bytes\n@@ -77,6 +79,11 @@ char *packet_read_line(int fd, int *size);\n */\n char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size);\n \n+/*\n+ * Reads a stream of variable sized packets until a flush packet is detected.\n+ */\n+ssize_t packet_read_till_flush(int fd_in, struct strbuf *sb_out);\n+\n #define DEFAULT_PACKET_MAX 1000\n #define LARGE_PACKET_MAX 65520\n #define PKTLINE_DATA_MAXLEN (LARGE_PACKET_MAX - 4)\n-- \n2.9.2\n\n"},{"id":"293614","messageId":"20160810130411.12419-13-larsxschneider@gmail.com","threadId":"43044","inReplyTo":"20160810130411.12419-1-larsxschneider@gmail.com","subject":"[PATCH v5 12/15] convert: generate large test files only once","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-10T13:04:08Z","receivedAt":"2016-08-10T19:30:28Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\nGenerate more interesting large test files with pseudo random characters\nin between and reuse these test files in multiple tests. Run tests formerly\nmarked as EXPENSIVE every time but with a smaller data set.\n\nSigned-off-by: Lars Schneider \n---\n t/t0021-conversion.sh | 48 ++++++++++++++++++++++++++++++++++++++----------\n 1 file changed, 38 insertions(+), 10 deletions(-)\n\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex 7b45136..34c8eb9 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -4,6 +4,15 @@ test_description='blob conversion via gitattributes'\n \n . ./test-lib.sh\n \n+if test_have_prereq EXPENSIVE\n+then\n+\tT0021_LARGE_FILE_SIZE=2048\n+\tT0021_LARGISH_FILE_SIZE=100\n+else\n+\tT0021_LARGE_FILE_SIZE=30\n+\tT0021_LARGISH_FILE_SIZE=2\n+fi\n+\n cat <rot13.sh\n #!$SHELL_PATH\n tr \\\n@@ -31,7 +40,26 @@ test_expect_success setup '\n \tcat test >test.i &&\n \tgit add test test.t test.i &&\n \trm -f test test.t test.i &&\n-\tgit checkout -- test test.t test.i\n+\tgit checkout -- test test.t test.i &&\n+\n+\tmkdir generated-test-data &&\n+\tfor i in $(test_seq 1 $T0021_LARGE_FILE_SIZE)\n+\tdo\n+\t\tRANDOM_STRING=\"$(test-genrandom end $i | tr -dc \"A-Za-z0-9\" )\"\n+\t\tROT_RANDOM_STRING=\"$(echo $RANDOM_STRING | ./rot13.sh )\"\n+\t\t# Generate 1MB of empty data and 100 bytes of random characters\n+\t\t# printf \"$(test-genrandom start $i)\"\n+\t\tprintf \"%1048576d\" 1 >>generated-test-data/large.file &&\n+\t\tprintf \"$RANDOM_STRING\" >>generated-test-data/large.file &&\n+\t\tprintf \"%1048576d\" 1 >>generated-test-data/large.file.rot13 &&\n+\t\tprintf \"$ROT_RANDOM_STRING\" >>generated-test-data/large.file.rot13 &&\n+\n+\t\tif test $i = $T0021_LARGISH_FILE_SIZE\n+\t\tthen\n+\t\t\tcat generated-test-data/large.file >generated-test-data/largish.file &&\n+\t\t\tcat generated-test-data/large.file.rot13 >generated-test-data/largish.file.rot13\n+\t\tfi\n+\tdone\n '\n \n script='s/^\\$Id: \\([0-9a-f]*\\) \\$/\\1/p'\n@@ -199,9 +227,9 @@ test_expect_success 'required filter clean failure' '\n test_expect_success 'filtering large input to small output should use little memory' '\n \ttest_config filter.devnull.clean \"cat >/dev/null\" &&\n \ttest_config filter.devnull.required true &&\n-\tfor i in $(test_seq 1 30); do printf \"%1048576d\" 1; done >30MB &&\n-\techo \"30MB filter=devnull\" >.gitattributes &&\n-\tGIT_MMAP_LIMIT=1m GIT_ALLOC_LIMIT=1m git add 30MB\n+\tcp generated-test-data/large.file large.file &&\n+\techo \"large.file filter=devnull\" >.gitattributes &&\n+\tGIT_MMAP_LIMIT=1m GIT_ALLOC_LIMIT=1m git add large.file\n '\n \n test_expect_success 'filter that does not read is fine' '\n@@ -214,15 +242,15 @@ test_expect_success 'filter that does not read is fine' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success EXPENSIVE 'filter large file' '\n+test_expect_success 'filter large file' '\n \ttest_config filter.largefile.smudge cat &&\n \ttest_config filter.largefile.clean cat &&\n-\tfor i in $(test_seq 1 2048); do printf \"%1048576d\" 1; done >2GB &&\n-\techo \"2GB filter=largefile\" >.gitattributes &&\n-\tgit add 2GB 2>err &&\n+\techo \"large.file filter=largefile\" >.gitattributes &&\n+\tcp generated-test-data/large.file large.file &&\n+\tgit add large.file 2>err &&\n \ttest_must_be_empty err &&\n-\trm -f 2GB &&\n-\tgit checkout -- 2GB 2>err &&\n+\trm -f large.file &&\n+\tgit checkout -- large.file 2>err &&\n \ttest_must_be_empty err\n '\n \n-- \n2.9.2\n\n"},{"id":"293616","messageId":"20160810132814.gqnipsdwyzjmuqjy@sigill.intra.peff.net","threadId":"43044","inReplyTo":"20160810130411.12419-5-larsxschneider@gmail.com","subject":"Re: [PATCH v5 04/15] pkt-line: add packet_write_gently()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-10T13:28:15Z","receivedAt":"2016-08-10T19:33:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 10, 2016 at 03:04:00PM +0200, larsxschneider@gmail.com wrote:\n\n> From: Lars Schneider \n> \n> packet_write() has two shortcomings. First, it uses format_packet() which\n> lets the caller only send string data via \"%s\". That means it cannot be\n> used for arbitrary data that may contain NULs. Second, it will always\n> die on error.\n> \n> Add packet_write_gently() which writes arbitrary data and returns `0` for\n> success and `-1` for an error.\n\nSo now we have packet_write() and packet_write_gently(), but they differ\nin more than just whether they are gentle. That seems like a weird\ninterface.\n\nShould we either be picking a new name (e.g., packet_write_mem() or\nsomething), or migrating packet_write() to packet_write_fmt()?\n\n> diff --git a/pkt-line.c b/pkt-line.c\n> index e6b8410..4f25748 100644\n> --- a/pkt-line.c\n> +++ b/pkt-line.c\n> @@ -3,6 +3,7 @@\n> #include \"run-command.h\"\n> \n> char packet_buffer[LARGE_PACKET_MAX];\n> +char packet_write_buffer[LARGE_PACKET_MAX];\n\nShould this be static? I.e., are random other bits of the code allowed\nto write into it (I guess not because it's not declared in pkt-line.h).\n\n> +int packet_write_gently(const int fd_out, const char *buf, size_t size)\n> +{\n> +\tif (size > PKTLINE_DATA_MAXLEN)\n> +\t\treturn -1;\n> +\tpacket_trace(buf, size, 1);\n> +\tmemmove(packet_write_buffer + 4, buf, size);\n\nIt looks like this iteration drops the idea of callers using a\nLARGE_PACKET_MAX buffer and only filling it at \"buf+4\" with\nPKTLINE_DATA_MAXLEN bytes (which is fine).\n\nI wonder if we still need PKTLINE_DATA_MAXLEN, or of it is just\nobscuring things. The magic number \"4\" still appears separately here,\nand it actually makes it harder to see that things are correct. I.e.,\ndoing:\n\n if (size > sizeof(packet_write_buffer) - 4)\n\treturn -1;\n memmove(packet_write_buffer + 4, buf, size);\n\nis more obviously correct, because you do not have to wonder about the\nrelationship between the size of your buffer and the macro.\n\nIt might still be worth having PKTLINE_DATA_MAXLEN public, though, if\ncallers use it to size their input to packet_write_gently().\n\n-Peff\n"},{"id":"293617","messageId":"20160810130411.12419-12-larsxschneider@gmail.com","threadId":"43044","inReplyTo":"20160810130411.12419-1-larsxschneider@gmail.com","subject":"[PATCH v5 11/15] convert: modernize tests","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-10T13:04:07Z","receivedAt":"2016-08-10T19:37:01Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\nUse `test_config` to set the config, check that files are empty with\n`test_must_be_empty`, compare files with `test_cmp`, and remove spaces\nafter \">\" and \"<\".\n\nSigned-off-by: Lars Schneider \n---\n t/t0021-conversion.sh | 62 +++++++++++++++++++++++++--------------------------\n 1 file changed, 31 insertions(+), 31 deletions(-)\n\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex 7bac2bc..7b45136 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -13,8 +13,8 @@ EOF\n chmod +x rot13.sh\n \n test_expect_success setup '\n-\tgit config filter.rot13.smudge ./rot13.sh &&\n-\tgit config filter.rot13.clean ./rot13.sh &&\n+\ttest_config filter.rot13.smudge ./rot13.sh &&\n+\ttest_config filter.rot13.clean ./rot13.sh &&\n \n \t{\n \t echo \"*.t filter=rot13\"\n@@ -38,8 +38,8 @@ script='s/^\\$Id: \\([0-9a-f]*\\) \\$/\\1/p'\n \n test_expect_success check '\n \n-\tcmp test.o test &&\n-\tcmp test.o test.t &&\n+\ttest_cmp test.o test &&\n+\ttest_cmp test.o test.t &&\n \n \t# ident should be stripped in the repository\n \tgit diff --raw --exit-code :test :test.i &&\n@@ -47,10 +47,10 @@ test_expect_success check '\n \tembedded=$(sed -ne \"$script\" test.i) &&\n \ttest \"z$id\" = \"z$embedded\" &&\n \n-\tgit cat-file blob :test.t > test.r &&\n+\tgit cat-file blob :test.t >test.r &&\n \n-\t./rot13.sh < test.o > test.t &&\n-\tcmp test.r test.t\n+\t./rot13.sh test.t &&\n+\ttest_cmp test.r test.t\n '\n \n # If an expanded ident ever gets into the repository, we want to make sure that\n@@ -130,7 +130,7 @@ test_expect_success 'filter shell-escaped filenames' '\n \n \t# delete the files and check them out again, using a smudge filter\n \t# that will count the args and echo the command-line back to us\n-\tgit config filter.argc.smudge \"sh ./argc.sh %f\" &&\n+\ttest_config filter.argc.smudge \"sh ./argc.sh %f\" &&\n \trm \"$normal\" \"$special\" &&\n \tgit checkout -- \"$normal\" \"$special\" &&\n \n@@ -141,7 +141,7 @@ test_expect_success 'filter shell-escaped filenames' '\n \ttest_cmp expect \"$special\" &&\n \n \t# do the same thing, but with more args in the filter expression\n-\tgit config filter.argc.smudge \"sh ./argc.sh %f --my-extra-arg\" &&\n+\ttest_config filter.argc.smudge \"sh ./argc.sh %f --my-extra-arg\" &&\n \trm \"$normal\" \"$special\" &&\n \tgit checkout -- \"$normal\" \"$special\" &&\n \n@@ -154,9 +154,9 @@ test_expect_success 'filter shell-escaped filenames' '\n '\n \n test_expect_success 'required filter should filter data' '\n-\tgit config filter.required.smudge ./rot13.sh &&\n-\tgit config filter.required.clean ./rot13.sh &&\n-\tgit config filter.required.required true &&\n+\ttest_config filter.required.smudge ./rot13.sh &&\n+\ttest_config filter.required.clean ./rot13.sh &&\n+\ttest_config filter.required.required true &&\n \n \techo \"*.r filter=required\" >.gitattributes &&\n \n@@ -165,17 +165,17 @@ test_expect_success 'required filter should filter data' '\n \n \trm -f test.r &&\n \tgit checkout -- test.r &&\n-\tcmp test.o test.r &&\n+\ttest_cmp test.o test.r &&\n \n \t./rot13.sh expected &&\n \tgit cat-file blob :test.r >actual &&\n-\tcmp expected actual\n+\ttest_cmp expected actual\n '\n \n test_expect_success 'required filter smudge failure' '\n-\tgit config filter.failsmudge.smudge false &&\n-\tgit config filter.failsmudge.clean cat &&\n-\tgit config filter.failsmudge.required true &&\n+\ttest_config filter.failsmudge.smudge false &&\n+\ttest_config filter.failsmudge.clean cat &&\n+\ttest_config filter.failsmudge.required true &&\n \n \techo \"*.fs filter=failsmudge\" >.gitattributes &&\n \n@@ -186,9 +186,9 @@ test_expect_success 'required filter smudge failure' '\n '\n \n test_expect_success 'required filter clean failure' '\n-\tgit config filter.failclean.smudge cat &&\n-\tgit config filter.failclean.clean false &&\n-\tgit config filter.failclean.required true &&\n+\ttest_config filter.failclean.smudge cat &&\n+\ttest_config filter.failclean.clean false &&\n+\ttest_config filter.failclean.required true &&\n \n \techo \"*.fc filter=failclean\" >.gitattributes &&\n \n@@ -197,8 +197,8 @@ test_expect_success 'required filter clean failure' '\n '\n \n test_expect_success 'filtering large input to small output should use little memory' '\n-\tgit config filter.devnull.clean \"cat >/dev/null\" &&\n-\tgit config filter.devnull.required true &&\n+\ttest_config filter.devnull.clean \"cat >/dev/null\" &&\n+\ttest_config filter.devnull.required true &&\n \tfor i in $(test_seq 1 30); do printf \"%1048576d\" 1; done >30MB &&\n \techo \"30MB filter=devnull\" >.gitattributes &&\n \tGIT_MMAP_LIMIT=1m GIT_ALLOC_LIMIT=1m git add 30MB\n@@ -207,7 +207,7 @@ test_expect_success 'filtering large input to small output should use little mem\n test_expect_success 'filter that does not read is fine' '\n \ttest-genrandom foo $((128 * 1024 + 1)) >big &&\n \techo \"big filter=epipe\" >.gitattributes &&\n-\tgit config filter.epipe.clean \"echo xyzzy\" &&\n+\ttest_config filter.epipe.clean \"echo xyzzy\" &&\n \tgit add big &&\n \tgit cat-file blob :big >actual &&\n \techo xyzzy >expect &&\n@@ -215,20 +215,20 @@ test_expect_success 'filter that does not read is fine' '\n '\n \n test_expect_success EXPENSIVE 'filter large file' '\n-\tgit config filter.largefile.smudge cat &&\n-\tgit config filter.largefile.clean cat &&\n+\ttest_config filter.largefile.smudge cat &&\n+\ttest_config filter.largefile.clean cat &&\n \tfor i in $(test_seq 1 2048); do printf \"%1048576d\" 1; done >2GB &&\n \techo \"2GB filter=largefile\" >.gitattributes &&\n \tgit add 2GB 2>err &&\n-\t! test -s err &&\n+\ttest_must_be_empty err &&\n \trm -f 2GB &&\n \tgit checkout -- 2GB 2>err &&\n-\t! test -s err\n+\ttest_must_be_empty err\n '\n \n test_expect_success \"filter: clean empty file\" '\n-\tgit config filter.in-repo-header.clean \"echo cleaned && cat\" &&\n-\tgit config filter.in-repo-header.smudge \"sed 1d\" &&\n+\ttest_config filter.in-repo-header.clean \"echo cleaned && cat\" &&\n+\ttest_config filter.in-repo-header.smudge \"sed 1d\" &&\n \n \techo \"empty-in-worktree filter=in-repo-header\" >>.gitattributes &&\n \t>empty-in-worktree &&\n@@ -240,8 +240,8 @@ test_expect_success \"filter: clean empty file\" '\n '\n \n test_expect_success \"filter: smudge empty file\" '\n-\tgit config filter.empty-in-repo.clean \"cat >/dev/null\" &&\n-\tgit config filter.empty-in-repo.smudge \"echo smudged && cat\" &&\n+\ttest_config filter.empty-in-repo.clean \"cat >/dev/null\" &&\n+\ttest_config filter.empty-in-repo.smudge \"echo smudged && cat\" &&\n \n \techo \"empty-in-repo filter=empty-in-repo\" >>.gitattributes &&\n \techo dead data walking >empty-in-repo &&\n-- \n2.9.2\n\n"},{"id":"293618","messageId":"F4D9C42A-5B7F-47B4-B334-704D5F6210F5@gmail.com","threadId":"43044","inReplyTo":"20160810131541.ovpvgwdxjibae5gy@sigill.intra.peff.net","subject":"Re: [PATCH v5 03/15] pkt-line: add `gentle` parameter to format_packet()","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-10T13:29:26Z","receivedAt":"2016-08-10T19:38:30Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 10 Aug 2016, at 15:15, Jeff King wrote:\n> \n> On Wed, Aug 10, 2016 at 03:03:59PM +0200, larsxschneider@gmail.com wrote:\n> \n>> From: Lars Schneider \n>> \n>> format_packet() dies if the caller wants to format a packet larger than\n>> LARGE_PACKET_MAX. Certain callers might prefer an error response instead.\n> \n> I am not sure I agree here. Certainly I see the usefulness of gently\n> handling a failure to write(). But if you are passing in too-large\n> buffers, isn't that a bug in the program?\n> \n> How would you recover, except by splitting up the content? That might\n> not be possible depending on how you are using the pkt-lines. And even\n> if it is, wouldn't it be simpler to split it up before sending it to\n> format_packet()?\n\nGood argument. I agree - this patch should be dropped.\n\nThanks,\nLars\n"},{"id":"293620","messageId":"20160810130411.12419-5-larsxschneider@gmail.com","threadId":"43044","inReplyTo":"20160810130411.12419-1-larsxschneider@gmail.com","subject":"[PATCH v5 04/15] pkt-line: add packet_write_gently()","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-10T13:04:00Z","receivedAt":"2016-08-10T19:42:41Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\npacket_write() has two shortcomings. First, it uses format_packet() which\nlets the caller only send string data via \"%s\". That means it cannot be\nused for arbitrary data that may contain NULs. Second, it will always\ndie on error.\n\nAdd packet_write_gently() which writes arbitrary data and returns `0` for\nsuccess and `-1` for an error.\n\nSigned-off-by: Lars Schneider \n---\n pkt-line.c | 12 ++++++++++++\n pkt-line.h | 1 +\n 2 files changed, 13 insertions(+)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex e6b8410..4f25748 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -3,6 +3,7 @@\n #include \"run-command.h\"\n \n char packet_buffer[LARGE_PACKET_MAX];\n+char packet_write_buffer[LARGE_PACKET_MAX];\n static const char *packet_trace_prefix = \"git\";\n static struct trace_key trace_packet = TRACE_KEY_INIT(PACKET);\n static struct trace_key trace_pack = TRACE_KEY_INIT(PACKFILE);\n@@ -141,6 +142,17 @@ void packet_write(int fd, const char *fmt, ...)\n \twrite_or_die(fd, buf.buf, buf.len);\n }\n \n+int packet_write_gently(const int fd_out, const char *buf, size_t size)\n+{\n+\tif (size > PKTLINE_DATA_MAXLEN)\n+\t\treturn -1;\n+\tpacket_trace(buf, size, 1);\n+\tmemmove(packet_write_buffer + 4, buf, size);\n+\tsize += 4;\n+\tset_packet_header(packet_write_buffer, size);\n+\treturn (write_in_full(fd_out, packet_write_buffer, size) == size ? 0 : -1);\n+}\n+\n void packet_buf_write(struct strbuf *buf, const char *fmt, ...)\n {\n \tva_list args;\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 3cb9d91..88584f1 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -77,6 +77,7 @@ char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size);\n \n #define DEFAULT_PACKET_MAX 1000\n #define LARGE_PACKET_MAX 65520\n+#define PKTLINE_DATA_MAXLEN (LARGE_PACKET_MAX - 4)\n extern char packet_buffer[LARGE_PACKET_MAX];\n \n #endif\n-- \n2.9.2\n\n"},{"id":"293621","messageId":"20160810130411.12419-3-larsxschneider@gmail.com","threadId":"43044","inReplyTo":"20160810130411.12419-1-larsxschneider@gmail.com","subject":"[PATCH v5 02/15] pkt-line: call packet_trace() only if a packet is actually send","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-10T13:03:58Z","receivedAt":"2016-08-10T19:44:06Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\nThe packet_trace() call is not ideal in format_packet() as we would print\na trace when a packet is formatted and (potentially) when the packet is\nactually send. This was no problem up until now because format_packet()\nwas only used by one function. Fix it by moving the trace call into the\nfunction that actually sends the packet.\n\nSigned-off-by: Lars Schneider \n---\n pkt-line.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 177dc73..9400b47 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -121,7 +121,6 @@ static void format_packet(struct strbuf *out, const char *fmt, va_list args)\n \t\tdie(\"protocol error: impossibly long line\");\n \n \tset_packet_header(&out->buf[orig_len], n);\n-\tpacket_trace(out->buf + orig_len + 4, n - 4, 1);\n }\n \n void packet_write(int fd, const char *fmt, ...)\n@@ -133,6 +132,7 @@ void packet_write(int fd, const char *fmt, ...)\n \tva_start(args, fmt);\n \tformat_packet(&buf, fmt, args);\n \tva_end(args);\n+\tpacket_trace(buf.buf + 4, buf.len - 4, 1);\n \twrite_or_die(fd, buf.buf, buf.len);\n }\n \n-- \n2.9.2\n\n"},{"id":"293623","messageId":"434CB5D7-3FC0-4398-9028-135701121E55@gmail.com","threadId":"43044","inReplyTo":"20160810132814.gqnipsdwyzjmuqjy@sigill.intra.peff.net","subject":"Re: [PATCH v5 04/15] pkt-line: add packet_write_gently()","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-10T13:36:45Z","receivedAt":"2016-08-10T19:46:17Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 10 Aug 2016, at 15:28, Jeff King wrote:\n> \n> On Wed, Aug 10, 2016 at 03:04:00PM +0200, larsxschneider@gmail.com wrote:\n> \n>> From: Lars Schneider \n>> \n>> packet_write() has two shortcomings. First, it uses format_packet() which\n>> lets the caller only send string data via \"%s\". That means it cannot be\n>> used for arbitrary data that may contain NULs. Second, it will always\n>> die on error.\n>> \n>> Add packet_write_gently() which writes arbitrary data and returns `0` for\n>> success and `-1` for an error.\n> \n> So now we have packet_write() and packet_write_gently(), but they differ\n> in more than just whether they are gentle. That seems like a weird\n> interface.\n> \n> Should we either be picking a new name (e.g., packet_write_mem() or\n> something), or migrating packet_write() to packet_write_fmt()?\n\nDone in \"[PATCH v5 08/15] pkt-line: rename packet_write() to packet_write_fmt()\"\n\n\n>> diff --git a/pkt-line.c b/pkt-line.c\n>> index e6b8410..4f25748 100644\n>> --- a/pkt-line.c\n>> +++ b/pkt-line.c\n>> @@ -3,6 +3,7 @@\n>> #include \"run-command.h\"\n>> \n>> char packet_buffer[LARGE_PACKET_MAX];\n>> +char packet_write_buffer[LARGE_PACKET_MAX];\n> \n> Should this be static? I.e., are random other bits of the code allowed\n> to write into it (I guess not because it's not declared in pkt-line.h).\n\nstatic is better!\n\n\n>> +int packet_write_gently(const int fd_out, const char *buf, size_t size)\n>> +{\n>> +\tif (size > PKTLINE_DATA_MAXLEN)\n>> +\t\treturn -1;\n>> +\tpacket_trace(buf, size, 1);\n>> +\tmemmove(packet_write_buffer + 4, buf, size);\n> \n> It looks like this iteration drops the idea of callers using a\n> LARGE_PACKET_MAX buffer and only filling it at \"buf+4\" with\n> PKTLINE_DATA_MAXLEN bytes (which is fine).\n> \n> I wonder if we still need PKTLINE_DATA_MAXLEN, or of it is just\n> obscuring things. The magic number \"4\" still appears separately here,\n> and it actually makes it harder to see that things are correct. I.e.,\n> doing:\n> \n> if (size > sizeof(packet_write_buffer) - 4)\n> \treturn -1;\n> memmove(packet_write_buffer + 4, buf, size);\n> \n> is more obviously correct, because you do not have to wonder about the\n> relationship between the size of your buffer and the macro.\n> \n> It might still be worth having PKTLINE_DATA_MAXLEN public, though, if\n> callers use it to size their input to packet_write_gently().\n\nI agree. In a later patch I am using PKTLINE_DATA_MAXLEN inside pkt-line.c,\ntoo. I will change it to your suggestion.\n\nFor now I would remove PKTLINE_DATA_MAXLEN because it should be an implementation\ndetail of pkt-line.c (plus it is not used by anyone).\n\nThanks,\nLars"},{"id":"293624","messageId":"20160810130411.12419-14-larsxschneider@gmail.com","threadId":"43044","inReplyTo":"20160810130411.12419-1-larsxschneider@gmail.com","subject":"[PATCH v5 13/15] convert: make apply_filter() adhere to standard Git error handling","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-10T13:04:09Z","receivedAt":"2016-08-10T19:46:53Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\napply_filter() returns a boolean that tells the caller if it\n\"did convert or did not convert\". The variable `ret` was used throughout\nthe function to track errors wheras `1` denoted success and `0` failure.\nThis is unusual for the Git source where `0` denotes success.\n\nRename the variable and flip its value to make the function easier\nreadable for Git developers.\n\nSigned-off-by: Lars Schneider \n---\n convert.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 522e2c5..bd17340 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -436,7 +436,7 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,\n \t *\n \t * (child --> cmd) --> us\n \t */\n-\tint ret = 1;\n+\tint err = 0;\n \tstruct strbuf nbuf = STRBUF_INIT;\n \tstruct async async;\n \tstruct filter_params params;\n@@ -463,22 +463,22 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,\n \n \tif (strbuf_read(&nbuf, async.out, len) < 0) {\n \t\terror(\"read from external filter '%s' failed\", cmd);\n-\t\tret = 0;\n+\t\terr = -1;\n \t}\n \tif (close(async.out)) {\n \t\terror(\"read from external filter '%s' failed\", cmd);\n-\t\tret = 0;\n+\t\terr = -1;\n \t}\n \tif (finish_async(&async)) {\n \t\terror(\"external filter '%s' failed\", cmd);\n-\t\tret = 0;\n+\t\terr = -1;\n \t}\n \n-\tif (ret) {\n+\tif (!err) {\n \t\tstrbuf_swap(dst, &nbuf);\n \t}\n \tstrbuf_release(&nbuf);\n-\treturn ret;\n+\treturn !err;\n }\n \n static struct convert_driver {\n-- \n2.9.2\n\n"},{"id":"293625","messageId":"20160810130411.12419-4-larsxschneider@gmail.com","threadId":"43044","inReplyTo":"20160810130411.12419-1-larsxschneider@gmail.com","subject":"[PATCH v5 03/15] pkt-line: add `gentle` parameter to format_packet()","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-10T13:03:59Z","receivedAt":"2016-08-10T19:47:48Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\nformat_packet() dies if the caller wants to format a packet larger than\nLARGE_PACKET_MAX. Certain callers might prefer an error response instead.\n\nAdd a parameter `gentle` to define if the function should signal an error\nwith the return value (gentle=1) or die (gentle=0).\n\nSigned-off-by: Lars Schneider \n---\n pkt-line.c | 15 ++++++++++-----\n 1 file changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 9400b47..e6b8410 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -108,7 +108,7 @@ static void set_packet_header(char *buf, const int size)\n \t#undef hex\n }\n \n-static void format_packet(struct strbuf *out, const char *fmt, va_list args)\n+static int format_packet(int gentle, struct strbuf *out, const char *fmt, va_list args)\n {\n \tsize_t orig_len, n;\n \n@@ -117,10 +117,15 @@ static void format_packet(struct strbuf *out, const char *fmt, va_list args)\n \tstrbuf_vaddf(out, fmt, args);\n \tn = out->len - orig_len;\n \n-\tif (n > LARGE_PACKET_MAX)\n-\t\tdie(\"protocol error: impossibly long line\");\n+\tif (n > LARGE_PACKET_MAX) {\n+\t\tif (gentle)\n+\t\t\treturn -1;\n+\t\telse\n+\t\t\tdie(\"protocol error: impossibly long line\");\n+\t}\n \n \tset_packet_header(&out->buf[orig_len], n);\n+\treturn 0;\n }\n \n void packet_write(int fd, const char *fmt, ...)\n@@ -130,7 +135,7 @@ void packet_write(int fd, const char *fmt, ...)\n \n \tstrbuf_reset(&buf);\n \tva_start(args, fmt);\n-\tformat_packet(&buf, fmt, args);\n+\tformat_packet(0, &buf, fmt, args);\n \tva_end(args);\n \tpacket_trace(buf.buf + 4, buf.len - 4, 1);\n \twrite_or_die(fd, buf.buf, buf.len);\n@@ -141,7 +146,7 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...)\n \tva_list args;\n \n \tva_start(args, fmt);\n-\tformat_packet(buf, fmt, args);\n+\tformat_packet(0, buf, fmt, args);\n \tva_end(args);\n }\n \n-- \n2.9.2\n\n"},{"id":"293633","messageId":"4111F00D-74A9-449D-BDE9-EE170DA4A403@gmail.com","threadId":"43044","inReplyTo":"20160810131317.a4npld5hf2qk6wiq@sigill.intra.peff.net","subject":"Re: [PATCH v5 02/15] pkt-line: call packet_trace() only if a packet is actually send","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-10T13:24:38Z","receivedAt":"2016-08-10T20:01:31Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 10 Aug 2016, at 15:13, Jeff King wrote:\n> \n> On Wed, Aug 10, 2016 at 03:03:58PM +0200, larsxschneider@gmail.com wrote:\n> \n>> From: Lars Schneider \n>> \n>> The packet_trace() call is not ideal in format_packet() as we would print\n>> a trace when a packet is formatted and (potentially) when the packet is\n>> actually send. This was no problem up until now because format_packet()\n>> was only used by one function. Fix it by moving the trace call into the\n>> function that actually sends the packet.\n> \n> It looks like there are two functions: packet_write() and\n> packet_buf_write().\n\nI did not call trace in packet_buf_write() because this function does not\nperform any writes.\n\n\n> Your patch only touches one of them, and it looks like we would fail to\n> trace many packets (e.g., see receive-pack.c:report(), which uses\n> packet_buf_write() and then write()s out the result).\n\nI see. But isn't it confusing if packet_buf_write() issues a trace call?\nIf I just call this function then nothing happens at all. Shouldn't the\ntrace call be made in receive-pack.c:report() ? Or shouldn't receive-pack\nlet pkt-line.c perform the write calls?\n\n-Lars\n\n\n> PS Also, s/send/sent/ in the commit message.\n\nThanks :)\n"},{"id":"293641","messageId":"xmqqtwes5ysb.fsf@gitster.mtv.corp.google.com","threadId":"43044","inReplyTo":"20160810134003.q6mzgkcrwmkxv5fw@sigill.intra.peff.net","subject":"Re: [PATCH v5 04/15] pkt-line: add packet_write_gently()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-10T17:17:24Z","receivedAt":"2016-08-10T20:37:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Aug 10, 2016 at 03:36:45PM +0200, Lars Schneider wrote:\n>\n>> > So now we have packet_write() and packet_write_gently(), but they differ\n>> > in more than just whether they are gentle. That seems like a weird\n>> > interface.\n>> > \n>> > Should we either be picking a new name (e.g., packet_write_mem() or\n>> > something), or migrating packet_write() to packet_write_fmt()?\n>> \n>> Done in \"[PATCH v5 08/15] pkt-line: rename packet_write() to packet_write_fmt()\"\n>\n> Ah, OK. Generally I'd suggest to reorder things so that each patch looks\n> like a step forward (and so the early patches become preparatory steps,\n> and the justification in them is something like \"we're going to add more\n> write functions, so let's give this a more descriptive name\").\n\nI am guilty for saying \"packet_write() should have been similar to\nwrite(2)\". We may want to have a time-period during which there is\nno \"packet_write()\" in the codebase, before we get to that stage.\nI.e. rename it to packet_write_fmt() to vacate the name and add\npacket_write_mem(), and then later rename packet_write_mem() to its\nfinal name packet_write(), or something like that. The two-step\nprocess would reduce the chance of misconversion.\n"},{"id":"293642","messageId":"xmqqpopg5yqf.fsf@gitster.mtv.corp.google.com","threadId":"43044","inReplyTo":"20160810134346.6nmf2sudwh56nq76@sigill.intra.peff.net","subject":"Re: [PATCH v5 05/15] pkt-line: add packet_write_gently_fmt()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-10T17:18:32Z","receivedAt":"2016-08-10T20:39:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Aug 10, 2016 at 03:04:01PM +0200, larsxschneider@gmail.com wrote:\n>\n>> +int packet_write_gently_fmt(int fd, const char *fmt, ...)\n>> +{\n>> +\tstatic struct strbuf buf = STRBUF_INIT;\n>> +\tva_list args;\n>> +\n>> +\tstrbuf_reset(&buf);\n>> +\tva_start(args, fmt);\n>> +\tformat_packet(1, &buf, fmt, args);\n>> +\tva_end(args);\n>> +\tpacket_trace(buf.buf + 4, buf.len - 4, 1);\n>> +\treturn (write_in_full(fd, buf.buf, buf.len) == buf.len ? 0 : -1);\n>> +}\n>\n> Could the end of this function just be:\n>\n> return packet_write_gently(fd, buf.buf, buf.len);\n>\n> ? I guess we'd prefer to avoid that, because it incurs an extra\n> memmove() of the data.\n>\n> Similarly, I'd think this could share code with the non-gentle form\n> (which should be able to just call this and die() if returns an error).\n> Though sometimes the va_list transformation makes that awkward.\n\nYes.\n\nAlso regarding the naming, please have \"_gently\" at the end; that is\nhow all other function families with _gently variant are named, I\nthink.\n"},{"id":"293646","messageId":"982CD9A4-E155-4DEE-9B47-BCEA4F0C493C@gmail.com","threadId":"43044","inReplyTo":"20160810133745.wagccvvf35o3pbwb@sigill.intra.peff.net","subject":"Re: [PATCH v5 03/15] pkt-line: add `gentle` parameter to format_packet()","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-10T13:59:19Z","receivedAt":"2016-08-10T21:14:58Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 10 Aug 2016, at 15:37, Jeff King wrote:\n> \n> On Wed, Aug 10, 2016 at 03:29:26PM +0200, Lars Schneider wrote:\n> \n>> \n>>> On 10 Aug 2016, at 15:15, Jeff King wrote:\n>>> \n>>> On Wed, Aug 10, 2016 at 03:03:59PM +0200, larsxschneider@gmail.com wrote:\n>>> \n>>>> From: Lars Schneider \n>>>> \n>>>> format_packet() dies if the caller wants to format a packet larger than\n>>>> LARGE_PACKET_MAX. Certain callers might prefer an error response instead.\n>>> \n>>> I am not sure I agree here. Certainly I see the usefulness of gently\n>>> handling a failure to write(). But if you are passing in too-large\n>>> buffers, isn't that a bug in the program?\n>>> \n>>> How would you recover, except by splitting up the content? That might\n>>> not be possible depending on how you are using the pkt-lines. And even\n>>> if it is, wouldn't it be simpler to split it up before sending it to\n>>> format_packet()?\n>> \n>> Good argument. I agree - this patch should be dropped.\n> \n> Actually, after reading further, one thought did occur to me. Let's say\n> you are writing to a smudge filter, and one of the header packets you\n> send has the filename in it. So you might do something like:\n> \n> if (packet_write_fmt_gently(fd, \"filename=%s\", filename) < 0) {\n> \tif (filter_required)\n> \t\tdie(...);\n> \telse\n> \t\treturn -1; /* we tried our best; skip smudge */\n> }\n> \n> The \"recovery\" there is not to try sending again, but rather to give up.\n> And that is presumably a sane outcome for somebody who tries to checkout\n> a filename larger than 64K.\n\nYes!\n\n\n> It does still feel a little weird that you cannot tell the difference\n> between a write() error and bad input. Because you really might want to\n> do something different between the two. Like:\n> \n> #define MAX_FILENAME (PKTLINE_DATA_MAXLEN - strlen(\"filename\"))\n> \n> if (filename > MAX_FILENAME) {\n> \twarning(\"woah, that name is ridiculous; truncating\");\n> \tret = packet_write_fmt_gently(fd, \"%.*s\", MAX_FILENAME, filename);\n> } else\n> \tret = packet_write_fmt_gently(fd, \"%s\", filename);\n\n\nI can do that. However, I wouldn't truncate the filename as this\nmight create a weird outcome. I would just let the filter fail.\n\nOK?\n\n- Lars"},{"id":"293680","messageId":"E6D0E830-764D-42E5-9F69-340F718B98B2@gmail.com","threadId":"43044","inReplyTo":"20160810134346.6nmf2sudwh56nq76@sigill.intra.peff.net","subject":"Re: [PATCH v5 05/15] pkt-line: add packet_write_gently_fmt()","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-10T14:10:02Z","receivedAt":"2016-08-10T23:16:06Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 10 Aug 2016, at 15:43, Jeff King wrote:\n> \n> On Wed, Aug 10, 2016 at 03:04:01PM +0200, larsxschneider@gmail.com wrote:\n> \n>> +int packet_write_gently_fmt(int fd, const char *fmt, ...)\n>> +{\n>> +\tstatic struct strbuf buf = STRBUF_INIT;\n>> +\tva_list args;\n>> +\n>> +\tstrbuf_reset(&buf);\n>> +\tva_start(args, fmt);\n>> +\tformat_packet(1, &buf, fmt, args);\n>> +\tva_end(args);\n>> +\tpacket_trace(buf.buf + 4, buf.len - 4, 1);\n>> +\treturn (write_in_full(fd, buf.buf, buf.len) == buf.len ? 0 : -1);\n>> +}\n> \n> Could the end of this function just be:\n> \n> return packet_write_gently(fd, buf.buf, buf.len);\n> \n> ? I guess we'd prefer to avoid that, because it incurs an extra\n> memmove() of the data.\n\nI don't think the memmove would be that expensive. However, format_packet()\nalready creates the packet_header and packet_write_gently would do the same\nagain, no?\n\n\n> Similarly, I'd think this could share code with the non-gentle form\n> (which should be able to just call this and die() if returns an error).\n> Though sometimes the va_list transformation makes that awkward.\n\nYeah, the code duplication annoyed me, too. va_list was the reason I did it\nthat way. Do you think that is something that needs to be addressed in the\nseries?\n\nThanks,\nLars\n"},{"id":"298863","messageId":"CAGZ79kboxgBRHSa2s7CKZ1Uo=13WT=rT8VHCNJNj_Q9jQzZAYw@mail.gmail.com","threadId":"43044","inReplyTo":"20160810130411.12419-15-larsxschneider@gmail.com","subject":"Re: [PATCH v5 14/15] convert: add filter..process option","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-12T16:33:18Z","receivedAt":"2016-08-12T16:33:27Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Aug 10, 2016 at 6:04 AM, wrote:\n> From: Lars Schneider \n>\n> Git's clean/smudge mechanism invokes an external filter process for every\n> single blob that is affected by a filter. If Git filters a lot of blobs\n> then the startup time of the external filter processes can become a\n> significant part of the overall Git execution time.\n>\n> In a preliminary performance test this developer used a clean/smudge filter\n> written in golang to filter 12,000 files. This process took 364s with the\n> existing filter mechanism and 5s with the new mechanism. See details here:\n> https://github.com/github/git-lfs/pull/1382\n>\n> This patch adds the `filter..process` string option which, if used,\n> keeps the external filter process running and processes all blobs with\n> the packet format (pkt-line) based protocol over standard input and standard\n> output described below.\n>\n> Git starts the filter when it encounters the first file\n> that needs to be cleaned or smudged. After the filter started\n> Git sends a welcome message, a list of supported protocol\n> version numbers, and a flush packet. Git expects to read the\n> welcome message and one protocol version number from the\n> previously sent list. Afterwards Git sends a list of supported\n> capabilities and a flush packet. Git expects to read a list of\n> desired capabilities, which must be a subset of the supported\n> capabilities list, and a flush packet as response:\n> ------------------------\n> packet: git> git-filter-client\n> packet: git> version=2\n> packet: git> version=42\n> packet: git> 0000\n> packet: git< git-filter-server\n> packet: git< version=2\n\nwhat follows is specific to version=2?\nversion 42 may deem capabilities a bad idea?\n\n> packet: git> clean=true\n> packet: git> smudge=true\n> packet: git> not-yet-invented=true\n> packet: git> 0000\n> packet: git< clean=true\n> packet: git< smudge=true\n> packet: git< 0000\n> ------------------------\n> Supported filter capabilities in version 2 are \"clean\" and\n> \"smudge\".\n\nI assume version 2 is an example here and we actually start with v1?\n\nCan you clarify why we need welcome messages?\n(Is there a technical reason, or better debuggability for humans?)\n\n>\n> Afterwards Git sends a list of \"key=value\" pairs terminated with\n> a flush packet. The list will contain at least the filter command\n> (based on the supported capabilities) and the pathname of the file\n> to filter relative to the repository root. Right after these packets\n> Git sends the content split in zero or more pkt-line packets and a\n> flush packet to terminate content.\n> ------------------------\n> packet: git> command=smudge\\n\n> packet: git> pathname=path/testfile.dat\\n\n> packet: git> 0000\n> packet: git> CONTENT\n> packet: git> 0000\n> ------------------------\n>\n> The filter is expected to respond with a list of \"key=value\" pairs\n> terminated with a flush packet. If the filter does not experience\n> problems then the list must contain a \"success\" status. Right after\n> these packets the filter is expected to send the content in zero\n> or more pkt-line packets and a flush packet at the end. Finally, a\n> second list of \"key=value\" pairs terminated with a flush packet\n> is expected. The filter can change the status in the second list.\n> ------------------------\n> packet: git< status=success\\n\n> packet: git< 0000\n> packet: git< SMUDGED_CONTENT\n> packet: git< 0000\n> packet: git< 0000 # empty list!\n> ------------------------\n>\n> If the result content is empty then the filter is expected to respond\n> with a success status and an empty list.\n> ------------------------\n> packet: git< status=success\\n\n> packet: git< 0000\n> packet: git< 0000 # empty content!\n> packet: git< 0000 # empty list!\n> ------------------------\n\nWhy do we need the last flush packet? We'd expect as many successes\nas we send out contents? Do we plan on interleaving operation, i.e.\nGit sends out 10 files but the filter process is not as fast as Git sending\nout and the answers trickle in slowly?\n\n>\n> In case the filter cannot or does not want to process the content,\n> it is expected to respond with an \"error\" status. Depending on the\n> `filter..required` flag Git will interpret that as error\n> but it will not stop or restart the filter process.\n> ------------------------\n> packet: git< status=error\\n\n> packet: git< 0000\n> ------------------------\n>\n> In case the filter cannot or does not want to process the content\n> as well as any future content for the lifetime of the Git process,\n> it is expected to respond with an \"error-all\" status. Depending on\n> the `filter..required` flag Git will interpret that as error\n> but it will not stop or restart the filter process.\n> ------------------------\n> packet: git< status=error-all\\n\n> packet: git< 0000\n> ------------------------\n>\n> If the filter experiences an error during processing, then it can\n> send the status \"error\". Depending on the `filter..required`\n> flag Git will interpret that as error but it will not stop or restart\n> the filter process.\n> ------------------------\n> packet: git< status=success\\n\n\nSo the first success is meaningless essentially?\nWould it make sense to move the sucess behind the content sending\nin all cases?\n\n> packet: git< 0000\n> packet: git< HALF_WRITTEN_ERRONEOUS_CONTENT\n> packet: git< 0000\n> packet: git< status=error\\n\n> packet: git< 0000\n> ------------------------\n>\n> If the filter dies during the communication or does not adhere to\n> the protocol then Git will stop the filter process and restart it\n> with the next file that needs to be processed.\n>\n> After the filter has processed a blob it is expected to wait for\n> the next \"key=value\" list containing a command. When the Git process\n> terminates, it will send a kill signal to the filter in that stage.\n>\n> If a `filter..clean` or `filter..smudge` command\n> is configured then these commands always take precedence over\n> a configured `filter..process` command.\n\nokay. I think you can omit most of the commit message as it is a duplicate\nof the documentation?\n\nInstead the commit message can answer questions that are not part of\nthe documentation. (See the questions above which can be summarized\nas \"Why do we do it this way and not differently?\")\n\n\n> + if (err || errno == EPIPE) {\n> + if (!strcmp(filter_status.buf, \"error\")) {\n> + /*\n> + * The filter signaled a problem with the file.\n> + */\n\n/* This could go into a single line comment. */\n\n> + } else if (!strcmp(filter_status.buf, \"error-all\")) {\n> + /*\n> + * The filter signaled a permanent problem. Don't try to filter\n> + * files with the same command for the lifetime of the current\n> + * Git process.\n> + */\n> + entry->supported_capabilities &= ~wanted_capability;\n> + } else {\n> + /*\n> + * Something went wrong with the protocol filter.\n> + * Force shutdown and restart if another blob requires filtering!\n> + */\n> + error(\"external filter '%s' failed\", cmd);\n\nfailed .. Can you give more information to the user such that they can easier\ndebug? (blob/path or state / expected state)\n\n\n> +\n> static int read_convert_config(const char *var, const char *value, void *cb)\n> {\n> const char *key, *name;\n> @@ -526,6 +818,10 @@ static int read_convert_config(const char *var, const char *value, void *cb)\n> if (!strcmp(\"clean\", key))\n> return git_config_string(&drv->clean, var, value);\n>\n> + if (!strcmp(\"process\", key)) {\n> + return git_config_string(&drv->process, var, value);\n> + }\n\noptional nit: braces unnecessary\n\nThanks,\nStefan\n"},{"id":"298864","messageId":"20160812163809.3wdkuqegxfjam2yn@sigill.intra.peff.net","threadId":"43044","inReplyTo":"CAGZ79kboxgBRHSa2s7CKZ1Uo=13WT=rT8VHCNJNj_Q9jQzZAYw@mail.gmail.com","subject":"Re: [PATCH v5 14/15] convert: add filter..process option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-12T16:38:09Z","receivedAt":"2016-08-12T16:38:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 12, 2016 at 09:33:18AM -0700, Stefan Beller wrote:\n\n> > If the result content is empty then the filter is expected to respond\n> > with a success status and an empty list.\n> > ------------------------\n> > packet: git< status=success\\n\n> > packet: git< 0000\n> > packet: git< 0000 # empty content!\n> > packet: git< 0000 # empty list!\n> > ------------------------\n> \n> Why do we need the last flush packet? We'd expect as many successes\n> as we send out contents? Do we plan on interleaving operation, i.e.\n> Git sends out 10 files but the filter process is not as fast as Git sending\n> out and the answers trickle in slowly?\n\nThere was prior discussion in the thread, but basically, it is there to\nbe able to signal an error that is encountered midway through sending\nthe file (i.e., to say \"status=error\"). If you do not have a final\nflush, then you would send nothing, and the receiver would be left\nwondering if you were successful, or if it simply did not get your error\nreport yet.\n\n> > If the filter experiences an error during processing, then it can\n> > send the status \"error\". Depending on the `filter..required`\n> > flag Git will interpret that as error but it will not stop or restart\n> > the filter process.\n> > ------------------------\n> > packet: git< status=success\\n\n> \n> So the first success is meaningless essentially?\n> Would it make sense to move the sucess behind the content sending\n> in all cases?\n\nNo, the first success says \"good so far, here's the file content\". The\nsecond says \"I succeeded in sending you the file content\".\n\nYou _can_ drop the first one, but it may be more convenient for the\nreceiver to know up-front that there was a failure.\n\n-Peff\n"},{"id":"298868","messageId":"CAGZ79kaRKROUganF838w29rCkJ592sZvu+q9fo+h4a4sPeDXMQ@mail.gmail.com","threadId":"43044","inReplyTo":"20160812163809.3wdkuqegxfjam2yn@sigill.intra.peff.net","subject":"Re: [PATCH v5 14/15] convert: add filter..process option","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-12T16:48:21Z","receivedAt":"2016-08-12T16:48:26Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Aug 12, 2016 at 9:38 AM, Jeff King wrote:\n> On Fri, Aug 12, 2016 at 09:33:18AM -0700, Stefan Beller wrote:\n>\n>> > If the result content is empty then the filter is expected to respond\n>> > with a success status and an empty list.\n>> > ------------------------\n>> > packet: git< status=success\\n\n>> > packet: git< 0000\n>> > packet: git< 0000 # empty content!\n>> > packet: git< 0000 # empty list!\n>> > ------------------------\n>>\n>> Why do we need the last flush packet? We'd expect as many successes\n>> as we send out contents? Do we plan on interleaving operation, i.e.\n>> Git sends out 10 files but the filter process is not as fast as Git sending\n>> out and the answers trickle in slowly?\n>\n> There was prior discussion in the thread, but basically, it is there to\n> be able to signal an error that is encountered midway through sending\n> the file (i.e., to say \"status=error\"). If you do not have a final\n> flush, then you would send nothing, and the receiver would be left\n> wondering if you were successful, or if it simply did not get your error\n> report yet.\n\n I did not follow the prior discussion, so I approached this review with\n no prior knowledge from prior reviews, but instead read through and\n was asking a lot of questions that came up immediately. In case my\n questions are too dumb just omit them, but I thought they were good\n material to answer in a commit message (\"Why did we do it this way\n and not differently\").\n\nThanks for the explanation. So this is similar as the situation below\nthat we wait for the flush and then an error/success report?\n\n>\n>> > If the filter experiences an error during processing, then it can\n>> > send the status \"error\". Depending on the `filter..required`\n>> > flag Git will interpret that as error but it will not stop or restart\n>> > the filter process.\n>> > ------------------------\n>> > packet: git< status=success\\n\n>>\n>> So the first success is meaningless essentially?\n>> Would it make sense to move the sucess behind the content sending\n>> in all cases?\n>\n> No, the first success says \"good so far, here's the file content\". The\n> second says \"I succeeded in sending you the file content\".\n>\n> You _can_ drop the first one, but it may be more convenient for the\n> receiver to know up-front that there was a failure.\n\n\nIf there was a failure upfront, it would become\n\npacket: git< 0000\n# no content is encapsulated here\npacket: git< 0000\npacket: git< status=error\\n\npacket: git< 0000\n\nso from a protocol side I'd claim it doesn't look bad.\nI assume with convenient you mean the implementation\nside of things?\n\nIf we do the success first and then error out halfway, we\nstill have to clean up, so I do not see how this impacts\nimplementation?\n\nThanks,\nStefan\n"},{"id":"298869","messageId":"509A907F-B1B5-4244-B1C7-F1190296208D@gmail.com","threadId":"43044","inReplyTo":"CAGZ79kboxgBRHSa2s7CKZ1Uo=13WT=rT8VHCNJNj_Q9jQzZAYw@mail.gmail.com","subject":"Re: [PATCH v5 14/15] convert: add filter..process option","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-12T16:59:18Z","receivedAt":"2016-08-12T16:59:34Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 12 Aug 2016, at 18:33, Stefan Beller wrote:\n> \n> On Wed, Aug 10, 2016 at 6:04 AM, wrote:\n>> From: Lars Schneider \n>> \n>> Git's clean/smudge mechanism invokes an external filter process for every\n>> single blob that is affected by a filter. If Git filters a lot of blobs\n>> then the startup time of the external filter processes can become a\n>> significant part of the overall Git execution time.\n>> \n>> In a preliminary performance test this developer used a clean/smudge filter\n>> written in golang to filter 12,000 files. This process took 364s with the\n>> existing filter mechanism and 5s with the new mechanism. See details here:\n>> https://github.com/github/git-lfs/pull/1382\n>> \n>> This patch adds the `filter..process` string option which, if used,\n>> keeps the external filter process running and processes all blobs with\n>> the packet format (pkt-line) based protocol over standard input and standard\n>> output described below.\n>> \n>> Git starts the filter when it encounters the first file\n>> that needs to be cleaned or smudged. After the filter started\n>> Git sends a welcome message, a list of supported protocol\n>> version numbers, and a flush packet. Git expects to read the\n>> welcome message and one protocol version number from the\n>> previously sent list. Afterwards Git sends a list of supported\n>> capabilities and a flush packet. Git expects to read a list of\n>> desired capabilities, which must be a subset of the supported\n>> capabilities list, and a flush packet as response:\n>> ------------------------\n>> packet: git> git-filter-client\n>> packet: git> version=2\n>> packet: git> version=42\n>> packet: git> 0000\n>> packet: git< git-filter-server\n>> packet: git< version=2\n> \n> what follows is specific to version=2?\n> version 42 may deem capabilities a bad idea?\n\n\"version=42\" is just an example to show how the initialization could look\nlike in a distant future when we support even another protocol version.\n\nYou are correct, what follows is specific to version=2. I will state\nthat more clearly in the documentation.\n\nCan you try to rephrase \"version 42 may deem capabilities a bad idea?\"\nI am not sure I understand what you mean.\n\n\n> \n>> packet: git> clean=true\n>> packet: git> smudge=true\n>> packet: git> not-yet-invented=true\n>> packet: git> 0000\n>> packet: git< clean=true\n>> packet: git< smudge=true\n>> packet: git< 0000\n>> ------------------------\n>> Supported filter capabilities in version 2 are \"clean\" and\n>> \"smudge\".\n> \n> I assume version 2 is an example here and we actually start with v1?\n\nNo, it is actually called version 2 because I consider the current\nclean/smudge protocol version 1.\n\n\n> Can you clarify why we need welcome messages?\n> (Is there a technical reason, or better debuggability for humans?)\n\nThe welcome message is necessary to distinguish the long running\nfilter protocol (v2) from the current one-shot filter protocol (v1).\nThis is becomes important if a users tries to use a v1 clean/smudge\nfilter with the v2 git config settings.\n\n\n>> Afterwards Git sends a list of \"key=value\" pairs terminated with\n>> a flush packet. The list will contain at least the filter command\n>> (based on the supported capabilities) and the pathname of the file\n>> to filter relative to the repository root. Right after these packets\n>> Git sends the content split in zero or more pkt-line packets and a\n>> flush packet to terminate content.\n>> ------------------------\n>> packet: git> command=smudge\\n\n>> packet: git> pathname=path/testfile.dat\\n\n>> packet: git> 0000\n>> packet: git> CONTENT\n>> packet: git> 0000\n>> ------------------------\n>> \n>> The filter is expected to respond with a list of \"key=value\" pairs\n>> terminated with a flush packet. If the filter does not experience\n>> problems then the list must contain a \"success\" status. Right after\n>> these packets the filter is expected to send the content in zero\n>> or more pkt-line packets and a flush packet at the end. Finally, a\n>> second list of \"key=value\" pairs terminated with a flush packet\n>> is expected. The filter can change the status in the second list.\n>> ------------------------\n>> packet: git< status=success\\n\n>> packet: git< 0000\n>> packet: git< SMUDGED_CONTENT\n>> packet: git< 0000\n>> packet: git< 0000 # empty list!\n>> ------------------------\n>> \n>> If the result content is empty then the filter is expected to respond\n>> with a success status and an empty list.\n>> ------------------------\n>> packet: git< status=success\\n\n>> packet: git< 0000\n>> packet: git< 0000 # empty content!\n>> packet: git< 0000 # empty list!\n>> ------------------------\n> \n> Why do we need the last flush packet? We'd expect as many successes\n> as we send out contents? Do we plan on interleaving operation, i.e.\n> Git sends out 10 files but the filter process is not as fast as Git sending\n> out and the answers trickle in slowly?\n\nGit filter processes run sequentially right now (unfortunately).\n\nre flush: please see Peff's answer:\nhttp://public-inbox.org/git/20160812163809.3wdkuqegxfjam2yn%40sigill.intra.peff.net/\n\n\n>> In case the filter cannot or does not want to process the content,\n>> it is expected to respond with an \"error\" status. Depending on the\n>> `filter..required` flag Git will interpret that as error\n>> but it will not stop or restart the filter process.\n>> ------------------------\n>> packet: git< status=error\\n\n>> packet: git< 0000\n>> ------------------------\n>> \n>> In case the filter cannot or does not want to process the content\n>> as well as any future content for the lifetime of the Git process,\n>> it is expected to respond with an \"error-all\" status. Depending on\n>> the `filter..required` flag Git will interpret that as error\n>> but it will not stop or restart the filter process.\n>> ------------------------\n>> packet: git< status=error-all\\n\n>> packet: git< 0000\n>> ------------------------\n>> \n>> If the filter experiences an error during processing, then it can\n>> send the status \"error\". Depending on the `filter..required`\n>> flag Git will interpret that as error but it will not stop or restart\n>> the filter process.\n>> ------------------------\n>> packet: git< status=success\\n\n> \n> So the first success is meaningless essentially?\n> Would it make sense to move the sucess behind the content sending\n> in all cases?\n\nAgain, I refer to Peff's answer.\n\n\n>> packet: git< 0000\n>> packet: git< HALF_WRITTEN_ERRONEOUS_CONTENT\n>> packet: git< 0000\n>> packet: git< status=error\\n\n>> packet: git< 0000\n>> ------------------------\n>> \n>> If the filter dies during the communication or does not adhere to\n>> the protocol then Git will stop the filter process and restart it\n>> with the next file that needs to be processed.\n>> \n>> After the filter has processed a blob it is expected to wait for\n>> the next \"key=value\" list containing a command. When the Git process\n>> terminates, it will send a kill signal to the filter in that stage.\n>> \n>> If a `filter..clean` or `filter..smudge` command\n>> is configured then these commands always take precedence over\n>> a configured `filter..process` command.\n> \n> okay. I think you can omit most of the commit message as it is a duplicate\n> of the documentation?\n\nYes it duplicates the documentation. \n\n\n> Instead the commit message can answer questions that are not part of\n> the documentation. (See the questions above which can be summarized\n> as \"Why do we do it this way and not differently?\")\n\nOK, point taken. I will write a new commit message for v6.\n\n\n> \n>> + if (err || errno == EPIPE) {\n>> + if (!strcmp(filter_status.buf, \"error\")) {\n>> + /*\n>> + * The filter signaled a problem with the file.\n>> + */\n> \n> /* This could go into a single line comment. */\n\nOK, will change.\n\n\n>> + } else if (!strcmp(filter_status.buf, \"error-all\")) {\n>> + /*\n>> + * The filter signaled a permanent problem. Don't try to filter\n>> + * files with the same command for the lifetime of the current\n>> + * Git process.\n>> + */\n>> + entry->supported_capabilities &= ~wanted_capability;\n>> + } else {\n>> + /*\n>> + * Something went wrong with the protocol filter.\n>> + * Force shutdown and restart if another blob requires filtering!\n>> + */\n>> + error(\"external filter '%s' failed\", cmd);\n> \n> failed .. Can you give more information to the user such that they can easier\n> debug? (blob/path or state / expected state)\n\nAgreed, will add!\nHowever, we don't give this information with the current clean/smudge interface.\n\n\n>> +\n>> static int read_convert_config(const char *var, const char *value, void *cb)\n>> {\n>> const char *key, *name;\n>> @@ -526,6 +818,10 @@ static int read_convert_config(const char *var, const char *value, void *cb)\n>> if (!strcmp(\"clean\", key))\n>> return git_config_string(&drv->clean, var, value);\n>> \n>> + if (!strcmp(\"process\", key)) {\n>> + return git_config_string(&drv->process, var, value);\n>> + }\n> \n> optional nit: braces unnecessary\n\nAgreed, will remove!\n\n\nThanks a lot for the review,\nLars"},{"id":"298870","messageId":"CAGZ79kYL5ajdyJK8Yimy2O3b=x=46D2VuNgm2aZ5nD703y1A3Q@mail.gmail.com","threadId":"43044","inReplyTo":"509A907F-B1B5-4244-B1C7-F1190296208D@gmail.com","subject":"Re: [PATCH v5 14/15] convert: add filter..process option","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-12T17:07:11Z","receivedAt":"2016-08-12T17:07:16Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Aug 12, 2016 at 9:59 AM, Lars Schneider\n wrote:\n>\n> The welcome message is necessary to distinguish the long running\n> filter protocol (v2) from the current one-shot filter protocol (v1).\n> This is becomes important if a users tries to use a v1 clean/smudge\n> filter with the v2 git config settings.\n\nOh I see, that's why we're at v2 now.\nHow do you distinguish between v1 and v2? Does the welcome message\nneed to follow a certain pattern to be recognized to make it v2+ ?\n\n>\n> Thanks a lot for the review,\n> Lars\n\nSorry for repeating the questions (it seems I missed\na lot of the prior discussion), but I think these questions\nmay help future readers of the commit message,\n\nThanks,\nStefan\n"},{"id":"298871","messageId":"52049A60-6CEB-40E5-A013-409CFC20252F@gmail.com","threadId":"43044","inReplyTo":"CAGZ79kaRKROUganF838w29rCkJ592sZvu+q9fo+h4a4sPeDXMQ@mail.gmail.com","subject":"Re: [PATCH v5 14/15] convert: add filter..process option","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-12T17:08:51Z","receivedAt":"2016-08-12T17:09:08Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 12 Aug 2016, at 18:48, Stefan Beller wrote:\n> \n> On Fri, Aug 12, 2016 at 9:38 AM, Jeff King wrote:\n>> On Fri, Aug 12, 2016 at 09:33:18AM -0700, Stefan Beller wrote:\n>> \n>>>> If the result content is empty then the filter is expected to respond\n>>>> with a success status and an empty list.\n>>>> ------------------------\n>>>> packet: git< status=success\\n\n>>>> packet: git< 0000\n>>>> packet: git< 0000 # empty content!\n>>>> packet: git< 0000 # empty list!\n>>>> ------------------------\n>>> \n>>> Why do we need the last flush packet? We'd expect as many successes\n>>> as we send out contents? Do we plan on interleaving operation, i.e.\n>>> Git sends out 10 files but the filter process is not as fast as Git sending\n>>> out and the answers trickle in slowly?\n>> \n>> There was prior discussion in the thread, but basically, it is there to\n>> be able to signal an error that is encountered midway through sending\n>> the file (i.e., to say \"status=error\"). If you do not have a final\n>> flush, then you would send nothing, and the receiver would be left\n>> wondering if you were successful, or if it simply did not get your error\n>> report yet.\n> \n> I did not follow the prior discussion, so I approached this review with\n> no prior knowledge from prior reviews, but instead read through and\n> was asking a lot of questions that came up immediately. In case my\n> questions are too dumb just omit them, but I thought they were good\n> material to answer in a commit message (\"Why did we do it this way\n> and not differently\").\n\nThanks! That's very helpful and I will address your questions in the commit\nmessage as anyone looking at this commit in the future will have no prior \nknowledge, too.\n\n\n> Thanks for the explanation. So this is similar as the situation below\n> that we wait for the flush and then an error/success report?\n\nCorrect. If we would just process the status packet then we wouldn't\neven need to wait for the flush. I added flush because that allows us\nto send an arbitrary list of key=value pairs in the future.\n\n\n>>>> If the filter experiences an error during processing, then it can\n>>>> send the status \"error\". Depending on the `filter..required`\n>>>> flag Git will interpret that as error but it will not stop or restart\n>>>> the filter process.\n>>>> ------------------------\n>>>> packet: git< status=success\\n\n>>> \n>>> So the first success is meaningless essentially?\n>>> Would it make sense to move the sucess behind the content sending\n>>> in all cases?\n>> \n>> No, the first success says \"good so far, here's the file content\". The\n>> second says \"I succeeded in sending you the file content\".\n>> \n>> You _can_ drop the first one, but it may be more convenient for the\n>> receiver to know up-front that there was a failure.\n> \n> \n> If there was a failure upfront, it would become\n> \n> packet: git< 0000\n> # no content is encapsulated here\n> packet: git< 0000\n> packet: git< status=error\\n\n> packet: git< 0000\n\nNo, a failure upfront would look like this (see documentation):\n\n------------------------\npacket: git< status=error\\n\npacket: git< 0000\n------------------------\n\nNo content and no 2nd key=value pair list is exchanged after an error.\n\n\n\n> so from a protocol side I'd claim it doesn't look bad.\n> I assume with convenient you mean the implementation\n> side of things?\n> \n> If we do the success first and then error out halfway, we\n> still have to clean up, so I do not see how this impacts\n> implementation?\n\nThat is true. The reasoning is that an error in between is somewhat\nless expected. Therefore additional work is OK.\n\nAn error upfront is much more likely because it is also a mechanism\nfor the filter to reject certain files. If the filter is configured\nas \"required=false\" then this reject would actually be OK.\n\nThanks,\nLars"},{"id":"298872","messageId":"xmqqinv5ykoo.fsf@gitster.mtv.corp.google.com","threadId":"43044","inReplyTo":"52049A60-6CEB-40E5-A013-409CFC20252F@gmail.com","subject":"Re: [PATCH v5 14/15] convert: add filter..process option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-12T17:13:43Z","receivedAt":"2016-08-12T17:13:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n>> If we do the success first and then error out halfway, we\n>> still have to clean up, so I do not see how this impacts\n>> implementation?\n>\n> That is true. The reasoning is that an error in between is somewhat\n> less expected. Therefore additional work is OK.\n>\n> An error upfront is much more likely because it is also a mechanism\n> for the filter to reject certain files. If the filter is configured\n> as \"required=false\" then this reject would actually be OK.\n\nUnless the reasoning is \"an error in between is so rare that we are\nOK if the protocol misbehaves and the receiving end omits error\nhanding\", I am not so sure how \"therefore additional work is OK\" is\na reasonable conclusion.\n"},{"id":"298873","messageId":"3605A4A8-468D-4EBF-B834-07D27F841AB2@gmail.com","threadId":"43044","inReplyTo":"CAGZ79kYL5ajdyJK8Yimy2O3b=x=46D2VuNgm2aZ5nD703y1A3Q@mail.gmail.com","subject":"Re: [PATCH v5 14/15] convert: add filter..process option","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-12T17:14:00Z","receivedAt":"2016-08-12T17:14:07Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 12 Aug 2016, at 19:07, Stefan Beller wrote:\n> \n> On Fri, Aug 12, 2016 at 9:59 AM, Lars Schneider\n> wrote:\n>> \n>> The welcome message is necessary to distinguish the long running\n>> filter protocol (v2) from the current one-shot filter protocol (v1).\n>> This is becomes important if a users tries to use a v1 clean/smudge\n>> filter with the v2 git config settings.\n> \n> Oh I see, that's why we're at v2 now.\n> How do you distinguish between v1 and v2? Does the welcome message\n> need to follow a certain pattern to be recognized to make it v2+ ?\n\nv1 has no format at all. It works like this:\n\n1. Git starts the filter process\n2. Git writes the entire file via stdin to the filter process\n3. Git reads the result via stdout from the filter process\n3. Git stops the filter process\n\n\nAny v2+ would need to deal with the following:\n\npacket: git> git-filter-client\npacket: git> version=2\npacket: git> version=2+\npacket: git> 0000\npacket: git< git-filter-server\n\nEverything after could be different in v2+ compared to v2.\n\nThanks,\nLars\n"},{"id":"298874","messageId":"EF153355-2DFB-41AE-A298-CCBE69184294@gmail.com","threadId":"43044","inReplyTo":"xmqqinv5ykoo.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v5 14/15] convert: add filter..process option","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-12T17:21:26Z","receivedAt":"2016-08-12T17:21:33Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 12 Aug 2016, at 19:13, Junio C Hamano wrote:\n> \n> Lars Schneider writes:\n> \n>>> If we do the success first and then error out halfway, we\n>>> still have to clean up, so I do not see how this impacts\n>>> implementation?\n>> \n>> That is true. The reasoning is that an error in between is somewhat\n>> less expected. Therefore additional work is OK.\n>> \n>> An error upfront is much more likely because it is also a mechanism\n>> for the filter to reject certain files. If the filter is configured\n>> as \"required=false\" then this reject would actually be OK.\n> \n> Unless the reasoning is \"an error in between is so rare that we are\n> OK if the protocol misbehaves and the receiving end omits error\n> handing\", I am not so sure how \"therefore additional work is OK\" is\n> a reasonable conclusion.\n\nMaybe I need to reword. An error is detected in either way if something\ngoes wrong. The advantage of the two step status is that if we fail early\nthen Git does not even need to create structures to read the response.\n\nSee Peff's answer here:\nhttp://public-inbox.org/git/20160806121421.bs7n4lhed7phdshb%40sigill.intra.peff.net/\nhttp://public-inbox.org/git/20160805222710.chefh5kiktyzketh%40sigill.intra.peff.net/\n\nThanks,\nLars"},{"id":"298878","messageId":"xmqq60r5yidh.fsf@gitster.mtv.corp.google.com","threadId":"43044","inReplyTo":"EF153355-2DFB-41AE-A298-CCBE69184294@gmail.com","subject":"Re: [PATCH v5 14/15] convert: add filter..process option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-12T18:03:38Z","receivedAt":"2016-08-12T18:03:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n>> Unless the reasoning is \"an error in between is so rare that we are\n>> OK if the protocol misbehaves and the receiving end omits error\n>> handing\", I am not so sure how \"therefore additional work is OK\" is\n>> a reasonable conclusion.\n>\n> Maybe I need to reword. An error is detected in either way if something\n> goes wrong. The advantage of the two step status is that if we fail early\n> then Git does not even need to create structures to read the response.\n\nOK, that's much better.\n"},{"id":"299614","messageId":"alpine.DEB.2.20.1608181617240.4924@virtualbox","threadId":"43044","inReplyTo":"20160810130411.12419-16-larsxschneider@gmail.com","subject":"Re: [PATCH v5 15/15] read-cache: make sure file handles are not inherited by child processes","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-18T14:23:42Z","receivedAt":"2016-08-18T14:24:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Lars,\n\nOn Wed, 10 Aug 2016, larsxschneider@gmail.com wrote:\n\n> From: Lars Schneider \n> \n> Consider the case of a file that requires filtering and is present in branch A\n> but not in branch B. If A is the current HEAD and we checkout B then the\n> following happens:\n> \n> 1. ce_compare_data() opens the file\n> 2. index_fd() detects that the file requires to run a clean filter and\n> calls index_stream_convert_blob()\n> 4. index_stream_convert_blob() calls convert_to_git_filter_fd()\n> 5. convert_to_git_filter_fd() calls apply_filter() which creates a new\n> long running filter process (in case it is the first file of this kind\n> to be filtered)\n> 6. The new filter process inherits all file handles. This is the default\n> on Linux/OSX and is explicitly defined in the `CreateProcessW` call\n> in `mingw.c` on Windows.\n> 7. ce_compare_data() closes the file\n> 8. Git unlinks the file as it is not present in B\n> \n> The unlink operation does not work on Windows because the filter process has\n> still an open handle to the file. Apparently that is no problem on Linux/OSX.\n> Probably because \"[...] the two file descriptors share open file status flags\"\n> (see fork(2)).\n\nWe typically wrap the commit messages at 76 columns per row (personally,\nI wrap already at 72, and it seems Junio wraps at 70).\n\n> Fix this problem by opening files in read-cache with the `O_CLOEXEC` flag to\n> ensure that the file descriptor does not remain open in a newly spawned process.\n> `O_CLOEXEX` is defined as `O_NOINHERIT` on Windows. A similar fix for temporary\n\ns/CLOEXEX/CLOEXEC/\n\n> file handles was applied on Git for Windows already:\n> https://github.com/git-for-windows/git/commit/667b8b51ec850c3e1c7d75dee69dc13c29d1f162\n\nIn response to your commit note on GitHub, I submitted this patch series\n(slightly cleaned up) yesterday (and you already commented on it):\n\nhttps://public-inbox.org/git/cover.1471437637.git.johannes.schindelin@gmx.de\n\nThe patch is obviously correct, and needs the patch series I submitted to\ncompile on Windows (this note is more for Junio's interest than a comment\non this patch).\n\nCiao,\nDscho\n"}]}