Re: [PATCH 4/1] t3920: replace two cats with a tee

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 3, 2022, 12:53 UTC
Message-ID: <221203.86lenoeipb.gmgdl@evledraar.gmail.com>
In-Reply-To: <4611a23a-e7e9-6039-5d54-c2f8cadc2f73@web.de>

On Sat, Dec 03 2022, René Scharfe wrote:

Show 27 quoted lines

> Am 03.12.22 um 06:09 schrieb Eric Sunshine:
>> On Fri, Dec 2, 2022 at 11:51 AM René Scharfe <l.s.r@web.de> wrote:
>>> Use tee(1) to replace two calls of cat(1) for writing files with
>>> different line endings.  That's shorter and spawns less processes.
>>> [...]
>>> Signed-off-by: René Scharfe <l.s.r@web.de>
>>> ---
>>> diff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh
>>> @@ -9,8 +9,7 @@ LIB_CRLF_BRANCHES=""
>>>  create_crlf_ref () {
>>> -       cat >.crlf-orig-$branch.txt &&
>>> -       cat .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&
>>> +       tee .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&
>>
>> This feels slightly magical and more difficult to reason about than
>> using simple redirection to eliminate the second `cat`. Wouldn't this
>> work just as well?
>>
>>     cat >.crlf-orig-$branch.txt &&
>>     append_cr <.crlf-orig-$branch.txt >.crlf-message-$branch.txt &&
>
> It would work, of course, but this is the exact use case for tee(1).  No
> repetition, no extra redirection symbols, just an nicely fitting piece
> of pipework.  Don't fear the tee! ;-)
>
> (I'm delighted to learn from https://en.wikipedia.org/wiki/Tee_(command)
> that PowerShell has a tee command as well.)

I don't really care, but I must say I agree with Eric here. Not having surprising patterns in the test suite has a value of its own.

In this case I wonder if you want to optimize this whether we couldn't do much better with "test_commit_bulk", maybe by teaching it a small set of new tricks.

I.e. if I do:

	git fast-export --all

At the end of the setup test it seems we just end up with refs with names that correspond to their contents, and with double newlines in them or whatever. This is a lot of "grep", "sed", "tr" etc. just to end up with that.

So maybe we can create them as a patch, possibly with some slight "sed" munging on the input stream, just just teach it to accept a "ref prefix" and "commit message contents". That could just be an argument that you "$(printf "...")", so we don't even need a sub-process....

Also this:

     perl -wE 'say for 1..1024*100' | tee /tmp/x | perl -nE 'print "in: $_"; exit 1 if $_ == 512'; tail -n 1 /tmp/x

Isn't deterministic. Now, in this case I doubt it matters, but it's nice to have intermediate files in the test suite be determanistic, i.e. to always have the full content be in the file at the top after the "top".

With a "tee" you need to worry about the "append_cr" function it's being piped in stopping the stdin.

I don't think it matters in this case, but in general as a pattern: I do fear the "tee" a bit :)