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

From: Lars Schneider <larsxschneider@gmail.com>
Date: Jul 24, 2016, 12:09 UTC
Message-ID: <D4012F2A-9774-408B-A355-B7442A4FBF26@gmail.com>
In-Reply-To: <32d8feda-0fff-6c8c-1ac3-9cc3d783d0ef@web.de>

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

Show 12 quoted lines

> 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!

Show 5 quoted lines

>> [...]
>> 
>> 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!

Show 18 quoted lines

>> [...]
>> 
>> +
>> +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.

Show 6 quoted lines

>> [...]
>> +
>> +		printf "" >output.log &&
> Is this the same as
> >output.log
> to produce an empty file ?

Yes, thank you :-)

Show 6 quoted lines

>> [...]
>> +++ 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.

Show 8 quoted lines

>> [...]
>> +
>> +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