diff mbox series

[2/2] t9700: ensure cat-file info isn't buffered by default

Message ID 20240617104326.3522535-3-e@80x24.org (mailing list archive)
State New
Headers show
Series cat-file related doc and test | expand

Commit Message

Eric Wong June 17, 2024, 10:43 a.m. UTC
While working on buffering changes to `git cat-file' in a
separate patch, I inadvertently made the output of --batch-check
and the `info' command of --batch-command buffered by default.

Buffering by default breaks some 3rd-party Perl scripts using
cat-file, but this breakage was not detected anywhere in our
test suite.  The easiest place to test this behavior is with
Git.pm, since (AFAIK) other equivalent way to test this behavior
from Bourne shell and/or awk would require racy sleeps,
non-portable FIFOs or tedious C code.

Signed-off-by: Eric Wong <e@80x24.org>
---
 t/t9700/test.pl | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Junio C Hamano June 17, 2024, 8:50 p.m. UTC | #1
Eric Wong <e@80x24.org> writes:

> Buffering by default breaks some 3rd-party Perl scripts using
> cat-file, but this breakage was not detected anywhere in our
> test suite.  The easiest place to test this behavior is with
> Git.pm, since (AFAIK) other equivalent way to test this behavior
> from Bourne shell and/or awk would require racy sleeps,
> non-portable FIFOs or tedious C code.

Yes, using Perl is a good substitute for writing it in C in this
case.  I however question the choice to use t9700/test.pl here,
which is clearly stated that its purpose is to "test perl interface
which is Git.pm", and added tests are not testing anything in Git.pm
at all.

Using t9700/test.pl only because it happens to use "perl -MTest::More"
sounds a bit eh, suboptimal.

It seems that there are Perl snippets in other tests (including
t1006 that is specifically about cat-file).  How involved would it
be to implement these new tests without modifying unrelated test
scripts?

> Signed-off-by: Eric Wong <e@80x24.org>
> ---
>  t/t9700/test.pl | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/t/t9700/test.pl b/t/t9700/test.pl
> index d8e85482ab..94a2e2c09d 100755
> --- a/t/t9700/test.pl
> +++ b/t/t9700/test.pl
> @@ -154,6 +154,20 @@ sub adjust_dirsep {
>  		     "abc\"\\ \x07\x08\x09\x0a\x0b\x0c\x0d\x01 ",
>  		     'unquote escape sequences');
>  
> +# ensure --batch-check is unbuffered by default
> +my ($pid, $in, $out, $ctx) = $r->command_bidi_pipe(qw(cat-file --batch-check));
> +print $out $file1hash, "\n" or die $!;
> +my $info = <$in>;
> +is $info, "$file1hash blob 15\n", 'command_bidi_pipe w/ --batch-check';
> +$r->command_close_bidi_pipe($pid, $in, $out, $ctx);
> +
> +# ditto with `info' with --batch-command
> +($pid, $in, $out, $ctx) = $r->command_bidi_pipe(qw(cat-file --batch-command));
> +print $out 'info ', $file1hash, "\n" or die $!;
> +$info = <$in>;
> +is $info, "$file1hash blob 15\n", 'command_bidi_pipe w/ --batch-command=info';
> +$r->command_close_bidi_pipe($pid, $in, $out, $ctx);
> +
>  printf "1..%d\n", Test::More->builder->current_test;
>  
>  my $is_passing = eval { Test::More->is_passing };
Junio C Hamano June 17, 2024, 11:08 p.m. UTC | #2
Eric Wong <e@80x24.org> writes:

> While working on buffering changes to `git cat-file' in a
> separate patch, I inadvertently made the output of --batch-check
> and the `info' command of --batch-command buffered by default.

Here "buffered" means "as if opt->buffer_output is turned on", which
in turn means "the output goes through stdio"?  Just making sure
that my understanding of what the breakage was is in line with what
you wanted to convey.

Thanks.
Phillip Wood June 19, 2024, 9:08 a.m. UTC | #3
Hi Eric

On 17/06/2024 11:43, Eric Wong wrote:
> +# ensure --batch-check is unbuffered by default
> +my ($pid, $in, $out, $ctx) = $r->command_bidi_pipe(qw(cat-file --batch-check));
> +print $out $file1hash, "\n" or die $!;

It's been a while since I did any perl scripting and I'm not clear 
whether $out is buffered or not and if it is whether it is guaranteed to 
be flushed when we print "\n". It might be worth adding a explicit flush 
so it is clear that any deadlocks come from cat-file and not our test code.

> +my $info = <$in>;

Is there an easy way to add a timeout to this read so that the failure 
mode isn't "the test hangs without printing anything"? I'm not sure that 
failure mode is easy to diagnose from our CI output as it is hard to 
tell which test caused the CI to timeout and it takes ages for the CI to 
time out.

Best Wishes

Phillip

> +is $info, "$file1hash blob 15\n", 'command_bidi_pipe w/ --batch-check';
> +$r->command_close_bidi_pipe($pid, $in, $out, $ctx);
> +
> +# ditto with `info' with --batch-command
> +($pid, $in, $out, $ctx) = $r->command_bidi_pipe(qw(cat-file --batch-command));
> +print $out 'info ', $file1hash, "\n" or die $!;
> +$info = <$in>;
> +is $info, "$file1hash blob 15\n", 'command_bidi_pipe w/ --batch-command=info';
> +$r->command_close_bidi_pipe($pid, $in, $out, $ctx);
> +
>   printf "1..%d\n", Test::More->builder->current_test;
>   
>   my $is_passing = eval { Test::More->is_passing };
>
Eric Wong June 19, 2024, 6:08 p.m. UTC | #4
Phillip Wood <phillip.wood123@gmail.com> wrote:
> Hi Eric
> 
> On 17/06/2024 11:43, Eric Wong wrote:
> > +# ensure --batch-check is unbuffered by default
> > +my ($pid, $in, $out, $ctx) = $r->command_bidi_pipe(qw(cat-file --batch-check));
> > +print $out $file1hash, "\n" or die $!;
> 
> It's been a while since I did any perl scripting and I'm not clear whether
> $out is buffered or not and if it is whether it is guaranteed to be flushed
> when we print "\n". It might be worth adding a explicit flush so it is clear
> that any deadlocks come from cat-file and not our test code.

Pipes and sockets created by Perl are always unbuffered since
5.8, at least.  If they were buffered, Git.pm users (including
git-svn) wouldn't have worked at all.

> > +my $info = <$in>;
> 
> Is there an easy way to add a timeout to this read so that the failure mode
> isn't "the test hangs without printing anything"? I'm not sure that failure
> mode is easy to diagnose from our CI output as it is hard to tell which test
> caused the CI to timeout and it takes ages for the CI to time out.

Yeah, select() has been added in v2.
Phillip Wood June 21, 2024, 1:03 p.m. UTC | #5
On 19/06/2024 19:08, Eric Wong wrote:
> Phillip Wood <phillip.wood123@gmail.com> wrote:
>> Hi Eric
>>
>> On 17/06/2024 11:43, Eric Wong wrote:
>>> +# ensure --batch-check is unbuffered by default
>>> +my ($pid, $in, $out, $ctx) = $r->command_bidi_pipe(qw(cat-file --batch-check));
>>> +print $out $file1hash, "\n" or die $!;
>>
>> It's been a while since I did any perl scripting and I'm not clear whether
>> $out is buffered or not and if it is whether it is guaranteed to be flushed
>> when we print "\n". It might be worth adding a explicit flush so it is clear
>> that any deadlocks come from cat-file and not our test code.
> 
> Pipes and sockets created by Perl are always unbuffered since
> 5.8, at least.  If they were buffered, Git.pm users (including
> git-svn) wouldn't have worked at all.

Thanks for clarifying that

>>> +my $info = <$in>;
>>
>> Is there an easy way to add a timeout to this read so that the failure mode
>> isn't "the test hangs without printing anything"? I'm not sure that failure
>> mode is easy to diagnose from our CI output as it is hard to tell which test
>> caused the CI to timeout and it takes ages for the CI to time out.
> 
> Yeah, select() has been added in v2.

That's much nicer.

Thanks

Phillip
diff mbox series

Patch

diff --git a/t/t9700/test.pl b/t/t9700/test.pl
index d8e85482ab..94a2e2c09d 100755
--- a/t/t9700/test.pl
+++ b/t/t9700/test.pl
@@ -154,6 +154,20 @@  sub adjust_dirsep {
 		     "abc\"\\ \x07\x08\x09\x0a\x0b\x0c\x0d\x01 ",
 		     'unquote escape sequences');
 
+# ensure --batch-check is unbuffered by default
+my ($pid, $in, $out, $ctx) = $r->command_bidi_pipe(qw(cat-file --batch-check));
+print $out $file1hash, "\n" or die $!;
+my $info = <$in>;
+is $info, "$file1hash blob 15\n", 'command_bidi_pipe w/ --batch-check';
+$r->command_close_bidi_pipe($pid, $in, $out, $ctx);
+
+# ditto with `info' with --batch-command
+($pid, $in, $out, $ctx) = $r->command_bidi_pipe(qw(cat-file --batch-command));
+print $out 'info ', $file1hash, "\n" or die $!;
+$info = <$in>;
+is $info, "$file1hash blob 15\n", 'command_bidi_pipe w/ --batch-command=info';
+$r->command_close_bidi_pipe($pid, $in, $out, $ctx);
+
 printf "1..%d\n", Test::More->builder->current_test;
 
 my $is_passing = eval { Test::More->is_passing };