{"thread":{"id":"61099","subject":"[PATCH 2/3] config.txt: perform some minor reformatting","startedAt":"2024-03-12T15:56:02Z","lastAt":"2024-03-14T18:48:24Z","messageCount":14,"participants":["Dragan Simic","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"490489","messageId":"d50c0f22c41ec36b574e1ff67e68485d9a6f2a84.1710258538.git.dsimic@manjaro.org","threadId":"61099","inReplyTo":"cover.1710258538.git.dsimic@manjaro.org","subject":"[PATCH 2/3] config.txt: perform some minor reformatting","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-12T15:55:45Z","receivedAt":"2024-03-12T15:56:02Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"Reformat a few lines a bit, to utilize the available horizontal space better.\nThere are no changes to the actual contents of the documentation.\n\nSigned-off-by: Dragan Simic \n---\n Documentation/config.txt | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 4480bb44203b..2fc4a52d8d76 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -58,11 +58,11 @@ compared case sensitively. These subsection names follow the same\n restrictions as section names.\n \n All the other lines (and the remainder of the line after the section\n-header) are recognized as setting variables, in the form\n-'name = value' (or just 'name', which is a short-hand to say that\n-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+header) are recognized as setting variables, in the form 'name = value'\n+(or just 'name', which is a short-hand to say that the variable is the\n+boolean \"true\"). The variable names are case-insensitive, allow only\n+alphanumeric characters and `-`, and must start with an alphabetic\n+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 stripped.\n"},{"id":"490490","messageId":"1c670101fc29a9ccc71cf4d213545a564e14aa05.1710258538.git.dsimic@manjaro.org","threadId":"61099","inReplyTo":"cover.1710258538.git.dsimic@manjaro.org","subject":"[PATCH 1/3] config.txt: describe whitespace characters further and more accurately","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-12T15:55:44Z","receivedAt":"2024-03-12T15:56:02Z","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, improve the description of the trailing whitespace handling,\nand correct the description of the value-internal whitespace handling.\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..4480bb44203b 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. This discarding of the trailing whitespace characters\n+also applies after the remainder of the line after the comment character\n+is discarded. Any number of internal whitespace characters found within\n+the value are converted to the same number of space (SP) characters.\n \n Inside double quotes, double quote `\"` and backslash `\\` characters\n must be escaped: use `\\\"` for `\"` and `\\\\` for `\\`.\n"},{"id":"490491","messageId":"292ab942c38de44476c6c5bf045c375bf4f6a70d.1710258538.git.dsimic@manjaro.org","threadId":"61099","inReplyTo":"cover.1710258538.git.dsimic@manjaro.org","subject":"[PATCH 3/3] t1300: add tests for internal whitespace and inline comments","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-12T15:55:46Z","receivedAt":"2024-03-12T15:56:02Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"Add a couple of additional automated tests, to improve the coverage of\nconfiguration file entries whose values contain internal whitespace, or have\nan additional inline comment.\n\nSigned-off-by: Dragan Simic \n---\n t/t1300-config.sh | 20 ++++++++++++++++++++\n 1 file changed, 20 insertions(+)\n\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex 31c387868708..589af5e81d61 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -11,6 +11,26 @@ 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+\tsparse = big \t\t blue\n+\tannotated = big blue\t# to be discarded\n+EOF\n+\n+echo 'big blue' > expect\n+\n+test_expect_success 'internal whitespace' '\n+\tgit config --get section.sparse > 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 'clear default config' '\n \trm -f .git/config\n '\n"},{"id":"490492","messageId":"cover.1710258538.git.dsimic@manjaro.org","threadId":"61099","inReplyTo":null,"subject":"[PATCH 0/3] Improve the documentation and test coverage for whitespace and comments","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-12T15:55:43Z","receivedAt":"2024-03-12T15:56:02Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"Following up a recent discussion, [1] this small series addresses some\ninaccuracy spotted in the git-config(1) documentation, performs some more\nimprovements and small cleanups, and adds a couple of automated tests for\nimproved whitespace and comment coverage.\n\n[1] https://lore.kernel.org/git/pull.1681.v2.git.1709824540636.gitgitgadget@gmail.com/T/#m6372f6b5f2a26f7b1a6297241f5cbfa7a29f4fe5\n\nDragan Simic (3):\n config.txt: describe whitespace characters further and more accurately\n config.txt: perform some minor reformatting\n t1300: add tests for internal whitespace and inline comments\n\n Documentation/config.txt | 29 ++++++++++++++++-------------\n t/t1300-config.sh | 20 ++++++++++++++++++++\n 2 files changed, 36 insertions(+), 13 deletions(-)\n\n"},{"id":"490598","messageId":"xmqqjzm51ugt.fsf@gitster.g","threadId":"61099","inReplyTo":"1c670101fc29a9ccc71cf4d213545a564e14aa05.1710258538.git.dsimic@manjaro.org","subject":"Re: [PATCH 1/3] config.txt: describe whitespace characters further and more accurately","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-14T01:18:58Z","receivedAt":"2024-03-14T01:19:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dragan Simic writes:\n\n> Make it more clear what the whitespace characters are in the context of git\n> configuration, improve the description of the trailing whitespace handling,\n> and correct the description of the value-internal whitespace handling.\n>\n> Signed-off-by: Dragan Simic \n> ---\n> Documentation/config.txt | 19 +++++++++++--------\n> 1 file changed, 11 insertions(+), 8 deletions(-)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 782c2bab906c..4480bb44203b 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\nOK, except for \"whitespace characters\"---do we need to say\n\"whitespace characters\", after we already listed HT and SP are the\nones, instead of just \"whitespaces\"?\n\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 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. This discarding of the trailing whitespace characters\n> +also applies after the remainder of the line after the comment character\n> +is discarded.\n\n\"also\" makes it sound as if we do it twice, once to remove trailing\nwhitespaces after the remainder of the line after '#\", and then trim\nthe trailing whitespaces after we removed the comment.\n\nI wonder if we can make it clearer by following the step-by-step\nnature of the earlier part of the paragraph through. We already say\nthe folded line processing is done first, so break things down in\nconceptual phases/steps, perhaps like\n\n * The backslash at the end-of-line is removed, together with the\n end-of-line, to form a single long line.\n\n * Anything that come after the first unquoted comment character,\n either '#' or ';', are discarded.\n\n * The leading and trailing whitespaces around the value part\n (i.e. what follows 'name =') are discarded.\n\n * Remaining unquoted whitespaces inside the value part are munged.\n\n> +Any number of internal whitespace characters found within\n> +the value are converted to the same number of space (SP) characters.\n\nThe last one sounds like a bug to me, by the way.\n\nAt least the very original 17712991 (Add \".git/config\" file parser,\n2005-10-10) squashed a run of whitespace characters into a single\nSP, which makes sense as a \"clean-up\".\n\nBut ebdaae37 (config: Keep inner whitespace verbatim, 2009-07-30),\nwhile claiming to \"Keep\" inner whitespaces, broke it by replacing\nany isspace() bytes that are not SP with SP, contradicting its\nstated purpose.\n\nAs the latest change by the author of that change is from more than\n10 years ago, I do not expect that he is still interested in this\npart of the codebase, but thanks to a very clearly written log\nmessage, we can read what the motivation behind that change was, and\nseeing that what the code does contradicts with the stated\nmotivation we can safely declare that this is an ancient bug.\n\nFixing that bug can of course be left outside the series. For those\nwho are looking for microproject ideas who discovered this message\nby searching for the #leftoverbits keyword, one possible fix would\nbe to revert ebdaae37, make sure a value with any whitespace in it\ngets quoted, and document clearly that an unquoted run of\nwhitespaces is squashed into a single SP. Another way that is\nmilder is to finish what ebdaae37 wanted to do and retain the\nwhitespaces \"verbatim\".\n\nThanks.\n"},{"id":"490600","messageId":"xmqqedcd1sm5.fsf@gitster.g","threadId":"61099","inReplyTo":"d50c0f22c41ec36b574e1ff67e68485d9a6f2a84.1710258538.git.dsimic@manjaro.org","subject":"Re: [PATCH 2/3] config.txt: perform some minor reformatting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-14T01:58:58Z","receivedAt":"2024-03-14T01:59:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dragan Simic writes:\n\n> Reformat a few lines a bit, to utilize the available horizontal space better.\n> There are no changes to the actual contents of the documentation.\n\nI was a bit surprised to see such a \"preliminary clean-up\" step to\ncome before the main change, not after, but separating this from the\nchange to the next paragraph, which is the main change in this series,\nis nevertheless a very good idea.\n\n>\n> Signed-off-by: Dragan Simic \n> ---\n> Documentation/config.txt | 10 +++++-----\n> 1 file changed, 5 insertions(+), 5 deletions(-)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 4480bb44203b..2fc4a52d8d76 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -58,11 +58,11 @@ compared case sensitively. These subsection names follow the same\n> restrictions as section names.\n> \n> All the other lines (and the remainder of the line after the section\n> -header) are recognized as setting variables, in the form\n> -'name = value' (or just 'name', which is a short-hand to say that\n> -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> +header) are recognized as setting variables, in the form 'name = value'\n> +(or just 'name', which is a short-hand to say that the variable is the\n> +boolean \"true\"). The variable names are case-insensitive, allow only\n> +alphanumeric characters and `-`, and must start with an alphabetic\n> +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 stripped.\n"},{"id":"490601","messageId":"xmqq8r2l1rot.fsf@gitster.g","threadId":"61099","inReplyTo":"292ab942c38de44476c6c5bf045c375bf4f6a70d.1710258538.git.dsimic@manjaro.org","subject":"Re: [PATCH 3/3] t1300: add tests for internal whitespace and inline comments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-14T02:18:58Z","receivedAt":"2024-03-14T02:19:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dragan Simic writes:\n\n> Add a couple of additional automated tests, to improve the coverage of\n> configuration file entries whose values contain internal whitespace, or have\n> an additional inline comment.\n\nWhile this may document the current behaviour, I am not sure of the\nvalue of carving the current behaviour in stone, especially after\nchecking if the current behaviour is a bug.\n\n> Signed-off-by: Dragan Simic \n> ---\n> t/t1300-config.sh | 20 ++++++++++++++++++++\n> 1 file changed, 20 insertions(+)\n>\n> diff --git a/t/t1300-config.sh b/t/t1300-config.sh\n> index 31c387868708..589af5e81d61 100755\n> --- a/t/t1300-config.sh\n> +++ b/t/t1300-config.sh\n> @@ -11,6 +11,26 @@ 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> +\tsparse = big \t\t blue\n> +\tannotated = big blue\t# to be discarded\n> +EOF\n> +\n> +echo 'big blue' > expect\n> +\n> +test_expect_success 'internal whitespace' '\n> +\tgit config --get section.sparse > 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 'clear default config' '\n> \trm -f .git/config\n> '\n"},{"id":"490608","messageId":"ff7b0a2ead90ad9a9456141da5e4df4a@manjaro.org","threadId":"61099","inReplyTo":"xmqqjzm51ugt.fsf@gitster.g","subject":"Re: [PATCH 1/3] config.txt: describe whitespace characters further and more accurately","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-14T06:20:00Z","receivedAt":"2024-03-14T06:20:03Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-03-14 02:18, Junio C Hamano wrote:\n> Dragan Simic writes:\n>> -The syntax is fairly flexible and permissive; whitespaces are mostly\n>> -ignored. The '#' and ';' characters begin comments to the end of \n>> 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 \n>> begin\n>> +comments to the end of line. Blank lines are ignored.\n> \n> OK, except for \"whitespace characters\"---do we need to say\n> \"whitespace characters\", after we already listed HT and SP are the\n> ones, instead of just \"whitespaces\"?\n\nI also spent some time thinking about that. To me, the plural form, \ni.e.\n\"whitespaces\", simply doesn't sound very good, because \"whitespace\" \nfeels\nto me more like a mass noun, and I really haven't seen it used in plural\nform in other projects.\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. This discarding of the trailing whitespace \n>> characters\n>> +also applies after the remainder of the line after the comment \n>> character\n>> +is discarded.\n> \n> \"also\" makes it sound as if we do it twice, once to remove trailing\n> whitespaces after the remainder of the line after '#\", and then trim\n> the trailing whitespaces after we removed the comment.\n\nGood point, I also felt it the same way, but went with such wording \nsimply\nbecause I thought it should be more understandable to the users, despite\nbeing technically a bit incorrect.\n\n> I wonder if we can make it clearer by following the step-by-step\n> nature of the earlier part of the paragraph through. We already say\n> the folded line processing is done first, so break things down in\n> conceptual phases/steps, perhaps like\n> \n> * The backslash at the end-of-line is removed, together with the\n> end-of-line, to form a single long line.\n> \n> * Anything that come after the first unquoted comment character,\n> either '#' or ';', are discarded.\n> \n> * The leading and trailing whitespaces around the value part\n> (i.e. what follows 'name =') are discarded.\n> \n> * Remaining unquoted whitespaces inside the value part are munged.\n\nHmm, I'm not really sure that such a description would be more clear\nto the users, despite being technically more correct. I'll think a bit\nmore about it.\n\n>> +Any number of internal whitespace characters found within\n>> +the value are converted to the same number of space (SP) characters.\n> \n> The last one sounds like a bug to me, by the way.\n> \n> At least the very original 17712991 (Add \".git/config\" file parser,\n> 2005-10-10) squashed a run of whitespace characters into a single\n> SP, which makes sense as a \"clean-up\".\n> \n> But ebdaae37 (config: Keep inner whitespace verbatim, 2009-07-30),\n> while claiming to \"Keep\" inner whitespaces, broke it by replacing\n> any isspace() bytes that are not SP with SP, contradicting its\n> stated purpose.\n\nThank you for the investigation. The ebdaae37 commit certainly\nintroduced a bug to the value parsing, which presumably has remained\nundected because the included test passes.\n\nThe way I see it, fixing the bug may actually be a breaking change,\nbecause some user configurations may actually rely on the current\n(mis)behavior. This makes me somewhat afraid that fixing this bug,\nwhich I already thought about, may actually do more harm than good.\nHowever, fixing this bug seems to be only right thing to do, which\nI'll explain further below.\n\n> As the latest change by the author of that change is from more than\n> 10 years ago, I do not expect that he is still interested in this\n> part of the codebase, but thanks to a very clearly written log\n> message, we can read what the motivation behind that change was, and\n> seeing that what the code does contradicts with the stated\n> motivation we can safely declare that this is an ancient bug.\n\nAgreed, the evidence is clear.\n\n> Fixing that bug can of course be left outside the series. For those\n> who are looking for microproject ideas who discovered this message\n> by searching for the #leftoverbits keyword, one possible fix would\n> be to revert ebdaae37, make sure a value with any whitespace in it\n> gets quoted, and document clearly that an unquoted run of\n> whitespaces is squashed into a single SP. Another way that is\n> milder is to finish what ebdaae37 wanted to do and retain the\n> whitespaces \"verbatim\".\n\nI already though about fixing the bug so the value parser actually does\nwhat git-config(1) currently says, but as I already noted above, I'm\nafraid a bit that fixing this bug may actually do more harm than good.\n\nThough, further investigation shows that setting a configuration value,\nby invoking git-config(1), converts value-internal tabs into \"\\t\" escape\nsequences, which the value-parsing logic doesn't \"squash\" into spaces.\nThat's why the test included in the ebdaae37 commit passes. On the \nother\nhand, value-internal literal tab characters, found in a configuration\nfile, do get \"squashed\" by the value-parsing logic, so I'd say that the\nonly right thing to do is to fix this bug by making the value-internal\nwhitespace characters preserved verbatim.\n\nI'd be happy to include the bugfix into this series, if my \nabove-mentioned\nfears prove to be unnecessary.\n"},{"id":"490609","messageId":"07d0ac0d11b47983d3aa1760fc01ad92@manjaro.org","threadId":"61099","inReplyTo":"xmqqedcd1sm5.fsf@gitster.g","subject":"Re: [PATCH 2/3] config.txt: perform some minor reformatting","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-14T06:20:25Z","receivedAt":"2024-03-14T06:20:27Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-03-14 02:58, Junio C Hamano wrote:\n> Dragan Simic writes:\n> \n>> Reformat a few lines a bit, to utilize the available horizontal space \n>> better.\n>> There are no changes to the actual contents of the documentation.\n> \n> I was a bit surprised to see such a \"preliminary clean-up\" step to\n> come before the main change, not after, but separating this from the\n> change to the next paragraph, which is the main change in this series,\n> is nevertheless a very good idea.\n\nThe reason why this patch went as the second in the series was simply\nbecause it's a somewhat unrelated cleanup that performs no actual \nchanges\nto the contents of the documentation.\n\nThus, the first patch in the series brings the changes, the second patch\nstays in the same domain but only as a cleanup, and the third patch \nmoves\nto another domain. I find it quite logical.\n"},{"id":"490610","messageId":"9ed8b0f2ad3521eb14dd24082b4aa993@manjaro.org","threadId":"61099","inReplyTo":"xmqq8r2l1rot.fsf@gitster.g","subject":"Re: [PATCH 3/3] t1300: add tests for internal whitespace and inline comments","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-14T06:20:43Z","receivedAt":"2024-03-14T06:20:45Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-03-14 03:18, Junio C Hamano wrote:\n> Dragan Simic writes:\n> \n>> Add a couple of additional automated tests, to improve the coverage of\n>> configuration file entries whose values contain internal whitespace, \n>> or have\n>> an additional inline comment.\n> \n> While this may document the current behaviour, I am not sure of the\n> value of carving the current behaviour in stone, especially after\n> checking if the current behaviour is a bug.\n\nPlease see my comments for the first patch in the series.\n"},{"id":"490621","messageId":"xmqqfrwsyeac.fsf@gitster.g","threadId":"61099","inReplyTo":"07d0ac0d11b47983d3aa1760fc01ad92@manjaro.org","subject":"Re: [PATCH 2/3] config.txt: perform some minor reformatting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-14T16:22:03Z","receivedAt":"2024-03-14T16:22:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dragan Simic writes:\n\n>> I was a bit surprised to see such a \"preliminary clean-up\" step to\n>> come before the main change, not after, but separating this from the\n>> change to the next paragraph, which is the main change in this series,\n>> is nevertheless a very good idea.\n>\n> The reason why this patch went as the second in the series was simply\n> because it's a somewhat unrelated cleanup that performs no actual\n> changes\n> to the contents of the documentation.\n\nIt would have been understandable if it were left at the end, as\n\"after the dust settles\". It would made even more sense if it were\nat the front, \"before doing anything else, let's clean up the\nmess--we do not intend to change the behaviour with this change at\nall\". Having it in the middle was what made me surprised.\n\nGenerally, the order of preference is to do \"preliminary clean-up\"\nfirst, followed by the real change. That way, trivial clean-up that\nis designed not to change any behaviour can go ahead and merged down\neven before the real change solidifies.\n\nUnrelated changes has no place in a series with a real purpose.\nUnless the series is about \"assorted clean-ups that are not related\nwith each other\", that is.\n\nThanks.\n\n\n"},{"id":"490624","messageId":"xmqqh6h8wymc.fsf@gitster.g","threadId":"61099","inReplyTo":"ff7b0a2ead90ad9a9456141da5e4df4a@manjaro.org","subject":"Re: [PATCH 1/3] config.txt: describe whitespace characters further and more accurately","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-14T16:45:47Z","receivedAt":"2024-03-14T16:45:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dragan Simic writes:\n\n> Though, further investigation shows that setting a configuration value,\n> by invoking git-config(1), converts value-internal tabs into \"\\t\" escape\n> sequences, which the value-parsing logic doesn't \"squash\" into spaces.\n\nCorrect. It would have been nicer to just quote values that had\nwhitespaces in them, but replacing HT to SP while turning HT that\ncomes from our tool into \"\\t\" would still let the value round-trip,\nwhile breaking anything written manually in editors. If you stay\nwithin Git without using any editor, what ebdaae37 (config: Keep\ninner whitespace verbatim, 2009-07-30) left us is at least\ninternally consistent.\n\n> I'd be happy to include the bugfix into this series, if my\n> above-mentioned\n> fears prove to be unnecessary.\n\nDocumenting status quo is a good place to stop for now. I do not\nknow if it is a good idea to add too many tests to etch the current\nbehaviour that we know is wrong and we'll need to update when we fix\nthe bug, though.\n\nThanks.\n"},{"id":"490639","messageId":"f7fa6d9bc9835ad1139bced5ea2b20b3@manjaro.org","threadId":"61099","inReplyTo":"xmqqfrwsyeac.fsf@gitster.g","subject":"Re: [PATCH 2/3] config.txt: perform some minor reformatting","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-14T18:40:01Z","receivedAt":"2024-03-14T18:40:10Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-03-14 17:22, Junio C Hamano wrote:\n> Dragan Simic writes:\n> \n>>> I was a bit surprised to see such a \"preliminary clean-up\" step to\n>>> come before the main change, not after, but separating this from the\n>>> change to the next paragraph, which is the main change in this \n>>> series,\n>>> is nevertheless a very good idea.\n>> \n>> The reason why this patch went as the second in the series was simply\n>> because it's a somewhat unrelated cleanup that performs no actual\n>> changes\n>> to the contents of the documentation.\n> \n> It would have been understandable if it were left at the end, as\n> \"after the dust settles\". It would made even more sense if it were\n> at the front, \"before doing anything else, let's clean up the\n> mess--we do not intend to change the behaviour with this change at\n> all\". Having it in the middle was what made me surprised.\n> \n> Generally, the order of preference is to do \"preliminary clean-up\"\n> first, followed by the real change. That way, trivial clean-up that\n> is designed not to change any behaviour can go ahead and merged down\n> even before the real change solidifies.\n\nAfter thinking a bit more about it, I'd agree, especially because\nsuch an approach makes accepting patches easier. Thank you for\npointing that out!\n\n> Unrelated changes has no place in a series with a real purpose.\n> Unless the series is about \"assorted clean-ups that are not related\n> with each other\", that is.\n\nHaving all in mind, especially the addition of a bugfix for the\nvalue parsing into the series, I think it's the best if I take the\ncleanup patch out of the series and send it separately.\n"},{"id":"490640","messageId":"11be11f231f3bf41d0245c780c20693f@manjaro.org","threadId":"61099","inReplyTo":"xmqqh6h8wymc.fsf@gitster.g","subject":"Re: [PATCH 1/3] config.txt: describe whitespace characters further and more accurately","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-14T18:48:22Z","receivedAt":"2024-03-14T18:48:24Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-03-14 17:45, Junio C Hamano wrote:\n> Dragan Simic writes:\n> \n>> Though, further investigation shows that setting a configuration \n>> value,\n>> by invoking git-config(1), converts value-internal tabs into \"\\t\" \n>> escape\n>> sequences, which the value-parsing logic doesn't \"squash\" into spaces.\n> \n> Correct. It would have been nicer to just quote values that had\n> whitespaces in them, but replacing HT to SP while turning HT that\n> comes from our tool into \"\\t\" would still let the value round-trip,\n> while breaking anything written manually in editors. If you stay\n> within Git without using any editor, what ebdaae37 (config: Keep\n> inner whitespace verbatim, 2009-07-30) left us is at least\n> internally consistent.\n\nYes, but we already support unquoted values that contain whitespace\ncharacters, and people use editors to configure variables. For example,\nI never use git-config(1) to make changes to my ~/.gitconfig file.\n\n>> I'd be happy to include the bugfix into this series, if my\n>> above-mentioned\n>> fears prove to be unnecessary.\n> \n> Documenting status quo is a good place to stop for now. I do not\n> know if it is a good idea to add too many tests to etch the current\n> behaviour that we know is wrong and we'll need to update when we fix\n> the bug, though.\n\nBut I already started to work on a bugfix? I'm pretty much close to\ncompleting the bugfix and doing some testing.\n"}]}