{"thread":{"id":"43930","subject":"[PATCH v6 10/13] convert: generate large test files only once","startedAt":"2016-08-25T11:19:42Z","lastAt":"2016-09-05T19:48:02Z","messageCount":66,"participants":["larsxschneider@gmail.com","Stefan Beller","Lars Schneider","Junio C Hamano","Jacob Keller","Jeff King","Torsten Bögershausen","Jakub Narębski"],"isPatch":true,"patchVersion":6,"patchTotal":13},"messages":[{"id":"300138","messageId":"20160825110752.31581-11-larsxschneider@gmail.com","threadId":"43930","inReplyTo":"20160825110752.31581-1-larsxschneider@gmail.com","subject":"[PATCH v6 10/13] convert: generate large test files only once","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-25T11:07:49Z","receivedAt":"2016-08-25T11:19:42Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\nGenerate more interesting large test files with pseudo random characters\nin between and reuse these test files in multiple tests. Run tests\nformerly marked as EXPENSIVE every time but with a smaller data set.\n\nSigned-off-by: Lars Schneider \n---\n t/t0021-conversion.sh | 48 ++++++++++++++++++++++++++++++++++++++----------\n 1 file changed, 38 insertions(+), 10 deletions(-)\n\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex 7b45136..34c8eb9 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -4,6 +4,15 @@ test_description='blob conversion via gitattributes'\n \n . ./test-lib.sh\n \n+if test_have_prereq EXPENSIVE\n+then\n+\tT0021_LARGE_FILE_SIZE=2048\n+\tT0021_LARGISH_FILE_SIZE=100\n+else\n+\tT0021_LARGE_FILE_SIZE=30\n+\tT0021_LARGISH_FILE_SIZE=2\n+fi\n+\n cat <rot13.sh\n #!$SHELL_PATH\n tr \\\n@@ -31,7 +40,26 @@ test_expect_success setup '\n \tcat test >test.i &&\n \tgit add test test.t test.i &&\n \trm -f test test.t test.i &&\n-\tgit checkout -- test test.t test.i\n+\tgit checkout -- test test.t test.i &&\n+\n+\tmkdir generated-test-data &&\n+\tfor i in $(test_seq 1 $T0021_LARGE_FILE_SIZE)\n+\tdo\n+\t\tRANDOM_STRING=\"$(test-genrandom end $i | tr -dc \"A-Za-z0-9\" )\"\n+\t\tROT_RANDOM_STRING=\"$(echo $RANDOM_STRING | ./rot13.sh )\"\n+\t\t# Generate 1MB of empty data and 100 bytes of random characters\n+\t\t# printf \"$(test-genrandom start $i)\"\n+\t\tprintf \"%1048576d\" 1 >>generated-test-data/large.file &&\n+\t\tprintf \"$RANDOM_STRING\" >>generated-test-data/large.file &&\n+\t\tprintf \"%1048576d\" 1 >>generated-test-data/large.file.rot13 &&\n+\t\tprintf \"$ROT_RANDOM_STRING\" >>generated-test-data/large.file.rot13 &&\n+\n+\t\tif test $i = $T0021_LARGISH_FILE_SIZE\n+\t\tthen\n+\t\t\tcat generated-test-data/large.file >generated-test-data/largish.file &&\n+\t\t\tcat generated-test-data/large.file.rot13 >generated-test-data/largish.file.rot13\n+\t\tfi\n+\tdone\n '\n \n script='s/^\\$Id: \\([0-9a-f]*\\) \\$/\\1/p'\n@@ -199,9 +227,9 @@ test_expect_success 'required filter clean failure' '\n test_expect_success 'filtering large input to small output should use little memory' '\n \ttest_config filter.devnull.clean \"cat >/dev/null\" &&\n \ttest_config filter.devnull.required true &&\n-\tfor i in $(test_seq 1 30); do printf \"%1048576d\" 1; done >30MB &&\n-\techo \"30MB filter=devnull\" >.gitattributes &&\n-\tGIT_MMAP_LIMIT=1m GIT_ALLOC_LIMIT=1m git add 30MB\n+\tcp generated-test-data/large.file large.file &&\n+\techo \"large.file filter=devnull\" >.gitattributes &&\n+\tGIT_MMAP_LIMIT=1m GIT_ALLOC_LIMIT=1m git add large.file\n '\n \n test_expect_success 'filter that does not read is fine' '\n@@ -214,15 +242,15 @@ test_expect_success 'filter that does not read is fine' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success EXPENSIVE 'filter large file' '\n+test_expect_success 'filter large file' '\n \ttest_config filter.largefile.smudge cat &&\n \ttest_config filter.largefile.clean cat &&\n-\tfor i in $(test_seq 1 2048); do printf \"%1048576d\" 1; done >2GB &&\n-\techo \"2GB filter=largefile\" >.gitattributes &&\n-\tgit add 2GB 2>err &&\n+\techo \"large.file filter=largefile\" >.gitattributes &&\n+\tcp generated-test-data/large.file large.file &&\n+\tgit add large.file 2>err &&\n \ttest_must_be_empty err &&\n-\trm -f 2GB &&\n-\tgit checkout -- 2GB 2>err &&\n+\trm -f large.file &&\n+\tgit checkout -- large.file 2>err &&\n \ttest_must_be_empty err\n '\n \n-- \n2.9.2\n\n"},{"id":"300139","messageId":"20160825110752.31581-7-larsxschneider@gmail.com","threadId":"43930","inReplyTo":"20160825110752.31581-1-larsxschneider@gmail.com","subject":"[PATCH v6 06/13] pkt-line: add functions to read/write flush terminated packet streams","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-25T11:07:45Z","receivedAt":"2016-08-25T11:19:57Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\npacket_write_stream_with_flush_from_fd() and\npacket_write_stream_with_flush_from_buf() write a stream of packets. All\ncontent packets use the maximal packet size except for the last one.\nAfter the last content packet a `flush` control packet is written.\n\npacket_read_till_flush() reads arbitrary sized packets until it detects\na `flush` packet.\n\nSigned-off-by: Lars Schneider \n---\n pkt-line.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n pkt-line.h | 7 +++++\n 2 files changed, 98 insertions(+)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 7e8a803..3033aa3 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -176,6 +176,47 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...)\n \tva_end(args);\n }\n \n+int packet_write_stream_with_flush_from_fd(int fd_in, int fd_out)\n+{\n+\tint err = 0;\n+\tssize_t bytes_to_write;\n+\n+\twhile (!err) {\n+\t\tbytes_to_write = xread(fd_in, packet_write_buffer, sizeof(packet_write_buffer) - 4);\n+\t\tif (bytes_to_write < 0)\n+\t\t\treturn COPY_READ_ERROR;\n+\t\tif (bytes_to_write == 0)\n+\t\t\tbreak;\n+\t\tif (bytes_to_write > sizeof(packet_write_buffer) - 4)\n+\t\t\treturn COPY_WRITE_ERROR;\n+\t\terr = packet_write_gently(fd_out, packet_write_buffer, bytes_to_write);\n+\t}\n+\tif (!err)\n+\t\terr = packet_flush_gently(fd_out);\n+\treturn err;\n+}\n+\n+int packet_write_stream_with_flush_from_buf(const char *src_in, size_t len, int fd_out)\n+{\n+\tint err = 0;\n+\tsize_t bytes_written = 0;\n+\tsize_t bytes_to_write;\n+\n+\twhile (!err) {\n+\t\tif ((len - bytes_written) > sizeof(packet_write_buffer) - 4)\n+\t\t\tbytes_to_write = sizeof(packet_write_buffer) - 4;\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@@ -285,3 +326,53 @@ char *packet_read_line_buf(char **src, size_t *src_len, int *dst_len)\n {\n \treturn packet_read_line_generic(-1, src, src_len, dst_len);\n }\n+\n+ssize_t packet_read_till_flush(int fd_in, struct strbuf *sb_out)\n+{\n+\tint len, ret;\n+\tint options = PACKET_READ_GENTLE_ON_EOF;\n+\tchar linelen[4];\n+\n+\tsize_t oldlen = sb_out->len;\n+\tsize_t oldalloc = sb_out->alloc;\n+\n+\tfor (;;) {\n+\t\t/* Read packet header */\n+\t\tret = get_packet_data(fd_in, NULL, NULL, linelen, 4, options);\n+\t\tif (ret < 0)\n+\t\t\tgoto done;\n+\t\tlen = packet_length(linelen);\n+\t\tif (len < 0)\n+\t\t\tdie(\"protocol error: bad line length character: %.4s\", linelen);\n+\t\tif (!len) {\n+\t\t\t/* Found a flush packet - Done! */\n+\t\t\tpacket_trace(\"0000\", 4, 0);\n+\t\t\tbreak;\n+\t\t}\n+\t\tlen -= 4;\n+\n+\t\t/* Read packet content */\n+\t\tstrbuf_grow(sb_out, len);\n+\t\tret = get_packet_data(fd_in, NULL, NULL, sb_out->buf + sb_out->len, len, options);\n+\t\tif (ret < 0)\n+\t\t\tgoto done;\n+\n+\t\tif (ret != len) {\n+\t\t\terror(\"protocol error: incomplete read (expected %d, got %d)\", len, ret);\n+\t\t\tgoto done;\n+\t\t}\n+\n+\t\tpacket_trace(sb_out->buf + sb_out->len, len, 0);\n+\t\tsb_out->len += len;\n+\t}\n+\n+done:\n+\tif (ret < 0) {\n+\t\tif (oldalloc == 0)\n+\t\t\tstrbuf_release(sb_out);\n+\t\telse\n+\t\t\tstrbuf_setlen(sb_out, oldlen);\n+\t\treturn ret; /* unexpected EOF */\n+\t}\n+\treturn sb_out->len - oldlen;\n+}\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 3fa0899..9616117 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 packet_write_stream_with_flush_from_fd(int fd_in, int fd_out);\n+int packet_write_stream_with_flush_from_buf(const char *src_in, size_t len, int fd_out);\n \n /*\n * Read a packetized line into the buffer, which must be at least size bytes\n@@ -77,6 +79,11 @@ char *packet_read_line(int fd, int *size);\n */\n char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size);\n \n+/*\n+ * Reads a stream of variable sized packets until a flush packet is detected.\n+ */\n+ssize_t packet_read_till_flush(int fd_in, struct strbuf *sb_out);\n+\n #define DEFAULT_PACKET_MAX 1000\n #define LARGE_PACKET_MAX 65520\n extern char packet_buffer[LARGE_PACKET_MAX];\n-- \n2.9.2\n\n"},{"id":"300140","messageId":"20160825110752.31581-2-larsxschneider@gmail.com","threadId":"43930","inReplyTo":"20160825110752.31581-1-larsxschneider@gmail.com","subject":"[PATCH v6 01/13] pkt-line: rename packet_write() to packet_write_fmt()","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-25T11:07:40Z","receivedAt":"2016-08-25T11:20:11Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\npacket_write() should be called packet_write_fmt() as the string\nparameter can be formatted.\n\nSuggested-by: Junio C Hamano \nSigned-off-by: Lars Schneider \n---\n builtin/archive.c | 4 ++--\n builtin/receive-pack.c | 4 ++--\n builtin/remote-ext.c | 4 ++--\n builtin/upload-archive.c | 4 ++--\n connect.c | 2 +-\n daemon.c | 2 +-\n http-backend.c | 2 +-\n pkt-line.c | 2 +-\n pkt-line.h | 2 +-\n shallow.c | 2 +-\n upload-pack.c | 30 +++++++++++++++---------------\n 11 files changed, 29 insertions(+), 29 deletions(-)\n\ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex a1e3b94..49f4914 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -47,10 +47,10 @@ static int run_remote_archiver(int argc, const char **argv,\n \tif (name_hint) {\n \t\tconst char *format = archive_format_from_filename(name_hint);\n \t\tif (format)\n-\t\t\tpacket_write(fd[1], \"argument --format=%s\\n\", format);\n+\t\t\tpacket_write_fmt(fd[1], \"argument --format=%s\\n\", format);\n \t}\n \tfor (i = 1; i < argc; i++)\n-\t\tpacket_write(fd[1], \"argument %s\\n\", argv[i]);\n+\t\tpacket_write_fmt(fd[1], \"argument %s\\n\", argv[i]);\n \tpacket_flush(fd[1]);\n \n \tbuf = packet_read_line(fd[0], NULL);\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 15c323a..d60a412 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -199,7 +199,7 @@ static int receive_pack_config(const char *var, const char *value, void *cb)\n static void show_ref(const char *path, const unsigned char *sha1)\n {\n \tif (sent_capabilities) {\n-\t\tpacket_write(1, \"%s %s\\n\", sha1_to_hex(sha1), path);\n+\t\tpacket_write_fmt(1, \"%s %s\\n\", sha1_to_hex(sha1), path);\n \t} else {\n \t\tstruct strbuf cap = STRBUF_INIT;\n \n@@ -212,7 +212,7 @@ static void show_ref(const char *path, const unsigned char *sha1)\n \t\tif (push_cert_nonce)\n \t\t\tstrbuf_addf(&cap, \" push-cert=%s\", push_cert_nonce);\n \t\tstrbuf_addf(&cap, \" agent=%s\", git_user_agent_sanitized());\n-\t\tpacket_write(1, \"%s %s%c%s\\n\",\n+\t\tpacket_write_fmt(1, \"%s %s%c%s\\n\",\n \t\t\t sha1_to_hex(sha1), path, 0, cap.buf);\n \t\tstrbuf_release(&cap);\n \t\tsent_capabilities = 1;\ndiff --git a/builtin/remote-ext.c b/builtin/remote-ext.c\nindex 88eb8f9..11b48bf 100644\n--- a/builtin/remote-ext.c\n+++ b/builtin/remote-ext.c\n@@ -128,9 +128,9 @@ static void send_git_request(int stdin_fd, const char *serv, const char *repo,\n \tconst char *vhost)\n {\n \tif (!vhost)\n-\t\tpacket_write(stdin_fd, \"%s %s%c\", serv, repo, 0);\n+\t\tpacket_write_fmt(stdin_fd, \"%s %s%c\", serv, repo, 0);\n \telse\n-\t\tpacket_write(stdin_fd, \"%s %s%chost=%s%c\", serv, repo, 0,\n+\t\tpacket_write_fmt(stdin_fd, \"%s %s%chost=%s%c\", serv, repo, 0,\n \t\t\t vhost, 0);\n }\n \ndiff --git a/builtin/upload-archive.c b/builtin/upload-archive.c\nindex 2caedf1..dc872f6 100644\n--- a/builtin/upload-archive.c\n+++ b/builtin/upload-archive.c\n@@ -88,11 +88,11 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)\n \twriter.git_cmd = 1;\n \tif (start_command(&writer)) {\n \t\tint err = errno;\n-\t\tpacket_write(1, \"NACK unable to spawn subprocess\\n\");\n+\t\tpacket_write_fmt(1, \"NACK unable to spawn subprocess\\n\");\n \t\tdie(\"upload-archive: %s\", strerror(err));\n \t}\n \n-\tpacket_write(1, \"ACK\\n\");\n+\tpacket_write_fmt(1, \"ACK\\n\");\n \tpacket_flush(1);\n \n \twhile (1) {\ndiff --git a/connect.c b/connect.c\nindex 722dc3f..5330d9c 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -730,7 +730,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t * Note: Do not add any other headers here! Doing so\n \t\t * will cause older git-daemon servers to crash.\n \t\t */\n-\t\tpacket_write(fd[1],\n+\t\tpacket_write_fmt(fd[1],\n \t\t\t \"%s %s%chost=%s%c\",\n \t\t\t prog, path, 0,\n \t\t\t target_host, 0);\ndiff --git a/daemon.c b/daemon.c\nindex e647254..2646d0f 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -281,7 +281,7 @@ static int daemon_error(const char *dir, const char *msg)\n {\n \tif (!informative_errors)\n \t\tmsg = \"access denied or repository not exported\";\n-\tpacket_write(1, \"ERR %s: %s\", msg, dir);\n+\tpacket_write_fmt(1, \"ERR %s: %s\", msg, dir);\n \treturn -1;\n }\n \ndiff --git a/http-backend.c b/http-backend.c\nindex 0d59499..aa61c18 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -460,7 +460,7 @@ static void get_info_refs(char *arg)\n \t\thdr_str(content_type, buf.buf);\n \t\tend_headers();\n \n-\t\tpacket_write(1, \"# service=git-%s\\n\", svc->name);\n+\t\tpacket_write_fmt(1, \"# service=git-%s\\n\", svc->name);\n \t\tpacket_flush(1);\n \n \t\targv[0] = svc->name;\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 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 4d554ca..1826114 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -260,7 +260,7 @@ static int advertise_shallow_grafts_cb(const struct commit_graft *graft, void *c\n {\n \tint fd = *(int *)cb;\n \tif (graft->nr_parent == -1)\n-\t\tpacket_write(fd, \"shallow %s\\n\", oid_to_hex(&graft->oid));\n+\t\tpacket_write_fmt(fd, \"shallow %s\\n\", oid_to_hex(&graft->oid));\n \treturn 0;\n }\n \ndiff --git a/upload-pack.c b/upload-pack.c\nindex d4cc414..8e77b00 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -393,13 +393,13 @@ static int get_common_commits(void)\n \t\t\tif (multi_ack == 2 && got_common\n \t\t\t && !got_other && ok_to_give_up()) {\n \t\t\t\tsent_ready = 1;\n-\t\t\t\tpacket_write(1, \"ACK %s ready\\n\", last_hex);\n+\t\t\t\tpacket_write_fmt(1, \"ACK %s ready\\n\", last_hex);\n \t\t\t}\n \t\t\tif (have_obj.nr == 0 || multi_ack)\n-\t\t\t\tpacket_write(1, \"NAK\\n\");\n+\t\t\t\tpacket_write_fmt(1, \"NAK\\n\");\n \n \t\t\tif (no_done && sent_ready) {\n-\t\t\t\tpacket_write(1, \"ACK %s\\n\", last_hex);\n+\t\t\t\tpacket_write_fmt(1, \"ACK %s\\n\", last_hex);\n \t\t\t\treturn 0;\n \t\t\t}\n \t\t\tif (stateless_rpc)\n@@ -416,20 +416,20 @@ static int get_common_commits(void)\n \t\t\t\t\tconst char *hex = sha1_to_hex(sha1);\n \t\t\t\t\tif (multi_ack == 2) {\n \t\t\t\t\t\tsent_ready = 1;\n-\t\t\t\t\t\tpacket_write(1, \"ACK %s ready\\n\", hex);\n+\t\t\t\t\t\tpacket_write_fmt(1, \"ACK %s ready\\n\", hex);\n \t\t\t\t\t} else\n-\t\t\t\t\t\tpacket_write(1, \"ACK %s continue\\n\", hex);\n+\t\t\t\t\t\tpacket_write_fmt(1, \"ACK %s continue\\n\", hex);\n \t\t\t\t}\n \t\t\t\tbreak;\n \t\t\tdefault:\n \t\t\t\tgot_common = 1;\n \t\t\t\tmemcpy(last_hex, sha1_to_hex(sha1), 41);\n \t\t\t\tif (multi_ack == 2)\n-\t\t\t\t\tpacket_write(1, \"ACK %s common\\n\", last_hex);\n+\t\t\t\t\tpacket_write_fmt(1, \"ACK %s common\\n\", last_hex);\n \t\t\t\telse if (multi_ack)\n-\t\t\t\t\tpacket_write(1, \"ACK %s continue\\n\", last_hex);\n+\t\t\t\t\tpacket_write_fmt(1, \"ACK %s continue\\n\", last_hex);\n \t\t\t\telse if (have_obj.nr == 1)\n-\t\t\t\t\tpacket_write(1, \"ACK %s\\n\", last_hex);\n+\t\t\t\t\tpacket_write_fmt(1, \"ACK %s\\n\", last_hex);\n \t\t\t\tbreak;\n \t\t\t}\n \t\t\tcontinue;\n@@ -437,10 +437,10 @@ static int get_common_commits(void)\n \t\tif (!strcmp(line, \"done\")) {\n \t\t\tif (have_obj.nr > 0) {\n \t\t\t\tif (multi_ack)\n-\t\t\t\t\tpacket_write(1, \"ACK %s\\n\", last_hex);\n+\t\t\t\t\tpacket_write_fmt(1, \"ACK %s\\n\", last_hex);\n \t\t\t\treturn 0;\n \t\t\t}\n-\t\t\tpacket_write(1, \"NAK\\n\");\n+\t\t\tpacket_write_fmt(1, \"NAK\\n\");\n \t\t\treturn -1;\n \t\t}\n \t\tdie(\"git upload-pack: expected SHA1 list, got '%s'\", line);\n@@ -650,7 +650,7 @@ static void receive_needs(void)\n \t\twhile (result) {\n \t\t\tstruct object *object = &result->item->object;\n \t\t\tif (!(object->flags & (CLIENT_SHALLOW|NOT_SHALLOW))) {\n-\t\t\t\tpacket_write(1, \"shallow %s\",\n+\t\t\t\tpacket_write_fmt(1, \"shallow %s\",\n \t\t\t\t\t\toid_to_hex(&object->oid));\n \t\t\t\tregister_shallow(object->oid.hash);\n \t\t\t\tshallow_nr++;\n@@ -662,7 +662,7 @@ static void receive_needs(void)\n \t\t\tstruct object *object = shallows.objects[i].item;\n \t\t\tif (object->flags & NOT_SHALLOW) {\n \t\t\t\tstruct commit_list *parents;\n-\t\t\t\tpacket_write(1, \"unshallow %s\",\n+\t\t\t\tpacket_write_fmt(1, \"unshallow %s\",\n \t\t\t\t\toid_to_hex(&object->oid));\n \t\t\t\tobject->flags &= ~CLIENT_SHALLOW;\n \t\t\t\t/* make sure the real parents are parsed */\n@@ -741,7 +741,7 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \t\tstruct strbuf symref_info = STRBUF_INIT;\n \n \t\tformat_symref_info(&symref_info, cb_data);\n-\t\tpacket_write(1, \"%s %s%c%s%s%s%s%s agent=%s\\n\",\n+\t\tpacket_write_fmt(1, \"%s %s%c%s%s%s%s%s agent=%s\\n\",\n \t\t\t oid_to_hex(oid), refname_nons,\n \t\t\t 0, capabilities,\n \t\t\t (allow_unadvertised_object_request & ALLOW_TIP_SHA1) ?\n@@ -753,11 +753,11 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \t\t\t git_user_agent_sanitized());\n \t\tstrbuf_release(&symref_info);\n \t} else {\n-\t\tpacket_write(1, \"%s %s\\n\", oid_to_hex(oid), refname_nons);\n+\t\tpacket_write_fmt(1, \"%s %s\\n\", oid_to_hex(oid), refname_nons);\n \t}\n \tcapabilities = NULL;\n \tif (!peel_ref(refname, peeled.hash))\n-\t\tpacket_write(1, \"%s %s^{}\\n\", oid_to_hex(&peeled), refname_nons);\n+\t\tpacket_write_fmt(1, \"%s %s^{}\\n\", oid_to_hex(&peeled), refname_nons);\n \treturn 0;\n }\n \n-- \n2.9.2\n\n"},{"id":"300141","messageId":"20160825110752.31581-4-larsxschneider@gmail.com","threadId":"43930","inReplyTo":"20160825110752.31581-1-larsxschneider@gmail.com","subject":"[PATCH v6 03/13] pkt-line: add packet_write_fmt_gently()","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-25T11:07:42Z","receivedAt":"2016-08-25T11:20:16Z","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 | 12 ++++++++++++\n pkt-line.h | 1 +\n 2 files changed, 13 insertions(+)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex e8adc0f..3e8b2fb 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -137,6 +137,18 @@ void packet_write_fmt(int fd, const char *fmt, ...)\n \twrite_or_die(fd, buf.buf, buf.len);\n }\n \n+int packet_write_fmt_gently(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+\tva_end(args);\n+\treturn (write_in_full(fd, buf.buf, buf.len) == buf.len ? 0 : -1);\n+}\n+\n void packet_buf_write(struct strbuf *buf, const char *fmt, ...)\n {\n \tva_list args;\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 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.9.2\n\n"},{"id":"300142","messageId":"20160825110752.31581-12-larsxschneider@gmail.com","threadId":"43930","inReplyTo":"20160825110752.31581-1-larsxschneider@gmail.com","subject":"[PATCH v6 11/13] convert: make apply_filter() adhere to standard Git error handling","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-25T11:07:50Z","receivedAt":"2016-08-25T11:20:29Z","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 522e2c5..bd17340 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -436,7 +436,7 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,\n \t *\n \t * (child --> cmd) --> us\n \t */\n-\tint ret = 1;\n+\tint err = 0;\n \tstruct strbuf nbuf = STRBUF_INIT;\n \tstruct async async;\n \tstruct filter_params params;\n@@ -463,22 +463,22 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,\n \n \tif (strbuf_read(&nbuf, async.out, len) < 0) {\n \t\terror(\"read from external filter '%s' failed\", cmd);\n-\t\tret = 0;\n+\t\terr = -1;\n \t}\n \tif (close(async.out)) {\n \t\terror(\"read from external filter '%s' failed\", cmd);\n-\t\tret = 0;\n+\t\terr = -1;\n \t}\n \tif (finish_async(&async)) {\n \t\terror(\"external filter '%s' failed\", cmd);\n-\t\tret = 0;\n+\t\terr = -1;\n \t}\n \n-\tif (ret) {\n+\tif (!err) {\n \t\tstrbuf_swap(dst, &nbuf);\n \t}\n \tstrbuf_release(&nbuf);\n-\treturn ret;\n+\treturn !err;\n }\n \n static struct convert_driver {\n-- \n2.9.2\n\n"},{"id":"300143","messageId":"20160825110752.31581-13-larsxschneider@gmail.com","threadId":"43930","inReplyTo":"20160825110752.31581-1-larsxschneider@gmail.com","subject":"[PATCH v6 12/13] convert: add filter..process option","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-25T11:07:51Z","receivedAt":"2016-08-25T11:21:40Z","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 | 146 +++++++++++++++-\n convert.c | 349 +++++++++++++++++++++++++++++++++----\n pkt-line.h | 1 +\n t/t0021-conversion.sh | 372 ++++++++++++++++++++++++++++++++++++++++\n t/t0021/rot13-filter.pl | 176 +++++++++++++++++++\n unpack-trees.c | 1 +\n 6 files changed, 1015 insertions(+), 30 deletions(-)\n create mode 100755 t/t0021/rot13-filter.pl\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 8882a3e..6346700 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -300,7 +300,13 @@ checkout, when the `smudge` command is specified, the command is\n fed the blob object from its standard input, and its standard\n output is used to update the worktree file. Similarly, the\n `clean` command is used to convert the contents of worktree file\n-upon checkin.\n+upon checkin. By default these commands process only a single\n+blob and terminate. If a long running `process` filter is used\n+in place of `clean` and/or `smudge` filters, then Git can process\n+all blobs with a single filter command invocation for the entire\n+life of a single Git command, for example `git add --all`. See\n+section below for the description of the protocol used to\n+communicate with a `process` filter.\n \n One use of the content filtering is to massage the content into a shape\n that is more convenient for the platform, filesystem, and the user to use.\n@@ -375,6 +381,144 @@ substitution. For example:\n ------------------------\n \n \n+Long Running Filter Process\n+^^^^^^^^^^^^^^^^^^^^^^^^^^^\n+\n+If the filter command (a string value) is defined via\n+`filter..process` then Git can process all blobs with a\n+single filter invocation for the entire life of a single Git\n+command. This is achieved by using the following packet format\n+(pkt-line, see technical/protocol-common.txt) based protocol over\n+standard input and standard output.\n+\n+Git starts the filter when it encounters the first file\n+that needs to be cleaned or smudged. After the filter started\n+Git sends a welcome message (\"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+In case the filter cannot or does not want to process the content\n+as well as any future content for the lifetime of the Git process,\n+it is expected to respond with an \"error-all\" status. Depending on\n+the `filter..required` flag Git will interpret that as error\n+but it will not stop or restart the filter process.\n+------------------------\n+packet: git< status=error-all\\n\n+packet: git< 0000\n+------------------------\n+\n+If the filter experiences an error during processing, then it can\n+send the status \"error\". Depending on the `filter..required`\n+flag Git will interpret that as error but it will not stop or restart\n+the filter process.\n+------------------------\n+packet: git< status=success\\n\n+packet: git< 0000\n+packet: git< HALF_WRITTEN_ERRONEOUS_CONTENT\n+packet: git< 0000\n+packet: git< status=error\\n\n+packet: git< 0000\n+------------------------\n+\n+If the filter dies during the communication or does not adhere to\n+the protocol then Git will stop the filter process and restart it\n+with the next file that needs to be processed.\n+\n+After the filter has processed a blob it is expected to wait for\n+the next \"key=value\" list containing a command. When the Git process\n+terminates, it will send a kill signal to the filter in that stage.\n+\n+A long running filter demo implementation can be found in\n+`t/t0021/rot13-filter.pl` located in the Git core repository.\n+If you develop your own long running filter process then the\n+`GIT_TRACE_PACKET` environment variables can be very helpful\n+for debugging (see linkgit:git[1]).\n+\n+If a `filter..clean` or `filter..smudge` command\n+is configured then these commands always take precedence over\n+a configured `filter..process` command.\n+\n+Please note that you cannot use an existing `filter..clean`\n+or `filter..smudge` command with `filter..process`\n+because the former two use a different inter process communication\n+protocol than the latter one.\n+\n+\n Interaction between checkin/checkout attributes\n ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n \ndiff --git a/convert.c b/convert.c\nindex bd17340..362a0af 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -3,6 +3,7 @@\n #include \"run-command.h\"\n #include \"quote.h\"\n #include \"sigchain.h\"\n+#include \"pkt-line.h\"\n \n /*\n * convert.c - convert a file when checking it out and checking it in.\n@@ -427,7 +428,7 @@ static int filter_buffer_or_fd(int in, int out, void *data)\n \treturn (write_err || status);\n }\n \n-static int apply_filter(const char *path, const char *src, size_t len, int fd,\n+static int apply_single_file_filter(const char *path, const char *src, size_t len, int fd,\n struct strbuf *dst, const char *cmd)\n {\n \t/*\n@@ -441,12 +442,6 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,\n \tstruct async async;\n \tstruct filter_params params;\n \n-\tif (!cmd || !*cmd)\n-\t\treturn 0;\n-\n-\tif (!dst)\n-\t\treturn 1;\n-\n \tmemset(&async, 0, sizeof(async));\n \tasync.proc = filter_buffer_or_fd;\n \tasync.data = ¶ms;\n@@ -481,14 +476,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 = packet_write_stream_with_flush_from_fd(fd, process->in);\n+\telse\n+\t\terr = packet_write_stream_with_flush_from_buf(src, len, process->in);\n+\tif (err)\n+\t\tgoto done;\n+\n+\tread_multi_file_filter_values(process->out, &filter_status);\n+\terr = strcmp(filter_status.buf, \"success\");\n+\tif (err)\n+\t\tgoto done;\n+\n+\terr = packet_read_till_flush(process->out, &nbuf) < 0;\n+\tif (err)\n+\t\tgoto done;\n+\n+\tread_multi_file_filter_values(process->out, &filter_status);\n+\terr = strcmp(filter_status.buf, \"success\");\n+\n+done:\n+\tsigchain_pop(SIGPIPE);\n+\n+\tif (err || errno == EPIPE) {\n+\t\tif (!strcmp(filter_status.buf, \"error\")) {\n+\t\t\t/* The filter signaled a problem with the file. */\n+\t\t} else if (!strcmp(filter_status.buf, \"error-all\")) {\n+\t\t\t/*\n+\t\t\t * The filter signaled a permanent problem. Don't try to filter\n+\t\t\t * files with the same command for the lifetime of the current\n+\t\t\t * Git process.\n+\t\t\t */\n+\t\t\t entry->supported_capabilities &= ~wanted_capability;\n+\t\t} else {\n+\t\t\t/*\n+\t\t\t * Something went wrong with the protocol filter.\n+\t\t\t * Force shutdown and restart if another blob requires filtering!\n+\t\t\t */\n+\t\t\terror(\"external filter '%s' failed\", cmd);\n+\t\t\tkill_multi_file_filter(&cmd_process_map, entry);\n+\t\t}\n+\t} else {\n+\t\tstrbuf_swap(dst, &nbuf);\n+\t}\n+\tstrbuf_release(&nbuf);\n+\treturn !err;\n+}\n+\n static struct convert_driver {\n \tconst char *name;\n \tstruct convert_driver *next;\n \tconst char *smudge;\n \tconst char *clean;\n+\tconst char *process;\n \tint required;\n } *user_convert, **user_convert_tail;\n \n+static int apply_filter(const char *path, const char *src, size_t len,\n+ int fd, struct strbuf *dst, struct convert_driver *drv,\n+ const int wanted_capability)\n+{\n+\tconst char* cmd = NULL;\n+\n+\tif (!drv)\n+\t\treturn 0;\n+\n+\tif (!dst)\n+\t\treturn 1;\n+\n+\tif ((CAP_CLEAN & wanted_capability) && drv->clean)\n+\t\tcmd = drv->clean;\n+\telse if ((CAP_SMUDGE & wanted_capability) && drv->smudge)\n+\t\tcmd = drv->smudge;\n+\n+\tif (cmd && *cmd)\n+\t\treturn apply_single_file_filter(path, src, len, fd, dst, cmd);\n+\telse if (drv->process && *drv->process)\n+\t\treturn apply_multi_file_filter(path, src, len, fd, dst, drv->process, wanted_capability);\n+\n+\treturn 0;\n+}\n+\n static int read_convert_config(const char *var, const char *value, void *cb)\n {\n \tconst char *key, *name;\n@@ -526,6 +825,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@@ -823,7 +1125,7 @@ int would_convert_to_git_filter_fd(const char *path)\n \tif (!ca.drv->required)\n \t\treturn 0;\n \n-\treturn apply_filter(path, NULL, 0, -1, NULL, ca.drv->clean);\n+\treturn apply_filter(path, NULL, 0, -1, NULL, ca.drv, CAP_CLEAN);\n }\n \n const char *get_convert_attr_ascii(const char *path)\n@@ -856,18 +1158,12 @@ int convert_to_git(const char *path, const char *src, size_t len,\n struct strbuf *dst, enum safe_crlf checksafe)\n {\n \tint ret = 0;\n-\tconst char *filter = NULL;\n-\tint required = 0;\n \tstruct conv_attrs ca;\n \n \tconvert_attrs(&ca, path);\n-\tif (ca.drv) {\n-\t\tfilter = ca.drv->clean;\n-\t\trequired = ca.drv->required;\n-\t}\n \n-\tret |= apply_filter(path, src, len, -1, dst, filter);\n-\tif (!ret && required)\n+\tret |= apply_filter(path, src, len, -1, dst, ca.drv, CAP_CLEAN);\n+\tif (!ret && ca.drv && ca.drv->required)\n \t\tdie(\"%s: clean filter '%s' failed\", path, ca.drv->name);\n \n \tif (ret && dst) {\n@@ -889,9 +1185,9 @@ void convert_to_git_filter_fd(const char *path, int fd, struct strbuf *dst,\n \tconvert_attrs(&ca, path);\n \n \tassert(ca.drv);\n-\tassert(ca.drv->clean);\n+\tassert(ca.drv->clean || ca.drv->process);\n \n-\tif (!apply_filter(path, NULL, 0, fd, dst, ca.drv->clean))\n+\tif (!apply_filter(path, NULL, 0, fd, dst, ca.drv, CAP_CLEAN))\n \t\tdie(\"%s: clean filter '%s' failed\", path, ca.drv->name);\n \n \tcrlf_to_git(path, dst->buf, dst->len, dst, ca.crlf_action, checksafe);\n@@ -903,15 +1199,9 @@ static int convert_to_working_tree_internal(const char *path, const char *src,\n \t\t\t\t\t int normalizing)\n {\n \tint ret = 0, ret_filter = 0;\n-\tconst char *filter = NULL;\n-\tint required = 0;\n \tstruct conv_attrs ca;\n \n \tconvert_attrs(&ca, path);\n-\tif (ca.drv) {\n-\t\tfilter = ca.drv->smudge;\n-\t\trequired = ca.drv->required;\n-\t}\n \n \tret |= ident_to_worktree(path, src, len, dst, ca.ident);\n \tif (ret) {\n@@ -920,9 +1210,10 @@ static int convert_to_working_tree_internal(const char *path, const char *src,\n \t}\n \t/*\n \t * CRLF conversion can be skipped if normalizing, unless there\n-\t * is a smudge filter. The filter might expect CRLFs.\n+\t * is a smudge or process filter (even if the process filter doesn't\n+\t * support smudge). The filters might expect CRLFs.\n \t */\n-\tif (filter || !normalizing) {\n+\tif ((ca.drv && (ca.drv->smudge || ca.drv->process)) || !normalizing) {\n \t\tret |= crlf_to_worktree(path, src, len, dst, ca.crlf_action);\n \t\tif (ret) {\n \t\t\tsrc = dst->buf;\n@@ -930,8 +1221,8 @@ static int convert_to_working_tree_internal(const char *path, const char *src,\n \t\t}\n \t}\n \n-\tret_filter = apply_filter(path, src, len, -1, dst, filter);\n-\tif (!ret_filter && required)\n+\tret_filter = apply_filter(path, src, len, -1, dst, ca.drv, CAP_SMUDGE);\n+\tif (!ret_filter && ca.drv && ca.drv->required)\n \t\tdie(\"%s: smudge filter %s failed\", path, ca.drv->name);\n \n \treturn ret | ret_filter;\n@@ -1383,7 +1674,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 9616117..6a3b7cf 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -86,6 +86,7 @@ ssize_t packet_read_till_flush(int fd_in, struct strbuf *sb_out);\n \n #define DEFAULT_PACKET_MAX 1000\n #define LARGE_PACKET_MAX 65520\n+#define PKTLINE_DATA_MAXLEN (LARGE_PACKET_MAX - 4)\n extern char packet_buffer[LARGE_PACKET_MAX];\n \n #endif\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex 34c8eb9..5ba694b 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -42,6 +42,9 @@ test_expect_success setup '\n \trm -f test test.t test.i &&\n \tgit checkout -- test test.t test.i &&\n \n+\techo \"content-test2\" >test2.o &&\n+\techo \"content-test3-subdir\" >test3-subdir.o &&\n+\n \tmkdir generated-test-data &&\n \tfor i in $(test_seq 1 $T0021_LARGE_FILE_SIZE)\n \tdo\n@@ -296,4 +299,373 @@ test_expect_success 'disable filter with empty override' '\n \ttest_must_be_empty err\n '\n \n+check_filter () {\n+\trm -f rot13-filter.log actual.log &&\n+\t\"$@\" 2> git_stderr.log &&\n+\ttest_must_be_empty git_stderr.log &&\n+\tcat >expected.log &&\n+\tsort rot13-filter.log | uniq -c | sed \"s/^[ ]*//\" >actual.log &&\n+\ttest_cmp expected.log actual.log\n+}\n+\n+check_filter_count_clean () {\n+\trm -f rot13-filter.log actual.log &&\n+\t\"$@\" 2> git_stderr.log &&\n+\ttest_must_be_empty git_stderr.log &&\n+\tcat >expected.log &&\n+\tsort rot13-filter.log | uniq -c | sed \"s/^[ ]*//\" |\n+\t\tsed \"s/^\\([0-9]\\) IN: clean/x IN: clean/\" >actual.log &&\n+\ttest_cmp expected.log actual.log\n+}\n+\n+check_filter_ignore_clean () {\n+\trm -f rot13-filter.log actual.log &&\n+\t\"$@\" &&\n+\tcat >expected.log &&\n+\tgrep -v \"IN: clean\" rot13-filter.log >actual.log &&\n+\ttest_cmp expected.log actual.log\n+}\n+\n+check_filter_no_call () {\n+\trm -f rot13-filter.log &&\n+\t\"$@\" 2> git_stderr.log &&\n+\ttest_must_be_empty git_stderr.log &&\n+\ttest_must_be_empty rot13-filter.log\n+}\n+\n+check_rot13 () {\n+\ttest_cmp $1 $2 &&\n+\t./../rot13.sh <$1 >expected &&\n+\tgit cat-file blob :$2 >actual &&\n+\ttest_cmp expected actual\n+}\n+\n+test_expect_success PERL 'required process filter should filter data' '\n+\ttest_config_global filter.protocol.process \"$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge\" &&\n+\ttest_config_global filter.protocol.required true &&\n+\trm -rf repo &&\n+\tmkdir repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit init &&\n+\n+\t\techo \"*.r filter=protocol\" >.gitattributes &&\n+\t\tgit add . &&\n+\t\tgit commit . -m \"test commit\" &&\n+\t\tgit branch empty &&\n+\n+\t\tcp ../test.o test.r &&\n+\t\tcp ../test2.o test2.r &&\n+\t\tmkdir testsubdir &&\n+\t\tcp ../test3-subdir.o testsubdir/test3-subdir.r &&\n+\t\t>test4-empty.r &&\n+\n+\t\tcheck_filter \\\n+\t\t\tgit add . \\\n+\t\t\t\t<<-\\EOF &&\n+\t\t\t\t\t1 IN: clean test.r 57 [OK] -- OUT: 57 [OK]\n+\t\t\t\t\t1 IN: clean test2.r 14 [OK] -- OUT: 14 [OK]\n+\t\t\t\t\t1 IN: clean test4-empty.r 0 [OK] -- OUT: 0 [OK]\n+\t\t\t\t\t1 IN: clean testsubdir/test3-subdir.r 21 [OK] -- OUT: 21 [OK]\n+\t\t\t\t\t1 start\n+\t\t\t\t\t1 wrote filter header\n+\t\t\t\tEOF\n+\n+\t\tcheck_filter_count_clean \\\n+\t\t\tgit commit . -m \"test commit\" \\\n+\t\t\t\t<<-\\EOF &&\n+\t\t\t\t\tx IN: clean test.r 57 [OK] -- OUT: 57 [OK]\n+\t\t\t\t\tx IN: clean test2.r 14 [OK] -- OUT: 14 [OK]\n+\t\t\t\t\tx IN: clean test4-empty.r 0 [OK] -- OUT: 0 [OK]\n+\t\t\t\t\tx IN: clean testsubdir/test3-subdir.r 21 [OK] -- OUT: 21 [OK]\n+\t\t\t\t\t1 start\n+\t\t\t\t\t1 wrote filter header\n+\t\t\t\tEOF\n+\n+\t\trm -f test?.r testsubdir/test3-subdir.r &&\n+\n+\t\tcheck_filter_ignore_clean \\\n+\t\t\tgit checkout . \\\n+\t\t\t\t<<-\\EOF &&\n+\t\t\t\t\tstart\n+\t\t\t\t\twrote filter header\n+\t\t\t\t\tIN: smudge test2.r 14 [OK] -- OUT: 14 [OK]\n+\t\t\t\t\tIN: smudge testsubdir/test3-subdir.r 21 [OK] -- OUT: 21 [OK]\n+\t\t\t\tEOF\n+\n+\t\tcheck_filter_ignore_clean \\\n+\t\t\tgit checkout empty \\\n+\t\t\t\t<<-\\EOF &&\n+\t\t\t\t\tstart\n+\t\t\t\t\twrote filter header\n+\t\t\t\tEOF\n+\n+\t\tcheck_filter_ignore_clean \\\n+\t\t\tgit checkout master \\\n+\t\t\t\t<<-\\EOF &&\n+\t\t\t\t\tstart\n+\t\t\t\t\twrote filter header\n+\t\t\t\t\tIN: smudge test.r 57 [OK] -- OUT: 57 [OK]\n+\t\t\t\t\tIN: smudge test2.r 14 [OK] -- OUT: 14 [OK]\n+\t\t\t\t\tIN: smudge test4-empty.r 0 [OK] -- OUT: 0 [OK]\n+\t\t\t\t\tIN: smudge testsubdir/test3-subdir.r 21 [OK] -- OUT: 21 [OK]\n+\t\t\t\tEOF\n+\n+\t\tcheck_rot13 ../test.o test.r &&\n+\t\tcheck_rot13 ../test2.o test2.r &&\n+\t\tcheck_rot13 ../test3-subdir.o testsubdir/test3-subdir.r\n+\t)\n+'\n+\n+test_expect_success PERL 'required process filter should filter smudge data and one-shot filter should clean' '\n+\ttest_config_global filter.protocol.clean ./../rot13.sh &&\n+\ttest_config_global filter.protocol.process \"$TEST_DIRECTORY/t0021/rot13-filter.pl smudge\" &&\n+\ttest_config_global filter.protocol.required true &&\n+\trm -rf repo &&\n+\tmkdir repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit init &&\n+\n+\t\techo \"*.r filter=protocol\" >.gitattributes &&\n+\t\tgit add . &&\n+\t\tgit commit . -m \"test commit\" &&\n+\t\tgit branch empty &&\n+\n+\t\tcp ../test.o test.r &&\n+\t\tcp ../test2.o test2.r &&\n+\n+\t\tcheck_filter_no_call \\\n+\t\t\tgit add . &&\n+\n+\t\tcheck_filter_no_call \\\n+\t\t\tgit commit . -m \"test commit\" &&\n+\n+\t\trm -f test?.r testsubdir/test3-subdir.r &&\n+\n+\t\tcheck_filter_ignore_clean \\\n+\t\t\tgit checkout . \\\n+\t\t\t\t<<-\\EOF &&\n+\t\t\t\t\tstart\n+\t\t\t\t\twrote filter header\n+\t\t\t\t\tIN: smudge test2.r 14 [OK] -- OUT: 14 [OK]\n+\t\t\t\tEOF\n+\n+\t\tgit checkout empty &&\n+\n+\t\tcheck_filter_ignore_clean \\\n+\t\t\tgit checkout master\\\n+\t\t\t\t<<-\\EOF &&\n+\t\t\t\t\tstart\n+\t\t\t\t\twrote filter header\n+\t\t\t\t\tIN: smudge test.r 57 [OK] -- OUT: 57 [OK]\n+\t\t\t\t\tIN: smudge test2.r 14 [OK] -- OUT: 14 [OK]\n+\t\t\t\tEOF\n+\n+\t\tcheck_rot13 ../test.o test.r &&\n+\t\tcheck_rot13 ../test2.o test2.r\n+\t)\n+'\n+\n+test_expect_success PERL 'required process filter should clean only' '\n+\ttest_config_global filter.protocol.process \"$TEST_DIRECTORY/t0021/rot13-filter.pl clean\" &&\n+\ttest_config_global filter.protocol.required true &&\n+\trm -rf repo &&\n+\tmkdir repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit init &&\n+\n+\t\techo \"*.r filter=protocol\" >.gitattributes &&\n+\t\tgit add . &&\n+\t\tgit commit . -m \"test commit\" &&\n+\t\tgit branch empty &&\n+\n+\t\tcp ../test.o test.r &&\n+\n+\t\tcheck_filter \\\n+\t\t\tgit add . \\\n+\t\t\t\t<<-\\EOF &&\n+\t\t\t\t\t1 IN: clean test.r 57 [OK] -- OUT: 57 [OK]\n+\t\t\t\t\t1 start\n+\t\t\t\t\t1 wrote filter header\n+\t\t\t\tEOF\n+\n+\t\tcheck_filter_count_clean \\\n+\t\t\tgit commit . -m \"test commit\" \\\n+\t\t\t\t<<-\\EOF\n+\t\t\t\t\tx IN: clean test.r 57 [OK] -- OUT: 57 [OK]\n+\t\t\t\t\t1 start\n+\t\t\t\t\t1 wrote filter header\n+\t\t\t\tEOF\n+\t)\n+'\n+\n+test_expect_success PERL 'required process filter should process binary files larger LARGE_PACKET_MAX' '\n+\ttest_config_global filter.protocol.process \"$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge\" &&\n+\ttest_config_global filter.protocol.required true &&\n+\trm -rf repo &&\n+\tmkdir repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit init &&\n+\n+\t\techo \"*.file filter=protocol\" >.gitattributes &&\n+\t\tcat ../generated-test-data/largish.file.rot13 >large.rot13 &&\n+\t\tcat ../generated-test-data/largish.file >large.file &&\n+\t\tcat large.file >large.original &&\n+\n+\t\tgit add large.file .gitattributes &&\n+\t\tgit commit . -m \"test commit\" &&\n+\n+\t\trm -f large.file &&\n+\t\tgit checkout -- large.file &&\n+\t\tgit cat-file blob :large.file >actual &&\n+\t\ttest_cmp large.rot13 actual\n+\t)\n+'\n+\n+test_expect_success PERL 'required process filter should with clean error should fail' '\n+\ttest_config_global filter.protocol.process \"$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge\" &&\n+\ttest_config_global filter.protocol.required true &&\n+\trm -rf repo &&\n+\tmkdir repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit init &&\n+\n+\t\techo \"*.r filter=protocol\" >.gitattributes &&\n+\n+\t\tcp ../test.o test.r &&\n+\t\techo \"this is going to fail\" >clean-write-fail.r &&\n+\t\techo \"content-test3-subdir\" >test3.r &&\n+\n+\t\t# Note: There are three clean paths in convert.c we just test one here.\n+\t\ttest_must_fail git add .\n+\t)\n+'\n+\n+test_expect_success PERL 'process filter should restart after unexpected write failure' '\n+\ttest_config_global filter.protocol.process \"$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge\" &&\n+\trm -rf repo &&\n+\tmkdir repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit init &&\n+\n+\t\techo \"*.r filter=protocol\" >.gitattributes &&\n+\n+\t\tcp ../test.o test.r &&\n+\t\tcp ../test2.o test2.r &&\n+\t\techo \"this is going to fail\" >smudge-write-fail.o &&\n+\t\tcat smudge-write-fail.o >smudge-write-fail.r &&\n+\t\tgit add . &&\n+\t\tgit commit . -m \"test commit\" &&\n+\t\trm -f *.r &&\n+\n+\t\tcheck_filter_ignore_clean \\\n+\t\t\tgit checkout . \\\n+\t\t\t\t<<-\\EOF &&\n+\t\t\t\t\tstart\n+\t\t\t\t\twrote filter header\n+\t\t\t\t\tIN: smudge smudge-write-fail.r 22 [OK] -- OUT: 22 [WRITE FAIL]\n+\t\t\t\t\tstart\n+\t\t\t\t\twrote filter header\n+\t\t\t\t\tIN: smudge test.r 57 [OK] -- OUT: 57 [OK]\n+\t\t\t\t\tIN: smudge test2.r 14 [OK] -- OUT: 14 [OK]\n+\t\t\t\tEOF\n+\n+\t\tcheck_rot13 ../test.o test.r &&\n+\t\tcheck_rot13 ../test2.o test2.r &&\n+\n+\t\t! test_cmp smudge-write-fail.o smudge-write-fail.r && # Smudge failed!\n+\t\t./../rot13.sh expected &&\n+\t\tgit cat-file blob :smudge-write-fail.r >actual &&\n+\t\ttest_cmp expected actual\t\t\t\t\t\t\t # Clean worked!\n+\t)\n+'\n+\n+test_expect_success PERL 'process filter should not restart in case of an error' '\n+\ttest_config_global filter.protocol.process \"$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge\" &&\n+\trm -rf repo &&\n+\tmkdir repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit init &&\n+\n+\t\techo \"*.r filter=protocol\" >.gitattributes &&\n+\n+\t\tcp ../test.o test.r &&\n+\t\tcp ../test2.o test2.r &&\n+\t\techo \"this will cause an error\" >error.o &&\n+\t\tcp error.o error.r &&\n+\t\tgit add . &&\n+\t\tgit commit . -m \"test commit\" &&\n+\t\trm -f *.r &&\n+\n+\t\tcheck_filter_ignore_clean \\\n+\t\t\tgit checkout . \\\n+\t\t\t\t<<-\\EOF &&\n+\t\t\t\t\tstart\n+\t\t\t\t\twrote filter header\n+\t\t\t\t\tIN: smudge error.r 25 [OK] -- OUT: 0 [ERROR]\n+\t\t\t\t\tIN: smudge test.r 57 [OK] -- OUT: 57 [OK]\n+\t\t\t\t\tIN: smudge test2.r 14 [OK] -- OUT: 14 [OK]\n+\t\t\t\tEOF\n+\n+\t\tcheck_rot13 ../test.o test.r &&\n+\t\tcheck_rot13 ../test2.o test2.r &&\n+\t\ttest_cmp error.o error.r\n+\t)\n+'\n+\n+test_expect_success PERL 'process filter should be able to signal an error for all future files' '\n+\ttest_config_global filter.protocol.process \"$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge\" &&\n+\trm -rf repo &&\n+\tmkdir repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit init &&\n+\n+\t\techo \"*.r filter=protocol\" >.gitattributes &&\n+\n+\t\tcp ../test.o test.r &&\n+\t\tcp ../test2.o test2.r &&\n+\t\techo \"error this blob and all future blobs\" >error-all.o &&\n+\t\tcp error-all.o error-all.r &&\n+\t\tgit add . &&\n+\t\tgit commit . -m \"test commit\" &&\n+\t\trm -f *.r &&\n+\n+\t\tcheck_filter_ignore_clean \\\n+\t\t\tgit checkout . \\\n+\t\t\t\t<<-\\EOF &&\n+\t\t\t\t\tstart\n+\t\t\t\t\twrote filter header\n+\t\t\t\t\tIN: smudge error-all.r 37 [OK] -- OUT: 0 [ERROR-ALL]\n+\t\t\t\tEOF\n+\n+\t\ttest_cmp ../test.o test.r &&\n+\t\ttest_cmp ../test2.o test2.r &&\n+\t\ttest_cmp error-all.o error-all.r\n+\t)\n+'\n+\n+test_expect_success PERL 'invalid process filter must fail (and not hang!)' '\n+\ttest_config_global filter.protocol.process cat &&\n+\ttest_config_global filter.protocol.required true &&\n+\trm -rf repo &&\n+\tmkdir repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit init &&\n+\n+\t\techo \"*.r filter=protocol\" >.gitattributes &&\n+\n+\t\tcp ../test.o test.r &&\n+\t\ttest_must_fail git add . 2> git_stderr.log &&\n+\t\tgrep \"not support long running filter protocol\" git_stderr.log\n+\t)\n+'\n+\n test_done\ndiff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl\nnew file mode 100755\nindex 0000000..0e1181d\n--- /dev/null\n+++ b/t/t0021/rot13-filter.pl\n@@ -0,0 +1,176 @@\n+#!/usr/bin/perl\n+#\n+# Example implementation for the Git filter protocol version 2\n+# See Documentation/gitattributes.txt, section \"Filter Protocol\"\n+#\n+# The script takes the list of supported protocol capabilities as\n+# arguments (\"clean\", \"smudge\", etc).\n+#\n+# This implementation supports special test cases:\n+# (1) If data with the pathname \"clean-write-fail.r\" is processed with\n+# a \"clean\" operation then the write operation will die.\n+# (2) If data with the pathname \"smudge-write-fail.r\" is processed with\n+# a \"smudge\" operation then the write operation will die.\n+# (3) If data with the pathname \"error.r\" is processed with any\n+# operation then the filter signals that it cannot or does not want\n+# to process the file.\n+# (4) If data with the pathname \"error-all.r\" is processed with any\n+# operation then the filter signals that it cannot or does not want\n+# to process the file and any file after that is processed with the\n+# same command.\n+#\n+\n+use strict;\n+use warnings;\n+\n+my $MAX_PACKET_CONTENT_SIZE = 65516;\n+my @capabilities = @ARGV;\n+\n+sub rot13 {\n+ my ($str) = @_;\n+ $str =~ y/A-Za-z/N-ZA-Mn-za-m/;\n+ return $str;\n+}\n+\n+sub packet_read {\n+ my $buffer;\n+ my $bytes_read = read STDIN, $buffer, 4;\n+ if ( $bytes_read == 0 ) {\n+\n+ # EOF - Git stopped talking to us!\n+ exit();\n+ }\n+ elsif ( $bytes_read != 4 ) {\n+ die \"invalid packet size '$bytes_read' field\";\n+ }\n+ my $pkt_size = hex($buffer);\n+ if ( $pkt_size == 0 ) {\n+ return ( 1, \"\" );\n+ }\n+ elsif ( $pkt_size > 4 ) {\n+ my $content_size = $pkt_size - 4;\n+ $bytes_read = read STDIN, $buffer, $content_size;\n+ if ( $bytes_read != $content_size ) {\n+ die \"invalid packet ($content_size expected; $bytes_read read)\";\n+ }\n+ return ( 0, $buffer );\n+ }\n+ else {\n+ die \"invalid packet size\";\n+ }\n+}\n+\n+sub packet_write {\n+ my ($packet) = @_;\n+ print STDOUT sprintf( \"%04x\", length($packet) + 4 );\n+ print STDOUT $packet;\n+ STDOUT->flush();\n+}\n+\n+sub packet_flush {\n+ print STDOUT sprintf( \"%04x\", 0 );\n+ STDOUT->flush();\n+}\n+\n+open my $debug, \">>\", \"rot13-filter.log\";\n+print $debug \"start\\n\";\n+$debug->flush();\n+\n+( packet_read() eq ( 0, \"git-filter-client\" ) ) || die \"bad initialization\";\n+( packet_read() eq ( 0, \"version=2\" ) ) || die \"bad version\";\n+( packet_read() eq ( 1, \"\" ) ) || die \"bad version end\";\n+\n+packet_write(\"git-filter-server\\n\");\n+packet_write(\"version=2\\n\");\n+\n+( packet_read() eq ( 0, \"clean=true\" ) ) || die \"bad capability\";\n+( packet_read() eq ( 0, \"smudge=true\" ) ) || die \"bad capability\";\n+( packet_read() eq ( 1, \"\" ) ) || die \"bad capability end\";\n+\n+foreach (@capabilities) {\n+ packet_write( $_ . \"=true\\n\" );\n+}\n+packet_flush();\n+print $debug \"wrote filter header\\n\";\n+$debug->flush();\n+\n+while (1) {\n+ my ($command) = packet_read() =~ /^command=([^=]+)\\n$/;\n+ print $debug \"IN: $command\";\n+ $debug->flush();\n+\n+ my ($pathname) = packet_read() =~ /^pathname=([^=]+)\\n$/;\n+ print $debug \" $pathname\";\n+ $debug->flush();\n+\n+ # Flush\n+ packet_read();\n+\n+ my $input = \"\";\n+ {\n+ binmode(STDIN);\n+ my $buffer;\n+ my $done = 0;\n+ while ( !$done ) {\n+ ( $done, $buffer ) = packet_read();\n+ $input .= $buffer;\n+ }\n+ print $debug \" \" . length($input) . \" [OK] -- \";\n+ $debug->flush();\n+ }\n+\n+ my $output;\n+ if ( $pathname eq \"error.r\" or $pathname eq \"error-all.r\" ) {\n+ $output = \"\";\n+ }\n+ elsif ( $command eq \"clean\" and grep( /^clean$/, @capabilities ) ) {\n+ $output = rot13($input);\n+ }\n+ elsif ( $command eq \"smudge\" and grep( /^smudge$/, @capabilities ) ) {\n+ $output = rot13($input);\n+ }\n+ else {\n+ die \"bad command '$command'\";\n+ }\n+\n+ print $debug \"OUT: \" . length($output) . \" \";\n+ $debug->flush();\n+\n+ if ( $pathname eq \"error.r\" ) {\n+ print $debug \"[ERROR]\\n\";\n+ $debug->flush();\n+ packet_write(\"status=error\\n\");\n+ packet_flush();\n+ }\n+ elsif ( $pathname eq \"error-all.r\" ) {\n+ print $debug \"[ERROR-ALL]\\n\";\n+ $debug->flush();\n+ packet_write(\"status=error-all\\n\");\n+ packet_flush();\n+ }\n+ else {\n+ packet_write(\"status=success\\n\");\n+ packet_flush();\n+\n+ if ( $pathname eq \"${command}-write-fail.r\" ) {\n+ print $debug \"[WRITE FAIL]\\n\";\n+ $debug->flush();\n+ die \"${command} write error\";\n+ }\n+\n+ while ( length($output) > 0 ) {\n+ my $packet = substr( $output, 0, $MAX_PACKET_CONTENT_SIZE );\n+ packet_write($packet);\n+ if ( length($output) > $MAX_PACKET_CONTENT_SIZE ) {\n+ $output = substr( $output, $MAX_PACKET_CONTENT_SIZE );\n+ }\n+ else {\n+ $output = \"\";\n+ }\n+ }\n+ packet_flush();\n+ print $debug \"[OK]\\n\";\n+ $debug->flush();\n+ packet_flush();\n+ }\n+}\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 11c37fb..f6798f8 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -10,6 +10,7 @@\n #include \"attr.h\"\n #include \"split-index.h\"\n #include \"dir.h\"\n+#include \"convert.h\"\n \n /*\n * Error messages expected by scripts out of plumbing commands such as\n-- \n2.9.2\n\n"},{"id":"300144","messageId":"20160825110752.31581-6-larsxschneider@gmail.com","threadId":"43930","inReplyTo":"20160825110752.31581-1-larsxschneider@gmail.com","subject":"[PATCH v6 05/13] pkt-line: add packet_write_gently()","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-25T11:07:44Z","receivedAt":"2016-08-25T11:27:10Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\npacket_write_fmt() has two shortcomings. First, it uses format_packet()\nwhich lets the caller only send string data via \"%s\". That means it\ncannot be used for arbitrary data that may contain NULs. Second, it will\nalways die on error.\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 | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex cad26df..7e8a803 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -3,6 +3,7 @@\n #include \"run-command.h\"\n \n char packet_buffer[LARGE_PACKET_MAX];\n+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@@ -155,6 +156,17 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)\n \treturn (write_in_full(fd, buf.buf, buf.len) == buf.len ? 0 : -1);\n }\n \n+int packet_write_gently(const int fd_out, const char *buf, size_t size)\n+{\n+\tif (size > sizeof(packet_write_buffer) - 4)\n+\t\treturn -1;\n+\tpacket_trace(buf, size, 1);\n+\tmemmove(packet_write_buffer + 4, buf, size);\n+\tsize += 4;\n+\tset_packet_header(packet_write_buffer, size);\n+\treturn (write_in_full(fd_out, packet_write_buffer, size) == size ? 0 : -1);\n+}\n+\n void packet_buf_write(struct strbuf *buf, const char *fmt, ...)\n {\n \tva_list args;\n-- \n2.9.2\n\n"},{"id":"300145","messageId":"20160825110752.31581-14-larsxschneider@gmail.com","threadId":"43930","inReplyTo":"20160825110752.31581-1-larsxschneider@gmail.com","subject":"[PATCH v6 13/13] read-cache: make sure file handles are not inherited by child processes","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-25T11:07:52Z","receivedAt":"2016-08-25T11:27:14Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\nConsider the case of a file that requires filtering and is present in\nbranch A but not in branch B. If A is the current HEAD and we checkout B\nthen the following happens:\n\n1. ce_compare_data() opens the file\n2. index_fd() detects that the file requires to run a clean filter and\n calls index_stream_convert_blob()\n4. index_stream_convert_blob() calls convert_to_git_filter_fd()\n5. convert_to_git_filter_fd() calls apply_filter() which creates a\n new long running filter process (in case it is the first file\n of this kind to be filtered)\n6. The new filter process inherits all file handles. This is the\n default on Linux/OSX and is explicitly defined in the\n `CreateProcessW` call in `mingw.c` on Windows.\n7. ce_compare_data() closes the file\n8. Git unlinks the file as it is not present in B\n\nThe unlink operation does not work on Windows because the filter process\nhas still an open handle to the file. Apparently that is no problem on\nLinux/OSX. Probably because \"[...] the two file descriptors share open\nfile status flags\" (see fork(2)).\n\nFix this problem by opening files in read-cache with the `O_CLOEXEC`\nflag to ensure that the file descriptor does not remain open in a newly\nspawned process. `O_CLOEXEC` is defined as `O_NOINHERIT` on Windows. A\nsimilar fix for temporary file handles was applied on Git for Windows\nalready:\nhttps://github.com/git-for-windows/git/commit/667b8b51ec850c3e1c7d75dee69dc13c29d1f162\n\nSigned-off-by: Lars Schneider \n---\n read-cache.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex db27766..f481dee 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -159,7 +159,7 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)\n static int ce_compare_data(const struct cache_entry *ce, struct stat *st)\n {\n \tint match = -1;\n-\tint fd = open(ce->name, O_RDONLY);\n+\tint fd = open(ce->name, O_RDONLY | O_CLOEXEC);\n \n \tif (fd >= 0) {\n \t\tunsigned char sha1[20];\n-- \n2.9.2\n\n"},{"id":"300146","messageId":"20160825110752.31581-3-larsxschneider@gmail.com","threadId":"43930","inReplyTo":"20160825110752.31581-1-larsxschneider@gmail.com","subject":"[PATCH v6 02/13] pkt-line: extract set_packet_header()","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-25T11:07:41Z","receivedAt":"2016-08-25T11:27:19Z","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.9.2\n\n"},{"id":"300149","messageId":"20160825110752.31581-1-larsxschneider@gmail.com","threadId":"43930","inReplyTo":null,"subject":"[PATCH v6 00/13] Git filter protocol","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-25T11:07:39Z","receivedAt":"2016-08-25T12:03:01Z","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/10\n\nThanks a lot for your reviews,\nLars\n\n\n## Changes since v5\n\n### Peff\n* http://public-inbox.org/git/20160810131541.ovpvgwdxjibae5gy%40sigill.intra.peff.net/\n * drop patch \"pkt-line: add `gentle` parameter to format_packet()\"\n* http://public-inbox.org/git/20160810143321.q7mjirgr5ynml5ff@sigill.intra.peff.net/\n * drop patch \"pkt-line: call packet_trace() only if a packet is actually send\"\n* http://public-inbox.org/git/20160810132814.gqnipsdwyzjmuqjy%40sigill.intra.peff.net/\n * make pkt-line write buffer static\n * replace PKTLINE_DATA_MAXLEN with sizeof(packet_write_buffer) - 4)\n in pkt-line.c to --> makes it easier to see that things are correct\n* http://public-inbox.org/git/20160810133745.wagccvvf35o3pbwb%40sigill.intra.peff.net/\n * check max content length before using packet_write_fmt_gently()\n\n\n### Junio\n* http://public-inbox.org/git/xmqqpopg5yqf.fsf%40gitster.mtv.corp.google.com/\n * change packet_write_gently_fmt() to packet_write_fmt_gently()\n* http://public-inbox.org/git/xmqq8tw45vtg.fsf@gitster.mtv.corp.google.com/\n * rename packet_write() to packet_write_fmt() before adding new functions\n\n\n## Stefan\n* http://public-inbox.org/git/CAGZ79kboxgBRHSa2s7CKZ1Uo%3D13WT%3DrT8VHCNJNj_Q9jQzZAYw%40mail.gmail.com/\n * state more clearly that everything after version=2 is specific to version=2\n * do not duplicate protocol in commit message,\n summarize design decisions instead\n * use single line comment\n * remove unnessary braces\n\n\n## Dscho\n* http://public-inbox.org/git/alpine.DEB.2.20.1608181617240.4924@virtualbox/\n * wrap commit messages at 72\n * s/CLOEXEX/CLOEXEC/\n\n\n## Interdiff (v5..v6)\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 6e563a6..6346700 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -393,13 +393,20 @@ standard input and standard output.\n\n Git starts the filter when it encounters the first file\n that needs to be cleaned or smudged. After the filter started\n-Git sends a welcome message, a list of supported protocol\n-version numbers, and a flush packet. Git expects to read the\n-welcome message and one protocol version number from the\n-previously sent list. Afterwards Git sends a list of supported\n-capabilities and a flush packet. Git expects to read a list of\n-desired capabilities, which must be a subset of the supported\n-capabilities list, and a flush packet as response:\n+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\ndiff --git a/convert.c b/convert.c\nindex e421f4a..362a0af 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -530,7 +530,9 @@ static int packet_write_list(int fd, const char *line, ...)\n {\n if (!line)\n break;\n- err = packet_write_gently_fmt(fd, \"%s\", line);\n+ if (strlen(line) > PKTLINE_DATA_MAXLEN)\n+ return -1;\n+ err = packet_write_fmt_gently(fd, \"%s\", line);\n if (err)\n return err;\n line = va_arg(args, const char*);\n@@ -686,11 +688,18 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n\n sigchain_push(SIGPIPE, SIG_IGN);\n\n- err = packet_write_gently_fmt(process->in, \"command=%s\\n\", filter_type);\n+\n+ err = (strlen(filter_type) > PKTLINE_DATA_MAXLEN);\n+ if (err)\n+ goto done;\n+ err = packet_write_fmt_gently(process->in, \"command=%s\\n\", filter_type);\n if (err)\n goto done;\n\n- err = packet_write_gently_fmt(process->in, \"pathname=%s\\n\", path);\n+ err = (strlen(path) > PKTLINE_DATA_MAXLEN);\n+ if (err)\n+ goto done;\n+ err = packet_write_fmt_gently(process->in, \"pathname=%s\\n\", path);\n if (err)\n goto done;\n\n@@ -722,9 +731,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n\n if (err || errno == EPIPE) {\n if (!strcmp(filter_status.buf, \"error\")) {\n- /*\n- * The filter signaled a problem with the file.\n- */\n+ /* The filter signaled a problem with the file. */\n } else if (!strcmp(filter_status.buf, \"error-all\")) {\n /*\n * The filter signaled a permanent problem. Don't try to filter\n@@ -818,9 +825,8 @@ static int read_convert_config(const char *var, const char *value, void *cb)\n if (!strcmp(\"clean\", key))\n return git_config_string(&drv->clean, var, value);\n\n- if (!strcmp(\"process\", key)) {\n+ if (!strcmp(\"process\", key))\n return git_config_string(&drv->process, var, value);\n- }\n\n if (!strcmp(\"required\", key)) {\n drv->required = git_config_bool(var, value);\ndiff --git a/pkt-line.c b/pkt-line.c\nindex b98e291..3033aa3 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -3,7 +3,7 @@\n #include \"run-command.h\"\n\n char packet_buffer[LARGE_PACKET_MAX];\n-char packet_write_buffer[LARGE_PACKET_MAX];\n+static 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@@ -107,6 +107,7 @@ void packet_buf_flush(struct strbuf *buf)\n static void set_packet_header(char *buf, const int size)\n {\n static char hexchar[] = \"0123456789abcdef\";\n+\n #define hex(a) (hexchar[(a) & 15])\n buf[0] = hex(size >> 12);\n buf[1] = hex(size >> 8);\n@@ -115,7 +116,7 @@ static void set_packet_header(char *buf, const int size)\n #undef hex\n }\n\n-static int format_packet(int gentle, struct strbuf *out, const char *fmt, va_list args)\n+static void format_packet(struct strbuf *out, const char *fmt, va_list args)\n {\n size_t orig_len, n;\n\n@@ -124,15 +125,11 @@ static int format_packet(int gentle, struct strbuf *out, const char *fmt, va_lis\n strbuf_vaddf(out, fmt, args);\n n = out->len - orig_len;\n\n- if (n > LARGE_PACKET_MAX) {\n- if (gentle)\n- return -1;\n- else\n- die(\"protocol error: impossibly long line\");\n- }\n+ if (n > LARGE_PACKET_MAX)\n+ die(\"protocol error: impossibly long line\");\n\n set_packet_header(&out->buf[orig_len], n);\n- return 0;\n+ packet_trace(out->buf + orig_len + 4, n - 4, 1);\n }\n\n void packet_write_fmt(int fd, const char *fmt, ...)\n@@ -142,28 +139,26 @@ void packet_write_fmt(int fd, const char *fmt, ...)\n\n strbuf_reset(&buf);\n va_start(args, fmt);\n- format_packet(0, &buf, fmt, args);\n+ format_packet(&buf, fmt, args);\n va_end(args);\n- packet_trace(buf.buf + 4, buf.len - 4, 1);\n write_or_die(fd, buf.buf, buf.len);\n }\n\n-int packet_write_gently_fmt(int fd, const char *fmt, ...)\n+int packet_write_fmt_gently(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(1, &buf, fmt, args);\n+ format_packet(&buf, fmt, args);\n va_end(args);\n- packet_trace(buf.buf + 4, buf.len - 4, 1);\n return (write_in_full(fd, buf.buf, buf.len) == buf.len ? 0 : -1);\n }\n\n int packet_write_gently(const int fd_out, const char *buf, size_t size)\n {\n- if (size > PKTLINE_DATA_MAXLEN)\n+ if (size > sizeof(packet_write_buffer) - 4)\n return -1;\n packet_trace(buf, size, 1);\n memmove(packet_write_buffer + 4, buf, size);\n@@ -177,7 +172,7 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...)\n va_list args;\n\n va_start(args, fmt);\n- format_packet(0, buf, fmt, args);\n+ format_packet(buf, fmt, args);\n va_end(args);\n }\n\n@@ -185,13 +180,14 @@ int packet_write_stream_with_flush_from_fd(int fd_in, int fd_out)\n {\n int err = 0;\n ssize_t bytes_to_write;\n+\n while (!err) {\n- bytes_to_write = xread(fd_in, packet_write_buffer, PKTLINE_DATA_MAXLEN);\n+ bytes_to_write = xread(fd_in, packet_write_buffer, sizeof(packet_write_buffer) - 4);\n if (bytes_to_write < 0)\n return COPY_READ_ERROR;\n if (bytes_to_write == 0)\n break;\n- if (bytes_to_write > PKTLINE_DATA_MAXLEN)\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 }\n@@ -205,9 +201,10 @@ int packet_write_stream_with_flush_from_buf(const char *src_in, size_t len, int\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) > PKTLINE_DATA_MAXLEN)\n- bytes_to_write = PKTLINE_DATA_MAXLEN;\n+ if ((len - bytes_written) > sizeof(packet_write_buffer) - 4)\n+ bytes_to_write = sizeof(packet_write_buffer) - 4;\n else\n bytes_to_write = len - bytes_written;\n if (bytes_to_write == 0)\ndiff --git a/pkt-line.h b/pkt-line.h\nindex ddd6041..6a3b7cf 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -24,7 +24,7 @@ void packet_write_fmt(int fd, const char *fmt, ...) __attribute__((format (print\n void packet_buf_flush(struct strbuf *buf);\n void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n int packet_flush_gently(int fd);\n-int packet_write_gently_fmt(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n+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\n\n\nLars Schneider (13):\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 pack-protocol: fix maximum pkt-line size\n convert: quote filter names in error messages\n convert: modernize tests\n convert: generate large test files only once\n convert: make apply_filter() adhere to standard Git error handling\n convert: add filter..process option\n read-cache: make sure file handles are not inherited by child\n processes\n\n Documentation/gitattributes.txt | 146 ++++++++-\n Documentation/technical/protocol-common.txt | 6 +-\n builtin/archive.c | 4 +-\n builtin/receive-pack.c | 4 +-\n builtin/remote-ext.c | 4 +-\n builtin/upload-archive.c | 4 +-\n connect.c | 2 +-\n convert.c | 373 ++++++++++++++++++---\n daemon.c | 2 +-\n http-backend.c | 2 +-\n pkt-line.c | 142 +++++++-\n pkt-line.h | 12 +-\n read-cache.c | 2 +-\n shallow.c | 2 +-\n t/t0021-conversion.sh | 482 +++++++++++++++++++++++++---\n t/t0021/rot13-filter.pl | 176 ++++++++++\n unpack-trees.c | 1 +\n upload-pack.c | 30 +-\n 18 files changed, 1272 insertions(+), 122 deletions(-)\n create mode 100755 t/t0021/rot13-filter.pl\n\n--\n2.9.2\n\n"},{"id":"300150","messageId":"20160825110752.31581-10-larsxschneider@gmail.com","threadId":"43930","inReplyTo":"20160825110752.31581-1-larsxschneider@gmail.com","subject":"[PATCH v6 09/13] convert: modernize tests","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-25T11:07:48Z","receivedAt":"2016-08-25T12:06:01Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\nUse `test_config` to set the config, check that files are empty with\n`test_must_be_empty`, compare files with `test_cmp`, and remove spaces\nafter \">\" and \"<\".\n\nSigned-off-by: Lars Schneider \n---\n t/t0021-conversion.sh | 62 +++++++++++++++++++++++++--------------------------\n 1 file changed, 31 insertions(+), 31 deletions(-)\n\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex 7bac2bc..7b45136 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -13,8 +13,8 @@ EOF\n chmod +x rot13.sh\n \n test_expect_success setup '\n-\tgit config filter.rot13.smudge ./rot13.sh &&\n-\tgit config filter.rot13.clean ./rot13.sh &&\n+\ttest_config filter.rot13.smudge ./rot13.sh &&\n+\ttest_config filter.rot13.clean ./rot13.sh &&\n \n \t{\n \t echo \"*.t filter=rot13\"\n@@ -38,8 +38,8 @@ script='s/^\\$Id: \\([0-9a-f]*\\) \\$/\\1/p'\n \n test_expect_success check '\n \n-\tcmp test.o test &&\n-\tcmp test.o test.t &&\n+\ttest_cmp test.o test &&\n+\ttest_cmp test.o test.t &&\n \n \t# ident should be stripped in the repository\n \tgit diff --raw --exit-code :test :test.i &&\n@@ -47,10 +47,10 @@ test_expect_success check '\n \tembedded=$(sed -ne \"$script\" test.i) &&\n \ttest \"z$id\" = \"z$embedded\" &&\n \n-\tgit cat-file blob :test.t > test.r &&\n+\tgit cat-file blob :test.t >test.r &&\n \n-\t./rot13.sh < test.o > test.t &&\n-\tcmp test.r test.t\n+\t./rot13.sh test.t &&\n+\ttest_cmp test.r test.t\n '\n \n # If an expanded ident ever gets into the repository, we want to make sure that\n@@ -130,7 +130,7 @@ test_expect_success 'filter shell-escaped filenames' '\n \n \t# delete the files and check them out again, using a smudge filter\n \t# that will count the args and echo the command-line back to us\n-\tgit config filter.argc.smudge \"sh ./argc.sh %f\" &&\n+\ttest_config filter.argc.smudge \"sh ./argc.sh %f\" &&\n \trm \"$normal\" \"$special\" &&\n \tgit checkout -- \"$normal\" \"$special\" &&\n \n@@ -141,7 +141,7 @@ test_expect_success 'filter shell-escaped filenames' '\n \ttest_cmp expect \"$special\" &&\n \n \t# do the same thing, but with more args in the filter expression\n-\tgit config filter.argc.smudge \"sh ./argc.sh %f --my-extra-arg\" &&\n+\ttest_config filter.argc.smudge \"sh ./argc.sh %f --my-extra-arg\" &&\n \trm \"$normal\" \"$special\" &&\n \tgit checkout -- \"$normal\" \"$special\" &&\n \n@@ -154,9 +154,9 @@ test_expect_success 'filter shell-escaped filenames' '\n '\n \n test_expect_success 'required filter should filter data' '\n-\tgit config filter.required.smudge ./rot13.sh &&\n-\tgit config filter.required.clean ./rot13.sh &&\n-\tgit config filter.required.required true &&\n+\ttest_config filter.required.smudge ./rot13.sh &&\n+\ttest_config filter.required.clean ./rot13.sh &&\n+\ttest_config filter.required.required true &&\n \n \techo \"*.r filter=required\" >.gitattributes &&\n \n@@ -165,17 +165,17 @@ test_expect_success 'required filter should filter data' '\n \n \trm -f test.r &&\n \tgit checkout -- test.r &&\n-\tcmp test.o test.r &&\n+\ttest_cmp test.o test.r &&\n \n \t./rot13.sh expected &&\n \tgit cat-file blob :test.r >actual &&\n-\tcmp expected actual\n+\ttest_cmp expected actual\n '\n \n test_expect_success 'required filter smudge failure' '\n-\tgit config filter.failsmudge.smudge false &&\n-\tgit config filter.failsmudge.clean cat &&\n-\tgit config filter.failsmudge.required true &&\n+\ttest_config filter.failsmudge.smudge false &&\n+\ttest_config filter.failsmudge.clean cat &&\n+\ttest_config filter.failsmudge.required true &&\n \n \techo \"*.fs filter=failsmudge\" >.gitattributes &&\n \n@@ -186,9 +186,9 @@ test_expect_success 'required filter smudge failure' '\n '\n \n test_expect_success 'required filter clean failure' '\n-\tgit config filter.failclean.smudge cat &&\n-\tgit config filter.failclean.clean false &&\n-\tgit config filter.failclean.required true &&\n+\ttest_config filter.failclean.smudge cat &&\n+\ttest_config filter.failclean.clean false &&\n+\ttest_config filter.failclean.required true &&\n \n \techo \"*.fc filter=failclean\" >.gitattributes &&\n \n@@ -197,8 +197,8 @@ test_expect_success 'required filter clean failure' '\n '\n \n test_expect_success 'filtering large input to small output should use little memory' '\n-\tgit config filter.devnull.clean \"cat >/dev/null\" &&\n-\tgit config filter.devnull.required true &&\n+\ttest_config filter.devnull.clean \"cat >/dev/null\" &&\n+\ttest_config filter.devnull.required true &&\n \tfor i in $(test_seq 1 30); do printf \"%1048576d\" 1; done >30MB &&\n \techo \"30MB filter=devnull\" >.gitattributes &&\n \tGIT_MMAP_LIMIT=1m GIT_ALLOC_LIMIT=1m git add 30MB\n@@ -207,7 +207,7 @@ test_expect_success 'filtering large input to small output should use little mem\n test_expect_success 'filter that does not read is fine' '\n \ttest-genrandom foo $((128 * 1024 + 1)) >big &&\n \techo \"big filter=epipe\" >.gitattributes &&\n-\tgit config filter.epipe.clean \"echo xyzzy\" &&\n+\ttest_config filter.epipe.clean \"echo xyzzy\" &&\n \tgit add big &&\n \tgit cat-file blob :big >actual &&\n \techo xyzzy >expect &&\n@@ -215,20 +215,20 @@ test_expect_success 'filter that does not read is fine' '\n '\n \n test_expect_success EXPENSIVE 'filter large file' '\n-\tgit config filter.largefile.smudge cat &&\n-\tgit config filter.largefile.clean cat &&\n+\ttest_config filter.largefile.smudge cat &&\n+\ttest_config filter.largefile.clean cat &&\n \tfor i in $(test_seq 1 2048); do printf \"%1048576d\" 1; done >2GB &&\n \techo \"2GB filter=largefile\" >.gitattributes &&\n \tgit add 2GB 2>err &&\n-\t! test -s err &&\n+\ttest_must_be_empty err &&\n \trm -f 2GB &&\n \tgit checkout -- 2GB 2>err &&\n-\t! test -s err\n+\ttest_must_be_empty err\n '\n \n test_expect_success \"filter: clean empty file\" '\n-\tgit config filter.in-repo-header.clean \"echo cleaned && cat\" &&\n-\tgit config filter.in-repo-header.smudge \"sed 1d\" &&\n+\ttest_config filter.in-repo-header.clean \"echo cleaned && cat\" &&\n+\ttest_config filter.in-repo-header.smudge \"sed 1d\" &&\n \n \techo \"empty-in-worktree filter=in-repo-header\" >>.gitattributes &&\n \t>empty-in-worktree &&\n@@ -240,8 +240,8 @@ test_expect_success \"filter: clean empty file\" '\n '\n \n test_expect_success \"filter: smudge empty file\" '\n-\tgit config filter.empty-in-repo.clean \"cat >/dev/null\" &&\n-\tgit config filter.empty-in-repo.smudge \"echo smudged && cat\" &&\n+\ttest_config filter.empty-in-repo.clean \"cat >/dev/null\" &&\n+\ttest_config filter.empty-in-repo.smudge \"echo smudged && cat\" &&\n \n \techo \"empty-in-repo filter=empty-in-repo\" >>.gitattributes &&\n \techo dead data walking >empty-in-repo &&\n-- \n2.9.2\n\n"},{"id":"300151","messageId":"20160825110752.31581-9-larsxschneider@gmail.com","threadId":"43930","inReplyTo":"20160825110752.31581-1-larsxschneider@gmail.com","subject":"[PATCH v6 08/13] convert: quote filter names in error messages","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-25T11:07:47Z","receivedAt":"2016-08-25T12:19:23Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\nGit filter driver commands with spaces (e.g. `filter.sh foo`) are hard to\nread in error messages. Quote them to improve the readability.\n\nSigned-off-by: Lars Schneider \n---\n convert.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex b1614bf..522e2c5 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -397,7 +397,7 @@ static int filter_buffer_or_fd(int in, int out, void *data)\n \tchild_process.out = out;\n \n \tif (start_command(&child_process))\n-\t\treturn error(\"cannot fork to run external filter %s\", params->cmd);\n+\t\treturn error(\"cannot fork to run external filter '%s'\", params->cmd);\n \n \tsigchain_push(SIGPIPE, SIG_IGN);\n \n@@ -415,13 +415,13 @@ static int filter_buffer_or_fd(int in, int out, void *data)\n \tif (close(child_process.in))\n \t\twrite_err = 1;\n \tif (write_err)\n-\t\terror(\"cannot feed the input to external filter %s\", params->cmd);\n+\t\terror(\"cannot feed the input to external filter '%s'\", params->cmd);\n \n \tsigchain_pop(SIGPIPE);\n \n \tstatus = finish_command(&child_process);\n \tif (status)\n-\t\terror(\"external filter %s failed %d\", params->cmd, status);\n+\t\terror(\"external filter '%s' failed %d\", params->cmd, status);\n \n \tstrbuf_release(&cmd);\n \treturn (write_err || status);\n@@ -462,15 +462,15 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,\n \t\treturn 0;\t/* error was already reported */\n \n \tif (strbuf_read(&nbuf, async.out, len) < 0) {\n-\t\terror(\"read from external filter %s failed\", cmd);\n+\t\terror(\"read from external filter '%s' failed\", cmd);\n \t\tret = 0;\n \t}\n \tif (close(async.out)) {\n-\t\terror(\"read from external filter %s failed\", cmd);\n+\t\terror(\"read from external filter '%s' failed\", cmd);\n \t\tret = 0;\n \t}\n \tif (finish_async(&async)) {\n-\t\terror(\"external filter %s failed\", cmd);\n+\t\terror(\"external filter '%s' failed\", cmd);\n \t\tret = 0;\n \t}\n \n-- \n2.9.2\n\n"},{"id":"300153","messageId":"20160825110752.31581-5-larsxschneider@gmail.com","threadId":"43930","inReplyTo":"20160825110752.31581-1-larsxschneider@gmail.com","subject":"[PATCH v6 04/13] pkt-line: add packet_flush_gently()","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-25T11:07:43Z","receivedAt":"2016-08-25T12:47:26Z","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 | 6 ++++++\n pkt-line.h | 1 +\n 2 files changed, 7 insertions(+)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 3e8b2fb..cad26df 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -91,6 +91,12 @@ void packet_flush(int fd)\n \twrite_or_die(fd, \"0000\", 4);\n }\n \n+int packet_flush_gently(int fd)\n+{\n+\tpacket_trace(\"0000\", 4, 1);\n+\treturn (write_in_full(fd, \"0000\", 4) == 4 ? 0 : -1);\n+}\n+\n void packet_buf_flush(struct strbuf *buf)\n {\n \tpacket_trace(\"0000\", 4, 1);\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 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.9.2\n\n"},{"id":"300162","messageId":"20160825110752.31581-8-larsxschneider@gmail.com","threadId":"43930","inReplyTo":"20160825110752.31581-1-larsxschneider@gmail.com","subject":"[PATCH v6 07/13] pack-protocol: fix maximum pkt-line size","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-25T11:07:46Z","receivedAt":"2016-08-25T15:00:49Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\nAccording to LARGE_PACKET_MAX in pkt-line.h the maximal length of a\npkt-line packet is 65520 bytes. The pkt-line header takes 4 bytes and\ntherefore the pkt-line data component must not exceed 65516 bytes.\n\nSigned-off-by: Lars Schneider \n---\n Documentation/technical/protocol-common.txt | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/technical/protocol-common.txt b/Documentation/technical/protocol-common.txt\nindex bf30167..ecedb34 100644\n--- a/Documentation/technical/protocol-common.txt\n+++ b/Documentation/technical/protocol-common.txt\n@@ -67,9 +67,9 @@ with non-binary data the same whether or not they contain the trailing\n LF (stripping the LF if present, and not complaining when it is\n missing).\n \n-The maximum length of a pkt-line's data component is 65520 bytes.\n-Implementations MUST NOT send pkt-line whose length exceeds 65524\n-(65520 bytes of payload + 4 bytes of length data).\n+The maximum length of a pkt-line's data component is 65516 bytes.\n+Implementations MUST NOT send pkt-line whose length exceeds 65520\n+(65516 bytes of payload + 4 bytes of length data).\n \n Implementations SHOULD NOT send an empty pkt-line (\"0004\").\n \n-- \n2.9.2\n\n"},{"id":"300179","messageId":"CAGZ79kbEoYVM_+MWkajT2pHN1OKZXATe5d+Dv_uSJ7dyPGxUeg@mail.gmail.com","threadId":"43930","inReplyTo":"20160825110752.31581-4-larsxschneider@gmail.com","subject":"Re: [PATCH v6 03/13] pkt-line: add packet_write_fmt_gently()","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-25T18:12:14Z","receivedAt":"2016-08-25T18:12:31Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> +int packet_write_fmt_gently(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\nformat_packet also takes care of tracing the contents,\nwhich was a bit unexpected to me.\nDo we also want to trace failure?\n"},{"id":"300183","messageId":"CAGZ79kaVg40H-LeDtFfDYqDFDDbr+um3ZYj8oAaqGu+q6k5e+A@mail.gmail.com","threadId":"43930","inReplyTo":"20160825110752.31581-7-larsxschneider@gmail.com","subject":"Re: [PATCH v6 06/13] pkt-line: add functions to read/write flush terminated packet streams","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-25T18:46:06Z","receivedAt":"2016-08-25T18:46:53Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Aug 25, 2016 at 4:07 AM, wrote:\n> From: Lars Schneider \n>\n> packet_write_stream_with_flush_from_fd() and\n> packet_write_stream_with_flush_from_buf() write a stream of packets. All\n> content packets use the maximal packet size except for the last one.\n> After the last content packet a `flush` control packet is written.\n>\n> packet_read_till_flush() reads arbitrary sized packets until it detects\n> a `flush` packet.\n\nSo the API provided by these read/write functions is intended\nto move a huge chunks of data. And as it puts the data on the wire one\npacket after the other without the possibility to intervene and e.g. send\na side channel progress bar update, I would question the design of this.\nIf I understand correctly this will be specifically used for large\nfiles locally,\nso e.g. a file of 5 GB (such as a virtual machine tracked in Git), would\nrequire about 80k packets.\n\nInstead of having many packets of max length and then a remainder,\nI would suggest to invent larger packets for this use case. Then we can\njust send one packet instead.\n\nCurrently a packet consists of 4 bytes indicating the length in hex\nand then the payload of length-4 bytes. As the length is in hex\nthe characters in the first 4 bytes are [0-9a-f], we can easily add another\nmeaning for the length, e.g.:\n\n A packet starts with the overall length and then the payload.\n If the first character of the length is 'v' the length is encoded as a\n variable length quantity[1]. The high bit of the char indicates if\n the next char is still part of the length field. The length must not exceed\n LLONG_MAX (which results in a payload of 9223 Petabyte, so\n enough for the foreseeable future).\n\n [1] A variable-length quantity (VLQ) is a universal code that uses\n an arbitrary number of bytes to represent an arbitrarily large integer.\n https://en.wikipedia.org/wiki/Variable-length_quantity\n\nThe neat thing about the packet system is we can dedicate packets\nto different channels (such as the side channels), but with the provided\nAPI here this makes it impossible to later add in these side channel\nas it is a pure streaming API now. So let's remove the complication\nof having to send multiple packets and just go with one large packet\ninstead.\n\n--\n I understand that my proposal would require writing code again,\n but it has also some long term advantages in the networking stack\n of Git: There are some worries that a capabilities line in fetch/push\n might overflow in the far future, when there are lots of capabilities.\n\n Also a few days ago there was a proposal to add all symbolic refs\n to a capabilities line, which Peff shot down as \"the packet may be\n too small\".\n\n There is an incredible hack that allows transporting refs > 64kB IIRC.\n\n All these things could go away with the variable length encoded\n packets. But to make them go away in the future we would need\n to start with these variable length packets today. ;)\n\nJust food for thought.\n\nThanks,\nStefan\n"},{"id":"300184","messageId":"EEB25464-5EBC-4CAE-A104-2896290A1E06@gmail.com","threadId":"43930","inReplyTo":"CAGZ79kbEoYVM_+MWkajT2pHN1OKZXATe5d+Dv_uSJ7dyPGxUeg@mail.gmail.com","subject":"Re: [PATCH v6 03/13] pkt-line: add packet_write_fmt_gently()","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-25T18:47:30Z","receivedAt":"2016-08-25T18:47:57Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"On 25 Aug 2016, at 20:12, Stefan Beller wrote:\n\n>> +int packet_write_fmt_gently(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> \n> format_packet also takes care of tracing the contents,\n> which was a bit unexpected to me.\n\nTo me, too :)\nhttp://public-inbox.org/git/20160810143321.q7mjirgr5ynml5ff@sigill.intra.peff.net/\n\nThe series is already pretty large and therefore I decided to leave this as-is.\n\n> Do we also want to trace failure?\n\nYou mean in all new *_gently() functions? Good idea!\n\nThanks,\nLars"},{"id":"300185","messageId":"CAGZ79kaJn-yf28+Ls2JyqSs6Jt-Oj2JW-sAXQn-Oe5xaxSyjMQ@mail.gmail.com","threadId":"43930","inReplyTo":"20160825110752.31581-8-larsxschneider@gmail.com","subject":"Re: [PATCH v6 07/13] pack-protocol: fix maximum pkt-line size","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-25T18:59:41Z","receivedAt":"2016-08-25T18:59:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Aug 25, 2016 at 4:07 AM, wrote:\n> From: Lars Schneider \n>\n> According to LARGE_PACKET_MAX in pkt-line.h the maximal length of a\n> pkt-line packet is 65520 bytes. The pkt-line header takes 4 bytes and\n> therefore the pkt-line data component must not exceed 65516 bytes.\n>\n> Signed-off-by: Lars Schneider \n> ---\n\nI was recently encouraged off list to really try hard to go with\nshorter series's.\n(Duh! Everyone knows that shorter series go in faster ;)\n\nAnd as this is a strict bug fix of Documentation that makes sense\noutside this series,\nI'd like to encourage you to send this as a separate patch in case you\nneed further\nrerolls.\n\nThanks,\nStefan\n\n>\n"},{"id":"300187","messageId":"CAGZ79kZ5utc+0iXKPh=tu4rD9H6+AKZmc08_jLxQhgiuFUA+Yw@mail.gmail.com","threadId":"43930","inReplyTo":"20160825110752.31581-11-larsxschneider@gmail.com","subject":"Re: [PATCH v6 10/13] convert: generate large test files only once","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-25T19:17:21Z","receivedAt":"2016-08-25T19:25:01Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Aug 25, 2016 at 4:07 AM, wrote:\n> From: Lars Schneider \n>\n> Generate more interesting large test files\n\nHow are the large test files more interesting?\n(interesting in the notion of covering more potential bugs?\neasier to debug? better to maintain, or just a pleasant read?)\n\n> with pseudo random characters\n> in between and reuse these test files in multiple tests. Run tests\n> formerly marked as EXPENSIVE every time but with a smaller data set.\n\nSounds good to me.\n\n>\n> Signed-off-by: Lars Schneider \n> ---\n"},{"id":"300188","messageId":"6EC60D1B-3A92-4381-A87E-50417CF04A49@gmail.com","threadId":"43930","inReplyTo":"CAGZ79kaJn-yf28+Ls2JyqSs6Jt-Oj2JW-sAXQn-Oe5xaxSyjMQ@mail.gmail.com","subject":"Re: [PATCH v6 07/13] pack-protocol: fix maximum pkt-line size","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-25T19:35:08Z","receivedAt":"2016-08-25T19:36:22Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 25 Aug 2016, at 20:59, Stefan Beller wrote:\n> \n>> On Thu, Aug 25, 2016 at 4:07 AM, wrote:\n>> From: Lars Schneider \n>> \n>> According to LARGE_PACKET_MAX in pkt-line.h the maximal length of a\n>> pkt-line packet is 65520 bytes. The pkt-line header takes 4 bytes and\n>> therefore the pkt-line data component must not exceed 65516 bytes.\n>> \n>> Signed-off-by: Lars Schneider \n>> ---\n> \n> I was recently encouraged off list to really try hard to go with\n> shorter series's.\n> (Duh! Everyone knows that shorter series go in faster ;)\n> \n> And as this is a strict bug fix of Documentation that makes sense\n> outside this series,\n> I'd like to encourage you to send this as a separate patch in case you\n> need further\n> rerolls.\n\nAgreed!\n\nThanks,\nLars\n"},{"id":"300190","messageId":"E574E25A-F87F-416C-88E5-E3CE1F6B54D0@gmail.com","threadId":"43930","inReplyTo":"CAGZ79kZ5utc+0iXKPh=tu4rD9H6+AKZmc08_jLxQhgiuFUA+Yw@mail.gmail.com","subject":"Re: [PATCH v6 10/13] convert: generate large test files only once","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-25T19:54:36Z","receivedAt":"2016-08-25T19:54:48Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n\n> On 25 Aug 2016, at 21:17, Stefan Beller wrote:\n> \n>> On Thu, Aug 25, 2016 at 4:07 AM, wrote:\n>> From: Lars Schneider \n>> \n>> Generate more interesting large test files\n> \n> How are the large test files more interesting?\n> (interesting in the notion of covering more potential bugs?\n> easier to debug? better to maintain, or just a pleasant read?)\n\nThe old large test file was 1MB of zeros and 1 byte with a one, repeated 2048 times.\n\nSince the filter uses 64k packets we would test a large number of equally looking packets.\n\nThat's why I thought the pseudo random content is more interesting.\n\n\n>> with pseudo random characters\n>> in between and reuse these test files in multiple tests. Run tests\n>> formerly marked as EXPENSIVE every time but with a smaller data set.\n> \n> Sounds good to me.\n\nThank you.\nLars"},{"id":"300202","messageId":"xmqq8tvkle6o.fsf@gitster.mtv.corp.google.com","threadId":"43930","inReplyTo":"20160825110752.31581-4-larsxschneider@gmail.com","subject":"Re: [PATCH v6 03/13] pkt-line: add packet_write_fmt_gently()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-25T21:41:03Z","receivedAt":"2016-08-25T21:41:12Z","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_write_fmt() would die in case of a write error even though for\n> some callers an error would be acceptable. Add packet_write_fmt_gently()\n> which writes a formatted pkt-line and returns `0` for success and `-1`\n> for an error.\n>\n> Signed-off-by: Lars Schneider \n> ---\n> pkt-line.c | 12 ++++++++++++\n> pkt-line.h | 1 +\n> 2 files changed, 13 insertions(+)\n>\n> diff --git a/pkt-line.c b/pkt-line.c\n> index e8adc0f..3e8b2fb 100644\n> --- a/pkt-line.c\n> +++ b/pkt-line.c\n> @@ -137,6 +137,18 @@ void packet_write_fmt(int fd, const char *fmt, ...)\n> \twrite_or_die(fd, buf.buf, buf.len);\n> }\n> \n> +int packet_write_fmt_gently(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> +\tva_end(args);\n> +\treturn (write_in_full(fd, buf.buf, buf.len) == buf.len ? 0 : -1);\n> +}\n\nEven though its only a handful lines, it is a bit ugly to have a\ncompletely copied implementation only to have _gently(). I suspect\nthat you should be able to\n\n\tstatic int packet_write_fmt_1(int fd, int gently,\n\t\t\t\t\tconst char *fmt, va_list args)\n {\n\t\tstruct strbuf buf = STRBUF_INIT;\n\t\tsize_t count;\n\n\t\tformat_packet(&buf, fmt, args);\n\t\t\n\t\tcount = write_in_full(fd, buf.buf, buf.len);\n if (count == buf.len)\n \treturn 0;\n\t\tif (!gently) {\n\t\t\tcheck_pipe(errno);\n \tdie_errno(\"write error\");\n\t\t}\n return -1;\n\t}\n\nand then share that between the existing one:\n\n\tvoid packet_write_fmt(int fd, const char *fmt, ...)\n {\n\t\tva_list args;\n\t va_start(args, fmt);\n packet_write_fmt_1(fd, 0, fmt, args);\n va_end(args);\n\t}\n\nand the new one:\n\n\tvoid packet_write_fmt_gently(int fd, const char *fmt, ...)\n {\n\t\tint status;\n\t\tva_list args;\n\t va_start(args, fmt);\n status = packet_write_fmt_1(fd, 1, fmt, args);\n va_end(args);\n\t\treturn status;\n\t}\n\n> void packet_buf_write(struct strbuf *buf, const char *fmt, ...)\n> {\n> \tva_list args;\n> diff --git a/pkt-line.h b/pkt-line.h\n> index 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"},{"id":"300203","messageId":"xmqq4m68ldrg.fsf@gitster.mtv.corp.google.com","threadId":"43930","inReplyTo":"20160825110752.31581-6-larsxschneider@gmail.com","subject":"Re: [PATCH v6 05/13] pkt-line: add packet_write_gently()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-25T21:50:11Z","receivedAt":"2016-08-25T21:50:21Z","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_write_fmt() has two shortcomings. First, it uses format_packet()\n> which lets the caller only send string data via \"%s\". That means it\n> cannot be used for arbitrary data that may contain NULs. Second, it will\n> always die on error.\n\nAs you introduced _gently in 3/13, the latter is no longer a valid\nexcuse to add this function. Just remove the sentence \"Second, ...\".\n\n> Add packet_write_gently() which writes arbitrary data and returns `0`\n> for success and `-1` for an error. This function is used by other\n> pkt-line functions in a subsequent patch.\n>\n> Signed-off-by: Lars Schneider \n> ---\n> pkt-line.c | 12 ++++++++++++\n> 1 file changed, 12 insertions(+)\n>\n> diff --git a/pkt-line.c b/pkt-line.c\n> index cad26df..7e8a803 100644\n> --- a/pkt-line.c\n> +++ b/pkt-line.c\n> @@ -3,6 +3,7 @@\n> #include \"run-command.h\"\n> \n> char packet_buffer[LARGE_PACKET_MAX];\n> +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> @@ -155,6 +156,17 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)\n> \treturn (write_in_full(fd, buf.buf, buf.len) == buf.len ? 0 : -1);\n> }\n> \n> +int packet_write_gently(const int fd_out, const char *buf, size_t size)\n> +{\n> +\tif (size > sizeof(packet_write_buffer) - 4)\n> +\t\treturn -1;\n> +\tpacket_trace(buf, size, 1);\n> +\tmemmove(packet_write_buffer + 4, buf, size);\n> +\tsize += 4;\n> +\tset_packet_header(packet_write_buffer, size);\n\nIt may not matter all that much, but from code-reader's point of\nview, when you know packet_write_buffer[] will contain things A and\nB in this order, and when you have enough information to compute A\nbefore stasrting to fill packet_write_buffer[], I would prefer to\nsee you actually fill the buffer in that natural order.\n\nDo you anticipate future need of non-gently variant of this\nfunction? If so, perhaps a helper that takes a boolean \"am I\nworking for the gently variant?\" may help share more code.\n\n> +\treturn (write_in_full(fd_out, packet_write_buffer, size) == size ? 0 : -1);\n> +}\n> +\n> void packet_buf_write(struct strbuf *buf, const char *fmt, ...)\n> {\n> \tva_list args;\n"},{"id":"300205","messageId":"xmqqvayojx9e.fsf@gitster.mtv.corp.google.com","threadId":"43930","inReplyTo":"CAGZ79kaVg40H-LeDtFfDYqDFDDbr+um3ZYj8oAaqGu+q6k5e+A@mail.gmail.com","subject":"Re: [PATCH v6 06/13] pkt-line: add functions to read/write flush terminated packet streams","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-25T22:31:57Z","receivedAt":"2016-08-25T22:32:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> So the API provided by these read/write functions is intended\n> to move a huge chunks of data. And as it puts the data on the wire one\n> packet after the other without the possibility to intervene and e.g. send\n> a side channel progress bar update, I would question the design of this.\n\nHmph, I didn't think about it.\n\nBut shouldn't one be able to set up sideband and channel one such\nlarge transfer on one band, while multiplexing other payload on\nother bands?\n\n> If I understand correctly this will be specifically used for large\n> files locally,\n> so e.g. a file of 5 GB (such as a virtual machine tracked in Git), would\n> require about 80k packets.\n\nWhat is wrong about that? 4*80k = 320kB overhead for length fields\nto transfer 5GB worth of data? I do not think it is worth worrying\nabout it.\n\nBut I am more surprised by seeing that \"why not a single huge\npacket\" suggestion immediately after you talked about \"without the\npossibility to intervene\". They do not seem to be remotely related;\nin fact, they are going into opposite directions.\n\nPuzzled.\n"},{"id":"300206","messageId":"xmqqzio0jxh7.fsf@gitster.mtv.corp.google.com","threadId":"43930","inReplyTo":"20160825110752.31581-7-larsxschneider@gmail.com","subject":"Re: [PATCH v6 06/13] pkt-line: add functions to read/write flush terminated packet streams","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-25T22:27:16Z","receivedAt":"2016-08-25T22:45:33Z","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_write_stream_with_flush_from_fd() and\n> packet_write_stream_with_flush_from_buf() write a stream of packets. All\n> content packets use the maximal packet size except for the last one.\n> After the last content packet a `flush` control packet is written.\n> packet_read_till_flush() reads arbitrary sized packets until it detects\n> a `flush` packet.\n\nThese are awkwardly named and I couldn't guess what the input is (I\ncan tell one is to read from fd and the other is buffer,\nbut it is unclear if that is in packetized form or just raw data\nstream to be copied to the end from their names) without reading the\nimplementation. I _think_ you read a raw stream of data through the\nend (either EOF or length limit) and write it out packetized, and\nuse the flush packet to mark the end of the stream. In my mind,\nthat is \"writing a packetized stream\". The words \"packetizing\" and\n\"stream\" imply that the stream could consist of more data than what\nwould fit in a single packet, which in turn implies that there needs\na way to mark the end of one data item, so with_flush does not\nnecessarily have to be their names.\n\nThe counter-part would be \"reading a packetized stream\".\n\n> +int packet_write_stream_with_flush_from_fd(int fd_in, int fd_out)\n> +{\n\nEspecially this one I am tempted to suggest \"copy-to-packetized-stream\",\nas it reads a stream from one fd and then copies out while packetizing.\n\n> +\tint err = 0;\n> +\tssize_t bytes_to_write;\n> +\n> +\twhile (!err) {\n> +\t\tbytes_to_write = xread(fd_in, packet_write_buffer, sizeof(packet_write_buffer) - 4);\n> +\t\tif (bytes_to_write < 0)\n> +\t\t\treturn COPY_READ_ERROR;\n> +\t\tif (bytes_to_write == 0)\n> +\t\t\tbreak;\n> +\t\tif (bytes_to_write > sizeof(packet_write_buffer) - 4)\n> +\t\t\treturn COPY_WRITE_ERROR;\n\n... and you seem to agree with me by using COPY here.\n\n> +\t\terr = packet_write_gently(fd_out, packet_write_buffer, bytes_to_write);\n> +\t}\n> +\tif (!err)\n> +\t\terr = packet_flush_gently(fd_out);\n> +\treturn err;\n> +}\n> +\n> +int packet_write_stream_with_flush_from_buf(const char *src_in, size_t len, int fd_out)\n> +{\n> +\tint err = 0;\n> +\tsize_t bytes_written = 0;\n> +\tsize_t bytes_to_write;\n> +\n> +\twhile (!err) {\n> +\t\tif ((len - bytes_written) > sizeof(packet_write_buffer) - 4)\n> +\t\t\tbytes_to_write = sizeof(packet_write_buffer) - 4;\n> +\t\telse\n> +\t\t\tbytes_to_write = len - bytes_written;\n> +\t\tif (bytes_to_write == 0)\n> +\t\t\tbreak;\n\nThe lack of COPY_WRITE_ERROR puzzled me briefly here. If you are\nassuming that your math at the beginning of this loop is correct and\nbytes_to_write will never exceed the write-buffer size, I think you\nshould be able to (and it would be better to) assume that the math\nyou do to tell xread() up to how many bytes it is allowed to read at\nonce is also correct, losing the COPY_WRITE_ERROR check in the other\nfunction. You can choose to play safer and do a check in this\nfunction, too. Either way, we would want to be consistent.\n\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> +ssize_t packet_read_till_flush(int fd_in, struct strbuf *sb_out)\n> +{\n> +\tint len, ret;\n> +\tint options = PACKET_READ_GENTLE_ON_EOF;\n> +\tchar linelen[4];\n> +\n> +\tsize_t oldlen = sb_out->len;\n> +\tsize_t oldalloc = sb_out->alloc;\n> +\n> +\tfor (;;) {\n> +\t\t/* Read packet header */\n> +\t\tret = get_packet_data(fd_in, NULL, NULL, linelen, 4, options);\n> +\t\tif (ret < 0)\n> +\t\t\tgoto done;\n> +\t\tlen = packet_length(linelen);\n> +\t\tif (len < 0)\n> +\t\t\tdie(\"protocol error: bad line length character: %.4s\", linelen);\n> +\t\tif (!len) {\n> +\t\t\t/* Found a flush packet - Done! */\n> +\t\t\tpacket_trace(\"0000\", 4, 0);\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tlen -= 4;\n> +\n> +\t\t/* Read packet content */\n> +\t\tstrbuf_grow(sb_out, len);\n> +\t\tret = get_packet_data(fd_in, NULL, NULL, sb_out->buf + sb_out->len, len, options);\n> +\t\tif (ret < 0)\n> +\t\t\tgoto done;\n> +\t\tif (ret != len) {\n> +\t\t\terror(\"protocol error: incomplete read (expected %d, got %d)\", len, ret);\n> +\t\t\tgoto done;\n> +\t\t}\n> +\n> +\t\tpacket_trace(sb_out->buf + sb_out->len, len, 0);\n\nAll of the above seems to pretty much duplicate the logic in\npacket_read(), except that this user does not need options handling\nit has. Is optimizing that out the reason why you open-coded it\nhere?\n\nOr is it because you cannot tell if you got a truly empty packet or\nyou got a flush from outside packet_read(), and you wanted to make\nsure that you won't be fooled by a normal packet with 0-length\npayload?\n\nIf the latter is the reason, it may be a viable alternative to\nupdate packet_read() to take PACKET_READ_IGNORE_EMPTY_PACKET, i.e. a\nnew bit in its options parameter, so that a normal packet with\n0-length payload is simply ignored there (i.e. even without\nreturning, packet_read() would repeat from the beginning when it got\nsuch a packet). That way, the above would become \n\n\tstrbuf_grow(); /* enough to hold max-packet-len more bytes */\n\tlen = packet_read();\n if (!len)\n \t/* we cannot get 0 unless we see flush */\n break;\n\nwhich may be a lot cleaner?\n\n> +\t\tsb_out->len += len;\n> +\t}\n> +\n> +done:\n> +\tif (ret < 0) {\n> +\t\tif (oldalloc == 0)\n> +\t\t\tstrbuf_release(sb_out);\n> +\t\telse\n> +\t\t\tstrbuf_setlen(sb_out, oldlen);\n> +\t\treturn ret; /* unexpected EOF */\n> +\t}\n> +\treturn sb_out->len - oldlen;\n> +}\n> diff --git a/pkt-line.h b/pkt-line.h\n> index 3fa0899..9616117 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 packet_write_stream_with_flush_from_fd(int fd_in, int fd_out);\n> +int packet_write_stream_with_flush_from_buf(const char *src_in, size_t len, int fd_out);\n> \n> /*\n> * Read a packetized line into the buffer, which must be at least size bytes\n> @@ -77,6 +79,11 @@ char *packet_read_line(int fd, int *size);\n> */\n> char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size);\n> \n> +/*\n> + * Reads a stream of variable sized packets until a flush packet is detected.\n> + */\n> +ssize_t packet_read_till_flush(int fd_in, struct strbuf *sb_out);\n> +\n> #define DEFAULT_PACKET_MAX 1000\n> #define LARGE_PACKET_MAX 65520\n> extern char packet_buffer[LARGE_PACKET_MAX];\n"},{"id":"300212","messageId":"269094FB-81F3-4220-BE7A-F957EDF6F808@gmail.com","threadId":"43930","inReplyTo":"CAGZ79kaVg40H-LeDtFfDYqDFDDbr+um3ZYj8oAaqGu+q6k5e+A@mail.gmail.com","subject":"Re: [PATCH v6 06/13] pkt-line: add functions to read/write flush terminated packet streams","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-25T19:33:34Z","receivedAt":"2016-08-25T23:31:05Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 25 Aug 2016, at 20:46, Stefan Beller wrote:\n> \n>> On Thu, Aug 25, 2016 at 4:07 AM, wrote:\n>> From: Lars Schneider \n>> \n>> packet_write_stream_with_flush_from_fd() and\n>> packet_write_stream_with_flush_from_buf() write a stream of packets. All\n>> content packets use the maximal packet size except for the last one.\n>> After the last content packet a `flush` control packet is written.\n>> \n>> packet_read_till_flush() reads arbitrary sized packets until it detects\n>> a `flush` packet.\n> \n> So the API provided by these read/write functions is intended\n> to move a huge chunks of data. And as it puts the data on the wire one\n> packet after the other without the possibility to intervene and e.g. send\n> a side channel progress bar update, I would question the design of this.\n> If I understand correctly this will be specifically used for large\n> files locally,\n> so e.g. a file of 5 GB (such as a virtual machine tracked in Git), would\n> require about 80k packets.\n\nPeff suggested this approach arguing that the overhead is neglectable:\nhttp://public-inbox.org/git/20160720134916.GB19359@sigill.intra.peff.net/\n\n\n> Instead of having many packets of max length and then a remainder,\n> I would suggest to invent larger packets for this use case. Then we can\n> just send one packet instead.\n> \n> Currently a packet consists of 4 bytes indicating the length in hex\n> and then the payload of length-4 bytes. As the length is in hex\n> the characters in the first 4 bytes are [0-9a-f], we can easily add another\n> meaning for the length, e.g.:\n> \n> A packet starts with the overall length and then the payload.\n> If the first character of the length is 'v' the length is encoded as a\n> variable length quantity[1]. The high bit of the char indicates if\n> the next char is still part of the length field. The length must not exceed\n> LLONG_MAX (which results in a payload of 9223 Petabyte, so\n> enough for the foreseeable future).\n\nEventually I would like to resurrect Joey's cleanFromFile/smudgeToFile idea:\n\nhttp://public-inbox.org/git/1468277112-9909-3-git-send-email-joeyh@joeyh.name/\n\nThen we would not need to transfer that much data over the pipes. However, I wonder if the large amount of packets would actually be a problem. Honestly, I would prefer to not change Git's packet format in this already large series ;-)\n\n\n> [1] A variable-length quantity (VLQ) is a universal code that uses\n> an arbitrary number of bytes to represent an arbitrarily large integer.\n> https://en.wikipedia.org/wiki/Variable-length_quantity\n> \n> The neat thing about the packet system is we can dedicate packets\n> to different channels (such as the side channels), but with the provided\n> API here this makes it impossible to later add in these side channel\n> as it is a pure streaming API now. So let's remove the complication\n> of having to send multiple packets and just go with one large packet\n> instead.\n\nI tried to design the protocol as flexible as possible for the future with a version negotiation and a capabilities list. Therefore, I would think it should be possible to implement these ideas in the future if they are required.\n\n\n> --\n> I understand that my proposal would require writing code again,\n> but it has also some long term advantages in the networking stack\n> of Git: There are some worries that a capabilities line in fetch/push\n> might overflow in the far future, when there are lots of capabilities.\n> \n> Also a few days ago there was a proposal to add all symbolic refs\n> to a capabilities line, which Peff shot down as \"the packet may be\n> too small\".\n> \n> There is an incredible hack that allows transporting refs > 64kB IIRC.\n> \n> All these things could go away with the variable length encoded\n> packets. But to make them go away in the future we would need\n> to start with these variable length packets today. ;)\n> \n> Just food for thought.\n\nThanks for thinking it through that thoroughly! I understand your point of view and I am curious what others thing.\n\nCheers,\nLars\n\n"},{"id":"300223","messageId":"CA+P7+xrYM_r+Q_27hWM2FwbreV90AqPgoiH-qPZN0hoPiHpKhQ@mail.gmail.com","threadId":"43930","inReplyTo":"xmqqvayojx9e.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6 06/13] pkt-line: add functions to read/write flush terminated packet streams","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-08-26T00:55:54Z","receivedAt":"2016-08-26T00:56:20Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Thu, Aug 25, 2016 at 3:31 PM, Junio C Hamano wrote:\n> What is wrong about that? 4*80k = 320kB overhead for length fields\n> to transfer 5GB worth of data? I do not think it is worth worrying\n> about it.\n>\n> But I am more surprised by seeing that \"why not a single huge\n> packet\" suggestion immediately after you talked about \"without the\n> possibility to intervene\". They do not seem to be remotely related;\n> in fact, they are going into opposite directions.\n>\n> Puzzled.\n\nStefan's argument to me is thus \"If we're already going to ignore\nsideband packets here, why not go all the way and make variable length\npackets and send a single packet of a maximum length? Doing thus will\nsolve some set of future problems nicely and makes this code easier.\"\n\nI'm not sure I agree myself, but that's the logic as I understand it.\n"},{"id":"300230","messageId":"6A26AA14-E393-492C-ADE0-3206DF78AFF6@gmail.com","threadId":"43930","inReplyTo":"xmqq8tvkle6o.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6 03/13] pkt-line: add packet_write_fmt_gently()","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-26T09:17:34Z","receivedAt":"2016-08-26T09:17:43Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 25 Aug 2016, at 23:41, Junio C Hamano wrote:\n> \n> larsxschneider@gmail.com writes:\n> \n>> From: Lars Schneider \n>> \n>> packet_write_fmt() would die in case of a write error even though for\n>> some callers an error would be acceptable. Add packet_write_fmt_gently()\n>> which writes a formatted pkt-line and returns `0` for success and `-1`\n>> for an error.\n>> \n>> Signed-off-by: Lars Schneider \n>> ---\n>> pkt-line.c | 12 ++++++++++++\n>> pkt-line.h | 1 +\n>> 2 files changed, 13 insertions(+)\n>> \n>> diff --git a/pkt-line.c b/pkt-line.c\n>> index e8adc0f..3e8b2fb 100644\n>> --- a/pkt-line.c\n>> +++ b/pkt-line.c\n>> @@ -137,6 +137,18 @@ void packet_write_fmt(int fd, const char *fmt, ...)\n>> \twrite_or_die(fd, buf.buf, buf.len);\n>> }\n>> \n>> +int packet_write_fmt_gently(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>> +\tva_end(args);\n>> +\treturn (write_in_full(fd, buf.buf, buf.len) == buf.len ? 0 : -1);\n>> +}\n> \n> Even though its only a handful lines, it is a bit ugly to have a\n> completely copied implementation only to have _gently(). I suspect\n> that you should be able to\n> \n> \tstatic int packet_write_fmt_1(int fd, int gently,\n> \t\t\t\t\tconst char *fmt, va_list args)\n> {\n> \t\tstruct strbuf buf = STRBUF_INIT;\n> \t\tsize_t count;\n> \n> \t\tformat_packet(&buf, fmt, args);\n> \t\t\n> \t\tcount = write_in_full(fd, buf.buf, buf.len);\n> if (count == buf.len)\n> \treturn 0;\n> \t\tif (!gently) {\n> \t\t\tcheck_pipe(errno);\n> \tdie_errno(\"write error\");\n> \t\t}\n> return -1;\n> \t}\n> \n> and then share that between the existing one:\n> \n> \tvoid packet_write_fmt(int fd, const char *fmt, ...)\n> {\n> \t\tva_list args;\n> \t va_start(args, fmt);\n> packet_write_fmt_1(fd, 0, fmt, args);\n> va_end(args);\n> \t}\n> \n> and the new one:\n> \n> \tvoid packet_write_fmt_gently(int fd, const char *fmt, ...)\n> {\n> \t\tint status;\n> \t\tva_list args;\n> \t va_start(args, fmt);\n> status = packet_write_fmt_1(fd, 1, fmt, args);\n> va_end(args);\n> \t\treturn status;\n> \t}\n\nI agree with your criticism of the code duplication. \n\nHowever, I thought it would be OK, as Peff already \ntried to refactor it...\nhttp://public-inbox.org/git/20160810150139.lpxyrqkr53s5f4sx@sigill.intra.peff.net/\n\n... and I got the impression you agreed with Peff:\nhttp://public-inbox.org/git/xmqqvaz84g9y.fsf@gitster.mtv.corp.google.com/\n\n\nI will try to refactor it according to your suggestion above. \nWould \"packet_write_fmt_1()\" be an acceptable name or should \nI come up with something more expressive?\n\nThanks you,\nLars"},{"id":"300231","messageId":"0BD3A97F-A9FD-45A6-AF82-E01CC6B9BA89@gmail.com","threadId":"43930","inReplyTo":"xmqq4m68ldrg.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6 05/13] pkt-line: add packet_write_gently()","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-26T09:40:44Z","receivedAt":"2016-08-26T09:40:54Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 25 Aug 2016, at 23:50, Junio C Hamano wrote:\n> \n> larsxschneider@gmail.com writes:\n> \n>> From: Lars Schneider \n>> \n>> packet_write_fmt() has two shortcomings. First, it uses format_packet()\n>> which lets the caller only send string data via \"%s\". That means it\n>> cannot be used for arbitrary data that may contain NULs. Second, it will\n>> always die on error.\n> \n> As you introduced _gently in 3/13, the latter is no longer a valid\n> excuse to add this function. Just remove the sentence \"Second, ...\".\n\nAgreed!\n\n\n>> Add packet_write_gently() which writes arbitrary data and returns `0`\n>> for success and `-1` for an error. This function is used by other\n>> pkt-line functions in a subsequent patch.\n>> \n>> Signed-off-by: Lars Schneider \n>> ---\n>> pkt-line.c | 12 ++++++++++++\n>> 1 file changed, 12 insertions(+)\n>> \n>> diff --git a/pkt-line.c b/pkt-line.c\n>> index cad26df..7e8a803 100644\n>> --- a/pkt-line.c\n>> +++ b/pkt-line.c\n>> @@ -3,6 +3,7 @@\n>> #include \"run-command.h\"\n>> \n>> char packet_buffer[LARGE_PACKET_MAX];\n>> +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>> @@ -155,6 +156,17 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)\n>> \treturn (write_in_full(fd, buf.buf, buf.len) == buf.len ? 0 : -1);\n>> }\n>> \n>> +int packet_write_gently(const int fd_out, const char *buf, size_t size)\n>> +{\n>> +\tif (size > sizeof(packet_write_buffer) - 4)\n>> +\t\treturn -1;\n>> +\tpacket_trace(buf, size, 1);\n>> +\tmemmove(packet_write_buffer + 4, buf, size);\n>> +\tsize += 4;\n>> +\tset_packet_header(packet_write_buffer, size);\n> \n> It may not matter all that much, but from code-reader's point of\n> view, when you know packet_write_buffer[] will contain things A and\n> B in this order, and when you have enough information to compute A\n> before stasrting to fill packet_write_buffer[], I would prefer to\n> see you actually fill the buffer in that natural order.\n\nI did that because when packet_write_stream_with_flush_from_fd()\ncalls packet_write_gently() then buf[] is actually packet_write_buffer[]:\n\nhttps://github.com/larsxschneider/git/blob/d474e6a4c2523b87624a07111eb7a4f2dcd12426/pkt-line.c#L185-L192\n\nTherefore I would override the first 4 bytes. However, the code evolved for\nsome reason in that way but looking at it now I think that is an obscure,\nlikely meaningless optimization. I'll use a separate buffer in \npacket_write_stream_with_flush_from_fd() and then fix the order here\nfollowing your advice.\n\n\n> Do you anticipate future need of non-gently variant of this\n> function? If so, perhaps a helper that takes a boolean \"am I\n> working for the gently variant?\" may help share more code.\n\nWith helper you mean \"an additional boolean parameter\"? I don't \nsee a need for a non-gently variant right now but I will\nadd this parameter if you think it is a good idea. How would the\nsignature look like?\n\nint packet_write_gently(const int fd_out, const char *buf, size_t size, int gentle)\n\nThis would follow type_from_string_gently() in object.h.\n\nThanks,\nLars"},{"id":"300232","messageId":"D8C981C6-22F5-4922-BBF7-F71961B3271B@gmail.com","threadId":"43930","inReplyTo":"xmqqzio0jxh7.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6 06/13] pkt-line: add functions to read/write flush terminated packet streams","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-26T10:13:35Z","receivedAt":"2016-08-26T10:13:45Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 26 Aug 2016, at 00:27, Junio C Hamano wrote:\n> \n> larsxschneider@gmail.com writes:\n> \n>> From: Lars Schneider \n>> \n>> packet_write_stream_with_flush_from_fd() and\n>> packet_write_stream_with_flush_from_buf() write a stream of packets. All\n>> content packets use the maximal packet size except for the last one.\n>> After the last content packet a `flush` control packet is written.\n>> packet_read_till_flush() reads arbitrary sized packets until it detects\n>> a `flush` packet.\n> \n> These are awkwardly named and I couldn't guess what the input is (I\n> can tell one is to read from fd and the other is buffer,\n> but it is unclear if that is in packetized form or just raw data\n> stream to be copied to the end from their names) without reading the\n> implementation. I _think_ you read a raw stream of data through the\n> end (either EOF or length limit) and write it out packetized, and\n> use the flush packet to mark the end of the stream. In my mind,\n> that is \"writing a packetized stream\". The words \"packetizing\" and\n> \"stream\" imply that the stream could consist of more data than what\n> would fit in a single packet, which in turn implies that there needs\n> a way to mark the end of one data item, so with_flush does not\n> necessarily have to be their names.\n> \n> The counter-part would be \"reading a packetized stream\".\n> \n>> +int packet_write_stream_with_flush_from_fd(int fd_in, int fd_out)\n>> +{\n> \n> Especially this one I am tempted to suggest \"copy-to-packetized-stream\",\n> as it reads a stream from one fd and then copies out while packetizing.\n\nOK, what function names would be more clear from your point of view?\n\ncopy_to_packetized_stream_from_fd()\ncopy_to_packetized_stream_from_buf()\ncopy_to_packetized_stream_to_buf()\n\nor\n\nwrite_packetized_stream_from_fd()\nwrite_packetized_stream_from_buf()\nread_packetized_stream_to_buf()\n\n?\n\n\n>> +int packet_write_stream_with_flush_from_buf(const char *src_in, size_t len, int fd_out)\n>> +{\n>> +\tint err = 0;\n>> +\tsize_t bytes_written = 0;\n>> +\tsize_t bytes_to_write;\n>> +\n>> +\twhile (!err) {\n>> +\t\tif ((len - bytes_written) > sizeof(packet_write_buffer) - 4)\n>> +\t\t\tbytes_to_write = sizeof(packet_write_buffer) - 4;\n>> +\t\telse\n>> +\t\t\tbytes_to_write = len - bytes_written;\n>> +\t\tif (bytes_to_write == 0)\n>> +\t\t\tbreak;\n> \n> The lack of COPY_WRITE_ERROR puzzled me briefly here. If you are\n> assuming that your math at the beginning of this loop is correct and\n> bytes_to_write will never exceed the write-buffer size, I think you\n> should be able to (and it would be better to) assume that the math\n> you do to tell xread() up to how many bytes it is allowed to read at\n> once is also correct, losing the COPY_WRITE_ERROR check in the other\n> function. You can choose to play safer and do a check in this\n> function, too. Either way, we would want to be consistent.\n\nOK. I'll remove the (I just realized meaningless) check in the other function:\n\n+\t\tif (bytes_to_write > sizeof(packet_write_buffer) - 4)\n+\t\t\treturn COPY_WRITE_ERROR;\n\n> \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>> +ssize_t packet_read_till_flush(int fd_in, struct strbuf *sb_out)\n>> +{\n>> +\tint len, ret;\n>> +\tint options = PACKET_READ_GENTLE_ON_EOF;\n>> +\tchar linelen[4];\n>> +\n>> +\tsize_t oldlen = sb_out->len;\n>> +\tsize_t oldalloc = sb_out->alloc;\n>> +\n>> +\tfor (;;) {\n>> +\t\t/* Read packet header */\n>> +\t\tret = get_packet_data(fd_in, NULL, NULL, linelen, 4, options);\n>> +\t\tif (ret < 0)\n>> +\t\t\tgoto done;\n>> +\t\tlen = packet_length(linelen);\n>> +\t\tif (len < 0)\n>> +\t\t\tdie(\"protocol error: bad line length character: %.4s\", linelen);\n>> +\t\tif (!len) {\n>> +\t\t\t/* Found a flush packet - Done! */\n>> +\t\t\tpacket_trace(\"0000\", 4, 0);\n>> +\t\t\tbreak;\n>> +\t\t}\n>> +\t\tlen -= 4;\n>> +\n>> +\t\t/* Read packet content */\n>> +\t\tstrbuf_grow(sb_out, len);\n>> +\t\tret = get_packet_data(fd_in, NULL, NULL, sb_out->buf + sb_out->len, len, options);\n>> +\t\tif (ret < 0)\n>> +\t\t\tgoto done;\n>> +\t\tif (ret != len) {\n>> +\t\t\terror(\"protocol error: incomplete read (expected %d, got %d)\", len, ret);\n>> +\t\t\tgoto done;\n>> +\t\t}\n>> +\n>> +\t\tpacket_trace(sb_out->buf + sb_out->len, len, 0);\n> \n> All of the above seems to pretty much duplicate the logic in\n> packet_read(), except that this user does not need options handling\n> it has. Is optimizing that out the reason why you open-coded it\n> here?\n\nNo.\n\n> Or is it because you cannot tell if you got a truly empty packet or\n> you got a flush from outside packet_read(), and you wanted to make\n> sure that you won't be fooled by a normal packet with 0-length\n> payload?\n\nCorrect!\n\n> \n> If the latter is the reason, it may be a viable alternative to\n> update packet_read() to take PACKET_READ_IGNORE_EMPTY_PACKET, i.e. a\n> new bit in its options parameter, so that a normal packet with\n> 0-length payload is simply ignored there (i.e. even without\n> returning, packet_read() would repeat from the beginning when it got\n> such a packet). That way, the above would become \n> \n> \tstrbuf_grow(); /* enough to hold max-packet-len more bytes */\n> \tlen = packet_read();\n> if (!len)\n> \t/* we cannot get 0 unless we see flush */\n> break;\n> \n> which may be a lot cleaner?\n\nGood idea! I will refactor it that way!\n\n\nThanks a lot for the review,\nLars\n\n"},{"id":"300265","messageId":"CAGZ79kYD7KSzdhe-60fs5iUKmYcMd6+V3fSN513+sr15q+g39Q@mail.gmail.com","threadId":"43930","inReplyTo":"CA+P7+xrYM_r+Q_27hWM2FwbreV90AqPgoiH-qPZN0hoPiHpKhQ@mail.gmail.com","subject":"Re: [PATCH v6 06/13] pkt-line: add functions to read/write flush terminated packet streams","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-26T17:02:52Z","receivedAt":"2016-08-26T17:02:58Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Aug 25, 2016 at 5:55 PM, Jacob Keller wrote:\n> On Thu, Aug 25, 2016 at 3:31 PM, Junio C Hamano wrote:\n>> What is wrong about that? 4*80k = 320kB overhead for length fields\n>> to transfer 5GB worth of data? I do not think it is worth worrying\n>> about it.\n>>\n>> But I am more surprised by seeing that \"why not a single huge\n>> packet\" suggestion immediately after you talked about \"without the\n>> possibility to intervene\". They do not seem to be remotely related;\n>> in fact, they are going into opposite directions.\n>>\n>> Puzzled.\n>\n> Stefan's argument to me is thus \"If we're already going to ignore\n> sideband packets here, why not go all the way and make variable length\n> packets and send a single packet of a maximum length? Doing thus will\n> solve some set of future problems nicely and makes this code easier.\"\n>\n> I'm not sure I agree myself, but that's the logic as I understand it.\n\nYeah. To me it seems this design explicitly makes it hard for side bands.\nAs we do not need sidebands for local transfers, this is fine for sure.\n\n(If we wanted to make it sideband friendly, I'd expect you could register\ncallbacks for either all packets or for a given sideband until the next\nflush comes.)\n\nSo as hinted by this design, we want a protocol that\n* doesn't care about sidebands\n* cares about large data (hence maybe throughput)\n* has easy/clean interface\n\nAnd one large packet would suffice for these three points as well\nand additionally has benefits for the network stuff.\n\nThe 320kB additional transmission are negligible overhead, so I was not\nconcerned about the size, but rather the code being bloated, i.e. we need\none layer of additional code to cope with the repetitive packets.\n\n---\nMy background is mostly submodule related, and whenever I come up\nwith a shiny novel idea that would help submodules tremendously, someone\n(mostly Peff) comes along and suggests a broader more generic thing, that\nworks just as well for submodules but is applicable to all of Git.\n\nSo I picked up that way of thinking: If we write code here, that helps with a\nvery special niche of things, can we make so, that the rest also benefits?\n\nI may not understand all the requirements in this case, but to me it looks like\nthe \"one packet\" approach covers all requirements, but has the huge potential\nto make other parts better in the long run.\n-- \n\nThanks,\nStefan\n"},{"id":"300268","messageId":"xmqqinunihgl.fsf@gitster.mtv.corp.google.com","threadId":"43930","inReplyTo":"6A26AA14-E393-492C-ADE0-3206DF78AFF6@gmail.com","subject":"Re: [PATCH v6 03/13] pkt-line: add packet_write_fmt_gently()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-26T17:10:50Z","receivedAt":"2016-08-26T17:10:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n> I agree with your criticism of the code duplication. \n>\n> However, I thought it would be OK, as Peff already \n> tried to refactor it...\n> http://public-inbox.org/git/20160810150139.lpxyrqkr53s5f4sx@sigill.intra.peff.net/\n>\n> ... and I got the impression you agreed with Peff:\n> http://public-inbox.org/git/xmqqvaz84g9y.fsf@gitster.mtv.corp.google.com/\n\nThe former does not exactly show how ugly it was, but I do not have\nto see it. It is talking about eliminating the need for memcpy()\nand duplicated header generation code, which the suggestion you are\nresponding to didn't even attempt. If Peff said he tried an even\nmore aggressive refactoring and it ended up too ugly to live, I\nbelieve him and agree with his assessment.\n\n> I will try to refactor it according to your suggestion above. \n> Would \"packet_write_fmt_1()\" be an acceptable name or should \n> I come up with something more expressive?\n\nThe latter is preferrable, but we do not mind too strongly about\nthe name of file-scope static helper that will never be called\ndirectly by anybody other than the two more public entry points the\nhelper was designed to serve.\n"},{"id":"300269","messageId":"xmqqeg5bih8h.fsf@gitster.mtv.corp.google.com","threadId":"43930","inReplyTo":"0BD3A97F-A9FD-45A6-AF82-E01CC6B9BA89@gmail.com","subject":"Re: [PATCH v6 05/13] pkt-line: add packet_write_gently()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-26T17:15:42Z","receivedAt":"2016-08-26T17:19:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n>> Do you anticipate future need of non-gently variant of this\n>> function? If so, perhaps a helper that takes a boolean \"am I\n>> working for the gently variant?\" may help share more code.\n>\n> With helper you mean \"an additional boolean parameter\"? I don't \n> see a need for a non-gently variant right now but I will\n> add this parameter if you think it is a good idea. How would the\n> signature look like?\n>\n> int packet_write_gently(const int fd_out, const char *buf, size_t size, int gentle)\n>\n> This would follow type_from_string_gently() in object.h.\n\nI actually imagined it would be more like your packet_write_fmt vs\npacket_write_fmt_gently pair of functions. If you do not have an\nimmediate need for a non-gentle packet_write() right now, but you\nstill forsee that it is likely some other caller may want one, you\ncould still prepare for it by doing a static\n\n\tpacket_write_1((const int fd_out, const char *buf, size_t size, int gentle)\n\nand make packet_write_gently() call it with gentle=1, without\nactually introducing packet_write() nobody yet calls.\n\n\n"},{"id":"300270","messageId":"xmqqa8fzih6b.fsf@gitster.mtv.corp.google.com","threadId":"43930","inReplyTo":"CA+P7+xrYM_r+Q_27hWM2FwbreV90AqPgoiH-qPZN0hoPiHpKhQ@mail.gmail.com","subject":"Re: [PATCH v6 06/13] pkt-line: add functions to read/write flush terminated packet streams","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-26T17:17:00Z","receivedAt":"2016-08-26T17:20:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> Stefan's argument to me is thus \"If we're already going to ignore\n> sideband packets here, why not go all the way and make variable length\n> packets and send a single packet of a maximum length? Doing thus will\n> solve some set of future problems nicely and makes this code easier.\"\n>\n> I'm not sure I agree myself, but that's the logic as I understand it.\n\nI do not know if I agree, but now I understand the flow of logic.\n\nThanks.\n"},{"id":"300271","messageId":"xmqq60qnigyz.fsf@gitster.mtv.corp.google.com","threadId":"43930","inReplyTo":"D8C981C6-22F5-4922-BBF7-F71961B3271B@gmail.com","subject":"Re: [PATCH v6 06/13] pkt-line: add functions to read/write flush terminated packet streams","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-26T17:21:24Z","receivedAt":"2016-08-26T17:21:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n> OK, what function names would be more clear from your point of view?\n>\n> write_packetized_stream_from_fd()\n> write_packetized_stream_from_buf()\n> read_packetized_stream_to_buf()\n\nWould\n\n write_packetized_from_fd()\n write_packetized_from_buf()\n read_packetized_to_buf()\n\nbe shorter, still understandable, be consistent with the existing\nconvention to name write_/read_ a function that shuffles bytes via a\nfile descriptor, and to the point, perhaps?\n\n"},{"id":"300272","messageId":"20160826172130.v2zh7dbazg6ac2zj@sigill.intra.peff.net","threadId":"43930","inReplyTo":"CAGZ79kYD7KSzdhe-60fs5iUKmYcMd6+V3fSN513+sr15q+g39Q@mail.gmail.com","subject":"Re: [PATCH v6 06/13] pkt-line: add functions to read/write flush terminated packet streams","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-26T17:21:31Z","receivedAt":"2016-08-26T17:21:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 26, 2016 at 10:02:52AM -0700, Stefan Beller wrote:\n\n> Yeah. To me it seems this design explicitly makes it hard for side bands.\n> As we do not need sidebands for local transfers, this is fine for sure.\n> \n> (If we wanted to make it sideband friendly, I'd expect you could register\n> callbacks for either all packets or for a given sideband until the next\n> flush comes.)\n> \n> So as hinted by this design, we want a protocol that\n> * doesn't care about sidebands\n> * cares about large data (hence maybe throughput)\n> * has easy/clean interface\n> \n> And one large packet would suffice for these three points as well\n> and additionally has benefits for the network stuff.\n> \n> The 320kB additional transmission are negligible overhead, so I was not\n> concerned about the size, but rather the code being bloated, i.e. we need\n> one layer of additional code to cope with the repetitive packets.\n\nMaybe I don't understand what you mean by \"one large packet\". But if you\nmean sending \"I am about to send you 100MB\" followed by the 100MB, the\npoint is that the sender does not necessarily have that exact value\nahead of time. So it would want to write it in chunks.\n\nE.g., consider a clean which replaces s/foo/bar/ in its content. It\nwould write out all the content up to the next \"foo\", then write \"bar\",\nand repeat.\n\nWe could let each chunk be arbitrarily big. I.e., \"I am about to send\nyou 50MB\", then \"here are 3 bytes\", then \"here are the other 50MB\".\n\nBut using a fixed-size length header makes the packets easy to generate\nand parse. And 64KB is small enough that senders and receivers can\neasily buffer single packets, making interfaces simpler. And it's big\nenough that the overhead (4 bytes per 64KB) is negligible.\n\nAnyway. It certainly does not seem worth moving the network protocols to\na new data format. The compatibility changes would not be worth it. But\nif we _were_ to do so, there are tons of efficient well-thought-out\ndata-marshaling formats we could use rather than inventing our own. But\nI just don't see the benefit.\n\n> ---\n> My background is mostly submodule related, and whenever I come up\n> with a shiny novel idea that would help submodules tremendously, someone\n> (mostly Peff) comes along and suggests a broader more generic thing, that\n> works just as well for submodules but is applicable to all of Git.\n\nHeh. It is not always a good thing, if it derails the original purpose\nof the code (and I include some of my suggestions in that). :)\n\nThe trick is to make it general enough, without getting lost in the\nweeds of what _might_ happen in the future and wasting time (and\nadding complexity).\n\n-Peff\n"},{"id":"300273","messageId":"20160826172323.t2b56zrwbl7jw5p6@sigill.intra.peff.net","threadId":"43930","inReplyTo":"xmqqinunihgl.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6 03/13] pkt-line: add packet_write_fmt_gently()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-26T17:23:23Z","receivedAt":"2016-08-26T17:23:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 26, 2016 at 10:10:50AM -0700, Junio C Hamano wrote:\n\n> Lars Schneider writes:\n> \n> > I agree with your criticism of the code duplication. \n> >\n> > However, I thought it would be OK, as Peff already \n> > tried to refactor it...\n> > http://public-inbox.org/git/20160810150139.lpxyrqkr53s5f4sx@sigill.intra.peff.net/\n> >\n> > ... and I got the impression you agreed with Peff:\n> > http://public-inbox.org/git/xmqqvaz84g9y.fsf@gitster.mtv.corp.google.com/\n> \n> The former does not exactly show how ugly it was, but I do not have\n> to see it. It is talking about eliminating the need for memcpy()\n> and duplicated header generation code, which the suggestion you are\n> responding to didn't even attempt. If Peff said he tried an even\n> more aggressive refactoring and it ended up too ugly to live, I\n> believe him and agree with his assessment.\n\nRight, what I found difficulty with was factoring out format_packet(). I\nthink the \"gently\" part is easy.\n\n-Peff\n"},{"id":"300290","messageId":"xmqqshtrfh6s.fsf@gitster.mtv.corp.google.com","threadId":"43930","inReplyTo":"6EC60D1B-3A92-4381-A87E-50417CF04A49@gmail.com","subject":"Re: [PATCH v6 07/13] pack-protocol: fix maximum pkt-line size","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-26T19:44:59Z","receivedAt":"2016-08-26T19:45:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n>> And as this is a strict bug fix of Documentation that makes sense\n>> outside this series,\n>\n> Agreed!\n\nAmen.\n"},{"id":"300291","messageId":"xmqqoa4ffh69.fsf@gitster.mtv.corp.google.com","threadId":"43930","inReplyTo":"20160825110752.31581-9-larsxschneider@gmail.com","subject":"Re: [PATCH v6 08/13] convert: quote filter names in error messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-26T19:45:18Z","receivedAt":"2016-08-26T19:45:27Z","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> Git filter driver commands with spaces (e.g. `filter.sh foo`) are hard to\n> read in error messages. Quote them to improve the readability.\n\nSounds good. Thanks.\n"},{"id":"300296","messageId":"xmqqk2f3fgbd.fsf@gitster.mtv.corp.google.com","threadId":"43930","inReplyTo":"20160825110752.31581-10-larsxschneider@gmail.com","subject":"Re: [PATCH v6 09/13] convert: modernize tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-26T20:03:50Z","receivedAt":"2016-08-26T20:10:21Z","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> 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\nAll of the above are good things to do, but the first one needs to\nbe done a bit carefully.\n\nIt is unclear in the above description if you made sure that no\nsubsequent test depends on the settings left by earlier test before\nreplacing \"git config\" with \"test_config\".\n\n> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\n> index 7bac2bc..7b45136 100755\n> --- a/t/t0021-conversion.sh\n> +++ b/t/t0021-conversion.sh\n> @@ -13,8 +13,8 @@ EOF\n> chmod +x rot13.sh\n> \n> test_expect_success setup '\n> -\tgit config filter.rot13.smudge ./rot13.sh &&\n> -\tgit config filter.rot13.clean ./rot13.sh &&\n> +\ttest_config filter.rot13.smudge ./rot13.sh &&\n> +\ttest_config filter.rot13.clean ./rot13.sh &&\n> \n> \t{\n> \t echo \"*.t filter=rot13\"\n\nFor example, after this conversion, filter.rot13.* will be reverted\nback to unconfigured once setup is done.\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\nIt happens to be true that this subsequent test does not do any\noperation to cause data come from and go into the object database\nfor any path that match the pattern \"*.t\", because for whatever\nreason the previous \"setup\" step happens to do more than just\n\"setup\". It already exercised the filtering by doing \"git add\" and\n\"git checkout\".\n\nIf we were writing the script t0021 from scratch today, we would\nhave used test_config *AND* squashed there two tests into one\n(i.e. making it a single \"the filter and ident operation\" test).\nThen it is crystal clear that additional tests on commands that may\ninvolve filtering will always be added to that combined test\n(e.g. we may try \"cat-file --filters\" to ensure that rot13 filter is\nexcercised).\n\nBut because we are not doing that, it may become tempting to add\ntest for a new command that pays attention to a filter to either of\nthe test, and it would have worked OK if this patch weren't there.\nSuch an addition will break the test, because in the second \"check\"\ntest, the filter commands are no longer active with this patch.\n\nSo while I do appreciate the change for the longer term, I am not\nquite happy with it. It just looks like an invitation for future\nmistakes.\n\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\nThis one is clearly OK. Anything related to argc filter only\nappears in this single test so it is not just OK to use test_config,\nbut it makes our intention very clear that nobody else would use the\nargc filter. I think similar reasoning would apply to the test_config\nconversion in the remainder of the script.\n\nAs to other types of changes you did in this, I see no problem with\nthem.\n\nThanks.\n"},{"id":"300437","messageId":"40503C79-2041-4928-B9A1-04D720D95492@gmail.com","threadId":"43930","inReplyTo":"xmqqeg5bih8h.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6 05/13] pkt-line: add packet_write_gently()","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-29T09:40:42Z","receivedAt":"2016-08-29T09:40:51Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 26 Aug 2016, at 19:15, Junio C Hamano wrote:\n> \n> Lars Schneider writes:\n> \n>>> Do you anticipate future need of non-gently variant of this\n>>> function? If so, perhaps a helper that takes a boolean \"am I\n>>> working for the gently variant?\" may help share more code.\n>> \n>> With helper you mean \"an additional boolean parameter\"? I don't \n>> see a need for a non-gently variant right now but I will\n>> add this parameter if you think it is a good idea. How would the\n>> signature look like?\n>> \n>> int packet_write_gently(const int fd_out, const char *buf, size_t size, int gentle)\n>> \n>> This would follow type_from_string_gently() in object.h.\n> \n> I actually imagined it would be more like your packet_write_fmt vs\n> packet_write_fmt_gently pair of functions. If you do not have an\n> immediate need for a non-gentle packet_write() right now, but you\n> still forsee that it is likely some other caller may want one, you\n> could still prepare for it by doing a static\n> \n> \tpacket_write_1((const int fd_out, const char *buf, size_t size, int gentle)\n> \n> and make packet_write_gently() call it with gentle=1, without\n> actually introducing packet_write() nobody yet calls.\n\nI see. In that case I would like to keep packet_write_gently() as is\nbecause I don't see the need for a non-gently variant right now.\n\nIf there is a need for packet_write() then we could just add it and\nmove the packet_write_gently() code to packet_write_1() following your\nsuggestion. No caller would need to change for this refactoring.\n\nIf you strongly disagree then I would use the \"two function\" approach\nyou suggested above right away, though.\n\nThanks,\nLars"},{"id":"300438","messageId":"64722B90-8C48-409A-8FAC-75DCD4CBACCD@gmail.com","threadId":"43930","inReplyTo":"xmqq60qnigyz.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6 06/13] pkt-line: add functions to read/write flush terminated packet streams","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-29T09:43:12Z","receivedAt":"2016-08-29T09:43:19Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 26 Aug 2016, at 19:21, Junio C Hamano wrote:\n> \n> Lars Schneider writes:\n> \n>> OK, what function names would be more clear from your point of view?\n>> \n>> write_packetized_stream_from_fd()\n>> write_packetized_stream_from_buf()\n>> read_packetized_stream_to_buf()\n> \n> Would\n> \n> write_packetized_from_fd()\n> write_packetized_from_buf()\n> read_packetized_to_buf()\n> \n> be shorter, still understandable, be consistent with the existing\n> convention to name write_/read_ a function that shuffles bytes via a\n> file descriptor, and to the point, perhaps?\n\nAgreed.\n\nThanks,\nLars\n"},{"id":"300443","messageId":"70802AA1-8455-49FA-8457-7E590289FFCC@gmail.com","threadId":"43930","inReplyTo":"xmqqk2f3fgbd.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6 09/13] convert: modernize tests","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-29T10:09:17Z","receivedAt":"2016-08-29T10:09:26Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 26 Aug 2016, at 22:03, Junio C Hamano wrote:\n> \n> larsxschneider@gmail.com writes:\n> \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> All of the above are good things to do, but the first one needs to\n> be done a bit carefully.\n> \n> It is unclear in the above description if you made sure that no\n> subsequent test depends on the settings left by earlier test before\n> replacing \"git config\" with \"test_config\".\n\nI assumed that I would see test failures if a subsequent test depends\non the settings left by earlier tests. I'll add this comment to the\ncommit message.\n\n\n>> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\n>> index 7bac2bc..7b45136 100755\n>> --- a/t/t0021-conversion.sh\n>> +++ b/t/t0021-conversion.sh\n>> @@ -13,8 +13,8 @@ EOF\n>> chmod +x rot13.sh\n>> \n>> test_expect_success setup '\n>> -\tgit config filter.rot13.smudge ./rot13.sh &&\n>> -\tgit config filter.rot13.clean ./rot13.sh &&\n>> +\ttest_config filter.rot13.smudge ./rot13.sh &&\n>> +\ttest_config filter.rot13.clean ./rot13.sh &&\n>> \n>> \t{\n>> \t echo \"*.t filter=rot13\"\n> \n> For example, after this conversion, filter.rot13.* will be reverted\n> back to unconfigured once setup is done.\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> \n> It happens to be true that this subsequent test does not do any\n> operation to cause data come from and go into the object database\n> for any path that match the pattern \"*.t\", because for whatever\n> reason the previous \"setup\" step happens to do more than just\n> \"setup\". It already exercised the filtering by doing \"git add\" and\n> \"git checkout\".\n> \n> If we were writing the script t0021 from scratch today, we would\n> have used test_config *AND* squashed there two tests into one\n> (i.e. making it a single \"the filter and ident operation\" test).\n> Then it is crystal clear that additional tests on commands that may\n> involve filtering will always be added to that combined test\n> (e.g. we may try \"cat-file --filters\" to ensure that rot13 filter is\n> excercised).\n> \n> But because we are not doing that, it may become tempting to add\n> test for a new command that pays attention to a filter to either of\n> the test, and it would have worked OK if this patch weren't there.\n> Such an addition will break the test, because in the second \"check\"\n> test, the filter commands are no longer active with this patch.\n> \n> So while I do appreciate the change for the longer term, I am not\n> quite happy with it. It just looks like an invitation for future\n> mistakes.\n\nI'll follow your judgement here. However, from my point of view a future \naddition that causes test failures is no issue. Because these test failures\nwould be analyzed and then the tests could be refactored accordingly.\n\n\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> This one is clearly OK. Anything related to argc filter only\n> appears in this single test so it is not just OK to use test_config,\n> but it makes our intention very clear that nobody else would use the\n> argc filter. I think similar reasoning would apply to the test_config\n> conversion in the remainder of the script.\n\nOK, then I will keep these test_config's and revert the first one.\n\n\n> As to other types of changes you did in this, I see no problem with\n> them.\n\n\nThanks,\nLars\n"},{"id":"300462","messageId":"D7C1480E-8FF4-4EA5-BA77-7A4672853614@gmail.com","threadId":"43930","inReplyTo":"20160825110752.31581-1-larsxschneider@gmail.com","subject":"Re: [PATCH v6 00/13] Git filter protocol","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-29T15:39:31Z","receivedAt":"2016-08-29T15:39:47Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 25 Aug 2016, at 13:07, larsxschneider@gmail.com wrote:\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> \n> A short summary about v1 to v5 can be found here:\n> https://git.github.io/rev_news/2016/08/17/edition-18/\n> \n> This series is also published on web:\n> https://github.com/larsxschneider/git/pull/10\n\nHi Junio,\n\nI am working on v7 of this patch series and I noticed that is does\nnot merge clean anymore (not even with git/git master). Should\nI rebase my patch series? If yes, what would be the most\nconvenient base for you?\n\nThanks,\nLars\n\n"},{"id":"300473","messageId":"xmqq7fazcvsk.fsf@gitster.mtv.corp.google.com","threadId":"43930","inReplyTo":"20160825110752.31581-11-larsxschneider@gmail.com","subject":"Re: [PATCH v6 10/13] convert: generate large test files only once","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-29T17:46:51Z","receivedAt":"2016-08-29T17:46:59Z","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/t/t0021-conversion.sh b/t/t0021-conversion.sh\n> index 7b45136..34c8eb9 100755\n> --- a/t/t0021-conversion.sh\n> +++ b/t/t0021-conversion.sh\n> @@ -4,6 +4,15 @@ test_description='blob conversion via gitattributes'\n> \n> . ./test-lib.sh\n> \n> +if test_have_prereq EXPENSIVE\n> +then\n> +\tT0021_LARGE_FILE_SIZE=2048\n> +\tT0021_LARGISH_FILE_SIZE=100\n> +else\n> +\tT0021_LARGE_FILE_SIZE=30\n> +\tT0021_LARGISH_FILE_SIZE=2\n> +fi\n\nMinor: do we need T0021_ prefix? What are you trying to avoid\ncollisions with?\n\n> +\tgit checkout -- test test.t test.i &&\n> +\n> +\tmkdir generated-test-data &&\n> +\tfor i in $(test_seq 1 $T0021_LARGE_FILE_SIZE)\n> +\tdo\n> +\t\tRANDOM_STRING=\"$(test-genrandom end $i | tr -dc \"A-Za-z0-9\" )\"\n> +\t\tROT_RANDOM_STRING=\"$(echo $RANDOM_STRING | ./rot13.sh )\"\n\nIn earlier iteration of loop with lower $i, what guarantees that\nsome bytes survive \"tr -dc\"?\n\n> +\t\t# Generate 1MB of empty data and 100 bytes of random characters\n\n100 bytes? It seems to me that you are giving 1MB and then $i-byte\nor less (which sometimes can be zero) of random string.\n\n> +\t\t# printf \"$(test-genrandom start $i)\"\n> +\t\tprintf \"%1048576d\" 1 >>generated-test-data/large.file &&\n> +\t\tprintf \"$RANDOM_STRING\" >>generated-test-data/large.file &&\n> +\t\tprintf \"%1048576d\" 1 >>generated-test-data/large.file.rot13 &&\n> +\t\tprintf \"$ROT_RANDOM_STRING\" >>generated-test-data/large.file.rot13 &&\n> +\n> +\t\tif test $i = $T0021_LARGISH_FILE_SIZE\n> +\t\tthen\n> +\t\t\tcat generated-test-data/large.file >generated-test-data/largish.file &&\n> +\t\t\tcat generated-test-data/large.file.rot13 >generated-test-data/largish.file.rot13\n> +\t\tfi\n> +\tdone\n\nThis \"now we are done with the loop, so copy them to the second\npair\" needs to be in the loop? Shouldn't it come after 'done'?\n\nI do not quite get the point of this complexity. You are using\nexactly the same seed \"end\" every time, so in the first round you\nhave 1M of SP, letter '1', letter 'S' (from the genrandom), then\nin the second round you have 1M of SP, letter '1', letter 'S' and\nletter 'p' (the last two from the genrandom), and go on. Is it\nsignificant for the purpose of your test that the cruft inserted\nbetween the repetition of 1M of SP gets longer by one byte but they\nall share the same prefix (e.g. \"1S\", \"1Sp\", \"1SpZ\", \"1SpZT\",\n... are what you insert between a large run of spaces)?\n"},{"id":"300474","messageId":"xmqq37lncvj6.fsf@gitster.mtv.corp.google.com","threadId":"43930","inReplyTo":"E574E25A-F87F-416C-88E5-E3CE1F6B54D0@gmail.com","subject":"Re: [PATCH v6 10/13] convert: generate large test files only once","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-29T17:52:29Z","receivedAt":"2016-08-29T17:52:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n>> On 25 Aug 2016, at 21:17, Stefan Beller wrote:\n>> \n>>> On Thu, Aug 25, 2016 at 4:07 AM, wrote:\n>>> From: Lars Schneider \n>>> \n>>> Generate more interesting large test files\n>> \n>> How are the large test files more interesting?\n>> (interesting in the notion of covering more potential bugs?\n>> easier to debug? better to maintain, or just a pleasant read?)\n>\n> The old large test file was 1MB of zeros and 1 byte with a one, repeated 2048 times.\n>\n> Since the filter uses 64k packets we would test a large number of equally looking packets.\n>\n> That's why I thought the pseudo random content is more interesting.\n\nI guess my real question is why it is not just a single invocation\nof test-genrandom that gives you the whole test file; if you are\nusing 20MB, the simplest would be to grab 20MB out of test-genrandom.\nWith that hopefully you won't see large number of equally looking\npackets, no?\n"},{"id":"300476","messageId":"xmqqy43fbgcj.fsf@gitster.mtv.corp.google.com","threadId":"43930","inReplyTo":"20160825110752.31581-14-larsxschneider@gmail.com","subject":"Re: [PATCH v6 13/13] read-cache: make sure file handles are not inherited by child processes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-29T18:05:48Z","receivedAt":"2016-08-29T18:05:56Z","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> Consider the case of a file that requires filtering and is present in\n> branch A but not in branch B. If A is the current HEAD and we checkout B\n> then the following happens:\n>\n> 1. ce_compare_data() opens the file\n> 2. index_fd() detects that the file requires to run a clean filter and\n> calls index_stream_convert_blob()\n> 4. index_stream_convert_blob() calls convert_to_git_filter_fd()\n> 5. convert_to_git_filter_fd() calls apply_filter() which creates a\n> new long running filter process (in case it is the first file\n> of this kind to be filtered)\n> 6. The new filter process inherits all file handles. This is the\n> default on Linux/OSX and is explicitly defined in the\n> `CreateProcessW` call in `mingw.c` on Windows.\n> 7. ce_compare_data() closes the file\n> 8. Git unlinks the file as it is not present in B\n>\n> The unlink operation does not work on Windows because the filter process\n> has still an open handle to the file. Apparently that is no problem on\n> Linux/OSX. Probably because \"[...] the two file descriptors share open\n> file status flags\" (see fork(2)).\n\nWait, a, minute. \"that is no problem\" may be true as long as \"that\"\nis \"unlinking the now-gone file in the filesystem\", but the reason\ndoes not have anything to do with the \"open-file status flags\";\nunlike Windows, you _can_ unlink file that has an open file\ndescriptor on it.\n\nAnd even on POSIX systems, if you are doing a long-running helper\nany open file descriptor in the parent process when the long-running\nhelper is spawned will become leaked fd. CLOEXEC is a possible\nsolution (but not necessarily the only or the best one) to the fd\nleak in this case.\n\nHow much does the code that spawns these long-running helpers know\nabout the file descriptors that happen to be open? The parent is\nvery likely to have pack windows open into .pack files and they need\nto be closed on the child side after fork(2) starts the child\nprocess but before execve(2) runs the helper, if we want to avoid\nfile descriptor leaks.\n\n> Fix this problem by opening files in read-cache with the `O_CLOEXEC`\n> flag to ensure that the file descriptor does not remain open in a newly\n> spawned process. `O_CLOEXEC` is defined as `O_NOINHERIT` on Windows. A\n> similar fix for temporary file handles was applied on Git for Windows\n> already:\n> https://github.com/git-for-windows/git/commit/667b8b51ec850c3e1c7d75dee69dc13c29d1f162\n\nAfter a few iterations, the final version of the same commit is now\nin our tree as d5cb9cbd (\"Git 2.10-rc2\", 2016-08-26).\n"},{"id":"300477","messageId":"xmqqtwe3bg6j.fsf@gitster.mtv.corp.google.com","threadId":"43930","inReplyTo":"D7C1480E-8FF4-4EA5-BA77-7A4672853614@gmail.com","subject":"Re: [PATCH v6 00/13] Git filter protocol","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-29T18:09:24Z","receivedAt":"2016-08-29T18:09:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n>> On 25 Aug 2016, at 13:07, larsxschneider@gmail.com wrote:\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>> \n>> A short summary about v1 to v5 can be found here:\n>> https://git.github.io/rev_news/2016/08/17/edition-18/\n>> \n>> This series is also published on web:\n>> https://github.com/larsxschneider/git/pull/10\n>\n> Hi Junio,\n>\n> I am working on v7 of this patch series and I noticed that is does\n> not merge clean anymore (not even with git/git master). Should\n> I rebase my patch series? If yes, what would be the most\n> convenient base for you?\n\nDepends on what you are conflicting with. I think you want to use\nsomething that has at least 05d1ed61 (\"mingw: ensure temporary file\nhandles are not inherited by child processes\", 2016-08-22). Perhaps\n2.10 final?\n"},{"id":"300482","messageId":"4D9E5AED-7003-4707-8791-1C25432DB558@gmail.com","threadId":"43930","inReplyTo":"xmqqy43fbgcj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6 13/13] read-cache: make sure file handles are not inherited by child processes","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-29T19:03:09Z","receivedAt":"2016-08-29T19:03:18Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 29 Aug 2016, at 20:05, Junio C Hamano wrote:\n> \n> larsxschneider@gmail.com writes:\n> \n>> From: Lars Schneider \n>> \n>> Consider the case of a file that requires filtering and is present in\n>> branch A but not in branch B. If A is the current HEAD and we checkout B\n>> then the following happens:\n>> \n>> 1. ce_compare_data() opens the file\n>> 2. index_fd() detects that the file requires to run a clean filter and\n>> calls index_stream_convert_blob()\n>> 4. index_stream_convert_blob() calls convert_to_git_filter_fd()\n>> 5. convert_to_git_filter_fd() calls apply_filter() which creates a\n>> new long running filter process (in case it is the first file\n>> of this kind to be filtered)\n>> 6. The new filter process inherits all file handles. This is the\n>> default on Linux/OSX and is explicitly defined in the\n>> `CreateProcessW` call in `mingw.c` on Windows.\n>> 7. ce_compare_data() closes the file\n>> 8. Git unlinks the file as it is not present in B\n>> \n>> The unlink operation does not work on Windows because the filter process\n>> has still an open handle to the file. Apparently that is no problem on\n>> Linux/OSX. Probably because \"[...] the two file descriptors share open\n>> file status flags\" (see fork(2)).\n> \n> Wait, a, minute. \"that is no problem\" may be true as long as \"that\"\n> is \"unlinking the now-gone file in the filesystem\", but the reason\n> does not have anything to do with the \"open-file status flags\";\n> unlike Windows, you _can_ unlink file that has an open file\n> descriptor on it.\n\nI see. Thanks for the explanation.\n\n> \n> And even on POSIX systems, if you are doing a long-running helper\n> any open file descriptor in the parent process when the long-running\n> helper is spawned will become leaked fd. CLOEXEC is a possible\n> solution (but not necessarily the only or the best one) to the fd\n> leak in this case.\n> \n> How much does the code that spawns these long-running helpers know\n> about the file descriptors that happen to be open?\n\nNothing really.\n\n> The parent is\n> very likely to have pack windows open into .pack files and they need\n> to be closed on the child side after fork(2) starts the child\n> process but before execve(2) runs the helper, if we want to avoid\n> file descriptor leaks.\n\nI think I understand what you are saying. However, during my tests\n.pack file fd's were never a problem. I use start_command() [1]\nwhich wraps the fork() and exec calls [2].\n\nHow would I find the open .pack file fd's? Should I go through \n/proc/PID/fd? Why is this no problem for other longer running \ncommands such as the git-credential-cache--daemon or git-daemon?\n\nThanks,\nLars\n\n\n[1] https://github.com/larsxschneider/git/blob/protocol-filter/v6/convert.c#L566\n[2] https://github.com/larsxschneider/git/blob/protocol-filter/v6/run-command.c#L345-L412\n\n\n"},{"id":"300486","messageId":"xmqq37lnbbpk.fsf@gitster.mtv.corp.google.com","threadId":"43930","inReplyTo":"4D9E5AED-7003-4707-8791-1C25432DB558@gmail.com","subject":"Re: [PATCH v6 13/13] read-cache: make sure file handles are not inherited by child processes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-29T19:45:59Z","receivedAt":"2016-08-29T19:46:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n> I see. Thanks for the explanation.\n\nI expect the updated log message to explain the issue correctly\nthen.\n\n>> And even on POSIX systems, if you are doing a long-running helper\n>> any open file descriptor in the parent process when the long-running\n>> helper is spawned will become leaked fd. CLOEXEC is a possible\n>> solution (but not necessarily the only or the best one) to the fd\n>> leak in this case.\n>> \n>> How much does the code that spawns these long-running helpers know\n>> about the file descriptors that happen to be open?\n>\n> Nothing really.\n\nOK.\n\n>> The parent is\n>> very likely to have pack windows open into .pack files and they need\n>> to be closed on the child side after fork(2) starts the child\n>> process but before execve(2) runs the helper, if we want to avoid\n>> file descriptor leaks.\n>\n> I think I understand what you are saying. However, during my tests\n> .pack file fd's were never a problem.\n\nI do not expect during the lifetime of your long-running helper\nanybody would try to unlink an existing packfile, so it is unlikely\nthat \"cannot unlink an open file on Windows\" issue to come up. And\nthe cross-platform problem I pointed out is a fd leak; leaks would\nnot show up until you run out of the resource, just like you\nwouldn't notice small memory leak here and there UNLESS you actively\nlook for them. I would be surprised if your \"tests\" found anything.\n\n> How would I find the open .pack file fd's? Should I go through\n> /proc/PID/fd? Why is this no problem for other longer running\n> commands such as the git-credential-cache--daemon or git-daemon?\n\nNobody said this is \"no problem for others\". While discussing the\npatch that started this thread, we noticed that any open file\ndescriptor the main process has when the long-running clean/smudge\nhelper is spawned _is_ a problem. Other helpers may share the same\nproblem, and if they do, that is all the more reason that fixing the\nfile descriptor leak is a good thing.\n\nThe primary thing I was wondering was if we should open the window\ninto packfiles with CLOEXEC, just like we recently started opening\nthe tempfiles and lockfiles with the flag. The reason why I asked\nif the site that spawns (i.e. run_command API) has an easy access to\nthe list of file descriptors that we opened ONLY for ourselves is\nbecause that would make it possible to consider an alternative\napproach close them before execve(2) in run_commands API. That\nwould save us from having to sprinkle existing calls to open(2) with\nCLOEXEC. But if that is not the case, opening them with CLOEXEC is\na much better solution than going outside the program to \"find\" them\nin non-portable ways.\n\nThanks.\n"},{"id":"300517","messageId":"xmqqzinv6wtg.fsf@gitster.mtv.corp.google.com","threadId":"43930","inReplyTo":"20160825110752.31581-13-larsxschneider@gmail.com","subject":"Re: [PATCH v6 12/13] convert: add filter..process option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-29T22:21:15Z","receivedAt":"2016-08-29T22:21:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"larsxschneider@gmail.com writes:\n\n> +In case the filter cannot or does not want to process the content,\n> +it is expected to respond with an \"error\" status. Depending on the\n> +`filter..required` flag Git will interpret that as error\n> +but it will not stop or restart the filter process.\n> +------------------------\n> +packet: git< status=error\\n\n> +packet: git< 0000\n> +------------------------\n> +\n> +In case the filter cannot or does not want to process the content\n> +as well as any future content for the lifetime of the Git process,\n> +it is expected to respond with an \"error-all\" status. Depending on\n> +the `filter..required` flag Git will interpret that as error\n> +but it will not stop or restart the filter process.\n> +------------------------\n> +packet: git< status=error-all\\n\n> +packet: git< 0000\n> +------------------------\n\nThis part of the document is well-written to help filter-writers.\n\nOne thing that was unclear from the above to me, when read as a\npotential filter-writer, is when I am supposed to exit(2). After I\ntell Git with error-all (I would have called it \"abort\", but that's\nOK) that I desire no further communication, am I free to go? Or do\nI wait until Git somehow disconnects (perhaps by closing the packet\nstream I have been reading)?\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\nHmph, is there a reason not to retry a half-converted-and-failed\nblob with the fresh process? Note that this is not \"you must do it\nthat way\", and it is not even \"I think doing so may be a better\nidea\". I merely want to know the reason behind this decision.\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\nThe \"kill\" may not be very nice. As Git side _knows_ that the\nfilter is waiting for the next command, having an explicit\n\"shutdown\" command would give the filter a chance to implement a\nclean exit--it may have some housekeeping tasks it wants to perform\nonce it is done. The \"explicit shutdown\" could just be \"the pipe\ngets closed\", so from the implementation point of view there may not\nbe anything you need to further add to this patch (after all, when\nwe exit, the pipes to them would be closed), but the shutdown\nprotocol and the expectation on the behaviour of filter processes\nwould need to be documented.\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\nIt may make more sense to give precedence to the .process (which is\na late-comer) if defined, ignoring .clean and .smudge, than the\nother way around.\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\nWould it be a useful sample program we can ship in contrib/ if you\ncreated a \"filter adapter\" that reads these two configuration\nvariables and act as a filter..process?\n\nDuring an imaginary session of \"git add .\", I think I found where\nyou start THE filter process upon the first path that needs to be\nfiltered with one for the configured , and I think the same\nplace is where you reuse THE filter process, but I am not sure where\nyou are cleaning up by killing the filter once all paths are added.\nWouldn't you need some hooks at strategic places after such bulk\noperation to tell the multi-file-filter machinery to walk all the\nentries in cmd_process_map and tell the remaining filter processes\nthat they have no more tasks, or something? Are you relying on\nthese processes to exit upon a read failure after we exit and the\npipe going to the filter is severed?\n\nThanks.\n"},{"id":"300554","messageId":"F976C223-0CF3-43FB-ABF2-FFE7C600138E@gmail.com","threadId":"43930","inReplyTo":"xmqq7fazcvsk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6 10/13] convert: generate large test files only once","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-30T11:41:59Z","receivedAt":"2016-08-30T11:42:09Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 29 Aug 2016, at 19:46, Junio C Hamano wrote:\n> \n> larsxschneider@gmail.com writes:\n> \n>> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\n>> index 7b45136..34c8eb9 100755\n>> --- a/t/t0021-conversion.sh\n>> +++ b/t/t0021-conversion.sh\n>> @@ -4,6 +4,15 @@ test_description='blob conversion via gitattributes'\n>> \n>> . ./test-lib.sh\n>> \n>> +if test_have_prereq EXPENSIVE\n>> +then\n>> +\tT0021_LARGE_FILE_SIZE=2048\n>> +\tT0021_LARGISH_FILE_SIZE=100\n>> +else\n>> +\tT0021_LARGE_FILE_SIZE=30\n>> +\tT0021_LARGISH_FILE_SIZE=2\n>> +fi\n> \n> Minor: do we need T0021_ prefix? What are you trying to avoid\n> collisions with?\n\nNot necessary. I'll remove the prefix.\n\n\n>> +\tgit checkout -- test test.t test.i &&\n>> +\n>> +\tmkdir generated-test-data &&\n>> +\tfor i in $(test_seq 1 $T0021_LARGE_FILE_SIZE)\n>> +\tdo\n>> +\t\tRANDOM_STRING=\"$(test-genrandom end $i | tr -dc \"A-Za-z0-9\" )\"\n>> +\t\tROT_RANDOM_STRING=\"$(echo $RANDOM_STRING | ./rot13.sh )\"\n> \n> In earlier iteration of loop with lower $i, what guarantees that\n> some bytes survive \"tr -dc\"?\n\nNothing really, good catch! The seed \"end\" produces as first character always a \n\"S\" which would survive \"tr -dc\". However, that is clunky. I will always set \"1\"\nas first character in $RANDOM_STRING to mitigate the problem.\n\n> \n>> +\t\t# Generate 1MB of empty data and 100 bytes of random characters\n> \n> 100 bytes? It seems to me that you are giving 1MB and then $i-byte\n> or less (which sometimes can be zero) of random string.\n\nOutdated comment. Will fix!\n\n> \n>> +\t\t# printf \"$(test-genrandom start $i)\"\n>> +\t\tprintf \"%1048576d\" 1 >>generated-test-data/large.file &&\n>> +\t\tprintf \"$RANDOM_STRING\" >>generated-test-data/large.file &&\n>> +\t\tprintf \"%1048576d\" 1 >>generated-test-data/large.file.rot13 &&\n>> +\t\tprintf \"$ROT_RANDOM_STRING\" >>generated-test-data/large.file.rot13 &&\n>> +\n>> +\t\tif test $i = $T0021_LARGISH_FILE_SIZE\n>> +\t\tthen\n>> +\t\t\tcat generated-test-data/large.file >generated-test-data/largish.file &&\n>> +\t\t\tcat generated-test-data/large.file.rot13 >generated-test-data/largish.file.rot13\n>> +\t\tfi\n>> +\tdone\n> \n> This \"now we are done with the loop, so copy them to the second\n> pair\" needs to be in the loop? Shouldn't it come after 'done'?\n\nNo, it does not need to be in the loop. I think I could do this\nafter the loop instead:\n\nhead -c $((1048576*$T0021_LARGISH_FILE_SIZE)) generated-test-data/large.file >generated-test-data/largish.file\n\n\n> I do not quite get the point of this complexity. You are using\n> exactly the same seed \"end\" every time, so in the first round you\n> have 1M of SP, letter '1', letter 'S' (from the genrandom), then\n> in the second round you have 1M of SP, letter '1', letter 'S' and\n> letter 'p' (the last two from the genrandom), and go on. Is it\n> significant for the purpose of your test that the cruft inserted\n> between the repetition of 1M of SP gets longer by one byte but they\n> all share the same prefix (e.g. \"1S\", \"1Sp\", \"1SpZ\", \"1SpZT\",\n> ... are what you insert between a large run of spaces)?\n\nThe pktline packets have a constant size. If the cruft between 1M of SP \nhas a constant size as well then the generated packets for the test data\nwould repeat themselves. That's why I increased the length after every 1M\nof SP.\n\nHowever, I realized that this test complexity is not necessary. I'll\nsimplify it in the next round.\n\nThanks,\nLars"},{"id":"300556","messageId":"49AB8322-E322-486E-AF36-2C46BC55C82A@gmail.com","threadId":"43930","inReplyTo":"xmqq37lncvj6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6 10/13] convert: generate large test files only once","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-30T11:47:23Z","receivedAt":"2016-08-30T11:47:30Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 29 Aug 2016, at 19:52, Junio C Hamano wrote:\n> \n> Lars Schneider writes:\n> \n>>> On 25 Aug 2016, at 21:17, Stefan Beller wrote:\n>>> \n>>>> On Thu, Aug 25, 2016 at 4:07 AM, wrote:\n>>>> From: Lars Schneider \n>>>> \n>>>> Generate more interesting large test files\n>>> \n>>> How are the large test files more interesting?\n>>> (interesting in the notion of covering more potential bugs?\n>>> easier to debug? better to maintain, or just a pleasant read?)\n>> \n>> The old large test file was 1MB of zeros and 1 byte with a one, repeated 2048 times.\n>> \n>> Since the filter uses 64k packets we would test a large number of equally looking packets.\n>> \n>> That's why I thought the pseudo random content is more interesting.\n> \n> I guess my real question is why it is not just a single invocation\n> of test-genrandom that gives you the whole test file; if you are\n> using 20MB, the simplest would be to grab 20MB out of test-genrandom.\n> With that hopefully you won't see large number of equally looking\n> packets, no?\n\nTrue, but applying rot13 (via tr ...) on 20+ MB takes quite a bit of\ntime. That's why I came up with the 1M SP in between.\n\nHowever, I realized that testing a large amount of data is not really\nnecessary for the final series. A single packet is 64k. A 500k pseudo random\ntest file should be sufficient. This will make the test way simpler.\n\nThanks,\nLars"},{"id":"300557","messageId":"4A177D61-AA25-415A-808D-B6BDA3BB5C47@gmail.com","threadId":"43930","inReplyTo":"xmqq37lnbbpk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6 13/13] read-cache: make sure file handles are not inherited by child processes","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-30T12:32:22Z","receivedAt":"2016-08-30T12:32:32Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 29 Aug 2016, at 21:45, Junio C Hamano wrote:\n> \n> Lars Schneider writes:\n> \n>> I see. Thanks for the explanation.\n> \n> I expect the updated log message to explain the issue correctly\n> then.\n\nSure!\n\n\n>>> The parent is\n>>> very likely to have pack windows open into .pack files and they need\n>>> to be closed on the child side after fork(2) starts the child\n>>> process but before execve(2) runs the helper, if we want to avoid\n>>> file descriptor leaks.\n>> \n>> I think I understand what you are saying. However, during my tests\n>> .pack file fd's were never a problem.\n> \n> I do not expect during the lifetime of your long-running helper\n> anybody would try to unlink an existing packfile, so it is unlikely\n> that \"cannot unlink an open file on Windows\" issue to come up. And\n> the cross-platform problem I pointed out is a fd leak; leaks would\n> not show up until you run out of the resource, just like you\n> wouldn't notice small memory leak here and there UNLESS you actively\n> look for them. I would be surprised if your \"tests\" found anything.\n> \n>> How would I find the open .pack file fd's? Should I go through\n>> /proc/PID/fd? Why is this no problem for other longer running\n>> commands such as the git-credential-cache--daemon or git-daemon?\n> \n> Nobody said this is \"no problem for others\". While discussing the\n> patch that started this thread, we noticed that any open file\n> descriptor the main process has when the long-running clean/smudge\n> helper is spawned _is_ a problem. Other helpers may share the same\n> problem, and if they do, that is all the more reason that fixing the\n> file descriptor leak is a good thing.\n> \n> The primary thing I was wondering was if we should open the window\n> into packfiles with CLOEXEC, just like we recently started opening\n> the tempfiles and lockfiles with the flag. The reason why I asked\n> if the site that spawns (i.e. run_command API) has an easy access to\n> the list of file descriptors that we opened ONLY for ourselves is\n> because that would make it possible to consider an alternative\n> approach close them before execve(2) in run_commands API. That\n> would save us from having to sprinkle existing calls to open(2) with\n> CLOEXEC. But if that is not the case, opening them with CLOEXEC is\n> a much better solution than going outside the program to \"find\" them\n> in non-portable ways.\n\nThe pack files are opened before the filter process is forked. Therefore,\nI think CLOEXEC makes sense for them. Should this change be part of this \nseries? If yes, would it look like below? Should I adjust the function name?\n\nThanks,\nLars\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex d5e1121..759991e 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1485,7 +1485,7 @@ int check_sha1_signature(const unsigned char *sha1, void *map,\n \n int git_open_noatime(const char *name)\n {\n-\tstatic int sha1_file_open_flag = O_NOATIME;\n+\tstatic int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;\n \n \tfor (;;) {\n \t\tint fd;\n\n\n\n"},{"id":"300565","messageId":"20160830145429.GA11221@tb-raspi","threadId":"43930","inReplyTo":"4A177D61-AA25-415A-808D-B6BDA3BB5C47@gmail.com","subject":"Re: [PATCH v6 13/13] read-cache: make sure file handles are not inherited by child processes","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-08-30T14:54:29Z","receivedAt":"2016-08-30T14:54:54Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"\n> \n> diff --git a/sha1_file.c b/sha1_file.c\n> index d5e1121..759991e 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -1485,7 +1485,7 @@ int check_sha1_signature(const unsigned char *sha1, void *map,\n> \n> int git_open_noatime(const char *name)\n\nHm, should the function then be renamed into\n\ngit_open_noatime_cloexec()\n\n> {\n> -\tstatic int sha1_file_open_flag = O_NOATIME;\n> +\tstatic int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;\n> \n> \tfor (;;) {\n> \t\tint fd;\n> \n> \n> \n"},{"id":"300570","messageId":"7A378528-58DE-4C17-9F00-9708947716DD@gmail.com","threadId":"43930","inReplyTo":"xmqqzinv6wtg.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6 12/13] convert: add filter..process option","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-30T16:27:17Z","receivedAt":"2016-08-30T16:27:34Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 30 Aug 2016, at 00:21, Junio C Hamano wrote:\n> \n> larsxschneider@gmail.com writes:\n> \n>> +In case the filter cannot or does not want to process the content,\n>> +it is expected to respond with an \"error\" status. Depending on the\n>> +`filter..required` flag Git will interpret that as error\n>> +but it will not stop or restart the filter process.\n>> +------------------------\n>> +packet: git< status=error\\n\n>> +packet: git< 0000\n>> +------------------------\n>> +\n>> +In case the filter cannot or does not want to process the content\n>> +as well as any future content for the lifetime of the Git process,\n>> +it is expected to respond with an \"error-all\" status. Depending on\n>> +the `filter..required` flag Git will interpret that as error\n>> +but it will not stop or restart the filter process.\n>> +------------------------\n>> +packet: git< status=error-all\\n\n>> +packet: git< 0000\n>> +------------------------\n> \n> This part of the document is well-written to help filter-writers.\n\nThanks!\n\n\n> One thing that was unclear from the above to me, when read as a\n> potential filter-writer, is when I am supposed to exit(2). After I\n> tell Git with error-all (I would have called it \"abort\", but that's\n> OK) that I desire no further communication, am I free to go? Or do\n> I wait until Git somehow disconnects (perhaps by closing the packet\n> stream I have been reading)?\n\nThe filter can exit right after the \"error-all\". If the filter does\nnot exit then Git will kill the filter. I'll add this to the docs.\n\n\"abort\" could be ambiguous because it could be read as \"abort only\nthis file\". \"abort-all\" would work, though. Would you prefer to see\n\"error\" replaced by \"abort\" and \"error-all\" by \"abort-all\"?\n\n\n>> +If the filter dies during the communication or does not adhere to\n>> +the protocol then Git will stop the filter process and restart it\n>> +with the next file that needs to be processed.\n> \n> Hmph, is there a reason not to retry a half-converted-and-failed\n> blob with the fresh process? Note that this is not \"you must do it\n> that way\", and it is not even \"I think doing so may be a better\n> idea\". I merely want to know the reason behind this decision.\n\nA filter that dies during communication or does not adhere to the protocol\nis a faulty filter. Feeding the faulty filter after restart with the same \nblob would likely cause the same error. \n\nThere are legitimate reasons for retries. E.g. if the filter communicates\nwith the network. In these cases I expect the filter to handle the retry\nlogic. Git just writes to and reads from pipes. I don't expect frequent\nproblems in that area. Plus the existing filter mechanism has no retry\neither.\n\nLater on we could easily add a \"retry\" capability if we deem it necessary,\nthough.\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> \n> The \"kill\" may not be very nice. As Git side _knows_ that the\n> filter is waiting for the next command, having an explicit\n> \"shutdown\" command would give the filter a chance to implement a\n> clean exit--it may have some housekeeping tasks it wants to perform\n> once it is done. The \"explicit shutdown\" could just be \"the pipe\n> gets closed\", so from the implementation point of view there may not\n> be anything you need to further add to this patch (after all, when\n> we exit, the pipes to them would be closed), but the shutdown\n> protocol and the expectation on the behaviour of filter processes\n> would need to be documented.\n\nI implemented a shutdown command in v4 [1][2] but dropped it in v5 after\na discussion with Peff [3].\n\n[1] http://public-inbox.org/git/20160803164225.46355-8-larsxschneider@gmail.com/\n[2] http://public-inbox.org/git/20160803164225.46355-13-larsxschneider@gmail.com/\n[3] http://public-inbox.org/git/20160803225313.pk3tfe5ovz4y3i7l@sigill.intra.peff.net/\n\nMy main reasons to drop it:\n\nA) There is no central place in Git that could execute code *after*\n all filter operations are complete and *before* Git exits. Therefore,\n I had to add a \"clean_on_exit_handler()\" to \"run-command\" [1]. This\n change made this series even larger and therefore harder to review.\n\nB) If we communicate \"shutdown\" to the filter then we need to give the\n filter some time to perform the exit before the filter is killed on\n Git exit. I wasn't able to come up with a good answer how long Git \n should wait for the exit.\n\nDo you think I should resurrect the \"shutdown\" patch?\n\n\n>> +If a `filter..clean` or `filter..smudge` command\n>> +is configured then these commands always take precedence over\n>> +a configured `filter..process` command.\n> \n> It may make more sense to give precedence to the .process (which is\n> a late-comer) if defined, ignoring .clean and .smudge, than the\n> other way around.\n\nI agree.\n\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> Would it be a useful sample program we can ship in contrib/ if you\n> created a \"filter adapter\" that reads these two configuration\n> variables and act as a filter..process?\n\nYou mean a v2 filter that would use v1 filters under the hood?\nIf we would drop v1, then this would be useful. Otherwise I don't\nsee any need for such a thing.\n\n\n> During an imaginary session of \"git add .\", I think I found where\n> you start THE filter process upon the first path that needs to be\n> filtered with one for the configured , and I think the same\n> place is where you reuse THE filter process, but I am not sure where\n> you are cleaning up by killing the filter once all paths are added.\n\nI don't clean them up. The filter gets killed when Git exits.\n\n\n> Wouldn't you need some hooks at strategic places after such bulk\n> operation to tell the multi-file-filter machinery to walk all the\n> entries in cmd_process_map and tell the remaining filter processes\n> that they have no more tasks, or something?\n\nThat would be nice. I tried to find these strategic places but it\nturned out to be harder than I thought because Git can exit in a lot\nof places.\n\n\n> Are you relying on\n> these processes to exit upon a read failure after we exit and the\n> pipe going to the filter is severed?\n\nNot really. The filter process is killed by the run-command machinery\nwith the atexit child_cleanup_handler.\n\nThanks,\nLars"},{"id":"300571","messageId":"20160830163749.it4nedannqzqeh5c@sigill.intra.peff.net","threadId":"43930","inReplyTo":"F976C223-0CF3-43FB-ABF2-FFE7C600138E@gmail.com","subject":"Re: [PATCH v6 10/13] convert: generate large test files only once","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-30T16:37:49Z","receivedAt":"2016-08-30T16:37:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 30, 2016 at 01:41:59PM +0200, Lars Schneider wrote:\n\n> >> +\tgit checkout -- test test.t test.i &&\n> >> +\n> >> +\tmkdir generated-test-data &&\n> >> +\tfor i in $(test_seq 1 $T0021_LARGE_FILE_SIZE)\n> >> +\tdo\n> >> +\t\tRANDOM_STRING=\"$(test-genrandom end $i | tr -dc \"A-Za-z0-9\" )\"\n> >> +\t\tROT_RANDOM_STRING=\"$(echo $RANDOM_STRING | ./rot13.sh )\"\n> > \n> > In earlier iteration of loop with lower $i, what guarantees that\n> > some bytes survive \"tr -dc\"?\n> \n> Nothing really, good catch! The seed \"end\" produces as first character always a \n> \"S\" which would survive \"tr -dc\". However, that is clunky. I will always set \"1\"\n> as first character in $RANDOM_STRING to mitigate the problem.\n\nIt seems odd that you would generate a larger set of random bytes and\nthen throw most of them away (about 1 in 5, on average). So you don't\nactually know how long your inputs are, and you're wasting time\ngenerating bytes which are discarded.\n\nThe goal looks like it is just to clean up the string to only-ASCII\ncharacters. Perhaps converting to to base64 or hex would be conceptually\nsimpler? Like:\n\n test-genrandom ... |\n perl -pe 's/./hex(ord($&))/sge'\n\n> > I do not quite get the point of this complexity. You are using\n> > exactly the same seed \"end\" every time, so in the first round you\n> > have 1M of SP, letter '1', letter 'S' (from the genrandom), then\n> > in the second round you have 1M of SP, letter '1', letter 'S' and\n> > letter 'p' (the last two from the genrandom), and go on. Is it\n> > significant for the purpose of your test that the cruft inserted\n> > between the repetition of 1M of SP gets longer by one byte but they\n> > all share the same prefix (e.g. \"1S\", \"1Sp\", \"1SpZ\", \"1SpZT\",\n> > ... are what you insert between a large run of spaces)?\n> \n> The pktline packets have a constant size. If the cruft between 1M of SP \n> has a constant size as well then the generated packets for the test data\n> would repeat themselves. That's why I increased the length after every 1M\n> of SP.\n> \n> However, I realized that this test complexity is not necessary. I'll\n> simplify it in the next round.\n\nI was also confused by this, and wondered if other patterns (perhaps\nusing a single longer genrandom) might be applicable. Simplification (or\nexplanation in comments about what properties the content _needs_ to\nhave) would be welcome. :)\n\n-Peff\n"},{"id":"300573","messageId":"xmqqshtm5h8c.fsf@gitster.mtv.corp.google.com","threadId":"43930","inReplyTo":"49AB8322-E322-486E-AF36-2C46BC55C82A@gmail.com","subject":"Re: [PATCH v6 10/13] convert: generate large test files only once","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-30T16:55:31Z","receivedAt":"2016-08-30T16:55:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n> True, but applying rot13 (via tr ...) on 20+ MB takes quite a bit of\n> time. That's why I came up with the 1M SP in between.\n\nAh, OK, that makes sense (it may not necessarily make it a good\ndecision, but it certainly explains it).\n\n> However, I realized that testing a large amount of data is not really\n> necessary for the final series. A single packet is 64k. A 500k pseudo random\n> test file should be sufficient. This will make the test way simpler.\n\n;-) Thanks.\n"},{"id":"300597","messageId":"xmqqbn0a3wy3.fsf@gitster.mtv.corp.google.com","threadId":"43930","inReplyTo":"7A378528-58DE-4C17-9F00-9708947716DD@gmail.com","subject":"Re: [PATCH v6 12/13] convert: add filter..process option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-30T18:59:00Z","receivedAt":"2016-08-30T18:59:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n>> This part of the document is well-written to help filter-writers.\n>\n> Thanks!\n\nDon't thank me; thank yourself and reviewers of the previous rounds.\n\n\n> The filter can exit right after the \"error-all\". If the filter does\n> not exit then Git will kill the filter. I'll add this to the docs.\n\nOK.\n\n> \"abort\" could be ambiguous because it could be read as \"abort only\n> this file\". \"abort-all\" would work, though. Would you prefer to see\n> \"error\" replaced by \"abort\" and \"error-all\" by \"abort-all\"?\n\nNo.\n\nI was primarily reacting to \"-all\" part, so anything that ends with\n\"-all\" is equally ugly from my point of view and not an improvement.\n\nAs I said, \"error-all\" as long as other reviewers are happy with is\nOK by me, too.\n\n> A filter that dies during communication or does not adhere to the protocol\n> is a faulty filter. Feeding the faulty filter after restart with the same \n> blob would likely cause the same error. \n\nWhy does Git restart it and continue with the rest of the blobs\nthen? Is there a reason to believe it may produce correct results\nfor other blobs if you run it again?\n\n> B) If we communicate \"shutdown\" to the filter then we need to give the\n> filter some time to perform the exit before the filter is killed on\n> Git exit. I wasn't able to come up with a good answer how long Git \n> should wait for the exit.\n\nWould that be an issue? A filter is buggy if told to shutdown,\nignores the request and hangs around forever. I do not think we\neven need to kill it. It is not Git's problem.\n\nI personally would be reluctant to become a filter process writer if\nthe system it will be talking to can kill it without giving it a\nchance to do a graceful exit, but perhaps that is just me. I don't\nknow if closing the pipe going there where you are having Git to\nkill the process on the other end is any more involved than what you\nhave without extra patches.\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>> Would it be a useful sample program we can ship in contrib/ if you\n>> created a \"filter adapter\" that reads these two configuration\n>> variables and act as a filter..process?\n>\n> You mean a v2 filter that would use v1 filters under the hood?\n> If we would drop v1, then this would be useful. Otherwise I don't\n> see any need for such a thing.\n\nI meant it as primarily an example people can learn from when they\nwant to write their own.\n"},{"id":"300604","messageId":"4020D8E2-77D9-46AE-95DC-F644F7C6FDAF@gmail.com","threadId":"43930","inReplyTo":"xmqqbn0a3wy3.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6 12/13] convert: add filter..process option","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-30T20:38:15Z","receivedAt":"2016-08-30T20:38:08Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\nOn 30 Aug 2016, at 20:59, Junio C Hamano wrote:\n> \n>> \"abort\" could be ambiguous because it could be read as \"abort only\n>> this file\". \"abort-all\" would work, though. Would you prefer to see\n>> \"error\" replaced by \"abort\" and \"error-all\" by \"abort-all\"?\n> \n> No.\n> \n> I was primarily reacting to \"-all\" part, so anything that ends with\n> \"-all\" is equally ugly from my point of view and not an improvement.\n> \n> As I said, \"error-all\" as long as other reviewers are happy with is\n> OK by me, too.\n\nNow, I see your point. How about \"error-and-stop\" or \"error-stop\"\ninstead of \"error-all\"?\n\n\n>> A filter that dies during communication or does not adhere to the protocol\n>> is a faulty filter. Feeding the faulty filter after restart with the same \n>> blob would likely cause the same error. \n> \n> Why does Git restart it and continue with the rest of the blobs\n> then? Is there a reason to believe it may produce correct results\n> for other blobs if you run it again?\n\nGood argument. However, my intention here was to mimic the v1 filter\nmechanism. If a filter fails then Git will run the same v1 filter on the\nnext file that needs to be filtered. If `filter..required=false`\nthen a failure is even a legitimate result.\n\n\n>> B) If we communicate \"shutdown\" to the filter then we need to give the\n>> filter some time to perform the exit before the filter is killed on\n>> Git exit. I wasn't able to come up with a good answer how long Git \n>> should wait for the exit.\n> \n> Would that be an issue? A filter is buggy if told to shutdown,\n> ignores the request and hangs around forever. I do not think we\n> even need to kill it. It is not Git's problem.\n> I personally would be reluctant to become a filter process writer if\n> the system it will be talking to can kill it without giving it a\n> chance to do a graceful exit, but perhaps that is just me. I don't\n> know if closing the pipe going there where you are having Git to\n> kill the process on the other end is any more involved than what you\n> have without extra patches.\n\nI am not sure I understand your last sentence. Just to be clear:\nWould you prefer it, if Git would just close the pipe to the filter process\non Git exit and leave the filter running? I think I wouldn't even need\nto close the pipe explicitly. The pipe would be closed automatically when\ngit exits or dies, right? The filter could detect EOF and exit on its own.\nThat would be OK with me.\n\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>>> Would it be a useful sample program we can ship in contrib/ if you\n>>> created a \"filter adapter\" that reads these two configuration\n>>> variables and act as a filter..process?\n>> \n>> You mean a v2 filter that would use v1 filters under the hood?\n>> If we would drop v1, then this would be useful. Otherwise I don't\n>> see any need for such a thing.\n> \n> I meant it as primarily an example people can learn from when they\n> want to write their own.\n\nI think `t/t0021/rot13-filter.pl` (part of this patch) serves this purpose \nalready.\n\n\nThanks,\nLars\n"},{"id":"300605","messageId":"7fbd02a1-ad62-7391-df2a-835aae8a62a7@gmail.com","threadId":"43930","inReplyTo":"xmqqbn0a3wy3.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6 12/13] convert: add filter..process option","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2016-08-30T20:46:12Z","receivedAt":"2016-08-30T20:46:23Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Hello,\n\nW dniu 30.08.2016 o 20:59, Junio C Hamano pisze:\n> Lars Schneider writes:\n[...]\n\n>> The filter can exit right after the \"error-all\". If the filter does\n>> not exit then Git will kill the filter. I'll add this to the docs.\n> \n> OK.\n\nIs it explicit kill, or implicit kill: close pipe and end process?\n\n>> \"abort\" could be ambiguous because it could be read as \"abort only\n>> this file\". \"abort-all\" would work, though. Would you prefer to see\n>> \"error\" replaced by \"abort\" and \"error-all\" by \"abort-all\"?\n> \n> No.\n> \n> I was primarily reacting to \"-all\" part, so anything that ends with\n> \"-all\" is equally ugly from my point of view and not an improvement.\n> \n> As I said, \"error-all\" as long as other reviewers are happy with is\n> OK by me, too.\n\nI'm rather partial to \"abort\" instead of \"error-all\", or \"quit\"/\"exit\"\n(but I prefer \"abort\" or \"bail-out\"), as it better reflects what this\nresponse is about - ending filter process.\n \n>> A filter that dies during communication or does not adhere to the protocol\n>> is a faulty filter. Feeding the faulty filter after restart with the same \n>> blob would likely cause the same error. \n> \n> Why does Git restart it and continue with the rest of the blobs\n> then? Is there a reason to believe it may produce correct results\n> for other blobs if you run it again?\n\nI guess the idea is that single filter can be run on different types\nof blobs, and it could fail on some types (some files) and not others.\nLike LFS-type filter failing on files with size larger than 2 GiB,\nor iconv-like filter to convert UTF-16 to UTF-8 failing on invalid\nbyte sequences.\n\n>> B) If we communicate \"shutdown\" to the filter then we need to give the\n>> filter some time to perform the exit before the filter is killed on\n>> Git exit. I wasn't able to come up with a good answer how long Git \n>> should wait for the exit.\n> \n> Would that be an issue? A filter is buggy if told to shutdown,\n> ignores the request and hangs around forever. I do not think we\n> even need to kill it. It is not Git's problem.\n\nI think the idea was for Git to request shutdown, and filter to tell\nwhen it finished (or just exiting, and Git getting SIGCHLD, I guess).\nIt is hard to tell how much to wait, for example for filter process\nto send \"sync\" command, or finish unmounting, or something...\n\n> I personally would be reluctant to become a filter process writer if\n> the system it will be talking to can kill it without giving it a\n> chance to do a graceful exit, but perhaps that is just me. I don't\n> know if closing the pipe going there where you are having Git to\n> kill the process on the other end is any more involved than what you\n> have without extra patches.\n\nIsn't it the same problem with v1 filters being killed on Git process\nexit? Unless v2 filter wants to do something _after_ writing output\nto Git, it should be safe... unless Git process is killed, but it\nis not different from filter being stray-killed.\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>>> Would it be a useful sample program we can ship in contrib/ if you\n>>> created a \"filter adapter\" that reads these two configuration\n>>> variables and act as a filter..process?\n>>\n>> You mean a v2 filter that would use v1 filters under the hood?\n>> If we would drop v1, then this would be useful. Otherwise I don't\n>> see any need for such a thing.\n> \n> I meant it as primarily an example people can learn from when they\n> want to write their own.\n\nErrr... if it were any v1 filter, it would be useless (as bad or\nworse performance than ordinary v1 clean or smudge filter). It\nmight make sense if v1 filter and v2 wrapper were written in the\nsame [dynamic] language, and wrapper could just load / require\nfilter as a function / procedure, [compile it], and use it.\nFor example smudge/clean filter in Perl (e.g. rot13), and v2 wrapper\nin Perl too.\n\nBest,\n-- \nJakub Narębski \n \n\n"},{"id":"300612","messageId":"xmqqmvjt3nht.fsf@gitster.mtv.corp.google.com","threadId":"43930","inReplyTo":"4020D8E2-77D9-46AE-95DC-F644F7C6FDAF@gmail.com","subject":"Re: [PATCH v6 12/13] convert: add filter..process option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-30T22:23:10Z","receivedAt":"2016-08-30T22:23:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n> On 30 Aug 2016, at 20:59, Junio C Hamano wrote:\n>> \n>>> \"abort\" could be ambiguous because it could be read as \"abort only\n>>> this file\". \"abort-all\" would work, though. Would you prefer to see\n>>> \"error\" replaced by \"abort\" and \"error-all\" by \"abort-all\"?\n>> \n>> No.\n>> \n>> I was primarily reacting to \"-all\" part, so anything that ends with\n>> \"-all\" is equally ugly from my point of view and not an improvement.\n>> \n>> As I said, \"error-all\" as long as other reviewers are happy with is\n>> OK by me, too.\n>\n> Now, I see your point. How about \"error-and-stop\" or \"error-stop\"\n> instead of \"error-all\"?\n\nNot really. I was primarily reacting to having \"error\" and\n\"error-all\", that share the same prefix. Changing \"-all\" suffix to\n\"-stop\" does not make all that difference to me. But in any case,\nas long as other reviewers are happy with it, it is OK by me, too.\n\n> Good argument. However, my intention here was to mimic the v1 filter\n> mechanism.\n\nI am not making any argument and in no way against the chosen\nbehaviour. That \"intention here\" that was revealed after two\niterations is something we would want to see as the reasoning\nbehind the choice of the final behaviour recorded somewhere,\nand now I drew it out of you, I achieved what I set out to do\ninitially ;-)\n\n> I am not sure I understand your last sentence. Just to be clear:\n> Would you prefer it, if Git would just close the pipe to the filter process\n> on Git exit and leave the filter running?\n\nAs a potential filter writer, I would probably feel it the easiest\nto handle if there is no signal involved. My job would be to read\nfrom the command pipe, react to it, and when the command pipe gives\nme EOF, I am expected to exit myself. That would be simple enough.\n\n>> I meant it as primarily an example people can learn from when they\n>> want to write their own.\n>\n> I think `t/t0021/rot13-filter.pl` (part of this patch) serves this purpose \n> already.\n\nI would expect them to peek at contrib/, but do you seriously expect\npeople to comb through t/ directory?\n"},{"id":"300618","messageId":"20160831045713.GA15209@tb-raspi","threadId":"43930","inReplyTo":"xmqqmvjt3nht.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6 12/13] convert: add filter..process option","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-08-31T04:57:13Z","receivedAt":"2016-08-31T04:57:38Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Tue, Aug 30, 2016 at 03:23:10PM -0700, Junio C Hamano wrote:\n> Lars Schneider writes:\n> \n> > On 30 Aug 2016, at 20:59, Junio C Hamano wrote:\n> >> \n> >>> \"abort\" could be ambiguous because it could be read as \"abort only\n> >>> this file\". \"abort-all\" would work, though. Would you prefer to see\n> >>> \"error\" replaced by \"abort\" and \"error-all\" by \"abort-all\"?\n> >> \n> >> No.\n> >> \n> >> I was primarily reacting to \"-all\" part, so anything that ends with\n> >> \"-all\" is equally ugly from my point of view and not an improvement.\n> >> \n> >> As I said, \"error-all\" as long as other reviewers are happy with is\n> >> OK by me, too.\n> >\n> > Now, I see your point. How about \"error-and-stop\" or \"error-stop\"\n> > instead of \"error-all\"?\n> \n> Not really. I was primarily reacting to having \"error\" and\n> \"error-all\", that share the same prefix. Changing \"-all\" suffix to\n> \"-stop\" does not make all that difference to me. But in any case,\n> as long as other reviewers are happy with it, it is OK by me, too.\n> \n> > Good argument. However, my intention here was to mimic the v1 filter\n> > mechanism.\n> \n> I am not making any argument and in no way against the chosen\n> behaviour. That \"intention here\" that was revealed after two\n> iterations is something we would want to see as the reasoning\n> behind the choice of the final behaviour recorded somewhere,\n> and now I drew it out of you, I achieved what I set out to do\n> initially ;-)\n> \n> > I am not sure I understand your last sentence. Just to be clear:\n> > Would you prefer it, if Git would just close the pipe to the filter process\n> > on Git exit and leave the filter running?\n> \n> As a potential filter writer, I would probably feel it the easiest\n> to handle if there is no signal involved. My job would be to read\n> from the command pipe, react to it, and when the command pipe gives\n> me EOF, I am expected to exit myself. That would be simple enough.\n> \n> >> I meant it as primarily an example people can learn from when they\n> >> want to write their own.\n> >\n> > I think `t/t0021/rot13-filter.pl` (part of this patch) serves this purpose \n> > already.\n> \n> I would expect them to peek at contrib/, but do you seriously expect\n> people to comb through t/ directory?\n\nHow about a filter written in C, and placed in ./t/helper/ ?\nAt least I feel that a filter in C-language could be a starting point\nfor others which prefer, depending on the task the filter is going to do,\nto use shell scripts, perl, python or any other high-level language.\n\nA test case, where data can not be filtered, would be a minimum.\nAs Jakub pointed out, you can use iconv with good and bad data.\n"},{"id":"300688","messageId":"f02e2a1d-7e42-2cf8-9b0d-02034e560835@gmail.com","threadId":"43930","inReplyTo":"20160831045713.GA15209@tb-raspi","subject":"Re: [PATCH v6 12/13] convert: add filter..process option","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2016-08-31T13:14:38Z","receivedAt":"2016-08-31T13:15:28Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Hi,\n\nW dniu 31.08.2016 o 06:57, Torsten Bögershausen pisze:\n> On Tue, Aug 30, 2016 at 03:23:10PM -0700, Junio C Hamano wrote:\n>> Lars Schneider writes:\n>>> On 30 Aug 2016, at 20:59, Junio C Hamano wrote:\n\nTorsten, could you please in the future remove irrelevant parts of the\ncited email you are responding to? Thanks in advance,\n\n[...]\n>>>> I meant it as primarily an example people can learn from when they\n>>>> want to write their own.\n>>>\n>>> I think `t/t0021/rot13-filter.pl` (part of this patch) serves this purpose \n>>> already.\n>>\n>> I would expect them to peek at contrib/, but do you seriously expect\n>> people to comb through t/ directory?\n> \n> How about a filter written in C, and placed in ./t/helper/ ?\n>\n> At least I feel that a filter in C-language could be a starting point\n> for others which prefer, depending on the task the filter is going to do,\n> to use shell scripts, perl, python or any other high-level language.\n\nPeople do not look into t/helper/ area, but they do into contrib/.\nGit provides examples on how to use its features there (among other\nthings). Examples for how use the fast-import feature / protocol\nare here, in contrib/fast-import.\n\nI think that C language would be not a good choice, as required\nlow level wrangling of the protocol would obscure the relevant parts,\n... well perhaps except if pkt-line was reused. High-level language\nwould be better. The contrib/fast-import directory includes examples\nin shell script, Perl and Python. The same could be done here\n(possibly with more languages).\n \n> A test case, where data can not be filtered, would be a minimum.\n> As Jakub pointed out, you can use iconv with good and bad data.\n\nThe iconv might be good example, but I think it is not a practical\none; are there development environments that do not handle UTF-8?\n\nMore practical would be to have for example LFS-ish solution of\nstoring files as-is in a specified directory. ROT13 is also good,\nif not a very practical example. Or rezipping all ZIP-compression\nbased files (ODF, JAR/WAR, DOCX / OOXML, CBZ, EPUB, APK, etc.).\n\n-- \nJakub Narębski\n\n"},{"id":"300819","messageId":"xmqqd1kny20k.fsf@gitster.mtv.corp.google.com","threadId":"43930","inReplyTo":"20160830145429.GA11221@tb-raspi","subject":"Re: [PATCH v6 13/13] read-cache: make sure file handles are not inherited by child processes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-01T17:15:55Z","receivedAt":"2016-09-01T20:55:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n>> \n>> diff --git a/sha1_file.c b/sha1_file.c\n>> index d5e1121..759991e 100644\n>> --- a/sha1_file.c\n>> +++ b/sha1_file.c\n>> @@ -1485,7 +1485,7 @@ int check_sha1_signature(const unsigned char *sha1, void *map,\n>> \n>> int git_open_noatime(const char *name)\n>\n> Hm, should the function then be renamed into\n>\n> git_open_noatime_cloexec()\n>\n>> {\n>> -\tstatic int sha1_file_open_flag = O_NOATIME;\n>> +\tstatic int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;\n\nPerhaps.\n\nIn any case, this is probably something that can and should be done\noutside this series.\n\nI am tempted to suggest that the patch 13/13 under discussion may\nalso want to be done outside the scope of, and before, this series.\nEven though with the current system an inherited file descriptor to\nv1 filter processes would cause issues, there is no good reason to\nexpose this file desciptor to them.\n\nThanks.\n\n"},{"id":"301115","messageId":"CCFE4E4A-0724-4AE0-9356-6E95B7C331AF@gmail.com","threadId":"43930","inReplyTo":"7fbd02a1-ad62-7391-df2a-835aae8a62a7@gmail.com","subject":"Re: [PATCH v6 12/13] convert: add filter..process option","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-09-05T19:47:50Z","receivedAt":"2016-09-05T19:48:02Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 30 Aug 2016, at 22:46, Jakub Narębski wrote:\n> \n>>> The filter can exit right after the \"error-all\". If the filter does\n>>> not exit then Git will kill the filter. I'll add this to the docs.\n>> \n>> OK.\n> \n> Is it explicit kill, or implicit kill: close pipe and end process?\n\nI close the pipe and call \"finish_command\".\n\n\n>>> \"abort\" could be ambiguous because it could be read as \"abort only\n>>> this file\". \"abort-all\" would work, though. Would you prefer to see\n>>> \"error\" replaced by \"abort\" and \"error-all\" by \"abort-all\"?\n>> \n>> No.\n>> \n>> I was primarily reacting to \"-all\" part, so anything that ends with\n>> \"-all\" is equally ugly from my point of view and not an improvement.\n>> \n>> As I said, \"error-all\" as long as other reviewers are happy with is\n>> OK by me, too.\n> \n> I'm rather partial to \"abort\" instead of \"error-all\", or \"quit\"/\"exit\"\n> (but I prefer \"abort\" or \"bail-out\"), as it better reflects what this\n> response is about - ending filter process.\n\nAfter some thinking I agree with \"abort\" instead of \"error-all\".\n\n\n>>> A filter that dies during communication or does not adhere to the protocol\n>>> is a faulty filter. Feeding the faulty filter after restart with the same \n>>> blob would likely cause the same error. \n>> \n>> Why does Git restart it and continue with the rest of the blobs\n>> then? Is there a reason to believe it may produce correct results\n>> for other blobs if you run it again?\n> \n> I guess the idea is that single filter can be run on different types\n> of blobs, and it could fail on some types (some files) and not others.\n> Like LFS-type filter failing on files with size larger than 2 GiB,\n> or iconv-like filter to convert UTF-16 to UTF-8 failing on invalid\n> byte sequences.\n\nThis mimics the v1 behavior and I will explain that in the documentation.\n\n\n>>> B) If we communicate \"shutdown\" to the filter then we need to give the\n>>> filter some time to perform the exit before the filter is killed on\n>>> Git exit. I wasn't able to come up with a good answer how long Git \n>>> should wait for the exit.\n>> \n>> Would that be an issue? A filter is buggy if told to shutdown,\n>> ignores the request and hangs around forever. I do not think we\n>> even need to kill it. It is not Git's problem.\n> \n> I think the idea was for Git to request shutdown, and filter to tell\n> when it finished (or just exiting, and Git getting SIGCHLD, I guess).\n> It is hard to tell how much to wait, for example for filter process\n> to send \"sync\" command, or finish unmounting, or something...\n\nI like Junios approach because then we don't need to wait at all...\n\n\n>> I personally would be reluctant to become a filter process writer if\n>> the system it will be talking to can kill it without giving it a\n>> chance to do a graceful exit, but perhaps that is just me. I don't\n>> know if closing the pipe going there where you are having Git to\n>> kill the process on the other end is any more involved than what you\n>> have without extra patches.\n> \n> Isn't it the same problem with v1 filters being killed on Git process\n> exit? Unless v2 filter wants to do something _after_ writing output\n> to Git, it should be safe... unless Git process is killed, but it\n> is not different from filter being stray-killed.\n\nYes, it should be safe. However, I think it is probably nicer if the filter\nprocess can shutdown gracefully instead of being killed.\n\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>>>> Would it be a useful sample program we can ship in contrib/ if you\n>>>> created a \"filter adapter\" that reads these two configuration\n>>>> variables and act as a filter..process?\n>>> \n>>> You mean a v2 filter that would use v1 filters under the hood?\n>>> If we would drop v1, then this would be useful. Otherwise I don't\n>>> see any need for such a thing.\n>> \n>> I meant it as primarily an example people can learn from when they\n>> want to write their own.\n> \n> Errr... if it were any v1 filter, it would be useless (as bad or\n> worse performance than ordinary v1 clean or smudge filter). It\n> might make sense if v1 filter and v2 wrapper were written in the\n> same [dynamic] language, and wrapper could just load / require\n> filter as a function / procedure, [compile it], and use it.\n> For example smudge/clean filter in Perl (e.g. rot13), and v2 wrapper\n> in Perl too.\n\nI'll provide a simple Perl rot13 v2 filter in contrib.\n\nThanks,\nLars"}]}