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

> Hi John
>
> I've concentrated on the tests again, I think the flush tests still need some work but the others are looking good
>
>> [...]>   Documentation/git-cat-file.txt |  41 +++++++-
>>   builtin/cat-file.c             | 133 ++++++++++++++++++++++++
>>   t/t1006-cat-file.sh            | 178 ++++++++++++++++++++++++++++++++-
>>   3 files changed, 347 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
>> index bef76f4dd06..e8da704477d 100644
>> --- a/Documentation/git-cat-file.txt
>> +++ b/Documentation/git-cat-file.txt
>> @@ -96,6 +96,32 @@ OPTIONS
>>   	need to specify the path, separated by whitespace.  See the
>>   	section `BATCH OUTPUT` below for details.
>>  +--batch-command::
>
> +--batch-command=<format>::
>
> as we also take an optional format string

>
>> +	Enter a command mode that reads commands and arguments from stdin. May
>> +	only be combined with `--buffer`, `--textconv` or `--filters`. In the
>> +	case of `--textconv` or `--filters`, the input lines also need to specify
>> +	the path, separated by whitespace. See the section `BATCH OUTPUT` below
>> +	for details.
>> ++
>> +`--batch-command` recognizes the following commands:
>> ++
>> +--
>> +contents `<object>`::
>> +	Print object contents for object reference `<object>`. This corresponds to
>> +	the output of `--batch`.
>> +
>> +info `<object>`::
>> +	Print object info for object reference `<object>`. This corresponds to the
>> +	output of `--batch-check`.
>> +
>> +flush::
>> +	Used with `--buffer` to execute all preceding commands that were issued
>> +	since the beginning or since the last flush was issued. When `--buffer`
>> +	is used, no output will come until a `flush` is issued. When `--buffer`
>> +	is not used, commands are flushed each time without issuing `flush`.
>> +--
>> ++
>> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
>> index 145eee11df9..a501dbcc39b 100755
>> --- a/t/t1006-cat-file.sh
>> +++ b/t/t1006-cat-file.sh
>> @@ -177,6 +177,24 @@ $content"
>>   	test_cmp expect actual
>>       '
>>  +    for opt in --buffer --no-buffer
>> +    do
>> +	test -z "$content" ||
>> +		test_expect_success "--batch-command $opt output of $type content is correct" '
>> +		maybe_remove_timestamp "$batch_output" $no_ts >expect &&
>> +		maybe_remove_timestamp "$(test_write_lines "contents $sha1" \
>> +		| git cat-file --batch-command $opt)" $no_ts >actual &&
>> +		test_cmp expect actual
>> +	'
>> +
>> +	test_expect_success "--batch-command $opt output of $type info is correct" '
>> +		echo "$sha1 $type $size" >expect &&
>> +		test_write_lines "info $sha1" \
>> +		| git cat-file --batch-command $opt >actual &&
>> +		test_cmp expect actual
>> +	'
>> +    done
>> +
>>       test_expect_success "custom --batch-check format" '
>>   	echo "$type $sha1" >expect &&
>>   	echo $sha1 | git cat-file --batch-check="%(objecttype) %(objectname)" >actual &&
>> @@ -213,6 +231,84 @@ $content"
>>       '
>>   }
>>  +run_buffer_test_flush () {
>> +	type=$1
>> +	sha1=$2
>> +	size=$3
>> +
>> +	rm -f input output &&
>
> I think that this should not be needed with the addition of "test_when_finished 'rm input output'" in run_buffer_test_no_flush()
>
>> +	mkfifo input &&
>> +	test_when_finished 'rm input'
>> +	mkfifo output &&
>> +	exec 9<>output &&
>
> To address the worries about this test hanging rather than failing if something goes wrong I wonder if we could do something like
>
>     (
>     	sleep 10
>     	echo "error: timeout" >&2
>     	echo TIMEOUT >&9
>     ) &
>     watchdog_pid=$! &&
>     test_when_finished 'kill $watchdog_pid; wait $watchdog_pid'
>
> That should unblock any reads from fd 9 if the test hangs
>
>> +	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 $!
>> +		echo $?
>> +	) >&9 &
>> +	sh_pid=$! &&
>> +	read cat_file_pid <&9 &&
>> +	test_when_finished "kill $cat_file_pid
>> +			    kill $sh_pid; wait $sh_pid; :" &&
>> +	(
>> +		test_write_lines "info $sha1" flush "info $sha1" &&
>> +		# TODO - consume all available input, not just one
>> +		# line (see above).
>> +		read actual <&9 &&
>> +		echo "$actual" >actual &&
>> +		echo "$sha1 $type $size" >expect &&
>> +		test_cmp expect actual
>> +	) >input &&
>> +	# check output is flushed on exit
>> +	read actual <&9 &&
>> +	echo "$actual" >actual &&
>> +	test_cmp expect actual &&
>> +	test_must_be_empty err &&
>> +	read status <&9 &&
>> +	test "$status" -eq 0
>> +}
>> +
>> +run_buffer_test_no_flush () {
>
> This test reliably hangs for me when running with --stress
>
>> +	type=$1
>> +	sha1=$2
>> +	size=$3
>> +
>> +	touch output &&
>
> If output is missing at the end it means cat-file never ran which is an error which we do not want to hide. This is because the subshell creates output after opening input and before it executes cat-file below. As input is a fifo the open will block until it is opened for writing by another process and nothing wrote to it in V4 so I think that is why you saw an error there.
>
>> +	test_when_finished 'rm output'
>> +	mkfifo input &&
>> +	test_when_finished 'rm input'
>> +	mkfifo pid &&
>> +	exec 9<>pid &&
>> +	test_when_finished 'rm pid; exec 9<&-'
>> +	(
>> +		git cat-file --buffer --batch-command <input >>output &
>> +		echo $! &&
>> +		wait $!
>> +		echo $?
>> +	) >&9 &
>> +	sh_pid=$! &&
>> +	read cat_file_pid <&9 &&
>> +	test_when_finished "kill $cat_file_pid
>> +			    kill $sh_pid; wait $sh_pid; :" &&
>> +	(
>> +		test_write_lines "info $sha1" "info $sha1" &&
>> +		kill $cat_file_pid &&
>> +		read status <&9 &&
>
> This is where the test hangs. There seems to be a race (which I don't understand) where we're able to read the pid of cat-file but it is not killed by the kill above (the subshell above is blocked on "wait $!"). Adding "sleep 1" before the kill above makes everything work but I'm not very comfortable with it. I think we might be better taking a different approach and introducing an environment variable such as GIT_TEST_CAT_FILE_NO_FLUSH_ON_EXIT which stops cat-file flushing its output on exit and having a test along the lines of
>
> test_write_lines "info $sha1" | GIT_TEST_CAT_FILE_NO_FLUSH_ON_EXIT=1 git cat-file --batch-command --buffer >output &&
> test_must_be_empty_output

>
>
>
>> +		test "$status" -ne 0 &&
>> +		test_must_be_empty output
>> +	) >input
>> +}
>> +
>> +
>>   hello_content="Hello World"
>>   hello_size=$(strlen "$hello_content")
>>   hello_sha1=$(echo_without_newline "$hello_content" | git hash-object --stdin)
>> @@ -224,6 +320,14 @@ test_expect_success "setup" '
>>    run_tests 'blob' $hello_sha1 $hello_size "$hello_content" "$hello_content"
>>  +test_expect_success PIPE '--batch-command --buffer with flush for blob info' '
>> +       run_buffer_test_flush blob $hello_sha1 $hello_size
>> +'
>> +
>> +test_expect_success PIPE '--batch-command --buffer without flush for blob info' '
>> +       run_buffer_test_no_flush blob $hello_sha1 $hello_size false
>> +'
>> +
>>   test_expect_success '--batch-check without %(rest) considers whole line' '
>>   	echo "$hello_sha1 blob $hello_size" >expect &&
>>   	git update-index --add --cacheinfo 100644 $hello_sha1 "white space" &&
>> @@ -267,7 +371,7 @@ test_expect_success \
>>       "Reach a blob from a tag pointing to it" \
>>       "test '$hello_content' = \"\$(git cat-file blob $tag_sha1)\""
>>  -for batch in batch batch-check
>> +for batch in batch batch-check batch-command
>>   do
>>       for opt in t s e p
>>       do
>> @@ -373,6 +477,43 @@ test_expect_success "--batch-check with multiple sha1s gives correct format" '
>>       "$(echo_without_newline "$batch_check_input" | git cat-file --batch-check)"
>>   '
>>  +test_expect_success '--batch-command with multiple info calls gives correct format' '
>> +	cat >expect <<-EOF &&
>> +	$hello_sha1 blob $hello_size
>> +	$tree_sha1 tree $tree_size
>> +	$commit_sha1 commit $commit_size
>> +	$tag_sha1 tag $tag_size
>> +	deadbeef missing
>> +	EOF
>> +
>> +	test_write_lines "info $hello_sha1"\
>> +	"info $tree_sha1"\
>> +	"info $commit_sha1"\
>> +	"info $tag_sha1"\
>> +	"info deadbeef" | git cat-file --batch-command --buffer >actual &&
>> +	test_cmp expect actual
>> +'
>> +
>> +test_expect_success '--batch-command with multiple command calls gives correct format' '
>> +	cat >expect <<-EOF &&
>> +	$hello_sha1 blob $hello_size
>> +	$hello_content
>> +	$commit_sha1 commit $commit_size
>> +	$commit_content
>> +	$tag_sha1 tag $tag_size
>> +	$tag_content
>> +	deadbeef missing
>> +	EOF
>> +
>> +	maybe_remove_timestamp "$(cat expect)" 1 >expect &&
>> +	maybe_remove_timestamp "$(test_write_lines "contents $hello_sha1"\
>> +	"contents $commit_sha1"\
>> +	"contents $tag_sha1"\
>> +	"contents deadbeef"\
>> +	"flush" | git cat-file --batch-command --buffer)" 1 >actual &&
>> +	test_cmp expect actual
>
> It is a shame that maybe_remove_timestamp does no read from stdin, this test would look much nicer if it did. Apart from that these and the ones below are looking good

>
> Best Wishes
>
> Phillip
>
>> +'
>> +
>>   test_expect_success 'setup blobs which are likely to delta' '
>>   	test-tool genrandom foo 10240 >foo &&
>>   	{ cat foo && echo plus; } >foo-plus &&
>> @@ -963,5 +1104,40 @@ test_expect_success 'cat-file --batch-all-objects --batch-check ignores replace'
>>   	echo "$orig commit $orig_size" >expect &&
>>   	test_cmp expect actual
>>   '
>> +test_expect_success 'batch-command empty command' '
>> +	echo "" >cmd &&
>> +	test_expect_code 128 git cat-file --batch-command <cmd 2>err &&
>> +	grep "^fatal:.*empty command in input.*" err
>> +'
>> +
>> +test_expect_success 'batch-command whitespace before command' '
>> +	echo " info deadbeef" >cmd &&
>> +	test_expect_code 128 git cat-file --batch-command <cmd 2>err &&
>> +	grep "^fatal:.*whitespace before command.*" err
>> +'
>> +
>> +test_expect_success 'batch-command unknown command' '
>> +	echo unknown_command >cmd &&
>> +	test_expect_code 128 git cat-file --batch-command <cmd 2>err &&
>> +	grep "^fatal:.*unknown command.*" err
>> +'
>> +
>> +test_expect_success 'batch-command missing arguments' '
>> +	echo "info" >cmd &&
>> +	test_expect_code 128 git cat-file --batch-command <cmd 2>err &&
>> +	grep "^fatal:.*info requires arguments.*" err
>> +'
>> +
>> +test_expect_success 'batch-command flush with arguments' '
>> +	echo "flush arg" >cmd &&
>> +	test_expect_code 128 git cat-file --batch-command --buffer <cmd 2>err &&
>> +	grep "^fatal:.*flush takes no arguments.*" err
>> +'
>> +
>> +test_expect_success 'batch-command flush without --buffer' '
>> +	echo "flush arg" >cmd &&
>> +	test_expect_code 128 git cat-file --batch-command <cmd 2>err &&
>> +	grep "^fatal:.*flush is only for --buffer mode.*" err
>> +'
>>    test_done