[v2,3/5] test: introduce new x_to_tab() helper function

Commit Message

Dragan Simic March 17, 2024, 3:48 a.m. UTC

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 <[e-mail]>
---

Notes:
    This patch didn't exist in the v1 of this patch series.

 t/test-lib-functions.sh | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Eric Sunshine March 17, 2024, 4:03 a.m. UTC | #1

On Sat, Mar 16, 2024 at 11:48 PM Dragan Simic <[e-mail]> 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 <[e-mail]>
> ---
> 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
        ...
    '

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.

Dragan Simic March 17, 2024, 4:16 a.m. UTC | #2

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 <[e-mail]> 
> 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 <[e-mail]>
>> ---
>> 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.

Patch

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 6eaf116346be..362d3205b7b0 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -107,6 +107,10 @@  q_to_tab () {
 	tr Q '\011'
 }
 
+x_to_tab () {
+	tr X '\011'
+}
+
 qz_to_tab_space () {
 	tr QZ '\011\040'
 }