{"thread":{"id":"46086","subject":"[PATCH v5 0/5] convert: add \"status=delayed\" to filter process protocol","startedAt":"2017-06-01T08:22:13Z","lastAt":"2017-06-24T22:14:41Z","messageCount":20,"participants":["Lars Schneider","Junio C Hamano","Torsten Bögershausen","Jeff King"],"isPatch":true,"patchVersion":5,"patchTotal":5},"messages":[{"id":"321255","messageId":"20170601082203.50397-1-larsxschneider@gmail.com","threadId":"46086","inReplyTo":null,"subject":"[PATCH v5 0/5] convert: add \"status=delayed\" to filter process protocol","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-06-01T08:21:58Z","receivedAt":"2017-06-01T08:22:13Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"Hi,\n\nhere is the 5th 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 a minor \"extract method\" refactoring in convert. Patch 5 is the new\nfeature.\n\nChanges since v4:\n\n* rebased the topic on top of master to resolve conflicts and be able to\n  share code with on Ben Peart's bp/sub-process-convert-filter topic\n  http://public-inbox.org/git/xmqq1srgm9kq.fsf@gitster.mtv.corp.google.com/\n\n* removed invalid errno check\n  http://public-inbox.org/git/64b1fda4-9f79-1bd8-ad6d-43196b808d61@web.de/\n\n* minor documentation improvement\n  http://public-inbox.org/git/E2C871B0-DCDF-40C5-A559-C396F3C8AA66@gmail.com/\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/\n\n\n\nBase Ref: master\nWeb-Diff: https://github.com/larsxschneider/git/commit/23001c0fe2\nCheckout: git fetch https://github.com/larsxschneider/git filter-process/delay-v5 && git checkout 23001c0fe2\n\nLars Schneider (5):\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: 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                         |   6 +-\n convert.c                       | 162 ++++++++++++++++++++++++--------\n convert.h                       |  21 +++++\n entry.c                         | 110 +++++++++++++++++++++-\n t/t0021-conversion.sh           | 136 ++++++++++++++++++++-------\n t/t0021/rot13-filter.pl         | 199 ++++++++++++++++++++++++++--------------\n unpack-trees.c                  |   2 +\n 9 files changed, 559 insertions(+), 145 deletions(-)\n\n\nbase-commit: 0339965c70d68fd3831c9a5306443c869de3f6a8\n--\n2.13.0\n\n"},{"id":"321256","messageId":"20170601082203.50397-2-larsxschneider@gmail.com","threadId":"46086","inReplyTo":"20170601082203.50397-1-larsxschneider@gmail.com","subject":"[PATCH v5 1/5] t0021: keep filter log files on comparison","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-06-01T08:21:59Z","receivedAt":"2017-06-01T08:22:15Z","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 <larsxschneider@gmail.com>\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.0\n\n"},{"id":"321257","messageId":"20170601082203.50397-3-larsxschneider@gmail.com","threadId":"46086","inReplyTo":"20170601082203.50397-1-larsxschneider@gmail.com","subject":"[PATCH v5 2/5] t0021: make debug log file name configurable","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-06-01T08:22:00Z","receivedAt":"2017-06-01T08:22:17Z","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 <larsxschneider@gmail.com>\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.0\n\n"},{"id":"321258","messageId":"20170601082203.50397-6-larsxschneider@gmail.com","threadId":"46086","inReplyTo":"20170601082203.50397-1-larsxschneider@gmail.com","subject":"[PATCH v5 5/5] convert: add \"status=delayed\" to filter process protocol","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-06-01T08:22:03Z","receivedAt":"2017-06-01T08:22:19Z","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.\n\nSigned-off-by: Lars Schneider <larsxschneider@gmail.com>\n---\n Documentation/gitattributes.txt |  65 +++++++++++++-\n builtin/checkout.c              |   3 +\n cache.h                         |   6 +-\n convert.c                       | 119 +++++++++++++++++++++----\n convert.h                       |  21 +++++\n entry.c                         | 110 +++++++++++++++++++++--\n t/t0021-conversion.sh           |  74 ++++++++++++++++\n t/t0021/rot13-filter.pl         | 189 ++++++++++++++++++++++++++--------------\n unpack-trees.c                  |   2 +\n 9 files changed, 498 insertions(+), 91 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.<driver>.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..523ead1d95 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1543,16 +1543,20 @@ 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 \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, \"\" }\n+#define CHECKOUT_INIT { NULL, \"\", 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 a5e09bb0e8..c4df174378 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 (CAP_DELAY & entry->supported_capabilities &&\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,78 @@ 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+\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+\t}\n+\n \terr = subprocess_read_status(process->out, &filter_status);\n \tif (err)\n \t\tgoto done;\n@@ -680,10 +758,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 +773,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 +792,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 +1134,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 +1171,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 +1196,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 +1205,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 +1230,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..c4beaa5101 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,21 @@ 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+\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 +59,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..c9cb557559 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -137,6 +137,81 @@ 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+\t}\n+\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\tdco->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+\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 +254,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.0\n\n"},{"id":"321259","messageId":"20170601082203.50397-5-larsxschneider@gmail.com","threadId":"46086","inReplyTo":"20170601082203.50397-1-larsxschneider@gmail.com","subject":"[PATCH v5 4/5] convert: move multiple file filter error handling to separate function","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-06-01T08:22:02Z","receivedAt":"2017-06-01T08:22:22Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"This is useful for the subsequent patch 'convert: add \"status=delayed\" to\nfilter process protocol'.\n\nSigned-off-by: Lars Schneider <larsxschneider@gmail.com>\n---\n convert.c | 47 ++++++++++++++++++++++++++---------------------\n 1 file changed, 26 insertions(+), 21 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex f1e168bc30..a5e09bb0e8 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+\t} else 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.0\n\n"},{"id":"321260","messageId":"20170601082203.50397-4-larsxschneider@gmail.com","threadId":"46086","inReplyTo":"20170601082203.50397-1-larsxschneider@gmail.com","subject":"[PATCH v5 3/5] t0021: write \"OUT\" only on success","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-06-01T08:22:01Z","receivedAt":"2017-06-01T08:22:24Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\"rot13-filter.pl\" used to write \"OUT <size>\" to the debug log even in case of\nan abort or error. Fix this by writing \"OUT <size>\" 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 <larsxschneider@gmail.com>\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.0\n\n"},{"id":"321270","messageId":"xmqq1sr459fx.fsf@gitster.mtv.corp.google.com","threadId":"46086","inReplyTo":"20170601082203.50397-1-larsxschneider@gmail.com","subject":"Re: [PATCH v5 0/5] convert: add \"status=delayed\" to filter process protocol","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-01T09:44:18Z","receivedAt":"2017-06-01T09:44:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider <larsxschneider@gmail.com> writes:\n\n> here is the 5th iteration of my \"status delayed\" topic. Patch 1 to 3 are\n> minor t0021 test adjustments and haven't been changed since v3. Patch 4\n> is a minor \"extract method\" refactoring in convert. Patch 5 is the new\n> feature.\n\nThanks.  \n\nWill replace but it will take a while for me to get to it (I expect\nthat others may very well beat me to reviewing this series).\n\n"},{"id":"321379","messageId":"xmqqbmq716u7.fsf@gitster.mtv.corp.google.com","threadId":"46086","inReplyTo":"20170601082203.50397-1-larsxschneider@gmail.com","subject":"Re: [PATCH v5 0/5] convert: add \"status=delayed\" to filter process protocol","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-02T02:06:24Z","receivedAt":"2017-06-02T02:06:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider <larsxschneider@gmail.com> writes:\n\n> If you review this series then please read the \"Delay\" section in\n> \"Documentation/gitattributes.txt\" first for an overview of the delay mechanism.\n\nOK.\n"},{"id":"321380","messageId":"xmqq60gf1650.fsf@gitster.mtv.corp.google.com","threadId":"46086","inReplyTo":"20170601082203.50397-6-larsxschneider@gmail.com","subject":"Re: [PATCH v5 5/5] convert: add \"status=delayed\" to filter process protocol","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-02T02:21:31Z","receivedAt":"2017-06-02T02:21:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider <larsxschneider@gmail.com> writes:\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\nRandom things that come to mind (which I suspect has been dealt with\nin code but not described in the above in detail):\n\n * Using pathname as a \"delay key\" would mean that we assume that a\n   single helper process is spawned to serve only a single Git\n   invocation (as opposed to be sitting as a daemon accepting\n   requests from instances of Git invoked much later than the daemon\n   started), which is OK, but it is somewhat unclear when a filter\n   is allowed to discard the requests that used to be outstanding\n   from the write-up.  Imagine Git asked for A and B, both of which\n   was delayed, and then asked for a list of available ones and\n   learned that A is now ready.  \n\n   - Is it an error to ask for B at this point, and if so how is the\n     error handled?  Or will it turn into a blocking operation?\n\n   - As A is available, Git can ask for it.  After retrieving\n     smudged content for A, can Git ask for it again?  Or is it an\n     error to do so, without starting over with another can-delay=1\n     request for the same path?\n\n * The pathname does not have to be encoded/quoted in any way as it\n   is just a payload on the tail end of a packet line, so I am\n   guessing it is just a sequence raw bytes.  It is somewhat unclear\n   in the above write-up.  Also, is this considered to be a part of\n   \"textual\" exchange over the packet-line protocol, where it is\n   customary to remove the trailing LF from the packet?  \"We do not\n   support a file whose name ends with LF\" may or may not be an\n   acceptable stance (I am personally OK, but some people who use\n   Git may not), but it needs to be documented if there is such an\n   issue.\n\n * The continued request throws an empty content in the above\n   illustration.  Is a filter allowed/encouraged to assume that an\n   empty content in the request is a continuation?  I am guessing\n   that the answer is NO (otherwise you cannot filter an empty\n   file), and it somehow need to be documented, perhaps?\n\n"},{"id":"321529","messageId":"AD0F28DB-9E33-4F72-A632-3AE5C35A7A1B@gmail.com","threadId":"46086","inReplyTo":"xmqq60gf1650.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v5 5/5] convert: add \"status=delayed\" to filter process protocol","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-06-05T11:36:27Z","receivedAt":"2017-06-05T11:36:37Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 02 Jun 2017, at 04:21, Junio C Hamano <gitster@pobox.com> wrote:\n> \n> Lars Schneider <larsxschneider@gmail.com> writes:\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> \n> Random things that come to mind (which I suspect has been dealt with\n> in code but not described in the above in detail):\n> \n> * Using pathname as a \"delay key\" would mean that we assume that a\n>   single helper process is spawned to serve only a single Git\n>   invocation (as opposed to be sitting as a daemon accepting\n>   requests from instances of Git invoked much later than the daemon\n>   started), which is OK, but it is somewhat unclear when a filter\n>   is allowed to discard the requests that used to be outstanding\n>   from the write-up.  Imagine Git asked for A and B, both of which\n>   was delayed, and then asked for a list of available ones and\n>   learned that A is now ready.  \n> \n>   - Is it an error to ask for B at this point, and if so how is the\n>     error handled?  Or will it turn into a blocking operation?\n\nIt is no error per se and it should just work. If CE_DELAYED is set\nthen the filter could even delay the blob. However, I would consider \nthis bug in Git that would need to be fixed.\n\nWould this make the documentation clear enough?\n\n    After Git received the pathnames, it will request the \n    corresponding blobs again (and only these blobs).\n\n\n>   - As A is available, Git can ask for it.  After retrieving\n>     smudged content for A, can Git ask for it again?  Or is it an\n>     error to do so, without starting over with another can-delay=1\n>     request for the same path?\n\nI'd consider it a bug if Git asks for a blob again. How about this?\n\n    The filter is expected to respond with the smudged content\n    in the usual way as explained above. The filter can free any\n    resources associated with the blob after Git acknowledged\n    the delivery. Git will not ask for the blob again. \n\n\n> * The pathname does not have to be encoded/quoted in any way as it\n>   is just a payload on the tail end of a packet line, so I am\n>   guessing it is just a sequence raw bytes.  It is somewhat unclear\n>   in the above write-up.  Also, is this considered to be a part of\n>   \"textual\" exchange over the packet-line protocol, where it is\n>   customary to remove the trailing LF from the packet?  \"We do not\n>   support a file whose name ends with LF\" may or may not be an\n>   acceptable stance (I am personally OK, but some people who use\n>   Git may not), but it needs to be documented if there is such an\n>   issue.\n\nHow about this?\n\n    If the filter supports the \"delay\" capability then it must support \n    the \"list_available_blobs\" command. If Git sends this command, then \n    the filter is expected to return a list of pathnames of blobs that \n    are available. The pathname packets send by the filter must exactly \n    match the pathname packets that Git requested initially.\n\nMaybe even \"must match bit-exact\"?\n\n\n> * The continued request throws an empty content in the above\n>   illustration.  Is a filter allowed/encouraged to assume that an\n>   empty content in the request is a continuation?  I am guessing\n>   that the answer is NO (otherwise you cannot filter an empty\n>   file), and it somehow need to be documented, perhaps?\n\nCorrect, the answer is NO. Would \"irrelevant content section\" be\nOK or should I try to find a more explicit way to document that?\n\n    After Git received the pathnames, it will request the corresponding\n    blobs again. These requests contain a pathname and an irrelevant \n    (usually empty) content section. The filter is expected to respond \n    with the smudged content in the usual way as explained above.\n\n\n\nThanks a lot for the review,\nLars\n"},{"id":"322514","messageId":"be787e74-5f37-8429-9976-abdf8725737c@web.de","threadId":"46086","inReplyTo":"20170601082203.50397-5-larsxschneider@gmail.com","subject":"Re: [PATCH v5 4/5] convert: move multiple file filter error handling to separate function","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2017-06-18T07:20:49Z","receivedAt":"2017-06-18T07:21:30Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"\nOn 2017-06-01 10:22, Lars Schneider wrote:\n> This is useful for the subsequent patch 'convert: add \"status=delayed\" to\n> filter process protocol'.\n\nMay be\n Collecting all filter error handling is useful for the subsequent patch\n 'convert: add \"status=delayed\" to filter process protocol'.\n\n> \n> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>\n> ---\n>  convert.c | 47 ++++++++++++++++++++++++++---------------------\n>  1 file changed, 26 insertions(+), 21 deletions(-)\n> \n> diff --git a/convert.c b/convert.c\n> index f1e168bc30..a5e09bb0e8 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> +\t} else 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                Note1: Do we need a line with a single \";\" here ?\n                Question: What should/will happen to the file ?\n                Will Git complain later ? Retry later ?\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                         Hm, could this be clarified somewhat ?\n                         Mapping \"abort\" to \"permanent problem\" makes sense as\n                         such, but the only action that is done is to reset\n                         a capablity.\n\n\t\t/*\n\t\t * The filter signaled a missing capabilty. 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\n                 # And the there is a question why the answer is \"abort\" and not\n                 # \"unsupported\"\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> \n\n"},{"id":"322522","messageId":"28D2B758-7E5D-4EEA-A33D-57871960D5E1@gmail.com","threadId":"46086","inReplyTo":"be787e74-5f37-8429-9976-abdf8725737c@web.de","subject":"Re: [PATCH v5 4/5] convert: move multiple file filter error handling to separate function","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-06-18T11:47:49Z","receivedAt":"2017-06-18T11:49:31Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 18 Jun 2017, at 09:20, Torsten Bögershausen <tboegi@web.de> wrote:\n> \n> \n> On 2017-06-01 10:22, Lars Schneider wrote:\n>> This is useful for the subsequent patch 'convert: add \"status=delayed\" to\n>> filter process protocol'.\n> \n> May be\n> Collecting all filter error handling is useful for the subsequent patch\n> 'convert: add \"status=delayed\" to filter process protocol'.\n\nI think I get your point. However, I feel \"Collecting\" is not the right word.\n\nHow about:\n\"Refactoring filter error handling is useful...\"?\n\n\n>> \n>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>\n>> ---\n>> convert.c | 47 ++++++++++++++++++++++++++---------------------\n>> 1 file changed, 26 insertions(+), 21 deletions(-)\n>> \n>> diff --git a/convert.c b/convert.c\n>> index f1e168bc30..a5e09bb0e8 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>> +\t} else 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>                Note1: Do we need a line with a single \";\" here ?\n\nThe single \";\" wouldn't hurt but I don't think it is helpful either.\nI can't find anything in the coding guidelines about this...\n\n\n>                Question: What should/will happen to the file ?\n>                Will Git complain later ? Retry later ?\n\nThe file is not filtered and Git does not try, again. \nThat might be OK for certain filters:\nIf \"filter.foo.required = false\" then this would be OK. \nIf \"filter.foo.required = true\" then this would cause an error.\n\n\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>                         Hm, could this be clarified somewhat ?\n>                         Mapping \"abort\" to \"permanent problem\" makes sense as\n>                         such, but the only action that is done is to reset\n>                         a capablity.\n\nI am not sure what you mean with \"reset capability\". We disable the capability here.\nThat means Git will not use the capability for the active session.\n\n\n> \t\t/*\n> \t\t * The filter signaled a missing capabilty. 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\nI like the original version better because the capability is not \"missing\".\nThere is \"a permanent problem\" and the filter wants to signal Git to not use\nthis capability for the current session anymore.\n\n\n>                 # And the there is a question why the answer is \"abort\" and not\n>                 # \"unsupported\"\n\nBecause the filter supports the capability. There is just some kind of failure that \nthat causes the capability to not work as expected. Again, depending on the filter\n\"required\" flag this is an error or not.\n\n\nThanks for the review,\nLars\n\n"},{"id":"322576","messageId":"cedddaed-829d-9bde-3399-5b4e85dcbe57@web.de","threadId":"46086","inReplyTo":"28D2B758-7E5D-4EEA-A33D-57871960D5E1@gmail.com","subject":"Re: [PATCH v5 4/5] convert: move multiple file filter error handling to separate function","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2017-06-19T17:18:36Z","receivedAt":"2017-06-19T17:19:07Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2017-06-18 13:47, Lars Schneider wrote:\n> \n>> On 18 Jun 2017, at 09:20, Torsten Bögershausen <tboegi@web.de> wrote:\n>>\n>>\n>> On 2017-06-01 10:22, Lars Schneider wrote:\n>>> This is useful for the subsequent patch 'convert: add \"status=delayed\" to\n>>> filter process protocol'.\n>>\n>> May be\n>> Collecting all filter error handling is useful for the subsequent patch\n>> 'convert: add \"status=delayed\" to filter process protocol'.\n> \n> I think I get your point. However, I feel \"Collecting\" is not the right word.\n> \n> How about:\n> \"Refactoring filter error handling is useful...\"?\n\nOK\n\n\n> \n>>>\n>>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>\n>>> ---\n>>> convert.c | 47 ++++++++++++++++++++++++++---------------------\n>>> 1 file changed, 26 insertions(+), 21 deletions(-)\n>>>\n>>> diff --git a/convert.c b/convert.c\n>>> index f1e168bc30..a5e09bb0e8 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>>> +\t} else 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>>                Note1: Do we need a line with a single \";\" here ?\n> \n> The single \";\" wouldn't hurt but I don't think it is helpful either.\n> I can't find anything in the coding guidelines about this...\n\nOK about that. I was thinking to remove the {}, and the we -need- the \";\"\n\n> \n> \n>>                Question: What should/will happen to the file ?\n>>                Will Git complain later ? Retry later ?\n> \n> The file is not filtered and Git does not try, again. \n> That might be OK for certain filters:\n> If \"filter.foo.required = false\" then this would be OK. \n> If \"filter.foo.required = true\" then this would cause an error.\n\nDoes it make sense to add it as a comment ?\nI know, everything is documented otherwise, but when looking at the source\n code only, the context may be useful, it may be not.\n\n> \n> \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>>                         Hm, could this be clarified somewhat ?\n>>                         Mapping \"abort\" to \"permanent problem\" makes sense as\n>>                         such, but the only action that is done is to reset\n>>                         a capablity.\n> \n> I am not sure what you mean with \"reset capability\". We disable the capability here.\n> That means Git will not use the capability for the active session.\n\nSorry, my wrong - reset is a bad word here.\n\"Git will not use the capability for the active session\" is perfect!\n\n> \n> \n>> \t\t/*\n>> \t\t * The filter signaled a missing capabilty. 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> \n> I like the original version better because the capability is not \"missing\".\n> There is \"a permanent problem\" and the filter wants to signal Git to not use\n> this capability for the current session anymore.\n\nGit and the filter are in a negotiation phase to find out which side is able\nto do what.So I don't think there is a \"problem\" (in the sense that I understand\nit) at all.\n\nAnd back to the \"abort\":\nI still think that the word feels to harsh...\n\"abort\" in my understanding smells too much \"a program is terminated\".\nIf it is not too late, does it may sense to use something like \"nack\" ?\n\n\n> \n> \n>>                 # And the there is a question why the answer is \"abort\" and not\n>>                 # \"unsupported\"\n> \n> Because the filter supports the capability. There is just some kind of failure that \n> that causes the capability to not work as expected. Again, depending on the filter\n> \"required\" flag this is an error or not.\n> \n\nMay be I misunderstood the whole sequence, and abort is the right thing.\nIf yes, how about this ?\n\n\t} else if (!strcmp(filter_status->buf, \"abort\") && wanted_capability) {\n\t\t/*\n\t\t * Don't try to filter files with this capability for lifetime\n\t\t * of the current Git process.\n\t\t */\n\t\t entry->supported_capabilities &= ~wanted_capability;\n\n\n"},{"id":"322580","messageId":"AE3AB022-B7C8-4C13-AF8F-E561E9AA57D2@gmail.com","threadId":"46086","inReplyTo":"cedddaed-829d-9bde-3399-5b4e85dcbe57@web.de","subject":"Re: [PATCH v5 4/5] convert: move multiple file filter error handling to separate function","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-06-19T17:47:22Z","receivedAt":"2017-06-19T17:47:30Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 19 Jun 2017, at 19:18, Torsten Bögershausen <tboegi@web.de> wrote:\n> \n> On 2017-06-18 13:47, Lars Schneider wrote:\n>> \n>>> On 18 Jun 2017, at 09:20, Torsten Bögershausen <tboegi@web.de> wrote:\n>>> \n>>> \n>>> On 2017-06-01 10:22, Lars Schneider wrote:\n>>>> This is useful for the subsequent patch 'convert: add \"status=delayed\" to\n>>>> filter process protocol'.\n>>> \n>>> May be\n>>> Collecting all filter error handling is useful for the subsequent patch\n>>> 'convert: add \"status=delayed\" to filter process protocol'.\n>> \n>> I think I get your point. However, I feel \"Collecting\" is not the right word.\n>> \n>> How about:\n>> \"Refactoring filter error handling is useful...\"?\n> \n> OK\n\nOK, I'll change it in the next round.\n\n>>>> \n>>>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>\n>>>> ---\n>>>> convert.c | 47 ++++++++++++++++++++++++++---------------------\n>>>> 1 file changed, 26 insertions(+), 21 deletions(-)\n>>>> \n>>>> diff --git a/convert.c b/convert.c\n>>>> index f1e168bc30..a5e09bb0e8 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>>>> +\t} else 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>>>               Note1: Do we need a line with a single \";\" here ?\n>> \n>> The single \";\" wouldn't hurt but I don't think it is helpful either.\n>> I can't find anything in the coding guidelines about this...\n> \n> OK about that. I was thinking to remove the {}, and the we -need- the \";\"\n\nTrue. However, I prefer the {} style here. Would that be OK with you?\nPlus, this commit is not supposed to change code. I just want to move the\ncode to a different function.\n\n\n>>>               Question: What should/will happen to the file ?\n>>>               Will Git complain later ? Retry later ?\n>> \n>> The file is not filtered and Git does not try, again. \n>> That might be OK for certain filters:\n>> If \"filter.foo.required = false\" then this would be OK. \n>> If \"filter.foo.required = true\" then this would cause an error.\n> \n> Does it make sense to add it as a comment ?\n> I know, everything is documented otherwise, but when looking at the source\n> code only, the context may be useful, it may be not.\n\nI agree. I'll add a comment!\n\n>> \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>>>                        Hm, could this be clarified somewhat ?\n>>>                        Mapping \"abort\" to \"permanent problem\" makes sense as\n>>>                        such, but the only action that is done is to reset\n>>>                        a capablity.\n>> \n>> I am not sure what you mean with \"reset capability\". We disable the capability here.\n>> That means Git will not use the capability for the active session.\n> \n> Sorry, my wrong - reset is a bad word here.\n> \"Git will not use the capability for the active session\" is perfect!\n\nOK :)\n\n\n>>> \t\t/*\n>>> \t\t * The filter signaled a missing capabilty. 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>> \n>> I like the original version better because the capability is not \"missing\".\n>> There is \"a permanent problem\" and the filter wants to signal Git to not use\n>> this capability for the current session anymore.\n> \n> Git and the filter are in a negotiation phase to find out which side is able\n> to do what.So I don't think there is a \"problem\" (in the sense that I understand\n> it) at all.\n\nNo, at this point they are passed the negotiation phase. A problem actually\nhappened.\n\n\n> And back to the \"abort\":\n> I still think that the word feels to harsh...\n> \"abort\" in my understanding smells too much \"a program is terminated\".\n> If it is not too late, does it may sense to use something like \"nack\" ?\n\nWell, at this point it is too late because we don't retry.\n\nPlus, I would prefer to not change code here as this commit is supposed\nto just move existing code. Changing \"abort\" would change the protocol\nthat was released with Git 2.11.\n\n\n>> \n>>>                # And the there is a question why the answer is \"abort\" and not\n>>>                # \"unsupported\"\n>> \n>> Because the filter supports the capability. There is just some kind of failure that \n>> that causes the capability to not work as expected. Again, depending on the filter\n>> \"required\" flag this is an error or not.\n>> \n> \n> May be I misunderstood the whole sequence, and abort is the right thing.\n> If yes, how about this ?\n> \n> \t} else if (!strcmp(filter_status->buf, \"abort\") && wanted_capability) {\n> \t\t/*\n> \t\t * Don't try to filter files with this capability for lifetime\n> \t\t * of the current Git process.\n> \t\t */\n> \t\t entry->supported_capabilities &= ~wanted_capability;\n\nHow about this:\n\nThe filter signaled a problem. Don't try to filter files with this capability \nfor the lifetime of the current Git process.\n\n?\n\nThanks,\nLars"},{"id":"323208","messageId":"20170624141941.usy2pyhid3jrf3ku@sigill.intra.peff.net","threadId":"46086","inReplyTo":"20170601082203.50397-6-larsxschneider@gmail.com","subject":"Re: [PATCH v5 5/5] convert: add \"status=delayed\" to filter process protocol","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-24T14:19:41Z","receivedAt":"2017-06-24T14:19:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 01, 2017 at 10:22:03AM +0200, Lars Schneider wrote:\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> Teach the filter process protocol (introduced in edcc858) to accept the\n> status \"delayed\" as response to a filter request. Upon this response Git\n> continues with the checkout operation. After the checkout operation Git\n> calls \"finish_delayed_checkout\" which queries the filter for remaining\n> blobs. If the filter is still working on the completion, then the filter\n> is expected to block. If the filter has completed all remaining blobs\n> then an empty response is expected.\n> \n> Git has a multiple code paths that checkout a blob. Support delayed\n> checkouts only in `clone` (in unpack-trees.c) and `checkout` operations.\n\nIt might be worth giving a reason in this last paragraph. I think the\nreason is \"because it's more complicated for the caller, as they have to\nbe OK with out-of-order processing and remembering to go back and handle\nthe delayed cases\".\n\nSpeaking of which, _are_ we OK with out-of-order processing in things\nlike checkout? Certainly we care about deleting items before checking\nout new ones (so getting rid of \"foo/bar\" to make room for \"foo\" and\nvice versa).  I guess it's OK as long as the delayed items are always\nfilters that check out new items.\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\nOK, so the client has to opt into delay for each entry. Makes sense.\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\nEarlier I proposed just having the client ask \"give me something that's\nready\". This introduces an extra round-trip to say \"show me what's\nready\" and ask for it. But I think that's OK, as it should be a local\nprocess running over a pipe. The important thing is that the client is\nable to always fetch without blocking when something is ready, and to\nblock when nothing is ready, which this does. Good.\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\nOut of curiosity, what should happen if we re-ask for a pathname that\nwasn't mentioned in list_available_blobs? I guess it would depend on the\nfilter implementation, but probably most would just block (since we\nwouldn't have asked for can-delay). The other option is returning an\nerror, I suppose, but blocking and filling the request sounds\nfriendlier.\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\nThe changes to the caller here are satisfyingly small.\n\n> diff --git a/cache.h b/cache.h\n> index ae4c45d379..523ead1d95 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1543,16 +1543,20 @@ 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\nShould base_dir_len and base_dir be kept together?\n\nI suspect you ordered it this way because...\n\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, \"\" }\n> +#define CHECKOUT_INIT { NULL, \"\", NULL }\n> +\n\n...you wanted to add the NULL to the initializer here. But it's not\nnecessary. Once one element of a struct is initialized, all the\nremaining members are initialized according to the usual static rules\n(so NULL for pointers). We're already using that to zero out\nbase_dir_len and the flags (the \"\" is necessary because we want a real\nempty string, not NULL).\n\nSince you want NULL for your new member, you can just leave the\ninitializer as-is and it will do the right thing.\n\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\nWhy do we need to tell the filter we know about delay? Shouldn't it just\nneed to tell us that it knows about delay, and then we choose whether to\nask for can-delay for particular entries?\n\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 (CAP_DELAY & entry->supported_capabilities &&\n> +\t    dco && dco->state == CE_CAN_DELAY) {\n\nIn a complicated conditional with bit operations, we usually put the bit\noperation in its own parentheses so it's more obvious that it wasn't\nsupposed to be \"&&\". Like:\n\n  if ((CAP_DELAY & entry->supported_capabilities) &&\n      dco && dco->state == CE_CAN_DELAY))\n\nThe operator precedence is such that it works without them, so this is\njust a style question (I'd also usually put the flags field before the\nflag itself, but that's really getting into aesthetics).\n\nThe dco check here is basically asking if can-delay is enabled for the\nwhole checkout. Would we ever need to mark a specific entry as\nnot-delayable? Probably not from the start, but I wondered if something\nmight get marked as a failure and need to be retried without delay or\nsimilar. It's probably not worth going down that road until some caller\nneeds it.\n\n> @@ -662,14 +676,78 @@ 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\nThese string_list_insert()s have bad worst-case performance, because\nthey keep the list sorted. So the worst case is reverse-sorted input,\nwhere each entry goes at the front of the existing array and we have to\nmemmove() the whole array on each insert, and we do O(n^2) byte copies.\n\nThe best case is when the input is already sorted. And I think in\ngeneral that's how we'll iterate over paths to checkout. The cmds are\nmore likely to be random, but we're also not likely to have more than a\nhandful of them. So this is probably an OK data structure to use, at\nleast for now.\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> +\t}\n\nI think the prefix matching can be done more simply (and slightly more\nefficiently) with:\n\n  const char *path;\n  ...\n  err = !skip_prefix(line, \"pathname=\", &path);\n  if (err)\n\tgoto done;\n  string_list_insert(delayed_paths, xstrdup(path));\n\nI notice we loop here, though. Would we want to be resilient to seeing\nother keys besides pathname? Something like:\n\n  while ((line = packet_read_line(process->out, NULL)) {\n\tconst char *path;\n\tif (skip_prefix(line, \"pathname=\", &path))\n\t\tstring_list_insert(delayed_paths, xstrdup(path);\n\telse\n\t\t; /* ignore unknown keys */\n  }\n\nI can imagine some future where the filter passes back additional\ninformation about the delayed item. It would be nice if older versions\nof Git would just quietly ignore that if they don't know how to handle\nit.\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> +\t}\n\nStyle: we'd usually omit the braces here.\n\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\nThis is true because we know the filter would block until it had at\nleast one item to return.\n\nIt does seem to put a lot of faith in the filter. What happens if we hit\nthis conditional and we know that we still have entries in dco->paths to\nbe handled?\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\nThis is O(m log n), which is fine. Since both lists are sorted, we could\ntechnically do it in an O(m+n) merge, but I don't think we have a\nfunction to do that easily.\n\nSorry to harp on run-times, but it's one place I think it would be easy\nto subtly screw this up, so I'm trying to pay close attention.\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\tdco->state = CE_RETRY;\n> +\t\t\t\terrs |= (ce ? checkout_entry(ce, state, NULL) : 1);\n> +\t\t\t}\n\nSo once we know something is available, we try again. Which makes sense.\nWe presumably set dco->state to retry here so that we don't re-ask for\nit to be delayed. I guess that sort-of answers my earlier question about\nper-entry state. We just use the dco->state as a big hammer. It gets\nset to retry on the first time through the loop, and then that applies\nto everything for the rest of the checkout. But since we do all of the\ninitial requests in a single pass, followed by all delayed-resolution in\na second pass, that works fine.\n\n> +\t/* At this point we should not have any delayed paths anymore. */\n> +\terrs |= dco->paths.nr;\n\nAnd I guess this answers my question above about a filter with no\nentries. When we bail from the loop, we'll hit this point and complain.\n\nWe probably should call string_list_clear(&dco->paths) here to avoid\nleaking the paths on error (possibly we should also print them? Or maybe\nthey'd be warned about elsewhere).\n\n> @@ -179,11 +254,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\nHmm. This \"reset the state\" bit at the end surprised me. I guess it's\nnot wrong, but it goes against the mental model I had formed above. ;)\n\nWe really are using dco->state as a per-entry state flag. It just\nhappens to be in a persistent shared struct. I don't think it's wrong,\nit was mostly just surprising. I don't know if it's worth trying to\nsimplify, but I think you could do it by:\n\n  1. Passing back the \"was delayed\" state from async_convert... in the\n     return value or via a separate out-parameter.\n\n  2. Setting dco->state to CE_RETRY at the top of finish_delayed... so\n     that it's clear that it's about what phase of the conversation\n     we're in.\n\nBut I'm OK with it as-is, too.\n\n> diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl\n> index 5e43faeec1..f0dc0aff4a 100644\n> --- a/t/t0021/rot13-filter.pl\n> +++ b/t/t0021/rot13-filter.pl\n\nI'll admit to skimming the perl changes here, but I didn't see anything\nobviously wrong.\n\n-Peff\n"},{"id":"323209","messageId":"20170624142301.c5jl7xxf4ici3cug@sigill.intra.peff.net","threadId":"46086","inReplyTo":"20170601082203.50397-1-larsxschneider@gmail.com","subject":"Re: [PATCH v5 0/5] convert: add \"status=delayed\" to filter process protocol","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-24T14:23:01Z","receivedAt":"2017-06-24T14:23:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 01, 2017 at 10:21:58AM +0200, Lars Schneider wrote:\n\n> here is the 5th iteration of my \"status delayed\" topic. Patch 1 to 3 are\n> minor t0021 test adjustments and haven't been changed since v3. Patch 4\n> is a minor \"extract method\" refactoring in convert. Patch 5 is the new\n> feature.\n\nYeah, patches 1-4 were pretty straightforward and obvious. I just left\nsome comments after a detailed read-through of patch 5. Overall the\ndirection looks good. I had a few minor comments on the code, but I\nthink with those addressed it will probably be ready for 'next'.\n\nNote that I'll be offline all next week, so don't expect a timely\nreview on the next iteration (unlike this not-so-timely one ;) ).\n\n-Peff\n"},{"id":"323212","messageId":"A8FAEFDC-1ADA-4355-80DC-069A739F829C@gmail.com","threadId":"46086","inReplyTo":"20170624141941.usy2pyhid3jrf3ku@sigill.intra.peff.net","subject":"Re: [PATCH v5 5/5] convert: add \"status=delayed\" to filter process protocol","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-06-24T17:22:40Z","receivedAt":"2017-06-24T17:22:50Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 24 Jun 2017, at 16:19, Jeff King <peff@peff.net> wrote:\n> \n> On Thu, Jun 01, 2017 at 10:22:03AM +0200, Lars Schneider wrote:\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>> Teach the filter process protocol (introduced in edcc858) to accept the\n>> status \"delayed\" as response to a filter request. Upon this response Git\n>> continues with the checkout operation. After the checkout operation Git\n>> calls \"finish_delayed_checkout\" which queries the filter for remaining\n>> blobs. If the filter is still working on the completion, then the filter\n>> is expected to block. If the filter has completed all remaining blobs\n>> then an empty response is expected.\n>> \n>> Git has a multiple code paths that checkout a blob. Support delayed\n>> checkouts only in `clone` (in unpack-trees.c) and `checkout` operations.\n> \n> It might be worth giving a reason in this last paragraph. I think the\n> reason is \"because it's more complicated for the caller, as they have to\n> be OK with out-of-order processing and remembering to go back and handle\n> the delayed cases\".\n\nCorrect! However, my real reason was that these code paths process all\nfiles of the tree. Therefore the \"out-of-order\" processing can be\neffective. \n\nHow about this:\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\n\n> Speaking of which, _are_ we OK with out-of-order processing in things\n> like checkout? Certainly we care about deleting items before checking\n> out new ones (so getting rid of \"foo/bar\" to make room for \"foo\" and\n> vice versa).  I guess it's OK as long as the delayed items are always\n> filters that check out new items.\n\nJunio noticed that too but thinks we should be OK:\n\"[...] We checkout removals first to make room so\nthat creation of a path X can succeed if an existing path X/Y\nthat used to want to see X as a directory can succeed [...]\"\n\nhttp://public-inbox.org/git/xmqqvavotych.fsf@gitster.mtv.corp.google.com/\n\n\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> Out of curiosity, what should happen if we re-ask for a pathname that\n> wasn't mentioned in list_available_blobs? I guess it would depend on the\n> filter implementation, but probably most would just block (since we\n> wouldn't have asked for can-delay). The other option is returning an\n> error, I suppose, but blocking and filling the request sounds\n> friendlier.\n\nI agree that is up to the filter. I would expect the blocking strategy.\nThe filter cannot delay it anymore as we have not send the \"can-delay\"\nflag.\n\n\n> ...\n>> diff --git a/cache.h b/cache.h\n>> index ae4c45d379..523ead1d95 100644\n>> --- a/cache.h\n>> +++ b/cache.h\n>> @@ -1543,16 +1543,20 @@ 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> \n> Should base_dir_len and base_dir be kept together?\n> \n> I suspect you ordered it this way because...\n> \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, \"\" }\n>> +#define CHECKOUT_INIT { NULL, \"\", NULL }\n>> +\n> \n> ...you wanted to add the NULL to the initializer here. But it's not\n> necessary. Once one element of a struct is initialized, all the\n> remaining members are initialized according to the usual static rules\n> (so NULL for pointers). We're already using that to zero out\n> base_dir_len and the flags (the \"\" is necessary because we want a real\n> empty string, not NULL).\n> \n> Since you want NULL for your new member, you can just leave the\n> initializer as-is and it will do the right thing.\n\nThat was indeed my reasoning. I am no C expert and I wasn't sure about\nthe initialization. The \"Once one element of a struct is initialized\"\nthing is news to me :-) Thanks!\n\nI'll keep the base_dir* together then!\n\n\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> Why do we need to tell the filter we know about delay? Shouldn't it just\n> need to tell us that it knows about delay, and then we choose whether to\n> ask for can-delay for particular entries?\n\nBecause in the protocol I defined that the filter needs to answer with\na strict subset of this list [1]. I thought that this would make the protocol\nmore future proof/backward compatible. Because the filter is not allowed to\nanswer with something that Git does not understand.\n\n[1] https://github.com/git/git/blob/5402b1352f5181247405fbff1887008a0cb3b04a/Documentation/gitattributes.txt#L408-L411\n\n\n> \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 (CAP_DELAY & entry->supported_capabilities &&\n>> +\t    dco && dco->state == CE_CAN_DELAY) {\n> \n> In a complicated conditional with bit operations, we usually put the bit\n> operation in its own parentheses so it's more obvious that it wasn't\n> supposed to be \"&&\". Like:\n> \n>  if ((CAP_DELAY & entry->supported_capabilities) &&\n>      dco && dco->state == CE_CAN_DELAY))\n\nAgreed!\n\n\n> The operator precedence is such that it works without them, so this is\n> just a style question (I'd also usually put the flags field before the\n> flag itself, but that's really getting into aesthetics).\n\nYou mean (entry & CAP_DELAY) instead of (CAP_DELAY & entry)?\n\n\n> The dco check here is basically asking if can-delay is enabled for the\n> whole checkout. Would we ever need to mark a specific entry as\n> not-delayable? Probably not from the start, but I wondered if something\n> might get marked as a failure and need to be retried without delay or\n> similar. It's probably not worth going down that road until some caller\n> needs it.\n\nAgreed.\n\n> \n>> @@ -662,14 +676,78 @@ 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> These string_list_insert()s have bad worst-case performance, because\n> they keep the list sorted. So the worst case is reverse-sorted input,\n> where each entry goes at the front of the existing array and we have to\n> memmove() the whole array on each insert, and we do O(n^2) byte copies.\n> \n> The best case is when the input is already sorted. And I think in\n> general that's how we'll iterate over paths to checkout. The cmds are\n> more likely to be random, but we're also not likely to have more than a\n> handful of them. So this is probably an OK data structure to use, at\n> least for now.\n\nAgreed! Up until now I tested the delayed technique only with my small test \ncase. It will become really interesting when GitLFS implements this feature.\nThen I can test it with our repos that have +20k filtered files. At this point\nI will be able to identify the spots that require more optimization.\n\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>> +\t}\n> \n> I think the prefix matching can be done more simply (and slightly more\n> efficiently) with:\n> \n>  const char *path;\n>  ...\n>  err = !skip_prefix(line, \"pathname=\", &path);\n>  if (err)\n> \tgoto done;\n>  string_list_insert(delayed_paths, xstrdup(path));\n> \n> I notice we loop here, though. Would we want to be resilient to seeing\n> other keys besides pathname? Something like:\n> \n>  while ((line = packet_read_line(process->out, NULL)) {\n> \tconst char *path;\n> \tif (skip_prefix(line, \"pathname=\", &path))\n> \t\tstring_list_insert(delayed_paths, xstrdup(path);\n> \telse\n> \t\t; /* ignore unknown keys */\n>  }\n> \n> I can imagine some future where the filter passes back additional\n> information about the delayed item. It would be nice if older versions\n> of Git would just quietly ignore that if they don't know how to handle\n> it.\n\nI agree with all the above. skip_prefix is neat! Although it's so\ndominant in the source code I never noticed it.\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>> +\t}\n> \n> Style: we'd usually omit the braces here.\n\nSure! I'll remove it.\n\n(This is the only rule of the Git style guide that\ndon't agree with :-) \n\n> \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> This is true because we know the filter would block until it had at\n> least one item to return.\n> \n> It does seem to put a lot of faith in the filter. What happens if we hit\n> this conditional and we know that we still have entries in dco->paths to\n> be handled?\n\nWe check this later as you noticed below.\n\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> This is O(m log n), which is fine. Since both lists are sorted, we could\n> technically do it in an O(m+n) merge, but I don't think we have a\n> function to do that easily.\n> \n> Sorry to harp on run-times, but it's one place I think it would be easy\n> to subtly screw this up, so I'm trying to pay close attention.\n\nNo worries at all! Thanks for thinking it through on that level.\nAs I wrote above - I plan to really profile it under \"real life conditions\"\nas soon as we have full GitLFS support.\n\n\n>> ...\n> \n>> +\t/* At this point we should not have any delayed paths anymore. */\n>> +\terrs |= dco->paths.nr;\n> \n> And I guess this answers my question above about a filter with no\n> entries. When we bail from the loop, we'll hit this point and complain.\n> \n> We probably should call string_list_clear(&dco->paths) here to avoid\n> leaking the paths on error (possibly we should also print them? Or maybe\n> they'd be warned about elsewhere).\n\nAgreed, I will clear the list. I'll also print the items because as far\nas I can see they are not printed elsewhere.\n\nGit just says:\nfatal: unable to checkout working tree\nwarning: Clone succeeded, but checkout failed.\nYou can inspect what was checked out with 'git status'\nand retry the checkout with 'git checkout -f HEAD'\n\nHow about this?\n\n\terrs |= dco->paths.nr;\n\tfor_each_string_list_item(path, &dco->paths) {\n\t\twarning(\"%s was not processed properly.\", path->string);\n\t}\n\tstring_list_clear(&dco->paths, 0);\n\nThe output would be:\n\nwarning: test-delay10.a was not processed properly.\nwarning: test-delay10.b was not processed properly.\nwarning: test-delay11.a was not processed properly.\nwarning: test-delay20.a was not processed properly.\nfatal: unable to checkout working tree\nwarning: Clone succeeded, but checkout failed.\nYou can inspect what was checked out with 'git status'\nand retry the checkout with 'git checkout -f HEAD'\n\nI contemplated about the warning text.\n\"$FILE was not filtered properly.\" is technical more\ncorrect but maybe it would confuse the user?\n\nPlus, at this point I don't really know what filter should\nhave given me the files. That's why I can't print it in \nthe warning message.\n\n> \n>> @@ -179,11 +254,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> \n> Hmm. This \"reset the state\" bit at the end surprised me. I guess it's\n> not wrong, but it goes against the mental model I had formed above. ;)\n> \n> We really are using dco->state as a per-entry state flag. It just\n> happens to be in a persistent shared struct. I don't think it's wrong,\n> it was mostly just surprising. I don't know if it's worth trying to\n> simplify, but I think you could do it by:\n> \n>  1. Passing back the \"was delayed\" state from async_convert... in the\n>     return value or via a separate out-parameter.\n\nIn the beginning I had it implemented that way. But that meant that I\nhad to pass two variables through the entire convert stack:\n\nasync_convert_to_working_tree\n-> convert_to_working_tree_internal\n--> apply_filter\n---> apply_multi_file_filter\n\n> \n>  2. Setting dco->state to CE_RETRY at the top of finish_delayed... so\n>     that it's clear that it's about what phase of the conversation\n>     we're in.\n\nI could do that. However, I thought it is safer to set the state *before*\nevery checkout operation in case convert.c messes with this field (it\nshould not in this phase).\n\n\n> But I'm OK with it as-is, too.\n\nI'll try 2.\n\n\n>> diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl\n>> index 5e43faeec1..f0dc0aff4a 100644\n>> --- a/t/t0021/rot13-filter.pl\n>> +++ b/t/t0021/rot13-filter.pl\n> \n> I'll admit to skimming the perl changes here, but I didn't see anything\n> obviously wrong.\n\n\nThanks a lot for the review,\nLars\n\n"},{"id":"323220","messageId":"xmqq37ap9q2i.fsf@gitster.mtv.corp.google.com","threadId":"46086","inReplyTo":"A8FAEFDC-1ADA-4355-80DC-069A739F829C@gmail.com","subject":"Re: [PATCH v5 5/5] convert: add \"status=delayed\" to filter process protocol","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-24T18:51:49Z","receivedAt":"2017-06-24T18:52:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider <larsxschneider@gmail.com> writes:\n\n>> On 24 Jun 2017, at 16:19, Jeff King <peff@peff.net> wrote:\n>> \n>> Speaking of which, _are_ we OK with out-of-order processing in things\n>> like checkout? Certainly we care about deleting items before checking\n>> out new ones (so getting rid of \"foo/bar\" to make room for \"foo\" and\n>> vice versa).  I guess it's OK as long as the delayed items are always\n>> filters that check out new items.\n>\n> Junio noticed that too but thinks we should be OK:\n> \"[...] We checkout removals first to make room so\n> that creation of a path X can succeed if an existing path X/Y\n> that used to want to see X as a directory can succeed [...]\"\n>\n> http://public-inbox.org/git/xmqqvavotych.fsf@gitster.mtv.corp.google.com/\n\nIn which I said\n\n   ...  I\n   think \"remove all and then create\" you do not specifically have\n   to worry about with the proposed change, but you may need to\n   inspect and verify there aren't other kind of order dependency.\n\nYes, I think having two separate loops in the caller can help\nguaranteeing this, as long as the delayed items are always filters\nthat check out new things.  It will break once you have delayed\nremovals but I do not see how such a thing would be necessary ;-)\n\nBut there may be other kinds of order dependency--I didn't look for\nthem.\n\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 (CAP_DELAY & entry->supported_capabilities &&\n>>> +\t    dco && dco->state == CE_CAN_DELAY) {\n>> \n>> In a complicated conditional with bit operations, we usually put the bit\n>> operation in its own parentheses so it's more obvious that it wasn't\n>> supposed to be \"&&\". Like:\n>> \n>>  if ((CAP_DELAY & entry->supported_capabilities) &&\n>>      dco && dco->state == CE_CAN_DELAY))\n>\n> Agreed!\n\nWhy wasn't this caught earlier?  I thought this is something gcc warns about.\n\n>> The operator precedence is such that it works without them, so this is\n>> just a style question (I'd also usually put the flags field before the\n>> flag itself, but that's really getting into aesthetics).\n>\n> You mean (entry & CAP_DELAY) instead of (CAP_DELAY & entry)?\n\nPeff is continuing his explanation why (A & B && C) is technically\ncorrect and preferring ((A & B) && C) is purely stylistic.  \"A & B\"\nbinds tighter than \"something && C\" which means that (A & B && C)\ncannot be misinterpreted as (A & (B && C)).\n\n"},{"id":"323222","messageId":"20170624203237.tmismuaits5pi7vq@sigill.intra.peff.net","threadId":"46086","inReplyTo":"A8FAEFDC-1ADA-4355-80DC-069A739F829C@gmail.com","subject":"Re: [PATCH v5 5/5] convert: add \"status=delayed\" to filter process protocol","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-24T20:32:37Z","receivedAt":"2017-06-24T22:14:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jun 24, 2017 at 07:22:40PM +0200, Lars Schneider wrote:\n\n> > It might be worth giving a reason in this last paragraph. I think the\n> > reason is \"because it's more complicated for the caller, as they have to\n> > be OK with out-of-order processing and remembering to go back and handle\n> > the delayed cases\".\n> \n> Correct! However, my real reason was that these code paths process all\n> files of the tree. Therefore the \"out-of-order\" processing can be\n> effective. \n> \n> How about this:\n> \n> Git has a multiple code paths that checkout a blob. Support delayed\n> checkouts only in `clone` (in unpack-trees.c) and `checkout` operations\n> for now. The optimization is most effective in these code paths as all \n> files of the tree are processed.\n\nSounds good.\n\n> > Why do we need to tell the filter we know about delay? Shouldn't it just\n> > need to tell us that it knows about delay, and then we choose whether to\n> > ask for can-delay for particular entries?\n> \n> Because in the protocol I defined that the filter needs to answer with\n> a strict subset of this list [1]. I thought that this would make the protocol\n> more future proof/backward compatible. Because the filter is not allowed to\n> answer with something that Git does not understand.\n> \n> [1] https://github.com/git/git/blob/5402b1352f5181247405fbff1887008a0cb3b04a/Documentation/gitattributes.txt#L408-L411\n\nOK. That makes sense, then.\n\n> > The operator precedence is such that it works without them, so this is\n> > just a style question (I'd also usually put the flags field before the\n> > flag itself, but that's really getting into aesthetics).\n> \n> You mean (entry & CAP_DELAY) instead of (CAP_DELAY & entry)?\n\nYes, exactly.\n\n> How about this?\n> \n> \terrs |= dco->paths.nr;\n> \tfor_each_string_list_item(path, &dco->paths) {\n> \t\twarning(\"%s was not processed properly.\", path->string);\n> \t}\n> \tstring_list_clear(&dco->paths, 0);\n> \n> The output would be:\n> \n> warning: test-delay10.a was not processed properly.\n> warning: test-delay10.b was not processed properly.\n> warning: test-delay11.a was not processed properly.\n> warning: test-delay20.a was not processed properly.\n> fatal: unable to checkout working tree\n> warning: Clone succeeded, but checkout failed.\n> You can inspect what was checked out with 'git status'\n> and retry the checkout with 'git checkout -f HEAD'\n\nI think it may make sense to use something more specific than\n\"processed\". The user might not even be thinking about filters during\ntheir operation. It would be really nice if we could mention the name of\nthe filter. As you noted, we don't have it here but I wonder how hard it\nwould be. Anyway, I'm OK with leaving it more vague for now.\n\n> I contemplated about the warning text.\n> \"$FILE was not filtered properly.\" is technical more\n> correct but maybe it would confuse the user?\n\nI like it better because \"filter\" is a word the user might associate\nwith the filter feature. Whereas \"processed\" is vague and could mean\nmany things.\n\n> > Hmm. This \"reset the state\" bit at the end surprised me. I guess it's\n> > not wrong, but it goes against the mental model I had formed above. ;)\n> > \n> > We really are using dco->state as a per-entry state flag. It just\n> > happens to be in a persistent shared struct. I don't think it's wrong,\n> > it was mostly just surprising. I don't know if it's worth trying to\n> > simplify, but I think you could do it by:\n> > \n> >  1. Passing back the \"was delayed\" state from async_convert... in the\n> >     return value or via a separate out-parameter.\n> \n> In the beginning I had it implemented that way. But that meant that I\n> had to pass two variables through the entire convert stack:\n> \n> async_convert_to_working_tree\n> -> convert_to_working_tree_internal\n> --> apply_filter\n> ---> apply_multi_file_filter\n\nRight, I see. I wonder if just a comment in the definition of the\ndelayed_checkout struct would make it more clear exactly how we expect\nthe member to be used.\n\n> >  2. Setting dco->state to CE_RETRY at the top of finish_delayed... so\n> >     that it's clear that it's about what phase of the conversation\n> >     we're in.\n> \n> I could do that. However, I thought it is safer to set the state *before*\n> every checkout operation in case convert.c messes with this field (it\n> should not in this phase).\n>\n> > But I'm OK with it as-is, too.\n> \n> I'll try 2.\n\nI think you'd have to do (1) and (2) together. But if it causes pain, I\nthink the comment I suggested above may be the simplest way to go.\n\n> Thanks a lot for the review,\n\nYou're welcome. The bits of your response I didn't quote all made sense\nto me.\n\n-Peff\n"},{"id":"323223","messageId":"20170624203640.qomgbsakrgseungl@sigill.intra.peff.net","threadId":"46086","inReplyTo":"xmqq37ap9q2i.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v5 5/5] convert: add \"status=delayed\" to filter process protocol","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-24T20:36:40Z","receivedAt":"2017-06-24T22:14:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jun 24, 2017 at 11:51:49AM -0700, Junio C Hamano wrote:\n\n> >>  if ((CAP_DELAY & entry->supported_capabilities) &&\n> >>      dco && dco->state == CE_CAN_DELAY))\n> >\n> > Agreed!\n> \n> Why wasn't this caught earlier?  I thought this is something gcc warns about.\n\nI thought so, too. If it warned about:\n\n  if (A & B)\n\nthat would probably be too annoying. But:\n\n  if (A & B && C)\n\nis much more questionable. I wonder if it used to exist and gcc dropped\nit (or dropped it from -Wall). -Wlogical-op seems like the most likely\ncandidate, but it does not catch it (and it has a false positive in\nhandle_nonblock, or perhaps I just can't see the problem).\n\n> >> The operator precedence is such that it works without them, so this is\n> >> just a style question (I'd also usually put the flags field before the\n> >> flag itself, but that's really getting into aesthetics).\n> >\n> > You mean (entry & CAP_DELAY) instead of (CAP_DELAY & entry)?\n> \n> Peff is continuing his explanation why (A & B && C) is technically\n> correct and preferring ((A & B) && C) is purely stylistic.  \"A & B\"\n> binds tighter than \"something && C\" which means that (A & B && C)\n> cannot be misinterpreted as (A & (B && C)).\n\nI actually meant both. The bitwise operator binds tighter so it's OK\neither way. But I would write \"flags & MY_FLAG\" and never \"MY_FLAG &\nflags\".\n\n-Peff\n"}]}