{"thread":{"id":"44041","subject":"[PATCH v7 00/10] Git filter protocol","startedAt":"2016-09-08T18:21:46Z","lastAt":"2016-09-29T09:38:10Z","messageCount":45,"participants":["larsxschneider@gmail.com","Stefan Beller","Torsten Bögershausen","Lars Schneider","Junio C Hamano","Jeff King","Jakub Narębski"],"isPatch":true,"patchVersion":7,"patchTotal":10},"messages":[{"id":"301416","messageId":"20160908182132.50788-1-larsxschneider@gmail.com","threadId":"44041","inReplyTo":null,"subject":"[PATCH v7 00/10] Git filter protocol","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-09-08T18:21:22Z","receivedAt":"2016-09-08T18:21:46Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\nThe goal of this series is to avoid launching a new clean/smudge filter\nprocess for each file that is filtered.\n\nA short summary about v1 to v5 can be found here:\nhttps://git.github.io/rev_news/2016/08/17/edition-18/\n\nThis series is also published on web:\nhttps://github.com/larsxschneider/git/pull/11\n\nThanks a lot to\n Stefan, Junio, Jakub (Narebski and Keller), Peff, and Torsten\nfor very helpful reviews,\nLars\n\n\n\n## Major changes since v6\n\n* series rebased to Git v2.10.0\n* simplified and improved multi-packet test\n* v2 filter takes precedence over v1 filter\n* rename \"error-all\" to \"abort\"\n* patch 07/13 \"pack-protocol: fix maximum pkt-line size\" submitted separately:\n http://public-inbox.org/git/20160829175509.69050-1-larsxschneider@gmail.com/\n* removed \"10/13 convert: generate large test files only once\"\n* patch 13/13 \"read-cache: make sure file handles are not inherited by child\n processes\" moved to an own series:\n http://public-inbox.org/git/20160905211111.72956-1-larsxschneider@gmail.com/\n\n\n\n## All changes since v6\n\n### Stefan\n\n* http://public-inbox.org/git/CAGZ79kaJn-yf28+Ls2JyqSs6Jt-Oj2JW-sAXQn-Oe5xaxSyjMQ@mail.gmail.com/\n * submit patch \"pack-protocol: fix maximum pkt-line size\" separately:\n http://public-inbox.org/git/20160829175509.69050-1-larsxschneider@gmail.com/\n\n* http://public-inbox.org/git/CAGZ79kbEoYVM_+MWkajT2pHN1OKZXATe5d+Dv_uSJ7dyPGxUeg@mail.gmail.com/\n * trace failures in new *_gently() pkt-line functions\n\n\n### Junio\n\n* http://public-inbox.org/git/xmqq8tvkle6o.fsf@gitster.mtv.corp.google.com/\n * share code between packet_write_fmt() and packet_write_fmt_gently()\n\n* http://public-inbox.org/git/xmqq4m68ldrg.fsf@gitster.mtv.corp.google.com/\n * remove \"Second, it will...\" from \"pkt-line: add packet_write_gently()\"\n commit message\n * use separate buffer in write_packetized_from_fd()\n * fix write order in packet_write_gently()\n\n* http://public-inbox.org/git/xmqqzio0jxh7.fsf@gitster.mtv.corp.google.com/\n * rename read/write flush terminated packet streams functions\n * remove meaningless \"bytes_to_write > ...\" check\n * reuse packet_read() function in read_packetized_to_buf()\n\n* http://public-inbox.org/git/xmqqk2f3fgbd.fsf@gitster.mtv.corp.google.com/\n * revert test_config replacement in test setup\n\n* http://public-inbox.org/git/xmqq37lncvj6.fsf@gitster.mtv.corp.google.com/\n * improve multi-packet test\n\n* http://public-inbox.org/git/xmqqzinv6wtg.fsf@gitster.mtv.corp.google.com/\n * give v2 filter precedence over v1 filter\n\n* http://public-inbox.org/git/xmqqmvjt3nht.fsf@gitster.mtv.corp.google.com/\n * explain that \"error\" behavior mimics v1 filter\n * explain that Git closes the pipe on exit to allow the filter a graceful\n exit\n\n\n### Jakub\n\n* http://public-inbox.org/git/7fbd02a1-ad62-7391-df2a-835aae8a62a7@gmail.com/\n * rename \"error-all\" to \"abort\"\n * add a simple Perl v2 filter example in contrib\n\n\n\n## Interdiff (v6..v7)\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex df059e9..ac000ea 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -470,20 +470,11 @@ 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+send the status \"error\" after the content was (partially or\n+completely) sent. Depending on the `filter..required` flag\n+Git will interpret that as error but it will not stop or restart the\n+filter process.\n ------------------------\n packet: git< status=success\\n\n packet: git< 0000\n@@ -495,21 +486,38 @@ packet: git< 0000\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+with the next file that needs to be processed. Depending on the\n+`filter..required` flag Git will interpret that as error.\n+\n+The error handling for all cases above mimic the behavior of\n+the `filter..clean` / `filter..smudge` error\n+handling.\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 \"abort\" status. Depending on\n+the `filter..required` flag Git will interpret that as error\n+for the content as well as any future content for the lifetime of the\n+Git process but it will not stop or restart the filter process.\n+------------------------\n+packet: git< status=abort\\n\n+packet: git< 0000\n+------------------------\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+the next \"key=value\" list containing a command. Git will close\n+the command pipe on exit. The filter is expected to detect EOF\n+and exit gracefully on its own.\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+`contrib/long-running-filter/example.pl` located in the Git\n+core repository. If you develop your own long running filter\n+process then the `GIT_TRACE_PACKET` environment variables can be\n+very helpful for debugging (see linkgit:git[1]).\n+\n+If a `filter..process` command is configured then it\n+always takes precedence over a configured `filter..clean`\n+or `filter..smudge` command.\n\n Please note that you cannot use an existing `filter..clean`\n or `filter..smudge` command with `filter..process`\ndiff --git a/contrib/long-running-filter/example.pl b/contrib/long-running-filter/example.pl\nnew file mode 100755\nindex 0000000..279fbfb\n--- /dev/null\n+++ b/contrib/long-running-filter/example.pl\n@@ -0,0 +1,111 @@\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+\n+use strict;\n+use warnings;\n+\n+my $MAX_PACKET_CONTENT_SIZE = 65516;\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+( 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+packet_write( \"clean=true\\n\" );\n+packet_write( \"smudge=true\\n\" );\n+packet_flush();\n+\n+while (1) {\n+ my ($command) = packet_read() =~ /^command=([^=]+)\\n$/;\n+ my ($pathname) = packet_read() =~ /^pathname=([^=]+)\\n$/;\n+\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+ }\n+\n+ my $output;\n+ if ( $command eq \"clean\" ) {\n+ ### Perform clean here ###\n+ $output = $input;\n+ }\n+ elsif ( $command eq \"smudge\" ) {\n+ ### Perform smudge here ###\n+ $output = $input;\n+ }\n+ else {\n+ die \"bad command '$command'\";\n+ }\n+\n+ packet_write(\"status=success\\n\");\n+ packet_flush();\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(); # flush content!\n+ packet_flush(); # empty list!\n+}\ndiff --git a/convert.c b/convert.c\nindex 0269a49..0ed48ed 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -723,9 +723,9 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n goto done;\n\n if (fd >= 0)\n- err = packet_write_stream_with_flush_from_fd(fd, process->in);\n+ err = write_packetized_from_fd(fd, process->in);\n else\n- err = packet_write_stream_with_flush_from_buf(src, len, process->in);\n+ err = write_packetized_from_buf(src, len, process->in);\n if (err)\n goto done;\n\n@@ -734,7 +734,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n if (err)\n goto done;\n\n- err = packet_read_till_flush(process->out, &nbuf) < 0;\n+ err = read_packetized_to_buf(process->out, &nbuf) < 0;\n if (err)\n goto done;\n\n@@ -747,7 +747,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n if (err || errno == EPIPE) {\n if (!strcmp(filter_status.buf, \"error\")) {\n /* The filter signaled a problem with the file. */\n- } else if (!strcmp(filter_status.buf, \"error-all\")) {\n+ } else if (!strcmp(filter_status.buf, \"abort\")) {\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@@ -790,9 +790,9 @@ static int apply_filter(const char *path, const char *src, size_t len,\n if (!dst)\n return 1;\n\n- if ((CAP_CLEAN & wanted_capability) && drv->clean)\n+ if (!drv->process && (CAP_CLEAN & wanted_capability) && drv->clean)\n cmd = drv->clean;\n- else if ((CAP_SMUDGE & wanted_capability) && drv->smudge)\n+ else if (!drv->process && (CAP_SMUDGE & wanted_capability) && drv->smudge)\n cmd = drv->smudge;\n\n if (cmd && *cmd)\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 3033aa3..5001a07 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -3,7 +3,6 @@\n #include \"run-command.h\"\n\n char packet_buffer[LARGE_PACKET_MAX];\n-static 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@@ -95,7 +94,10 @@ void packet_flush(int fd)\n int packet_flush_gently(int fd)\n {\n packet_trace(\"0000\", 4, 1);\n- return (write_in_full(fd, \"0000\", 4) == 4 ? 0 : -1);\n+ if (write_in_full(fd, \"0000\", 4) == 4)\n+ return 0;\n+ error(\"flush packet write failed\");\n+ return -1;\n }\n\n void packet_buf_flush(struct strbuf *buf)\n@@ -132,39 +134,70 @@ static void format_packet(struct strbuf *out, const char *fmt, va_list args)\n packet_trace(out->buf + orig_len + 4, n - 4, 1);\n }\n\n+static int packet_write_fmt_1(int fd, int gently,\n+ const char *fmt, va_list args)\n+{\n+ struct strbuf buf = STRBUF_INIT;\n+ size_t count;\n+\n+ format_packet(&buf, fmt, args);\n+ count = write_in_full(fd, buf.buf, buf.len);\n+ if (count == buf.len)\n+ return 0;\n+\n+ if (!gently) {\n+ if (errno == EPIPE) {\n+ if (in_async())\n+ async_exit(141);\n+\n+ signal(SIGPIPE, SIG_DFL);\n+ raise(SIGPIPE);\n+ /* Should never happen, but just in case... */\n+ exit(141);\n+ }\n+ die_errno(\"packet write error\");\n+ }\n+ error(\"packet write failed\");\n+ return -1;\n+}\n+\n void packet_write_fmt(int fd, const char *fmt, ...)\n {\n- static struct strbuf buf = STRBUF_INIT;\n va_list args;\n\n- strbuf_reset(&buf);\n va_start(args, fmt);\n- format_packet(&buf, fmt, args);\n+ packet_write_fmt_1(fd, 0, fmt, args);\n va_end(args);\n- write_or_die(fd, buf.buf, buf.len);\n }\n\n int packet_write_fmt_gently(int fd, const char *fmt, ...)\n {\n- static struct strbuf buf = STRBUF_INIT;\n+ int status;\n va_list args;\n\n- strbuf_reset(&buf);\n va_start(args, fmt);\n- format_packet(&buf, fmt, args);\n+ status = packet_write_fmt_1(fd, 1, fmt, args);\n va_end(args);\n- return (write_in_full(fd, buf.buf, buf.len) == buf.len ? 0 : -1);\n+ return status;\n }\n\n int packet_write_gently(const int fd_out, const char *buf, size_t size)\n {\n- if (size > sizeof(packet_write_buffer) - 4)\n+ static char packet_write_buffer[LARGE_PACKET_MAX];\n+\n+ if (size > sizeof(packet_write_buffer) - 4) {\n+ error(\"packet write failed\");\n return -1;\n+ }\n packet_trace(buf, size, 1);\n- memmove(packet_write_buffer + 4, buf, size);\n size += 4;\n set_packet_header(packet_write_buffer, size);\n- return (write_in_full(fd_out, packet_write_buffer, size) == size ? 0 : -1);\n+ memcpy(packet_write_buffer + 4, buf, size - 4);\n+ if (write_in_full(fd_out, packet_write_buffer, size) == size)\n+ return 0;\n+\n+ error(\"packet write failed\");\n+ return -1;\n }\n\n void packet_buf_write(struct strbuf *buf, const char *fmt, ...)\n@@ -176,35 +209,35 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...)\n va_end(args);\n }\n\n-int packet_write_stream_with_flush_from_fd(int fd_in, int fd_out)\n+int write_packetized_from_fd(int fd_in, int fd_out)\n {\n+ static char buf[PKTLINE_DATA_MAXLEN];\n int err = 0;\n ssize_t bytes_to_write;\n\n while (!err) {\n- bytes_to_write = xread(fd_in, packet_write_buffer, sizeof(packet_write_buffer) - 4);\n+ bytes_to_write = xread(fd_in, buf, sizeof(buf));\n if (bytes_to_write < 0)\n return COPY_READ_ERROR;\n if (bytes_to_write == 0)\n break;\n- if (bytes_to_write > sizeof(packet_write_buffer) - 4)\n- return COPY_WRITE_ERROR;\n- err = packet_write_gently(fd_out, packet_write_buffer, bytes_to_write);\n+ err = packet_write_gently(fd_out, buf, bytes_to_write);\n }\n if (!err)\n err = packet_flush_gently(fd_out);\n return err;\n }\n\n-int packet_write_stream_with_flush_from_buf(const char *src_in, size_t len, int fd_out)\n+int write_packetized_from_buf(const char *src_in, size_t len, int fd_out)\n {\n+ static char buf[PKTLINE_DATA_MAXLEN];\n int err = 0;\n size_t bytes_written = 0;\n size_t bytes_to_write;\n\n while (!err) {\n- if ((len - bytes_written) > sizeof(packet_write_buffer) - 4)\n- bytes_to_write = sizeof(packet_write_buffer) - 4;\n+ if ((len - bytes_written) > sizeof(buf))\n+ bytes_to_write = sizeof(buf);\n else\n bytes_to_write = len - bytes_written;\n if (bytes_to_write == 0)\n@@ -327,52 +360,29 @@ char *packet_read_line_buf(char **src, size_t *src_len, int *dst_len)\n return 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+ssize_t read_packetized_to_buf(int fd_in, struct strbuf *sb_out)\n {\n- int len, ret;\n+ int paket_len;\n int options = PACKET_READ_GENTLE_ON_EOF;\n- char linelen[4];\n\n size_t oldlen = sb_out->len;\n size_t oldalloc = sb_out->alloc;\n\n for (;;) {\n- /* Read packet header */\n- ret = get_packet_data(fd_in, NULL, NULL, linelen, 4, options);\n- if (ret < 0)\n- goto done;\n- len = packet_length(linelen);\n- if (len < 0)\n- die(\"protocol error: bad line length character: %.4s\", linelen);\n- if (!len) {\n- /* Found a flush packet - Done! */\n- packet_trace(\"0000\", 4, 0);\n+ strbuf_grow(sb_out, PKTLINE_DATA_MAXLEN+1);\n+ paket_len = packet_read(fd_in, NULL, NULL,\n+ sb_out->buf + sb_out->len, PKTLINE_DATA_MAXLEN+1, options);\n+ if (paket_len <= 0)\n break;\n- }\n- len -= 4;\n-\n- /* Read packet content */\n- strbuf_grow(sb_out, len);\n- ret = get_packet_data(fd_in, NULL, NULL, sb_out->buf + sb_out->len, len, options);\n- if (ret < 0)\n- goto done;\n-\n- if (ret != len) {\n- error(\"protocol error: incomplete read (expected %d, got %d)\", len, ret);\n- goto done;\n- }\n-\n- packet_trace(sb_out->buf + sb_out->len, len, 0);\n- sb_out->len += len;\n+ sb_out->len += paket_len;\n }\n\n-done:\n- if (ret < 0) {\n+ if (paket_len < 0) {\n if (oldalloc == 0)\n strbuf_release(sb_out);\n else\n strbuf_setlen(sb_out, oldlen);\n- return ret; /* unexpected EOF */\n+ return paket_len;\n }\n return sb_out->len - oldlen;\n }\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 6a3b7cf..3d873f3 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -25,8 +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_fmt_gently(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+int write_packetized_from_fd(int fd_in, int fd_out);\n+int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);\n\n /*\n * Read a packetized line into the buffer, which must be at least size bytes\n@@ -82,7 +82,7 @@ char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size);\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+ssize_t read_packetized_to_buf(int fd_in, struct strbuf *sb_out);\n\n #define DEFAULT_PACKET_MAX 1000\n #define LARGE_PACKET_MAX 65520\ndiff --git a/read-cache.c b/read-cache.c\nindex 02f74d3..491e52d 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -156,7 +156,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 int match = -1;\n- int fd = open(ce->name, O_RDONLY | O_CLOEXEC);\n+ int fd = open(ce->name, O_RDONLY);\n\n if (fd >= 0) {\n unsigned char sha1[20];\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex 94b0bf3..1c98ac3 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -4,15 +4,6 @@ test_description='blob conversion via gitattributes'\n\n . ./test-lib.sh\n\n-if test_have_prereq EXPENSIVE\n-then\n- T0021_LARGE_FILE_SIZE=2048\n- T0021_LARGISH_FILE_SIZE=100\n-else\n- T0021_LARGE_FILE_SIZE=30\n- T0021_LARGISH_FILE_SIZE=2\n-fi\n-\n cat <rot13.sh\n #!$SHELL_PATH\n tr \\\n@@ -22,8 +13,8 @@ EOF\n chmod +x rot13.sh\n\n test_expect_success setup '\n- test_config filter.rot13.smudge ./rot13.sh &&\n- test_config filter.rot13.clean ./rot13.sh &&\n+ git config filter.rot13.smudge ./rot13.sh &&\n+ git config filter.rot13.clean ./rot13.sh &&\n\n {\n echo \"*.t filter=rot13\"\n@@ -43,26 +34,7 @@ test_expect_success setup '\n git checkout -- test test.t test.i &&\n\n echo \"content-test2\" >test2.o &&\n- echo \"content-test3-subdir\" >test3-subdir.o &&\n-\n- mkdir generated-test-data &&\n- for i in $(test_seq 1 $T0021_LARGE_FILE_SIZE)\n- do\n- RANDOM_STRING=\"$(test-genrandom end $i | tr -dc \"A-Za-z0-9\" )\"\n- ROT_RANDOM_STRING=\"$(echo $RANDOM_STRING | ./rot13.sh )\"\n- # Generate 1MB of empty data and 100 bytes of random characters\n- # printf \"$(test-genrandom start $i)\"\n- printf \"%1048576d\" 1 >>generated-test-data/large.file &&\n- printf \"$RANDOM_STRING\" >>generated-test-data/large.file &&\n- printf \"%1048576d\" 1 >>generated-test-data/large.file.rot13 &&\n- printf \"$ROT_RANDOM_STRING\" >>generated-test-data/large.file.rot13 &&\n-\n- if test $i = $T0021_LARGISH_FILE_SIZE\n- then\n- cat generated-test-data/large.file >generated-test-data/largish.file &&\n- cat generated-test-data/large.file.rot13 >generated-test-data/largish.file.rot13\n- fi\n- done\n+ echo \"content-test3-subdir\" >test3-subdir.o\n '\n\n script='s/^\\$Id: \\([0-9a-f]*\\) \\$/\\1/p'\n@@ -230,9 +202,9 @@ test_expect_success 'required filter clean failure' '\n test_expect_success 'filtering large input to small output should use little memory' '\n test_config filter.devnull.clean \"cat >/dev/null\" &&\n test_config filter.devnull.required true &&\n- cp generated-test-data/large.file large.file &&\n- echo \"large.file filter=devnull\" >.gitattributes &&\n- GIT_MMAP_LIMIT=1m GIT_ALLOC_LIMIT=1m git add large.file\n+ for i in $(test_seq 1 30); do printf \"%1048576d\" 1; done >30MB &&\n+ echo \"30MB filter=devnull\" >.gitattributes &&\n+ GIT_MMAP_LIMIT=1m GIT_ALLOC_LIMIT=1m git add 30MB\n '\n\n test_expect_success 'filter that does not read is fine' '\n@@ -245,15 +217,15 @@ test_expect_success 'filter that does not read is fine' '\n test_cmp expect actual\n '\n\n-test_expect_success 'filter large file' '\n+test_expect_success EXPENSIVE 'filter large file' '\n test_config filter.largefile.smudge cat &&\n test_config filter.largefile.clean cat &&\n- echo \"large.file filter=largefile\" >.gitattributes &&\n- cp generated-test-data/large.file large.file &&\n- git add large.file 2>err &&\n+ for i in $(test_seq 1 2048); do printf \"%1048576d\" 1; done >2GB &&\n+ echo \"2GB filter=largefile\" >.gitattributes &&\n+ git add 2GB 2>err &&\n test_must_be_empty err &&\n- rm -f large.file &&\n- git checkout -- large.file 2>err &&\n+ rm -f 2GB &&\n+ git checkout -- 2GB 2>err &&\n test_must_be_empty err\n '\n\n@@ -374,22 +346,24 @@ test_expect_success PERL 'required process filter should filter data' '\n check_filter \\\n git add . \\\n <<-\\EOF &&\n- 1 IN: clean test.r 57 [OK] -- OUT: 57 [OK]\n- 1 IN: clean test2.r 14 [OK] -- OUT: 14 [OK]\n- 1 IN: clean test4-empty.r 0 [OK] -- OUT: 0 [OK]\n- 1 IN: clean testsubdir/test3-subdir.r 21 [OK] -- OUT: 21 [OK]\n- 1 start\n+ 1 IN: clean test.r 57 [OK] -- OUT: 57 . [OK]\n+ 1 IN: clean test2.r 14 [OK] -- OUT: 14 . [OK]\n+ 1 IN: clean test4-empty.r 0 [OK] -- OUT: 0 [OK]\n+ 1 IN: clean testsubdir/test3-subdir.r 21 [OK] -- OUT: 21 . [OK]\n+ 1 START\n+ 1 STOP\n 1 wrote filter header\n EOF\n\n check_filter_count_clean \\\n git commit . -m \"test commit\" \\\n <<-\\EOF &&\n- x IN: clean test.r 57 [OK] -- OUT: 57 [OK]\n- x IN: clean test2.r 14 [OK] -- OUT: 14 [OK]\n- x IN: clean test4-empty.r 0 [OK] -- OUT: 0 [OK]\n- x IN: clean testsubdir/test3-subdir.r 21 [OK] -- OUT: 21 [OK]\n- 1 start\n+ x IN: clean test.r 57 [OK] -- OUT: 57 . [OK]\n+ x IN: clean test2.r 14 [OK] -- OUT: 14 . [OK]\n+ x IN: clean test4-empty.r 0 [OK] -- OUT: 0 [OK]\n+ x IN: clean testsubdir/test3-subdir.r 21 [OK] -- OUT: 21 . [OK]\n+ 1 START\n+ 1 STOP\n 1 wrote filter header\n EOF\n\n@@ -398,28 +372,31 @@ test_expect_success PERL 'required process filter should filter data' '\n check_filter_ignore_clean \\\n git checkout . \\\n <<-\\EOF &&\n- start\n+ START\n wrote filter header\n- IN: smudge test2.r 14 [OK] -- OUT: 14 [OK]\n- IN: smudge testsubdir/test3-subdir.r 21 [OK] -- OUT: 21 [OK]\n+ IN: smudge test2.r 14 [OK] -- OUT: 14 . [OK]\n+ IN: smudge testsubdir/test3-subdir.r 21 [OK] -- OUT: 21 . [OK]\n+ STOP\n EOF\n\n check_filter_ignore_clean \\\n git checkout empty \\\n <<-\\EOF &&\n- start\n+ START\n wrote filter header\n+ STOP\n EOF\n\n check_filter_ignore_clean \\\n git checkout master \\\n <<-\\EOF &&\n- start\n+ START\n wrote filter header\n- IN: smudge test.r 57 [OK] -- OUT: 57 [OK]\n- IN: smudge test2.r 14 [OK] -- OUT: 14 [OK]\n- IN: smudge test4-empty.r 0 [OK] -- OUT: 0 [OK]\n- IN: smudge testsubdir/test3-subdir.r 21 [OK] -- OUT: 21 [OK]\n+ IN: smudge test.r 57 [OK] -- OUT: 57 . [OK]\n+ IN: smudge test2.r 14 [OK] -- OUT: 14 . [OK]\n+ IN: smudge test4-empty.r 0 [OK] -- OUT: 0 [OK]\n+ IN: smudge testsubdir/test3-subdir.r 21 [OK] -- OUT: 21 . [OK]\n+ STOP\n EOF\n\n check_rot13 ../test.o test.r &&\n@@ -428,57 +405,8 @@ test_expect_success PERL 'required process filter should filter data' '\n )\n '\n\n-test_expect_success PERL 'required process filter should filter smudge data and one-shot filter should clean' '\n+test_expect_success PERL 'required process filter should clean only and take precedence' '\n test_config_global filter.protocol.clean ./../rot13.sh &&\n- test_config_global filter.protocol.process \"$TEST_DIRECTORY/t0021/rot13-filter.pl smudge\" &&\n- test_config_global filter.protocol.required true &&\n- rm -rf repo &&\n- mkdir repo &&\n- (\n- cd repo &&\n- git init &&\n-\n- echo \"*.r filter=protocol\" >.gitattributes &&\n- git add . &&\n- git commit . -m \"test commit\" &&\n- git branch empty &&\n-\n- cp ../test.o test.r &&\n- cp ../test2.o test2.r &&\n-\n- check_filter_no_call \\\n- git add . &&\n-\n- check_filter_no_call \\\n- git commit . -m \"test commit\" &&\n-\n- rm -f test?.r testsubdir/test3-subdir.r &&\n-\n- check_filter_ignore_clean \\\n- git checkout . \\\n- <<-\\EOF &&\n- start\n- wrote filter header\n- IN: smudge test2.r 14 [OK] -- OUT: 14 [OK]\n- EOF\n-\n- git checkout empty &&\n-\n- check_filter_ignore_clean \\\n- git checkout master\\\n- <<-\\EOF &&\n- start\n- wrote filter header\n- IN: smudge test.r 57 [OK] -- OUT: 57 [OK]\n- IN: smudge test2.r 14 [OK] -- OUT: 14 [OK]\n- EOF\n-\n- check_rot13 ../test.o test.r &&\n- check_rot13 ../test2.o test2.r\n- )\n-'\n-\n-test_expect_success PERL 'required process filter should clean only' '\n test_config_global filter.protocol.process \"$TEST_DIRECTORY/t0021/rot13-filter.pl clean\" &&\n test_config_global filter.protocol.required true &&\n rm -rf repo &&\n@@ -497,42 +425,74 @@ test_expect_success PERL 'required process filter should clean only' '\n check_filter \\\n git add . \\\n <<-\\EOF &&\n- 1 IN: clean test.r 57 [OK] -- OUT: 57 [OK]\n- 1 start\n+ 1 IN: clean test.r 57 [OK] -- OUT: 57 . [OK]\n+ 1 START\n+ 1 STOP\n 1 wrote filter header\n EOF\n\n check_filter_count_clean \\\n git commit . -m \"test commit\" \\\n <<-\\EOF\n- x IN: clean test.r 57 [OK] -- OUT: 57 [OK]\n- 1 start\n+ x IN: clean test.r 57 [OK] -- OUT: 57 . [OK]\n+ 1 START\n+ 1 STOP\n 1 wrote filter header\n EOF\n )\n '\n\n-test_expect_success PERL 'required process filter should process binary files larger LARGE_PACKET_MAX' '\n+generate_test_data () {\n+ LEN=$1\n+ NAME=$2\n+ test-genrandom end $LEN |\n+ perl -pe \"s/./chr((ord($&) % 26) + 97)/sge\" >../$NAME.file &&\n+ cp ../$NAME.file . &&\n+ ./../rot13.sh <../$NAME.file >../$NAME.file.rot13\n+}\n+\n+test_expect_success PERL 'required process filter should process multiple packets' '\n test_config_global filter.protocol.process \"$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge\" &&\n test_config_global filter.protocol.required true &&\n+\n rm -rf repo &&\n mkdir repo &&\n (\n cd repo &&\n git init &&\n\n- echo \"*.file filter=protocol\" >.gitattributes &&\n- cat ../generated-test-data/largish.file.rot13 >large.rot13 &&\n- cat ../generated-test-data/largish.file >large.file &&\n- cat large.file >large.original &&\n+ # Generate data that requires 3 packets\n+ PKTLINE_DATA_MAXLEN=65516 &&\n+\n+ generate_test_data $(($PKTLINE_DATA_MAXLEN )) 1pkt_1__ &&\n+ generate_test_data $(($PKTLINE_DATA_MAXLEN + 1)) 2pkt_1+1 &&\n+ generate_test_data $(($PKTLINE_DATA_MAXLEN * 2 - 1)) 2pkt_2-1 &&\n+ generate_test_data $(($PKTLINE_DATA_MAXLEN * 2 )) 2pkt_2__ &&\n+ generate_test_data $(($PKTLINE_DATA_MAXLEN * 2 + 1)) 3pkt_2+1 &&\n\n- git add large.file .gitattributes &&\n+ echo \"*.file filter=protocol\" >.gitattributes &&\n+ check_filter \\\n+ git add *.file .gitattributes \\\n+ <<-\\EOF &&\n+ 1 IN: clean 1pkt_1__.file 65516 [OK] -- OUT: 65516 . [OK]\n+ 1 IN: clean 2pkt_1+1.file 65517 [OK] -- OUT: 65517 .. [OK]\n+ 1 IN: clean 2pkt_2-1.file 131031 [OK] -- OUT: 131031 .. [OK]\n+ 1 IN: clean 2pkt_2__.file 131032 [OK] -- OUT: 131032 .. [OK]\n+ 1 IN: clean 3pkt_2+1.file 131033 [OK] -- OUT: 131033 ... [OK]\n+ 1 START\n+ 1 STOP\n+ 1 wrote filter header\n+ EOF\n git commit . -m \"test commit\" &&\n\n- rm -f large.file &&\n- git checkout -- large.file &&\n- git cat-file blob :large.file >actual &&\n- test_cmp large.rot13 actual\n+ rm -f *.file &&\n+ git checkout -- *.file &&\n+\n+ for f in *.file\n+ do\n+ git cat-file blob :$f >actual &&\n+ test_cmp ../$f.rot13 actual\n+ done\n )\n '\n\n@@ -577,13 +537,14 @@ test_expect_success PERL 'process filter should restart after unexpected write f\n check_filter_ignore_clean \\\n git checkout . \\\n <<-\\EOF &&\n- start\n+ START\n wrote filter header\n IN: smudge smudge-write-fail.r 22 [OK] -- OUT: 22 [WRITE FAIL]\n- start\n+ START\n wrote filter header\n- IN: smudge test.r 57 [OK] -- OUT: 57 [OK]\n- IN: smudge test2.r 14 [OK] -- OUT: 14 [OK]\n+ IN: smudge test.r 57 [OK] -- OUT: 57 . [OK]\n+ IN: smudge test2.r 14 [OK] -- OUT: 14 . [OK]\n+ STOP\n EOF\n\n check_rot13 ../test.o test.r &&\n@@ -617,11 +578,12 @@ test_expect_success PERL 'process filter should not restart in case of an error'\n check_filter_ignore_clean \\\n git checkout . \\\n <<-\\EOF &&\n- start\n+ START\n wrote filter header\n IN: smudge error.r 25 [OK] -- OUT: 0 [ERROR]\n- IN: smudge test.r 57 [OK] -- OUT: 57 [OK]\n- IN: smudge test2.r 14 [OK] -- OUT: 14 [OK]\n+ IN: smudge test.r 57 [OK] -- OUT: 57 . [OK]\n+ IN: smudge test2.r 14 [OK] -- OUT: 14 . [OK]\n+ STOP\n EOF\n\n check_rot13 ../test.o test.r &&\n@@ -642,8 +604,8 @@ test_expect_success PERL 'process filter should be able to signal an error for a\n\n cp ../test.o test.r &&\n cp ../test2.o test2.r &&\n- echo \"error this blob and all future blobs\" >error-all.o &&\n- cp error-all.o error-all.r &&\n+ echo \"error this blob and all future blobs\" >abort.o &&\n+ cp abort.o abort.r &&\n git add . &&\n git commit . -m \"test commit\" &&\n rm -f *.r &&\n@@ -651,14 +613,15 @@ test_expect_success PERL 'process filter should be able to signal an error for a\n check_filter_ignore_clean \\\n git checkout . \\\n <<-\\EOF &&\n- start\n+ START\n wrote filter header\n- IN: smudge error-all.r 37 [OK] -- OUT: 0 [ERROR-ALL]\n+ IN: smudge abort.r 37 [OK] -- OUT: 0 [ABORT]\n+ STOP\n EOF\n\n test_cmp ../test.o test.r &&\n test_cmp ../test2.o test2.r &&\n- test_cmp error-all.o error-all.r\n+ test_cmp abort.o abort.r\n )\n '\n\ndiff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl\nindex 0e1181d..8e27877 100755\n--- a/t/t0021/rot13-filter.pl\n+++ b/t/t0021/rot13-filter.pl\n@@ -14,7 +14,7 @@\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+# (4) If data with the pathname \"abort.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@@ -26,6 +26,8 @@ use warnings;\n my $MAX_PACKET_CONTENT_SIZE = 65516;\n my @capabilities = @ARGV;\n\n+open my $debug, \">>\", \"rot13-filter.log\";\n+\n sub rot13 {\n my ($str) = @_;\n $str =~ y/A-Za-z/N-ZA-Mn-za-m/;\n@@ -38,6 +40,7 @@ sub packet_read {\n if ( $bytes_read == 0 ) {\n\n # EOF - Git stopped talking to us!\n+ print $debug \"STOP\\n\";\n exit();\n }\n elsif ( $bytes_read != 4 ) {\n@@ -72,8 +75,7 @@ sub packet_flush {\n STDOUT->flush();\n }\n\n-open my $debug, \">>\", \"rot13-filter.log\";\n-print $debug \"start\\n\";\n+print $debug \"START\\n\";\n $debug->flush();\n\n ( packet_read() eq ( 0, \"git-filter-client\" ) ) || die \"bad initialization\";\n@@ -120,7 +122,7 @@ while (1) {\n }\n\n my $output;\n- if ( $pathname eq \"error.r\" or $pathname eq \"error-all.r\" ) {\n+ if ( $pathname eq \"error.r\" or $pathname eq \"abort.r\" ) {\n $output = \"\";\n }\n elsif ( $command eq \"clean\" and grep( /^clean$/, @capabilities ) ) {\n@@ -142,10 +144,10 @@ while (1) {\n packet_write(\"status=error\\n\");\n packet_flush();\n }\n- elsif ( $pathname eq \"error-all.r\" ) {\n- print $debug \"[ERROR-ALL]\\n\";\n+ elsif ( $pathname eq \"abort.r\" ) {\n+ print $debug \"[ABORT]\\n\";\n $debug->flush();\n- packet_write(\"status=error-all\\n\");\n+ packet_write(\"status=abort\\n\");\n packet_flush();\n }\n else {\n@@ -161,6 +163,7 @@ while (1) {\n while ( length($output) > 0 ) {\n my $packet = substr( $output, 0, $MAX_PACKET_CONTENT_SIZE );\n packet_write($packet);\n+ print $debug \".\";\n if ( length($output) > $MAX_PACKET_CONTENT_SIZE ) {\n $output = substr( $output, $MAX_PACKET_CONTENT_SIZE );\n }\n@@ -169,7 +172,7 @@ while (1) {\n }\n }\n packet_flush();\n- print $debug \"[OK]\\n\";\n+ print $debug \" [OK]\\n\";\n $debug->flush();\n packet_flush();\n }\n\n\n\n\nLars Schneider (10):\n pkt-line: rename packet_write() to packet_write_fmt()\n pkt-line: extract set_packet_header()\n pkt-line: add packet_write_fmt_gently()\n pkt-line: add packet_flush_gently()\n pkt-line: add packet_write_gently()\n pkt-line: add functions to read/write flush terminated packet streams\n convert: quote filter names in error messages\n convert: modernize tests\n convert: make apply_filter() adhere to standard Git error handling\n convert: add filter..process option\n\n Documentation/gitattributes.txt | 154 +++++++++++-\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 contrib/long-running-filter/example.pl | 111 +++++++++\n convert.c | 373 +++++++++++++++++++++++++----\n daemon.c | 2 +-\n http-backend.c | 2 +-\n pkt-line.c | 160 ++++++++++++-\n pkt-line.h | 12 +-\n shallow.c | 2 +-\n t/t0021-conversion.sh | 423 ++++++++++++++++++++++++++++++---\n t/t0021/rot13-filter.pl | 179 ++++++++++++++\n unpack-trees.c | 1 +\n upload-pack.c | 30 +--\n 17 files changed, 1356 insertions(+), 111 deletions(-)\n create mode 100755 contrib/long-running-filter/example.pl\n create mode 100755 t/t0021/rot13-filter.pl\n\n--\n2.10.0\n\n"},{"id":"301417","messageId":"20160908182132.50788-2-larsxschneider@gmail.com","threadId":"44041","inReplyTo":"20160908182132.50788-1-larsxschneider@gmail.com","subject":"[PATCH v7 01/10] pkt-line: rename packet_write() to packet_write_fmt()","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-09-08T18:21:23Z","receivedAt":"2016-09-08T18:21:55Z","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 011db00..1ce7682 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -218,7 +218,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@@ -233,7 +233,7 @@ static void show_ref(const char *path, const unsigned char *sha1)\n \t\tif (advertise_push_options)\n \t\t\tstrbuf_addstr(&cap, \" push-options\");\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 425aad0..afce1b9 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 adc8c8c..eef0a36 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -464,7 +464,7 @@ static void get_info_refs(struct strbuf *hdr, char *arg)\n \t\thdr_str(hdr, content_type, buf.buf);\n \t\tend_headers(hdr);\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 62fdb37..0a9b61c 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -118,7 +118,7 @@ static void format_packet(struct strbuf *out, const char *fmt, va_list args)\n \tpacket_trace(out->buf + orig_len + 4, n - 4, 1);\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 3cb9d91..1902fb3 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 \ndiff --git a/shallow.c b/shallow.c\nindex 54e2db7..d666e24 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 ca7f941..cd47de6 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.10.0\n\n"},{"id":"301418","messageId":"20160908182132.50788-3-larsxschneider@gmail.com","threadId":"44041","inReplyTo":"20160908182132.50788-1-larsxschneider@gmail.com","subject":"[PATCH v7 02/10] pkt-line: extract set_packet_header()","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-09-08T18:21:24Z","receivedAt":"2016-09-08T18:21:58Z","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 | 19 +++++++++++++------\n 1 file changed, 13 insertions(+), 6 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 0a9b61c..e8adc0f 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -97,10 +97,20 @@ 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+\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 +121,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.10.0\n\n"},{"id":"301419","messageId":"20160908182132.50788-4-larsxschneider@gmail.com","threadId":"44041","inReplyTo":"20160908182132.50788-1-larsxschneider@gmail.com","subject":"[PATCH v7 03/10] pkt-line: add packet_write_fmt_gently()","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-09-08T18:21:25Z","receivedAt":"2016-09-08T18:22:01Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\npacket_write_fmt() would die in case of a write error even though for\nsome callers an error would be acceptable. Add packet_write_fmt_gently()\nwhich writes a formatted pkt-line and returns `0` for success and `-1`\nfor an error.\n\nSigned-off-by: Lars Schneider \n---\n pkt-line.c | 43 +++++++++++++++++++++++++++++++++++++++----\n pkt-line.h | 1 +\n 2 files changed, 40 insertions(+), 4 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex e8adc0f..3824d05 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -125,16 +125,51 @@ static void format_packet(struct strbuf *out, const char *fmt, va_list args)\n \tpacket_trace(out->buf + orig_len + 4, n - 4, 1);\n }\n \n+static int packet_write_fmt_1(int fd, int gently,\n+ const char *fmt, va_list args)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tsize_t count;\n+\n+\tformat_packet(&buf, fmt, args);\n+\tcount = write_in_full(fd, buf.buf, buf.len);\n+\tif (count == buf.len)\n+\t\treturn 0;\n+\n+\tif (!gently) {\n+\t\tif (errno == EPIPE) {\n+\t\t\tif (in_async())\n+\t\t\t\tasync_exit(141);\n+\n+\t\t\tsignal(SIGPIPE, SIG_DFL);\n+\t\t\traise(SIGPIPE);\n+\t\t\t/* Should never happen, but just in case... */\n+\t\t\texit(141);\n+\t\t}\n+\t\tdie_errno(\"packet write error\");\n+\t}\n+\terror(\"packet write failed\");\n+\treturn -1;\n+}\n+\n void packet_write_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(&buf, fmt, args);\n+\tpacket_write_fmt_1(fd, 0, fmt, args);\n+\tva_end(args);\n+}\n+\n+int packet_write_fmt_gently(int fd, const char *fmt, ...)\n+{\n+\tint status;\n+\tva_list args;\n+\n+\tva_start(args, fmt);\n+\tstatus = packet_write_fmt_1(fd, 1, fmt, args);\n \tva_end(args);\n-\twrite_or_die(fd, buf.buf, buf.len);\n+\treturn status;\n }\n \n void packet_buf_write(struct strbuf *buf, const char *fmt, ...)\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 1902fb3..3caea77 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_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_write_fmt_gently(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.10.0\n\n"},{"id":"301420","messageId":"20160908182132.50788-5-larsxschneider@gmail.com","threadId":"44041","inReplyTo":"20160908182132.50788-1-larsxschneider@gmail.com","subject":"[PATCH v7 04/10] pkt-line: add packet_flush_gently()","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-09-08T18:21:26Z","receivedAt":"2016-09-08T18:22:04Z","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\ncallers an error would be acceptable. Add packet_flush_gently() which\nwrites a pkt-line flush packet and returns `0` for success and `-1` for\nfailure.\n\nSigned-off-by: Lars Schneider \n---\n pkt-line.c | 9 +++++++++\n pkt-line.h | 1 +\n 2 files changed, 10 insertions(+)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 3824d05..37345ca 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -91,6 +91,15 @@ 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+\tif (write_in_full(fd, \"0000\", 4) == 4)\n+\t\treturn 0;\n+\terror(\"flush packet write failed\");\n+\treturn -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 3caea77..3fa0899 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_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);\n int packet_write_fmt_gently(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n \n /*\n-- \n2.10.0\n\n"},{"id":"301421","messageId":"20160908182132.50788-6-larsxschneider@gmail.com","threadId":"44041","inReplyTo":"20160908182132.50788-1-larsxschneider@gmail.com","subject":"[PATCH v7 05/10] pkt-line: add packet_write_gently()","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-09-08T18:21:27Z","receivedAt":"2016-09-08T18:22:05Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\npacket_write_fmt_gently() uses format_packet() which lets the caller\nonly send string data via \"%s\". That means it cannot be used for\narbitrary data that may contain NULs.\n\nAdd packet_write_gently() which writes arbitrary data and returns `0`\nfor success and `-1` for an error. This function is used by other\npkt-line functions in a subsequent patch.\n\nSigned-off-by: Lars Schneider \n---\n pkt-line.c | 19 +++++++++++++++++++\n 1 file changed, 19 insertions(+)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 37345ca..1d3d725 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -181,6 +181,25 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)\n \treturn status;\n }\n \n+int packet_write_gently(const int fd_out, const char *buf, size_t size)\n+{\n+\tstatic char packet_write_buffer[LARGE_PACKET_MAX];\n+\n+\tif (size > sizeof(packet_write_buffer) - 4) {\n+\t\terror(\"packet write failed\");\n+\t\treturn -1;\n+\t}\n+\tpacket_trace(buf, size, 1);\n+\tsize += 4;\n+\tset_packet_header(packet_write_buffer, size);\n+\tmemcpy(packet_write_buffer + 4, buf, size - 4);\n+\tif (write_in_full(fd_out, packet_write_buffer, size) == size)\n+\t\treturn 0;\n+\n+\terror(\"packet write failed\");\n+\treturn -1;\n+}\n+\n void packet_buf_write(struct strbuf *buf, const char *fmt, ...)\n {\n \tva_list args;\n-- \n2.10.0\n\n"},{"id":"301422","messageId":"20160908182132.50788-7-larsxschneider@gmail.com","threadId":"44041","inReplyTo":"20160908182132.50788-1-larsxschneider@gmail.com","subject":"[PATCH v7 06/10] pkt-line: add functions to read/write flush terminated packet streams","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-09-08T18:21:28Z","receivedAt":"2016-09-08T18:22:13Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\nwrite_packetized_from_fd() and write_packetized_from_buf() write a\nstream of packets. All content packets use the maximal packet size\nexcept for the last one. After the last content packet a `flush` control\npacket is written.\n\nread_packetized_to_buf() reads arbitrary sized packets until it detects\na `flush` packet.\n\nSigned-off-by: Lars Schneider \n---\n pkt-line.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n pkt-line.h | 7 +++++++\n 2 files changed, 75 insertions(+)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 1d3d725..5001a07 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -209,6 +209,47 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...)\n \tva_end(args);\n }\n \n+int write_packetized_from_fd(int fd_in, int fd_out)\n+{\n+\tstatic char buf[PKTLINE_DATA_MAXLEN];\n+\tint err = 0;\n+\tssize_t bytes_to_write;\n+\n+\twhile (!err) {\n+\t\tbytes_to_write = xread(fd_in, buf, sizeof(buf));\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\terr = packet_write_gently(fd_out, buf, bytes_to_write);\n+\t}\n+\tif (!err)\n+\t\terr = packet_flush_gently(fd_out);\n+\treturn err;\n+}\n+\n+int write_packetized_from_buf(const char *src_in, size_t len, int fd_out)\n+{\n+\tstatic char buf[PKTLINE_DATA_MAXLEN];\n+\tint err = 0;\n+\tsize_t bytes_written = 0;\n+\tsize_t bytes_to_write;\n+\n+\twhile (!err) {\n+\t\tif ((len - bytes_written) > sizeof(buf))\n+\t\t\tbytes_to_write = sizeof(buf);\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@@ -318,3 +359,30 @@ 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 read_packetized_to_buf(int fd_in, struct strbuf *sb_out)\n+{\n+\tint paket_len;\n+\tint options = PACKET_READ_GENTLE_ON_EOF;\n+\n+\tsize_t oldlen = sb_out->len;\n+\tsize_t oldalloc = sb_out->alloc;\n+\n+\tfor (;;) {\n+\t\tstrbuf_grow(sb_out, PKTLINE_DATA_MAXLEN+1);\n+\t\tpaket_len = packet_read(fd_in, NULL, NULL,\n+\t\t\tsb_out->buf + sb_out->len, PKTLINE_DATA_MAXLEN+1, options);\n+\t\tif (paket_len <= 0)\n+\t\t\tbreak;\n+\t\tsb_out->len += paket_len;\n+\t}\n+\n+\tif (paket_len < 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 paket_len;\n+\t}\n+\treturn sb_out->len - oldlen;\n+}\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 3fa0899..6df8449 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_fmt_gently(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n+int write_packetized_from_fd(int fd_in, int fd_out);\n+int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);\n \n /*\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 read_packetized_to_buf(int fd_in, struct strbuf *sb_out);\n+\n #define DEFAULT_PACKET_MAX 1000\n #define LARGE_PACKET_MAX 65520\n extern char packet_buffer[LARGE_PACKET_MAX];\n-- \n2.10.0\n\n"},{"id":"301423","messageId":"20160908182132.50788-10-larsxschneider@gmail.com","threadId":"44041","inReplyTo":"20160908182132.50788-1-larsxschneider@gmail.com","subject":"[PATCH v7 09/10] convert: make apply_filter() adhere to standard Git error handling","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-09-08T18:21:31Z","receivedAt":"2016-09-08T18:22:15Z","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 whereas `1` denoted success and `0`\nfailure. This 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 986c239..a068df7 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -451,7 +451,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@@ -478,22 +478,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.10.0\n\n"},{"id":"301424","messageId":"20160908182132.50788-9-larsxschneider@gmail.com","threadId":"44041","inReplyTo":"20160908182132.50788-1-larsxschneider@gmail.com","subject":"[PATCH v7 08/10] convert: modernize tests","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-09-08T18:21:30Z","receivedAt":"2016-09-08T18:22:40Z","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\nPlease note that the \"rot13\" filter configured in \"setup\" keeps using\n`git config` instead of `test_config` because subsequent tests might\ndepend on it.\n\nSigned-off-by: Lars Schneider \n---\n t/t0021-conversion.sh | 58 +++++++++++++++++++++++++--------------------------\n 1 file changed, 29 insertions(+), 29 deletions(-)\n\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex e799e59..dc50938 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\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.10.0\n\n"},{"id":"301425","messageId":"20160908182132.50788-8-larsxschneider@gmail.com","threadId":"44041","inReplyTo":"20160908182132.50788-1-larsxschneider@gmail.com","subject":"[PATCH v7 07/10] convert: quote filter names in error messages","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-09-08T18:21:29Z","receivedAt":"2016-09-08T18:22:47Z","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 077f5e6..986c239 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -412,7 +412,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@@ -430,13 +430,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@@ -477,15 +477,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.10.0\n\n"},{"id":"301426","messageId":"20160908182132.50788-11-larsxschneider@gmail.com","threadId":"44041","inReplyTo":"20160908182132.50788-1-larsxschneider@gmail.com","subject":"[PATCH v7 10/10] convert: add filter..process option","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-09-08T18:21:32Z","receivedAt":"2016-09-08T18:22:50Z","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\nevery single blob that is affected by a filter. If Git filters a lot of\nblobs then the startup time of the external filter processes can become\na significant part of the overall Git execution time.\n\nIn a preliminary performance test this developer used a clean/smudge\nfilter written in golang to filter 12,000 files. This process took 364s\nwith the existing filter mechanism and 5s with the new mechanism. See\ndetails here: https://github.com/github/git-lfs/pull/1382\n\nThis patch adds the `filter..process` string option which, if\nused, keeps the external filter process running and processes all blobs\nwith the packet format (pkt-line) based protocol over standard input and\nstandard output. The full protocol is explained in detail in\n`Documentation/gitattributes.txt`.\n\nA few key decisions:\n\n* The long running filter process is referred to as filter protocol\n version 2 because the existing single shot filter invocation is\n considered version 1.\n* Git sends a welcome message and expects a response right after the\n external filter process has started. This ensures that Git will not\n hang if a version 1 filter is incorrectly used with the\n filter..process option for version 2 filters. In addition,\n Git can detect this kind of error and warn the user.\n* The status of a filter operation (e.g. \"success\" or \"error) is set\n before the actual response and (if necessary!) re-set after the\n response. The advantage of this two step status response is that if\n the filter detects an error early, then the filter can communicate\n this and Git does not even need to create structures to read the\n response.\n* All status responses are pkt-line lists terminated with a flush\n packet. This allows us to send other status fields with the same\n protocol in the future.\n\nHelped-by: Martin-Louis Bright \nReviewed-by: Jakub Narebski \nSigned-off-by: Lars Schneider \n---\n Documentation/gitattributes.txt | 154 +++++++++++++-\n contrib/long-running-filter/example.pl | 111 ++++++++++\n convert.c | 349 ++++++++++++++++++++++++++++---\n pkt-line.h | 1 +\n t/t0021-conversion.sh | 365 ++++++++++++++++++++++++++++++++-\n t/t0021/rot13-filter.pl | 179 ++++++++++++++++\n unpack-trees.c | 1 +\n 7 files changed, 1129 insertions(+), 31 deletions(-)\n create mode 100755 contrib/long-running-filter/example.pl\n create mode 100755 t/t0021/rot13-filter.pl\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 7aff940..ac000ea 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -293,7 +293,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@@ -373,6 +379,152 @@ not exist, or may have different contents. So, smudge and clean commands\n should not try to access the file on disk, but only act as filters on the\n content provided to them on standard input.\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 (\"git-filter-client\"), a list of\n+supported protocol version numbers, and a flush packet. Git expects\n+to read a welcome response message (\"git-filter-server\") and exactly\n+one protocol version number from the previously sent list. All further\n+communication will be based on the selected version. The remaining\n+protocol description below documents \"version=2\". Please note that\n+\"version=42\" in the example below does not exist and is only there\n+to illustrate how the protocol would look like with more than one\n+version.\n+\n+After the version negotiation Git sends a list of supported capabilities\n+and a flush packet. Git expects to read a list of desired capabilities,\n+which must be a subset of the supported capabilities list, and a flush\n+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+If the filter experiences an error during processing, then it can\n+send the status \"error\" after the content was (partially or\n+completely) sent. Depending on the `filter..required` flag\n+Git will interpret that as error but it will not stop or restart the\n+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. Depending on the\n+`filter..required` flag Git will interpret that as error.\n+\n+The error handling for all cases above mimic the behavior of\n+the `filter..clean` / `filter..smudge` error\n+handling.\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 \"abort\" status. Depending on\n+the `filter..required` flag Git will interpret that as error\n+for the content as well as any future content for the lifetime of the\n+Git process but it will not stop or restart the filter process.\n+------------------------\n+packet: git< status=abort\\n\n+packet: git< 0000\n+------------------------\n+\n+After the filter has processed a blob it is expected to wait for\n+the next \"key=value\" list containing a command. Git will close\n+the command pipe on exit. The filter is expected to detect EOF\n+and exit gracefully on its own.\n+\n+A long running filter demo implementation can be found in\n+`contrib/long-running-filter/example.pl` located in the Git \n+core repository. If you develop your own long running filter \n+process then the `GIT_TRACE_PACKET` environment variables can be\n+very helpful for debugging (see linkgit:git[1]).\n+\n+If a `filter..process` command is configured then it\n+always takes precedence over a configured `filter..clean`\n+or `filter..smudge` 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/contrib/long-running-filter/example.pl b/contrib/long-running-filter/example.pl\nnew file mode 100755\nindex 0000000..279fbfb\n--- /dev/null\n+++ b/contrib/long-running-filter/example.pl\n@@ -0,0 +1,111 @@\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+\n+use strict;\n+use warnings;\n+\n+my $MAX_PACKET_CONTENT_SIZE = 65516;\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+( 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+packet_write( \"clean=true\\n\" );\n+packet_write( \"smudge=true\\n\" );\n+packet_flush();\n+\n+while (1) {\n+ my ($command) = packet_read() =~ /^command=([^=]+)\\n$/;\n+ my ($pathname) = packet_read() =~ /^pathname=([^=]+)\\n$/;\n+\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+ }\n+\n+ my $output;\n+ if ( $command eq \"clean\" ) {\n+ ### Perform clean here ###\n+ $output = $input;\n+ }\n+ elsif ( $command eq \"smudge\" ) {\n+ ### Perform smudge here ###\n+ $output = $input;\n+ }\n+ else {\n+ die \"bad command '$command'\";\n+ }\n+\n+ packet_write(\"status=success\\n\");\n+ packet_flush();\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(); # flush content!\n+ packet_flush(); # empty list!\n+}\ndiff --git a/convert.c b/convert.c\nindex a068df7..0ed48ed 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@@ -442,7 +443,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@@ -456,12 +457,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@@ -496,14 +491,318 @@ 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\tif (strlen(line) > PKTLINE_DATA_MAXLEN)\n+\t\t\treturn -1;\n+\t\terr = packet_write_fmt_gently(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+\n+\terr = (strlen(filter_type) > PKTLINE_DATA_MAXLEN);\n+\tif (err)\n+\t\tgoto done;\n+\terr = packet_write_fmt_gently(process->in, \"command=%s\\n\", filter_type);\n+\tif (err)\n+\t\tgoto done;\n+\n+\terr = (strlen(path) > PKTLINE_DATA_MAXLEN);\n+\tif (err)\n+\t\tgoto done;\n+\terr = packet_write_fmt_gently(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 = write_packetized_from_fd(fd, process->in);\n+\telse\n+\t\terr = write_packetized_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 = read_packetized_to_buf(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/* The filter signaled a problem with the file. */\n+\t\t} else if (!strcmp(filter_status.buf, \"abort\")) {\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 (!drv->process && (CAP_CLEAN & wanted_capability) && drv->clean)\n+\t\tcmd = drv->clean;\n+\telse if (!drv->process && (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@@ -541,6 +840,9 @@ 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+\n \tif (!strcmp(\"required\", key)) {\n \t\tdrv->required = git_config_bool(var, value);\n \t\treturn 0;\n@@ -842,7 +1144,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@@ -875,18 +1177,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@@ -908,9 +1204,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@@ -922,15 +1218,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@@ -939,9 +1229,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@@ -949,8 +1240,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@@ -1402,7 +1693,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/pkt-line.h b/pkt-line.h\nindex 6df8449..3d873f3 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -86,6 +86,7 @@ ssize_t read_packetized_to_buf(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 extern char packet_buffer[LARGE_PACKET_MAX];\n \n #endif\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex dc50938..1c98ac3 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -31,7 +31,10 @@ 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+\techo \"content-test2\" >test2.o &&\n+\techo \"content-test3-subdir\" >test3-subdir.o\n '\n \n script='s/^\\$Id: \\([0-9a-f]*\\) \\$/\\1/p'\n@@ -279,4 +282,364 @@ test_expect_success 'diff does not reuse worktree files that need cleaning' '\n \ttest_line_count = 0 count\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 STOP\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 STOP\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\t\tSTOP\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\t\tSTOP\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\t\tSTOP\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 clean only and take precedence' '\n+\ttest_config_global filter.protocol.clean ./../rot13.sh &&\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 STOP\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 STOP\n+\t\t\t\t\t1 wrote filter header\n+\t\t\t\tEOF\n+\t)\n+'\n+\n+generate_test_data () {\n+\tLEN=$1\n+\tNAME=$2\n+\ttest-genrandom end $LEN |\n+\t\tperl -pe \"s/./chr((ord($&) % 26) + 97)/sge\" >../$NAME.file &&\n+\tcp ../$NAME.file . &&\n+\t./../rot13.sh <../$NAME.file >../$NAME.file.rot13\n+}\n+\n+test_expect_success PERL 'required process filter should process multiple packets' '\n+\ttest_config_global filter.protocol.process \"$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge\" &&\n+\ttest_config_global filter.protocol.required true &&\n+\n+\trm -rf repo &&\n+\tmkdir repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit init &&\n+\n+\t\t# Generate data that requires 3 packets\n+\t\tPKTLINE_DATA_MAXLEN=65516 &&\n+\n+\t\tgenerate_test_data $(($PKTLINE_DATA_MAXLEN )) 1pkt_1__ &&\n+\t\tgenerate_test_data $(($PKTLINE_DATA_MAXLEN + 1)) 2pkt_1+1 &&\n+\t\tgenerate_test_data $(($PKTLINE_DATA_MAXLEN * 2 - 1)) 2pkt_2-1 &&\n+\t\tgenerate_test_data $(($PKTLINE_DATA_MAXLEN * 2 )) 2pkt_2__ &&\n+\t\tgenerate_test_data $(($PKTLINE_DATA_MAXLEN * 2 + 1)) 3pkt_2+1 &&\n+\n+\t\techo \"*.file filter=protocol\" >.gitattributes &&\n+\t\tcheck_filter \\\n+\t\t\tgit add *.file .gitattributes \\\n+\t\t\t\t<<-\\EOF &&\n+\t\t\t\t\t1 IN: clean 1pkt_1__.file 65516 [OK] -- OUT: 65516 . [OK]\n+\t\t\t\t\t1 IN: clean 2pkt_1+1.file 65517 [OK] -- OUT: 65517 .. [OK]\n+\t\t\t\t\t1 IN: clean 2pkt_2-1.file 131031 [OK] -- OUT: 131031 .. [OK]\n+\t\t\t\t\t1 IN: clean 2pkt_2__.file 131032 [OK] -- OUT: 131032 .. [OK]\n+\t\t\t\t\t1 IN: clean 3pkt_2+1.file 131033 [OK] -- OUT: 131033 ... [OK]\n+\t\t\t\t\t1 START\n+\t\t\t\t\t1 STOP\n+\t\t\t\t\t1 wrote filter header\n+\t\t\t\tEOF\n+\t\tgit commit . -m \"test commit\" &&\n+\n+\t\trm -f *.file &&\n+\t\tgit checkout -- *.file &&\n+\n+\t\tfor f in *.file\n+\t\tdo\n+\t\t\tgit cat-file blob :$f >actual &&\n+\t\t\ttest_cmp ../$f.rot13 actual\n+\t\tdone\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\t\tSTOP\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\t\tSTOP\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\" >abort.o &&\n+\t\tcp abort.o abort.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 abort.r 37 [OK] -- OUT: 0 [ABORT]\n+\t\t\t\t\tSTOP\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 abort.o abort.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..8e27877\n--- /dev/null\n+++ b/t/t0021/rot13-filter.pl\n@@ -0,0 +1,179 @@\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 \"abort.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+open my $debug, \">>\", \"rot13-filter.log\";\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+ print $debug \"STOP\\n\";\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+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 \"abort.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 \"abort.r\" ) {\n+ print $debug \"[ABORT]\\n\";\n+ $debug->flush();\n+ packet_write(\"status=abort\\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+ print $debug \".\";\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.10.0\n\n"},{"id":"301449","messageId":"CAGZ79kY0GaWuuh_MzKL6FZ7KWF2Kwhfh9qnEYd-qX8VDQWNmCQ@mail.gmail.com","threadId":"44041","inReplyTo":"20160908182132.50788-4-larsxschneider@gmail.com","subject":"Re: [PATCH v7 03/10] pkt-line: add packet_write_fmt_gently()","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-08T21:18:37Z","receivedAt":"2016-09-08T21:18:44Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Sep 8, 2016 at 11:21 AM, wrote:\n\n> +static int packet_write_fmt_1(int fd, int gently,\n> + const char *fmt, va_list args)\n> +{\n> + struct strbuf buf = STRBUF_INIT;\n> + size_t count;\n> +\n> + format_packet(&buf, fmt, args);\n> + count = write_in_full(fd, buf.buf, buf.len);\n> + if (count == buf.len)\n> + return 0;\n> +\n> + if (!gently) {\n\n call check_pipe from write_or_die here instead of\n reproducing that function?\n\n> + if (errno == EPIPE) {\n> + if (in_async())\n> + async_exit(141);\n> +\n> + signal(SIGPIPE, SIG_DFL);\n> + raise(SIGPIPE);\n> + /* Should never happen, but just in case... */\n> + exit(141);\n> + }\n> + die_errno(\"packet write error\");\n> + }\n> + error(\"packet write failed\");\n> + return -1;\n\nI think the more idiomatic way is to\n\n return error(...);\n\nas error always return -1.\n"},{"id":"301451","messageId":"CAGZ79kZdroDdD5SHP+-9svSTYbJfn2vsFXAwC4aen3hMVEOOPA@mail.gmail.com","threadId":"44041","inReplyTo":"20160908182132.50788-6-larsxschneider@gmail.com","subject":"Re: [PATCH v7 05/10] pkt-line: add packet_write_gently()","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-08T21:24:02Z","receivedAt":"2016-09-08T21:24:08Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Sep 8, 2016 at 11:21 AM, wrote:\n> From: Lars Schneider \n>\n> packet_write_fmt_gently() uses format_packet() which lets the caller\n> only send string data via \"%s\". That means it cannot be used for\n> arbitrary data that may contain NULs.\n\nMakes sense.\n\n>\n> Add packet_write_gently() which writes arbitrary data and returns `0`\n> for success and `-1` for an error.\n\nI think documenting the return code is better done in either the header file\nor in a commend preceding the implementation instead of the commit message?\n\nMaybe just a generic comment for *_gently is good enough, maybe even no\ncomment. So the commit is fine, too. I dunno.\n\n\n> This function is used by other\n> pkt-line functions in a subsequent patch.\n\nThat's what I figured. Do we also need to mention that in the preceding patch\nfor packet_flush_gently ?\n\n>\n> Signed-off-by: Lars Schneider \n> ---\n> pkt-line.c | 19 +++++++++++++++++++\n> 1 file changed, 19 insertions(+)\n>\n> diff --git a/pkt-line.c b/pkt-line.c\n> index 37345ca..1d3d725 100644\n> --- a/pkt-line.c\n> +++ b/pkt-line.c\n> @@ -181,6 +181,25 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)\n> return status;\n> }\n>\n> +int packet_write_gently(const int fd_out, const char *buf, size_t size)\n> +{\n> + static char packet_write_buffer[LARGE_PACKET_MAX];\n> +\n> + if (size > sizeof(packet_write_buffer) - 4) {\n> + error(\"packet write failed\");\n> + return -1;\n> + }\n> + packet_trace(buf, size, 1);\n> + size += 4;\n> + set_packet_header(packet_write_buffer, size);\n> + memcpy(packet_write_buffer + 4, buf, size - 4);\n> + if (write_in_full(fd_out, packet_write_buffer, size) == size)\n> + return 0;\n> +\n> + error(\"packet write failed\");\n> + return -1;\n> +}\n> +\n> void packet_buf_write(struct strbuf *buf, const char *fmt, ...)\n> {\n> va_list args;\n> --\n> 2.10.0\n>\n"},{"id":"301459","messageId":"CAGZ79kYHX6-hhKW_6JEd1hBNi8R+sBY-RjZARfHDMnyLKFZnsQ@mail.gmail.com","threadId":"44041","inReplyTo":"20160908182132.50788-7-larsxschneider@gmail.com","subject":"Re: [PATCH v7 06/10] pkt-line: add functions to read/write flush terminated packet streams","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-08T21:49:32Z","receivedAt":"2016-09-08T21:49:38Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Sep 8, 2016 at 11:21 AM, wrote:\n> From: Lars Schneider \n>\n> write_packetized_from_fd() and write_packetized_from_buf() write a\n> stream of packets. All content packets use the maximal packet size\n> except for the last one. After the last content packet a `flush` control\n> packet is written.\n\nI presume we need both write_* things in a later patch; can you clarify why\nwe need both of them?\n\n\n> + if (paket_len < 0) {\n> + if (oldalloc == 0)\n> + strbuf_release(sb_out);\n\nSo if old alloc is 0, we release it, which is documented as\n/**\n * Release a string buffer and the memory it used. You should not use the\n * string buffer after using this function, unless you initialize it again.\n */\n\n> + else\n> + strbuf_setlen(sb_out, oldlen);\n\nOtherwise we just set the length back, such that it looks like before.\n\nSo as a caller the strbuf is in a different state in case of error\ndepending whether\nthe strbuf already had some data in it. I think it would be better if\nwe only did\n`strbuf_setlen(sb_out, oldlen);` here, such that the caller can\nstrbuf_release it\nunconditionally.\n\nOr to make things more confusing, you could use strbuf_reset in case of 0,\nas that is a strbuf_setlen internally. ;)\n\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\nStrictly speaking we read until a packet of size 0 appears, but as per\nthe implementation\nof packet_read we cannot distinguish between \"0000\" and \"0004\", i.e.\nan empty non-flush\npacket. So I think we're fine both in the implementation as well as\nthe documentation here.\n"},{"id":"301460","messageId":"CAGZ79kbG3Zza0e_kKhz5sBZcippwjewEBasYCioJF9drEov_Aw@mail.gmail.com","threadId":"44041","inReplyTo":"20160908182132.50788-9-larsxschneider@gmail.com","subject":"Re: [PATCH v7 08/10] convert: modernize tests","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-08T22:05:27Z","receivedAt":"2016-09-08T22:05:39Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Sep 8, 2016 at 11:21 AM, wrote:\n> From: Lars Schneider \n>\n> Use `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\n> after \">\" and \"<\".\n>\n> Please note that the \"rot13\" filter configured in \"setup\" keeps using\n> `git config` instead of `test_config` because subsequent tests might\n> depend on it.\n>\n> Signed-off-by: Lars Schneider \n> ---\n\nMakes sense & Reviewed-by \"Stefan Beller \"\n\nThanks,\nStefan\n"},{"id":"301590","messageId":"20160910062919.GB11001@tb-raspi","threadId":"44041","inReplyTo":"20160908182132.50788-11-larsxschneider@gmail.com","subject":"Re: [PATCH v7 10/10] convert: add filter..process option","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-09-10T06:29:20Z","receivedAt":"2016-09-10T06:29:43Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Thu, Sep 08, 2016 at 08:21:32PM +0200, larsxschneider@gmail.com wrote:\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\nIt's probalby only me who has difficulties to distinguish\n'>' from '<'.\n\npacket: git> git-filter-client\npacket: git> version=2\npacket: git> version=42\npacket: git> 0000\npacket: filter> git-filter-server\npacket: filter> version=2\n\n(Otherwise the dialoge description is nice)\n\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\nHow do we send pathnames the have '\\n' ?\nNot really recommended, but allowed.\nAnd here I am a little bit lost, is each of the lines packed into\na pkt-line ?\ncommand=smudge is packet as pkt-line and pathname= is packed into\nanother one ? (The we don't need the '\\n' at all)\nOr go both lines into one pkt-line (thats what I think), then\nwe don't need the '\\n' afther the pathname.\nAnd in this case the pathname is always the last element, and a '\\n'\nmay occur in the pathname, since we know the length of the packet\nwe know how long the pathname must be.\n\n\n\n> +packet: git> 0000\n> +packet: git> CONTENT\n> +packet: git> 0000\n> +------------------------\n> +\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\nDoes not want ? \nI can see something like \"I read through the file, there is nothing\nto do. So Git, I don't send anything back, you know where the file is.\n\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> +If the filter experiences an error during processing, then it can\n> +send the status \"error\" after the content was (partially or\n> +completely) sent. Depending on the `filter..required` flag\n> +Git will interpret that as error but it will not stop or restart the\n> +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\nMy personal comment:\nWhen a filter is mis-behaving, Git should say so loud and clear, and\ndie(). \nThe filter process can be left running, so that it can be debugged.\n\nHere I stopped the review for a moment, \nI still think that Git shouldn't kill anything, because we loose\nthe ability to debug these processes.\n\n"},{"id":"301604","messageId":"20160910164056.GA14646@tb-raspi","threadId":"44041","inReplyTo":"20160908182132.50788-11-larsxschneider@gmail.com","subject":"Re: [PATCH v7 10/10] convert: add filter..process option","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-09-10T16:40:57Z","receivedAt":"2016-09-10T16:41:23Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"[]\n\nOne general question up here, more comments inline.\nThe current order for a clean-filter is like this, I removed the error handling:\n\nint convert_to_git()\n{\n\tret |= apply_filter(path, src, len, -1, dst, filter);\n\tif (ret && dst) {\n\t\tsrc = dst->buf;\n\t\tlen = dst->len;\n\t}\n\tret |= crlf_to_git(path, src, len, dst, ca.crlf_action, checksafe);\n\treturn ret | ident_to_git(path, src, len, dst, ca.ident);\n}\n\nThe first step is the clean filter, the CRLF-LF conversion (if needed),\nthen ident.\nThe current implementation streams the whole file content to the filter,\n(STDIN of the filter) and reads back STDOUT from the filter into a STRBUF.\nThis is to use the UNIX-like STDIN--STDOUT method for writing a filter.\n\nHowever, int would_convert_to_git_filter_fd() and convert_to_git_filter_fd()\noffer a sort of short-cut:\nThe filter reads from the file directly, and the output of the filter is\nread into a STRBUF.\n\nIt looks as if the multi-filter approach can use this in a similar way:\nGive the pathname to the filter, the filter opens the file for reading\nand stream the result via the pkt-line protocol into Git.\nThis needs some more changes, and may be very well go into a separate patch\nseries. (and should).\n\nWhat I am asking for:\nWhen a multi-filter is used, the content is handled to the filter via pkt-line,\nand the result is given to Git via pkt-line ?\nNothing wrong with it, I just wonder, if it should be mentioned somewhere.\n\n> diff --git a/contrib/long-running-filter/example.pl b/contrib/long-running-filter/example.pl\n> new file mode 100755\n> index 0000000..279fbfb\n> --- /dev/null\n> +++ b/contrib/long-running-filter/example.pl\n> @@ -0,0 +1,111 @@\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> +\n> +use strict;\n> +use warnings;\n> +\n> +my $MAX_PACKET_CONTENT_SIZE = 65516;\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\nThis is half-kosher, I would say,\n(And I really. really would like to see an implementation in C ;-)\n\nA read function may look like this:\n\n ret = read(0, &buffer, 4);\n if (ret < 0) {\n /* Error, nothing we can do */\n exit(1);\n } else if (ret == 0) {\n /* EOF */\n exit(0);\n } else if (ret < 4) {\n /* \n * Go and read more, until we have 4 bytes or EOF or Error */\n } else {\n /* Good case, see below */\n }\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> +( 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> +packet_write( \"clean=true\\n\" );\n> +packet_write( \"smudge=true\\n\" );\n> +packet_flush();\n> +\n> +while (1) {\n> + my ($command) = packet_read() =~ /^command=([^=]+)\\n$/;\n> + my ($pathname) = packet_read() =~ /^pathname=([^=]+)\\n$/;\n> +\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> + }\n> +\n> + my $output;\n> + if ( $command eq \"clean\" ) {\n> + ### Perform clean here ###\n> + $output = $input;\n> + }\n> + elsif ( $command eq \"smudge\" ) {\n> + ### Perform smudge here ###\n> + $output = $input;\n> + }\n> + else {\n> + die \"bad command '$command'\";\n> + }\n> +\n> + packet_write(\"status=success\\n\");\n> + packet_flush();\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(); # flush content!\n> + packet_flush(); # empty list!\n> +}\n> diff --git a/convert.c b/convert.c\n> index a068df7..0ed48ed 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> @@ -442,7 +443,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> @@ -456,12 +457,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> @@ -496,14 +491,318 @@ 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\nIs CAP_ too generic, and GIT_FILTER_CAP (or so) less calling for trouble ? \n\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\tif (strlen(line) > PKTLINE_DATA_MAXLEN)\n> +\t\t\treturn -1;\n> +\t\terr = packet_write_fmt_gently(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> +\n> +\terr = (strlen(filter_type) > PKTLINE_DATA_MAXLEN);\n\nExtra () needed ?\nMore () in the code...\n\nNo more comments today (except that the kill is overkill)\n"},{"id":"301657","messageId":"714C1346-6A7D-4A8B-AAA2-BA936EB6B739@gmail.com","threadId":"44041","inReplyTo":"CAGZ79kY0GaWuuh_MzKL6FZ7KWF2Kwhfh9qnEYd-qX8VDQWNmCQ@mail.gmail.com","subject":"Re: [PATCH v7 03/10] pkt-line: add packet_write_fmt_gently()","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-09-11T11:36:25Z","receivedAt":"2016-09-11T11:43:17Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 08 Sep 2016, at 23:18, Stefan Beller wrote:\n> \n> On Thu, Sep 8, 2016 at 11:21 AM, wrote:\n> \n>> +static int packet_write_fmt_1(int fd, int gently,\n>> + const char *fmt, va_list args)\n>> +{\n>> + struct strbuf buf = STRBUF_INIT;\n>> + size_t count;\n>> +\n>> + format_packet(&buf, fmt, args);\n>> + count = write_in_full(fd, buf.buf, buf.len);\n>> + if (count == buf.len)\n>> + return 0;\n>> +\n>> + if (!gently) {\n> \n> call check_pipe from write_or_die here instead of\n> reproducing that function?\n\nYes, might be better. I wasn't sure because the check_pipe is\nnot public.\n\nWhere would you declare check_pipe? In cache.h?\nMaybe it would be more suitable to move check_pipe to \nrun-command.h/c?\n\n\n>> + if (errno == EPIPE) {\n>> + if (in_async())\n>> + async_exit(141);\n>> +\n>> + signal(SIGPIPE, SIG_DFL);\n>> + raise(SIGPIPE);\n>> + /* Should never happen, but just in case... */\n>> + exit(141);\n>> + }\n>> + die_errno(\"packet write error\");\n>> + }\n>> + error(\"packet write failed\");\n>> + return -1;\n> \n> I think the more idiomatic way is to\n> \n> return error(...);\n> \n> as error always return -1.\n\nOf course!!\n\n\nThank you,\nLars\n\n"},{"id":"301658","messageId":"7EA8897A-B283-44B3-BC9B-5F0518C9C1CA@gmail.com","threadId":"44041","inReplyTo":"CAGZ79kZdroDdD5SHP+-9svSTYbJfn2vsFXAwC4aen3hMVEOOPA@mail.gmail.com","subject":"Re: [PATCH v7 05/10] pkt-line: add packet_write_gently()","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-09-11T11:44:29Z","receivedAt":"2016-09-11T11:44:36Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 08 Sep 2016, at 23:24, Stefan Beller wrote:\n> \n> On Thu, Sep 8, 2016 at 11:21 AM, wrote:\n> \n>> \n>> Add packet_write_gently() which writes arbitrary data and returns `0`\n>> for success and `-1` for an error.\n> \n> I think documenting the return code is better done in either the header file\n> or in a commend preceding the implementation instead of the commit message?\n> \n> Maybe just a generic comment for *_gently is good enough, maybe even no\n> comment. So the commit is fine, too. I dunno.\n\nI agree that this is too verbose as this function follows the standard\nGit return value conventions (AFAIK). I'll remove this from all commit\nmessages.\n\n\n>> This function is used by other\n>> pkt-line functions in a subsequent patch.\n> \n> That's what I figured. Do we also need to mention that in the preceding patch\n> for packet_flush_gently ?\n\nI'll add this note to all commit messages that introduce new functions which\nare used later.\n\n\nThanks,\nLars"},{"id":"301659","messageId":"6CCFB3A0-2C3B-4142-8E62-47C3DD6AE7A3@gmail.com","threadId":"44041","inReplyTo":"CAGZ79kYHX6-hhKW_6JEd1hBNi8R+sBY-RjZARfHDMnyLKFZnsQ@mail.gmail.com","subject":"Re: [PATCH v7 06/10] pkt-line: add functions to read/write flush terminated packet streams","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-09-11T12:33:19Z","receivedAt":"2016-09-11T12:33:35Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 08 Sep 2016, at 23:49, Stefan Beller wrote:\n> \n> On Thu, Sep 8, 2016 at 11:21 AM, wrote:\n>> From: Lars Schneider \n>> \n>> write_packetized_from_fd() and write_packetized_from_buf() write a\n>> stream of packets. All content packets use the maximal packet size\n>> except for the last one. After the last content packet a `flush` control\n>> packet is written.\n> \n> I presume we need both write_* things in a later patch; can you clarify why\n> we need both of them?\n\nSince 9035d7 Git streams from fd to required filters and from buf to\nnon-required filters. The Git filter protocol v2 makes use of all that,\ntoo.\n\nhttps://github.com/git/git/commit/9035d75a2be9d80d82676504d69553245017f6d4\n\n\n>> + if (paket_len < 0) {\n>> + if (oldalloc == 0)\n>> + strbuf_release(sb_out);\n> \n> So if old alloc is 0, we release it, which is documented as\n> /**\n> * Release a string buffer and the memory it used. You should not use the\n> * string buffer after using this function, unless you initialize it again.\n> */\n> \n>> + else\n>> + strbuf_setlen(sb_out, oldlen);\n> \n> Otherwise we just set the length back, such that it looks like before.\n> \n> So as a caller the strbuf is in a different state in case of error\n> depending whether\n> the strbuf already had some data in it. I think it would be better if\n> we only did\n> `strbuf_setlen(sb_out, oldlen);` here, such that the caller can\n> strbuf_release it\n> unconditionally.\n\nI tried to mimic the behavior of strbuf_read() [1]. The error handling\nwas introduced in 2fc647 [2] to ease error handling:\n\n\"This allows for easier error handling, as callers only need to call\nstrbuf_release() if A) the command succeeded or B) if they would have had\nto do so anyway because they added something to the strbuf themselves.\"\n\nDoes this convince you to keep the proposed error handling? If yes, then\nI would add a comment to the function to document that behavior explicitly!\n\n[1] https://github.com/git/git/blob/cda1bbd474805e653dda8a71d4ea3790e2a66cbb/strbuf.c#L377-L383\n[2] https://github.com/git/git/commit/2fc647004ac7016128372a85db8245581e493812\n\n\n> Or to make things more confusing, you could use strbuf_reset in case of 0,\n> as that is a strbuf_setlen internally. ;)\n\n\nThanks,\nLars"},{"id":"301660","messageId":"02449FE2-5997-4BDB-A3F4-2276470F27B9@gmail.com","threadId":"44041","inReplyTo":"CAGZ79kbG3Zza0e_kKhz5sBZcippwjewEBasYCioJF9drEov_Aw@mail.gmail.com","subject":"Re: [PATCH v7 08/10] convert: modernize tests","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-09-11T12:34:49Z","receivedAt":"2016-09-11T12:34:57Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 09 Sep 2016, at 00:05, Stefan Beller wrote:\n> \n> On Thu, Sep 8, 2016 at 11:21 AM, wrote:\n>> From: Lars Schneider \n>> \n>> Use `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\n>> after \">\" and \"<\".\n>> \n>> Please note that the \"rot13\" filter configured in \"setup\" keeps using\n>> `git config` instead of `test_config` because subsequent tests might\n>> depend on it.\n>> \n>> Signed-off-by: Lars Schneider \n>> ---\n> \n> Makes sense & Reviewed-by \"Stefan Beller \"\n\nThank you,\nLars\n\n"},{"id":"301662","messageId":"CAGZ79kZaRdJqbjfacdxT5eqOaK-RObJo+AddZ8heaU-KzWCOwQ@mail.gmail.com","threadId":"44041","inReplyTo":"714C1346-6A7D-4A8B-AAA2-BA936EB6B739@gmail.com","subject":"Re: [PATCH v7 03/10] pkt-line: add packet_write_fmt_gently()","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-11T16:01:16Z","receivedAt":"2016-09-11T16:01:21Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sun, Sep 11, 2016 at 4:36 AM, Lars Schneider\n wrote:\n\n>>\n>> call check_pipe from write_or_die here instead of\n>> reproducing that function?\n>\n> Yes, might be better. I wasn't sure because the check_pipe is\n> not public.\n>\n> Where would you declare check_pipe? In cache.h?\n\nIIRC, once upon a time the community decided to not\nclutter cache.h any more as it is like a dirty kitchen sink,\npiling up all unrelated things, but on the other hand that\nwould be handy.\n\n> Maybe it would be more suitable to move check_pipe to\n> run-command.h/c?\n\nThat's certainly possible.\nI don't have a strong opinion, where the code actually\nresides, but I do have a strong-ish opinion on code\nduplication. ;)\n"},{"id":"301663","messageId":"CAGZ79kY4_ASUopbXg3AgtGu-hJ87thD46NgzggeB1635Yvk5dA@mail.gmail.com","threadId":"44041","inReplyTo":"6CCFB3A0-2C3B-4142-8E62-47C3DD6AE7A3@gmail.com","subject":"Re: [PATCH v7 06/10] pkt-line: add functions to read/write flush terminated packet streams","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-11T16:03:56Z","receivedAt":"2016-09-11T16:04:01Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sun, Sep 11, 2016 at 5:33 AM, Lars Schneider\n wrote:\n\n> Does this convince you to keep the proposed error handling? If yes, then\n> I would add a comment to the function to document that behavior explicitly!\n\noops. I should read the docs more carefully.\n\nThanks for pointing out.\nThen I'd be happy with the patch as is.\n\nThanks,\nStefan\n"},{"id":"301669","messageId":"xmqq4m5m865t.fsf@gitster.mtv.corp.google.com","threadId":"44041","inReplyTo":"CAGZ79kYHX6-hhKW_6JEd1hBNi8R+sBY-RjZARfHDMnyLKFZnsQ@mail.gmail.com","subject":"Re: [PATCH v7 06/10] pkt-line: add functions to read/write flush terminated packet streams","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-11T21:42:38Z","receivedAt":"2016-09-11T21:42:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> So as a caller the strbuf is in a different state in case of error\n> depending whether\n> the strbuf already had some data in it. I think it would be better if\n> we only did\n> `strbuf_setlen(sb_out, oldlen);` here, such that the caller can\n> strbuf_release it\n> unconditionally.\n\nIf the caller _knows_ that the strbuf is no longer needed, it can\nunconditionally call strbuf_release() on it, whether its buffer was\nalready released or only its length was set to 0, no?\n\nThe callee is merely trying to be nice by resetting the strbuf to a\nstate close to the original in the error return codepath, I would\nthink. It may be debatable if such a niceness is needed, but it is\na different matter that does not relate to the burden imposed on the\ncaller.\n"},{"id":"301716","messageId":"0365589B-7733-4C97-A566-957BACC67B29@gmail.com","threadId":"44041","inReplyTo":"CAGZ79kZaRdJqbjfacdxT5eqOaK-RObJo+AddZ8heaU-KzWCOwQ@mail.gmail.com","subject":"Re: [PATCH v7 03/10] pkt-line: add packet_write_fmt_gently()","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-09-12T09:22:24Z","receivedAt":"2016-09-12T11:58:04Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 11 Sep 2016, at 18:01, Stefan Beller wrote:\n> \n> On Sun, Sep 11, 2016 at 4:36 AM, Lars Schneider\n> wrote:\n> \n>>> \n>>> call check_pipe from write_or_die here instead of\n>>> reproducing that function?\n>> [...]\n> \n>> Maybe it would be more suitable to move check_pipe to\n>> run-command.h/c?\n> \n> That's certainly possible.\n> I don't have a strong opinion, where the code actually\n> resides, but I do have a strong-ish opinion on code\n> duplication. ;)\n\nOK, then I will move check_pipe() to run-command.\n\nThanks,\nLars\n\n"},{"id":"301717","messageId":"10020380-76ED-4371-A0BA-59D07AF33CE0@gmail.com","threadId":"44041","inReplyTo":"20160910062919.GB11001@tb-raspi","subject":"Re: [PATCH v7 10/10] convert: add filter..process option","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-09-12T09:49:15Z","receivedAt":"2016-09-12T11:58:15Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 10 Sep 2016, at 08:29, Torsten Bögershausen wrote:\n> \n> On Thu, Sep 08, 2016 at 08:21:32PM +0200, larsxschneider@gmail.com wrote:\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> It's probalby only me who has difficulties to distinguish\n> '>' from '<'.\n\nI see what you mean. However, this format is used with \"GIT_TRACE_PACKET\"\nas well and therefore I would prefer to keep it.\n\n\n> packet: git> git-filter-client\n> packet: git> version=2\n> packet: git> version=42\n> packet: git> 0000\n> packet: filter> git-filter-server\n> packet: filter> version=2\n> \n> (Otherwise the dialoge description is nice)\n\nThanks!\n\n\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> \n> How do we send pathnames the have '\\n' ?\n> Not really recommended, but allowed.\n> And here I am a little bit lost, is each of the lines packed into\n> a pkt-line ?\n> command=smudge is packet as pkt-line and pathname= is packed into\n> another one ? (The we don't need the '\\n' at all)\n\nEvery line is a dedicated packet. That's why '\\n' in a path name would\nnot be a problem as the receiver is expected to read the entire packet\nwhen parsing the value (and the receiver knows the packet length, too).\n\nThe '\\n' at the end is required by the pkt-line format:\n\"A non-binary line SHOULD BE terminated by an LF...\"\n(see protocol-common.txt)\n\n\n> Or go both lines into one pkt-line (thats what I think), then\n> we don't need the '\\n' afther the pathname.\n\nNo (see above).\n\n\n> And in this case the pathname is always the last element, and a '\\n'\n> may occur in the pathname, since we know the length of the packet\n> we know how long the pathname must be.\n> \n> \n> [...]\n>> \n>> +In case the filter cannot or does not want to process the content,\n> \n> Does not want ? \n> I can see something like \"I read through the file, there is nothing\n> to do. So Git, I don't send anything back, you know where the file is.\n\nThat's right. Isn't that covered with \"does not want\"?\n\n\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>> +If the filter experiences an error during processing, then it can\n>> +send the status \"error\" after the content was (partially or\n>> +completely) sent. Depending on the `filter..required` flag\n>> +Git will interpret that as error but it will not stop or restart the\n>> +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> \n> My personal comment:\n> When a filter is mis-behaving, Git should say so loud and clear, and\n> die(). \n> The filter process can be left running, so that it can be debugged.\n\nIn the current implementation Git would die already if the filter is \n\"required\". In this particular case we *could* die, too. However, \nI would prefer to keep it as is because I think the users of a \n\"non-required\" filter do not expect Git to die if there is *any* \nproblem with the filter.\n\n\n> Here I stopped the review for a moment, \n> I still think that Git shouldn't kill anything, because we loose\n> the ability to debug these processes.\n\nBased on my experience debugging the filter in this state is hard\nanyways. I think a user would rather try to reproduce the problem\nand run Git with the \"GIT_TRACE_PACKET\" flag enabled for debugging.\n\n\nThanks,\nLars"},{"id":"301782","messageId":"xmqqinu0y9v1.fsf@gitster.mtv.corp.google.com","threadId":"44041","inReplyTo":"20160908182132.50788-5-larsxschneider@gmail.com","subject":"Re: [PATCH v7 04/10] pkt-line: add packet_flush_gently()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-12T23:30:26Z","receivedAt":"2016-09-12T23:30:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"larsxschneider@gmail.com writes:\n\n> From: Lars Schneider \n>\n> packet_flush() would die in case of a write error even though for some\n> callers an error would be acceptable. Add packet_flush_gently() which\n> writes a pkt-line flush packet and returns `0` for success and `-1` for\n> failure.\n> ...\n> +int packet_flush_gently(int fd)\n> +{\n> +\tpacket_trace(\"0000\", 4, 1);\n> +\tif (write_in_full(fd, \"0000\", 4) == 4)\n> +\t\treturn 0;\n> +\terror(\"flush packet write failed\");\n> +\treturn -1;\n\nIt is more idiomatic to do\n\n\treturn error(...);\n\nbut more importantly, does the caller even want an error message\nunconditionally printed here?\n\nI suspect that it is a strong sign that the caller wants to be in\ncontrol of when and what error message is produced; otherwise it\nwouldn't be calling the _gently() variant, no?\n\nOf course, if you have written callers to this function in later\npatches in this series, they would be responsible for reporting (or\nchoosing not to report) this failure, but I think making this\nfunction silent is a better course in the longer term.\n"},{"id":"301783","messageId":"xmqqeg4oy9tw.fsf@gitster.mtv.corp.google.com","threadId":"44041","inReplyTo":"20160908182132.50788-6-larsxschneider@gmail.com","subject":"Re: [PATCH v7 05/10] pkt-line: add packet_write_gently()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-12T23:31:07Z","receivedAt":"2016-09-12T23:31:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"larsxschneider@gmail.com writes:\n\n> +int packet_write_gently(const int fd_out, const char *buf, size_t size)\n> +{\n> +\tstatic char packet_write_buffer[LARGE_PACKET_MAX];\n> +\n> +\tif (size > sizeof(packet_write_buffer) - 4) {\n> +\t\terror(\"packet write failed\");\n> +\t\treturn -1;\n> +\t}\n> +\tpacket_trace(buf, size, 1);\n> +\tsize += 4;\n> +\tset_packet_header(packet_write_buffer, size);\n> +\tmemcpy(packet_write_buffer + 4, buf, size - 4);\n> +\tif (write_in_full(fd_out, packet_write_buffer, size) == size)\n> +\t\treturn 0;\n> +\n> +\terror(\"packet write failed\");\n> +\treturn -1;\n> +}\n\nThe same comment as 4/10 applies here.\n"},{"id":"301844","messageId":"96554f6d-988d-e0b8-7936-8d0f29a7564f@web.de","threadId":"44041","inReplyTo":"10020380-76ED-4371-A0BA-59D07AF33CE0@gmail.com","subject":"Re: [PATCH v7 10/10] convert: add filter..process option","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-09-13T14:44:18Z","receivedAt":"2016-09-13T14:44:46Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 12.09.16 11:49, Lars Schneider wrote:\n\n>> How do we send pathnames the have '\\n' ?\n>> Not really recommended, but allowed.\n>> And here I am a little bit lost, is each of the lines packed into\n>> a pkt-line ?\n>> command=smudge is packet as pkt-line and pathname= is packed into\n>> another one ? (The we don't need the '\\n' at all)\n> \n> Every line is a dedicated packet. That's why '\\n' in a path name would\n> not be a problem as the receiver is expected to read the entire packet\n> when parsing the value (and the receiver knows the packet length, too).\n> \n> The '\\n' at the end is required by the pkt-line format:\n> \"A non-binary line SHOULD BE terminated by an LF...\"\n> (see protocol-common.txt)\n\nThat is only the half part of the story:\nA non-binary line SHOULD BE terminated by an LF, which if present\nMUST be included in the total length. Receivers MUST treat pkt-lines\nwith non-binary data the same whether or not they contain the trailing\nLF (stripping the LF if present, and not complaining when it is\nmissing).\n\n\nHow do we treat pathnames ?\nThey can have each byte value except '\\0'.\nWhat should a receiver do, which reads a string like \"ABC\\n\\n\" ?\nIs it \"ABC\\n\" or \"ABC\\n\\n\" ?\n\nI would really consider to treat pathnames as binary, and not add a trailing '\\n',\nare there other opinions ?\n \n\n\n\n\n\n"},{"id":"301846","messageId":"xmqq8tuvx1sz.fsf@gitster.mtv.corp.google.com","threadId":"44041","inReplyTo":"20160908182132.50788-11-larsxschneider@gmail.com","subject":"Re: [PATCH v7 10/10] convert: add filter..process option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-13T15:22:04Z","receivedAt":"2016-09-13T15:22:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"larsxschneider@gmail.com writes:\n\n> diff --git a/contrib/long-running-filter/example.pl b/contrib/long-running-filter/example.pl\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> +packet_write( \"clean=true\\n\" );\n> +packet_write( \"smudge=true\\n\" );\n> +packet_flush();\n> +\n> +while (1) {\n\nThese extra SP around the contents inside () pair look unfamiliar\nand somewhat strange to me, but as long as they are consistently\ndone (and I think you are mostly being consistent), it is OK.\n\n> +#define CAP_CLEAN (1u<<0)\n> +#define CAP_SMUDGE (1u<<1)\n\nAs these are meant to be usable together, i.e. bits in a single flag\nword, they are of type \"unsigned int\", which makes perfect sense.\n\nMake sure your variables and fields that store them are of the same\ntype. I think I saw \"int' used to pass them in at least one place.\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\"const char *cmd = NULL;\" of course.\n\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index 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\nWhy? The resulting file seems to compile without this addition.\n"},{"id":"301848","messageId":"xmqq4m5jx00o.fsf@gitster.mtv.corp.google.com","threadId":"44041","inReplyTo":"20160908182132.50788-1-larsxschneider@gmail.com","subject":"Re: [PATCH v7 00/10] Git filter protocol","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-13T16:00:39Z","receivedAt":"2016-09-13T16:01:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"larsxschneider@gmail.com writes:\n\n> From: Lars Schneider \n>\n> The goal of this series is to avoid launching a new clean/smudge filter\n> process for each file that is filtered.\n\nI tentatively queued this in 'pu' because I wanted to see these\nchanges in context and also wanted to know if there are overlaps and\nconflicts with other topics in flight. As your earlier steps\nrenamed packet_write() to packet_write_fmt() but didn't add a new\npacket_write() that has different semantics, it was pleasantly easy\nto make sure there is no new caller added in the meantime (it did\nconflict with Duy's shallow-deepen topic and the resolution had to\ntouch outside the <<< conflicted === regions >>>, but it was\notherwise trivial).\n\nSome details of the protocol (I think Torsten has already pointed\nout that not all paths can be representable with the current\nincarnation; there may be other minor nits like that) may still need\nto be worked out, but I think the series at this point gets the\nbasic code structure right and such additional fixes would not\nchange things too drastically (IOW, I think we are very close to\nbeing 'next'-ready).\n\nThanks.\n\n"},{"id":"301853","messageId":"xmqqvaxzvjj0.fsf@gitster.mtv.corp.google.com","threadId":"44041","inReplyTo":"96554f6d-988d-e0b8-7936-8d0f29a7564f@web.de","subject":"Re: [PATCH v7 10/10] convert: add filter..process option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-13T16:42:11Z","receivedAt":"2016-09-13T16:42:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> I would really consider to treat pathnames as binary, and not add a trailing '\\n',\n> are there other opinions ?\n\nIt would be the most consistent if the same format as\nwrite_name_quoted() is used for this, I would think.\n"},{"id":"301874","messageId":"6CF905A2-3A06-42C8-8E28-463E50867036@gmail.com","threadId":"44041","inReplyTo":"20160910164056.GA14646@tb-raspi","subject":"Re: [PATCH v7 10/10] convert: add filter..process option","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-09-13T22:04:49Z","receivedAt":"2016-09-13T22:05:03Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 10 Sep 2016, at 17:40, Torsten Bögershausen wrote:\n> \n> []\n> \n> One general question up here, more comments inline.\n> The current order for a clean-filter is like this, I removed the error handling:\n> \n> int convert_to_git()\n> {\n> \tret |= apply_filter(path, src, len, -1, dst, filter);\n> \tif (ret && dst) {\n> \t\tsrc = dst->buf;\n> \t\tlen = dst->len;\n> \t}\n> \tret |= crlf_to_git(path, src, len, dst, ca.crlf_action, checksafe);\n> \treturn ret | ident_to_git(path, src, len, dst, ca.ident);\n> }\n> \n> The first step is the clean filter, the CRLF-LF conversion (if needed),\n> then ident.\n> The current implementation streams the whole file content to the filter,\n> (STDIN of the filter) and reads back STDOUT from the filter into a STRBUF.\n> This is to use the UNIX-like STDIN--STDOUT method for writing a filter.\n> \n> However, int would_convert_to_git_filter_fd() and convert_to_git_filter_fd()\n> offer a sort of short-cut:\n> The filter reads from the file directly, and the output of the filter is\n> read into a STRBUF.\n\nAre you sure? As far as I understand the code the filter does not read from \nthe file in any case today. The functions would_convert_to_git_filter_fd() and \nconvert_to_git_filter_fd() just avoid avoid mapping the file in Git. The content \nis still streamed via pipes:\nhttps://github.com/git/git/commit/9035d75a2be9d80d82676504d69553245017f6d4\n\n\n> It looks as if the multi-filter approach can use this in a similar way:\n> Give the pathname to the filter, the filter opens the file for reading\n> and stream the result via the pkt-line protocol into Git.\n> This needs some more changes, and may be very well go into a separate patch\n> series. (and should).\n> \n> What I am asking for:\n> When a multi-filter is used, the content is handled to the filter via pkt-line,\n> and the result is given to Git via pkt-line ?\n> Nothing wrong with it, I just wonder, if it should be mentioned somewhere.\n\nThat is most certainly a good idea and the main reason I added \"capabilities\"\nto the protocol. Joey Hess worked on this topic (not merged, yet) and I would \nlike to make this available to the long-running filter protocol as soon as the\nfeature is available:\nhttp://public-inbox.org/git/1468277112-9909-1-git-send-email-joeyh@joeyh.name/\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> \n> This is half-kosher, I would say,\n> (And I really. really would like to see an implementation in C ;-)\n\nWould you be willing to contribute a patch? :-)\n\n\n> A read function may look like this:\n> \n> ret = read(0, &buffer, 4);\n> if (ret < 0) {\n> /* Error, nothing we can do */\n> exit(1);\n> } else if (ret == 0) {\n> /* EOF */\n> exit(0);\n> } else if (ret < 4) {\n> /* \n> * Go and read more, until we have 4 bytes or EOF or Error */\n> } else {\n> /* Good case, see below */\n> }\n\nI see. However, my intention was to provide an absolute minimal\nexample to teach a reader how the protocol works. I consider\nall proper error handling an exercise for the reader ;-)\n\n\n>> +#define CAP_CLEAN (1u<<0)\n>> +#define CAP_SMUDGE (1u<<1)\n> \n> Is CAP_ too generic, and GIT_FILTER_CAP (or so) less calling for trouble ?\n\nI had something like that but Junio suggested these names in V4:\nhttp://public-inbox.org/git/xmqq8twd8uld.fsf@gitster.mtv.corp.google.com/\n\n\n>> +\n>> +\terr = (strlen(filter_type) > PKTLINE_DATA_MAXLEN);\n> \n> Extra () needed ?\n> More () in the code...\n\nI thought it might improve readability, but I will remove them\nif you think this would be more consistent with existing Git code.\n\n\nThanks,\nLars"},{"id":"301875","messageId":"B138B703-08A6-45C4-9D54-80F87C75356B@gmail.com","threadId":"44041","inReplyTo":"xmqqinu0y9v1.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v7 04/10] pkt-line: add packet_flush_gently()","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-09-13T22:12:33Z","receivedAt":"2016-09-13T22:12:42Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 13 Sep 2016, at 00:30, Junio C Hamano wrote:\n> \n> larsxschneider@gmail.com writes:\n> \n>> From: Lars Schneider \n>> \n>> packet_flush() would die in case of a write error even though for some\n>> callers an error would be acceptable. Add packet_flush_gently() which\n>> writes a pkt-line flush packet and returns `0` for success and `-1` for\n>> failure.\n>> ...\n>> +int packet_flush_gently(int fd)\n>> +{\n>> +\tpacket_trace(\"0000\", 4, 1);\n>> +\tif (write_in_full(fd, \"0000\", 4) == 4)\n>> +\t\treturn 0;\n>> +\terror(\"flush packet write failed\");\n>> +\treturn -1;\n> \n> It is more idiomatic to do\n> \n> \treturn error(...);\n> \n> but more importantly, does the caller even want an error message\n> unconditionally printed here?\n> \n> I suspect that it is a strong sign that the caller wants to be in\n> control of when and what error message is produced; otherwise it\n> wouldn't be calling the _gently() variant, no?\n\nAgreed!\n\nThanks,\nLars\n"},{"id":"301880","messageId":"xmqq37l3v2rk.fsf@gitster.mtv.corp.google.com","threadId":"44041","inReplyTo":"B138B703-08A6-45C4-9D54-80F87C75356B@gmail.com","subject":"Re: [PATCH v7 04/10] pkt-line: add packet_flush_gently()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-13T22:44:15Z","receivedAt":"2016-09-13T22:44:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n>> On 13 Sep 2016, at 00:30, Junio C Hamano wrote:\n>> \n>> larsxschneider@gmail.com writes:\n>> \n>>> From: Lars Schneider \n>>> \n>>> packet_flush() would die in case of a write error even though for some\n>>> callers an error would be acceptable. Add packet_flush_gently() which\n>>> writes a pkt-line flush packet and returns `0` for success and `-1` for\n>>> failure.\n>>> ...\n>>> +int packet_flush_gently(int fd)\n>>> +{\n>>> +\tpacket_trace(\"0000\", 4, 1);\n>>> +\tif (write_in_full(fd, \"0000\", 4) == 4)\n>>> +\t\treturn 0;\n>>> +\terror(\"flush packet write failed\");\n>>> +\treturn -1;\n>> \n>> It is more idiomatic to do\n>> \n>> \treturn error(...);\n>> \n>> but more importantly, does the caller even want an error message\n>> unconditionally printed here?\n>> \n>> I suspect that it is a strong sign that the caller wants to be in\n>> control of when and what error message is produced; otherwise it\n>> wouldn't be calling the _gently() variant, no?\n>\n> Agreed!\n\nI am also OK with the current form, too. Those who need to enhance\nit to packet_flush_gently(int fd, int quiet) can come later.\n\n"},{"id":"302003","messageId":"385016D3-8298-4273-81E7-876CB25B2789@gmail.com","threadId":"44041","inReplyTo":"xmqq37l3v2rk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v7 04/10] pkt-line: add packet_flush_gently()","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-09-15T16:42:58Z","receivedAt":"2016-09-15T19:36:03Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 13 Sep 2016, at 23:44, Junio C Hamano wrote:\n> \n> Lars Schneider writes:\n> \n>>> On 13 Sep 2016, at 00:30, Junio C Hamano wrote:\n>>> \n>>> larsxschneider@gmail.com writes:\n>>> \n>>>> From: Lars Schneider \n>>>> \n>>>> packet_flush() would die in case of a write error even though for some\n>>>> callers an error would be acceptable. Add packet_flush_gently() which\n>>>> writes a pkt-line flush packet and returns `0` for success and `-1` for\n>>>> failure.\n>>>> ...\n>>>> +int packet_flush_gently(int fd)\n>>>> +{\n>>>> +\tpacket_trace(\"0000\", 4, 1);\n>>>> +\tif (write_in_full(fd, \"0000\", 4) == 4)\n>>>> +\t\treturn 0;\n>>>> +\terror(\"flush packet write failed\");\n>>>> +\treturn -1;\n>>> \n>>> It is more idiomatic to do\n>>> \n>>> \treturn error(...);\n>>> \n>>> but more importantly, does the caller even want an error message\n>>> unconditionally printed here?\n>>> \n>>> I suspect that it is a strong sign that the caller wants to be in\n>>> control of when and what error message is produced; otherwise it\n>>> wouldn't be calling the _gently() variant, no?\n>> \n>> Agreed!\n> \n> I am also OK with the current form, too. Those who need to enhance\n> it to packet_flush_gently(int fd, int quiet) can come later.\n\n\"caller wants to be in control [...] otherwise it wouldn't be calling \nthe _gently() variant\" convinced me. I would like to change it like\nthis:\n\n\ttrace_printf_key(&trace_packet, \"flush packet write failed\");\n\treturn -1;\n\nObjections?\n\nThanks,\nLars\n"},{"id":"302005","messageId":"5E675A29-628F-4076-A5D9-E359CCF04295@gmail.com","threadId":"44041","inReplyTo":"xmqqvaxzvjj0.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v7 10/10] convert: add filter..process option","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-09-15T17:23:25Z","receivedAt":"2016-09-15T19:36:17Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 13 Sep 2016, at 17:42, Junio C Hamano wrote:\n> \n> Torsten Bögershausen writes:\n> \n>> I would really consider to treat pathnames as binary, and not add a trailing '\\n',\n>> are there other opinions ?\n> \n> It would be the most consistent if the same format as\n> write_name_quoted() is used for this, I would think.\n\nIs that the solution you had in mind?\n\n\tquote_c_style(path, "ed_path, NULL, 0);\n\terr = packet_write_fmt_gently(process->in, \"pathname=%s\\n\", quoted_path.buf);\n\nWouldn't that complicate the pathname parsing on the filter side?\nCan't we just define in our filter protocol documentation that our \n\"pathname\" packet _always_ has a trailing \"\\n\"? That would mean the \nreceiver would know a packet \"pathname=ABC\\n\\n\" encodes the path\n\"ABC\\n\" [1].\n\nThanks,\nLars\n\n\n[1] Following Torsten's example in \nhttp://public-inbox.org/git/96554f6d-988d-e0b8-7936-8d0f29a7564f@web.de )\n\n"},{"id":"302007","messageId":"20160915194443.x7zvkkryvworqcxt@sigill.intra.peff.net","threadId":"44041","inReplyTo":"385016D3-8298-4273-81E7-876CB25B2789@gmail.com","subject":"Re: [PATCH v7 04/10] pkt-line: add packet_flush_gently()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-15T19:44:45Z","receivedAt":"2016-09-15T19:45:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 15, 2016 at 05:42:58PM +0100, Lars Schneider wrote:\n\n> >>>> +int packet_flush_gently(int fd)\n> >>>> +{\n> >>>> +\tpacket_trace(\"0000\", 4, 1);\n> >>>> +\tif (write_in_full(fd, \"0000\", 4) == 4)\n> >>>> +\t\treturn 0;\n> >>>> +\terror(\"flush packet write failed\");\n> >>>> +\treturn -1;\n> [...]\n> >>> I suspect that it is a strong sign that the caller wants to be in\n> >>> control of when and what error message is produced; otherwise it\n> >>> wouldn't be calling the _gently() variant, no?\n> >> \n> >> Agreed!\n> > \n> > I am also OK with the current form, too. Those who need to enhance\n> > it to packet_flush_gently(int fd, int quiet) can come later.\n> \n> \"caller wants to be in control [...] otherwise it wouldn't be calling \n> the _gently() variant\" convinced me. I would like to change it like\n> this:\n> \n> \ttrace_printf_key(&trace_packet, \"flush packet write failed\");\n> \treturn -1;\n> \n> Objections?\n\nI'm not sure that a trace makes sense, because it means that 99% of the\ntime we are silent. AFAICT, the question is not \"sometimes the user\nneeds to see an error and sometimes not, and they should decide before\nstarting the program\". It is \"sometimes the caller will report the error\nto the user as appropriate, and sometimes we need to do so\". And only\nthe calling code knows which is which.\n\nSo the \"right\" pattern is either:\n\n 1. Return -1 and the caller is responsible for telling the user.\n\nor\n\n 2. Return -1 and stuff the error into an error strbuf, so it can be\n passed up the call chain easily (and callers do not have to come up\n with their own wording).\n\nBut if all current callers would just call error() themselves anyway,\nthen it's OK to punt on this and let somebody else handle it later if\nthey add a new caller who wants different behavior (and that is what\nJunio was saying above, I think).\n\n-Peff\n"},{"id":"302010","messageId":"xmqq37l1nd42.fsf@gitster.mtv.corp.google.com","threadId":"44041","inReplyTo":"5E675A29-628F-4076-A5D9-E359CCF04295@gmail.com","subject":"Re: [PATCH v7 10/10] convert: add filter..process option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-15T20:04:45Z","receivedAt":"2016-09-15T20:04:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n> Wouldn't that complicate the pathname parsing on the filter side?\n> Can't we just define in our filter protocol documentation that our \n> \"pathname\" packet _always_ has a trailing \"\\n\"? That would mean the \n> receiver would know a packet \"pathname=ABC\\n\\n\" encodes the path\n> \"ABC\\n\" [1].\n\nThat's fine, too. If you declare that pathname over the protocol is\na binary thing, you can also define that the packet does not have\nthe terminating \\n, i.e. the example encodes the path \"ABC\\n\\n\",\nwhich is also OK ;-)\n\nAs long as the rule is clearly documented, easy for filter\nimplementors to follow it, and hard for them to get it wrong, I'd be\nperfectly happy.\n\nThanks.\n"},{"id":"302012","messageId":"928655D2-312B-4805-99DB-E73448232B9A@gmail.com","threadId":"44041","inReplyTo":"xmqq8tuvx1sz.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v7 10/10] convert: add filter..process option","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-09-15T20:16:41Z","receivedAt":"2016-09-15T20:16:47Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 13 Sep 2016, at 17:22, Junio C Hamano wrote:\n> \n> larsxschneider@gmail.com writes:\n> \n>> diff --git a/contrib/long-running-filter/example.pl b/contrib/long-running-filter/example.pl\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>> +packet_write( \"clean=true\\n\" );\n>> +packet_write( \"smudge=true\\n\" );\n>> +packet_flush();\n>> +\n>> +while (1) {\n> \n> These extra SP around the contents inside () pair look unfamiliar\n> and somewhat strange to me, but as long as they are consistently\n> done (and I think you are mostly being consistent), it is OK.\n\nUps. I forgot to run PerlTidy here. I run PerlTidy with the flag \n\"-pbp\" (= Perl Best Practices). This seems to add no extra SP for\nfunctions with one parameter (e.g. `foo(\"bar\")`) and extra SP\nfor functions with multiple parameter (e.g. `foo( \"bar\", 1 )`).\nIs this still OK?\n\nDoes anyone have a \"Git PerlTidy configuration\"?\n\n\n> \n>> +#define CAP_CLEAN (1u<<0)\n>> +#define CAP_SMUDGE (1u<<1)\n> \n> As these are meant to be usable together, i.e. bits in a single flag\n> word, they are of type \"unsigned int\", which makes perfect sense.\n> \n> Make sure your variables and fields that store them are of the same\n> type. I think I saw \"int' used to pass them in at least one place.\n\nFixed!\n\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> \"const char *cmd = NULL;\" of course.\n\nFixed!\n\n\n>> diff --git a/unpack-trees.c b/unpack-trees.c\n>> index 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> \n> Why? The resulting file seems to compile without this addition.\n\nOf course. That shouldn't have been part of this commit.\n\n\nThank you,\nLars\n\n\n\n\n"},{"id":"302014","messageId":"744FA3D5-888A-4032-90C4-6BFC7D5D4010@gmail.com","threadId":"44041","inReplyTo":"20160915194443.x7zvkkryvworqcxt@sigill.intra.peff.net","subject":"Re: [PATCH v7 04/10] pkt-line: add packet_flush_gently()","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-09-15T20:19:19Z","receivedAt":"2016-09-15T20:19:25Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 15 Sep 2016, at 21:44, Jeff King wrote:\n> \n> On Thu, Sep 15, 2016 at 05:42:58PM +0100, Lars Schneider wrote:\n> \n>>>>>> +int packet_flush_gently(int fd)\n>>>>>> +{\n>>>>>> +\tpacket_trace(\"0000\", 4, 1);\n>>>>>> +\tif (write_in_full(fd, \"0000\", 4) == 4)\n>>>>>> +\t\treturn 0;\n>>>>>> +\terror(\"flush packet write failed\");\n>>>>>> +\treturn -1;\n>> [...]\n>>>>> I suspect that it is a strong sign that the caller wants to be in\n>>>>> control of when and what error message is produced; otherwise it\n>>>>> wouldn't be calling the _gently() variant, no?\n>>>> \n>>>> Agreed!\n>>> \n>>> I am also OK with the current form, too. Those who need to enhance\n>>> it to packet_flush_gently(int fd, int quiet) can come later.\n>> \n>> \"caller wants to be in control [...] otherwise it wouldn't be calling \n>> the _gently() variant\" convinced me. I would like to change it like\n>> this:\n>> \n>> \ttrace_printf_key(&trace_packet, \"flush packet write failed\");\n>> \treturn -1;\n>> \n>> Objections?\n> \n> I'm not sure that a trace makes sense, because it means that 99% of the\n> time we are silent. AFAICT, the question is not \"sometimes the user\n> needs to see an error and sometimes not, and they should decide before\n> starting the program\". It is \"sometimes the caller will report the error\n> to the user as appropriate, and sometimes we need to do so\". And only\n> the calling code knows which is which.\n> \n> So the \"right\" pattern is either:\n> \n> 1. Return -1 and the caller is responsible for telling the user.\n> \n> or\n> \n> 2. Return -1 and stuff the error into an error strbuf, so it can be\n> passed up the call chain easily (and callers do not have to come up\n> with their own wording).\n> \n> But if all current callers would just call error() themselves anyway,\n> then it's OK to punt on this and let somebody else handle it later if\n> they add a new caller who wants different behavior (and that is what\n> Junio was saying above, I think).\n\nOK. I'll go with 1. then.\n\nThanks,\nLars\n"},{"id":"302015","messageId":"xmqqpoo4nc7d.fsf@gitster.mtv.corp.google.com","threadId":"44041","inReplyTo":"928655D2-312B-4805-99DB-E73448232B9A@gmail.com","subject":"Re: [PATCH v7 10/10] convert: add filter..process option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-15T20:24:22Z","receivedAt":"2016-09-15T20:24:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n>> On 13 Sep 2016, at 17:22, Junio C Hamano wrote:\n>> \n>> larsxschneider@gmail.com writes:\n>> \n>>> diff --git a/contrib/long-running-filter/example.pl b/contrib/long-running-filter/example.pl\n>>> ...\n>>> +packet_write( \"clean=true\\n\" );\n>>> +packet_write( \"smudge=true\\n\" );\n>>\n>> These extra SP around the contents inside () pair look unfamiliar\n>> and somewhat strange to me, but as long as they are consistently\n>> done (and I think you are mostly being consistent), it is OK.\n>\n> Ups. I forgot to run PerlTidy here. I run PerlTidy with the flag \n> \"-pbp\" (= Perl Best Practices). This seems to add no extra SP for\n> functions with one parameter (e.g. `foo(\"bar\")`) and extra SP\n> for functions with multiple parameter (e.g. `foo( \"bar\", 1 )`).\n> Is this still OK?\n\nYour choice. I already said I do not care too much either way as\nlong as you are consistent.\n\nIf you prefer PerlTidy's PBP output over what you wrote, and if you\nare resending the patch anyway, then why not? ;-)\n\n> Does anyone have a \"Git PerlTidy configuration\"?\n\nNot me.\n\nThanks.\n"},{"id":"302016","messageId":"xmqqlgysnbrt.fsf@gitster.mtv.corp.google.com","threadId":"44041","inReplyTo":"744FA3D5-888A-4032-90C4-6BFC7D5D4010@gmail.com","subject":"Re: [PATCH v7 04/10] pkt-line: add packet_flush_gently()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-15T20:33:42Z","receivedAt":"2016-09-15T20:33:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n>> So the \"right\" pattern is either:\n>> \n>> 1. Return -1 and the caller is responsible for telling the user.\n>> \n\n... which is valid only if there aren't different kinds of errors\nthat all return -1; with \"return error(...)\" with different\nmessages, the users can tell what kind of error they got (while the\ncaller may just do the same abort-procedure no matter what kind of\nerror it got), but if all of them are replaced with \"return -1\", the\ncaller cannot produce different error messages to tell the users.\n\n>> 2. Return -1 and stuff the error into an error strbuf, so it can be\n>> passed up the call chain easily (and callers do not have to come up\n>> with their own wording).\n\n... and this would become one of the viable options (the other is to\ndefine your own error code so that the caller can tell what error it\ngot).\n\n>> But if all current callers would just call error() themselves anyway,\n>> then it's OK to punt on this and let somebody else handle it later if\n>> they add a new caller who wants different behavior (and that is what\n>> Junio was saying above, I think).\n\nYes. Just keeping it noisy until somebody wants a quiet-and-gentle\nversion is probably the best course to take.\n"},{"id":"302863","messageId":"30461d2d-5a9c-4b0c-25a0-aa3327622764@web.de","threadId":"44041","inReplyTo":"xmqq37l1nd42.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v7 10/10] convert: add filter..process option","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-09-29T06:33:46Z","receivedAt":"2016-09-29T06:34:44Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 15.09.16 22:04, Junio C Hamano wrote:\n> Lars Schneider writes:\n> \n>> Wouldn't that complicate the pathname parsing on the filter side?\n>> Can't we just define in our filter protocol documentation that our \n>> \"pathname\" packet _always_ has a trailing \"\\n\"? That would mean the \n>> receiver would know a packet \"pathname=ABC\\n\\n\" encodes the path\n>> \"ABC\\n\" [1].\n> \n> That's fine, too. If you declare that pathname over the protocol is\n> a binary thing, you can also define that the packet does not have\n> the terminating \\n, i.e. the example encodes the path \"ABC\\n\\n\",\n> which is also OK ;-)\n> \n> As long as the rule is clearly documented, easy for filter\n> implementors to follow it, and hard for them to get it wrong, I'd be\n> perfectly happy.\n>\n\n(Sorry for the late reply)\n\nIn V8 the additional \"\\n\" is clearly documented.\n\nOn the long run,\nI would suggest to be more clear what BINARY is:\n\n--- a/Documentation/technical/protocol-common.txt\n+++ b/Documentation/technical/protocol-common.txt\n@@ -61,6 +61,9 @@ the length's hexadecimal representation.\n A pkt-line MAY contain binary data, so implementors MUST ensure\n pkt-line parsing/formatting routines are 8-bit clean.\n \n+Each pkt-line that may contain ASCII control characters should\n+be treated as binary.\n+\n\n"},{"id":"302874","messageId":"1a5e30b0-ffa1-fbef-fda5-88e363d9a530@gmail.com","threadId":"44041","inReplyTo":"30461d2d-5a9c-4b0c-25a0-aa3327622764@web.de","subject":"Re: [PATCH v7 10/10] convert: add filter..process option","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2016-09-29T09:37:54Z","receivedAt":"2016-09-29T09:38:10Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W dniu 29.09.2016 o 08:33, Torsten Bögershausen pisze:\n> On 15.09.16 22:04, Junio C Hamano wrote:\n>> Lars Schneider writes:\n>>\n>>> Wouldn't that complicate the pathname parsing on the filter side?\n>>> Can't we just define in our filter protocol documentation that our \n>>> \"pathname\" packet _always_ has a trailing \"\\n\"? That would mean the \n>>> receiver would know a packet \"pathname=ABC\\n\\n\" encodes the path\n>>> \"ABC\\n\" [1].\n>>\n>> That's fine, too. If you declare that pathname over the protocol is\n>> a binary thing, you can also define that the packet does not have\n>> the terminating \\n, i.e. the example encodes the path \"ABC\\n\\n\",\n>> which is also OK ;-)\n>>\n>> As long as the rule is clearly documented, easy for filter\n>> implementors to follow it, and hard for them to get it wrong, I'd be\n>> perfectly happy.\n>>\n> \n> (Sorry for the late reply)\n> \n> In V8 the additional \"\\n\" is clearly documented.\n> \n> On the long run,\n> I would suggest to be more clear what BINARY is:\n> \n> --- a/Documentation/technical/protocol-common.txt\n> +++ b/Documentation/technical/protocol-common.txt\n> @@ -61,6 +61,9 @@ the length's hexadecimal representation.\n> A pkt-line MAY contain binary data, so implementors MUST ensure\n> pkt-line parsing/formatting routines are 8-bit clean.\n> \n> +Each pkt-line that may contain ASCII control characters should\n> +be treated as binary.\n> +\n\nWell, it is not as clear cut with pathnames. Sane pathnames should\nnot contain control characters, even if they are outside US-ASCII,\nassuming sane filesystem pathnames charset (like UTF-8).\n\nOne thing pathname cannot include is NUL (\"\\0\") character.\n\nSo in most cases they are ASCII, but might not be. Not that \npkt-line text packets are binary-unsafe... I think the trailing\n\"\\n\" is here for easier debugging.\n\nhttp://www.dwheeler.com/essays/filenames-in-shell.html\nhttp://www.dwheeler.com/essays/fixing-unix-linux-filenames.html\n\n-- \nJakub Narębski\n"}]}