{"thread":{"id":"46258","subject":"[PATCH v6 0/6] convert: add \"status=delayed\" to filter process protocol","startedAt":"2017-06-25T18:21:34Z","lastAt":"2017-06-27T12:07:12Z","messageCount":24,"participants":["Lars Schneider","Junio C Hamano","Stefan Beller"],"isPatch":true,"patchVersion":6,"patchTotal":6},"messages":[{"id":"323242","messageId":"20170625182125.6741-1-larsxschneider@gmail.com","threadId":"46258","inReplyTo":null,"subject":"[PATCH v6 0/6] convert: add \"status=delayed\" to filter process protocol","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-06-25T18:21:19Z","receivedAt":"2017-06-25T18:21:34Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"Hi,\n\nhere is the 6th iteration of my \"status delayed\" topic. Patch 1 to 3 are\nminor t0021 test adjustments and haven't been changed since v3. Patch 4\nis new and a minor style adjustment. Patch 5 is a minor \"extract method\"\nrefactoring in convert.c with an additional minor style adjustment in\nthis round. Patch 6 is the new feature.\n\nChanges since v5 (full inter diff below!):\n* commit message improvements (Torsten/Peff)\n* reorder checkout struct to keep base_dir* member together (Peff)\n* remove unnecessary initialization in CHECKOUT_INIT (Peff)\n* adhere to Git style (\";\" instead \"{}\") for empty if blocks (Torsten)\n* flag fields before flags (Peff)\n* make operator precedence explicit with parenthesis (Peff)\n* use skip_prefix() to simplify parsing and silently parse unknown keys (Peff)\n* add ce_delay_state explanation (Peff)\n* print files that the filter delayed but not delivered; clean up data structure (Peff)\n\nIf you review this series then please read the \"Delay\" section in\n\"Documentation/gitattributes.txt\" first for an overview of the delay mechanism.\nThe changes in \"t/t0021/rot13-filter.pl\" are easier to review if you ignore\nwhitespace changes.\n\nI also noticed that the diff around the \"static int apply_multi_file_filter\"\nlooks really weird as my new function \"int async_query_available_blobs\" is\nright behind it with similar lines.\n\nThanks,\nLars\n\n\nRFC: http://public-inbox.org/git/D10F7C47-14E8-465B-8B7A-A09A1B28A39F@gmail.com/\nv1: http://public-inbox.org/git/20170108191736.47359-1-larsxschneider@gmail.com/\nv2: http://public-inbox.org/git/20170226184816.30010-1-larsxschneider@gmail.com/\nv3: http://public-inbox.org/git/20170409191107.20547-1-larsxschneider@gmail.com/\nv4: http://public-inbox.org/git/20170522135001.54506-1-larsxschneider@gmail.com/\nv5: http://public-inbox.org/git/20170601082203.50397-1-larsxschneider@gmail.com/\n\n\nBase Ref: master\nWeb-Diff: https://github.com/larsxschneider/git/commit/ab3535aa60\nCheckout: git fetch https://github.com/larsxschneider/git filter-process/delay-v6 && git checkout ab3535aa60\nInterdiff (v5..v6):\n\ndiff --git a/cache.h b/cache.h\nindex 523ead1d95..2ec12c4477 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1543,14 +1543,14 @@ extern int ident_cmp(const struct ident_split *, const struct ident_split *);\n struct checkout {\n \tstruct index_state *istate;\n \tconst char *base_dir;\n-\tstruct delayed_checkout *delayed_checkout;\n \tint base_dir_len;\n+\tstruct delayed_checkout *delayed_checkout;\n \tunsigned force:1,\n \t\t quiet:1,\n \t\t not_new:1,\n \t\t refresh_cache:1;\n };\n-#define CHECKOUT_INIT { NULL, \"\", NULL }\n+#define CHECKOUT_INIT { NULL, \"\" }\n\n\n #define TEMPORARY_FILENAME_LENGTH 25\ndiff --git a/convert.c b/convert.c\nindex c4df174378..6452ab546a 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -572,9 +572,9 @@ static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)\n static void handle_filter_error(const struct strbuf *filter_status,\n \t\t\t\tstruct cmd2process *entry,\n \t\t\t\tconst unsigned int wanted_capability) {\n-\tif (!strcmp(filter_status->buf, \"error\")) {\n-\t\t/* The filter signaled a problem with the file. */\n-\t} else if (!strcmp(filter_status->buf, \"abort\") && wanted_capability) {\n+\tif (!strcmp(filter_status->buf, \"error\"))\n+\t\t; /* The filter signaled a problem with the file. */\n+\telse if (!strcmp(filter_status->buf, \"abort\") && wanted_capability) {\n \t\t/*\n \t\t * The filter signaled a permanent problem. Don't try to filter\n \t\t * files with the same command for the lifetime of the current\n@@ -626,12 +626,12 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n \t}\n \tprocess = &entry->subprocess.process;\n\n-\tif (!(wanted_capability & entry->supported_capabilities))\n+\tif (!(entry->supported_capabilities & wanted_capability))\n \t\treturn 0;\n\n-\tif (CAP_CLEAN & wanted_capability)\n+\tif (wanted_capability & CAP_CLEAN)\n \t\tfilter_type = \"clean\";\n-\telse if (CAP_SMUDGE & wanted_capability)\n+\telse if (wanted_capability & CAP_SMUDGE)\n \t\tfilter_type = \"smudge\";\n \telse\n \t\tdie(\"unexpected filter type\");\n@@ -653,7 +653,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n \tif (err)\n \t\tgoto done;\n\n-\tif (CAP_DELAY & entry->supported_capabilities &&\n+\tif ((entry->supported_capabilities & CAP_DELAY) &&\n \t dco && dco->state == CE_CAN_DELAY) {\n \t\tcan_delay = 1;\n \t\terr = packet_write_fmt_gently(process->in, \"can-delay=1\\n\");\n@@ -736,16 +736,12 @@ int async_query_available_blobs(const char *cmd, struct string_list *delayed_pat\n \tif (err)\n \t\tgoto done;\n\n-\tfor (;;) {\n-\t\tconst char* pre = \"pathname=\";\n-\t\tconst int pre_len = strlen(pre);\n-\t\tline = packet_read_line(process->out, NULL);\n-\t\tif (!line)\n-\t\t\tbreak;\n-\t\terr = strlen(line) <= pre_len || strncmp(line, pre, pre_len);\n-\t\tif (err)\n-\t\t\tgoto done;\n-\t\tstring_list_insert(delayed_paths, xstrdup(line+pre_len));\n+\twhile ((line = packet_read_line(process->out, NULL))) {\n+\t\tconst char *path;\n+\t\tif (skip_prefix(line, \"pathname=\", &path))\n+\t\t\tstring_list_insert(delayed_paths, xstrdup(path));\n+\t\telse\n+\t\t\t; /* ignore unknown keys */\n \t}\n\n \terr = subprocess_read_status(process->out, &filter_status);\n@@ -784,9 +780,9 @@ static int apply_filter(const char *path, const char *src, size_t len,\n \tif (!dst)\n \t\treturn 1;\n\n-\tif ((CAP_CLEAN & wanted_capability) && !drv->process && drv->clean)\n+\tif ((wanted_capability & CAP_CLEAN) && !drv->process && drv->clean)\n \t\tcmd = drv->clean;\n-\telse if ((CAP_SMUDGE & wanted_capability) && !drv->process && drv->smudge)\n+\telse if ((wanted_capability & CAP_SMUDGE) && !drv->process && drv->smudge)\n \t\tcmd = drv->smudge;\n\n \tif (cmd && *cmd)\ndiff --git a/convert.h b/convert.h\nindex c4beaa5101..51f0fb7fbe 100644\n--- a/convert.h\n+++ b/convert.h\n@@ -42,6 +42,10 @@ enum ce_delay_state {\n };\n\n struct delayed_checkout {\n+\t/* State of the currently processed cache entry. If the state is\n+\tCE_CAN_DELAY, then the filter can change this to CE_DELAYED. If\n+\tthe state is CE_RETRY, then this signals the filter that the cache\n+\tentry was requested before. */\n \tenum ce_delay_state state;\n \t/* List of filter drivers that signaled delayed blobs. */\n \tstruct string_list filters;\ndiff --git a/entry.c b/entry.c\nindex c9cb557559..c6d5cc01dc 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -159,10 +159,10 @@ int finish_delayed_checkout(struct checkout *state)\n \tstruct string_list_item *filter, *path;\n \tstruct delayed_checkout *dco = state->delayed_checkout;\n\n-\tif (!state->delayed_checkout) {\n+\tif (!state->delayed_checkout)\n \t\treturn errs;\n-\t}\n\n+\tdco->state = CE_RETRY;\n \twhile (dco->filters.nr > 0) {\n \t\tfor_each_string_list_item(filter, &dco->filters) {\n \t\t\tstruct string_list available_paths;\n@@ -195,7 +195,7 @@ int finish_delayed_checkout(struct checkout *state)\n \t\t\t\tstruct cache_entry* ce = index_file_exists(\n \t\t\t\t\tstate->istate, path->string,\n \t\t\t\t\tstrlen(path->string), 0);\n-\t\t\t\tdco->state = CE_RETRY;\n+\t\t\t\tassert(dco->state == CE_RETRY);\n \t\t\t\terrs |= (ce ? checkout_entry(ce, state, NULL) : 1);\n \t\t\t}\n \t\t}\n@@ -205,6 +205,10 @@ int finish_delayed_checkout(struct checkout *state)\n\n \t/* At this point we should not have any delayed paths anymore. */\n \terrs |= dco->paths.nr;\n+\tfor_each_string_list_item(path, &dco->paths) {\n+\t\twarning(_(\"%s was not filtered properly.\"), path->string);\n+\t}\n+\tstring_list_clear(&dco->paths, 0);\n\n \tfree(dco);\n \tstate->delayed_checkout = NULL;\n\n\nLars Schneider (6):\n t0021: keep filter log files on comparison\n t0021: make debug log file name configurable\n t0021: write \"OUT\" only on success\n convert: put the flags field before the flag itself for consistent\n style\n convert: move multiple file filter error handling to separate function\n convert: add \"status=delayed\" to filter process protocol\n\n Documentation/gitattributes.txt | 65 ++++++++++++-\n builtin/checkout.c | 3 +\n cache.h | 4 +\n convert.c | 168 ++++++++++++++++++++++++---------\n convert.h | 25 +++++\n entry.c | 114 ++++++++++++++++++++++-\n t/t0021-conversion.sh | 136 ++++++++++++++++++++-------\n t/t0021/rot13-filter.pl | 199 ++++++++++++++++++++++++++--------------\n unpack-trees.c | 2 +\n 9 files changed, 567 insertions(+), 149 deletions(-)\n\n\nbase-commit: 0339965c70d68fd3831c9a5306443c869de3f6a8\n--\n2.13.1\n\n"},{"id":"323243","messageId":"20170625182125.6741-2-larsxschneider@gmail.com","threadId":"46258","inReplyTo":"20170625182125.6741-1-larsxschneider@gmail.com","subject":"[PATCH v6 1/6] t0021: keep filter log files on comparison","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-06-25T18:21:20Z","receivedAt":"2017-06-25T18:21:35Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"The filter log files are modified on comparison. Write the modified log files\nto temp files for comparison to fix this.\n\nThis is useful for the subsequent patch 'convert: add \"status=delayed\" to\nfilter process protocol'.\n\nSigned-off-by: Lars Schneider \n---\n t/t0021-conversion.sh | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex 161f560446..ff2424225b 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -42,10 +42,10 @@ test_cmp_count () {\n \tfor FILE in \"$expect\" \"$actual\"\n \tdo\n \t\tsort \"$FILE\" | uniq -c |\n-\t\tsed -e \"s/^ *[0-9][0-9]*[ \t]*IN: /x IN: /\" >\"$FILE.tmp\" &&\n-\t\tmv \"$FILE.tmp\" \"$FILE\" || return\n+\t\tsed -e \"s/^ *[0-9][0-9]*[ \t]*IN: /x IN: /\" >\"$FILE.tmp\"\n \tdone &&\n-\ttest_cmp \"$expect\" \"$actual\"\n+\ttest_cmp \"$expect.tmp\" \"$actual.tmp\" &&\n+\trm \"$expect.tmp\" \"$actual.tmp\"\n }\n \n # Compare two files but exclude all `clean` invocations because Git can\n@@ -56,10 +56,10 @@ test_cmp_exclude_clean () {\n \tactual=$2\n \tfor FILE in \"$expect\" \"$actual\"\n \tdo\n-\t\tgrep -v \"IN: clean\" \"$FILE\" >\"$FILE.tmp\" &&\n-\t\tmv \"$FILE.tmp\" \"$FILE\"\n+\t\tgrep -v \"IN: clean\" \"$FILE\" >\"$FILE.tmp\"\n \tdone &&\n-\ttest_cmp \"$expect\" \"$actual\"\n+\ttest_cmp \"$expect.tmp\" \"$actual.tmp\" &&\n+\trm \"$expect.tmp\" \"$actual.tmp\"\n }\n \n # Check that the contents of two files are equal and that their rot13 version\n-- \n2.13.1\n\n"},{"id":"323244","messageId":"20170625182125.6741-3-larsxschneider@gmail.com","threadId":"46258","inReplyTo":"20170625182125.6741-1-larsxschneider@gmail.com","subject":"[PATCH v6 2/6] t0021: make debug log file name configurable","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-06-25T18:21:21Z","receivedAt":"2017-06-25T18:21:37Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"The \"rot13-filter.pl\" helper wrote its debug logs always to \"rot13-filter.log\".\nMake this configurable by defining the log file as first parameter of\n\"rot13-filter.pl\".\n\nThis is useful if \"rot13-filter.pl\" is configured multiple times similar to the\nsubsequent patch 'convert: add \"status=delayed\" to filter process protocol'.\n\nSigned-off-by: Lars Schneider \n---\n t/t0021-conversion.sh | 44 ++++++++++++++++++++++----------------------\n t/t0021/rot13-filter.pl | 8 +++++---\n 2 files changed, 27 insertions(+), 25 deletions(-)\n\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex ff2424225b..0139b460e7 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -28,7 +28,7 @@ file_size () {\n }\n \n filter_git () {\n-\trm -f rot13-filter.log &&\n+\trm -f *.log &&\n \tgit \"$@\"\n }\n \n@@ -342,7 +342,7 @@ test_expect_success 'diff does not reuse worktree files that need cleaning' '\n '\n \n test_expect_success PERL 'required process filter should filter data' '\n-\ttest_config_global filter.protocol.process \"rot13-filter.pl clean smudge\" &&\n+\ttest_config_global filter.protocol.process \"rot13-filter.pl debug.log clean smudge\" &&\n \ttest_config_global filter.protocol.required true &&\n \trm -rf repo &&\n \tmkdir repo &&\n@@ -375,7 +375,7 @@ test_expect_success PERL 'required process filter should filter data' '\n \t\t\tIN: clean testsubdir/test3 '\\''sq'\\'',\\$x=.r $S3 [OK] -- OUT: $S3 . [OK]\n \t\t\tSTOP\n \t\tEOF\n-\t\ttest_cmp_count expected.log rot13-filter.log &&\n+\t\ttest_cmp_count expected.log debug.log &&\n \n \t\tgit commit -m \"test commit 2\" &&\n \t\trm -f test2.r \"testsubdir/test3 '\\''sq'\\'',\\$x=.r\" &&\n@@ -388,7 +388,7 @@ test_expect_success PERL 'required process filter should filter data' '\n \t\t\tIN: smudge testsubdir/test3 '\\''sq'\\'',\\$x=.r $S3 [OK] -- OUT: $S3 . [OK]\n \t\t\tSTOP\n \t\tEOF\n-\t\ttest_cmp_exclude_clean expected.log rot13-filter.log &&\n+\t\ttest_cmp_exclude_clean expected.log debug.log &&\n \n \t\tfilter_git checkout --quiet --no-progress empty-branch &&\n \t\tcat >expected.log <<-EOF &&\n@@ -397,7 +397,7 @@ test_expect_success PERL 'required process filter should filter data' '\n \t\t\tIN: clean test.r $S [OK] -- OUT: $S . [OK]\n \t\t\tSTOP\n \t\tEOF\n-\t\ttest_cmp_exclude_clean expected.log rot13-filter.log &&\n+\t\ttest_cmp_exclude_clean expected.log debug.log &&\n \n \t\tfilter_git checkout --quiet --no-progress master &&\n \t\tcat >expected.log <<-EOF &&\n@@ -409,7 +409,7 @@ test_expect_success PERL 'required process filter should filter data' '\n \t\t\tIN: smudge testsubdir/test3 '\\''sq'\\'',\\$x=.r $S3 [OK] -- OUT: $S3 . [OK]\n \t\t\tSTOP\n \t\tEOF\n-\t\ttest_cmp_exclude_clean expected.log rot13-filter.log &&\n+\t\ttest_cmp_exclude_clean expected.log debug.log &&\n \n \t\ttest_cmp_committed_rot13 \"$TEST_ROOT/test.o\" test.r &&\n \t\ttest_cmp_committed_rot13 \"$TEST_ROOT/test2.o\" test2.r &&\n@@ -419,7 +419,7 @@ test_expect_success PERL 'required process filter should filter data' '\n \n test_expect_success PERL 'required process filter takes precedence' '\n \ttest_config_global filter.protocol.clean false &&\n-\ttest_config_global filter.protocol.process \"rot13-filter.pl clean\" &&\n+\ttest_config_global filter.protocol.process \"rot13-filter.pl debug.log clean\" &&\n \ttest_config_global filter.protocol.required true &&\n \trm -rf repo &&\n \tmkdir repo &&\n@@ -439,12 +439,12 @@ test_expect_success PERL 'required process filter takes precedence' '\n \t\t\tIN: clean test.r $S [OK] -- OUT: $S . [OK]\n \t\t\tSTOP\n \t\tEOF\n-\t\ttest_cmp_count expected.log rot13-filter.log\n+\t\ttest_cmp_count expected.log debug.log\n \t)\n '\n \n test_expect_success PERL 'required process filter should be used only for \"clean\" operation only' '\n-\ttest_config_global filter.protocol.process \"rot13-filter.pl clean\" &&\n+\ttest_config_global filter.protocol.process \"rot13-filter.pl debug.log clean\" &&\n \trm -rf repo &&\n \tmkdir repo &&\n \t(\n@@ -462,7 +462,7 @@ test_expect_success PERL 'required process filter should be used only for \"clean\n \t\t\tIN: clean test.r $S [OK] -- OUT: $S . [OK]\n \t\t\tSTOP\n \t\tEOF\n-\t\ttest_cmp_count expected.log rot13-filter.log &&\n+\t\ttest_cmp_count expected.log debug.log &&\n \n \t\trm test.r &&\n \n@@ -474,12 +474,12 @@ test_expect_success PERL 'required process filter should be used only for \"clean\n \t\t\tinit handshake complete\n \t\t\tSTOP\n \t\tEOF\n-\t\ttest_cmp_exclude_clean expected.log rot13-filter.log\n+\t\ttest_cmp_exclude_clean expected.log debug.log\n \t)\n '\n \n test_expect_success PERL 'required process filter should process multiple packets' '\n-\ttest_config_global filter.protocol.process \"rot13-filter.pl clean smudge\" &&\n+\ttest_config_global filter.protocol.process \"rot13-filter.pl debug.log clean smudge\" &&\n \ttest_config_global filter.protocol.required true &&\n \n \trm -rf repo &&\n@@ -514,7 +514,7 @@ test_expect_success PERL 'required process filter should process multiple packet\n \t\t\tIN: clean 3pkt_2+1.file $(($S*2+1)) [OK] -- OUT: $(($S*2+1)) ... [OK]\n \t\t\tSTOP\n \t\tEOF\n-\t\ttest_cmp_count expected.log rot13-filter.log &&\n+\t\ttest_cmp_count expected.log debug.log &&\n \n \t\trm -f *.file &&\n \n@@ -529,7 +529,7 @@ test_expect_success PERL 'required process filter should process multiple packet\n \t\t\tIN: smudge 3pkt_2+1.file $(($S*2+1)) [OK] -- OUT: $(($S*2+1)) ... [OK]\n \t\t\tSTOP\n \t\tEOF\n-\t\ttest_cmp_exclude_clean expected.log rot13-filter.log &&\n+\t\ttest_cmp_exclude_clean expected.log debug.log &&\n \n \t\tfor FILE in *.file\n \t\tdo\n@@ -539,7 +539,7 @@ test_expect_success PERL 'required process filter should process multiple packet\n '\n \n test_expect_success PERL 'required process filter with clean error should fail' '\n-\ttest_config_global filter.protocol.process \"rot13-filter.pl clean smudge\" &&\n+\ttest_config_global filter.protocol.process \"rot13-filter.pl debug.log clean smudge\" &&\n \ttest_config_global filter.protocol.required true &&\n \trm -rf repo &&\n \tmkdir repo &&\n@@ -558,7 +558,7 @@ test_expect_success PERL 'required process filter with clean error should fail'\n '\n \n test_expect_success PERL 'process filter should restart after unexpected write failure' '\n-\ttest_config_global filter.protocol.process \"rot13-filter.pl clean smudge\" &&\n+\ttest_config_global filter.protocol.process \"rot13-filter.pl debug.log clean smudge\" &&\n \trm -rf repo &&\n \tmkdir repo &&\n \t(\n@@ -579,7 +579,7 @@ test_expect_success PERL 'process filter should restart after unexpected write f\n \t\tgit add . &&\n \t\trm -f *.r &&\n \n-\t\trm -f rot13-filter.log &&\n+\t\trm -f debug.log &&\n \t\tgit checkout --quiet --no-progress . 2>git-stderr.log &&\n \n \t\tgrep \"smudge write error at\" git-stderr.log &&\n@@ -595,7 +595,7 @@ test_expect_success PERL 'process filter should restart after unexpected write f\n \t\t\tIN: smudge test2.r $S2 [OK] -- OUT: $S2 . [OK]\n \t\t\tSTOP\n \t\tEOF\n-\t\ttest_cmp_exclude_clean expected.log rot13-filter.log &&\n+\t\ttest_cmp_exclude_clean expected.log debug.log &&\n \n \t\ttest_cmp_committed_rot13 \"$TEST_ROOT/test.o\" test.r &&\n \t\ttest_cmp_committed_rot13 \"$TEST_ROOT/test2.o\" test2.r &&\n@@ -609,7 +609,7 @@ test_expect_success PERL 'process filter should restart after unexpected write f\n '\n \n test_expect_success PERL 'process filter should not be restarted if it signals an error' '\n-\ttest_config_global filter.protocol.process \"rot13-filter.pl clean smudge\" &&\n+\ttest_config_global filter.protocol.process \"rot13-filter.pl debug.log clean smudge\" &&\n \trm -rf repo &&\n \tmkdir repo &&\n \t(\n@@ -639,7 +639,7 @@ test_expect_success PERL 'process filter should not be restarted if it signals a\n \t\t\tIN: smudge test2.r $S2 [OK] -- OUT: $S2 . [OK]\n \t\t\tSTOP\n \t\tEOF\n-\t\ttest_cmp_exclude_clean expected.log rot13-filter.log &&\n+\t\ttest_cmp_exclude_clean expected.log debug.log &&\n \n \t\ttest_cmp_committed_rot13 \"$TEST_ROOT/test.o\" test.r &&\n \t\ttest_cmp_committed_rot13 \"$TEST_ROOT/test2.o\" test2.r &&\n@@ -648,7 +648,7 @@ test_expect_success PERL 'process filter should not be restarted if it signals a\n '\n \n test_expect_success PERL 'process filter abort stops processing of all further files' '\n-\ttest_config_global filter.protocol.process \"rot13-filter.pl clean smudge\" &&\n+\ttest_config_global filter.protocol.process \"rot13-filter.pl debug.log clean smudge\" &&\n \trm -rf repo &&\n \tmkdir repo &&\n \t(\n@@ -676,7 +676,7 @@ test_expect_success PERL 'process filter abort stops processing of all further f\n \t\t\tIN: smudge abort.r $SA [OK] -- OUT: 0 [ABORT]\n \t\t\tSTOP\n \t\tEOF\n-\t\ttest_cmp_exclude_clean expected.log rot13-filter.log &&\n+\t\ttest_cmp_exclude_clean expected.log debug.log &&\n \n \t\ttest_cmp \"$TEST_ROOT/test.o\" test.r &&\n \t\ttest_cmp \"$TEST_ROOT/test2.o\" test2.r &&\ndiff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl\nindex 617f581e56..0b943bb377 100644\n--- a/t/t0021/rot13-filter.pl\n+++ b/t/t0021/rot13-filter.pl\n@@ -2,8 +2,9 @@\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+# The first argument defines a debug log file that the script write to.\n+# All remaining arguments define a list of supported protocol\n+# capabilities (\"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@@ -24,9 +25,10 @@ use warnings;\n use IO::File;\n \n my $MAX_PACKET_CONTENT_SIZE = 65516;\n+my $log_file = shift @ARGV;\n my @capabilities = @ARGV;\n \n-open my $debug, \">>\", \"rot13-filter.log\" or die \"cannot open log file: $!\";\n+open my $debug, \">>\", $log_file or die \"cannot open log file: $!\";\n \n sub rot13 {\n \tmy $str = shift;\n-- \n2.13.1\n\n"},{"id":"323245","messageId":"20170625182125.6741-4-larsxschneider@gmail.com","threadId":"46258","inReplyTo":"20170625182125.6741-1-larsxschneider@gmail.com","subject":"[PATCH v6 3/6] t0021: write \"OUT\" only on success","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-06-25T18:21:22Z","receivedAt":"2017-06-25T18:21:38Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\"rot13-filter.pl\" used to write \"OUT \" to the debug log even in case of\nan abort or error. Fix this by writing \"OUT \" to the debug log only in\nthe successful case if output is actually written.\n\nThis is useful for the subsequent patch 'convert: add \"status=delayed\" to\nfilter process protocol'.\n\nSigned-off-by: Lars Schneider \n---\n t/t0021-conversion.sh | 6 +++---\n t/t0021/rot13-filter.pl | 6 +++---\n 2 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex 0139b460e7..0c04d346a1 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -588,7 +588,7 @@ test_expect_success PERL 'process filter should restart after unexpected write f\n \t\tcat >expected.log <<-EOF &&\n \t\t\tSTART\n \t\t\tinit handshake complete\n-\t\t\tIN: smudge smudge-write-fail.r $SF [OK] -- OUT: $SF [WRITE FAIL]\n+\t\t\tIN: smudge smudge-write-fail.r $SF [OK] -- [WRITE FAIL]\n \t\t\tSTART\n \t\t\tinit handshake complete\n \t\t\tIN: smudge test.r $S [OK] -- OUT: $S . [OK]\n@@ -634,7 +634,7 @@ test_expect_success PERL 'process filter should not be restarted if it signals a\n \t\tcat >expected.log <<-EOF &&\n \t\t\tSTART\n \t\t\tinit handshake complete\n-\t\t\tIN: smudge error.r $SE [OK] -- OUT: 0 [ERROR]\n+\t\t\tIN: smudge error.r $SE [OK] -- [ERROR]\n \t\t\tIN: smudge test.r $S [OK] -- OUT: $S . [OK]\n \t\t\tIN: smudge test2.r $S2 [OK] -- OUT: $S2 . [OK]\n \t\t\tSTOP\n@@ -673,7 +673,7 @@ test_expect_success PERL 'process filter abort stops processing of all further f\n \t\tcat >expected.log <<-EOF &&\n \t\t\tSTART\n \t\t\tinit handshake complete\n-\t\t\tIN: smudge abort.r $SA [OK] -- OUT: 0 [ABORT]\n+\t\t\tIN: smudge abort.r $SA [OK] -- [ABORT]\n \t\t\tSTOP\n \t\tEOF\n \t\ttest_cmp_exclude_clean expected.log debug.log &&\ndiff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl\nindex 0b943bb377..5e43faeec1 100644\n--- a/t/t0021/rot13-filter.pl\n+++ b/t/t0021/rot13-filter.pl\n@@ -153,9 +153,6 @@ while (1) {\n \t\tdie \"bad command '$command'\";\n \t}\n \n-\tprint $debug \"OUT: \" . length($output) . \" \";\n-\t$debug->flush();\n-\n \tif ( $pathname eq \"error.r\" ) {\n \t\tprint $debug \"[ERROR]\\n\";\n \t\t$debug->flush();\n@@ -178,6 +175,9 @@ while (1) {\n \t\t\tdie \"${command} write error\";\n \t\t}\n \n+\t\tprint $debug \"OUT: \" . length($output) . \" \";\n+\t\t$debug->flush();\n+\n \t\twhile ( length($output) > 0 ) {\n \t\t\tmy $packet = substr( $output, 0, $MAX_PACKET_CONTENT_SIZE );\n \t\t\tpacket_bin_write($packet);\n-- \n2.13.1\n\n"},{"id":"323246","messageId":"20170625182125.6741-7-larsxschneider@gmail.com","threadId":"46258","inReplyTo":"20170625182125.6741-1-larsxschneider@gmail.com","subject":"[PATCH v6 6/6] convert: add \"status=delayed\" to filter process protocol","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-06-25T18:21:25Z","receivedAt":"2017-06-25T18:21:41Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"Some `clean` / `smudge` filters might require a significant amount of\ntime to process a single blob (e.g. the Git LFS smudge filter might\nperform network requests). During this process the Git checkout\noperation is blocked and Git needs to wait until the filter is done to\ncontinue with the checkout.\n\nTeach the filter process protocol (introduced in edcc858) to accept the\nstatus \"delayed\" as response to a filter request. Upon this response Git\ncontinues with the checkout operation. After the checkout operation Git\ncalls \"finish_delayed_checkout\" which queries the filter for remaining\nblobs. If the filter is still working on the completion, then the filter\nis expected to block. If the filter has completed all remaining blobs\nthen an empty response is expected.\n\nGit has a multiple code paths that checkout a blob. Support delayed\ncheckouts only in `clone` (in unpack-trees.c) and `checkout` operations\nfor now. The optimization is most effective in these code paths as all\nfiles of the tree are processed.\n\nSigned-off-by: Lars Schneider \n---\n Documentation/gitattributes.txt | 65 +++++++++++++-\n builtin/checkout.c | 3 +\n cache.h | 4 +\n convert.c | 115 ++++++++++++++++++++----\n convert.h | 25 ++++++\n entry.c | 114 ++++++++++++++++++++++--\n t/t0021-conversion.sh | 74 ++++++++++++++++\n t/t0021/rot13-filter.pl | 189 ++++++++++++++++++++++++++--------------\n unpack-trees.c | 2 +\n 9 files changed, 501 insertions(+), 90 deletions(-)\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 4736483865..5489e8b723 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -425,8 +425,8 @@ packet: git< capability=clean\n packet: git< capability=smudge\n packet: git< 0000\n ------------------------\n-Supported filter capabilities in version 2 are \"clean\" and\n-\"smudge\".\n+Supported filter capabilities in version 2 are \"clean\", \"smudge\",\n+and \"delay\".\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@@ -512,12 +512,69 @@ the protocol then Git will stop the filter process and restart it\n with the next file that needs to be processed. Depending on the\n `filter..required` flag Git will interpret that as error.\n \n-After the filter has processed a blob it is expected to wait for\n-the next \"key=value\" list containing a command. Git will close\n+After the filter has processed a command it is expected to wait for\n+a \"key=value\" list containing the next command. Git will close\n the command pipe on exit. The filter is expected to detect EOF\n and exit gracefully on its own. Git will wait until the filter\n process has stopped.\n \n+Delay\n+^^^^^\n+\n+If the filter supports the \"delay\" capability, then Git can send the\n+flag \"can-delay\" after the filter command and pathname. This flag\n+denotes that the filter can delay filtering the current blob (e.g. to\n+compensate network latencies) by responding with no content but with\n+the status \"delayed\" and a flush packet.\n+------------------------\n+packet: git> command=smudge\n+packet: git> pathname=path/testfile.dat\n+packet: git> can-delay=1\n+packet: git> 0000\n+packet: git> CONTENT\n+packet: git> 0000\n+packet: git< status=delayed\n+packet: git< 0000\n+------------------------\n+\n+If the filter supports the \"delay\" capability then it must support the\n+\"list_available_blobs\" command. If Git sends this command, then the\n+filter is expected to return a list of pathnames of blobs that are\n+available. The list must be terminated with a flush packet followed\n+by a \"success\" status that is also terminated with a flush packet. If\n+no blobs for the delayed paths are available, yet, then the filter is\n+expected to block the response until at least one blob becomes\n+available. The filter can tell Git that it has no more delayed blobs\n+by sending an empty list.\n+------------------------\n+packet: git> command=list_available_blobs\n+packet: git> 0000\n+packet: git< pathname=path/testfile.dat\n+packet: git< pathname=path/otherfile.dat\n+packet: git< 0000\n+packet: git< status=success\n+packet: git< 0000\n+------------------------\n+\n+After Git received the pathnames, it will request the corresponding\n+blobs again. These requests contain a pathname and an empty content\n+section. The filter is expected to respond with the smudged content\n+in the usual way as explained above.\n+------------------------\n+packet: git> command=smudge\n+packet: git> pathname=path/testfile.dat\n+packet: git> 0000\n+packet: git> 0000 # empty content!\n+packet: git< status=success\n+packet: git< 0000\n+packet: git< SMUDGED_CONTENT\n+packet: git< 0000\n+packet: git< 0000 # empty list, keep \"status=success\" unchanged!\n+------------------------\n+\n+Example\n+^^^^^^^\n+\n A long running filter demo implementation can be found in\n `contrib/long-running-filter/example.pl` located in the Git\n core repository. If you develop your own long running filter\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex a6b2af39d3..c1a256df8d 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -376,6 +376,8 @@ static int checkout_paths(const struct checkout_opts *opts,\n \tstate.force = 1;\n \tstate.refresh_cache = 1;\n \tstate.istate = &the_index;\n+\n+\tenable_delayed_checkout(&state);\n \tfor (pos = 0; pos < active_nr; pos++) {\n \t\tstruct cache_entry *ce = active_cache[pos];\n \t\tif (ce->ce_flags & CE_MATCHED) {\n@@ -390,6 +392,7 @@ static int checkout_paths(const struct checkout_opts *opts,\n \t\t\tpos = skip_same_name(ce, pos) - 1;\n \t\t}\n \t}\n+\terrs |= finish_delayed_checkout(&state);\n \n \tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n \t\tdie(_(\"unable to write new index file\"));\ndiff --git a/cache.h b/cache.h\nindex ae4c45d379..2ec12c4477 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1544,6 +1544,7 @@ struct checkout {\n \tstruct index_state *istate;\n \tconst char *base_dir;\n \tint base_dir_len;\n+\tstruct delayed_checkout *delayed_checkout;\n \tunsigned force:1,\n \t\t quiet:1,\n \t\t not_new:1,\n@@ -1551,8 +1552,11 @@ struct checkout {\n };\n #define CHECKOUT_INIT { NULL, \"\" }\n \n+\n #define TEMPORARY_FILENAME_LENGTH 25\n extern int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *topath);\n+extern void enable_delayed_checkout(struct checkout *state);\n+extern int finish_delayed_checkout(struct checkout *state);\n \n struct cache_def {\n \tstruct strbuf path;\ndiff --git a/convert.c b/convert.c\nindex e55c034d86..6452ab546a 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -496,6 +496,7 @@ static int apply_single_file_filter(const char *path, const char *src, size_t le\n \n #define CAP_CLEAN (1u<<0)\n #define CAP_SMUDGE (1u<<1)\n+#define CAP_DELAY (1u<<2)\n \n struct cmd2process {\n \tstruct subprocess_entry subprocess; /* must be the first member! */\n@@ -533,7 +534,8 @@ static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)\n \tif (err)\n \t\tgoto done;\n \n-\terr = packet_writel(process->in, \"capability=clean\", \"capability=smudge\", NULL);\n+\terr = packet_writel(process->in,\n+\t\t\"capability=clean\", \"capability=smudge\", \"capability=delay\", NULL);\n \n \tfor (;;) {\n \t\tcap_buf = packet_read_line(process->out, NULL);\n@@ -549,6 +551,8 @@ static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)\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 if (!strcmp(cap_name, \"delay\")) {\n+\t\t\tentry->supported_capabilities |= CAP_DELAY;\n \t\t} else {\n \t\t\twarning(\n \t\t\t\t\"external filter '%s' requested unsupported filter capability '%s'\",\n@@ -590,9 +594,11 @@ static void handle_filter_error(const struct strbuf *filter_status,\n \n static int apply_multi_file_filter(const char *path, const char *src, size_t len,\n \t\t\t\t int fd, struct strbuf *dst, const char *cmd,\n-\t\t\t\t const unsigned int wanted_capability)\n+\t\t\t\t const unsigned int wanted_capability,\n+\t\t\t\t struct delayed_checkout *dco)\n {\n \tint err;\n+\tint can_delay = 0;\n \tstruct cmd2process *entry;\n \tstruct child_process *process;\n \tstruct strbuf nbuf = STRBUF_INIT;\n@@ -647,6 +653,14 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n \tif (err)\n \t\tgoto done;\n \n+\tif ((entry->supported_capabilities & CAP_DELAY) &&\n+\t dco && dco->state == CE_CAN_DELAY) {\n+\t\tcan_delay = 1;\n+\t\terr = packet_write_fmt_gently(process->in, \"can-delay=1\\n\");\n+\t\tif (err)\n+\t\t\tgoto done;\n+\t}\n+\n \terr = packet_flush_gently(process->in);\n \tif (err)\n \t\tgoto done;\n@@ -662,14 +676,74 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n \tif (err)\n \t\tgoto done;\n \n-\terr = strcmp(filter_status.buf, \"success\");\n+\tif (can_delay && !strcmp(filter_status.buf, \"delayed\")) {\n+\t\tdco->state = CE_DELAYED;\n+\t\tstring_list_insert(&dco->filters, cmd);\n+\t\tstring_list_insert(&dco->paths, path);\n+\t} else {\n+\t\t/* The filter got the blob and wants to send us a response. */\n+\t\terr = strcmp(filter_status.buf, \"success\");\n+\t\tif (err)\n+\t\t\tgoto done;\n+\n+\t\terr = read_packetized_to_strbuf(process->out, &nbuf) < 0;\n+\t\tif (err)\n+\t\t\tgoto done;\n+\n+\t\terr = subprocess_read_status(process->out, &filter_status);\n+\t\tif (err)\n+\t\t\tgoto done;\n+\n+\t\terr = strcmp(filter_status.buf, \"success\");\n+\t}\n+\n+done:\n+\tsigchain_pop(SIGPIPE);\n+\n+\tif (err)\n+\t\thandle_filter_error(&filter_status, entry, wanted_capability);\n+\telse\n+\t\tstrbuf_swap(dst, &nbuf);\n+\tstrbuf_release(&nbuf);\n+\treturn !err;\n+}\n+\n+\n+int async_query_available_blobs(const char *cmd, struct string_list *delayed_paths)\n+{\n+\tint err;\n+\tchar *line;\n+\tstruct cmd2process *entry;\n+\tstruct child_process *process;\n+\tstruct strbuf filter_status = STRBUF_INIT;\n+\n+\tassert(subprocess_map_initialized);\n+\tentry = (struct cmd2process *)subprocess_find_entry(&subprocess_map, cmd);\n+\tif (!entry) {\n+\t\terror(\"external filter '%s' is not available anymore although \"\n+\t\t \"not all paths have been filtered\", cmd);\n+\t\treturn 0;\n+\t}\n+\tprocess = &entry->subprocess.process;\n+\tsigchain_push(SIGPIPE, SIG_IGN);\n+\n+\terr = packet_write_fmt_gently(\n+\t\tprocess->in, \"command=list_available_blobs\\n\");\n \tif (err)\n \t\tgoto done;\n \n-\terr = read_packetized_to_strbuf(process->out, &nbuf) < 0;\n+\terr = packet_flush_gently(process->in);\n \tif (err)\n \t\tgoto done;\n \n+\twhile ((line = packet_read_line(process->out, NULL))) {\n+\t\tconst char *path;\n+\t\tif (skip_prefix(line, \"pathname=\", &path))\n+\t\t\tstring_list_insert(delayed_paths, xstrdup(path));\n+\t\telse\n+\t\t\t; /* ignore unknown keys */\n+\t}\n+\n \terr = subprocess_read_status(process->out, &filter_status);\n \tif (err)\n \t\tgoto done;\n@@ -680,10 +754,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n \tsigchain_pop(SIGPIPE);\n \n \tif (err)\n-\t\thandle_filter_error(&filter_status, entry, wanted_capability);\n-\telse\n-\t\tstrbuf_swap(dst, &nbuf);\n-\tstrbuf_release(&nbuf);\n+\t\thandle_filter_error(&filter_status, entry, 0);\n \treturn !err;\n }\n \n@@ -698,7 +769,8 @@ static struct convert_driver {\n \n static int apply_filter(const char *path, const char *src, size_t len,\n \t\t\tint fd, struct strbuf *dst, struct convert_driver *drv,\n-\t\t\tconst unsigned int wanted_capability)\n+\t\t\tconst unsigned int wanted_capability,\n+\t\t\tstruct delayed_checkout *dco)\n {\n \tconst char *cmd = NULL;\n \n@@ -716,7 +788,8 @@ static int apply_filter(const char *path, const char *src, size_t len,\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+\t\treturn apply_multi_file_filter(path, src, len, fd, dst,\n+\t\t\tdrv->process, wanted_capability, dco);\n \n \treturn 0;\n }\n@@ -1057,7 +1130,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, CAP_CLEAN);\n+\treturn apply_filter(path, NULL, 0, -1, NULL, ca.drv, CAP_CLEAN, NULL);\n }\n \n const char *get_convert_attr_ascii(const char *path)\n@@ -1094,7 +1167,7 @@ int convert_to_git(const char *path, const char *src, size_t len,\n \n \tconvert_attrs(&ca, path);\n \n-\tret |= apply_filter(path, src, len, -1, dst, ca.drv, CAP_CLEAN);\n+\tret |= apply_filter(path, src, len, -1, dst, ca.drv, CAP_CLEAN, NULL);\n \tif (!ret && ca.drv && ca.drv->required)\n \t\tdie(\"%s: clean filter '%s' failed\", path, ca.drv->name);\n \n@@ -1119,7 +1192,7 @@ void convert_to_git_filter_fd(const char *path, int fd, struct strbuf *dst,\n \tassert(ca.drv);\n \tassert(ca.drv->clean || ca.drv->process);\n \n-\tif (!apply_filter(path, NULL, 0, fd, dst, ca.drv, CAP_CLEAN))\n+\tif (!apply_filter(path, NULL, 0, fd, dst, ca.drv, CAP_CLEAN, NULL))\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@@ -1128,7 +1201,7 @@ void convert_to_git_filter_fd(const char *path, int fd, struct strbuf *dst,\n \n static int convert_to_working_tree_internal(const char *path, const char *src,\n \t\t\t\t\t size_t len, struct strbuf *dst,\n-\t\t\t\t\t int normalizing)\n+\t\t\t\t\t int normalizing, struct delayed_checkout *dco)\n {\n \tint ret = 0, ret_filter = 0;\n \tstruct conv_attrs ca;\n@@ -1153,21 +1226,29 @@ 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, ca.drv, CAP_SMUDGE);\n+\tret_filter = apply_filter(\n+\t\tpath, src, len, -1, dst, ca.drv, CAP_SMUDGE, dco);\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 }\n \n+int async_convert_to_working_tree(const char *path, const char *src,\n+\t\t\t\t size_t len, struct strbuf *dst,\n+\t\t\t\t void *dco)\n+{\n+\treturn convert_to_working_tree_internal(path, src, len, dst, 0, dco);\n+}\n+\n int convert_to_working_tree(const char *path, const char *src, size_t len, struct strbuf *dst)\n {\n-\treturn convert_to_working_tree_internal(path, src, len, dst, 0);\n+\treturn convert_to_working_tree_internal(path, src, len, dst, 0, NULL);\n }\n \n int renormalize_buffer(const char *path, const char *src, size_t len, struct strbuf *dst)\n {\n-\tint ret = convert_to_working_tree_internal(path, src, len, dst, 1);\n+\tint ret = convert_to_working_tree_internal(path, src, len, dst, 1, NULL);\n \tif (ret) {\n \t\tsrc = dst->buf;\n \t\tlen = dst->len;\ndiff --git a/convert.h b/convert.h\nindex 82871a11d5..51f0fb7fbe 100644\n--- a/convert.h\n+++ b/convert.h\n@@ -4,6 +4,8 @@\n #ifndef CONVERT_H\n #define CONVERT_H\n \n+#include \"string-list.h\"\n+\n enum safe_crlf {\n \tSAFE_CRLF_FALSE = 0,\n \tSAFE_CRLF_FAIL = 1,\n@@ -32,6 +34,25 @@ enum eol {\n #endif\n };\n \n+enum ce_delay_state {\n+\tCE_NO_DELAY = 0,\n+\tCE_CAN_DELAY = 1,\n+\tCE_DELAYED = 2,\n+\tCE_RETRY = 3\n+};\n+\n+struct delayed_checkout {\n+\t/* State of the currently processed cache entry. If the state is\n+\tCE_CAN_DELAY, then the filter can change this to CE_DELAYED. If\n+\tthe state is CE_RETRY, then this signals the filter that the cache\n+\tentry was requested before. */\n+\tenum ce_delay_state state;\n+\t/* List of filter drivers that signaled delayed blobs. */\n+\tstruct string_list filters;\n+\t/* List of delayed blobs identified by their path. */\n+\tstruct string_list paths;\n+};\n+\n extern enum eol core_eol;\n extern const char *get_cached_convert_stats_ascii(const char *path);\n extern const char *get_wt_convert_stats_ascii(const char *path);\n@@ -42,6 +63,10 @@ extern int convert_to_git(const char *path, const char *src, size_t len,\n \t\t\t struct strbuf *dst, enum safe_crlf checksafe);\n extern int convert_to_working_tree(const char *path, const char *src,\n \t\t\t\t size_t len, struct strbuf *dst);\n+extern int async_convert_to_working_tree(const char *path, const char *src,\n+\t\t\t\t\t size_t len, struct strbuf *dst,\n+\t\t\t\t\t void *dco);\n+extern int async_query_available_blobs(const char *cmd, struct string_list *delayed_paths);\n extern int renormalize_buffer(const char *path, const char *src, size_t len,\n \t\t\t struct strbuf *dst);\n static inline int would_convert_to_git(const char *path)\ndiff --git a/entry.c b/entry.c\nindex d6b263f78e..c6d5cc01dc 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -137,6 +137,85 @@ static int streaming_write_entry(const struct cache_entry *ce, char *path,\n \treturn result;\n }\n \n+void enable_delayed_checkout(struct checkout *state)\n+{\n+\tif (!state->delayed_checkout) {\n+\t\tstate->delayed_checkout = xmalloc(sizeof(*state->delayed_checkout));\n+\t\tstate->delayed_checkout->state = CE_CAN_DELAY;\n+\t\tstring_list_init(&state->delayed_checkout->filters, 0);\n+\t\tstring_list_init(&state->delayed_checkout->paths, 0);\n+\t}\n+}\n+\n+static int remove_available_paths(struct string_list_item *item, void *cb_data)\n+{\n+\tstruct string_list *available_paths = cb_data;\n+\treturn !string_list_has_string(available_paths, item->string);\n+}\n+\n+int finish_delayed_checkout(struct checkout *state)\n+{\n+\tint errs = 0;\n+\tstruct string_list_item *filter, *path;\n+\tstruct delayed_checkout *dco = state->delayed_checkout;\n+\n+\tif (!state->delayed_checkout)\n+\t\treturn errs;\n+\n+\tdco->state = CE_RETRY;\n+\twhile (dco->filters.nr > 0) {\n+\t\tfor_each_string_list_item(filter, &dco->filters) {\n+\t\t\tstruct string_list available_paths;\n+\t\t\tstring_list_init(&available_paths, 0);\n+\n+\t\t\tif (!async_query_available_blobs(filter->string, &available_paths)) {\n+\t\t\t\t/* Filter reported an error */\n+\t\t\t\terrs = 1;\n+\t\t\t\tfilter->string = \"\";\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t\tif (available_paths.nr <= 0) {\n+\t\t\t\t/* Filter responded with no entries. That means\n+\t\t\t\t the filter is done and we can remove the\n+\t\t\t\t filter from the list (see\n+\t\t\t\t \"string_list_remove_empty_items\" call below).\n+\t\t\t\t*/\n+\t\t\t\tfilter->string = \"\";\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\n+\t\t\t/* In dco->paths we store a list of all delayed paths.\n+\t\t\t The filter just send us a list of available paths.\n+\t\t\t Remove them from the list.\n+\t\t\t*/\n+\t\t\tfilter_string_list(&dco->paths, 0,\n+\t\t\t\t&remove_available_paths, &available_paths);\n+\n+\t\t\tfor_each_string_list_item(path, &available_paths) {\n+\t\t\t\tstruct cache_entry* ce = index_file_exists(\n+\t\t\t\t\tstate->istate, path->string,\n+\t\t\t\t\tstrlen(path->string), 0);\n+\t\t\t\tassert(dco->state == CE_RETRY);\n+\t\t\t\terrs |= (ce ? checkout_entry(ce, state, NULL) : 1);\n+\t\t\t}\n+\t\t}\n+\t\tstring_list_remove_empty_items(&dco->filters, 0);\n+\t}\n+\tstring_list_clear(&dco->filters, 0);\n+\n+\t/* At this point we should not have any delayed paths anymore. */\n+\terrs |= dco->paths.nr;\n+\tfor_each_string_list_item(path, &dco->paths) {\n+\t\twarning(_(\"%s was not filtered properly.\"), path->string);\n+\t}\n+\tstring_list_clear(&dco->paths, 0);\n+\n+\tfree(dco);\n+\tstate->delayed_checkout = NULL;\n+\n+\treturn errs;\n+}\n+\n static int write_entry(struct cache_entry *ce,\n \t\t char *path, const struct checkout *state, int to_tempfile)\n {\n@@ -179,11 +258,36 @@ static int write_entry(struct cache_entry *ce,\n \t\t/*\n \t\t * Convert from git internal format to working tree format\n \t\t */\n-\t\tif (ce_mode_s_ifmt == S_IFREG &&\n-\t\t convert_to_working_tree(ce->name, new, size, &buf)) {\n-\t\t\tfree(new);\n-\t\t\tnew = strbuf_detach(&buf, &newsize);\n-\t\t\tsize = newsize;\n+\t\tif (ce_mode_s_ifmt == S_IFREG) {\n+\t\t\tstruct delayed_checkout *dco = state->delayed_checkout;\n+\t\t\tif (dco && dco->state != CE_NO_DELAY) {\n+\t\t\t\t/* Do not send the blob in case of a retry. */\n+\t\t\t\tif (dco->state == CE_RETRY) {\n+\t\t\t\t\tnew = NULL;\n+\t\t\t\t\tsize = 0;\n+\t\t\t\t}\n+\t\t\t\tret = async_convert_to_working_tree(\n+\t\t\t\t\tce->name, new, size, &buf, dco);\n+\t\t\t\tif (ret && dco->state == CE_DELAYED) {\n+\t\t\t\t\tfree(new);\n+\t\t\t\t\t/* Reset the state of the next blob */\n+\t\t\t\t\tdco->state = CE_CAN_DELAY;\n+\t\t\t\t\tgoto finish;\n+\t\t\t\t}\n+\t\t\t} else\n+\t\t\t\tret = convert_to_working_tree(\n+\t\t\t\t\tce->name, new, size, &buf);\n+\n+\t\t\tif (ret) {\n+\t\t\t\tfree(new);\n+\t\t\t\tnew = strbuf_detach(&buf, &newsize);\n+\t\t\t\tsize = newsize;\n+\t\t\t}\n+\t\t\t/*\n+\t\t\t * No \"else\" here as errors from convert are OK at this\n+\t\t\t * point. If the error would have been fatal (e.g.\n+\t\t\t * filter is required), then we would have died already.\n+\t\t\t */\n \t\t}\n \n \t\tfd = open_output_fd(path, ce, to_tempfile);\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex 0c04d346a1..4b5a45fd43 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -701,4 +701,78 @@ test_expect_success PERL 'invalid process filter must fail (and not hang!)' '\n \t)\n '\n \n+test_expect_success PERL 'delayed checkout in process filter' '\n+\ttest_config_global filter.a.process \"rot13-filter.pl a.log clean smudge delay\" &&\n+\ttest_config_global filter.a.required true &&\n+\ttest_config_global filter.b.process \"rot13-filter.pl b.log clean smudge delay\" &&\n+\ttest_config_global filter.b.required true &&\n+\n+\trm -rf repo &&\n+\tmkdir repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit init &&\n+\t\techo \"*.a filter=a\" >.gitattributes &&\n+\t\techo \"*.b filter=b\" >>.gitattributes &&\n+\t\tcp \"$TEST_ROOT/test.o\" test.a &&\n+\t\tcp \"$TEST_ROOT/test.o\" test-delay10.a &&\n+\t\tcp \"$TEST_ROOT/test.o\" test-delay11.a &&\n+\t\tcp \"$TEST_ROOT/test.o\" test-delay20.a &&\n+\t\tcp \"$TEST_ROOT/test.o\" test-delay10.b &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"test commit 1\"\n+\t) &&\n+\n+\tS=$(file_size \"$TEST_ROOT/test.o\") &&\n+\tcat >a.exp <<-EOF &&\n+\t\tSTART\n+\t\tinit handshake complete\n+\t\tIN: smudge test.a $S [OK] -- OUT: $S . [OK]\n+\t\tIN: smudge test-delay10.a $S [OK] -- [DELAYED]\n+\t\tIN: smudge test-delay11.a $S [OK] -- [DELAYED]\n+\t\tIN: smudge test-delay20.a $S [OK] -- [DELAYED]\n+\t\tIN: list_available_blobs test-delay10.a test-delay11.a [OK]\n+\t\tIN: smudge test-delay10.a 0 [OK] -- OUT: $S . [OK]\n+\t\tIN: smudge test-delay11.a 0 [OK] -- OUT: $S . [OK]\n+\t\tIN: list_available_blobs test-delay20.a [OK]\n+\t\tIN: smudge test-delay20.a 0 [OK] -- OUT: $S . [OK]\n+\t\tIN: list_available_blobs [OK]\n+\t\tSTOP\n+\tEOF\n+\tcat >b.exp <<-EOF &&\n+\t\tSTART\n+\t\tinit handshake complete\n+\t\tIN: smudge test-delay10.b $S [OK] -- [DELAYED]\n+\t\tIN: list_available_blobs test-delay10.b [OK]\n+\t\tIN: smudge test-delay10.b 0 [OK] -- OUT: $S . [OK]\n+\t\tIN: list_available_blobs [OK]\n+\t\tSTOP\n+\tEOF\n+\n+\trm -rf repo-cloned &&\n+\tfilter_git clone repo repo-cloned &&\n+\ttest_cmp_count a.exp repo-cloned/a.log &&\n+\ttest_cmp_count b.exp repo-cloned/b.log &&\n+\n+\t(\n+\t\tcd repo-cloned &&\n+\t\ttest_cmp_committed_rot13 \"$TEST_ROOT/test.o\" test.a &&\n+\t\ttest_cmp_committed_rot13 \"$TEST_ROOT/test.o\" test-delay10.a &&\n+\t\ttest_cmp_committed_rot13 \"$TEST_ROOT/test.o\" test-delay11.a &&\n+\t\ttest_cmp_committed_rot13 \"$TEST_ROOT/test.o\" test-delay20.a &&\n+\t\ttest_cmp_committed_rot13 \"$TEST_ROOT/test.o\" test-delay10.b &&\n+\n+\t\trm *.a *.b &&\n+\t\tfilter_git checkout . &&\n+\t\ttest_cmp_count ../a.exp a.log &&\n+\t\ttest_cmp_count ../b.exp b.log &&\n+\n+\t\ttest_cmp_committed_rot13 \"$TEST_ROOT/test.o\" test.a &&\n+\t\ttest_cmp_committed_rot13 \"$TEST_ROOT/test.o\" test-delay10.a &&\n+\t\ttest_cmp_committed_rot13 \"$TEST_ROOT/test.o\" test-delay11.a &&\n+\t\ttest_cmp_committed_rot13 \"$TEST_ROOT/test.o\" test-delay20.a &&\n+\t\ttest_cmp_committed_rot13 \"$TEST_ROOT/test.o\" test-delay10.b\n+\t)\n+'\n+\n test_done\ndiff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl\nindex 5e43faeec1..f0dc0aff4a 100644\n--- a/t/t0021/rot13-filter.pl\n+++ b/t/t0021/rot13-filter.pl\n@@ -18,6 +18,11 @@\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+# (5) If data with a pathname that is a key in the DELAY hash is\n+# requested (e.g. 'test-delay10.a') then the filter responds with\n+# a \"delay\" status and sets the \"requested\" field in the DELAY hash.\n+# The filter will signal the availability of this object after\n+# \"count\" (field in DELAY hash) \"list_available_blobs\" commands.\n #\n \n use strict;\n@@ -30,6 +35,13 @@ my @capabilities = @ARGV;\n \n open my $debug, \">>\", $log_file or die \"cannot open log file: $!\";\n \n+my %DELAY = (\n+\t'test-delay10.a' => { \"requested\" => 0, \"count\" => 1 },\n+\t'test-delay11.a' => { \"requested\" => 0, \"count\" => 1 },\n+\t'test-delay20.a' => { \"requested\" => 0, \"count\" => 2 },\n+\t'test-delay10.b' => { \"requested\" => 0, \"count\" => 1 },\n+);\n+\n sub rot13 {\n \tmy $str = shift;\n \t$str =~ y/A-Za-z/N-ZA-Mn-za-m/;\n@@ -66,7 +78,7 @@ sub packet_bin_read {\n \n sub packet_txt_read {\n \tmy ( $res, $buf ) = packet_bin_read();\n-\tunless ( $buf =~ s/\\n$// ) {\n+\tunless ( $buf eq '' or $buf =~ s/\\n$// ) {\n \t\tdie \"A non-binary line MUST be terminated by an LF.\";\n \t}\n \treturn ( $res, $buf );\n@@ -101,6 +113,7 @@ packet_flush();\n \n ( packet_txt_read() eq ( 0, \"capability=clean\" ) ) || die \"bad capability\";\n ( packet_txt_read() eq ( 0, \"capability=smudge\" ) ) || die \"bad capability\";\n+( packet_txt_read() eq ( 0, \"capability=delay\" ) ) || die \"bad capability\";\n ( packet_bin_read() eq ( 1, \"\" ) ) || die \"bad capability end\";\n \n foreach (@capabilities) {\n@@ -115,84 +128,132 @@ while (1) {\n \tprint $debug \"IN: $command\";\n \t$debug->flush();\n \n-\tmy ($pathname) = packet_txt_read() =~ /^pathname=(.+)$/;\n-\tprint $debug \" $pathname\";\n-\t$debug->flush();\n-\n-\tif ( $pathname eq \"\" ) {\n-\t\tdie \"bad pathname '$pathname'\";\n-\t}\n+\tif ( $command eq \"list_available_blobs\" ) {\n+\t\t# Flush\n+\t\tpacket_bin_read();\n \n-\t# Flush\n-\tpacket_bin_read();\n-\n-\tmy $input = \"\";\n-\t{\n-\t\tbinmode(STDIN);\n-\t\tmy $buffer;\n-\t\tmy $done = 0;\n-\t\twhile ( !$done ) {\n-\t\t\t( $done, $buffer ) = packet_bin_read();\n-\t\t\t$input .= $buffer;\n+\t\tforeach my $pathname (sort keys %DELAY) {\n+\t\t\tif ( $DELAY{$pathname}{\"requested\"} >= 1 ) {\n+\t\t\t\t$DELAY{$pathname}{\"count\"} = $DELAY{$pathname}{\"count\"} - 1;\n+\t\t\t\tif ($DELAY{$pathname}{\"count\"} == 0 ) {\n+\t\t\t\t\tprint $debug \" $pathname\";\n+\t\t\t\t\tpacket_txt_write(\"pathname=$pathname\");\n+\t\t\t\t}\n+\t\t\t}\n \t\t}\n-\t\tprint $debug \" \" . length($input) . \" [OK] -- \";\n-\t\t$debug->flush();\n-\t}\n-\n-\tmy $output;\n-\tif ( $pathname eq \"error.r\" or $pathname eq \"abort.r\" ) {\n-\t\t$output = \"\";\n-\t}\n-\telsif ( $command eq \"clean\" and grep( /^clean$/, @capabilities ) ) {\n-\t\t$output = rot13($input);\n-\t}\n-\telsif ( $command eq \"smudge\" and grep( /^smudge$/, @capabilities ) ) {\n-\t\t$output = rot13($input);\n-\t}\n-\telse {\n-\t\tdie \"bad command '$command'\";\n-\t}\n \n-\tif ( $pathname eq \"error.r\" ) {\n-\t\tprint $debug \"[ERROR]\\n\";\n-\t\t$debug->flush();\n-\t\tpacket_txt_write(\"status=error\");\n \t\tpacket_flush();\n-\t}\n-\telsif ( $pathname eq \"abort.r\" ) {\n-\t\tprint $debug \"[ABORT]\\n\";\n+\n+\t\tprint $debug \" [OK]\\n\";\n \t\t$debug->flush();\n-\t\tpacket_txt_write(\"status=abort\");\n+\t\tpacket_txt_write(\"status=success\");\n \t\tpacket_flush();\n \t}\n \telse {\n-\t\tpacket_txt_write(\"status=success\");\n-\t\tpacket_flush();\n+\t\tmy ($pathname) = packet_txt_read() =~ /^pathname=(.+)$/;\n+\t\tprint $debug \" $pathname\";\n+\t\t$debug->flush();\n+\n+\t\tif ( $pathname eq \"\" ) {\n+\t\t\tdie \"bad pathname '$pathname'\";\n+\t\t}\n+\n+\t\t# Read until flush\n+\t\tmy ( $done, $buffer ) = packet_txt_read();\n+\t\twhile ( $buffer ne '' ) {\n+\t\t\tif ( $buffer eq \"can-delay=1\" ) {\n+\t\t\t\tif ( exists $DELAY{$pathname} and $DELAY{$pathname}{\"requested\"} == 0 ) {\n+\t\t\t\t\t$DELAY{$pathname}{\"requested\"} = 1;\n+\t\t\t\t}\n+\t\t\t} else {\n+\t\t\t\tdie \"Unknown message '$buffer'\";\n+\t\t\t}\n \n-\t\tif ( $pathname eq \"${command}-write-fail.r\" ) {\n-\t\t\tprint $debug \"[WRITE FAIL]\\n\";\n+\t\t\t( $done, $buffer ) = packet_txt_read();\n+\t\t}\n+\n+\t\tmy $input = \"\";\n+\t\t{\n+\t\t\tbinmode(STDIN);\n+\t\t\tmy $buffer;\n+\t\t\tmy $done = 0;\n+\t\t\twhile ( !$done ) {\n+\t\t\t\t( $done, $buffer ) = packet_bin_read();\n+\t\t\t\t$input .= $buffer;\n+\t\t\t}\n+\t\t\tprint $debug \" \" . length($input) . \" [OK] -- \";\n \t\t\t$debug->flush();\n-\t\t\tdie \"${command} write error\";\n \t\t}\n \n-\t\tprint $debug \"OUT: \" . length($output) . \" \";\n-\t\t$debug->flush();\n+\t\tmy $output;\n+\t\tif ( exists $DELAY{$pathname} and exists $DELAY{$pathname}{\"output\"} ) {\n+\t\t\t$output = $DELAY{$pathname}{\"output\"}\n+\t\t}\n+\t\telsif ( $pathname eq \"error.r\" or $pathname eq \"abort.r\" ) {\n+\t\t\t$output = \"\";\n+\t\t}\n+\t\telsif ( $command eq \"clean\" and grep( /^clean$/, @capabilities ) ) {\n+\t\t\t$output = rot13($input);\n+\t\t}\n+\t\telsif ( $command eq \"smudge\" and grep( /^smudge$/, @capabilities ) ) {\n+\t\t\t$output = rot13($input);\n+\t\t}\n+\t\telse {\n+\t\t\tdie \"bad command '$command'\";\n+\t\t}\n+\n+\t\tif ( $pathname eq \"error.r\" ) {\n+\t\t\tprint $debug \"[ERROR]\\n\";\n+\t\t\t$debug->flush();\n+\t\t\tpacket_txt_write(\"status=error\");\n+\t\t\tpacket_flush();\n+\t\t}\n+\t\telsif ( $pathname eq \"abort.r\" ) {\n+\t\t\tprint $debug \"[ABORT]\\n\";\n+\t\t\t$debug->flush();\n+\t\t\tpacket_txt_write(\"status=abort\");\n+\t\t\tpacket_flush();\n+\t\t}\n+\t\telsif ( $command eq \"smudge\" and\n+\t\t\texists $DELAY{$pathname} and\n+\t\t\t$DELAY{$pathname}{\"requested\"} == 1\n+\t\t) {\n+\t\t\tprint $debug \"[DELAYED]\\n\";\n+\t\t\t$debug->flush();\n+\t\t\tpacket_txt_write(\"status=delayed\");\n+\t\t\tpacket_flush();\n+\t\t\t$DELAY{$pathname}{\"requested\"} = 2;\n+\t\t\t$DELAY{$pathname}{\"output\"} = $output;\n+\t\t}\n+\t\telse {\n+\t\t\tpacket_txt_write(\"status=success\");\n+\t\t\tpacket_flush();\n \n-\t\twhile ( length($output) > 0 ) {\n-\t\t\tmy $packet = substr( $output, 0, $MAX_PACKET_CONTENT_SIZE );\n-\t\t\tpacket_bin_write($packet);\n-\t\t\t# dots represent the number of packets\n-\t\t\tprint $debug \".\";\n-\t\t\tif ( length($output) > $MAX_PACKET_CONTENT_SIZE ) {\n-\t\t\t\t$output = substr( $output, $MAX_PACKET_CONTENT_SIZE );\n+\t\t\tif ( $pathname eq \"${command}-write-fail.r\" ) {\n+\t\t\t\tprint $debug \"[WRITE FAIL]\\n\";\n+\t\t\t\t$debug->flush();\n+\t\t\t\tdie \"${command} write error\";\n \t\t\t}\n-\t\t\telse {\n-\t\t\t\t$output = \"\";\n+\n+\t\t\tprint $debug \"OUT: \" . length($output) . \" \";\n+\t\t\t$debug->flush();\n+\n+\t\t\twhile ( length($output) > 0 ) {\n+\t\t\t\tmy $packet = substr( $output, 0, $MAX_PACKET_CONTENT_SIZE );\n+\t\t\t\tpacket_bin_write($packet);\n+\t\t\t\t# dots represent the number of packets\n+\t\t\t\tprint $debug \".\";\n+\t\t\t\tif ( length($output) > $MAX_PACKET_CONTENT_SIZE ) {\n+\t\t\t\t\t$output = substr( $output, $MAX_PACKET_CONTENT_SIZE );\n+\t\t\t\t}\n+\t\t\t\telse {\n+\t\t\t\t\t$output = \"\";\n+\t\t\t\t}\n \t\t\t}\n+\t\t\tpacket_flush();\n+\t\t\tprint $debug \" [OK]\\n\";\n+\t\t\t$debug->flush();\n+\t\t\tpacket_flush();\n \t\t}\n-\t\tpacket_flush();\n-\t\tprint $debug \" [OK]\\n\";\n-\t\t$debug->flush();\n-\t\tpacket_flush();\n \t}\n }\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex d38c37e38c..491a2562ad 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -379,6 +379,7 @@ static int check_updates(struct unpack_trees_options *o)\n \tif (should_update_submodules() && o->update && !o->dry_run)\n \t\treload_gitmodules_file(index, &state);\n \n+\tenable_delayed_checkout(&state);\n \tfor (i = 0; i < index->cache_nr; i++) {\n \t\tstruct cache_entry *ce = index->cache[i];\n \n@@ -393,6 +394,7 @@ static int check_updates(struct unpack_trees_options *o)\n \t\t\t}\n \t\t}\n \t}\n+\terrs |= finish_delayed_checkout(&state);\n \tstop_progress(&progress);\n \tif (o->update)\n \t\tgit_attr_set_direction(GIT_ATTR_CHECKIN, NULL);\n-- \n2.13.1\n\n"},{"id":"323247","messageId":"20170625182125.6741-5-larsxschneider@gmail.com","threadId":"46258","inReplyTo":"20170625182125.6741-1-larsxschneider@gmail.com","subject":"[PATCH v6 4/6] convert: put the flags field before the flag itself for consistent style","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-06-25T18:21:23Z","receivedAt":"2017-06-25T18:21:43Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"Suggested-by: Jeff King \nSigned-off-by: Lars Schneider \n---\n convert.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex f1e168bc30..9907e3b9ba 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -597,12 +597,12 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n \t}\n \tprocess = &entry->subprocess.process;\n \n-\tif (!(wanted_capability & entry->supported_capabilities))\n+\tif (!(entry->supported_capabilities & wanted_capability))\n \t\treturn 0;\n \n-\tif (CAP_CLEAN & wanted_capability)\n+\tif (wanted_capability & CAP_CLEAN)\n \t\tfilter_type = \"clean\";\n-\telse if (CAP_SMUDGE & wanted_capability)\n+\telse if (wanted_capability & CAP_SMUDGE)\n \t\tfilter_type = \"smudge\";\n \telse\n \t\tdie(\"unexpected filter type\");\n@@ -703,9 +703,9 @@ static int apply_filter(const char *path, const char *src, size_t len,\n \tif (!dst)\n \t\treturn 1;\n \n-\tif ((CAP_CLEAN & wanted_capability) && !drv->process && drv->clean)\n+\tif ((wanted_capability & CAP_CLEAN) && !drv->process && drv->clean)\n \t\tcmd = drv->clean;\n-\telse if ((CAP_SMUDGE & wanted_capability) && !drv->process && drv->smudge)\n+\telse if ((wanted_capability & CAP_SMUDGE) && !drv->process && drv->smudge)\n \t\tcmd = drv->smudge;\n \n \tif (cmd && *cmd)\n-- \n2.13.1\n\n"},{"id":"323248","messageId":"20170625182125.6741-6-larsxschneider@gmail.com","threadId":"46258","inReplyTo":"20170625182125.6741-1-larsxschneider@gmail.com","subject":"[PATCH v6 5/6] convert: move multiple file filter error handling to separate function","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-06-25T18:21:24Z","receivedAt":"2017-06-25T18:21:50Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"Refactoring the filter error handling is useful for the subsequent patch\n'convert: add \"status=delayed\" to filter process protocol'.\n\nIn addition, replace the parentheses around the empty \"if\" block with a\nsingle semicolon to adhere to the Git style guide.\n\nSigned-off-by: Lars Schneider \n---\n convert.c | 47 ++++++++++++++++++++++++++---------------------\n 1 file changed, 26 insertions(+), 21 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 9907e3b9ba..e55c034d86 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -565,6 +565,29 @@ static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)\n \treturn err;\n }\n \n+static void handle_filter_error(const struct strbuf *filter_status,\n+\t\t\t\tstruct cmd2process *entry,\n+\t\t\t\tconst unsigned int wanted_capability) {\n+\tif (!strcmp(filter_status->buf, \"error\"))\n+\t\t; /* The filter signaled a problem with the file. */\n+\telse if (!strcmp(filter_status->buf, \"abort\") && wanted_capability) {\n+\t\t/*\n+\t\t * The filter signaled a permanent problem. Don't try to filter\n+\t\t * files with the same command for the lifetime of the current\n+\t\t * Git process.\n+\t\t */\n+\t\t entry->supported_capabilities &= ~wanted_capability;\n+\t} else {\n+\t\t/*\n+\t\t * Something went wrong with the protocol filter.\n+\t\t * Force shutdown and restart if another blob requires filtering.\n+\t\t */\n+\t\terror(\"external filter '%s' failed\", entry->subprocess.cmd);\n+\t\tsubprocess_stop(&subprocess_map, &entry->subprocess);\n+\t\tfree(entry);\n+\t}\n+}\n+\n static int apply_multi_file_filter(const char *path, const char *src, size_t len,\n \t\t\t\t int fd, struct strbuf *dst, const char *cmd,\n \t\t\t\t const unsigned int wanted_capability)\n@@ -656,28 +679,10 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n done:\n \tsigchain_pop(SIGPIPE);\n \n-\tif (err) {\n-\t\tif (!strcmp(filter_status.buf, \"error\")) {\n-\t\t\t/* The filter signaled a problem with the file. */\n-\t\t} else if (!strcmp(filter_status.buf, \"abort\")) {\n-\t\t\t/*\n-\t\t\t * The filter signaled a permanent problem. Don't try to filter\n-\t\t\t * files with the same command for the lifetime of the current\n-\t\t\t * Git process.\n-\t\t\t */\n-\t\t\t entry->supported_capabilities &= ~wanted_capability;\n-\t\t} else {\n-\t\t\t/*\n-\t\t\t * Something went wrong with the protocol filter.\n-\t\t\t * Force shutdown and restart if another blob requires filtering.\n-\t\t\t */\n-\t\t\terror(\"external filter '%s' failed\", cmd);\n-\t\t\tsubprocess_stop(&subprocess_map, &entry->subprocess);\n-\t\t\tfree(entry);\n-\t\t}\n-\t} else {\n+\tif (err)\n+\t\thandle_filter_error(&filter_status, entry, wanted_capability);\n+\telse\n \t\tstrbuf_swap(dst, &nbuf);\n-\t}\n \tstrbuf_release(&nbuf);\n \treturn !err;\n }\n-- \n2.13.1\n\n"},{"id":"323251","messageId":"xmqqtw337m4b.fsf@gitster.mtv.corp.google.com","threadId":"46258","inReplyTo":"20170625182125.6741-2-larsxschneider@gmail.com","subject":"Re: [PATCH v6 1/6] t0021: keep filter log files on comparison","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-25T22:12:20Z","receivedAt":"2017-06-25T22:12:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n> The filter log files are modified on comparison. Write the modified log files\n> to temp files for comparison to fix this.\n\nThe phrase \"to fix this\" implies that it is _wrong_ to modify after\ncomparing it, but it is unclear _why_ you think is wrong. After\nall, the purpose of this comparison helper is to see if these two\nare the same with cruft removed, and once the helper finds the\nanswer to that question, the current users of the comparison helper\ndo not reuse these files, so from _their_ point of view, there is\nnothing to \"fix\", is there?\n\nIt would become a problem _if_ we want future users of this helper\nto reuse the same expect (or actual) multiple times and start from\nan unmodified one. There may be some other reason why you do not\nwant the comparison to smudge these files. Please state what that\nreason is before saying \"fix this\".\n\nThanks.\n\n>\n> This is useful for the subsequent patch 'convert: add \"status=delayed\" to\n> filter process protocol'.\n>\n> Signed-off-by: Lars Schneider \n> ---\n> t/t0021-conversion.sh | 12 ++++++------\n> 1 file changed, 6 insertions(+), 6 deletions(-)\n>\n> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\n> index 161f560446..ff2424225b 100755\n> --- a/t/t0021-conversion.sh\n> +++ b/t/t0021-conversion.sh\n> @@ -42,10 +42,10 @@ test_cmp_count () {\n> \tfor FILE in \"$expect\" \"$actual\"\n> \tdo\n> \t\tsort \"$FILE\" | uniq -c |\n> -\t\tsed -e \"s/^ *[0-9][0-9]*[ \t]*IN: /x IN: /\" >\"$FILE.tmp\" &&\n> -\t\tmv \"$FILE.tmp\" \"$FILE\" || return\n> +\t\tsed -e \"s/^ *[0-9][0-9]*[ \t]*IN: /x IN: /\" >\"$FILE.tmp\"\n> \tdone &&\n> -\ttest_cmp \"$expect\" \"$actual\"\n> +\ttest_cmp \"$expect.tmp\" \"$actual.tmp\" &&\n> +\trm \"$expect.tmp\" \"$actual.tmp\"\n> }\n> \n> # Compare two files but exclude all `clean` invocations because Git can\n> @@ -56,10 +56,10 @@ test_cmp_exclude_clean () {\n> \tactual=$2\n> \tfor FILE in \"$expect\" \"$actual\"\n> \tdo\n> -\t\tgrep -v \"IN: clean\" \"$FILE\" >\"$FILE.tmp\" &&\n> -\t\tmv \"$FILE.tmp\" \"$FILE\"\n> +\t\tgrep -v \"IN: clean\" \"$FILE\" >\"$FILE.tmp\"\n> \tdone &&\n> -\ttest_cmp \"$expect\" \"$actual\"\n> +\ttest_cmp \"$expect.tmp\" \"$actual.tmp\" &&\n> +\trm \"$expect.tmp\" \"$actual.tmp\"\n> }\n> \n> # Check that the contents of two files are equal and that their rot13 version\n"},{"id":"323252","messageId":"xmqqpodr7lyz.fsf@gitster.mtv.corp.google.com","threadId":"46258","inReplyTo":"20170625182125.6741-3-larsxschneider@gmail.com","subject":"Re: [PATCH v6 2/6] t0021: make debug log file name configurable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-25T22:15:32Z","receivedAt":"2017-06-25T22:15:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n> The \"rot13-filter.pl\" helper wrote its debug logs always to \"rot13-filter.log\".\n> Make this configurable by defining the log file as first parameter of\n> \"rot13-filter.pl\".\n>\n> This is useful if \"rot13-filter.pl\" is configured multiple times similar to the\n> subsequent patch 'convert: add \"status=delayed\" to filter process protocol'.\n\nMakes sense. If you didn't rename the output file to \"debug.log\"\nthe patch may have been less noisy (because you do not have to touch\nthe test_cmp_count calls), but I am guessing that it helped you to\nmake sure that you covered all the existing users of rot13 filter,\nso I am OK with that part of the change as well.\n\nLooks good. Thanks.\n\n>\n> Signed-off-by: Lars Schneider \n> ---\n> t/t0021-conversion.sh | 44 ++++++++++++++++++++++----------------------\n> t/t0021/rot13-filter.pl | 8 +++++---\n> 2 files changed, 27 insertions(+), 25 deletions(-)\n>\n> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\n> index ff2424225b..0139b460e7 100755\n> --- a/t/t0021-conversion.sh\n> +++ b/t/t0021-conversion.sh\n> @@ -28,7 +28,7 @@ file_size () {\n> }\n> \n> filter_git () {\n> -\trm -f rot13-filter.log &&\n> +\trm -f *.log &&\n> \tgit \"$@\"\n> }\n> \n> @@ -342,7 +342,7 @@ test_expect_success 'diff does not reuse worktree files that need cleaning' '\n> '\n> \n> test_expect_success PERL 'required process filter should filter data' '\n> -\ttest_config_global filter.protocol.process \"rot13-filter.pl clean smudge\" &&\n> +\ttest_config_global filter.protocol.process \"rot13-filter.pl debug.log clean smudge\" &&\n> \ttest_config_global filter.protocol.required true &&\n> \trm -rf repo &&\n> \tmkdir repo &&\n> @@ -375,7 +375,7 @@ test_expect_success PERL 'required process filter should filter data' '\n> \t\t\tIN: clean testsubdir/test3 '\\''sq'\\'',\\$x=.r $S3 [OK] -- OUT: $S3 . [OK]\n> \t\t\tSTOP\n> \t\tEOF\n> -\t\ttest_cmp_count expected.log rot13-filter.log &&\n> +\t\ttest_cmp_count expected.log debug.log &&\n> \n> \t\tgit commit -m \"test commit 2\" &&\n> \t\trm -f test2.r \"testsubdir/test3 '\\''sq'\\'',\\$x=.r\" &&\n> @@ -388,7 +388,7 @@ test_expect_success PERL 'required process filter should filter data' '\n> \t\t\tIN: smudge testsubdir/test3 '\\''sq'\\'',\\$x=.r $S3 [OK] -- OUT: $S3 . [OK]\n> \t\t\tSTOP\n> \t\tEOF\n> -\t\ttest_cmp_exclude_clean expected.log rot13-filter.log &&\n> +\t\ttest_cmp_exclude_clean expected.log debug.log &&\n> \n> \t\tfilter_git checkout --quiet --no-progress empty-branch &&\n> \t\tcat >expected.log <<-EOF &&\n> @@ -397,7 +397,7 @@ test_expect_success PERL 'required process filter should filter data' '\n> \t\t\tIN: clean test.r $S [OK] -- OUT: $S . [OK]\n> \t\t\tSTOP\n> \t\tEOF\n> -\t\ttest_cmp_exclude_clean expected.log rot13-filter.log &&\n> +\t\ttest_cmp_exclude_clean expected.log debug.log &&\n> \n> \t\tfilter_git checkout --quiet --no-progress master &&\n> \t\tcat >expected.log <<-EOF &&\n> @@ -409,7 +409,7 @@ test_expect_success PERL 'required process filter should filter data' '\n> \t\t\tIN: smudge testsubdir/test3 '\\''sq'\\'',\\$x=.r $S3 [OK] -- OUT: $S3 . [OK]\n> \t\t\tSTOP\n> \t\tEOF\n> -\t\ttest_cmp_exclude_clean expected.log rot13-filter.log &&\n> +\t\ttest_cmp_exclude_clean expected.log debug.log &&\n> \n> \t\ttest_cmp_committed_rot13 \"$TEST_ROOT/test.o\" test.r &&\n> \t\ttest_cmp_committed_rot13 \"$TEST_ROOT/test2.o\" test2.r &&\n> @@ -419,7 +419,7 @@ test_expect_success PERL 'required process filter should filter data' '\n> \n> test_expect_success PERL 'required process filter takes precedence' '\n> \ttest_config_global filter.protocol.clean false &&\n> -\ttest_config_global filter.protocol.process \"rot13-filter.pl clean\" &&\n> +\ttest_config_global filter.protocol.process \"rot13-filter.pl debug.log clean\" &&\n> \ttest_config_global filter.protocol.required true &&\n> \trm -rf repo &&\n> \tmkdir repo &&\n> @@ -439,12 +439,12 @@ test_expect_success PERL 'required process filter takes precedence' '\n> \t\t\tIN: clean test.r $S [OK] -- OUT: $S . [OK]\n> \t\t\tSTOP\n> \t\tEOF\n> -\t\ttest_cmp_count expected.log rot13-filter.log\n> +\t\ttest_cmp_count expected.log debug.log\n> \t)\n> '\n> \n> test_expect_success PERL 'required process filter should be used only for \"clean\" operation only' '\n> -\ttest_config_global filter.protocol.process \"rot13-filter.pl clean\" &&\n> +\ttest_config_global filter.protocol.process \"rot13-filter.pl debug.log clean\" &&\n> \trm -rf repo &&\n> \tmkdir repo &&\n> \t(\n> @@ -462,7 +462,7 @@ test_expect_success PERL 'required process filter should be used only for \"clean\n> \t\t\tIN: clean test.r $S [OK] -- OUT: $S . [OK]\n> \t\t\tSTOP\n> \t\tEOF\n> -\t\ttest_cmp_count expected.log rot13-filter.log &&\n> +\t\ttest_cmp_count expected.log debug.log &&\n> \n> \t\trm test.r &&\n> \n> @@ -474,12 +474,12 @@ test_expect_success PERL 'required process filter should be used only for \"clean\n> \t\t\tinit handshake complete\n> \t\t\tSTOP\n> \t\tEOF\n> -\t\ttest_cmp_exclude_clean expected.log rot13-filter.log\n> +\t\ttest_cmp_exclude_clean expected.log debug.log\n> \t)\n> '\n> \n> test_expect_success PERL 'required process filter should process multiple packets' '\n> -\ttest_config_global filter.protocol.process \"rot13-filter.pl clean smudge\" &&\n> +\ttest_config_global filter.protocol.process \"rot13-filter.pl debug.log clean smudge\" &&\n> \ttest_config_global filter.protocol.required true &&\n> \n> \trm -rf repo &&\n> @@ -514,7 +514,7 @@ test_expect_success PERL 'required process filter should process multiple packet\n> \t\t\tIN: clean 3pkt_2+1.file $(($S*2+1)) [OK] -- OUT: $(($S*2+1)) ... [OK]\n> \t\t\tSTOP\n> \t\tEOF\n> -\t\ttest_cmp_count expected.log rot13-filter.log &&\n> +\t\ttest_cmp_count expected.log debug.log &&\n> \n> \t\trm -f *.file &&\n> \n> @@ -529,7 +529,7 @@ test_expect_success PERL 'required process filter should process multiple packet\n> \t\t\tIN: smudge 3pkt_2+1.file $(($S*2+1)) [OK] -- OUT: $(($S*2+1)) ... [OK]\n> \t\t\tSTOP\n> \t\tEOF\n> -\t\ttest_cmp_exclude_clean expected.log rot13-filter.log &&\n> +\t\ttest_cmp_exclude_clean expected.log debug.log &&\n> \n> \t\tfor FILE in *.file\n> \t\tdo\n> @@ -539,7 +539,7 @@ test_expect_success PERL 'required process filter should process multiple packet\n> '\n> \n> test_expect_success PERL 'required process filter with clean error should fail' '\n> -\ttest_config_global filter.protocol.process \"rot13-filter.pl clean smudge\" &&\n> +\ttest_config_global filter.protocol.process \"rot13-filter.pl debug.log clean smudge\" &&\n> \ttest_config_global filter.protocol.required true &&\n> \trm -rf repo &&\n> \tmkdir repo &&\n> @@ -558,7 +558,7 @@ test_expect_success PERL 'required process filter with clean error should fail'\n> '\n> \n> test_expect_success PERL 'process filter should restart after unexpected write failure' '\n> -\ttest_config_global filter.protocol.process \"rot13-filter.pl clean smudge\" &&\n> +\ttest_config_global filter.protocol.process \"rot13-filter.pl debug.log clean smudge\" &&\n> \trm -rf repo &&\n> \tmkdir repo &&\n> \t(\n> @@ -579,7 +579,7 @@ test_expect_success PERL 'process filter should restart after unexpected write f\n> \t\tgit add . &&\n> \t\trm -f *.r &&\n> \n> -\t\trm -f rot13-filter.log &&\n> +\t\trm -f debug.log &&\n> \t\tgit checkout --quiet --no-progress . 2>git-stderr.log &&\n> \n> \t\tgrep \"smudge write error at\" git-stderr.log &&\n> @@ -595,7 +595,7 @@ test_expect_success PERL 'process filter should restart after unexpected write f\n> \t\t\tIN: smudge test2.r $S2 [OK] -- OUT: $S2 . [OK]\n> \t\t\tSTOP\n> \t\tEOF\n> -\t\ttest_cmp_exclude_clean expected.log rot13-filter.log &&\n> +\t\ttest_cmp_exclude_clean expected.log debug.log &&\n> \n> \t\ttest_cmp_committed_rot13 \"$TEST_ROOT/test.o\" test.r &&\n> \t\ttest_cmp_committed_rot13 \"$TEST_ROOT/test2.o\" test2.r &&\n> @@ -609,7 +609,7 @@ test_expect_success PERL 'process filter should restart after unexpected write f\n> '\n> \n> test_expect_success PERL 'process filter should not be restarted if it signals an error' '\n> -\ttest_config_global filter.protocol.process \"rot13-filter.pl clean smudge\" &&\n> +\ttest_config_global filter.protocol.process \"rot13-filter.pl debug.log clean smudge\" &&\n> \trm -rf repo &&\n> \tmkdir repo &&\n> \t(\n> @@ -639,7 +639,7 @@ test_expect_success PERL 'process filter should not be restarted if it signals a\n> \t\t\tIN: smudge test2.r $S2 [OK] -- OUT: $S2 . [OK]\n> \t\t\tSTOP\n> \t\tEOF\n> -\t\ttest_cmp_exclude_clean expected.log rot13-filter.log &&\n> +\t\ttest_cmp_exclude_clean expected.log debug.log &&\n> \n> \t\ttest_cmp_committed_rot13 \"$TEST_ROOT/test.o\" test.r &&\n> \t\ttest_cmp_committed_rot13 \"$TEST_ROOT/test2.o\" test2.r &&\n> @@ -648,7 +648,7 @@ test_expect_success PERL 'process filter should not be restarted if it signals a\n> '\n> \n> test_expect_success PERL 'process filter abort stops processing of all further files' '\n> -\ttest_config_global filter.protocol.process \"rot13-filter.pl clean smudge\" &&\n> +\ttest_config_global filter.protocol.process \"rot13-filter.pl debug.log clean smudge\" &&\n> \trm -rf repo &&\n> \tmkdir repo &&\n> \t(\n> @@ -676,7 +676,7 @@ test_expect_success PERL 'process filter abort stops processing of all further f\n> \t\t\tIN: smudge abort.r $SA [OK] -- OUT: 0 [ABORT]\n> \t\t\tSTOP\n> \t\tEOF\n> -\t\ttest_cmp_exclude_clean expected.log rot13-filter.log &&\n> +\t\ttest_cmp_exclude_clean expected.log debug.log &&\n> \n> \t\ttest_cmp \"$TEST_ROOT/test.o\" test.r &&\n> \t\ttest_cmp \"$TEST_ROOT/test2.o\" test2.r &&\n> diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl\n> index 617f581e56..0b943bb377 100644\n> --- a/t/t0021/rot13-filter.pl\n> +++ b/t/t0021/rot13-filter.pl\n> @@ -2,8 +2,9 @@\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> +# The first argument defines a debug log file that the script write to.\n> +# All remaining arguments define a list of supported protocol\n> +# capabilities (\"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> @@ -24,9 +25,10 @@ use warnings;\n> use IO::File;\n> \n> my $MAX_PACKET_CONTENT_SIZE = 65516;\n> +my $log_file = shift @ARGV;\n> my @capabilities = @ARGV;\n> \n> -open my $debug, \">>\", \"rot13-filter.log\" or die \"cannot open log file: $!\";\n> +open my $debug, \">>\", $log_file or die \"cannot open log file: $!\";\n> \n> sub rot13 {\n> \tmy $str = shift;\n"},{"id":"323253","messageId":"xmqqlgof7lw7.fsf@gitster.mtv.corp.google.com","threadId":"46258","inReplyTo":"20170625182125.6741-4-larsxschneider@gmail.com","subject":"Re: [PATCH v6 3/6] t0021: write \"OUT\" only on success","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-25T22:17:12Z","receivedAt":"2017-06-25T22:17:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n> \"rot13-filter.pl\" used to write \"OUT \" to the debug log even in case of\n> an abort or error. Fix this by writing \"OUT \" to the debug log only in\n> the successful case if output is actually written.\n\nAgain, use of \"Fix this\" without clarifying what the problem is. Is\nthis change needed because the size may not be known when the new\nfilter protocol is in use, or something?\n\n\n\n`\n"},{"id":"323254","messageId":"xmqqh8z37lrk.fsf@gitster.mtv.corp.google.com","threadId":"46258","inReplyTo":"20170625182125.6741-5-larsxschneider@gmail.com","subject":"Re: [PATCH v6 4/6] convert: put the flags field before the flag itself for consistent style","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-25T22:19:59Z","receivedAt":"2017-06-25T22:20:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n> Suggested-by: Jeff King \n> Signed-off-by: Lars Schneider \n> ---\n> convert.c | 10 +++++-----\n> 1 file changed, 5 insertions(+), 5 deletions(-)\n\nAn obviously correct no-op.\n\nI do not particularly see the first one making any improvement (but\nit is not making things any worse, either), but turning (CONST &\nvar) to (var & CONST) may make it easier to read for some people so\nI wouldn't complain.\n\nWill queue.\n\n>\n> diff --git a/convert.c b/convert.c\n> index f1e168bc30..9907e3b9ba 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -597,12 +597,12 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n> \t}\n> \tprocess = &entry->subprocess.process;\n> \n> -\tif (!(wanted_capability & entry->supported_capabilities))\n> +\tif (!(entry->supported_capabilities & wanted_capability))\n> \t\treturn 0;\n> \n> -\tif (CAP_CLEAN & wanted_capability)\n> +\tif (wanted_capability & CAP_CLEAN)\n> \t\tfilter_type = \"clean\";\n> -\telse if (CAP_SMUDGE & wanted_capability)\n> +\telse if (wanted_capability & CAP_SMUDGE)\n> \t\tfilter_type = \"smudge\";\n> \telse\n> \t\tdie(\"unexpected filter type\");\n> @@ -703,9 +703,9 @@ static int apply_filter(const char *path, const char *src, size_t len,\n> \tif (!dst)\n> \t\treturn 1;\n> \n> -\tif ((CAP_CLEAN & wanted_capability) && !drv->process && drv->clean)\n> +\tif ((wanted_capability & CAP_CLEAN) && !drv->process && drv->clean)\n> \t\tcmd = drv->clean;\n> -\telse if ((CAP_SMUDGE & wanted_capability) && !drv->process && drv->smudge)\n> +\telse if ((wanted_capability & CAP_SMUDGE) && !drv->process && drv->smudge)\n> \t\tcmd = drv->smudge;\n> \n> \tif (cmd && *cmd)\n"},{"id":"323259","messageId":"79C07E47-251D-4869-8EEA-11A9E336ABF3@gmail.com","threadId":"46258","inReplyTo":"xmqqtw337m4b.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6 1/6] t0021: keep filter log files on comparison","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-06-26T09:02:06Z","receivedAt":"2017-06-26T09:02:14Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 26 Jun 2017, at 00:12, Junio C Hamano wrote:\n> \n> Lars Schneider writes:\n> \n>> The filter log files are modified on comparison. Write the modified log files\n>> to temp files for comparison to fix this.\n> \n> The phrase \"to fix this\" implies that it is _wrong_ to modify after\n> comparing it, but it is unclear _why_ you think is wrong. After\n> all, the purpose of this comparison helper is to see if these two\n> are the same with cruft removed, and once the helper finds the\n> answer to that question, the current users of the comparison helper\n> do not reuse these files, so from _their_ point of view, there is\n> nothing to \"fix\", is there?\n> \n> It would become a problem _if_ we want future users of this helper\n> to reuse the same expect (or actual) multiple times and start from\n> an unmodified one. There may be some other reason why you do not\n> want the comparison to smudge these files. Please state what that\n> reason is before saying \"fix this\".\n\nUnderstood. How about this?\n\n The filter log files are modified on comparison. That might be \n unexpected by the caller. It would be even undesirable if the caller \n wants to reuse the original log files.\n\n Address these issues by using temp files for modifications. This is \n useful for the subsequent patch 'convert: add \"status=delayed\" to \n filter process protocol'.\n\nIf this is OK, then do you want me to resend the series or can you fix it\nin place?\n\nThanks,\nLars"},{"id":"323262","messageId":"0FC46284-650F-411D-845F-0DF4F32071FF@gmail.com","threadId":"46258","inReplyTo":"xmqqlgof7lw7.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6 3/6] t0021: write \"OUT\" only on success","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-06-26T09:26:30Z","receivedAt":"2017-06-26T09:26:40Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 26 Jun 2017, at 00:17, Junio C Hamano wrote:\n> \n> Lars Schneider writes:\n> \n>> \"rot13-filter.pl\" used to write \"OUT \" to the debug log even in case of\n>> an abort or error. Fix this by writing \"OUT \" to the debug log only in\n>> the successful case if output is actually written.\n> \n> Again, use of \"Fix this\" without clarifying what the problem is. Is\n> this change needed because the size may not be known when the new\n> filter protocol is in use, or something?\n\nHow about this?\n\n \"rot13-filter.pl\" always writes \"OUT \" to the debug log at the end\n of an interaction.\n\n This works without issues for the existing cases \"abort\", \"error\", and \n \"success\". In a subsequent patch 'convert: add \"status=delayed\" to \n filter process protocol' we will add a new case \"delayed\". In that case \n we do not send the data right away and it would be wrong/misleading to\n the reader if we would write \"OUT \" to the debug log.\n\n Address this issue by writing \"OUT \" to the debug log only if \n output is actually written in the successful case.\n\n- Lars"},{"id":"323279","messageId":"xmqqvani64g0.fsf@gitster.mtv.corp.google.com","threadId":"46258","inReplyTo":"79C07E47-251D-4869-8EEA-11A9E336ABF3@gmail.com","subject":"Re: [PATCH v6 1/6] t0021: keep filter log files on comparison","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-26T17:31:43Z","receivedAt":"2017-06-26T17:31:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n>> It would become a problem _if_ we want future users of this helper\n>> to reuse the same expect (or actual) multiple times and start from\n>> an unmodified one. There may be some other reason why you do not\n>> want the comparison to smudge these files. Please state what that\n>> reason is before saying \"fix this\".\n>\n> Understood. How about this?\n>\n> The filter log files are modified on comparison. That might be \n> unexpected by the caller. It would be even undesirable if the caller \n> wants to reuse the original log files.\n>\n> Address these issues by using temp files for modifications. This is \n> useful for the subsequent patch 'convert: add \"status=delayed\" to \n> filter process protocol'.\n\nThe updated one is much more understandable. Thanks.\n\n> If this is OK, then do you want me to resend the series or can you fix it\n> in place?\n\nIn general, I am OK running \"rebase -i\" to polish the log message\nunless there are other changes to the patches planned.\n\n"},{"id":"323281","messageId":"xmqqr2y663la.fsf@gitster.mtv.corp.google.com","threadId":"46258","inReplyTo":"0FC46284-650F-411D-845F-0DF4F32071FF@gmail.com","subject":"Re: [PATCH v6 3/6] t0021: write \"OUT\" only on success","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-26T17:50:09Z","receivedAt":"2017-06-26T17:50:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n>> On 26 Jun 2017, at 00:17, Junio C Hamano wrote:\n>> \n>> Lars Schneider writes:\n>> \n>>> \"rot13-filter.pl\" used to write \"OUT \" to the debug log even in case of\n>>> an abort or error. Fix this by writing \"OUT \" to the debug log only in\n>>> the successful case if output is actually written.\n>> \n>> Again, use of \"Fix this\" without clarifying what the problem is. Is\n>> this change needed because the size may not be known when the new\n>> filter protocol is in use, or something?\n>\n> How about this?\n>\n> \"rot13-filter.pl\" always writes \"OUT \" to the debug log at the end\n> of an interaction.\n>\n> This works without issues for the existing cases \"abort\", \"error\", and \n> \"success\". In a subsequent patch 'convert: add \"status=delayed\" to \n> filter process protocol' we will add a new case \"delayed\". In that case \n> we do not send the data right away and it would be wrong/misleading to\n> the reader if we would write \"OUT \" to the debug log.\n\nI still do not quite get \"we do not send the data right away\" as\nopposed to \"at the end of an interaction\" makes it wrong/misleading\nto write \"OUT \"?\n\n A new response \"delayed\" that will be introduced in subsequent\n patches accepts the input, without giving the filtered result\n right away, and at that moment, we do not even have the output\n size yet.\n\nmight be a very reasonable rationale for omitting \"OUT: \" in\nthe output when \"delayed\" request is seen, but that still does not\nexplain why it is sensible to drop \"OUT: \" for error or abort\ncase.\n\nHaving said all that, I tend to agree with the actual change. When\nwe abort or error, we aren't producing any useful output, so it may\nbe sensible _not_ to say \"OUT: 0\" in the log output from the test\nhelper in these cases. And if the change is explained that way,\nreaders would understand why this step is a good thing to have, with\nor without the future \"delayed\" step in the series.\n\nThanks.\n"},{"id":"323282","messageId":"xmqqmv8u63aj.fsf@gitster.mtv.corp.google.com","threadId":"46258","inReplyTo":"20170625182125.6741-6-larsxschneider@gmail.com","subject":"Re: [PATCH v6 5/6] convert: move multiple file filter error handling to separate function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-26T17:56:36Z","receivedAt":"2017-06-26T17:56:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Not about this patch, but viewing this with\n\n\tgit show -w --color-moved=zebra\n\ngives an interesting result. The bulk of the part moved are\nre-indented, and the comment string gets zebra stripes, as if the\nline movement detection code does not realize that these came from\nthe same place.\n\n"},{"id":"323283","messageId":"xmqqinji633p.fsf@gitster.mtv.corp.google.com","threadId":"46258","inReplyTo":"20170625182125.6741-6-larsxschneider@gmail.com","subject":"Re: [PATCH v6 5/6] convert: move multiple file filter error handling to separate function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-26T18:00:42Z","receivedAt":"2017-06-26T18:00:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n> Refactoring the filter error handling is useful for the subsequent patch\n> 'convert: add \"status=delayed\" to filter process protocol'.\n>\n> In addition, replace the parentheses around the empty \"if\" block with a\n> single semicolon to adhere to the Git style guide.\n>\n> Signed-off-by: Lars Schneider \n> ---\n> convert.c | 47 ++++++++++++++++++++++++++---------------------\n> 1 file changed, 26 insertions(+), 21 deletions(-)\n\nThe patch looks obviously correct. Thanks.\n\n> diff --git a/convert.c b/convert.c\n> index 9907e3b9ba..e55c034d86 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -565,6 +565,29 @@ static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)\n> \treturn err;\n> }\n> \n> +static void handle_filter_error(const struct strbuf *filter_status,\n> +\t\t\t\tstruct cmd2process *entry,\n> +\t\t\t\tconst unsigned int wanted_capability) {\n> +\tif (!strcmp(filter_status->buf, \"error\"))\n> +\t\t; /* The filter signaled a problem with the file. */\n> +\telse if (!strcmp(filter_status->buf, \"abort\") && wanted_capability) {\n> +\t\t/*\n> +\t\t * The filter signaled a permanent problem. Don't try to filter\n> +\t\t * files with the same command for the lifetime of the current\n> +\t\t * Git process.\n> +\t\t */\n> +\t\t entry->supported_capabilities &= ~wanted_capability;\n> +\t} else {\n> +\t\t/*\n> +\t\t * Something went wrong with the protocol filter.\n> +\t\t * Force shutdown and restart if another blob requires filtering.\n> +\t\t */\n> +\t\terror(\"external filter '%s' failed\", entry->subprocess.cmd);\n> +\t\tsubprocess_stop(&subprocess_map, &entry->subprocess);\n> +\t\tfree(entry);\n> +\t}\n> +}\n> +\n> static int apply_multi_file_filter(const char *path, const char *src, size_t len,\n> \t\t\t\t int fd, struct strbuf *dst, const char *cmd,\n> \t\t\t\t const unsigned int wanted_capability)\n> @@ -656,28 +679,10 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n> done:\n> \tsigchain_pop(SIGPIPE);\n> \n> -\tif (err) {\n> -\t\tif (!strcmp(filter_status.buf, \"error\")) {\n> -\t\t\t/* The filter signaled a problem with the file. */\n> -\t\t} else if (!strcmp(filter_status.buf, \"abort\")) {\n> -\t\t\t/*\n> -\t\t\t * The filter signaled a permanent problem. Don't try to filter\n> -\t\t\t * files with the same command for the lifetime of the current\n> -\t\t\t * Git process.\n> -\t\t\t */\n> -\t\t\t entry->supported_capabilities &= ~wanted_capability;\n> -\t\t} else {\n> -\t\t\t/*\n> -\t\t\t * Something went wrong with the protocol filter.\n> -\t\t\t * Force shutdown and restart if another blob requires filtering.\n> -\t\t\t */\n> -\t\t\terror(\"external filter '%s' failed\", cmd);\n> -\t\t\tsubprocess_stop(&subprocess_map, &entry->subprocess);\n> -\t\t\tfree(entry);\n> -\t\t}\n> -\t} else {\n> +\tif (err)\n> +\t\thandle_filter_error(&filter_status, entry, wanted_capability);\n> +\telse\n> \t\tstrbuf_swap(dst, &nbuf);\n> -\t}\n> \tstrbuf_release(&nbuf);\n> \treturn !err;\n> }\n"},{"id":"323288","messageId":"6AA7EA12-9EC1-48E3-A32A-BB7A406CE40F@gmail.com","threadId":"46258","inReplyTo":"xmqqr2y663la.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6 3/6] t0021: write \"OUT\" only on success","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-06-26T18:32:29Z","receivedAt":"2017-06-26T18:32:48Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 26 Jun 2017, at 19:50, Junio C Hamano wrote:\n> \n> Lars Schneider writes:\n> \n>>> On 26 Jun 2017, at 00:17, Junio C Hamano wrote:\n>>> \n>>> Lars Schneider writes:\n>>> \n>>>> \"rot13-filter.pl\" used to write \"OUT \" to the debug log even in case of\n>>>> an abort or error. Fix this by writing \"OUT \" to the debug log only in\n>>>> the successful case if output is actually written.\n>>> \n>>> Again, use of \"Fix this\" without clarifying what the problem is. Is\n>>> this change needed because the size may not be known when the new\n>>> filter protocol is in use, or something?\n>> \n>> How about this?\n>> \n>> \"rot13-filter.pl\" always writes \"OUT \" to the debug log at the end\n>> of an interaction.\n>> \n>> This works without issues for the existing cases \"abort\", \"error\", and \n>> \"success\". In a subsequent patch 'convert: add \"status=delayed\" to \n>> filter process protocol' we will add a new case \"delayed\". In that case \n>> we do not send the data right away and it would be wrong/misleading to\n>> the reader if we would write \"OUT \" to the debug log.\n> \n> I still do not quite get \"we do not send the data right away\" as\n> opposed to \"at the end of an interaction\" makes it wrong/misleading\n> to write \"OUT \"?\n> \n> A new response \"delayed\" that will be introduced in subsequent\n> patches accepts the input, without giving the filtered result\n> right away, and at that moment, we do not even have the output\n> size yet.\n> \n> might be a very reasonable rationale for omitting \"OUT: \" in\n> the output when \"delayed\" request is seen, but that still does not\n> explain why it is sensible to drop \"OUT: \" for error or abort\n> case.\n\nWell, \"rot13-filter.pl\" *does* have the output available right away\neven in the delayed case (because of the mock implementation). \nIf we print \"OUT: \" all the time then this would lead to\nmisleading log messages in this situation.\n\nIt's not necessary to drop \"OUT: \" for abort and error. It\nwas just the way that required the least number of lines of code.\n\n\n> Having said all that, I tend to agree with the actual change. When\n> we abort or error, we aren't producing any useful output, so it may\n> be sensible _not_ to say \"OUT: 0\" in the log output from the test\n> helper in these cases. And if the change is explained that way,\n> readers would understand why this step is a good thing to have, with\n> or without the future \"delayed\" step in the series.\n\nHow about this?\n\n \"rot13-filter.pl\" always writes \"OUT \" to the debug log at the end\n of a response.\n\n This works without issues for the existing responses \"abort\", \"error\", \n and \"success\". A new response \"delayed\", that will be introduced in \n subsequent patches, accepts the input without giving the filtered result\n right away. Since we actually have the data already available in our mock\n filter the debug log output would be wrong/misleading. Therefore, we\n do not write \"OUT \" for \"delayed\" responses.\n\n To simplify the code we do not write \"OUT \" for \"abort\" and \n \"error\" responses, too, as their size is always zero.\n\n- Lars"},{"id":"323289","messageId":"xmqqzicu4lnq.fsf@gitster.mtv.corp.google.com","threadId":"46258","inReplyTo":"20170625182125.6741-7-larsxschneider@gmail.com","subject":"Re: [PATCH v6 6/6] convert: add \"status=delayed\" to filter process protocol","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-26T19:02:49Z","receivedAt":"2017-06-26T19:02:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n> Some `clean` / `smudge` filters might require a significant amount of\n> time to process a single blob (e.g. the Git LFS smudge filter might\n> perform network requests). During this process the Git checkout\n> operation is blocked and Git needs to wait until the filter is done to\n> continue with the checkout.\n\nGood motivation. I'd say s/might/may/ to stress the fact that this\nis addressing a real-world problem, i.e. some filters incur network\nlatencies.\n\n> Teach the filter process protocol (introduced in edcc858) to accept the\n\nWhen referring to an old commit, we recommend this format\n\n edcc8581 (\"convert: add filter..process option\", 2016-10-16)\n\n(with or without dq around the title) which helps readers by telling\nthem how old the thing is and what it was about.\n\n> @@ -512,12 +512,69 @@ the protocol then Git will stop the filter process and restart it\n> with the next file that needs to be processed. Depending on the\n> `filter..required` flag Git will interpret that as error.\n> \n> -After the filter has processed a blob it is expected to wait for\n> -the next \"key=value\" list containing a command. Git will close\n> +After the filter has processed a command it is expected to wait for\n> +a \"key=value\" list containing the next command. Git will close\n\nGood generalization.\n\n> +Delay\n> +^^^^^\n> +\n> +If the filter supports the \"delay\" capability, then Git can send the\n> +flag \"can-delay\" after the filter command and pathname. This flag\n> +denotes that the filter can delay filtering the current blob (e.g. to\n> +compensate network latencies) by responding with no content but with\n> +the status \"delayed\" and a flush packet.\n> +------------------------\n> +packet: git> command=smudge\n> +packet: git> pathname=path/testfile.dat\n> +packet: git> can-delay=1\n> +packet: git> 0000\n> +packet: git> CONTENT\n> +packet: git> 0000\n> +packet: git< status=delayed\n> +packet: git< 0000\n> +------------------------\n> +\n> +If the filter supports the \"delay\" capability then it must support the\n> +\"list_available_blobs\" command. If Git sends this command, then the\n> +filter is expected to return a list of pathnames of blobs that are\n> +available. The list must be terminated with a flush packet followed\n\nIs it correct to read the above \"pathnames of blobs that are\navailble\" as \"pathnames, among which Git earlier requested to be\nfiltered with \"can-delay=1\", for which the filtered result is\nready\"? I do not mean to suggest this awkward wording to replace\nyours, but I found yours a bit too fuzzy for first time readers.\n\nPerhaps my brain has rotten by hearing the \"delayed/lazy transfer of\nlarge blobs\", but it went \"Huh?\" upon seeing \"...are available\".\n\n> +by a \"success\" status that is also terminated with a flush packet. If\n> +no blobs for the delayed paths are available, yet, then the filter is\n> +expected to block the response until at least one blob becomes\n> +available.\n\nAhh, this is better, at least you use \"the delayed paths\".\n\nWhat if the result never gets available (e.g. resulted in an error)?\nOr is it considered \"we _now_ know the result is an error\" and such\na delayed path that failed to retrieve from LFS store \"available\" at\nthat point?\n\nIt may be too late to raise to a series that went to v6, but this\nsmells more like \"ready\" not \"available\" to me.\n\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index a6b2af39d3..c1a256df8d 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -376,6 +376,8 @@ static int checkout_paths(const struct checkout_opts *opts,\n> \tstate.force = 1;\n> \tstate.refresh_cache = 1;\n> \tstate.istate = &the_index;\n> +\n> +\tenable_delayed_checkout(&state);\n> \tfor (pos = 0; pos < active_nr; pos++) {\n> \t\tstruct cache_entry *ce = active_cache[pos];\n> \t\tif (ce->ce_flags & CE_MATCHED) {\n> @@ -390,6 +392,7 @@ static int checkout_paths(const struct checkout_opts *opts,\n> \t\t\tpos = skip_same_name(ce, pos) - 1;\n> \t\t}\n> \t}\n> +\terrs |= finish_delayed_checkout(&state);\n\nOK.\n\n\n> \tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n> \t\tdie(_(\"unable to write new index file\"));\n\n> diff --git a/cache.h b/cache.h\n> index ae4c45d379..2ec12c4477 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1551,8 +1552,11 @@ struct checkout {\n> };\n> #define CHECKOUT_INIT { NULL, \"\" }\n> \n> +\n> #define TEMPORARY_FILENAME_LENGTH 25\n\nYou probably do not need that new blank line.\n\n> extern int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *topath);\n> +extern void enable_delayed_checkout(struct checkout *state);\n> +extern int finish_delayed_checkout(struct checkout *state);\n\nOK.\n\n> diff --git a/convert.c b/convert.c\n> index e55c034d86..6452ab546a 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -533,7 +534,8 @@ static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)\n> \tif (err)\n> \t\tgoto done;\n> \n> -\terr = packet_writel(process->in, \"capability=clean\", \"capability=smudge\", NULL);\n> +\terr = packet_writel(process->in,\n> +\t\t\"capability=clean\", \"capability=smudge\", \"capability=delay\", NULL);\n> \n> \tfor (;;) {\n> \t\tcap_buf = packet_read_line(process->out, NULL);\n> @@ -549,6 +551,8 @@ static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)\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 if (!strcmp(cap_name, \"delay\")) {\n> +\t\t\tentry->supported_capabilities |= CAP_DELAY;\n> \t\t} else {\n> \t\t\twarning(\n> \t\t\t\t\"external filter '%s' requested unsupported filter capability '%s'\",\n\nThe above makes me wonder if we want to introduce a table, e.g.\n\n\tstatic const struct {\n\t\tconst *name;\n\t\tunsigned cap;\n\t} known_caps[] = {\n\t\t{ \"clean\", CAP_CLEAN },\n\t\t{ \"smudge\", CAP_SMUDGE },\n\t\t{ \"delay\", CAP_DELAY },\n\t};\n\nand drive everything (i.e. capability announcement in hunk +534,8\nand parsing of request in hunk +551,8) off of it.\n\nThat would be overkill for two capabilities, but adding another to\nmake it three is when such a refactoring starts to become plausible.\n\n> @@ -590,9 +594,11 @@ static void handle_filter_error(const struct strbuf *filter_status,\n> \n> static int apply_multi_file_filter(const char *path, const char *src, size_t len,\n> \t\t\t\t int fd, struct strbuf *dst, const char *cmd,\n> -\t\t\t\t const unsigned int wanted_capability)\n> +\t\t\t\t const unsigned int wanted_capability,\n> +\t\t\t\t struct delayed_checkout *dco)\n> {\n> \tint err;\n> +\tint can_delay = 0;\n> \tstruct cmd2process *entry;\n> \tstruct child_process *process;\n> \tstruct strbuf nbuf = STRBUF_INIT;\n> @@ -647,6 +653,14 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n> \tif (err)\n> \t\tgoto done;\n> \n> +\tif ((entry->supported_capabilities & CAP_DELAY) &&\n> +\t dco && dco->state == CE_CAN_DELAY) {\n> +\t\tcan_delay = 1;\n> +\t\terr = packet_write_fmt_gently(process->in, \"can-delay=1\\n\");\n> +\t\tif (err)\n> +\t\t\tgoto done;\n> +\t}\n> +\n> \terr = packet_flush_gently(process->in);\n> \tif (err)\n> \t\tgoto done;\n> @@ -662,14 +676,74 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n> \tif (err)\n> \t\tgoto done;\n> \n> -\terr = strcmp(filter_status.buf, \"success\");\n> +\tif (can_delay && !strcmp(filter_status.buf, \"delayed\")) {\n> +\t\tdco->state = CE_DELAYED;\n> +\t\tstring_list_insert(&dco->filters, cmd);\n> +\t\tstring_list_insert(&dco->paths, path);\n\nThe dco->state CE_CAN_DELAY is global to all the paths involved in\nthis checkout, but filter_status.buf being \"delayed\" is per path\n(i.e. the filter can be told \"can-delay=1\" but still respond with\nsuccess right away), and in such a case dco->state will not advance\nfrom CE_CAN_DELAY to CE_DELAYED (which in turn means we will show\n\"can-delay=1\" to the next path). On the other hand, once we are\nresponded by \"delayed\" after sending \"can-delay=1\", we will go into\nCE_DELAYED state and will not say \"can-delay=1\" to any subsequent\npath.\n\nThis feels somewhat unsatisfactory. From the protocol exchange\ndescription in the documentation part of this patch, I was sort of\nexpecting that Git can selectively say \"You can delay response to\nthis path\" and \"I am willing to wait until you give the filtered\nresult\" for each command/pathname pair. But this code structure\ndoes not seem to allow that.\n\nI would understand it if CE_CAN_DELAY and CE_DELAYED are unified,\nand the assignment to dco->state here is removed. You may be using\nthese two states in other parts of the code to optimize between the\ncase where *no* delayed path exists (even if Git is capable of\ndelaying) and *some* delayed path exist, but I think that can be\ndone by checking if dco->paths is empty, or something like that.\n\nSuch a change will allow us later to add more logic (perhaps driven\nby attributes) to the decision to send \"can-delay=1\" in hunk +653,14\nabove, to express \"this won't be delayed\".\n\nAlternatively, the semantics of \"the entire checkout exchange is\nallowed to be delayed\" can be kept but then the protocol looks too\nconfusing---it shouldn't have \"can-delay=1\" after a command/pathname\npair, as if it can be specified per path.\n\n> +\t} else {\n> +\t\t/* The filter got the blob and wants to send us a response. */\n> +\t\terr = strcmp(filter_status.buf, \"success\");\n> +\t\tif (err)\n> +\t\t\tgoto done;\n> +\n> +\t\terr = read_packetized_to_strbuf(process->out, &nbuf) < 0;\n> +\t\tif (err)\n> +\t\t\tgoto done;\n> +\n> +\t\terr = subprocess_read_status(process->out, &filter_status);\n> +\t\tif (err)\n> +\t\t\tgoto done;\n> +\n> +\t\terr = strcmp(filter_status.buf, \"success\");\n> +\t}\n> +\n> +done:\n> +\tsigchain_pop(SIGPIPE);\n> +\n> +\tif (err)\n> +\t\thandle_filter_error(&filter_status, entry, wanted_capability);\n> +\telse\n> +\t\tstrbuf_swap(dst, &nbuf);\n> +\tstrbuf_release(&nbuf);\n> +\treturn !err;\n> +}\n\nOK.\n\n> +int async_query_available_blobs(const char *cmd, struct string_list *delayed_paths)\n> +{\n> +\tint err;\n> +\tchar *line;\n> +\tstruct cmd2process *entry;\n> +\tstruct child_process *process;\n> +\tstruct strbuf filter_status = STRBUF_INIT;\n> +\n> +\tassert(subprocess_map_initialized);\n> +\tentry = (struct cmd2process *)subprocess_find_entry(&subprocess_map, cmd);\n> +\tif (!entry) {\n> +\t\terror(\"external filter '%s' is not available anymore although \"\n> +\t\t \"not all paths have been filtered\", cmd);\n> +\t\treturn 0;\n> +\t}\n> +\tprocess = &entry->subprocess.process;\n> +\tsigchain_push(SIGPIPE, SIG_IGN);\n> +\n> +\terr = packet_write_fmt_gently(\n> +\t\tprocess->in, \"command=list_available_blobs\\n\");\n> \tif (err)\n> \t\tgoto done;\n> \n> -\terr = read_packetized_to_strbuf(process->out, &nbuf) < 0;\n> +\terr = packet_flush_gently(process->in);\n> \tif (err)\n> \t\tgoto done;\n> \n> +\twhile ((line = packet_read_line(process->out, NULL))) {\n> +\t\tconst char *path;\n> +\t\tif (skip_prefix(line, \"pathname=\", &path))\n> +\t\t\tstring_list_insert(delayed_paths, xstrdup(path));\n\nI am assuming that the caller passes an empty string list to\ndelayed_paths variable (shouldn't it be called available_paths, or\nready_paths if we follow the earlier discussion above, by the way?),\nand it will compare the resulting set with the set of paths that got\n\"delayed\" response in the apply_multi_file_filter() function earlier.\n\nI am wondering whose responsibility it will be to deal with a path\n\"list-available\" reports that are *not* asked by Git or Git got no\n\"delayed\" response. The list subtraction done by the caller is\nprobably the logical place to do so.\n\n> diff --git a/entry.c b/entry.c\n> index d6b263f78e..c6d5cc01dc 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -137,6 +137,85 @@ static int streaming_write_entry(const struct cache_entry *ce, char *path,\n> ...\n> +static int remove_available_paths(struct string_list_item *item, void *cb_data)\n> +{\n> +\tstruct string_list *available_paths = cb_data;\n> +\treturn !string_list_has_string(available_paths, item->string);\n> +}\n> +\n> +int finish_delayed_checkout(struct checkout *state)\n> +{\n> +\tint errs = 0;\n> +\tstruct string_list_item *filter, *path;\n> +\tstruct delayed_checkout *dco = state->delayed_checkout;\n> +\n> +\tif (!state->delayed_checkout)\n> +\t\treturn errs;\n> +\n> +\tdco->state = CE_RETRY;\n> +\twhile (dco->filters.nr > 0) {\n> +\t\tfor_each_string_list_item(filter, &dco->filters) {\n> +\t\t\tstruct string_list available_paths;\n> +\t\t\tstring_list_init(&available_paths, 0);\n\nSTRING_LIST_INIT_NODUP?\n\n> +\n> +\t\t\tif (!async_query_available_blobs(filter->string, &available_paths)) {\n> +\t\t\t\t/* Filter reported an error */\n> +\t\t\t\terrs = 1;\n> +\t\t\t\tfilter->string = \"\";\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> +\t\t\tif (available_paths.nr <= 0) {\n> +\t\t\t\t/* Filter responded with no entries. That means\n> +\t\t\t\t the filter is done and we can remove the\n> +\t\t\t\t filter from the list (see\n> +\t\t\t\t \"string_list_remove_empty_items\" call below).\n> +\t\t\t\t*/\n\n\t/*\n * Our multi-line comments\n\t * look like this.\n\t */\n\n> +\t\t\t\tfilter->string = \"\";\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> +\n> +\t\t\t/* In dco->paths we store a list of all delayed paths.\n> +\t\t\t The filter just send us a list of available paths.\n> +\t\t\t Remove them from the list.\n> +\t\t\t*/\n> +\t\t\tfilter_string_list(&dco->paths, 0,\n> +\t\t\t\t&remove_available_paths, &available_paths);\n\nWe first remove from the outstanding request list (dco->paths) what\nare now ready...\n\n> +\t\t\tfor_each_string_list_item(path, &available_paths) {\n\n...and go over those paths that are now ready.\n\n> +\t\t\t\tstruct cache_entry* ce = index_file_exists(\n> +\t\t\t\t\tstate->istate, path->string,\n> +\t\t\t\t\tstrlen(path->string), 0);\n> +\t\t\t\tassert(dco->state == CE_RETRY);\n> +\t\t\t\terrs |= (ce ? checkout_entry(ce, state, NULL) : 1);\n> +\t\t\t}\n\nBut we never checked if the contents of this available_paths list is\na subset of the set of paths we originally asked the external\nprocess to filter. This will allow the process to overwrite any\nrandom path that is not even involved in the checkout.\n\n> +\t\t}\n> +\t\tstring_list_remove_empty_items(&dco->filters, 0);\n> +\t}\n> +\tstring_list_clear(&dco->filters, 0);\n> +\n> +\t/* At this point we should not have any delayed paths anymore. */\n> +\terrs |= dco->paths.nr;\n> +\tfor_each_string_list_item(path, &dco->paths) {\n> +\t\twarning(_(\"%s was not filtered properly.\"), path->string);\n> +\t}\n> +\tstring_list_clear(&dco->paths, 0);\n\nAnd \"list-available\" that says \"path X is ready\" when we never asked\nfor X gets away free without detected as a bug, either.\n"},{"id":"323293","messageId":"F72755BA-6296-4C37-9EFA-4D7BCE9F1082@gmail.com","threadId":"46258","inReplyTo":"xmqqzicu4lnq.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6 6/6] convert: add \"status=delayed\" to filter process protocol","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-06-26T21:28:21Z","receivedAt":"2017-06-26T21:28:32Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 26 Jun 2017, at 21:02, Junio C Hamano wrote:\n> \n> Lars Schneider writes:\n> \n>> Some `clean` / `smudge` filters might require a significant amount of\n>> time to process a single blob (e.g. the Git LFS smudge filter might\n>> perform network requests). During this process the Git checkout\n>> operation is blocked and Git needs to wait until the filter is done to\n>> continue with the checkout.\n> \n> Good motivation. I'd say s/might/may/ to stress the fact that this\n> is addressing a real-world problem, i.e. some filters incur network\n> latencies.\n\nAgreed.\n\n\n>> Teach the filter process protocol (introduced in edcc858) to accept the\n> \n> When referring to an old commit, we recommend this format\n> \n> edcc8581 (\"convert: add filter..process option\", 2016-10-16)\n> \n> (with or without dq around the title) which helps readers by telling\n> them how old the thing is and what it was about.\n\nAgreed.\n\n\n> ...\n> \n>> +Delay\n>> +^^^^^\n>> +\n>> +If the filter supports the \"delay\" capability, then Git can send the\n>> +flag \"can-delay\" after the filter command and pathname. This flag\n>> +denotes that the filter can delay filtering the current blob (e.g. to\n>> +compensate network latencies) by responding with no content but with\n>> +the status \"delayed\" and a flush packet.\n>> +------------------------\n>> +packet: git> command=smudge\n>> +packet: git> pathname=path/testfile.dat\n>> +packet: git> can-delay=1\n>> +packet: git> 0000\n>> +packet: git> CONTENT\n>> +packet: git> 0000\n>> +packet: git< status=delayed\n>> +packet: git< 0000\n>> +------------------------\n>> +\n>> +If the filter supports the \"delay\" capability then it must support the\n>> +\"list_available_blobs\" command. If Git sends this command, then the\n>> +filter is expected to return a list of pathnames of blobs that are\n>> +available. The list must be terminated with a flush packet followed\n> \n> Is it correct to read the above \"pathnames of blobs that are\n> availble\" as \"pathnames, among which Git earlier requested to be\n> filtered with \"can-delay=1\", for which the filtered result is\n> ready\"? I do not mean to suggest this awkward wording to replace\n> yours, but I found yours a bit too fuzzy for first time readers.\n> \n> Perhaps my brain has rotten by hearing the \"delayed/lazy transfer of\n> large blobs\", but it went \"Huh?\" upon seeing \"...are available\".\n\nMaybe this?\n [...] If Git sends this command, then the\n filter is expected to return a list of pathnames representing blobs \n that have been delayed earlier and are now available. [...]\n\n\n>> +by a \"success\" status that is also terminated with a flush packet. If\n>> +no blobs for the delayed paths are available, yet, then the filter is\n>> +expected to block the response until at least one blob becomes\n>> +available.\n> \n> Ahh, this is better, at least you use \"the delayed paths\".\n> \n> What if the result never gets available (e.g. resulted in an error)?\n\nAs soon as the filter responds with an empty list, Git stops asking.\nAll blobs that Git has not received at this point are considered an\nerror.\n\nShould I mention that explicitly?\n\n\n> Or is it considered \"we _now_ know the result is an error\" and such\n> a delayed path that failed to retrieve from LFS store \"available\" at\n> that point?\n\nNo. \"list_available_blobs\" only returns blobs that are immediately\navailable. Errors are not available.\n\n\n> It may be too late to raise to a series that went to v6, but this\n> smells more like \"ready\" not \"available\" to me.\n\nYou mean you would call it \"list_ready_blobs\"? I am no native speaker\nbut I feel \"available\" sounds better. I also contemplated \"retrievable\".\n\nI think I understand your reasoning, though. In the case of GitLFS all these \nblobs are \"available\". Only the ones that GitLFS has downloaded are \nready, though. However, other filters might delay blobs for non-network\nrelated reasons and then \"available\" and \"ready\" would be the same, no?\n\nI would like to keep \"available\".\n\n> ...\n\n>> diff --git a/cache.h b/cache.h\n>> index ae4c45d379..2ec12c4477 100644\n>> --- a/cache.h\n>> +++ b/cache.h\n>> @@ -1551,8 +1552,11 @@ struct checkout {\n>> };\n>> #define CHECKOUT_INIT { NULL, \"\" }\n>> \n>> +\n>> #define TEMPORARY_FILENAME_LENGTH 25\n> \n> You probably do not need that new blank line.\n\nAgreed! I have no idea why/how that got in.\n\n\n> ...\n>> diff --git a/convert.c b/convert.c\n>> index e55c034d86..6452ab546a 100644\n>> --- a/convert.c\n>> +++ b/convert.c\n>> @@ -533,7 +534,8 @@ static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)\n>> \tif (err)\n>> \t\tgoto done;\n>> \n>> -\terr = packet_writel(process->in, \"capability=clean\", \"capability=smudge\", NULL);\n>> +\terr = packet_writel(process->in,\n>> +\t\t\"capability=clean\", \"capability=smudge\", \"capability=delay\", NULL);\n>> \n>> \tfor (;;) {\n>> \t\tcap_buf = packet_read_line(process->out, NULL);\n>> @@ -549,6 +551,8 @@ static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)\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 if (!strcmp(cap_name, \"delay\")) {\n>> +\t\t\tentry->supported_capabilities |= CAP_DELAY;\n>> \t\t} else {\n>> \t\t\twarning(\n>> \t\t\t\t\"external filter '%s' requested unsupported filter capability '%s'\",\n> \n> The above makes me wonder if we want to introduce a table, e.g.\n> \n> \tstatic const struct {\n> \t\tconst *name;\n> \t\tunsigned cap;\n> \t} known_caps[] = {\n> \t\t{ \"clean\", CAP_CLEAN },\n> \t\t{ \"smudge\", CAP_SMUDGE },\n> \t\t{ \"delay\", CAP_DELAY },\n> \t};\n> \n> and drive everything (i.e. capability announcement in hunk +534,8\n> and parsing of request in hunk +551,8) off of it.\n> \n> That would be overkill for two capabilities, but adding another to\n> make it three is when such a refactoring starts to become plausible.\n\nI agree with this way for many capabilities. I'll check if it makes\nsense for 3.\n\n> \n>> @@ -590,9 +594,11 @@ static void handle_filter_error(const struct strbuf *filter_status,\n>> \n>> static int apply_multi_file_filter(const char *path, const char *src, size_t len,\n>> \t\t\t\t int fd, struct strbuf *dst, const char *cmd,\n>> -\t\t\t\t const unsigned int wanted_capability)\n>> +\t\t\t\t const unsigned int wanted_capability,\n>> +\t\t\t\t struct delayed_checkout *dco)\n>> {\n>> \tint err;\n>> +\tint can_delay = 0;\n>> \tstruct cmd2process *entry;\n>> \tstruct child_process *process;\n>> \tstruct strbuf nbuf = STRBUF_INIT;\n>> @@ -647,6 +653,14 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n>> \tif (err)\n>> \t\tgoto done;\n>> \n>> +\tif ((entry->supported_capabilities & CAP_DELAY) &&\n>> +\t dco && dco->state == CE_CAN_DELAY) {\n>> +\t\tcan_delay = 1;\n>> +\t\terr = packet_write_fmt_gently(process->in, \"can-delay=1\\n\");\n>> +\t\tif (err)\n>> +\t\t\tgoto done;\n>> +\t}\n>> +\n>> \terr = packet_flush_gently(process->in);\n>> \tif (err)\n>> \t\tgoto done;\n>> @@ -662,14 +676,74 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n>> \tif (err)\n>> \t\tgoto done;\n>> \n>> -\terr = strcmp(filter_status.buf, \"success\");\n>> +\tif (can_delay && !strcmp(filter_status.buf, \"delayed\")) {\n>> +\t\tdco->state = CE_DELAYED;\n>> +\t\tstring_list_insert(&dco->filters, cmd);\n>> +\t\tstring_list_insert(&dco->paths, path);\n> \n> The dco->state CE_CAN_DELAY is global to all the paths involved in\n> this checkout, but filter_status.buf being \"delayed\" is per path\n> (i.e. the filter can be told \"can-delay=1\" but still respond with\n> success right away), and in such a case dco->state will not advance\n> from CE_CAN_DELAY to CE_DELAYED (which in turn means we will show\n> \"can-delay=1\" to the next path). On the other hand, once we are\n> responded by \"delayed\" after sending \"can-delay=1\", we will go into\n> CE_DELAYED state and will not say \"can-delay=1\" to any subsequent\n> path.\n\nI am not 100% happy with design of \"dco->state\". You are right, I use\nit as \"global\" flag to indicate if paths can be delayed. But I *also*\nuse it as \"flag\" to indicate if a certain path has been delayed. \nAfterwards it is reset (see entry.c:write_entry).\nPeff stumbled over that as well [1]. I need to change that!\n\nMaybe with an additional flag \"dco->delayed\". This flag is reset\nbefore any \"async_convert_to_working_tree\" and evaluated after. \n\n[1] \"Hmm. This \"reset the state\" bit at the end surprised me....\"\nhttp://public-inbox.org/git/20170624141941.usy2pyhid3jrf3ku@sigill.intra.peff.net/\n\n\n> This feels somewhat unsatisfactory. From the protocol exchange\n> description in the documentation part of this patch, I was sort of\n> expecting that Git can selectively say \"You can delay response to\n> this path\" and \"I am willing to wait until you give the filtered\n> result\" for each command/pathname pair. But this code structure\n> does not seem to allow that.\n> \n> I would understand it if CE_CAN_DELAY and CE_DELAYED are unified,\n> and the assignment to dco->state here is removed. You may be using\n> these two states in other parts of the code to optimize between the\n> case where *no* delayed path exists (even if Git is capable of\n> delaying) and *some* delayed path exist, but I think that can be\n> done by checking if dco->paths is empty, or something like that.\n> \n> Such a change will allow us later to add more logic (perhaps driven\n> by attributes) to the decision to send \"can-delay=1\" in hunk +653,14\n> above, to express \"this won't be delayed\".\n> \n> Alternatively, the semantics of \"the entire checkout exchange is\n> allowed to be delayed\" can be kept but then the protocol looks too\n> confusing---it shouldn't have \"can-delay=1\" after a command/pathname\n> pair, as if it can be specified per path.\n\nActually a delay \"per path\" is possible. I'll try to make this more\nclear with a little refactoring.\n\n\n> ...\n> \n>> +int async_query_available_blobs(const char *cmd, struct string_list *delayed_paths)\n>> +{\n>> +\tint err;\n>> +\tchar *line;\n>> +\tstruct cmd2process *entry;\n>> +\tstruct child_process *process;\n>> +\tstruct strbuf filter_status = STRBUF_INIT;\n>> +\n>> +\tassert(subprocess_map_initialized);\n>> +\tentry = (struct cmd2process *)subprocess_find_entry(&subprocess_map, cmd);\n>> +\tif (!entry) {\n>> +\t\terror(\"external filter '%s' is not available anymore although \"\n>> +\t\t \"not all paths have been filtered\", cmd);\n>> +\t\treturn 0;\n>> +\t}\n>> +\tprocess = &entry->subprocess.process;\n>> +\tsigchain_push(SIGPIPE, SIG_IGN);\n>> +\n>> +\terr = packet_write_fmt_gently(\n>> +\t\tprocess->in, \"command=list_available_blobs\\n\");\n>> \tif (err)\n>> \t\tgoto done;\n>> \n>> -\terr = read_packetized_to_strbuf(process->out, &nbuf) < 0;\n>> +\terr = packet_flush_gently(process->in);\n>> \tif (err)\n>> \t\tgoto done;\n>> \n>> +\twhile ((line = packet_read_line(process->out, NULL))) {\n>> +\t\tconst char *path;\n>> +\t\tif (skip_prefix(line, \"pathname=\", &path))\n>> +\t\t\tstring_list_insert(delayed_paths, xstrdup(path));\n> \n> I am assuming that the caller passes an empty string list to\n> delayed_paths variable (shouldn't it be called available_paths, or\n> ready_paths if we follow the earlier discussion above, by the way?),\n> and it will compare the resulting set with the set of paths that got\n> \"delayed\" response in the apply_multi_file_filter() function earlier.\n\nCorrect. I also agree with the rename although it seems a bit odd to \nname the function \"...available_blobs\" and the variable \"available_paths\".\nThe paths itself are not \"available\" anyways. They just represent\nthe blobs that are available. Therefore it should be OK and it probably\nbetter than just \"paths\".\n\n\n> I am wondering whose responsibility it will be to deal with a path\n> \"list-available\" reports that are *not* asked by Git or Git got no\n> \"delayed\" response. The list subtraction done by the caller is\n> probably the logical place to do so.\n\nCorrect. Git (the caller) will notice that not all \"delayed\" paths\nare listed by the filter and throw an error at the end.\n\n> \n>> diff --git a/entry.c b/entry.c\n>> index d6b263f78e..c6d5cc01dc 100644\n>> --- a/entry.c\n>> +++ b/entry.c\n>> @@ -137,6 +137,85 @@ static int streaming_write_entry(const struct cache_entry *ce, char *path,\n>> ...\n>> +static int remove_available_paths(struct string_list_item *item, void *cb_data)\n>> +{\n>> +\tstruct string_list *available_paths = cb_data;\n>> +\treturn !string_list_has_string(available_paths, item->string);\n>> +}\n>> +\n>> +int finish_delayed_checkout(struct checkout *state)\n>> +{\n>> +\tint errs = 0;\n>> +\tstruct string_list_item *filter, *path;\n>> +\tstruct delayed_checkout *dco = state->delayed_checkout;\n>> +\n>> +\tif (!state->delayed_checkout)\n>> +\t\treturn errs;\n>> +\n>> +\tdco->state = CE_RETRY;\n>> +\twhile (dco->filters.nr > 0) {\n>> +\t\tfor_each_string_list_item(filter, &dco->filters) {\n>> +\t\t\tstruct string_list available_paths;\n>> +\t\t\tstring_list_init(&available_paths, 0);\n> \n> STRING_LIST_INIT_NODUP?\n\nOf course!\n\n> \n>> +\n>> +\t\t\tif (!async_query_available_blobs(filter->string, &available_paths)) {\n>> +\t\t\t\t/* Filter reported an error */\n>> +\t\t\t\terrs = 1;\n>> +\t\t\t\tfilter->string = \"\";\n>> +\t\t\t\tcontinue;\n>> +\t\t\t}\n>> +\t\t\tif (available_paths.nr <= 0) {\n>> +\t\t\t\t/* Filter responded with no entries. That means\n>> +\t\t\t\t the filter is done and we can remove the\n>> +\t\t\t\t filter from the list (see\n>> +\t\t\t\t \"string_list_remove_empty_items\" call below).\n>> +\t\t\t\t*/\n> \n> \t/*\n> * Our multi-line comments\n> \t * look like this.\n> \t */\n\nOops. Will fix.\n\nBTW: In [1] you mentioned that you run checkpatch. Would checkpatch\ncatch this kind of error? If yes, is your version publicly available?\n\n[1] http://public-inbox.org/git/xmqq1ste2zwu.fsf@gitster.mtv.corp.google.com/\n\n\n>> +\t\t\t\tfilter->string = \"\";\n>> +\t\t\t\tcontinue;\n>> +\t\t\t}\n>> +\n>> +\t\t\t/* In dco->paths we store a list of all delayed paths.\n>> +\t\t\t The filter just send us a list of available paths.\n>> +\t\t\t Remove them from the list.\n>> +\t\t\t*/\n>> +\t\t\tfilter_string_list(&dco->paths, 0,\n>> +\t\t\t\t&remove_available_paths, &available_paths);\n> \n> We first remove from the outstanding request list (dco->paths) what\n> are now ready...\n> \n>> +\t\t\tfor_each_string_list_item(path, &available_paths) {\n> \n> ...and go over those paths that are now ready.\n> \n>> +\t\t\t\tstruct cache_entry* ce = index_file_exists(\n>> +\t\t\t\t\tstate->istate, path->string,\n>> +\t\t\t\t\tstrlen(path->string), 0);\n>> +\t\t\t\tassert(dco->state == CE_RETRY);\n>> +\t\t\t\terrs |= (ce ? checkout_entry(ce, state, NULL) : 1);\n>> +\t\t\t}\n> \n> But we never checked if the contents of this available_paths list is\n> a subset of the set of paths we originally asked the external\n> process to filter.\n\nCorrect.\n\n> This will allow the process to overwrite any\n> random path that is not even involved in the checkout.\n\nNo, not \"any random path\". Only paths that are part of the checkout.\nThere are three cases:\n\n(1) available_path is a path that was delayed before (= happy case!)\n(2) available_path is a path that was not delayed before, \n but filtered (= no problem, as filtering is a idempotent operation)\n(3) available_path is a path that was neither delayed nor filtered\n before (= if the filter returns the blob as-is then this would\n be no problem. otherwise we would indeed have a screwed checkout)\n\nCase 3 might introduce a problem if the filter is buggy. \n \nWould you be OK with this check to catch case 3?\n\n dco_path_count = dco->paths.nr;\n filter_string_list(&dco->paths, 0,\n &remove_available_paths, &available_paths);\n\n if (dco_path_count - dco->paths.nr != available_paths.nr) {\n /* The filter responded with entries that have not\n * been delay earlier. Do not ask the filter\n * for available blobs, again, as the filter is\n * likely buggy. This will generate an error at \n * the end as some files are not filtered properly.\n */\n filter->string = \"\";\n error(_(\"The external filter '%s' responded with \"\n \"available blobs which have not been delayed \"\n \"earlier.\"), filter->string);\n continue;\n }\n\n\n>> +\t\t}\n>> +\t\tstring_list_remove_empty_items(&dco->filters, 0);\n>> +\t}\n>> +\tstring_list_clear(&dco->filters, 0);\n>> +\n>> +\t/* At this point we should not have any delayed paths anymore. */\n>> +\terrs |= dco->paths.nr;\n>> +\tfor_each_string_list_item(path, &dco->paths) {\n>> +\t\twarning(_(\"%s was not filtered properly.\"), path->string);\n>> +\t}\n>> +\tstring_list_clear(&dco->paths, 0);\n> \n> And \"list-available\" that says \"path X is ready\" when we never asked\n> for X gets away free without detected as a bug, either.\n\nWith the addition above it would!\n\n\nThanks for the review :-)\n\n- Lars"},{"id":"323295","messageId":"xmqqa84u4cuk.fsf@gitster.mtv.corp.google.com","threadId":"46258","inReplyTo":"F72755BA-6296-4C37-9EFA-4D7BCE9F1082@gmail.com","subject":"Re: [PATCH v6 6/6] convert: add \"status=delayed\" to filter process protocol","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-26T22:13:07Z","receivedAt":"2017-06-26T22:13:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n> Maybe this?\n> [...] If Git sends this command, then the\n> filter is expected to return a list of pathnames representing blobs \n> that have been delayed earlier and are now available. [...]\n\nOK.\n\n>>> +by a \"success\" status that is also terminated with a flush packet. If\n>>> +no blobs for the delayed paths are available, yet, then the filter is\n>>> +expected to block the response until at least one blob becomes\n>>> +available.\n>> \n>> Ahh, this is better, at least you use \"the delayed paths\".\n>> \n>> What if the result never gets available (e.g. resulted in an error)?\n>\n> As soon as the filter responds with an empty list, Git stops asking.\n> All blobs that Git has not received at this point are considered an\n> error.\n>\n> Should I mention that explicitly?\n\nOtherwise I wouldn't have wondered \"what if\".\n\n>> I am wondering whose responsibility it will be to deal with a path\n>> \"list-available\" reports that are *not* asked by Git or Git got no\n>> \"delayed\" response. The list subtraction done by the caller is\n>> probably the logical place to do so.\n>\n> Correct. Git (the caller) will notice that not all \"delayed\" paths\n> are listed by the filter and throw an error at the end.\n\nI am wondering about the other case. Git didn't ask for a path to\nbe filtered at all, but the filter sneaked in a path that happens to\nin the index in its response---Git should at least ignore it, and\nbetter yet, diagnose it as an error in the filter process.\n\n>>> +\t\t\t\tfilter->string = \"\";\n>>> +\t\t\t\tcontinue;\n>>> +\t\t\t}\n>>> +\n>>> +\t\t\t/* In dco->paths we store a list of all delayed paths.\n>>> +\t\t\t The filter just send us a list of available paths.\n>>> +\t\t\t Remove them from the list.\n>>> +\t\t\t*/\n>>> +\t\t\tfilter_string_list(&dco->paths, 0,\n>>> +\t\t\t\t&remove_available_paths, &available_paths);\n>> \n>> We first remove from the outstanding request list (dco->paths) what\n>> are now ready...\n>> \n>>> +\t\t\tfor_each_string_list_item(path, &available_paths) {\n>> \n>> ...and go over those paths that are now ready.\n>> \n>>> +\t\t\t\tstruct cache_entry* ce = index_file_exists(\n>>> +\t\t\t\t\tstate->istate, path->string,\n>>> +\t\t\t\t\tstrlen(path->string), 0);\n>>> +\t\t\t\tassert(dco->state == CE_RETRY);\n>>> +\t\t\t\terrs |= (ce ? checkout_entry(ce, state, NULL) : 1);\n>>> +\t\t\t}\n>> \n>> But we never checked if the contents of this available_paths list is\n>> a subset of the set of paths we originally asked the external\n>> process to filter.\n>\n> Correct.\n>\n>> This will allow the process to overwrite any\n>> random path that is not even involved in the checkout.\n>\n> No, not \"any random path\". Only paths that are part of the checkout.\n\nIsn't it \"any path that index_file_exists() returns a CE for\". Did\nyou filter out elements in available_paths that weren't part of\ndco->paths? I thought the filter-string-list you have are for the\nother way around (which is necessary to keep track of work to be\ndone, but that filtering does not help rejecting rogue responses at\nall).\n\n> There are three cases:\n>\n> (1) available_path is a path that was delayed before (= happy case!)\n> (2) available_path is a path that was not delayed before, \n> but filtered (= no problem, as filtering is a idempotent operation)\n> (3) available_path is a path that was neither delayed nor filtered\n> before (= if the filter returns the blob as-is then this would\n> be no problem. otherwise we would indeed have a screwed checkout)\n>\n> Case 3 might introduce a problem if the filter is buggy. \n\n> Would you be OK with this check to catch case 3?\n\nI'd be very suspicious about anything you would do only with .nr\nfield, without filtering the other way around. After all, we may\nhave asked it for 3 paths to be filtered, and it may have answered\nwith its own 3 different paths.\n\n> dco_path_count = dco->paths.nr;\n> filter_string_list(&dco->paths, 0,\n> &remove_available_paths, &available_paths);\n>\n> if (dco_path_count - dco->paths.nr != available_paths.nr) {\n> /* The filter responded with entries that have not\n> * been delay earlier. Do not ask the filter\n> * for available blobs, again, as the filter is\n> * likely buggy. This will generate an error at \n> * the end as some files are not filtered properly.\n> */\n> filter->string = \"\";\n> error(_(\"The external filter '%s' responded with \"\n> \"available blobs which have not been delayed \"\n> \"earlier.\"), filter->string);\n> continue;\n> }\n>\n>\n>>> +\t\t}\n>>> +\t\tstring_list_remove_empty_items(&dco->filters, 0);\n>>> +\t}\n>>> +\tstring_list_clear(&dco->filters, 0);\n>>> +\n>>> +\t/* At this point we should not have any delayed paths anymore. */\n>>> +\terrs |= dco->paths.nr;\n>>> +\tfor_each_string_list_item(path, &dco->paths) {\n>>> +\t\twarning(_(\"%s was not filtered properly.\"), path->string);\n>>> +\t}\n>>> +\tstring_list_clear(&dco->paths, 0);\n>> \n>> And \"list-available\" that says \"path X is ready\" when we never asked\n>> for X gets away free without detected as a bug, either.\n>\n> With the addition above it would!\n>\n>\n> Thanks for the review :-)\n>\n> - Lars\n"},{"id":"323296","messageId":"xmqqzicu2x4a.fsf@gitster.mtv.corp.google.com","threadId":"46258","inReplyTo":"xmqqa84u4cuk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6 6/6] convert: add \"status=delayed\" to filter process protocol","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-26T22:38:13Z","receivedAt":"2017-06-26T22:38:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>>>> +\t\t\t\tfilter->string = \"\";\n>>>> +\t\t\t\tcontinue;\n>>>> +\t\t\t}\n>>>> +\n>>>> +\t\t\t/* In dco->paths we store a list of all delayed paths.\n>>>> +\t\t\t The filter just send us a list of available paths.\n>>>> +\t\t\t Remove them from the list.\n>>>> +\t\t\t*/\n>>>> +\t\t\tfilter_string_list(&dco->paths, 0,\n>>>> +\t\t\t\t&remove_available_paths, &available_paths);\n>>> \n>>> We first remove from the outstanding request list (dco->paths) what\n>>> are now ready...\n>>> \n>>>> +\t\t\tfor_each_string_list_item(path, &available_paths) {\n>>> \n>>> ...and go over those paths that are now ready.\n>>> \n>>>> +\t\t\t\tstruct cache_entry* ce = index_file_exists(\n>>>> +\t\t\t\t\tstate->istate, path->string,\n>>>> +\t\t\t\t\tstrlen(path->string), 0);\n>>>> +\t\t\t\tassert(dco->state == CE_RETRY);\n>>>> +\t\t\t\terrs |= (ce ? checkout_entry(ce, state, NULL) : 1);\n>>>> +\t\t\t}\n>>> \n>>> But we never checked if the contents of this available_paths list is\n>>> a subset of the set of paths we originally asked the external\n>>> process to filter.\n>>\n>> Correct.\n>>\n>>> This will allow the process to overwrite any\n>>> random path that is not even involved in the checkout.\n>>\n>> No, not \"any random path\". Only paths that are part of the checkout.\n>\n> Isn't it \"any path that index_file_exists() returns a CE for\". Did\n> you filter out elements in available_paths that weren't part of\n> dco->paths? I thought the filter-string-list you have are for the\n> other way around (which is necessary to keep track of work to be\n> done, but that filtering does not help rejecting rogue responses at\n> all).\n\nThat is, something along the lines of this on top of the series.\nWhen going over the list of delayed paths to see if any of them is\nanswered, in remove_available_paths(), we mark the util field of the\nanswered one. Later when we go over the list of answered one, we\nmake sure that it was actually matched with something on dco->paths\nbefore calling checkout_entry().\n\n entry.c | 20 ++++++++++++++++----\n 1 file changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex c6d5cc01dc..f2af67e015 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -150,7 +150,12 @@ void enable_delayed_checkout(struct checkout *state)\n static int remove_available_paths(struct string_list_item *item, void *cb_data)\n {\n \tstruct string_list *available_paths = cb_data;\n-\treturn !string_list_has_string(available_paths, item->string);\n+\tstruct string_list_item *available;\n+\n+\tavailable = string_list_lookup(available_paths, item->string);\n+\tif (available)\n+\t\tavaiable->util = (void *)item->string;\n+\treturn !available;\n }\n \n int finish_delayed_checkout(struct checkout *state)\n@@ -192,9 +197,16 @@ int finish_delayed_checkout(struct checkout *state)\n \t\t\t\t&remove_available_paths, &available_paths);\n \n \t\t\tfor_each_string_list_item(path, &available_paths) {\n-\t\t\t\tstruct cache_entry* ce = index_file_exists(\n-\t\t\t\t\tstate->istate, path->string,\n-\t\t\t\t\tstrlen(path->string), 0);\n+\t\t\t\tstruct cache_entry* ce;\n+\n+\t\t\t\tif (!path->util) {\n+\t\t\t\t\terror(\"filter gave '%s' which was unasked for\",\n+\t\t\t\t\t path->string);\n+\t\t\t\t\terrs |= 1;\n+\t\t\t\t\tcontinue;\n+\t\t\t\t}\n+\t\t\t\tce = index_file_exists(state->istate, path->string,\n+\t\t\t\t\t\t strlen(path->string), 0);\n \t\t\t\tassert(dco->state == CE_RETRY);\n \t\t\t\terrs |= (ce ? checkout_entry(ce, state, NULL) : 1);\n \t\t\t}\n"},{"id":"323306","messageId":"CAGZ79kZEff1WUmgmzWKt=K8j45tOn6xy6ik8_CUp0awRep8jSw@mail.gmail.com","threadId":"46258","inReplyTo":"xmqqmv8u63aj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6 5/6] convert: move multiple file filter error handling to separate function","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-06-27T02:51:12Z","receivedAt":"2017-06-27T02:51:18Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Jun 26, 2017 at 10:56 AM, Junio C Hamano wrote:\n> Not about this patch, but viewing this with\n>\n> git show -w --color-moved=zebra\n>\n> gives an interesting result. The bulk of the part moved are\n> re-indented, and the comment string gets zebra stripes, as if the\n> line movement detection code does not realize that these came from\n> the same place.\n>\n\nThanks for the pointer, I'll include the whitespace-less comparison in tests.\n\nThanks,\nStefan\n"},{"id":"323320","messageId":"22F3047C-F2B2-4327-97B4-3C2347E1DD0E@gmail.com","threadId":"46258","inReplyTo":"xmqqa84u4cuk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6 6/6] convert: add \"status=delayed\" to filter process protocol","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-06-27T12:07:01Z","receivedAt":"2017-06-27T12:07:12Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 27 Jun 2017, at 00:13, Junio C Hamano wrote:\n> \n> Lars Schneider writes:\n> \n>> ...\n> \n>>> I am wondering whose responsibility it will be to deal with a path\n>>> \"list-available\" reports that are *not* asked by Git or Git got no\n>>> \"delayed\" response. The list subtraction done by the caller is\n>>> probably the logical place to do so.\n>> \n>> Correct. Git (the caller) will notice that not all \"delayed\" paths\n>> are listed by the filter and throw an error at the end.\n> \n> I am wondering about the other case. Git didn't ask for a path to\n> be filtered at all, but the filter sneaked in a path that happens to\n> in the index in its response---Git should at least ignore it, and\n> better yet, diagnose it as an error in the filter process.\n\nAgreed. I've used your implementation suggestion [1] and added two\ntest cases to ensure we signal a proper error in case of a buggy filter.\n\nThanks,\nLars\n\n\n[1] http://public-inbox.org/git/xmqqzicu2x4a.fsf@gitster.mtv.corp.google.com/\n"}]}