Re: [PATCH 3/4] t1300: add more tests for whitespace and inline comments

From: Dragan Simic <dsimic@manjaro.org>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: git@vger.kernel.org, gitster@pobox.com, rsbecker@nexbridge.com,
	github@seichter.de
Subject: Re: [PATCH 3/4] t1300: add more tests for whitespace and inline comments
Date: Fri, 15 Mar 2024 21:04:12 +0100	[thread overview]
Message-ID: <eb550eebb158064f1780f610669bbda1@manjaro.org> (raw)
In-Reply-To: <CAPig+cRMPNExbG34xJ0w5npUc3DDwxQUGS_AQfam_mi4s53=sA@mail.gmail.com>

Hello Eric,

On 2024-03-15 20:39, Eric Sunshine wrote:
> On Fri, Mar 15, 2024 at 9:22 AM Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> Add a handful of additional automated tests, to improve the coverage 
>> of
>> configuration file entries whose values contain internal whitespace, 
>> leading
>> and/or trailing whitespace, or which contain an additional inline 
>> comment.
>> 
>> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
> 
> Just some minor style-related comments...
> 
>> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
>> @@ -11,6 +11,96 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>> +cat > .git/config << EOF
>> +[section]
>> +       solid = rock
>> +       sparse = big             blue
>> +       sparseAndTail = big              blue
>> +       sparseAndTailQuoted = "big               blue "
>> +       sparseAndBiggerTail = big                blue
>> +       sparseAndBiggerTailQuoted = "big                 blue          
>>  "
>> +       sparseAndBiggerTailQuotedPlus =  "big            blue          
>>  "
>> +       headAndTail =   big blue
>> +       headAndTailQuoted = "   big blue "
>> +       headAndTailQuotedPlus =  "      big blue "
>> +       annotated = big blue    # to be discarded
>> +       annotatedQuoted = "big blue"    # to be discarded
>> +EOF
> 
> These days we try to place all test-related code inside a
> test_expect_success() context rather than having it standalone. In
> this case, since the file being created is (presumably) shared by
> multiple tests in this script, you may want to add a new test which
> performs this setup step.
> 
> Use \EOF rather than EOF to signal to readers that we don't expect any
> variable interpolation to happen within the here-doc body.
> 
> Further, use -\EOF inside test_expect_success() to allow us to indent
> the body of the heredoc to match the test indentation.
> 
> Style guideline says to omit whitespace after redirection operators
> (such as `<<` and `>`).
> 
> We have a q_to_tab() function which lets us state explicitly for
> readers the location of TAB characters in the heredoc body. You'll
> often see that used instead of literal TABs.
> 
> Taking all the above into account, perhaps:
> 
>     test_expect_success 'setup whitespace' '
>         q_to_tab >.git/config <<-\EOF
>         [section]
>         solid = rock
>         sparse = bigQblue
>         ...
>         EOF
> 
> Same comments apply to rest of patch.

Thank you for your review and all suggestions!  I'll make sure to rework
the tests in v2, in the way you described it above.

I'll come back with any questions I might have while reworking the new
tests.  I hope that's fine.