On 23 Jul 2016, at 00:32, Torsten Bögershausen <tboegi@xxxxxx> wrote: > On 07/22/2016 05:49 PM, larsxschneider@xxxxxxxxx wrote: >> From: Lars Schneider <larsxschneider@xxxxxxxxx> >> >> [...] >> >> 1. Git starts the filter on first usage and expects a welcome message >> with protocol version number: >> Git <-- Filter: "git-filter-protocol\n" >> Git <-- Filter: "version 1" > Is there no terminator here ? > How long should the reading side wait without a '\n', or should it read > "version 1\n" ? I agree, I will add the "\n" terminator! >> [...] >> >> Please note that the protocol filters do not support stream processing >> with this implemenatation because the filter needs to know the length of > ^^^^^^^^^^^^^^^^typo Thanks! >> [...] >> >> + >> +test_expect_success EXPENSIVE 'protocol filter large file' ' >> + test_config_global filter.protocol.clean \"$TEST_DIRECTORY/t0021/rot13.pl\" && >> + test_config_global filter.protocol.smudge \"$TEST_DIRECTORY/t0021/rot13.pl\" && >> + rm -rf repo && >> + mkdir repo && >> + ( >> + cd repo && >> + git init && >> + >> + echo "2GB filter=largefile" >.gitattributes && >> + for i in $(test_seq 1 2048); do printf "%1048576d" 1; done >2GB && > Side question: > Is there a way to "re-use" the 2GB test file through t0021? > It takes a long time to produce it, especially on my 32 Bit systems ;-) > But this may be a different patch. I would like to keep the tests as unentangled as possible and therefore a direct reuse might not be ideal. However, I could add a new "EXPENSIVE setup test" that prepares the file for both tests. >> [...] >> + >> + printf "" >output.log && > Is this the same as > >output.log > to produce an empty file ? Yes, thank you :-) >> [...] >> +++ b/t/t0021/rot13.pl >> @@ -0,0 +1,80 @@ >> +#!/usr/bin/env perl > Should this be > "$PERL_PATH" ? I think we can't use this variable directly in the script. I could create the script file for the test and set the shebang to this value. However, no other "Perl file test" does it and therefore I wonder if it is necessary: t/t0202/test.pl t/t9000/test.pl t/t9700/test.pl According to the documentation this is useful to avoid trouble on Windows. I will check this test on Windows. I also just noticed that all other Perl tests use "#!/usr/bin/perl". Should I change mine to match those? > And do we need to protect the TC with > test_have_prereq PERL or similar ? Probably not as the documentation states "Even without the PERL prerequisite, tests can assume there is a usable perl interpreter". However, all other Perl file tests do the same and therefore I think it is a good idea. >> [...] >> + >> +print STDOUT "git-filter-protocol\nversion 1"; > Again, I don't like the missing terminator here. > What if we step up to protocol "version 10" ? > Could it work to use one and only one line, > with one terminator, like this ? > print STDOUT "git-filter-protocol version 1\1"; The missing terminator was a mistake. As mentioned above, I will add it! Thanks for the review, Lars-- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html