diff mbox series

[v4,3/4] t1300: add more tests for whitespace and inline comments

Message ID 688128d8ef09589712634888074ffd71a192a7aa.1710994548.git.dsimic@manjaro.org (mailing list archive)
State Superseded
Headers show
Series Fix a bug in configuration parsing, and improve tests and documentation | expand

Commit Message

Dragan Simic March 21, 2024, 4:17 a.m. UTC
Add a handful of additional tests, to improve the coverage of the handling
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 whitespace-related test a bit,
to ensure its consistency with the newly added tests.  This change introduced
no functional changes to the already existing test.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---

Notes:
    Changes in v4:
        - Improved the added configuration setup test, to make the included
          leading and trailing whitespace characters more robust and easier
          on the eyes, as suggested and outlined by Junio [5]
        - Expanded the patch description a bit and improved the wording
        - Added a Helped-by tag
    
    Changes in v3:
        - Removed a few unnecessary invocations of x_to_tab()
        - As pointed out by Eric Sunshine, [3] it's better not to introduce
          new random helper functions, so x_to_tab() was replaced by a direct
          invocation of tr(1), in one case that requires use of literal 'Q'
          characters, and by invocations of q_to_tab(), in the remaining cases
          that actually allow use of 'Q' characters to represent HTs
        - Dropped the change of the name of an already existing test, to not
          distract the reviewers, as suggested by Eric Sunshine [4]
        - Renamed the first added test, as suggested by Eric Sunshine, [4] to
          make it consistent with the expected way for naming setup tests
    
    Changes in v2:
        - All new tests and one already existing test reworked according to
          Eric Sunshine's review suggestions; [1][2]  the already existing
          test was reworked a bit to ensure consistency
        - Added a Helped-by tag
    
    [1] https://lore.kernel.org/git/CAPig+cRMPNExbG34xJ0w5npUc3DDwxQUGS_AQfam_mi4s53=sA@mail.gmail.com/
    [2] https://lore.kernel.org/git/CAPig+cRG8eFxepkaiN54H+fa7D=rFGsmEHdvTP+HSSaLO_6T_A@mail.gmail.com/
    [3] https://lore.kernel.org/git/CAPig+cSLb+Rsy81itvw9Tfvqv9vvKSPgO_ER9fWL04XZrgFvwg@mail.gmail.com/T/#u
    [4] https://lore.kernel.org/git/CAPig+cTVmQzC38DympSEtPNhgY=-+dYbZmkr0RTRbhG-hp2fmQ@mail.gmail.com/
    [5] https://lore.kernel.org/git/32c4718d09ff6675e37587e15e785413@manjaro.org/

 t/t1300-config.sh | 114 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 111 insertions(+), 3 deletions(-)

Comments

Eric Sunshine March 21, 2024, 4:55 a.m. UTC | #1
On Thu, Mar 21, 2024 at 12:17 AM Dragan Simic <dsimic@manjaro.org> wrote:
> Add a handful of additional tests, to improve the coverage of the handling
> 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 whitespace-related test a bit,
> to ensure its consistency with the newly added tests.  This change introduced
> no functional changes to the already existing test.
>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
> ---
> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> @@ -11,6 +11,98 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> +test_expect_success 'setup whitespace config' '
> +       sed -e "s/^|//" \
> +           -e "s/[$]$//" \
> +           -e "s/X/\\t/g" >.git/config <<-\EOF

Representing TAB as `\t` in the sed replacement is not necessarily
portable and (as far as I can see) we don't rely upon that in any
existing tests. It probably would be safer to employ a literal TAB
character in `s/X/ /g` as Junio showed in his example[1].

[1]: https://lore.kernel.org/git/xmqqzfutjtfo.fsf@gitster.g/
Dragan Simic March 21, 2024, 5:01 a.m. UTC | #2
On 2024-03-21 05:55, Eric Sunshine wrote:
> On Thu, Mar 21, 2024 at 12:17 AM Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> Add a handful of additional tests, to improve the coverage of the 
>> handling
>> 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 whitespace-related test 
>> a bit,
>> to ensure its consistency with the newly added tests.  This change 
>> introduced
>> no functional changes to the already existing test.
>> 
>> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
>> Helped-by: Junio C Hamano <gitster@pobox.com>
>> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
>> ---
>> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
>> @@ -11,6 +11,98 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>> +test_expect_success 'setup whitespace config' '
>> +       sed -e "s/^|//" \
>> +           -e "s/[$]$//" \
>> +           -e "s/X/\\t/g" >.git/config <<-\EOF
> 
> Representing TAB as `\t` in the sed replacement is not necessarily
> portable and (as far as I can see) we don't rely upon that in any
> existing tests. It probably would be safer to employ a literal TAB
> character in `s/X/ /g` as Junio showed in his example[1].

Hmm, I actually saw using a literal HT character as a disadvantage.
Thanks for the notice, I'll send the v5 with this fixed.

> [1]: https://lore.kernel.org/git/xmqqzfutjtfo.fsf@gitster.g/
diff mbox series

Patch

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 31c387868708..6ecc9dd4c3d2 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -11,6 +11,98 @@  export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
+test_expect_success 'setup whitespace config' '
+	sed -e "s/^|//" \
+	    -e "s/[$]$//" \
+	    -e "s/X/\\t/g" >.git/config <<-\EOF
+	[section]
+	|	solid = rock
+	|	sparse = big XX blue
+	|	sparseAndTail = big XX blue $
+	|	sparseAndTailQuoted = "big XX blue "
+	|	sparseAndBiggerTail = big XX blue X X
+	|	sparseAndBiggerTailQuoted = "big XX blue X X"
+	|	sparseAndBiggerTailQuotedPlus = "big XX blue X X"X $
+	|	headAndTail = Xbig blue $
+	|	headAndTailQuoted = "Xbig blue "
+	|	headAndTailQuotedPlus = "Xbig blue " $
+	|	annotated = big blueX# to be discarded
+	|	annotatedQuoted = "big blue"X# to be discarded
+	EOF
+'
+
+test_expect_success 'no internal whitespace' '
+	echo "rock" >expect &&
+	git config --get section.solid >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'internal whitespace' '
+	echo "big QQ blue" | q_to_tab >expect &&
+	git config --get section.sparse >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'internal and trailing whitespace' '
+	echo "big QQ blue" | q_to_tab >expect &&
+	git config --get section.sparseAndTail >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'internal and trailing whitespace, all quoted' '
+	echo "big QQ blue " | q_to_tab >expect &&
+	git config --get section.sparseAndTailQuoted >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'internal and more trailing whitespace' '
+	echo "big QQ blue" | q_to_tab >expect &&
+	git config --get section.sparseAndBiggerTail >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'internal and more trailing whitespace, all quoted' '
+	echo "big QQ blue Q Q" | q_to_tab >expect &&
+	git config --get section.sparseAndBiggerTailQuoted >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'internal and more trailing whitespace, not all quoted' '
+	echo "big QQ blue Q Q" | q_to_tab >expect &&
+	git config --get section.sparseAndBiggerTailQuotedPlus >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'leading and trailing whitespace' '
+	echo "big blue" >expect &&
+	git config --get section.headAndTail >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'leading and trailing whitespace, all quoted' '
+	echo "Qbig blue " | q_to_tab >expect &&
+	git config --get section.headAndTailQuoted >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'leading and trailing whitespace, not all quoted' '
+	echo "Qbig blue " | q_to_tab >expect &&
+	git config --get section.headAndTailQuotedPlus >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'inline comment' '
+	echo "big blue" >expect &&
+	git config --get section.annotated >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'inline comment, quoted' '
+	echo "big blue" >expect &&
+	git config --get section.annotatedQuoted >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'clear default config' '
 	rm -f .git/config
 '
@@ -1066,9 +1158,25 @@  test_expect_success '--null --get-regexp' '
 	test_cmp expect result
 '
 
-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
+'
+
+test_expect_success 'inner whitespace kept verbatim, horizontal tabs only' '
+	echo "fooQQbar" | q_to_tab >expect &&
+	git config section.val "$(cat expect)" &&
+	git config --get section.val >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'inner whitespace kept verbatim, horizontal tabs and spaces' '
+	echo "foo Q  bar" | q_to_tab >expect &&
+	git config section.val "$(cat expect)" &&
+	git config --get section.val >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success SYMLINKS 'symlinked configuration' '