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

From: Eric Sunshine <sunshine@sunshineco.com>
To: Dragan Simic <dsimic@manjaro.org>
Cc: git@vger.kernel.org, gitster@pobox.com, rsbecker@nexbridge.com,
	 github@seichter.de
Subject: Re: [PATCH v2 4/5] t1300: add more tests for whitespace and inline comments
Date: Sun, 17 Mar 2024 00:21:25 -0400	[thread overview]
Message-ID: <CAPig+cTVmQzC38DympSEtPNhgY=-+dYbZmkr0RTRbhG-hp2fmQ@mail.gmail.com> (raw)
In-Reply-To: <9a73e7d3cbb9ea210ed1098c5a304b0f5d5e1a2e.1710646998.git.dsimic@manjaro.org>

On Sat, Mar 16, 2024 at 11:48 PM 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, which may or may not be enclosed within quotation
> marks, or which contain an additional inline comment.
>
> At the same time, rework one already existing automated test a bit, to ensure
> consistency with the newly added tests.  This change introduced no functional
> changes to the already existing test.
>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
> ---
>     [2] https://lore.kernel.org/git/CAPig+cRG8eFxepkaiN54H+fa7D=rFGsmEHdvTP+HSSaLO_6T_A@mail.gmail.com/
>
> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> @@ -11,7 +11,97 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> +test_expect_success 'create test configuration' '

In [2] above, I intentionally suggested naming this new test "setup
whitespace" because "setup" is a common name used in the test suite
for this sort of test which prepares state for subsequent tests. Using
a common name (such as "setup") is important since it facilitates
running only specific tests within a script in which you are
interested rather than having to run all tests. The section "Skipping
Tests" in t/README says this:

    Sometimes there may be multiple tests with e.g. "setup" in their
    name that are needed and rather than figuring out the number for
    all of them we can just use "setup" as a substring/glob to match
    against the test description:

        $ sh ./t0050-filesystem.sh --run=setup,9-11

    or one could select both the setup tests and the rename ones
    (assuming all relevant tests had those words in their
    descriptions):

        $ sh ./t0050-filesystem.sh --run=setup,rename

> +       x_to_tab >.git/config <<-\EOF
> +       [section]
> +               Xsolid = rock
> +               Xsparse = big XX blue
> +               XsparseAndTail = big XX blue
> +               XsparseAndTailQuoted = "big XX blue "
> +               XsparseAndBiggerTail = big XX blue X X
> +               XsparseAndBiggerTailQuoted = "big XX blue X X"
> +               XsparseAndBiggerTailQuotedPlus = "big XX blue X X"X
> +               XheadAndTail = Xbig blue
> +               XheadAndTailQuoted = "Xbig blue "
> +               XheadAndTailQuotedPlus = "Xbig blue "
> +               Xannotated = big blueX# to be discarded
> +               XannotatedQuoted = "big blue"X# to be discarded
> +       EOF
> +'

The <<- operator strips all leading TAB characters, so the extra
indentation you've placed inside the "[section]" section is stripped
off. Thus, what you have above is the same as:

    x_to_tab >.git/config <<-\EOF
    [section]
    Xsolid = rock
    ...
    EOF

On a related note, it's not clear why you use 'X' to insert a TAB at
the beginning of each line. As I understand it, the configuration file
reader does not require such indentation, thus doing so is wasted.
Moreover, it confuses readers of this code (and reviewers) into
thinking that something unusual is going on, and leads to questions
such as this one: Why do you use 'X' to insert a TAB at the beginning
of the line?

> -test_expect_success 'clear default config' '
> +test_expect_success 'clear default configuration' '
>         rm -f .git/config
>  '

It's probably not worth a reroll, but it's usually better to avoid
this sort of do-nothing noise-change since it distracts reviewers from
the primary changes made by the patch.

> @@ -1066,9 +1156,25 @@ test_expect_success '--null --get-regexp' '
> -test_expect_success 'inner whitespace kept verbatim' '
> -       git config section.val "foo       bar" &&
> -       test_cmp_config "foo      bar" section.val
> +test_expect_success 'inner whitespace kept verbatim, spaces only' '
> +       echo "foo   bar" >expect &&
> +       git config section.val "foo   bar" &&
> +       git config --get section.val >actual &&
> +       test_cmp expect actual
> +'

I appreciate the revised test title ("spaces only") which indicates
that these aren't TABs which were missed when converting to use
q_to_tab() or x_to_tab().