On 2024-03-17 05:21, Eric Sunshine wrote:
On Sat, Mar 16, 2024 at 11:48 PM Dragan Simic <dsimic@xxxxxxxxxxx>
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@xxxxxxxxxxxxxx>
Signed-off-by: Dragan Simic <dsimic@xxxxxxxxxxx>
---
[2]
https://lore.kernel.org/git/CAPig+cRG8eFxepkaiN54H+fa7D=rFGsmEHdvTP+HSSaLO_6T_A@xxxxxxxxxxxxxx/
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
Totally agreed, thanks for pointing this out. Will be fixed in v3.
+ 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
Yes, I was already aware that such indentation ends up wasted, but
having
it makes the test a bit more readable. At least in my opinion, but if
you
find it better not to have such whitespace, for the sake of consistency,
I'll happily remove this indentation in the v3.
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?
Well, resorting to always not having such instances of 'X' to provide
leading indentation in test configuration files may actually make some
bugs go undetected in some tests. To me, having leading indentation is
to be expected in the real configuration files in the field, thus
providing
the same indentation in a test configuration feels natural to me,
despite
admittedly making the test configuration a bit less readable.
Of course, consistency is good, but variety is also good when it comes
to automated tests. I'm not very familiar with the tests in git, so
please let me know if consistency outweights variety in this case, and
I'll happily remove the leading "X" indentations in the v3.
-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.
The v3 is already inevitable, so I'll drop this change.
@@ -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().
Thanks. :)
