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

Hello Eric,

On 2024-03-15 20:39, Eric Sunshine wrote:

On Fri, Mar 15, 2024 at 9:22 AM Dragan Simic [off-list ref] 
wrote:

quoted

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 <redacted>

Just some minor style-related comments...

quoted

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.