| Message ID | b5ca6391fd0273fb7d6b92bc5ada96df93bc5cf2.1551487219.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | builtin/config.c: don't print a newline with --color | expand |
On Fri, Mar 1, 2019 at 7:41 PM Taylor Blau <me@ttaylorr.com> wrote: > [...] > In this case, printing a terminating newline is not desirable. Callers > may want to print out such a configuration variable in a sub-shell in > order to color something else, in which case they certainly don't want a > newline. It's extra confusing considering that this: color=$(git config --get --type=color foo.color) echo "${color}hello" >output works as expected since $(...) swallows the newline, whereas, the case you cite: ( git config --get --type=color foo.color && echo hello ) >output does not. Illustrating the problem in the commit message by using example code like the above might (or might not) help the reader understand the issue more easily. (I had to read the prose description of the problem a couple times to properly understand it.) Not necessarily worth a re-roll. > To do what callers expect, only print a newline when the type is not > 'color', and print the escape sequence itself for an exact comparison. > > Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- > diff --git a/builtin/config.c b/builtin/config.c > @@ -258,7 +258,8 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value > - strbuf_addch(buf, term); > + if (type != TYPE_COLOR) > + strbuf_addch(buf, term); The reasoning for this conditional is sufficiently subtle that it might deserve an in-code comment (though, perhaps is not worth a re-roll).
Hi Taylor, On Fri, 1 Mar 2019, Taylor Blau wrote: > Invocations of 'git config <name>' print a trailing newline after > writing the value assigned to the given configuration variable. > > This has an unexpected interaction with 63e2a0f8e9 (builtin/config: > introduce `color` type specifier, 2018-04-09), which causes a newline to > be printed after a color's ANSI escape sequence. > > In this case, printing a terminating newline is not desirable. Callers > may want to print out such a configuration variable in a sub-shell in > order to color something else, in which case they certainly don't want a > newline. > > This bug has survived because there was never a test that would have > caught it. The old test used 'test_decode_color', which checks that its > input begins with a color, but stops parsing once it has parsed a single > color successfully. In this case, it was ignoring the trailing '\n'. Isn't the reason rather that our test compared the output to an expected text *with a newline*? IOW: > diff --git a/t/t1300-config.sh b/t/t1300-config.sh > index 428177c390..ec1b3a852d 100755 > --- a/t/t1300-config.sh > +++ b/t/t1300-config.sh > @@ -907,9 +907,8 @@ test_expect_success 'get --expiry-date' ' > test_expect_success 'get --type=color' ' > rm .git/config && > git config foo.color "red" && > - git config --get --type=color foo.color >actual.raw && > - test_decode_color <actual.raw >actual && > - echo "<RED>" >expect && This should do the right thing if you write printf "<RED>" >expect instead? Ciao, Dscho > + printf "\\033[31m" >expect && > + git config --get --type=color foo.color >actual && > test_cmp expect actual > ' > > -- > 2.20.1 >
Taylor Blau <me@ttaylorr.com> writes: > Invocations of 'git config <name>' print a trailing newline after > writing the value assigned to the given configuration variable. > > This has an unexpected interaction with 63e2a0f8e9 (builtin/config: > introduce `color` type specifier, 2018-04-09), which causes a newline to > be printed after a color's ANSI escape sequence. > > In this case, printing a terminating newline is not desirable. Callers > may want to print out such a configuration variable in a sub-shell in > order to color something else, in which case they certainly don't want a > newline. > > This bug has survived because there was never a test that would have > caught it. The old test used 'test_decode_color', which checks that its > input begins with a color, but stops parsing once it has parsed a single > color successfully. In this case, it was ignoring the trailing '\n'. The output from "git config" plumbing command were designed to help people writing shell scripts Porcelain around it, so the expected use for them has always been ERR=$(git config --type=color --default=red ui.color.error) ... some time later .. echo "${ERR}this is an error message" where the first assignment will strip the final LF (i.e. the value of the $ERR variable does not have it). An interesting aspect of the above is that this is *NOT* limited to colors. Regardless of the type you are reading, be it an int or a bool, VAR=$(git config ...) will strip the trailing LF, and existing scripts people have do rely on that, i.e. when people write VAR=$(git config ...) echo "var setting is $VAR" they rely on VAR=$(...) assignment to strip trailing LF and echo to add a final LF to the string. So if we are going to change anything, the change MUST NOT single out "color". IOW, the title of the patch already tells us that it is giving a wrong solution. Whether you limit it to color or not, to Porcelain writers who are writing in shell, I suspect that the code after the proposed change will not be a huge regression. VAR=$(git config ...) assignment, when the output from the command ends without the final LF (i.e. an incomplete line), will keep the string intact, so the behaviour of these shell scripts would not change. If an existing Porcelain script were written in Perl and uses chop to strip the last LF coming out of "git config", however, the proposed change WILL BREAK such a script. Needless to say, "using chop in Perl is wrong to begin with" misses the point from two directions---(1) 'chop in Perl' is a mere example---scripts not written in Perl using chop may still rely on the existing behaviour that the output always has the final LF, and (2) even if we agree that using chop in Perl is a bad idea, such a script has happily been working, and suddenly breaking it is a regression no matter what. So, I am not hugely enthused by this change, even though I am somewhat sympathetic to it, as it would help one narrow use case, i.e. "interpolation". cat <<EOF $(git config ...)Foo Bar$(git config ...) EOF or ( git config ... echo Foo Bar git config ... ) would lack LF before Foo automatically, and forcing those who want to have LF to add it manually would sound easier than forcing those who want to strip LF when they do not want it. But when you are making a list, getting the final LF for free is a feature, so it cuts both ways.
On Sat, Mar 02, 2019 at 09:25:28PM +0100, Johannes Schindelin wrote: > > This bug has survived because there was never a test that would have > > caught it. The old test used 'test_decode_color', which checks that its > > input begins with a color, but stops parsing once it has parsed a single > > color successfully. In this case, it was ignoring the trailing '\n'. > > Isn't the reason rather that our test compared the output to an expected > text *with a newline*? IOW: > [...] > This should do the right thing if you write > > printf "<RED>" >expect No, there are actually two problems that cancel each other out. Because test_decode_color() is implemented in awk, it doesn't see if there's actually a newline or not in each record. So it _always_ adds a newline after printing out the line (and I don't think Taylor's explanation above is quite right; it does have a loop to handle multiple colors). So regardless of whether git-config is sending the newline or not, the "actual" file will always have a newline in it. And then to match that, we used "echo" which of course has a newline, and matches the test_decode_color output. So you're right that we need to switch to printf. But doing so without dropping test_decode_color() would mean the test would always fail. We have to printf the raw bytes, which is what the new test does. -Peff
On Sun, Mar 03, 2019 at 10:18:11AM +0900, Junio C Hamano wrote: > An interesting aspect of the above is that this is *NOT* limited to > colors. Regardless of the type you are reading, be it an int or a > bool, VAR=$(git config ...) will strip the trailing LF, and existing > scripts people have do rely on that, i.e. when people write > > VAR=$(git config ...) > echo "var setting is $VAR" > > they rely on VAR=$(...) assignment to strip trailing LF and echo to > add a final LF to the string. > > So if we are going to change anything, the change MUST NOT single > out "color". IOW, the title of the patch already tells us that it > is giving a wrong solution. I'm not sure I agree. Colors have always been special, and "--type=color" was advertised as a replacement for "--get-color". And "--get-color" never output the newline. Philosophically speaking, I'd make the argument that the "color" type is a bit special because unlike other output, it is unreadable binary gunk. But I dunno. It does suck for the output to be dependent on "--type", because it means that anything consuming it has to understand the various types. It also creates potential complications if we ever implement a "--stdin" mode to grab the (type-interpreted) values of several keys, where presumably we'd have to have newlines as part of the protocol. > Needless to say, "using chop in Perl is wrong to begin with" misses > the point from two directions---(1) 'chop in Perl' is a mere > example---scripts not written in Perl using chop may still rely on > the existing behaviour that the output always has the final LF, and > (2) even if we agree that using chop in Perl is a bad idea, such a > script has happily been working, and suddenly breaking it is a > regression no matter what. With respect to backwards compatibility, my thinking on the matter was basically: 1. Since --type=color was supposed to be a drop-in replacement for --get-color, it's a bug that they don't behave the same. 2. It's a fairly recent feature, so nobody really noticed the bug until recently (and it was in fact me who noticed and got annoyed by it). If it were an ancient behavior, we might think about retaining even bug compatibility, but that's not the case here. But I do admit your argument about consistency is giving me second thoughts. -Peff
Jeff King <peff@peff.net> writes: > I'm not sure I agree. Colors have always been special, and > "--type=color" was advertised as a replacement for "--get-color". And > "--get-color" never output the newline. OK, that part of the motivation I completely missed. If end-users and scripters are happy with the behaviour of --get-color that does not terminate its output with LF (which I think is a reasonable thing to do, as "color" is special in that context, i.e. having a dedicated --get option suitable for that type), as we advertise "--type=color" is the same as "--get-color" (only better), we need to special case it, and omitting LF at the end similarly does make sense. > With respect to backwards compatibility, my thinking on the matter was > basically: > > 1. Since --type=color was supposed to be a drop-in replacement for > --get-color, it's a bug that they don't behave the same. > > 2. It's a fairly recent feature, so nobody really noticed the bug > until recently (and it was in fact me who noticed and got annoyed > by it). If it were an ancient behavior, we might think about > retaining even bug compatibility, but that's not the case here. Now I think "we weren't consistent to begin with with --get-color, and treating --type=color as a special case is justifiable"; and I agree with the above two points. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Jeff King <peff@peff.net> writes: > >> I'm not sure I agree. Colors have always been special, and >> "--type=color" was advertised as a replacement for "--get-color". And >> "--get-color" never output the newline. > > OK, that part of the motivation I completely missed. If end-users > and scripters are happy with the behaviour of --get-color that does > not terminate its output with LF (which I think is a reasonable > thing to do, as "color" is special in that context, i.e. having a > dedicated --get option suitable for that type), as we advertise > "--type=color" is the same as "--get-color" (only better), we need > to special case it, and omitting LF at the end similarly does make > sense. > >> With respect to backwards compatibility, my thinking on the matter was >> basically: >> >> 1. Since --type=color was supposed to be a drop-in replacement for >> --get-color, it's a bug that they don't behave the same. >> >> 2. It's a fairly recent feature, so nobody really noticed the bug >> until recently (and it was in fact me who noticed and got annoyed >> by it). If it were an ancient behavior, we might think about >> retaining even bug compatibility, but that's not the case here. > > Now I think "we weren't consistent to begin with with --get-color, > and treating --type=color as a special case is justifiable"; and I > agree with the above two points. Just to avoid an awkward situation where the ball gets dropped and left on the floor forgotten, the above does not mean I am 100% happy with the patch as posted. There is no mention of --get-color anywhere, let alone it shows the ANSI sequence without traililng LF, which I would consider to be the most important part of the justification. It is much stronger than "I expected there won't be any trailing LF from 'git config'", which was the only thing I managed to read in the original and led to my response.
On Mon, Mar 04, 2019 at 02:05:40PM +0900, Junio C Hamano wrote: > >> With respect to backwards compatibility, my thinking on the matter was > >> basically: > >> > >> 1. Since --type=color was supposed to be a drop-in replacement for > >> --get-color, it's a bug that they don't behave the same. > >> > >> 2. It's a fairly recent feature, so nobody really noticed the bug > >> until recently (and it was in fact me who noticed and got annoyed > >> by it). If it were an ancient behavior, we might think about > >> retaining even bug compatibility, but that's not the case here. > > > > Now I think "we weren't consistent to begin with with --get-color, > > and treating --type=color as a special case is justifiable"; and I > > agree with the above two points. > > Just to avoid an awkward situation where the ball gets dropped and > left on the floor forgotten, the above does not mean I am 100% happy > with the patch as posted. There is no mention of --get-color > anywhere, let alone it shows the ANSI sequence without traililng LF, > which I would consider to be the most important part of the > justification. It is much stronger than "I expected there won't be > any trailing LF from 'git config'", which was the only thing I > managed to read in the original and led to my response. Yeah, I agree it needs to be the main justification in the commit message. I do wonder, though, if we're digging ourselves a hole with the inconsistency between different --types that will bite us later. Given that it's not that hard to chomp the output (and as you noted, the shell does it fairly transparently), and given that the caller has to switch between "--get-color" and "--type=color", it's not that hard to handle the output differently if you know to do so. Mostly I was just surprised by the new behavior. Perhaps the right solution is not a patch to the code, but to the documentation. Something like: diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt index 495bb57416..61f3a9cdd7 100644 --- a/Documentation/git-config.txt +++ b/Documentation/git-config.txt @@ -252,7 +252,9 @@ Valid `<type>`'s include: output. The optional `default` parameter is used instead, if there is no color configured for `name`. + -`--type=color [--default=<default>]` is preferred over `--get-color`. +`--type=color [--default=<default>]` is preferred over `--get-color` +(but note that `--get-color` will omit the trailing newline printed by +--type=color). -e:: --edit:: -Peff
Jeff King <peff@peff.net> writes: > I do wonder, though, if we're digging ourselves a hole with the > inconsistency between different --types that will bite us later. Given > that it's not that hard to chomp the output (and as you noted, the shell > does it fairly transparently), and given that the caller has to switch > between "--get-color" and "--type=color", it's not that hard to handle > the output differently if you know to do so. > > Mostly I was just surprised by the new behavior. Perhaps the right > solution is not a patch to the code, but to the documentation. Something > like: > > diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt > index 495bb57416..61f3a9cdd7 100644 > --- a/Documentation/git-config.txt > +++ b/Documentation/git-config.txt > @@ -252,7 +252,9 @@ Valid `<type>`'s include: > output. The optional `default` parameter is used instead, if > there is no color configured for `name`. > + > -`--type=color [--default=<default>]` is preferred over `--get-color`. > +`--type=color [--default=<default>]` is preferred over `--get-color` > +(but note that `--get-color` will omit the trailing newline printed by > +--type=color). > > -e:: > --edit:: Yup, that would be a very sensible first step, regardless of what the next step is. After that, choices are (1) we'd introduce new inconsistency among --type=<type> by matching what --type=color does to what --get-color does, to allow us to revert that documentation update, or (2) we'd drop LF from all --type=<type>, that makes everything consistent and risk breaking a few existing scripts while doing so, and get yelled at by end users, or (3) we stop at this documentation update and do nothing else.
Hi Peff, On Sun, 3 Mar 2019, Jeff King wrote: > On Sat, Mar 02, 2019 at 09:25:28PM +0100, Johannes Schindelin wrote: > > > > This bug has survived because there was never a test that would have > > > caught it. The old test used 'test_decode_color', which checks that its > > > input begins with a color, but stops parsing once it has parsed a single > > > color successfully. In this case, it was ignoring the trailing '\n'. > > > > Isn't the reason rather that our test compared the output to an expected > > text *with a newline*? IOW: > > [...] > > This should do the right thing if you write > > > > printf "<RED>" >expect > > No, there are actually two problems that cancel each other out. Because > test_decode_color() is implemented in awk, it doesn't see if there's > actually a newline or not in each record. Ah, so it is actually another instance of "we really need a precise test-tool command for this rather than a somewhat incomplete and fragile script" ;-) > So it _always_ adds a newline after printing out the line (and I don't > think Taylor's explanation above is quite right; it does have a loop to > handle multiple colors). > > So regardless of whether git-config is sending the newline or not, the > "actual" file will always have a newline in it. > > And then to match that, we used "echo" which of course has a newline, > and matches the test_decode_color output. > > So you're right that we need to switch to printf. But doing so without > dropping test_decode_color() would mean the test would always fail. We > have to printf the raw bytes, which is what the new test does. Fair enough. Ciao, Dscho
On Tue, Mar 05, 2019 at 02:57:32PM +0900, Junio C Hamano wrote: > Yup, that would be a very sensible first step, regardless of what > the next step is. > > After that, choices are > > (1) we'd introduce new inconsistency among --type=<type> by > matching what --type=color does to what --get-color does, to > allow us to revert that documentation update, or I suppose... though I think that if others agree, I'd rather update the documentation instead of introduce some inconsistency. Yes, there's an argument to be made that if we're encouraging users to go from '--get-color' -> '--type=color', that the two should behave the same, but I don't think the cost we pay for behavioral equivalence between the two is worth inconsistency among '--type=color' and all the rest. > (2) we'd drop LF from all --type=<type>, that makes everything > consistent and risk breaking a few existing scripts while doing > so, and get yelled at by end users, or As you indicate, I think that this option is one we should _not_ do. In the interpolation example you shared earlier in the thread, script writers most likely want and expect a trailing LF after each invocation of 'git config'. I'd argue that this case is more common than not wanting a LF when interpolating with `--type=color`, so I agree it seems the tradeoff here is not a good one. > (3) we stop at this documentation update and do nothing else. To restate my response to (1), I think that the documentation update in isolation makes the most sense here. I, too, was surprised in the same way that Peff was when we stumbled upon this, but I think that ultimately the consistency is most favorable. Thanks all for your discussion and feedback. Thanks, Taylor
Hi Johannes, On Sat, Mar 02, 2019 at 09:25:28PM +0100, Johannes Schindelin wrote: > Hi Taylor, > > On Fri, 1 Mar 2019, Taylor Blau wrote: > > > [ ... ] > > This should do the right thing if you write > > printf "<RED>" >expect > > instead? > > Ciao, > Dscho > > > + printf "\\033[31m" >expect && > > + git config --get --type=color foo.color >actual && > > test_cmp expect actual > > ' > > > > -- > > 2.20.1 Thank you for your suggestion. It occurred to me that this might be a preferable way to do things[1] after I sent the patch, but I am glad that you suggested it here explicitly. That said, I think that this patch has moved on from the code change, after some discussion between Junio and Peff, so I think that I will leave this (and the rest of the code changes) behind. Thanks, Taylor [1] ... preferable in the way that we don't have to write "\\033[31m" in the test.
Jeff King <peff@peff.net> writes: > Mostly I was just surprised by the new behavior. Perhaps the right > solution is not a patch to the code, but to the documentation. Something > like: Let me forge your sign-off and commit this to prevent us from forgetting. Thanks, all. -- >8 -- From: Jeff King <peff@peff.net> Date: Mon, 4 Mar 2019 23:20:51 -0500 Subject: [PATCH] config: document --type=color output is a complete line Even though the newer "--type=color" option to "git config" is meant to be upward compatible with the traditional "--get-color" option, unlike the latter, its output is not an incomplete line that lack the LF at the end. That makes it consistent with output of other types like "git config --type=bool". Document it, as it sometimes surprises unsuspecting users. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Documentation/git-config.txt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt index 1bfe9f56a7..611a32445c 100644 --- a/Documentation/git-config.txt +++ b/Documentation/git-config.txt @@ -240,7 +240,9 @@ Valid `<type>`'s include: output. The optional `default` parameter is used instead, if there is no color configured for `name`. + -`--type=color [--default=<default>]` is preferred over `--get-color`. +`--type=color [--default=<default>]` is preferred over `--get-color` +(but note that `--get-color` will omit the trailing newline printed by +--type=color). -e:: --edit::
On Thu, Mar 07, 2019 at 09:50:57AM +0900, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > Mostly I was just surprised by the new behavior. Perhaps the right > > solution is not a patch to the code, but to the documentation. Something > > like: > > Let me forge your sign-off and commit this to prevent us from > forgetting. > > Thanks, all. Thanks for tying this up. One minor nit: > diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt > index 1bfe9f56a7..611a32445c 100644 > --- a/Documentation/git-config.txt > +++ b/Documentation/git-config.txt > @@ -240,7 +240,9 @@ Valid `<type>`'s include: > output. The optional `default` parameter is used instead, if > there is no color configured for `name`. > + > -`--type=color [--default=<default>]` is preferred over `--get-color`. > +`--type=color [--default=<default>]` is preferred over `--get-color` > +(but note that `--get-color` will omit the trailing newline printed by > +--type=color). That final line probably should have literal quotes, like: `--type=color`). -Peff
diff --git a/builtin/config.c b/builtin/config.c index 98d65bc0ad..c8f088af38 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -258,7 +258,8 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value strbuf_setlen(buf, buf->len - 1); } } - strbuf_addch(buf, term); + if (type != TYPE_COLOR) + strbuf_addch(buf, term); return 0; } diff --git a/t/t1300-config.sh b/t/t1300-config.sh index 428177c390..ec1b3a852d 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -907,9 +907,8 @@ test_expect_success 'get --expiry-date' ' test_expect_success 'get --type=color' ' rm .git/config && git config foo.color "red" && - git config --get --type=color foo.color >actual.raw && - test_decode_color <actual.raw >actual && - echo "<RED>" >expect && + printf "\\033[31m" >expect && + git config --get --type=color foo.color >actual && test_cmp expect actual '
Invocations of 'git config <name>' print a trailing newline after writing the value assigned to the given configuration variable. This has an unexpected interaction with 63e2a0f8e9 (builtin/config: introduce `color` type specifier, 2018-04-09), which causes a newline to be printed after a color's ANSI escape sequence. In this case, printing a terminating newline is not desirable. Callers may want to print out such a configuration variable in a sub-shell in order to color something else, in which case they certainly don't want a newline. This bug has survived because there was never a test that would have caught it. The old test used 'test_decode_color', which checks that its input begins with a color, but stops parsing once it has parsed a single color successfully. In this case, it was ignoring the trailing '\n'. To do what callers expect, only print a newline when the type is not 'color', and print the escape sequence itself for an exact comparison. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- builtin/config.c | 3 ++- t/t1300-config.sh | 5 ++--- 2 files changed, 4 insertions(+), 4 deletions(-)