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

From: Eric Wong <e@80x24.org>
Date: Jun 18, 2024, 21:30 UTC
Message-ID: <20240618213041.M462972@dcvr>
In-Reply-To: <xmqq1q4v5m5a.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> wrote:

Show 17 quoted lines

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

*shrug* I figure Test::More is common enough since it's part of the Perl standard library; but I consider Perl a better scripting language than sh by far and wish our whole test suite were Perl :>

> 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?

> >  t/t9700/test.pl | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)

More code than that. At least IPC::Open2 takes care of the nasty portability bits, but getting the Perl quoting nested properly inside sh was confusing :x

v2: moved test to t1006 to avoid Test::More,
    add select timeout in case a buffering bug does get introduced,
    updated commit message and clarified the bug it's supposed
    to guard against
    (I initially tried stdio buffering, but moved away from it for the
    patch I'm testing...)

----8<----
Subject: [PATCH] t1006: ensure cat-file info isn't buffered by default

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 as if opt->buffer_output is turned on 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. Add a small Perl snippet to test this problem since (AFAIK) other equivalent ways 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/t1006-cat-file.sh | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index e12b221972..ff9bf213aa 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -1294,4 +1294,34 @@ test_expect_success 'batch-command flush without --buffer' '
 	grep "^fatal:.*flush is only for --buffer mode.*" err
 '
 
+script='
+use warnings;
+use strict;
+use IPC::Open2;
+my ($opt, $oid, $expect, @pfx) = @ARGV;
+my @cmd = (qw(git cat-file), $opt);
+my $pid = open2(my $out, my $in, @cmd) or die "open2: @cmd";
+print $in @pfx, $oid, "\n" or die "print $!";
+my $rvec = "";
+vec($rvec, fileno($out), 1) = 1;
+select($rvec, undef, undef, 30) or die "no response to `@pfx $oid` from @cmd";
+my $info = <$out>;
+chop($info) eq "\n" or die "no LF";
+$info eq $expect or die "`$info` != `$expect`";
+close $in or die "close in $!";
+close $out or die "close out $!";
+waitpid $pid, 0;
+$? == 0 or die "\$?=$?";
+'
+
+expect="$hello_oid blob $hello_size"
+
+test_expect_success PERL '--batch-check is unbuffered by default' '
+	perl -e "$script" -- --batch-check $hello_oid "$expect"
+'
+
+test_expect_success PERL '--batch-command info is unbuffered by default' '
+	perl -e "$script" -- --batch-command $hello_oid "$expect" "info "
+'
+
 test_done