From: Dragan Simic <dsimic@manjaro.org>
Date: Sun, 17 Mar 2024 04:16:02 GMT
Subject: Re: [PATCH v2 3/5] test: introduce new x_to_tab() helper function
Message-ID: <514d832b0399ccdbc354675068477fea@manjaro.org>
In-Reply-To: <CAPig+cSLb+Rsy81itvw9Tfvqv9vvKSPgO_ER9fWL04XZrgFvwg@mail.gmail.com>

Hello Eric,

Thanks for responding so quickly.  Please, see my comments below.

On 2024-03-17 05:03, Eric Sunshine wrote:
> On Sat, Mar 16, 2024 at 11:48 PM Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> There's nothing wrong with the already existing q_to_tab() function, 
>> except
>> when it's used on strings that contain uppercase letter "Q" in its 
>> literal
>> meaning, which, for example, can happen with git configurations that 
>> contain
>> "*.*Quoted" as the names of their configuration variables.
>> 
>> Thus, let's introduce new x_to_tab() helper function that does pretty 
>> much
>> the same job as the already existing q_to_tab() helper function, 
>> except for
>> replacing "X" with a horizontal tab (HT), instead of replacing "Q".
>> 
>> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
>> ---
>> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
>> @@ -107,6 +107,10 @@ q_to_tab () {
>> +x_to_tab () {
>> +       tr X '\011'
>> +}
> 
> I'd like to push back on this change since it may lead to an explosion
> of new almost-identical functions. For such a one-off case where
> q_to_tab() isn't appropriate, it's perfectly fine to simply use `tr X
> `\011'` directly in your test:
> 
>     test_expect_success 'foo' '
>         tr X "\011" >expect <<-\EOF
>         some Q stuff
>         whitespaceXhere
>         EOF
>         ...
>     '

Agreed, I'll take this approach in the v3.

> However, if you really insist upon using a library function, then
> either add a general-purpose function which accepts the special
> character as an argument, or just retrofit q_to_tab() to optionally
> accept the special character:
> 
>     # t/test-lib-functions.sh
> 
>     # usage: q_to_tab [<needle-char>]
>     # replace <needle-char> with TAB in stdin
>     q_to_tab () {
>         local c=$1
>         test -n "$c" || c=Q
>         tr "$c" '\011'
>     }
> 
> But this is probably overkill for a one-off case.

As far as I can see after doing a few really quick greps in the "t"
subdirectory, such an approach might actually make sense, but it would
require further work, to make some other already existing tests use
the enhanced q_to_tab() function, and to warrant the whole thing.

That might be an interesting #leftover for someone else to pick it up
at some point.