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

[PATCH v2 0/5] 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.

Changes in v2 are described in each of the patches.

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

Dragan Simic (5):
  config: minor addition of whitespace
  config: really keep value-internal whitespace verbatim
  test: introduce new x_to_tab() helper function
  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        | 114 +++++++++++++++++++++++++++++++++++++--
 t/test-lib-functions.sh  |   4 ++
 4 files changed, 135 insertions(+), 17 deletions(-)

[PATCH v2 1/5] config: minor addition of whitespace

[Dragan Simic]

In general, binary operators should be enclosed in a pair of leading and
trailing space 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 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);

[PATCH v2 2/5] 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 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 v2 3/5] test: introduce new x_to_tab() helper function

[Dragan Simic]

There's nothing wrong with the already existing q_to_tab() function, except
when it's used on strings that contain uppercase letter "Q" in its literal
meaning, which, for example, can happen with git configurations that contain
"*.*Quoted" as the names of their configuration variables.

Thus, let's introduce new x_to_tab() helper function that does pretty much
the same job as the already existing q_to_tab() helper function, except for
replacing "X" with a horizontal tab (HT), instead of replacing "Q".

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

Notes:
    This patch didn't exist in the v1 of this patch series.

 t/test-lib-functions.sh | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 6eaf116346be..362d3205b7b0 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -107,6 +107,10 @@ q_to_tab () {
 	tr Q '\011'
 }
 
+x_to_tab () {
+	tr X '\011'
+}
+
 qz_to_tab_space () {
 	tr QZ '\011\040'
 }

[PATCH v2 4/5] 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 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/

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

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 31c387868708..37ed078721ea 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -11,7 +11,97 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
-test_expect_success 'clear default config' '
+test_expect_success 'create test configuration' '
+	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
+'
+
+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 XX blue" | x_to_tab >expect &&
+	git config --get section.sparse >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'internal and trailing whitespace' '
+	echo "big XX blue" | x_to_tab >expect &&
+	git config --get section.sparseAndTail >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'internal and trailing whitespace, all quoted' '
+	echo "big XX blue " | x_to_tab >expect &&
+	git config --get section.sparseAndTailQuoted >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'internal and more trailing whitespace' '
+	echo "big XX blue" | x_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 XX blue X X" | x_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 XX blue X X" | x_to_tab >expect &&
+	git config --get section.sparseAndBiggerTailQuotedPlus >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'leading and trailing whitespace' '
+	echo "big blue" | x_to_tab >expect &&
+	git config --get section.headAndTail >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'leading and trailing whitespace, all quoted' '
+	echo "Xbig blue " | x_to_tab >expect &&
+	git config --get section.headAndTailQuoted >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'leading and trailing whitespace, not all quoted' '
+	echo "Xbig blue " | x_to_tab >expect &&
+	git config --get section.headAndTailQuotedPlus >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'inline comment' '
+	echo "big blue" | x_to_tab >expect &&
+	git config --get section.annotated >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'inline comment, quoted' '
+	echo "big blue" | x_to_tab >expect &&
+	git config --get section.annotatedQuoted >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'clear default configuration' '
 	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' '

[PATCH v2 5/5] 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.

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

Notes:
    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 v2 3/5] test: introduce new x_to_tab() helper function

[Eric Sunshine]

On Sat, Mar 16, 2024 at 11:48 PM Dragan Simic <dsimic@manjaro.org> wrote:
> There's nothing wrong with the already existing q_to_tab() function, except
> when it's used on strings that contain uppercase letter "Q" in its literal
> meaning, which, for example, can happen with git configurations that contain
> "*.*Quoted" as the names of their configuration variables.
>
> Thus, let's introduce new x_to_tab() helper function that does pretty much
> the same job as the already existing q_to_tab() helper function, except for
> replacing "X" with a horizontal tab (HT), instead of replacing "Q".
>
> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
> ---
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> @@ -107,6 +107,10 @@ q_to_tab () {
> +x_to_tab () {
> +       tr X '\011'
> +}

I'd like to push back on this change since it may lead to an explosion
of new almost-identical functions. For such a one-off case where
q_to_tab() isn't appropriate, it's perfectly fine to simply use `tr X
`\011'` directly in your test:

    test_expect_success 'foo' '
        tr X "\011" >expect <<-\EOF
        some Q stuff
        whitespaceXhere
        EOF
        ...
    '

However, if you really insist upon using a library function, then
either add a general-purpose function which accepts the special
character as an argument, or just retrofit q_to_tab() to optionally
accept the special character:

    # t/test-lib-functions.sh

    # usage: q_to_tab [<needle-char>]
    # replace <needle-char> with TAB in stdin
    q_to_tab () {
        local c=$1
        test -n "$c" || c=Q
        tr "$c" '\011'
    }

But this is probably overkill for a one-off case.

Re: [PATCH v2 3/5] test: introduce new x_to_tab() helper function

[Dragan Simic]

Hello Eric,

Thanks for responding so quickly.  Please, see my comments below.

On 2024-03-17 05:03, Eric Sunshine wrote:
> On Sat, Mar 16, 2024 at 11:48 PM Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> There's nothing wrong with the already existing q_to_tab() function, 
>> except
>> when it's used on strings that contain uppercase letter "Q" in its 
>> literal
>> meaning, which, for example, can happen with git configurations that 
>> contain
>> "*.*Quoted" as the names of their configuration variables.
>> 
>> Thus, let's introduce new x_to_tab() helper function that does pretty 
>> much
>> the same job as the already existing q_to_tab() helper function, 
>> except for
>> replacing "X" with a horizontal tab (HT), instead of replacing "Q".
>> 
>> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
>> ---
>> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
>> @@ -107,6 +107,10 @@ q_to_tab () {
>> +x_to_tab () {
>> +       tr X '\011'
>> +}
> 
> I'd like to push back on this change since it may lead to an explosion
> of new almost-identical functions. For such a one-off case where
> q_to_tab() isn't appropriate, it's perfectly fine to simply use `tr X
> `\011'` directly in your test:
> 
>     test_expect_success 'foo' '
>         tr X "\011" >expect <<-\EOF
>         some Q stuff
>         whitespaceXhere
>         EOF
>         ...
>     '

Agreed, I'll take this approach in the v3.

> However, if you really insist upon using a library function, then
> either add a general-purpose function which accepts the special
> character as an argument, or just retrofit q_to_tab() to optionally
> accept the special character:
> 
>     # t/test-lib-functions.sh
> 
>     # usage: q_to_tab [<needle-char>]
>     # replace <needle-char> with TAB in stdin
>     q_to_tab () {
>         local c=$1
>         test -n "$c" || c=Q
>         tr "$c" '\011'
>     }
> 
> But this is probably overkill for a one-off case.

As far as I can see after doing a few really quick greps in the "t"
subdirectory, such an approach might actually make sense, but it would
require further work, to make some other already existing tests use
the enhanced q_to_tab() function, and to warrant the whole thing.

That might be an interesting #leftover for someone else to pick it up
at some point.

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

[Eric Sunshine]

On Sat, Mar 16, 2024 at 11:48 PM Dragan Simic <dsimic@manjaro.org> 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@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

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

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

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

[Eric Sunshine]

On Sun, Mar 17, 2024 at 12:21 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sat, Mar 16, 2024 at 11:48 PM Dragan Simic <dsimic@manjaro.org> wrote:
> > 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" [...]

Of course, I meant [1].

[1]: https://lore.kernel.org/git/CAPig+cRMPNExbG34xJ0w5npUc3DDwxQUGS_AQfam_mi4s53=sA@mail.gmail.com/

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

[Dragan Simic]

On 2024-03-17 05:21, Eric Sunshine wrote:
> On Sat, Mar 16, 2024 at 11:48 PM Dragan Simic <dsimic@manjaro.org> 
> 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@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

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. :)

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

[Eric Sunshine]

On Sun, Mar 17, 2024 at 12:50 AM Dragan Simic <dsimic@manjaro.org> wrote:
> On 2024-03-17 05:21, Eric Sunshine wrote:
> > On Sat, Mar 16, 2024 at 11:48 PM Dragan Simic <dsimic@manjaro.org>
> > wrote:
> >> +       x_to_tab >.git/config <<-\EOF
> >> +       [section]
> >> +               Xsolid = rock
> >> +               Xsparse = big XX blue
> >> +               ...
> >> +       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.

Readability wasn't my reason for bringing this up. As a reviewer,
every time a question pops into my mind as I'm reading the code, that
indicates that something about the code is unclear or that the commit
message doesn't properly explain why it was done in this way. People
coming across this code in the future may have the same questions but
they won't have the benefit of being able to easily ask you why it was
done this way.

So, the ideal patch is one in which the reviewer reads the code and
simply nods in understanding without having to question any of the
author's choices. And the ideal test is one in which does the bare
minimum to verify that the condition being checked is in fact correct;
there should be no extraneous code or behavior which could mislead the
reader into wondering why it was done that way.

In this particular case, it wasn't clear whether you, as author,
realized that all leading TABs are stripped from the heredoc body, and
whether or not that mattered to you and whether or not you expected it
to be significant to the test's behavior.

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

My assumption, perhaps incorrectly, was that existing tests already
verified correct behavior of leading whitespace and that the tests
added by this patch were about internal whitespace. If that's not the
case (and perhaps I didn't fully digest the commit message) then my
question about the leading "X" is off the mark.

If these new tests are also checking leading whitespace behavior, then
to improve coverage, would it make sense to have the leading "X" on
some lines but not others?

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> >> +       x_to_tab >.git/config <<-\EOF
>> >> +       [section]
>> >> +               Xsolid = rock
>> >> +               Xsparse = big XX blue
>> >> +               ...
>> >> +       EOF
>> >> +'

Just this part.

> My assumption, perhaps incorrectly, was that existing tests already
> verified correct behavior of leading whitespace and that the tests
> added by this patch were about internal whitespace. If that's not the
> case (and perhaps I didn't fully digest the commit message) then my
> question about the leading "X" is off the mark.
>
> If these new tests are also checking leading whitespace behavior, then
> to improve coverage, would it make sense to have the leading "X" on
> some lines but not others?

If "<<-" (I have here-doc but please strip the leading tabs because
I am aligning the here-doc with them) gets in the way for testing
material with leading tabs, the way to write and preprocess such a
here-doc is:

	sed -e 's/^|//' -e 's/Q/   /g' >.git/config <<-\EOF
	|[section]
	|	solid = rock
	|	sparse = big QQ blue
	|	...
	EOF

It will make it clear where the left-edge of the "sheet of paper"
is, removal of leading '|' does not get in the way of using '|' in
the middle of the line if needed, and Q being the least used letter
makes them stand out more in the middle of the line.  As it is
obvious that what is before solid and sparse is a tab (otherwise you
would not be using that '|' trick), you do not have to write Xsolid
or Qsolid there and still the result is much easier to read.

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

[Dragan Simic]

Hello Eric,

On 2024-03-18 03:48, Eric Sunshine wrote:
> On Sun, Mar 17, 2024 at 12:50 AM Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> On 2024-03-17 05:21, Eric Sunshine wrote:
>> > On Sat, Mar 16, 2024 at 11:48 PM Dragan Simic <dsimic@manjaro.org>
>> > wrote:
>> >> +       x_to_tab >.git/config <<-\EOF
>> >> +       [section]
>> >> +               Xsolid = rock
>> >> +               Xsparse = big XX blue
>> >> +               ...
>> >> +       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.
> 
> Readability wasn't my reason for bringing this up. As a reviewer,
> every time a question pops into my mind as I'm reading the code, that
> indicates that something about the code is unclear or that the commit
> message doesn't properly explain why it was done in this way. People
> coming across this code in the future may have the same questions but
> they won't have the benefit of being able to easily ask you why it was
> done this way.

I see.  How about including a small comment in the t1300 that would
explain the additional indentation?

As a note, there are already more tests in the t1300 that contain such
indentation, so maybe we shoulddo something with those existing tests
as well;  the above-proposed comment, which would be placed at the very
beginning of t1300, may provide a satisfactory explanation for all the
tests in t1300 that contain such additional indentation.

Another option would be to either add the indentation to all relevant
tests in the t1300, or to remove the indentation from all tests in the
t1300 that already contain it.  I'd be happy to implement and submit
patches that do that, after we choose the direction we want to follow.

> So, the ideal patch is one in which the reviewer reads the code and
> simply nods in understanding without having to question any of the
> author's choices. And the ideal test is one in which does the bare
> minimum to verify that the condition being checked is in fact correct;
> there should be no extraneous code or behavior which could mislead the
> reader into wondering why it was done that way.
> 
> In this particular case, it wasn't clear whether you, as author,
> realized that all leading TABs are stripped from the heredoc body, and
> whether or not that mattered to you and whether or not you expected it
> to be significant to the test's behavior.

I fully agree with the most of your points above.  The only slight
disagreement would be about the bare minimum when it comes to automated
tests;  in my, opinion, it's actually good to have a few twisties here
and there, simply to exercise the underlying logic better by such tests.

This probably opens a question whether the automated tests should be
orthogonal?  Perhaps the majority od them should, but IMHO having a few
composite tests can only help with the validation of the underlying 
logic.

>> > 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.
> 
> My assumption, perhaps incorrectly, was that existing tests already
> verified correct behavior of leading whitespace and that the tests
> added by this patch were about internal whitespace. If that's not the
> case (and perhaps I didn't fully digest the commit message) then my
> question about the leading "X" is off the mark.

Frankly, I can't be 100% sure without spending quite a lot of time, but
I don't think that the already existing tests in the t1300 were tailored
specifically to cover the verification of the leading whitespace, i.e.
of the indentation in git configuration files.  To me, it seems more 
like
a random choice of including the indentation or not.

Such a random approach may actually be good, despite bringing back the
above-mentioned question about the orthogonality of the tests.

> If these new tests are also checking leading whitespace behavior, then
> to improve coverage, would it make sense to have the leading "X" on
> some lines but not others?

Good point, despite that not being the main purpose of the added tests.
I'll see to add a couple of tests that check the handling of 
indentation,
possibly at some places in the t1300 that fit the best;  improving the
tests coverage can only help in the long run.

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

[Dragan Simic]

Hello Junio,

On 2024-03-18 05:38, Junio C Hamano wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
>>> >> +       x_to_tab >.git/config <<-\EOF
>>> >> +       [section]
>>> >> +               Xsolid = rock
>>> >> +               Xsparse = big XX blue
>>> >> +               ...
>>> >> +       EOF
>>> >> +'
> 
> Just this part.
> 
>> My assumption, perhaps incorrectly, was that existing tests already
>> verified correct behavior of leading whitespace and that the tests
>> added by this patch were about internal whitespace. If that's not the
>> case (and perhaps I didn't fully digest the commit message) then my
>> question about the leading "X" is off the mark.
>> 
>> If these new tests are also checking leading whitespace behavior, then
>> to improve coverage, would it make sense to have the leading "X" on
>> some lines but not others?
> 
> If "<<-" (I have here-doc but please strip the leading tabs because
> I am aligning the here-doc with them) gets in the way for testing
> material with leading tabs, the way to write and preprocess such a
> here-doc is:
> 
> 	sed -e 's/^|//' -e 's/Q/   /g' >.git/config <<-\EOF
> 	|[section]
> 	|	solid = rock
> 	|	sparse = big QQ blue
> 	|	...
> 	EOF
> 
> It will make it clear where the left-edge of the "sheet of paper"
> is, removal of leading '|' does not get in the way of using '|' in
> the middle of the line if needed, and Q being the least used letter
> makes them stand out more in the middle of the line.  As it is
> obvious that what is before solid and sparse is a tab (otherwise you
> would not be using that '|' trick), you do not have to write Xsolid
> or Qsolid there and still the result is much easier to read.

This looks quite neat.  Furthermore, I think we should also consider
the already existing tests in the t1300 that contain such indentation.
As I already explained in my earlier response to Eric, [1] the choice
of including the indentation or not seems random to me, so we should
perhaps consider taking some broader approach.

How about this as a plan for moving forward:

1) Sprinkle a couple of tests onto the t1300, which try to be
    focused on the verification of the indentation-handling logic;
    maybe those additional tests could be even seen as redundant,
    but I think they can only help with the test coverage

2) Create a new helper function that uses the logic you described
    above, to make it simpler to include the indentation into configs

3) Finally, propagate the use of this new helper function into the
    new test and the already existing tests in the t1300 that already
    include the indentation

I'd be happy to implement all of the above-proposed steps in the next
couple of days.  Sure, it would be quite time-consuming, especially the
third proposed step, but it should be worth it in the long run.

[1] 
https://lore.kernel.org/git/c579edaac0d67a6ff46fe02072bddbb4@manjaro.org/

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

[Eric Sunshine]

On Mon, Mar 18, 2024 at 4:17 AM Dragan Simic <dsimic@manjaro.org> wrote:
> On 2024-03-18 03:48, Eric Sunshine wrote:
> > Readability wasn't my reason for bringing this up. As a reviewer,
> > every time a question pops into my mind as I'm reading the code, that
> > indicates that something about the code is unclear or that the commit
> > message doesn't properly explain why it was done in this way. People
> > coming across this code in the future may have the same questions but
> > they won't have the benefit of being able to easily ask you why it was
> > done this way.
>
> I see.  How about including a small comment in the t1300 that would
> explain the additional indentation?

I'm just one reviewer. Unless others chime in with similar
observations or questions regarding the patch, I don't think such a
comment is necessary. Aside from the other more significant points
(such as not introducing x_to_tab(), using "setup" in the function
title, etc.), this is extremely minor, and what you have here is "good
enough" (though you may want to take Junio's suggestion of using a
leading "|" to protect indentation).

> As a note, there are already more tests in the t1300 that contain such
> indentation, so maybe we shoulddo something with those existing tests
> as well;  the above-proposed comment, which would be placed at the very
> beginning of t1300, may provide a satisfactory explanation for all the
> tests in t1300 that contain such additional indentation.
>
> Another option would be to either add the indentation to all relevant
> tests in the t1300, or to remove the indentation from all tests in the
> t1300 that already contain it.  I'd be happy to implement and submit
> patches that do that, after we choose the direction we want to follow.

It would be better to keep this series focused on its primary goal of
fixing a bug rather than being held hostage to an ever increasing set
of potential cleanups. Such cleanups can be done as separate patch
series either atop this series or alongside it. Let's land this series
first, and then, if you wish, tackle those other less significant
issues.

> > If these new tests are also checking leading whitespace behavior, then
> > to improve coverage, would it make sense to have the leading "X" on
> > some lines but not others?
>
> Good point, despite that not being the main purpose of the added tests.
> I'll see to add a couple of tests that check the handling of
> indentation,
> possibly at some places in the t1300 that fit the best;  improving the
> tests coverage can only help in the long run.

As above, such additional tests probably aren't mandatory for this
bug-fix series. As a reviewer, I'd like to see fewer and fewer changes
between each version of a patch series; the series should converge so
that it can land rather than diverge from iteration to iteration. Such
additional leading-whitespace tests may be perfectly appropriate for a
follow-up series.

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

[Eric Sunshine]

On Mon, Mar 18, 2024 at 4:37 AM Dragan Simic <dsimic@manjaro.org> wrote:
> On 2024-03-18 05:38, Junio C Hamano wrote:
> >       sed -e 's/^|//' -e 's/Q/   /g' >.git/config <<-\EOF
> >       |[section]
> >       |       solid = rock
> >       |       sparse = big QQ blue
> >       |       ...
> >       EOF
> >
> This looks quite neat.  Furthermore, I think we should also consider
> the already existing tests in the t1300 that contain such indentation.
> As I already explained in my earlier response to Eric, [1] the choice
> of including the indentation or not seems random to me, so we should
> perhaps consider taking some broader approach.
>
> How about this as a plan for moving forward:
>
> 1) Sprinkle a couple of tests onto the t1300, which try to be
>     focused on the verification of the indentation-handling logic;
>     maybe those additional tests could be even seen as redundant,
>     but I think they can only help with the test coverage
>
> 2) Create a new helper function that uses the logic you described
>     above, to make it simpler to include the indentation into configs
>
> 3) Finally, propagate the use of this new helper function into the
>     new test and the already existing tests in the t1300 that already
>     include the indentation
>
> I'd be happy to implement all of the above-proposed steps in the next
> couple of days.  Sure, it would be quite time-consuming, especially the
> third proposed step, but it should be worth it in the long run.

As noted in my other response, while such cleanups may be nice in the
long-run, the bug-fix patch series under discussion should not be held
hostage by an ever-increasing set of potential cleanups. Let's focus
on landing the current series; these tangential cleanups can be done
in a separate series.

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> As a reviewer, I'd like to see fewer and fewer changes between
> each version of a patch series; the series should converge so that
> it can land rather than diverge from iteration to iteration.

Well said.

Thanks.

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

[Dragan Simic]

On 2024-03-18 20:17, Eric Sunshine wrote:
> On Mon, Mar 18, 2024 at 4:17 AM Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> On 2024-03-18 03:48, Eric Sunshine wrote:
>> > Readability wasn't my reason for bringing this up. As a reviewer,
>> > every time a question pops into my mind as I'm reading the code, that
>> > indicates that something about the code is unclear or that the commit
>> > message doesn't properly explain why it was done in this way. People
>> > coming across this code in the future may have the same questions but
>> > they won't have the benefit of being able to easily ask you why it was
>> > done this way.
>> 
>> I see.  How about including a small comment in the t1300 that would
>> explain the additional indentation?
> 
> I'm just one reviewer. Unless others chime in with similar
> observations or questions regarding the patch, I don't think such a
> comment is necessary. Aside from the other more significant points
> (such as not introducing x_to_tab(), using "setup" in the function
> title, etc.), this is extremely minor, and what you have here is "good
> enough" (though you may want to take Junio's suggestion of using a
> leading "|" to protect indentation).

Just to reiterate, both x_to_tab() and the test naming have already
been addressed in the future v3 of this series.

>> As a note, there are already more tests in the t1300 that contain such
>> indentation, so maybe we shoulddo something with those existing tests
>> as well;  the above-proposed comment, which would be placed at the 
>> very
>> beginning of t1300, may provide a satisfactory explanation for all the
>> tests in t1300 that contain such additional indentation.
>> 
>> Another option would be to either add the indentation to all relevant
>> tests in the t1300, or to remove the indentation from all tests in the
>> t1300 that already contain it.  I'd be happy to implement and submit
>> patches that do that, after we choose the direction we want to follow.
> 
> It would be better to keep this series focused on its primary goal of
> fixing a bug rather than being held hostage to an ever increasing set
> of potential cleanups. Such cleanups can be done as separate patch
> series either atop this series or alongside it. Let's land this series
> first, and then, if you wish, tackle those other less significant
> issues.

Thanks, I totally agree.

>> > If these new tests are also checking leading whitespace behavior, then
>> > to improve coverage, would it make sense to have the leading "X" on
>> > some lines but not others?
>> 
>> Good point, despite that not being the main purpose of the added 
>> tests.
>> I'll see to add a couple of tests that check the handling of
>> indentation,
>> possibly at some places in the t1300 that fit the best;  improving the
>> tests coverage can only help in the long run.
> 
> As above, such additional tests probably aren't mandatory for this
> bug-fix series. As a reviewer, I'd like to see fewer and fewer changes
> between each version of a patch series; the series should converge so
> that it can land rather than diverge from iteration to iteration. Such
> additional leading-whitespace tests may be perfectly appropriate for a
> follow-up series.

Agreed once again.  Let's wrap this up, and I'll come back with the
follow-up patches.

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

[Dragan Simic]

On 2024-03-18 20:21, Eric Sunshine wrote:
> On Mon, Mar 18, 2024 at 4:37 AM Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> On 2024-03-18 05:38, Junio C Hamano wrote:
>> >       sed -e 's/^|//' -e 's/Q/   /g' >.git/config <<-\EOF
>> >       |[section]
>> >       |       solid = rock
>> >       |       sparse = big QQ blue
>> >       |       ...
>> >       EOF
>> >
>> This looks quite neat.  Furthermore, I think we should also consider
>> the already existing tests in the t1300 that contain such indentation.
>> As I already explained in my earlier response to Eric, [1] the choice
>> of including the indentation or not seems random to me, so we should
>> perhaps consider taking some broader approach.
>> 
>> How about this as a plan for moving forward:
>> 
>> 1) Sprinkle a couple of tests onto the t1300, which try to be
>>     focused on the verification of the indentation-handling logic;
>>     maybe those additional tests could be even seen as redundant,
>>     but I think they can only help with the test coverage
>> 
>> 2) Create a new helper function that uses the logic you described
>>     above, to make it simpler to include the indentation into configs
>> 
>> 3) Finally, propagate the use of this new helper function into the
>>     new test and the already existing tests in the t1300 that already
>>     include the indentation
>> 
>> I'd be happy to implement all of the above-proposed steps in the next
>> couple of days.  Sure, it would be quite time-consuming, especially 
>> the
>> third proposed step, but it should be worth it in the long run.
> 
> As noted in my other response, while such cleanups may be nice in the
> long-run, the bug-fix patch series under discussion should not be held
> hostage by an ever-increasing set of potential cleanups. Let's focus
> on landing the current series; these tangential cleanups can be done
> in a separate series.

Totally agreed, let's keep this plan for the follow-up patches.