v3, 4 partsFix a bug in configuration parsing, and improve tests and documentation

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

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

Dragan Simic <dsimic@manjaro.org> writes:

Obviously correct.  Will queue.

On 2024-03-20 07:32, Junio C Hamano wrote:

Thanks.

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

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

Dragan Simic <dsimic@manjaro.org> writes:

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.

On 2024-03-20 07:42, Junio C Hamano wrote:

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.

On 2024-03-20 07:46, Dragan Simic wrote:

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?

Dragan Simic <dsimic@manjaro.org> writes:

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.

Hello Junio,

On 2024-03-20 15:28, Junio C Hamano wrote:

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

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

Dragan Simic <dsimic@manjaro.org> writes:

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

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

Good.

Thanks for working on this topic.

On 2024-03-20 08:12, Junio C Hamano wrote:

Makes sense, to make it less convoluted.

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.

I'll think about this as well.

Thank you for your highly detailed reviews!

Dragan Simic <dsimic@manjaro.org> writes:

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.

On 2024-03-20 15:42, Junio C Hamano wrote:

Good point.

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