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

From: Eric Wong <e@80x24.org>
Date: Jun 21, 2024, 20:00 UTC
Message-ID: <20240621200002.M726804@dcvr>
In-Reply-To: <20240621071640.GD2105230@coredump.intra.peff.net>

Jeff King <peff@peff.net> wrote:

> On Tue, Jun 18, 2024 at 09:30:41PM +0000, Eric Wong wrote:
> 
> > +script='

<snip>

Show 17 quoted lines

> > +expect="$hello_oid blob $hello_size"
> > +
> > +test_expect_success PERL '--batch-check is unbuffered by default' '
> > +	perl -e "$script" -- --batch-check $hello_oid "$expect"
> > +'
> 
> We often use "perl -e" for one-liners, etc, but this is pretty big.
> Maybe:
> 
>   cat >foo.pl <<-\EOF
>   ...
>   EOF
>   perl foo.pl -- ...
> 
> would be more readable? To be clear I don't think there's anything
> incorrect about your usage, but it would match the style of our suite a
> bit better.

*shrug* It doesn't save the nested quoting/expansion confusion; but it's Junio's call. I don't think a v3 is worth the effort.

> Likewise, it would be usual in our suite for the helper to do the
> minimum that needs to be in perl, and use our normal functions for
> things like comparing output (rather than taking its own "expect"
> argument).

<snip>

Show 5 quoted lines

> +test_expect_success PERL '--batch-check is unbuffered by default' '
> +	echo "$hello_oid" |
> +	perl run-and-wait.pl git cat-file --batch-check >out &&
> +	echo "$hello_oid blob $hello_size" >expect &&
> +	test_cmp expect out

I prefer to avoid process spawning overhead from test_cmp; but that's a small drop in a big bucket.

Show 11 quoted lines

> I went for brevity above. Notably missing are:
> 
>   - the use of strict/warnings. I think we've shied away from these in
>     the test suite because we want to run on any version of perl. In my
>     experience most strict/warnings output is actually telling you about
>     obvious garbage, but not always. IIRC perl got more strict about
>     "()" around lists in some contexts a few years back, and code which
>     used to be OK started generating warnings. OTOH, those warnings were
>     probably a sign of problems-to-come, anyway. Without "FATAL",
>     though, I think "use warnings" is not doing much good (nobody is
>     ever going to see its output if the test isn't failing).

It may make problems easier to find if there are failures, so I think the potential benefits outweight any downsides.

>   - I dropped the close/waitpid. I guess maybe it is valuable to confirm
>     that cat-file did not barf, but IMHO the important thing here is
>     testing that it produced the single line of output we expected.

I've found some unexpected bugs through excessive error checking in the past, so much preferred to keep them.