{"thread":{"id":"61127","subject":"[PATCH 1/4] config: minor addition of whitespace","startedAt":"2024-03-15T13:22:18Z","lastAt":"2024-03-15T21:42:14Z","messageCount":11,"participants":["Dragan Simic","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"490683","messageId":"33ae9976fc089fe8f59aa857373319183bf558b3.1710508691.git.dsimic@manjaro.org","threadId":"61127","inReplyTo":"cover.1710508691.git.dsimic@manjaro.org","subject":"[PATCH 1/4] config: minor addition of whitespace","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-15T13:22:07Z","receivedAt":"2024-03-15T13:22:18Z","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 characters. Thus, clean up one spotted expression that for\nsome reason had a \"bunched up\" operator.\n\nSigned-off-by: Dragan Simic \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":"490684","messageId":"590731e15a01558d1bbcdfc01df4f78573138742.1710508691.git.dsimic@manjaro.org","threadId":"61127","inReplyTo":"cover.1710508691.git.dsimic@manjaro.org","subject":"[PATCH 3/4] t1300: add more tests for whitespace and inline comments","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-15T13:22:09Z","receivedAt":"2024-03-15T13:22:18Z","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, or which contain an additional inline comment.\n\nSigned-off-by: Dragan Simic \n---\n t/t1300-config.sh | 102 +++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 101 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex 31c387868708..6eef8a48098c 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+cat > .git/config << EOF\n+[section]\n+\tsolid = rock\n+\tsparse = big \t\t blue\n+\tsparseAndTail = big \t\t blue \n+\tsparseAndTailQuoted = \"big \t\t blue \"\n+\tsparseAndBiggerTail = big \t\t blue \t \t\n+\tsparseAndBiggerTailQuoted = \"big \t\t blue \t \t\"\n+\tsparseAndBiggerTailQuotedPlus = \"big \t\t blue \t \t\"\t \n+\theadAndTail = \tbig blue \n+\theadAndTailQuoted = \"\tbig blue \"\n+\theadAndTailQuotedPlus = \"\tbig blue \" \n+\tannotated = big blue\t# to be discarded\n+\tannotatedQuoted = \"big blue\"\t# to be discarded\n+EOF\n+\n+echo 'rock' > expect\n+\n+test_expect_success 'no internal whitespace' '\n+\tgit config --get section.solid > output &&\n+\ttest_cmp expect output\n+'\n+\n+echo 'big \t\t blue' > expect\n+\n+test_expect_success 'internal whitespace' '\n+\tgit config --get section.sparse > output &&\n+\ttest_cmp expect output\n+'\n+\n+test_expect_success 'internal and trailing whitespace' '\n+\tgit config --get section.sparseAndTail > output &&\n+\ttest_cmp expect output\n+'\n+\n+test_expect_success 'internal and more trailing whitespace' '\n+\tgit config --get section.sparseAndBiggerTail > output &&\n+\ttest_cmp expect output\n+'\n+\n+echo 'big \t\t blue ' > expect\n+\n+test_expect_success 'internal and trailing whitespace, all quoted' '\n+\tgit config --get section.sparseAndTailQuoted > output &&\n+\ttest_cmp expect output\n+'\n+\n+echo 'big \t\t blue \t \t' > expect\n+\n+test_expect_success 'internal and more trailing whitespace, all quoted' '\n+\tgit config --get section.sparseAndBiggerTailQuoted > output &&\n+\ttest_cmp expect output\n+'\n+\n+test_expect_success 'internal and more trailing whitespace, not all quoted' '\n+\tgit config --get section.sparseAndBiggerTailQuotedPlus > output &&\n+\ttest_cmp expect output\n+'\n+\n+echo 'big blue' > expect\n+\n+test_expect_success 'leading and trailing whitespace' '\n+\tgit config --get section.headAndTail > output &&\n+\ttest_cmp expect output\n+'\n+\n+echo '\tbig blue ' > expect\n+\n+test_expect_success 'leading and trailing whitespace, all quoted' '\n+\tgit config --get section.headAndTailQuoted > output &&\n+\ttest_cmp expect output\n+'\n+\n+test_expect_success 'leading and trailing whitespace, not all quoted' '\n+\tgit config --get section.headAndTailQuotedPlus > output &&\n+\ttest_cmp expect output\n+'\n+\n+echo 'big blue' > expect\n+\n+test_expect_success 'inline comment' '\n+\tgit config --get section.annotated > output &&\n+\ttest_cmp expect output\n+'\n+\n+test_expect_success 'inline comment, quoted' '\n+\tgit config --get section.annotatedQuoted > output &&\n+\ttest_cmp expect output\n+'\n+\n test_expect_success 'clear default config' '\n \trm -f .git/config\n '\n@@ -1066,7 +1156,17 @@ test_expect_success '--null --get-regexp' '\n \ttest_cmp expect result\n '\n \n-test_expect_success 'inner whitespace kept verbatim' '\n+test_expect_success 'inner whitespace kept verbatim, spaces only' '\n+\tgit config section.val \"foo bar\" &&\n+\ttest_cmp_config \"foo bar\" section.val\n+'\n+\n+test_expect_success 'inner whitespace kept verbatim, horizontal tabs only' '\n+\tgit config section.val \"foo\t\tbar\" &&\n+\ttest_cmp_config \"foo\t\tbar\" section.val\n+'\n+\n+test_expect_success 'inner whitespace kept verbatim, horizontal tabs and spaces' '\n \tgit config section.val \"foo \t bar\" &&\n \ttest_cmp_config \"foo \t bar\" section.val\n '\n"},{"id":"490685","messageId":"cover.1710508691.git.dsimic@manjaro.org","threadId":"61127","inReplyTo":null,"subject":"[PATCH 0/4] Fix a bug in configuration parsing, and improve tests and documentation","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-15T13:22:06Z","receivedAt":"2024-03-15T13:22:18Z","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\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\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 | 102 ++++++++++++++++++++++++++++++++++++++-\n 3 files changed, 122 insertions(+), 14 deletions(-)\n\n"},{"id":"490686","messageId":"cd890a7015733e311e0e0a4939c539d2894e31cf.1710508691.git.dsimic@manjaro.org","threadId":"61127","inReplyTo":"cover.1710508691.git.dsimic@manjaro.org","subject":"[PATCH 2/4] config: really keep value-internal whitespace verbatim","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-15T13:22:08Z","receivedAt":"2024-03-15T13:22:18Z","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\nFixes: ebdaae372b46 (\"config: Keep inner whitespace verbatim\")\nSigned-off-by: Dragan Simic \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":"490687","messageId":"d310d8c28baaf9b938f7ebbbf534997364c6f6bc.1710508691.git.dsimic@manjaro.org","threadId":"61127","inReplyTo":"cover.1710508691.git.dsimic@manjaro.org","subject":"[PATCH 4/4] config.txt: describe handling of whitespace further","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-15T13:22:10Z","receivedAt":"2024-03-15T13:22:18Z","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.\n\nSigned-off-by: Dragan Simic \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":"490702","messageId":"xmqq4jd7qtg6.fsf@gitster.g","threadId":"61127","inReplyTo":"cd890a7015733e311e0e0a4939c539d2894e31cf.1710508691.git.dsimic@manjaro.org","subject":"Re: [PATCH 2/4] config: really keep value-internal whitespace verbatim","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-15T17:46:17Z","receivedAt":"2024-03-15T17:46:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dragan Simic writes:\n\n> Fix a bug in function parse_value() that prevented whitespace characters\n> (i.e. spaces and horizontal tabs) found inside configuration option values\n> from being parsed and returned in their original form. The bug caused any\n> number of consecutive whitespace characters to be wrongly \"squashed\" into\n> the same number of space characters.\n>\n> This bug was introduced back in July 2009, in commit ebdaae372b46 (\"config:\n> Keep inner whitespace verbatim\").\n>\n> Further investigation showed that setting a configuration value, by invoking\n> git-config(1), converts value-internal horizontal tabs into \"\\t\" escape\n> sequences, which the buggy value-parsing logic in function parse_value()\n> didn't \"squash\" into spaces. That's why the test included in the ebdaae37\n> commit passed, which presumably made the bug remain undetected for this long.\n> On the other hand, value-internal literal horizontal tab characters, found in\n> a configuration file edited by hand, do get \"squashed\" by the value-parsing\n> logic, so the right choice was to fix this bug by making the value-internal\n> whitespace characters preserved verbatim.\n\nOK.\n\n> Fixes: ebdaae372b46 (\"config: Keep inner whitespace verbatim\")\n\nNo need for this line. You mentioned it in the text already, and\nmore importantly, grepping for trailers is not the right thing to do\nbecause we may think we fixed all bugs in ebdaae372b46 did with this\npatch, which may in 6 months turn out to be false but we cannot undo\nthe trailer. On the other hand, the discussion of the problem in\nthe proposed log message gives readers a richer context and the\nfuture developers can understand that this fixed one thing in\nebdaae372b46 but didn't even know about other bugs introduced by\nthat old commit and make more intelligent decision based on that\nbetter understanding.\n\n> Signed-off-by: Dragan Simic \n> ---\n> config.c | 13 +++++++++----\n> 1 file changed, 9 insertions(+), 4 deletions(-)\n>\n> diff --git a/config.c b/config.c\n> index 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\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\nSo the idea is that we copy everything we read verbatim in cs->value\nbut keep track of the beginning of the run of whitespace characters\nat the end of the line in trim_len so that we can rtrim it? That\nshould be the most straight-forward implementation.\n\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\tstrbuf_addch(&cs->value, c);\n> \t\t\tcontinue;\n\nWhile we are not inside a dq-pair, we keep ignoring a whitespace\ncharacter at the beginning (i.e. cs->value.len == 0).\n\nIf we have some value in cs->value, however, we add the whitespace\ncharacter to cs->value verbatim. trim_len==0 signals that the last\ncharacter we processed was not a whitespace character, and we copy\nthe current length of cs->value there---this is so that we can rtrim\naway the run of whitespaces at the end of the input, including the\nbyte we are adding here, if it turns out that we are looking at the\nfirst whitespace character of such trailing blanks.\n\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\n> +\t\tif (trim_len)\n> +\t\t\ttrim_len = 0;\n\nIf we are outside a dq-pair, we reach here only when we are looking\nat a non-whitespace character. If we are counting a run of unquoted\nwhitespaces, we can reset trim_len here to record that we do not\nhave to trim.\n\nBut can we be seeing a whitespace that is inside quote here? Is\nresetting trim_len to zero what we want in such a case? Let's see.\nInside dq, we'd want to accumulate bytes, possibly after unwrapping\ntheir backslash escapes, to cs->value, and these bytes include the\nwhitespace characters---we want to keep them literally without\ntrimming. OK.\n\nLooking good. We should already demonstrate that this works well\nwith a new test in the same patch, can't we? If we can, we should.\n\nThanks.\n\n> \t\tif (c == '\\\\') {\n> \t\t\tc = get_next_char(cs);\n> \t\t\tswitch (c) {\n"},{"id":"490708","messageId":"CAPig+cRMPNExbG34xJ0w5npUc3DDwxQUGS_AQfam_mi4s53=sA@mail.gmail.com","threadId":"61127","inReplyTo":"590731e15a01558d1bbcdfc01df4f78573138742.1710508691.git.dsimic@manjaro.org","subject":"Re: [PATCH 3/4] t1300: add more tests for whitespace and inline comments","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-03-15T19:39:38Z","receivedAt":"2024-03-15T19:39:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 15, 2024 at 9:22 AM Dragan Simic wrote:\n> Add a handful of additional automated tests, to improve the coverage of\n> configuration file entries whose values contain internal whitespace, leading\n> and/or trailing whitespace, or which contain an additional inline comment.\n>\n> Signed-off-by: Dragan Simic \n\nJust some minor style-related comments...\n\n> diff --git a/t/t1300-config.sh b/t/t1300-config.sh\n> @@ -11,6 +11,96 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> +cat > .git/config << EOF\n> +[section]\n> + solid = rock\n> + sparse = big blue\n> + sparseAndTail = big blue\n> + sparseAndTailQuoted = \"big blue \"\n> + sparseAndBiggerTail = big blue\n> + sparseAndBiggerTailQuoted = \"big blue \"\n> + sparseAndBiggerTailQuotedPlus = \"big blue \"\n> + headAndTail = big blue\n> + headAndTailQuoted = \" big blue \"\n> + headAndTailQuotedPlus = \" big blue \"\n> + annotated = big blue # to be discarded\n> + annotatedQuoted = \"big blue\" # to be discarded\n> +EOF\n\nThese days we try to place all test-related code inside a\ntest_expect_success() context rather than having it standalone. In\nthis case, since the file being created is (presumably) shared by\nmultiple tests in this script, you may want to add a new test which\nperforms this setup step.\n\nUse \\EOF rather than EOF to signal to readers that we don't expect any\nvariable interpolation to happen within the here-doc body.\n\nFurther, use -\\EOF inside test_expect_success() to allow us to indent\nthe body of the heredoc to match the test indentation.\n\nStyle guideline says to omit whitespace after redirection operators\n(such as `<<` and `>`).\n\nWe have a q_to_tab() function which lets us state explicitly for\nreaders the location of TAB characters in the heredoc body. You'll\noften see that used instead of literal TABs.\n\nTaking all the above into account, perhaps:\n\n test_expect_success 'setup whitespace' '\n q_to_tab >.git/config <<-\\EOF\n [section]\n solid = rock\n sparse = bigQblue\n ...\n EOF\n\nSame comments apply to rest of patch.\n"},{"id":"490732","messageId":"1b6cfdd62b08b7bdc817e3c106e3ef50@manjaro.org","threadId":"61127","inReplyTo":"xmqq4jd7qtg6.fsf@gitster.g","subject":"Re: [PATCH 2/4] config: really keep value-internal whitespace verbatim","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-15T19:50:45Z","receivedAt":"2024-03-15T19:50:48Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-03-15 18:46, Junio C Hamano wrote:\n> Dragan Simic writes:\n> \n>> Fixes: ebdaae372b46 (\"config: Keep inner whitespace verbatim\")\n> \n> No need for this line. You mentioned it in the text already, and\n> more importantly, grepping for trailers is not the right thing to do\n> because we may think we fixed all bugs in ebdaae372b46 did with this\n> patch, which may in 6 months turn out to be false but we cannot undo\n> the trailer. On the other hand, the discussion of the problem in\n> the proposed log message gives readers a richer context and the\n> future developers can understand that this fixed one thing in\n> ebdaae372b46 but didn't even know about other bugs introduced by\n> that old commit and make more intelligent decision based on that\n> better understanding.\n\nGot it, and I agree that the discussions linked in the patch description\nprovide much better ways for understanding what brought us to the \ncurrent\nsituation, and what led to the proposed bugfixes.\n\nIt's simply that I'm kind of used to providing \"Fixes\" tags for patches\nin other projects; I'll make a mental note not to do that for git.\n\nI'll also send the v2 with no \"Fixes\" tag, after the v1 spends a few \ndays\non the mailing list.\n\n>> Signed-off-by: Dragan Simic \n>> ---\n>> config.c | 13 +++++++++----\n>> 1 file changed, 9 insertions(+), 4 deletions(-)\n>> \n>> diff --git a/config.c b/config.c\n>> index a86a20cdf5cb..5072f12e62e4 100644\n>> --- a/config.c\n>> +++ b/config.c\n>> @@ -817,33 +817,38 @@ static int get_next_char(struct config_source \n>> *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> \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> \n> So the idea is that we copy everything we read verbatim in cs->value\n> but keep track of the beginning of the run of whitespace characters\n> at the end of the line in trim_len so that we can rtrim it? That\n> should be the most straight-forward implementation.\n\nYes, that's it. It's as simple as it can be. A different, somewhat\nmore complex approach of putting the encountered whitespace in a \nseparate\nbuffer may be warranted if we expected _a lot_ of whitespace, i.e. much\nmore whitespace than the other characters, but that shouldn't be the \ncase,\nwhich makes the simple approach a good choice.\n\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\tstrbuf_addch(&cs->value, c);\n>> \t\t\tcontinue;\n> \n> While we are not inside a dq-pair, we keep ignoring a whitespace\n> character at the beginning (i.e. cs->value.len == 0).\n\nYes, that \"eats up\" any leading whitespace.\n\n> If we have some value in cs->value, however, we add the whitespace\n> character to cs->value verbatim. trim_len==0 signals that the last\n> character we processed was not a whitespace character, and we copy\n> the current length of cs->value there---this is so that we can rtrim\n> away the run of whitespaces at the end of the input, including the\n> byte we are adding here, if it turns out that we are looking at the\n> first whitespace character of such trailing blanks.\n\nExactly. By the way, please know that I _really_ appreciate your\nhighly detailed patch reviews!\n\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> \n>> +\t\tif (trim_len)\n>> +\t\t\ttrim_len = 0;\n> \n> If we are outside a dq-pair, we reach here only when we are looking\n> at a non-whitespace character. If we are counting a run of unquoted\n> whitespaces, we can reset trim_len here to record that we do not\n> have to trim.\n\nExactly. It resets the \"need to rtrim flag\", so to speak.\n\n> But can we be seeing a whitespace that is inside quote here? Is\n> resetting trim_len to zero what we want in such a case? Let's see.\n> Inside dq, we'd want to accumulate bytes, possibly after unwrapping\n> their backslash escapes, to cs->value, and these bytes include the\n> whitespace characters---we want to keep them literally without\n> trimming. OK.\n\nYes, any whitespace inside a quoted option value is to be taken\nverbatim, so we basically reset the \"need to rtrim flag\" in that case,\nbecause value-internal whitespace isn't to be rtrimmed.\n\n> Looking good. We should already demonstrate that this works well\n> with a new test in the same patch, can't we? If we can, we should.\n\nSorry, this sounds a bit confusing to me? I think there are already\nmore than enough new tests in the 3/4 patch I sent?\n\nCould you, please, clarify a bit?\n\n>> \t\tif (c == '\\\\') {\n>> \t\t\tc = get_next_char(cs);\n>> \t\t\tswitch (c) {\n"},{"id":"490733","messageId":"eb550eebb158064f1780f610669bbda1@manjaro.org","threadId":"61127","inReplyTo":"CAPig+cRMPNExbG34xJ0w5npUc3DDwxQUGS_AQfam_mi4s53=sA@mail.gmail.com","subject":"Re: [PATCH 3/4] t1300: add more tests for whitespace and inline comments","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-15T20:04:12Z","receivedAt":"2024-03-15T20:04:14Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"Hello Eric,\n\nOn 2024-03-15 20:39, Eric Sunshine wrote:\n> On Fri, Mar 15, 2024 at 9:22 AM Dragan Simic \n> wrote:\n>> Add a handful of additional automated tests, to improve the coverage \n>> of\n>> configuration file entries whose values contain internal whitespace, \n>> leading\n>> and/or trailing whitespace, or which contain an additional inline \n>> comment.\n>> \n>> Signed-off-by: Dragan Simic \n> \n> Just some minor style-related comments...\n> \n>> diff --git a/t/t1300-config.sh b/t/t1300-config.sh\n>> @@ -11,6 +11,96 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n>> +cat > .git/config << EOF\n>> +[section]\n>> + solid = rock\n>> + sparse = big blue\n>> + sparseAndTail = big blue\n>> + sparseAndTailQuoted = \"big blue \"\n>> + sparseAndBiggerTail = big blue\n>> + sparseAndBiggerTailQuoted = \"big blue \n>> \"\n>> + sparseAndBiggerTailQuotedPlus = \"big blue \n>> \"\n>> + headAndTail = big blue\n>> + headAndTailQuoted = \" big blue \"\n>> + headAndTailQuotedPlus = \" big blue \"\n>> + annotated = big blue # to be discarded\n>> + annotatedQuoted = \"big blue\" # to be discarded\n>> +EOF\n> \n> These days we try to place all test-related code inside a\n> test_expect_success() context rather than having it standalone. In\n> this case, since the file being created is (presumably) shared by\n> multiple tests in this script, you may want to add a new test which\n> performs this setup step.\n> \n> Use \\EOF rather than EOF to signal to readers that we don't expect any\n> variable interpolation to happen within the here-doc body.\n> \n> Further, use -\\EOF inside test_expect_success() to allow us to indent\n> the body of the heredoc to match the test indentation.\n> \n> Style guideline says to omit whitespace after redirection operators\n> (such as `<<` and `>`).\n> \n> We have a q_to_tab() function which lets us state explicitly for\n> readers the location of TAB characters in the heredoc body. You'll\n> often see that used instead of literal TABs.\n> \n> Taking all the above into account, perhaps:\n> \n> test_expect_success 'setup whitespace' '\n> q_to_tab >.git/config <<-\\EOF\n> [section]\n> solid = rock\n> sparse = bigQblue\n> ...\n> EOF\n> \n> Same comments apply to rest of patch.\n\nThank you for your review and all suggestions! I'll make sure to rework\nthe tests in v2, in the way you described it above.\n\nI'll come back with any questions I might have while reworking the new\ntests. I hope that's fine.\n"},{"id":"490735","messageId":"CAPig+cRG8eFxepkaiN54H+fa7D=rFGsmEHdvTP+HSSaLO_6T_A@mail.gmail.com","threadId":"61127","inReplyTo":"CAPig+cRMPNExbG34xJ0w5npUc3DDwxQUGS_AQfam_mi4s53=sA@mail.gmail.com","subject":"Re: [PATCH 3/4] t1300: add more tests for whitespace and inline comments","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-03-15T20:29:34Z","receivedAt":"2024-03-15T20:29:47Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 15, 2024 at 3:39 PM Eric Sunshine wrote:\n> These days we try to place all test-related code inside a\n> test_expect_success() context rather than having it standalone. In\n> this case, since the file being created is (presumably) shared by\n> multiple tests in this script, you may want to add a new test which\n> performs this setup step.\n>\n> Taking all the above into account, perhaps:\n>\n> test_expect_success 'setup whitespace' '\n> q_to_tab >.git/config <<-\\EOF\n> [section]\n> solid = rock\n> sparse = bigQblue\n> ...\n> EOF\n>\n> Same comments apply to rest of patch.\n\nTo be clear, this case is special because the file being created is\nshared by multiple tests, so it deserves being placed in its own\ntest_expect_success() invocation.\n\nFor the remaining cases where you're doing some set-up outside of\ntest_expect_success(), just move the set-up code into the\ncorresponding test_expect_success() invocation. For instance, rather\nthan:\n\n echo 'big blue' > expect\n\n test_expect_success 'internal whitespace' '\n git config --get section.sparse > output &&\n test_cmp expect output\n '\n\ndo this:\n\n test_expect_success 'internal whitespace' '\n echo 'bigQblue' | q_to_tab >expect\n git config --get section.sparse >actual &&\n test_cmp expect actual\n '\n\n(I changed \"output\" to \"actual\" above since the names \"expect\" and\n\"actual\" are common in the tests.)\n"},{"id":"490738","messageId":"ac99ea0a24c257eab8f29466589b6eef@manjaro.org","threadId":"61127","inReplyTo":"CAPig+cRG8eFxepkaiN54H+fa7D=rFGsmEHdvTP+HSSaLO_6T_A@mail.gmail.com","subject":"Re: [PATCH 3/4] t1300: add more tests for whitespace and inline comments","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-15T21:42:11Z","receivedAt":"2024-03-15T21:42:14Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-03-15 21:29, Eric Sunshine wrote:\n> On Fri, Mar 15, 2024 at 3:39 PM Eric Sunshine \n> wrote:\n>> These days we try to place all test-related code inside a\n>> test_expect_success() context rather than having it standalone. In\n>> this case, since the file being created is (presumably) shared by\n>> multiple tests in this script, you may want to add a new test which\n>> performs this setup step.\n>> \n>> Taking all the above into account, perhaps:\n>> \n>> test_expect_success 'setup whitespace' '\n>> q_to_tab >.git/config <<-\\EOF\n>> [section]\n>> solid = rock\n>> sparse = bigQblue\n>> ...\n>> EOF\n>> \n>> Same comments apply to rest of patch.\n> \n> To be clear, this case is special because the file being created is\n> shared by multiple tests, so it deserves being placed in its own\n> test_expect_success() invocation.\n> \n> For the remaining cases where you're doing some set-up outside of\n> test_expect_success(), just move the set-up code into the\n> corresponding test_expect_success() invocation. For instance, rather\n> than:\n> \n> echo 'big blue' > expect\n> \n> test_expect_success 'internal whitespace' '\n> git config --get section.sparse > output &&\n> test_cmp expect output\n> '\n> \n> do this:\n> \n> test_expect_success 'internal whitespace' '\n> echo 'bigQblue' | q_to_tab >expect\n> git config --get section.sparse >actual &&\n> test_cmp expect actual\n> '\n> \n> (I changed \"output\" to \"actual\" above since the names \"expect\" and\n> \"actual\" are common in the tests.)\n\nThis looks nice, thanks again. It keeps the expected results and\nthe test execution in a single \"block\", making it a bit easier to\nkeep track of different tests and their expected results.\n"}]}