{"thread":{"id":"61168","subject":"[PATCH v4 1/4] config: minor addition of whitespace","startedAt":"2024-03-21T04:17:48Z","lastAt":"2024-03-21T07:32:09Z","messageCount":14,"participants":["Dragan Simic","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":4},"messages":[{"id":"491091","messageId":"dda81fedd9f38cb57658f1343f803a339c9116e3.1710994548.git.dsimic@manjaro.org","threadId":"61168","inReplyTo":"cover.1710994548.git.dsimic@manjaro.org","subject":"[PATCH v4 1/4] config: minor addition of whitespace","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-21T04:17:40Z","receivedAt":"2024-03-21T04:17:48Z","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 \n---\n\nNotes:\n Changes in v4:\n - Junio responded with \"will queue\" in v3, [1] but this patch hasn't\n reached the \"next\" branch yet, so resending it as a reminder\n - No changes were introduced\n \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 [1] https://lore.kernel.org/git/xmqq5xxhl8gs.fsf@gitster.g/\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":"491092","messageId":"cover.1710994548.git.dsimic@manjaro.org","threadId":"61168","inReplyTo":null,"subject":"[PATCH v4 0/4] Fix a bug in configuration parsing, and improve tests and documentation","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-21T04:17:39Z","receivedAt":"2024-03-21T04:17:48Z","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 each version are\ndescribed in each patch.\n\nThere will most probably be follow-up patches, to address the remaining\npoints raised during the review of this series. [5]\n\nLink to v2: https://lore.kernel.org/git/cover.1710646998.git.dsimic@manjaro.org/T/#u\nLink to v3: https://lore.kernel.org/git/cover.1710800549.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 | 21 ++++----\n config.c | 15 ++++--\n t/t1300-config.sh | 114 +++++++++++++++++++++++++++++++++++++--\n 3 files changed, 133 insertions(+), 17 deletions(-)\n\n"},{"id":"491094","messageId":"36393da367dc6af7e4f045c4804309cb8cb04378.1710994548.git.dsimic@manjaro.org","threadId":"61168","inReplyTo":"cover.1710994548.git.dsimic@manjaro.org","subject":"[PATCH v4 2/4] config: really keep value-internal whitespace verbatim","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-21T04:17:41Z","receivedAt":"2024-03-21T04:17:49Z","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 \n---\n\nNotes:\n Changes in v4:\n - No changes were introduced\n \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":"491093","messageId":"688128d8ef09589712634888074ffd71a192a7aa.1710994548.git.dsimic@manjaro.org","threadId":"61168","inReplyTo":"cover.1710994548.git.dsimic@manjaro.org","subject":"[PATCH v4 3/4] t1300: add more tests for whitespace and inline comments","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-21T04:17:42Z","receivedAt":"2024-03-21T04:17:50Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"Add a handful of additional tests, to improve the coverage of the handling\nof configuration file entries whose values contain internal whitespace,\nleading and/or trailing whitespace, which may or may not be enclosed within\nquotation marks, or which contain an additional inline comment.\n\nAt the same time, rework one already existing whitespace-related test a bit,\nto ensure its consistency with the newly added tests. This change introduced\nno functional changes to the already existing test.\n\nHelped-by: Eric Sunshine \nHelped-by: Junio C Hamano \nSigned-off-by: Dragan Simic \n---\n\nNotes:\n Changes in v4:\n - Improved the added configuration setup test, to make the included\n leading and trailing whitespace characters more robust and easier\n on the eyes, as suggested and outlined by Junio [5]\n - Expanded the patch description a bit and improved the wording\n - Added a Helped-by tag\n \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 [5] https://lore.kernel.org/git/32c4718d09ff6675e37587e15e785413@manjaro.org/\n\n t/t1300-config.sh | 114 ++++++++++++++++++++++++++++++++++++++++++++--\n 1 file changed, 111 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex 31c387868708..6ecc9dd4c3d2 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -11,6 +11,98 @@ 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+\tsed -e \"s/^|//\" \\\n+\t -e \"s/[$]$//\" \\\n+\t -e \"s/X/\\\\t/g\" >.git/config <<-\\EOF\n+\t[section]\n+\t|\tsolid = rock\n+\t|\tsparse = big XX blue\n+\t|\tsparseAndTail = big XX blue $\n+\t|\tsparseAndTailQuoted = \"big XX blue \"\n+\t|\tsparseAndBiggerTail = big XX blue X X\n+\t|\tsparseAndBiggerTailQuoted = \"big XX blue X X\"\n+\t|\tsparseAndBiggerTailQuotedPlus = \"big XX blue X X\"X $\n+\t|\theadAndTail = Xbig blue $\n+\t|\theadAndTailQuoted = \"Xbig blue \"\n+\t|\theadAndTailQuotedPlus = \"Xbig blue \" $\n+\t|\tannotated = big blueX# to be discarded\n+\t|\tannotatedQuoted = \"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 +1158,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":"491095","messageId":"f84c5e8e4a90be3f9fe3cc853e0d40aed4e58826.1710994548.git.dsimic@manjaro.org","threadId":"61168","inReplyTo":"cover.1710994548.git.dsimic@manjaro.org","subject":"[PATCH v4 4/4] config.txt: describe handling of whitespace further","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-21T04:17:43Z","receivedAt":"2024-03-21T04:17:51Z","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 significantly improve the description of the leading\nand trailing whitespace handling, especially how it works out together with\nthe presence of inline comments.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Dragan Simic \n---\n\nNotes:\n Changes in v4:\n - Improved the wording and accuracy of the description of whitespace\n character handling, as discussed with Junio, [1][2] by taking a more\n radical approach and rewriting an entire paragraph, because it has\n reached the point where \"patching the patchwork\" no longer worked;\n I'm quite happy with the way it turned out this time\n - Expanded the patch description a tiny bit\n - Added a Helped-by tag\n \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 [1] https://lore.kernel.org/git/xmqqttl1js1o.fsf@gitster.g/\n [2] https://lore.kernel.org/git/ce041191a245ff888b1710cdcaad9e61@manjaro.org/\n\n Documentation/config.txt | 21 ++++++++++++---------\n 1 file changed, 12 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 782c2bab906c..9d4e99393530 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@@ -63,13 +64,15 @@ the variable is the boolean \"true\").\n The variable names are case-insensitive, allow only alphanumeric characters\n and `-`, and must start with an alphabetic character.\n \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-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+ending it with a `\\`; the backslash and the end-of-line are stripped.\n+Leading whitespace characters before 'name =' are discarded.\n+The portion of the line after the first comment character, including\n+the comment character itself, is discarded. Unless enclosed in double\n+quotation marks (`\"`), any leading or trailing whitespace characters\n+surrounding 'value' are discarded. Internal whitespace characters\n+within 'value' are retained verbatim.\n \n Inside double quotes, double quote `\"` and backslash `\\` characters\n must be escaped: use `\\\"` for `\"` and `\\\\` for `\\`.\n"},{"id":"491096","messageId":"CAPig+cRCCqRHYOpMxdL4MWaSF5S0Qb7fcJ5HqkT0j_MU1dRvfQ@mail.gmail.com","threadId":"61168","inReplyTo":"cover.1710994548.git.dsimic@manjaro.org","subject":"Re: [PATCH v4 0/4] Fix a bug in configuration parsing, and improve tests and documentation","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-03-21T04:50:34Z","receivedAt":"2024-03-21T04:50:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 21, 2024 at 12:17 AM Dragan Simic wrote:\n> In 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 each version are\n> described in each patch.\n\nIn addition to a written description of the changes since the previous\nversion, reviewers also greatly appreciate seeing a range-diff which\nyou can generate automatically in the cover-letter via:\n\n git format-patch --cover-letter --range-diff=<...> ...\n\nor less frequently, an interdiff via:\n\n git format-patch --cover-letter --interdiff=<...> ...\n\nBoth options (--range-diff & --interdiff) also work for single-patch\nseries which lack a cover-letter.\n"},{"id":"491097","messageId":"CAPig+cQekOLrpcK_NcL951SRZAg-1Y1aFzE0-i-uCTK_XuiajQ@mail.gmail.com","threadId":"61168","inReplyTo":"688128d8ef09589712634888074ffd71a192a7aa.1710994548.git.dsimic@manjaro.org","subject":"Re: [PATCH v4 3/4] t1300: add more tests for whitespace and inline comments","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-03-21T04:55:20Z","receivedAt":"2024-03-21T04:55:32Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 21, 2024 at 12:17 AM Dragan Simic wrote:\n> Add a handful of additional tests, to improve the coverage of the handling\n> of configuration file entries whose values contain internal whitespace,\n> leading and/or trailing whitespace, which may or may not be enclosed within\n> quotation marks, or which contain an additional inline comment.\n>\n> At the same time, rework one already existing whitespace-related test a bit,\n> to ensure its consistency with the newly added tests. This change introduced\n> no functional changes to the already existing test.\n>\n> Helped-by: Eric Sunshine \n> Helped-by: Junio C Hamano \n> Signed-off-by: Dragan Simic \n> ---\n> diff --git a/t/t1300-config.sh b/t/t1300-config.sh\n> @@ -11,6 +11,98 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> +test_expect_success 'setup whitespace config' '\n> + sed -e \"s/^|//\" \\\n> + -e \"s/[$]$//\" \\\n> + -e \"s/X/\\\\t/g\" >.git/config <<-\\EOF\n\nRepresenting TAB as `\\t` in the sed replacement is not necessarily\nportable and (as far as I can see) we don't rely upon that in any\nexisting tests. It probably would be safer to employ a literal TAB\ncharacter in `s/X/ /g` as Junio showed in his example[1].\n\n[1]: https://lore.kernel.org/git/xmqqzfutjtfo.fsf@gitster.g/\n"},{"id":"491098","messageId":"2f5f35d6171209cd1c9d5ea1a1d3cb92@manjaro.org","threadId":"61168","inReplyTo":"CAPig+cRCCqRHYOpMxdL4MWaSF5S0Qb7fcJ5HqkT0j_MU1dRvfQ@mail.gmail.com","subject":"Re: [PATCH v4 0/4] Fix a bug in configuration parsing, and improve tests and documentation","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-21T04:58:43Z","receivedAt":"2024-03-21T04:58:46Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-03-21 05:50, Eric Sunshine wrote:\n> On Thu, Mar 21, 2024 at 12:17 AM Dragan Simic \n> wrote:\n>> In v2, this series had five patches in total, out of which the third \n>> patch\n>> (i.e. patch 3/5) was dropped in v3. [4] Other changes in each version \n>> are\n>> described in each patch.\n> \n> In addition to a written description of the changes since the previous\n> version, reviewers also greatly appreciate seeing a range-diff which\n> you can generate automatically in the cover-letter via:\n> \n> git format-patch --cover-letter --range-diff=<...> ...\n> \n> or less frequently, an interdiff via:\n> \n> git format-patch --cover-letter --interdiff=<...> ...\n> \n> Both options (--range-diff & --interdiff) also work for single-patch\n> series which lack a cover-letter.\n\nI'll keep that in mind for future patches.\n"},{"id":"491099","messageId":"4c1d28fddc0e7277154cff51c0f0fde1@manjaro.org","threadId":"61168","inReplyTo":"CAPig+cQekOLrpcK_NcL951SRZAg-1Y1aFzE0-i-uCTK_XuiajQ@mail.gmail.com","subject":"Re: [PATCH v4 3/4] t1300: add more tests for whitespace and inline comments","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-21T05:01:37Z","receivedAt":"2024-03-21T05:01:39Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-03-21 05:55, Eric Sunshine wrote:\n> On Thu, Mar 21, 2024 at 12:17 AM Dragan Simic \n> wrote:\n>> Add a handful of additional tests, to improve the coverage of the \n>> handling\n>> of configuration file entries whose values contain internal \n>> whitespace,\n>> leading and/or trailing whitespace, which may or may not be enclosed \n>> within\n>> quotation marks, or which contain an additional inline comment.\n>> \n>> At the same time, rework one already existing whitespace-related test \n>> a bit,\n>> to ensure its consistency with the newly added tests. This change \n>> introduced\n>> no functional changes to the already existing test.\n>> \n>> Helped-by: Eric Sunshine \n>> Helped-by: Junio C Hamano \n>> Signed-off-by: Dragan Simic \n>> ---\n>> diff --git a/t/t1300-config.sh b/t/t1300-config.sh\n>> @@ -11,6 +11,98 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n>> +test_expect_success 'setup whitespace config' '\n>> + sed -e \"s/^|//\" \\\n>> + -e \"s/[$]$//\" \\\n>> + -e \"s/X/\\\\t/g\" >.git/config <<-\\EOF\n> \n> Representing TAB as `\\t` in the sed replacement is not necessarily\n> portable and (as far as I can see) we don't rely upon that in any\n> existing tests. It probably would be safer to employ a literal TAB\n> character in `s/X/ /g` as Junio showed in his example[1].\n\nHmm, I actually saw using a literal HT character as a disadvantage.\nThanks for the notice, I'll send the v5 with this fixed.\n\n> [1]: https://lore.kernel.org/git/xmqqzfutjtfo.fsf@gitster.g/\n"},{"id":"491100","messageId":"CAPig+cSYhYBa0NsvJCOYo4JsWzLJT9rU++U1QKA3jRB6Cptbhw@mail.gmail.com","threadId":"61168","inReplyTo":"f84c5e8e4a90be3f9fe3cc853e0d40aed4e58826.1710994548.git.dsimic@manjaro.org","subject":"Re: [PATCH v4 4/4] config.txt: describe handling of whitespace further","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-03-21T05:11:26Z","receivedAt":"2024-03-21T05:11:38Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 21, 2024 at 12:17 AM Dragan Simic wrote:\n> Make it more clear what the whitespace characters are in the context of git\n> configuration files, and significantly improve the description of the leading\n> and trailing whitespace handling, especially how it works out together with\n> the presence of inline comments.\n>\n> Helped-by: Junio C Hamano \n> Signed-off-by: Dragan Simic \n> ---\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> @@ -63,13 +64,15 @@ the variable is the boolean \"true\").\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 before 'name =' are discarded.\n> +The portion of the line after the first comment character, including\n> +the comment character itself, is discarded. Unless enclosed in double\n> +quotation marks (`\"`), any leading or trailing whitespace characters\n> +surrounding 'value' are discarded. Internal whitespace characters\n> +within 'value' are retained verbatim.\n\nI find this statement confusing and ambiguous:\n\n Unless enclosed in double quotation marks (`\"`), any leading or\n trailing whitespace characters surrounding 'value' are discarded.\n\nsince it might imply that the shown and whitespace is\nretained outside the quotes, as well:\n\n key =\" string \"\n\nIt should be possible to rephrase it to be more definite, while\ndropping the final sentence altogether. Perhaps:\n\n Whitespace surrounding `name`, `=` and `value` is ignored. If\n `value` is surrounding by double quotation marks (`\"`), all\n characters within the quoted string are retained verbatim,\n including whitespace. Comments starting with either `#` or `;` and\n extending to the end of line are discarded. A line that defines a\n value can be continued to the next line by ending it with a `\\`;\n the backslash and the end-of-line are stripped.\n"},{"id":"491101","messageId":"9a5217f7f7a35768a3eb45060fb3e4f4@manjaro.org","threadId":"61168","inReplyTo":"CAPig+cSYhYBa0NsvJCOYo4JsWzLJT9rU++U1QKA3jRB6Cptbhw@mail.gmail.com","subject":"Re: [PATCH v4 4/4] config.txt: describe handling of whitespace further","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-21T05:16:01Z","receivedAt":"2024-03-21T05:16:03Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-03-21 06:11, Eric Sunshine wrote:\n> On Thu, Mar 21, 2024 at 12:17 AM Dragan Simic \n> wrote:\n>> Make it more clear what the whitespace characters are in the context \n>> of git\n>> configuration files, and significantly improve the description of the \n>> leading\n>> and trailing whitespace handling, especially how it works out together \n>> with\n>> the presence of inline comments.\n>> \n>> Helped-by: Junio C Hamano \n>> Signed-off-by: Dragan Simic \n>> ---\n>> diff --git a/Documentation/config.txt b/Documentation/config.txt\n>> @@ -63,13 +64,15 @@ the variable is the boolean \"true\").\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 before 'name =' are discarded.\n>> +The portion of the line after the first comment character, including\n>> +the comment character itself, is discarded. Unless enclosed in \n>> double\n>> +quotation marks (`\"`), any leading or trailing whitespace characters\n>> +surrounding 'value' are discarded. Internal whitespace characters\n>> +within 'value' are retained verbatim.\n> \n> I find this statement confusing and ambiguous:\n> \n> Unless enclosed in double quotation marks (`\"`), any leading or\n> trailing whitespace characters surrounding 'value' are discarded.\n> \n> since it might imply that the shown and whitespace is\n> retained outside the quotes, as well:\n> \n> key =\" string \"\n> \n> It should be possible to rephrase it to be more definite, while\n> dropping the final sentence altogether. Perhaps:\n> \n> Whitespace surrounding `name`, `=` and `value` is ignored. If\n> `value` is surrounding by double quotation marks (`\"`), all\n> characters within the quoted string are retained verbatim,\n> including whitespace. Comments starting with either `#` or `;` and\n> extending to the end of line are discarded. A line that defines a\n> value can be continued to the next line by ending it with a `\\`;\n> the backslash and the end-of-line are stripped.\n\nLooking good to me, thanks. I'll include it into the v5, with\na small grammar issue fixed.\n"},{"id":"491102","messageId":"CAPig+cTgbjCg5jkidEufgHgpkPOZPhfSbd_gbBxOFK=B8mX+fw@mail.gmail.com","threadId":"61168","inReplyTo":"9a5217f7f7a35768a3eb45060fb3e4f4@manjaro.org","subject":"Re: [PATCH v4 4/4] config.txt: describe handling of whitespace further","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-03-21T05:21:22Z","receivedAt":"2024-03-21T05:21:33Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 21, 2024 at 1:16 AM Dragan Simic wrote:\n> On 2024-03-21 06:11, Eric Sunshine wrote:\n> > It should be possible to rephrase it to be more definite, while\n> > dropping the final sentence altogether. Perhaps:\n> >\n> > Whitespace surrounding `name`, `=` and `value` is ignored. If\n> > `value` is surrounding by double quotation marks (`\"`), all\n> > characters within the quoted string are retained verbatim,\n> > including whitespace. Comments starting with either `#` or `;` and\n> > extending to the end of line are discarded. A line that defines a\n> > value can be continued to the next line by ending it with a `\\`;\n> > the backslash and the end-of-line are stripped.\n>\n> Looking good to me, thanks. I'll include it into the v5, with\n> a small grammar issue fixed.\n\nFor completeness, I should mention that I intentionally reordered the\ntopics so that the most common/important ones are mentioned earlier\nrather than later; i.e. (1) surrounding whitespace ignored, (2)\ndouble-quoted value, (3) comments, (4) `\\` line-splicing with.\n"},{"id":"491103","messageId":"fdaec126df16bf6fe1c1fca9698f7dcc@manjaro.org","threadId":"61168","inReplyTo":"CAPig+cTgbjCg5jkidEufgHgpkPOZPhfSbd_gbBxOFK=B8mX+fw@mail.gmail.com","subject":"Re: [PATCH v4 4/4] config.txt: describe handling of whitespace further","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-21T05:31:06Z","receivedAt":"2024-03-21T05:31:08Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-03-21 06:21, Eric Sunshine wrote:\n> On Thu, Mar 21, 2024 at 1:16 AM Dragan Simic \n> wrote:\n>> On 2024-03-21 06:11, Eric Sunshine wrote:\n>> > It should be possible to rephrase it to be more definite, while\n>> > dropping the final sentence altogether. Perhaps:\n>> >\n>> > Whitespace surrounding `name`, `=` and `value` is ignored. If\n>> > `value` is surrounding by double quotation marks (`\"`), all\n>> > characters within the quoted string are retained verbatim,\n>> > including whitespace. Comments starting with either `#` or `;` and\n>> > extending to the end of line are discarded. A line that defines a\n>> > value can be continued to the next line by ending it with a `\\`;\n>> > the backslash and the end-of-line are stripped.\n>> \n>> Looking good to me, thanks. I'll include it into the v5, with\n>> a small grammar issue fixed.\n> \n> For completeness, I should mention that I intentionally reordered the\n> topics so that the most common/important ones are mentioned earlier\n> rather than later; i.e. (1) surrounding whitespace ignored, (2)\n> double-quoted value, (3) comments, (4) `\\` line-splicing with.\n\nHmm, I just noticed that your proposed description actually contains\nsome issues, e.g. it implies that the value-internal whitespace is\nretained verbatim only if the entire value is enclosed in double \nquotation\nmarks. I'll try to reword it, so this is fixed.\n"},{"id":"491111","messageId":"xmqq8r2cca6k.fsf@gitster.g","threadId":"61168","inReplyTo":"CAPig+cSYhYBa0NsvJCOYo4JsWzLJT9rU++U1QKA3jRB6Cptbhw@mail.gmail.com","subject":"Re: [PATCH v4 4/4] config.txt: describe handling of whitespace further","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-21T07:32:03Z","receivedAt":"2024-03-21T07:32:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> Whitespace surrounding `name`, `=` and `value` is ignored. If\n> `value` is surrounding by double quotation marks (`\"`), all\n> characters within the quoted string are retained verbatim,\n> including whitespace. Comments starting with either `#` or `;` and\n> extending to the end of line are discarded. A line that defines a\n> value can be continued to the next line by ending it with a `\\`;\n> the backslash and the end-of-line are stripped.\n\nNice, but I am not sure how this captures how whitespaces between\nvalue and comment are handled, e.g., in this line\n\n\t| name = value # comment$\n\nhumans know the space before '#' is removed because it is\n\"whitespace surrounding value\". But there is a bit of chicken and\negg problem; before you realize '# comment' is a comment and strip\nit from the line, you do not know where value ends, so your reading\nof the above need to backtrack.\n"}]}