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

[Eric Sunshine <sunshine@sunshineco.com>]

On Sun, Dec 4, 2022 at 4:41 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> On Sat, Dec 03 2022, René Scharfe wrote:
> > Am 03.12.22 um 13:53 schrieb Ævar Arnfjörð Bjarmason:
> >> On Sat, Dec 03 2022, René Scharfe wrote:
> >>> 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:
> >>>>> -       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 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.
> >
> > That's a good general guideline, but I wouldn't have expected a pipe
> > with three holes to startle anyone. *shrug*
>
> It's more that you're used to seeing one thing, the "cat >in" at the
> start of a function is a common pattern.
>
> Then it takes some time to stop and grok an a new pattern. If I was
> hacking on a function like that I'd probably stop to try to understand
> "why", even though I understood the "what".
>
> I'm not saying it's not worth it in this case, just pointing out that
> boring "standard" patterns have a value of their own in us collectively
> understanding them, which has a value of its own. Whether optimizing a
> test case outweighs that is another matter (sometimes it would).

Perhaps my experience is atypical, but in decades of using Unix, my
use of `tee` can (probably) be counted on a single finger, so the
patch, as implemented, did have higher cognitive load for me than a
patch using simple redirection would have had. Anyhow, I mentioned the
redirection approach, not to ask for a change, but only in case you
had overlooked the (to me) simpler approach. I didn't expect it to
spark so much discussion (though I do agree with everything Ævar has
said about following established patterns).

That said, I'm still rather unclear on the purpose of this patch. In a
sense, it feels like mere churn for 1/100 of a second gain (assuming
I'm reading the `hyperfine` output correctly).