Re: [PATCH v2 4/5] tests: replace remaining packetize() with "test-tool pkt-line"

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Jul 13 2021, Jeff King wrote:

> On Mon, Jul 12, 2021 at 06:44:19PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> Move the only remaining users of "packetize()" over to "test-tool
>> pkt-line", for this we need a new "pack-raw-stdin" subcommand in the
>> test-tool. The "pack" command takes input on stdin, but splits it by
>> "\n", furthermore we'll format the output using C-strings, so the
>> embedded "\0" being tested for here would cause the string to be
>> truncated.
>> 
>> So we need another mode that just calls packet_write() on the raw
>> input we got on stdin.
>
> Makes sense. Lacking this "raw" version was the sticking point in the
> past for using the helper in more places.
>
>> +static void pack_raw_stdin(void)
>> +{
>> +	struct strbuf sb = STRBUF_INIT;
>> +	strbuf_read(&sb, 0, 0);
>> +	if (strbuf_read(&sb, 0, 0) < 0)
>> +		die_errno("failed to read from stdin");
>
> What's up with the two reads here?
>
> It looks like just a duplication. And it happens to work because each
> one tries to read to eof, making the second one generally a noop.

Yes oops, just a copy/paste error I didn't spot.

>> diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
>> index 2dde0348816..b52afb0cdea 100755
>> --- a/t/t5570-git-daemon.sh
>> +++ b/t/t5570-git-daemon.sh
>> @@ -193,10 +193,12 @@ test_expect_success 'hostname cannot break out of directory' '
>>  '
>>  
>>  test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '
>> -	{
>> -		printf "git-upload-pack /interp.git\n\0host=localhost" | packetize
>> -		printf "0000"
>> -	} >input &&
>> +	printf "git-upload-pack /interp.git\n\0host=localhost" >has-null &&
>> +	test-tool pkt-line pack-raw-stdin >input <has-null &&
>> +	test-tool pkt-line pack >>input <<-\EOF &&
>> +	0000
>> +	EOF
>
> This is a minor style nit, but I really prefer redirecting output from
> a block (as in the original) rather than iterative appending (in the
> result). IMHO it makes it more obvious what is going into the file and
> what is not.
>
> I.e.:
>
>   {
>           printf "git-upload-pack /interp.git\n\0host=localhost" |
> 	  test-tool pkt-line pack-raw-stdin &&
> 	  printf "0000" | test-tool pkt-line pack
>   } >input
>
> (again here the packing of "0000" is pointless, but I'm OK with it for
> consistency).

Sure, I can use {} blocks, but re the reply on 3/5 about "0000"
v.s. "0000\n" you'd like to move back to print not-a-newline here
v.s. having the helper eat lines ending with \n like everywhere else?

It's not incorrect in this case, it just seems less obvious to
me. I.e. the printf in the former case is because we explicitly care
about the exact raw input, if there's a trailing \n or not, in the
latter case we don't, so I think it's clearier to use the usual <<-\EOF
pattern rather than printf.