[PATCH v5 0/4] Fix a bug in configuration parsing, and improve tests and documentation

[PATCH v5 0/4] Fix a bug in configuration parsing, and improve tests and documentation

[Dragan Simic]

This series is an evolvement from another recent series, [1] as a result
of a decision to fix a longstanding bug in the parsing of configuration
option values, instead of documenting the status quo. [2][3]

The bufgix introduced in this series _should_ have no hidden negative
effects.  All of the configuration-related tests, both the old and the
new ones, pass with the patches applied.

In v2, this series had five patches in total, out of which the third patch
(i.e. patch 3/5) was dropped in v3. [4]  Other changes in each version are
described in each patch.

There will most probably be follow-up patches, to address the remaining
points raised during the review of this series. [5]

Link to v1: https://lore.kernel.org/git/cover.1710508691.git.dsimic@manjaro.org/T/#u
Link to v2: https://lore.kernel.org/git/cover.1710646998.git.dsimic@manjaro.org/T/#u
Link to v3: https://lore.kernel.org/git/cover.1710800549.git.dsimic@manjaro.org/T/#u
Link to v4: https://lore.kernel.org/git/cover.1710994548.git.dsimic@manjaro.org/T/#u

[1] https://lore.kernel.org/git/cover.1710258538.git.dsimic@manjaro.org/T/#u
[2] https://lore.kernel.org/git/ff7b0a2ead90ad9a9456141da5e4df4a@manjaro.org/
[3] https://lore.kernel.org/git/11be11f231f3bf41d0245c780c20693f@manjaro.org/
[4] https://lore.kernel.org/git/514d832b0399ccdbc354675068477fea@manjaro.org/
[5] https://lore.kernel.org/git/f37d753485094a3ba66fde5e85d0e2dc@manjaro.org/

Dragan Simic (4):
  config: minor addition of whitespace
  config: really keep value-internal whitespace verbatim
  t1300: add more tests for whitespace and inline comments
  config.txt: describe handling of whitespace further

 Documentation/config.txt |  28 +++++-----
 config.c                 |  15 ++++--
 t/t1300-config.sh        | 114 +++++++++++++++++++++++++++++++++++++--
 3 files changed, 136 insertions(+), 21 deletions(-)

[PATCH v5 1/4] config: minor addition of whitespace

[Dragan Simic]

In general, binary operators should be enclosed in a pair of leading and
trailing space (SP) characters.  Thus, clean up one spotted expression that
for some reason had a "bunched up" operator.

Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---

Notes:
    Changes in v5:
        - No changes were introduced
    
    Changes in v4:
        - Junio responded with "will queue" in v3, [1] but this patch hasn't
          reached the "next" branch yet, so resending it as a reminder
        - No changes were introduced
    
    Changes in v3:
        - Patch description was expanded a tiny bit, to make it more accurate
        - No changes to the source code were introduced
    
    Changes in v2:
        - No changes were introduced
    
    [1] https://lore.kernel.org/git/xmqq5xxhl8gs.fsf@gitster.g/

 config.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/config.c b/config.c
index 3cfeb3d8bd99..a86a20cdf5cb 100644
--- a/config.c
+++ b/config.c
@@ -869,7 +869,7 @@ static char *parse_value(struct config_source *cs)
 			continue;
 		}
 		if (c == '"') {
-			quote = 1-quote;
+			quote = 1 - quote;
 			continue;
 		}
 		strbuf_addch(&cs->value, c);

[PATCH v5 2/4] config: really keep value-internal whitespace verbatim

[Dragan Simic]

Fix a bug in function parse_value() that prevented whitespace characters
(i.e. spaces and horizontal tabs) found inside configuration option values
from being parsed and returned in their original form.  The bug caused any
number of consecutive whitespace characters to be wrongly "squashed" into
the same number of space characters.

This bug was introduced back in July 2009, in commit ebdaae372b46 ("config:
Keep inner whitespace verbatim").

Further investigation showed that setting a configuration value, by invoking
git-config(1), converts value-internal horizontal tabs into "\t" escape
sequences, which the buggy value-parsing logic in function parse_value()
didn't "squash" into spaces.  That's why the test included in the ebdaae37
commit passed, which presumably made the bug remain undetected for this long.
On the other hand, value-internal literal horizontal tab characters, found in
a configuration file edited by hand, do get "squashed" by the value-parsing
logic, so the right choice was to fix this bug by making the value-internal
whitespace characters preserved verbatim.

Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---

Notes:
    Changes in v5:
        - No changes were introduced
    
    Changes in v4:
        - No changes were introduced
    
    Changes in v3:
        - No changes were introduced
    
    Changes in v2:
        - Dropped the "Fixes" tag, as explained and requested by Junio, [1]
          because having such tags actually doesn't help us in the long run
        - No changes to the source code were introduced
    
    [1] https://lore.kernel.org/git/xmqq4jd7qtg6.fsf@gitster.g/

 config.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/config.c b/config.c
index a86a20cdf5cb..5072f12e62e4 100644
--- a/config.c
+++ b/config.c
@@ -817,33 +817,38 @@ static int get_next_char(struct config_source *cs)
 
 static char *parse_value(struct config_source *cs)
 {
-	int quote = 0, comment = 0, space = 0;
+	int quote = 0, comment = 0;
+	size_t trim_len = 0;
 
 	strbuf_reset(&cs->value);
 	for (;;) {
 		int c = get_next_char(cs);
 		if (c == '\n') {
 			if (quote) {
 				cs->linenr--;
 				return NULL;
 			}
+			if (trim_len)
+				strbuf_setlen(&cs->value, trim_len);
 			return cs->value.buf;
 		}
 		if (comment)
 			continue;
 		if (isspace(c) && !quote) {
+			if (!trim_len)
+				trim_len = cs->value.len;
 			if (cs->value.len)
-				space++;
+				strbuf_addch(&cs->value, c);
 			continue;
 		}
 		if (!quote) {
 			if (c == ';' || c == '#') {
 				comment = 1;
 				continue;
 			}
 		}
-		for (; space; space--)
-			strbuf_addch(&cs->value, ' ');
+		if (trim_len)
+			trim_len = 0;
 		if (c == '\\') {
 			c = get_next_char(cs);
 			switch (c) {

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

[Dragan Simic]

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 v5:
        - Replaced the HT escape sequence with a literal HT character in the
          configuration setup test's invocation of sed(1), because it may not
          be portable, as pointed out by Eric Sunshine [6]
    
    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/
    [6] https://lore.kernel.org/git/CAPig+cQekOLrpcK_NcL951SRZAg-1Y1aFzE0-i-uCTK_XuiajQ@mail.gmail.com/

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

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 31c387868708..f4bd69512ed9 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/	/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' '

[PATCH v5 4/4] config.txt: describe handling of whitespace further

[Dragan Simic]

Make it more clear what the whitespace characters are in the context of git
configuration files, and significantly improve the description of the leading
and trailing whitespace handling, especially how it works out together with
the presence of inline comments.

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

Notes:
    Changes in v5:
        - Rewrote the description of whitespace character handling again,
          to eliminate ambiguity, as suggested by Eric Sunshine [3][4]
        - Extended the improvements to the following paragraph as well, to
          tie it all together, and to make it less ambiguous how to include
          leading and trailing whitespace characters into configuration
          option values, if desired so
        - Added a Helped-by tag
    
    Changes in v4:
        - Improved the wording and accuracy of the description of whitespace
          character handling, as discussed with Junio, [1][2] by taking a more
          radical approach and rewriting an entire paragraph, because it has
          reached the point where "patching the patchwork" no longer worked;
          I'm quite happy with the way it turned out this time
        - Expanded the patch description a tiny bit
        - Added a Helped-by tag
    
    Changes in v3:
        - Patch description was expanded a bit, to make it more on point
        - No changes to the documentation were introduced
    
    Changes in v2:
        - No changes were introduced
    
    [1] https://lore.kernel.org/git/xmqqttl1js1o.fsf@gitster.g/
    [2] https://lore.kernel.org/git/ce041191a245ff888b1710cdcaad9e61@manjaro.org/
    [3] https://lore.kernel.org/git/CAPig+cSYhYBa0NsvJCOYo4JsWzLJT9rU++U1QKA3jRB6Cptbhw@mail.gmail.com/
    [4] https://lore.kernel.org/git/fdaec126df16bf6fe1c1fca9698f7dcc@manjaro.org/

 Documentation/config.txt | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 782c2bab906c..70b448b13262 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -22,9 +22,10 @@ multivalued.
 Syntax
 ~~~~~~
 
-The syntax is fairly flexible and permissive; whitespaces are mostly
-ignored.  The '#' and ';' characters begin comments to the end of line,
-blank lines are ignored.
+The syntax is fairly flexible and permissive.  Whitespace characters,
+which in this context are the space character (SP) and the horizontal
+tabulation (HT), are mostly ignored.  The '#' and ';' characters begin
+comments to the end of line.  Blank lines are ignored.
 
 The file consists of sections and variables.  A section begins with
 the name of the section in square brackets and continues until the next
@@ -63,16 +64,17 @@ the variable is the boolean "true").
 The variable names are case-insensitive, allow only alphanumeric characters
 and `-`, and must start with an alphabetic character.
 
-A line that defines a value can be continued to the next line by
-ending it with a `\`; the backslash and the end-of-line are
-stripped.  Leading whitespaces after 'name =', the remainder of the
-line after the first comment character '#' or ';', and trailing
-whitespaces of the line are discarded unless they are enclosed in
-double quotes.  Internal whitespaces within the value are retained
-verbatim.
-
-Inside double quotes, double quote `"` and backslash `\` characters
-must be escaped: use `\"` for `"` and `\\` for `\`.
+Whitespace characters surrounding `name`, `=` and `value` are discarded.
+Internal whitespace characters within 'value' are retained verbatim.
+Comments starting with either `#` or `;` and extending to the end of line
+are discarded.  A line that defines a value can be continued to the next
+line by ending it with a backslash (`\`);  the backslash and the end-of-line
+characters are discarded.
+
+If `value` needs to contain leading or trailing whitespace characters,
+it must be enclosed in double quotation marks (`"`).  Inside double quotation
+marks, double quote (`"`) and backslash (`\`) characters must be escaped:
+use `\"` for `"` and `\\` for `\`.
 
 The following escape sequences (beside `\"` and `\\`) are recognized:
 `\n` for newline character (NL), `\t` for horizontal tabulation (HT, TAB)

Re: [PATCH v5 4/4] config.txt: describe handling of whitespace further

[Eric Sunshine]

On Thu, Mar 21, 2024 at 2:06 AM Dragan Simic <dsimic@manjaro.org> wrote:
> Make it more clear what the whitespace characters are in the context of git
> configuration files, and significantly improve the description of the leading
> and trailing whitespace handling, especially how it works out together with
> the presence of inline comments.
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
> ---
>     Changes in v5:
>         - Rewrote the description of whitespace character handling again,
>           to eliminate ambiguity, as suggested by Eric Sunshine [3][4]
>         - Extended the improvements to the following paragraph as well, to
>           tie it all together, and to make it less ambiguous how to include
>           leading and trailing whitespace characters into configuration
>           option values, if desired so
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> @@ -63,16 +64,17 @@ the variable is the boolean "true").
> +Whitespace characters surrounding `name`, `=` and `value` are discarded.
> +Internal whitespace characters within 'value' are retained verbatim.
> +Comments starting with either `#` or `;` and extending to the end of line
> +are discarded.  A line that defines a value can be continued to the next
> +line by ending it with a backslash (`\`);  the backslash and the end-of-line
> +characters are discarded.
> +
> +If `value` needs to contain leading or trailing whitespace characters,
> +it must be enclosed in double quotation marks (`"`).  Inside double quotation
> +marks, double quote (`"`) and backslash (`\`) characters must be escaped:
> +use `\"` for `"` and `\\` for `\`.

Thanks. I find this discussion clearer than v4.

I don't have any further review comments on v5.

Re: [PATCH v5 4/4] config.txt: describe handling of whitespace further

[Dragan Simic]

On 2024-03-21 07:22, Eric Sunshine wrote:
> On Thu, Mar 21, 2024 at 2:06 AM Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> Make it more clear what the whitespace characters are in the context 
>> of git
>> configuration files, and significantly improve the description of the 
>> leading
>> and trailing whitespace handling, especially how it works out together 
>> with
>> the presence of inline comments.
>> 
>> Helped-by: Junio C Hamano <gitster@pobox.com>
>> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
>> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
>> ---
>>     Changes in v5:
>>         - Rewrote the description of whitespace character handling 
>> again,
>>           to eliminate ambiguity, as suggested by Eric Sunshine [3][4]
>>         - Extended the improvements to the following paragraph as 
>> well, to
>>           tie it all together, and to make it less ambiguous how to 
>> include
>>           leading and trailing whitespace characters into 
>> configuration
>>           option values, if desired so
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> @@ -63,16 +64,17 @@ the variable is the boolean "true").
>> +Whitespace characters surrounding `name`, `=` and `value` are 
>> discarded.
>> +Internal whitespace characters within 'value' are retained verbatim.
>> +Comments starting with either `#` or `;` and extending to the end of 
>> line
>> +are discarded.  A line that defines a value can be continued to the 
>> next
>> +line by ending it with a backslash (`\`);  the backslash and the 
>> end-of-line
>> +characters are discarded.
>> +
>> +If `value` needs to contain leading or trailing whitespace 
>> characters,
>> +it must be enclosed in double quotation marks (`"`).  Inside double 
>> quotation
>> +marks, double quote (`"`) and backslash (`\`) characters must be 
>> escaped:
>> +use `\"` for `"` and `\\` for `\`.
> 
> Thanks. I find this discussion clearer than v4.
> 
> I don't have any further review comments on v5.

Great, thanks!  I really appreciate your prompt and detailed reviews.

Re: [PATCH v5 4/4] config.txt: describe handling of whitespace further

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> Thanks. I find this discussion clearer than v4.
>
> I don't have any further review comments on v5.

Thanks, both.  Let's mark it for 'next' then.

Re: [PATCH v5 4/4] config.txt: describe handling of whitespace further

[Dragan Simic]

On 2024-03-21 23:58, Junio C Hamano wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
>> Thanks. I find this discussion clearer than v4.
>> 
>> I don't have any further review comments on v5.
> 
> Thanks, both.  Let's mark it for 'next' then.

Great, thank you.