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

On 23 Jul 2016, at 00:32, Torsten Bögershausen [off-list ref] wrote:

On 07/22/2016 05:49 PM, larsxschneider@gmail.com wrote:

quoted

From: Lars Schneider <redacted>

[...]

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!

quoted

[...]

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!

quoted

[...]

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

quoted

[...]
+
+		printf "" >output.log &&

Is this the same as

quoted

output.log

to produce an empty file ?

Yes, thank you :-)

quoted

[...]

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

quoted

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