{"thread":{"id":"50624","subject":"[PATCH] builtin/config.c: don't print a newline with --color","startedAt":"2019-03-02T00:41:01Z","lastAt":"2019-03-07T03:47:47Z","messageCount":18,"participants":["Taylor Blau","Eric Sunshine","Johannes Schindelin","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"370456","messageId":"b5ca6391fd0273fb7d6b92bc5ada96df93bc5cf2.1551487219.git.me@ttaylorr.com","threadId":"50624","inReplyTo":null,"subject":"[PATCH] builtin/config.c: don't print a newline with --color","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-03-02T00:40:53Z","receivedAt":"2019-03-02T00:41:01Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Invocations of 'git config ' print a trailing newline after\nwriting the value assigned to the given configuration variable.\n\nThis has an unexpected interaction with 63e2a0f8e9 (builtin/config:\nintroduce `color` type specifier, 2018-04-09), which causes a newline to\nbe printed after a color's ANSI escape sequence.\n\nIn this case, printing a terminating newline is not desirable. Callers\nmay want to print out such a configuration variable in a sub-shell in\norder to color something else, in which case they certainly don't want a\nnewline.\n\nThis bug has survived because there was never a test that would have\ncaught it. The old test used 'test_decode_color', which checks that its\ninput begins with a color, but stops parsing once it has parsed a single\ncolor successfully. In this case, it was ignoring the trailing '\\n'.\n\nTo do what callers expect, only print a newline when the type is not\n'color', and print the escape sequence itself for an exact comparison.\n\nSigned-off-by: Taylor Blau \n---\n builtin/config.c | 3 ++-\n t/t1300-config.sh | 5 ++---\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 98d65bc0ad..c8f088af38 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -258,7 +258,8 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value\n \t\t\t\tstrbuf_setlen(buf, buf->len - 1);\n \t\t}\n \t}\n-\tstrbuf_addch(buf, term);\n+\tif (type != TYPE_COLOR)\n+\t\tstrbuf_addch(buf, term);\n \treturn 0;\n }\n \ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex 428177c390..ec1b3a852d 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -907,9 +907,8 @@ test_expect_success 'get --expiry-date' '\n test_expect_success 'get --type=color' '\n \trm .git/config &&\n \tgit config foo.color \"red\" &&\n-\tgit config --get --type=color foo.color >actual.raw &&\n-\ttest_decode_color actual &&\n-\techo \"\" >expect &&\n+\tprintf \"\\\\033[31m\" >expect &&\n+\tgit config --get --type=color foo.color >actual &&\n \ttest_cmp expect actual\n '\n \n-- \n2.20.1\n"},{"id":"370466","messageId":"CAPig+cQWCSVqXyOmBC9Wn-Ew79H0yGPRJxRiLCMNJjNOUu2yPw@mail.gmail.com","threadId":"50624","inReplyTo":"b5ca6391fd0273fb7d6b92bc5ada96df93bc5cf2.1551487219.git.me@ttaylorr.com","subject":"Re: [PATCH] builtin/config.c: don't print a newline with --color","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-03-02T12:34:57Z","receivedAt":"2019-03-02T12:35:12Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 1, 2019 at 7:41 PM Taylor Blau wrote:\n> [...]\n> In this case, printing a terminating newline is not desirable. Callers\n> may want to print out such a configuration variable in a sub-shell in\n> order to color something else, in which case they certainly don't want a\n> newline.\n\nIt's extra confusing considering that this:\n\n color=$(git config --get --type=color foo.color)\n echo \"${color}hello\" >output\n\nworks as expected since $(...) swallows the newline, whereas, the case you cite:\n\n (\n git config --get --type=color foo.color &&\n echo hello\n ) >output\n\ndoes not.\n\nIllustrating the problem in the commit message by using example code\nlike the above might (or might not) help the reader understand the\nissue more easily. (I had to read the prose description of the problem\na couple times to properly understand it.) Not necessarily worth a\nre-roll.\n\n> To do what callers expect, only print a newline when the type is not\n> 'color', and print the escape sequence itself for an exact comparison.\n>\n> Signed-off-by: Taylor Blau \n> ---\n> diff --git a/builtin/config.c b/builtin/config.c\n> @@ -258,7 +258,8 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value\n> - strbuf_addch(buf, term);\n> + if (type != TYPE_COLOR)\n> + strbuf_addch(buf, term);\n\nThe reasoning for this conditional is sufficiently subtle that it\nmight deserve an in-code comment (though, perhaps is not worth a\nre-roll).\n"},{"id":"370473","messageId":"nycvar.QRO.7.76.6.1903022123160.45@tvgsbejvaqbjf.bet","threadId":"50624","inReplyTo":"b5ca6391fd0273fb7d6b92bc5ada96df93bc5cf2.1551487219.git.me@ttaylorr.com","subject":"Re: [PATCH] builtin/config.c: don't print a newline with --color","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-03-02T20:25:28Z","receivedAt":"2019-03-02T20:25:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Taylor,\n\nOn Fri, 1 Mar 2019, Taylor Blau wrote:\n\n> Invocations of 'git config ' print a trailing newline after\n> writing the value assigned to the given configuration variable.\n> \n> This has an unexpected interaction with 63e2a0f8e9 (builtin/config:\n> introduce `color` type specifier, 2018-04-09), which causes a newline to\n> be printed after a color's ANSI escape sequence.\n> \n> In this case, printing a terminating newline is not desirable. Callers\n> may want to print out such a configuration variable in a sub-shell in\n> order to color something else, in which case they certainly don't want a\n> newline.\n> \n> This bug has survived because there was never a test that would have\n> caught it. The old test used 'test_decode_color', which checks that its\n> input begins with a color, but stops parsing once it has parsed a single\n> color successfully. In this case, it was ignoring the trailing '\\n'.\n\nIsn't the reason rather that our test compared the output to an expected\ntext *with a newline*? IOW:\n\n> diff --git a/t/t1300-config.sh b/t/t1300-config.sh\n> index 428177c390..ec1b3a852d 100755\n> --- a/t/t1300-config.sh\n> +++ b/t/t1300-config.sh\n> @@ -907,9 +907,8 @@ test_expect_success 'get --expiry-date' '\n> test_expect_success 'get --type=color' '\n> \trm .git/config &&\n> \tgit config foo.color \"red\" &&\n> -\tgit config --get --type=color foo.color >actual.raw &&\n> -\ttest_decode_color actual &&\n> -\techo \"\" >expect &&\n\nThis should do the right thing if you write\n\n\tprintf \"\" >expect\n\ninstead?\n\nCiao,\nDscho\n\n> +\tprintf \"\\\\033[31m\" >expect &&\n> +\tgit config --get --type=color foo.color >actual &&\n> \ttest_cmp expect actual\n> '\n> \n> -- \n> 2.20.1\n> \n"},{"id":"370482","messageId":"xmqqtvgk69ik.fsf@gitster-ct.c.googlers.com","threadId":"50624","inReplyTo":"b5ca6391fd0273fb7d6b92bc5ada96df93bc5cf2.1551487219.git.me@ttaylorr.com","subject":"Re: [PATCH] builtin/config.c: don't print a newline with --color","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-03T01:18:11Z","receivedAt":"2019-03-03T01:18:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> Invocations of 'git config ' print a trailing newline after\n> writing the value assigned to the given configuration variable.\n>\n> This has an unexpected interaction with 63e2a0f8e9 (builtin/config:\n> introduce `color` type specifier, 2018-04-09), which causes a newline to\n> be printed after a color's ANSI escape sequence.\n>\n> In this case, printing a terminating newline is not desirable. Callers\n> may want to print out such a configuration variable in a sub-shell in\n> order to color something else, in which case they certainly don't want a\n> newline.\n>\n> This bug has survived because there was never a test that would have\n> caught it. The old test used 'test_decode_color', which checks that its\n> input begins with a color, but stops parsing once it has parsed a single\n> color successfully. In this case, it was ignoring the trailing '\\n'.\n\nThe output from \"git config\" plumbing command were designed to help\npeople writing shell scripts Porcelain around it, so the expected\nuse for them has always been\n\n\tERR=$(git config --type=color --default=red ui.color.error)\n\t... some time later ..\n\techo \"${ERR}this is an error message\"\n\nwhere the first assignment will strip the final LF (i.e. the value\nof the $ERR variable does not have it).\n\nAn interesting aspect of the above is that this is *NOT* limited to\ncolors. Regardless of the type you are reading, be it an int or a\nbool, VAR=$(git config ...) will strip the trailing LF, and existing\nscripts people have do rely on that, i.e. when people write\n\n\tVAR=$(git config ...)\n\techo \"var setting is $VAR\"\n\nthey rely on VAR=$(...) assignment to strip trailing LF and echo to\nadd a final LF to the string.\n\nSo if we are going to change anything, the change MUST NOT single\nout \"color\". IOW, the title of the patch already tells us that it\nis giving a wrong solution.\n\nWhether you limit it to color or not, to Porcelain writers who are\nwriting in shell, I suspect that the code after the proposed change\nwill not be a huge regression. VAR=$(git config ...) assignment,\nwhen the output from the command ends without the final LF (i.e. an\nincomplete line), will keep the string intact, so the behaviour of\nthese shell scripts would not change.\n\nIf an existing Porcelain script were written in Perl and uses chop\nto strip the last LF coming out of \"git config\", however, the\nproposed change WILL BREAK such a script.\n\nNeedless to say, \"using chop in Perl is wrong to begin with\" misses\nthe point from two directions---(1) 'chop in Perl' is a mere\nexample---scripts not written in Perl using chop may still rely on\nthe existing behaviour that the output always has the final LF, and\n(2) even if we agree that using chop in Perl is a bad idea, such a\nscript has happily been working, and suddenly breaking it is a\nregression no matter what.\n\nSo, I am not hugely enthused by this change, even though I am\nsomewhat sympathetic to it, as it would help one narrow use case,\ni.e. \"interpolation\".\n\n\tcat < > This bug has survived because there was never a test that would have\n> > caught it. The old test used 'test_decode_color', which checks that its\n> > input begins with a color, but stops parsing once it has parsed a single\n> > color successfully. In this case, it was ignoring the trailing '\\n'.\n> \n> Isn't the reason rather that our test compared the output to an expected\n> text *with a newline*? IOW:\n> [...]\n> This should do the right thing if you write\n> \n> \tprintf \"\" >expect\n\nNo, there are actually two problems that cancel each other out. Because\ntest_decode_color() is implemented in awk, it doesn't see if there's\nactually a newline or not in each record. So it _always_ adds a newline\nafter printing out the line (and I don't think Taylor's explanation\nabove is quite right; it does have a loop to handle multiple colors).\n\nSo regardless of whether git-config is sending the newline or not, the\n\"actual\" file will always have a newline in it.\n\nAnd then to match that, we used \"echo\" which of course has a newline,\nand matches the test_decode_color output.\n\nSo you're right that we need to switch to printf. But doing so without\ndropping test_decode_color() would mean the test would always fail. We\nhave to printf the raw bytes, which is what the new test does.\n\n-Peff\n"},{"id":"370549","messageId":"20190303174214.GF23811@sigill.intra.peff.net","threadId":"50624","inReplyTo":"xmqqtvgk69ik.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] builtin/config.c: don't print a newline with --color","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-03T17:42:15Z","receivedAt":"2019-03-03T17:42:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 03, 2019 at 10:18:11AM +0900, Junio C Hamano wrote:\n\n> An interesting aspect of the above is that this is *NOT* limited to\n> colors. Regardless of the type you are reading, be it an int or a\n> bool, VAR=$(git config ...) will strip the trailing LF, and existing\n> scripts people have do rely on that, i.e. when people write\n> \n> \tVAR=$(git config ...)\n> \techo \"var setting is $VAR\"\n> \n> they rely on VAR=$(...) assignment to strip trailing LF and echo to\n> add a final LF to the string.\n> \n> So if we are going to change anything, the change MUST NOT single\n> out \"color\". IOW, the title of the patch already tells us that it\n> is giving a wrong solution.\n\nI'm not sure I agree. Colors have always been special, and\n\"--type=color\" was advertised as a replacement for \"--get-color\". And\n\"--get-color\" never output the newline.\n\nPhilosophically speaking, I'd make the argument that the \"color\" type is\na bit special because unlike other output, it is unreadable binary gunk.\nBut I dunno. It does suck for the output to be dependent on \"--type\",\nbecause it means that anything consuming it has to understand the\nvarious types. It also creates potential complications if we ever\nimplement a \"--stdin\" mode to grab the (type-interpreted) values of\nseveral keys, where presumably we'd have to have newlines as part of the\nprotocol.\n\n> Needless to say, \"using chop in Perl is wrong to begin with\" misses\n> the point from two directions---(1) 'chop in Perl' is a mere\n> example---scripts not written in Perl using chop may still rely on\n> the existing behaviour that the output always has the final LF, and\n> (2) even if we agree that using chop in Perl is a bad idea, such a\n> script has happily been working, and suddenly breaking it is a\n> regression no matter what.\n\nWith respect to backwards compatibility, my thinking on the matter was\nbasically:\n\n 1. Since --type=color was supposed to be a drop-in replacement for\n --get-color, it's a bug that they don't behave the same.\n\n 2. It's a fairly recent feature, so nobody really noticed the bug\n until recently (and it was in fact me who noticed and got annoyed\n by it). If it were an ancient behavior, we might think about\n retaining even bug compatibility, but that's not the case here.\n\nBut I do admit your argument about consistency is giving me second\nthoughts.\n\n-Peff\n"},{"id":"370572","messageId":"xmqqlg1vw9f2.fsf@gitster-ct.c.googlers.com","threadId":"50624","inReplyTo":"20190303174214.GF23811@sigill.intra.peff.net","subject":"Re: [PATCH] builtin/config.c: don't print a newline with --color","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-04T04:28:01Z","receivedAt":"2019-03-04T04:28:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I'm not sure I agree. Colors have always been special, and\n> \"--type=color\" was advertised as a replacement for \"--get-color\". And\n> \"--get-color\" never output the newline.\n\nOK, that part of the motivation I completely missed. If end-users\nand scripters are happy with the behaviour of --get-color that does\nnot terminate its output with LF (which I think is a reasonable\nthing to do, as \"color\" is special in that context, i.e. having a\ndedicated --get option suitable for that type), as we advertise\n\"--type=color\" is the same as \"--get-color\" (only better), we need\nto special case it, and omitting LF at the end similarly does make\nsense.\n\n> With respect to backwards compatibility, my thinking on the matter was\n> basically:\n>\n> 1. Since --type=color was supposed to be a drop-in replacement for\n> --get-color, it's a bug that they don't behave the same.\n>\n> 2. It's a fairly recent feature, so nobody really noticed the bug\n> until recently (and it was in fact me who noticed and got annoyed\n> by it). If it were an ancient behavior, we might think about\n> retaining even bug compatibility, but that's not the case here.\n\nNow I think \"we weren't consistent to begin with with --get-color,\nand treating --type=color as a special case is justifiable\"; and I\nagree with the above two points.\n\nThanks.\n"},{"id":"370573","messageId":"xmqqh8cjw7ob.fsf@gitster-ct.c.googlers.com","threadId":"50624","inReplyTo":"xmqqlg1vw9f2.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] builtin/config.c: don't print a newline with --color","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-04T05:05:40Z","receivedAt":"2019-03-04T05:05:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> I'm not sure I agree. Colors have always been special, and\n>> \"--type=color\" was advertised as a replacement for \"--get-color\". And\n>> \"--get-color\" never output the newline.\n>\n> OK, that part of the motivation I completely missed. If end-users\n> and scripters are happy with the behaviour of --get-color that does\n> not terminate its output with LF (which I think is a reasonable\n> thing to do, as \"color\" is special in that context, i.e. having a\n> dedicated --get option suitable for that type), as we advertise\n> \"--type=color\" is the same as \"--get-color\" (only better), we need\n> to special case it, and omitting LF at the end similarly does make\n> sense.\n>\n>> With respect to backwards compatibility, my thinking on the matter was\n>> basically:\n>>\n>> 1. Since --type=color was supposed to be a drop-in replacement for\n>> --get-color, it's a bug that they don't behave the same.\n>>\n>> 2. It's a fairly recent feature, so nobody really noticed the bug\n>> until recently (and it was in fact me who noticed and got annoyed\n>> by it). If it were an ancient behavior, we might think about\n>> retaining even bug compatibility, but that's not the case here.\n>\n> Now I think \"we weren't consistent to begin with with --get-color,\n> and treating --type=color as a special case is justifiable\"; and I\n> agree with the above two points.\n\nJust to avoid an awkward situation where the ball gets dropped and\nleft on the floor forgotten, the above does not mean I am 100% happy\nwith the patch as posted. There is no mention of --get-color\nanywhere, let alone it shows the ANSI sequence without traililng LF,\nwhich I would consider to be the most important part of the\njustification. It is much stronger than \"I expected there won't be\nany trailing LF from 'git config'\", which was the only thing I\nmanaged to read in the original and led to my response.\n"},{"id":"370659","messageId":"20190305042050.GE19800@sigill.intra.peff.net","threadId":"50624","inReplyTo":"xmqqh8cjw7ob.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] builtin/config.c: don't print a newline with --color","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-05T04:20:51Z","receivedAt":"2019-03-05T04:20:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 04, 2019 at 02:05:40PM +0900, Junio C Hamano wrote:\n\n> >> With respect to backwards compatibility, my thinking on the matter was\n> >> basically:\n> >>\n> >> 1. Since --type=color was supposed to be a drop-in replacement for\n> >> --get-color, it's a bug that they don't behave the same.\n> >>\n> >> 2. It's a fairly recent feature, so nobody really noticed the bug\n> >> until recently (and it was in fact me who noticed and got annoyed\n> >> by it). If it were an ancient behavior, we might think about\n> >> retaining even bug compatibility, but that's not the case here.\n> >\n> > Now I think \"we weren't consistent to begin with with --get-color,\n> > and treating --type=color as a special case is justifiable\"; and I\n> > agree with the above two points.\n> \n> Just to avoid an awkward situation where the ball gets dropped and\n> left on the floor forgotten, the above does not mean I am 100% happy\n> with the patch as posted. There is no mention of --get-color\n> anywhere, let alone it shows the ANSI sequence without traililng LF,\n> which I would consider to be the most important part of the\n> justification. It is much stronger than \"I expected there won't be\n> any trailing LF from 'git config'\", which was the only thing I\n> managed to read in the original and led to my response.\n\nYeah, I agree it needs to be the main justification in the commit\nmessage.\n\nI do wonder, though, if we're digging ourselves a hole with the\ninconsistency between different --types that will bite us later. Given\nthat it's not that hard to chomp the output (and as you noted, the shell\ndoes it fairly transparently), and given that the caller has to switch\nbetween \"--get-color\" and \"--type=color\", it's not that hard to handle\nthe output differently if you know to do so.\n\nMostly I was just surprised by the new behavior. Perhaps the right\nsolution is not a patch to the code, but to the documentation. Something\nlike:\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex 495bb57416..61f3a9cdd7 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -252,7 +252,9 @@ Valid ``'s include:\n \toutput. The optional `default` parameter is used instead, if\n \tthere is no color configured for `name`.\n +\n-`--type=color [--default=]` is preferred over `--get-color`.\n+`--type=color [--default=]` is preferred over `--get-color`\n+(but note that `--get-color` will omit the trailing newline printed by\n+--type=color).\n \n -e::\n --edit::\n\n-Peff\n"},{"id":"370670","messageId":"xmqqva0xvp6b.fsf@gitster-ct.c.googlers.com","threadId":"50624","inReplyTo":"20190305042050.GE19800@sigill.intra.peff.net","subject":"Re: [PATCH] builtin/config.c: don't print a newline with --color","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-05T05:57:32Z","receivedAt":"2019-03-05T05:57:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I do wonder, though, if we're digging ourselves a hole with the\n> inconsistency between different --types that will bite us later. Given\n> that it's not that hard to chomp the output (and as you noted, the shell\n> does it fairly transparently), and given that the caller has to switch\n> between \"--get-color\" and \"--type=color\", it's not that hard to handle\n> the output differently if you know to do so.\n>\n> Mostly I was just surprised by the new behavior. Perhaps the right\n> solution is not a patch to the code, but to the documentation. Something\n> like:\n>\n> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\n> index 495bb57416..61f3a9cdd7 100644\n> --- a/Documentation/git-config.txt\n> +++ b/Documentation/git-config.txt\n> @@ -252,7 +252,9 @@ Valid ``'s include:\n> \toutput. The optional `default` parameter is used instead, if\n> \tthere is no color configured for `name`.\n> +\n> -`--type=color [--default=]` is preferred over `--get-color`.\n> +`--type=color [--default=]` is preferred over `--get-color`\n> +(but note that `--get-color` will omit the trailing newline printed by\n> +--type=color).\n> \n> -e::\n> --edit::\n\nYup, that would be a very sensible first step, regardless of what\nthe next step is.\n\nAfter that, choices are\n\n (1) we'd introduce new inconsistency among --type= by\n matching what --type=color does to what --get-color does, to\n allow us to revert that documentation update, or\n\n (2) we'd drop LF from all --type=, that makes everything\n consistent and risk breaking a few existing scripts while doing\n so, and get yelled at by end users, or\n\n (3) we stop at this documentation update and do nothing else.\n\n"},{"id":"370724","messageId":"nycvar.QRO.7.76.6.1903051658200.41@tvgsbejvaqbjf.bet","threadId":"50624","inReplyTo":"20190303172428.GE23811@sigill.intra.peff.net","subject":"Re: [PATCH] builtin/config.c: don't print a newline with --color","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-03-05T15:59:29Z","receivedAt":"2019-03-05T16:00:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Sun, 3 Mar 2019, Jeff King wrote:\n\n> On Sat, Mar 02, 2019 at 09:25:28PM +0100, Johannes Schindelin wrote:\n> \n> > > This bug has survived because there was never a test that would have\n> > > caught it. The old test used 'test_decode_color', which checks that its\n> > > input begins with a color, but stops parsing once it has parsed a single\n> > > color successfully. In this case, it was ignoring the trailing '\\n'.\n> > \n> > Isn't the reason rather that our test compared the output to an expected\n> > text *with a newline*? IOW:\n> > [...]\n> > This should do the right thing if you write\n> > \n> > \tprintf \"\" >expect\n> \n> No, there are actually two problems that cancel each other out. Because\n> test_decode_color() is implemented in awk, it doesn't see if there's\n> actually a newline or not in each record.\n\nAh, so it is actually another instance of \"we really need a precise\ntest-tool command for this rather than a somewhat incomplete and fragile\nscript\" ;-)\n\n> So it _always_ adds a newline after printing out the line (and I don't\n> think Taylor's explanation above is quite right; it does have a loop to\n> handle multiple colors).\n> \n> So regardless of whether git-config is sending the newline or not, the\n> \"actual\" file will always have a newline in it.\n> \n> And then to match that, we used \"echo\" which of course has a newline,\n> and matches the test_decode_color output.\n> \n> So you're right that we need to switch to printf. But doing so without\n> dropping test_decode_color() would mean the test would always fail. We\n> have to printf the raw bytes, which is what the new test does.\n\nFair enough.\n\nCiao,\nDscho\n"},{"id":"370839","messageId":"20190306234236.GE27082@Taylors-MBP.hsd1.wa.comcast.net","threadId":"50624","inReplyTo":"xmqqva0xvp6b.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] builtin/config.c: don't print a newline with --color","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-03-06T23:42:36Z","receivedAt":"2019-03-06T23:42:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Mar 05, 2019 at 02:57:32PM +0900, Junio C Hamano wrote:\n> Yup, that would be a very sensible first step, regardless of what\n> the next step is.\n>\n> After that, choices are\n>\n> (1) we'd introduce new inconsistency among --type= by\n> matching what --type=color does to what --get-color does, to\n> allow us to revert that documentation update, or\n\nI suppose... though I think that if others agree, I'd rather update the\ndocumentation instead of introduce some inconsistency.\n\nYes, there's an argument to be made that if we're encouraging users to\ngo from '--get-color' -> '--type=color', that the two should behave the\nsame, but I don't think the cost we pay for behavioral equivalence\nbetween the two is worth inconsistency among '--type=color' and all the\nrest.\n\n> (2) we'd drop LF from all --type=, that makes everything\n> consistent and risk breaking a few existing scripts while doing\n> so, and get yelled at by end users, or\n\nAs you indicate, I think that this option is one we should _not_ do. In\nthe interpolation example you shared earlier in the thread, script\nwriters most likely want and expect a trailing LF after each invocation\nof 'git config'.\n\nI'd argue that this case is more common than not wanting a LF when\ninterpolating with `--type=color`, so I agree it seems the tradeoff here\nis not a good one.\n\n> (3) we stop at this documentation update and do nothing else.\n\nTo restate my response to (1), I think that the documentation update in\nisolation makes the most sense here. I, too, was surprised in the same\nway that Peff was when we stumbled upon this, but I think that\nultimately the consistency is most favorable.\n\nThanks all for your discussion and feedback.\n\n\nThanks,\nTaylor\n"},{"id":"370841","messageId":"20190306234449.GF27082@Taylors-MBP.hsd1.wa.comcast.net","threadId":"50624","inReplyTo":"nycvar.QRO.7.76.6.1903022123160.45@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] builtin/config.c: don't print a newline with --color","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-03-06T23:44:49Z","receivedAt":"2019-03-06T23:44:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Johannes,\n\nOn Sat, Mar 02, 2019 at 09:25:28PM +0100, Johannes Schindelin wrote:\n> Hi Taylor,\n>\n> On Fri, 1 Mar 2019, Taylor Blau wrote:\n>\n> > [ ... ]\n>\n> This should do the right thing if you write\n>\n> \tprintf \"\" >expect\n>\n> instead?\n>\n> Ciao,\n> Dscho\n>\n> > +\tprintf \"\\\\033[31m\" >expect &&\n> > +\tgit config --get --type=color foo.color >actual &&\n> > \ttest_cmp expect actual\n> > '\n> >\n> > --\n> > 2.20.1\n\nThank you for your suggestion. It occurred to me that this might be a\npreferable way to do things[1] after I sent the patch, but I am glad\nthat you suggested it here explicitly.\n\nThat said, I think that this patch has moved on from the code change,\nafter some discussion between Junio and Peff, so I think that I will\nleave this (and the rest of the code changes) behind.\n\nThanks,\nTaylor\n\n[1] ... preferable in the way that we don't have to write \"\\\\033[31m\" in\n the test.\n"},{"id":"370844","messageId":"be22686b93fa76f0ff63a7d93cb731545c57cb22.1551916338.git.me@ttaylorr.com","threadId":"50624","inReplyTo":"b5ca6391fd0273fb7d6b92bc5ada96df93bc5cf2.1551487219.git.me@ttaylorr.com","subject":"[PATCH v2] Documentation/config: note trailing newline with --type=color","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-03-06T23:52:38Z","receivedAt":"2019-03-06T23:52:43Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In 63e2a0f8e9 (builtin/config: introduce `color` type specifier,\n2018-04-09), `--type=color` was introduced and preferred to the\nold-style `--get-color`.\n\nThe two behave the same in almost every way, save for the fact that\n`--type=color` prints a trailing newline where `--get-color` does not.\nInstead of introducing ambiguity between `--type=color` and the other\n`--type` variants, document the difference between `--type=color` and\n`--get-color` instead.\n\nCo-authored-by: Jeff King \nSigned-off-by: Taylor Blau \n---\n Documentation/git-config.txt | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex 1bfe9f56a7..d0b9c50d20 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -240,7 +240,9 @@ Valid ``'s include:\n \toutput. The optional `default` parameter is used instead, if\n \tthere is no color configured for `name`.\n +\n-`--type=color [--default=]` is preferred over `--get-color`.\n+`--type=color [--default=]` is preferred over `--get-color`\n+(but note that `--get-color` will omit the trailing newline printed by\n+`--type=color`).\n \n -e::\n --edit::\n-- \n2.20.1\n"},{"id":"370854","messageId":"xmqqbm2no6by.fsf@gitster-ct.c.googlers.com","threadId":"50624","inReplyTo":"20190305042050.GE19800@sigill.intra.peff.net","subject":"Re: [PATCH] builtin/config.c: don't print a newline with --color","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-07T00:50:57Z","receivedAt":"2019-03-07T00:51:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Mostly I was just surprised by the new behavior. Perhaps the right\n> solution is not a patch to the code, but to the documentation. Something\n> like:\n\nLet me forge your sign-off and commit this to prevent us from\nforgetting.\n\nThanks, all.\n\n-- >8 --\nFrom: Jeff King \nDate: Mon, 4 Mar 2019 23:20:51 -0500\nSubject: [PATCH] config: document --type=color output is a complete line\n\nEven though the newer \"--type=color\" option to \"git config\" is meant\nto be upward compatible with the traditional \"--get-color\" option,\nunlike the latter, its output is not an incomplete line that lack\nthe LF at the end. That makes it consistent with output of other\ntypes like \"git config --type=bool\".\n\nDocument it, as it sometimes surprises unsuspecting users.\n\nSigned-off-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n Documentation/git-config.txt | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex 1bfe9f56a7..611a32445c 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -240,7 +240,9 @@ Valid ``'s include:\n \toutput. The optional `default` parameter is used instead, if\n \tthere is no color configured for `name`.\n +\n-`--type=color [--default=]` is preferred over `--get-color`.\n+`--type=color [--default=]` is preferred over `--get-color`\n+(but note that `--get-color` will omit the trailing newline printed by\n+--type=color).\n \n -e::\n --edit::\n-- \n2.21.0-4-g36eb1cb9cf\n\n"},{"id":"370857","messageId":"20190307025703.GA25656@sigill.intra.peff.net","threadId":"50624","inReplyTo":"xmqqbm2no6by.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] builtin/config.c: don't print a newline with --color","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-07T02:57:03Z","receivedAt":"2019-03-07T02:57:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 07, 2019 at 09:50:57AM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Mostly I was just surprised by the new behavior. Perhaps the right\n> > solution is not a patch to the code, but to the documentation. Something\n> > like:\n> \n> Let me forge your sign-off and commit this to prevent us from\n> forgetting.\n> \n> Thanks, all.\n\nThanks for tying this up. One minor nit:\n\n> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\n> index 1bfe9f56a7..611a32445c 100644\n> --- a/Documentation/git-config.txt\n> +++ b/Documentation/git-config.txt\n> @@ -240,7 +240,9 @@ Valid ``'s include:\n> \toutput. The optional `default` parameter is used instead, if\n> \tthere is no color configured for `name`.\n> +\n> -`--type=color [--default=]` is preferred over `--get-color`.\n> +`--type=color [--default=]` is preferred over `--get-color`\n> +(but note that `--get-color` will omit the trailing newline printed by\n> +--type=color).\n\nThat final line probably should have literal quotes, like:\n\n `--type=color`).\n\n-Peff\n"},{"id":"370858","messageId":"20190307025821.GB25656@sigill.intra.peff.net","threadId":"50624","inReplyTo":"be22686b93fa76f0ff63a7d93cb731545c57cb22.1551916338.git.me@ttaylorr.com","subject":"Re: [PATCH v2] Documentation/config: note trailing newline with --type=color","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-07T02:58:21Z","receivedAt":"2019-03-07T02:58:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 06, 2019 at 03:52:38PM -0800, Taylor Blau wrote:\n\n> In 63e2a0f8e9 (builtin/config: introduce `color` type specifier,\n> 2018-04-09), `--type=color` was introduced and preferred to the\n> old-style `--get-color`.\n> \n> The two behave the same in almost every way, save for the fact that\n> `--type=color` prints a trailing newline where `--get-color` does not.\n> Instead of introducing ambiguity between `--type=color` and the other\n> `--type` variants, document the difference between `--type=color` and\n> `--get-color` instead.\n\nI just responded to the one Junio posted elsewhere in the thread, but\nfor the record this one is fine to me, too (and it gets the literal\nquoting right ;) ).\n\n-Peff\n"},{"id":"370859","messageId":"xmqq4l8fny5f.fsf@gitster-ct.c.googlers.com","threadId":"50624","inReplyTo":"20190307025821.GB25656@sigill.intra.peff.net","subject":"Re: [PATCH v2] Documentation/config: note trailing newline with --type=color","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-07T03:47:40Z","receivedAt":"2019-03-07T03:47:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Mar 06, 2019 at 03:52:38PM -0800, Taylor Blau wrote:\n>\n>> In 63e2a0f8e9 (builtin/config: introduce `color` type specifier,\n>> 2018-04-09), `--type=color` was introduced and preferred to the\n>> old-style `--get-color`.\n>> \n>> The two behave the same in almost every way, save for the fact that\n>> `--type=color` prints a trailing newline where `--get-color` does not.\n>> Instead of introducing ambiguity between `--type=color` and the other\n>> `--type` variants, document the difference between `--type=color` and\n>> `--get-color` instead.\n>\n> I just responded to the one Junio posted elsewhere in the thread, but\n> for the record this one is fine to me, too (and it gets the literal\n> quoting right ;) ).\n\nYeah, I haven't reached that message when I was doing mine. Other\nthan \"co-authored-by\", which probably is not quite correct (i.e. I\nwould have added \"[tb: wrapped it up with a commit log message]\" and\nkept you as the author, if I were doing it), I do not have a problem\nwith Taylor's version, either.\n\n"}]}