{"thread":{"id":"61149","subject":"[PATCH v3 0/4] Fix a bug in configuration parsing, and improve tests and documentation","startedAt":"2024-03-18T22:24:27Z","lastAt":"2024-03-20T16:17:06Z","messageCount":16,"participants":["Dragan Simic","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":4},"messages":[{"id":"490906","messageId":"cover.1710800549.git.dsimic@manjaro.org","threadId":"61149","inReplyTo":null,"subject":"[PATCH v3 0/4] Fix a bug in configuration parsing, and improve tests and documentation","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-18T22:24:18Z","receivedAt":"2024-03-18T22:24:27Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"This series is an evolvement from another recent series, [1] as a result\nof a decision to fix a longstanding bug in the parsing of configuration\noption values, instead of documenting the status quo. [2][3]\n\nThe bufgix introduced in this series _should_ have no hidden negative\neffects.  All of the configuration-related tests, both the old and the\nnew ones, pass with the patches applied.\n\nIn v2, this series had five patches in total, out of which the third patch\n(i.e. patch 3/5) was dropped in v3. [4]  Other changes in v2 and v3 are\ndescribed in each of the patches.\n\nThere will be follow-up patches, to address the majority of the points\nraised during the review of this series. [5]\n\nLink to v2: https://lore.kernel.org/git/cover.1710646998.git.dsimic@manjaro.org/T/#u\n\n[1] https://lore.kernel.org/git/cover.1710258538.git.dsimic@manjaro.org/T/#u\n[2] https://lore.kernel.org/git/ff7b0a2ead90ad9a9456141da5e4df4a@manjaro.org/\n[3] https://lore.kernel.org/git/11be11f231f3bf41d0245c780c20693f@manjaro.org/\n[4] https://lore.kernel.org/git/514d832b0399ccdbc354675068477fea@manjaro.org/\n[5] https://lore.kernel.org/git/f37d753485094a3ba66fde5e85d0e2dc@manjaro.org/\n\nDragan Simic (4):\n  config: minor addition of whitespace\n  config: really keep value-internal whitespace verbatim\n  t1300: add more tests for whitespace and inline comments\n  config.txt: describe handling of whitespace further\n\n Documentation/config.txt |  19 ++++---\n config.c                 |  15 ++++--\n t/t1300-config.sh        | 112 +++++++++++++++++++++++++++++++++++++--\n 3 files changed, 130 insertions(+), 16 deletions(-)\n\n"},{"id":"490907","messageId":"2161355e5c9a8ca0c8c0d3e5115a8843be1f4111.1710800549.git.dsimic@manjaro.org","threadId":"61149","inReplyTo":"cover.1710800549.git.dsimic@manjaro.org","subject":"[PATCH v3 1/4] config: minor addition of whitespace","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-18T22:24:19Z","receivedAt":"2024-03-18T22:24:28Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"In general, binary operators should be enclosed in a pair of leading and\ntrailing space (SP) characters.  Thus, clean up one spotted expression that\nfor some reason had a \"bunched up\" operator.\n\nSigned-off-by: Dragan Simic <dsimic@manjaro.org>\n---\n\nNotes:\n    Changes in v3:\n        - Patch description was expanded a tiny bit, to make it more accurate\n        - No changes to the source code were introduced\n    \n    Changes in v2:\n        - No changes were introduced\n\n config.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex 3cfeb3d8bd99..a86a20cdf5cb 100644\n--- a/config.c\n+++ b/config.c\n@@ -869,7 +869,7 @@ static char *parse_value(struct config_source *cs)\n \t\t\tcontinue;\n \t\t}\n \t\tif (c == '\"') {\n-\t\t\tquote = 1-quote;\n+\t\t\tquote = 1 - quote;\n \t\t\tcontinue;\n \t\t}\n \t\tstrbuf_addch(&cs->value, c);\n"},{"id":"490908","messageId":"4a5fca5c701d59f123d7b1c8dd20f8b3b2e4d982.1710800549.git.dsimic@manjaro.org","threadId":"61149","inReplyTo":"cover.1710800549.git.dsimic@manjaro.org","subject":"[PATCH v3 2/4] config: really keep value-internal whitespace verbatim","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-18T22:24:20Z","receivedAt":"2024-03-18T22:24:28Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"Fix a bug in function parse_value() that prevented whitespace characters\n(i.e. spaces and horizontal tabs) found inside configuration option values\nfrom being parsed and returned in their original form.  The bug caused any\nnumber of consecutive whitespace characters to be wrongly \"squashed\" into\nthe same number of space characters.\n\nThis bug was introduced back in July 2009, in commit ebdaae372b46 (\"config:\nKeep inner whitespace verbatim\").\n\nFurther investigation showed that setting a configuration value, by invoking\ngit-config(1), converts value-internal horizontal tabs into \"\\t\" escape\nsequences, which the buggy value-parsing logic in function parse_value()\ndidn't \"squash\" into spaces.  That's why the test included in the ebdaae37\ncommit passed, which presumably made the bug remain undetected for this long.\nOn the other hand, value-internal literal horizontal tab characters, found in\na configuration file edited by hand, do get \"squashed\" by the value-parsing\nlogic, so the right choice was to fix this bug by making the value-internal\nwhitespace characters preserved verbatim.\n\nSigned-off-by: Dragan Simic <dsimic@manjaro.org>\n---\n\nNotes:\n    Changes in v3:\n        - No changes were introduced\n    \n    Changes in v2:\n        - Dropped the \"Fixes\" tag, as explained and requested by Junio, [1]\n          because having such tags actually doesn't help us in the long run\n        - No changes to the source code were introduced\n    \n    [1] https://lore.kernel.org/git/xmqq4jd7qtg6.fsf@gitster.g/\n\n config.c | 13 +++++++++----\n 1 file changed, 9 insertions(+), 4 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex a86a20cdf5cb..5072f12e62e4 100644\n--- a/config.c\n+++ b/config.c\n@@ -817,33 +817,38 @@ static int get_next_char(struct config_source *cs)\n \n static char *parse_value(struct config_source *cs)\n {\n-\tint quote = 0, comment = 0, space = 0;\n+\tint quote = 0, comment = 0;\n+\tsize_t trim_len = 0;\n \n \tstrbuf_reset(&cs->value);\n \tfor (;;) {\n \t\tint c = get_next_char(cs);\n \t\tif (c == '\\n') {\n \t\t\tif (quote) {\n \t\t\t\tcs->linenr--;\n \t\t\t\treturn NULL;\n \t\t\t}\n+\t\t\tif (trim_len)\n+\t\t\t\tstrbuf_setlen(&cs->value, trim_len);\n \t\t\treturn cs->value.buf;\n \t\t}\n \t\tif (comment)\n \t\t\tcontinue;\n \t\tif (isspace(c) && !quote) {\n+\t\t\tif (!trim_len)\n+\t\t\t\ttrim_len = cs->value.len;\n \t\t\tif (cs->value.len)\n-\t\t\t\tspace++;\n+\t\t\t\tstrbuf_addch(&cs->value, c);\n \t\t\tcontinue;\n \t\t}\n \t\tif (!quote) {\n \t\t\tif (c == ';' || c == '#') {\n \t\t\t\tcomment = 1;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t}\n-\t\tfor (; space; space--)\n-\t\t\tstrbuf_addch(&cs->value, ' ');\n+\t\tif (trim_len)\n+\t\t\ttrim_len = 0;\n \t\tif (c == '\\\\') {\n \t\t\tc = get_next_char(cs);\n \t\t\tswitch (c) {\n"},{"id":"490909","messageId":"92ddcd1f668906348e20a682cd737d90bb38ddc6.1710800549.git.dsimic@manjaro.org","threadId":"61149","inReplyTo":"cover.1710800549.git.dsimic@manjaro.org","subject":"[PATCH v3 3/4] t1300: add more tests for whitespace and inline comments","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-18T22:24:21Z","receivedAt":"2024-03-18T22:24:29Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"Add a handful of additional automated tests, to improve the coverage of\nconfiguration file entries whose values contain internal whitespace, leading\nand/or trailing whitespace, which may or may not be enclosed within quotation\nmarks, or which contain an additional inline comment.\n\nAt the same time, rework one already existing automated test a bit, to ensure\nconsistency with the newly added tests.  This change introduced no functional\nchanges to the already existing test.\n\nHelped-by: Eric Sunshine <sunshine@sunshineco.com>\nSigned-off-by: Dragan Simic <dsimic@manjaro.org>\n---\n\nNotes:\n    Changes in v3:\n        - Removed a few unnecessary invocations of x_to_tab()\n        - As pointed out by Eric Sunshine, [3] it's better not to introduce\n          new random helper functions, so x_to_tab() was replaced by a direct\n          invocation of tr(1), in one case that requires use of literal 'Q'\n          characters, and by invocations of q_to_tab(), in the remaining cases\n          that actually allow use of 'Q' characters to represent HTs\n        - Dropped the change of the name of an already existing test, to not\n          distract the reviewers, as suggested by Eric Sunshine [4]\n        - Renamed the first added test, as suggested by Eric Sunshine, [4] to\n          make it consistent with the expected way for naming setup tests\n    \n    Changes in v2:\n        - All new tests and one already existing test reworked according to\n          Eric Sunshine's review suggestions; [1][2]  the already existing\n          test was reworked a bit to ensure consistency\n        - Added a Helped-by tag\n    \n    [1] https://lore.kernel.org/git/CAPig+cRMPNExbG34xJ0w5npUc3DDwxQUGS_AQfam_mi4s53=sA@mail.gmail.com/\n    [2] https://lore.kernel.org/git/CAPig+cRG8eFxepkaiN54H+fa7D=rFGsmEHdvTP+HSSaLO_6T_A@mail.gmail.com/\n    [3] https://lore.kernel.org/git/CAPig+cSLb+Rsy81itvw9Tfvqv9vvKSPgO_ER9fWL04XZrgFvwg@mail.gmail.com/T/#u\n    [4] https://lore.kernel.org/git/CAPig+cTVmQzC38DympSEtPNhgY=-+dYbZmkr0RTRbhG-hp2fmQ@mail.gmail.com/\n\n t/t1300-config.sh | 112 ++++++++++++++++++++++++++++++++++++++++++++--\n 1 file changed, 109 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex 31c387868708..7688362826ea 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -11,6 +11,96 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n+test_expect_success 'setup whitespace config' '\n+\ttr \"X\" \"\\011\" >.git/config <<-\\EOF\n+\t[section]\n+\t\tXsolid = rock\n+\t\tXsparse = big XX blue\n+\t\tXsparseAndTail = big XX blue \n+\t\tXsparseAndTailQuoted = \"big XX blue \"\n+\t\tXsparseAndBiggerTail = big XX blue X X\n+\t\tXsparseAndBiggerTailQuoted = \"big XX blue X X\"\n+\t\tXsparseAndBiggerTailQuotedPlus = \"big XX blue X X\"X \n+\t\tXheadAndTail = Xbig blue \n+\t\tXheadAndTailQuoted = \"Xbig blue \"\n+\t\tXheadAndTailQuotedPlus = \"Xbig blue \" \n+\t\tXannotated = big blueX# to be discarded\n+\t\tXannotatedQuoted = \"big blue\"X# to be discarded\n+\tEOF\n+'\n+\n+test_expect_success 'no internal whitespace' '\n+\techo \"rock\" >expect &&\n+\tgit config --get section.solid >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'internal whitespace' '\n+\techo \"big QQ blue\" | q_to_tab >expect &&\n+\tgit config --get section.sparse >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'internal and trailing whitespace' '\n+\techo \"big QQ blue\" | q_to_tab >expect &&\n+\tgit config --get section.sparseAndTail >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'internal and trailing whitespace, all quoted' '\n+\techo \"big QQ blue \" | q_to_tab >expect &&\n+\tgit config --get section.sparseAndTailQuoted >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'internal and more trailing whitespace' '\n+\techo \"big QQ blue\" | q_to_tab >expect &&\n+\tgit config --get section.sparseAndBiggerTail >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'internal and more trailing whitespace, all quoted' '\n+\techo \"big QQ blue Q Q\" | q_to_tab >expect &&\n+\tgit config --get section.sparseAndBiggerTailQuoted >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'internal and more trailing whitespace, not all quoted' '\n+\techo \"big QQ blue Q Q\" | q_to_tab >expect &&\n+\tgit config --get section.sparseAndBiggerTailQuotedPlus >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'leading and trailing whitespace' '\n+\techo \"big blue\" >expect &&\n+\tgit config --get section.headAndTail >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'leading and trailing whitespace, all quoted' '\n+\techo \"Qbig blue \" | q_to_tab >expect &&\n+\tgit config --get section.headAndTailQuoted >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'leading and trailing whitespace, not all quoted' '\n+\techo \"Qbig blue \" | q_to_tab >expect &&\n+\tgit config --get section.headAndTailQuotedPlus >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'inline comment' '\n+\techo \"big blue\" >expect &&\n+\tgit config --get section.annotated >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'inline comment, quoted' '\n+\techo \"big blue\" >expect &&\n+\tgit config --get section.annotatedQuoted >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'clear default config' '\n \trm -f .git/config\n '\n@@ -1066,9 +1156,25 @@ test_expect_success '--null --get-regexp' '\n \ttest_cmp expect result\n '\n \n-test_expect_success 'inner whitespace kept verbatim' '\n-\tgit config section.val \"foo \t  bar\" &&\n-\ttest_cmp_config \"foo \t  bar\" section.val\n+test_expect_success 'inner whitespace kept verbatim, spaces only' '\n+\techo \"foo   bar\" >expect &&\n+\tgit config section.val \"foo   bar\" &&\n+\tgit config --get section.val >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'inner whitespace kept verbatim, horizontal tabs only' '\n+\techo \"fooQQbar\" | q_to_tab >expect &&\n+\tgit config section.val \"$(cat expect)\" &&\n+\tgit config --get section.val >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'inner whitespace kept verbatim, horizontal tabs and spaces' '\n+\techo \"foo Q  bar\" | q_to_tab >expect &&\n+\tgit config section.val \"$(cat expect)\" &&\n+\tgit config --get section.val >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success SYMLINKS 'symlinked configuration' '\n"},{"id":"490910","messageId":"e389acbfacd5046a926b87346d41f9c7962e3c23.1710800549.git.dsimic@manjaro.org","threadId":"61149","inReplyTo":"cover.1710800549.git.dsimic@manjaro.org","subject":"[PATCH v3 4/4] config.txt: describe handling of whitespace further","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-18T22:24:22Z","receivedAt":"2024-03-18T22:24:29Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"Make it more clear what the whitespace characters are in the context of git\nconfiguration files, and improve the description of the trailing whitespace\nhandling a bit, especially how it works out together with the presence of\ninline comments.\n\nSigned-off-by: Dragan Simic <dsimic@manjaro.org>\n---\n\nNotes:\n    Changes in v3:\n        - Patch description was expanded a bit, to make it more on point\n        - No changes to the documentation were introduced\n    \n    Changes in v2:\n        - No changes were introduced\n\n Documentation/config.txt | 19 +++++++++++--------\n 1 file changed, 11 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 782c2bab906c..20f3300dc706 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -22,9 +22,10 @@ multivalued.\n Syntax\n ~~~~~~\n \n-The syntax is fairly flexible and permissive; whitespaces are mostly\n-ignored.  The '#' and ';' characters begin comments to the end of line,\n-blank lines are ignored.\n+The syntax is fairly flexible and permissive.  Whitespace characters,\n+which in this context are the space character (SP) and the horizontal\n+tabulation (HT), are mostly ignored.  The '#' and ';' characters begin\n+comments to the end of line.  Blank lines are ignored.\n \n The file consists of sections and variables.  A section begins with\n the name of the section in square brackets and continues until the next\n@@ -64,12 +65,14 @@ The variable names are case-insensitive, allow only alphanumeric characters\n and `-`, and must start with an alphabetic character.\n \n A line that defines a value can be continued to the next line by\n-ending it with a `\\`; the backslash and the end-of-line are\n-stripped.  Leading whitespaces after 'name =', the remainder of the\n+ending it with a `\\`; the backslash and the end-of-line are stripped.\n+Leading whitespace characters after 'name =', the remainder of the\n line after the first comment character '#' or ';', and trailing\n-whitespaces of the line are discarded unless they are enclosed in\n-double quotes.  Internal whitespaces within the value are retained\n-verbatim.\n+whitespace characters of the line are discarded unless they are enclosed\n+in double quotes.  The discarding of the trailing whitespace characters\n+applies regardless of the discarding of the portion of the line after\n+the first comment character.  Internal whitespace characters within the\n+value are retained verbatim.\n \n Inside double quotes, double quote `\"` and backslash `\\` characters\n must be escaped: use `\\\"` for `\"` and `\\\\` for `\\`.\n"},{"id":"490990","messageId":"xmqq5xxhl8gs.fsf@gitster.g","threadId":"61149","inReplyTo":"2161355e5c9a8ca0c8c0d3e5115a8843be1f4111.1710800549.git.dsimic@manjaro.org","subject":"Re: [PATCH v3 1/4] config: minor addition of whitespace","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-20T06:32:03Z","receivedAt":"2024-03-20T06:32:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dragan Simic <dsimic@manjaro.org> writes:\n\n>  \t\tif (c == '\"') {\n> -\t\t\tquote = 1-quote;\n> +\t\t\tquote = 1 - quote;\n>  \t\t\tcontinue;\n\nObviously correct.  Will queue.\n"},{"id":"490991","messageId":"60422ac2db78435843f11f2ec8ddaff2@manjaro.org","threadId":"61149","inReplyTo":"xmqq5xxhl8gs.fsf@gitster.g","subject":"Re: [PATCH v3 1/4] config: minor addition of whitespace","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-20T06:36:07Z","receivedAt":"2024-03-20T06:36:11Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-03-20 07:32, Junio C Hamano wrote:\n> Dragan Simic <dsimic@manjaro.org> writes:\n> \n>>  \t\tif (c == '\"') {\n>> -\t\t\tquote = 1-quote;\n>> +\t\t\tquote = 1 - quote;\n>>  \t\t\tcontinue;\n> \n> Obviously correct.  Will queue.\n\nThanks.\n"},{"id":"490992","messageId":"xmqqzfutjtfo.fsf@gitster.g","threadId":"61149","inReplyTo":"92ddcd1f668906348e20a682cd737d90bb38ddc6.1710800549.git.dsimic@manjaro.org","subject":"Re: [PATCH v3 3/4] t1300: add more tests for whitespace and inline comments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-20T06:42:03Z","receivedAt":"2024-03-20T06:42:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dragan Simic <dsimic@manjaro.org> writes:\n\n> +test_expect_success 'setup whitespace config' '\n> +\ttr \"X\" \"\\011\" >.git/config <<-\\EOF\n> +\t[section]\n> +\t\tXsolid = rock\n> +\t\tXsparse = big XX blue\n> +\t\tXsparseAndTail = big XX blue \n> +\t\tXsparseAndTailQuoted = \"big XX blue \"\n> +\t\tXsparseAndBiggerTail = big XX blue X X\n> +\t\tXsparseAndBiggerTailQuoted = \"big XX blue X X\"\n> +\t\tXsparseAndBiggerTailQuotedPlus = \"big XX blue X X\"X \n> +\t\tXheadAndTail = Xbig blue \n> +\t\tXheadAndTailQuoted = \"Xbig blue \"\n> +\t\tXheadAndTailQuotedPlus = \"Xbig blue \" \n> +\t\tXannotated = big blueX# to be discarded\n> +\t\tXannotatedQuoted = \"big blue\"X# to be discarded\n> +\tEOF\n> +'\n\nIf you want to write tests where leading and trailing whitespace\nmatter in them, making these invisible characters visible is a good\nway to convey your intention to your readers.\n\n\tsed -e \"s/Q/\t/g\" \\\n\t    -e \"s/^|//\" \\\n\t    -e \"s/[$]$//\" <<-\\EOF\n\t|[section]\n\t|\tsolid = rock  $\n\t|\tsparse = tab and space Q $\n\tEOF\n\nThis is still true even if we assume there is no patch corruption\nwhile the e-mail is in transit.  It is safe to assume that the\nreceiving end of your patches uses \"git am --whitespace=fix\" and a\ncommon way to protect against it is to use the opposite of \"cat -e\"\nas a convention, additional to the \"'|' is the left edge of paper\"\nand \"Q stands for HT\" conventions.\n"},{"id":"490993","messageId":"32c4718d09ff6675e37587e15e785413@manjaro.org","threadId":"61149","inReplyTo":"xmqqzfutjtfo.fsf@gitster.g","subject":"Re: [PATCH v3 3/4] t1300: add more tests for whitespace and inline comments","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-20T06:46:41Z","receivedAt":"2024-03-20T06:46:44Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-03-20 07:42, Junio C Hamano wrote:\n> Dragan Simic <dsimic@manjaro.org> writes:\n> \n>> +test_expect_success 'setup whitespace config' '\n>> +\ttr \"X\" \"\\011\" >.git/config <<-\\EOF\n>> +\t[section]\n>> +\t\tXsolid = rock\n>> +\t\tXsparse = big XX blue\n>> +\t\tXsparseAndTail = big XX blue\n>> +\t\tXsparseAndTailQuoted = \"big XX blue \"\n>> +\t\tXsparseAndBiggerTail = big XX blue X X\n>> +\t\tXsparseAndBiggerTailQuoted = \"big XX blue X X\"\n>> +\t\tXsparseAndBiggerTailQuotedPlus = \"big XX blue X X\"X\n>> +\t\tXheadAndTail = Xbig blue\n>> +\t\tXheadAndTailQuoted = \"Xbig blue \"\n>> +\t\tXheadAndTailQuotedPlus = \"Xbig blue \"\n>> +\t\tXannotated = big blueX# to be discarded\n>> +\t\tXannotatedQuoted = \"big blue\"X# to be discarded\n>> +\tEOF\n>> +'\n> \n> If you want to write tests where leading and trailing whitespace\n> matter in them, making these invisible characters visible is a good\n> way to convey your intention to your readers.\n> \n> \tsed -e \"s/Q/\t/g\" \\\n> \t    -e \"s/^|//\" \\\n> \t    -e \"s/[$]$//\" <<-\\EOF\n> \t|[section]\n> \t|\tsolid = rock  $\n> \t|\tsparse = tab and space Q $\n> \tEOF\n> \n> This is still true even if we assume there is no patch corruption\n> while the e-mail is in transit.  It is safe to assume that the\n> receiving end of your patches uses \"git am --whitespace=fix\" and a\n> common way to protect against it is to use the opposite of \"cat -e\"\n> as a convention, additional to the \"'|' is the left edge of paper\"\n> and \"Q stands for HT\" conventions.\n\nAgreed, will make it so in the v4.  I already had some thoughts\nabout protecting the trailing whitespace, and making obvious it\nwasn't included by mistake.\n\nThough, I'll have to use 'X' istead of 'Q' for HTs, because the\noption names already contain 'Q' characters.\n"},{"id":"490994","messageId":"c5736219057c73a3a344237257534bdc@manjaro.org","threadId":"61149","inReplyTo":"32c4718d09ff6675e37587e15e785413@manjaro.org","subject":"Re: [PATCH v3 3/4] t1300: add more tests for whitespace and inline comments","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-20T06:59:18Z","receivedAt":"2024-03-20T06:59:20Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-03-20 07:46, Dragan Simic wrote:\n> On 2024-03-20 07:42, Junio C Hamano wrote:\n>> Dragan Simic <dsimic@manjaro.org> writes:\n>> \n>>> +test_expect_success 'setup whitespace config' '\n>>> +\ttr \"X\" \"\\011\" >.git/config <<-\\EOF\n>>> +\t[section]\n>>> +\t\tXsolid = rock\n>>> +\t\tXsparse = big XX blue\n>>> +\t\tXsparseAndTail = big XX blue\n>>> +\t\tXsparseAndTailQuoted = \"big XX blue \"\n>>> +\t\tXsparseAndBiggerTail = big XX blue X X\n>>> +\t\tXsparseAndBiggerTailQuoted = \"big XX blue X X\"\n>>> +\t\tXsparseAndBiggerTailQuotedPlus = \"big XX blue X X\"X\n>>> +\t\tXheadAndTail = Xbig blue\n>>> +\t\tXheadAndTailQuoted = \"Xbig blue \"\n>>> +\t\tXheadAndTailQuotedPlus = \"Xbig blue \"\n>>> +\t\tXannotated = big blueX# to be discarded\n>>> +\t\tXannotatedQuoted = \"big blue\"X# to be discarded\n>>> +\tEOF\n>>> +'\n>> \n>> If you want to write tests where leading and trailing whitespace\n>> matter in them, making these invisible characters visible is a good\n>> way to convey your intention to your readers.\n>> \n>> \tsed -e \"s/Q/\t/g\" \\\n>> \t    -e \"s/^|//\" \\\n>> \t    -e \"s/[$]$//\" <<-\\EOF\n>> \t|[section]\n>> \t|\tsolid = rock  $\n>> \t|\tsparse = tab and space Q $\n>> \tEOF\n>> \n>> This is still true even if we assume there is no patch corruption\n>> while the e-mail is in transit.  It is safe to assume that the\n>> receiving end of your patches uses \"git am --whitespace=fix\" and a\n>> common way to protect against it is to use the opposite of \"cat -e\"\n>> as a convention, additional to the \"'|' is the left edge of paper\"\n>> and \"Q stands for HT\" conventions.\n> \n> Agreed, will make it so in the v4.  I already had some thoughts\n> about protecting the trailing whitespace, and making obvious it\n> wasn't included by mistake.\n> \n> Though, I'll have to use 'X' istead of 'Q' for HTs, because the\n> option names already contain 'Q' characters.\n\nOh, I just saw that you've already picked this patch up in the \"seen\"\nbranch.  Would you prefer if I make this change and submit the v4, or\nto perform the change in the already planned follow-up patches, which\nwould also clean up some other tests a bit?\n"},{"id":"490995","messageId":"xmqqttl1js1o.fsf@gitster.g","threadId":"61149","inReplyTo":"e389acbfacd5046a926b87346d41f9c7962e3c23.1710800549.git.dsimic@manjaro.org","subject":"Re: [PATCH v3 4/4] config.txt: describe handling of whitespace further","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-20T07:12:03Z","receivedAt":"2024-03-20T07:12:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dragan Simic <dsimic@manjaro.org> writes:\n\n>  A line that defines a value can be continued to the next line by\n> +ending it with a `\\`; the backslash and the end-of-line are stripped.\n> +Leading whitespace characters after 'name =', the remainder of the\n>  line after the first comment character '#' or ';', and trailing\n> +whitespace characters of the line are discarded unless they are enclosed\n> +in double quotes.\n\nCan we directly tighten the \"trailing...\" part, instead of having to\nadd an extra long sentence ...\n\n> +The discarding of the trailing whitespace characters\n> +applies regardless of the discarding of the portion of the line after\n> +the first comment character.\n\n... like this as an attempt to clarify?\n\n    Leading whitespace characters before and after 'name =', and the\n    remainder of the line after the first comment character '#' or\n    ';', are removed, and then trailing whitespace characters at the\n    end of the line are discarded.\n\nBy the way, if a run of whitespace characters are enclosed in double\nquotes, they cannot be trailing at the end of the line, as the\nclosing double quote is not a whitespace character, so it is out of\nplace to talk about quoted string in the context of trailing blank\nremoval.  The unquoting would want to be discussed separately.\n\n> +Internal whitespace characters within the\n> +value are retained verbatim.\n\nGood.\n\n>  \n>  Inside double quotes, double quote `\"` and backslash `\\` characters\n>  must be escaped: use `\\\"` for `\"` and `\\\\` for `\\`.\n\nThanks for working on this topic.\n"},{"id":"490997","messageId":"c4f0e7311edce5d4ed4a55f290de9eaf@manjaro.org","threadId":"61149","inReplyTo":"xmqqttl1js1o.fsf@gitster.g","subject":"Re: [PATCH v3 4/4] config.txt: describe handling of whitespace further","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-20T07:23:48Z","receivedAt":"2024-03-20T07:23:51Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-03-20 08:12, Junio C Hamano wrote:\n> Dragan Simic <dsimic@manjaro.org> writes:\n> \n>>  A line that defines a value can be continued to the next line by\n>> +ending it with a `\\`; the backslash and the end-of-line are stripped.\n>> +Leading whitespace characters after 'name =', the remainder of the\n>>  line after the first comment character '#' or ';', and trailing\n>> +whitespace characters of the line are discarded unless they are \n>> enclosed\n>> +in double quotes.\n> \n> Can we directly tighten the \"trailing...\" part, instead of having to\n> add an extra long sentence ...\n\nMakes sense, to make it less convoluted.\n\n>> +The discarding of the trailing whitespace characters\n>> +applies regardless of the discarding of the portion of the line after\n>> +the first comment character.\n> \n> ... like this as an attempt to clarify?\n> \n>     Leading whitespace characters before and after 'name =', and the\n\nHmm, \"leading whitespace\" and \"after\" don't go very well together.\nSuch a construct seems a bit confusing, because it implies there's\nsomething else after, which the leading whitespace refers to, which\nmay or may not be easily understandable to the users.\n\nI'll think about how to rephrase this a bit better.\n\n>     remainder of the line after the first comment character '#' or\n>     ';', are removed, and then trailing whitespace characters at the\n>     end of the line are discarded.\n> \n> By the way, if a run of whitespace characters are enclosed in double\n> quotes, they cannot be trailing at the end of the line, as the\n> closing double quote is not a whitespace character, so it is out of\n> place to talk about quoted string in the context of trailing blank\n> removal.  The unquoting would want to be discussed separately.\n\nI'll think about this as well.\n\n>>  Inside double quotes, double quote `\"` and backslash `\\` characters\n>>  must be escaped: use `\\\"` for `\"` and `\\\\` for `\\`.\n> \n> Thanks for working on this topic.\n\nThank you for your highly detailed reviews!\n"},{"id":"491012","messageId":"xmqq4jd1j7tp.fsf@gitster.g","threadId":"61149","inReplyTo":"c5736219057c73a3a344237257534bdc@manjaro.org","subject":"Re: [PATCH v3 3/4] t1300: add more tests for whitespace and inline comments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-20T14:28:50Z","receivedAt":"2024-03-20T14:28:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dragan Simic <dsimic@manjaro.org> writes:\n\n> Oh, I just saw that you've already picked this patch up in the \"seen\"\n> branch.  Would you prefer if I make this change and submit the v4, or\n> to perform the change in the already planned follow-up patches, which\n> would also clean up some other tests a bit?\n\nThe purpose of the \"seen\" branch is to bundle the branches the\nmaintainer happens to have seen, and to remind the maintainer that\nthe topics in them might turn out to be interesting when they are\npolished.  Nothing more than that.  Consider that a topic only in\n\"seen\" is not part of \"git\" yet.\n\nThe contributors can use it to anticipate what topics from others\nmay cause conflict with their own work, and find people who are\nworking on these topics to talk to before the potential conflicts\nget out of control.  It would be a good idea to fork from maint or\nmaster to grow a topic and to test (1) it by itself, (2) a temporary\nmerge of it to 'next' and (3) a temporary merge to it to 'seen',\nbefore publishing it.\n"},{"id":"491016","messageId":"xmqq4jd1hsm0.fsf@gitster.g","threadId":"61149","inReplyTo":"c4f0e7311edce5d4ed4a55f290de9eaf@manjaro.org","subject":"Re: [PATCH v3 4/4] config.txt: describe handling of whitespace further","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-20T14:42:47Z","receivedAt":"2024-03-20T14:42:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dragan Simic <dsimic@manjaro.org> writes:\n\n>>     Leading whitespace characters before and after 'name =', and the\n> Hmm, \"leading whitespace\" and \"after\" don't go very well together.\n\nTrue.  We can drop \"leading\" of course.  I meant to refer to, in\nthis sample illustration,\n\n[section]\n\tvar = value # comment\n\nthe fact that \"\\t\" before \"var =\" is discarded, and \" \" after \"var =\"\nbefore \"value\" is also discarded.\n\nThanks.\n"},{"id":"491020","messageId":"1b475b48a88be57efde6d9fc2d8d50c8@manjaro.org","threadId":"61149","inReplyTo":"xmqq4jd1j7tp.fsf@gitster.g","subject":"Re: [PATCH v3 3/4] t1300: add more tests for whitespace and inline comments","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-20T16:11:01Z","receivedAt":"2024-03-20T16:11:09Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"Hello Junio,\n\nOn 2024-03-20 15:28, Junio C Hamano wrote:\n> Dragan Simic <dsimic@manjaro.org> writes:\n> \n>> Oh, I just saw that you've already picked this patch up in the \"seen\"\n>> branch.  Would you prefer if I make this change and submit the v4, or\n>> to perform the change in the already planned follow-up patches, which\n>> would also clean up some other tests a bit?\n> \n> The purpose of the \"seen\" branch is to bundle the branches the\n> maintainer happens to have seen, and to remind the maintainer that\n> the topics in them might turn out to be interesting when they are\n> polished.  Nothing more than that.  Consider that a topic only in\n> \"seen\" is not part of \"git\" yet.\n> \n> The contributors can use it to anticipate what topics from others\n> may cause conflict with their own work, and find people who are\n> working on these topics to talk to before the potential conflicts\n> get out of control.  It would be a good idea to fork from maint or\n> master to grow a topic and to test (1) it by itself, (2) a temporary\n> merge of it to 'next' and (3) a temporary merge to it to 'seen',\n> before publishing it.\n\nThank you for the clarification.  Hence, I'll send the v4.\n"},{"id":"491021","messageId":"ce041191a245ff888b1710cdcaad9e61@manjaro.org","threadId":"61149","inReplyTo":"xmqq4jd1hsm0.fsf@gitster.g","subject":"Re: [PATCH v3 4/4] config.txt: describe handling of whitespace further","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-20T16:17:04Z","receivedAt":"2024-03-20T16:17:06Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-03-20 15:42, Junio C Hamano wrote:\n> Dragan Simic <dsimic@manjaro.org> writes:\n> \n>>>     Leading whitespace characters before and after 'name =', and the\n>> Hmm, \"leading whitespace\" and \"after\" don't go very well together.\n> \n> True.  We can drop \"leading\" of course.\n\nGood point.\n\n> I meant to refer to, in this sample illustration,\n> \n> [section]\n> \tvar = value # comment\n> \n> the fact that \"\\t\" before \"var =\" is discarded, and \" \" after \"var =\"\n> before \"value\" is also discarded.\n\nSure, thanks for the clarification.  I'll try to tweak the wording\na bit further.\n"}]}