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

[PATCH v3 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 v2 and v3 are
described in each of the patches.

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

Link to v2: https://lore.kernel.org/git/cover.1710646998.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 |  19 ++++---
 config.c                 |  15 ++++--
 t/t1300-config.sh        | 112 +++++++++++++++++++++++++++++++++++++--
 3 files changed, 130 insertions(+), 16 deletions(-)

[PATCH v3 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 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

 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);

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

[Junio C Hamano]

Dragan Simic <dsimic@manjaro.org> writes:

>  		if (c == '"') {
> -			quote = 1-quote;
> +			quote = 1 - quote;
>  			continue;

Obviously correct.  Will queue.

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

[Dragan Simic]

On 2024-03-20 07:32, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
> 
>>  		if (c == '"') {
>> -			quote = 1-quote;
>> +			quote = 1 - quote;
>>  			continue;
> 
> Obviously correct.  Will queue.

Thanks.

[PATCH v3 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 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 v3 3/4] t1300: add more tests for whitespace and inline comments

[Dragan Simic]

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

Notes:
    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/

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

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 31c387868708..7688362826ea 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -11,6 +11,96 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
+test_expect_success 'setup whitespace config' '
+	tr "X" "\011" >.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
+'
+
+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 +1156,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' '

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

[Junio C Hamano]

Dragan Simic <dsimic@manjaro.org> writes:

> +test_expect_success 'setup whitespace config' '
> +	tr "X" "\011" >.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
> +'

If you want to write tests where leading and trailing whitespace
matter in them, making these invisible characters visible is a good
way to convey your intention to your readers.

	sed -e "s/Q/	/g" \
	    -e "s/^|//" \
	    -e "s/[$]$//" <<-\EOF
	|[section]
	|	solid = rock  $
	|	sparse = tab and space Q $
	EOF

This is still true even if we assume there is no patch corruption
while the e-mail is in transit.  It is safe to assume that the
receiving end of your patches uses "git am --whitespace=fix" and a
common way to protect against it is to use the opposite of "cat -e"
as a convention, additional to the "'|' is the left edge of paper"
and "Q stands for HT" conventions.

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

[Dragan Simic]

On 2024-03-20 07:42, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
> 
>> +test_expect_success 'setup whitespace config' '
>> +	tr "X" "\011" >.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
>> +'
> 
> If you want to write tests where leading and trailing whitespace
> matter in them, making these invisible characters visible is a good
> way to convey your intention to your readers.
> 
> 	sed -e "s/Q/	/g" \
> 	    -e "s/^|//" \
> 	    -e "s/[$]$//" <<-\EOF
> 	|[section]
> 	|	solid = rock  $
> 	|	sparse = tab and space Q $
> 	EOF
> 
> This is still true even if we assume there is no patch corruption
> while the e-mail is in transit.  It is safe to assume that the
> receiving end of your patches uses "git am --whitespace=fix" and a
> common way to protect against it is to use the opposite of "cat -e"
> as a convention, additional to the "'|' is the left edge of paper"
> and "Q stands for HT" conventions.

Agreed, will make it so in the v4.  I already had some thoughts
about protecting the trailing whitespace, and making obvious it
wasn't included by mistake.

Though, I'll have to use 'X' istead of 'Q' for HTs, because the
option names already contain 'Q' characters.

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

[Dragan Simic]

On 2024-03-20 07:46, Dragan Simic wrote:
> On 2024-03-20 07:42, Junio C Hamano wrote:
>> Dragan Simic <dsimic@manjaro.org> writes:
>> 
>>> +test_expect_success 'setup whitespace config' '
>>> +	tr "X" "\011" >.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
>>> +'
>> 
>> If you want to write tests where leading and trailing whitespace
>> matter in them, making these invisible characters visible is a good
>> way to convey your intention to your readers.
>> 
>> 	sed -e "s/Q/	/g" \
>> 	    -e "s/^|//" \
>> 	    -e "s/[$]$//" <<-\EOF
>> 	|[section]
>> 	|	solid = rock  $
>> 	|	sparse = tab and space Q $
>> 	EOF
>> 
>> This is still true even if we assume there is no patch corruption
>> while the e-mail is in transit.  It is safe to assume that the
>> receiving end of your patches uses "git am --whitespace=fix" and a
>> common way to protect against it is to use the opposite of "cat -e"
>> as a convention, additional to the "'|' is the left edge of paper"
>> and "Q stands for HT" conventions.
> 
> Agreed, will make it so in the v4.  I already had some thoughts
> about protecting the trailing whitespace, and making obvious it
> wasn't included by mistake.
> 
> Though, I'll have to use 'X' istead of 'Q' for HTs, because the
> option names already contain 'Q' characters.

Oh, I just saw that you've already picked this patch up in the "seen"
branch.  Would you prefer if I make this change and submit the v4, or
to perform the change in the already planned follow-up patches, which
would also clean up some other tests a bit?

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

[Junio C Hamano]

Dragan Simic <dsimic@manjaro.org> writes:

> Oh, I just saw that you've already picked this patch up in the "seen"
> branch.  Would you prefer if I make this change and submit the v4, or
> to perform the change in the already planned follow-up patches, which
> would also clean up some other tests a bit?

The purpose of the "seen" branch is to bundle the branches the
maintainer happens to have seen, and to remind the maintainer that
the topics in them might turn out to be interesting when they are
polished.  Nothing more than that.  Consider that a topic only in
"seen" is not part of "git" yet.

The contributors can use it to anticipate what topics from others
may cause conflict with their own work, and find people who are
working on these topics to talk to before the potential conflicts
get out of control.  It would be a good idea to fork from maint or
master to grow a topic and to test (1) it by itself, (2) a temporary
merge of it to 'next' and (3) a temporary merge to it to 'seen',
before publishing it.

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

[Dragan Simic]

Hello Junio,

On 2024-03-20 15:28, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
> 
>> Oh, I just saw that you've already picked this patch up in the "seen"
>> branch.  Would you prefer if I make this change and submit the v4, or
>> to perform the change in the already planned follow-up patches, which
>> would also clean up some other tests a bit?
> 
> The purpose of the "seen" branch is to bundle the branches the
> maintainer happens to have seen, and to remind the maintainer that
> the topics in them might turn out to be interesting when they are
> polished.  Nothing more than that.  Consider that a topic only in
> "seen" is not part of "git" yet.
> 
> The contributors can use it to anticipate what topics from others
> may cause conflict with their own work, and find people who are
> working on these topics to talk to before the potential conflicts
> get out of control.  It would be a good idea to fork from maint or
> master to grow a topic and to test (1) it by itself, (2) a temporary
> merge of it to 'next' and (3) a temporary merge to it to 'seen',
> before publishing it.

Thank you for the clarification.  Hence, I'll send the v4.

[PATCH v3 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 improve the description of the trailing whitespace
handling a bit, especially how it works out together with the presence of
inline comments.

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

Notes:
    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

 Documentation/config.txt | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 782c2bab906c..20f3300dc706 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
@@ -64,12 +65,14 @@ 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
+ending it with a `\`; the backslash and the end-of-line are stripped.
+Leading whitespace characters 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.
+whitespace characters of the line are discarded unless they are enclosed
+in double quotes.  The discarding of the trailing whitespace characters
+applies regardless of the discarding of the portion of the line after
+the first comment character.  Internal whitespace characters within the
+value are retained verbatim.
 
 Inside double quotes, double quote `"` and backslash `\` characters
 must be escaped: use `\"` for `"` and `\\` for `\`.

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

[Junio C Hamano]

Dragan Simic <dsimic@manjaro.org> writes:

>  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 whitespace characters after 'name =', the remainder of the
>  line after the first comment character '#' or ';', and trailing
> +whitespace characters of the line are discarded unless they are enclosed
> +in double quotes.

Can we directly tighten the "trailing..." part, instead of having to
add an extra long sentence ...

> +The discarding of the trailing whitespace characters
> +applies regardless of the discarding of the portion of the line after
> +the first comment character.

... like this as an attempt to clarify?

    Leading whitespace characters before and after 'name =', and the
    remainder of the line after the first comment character '#' or
    ';', are removed, and then trailing whitespace characters at the
    end of the line are discarded.

By the way, if a run of whitespace characters are enclosed in double
quotes, they cannot be trailing at the end of the line, as the
closing double quote is not a whitespace character, so it is out of
place to talk about quoted string in the context of trailing blank
removal.  The unquoting would want to be discussed separately.

> +Internal whitespace characters within the
> +value are retained verbatim.

Good.

>  
>  Inside double quotes, double quote `"` and backslash `\` characters
>  must be escaped: use `\"` for `"` and `\\` for `\`.

Thanks for working on this topic.

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

[Dragan Simic]

On 2024-03-20 08:12, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
> 
>>  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 whitespace characters after 'name =', the remainder of the
>>  line after the first comment character '#' or ';', and trailing
>> +whitespace characters of the line are discarded unless they are 
>> enclosed
>> +in double quotes.
> 
> Can we directly tighten the "trailing..." part, instead of having to
> add an extra long sentence ...

Makes sense, to make it less convoluted.

>> +The discarding of the trailing whitespace characters
>> +applies regardless of the discarding of the portion of the line after
>> +the first comment character.
> 
> ... like this as an attempt to clarify?
> 
>     Leading whitespace characters before and after 'name =', and the

Hmm, "leading whitespace" and "after" don't go very well together.
Such a construct seems a bit confusing, because it implies there's
something else after, which the leading whitespace refers to, which
may or may not be easily understandable to the users.

I'll think about how to rephrase this a bit better.

>     remainder of the line after the first comment character '#' or
>     ';', are removed, and then trailing whitespace characters at the
>     end of the line are discarded.
> 
> By the way, if a run of whitespace characters are enclosed in double
> quotes, they cannot be trailing at the end of the line, as the
> closing double quote is not a whitespace character, so it is out of
> place to talk about quoted string in the context of trailing blank
> removal.  The unquoting would want to be discussed separately.

I'll think about this as well.

>>  Inside double quotes, double quote `"` and backslash `\` characters
>>  must be escaped: use `\"` for `"` and `\\` for `\`.
> 
> Thanks for working on this topic.

Thank you for your highly detailed reviews!

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

[Junio C Hamano]

Dragan Simic <dsimic@manjaro.org> writes:

>>     Leading whitespace characters before and after 'name =', and the
> Hmm, "leading whitespace" and "after" don't go very well together.

True.  We can drop "leading" of course.  I meant to refer to, in
this sample illustration,

[section]
	var = value # comment

the fact that "\t" before "var =" is discarded, and " " after "var ="
before "value" is also discarded.

Thanks.

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

[Dragan Simic]

On 2024-03-20 15:42, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
> 
>>>     Leading whitespace characters before and after 'name =', and the
>> Hmm, "leading whitespace" and "after" don't go very well together.
> 
> True.  We can drop "leading" of course.

Good point.

> I meant to refer to, in this sample illustration,
> 
> [section]
> 	var = value # comment
> 
> the fact that "\t" before "var =" is discarded, and " " after "var ="
> before "value" is also discarded.

Sure, thanks for the clarification.  I'll try to tweak the wording
a bit further.