Re: [PATCH v1 3/3] convert: add filter.<driver>.useProtocol option

[Lars Schneider <larsxschneider@gmail.com>]

On 23 Jul 2016, at 00:32, Torsten Bögershausen <tboegi@web.de> wrote:

> On 07/22/2016 05:49 PM, larsxschneider@gmail.com wrote:
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> [...]
>> 
>> 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