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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 17, 2024, 04:21 UTC
Message-ID: <CAPig+cTVmQzC38DympSEtPNhgY=-+dYbZmkr0RTRbhG-hp2fmQ@mail.gmail.com>
In-Reply-To: <9a73e7d3cbb9ea210ed1098c5a304b0f5d5e1a2e.1710646998.git.dsimic@manjaro.org>

On Sat, Mar 16, 2024 at 11:48 PM Dragan Simic <dsimic@manjaro.org> wrote:

Show 17 quoted lines

> 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

Show 16 quoted lines

> +       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.

Show 10 quoted lines

> @@ -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().