{"thread":{"id":"61641","subject":"[PATCH 0/2] cat-file related doc and test","startedAt":"2024-06-17T10:43:27Z","lastAt":"2024-06-24T15:19:15Z","messageCount":16,"participants":["Eric Wong","Junio C Hamano","Phillip Wood","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"497240","messageId":"20240617104326.3522535-1-e@80x24.org","threadId":"61641","inReplyTo":null,"subject":"[PATCH 0/2] cat-file related doc and test","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-06-17T10:43:24Z","receivedAt":"2024-06-17T10:43:27Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"I was working on reducing syscalls required for cat-file --batch\n(and readers) in a different patch, but noticed a bug in my\nyet-to-be-published patch wasn't detected in our test suite,\nonly via 3rd-party Perl code.\n\nThen I noticed Git.pm documentation was wrong..., so fixes\nare in reverse order.\n\nEric Wong (2):\n  Git.pm: use array in command_bidi_pipe example\n  t9700: ensure cat-file info isn't buffered by default\n\n perl/Git.pm     |  4 ++--\n t/t9700/test.pl | 14 ++++++++++++++\n 2 files changed, 16 insertions(+), 2 deletions(-)\n"},{"id":"497241","messageId":"20240617104326.3522535-2-e@80x24.org","threadId":"61641","inReplyTo":"20240617104326.3522535-1-e@80x24.org","subject":"[PATCH 1/2] Git.pm: use array in command_bidi_pipe example","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-06-17T10:43:25Z","receivedAt":"2024-06-17T10:43:34Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"command_bidi_pipe takes the git command and optional arguments as an\narray, not a string.  Make sure the documentation example is usable\ncode.\n\nSigned-off-by: Eric Wong <e@80x24.org>\n---\n perl/Git.pm | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 03bf570bf4..aebfe0c6e0 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -418,7 +418,7 @@ sub command_bidi_pipe {\n and it is the fourth value returned by C<command_bidi_pipe()>.  The call idiom\n is:\n \n-\tmy ($pid, $in, $out, $ctx) = $r->command_bidi_pipe('cat-file --batch-check');\n+\tmy ($pid, $in, $out, $ctx) = $r->command_bidi_pipe(qw(cat-file --batch-check));\n \tprint $out \"000000000\\n\";\n \twhile (<$in>) { ... }\n \t$r->command_close_bidi_pipe($pid, $in, $out, $ctx);\n@@ -431,7 +431,7 @@ sub command_bidi_pipe {\n calling this function.  This may be useful in a query-response type of\n commands where caller first writes a query and later reads response, eg:\n \n-\tmy ($pid, $in, $out, $ctx) = $r->command_bidi_pipe('cat-file --batch-check');\n+\tmy ($pid, $in, $out, $ctx) = $r->command_bidi_pipe(qw(cat-file --batch-check));\n \tprint $out \"000000000\\n\";\n \tclose $out;\n \twhile (<$in>) { ... }\n"},{"id":"497242","messageId":"20240617104326.3522535-3-e@80x24.org","threadId":"61641","inReplyTo":"20240617104326.3522535-1-e@80x24.org","subject":"[PATCH 2/2] t9700: ensure cat-file info isn't buffered by default","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-06-17T10:43:26Z","receivedAt":"2024-06-17T10:43:41Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"While working on buffering changes to `git cat-file' in a\nseparate patch, I inadvertently made the output of --batch-check\nand the `info' command of --batch-command buffered by default.\n\nBuffering by default breaks some 3rd-party Perl scripts using\ncat-file, but this breakage was not detected anywhere in our\ntest suite.  The easiest place to test this behavior is with\nGit.pm, since (AFAIK) other equivalent way to test this behavior\nfrom Bourne shell and/or awk would require racy sleeps,\nnon-portable FIFOs or tedious C code.\n\nSigned-off-by: Eric Wong <e@80x24.org>\n---\n t/t9700/test.pl | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\ndiff --git a/t/t9700/test.pl b/t/t9700/test.pl\nindex d8e85482ab..94a2e2c09d 100755\n--- a/t/t9700/test.pl\n+++ b/t/t9700/test.pl\n@@ -154,6 +154,20 @@ sub adjust_dirsep {\n \t\t     \"abc\\\"\\\\ \\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x01 \",\n \t\t     'unquote escape sequences');\n \n+# ensure --batch-check is unbuffered by default\n+my ($pid, $in, $out, $ctx) = $r->command_bidi_pipe(qw(cat-file --batch-check));\n+print $out $file1hash, \"\\n\" or die $!;\n+my $info = <$in>;\n+is $info, \"$file1hash blob 15\\n\", 'command_bidi_pipe w/ --batch-check';\n+$r->command_close_bidi_pipe($pid, $in, $out, $ctx);\n+\n+# ditto with `info' with --batch-command\n+($pid, $in, $out, $ctx) = $r->command_bidi_pipe(qw(cat-file --batch-command));\n+print $out 'info ', $file1hash, \"\\n\" or die $!;\n+$info = <$in>;\n+is $info, \"$file1hash blob 15\\n\", 'command_bidi_pipe w/ --batch-command=info';\n+$r->command_close_bidi_pipe($pid, $in, $out, $ctx);\n+\n printf \"1..%d\\n\", Test::More->builder->current_test;\n \n my $is_passing = eval { Test::More->is_passing };\n"},{"id":"497259","messageId":"xmqq8qz35mxg.fsf@gitster.g","threadId":"61641","inReplyTo":"20240617104326.3522535-2-e@80x24.org","subject":"Re: [PATCH 1/2] Git.pm: use array in command_bidi_pipe example","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-17T20:33:47Z","receivedAt":"2024-06-17T20:33:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong <e@80x24.org> writes:\n\n> command_bidi_pipe takes the git command and optional arguments as an\n> array, not a string.  Make sure the documentation example is usable\n> code.\n\nMakes sense.\n\n>\n> Signed-off-by: Eric Wong <e@80x24.org>\n> ---\n>  perl/Git.pm | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/perl/Git.pm b/perl/Git.pm\n> index 03bf570bf4..aebfe0c6e0 100644\n> --- a/perl/Git.pm\n> +++ b/perl/Git.pm\n> @@ -418,7 +418,7 @@ sub command_bidi_pipe {\n>  and it is the fourth value returned by C<command_bidi_pipe()>.  The call idiom\n>  is:\n>  \n> -\tmy ($pid, $in, $out, $ctx) = $r->command_bidi_pipe('cat-file --batch-check');\n> +\tmy ($pid, $in, $out, $ctx) = $r->command_bidi_pipe(qw(cat-file --batch-check));\n>  \tprint $out \"000000000\\n\";\n>  \twhile (<$in>) { ... }\n>  \t$r->command_close_bidi_pipe($pid, $in, $out, $ctx);\n> @@ -431,7 +431,7 @@ sub command_bidi_pipe {\n>  calling this function.  This may be useful in a query-response type of\n>  commands where caller first writes a query and later reads response, eg:\n>  \n> -\tmy ($pid, $in, $out, $ctx) = $r->command_bidi_pipe('cat-file --batch-check');\n> +\tmy ($pid, $in, $out, $ctx) = $r->command_bidi_pipe(qw(cat-file --batch-check));\n>  \tprint $out \"000000000\\n\";\n>  \tclose $out;\n>  \twhile (<$in>) { ... }\n"},{"id":"497260","messageId":"xmqq1q4v5m5a.fsf@gitster.g","threadId":"61641","inReplyTo":"20240617104326.3522535-3-e@80x24.org","subject":"Re: [PATCH 2/2] t9700: ensure cat-file info isn't buffered by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-17T20:50:41Z","receivedAt":"2024-06-17T20:50:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong <e@80x24.org> writes:\n\n> Buffering by default breaks some 3rd-party Perl scripts using\n> cat-file, but this breakage was not detected anywhere in our\n> test suite.  The easiest place to test this behavior is with\n> Git.pm, since (AFAIK) other equivalent way to test this behavior\n> from Bourne shell and/or awk would require racy sleeps,\n> non-portable FIFOs or tedious C code.\n\nYes, using Perl is a good substitute for writing it in C in this\ncase.  I however question the choice to use t9700/test.pl here,\nwhich is clearly stated that its purpose is to \"test perl interface\nwhich is Git.pm\", and added tests are not testing anything in Git.pm\nat all.\n\nUsing t9700/test.pl only because it happens to use \"perl -MTest::More\"\nsounds a bit eh, suboptimal.\n\nIt seems that there are Perl snippets in other tests (including\nt1006 that is specifically about cat-file).  How involved would it\nbe to implement these new tests without modifying unrelated test\nscripts?\n\n> Signed-off-by: Eric Wong <e@80x24.org>\n> ---\n>  t/t9700/test.pl | 14 ++++++++++++++\n>  1 file changed, 14 insertions(+)\n>\n> diff --git a/t/t9700/test.pl b/t/t9700/test.pl\n> index d8e85482ab..94a2e2c09d 100755\n> --- a/t/t9700/test.pl\n> +++ b/t/t9700/test.pl\n> @@ -154,6 +154,20 @@ sub adjust_dirsep {\n>  \t\t     \"abc\\\"\\\\ \\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x01 \",\n>  \t\t     'unquote escape sequences');\n>  \n> +# ensure --batch-check is unbuffered by default\n> +my ($pid, $in, $out, $ctx) = $r->command_bidi_pipe(qw(cat-file --batch-check));\n> +print $out $file1hash, \"\\n\" or die $!;\n> +my $info = <$in>;\n> +is $info, \"$file1hash blob 15\\n\", 'command_bidi_pipe w/ --batch-check';\n> +$r->command_close_bidi_pipe($pid, $in, $out, $ctx);\n> +\n> +# ditto with `info' with --batch-command\n> +($pid, $in, $out, $ctx) = $r->command_bidi_pipe(qw(cat-file --batch-command));\n> +print $out 'info ', $file1hash, \"\\n\" or die $!;\n> +$info = <$in>;\n> +is $info, \"$file1hash blob 15\\n\", 'command_bidi_pipe w/ --batch-command=info';\n> +$r->command_close_bidi_pipe($pid, $in, $out, $ctx);\n> +\n>  printf \"1..%d\\n\", Test::More->builder->current_test;\n>  \n>  my $is_passing = eval { Test::More->is_passing };\n"},{"id":"497264","messageId":"xmqqmsnj416n.fsf@gitster.g","threadId":"61641","inReplyTo":"20240617104326.3522535-3-e@80x24.org","subject":"Re: [PATCH 2/2] t9700: ensure cat-file info isn't buffered by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-17T23:08:48Z","receivedAt":"2024-06-17T23:08:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong <e@80x24.org> writes:\n\n> While working on buffering changes to `git cat-file' in a\n> separate patch, I inadvertently made the output of --batch-check\n> and the `info' command of --batch-command buffered by default.\n\nHere \"buffered\" means \"as if opt->buffer_output is turned on\", which\nin turn means \"the output goes through stdio\"?  Just making sure\nthat my understanding of what the breakage was is in line with what\nyou wanted to convey.\n\nThanks.\n\n"},{"id":"497302","messageId":"20240618213041.M462972@dcvr","threadId":"61641","inReplyTo":"xmqq1q4v5m5a.fsf@gitster.g","subject":"[PATCH v2 2/2] t1006: ensure cat-file info isn't buffered by default","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-06-18T21:30:41Z","receivedAt":"2024-06-18T21:30:47Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n> Eric Wong <e@80x24.org> writes:\n> \n> > Buffering by default breaks some 3rd-party Perl scripts using\n> > cat-file, but this breakage was not detected anywhere in our\n> > test suite.  The easiest place to test this behavior is with\n> > Git.pm, since (AFAIK) other equivalent way to test this behavior\n> > from Bourne shell and/or awk would require racy sleeps,\n> > non-portable FIFOs or tedious C code.\n> \n> Yes, using Perl is a good substitute for writing it in C in this\n> case.  I however question the choice to use t9700/test.pl here,\n> which is clearly stated that its purpose is to \"test perl interface\n> which is Git.pm\", and added tests are not testing anything in Git.pm\n> at all.\n> \n> Using t9700/test.pl only because it happens to use \"perl -MTest::More\"\n> sounds a bit eh, suboptimal.\n\n*shrug*  I figure Test::More is common enough since it's part of\nthe Perl standard library; but I consider Perl a better scripting\nlanguage than sh by far and wish our whole test suite were Perl :>\n\n> It seems that there are Perl snippets in other tests (including\n> t1006 that is specifically about cat-file).  How involved would it\n> be to implement these new tests without modifying unrelated test\n> scripts?\n\n> >  t/t9700/test.pl | 14 ++++++++++++++\n> >  1 file changed, 14 insertions(+)\n\nMore code than that.  At least IPC::Open2 takes care of the nasty\nportability bits, but getting the Perl quoting nested properly\ninside sh was confusing :x\n\nv2: moved test to t1006 to avoid Test::More,\n    add select timeout in case a buffering bug does get introduced,\n    updated commit message and clarified the bug it's supposed\n    to guard against\n    (I initially tried stdio buffering, but moved away from it for the\n    patch I'm testing...)\n\n----8<----\nSubject: [PATCH] t1006: ensure cat-file info isn't buffered by default\n\nWhile working on buffering changes to `git cat-file' in a\nseparate patch, I inadvertently made the output of --batch-check\nand the `info' command of --batch-command buffered as if\nopt->buffer_output is turned on by default.\n\nBuffering by default breaks some 3rd-party Perl scripts using\ncat-file, but this breakage was not detected anywhere in our\ntest suite.  Add a small Perl snippet to test this problem since\n(AFAIK) other equivalent ways to test this behavior from Bourne\nshell and/or awk would require racy sleeps, non-portable FIFOs\nor tedious C code.\n\nSigned-off-by: Eric Wong <e@80x24.org>\n---\n t/t1006-cat-file.sh | 30 ++++++++++++++++++++++++++++++\n 1 file changed, 30 insertions(+)\n\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex e12b221972..ff9bf213aa 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -1294,4 +1294,34 @@ test_expect_success 'batch-command flush without --buffer' '\n \tgrep \"^fatal:.*flush is only for --buffer mode.*\" err\n '\n \n+script='\n+use warnings;\n+use strict;\n+use IPC::Open2;\n+my ($opt, $oid, $expect, @pfx) = @ARGV;\n+my @cmd = (qw(git cat-file), $opt);\n+my $pid = open2(my $out, my $in, @cmd) or die \"open2: @cmd\";\n+print $in @pfx, $oid, \"\\n\" or die \"print $!\";\n+my $rvec = \"\";\n+vec($rvec, fileno($out), 1) = 1;\n+select($rvec, undef, undef, 30) or die \"no response to `@pfx $oid` from @cmd\";\n+my $info = <$out>;\n+chop($info) eq \"\\n\" or die \"no LF\";\n+$info eq $expect or die \"`$info` != `$expect`\";\n+close $in or die \"close in $!\";\n+close $out or die \"close out $!\";\n+waitpid $pid, 0;\n+$? == 0 or die \"\\$?=$?\";\n+'\n+\n+expect=\"$hello_oid blob $hello_size\"\n+\n+test_expect_success PERL '--batch-check is unbuffered by default' '\n+\tperl -e \"$script\" -- --batch-check $hello_oid \"$expect\"\n+'\n+\n+test_expect_success PERL '--batch-command info is unbuffered by default' '\n+\tperl -e \"$script\" -- --batch-command $hello_oid \"$expect\" \"info \"\n+'\n+\n test_done\n"},{"id":"497304","messageId":"xmqqzfrhyg8j.fsf@gitster.g","threadId":"61641","inReplyTo":"20240618213041.M462972@dcvr","subject":"Re: [PATCH v2 2/2] t1006: ensure cat-file info isn't buffered by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-18T23:37:48Z","receivedAt":"2024-06-18T23:38:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong <e@80x24.org> writes:\n\n>> Yes, using Perl is a good substitute for writing it in C in this\n>> case.  I however question the choice to use t9700/test.pl here,\n>> which is clearly stated that its purpose is to \"test perl interface\n>> which is Git.pm\", and added tests are not testing anything in Git.pm\n>> at all.\n>> \n>> Using t9700/test.pl only because it happens to use \"perl -MTest::More\"\n>> sounds a bit eh, suboptimal.\n>\n> *shrug*  I figure Test::More is common enough since it's part of\n> the Perl standard library; but I consider Perl a better scripting\n> language than sh by far and wish our whole test suite were Perl :>\n\nOh, I think we (actually the author of t9700) considers it common\nenough that we have PERL_TEST_MORE prerequisite to allow us to write\ntests, assuming that it is available, and let us easily skip where\nit is not available.  So I do not think I mind the dependency on\nTest::More at all.  Moving the tests to t1006 and rewriting the\ntests not to use Test::More are two separate and unrelated things,\nand if you are more comfortable with Test::More (and more\nimportantly if it is natural to write Perl based tests using\nTest::More), it is not necessary to switch away from it.\n\n"},{"id":"497341","messageId":"6e80eea5-b6ce-4218-8c43-dde2b5a698f5@gmail.com","threadId":"61641","inReplyTo":"20240617104326.3522535-3-e@80x24.org","subject":"Re: [PATCH 2/2] t9700: ensure cat-file info isn't buffered by default","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-06-19T09:08:40Z","receivedAt":"2024-06-19T09:08:41Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Eric\n\nOn 17/06/2024 11:43, Eric Wong wrote:\n> +# ensure --batch-check is unbuffered by default\n> +my ($pid, $in, $out, $ctx) = $r->command_bidi_pipe(qw(cat-file --batch-check));\n> +print $out $file1hash, \"\\n\" or die $!;\n\nIt's been a while since I did any perl scripting and I'm not clear \nwhether $out is buffered or not and if it is whether it is guaranteed to \nbe flushed when we print \"\\n\". It might be worth adding a explicit flush \nso it is clear that any deadlocks come from cat-file and not our test code.\n\n> +my $info = <$in>;\n\nIs there an easy way to add a timeout to this read so that the failure \nmode isn't \"the test hangs without printing anything\"? I'm not sure that \nfailure mode is easy to diagnose from our CI output as it is hard to \ntell which test caused the CI to timeout and it takes ages for the CI to \ntime out.\n\nBest Wishes\n\nPhillip\n\n> +is $info, \"$file1hash blob 15\\n\", 'command_bidi_pipe w/ --batch-check';\n> +$r->command_close_bidi_pipe($pid, $in, $out, $ctx);\n> +\n> +# ditto with `info' with --batch-command\n> +($pid, $in, $out, $ctx) = $r->command_bidi_pipe(qw(cat-file --batch-command));\n> +print $out 'info ', $file1hash, \"\\n\" or die $!;\n> +$info = <$in>;\n> +is $info, \"$file1hash blob 15\\n\", 'command_bidi_pipe w/ --batch-command=info';\n> +$r->command_close_bidi_pipe($pid, $in, $out, $ctx);\n> +\n>   printf \"1..%d\\n\", Test::More->builder->current_test;\n>   \n>   my $is_passing = eval { Test::More->is_passing };\n> \n"},{"id":"497367","messageId":"20240619175633.M826655@dcvr","threadId":"61641","inReplyTo":"xmqqzfrhyg8j.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] t1006: ensure cat-file info isn't buffered by default","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-06-19T17:56:33Z","receivedAt":"2024-06-19T17:56:34Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n> Eric Wong <e@80x24.org> writes:\n> \n> >> Yes, using Perl is a good substitute for writing it in C in this\n> >> case.  I however question the choice to use t9700/test.pl here,\n> >> which is clearly stated that its purpose is to \"test perl interface\n> >> which is Git.pm\", and added tests are not testing anything in Git.pm\n> >> at all.\n> >> \n> >> Using t9700/test.pl only because it happens to use \"perl -MTest::More\"\n> >> sounds a bit eh, suboptimal.\n> >\n> > *shrug*  I figure Test::More is common enough since it's part of\n> > the Perl standard library; but I consider Perl a better scripting\n> > language than sh by far and wish our whole test suite were Perl :>\n> \n> Oh, I think we (actually the author of t9700) considers it common\n> enough that we have PERL_TEST_MORE prerequisite to allow us to write\n> tests, assuming that it is available, and let us easily skip where\n> it is not available.  So I do not think I mind the dependency on\n> Test::More at all.  Moving the tests to t1006 and rewriting the\n> tests not to use Test::More are two separate and unrelated things,\n> and if you are more comfortable with Test::More (and more\n> importantly if it is natural to write Perl based tests using\n> Test::More), it is not necessary to switch away from it.\n\nOK, fair enough.  Given t1006 is mostly sh, I prefer keeping v2\nas-is because the Test::More->builder munging of test numbers in\nt9700/test.pl is nasty too and I wouldn't enjoy duplicating\nthose bits in a hypothetical t1006/test.pl, either.\n\nIt would be nice to have first class support for Test::More in\nour suite so we could just have t/t0006-cat-file.t and\nt/t9700-perl-git.t implemented in Perl without sh at all, but\nthat's a separate discussion.\n"},{"id":"497368","messageId":"20240619180807.M97115@dcvr","threadId":"61641","inReplyTo":"6e80eea5-b6ce-4218-8c43-dde2b5a698f5@gmail.com","subject":"Re: [PATCH 2/2] t9700: ensure cat-file info isn't buffered by default","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-06-19T18:08:07Z","receivedAt":"2024-06-19T18:08:07Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Phillip Wood <phillip.wood123@gmail.com> wrote:\n> Hi Eric\n> \n> On 17/06/2024 11:43, Eric Wong wrote:\n> > +# ensure --batch-check is unbuffered by default\n> > +my ($pid, $in, $out, $ctx) = $r->command_bidi_pipe(qw(cat-file --batch-check));\n> > +print $out $file1hash, \"\\n\" or die $!;\n> \n> It's been a while since I did any perl scripting and I'm not clear whether\n> $out is buffered or not and if it is whether it is guaranteed to be flushed\n> when we print \"\\n\". It might be worth adding a explicit flush so it is clear\n> that any deadlocks come from cat-file and not our test code.\n\nPipes and sockets created by Perl are always unbuffered since\n5.8, at least.  If they were buffered, Git.pm users (including\ngit-svn) wouldn't have worked at all.\n\n> > +my $info = <$in>;\n> \n> Is there an easy way to add a timeout to this read so that the failure mode\n> isn't \"the test hangs without printing anything\"? I'm not sure that failure\n> mode is easy to diagnose from our CI output as it is hard to tell which test\n> caused the CI to timeout and it takes ages for the CI to time out.\n\nYeah, select() has been added in v2.\n"},{"id":"497417","messageId":"xmqqiky3wmkl.fsf@gitster.g","threadId":"61641","inReplyTo":"20240619175633.M826655@dcvr","subject":"Re: [PATCH v2 2/2] t1006: ensure cat-file info isn't buffered by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-20T17:28:26Z","receivedAt":"2024-06-20T17:28:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong <e@80x24.org> writes:\n\n> OK, fair enough.  Given t1006 is mostly sh, I prefer keeping v2\n> as-is because the Test::More->builder munging of test numbers in\n> t9700/test.pl is nasty too and I wouldn't enjoy duplicating\n> those bits in a hypothetical t1006/test.pl, either.\n\nThat is quite sensible.\n"},{"id":"497469","messageId":"20240621071640.GD2105230@coredump.intra.peff.net","threadId":"61641","inReplyTo":"20240618213041.M462972@dcvr","subject":"Re: [PATCH v2 2/2] t1006: ensure cat-file info isn't buffered by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-21T07:16:40Z","receivedAt":"2024-06-21T07:16:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 18, 2024 at 09:30:41PM +0000, Eric Wong wrote:\n\n> +script='\n> +use warnings;\n> +use strict;\n> +use IPC::Open2;\n> +my ($opt, $oid, $expect, @pfx) = @ARGV;\n> +my @cmd = (qw(git cat-file), $opt);\n> +my $pid = open2(my $out, my $in, @cmd) or die \"open2: @cmd\";\n> +print $in @pfx, $oid, \"\\n\" or die \"print $!\";\n> +my $rvec = \"\";\n> +vec($rvec, fileno($out), 1) = 1;\n> +select($rvec, undef, undef, 30) or die \"no response to `@pfx $oid` from @cmd\";\n> +my $info = <$out>;\n> +chop($info) eq \"\\n\" or die \"no LF\";\n> +$info eq $expect or die \"`$info` != `$expect`\";\n> +close $in or die \"close in $!\";\n> +close $out or die \"close out $!\";\n> +waitpid $pid, 0;\n> +$? == 0 or die \"\\$?=$?\";\n> +'\n> +\n> +expect=\"$hello_oid blob $hello_size\"\n> +\n> +test_expect_success PERL '--batch-check is unbuffered by default' '\n> +\tperl -e \"$script\" -- --batch-check $hello_oid \"$expect\"\n> +'\n\nWe often use \"perl -e\" for one-liners, etc, but this is pretty big.\nMaybe:\n\n  cat >foo.pl <<-\\EOF\n  ...\n  EOF\n  perl foo.pl -- ...\n\nwould be more readable? To be clear I don't think there's anything\nincorrect about your usage, but it would match the style of our suite a\nbit better.\n\nLikewise, it would be usual in our suite for the helper to do the\nminimum that needs to be in perl, and use our normal functions for\nthings like comparing output (rather than taking its own \"expect\"\nargument).\n\nSo maybe:\n\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex e12b221972..929d7a7579 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -1294,4 +1294,33 @@ test_expect_success 'batch-command flush without --buffer' '\n \tgrep \"^fatal:.*flush is only for --buffer mode.*\" err\n '\n \n+# Copy a single line from stdin to the program specified\n+# by @ARGV, and then wait for a response _without_ closing\n+# the pipe.\n+cat >run-and-wait.pl <<-\\EOF\n+use IPC::Open2;\n+open2(my $out, my $in, @ARGV) or die \"open2: @ARGV\";\n+print $in scalar(<STDIN>) or die \"print $!\";\n+\n+my $rvec = \"\";\n+vec($rvec, fileno($out), 1) = 1;\n+select($rvec, undef, undef, 30) or die \"no response after 30 seconds\";\n+\n+print scalar(<$out>);\n+EOF\n+\n+test_expect_success PERL '--batch-check is unbuffered by default' '\n+\techo \"$hello_oid\" |\n+\tperl run-and-wait.pl git cat-file --batch-check >out &&\n+\techo \"$hello_oid blob $hello_size\" >expect &&\n+\ttest_cmp expect out\n+'\n+\n+test_expect_success PERL '--batch-command info is unbuffered by default' '\n+\techo \"info $hello_oid\" |\n+\tperl run-and-wait.pl git cat-file --batch-command >out &&\n+\techo \"$hello_oid blob $hello_size\" >expect &&\n+\ttest_cmp expect out\n+'\n+\n test_done\n\nI went for brevity above. Notably missing are:\n\n  - the use of strict/warnings. I think we've shied away from these in\n    the test suite because we want to run on any version of perl. In my\n    experience most strict/warnings output is actually telling you about\n    obvious garbage, but not always. IIRC perl got more strict about\n    \"()\" around lists in some contexts a few years back, and code which\n    used to be OK started generating warnings. OTOH, those warnings were\n    probably a sign of problems-to-come, anyway. Without \"FATAL\",\n    though, I think \"use warnings\" is not doing much good (nobody is\n    ever going to see its output if the test isn't failing).\n\n  - I dropped the close/waitpid. I guess maybe it is valuable to confirm\n    that cat-file did not barf, but IMHO the important thing here is\n    testing that it produced the single line of output we expected.\n\n-Peff\n"},{"id":"497493","messageId":"d5dc3cbd-72ae-4f1a-bd9d-d2608364a08c@gmail.com","threadId":"61641","inReplyTo":"20240619180807.M97115@dcvr","subject":"Re: [PATCH 2/2] t9700: ensure cat-file info isn't buffered by default","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-06-21T13:03:14Z","receivedAt":"2024-06-21T13:03:19Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 19/06/2024 19:08, Eric Wong wrote:\n> Phillip Wood <phillip.wood123@gmail.com> wrote:\n>> Hi Eric\n>>\n>> On 17/06/2024 11:43, Eric Wong wrote:\n>>> +# ensure --batch-check is unbuffered by default\n>>> +my ($pid, $in, $out, $ctx) = $r->command_bidi_pipe(qw(cat-file --batch-check));\n>>> +print $out $file1hash, \"\\n\" or die $!;\n>>\n>> It's been a while since I did any perl scripting and I'm not clear whether\n>> $out is buffered or not and if it is whether it is guaranteed to be flushed\n>> when we print \"\\n\". It might be worth adding a explicit flush so it is clear\n>> that any deadlocks come from cat-file and not our test code.\n> \n> Pipes and sockets created by Perl are always unbuffered since\n> 5.8, at least.  If they were buffered, Git.pm users (including\n> git-svn) wouldn't have worked at all.\n\nThanks for clarifying that\n\n>>> +my $info = <$in>;\n>>\n>> Is there an easy way to add a timeout to this read so that the failure mode\n>> isn't \"the test hangs without printing anything\"? I'm not sure that failure\n>> mode is easy to diagnose from our CI output as it is hard to tell which test\n>> caused the CI to timeout and it takes ages for the CI to time out.\n> \n> Yeah, select() has been added in v2.\n\nThat's much nicer.\n\nThanks\n\nPhillip\n\n"},{"id":"497522","messageId":"20240621200002.M726804@dcvr","threadId":"61641","inReplyTo":"20240621071640.GD2105230@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/2] t1006: ensure cat-file info isn't buffered by default","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-06-21T20:00:02Z","receivedAt":"2024-06-21T20:00:03Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Jeff King <peff@peff.net> wrote:\n> On Tue, Jun 18, 2024 at 09:30:41PM +0000, Eric Wong wrote:\n> \n> > +script='\n\n<snip>\n\n> > +expect=\"$hello_oid blob $hello_size\"\n> > +\n> > +test_expect_success PERL '--batch-check is unbuffered by default' '\n> > +\tperl -e \"$script\" -- --batch-check $hello_oid \"$expect\"\n> > +'\n> \n> We often use \"perl -e\" for one-liners, etc, but this is pretty big.\n> Maybe:\n> \n>   cat >foo.pl <<-\\EOF\n>   ...\n>   EOF\n>   perl foo.pl -- ...\n> \n> would be more readable? To be clear I don't think there's anything\n> incorrect about your usage, but it would match the style of our suite a\n> bit better.\n\n*shrug*  It doesn't save the nested quoting/expansion confusion;\nbut it's Junio's call.  I don't think a v3 is worth the effort.\n\n> Likewise, it would be usual in our suite for the helper to do the\n> minimum that needs to be in perl, and use our normal functions for\n> things like comparing output (rather than taking its own \"expect\"\n> argument).\n\n<snip>\n\n> +test_expect_success PERL '--batch-check is unbuffered by default' '\n> +\techo \"$hello_oid\" |\n> +\tperl run-and-wait.pl git cat-file --batch-check >out &&\n> +\techo \"$hello_oid blob $hello_size\" >expect &&\n> +\ttest_cmp expect out\n\nI prefer to avoid process spawning overhead from test_cmp;\nbut that's a small drop in a big bucket.\n\n> I went for brevity above. Notably missing are:\n> \n>   - the use of strict/warnings. I think we've shied away from these in\n>     the test suite because we want to run on any version of perl. In my\n>     experience most strict/warnings output is actually telling you about\n>     obvious garbage, but not always. IIRC perl got more strict about\n>     \"()\" around lists in some contexts a few years back, and code which\n>     used to be OK started generating warnings. OTOH, those warnings were\n>     probably a sign of problems-to-come, anyway. Without \"FATAL\",\n>     though, I think \"use warnings\" is not doing much good (nobody is\n>     ever going to see its output if the test isn't failing).\n\nIt may make problems easier to find if there are failures,\nso I think the potential benefits outweight any downsides.\n\n>   - I dropped the close/waitpid. I guess maybe it is valuable to confirm\n>     that cat-file did not barf, but IMHO the important thing here is\n>     testing that it produced the single line of output we expected.\n\nI've found some unexpected bugs through excessive error checking\nin the past, so much preferred to keep them.\n"},{"id":"497571","messageId":"20240624151906.GB19841@coredump.intra.peff.net","threadId":"61641","inReplyTo":"20240621200002.M726804@dcvr","subject":"Re: [PATCH v2 2/2] t1006: ensure cat-file info isn't buffered by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-24T15:19:06Z","receivedAt":"2024-06-24T15:19:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 21, 2024 at 08:00:02PM +0000, Eric Wong wrote:\n\n> > We often use \"perl -e\" for one-liners, etc, but this is pretty big.\n> > Maybe:\n> > \n> >   cat >foo.pl <<-\\EOF\n> >   ...\n> >   EOF\n> >   perl foo.pl -- ...\n> > \n> > would be more readable? To be clear I don't think there's anything\n> > incorrect about your usage, but it would match the style of our suite a\n> > bit better.\n> \n> *shrug*  It doesn't save the nested quoting/expansion confusion;\n> but it's Junio's call.  I don't think a v3 is worth the effort.\n\nIt does allow you to use single quotes in the script, though I think you\nmanaged without it.\n\n> > +test_expect_success PERL '--batch-check is unbuffered by default' '\n> > +\techo \"$hello_oid\" |\n> > +\tperl run-and-wait.pl git cat-file --batch-check >out &&\n> > +\techo \"$hello_oid blob $hello_size\" >expect &&\n> > +\ttest_cmp expect out\n> \n> I prefer to avoid process spawning overhead from test_cmp;\n> but that's a small drop in a big bucket.\n\nIf we care about that, I'd rather see us make test_cmp zero-process with\na shell helper than come up with ad-hoc solutions. I've tried to measure\nsomething like that before, but couldn't come up with any conclusive\nimprovements (my findings were mostly that running Git itself accounts\nfor most of the process overhead).\n\n-Peff\n"}]}