Re: [PATCH v4 3/3] cat-file: add --batch-command mode

[John Cai <johncai86@gmail.com>]

Hi Junio

On 10 Feb 2022, at 12:05, Junio C Hamano wrote:

> Phillip Wood <phillip.wood123@gmail.com> writes:
>
>>> +	type=$1
>>> +	sha1=$2
>>> +	size=$3
>>> +
>>> +	mkfifo input &&
>>> +	test_when_finished 'rm input' &&
>>> +	mkfifo output &&
>>> +	exec 9<>output &&
>>> +	test_when_finished 'rm output; exec 9<&-'
>>> +	(
>>> +		# TODO - Ideally we'd pipe the output of cat-file
>>> +		# through "sed s'/$/\\/'" to make sure that that read
>>> +		# would consume all the available
>>> +		# output. Unfortunately we cannot do this as we cannot
>>> +		# control when sed flushes its output. We could write
>>> +		# a test helper in C that appended a '\' to the end of
>>> +		# each line and flushes its output after every line.
>>> +		git cat-file --buffer --batch-command <input 2>err &
>>> +		echo $! &&
>>> +		wait $!
>>> +	) >&9 &
>>> +	sh_pid=$! &&
>>> +	read cat_file_pid <&9 &&
>>> +	test_when_finished "kill $cat_file_pid
>>> +			    kill $sh_pid; wait $sh_pid; :" &&
>>> +	echo "$sha1 $type $size" >expect &&
>>> +	test_write_lines "info $sha1" flush "info $sha1" >input
>>
>> This closes input and so cat-file exits and flushes its output -
>> therefore you are not testing whether flush actually flushes. When I
>> wrote this test in[1] this line was inside a subshell that was
>> redirected to the input fifo so that the read happened before cat-file
>> exited.
>
> Yeah, very good point.
>
>> This test is also not testing the exit code of cat-file or
>> that the output is flushed on exit. Is there a reason you can't just
>> use the test as I wrote it? I'm happy to explain anything that isn't
>> clear.
>
> I admit I do not offhand recall what your tests did but help with
> this (and more) level of detail with an offer to collaborate is
> something I am very happy to see.  Thanks for working well together.
>
> One thing that I wasn't quite sure was how well failure cases are
> tested.  If we ask, in a batch mode, "info" for two objects and then
> "flush", does the asker get enough clue when to read and when to
> stop reading with all four combinations of states, i.e. asking for
> two missing objects, one good object and one bad object, one bad
> object and one good object, two good objects, for example?

This is a good point. We currently don't have tests that exercise these
combinations.

>
> Testing such combinations reliably is tricky---if the asker needs to
> react to different response differently, a test that expects good
> and then bad may not just fail but can get into deadlock, for
> example if the reaction to good response has to read a lot but the
> reaction to bad response is to just consume the "bad object" notice,
> when a bug in the program being tested makes it issue the response
> for a bad case when the asker is expecting a response for a good
> object, because the asker will keep waiting for more response to
> read which may not come.

Let me see if I understand you. What I'm hearing is that it's hard to test a git
processes (A) that read/write from/to pipes without knowing exactly how (A) will
behave. By necessity, the test logic will have embedded some logic in it that
assumes certain behavior from (A), which might or might not be the case.

This can lead to a hanging test if, say, it is waiting around for (A) to output
data when due to a bug in the code, it never does. Did I get that right?

I still see value in having a test that hangs when it doesn't receive expected
output from the git process. If we had something that detected timeout on tests
then this could catch such a case. But since we don't, then that means having
tests like run_buffer_test_flush() and run_buffer_test_no_flush() will run the
risk of being a deadlocked test if there is a regression of the code in the future.
While still providing value in showing that something is wrong, these deadlocked
tests can be inconvenient to debug.