New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

header values also shown in bold #2736

Closed
l2dy opened this Issue Jul 12, 2018 · 22 comments

Comments

Projects
None yet
10 participants
@l2dy

l2dy commented Jul 12, 2018

I did this

curl -I https://curl.haxx.se
screen shot 2018-07-12

I expected the following

bold-headers-terminal

curl/libcurl version

curl 7.61.0 (x86_64-apple-darwin17.7.0) libcurl/7.61.0 OpenSSL/1.0.2o zlib/1.2.11 libidn2/2.0.5 libpsl/0.20.2 (+libidn2/2.0.5) nghttp2/1.32.0
Release-Date: 2018-07-11
Protocols: dict file ftp ftps gopher http https imap imaps pop3 pop3s rtsp smb smbs smtp smtps telnet tftp 
Features: AsynchDNS IDN IPv6 Largefile NTLM NTLM_WB SSL libz TLS-SRP HTTP2 UnixSockets HTTPS-proxy PSL 

operating system

macOS 10.13.6, curl installed from MacPorts, shell is fish.

@bagder bagder added the cmdline tool label Jul 12, 2018

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Jul 12, 2018

Member

It seems the mac terminal doesn't do "bold off" so it sticks with bold! This is independent of what shell that's used.

Member

bagder commented Jul 12, 2018

It seems the mac terminal doesn't do "bold off" so it sticks with bold! This is independent of what shell that's used.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Jul 12, 2018

Member

An alternative approach that probably is more portable is to switch off all "styles":

diff --git a/src/tool_cb_hdr.c b/src/tool_cb_hdr.c
index 88ce5e13b..d317b9f39 100644
--- a/src/tool_cb_hdr.c
+++ b/src/tool_cb_hdr.c
@@ -40,11 +40,11 @@ static char *parse_filename(const char *ptr, size_t len);
 #ifdef WIN32
 #define BOLD
 #define BOLDOFF
 #else
 #define BOLD "\x1b[1m"
-#define BOLDOFF "\x1b[21m"
+#define BOLDOFF "\x1b[0m"
 #endif
 
 /*
 ** callback for CURLOPT_HEADERFUNCTION
 */
Member

bagder commented Jul 12, 2018

An alternative approach that probably is more portable is to switch off all "styles":

diff --git a/src/tool_cb_hdr.c b/src/tool_cb_hdr.c
index 88ce5e13b..d317b9f39 100644
--- a/src/tool_cb_hdr.c
+++ b/src/tool_cb_hdr.c
@@ -40,11 +40,11 @@ static char *parse_filename(const char *ptr, size_t len);
 #ifdef WIN32
 #define BOLD
 #define BOLDOFF
 #else
 #define BOLD "\x1b[1m"
-#define BOLDOFF "\x1b[21m"
+#define BOLDOFF "\x1b[0m"
 #endif
 
 /*
 ** callback for CURLOPT_HEADERFUNCTION
 */

bagder added a commit that referenced this issue Jul 12, 2018

header output: switch off all styles, not just unbold
... the "unbold" sequence doesn't work on the mac Terminal.

Fixes #2736

bagder added a commit that referenced this issue Jul 12, 2018

header output: switch off all styles, not just unbold
... the "unbold" sequence doesn't work on the mac Terminal.

Reported-by: Zero King
Fixes #2736
Closes #2738

@bagder bagder closed this in a82372e Jul 16, 2018

@grawity

This comment has been minimized.

Show comment
Hide comment
@grawity

grawity Jul 17, 2018

For the record, the more common unbold sequence is \e[22m. That's the one found in Xterm's documentation for example. I don't know where \e[21m came from. Resetting all styles is even more reliable, I guess.

grawity commented Jul 17, 2018

For the record, the more common unbold sequence is \e[22m. That's the one found in Xterm's documentation for example. I don't know where \e[21m came from. Resetting all styles is even more reliable, I guess.

@eli-schwartz

This comment has been minimized.

Show comment
Hide comment
@eli-schwartz

eli-schwartz Jul 17, 2018

Using guake on Linux (vte) I was getting lots of double-underlined everything, which bled into the prompt after curl was finished: https://paste.xinu.at/MeNlxzN/

(So, not just macOS.)

But this commit fixes it.

eli-schwartz commented Jul 17, 2018

Using guake on Linux (vte) I was getting lots of double-underlined everything, which bled into the prompt after curl was finished: https://paste.xinu.at/MeNlxzN/

(So, not just macOS.)

But this commit fixes it.

@lotheac

This comment has been minimized.

Show comment
Hide comment
@lotheac

lotheac Jul 18, 2018

@grawity xterm's documentation is actually at http://invisible-island.net/xterm/ctlseqs/ctlseqs.html (and it documents \e[21m too):

CSI Pm m  Character Attributes (SGR).
        Ps = 2 1  -> Doubly-underlined (ECMA-48).

lotheac commented Jul 18, 2018

@grawity xterm's documentation is actually at http://invisible-island.net/xterm/ctlseqs/ctlseqs.html (and it documents \e[21m too):

CSI Pm m  Character Attributes (SGR).
        Ps = 2 1  -> Doubly-underlined (ECMA-48).
@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Jul 20, 2018

Member

A fix has already landed! We don't need more details on the error but we could use some confirmations of the fix...

Member

bagder commented Jul 20, 2018

A fix has already landed! We don't need more details on the error but we could use some confirmations of the fix...

@eli-schwartz

This comment has been minimized.

Show comment
Hide comment
@eli-schwartz

eli-schwartz Jul 20, 2018

But this commit fixes it.

eli-schwartz commented Jul 20, 2018

But this commit fixes it.

xquery added a commit to xquery/curl that referenced this issue Aug 9, 2018

header output: switch off all styles, not just unbold
... the "unbold" sequence doesn't work on the mac Terminal.

Reported-by: Zero King
Fixes #2736
Closes #2738
@kbabioch

This comment has been minimized.

Show comment
Hide comment
@kbabioch

kbabioch Aug 22, 2018

I can confirm that this commit fixes the issue. It messed up my terminal badly (double underline). There are bugs in openSUSE and Arch Linux also ships this as patch already. Potentially a lot of (Linux?) users are affected. You might consider to do a new release containing this fix, since this a bad bug from the user experience point of view.

kbabioch commented Aug 22, 2018

I can confirm that this commit fixes the issue. It messed up my terminal badly (double underline). There are bugs in openSUSE and Arch Linux also ships this as patch already. Potentially a lot of (Linux?) users are affected. You might consider to do a new release containing this fix, since this a bad bug from the user experience point of view.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Aug 22, 2018

Member

This will be in the next release; 7.61.1 is planned to ship on September 5.

Member

bagder commented Aug 22, 2018

This will be in the next release; 7.61.1 is planned to ship on September 5.

@eli-schwartz

This comment has been minimized.

Show comment
Hide comment
@eli-schwartz

eli-schwartz Aug 22, 2018

There are bugs in openSUSE

There is one bug, which you opened and then backported this fix for.

The intersection of people who actually have an up-to-date curl, actually use -I, and don't use one of the two rolling release distros that backported this patch, is probably sufficiently low that it may not be worth an atypical-for-this-project point release just for that.

eli-schwartz commented Aug 22, 2018

There are bugs in openSUSE

There is one bug, which you opened and then backported this fix for.

The intersection of people who actually have an up-to-date curl, actually use -I, and don't use one of the two rolling release distros that backported this patch, is probably sufficiently low that it may not be worth an atypical-for-this-project point release just for that.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Aug 22, 2018

Member

And quite honestly: we have not gotten this reported very much and masses of users have not came shouting with pitchforks in their hands. My impression is that this bug has not hurt people that badly.

Had there been a constant flood of complaints, remarks and questions about this then maybe we would have considered a more rushed patch release.

Now, we're just two weeks off from the date where we already planned the next release to happen...

Member

bagder commented Aug 22, 2018

And quite honestly: we have not gotten this reported very much and masses of users have not came shouting with pitchforks in their hands. My impression is that this bug has not hurt people that badly.

Had there been a constant flood of complaints, remarks and questions about this then maybe we would have considered a more rushed patch release.

Now, we're just two weeks off from the date where we already planned the next release to happen...

@75lb

This comment has been minimized.

Show comment
Hide comment
@75lb

75lb Aug 24, 2018

My impression is that this bug has not hurt people that badly.

It's an annoyance but one I can live with until September. Any invocation of curl -I or curl -i leaves my mac terminal font in a bold state but i can reset that with a quick invocation of ls. Tbh, i'm more surprised no-one tested this feature on a mac before it hit production.

75lb commented Aug 24, 2018

My impression is that this bug has not hurt people that badly.

It's an annoyance but one I can live with until September. Any invocation of curl -I or curl -i leaves my mac terminal font in a bold state but i can reset that with a quick invocation of ls. Tbh, i'm more surprised no-one tested this feature on a mac before it hit production.

@lotheac

This comment has been minimized.

Show comment
Hide comment
@lotheac

lotheac Aug 24, 2018

lotheac commented Aug 24, 2018

@75lb

This comment has been minimized.

Show comment
Hide comment
@75lb

75lb Aug 24, 2018

Here's a screenshot to illustrate what I was talking about above. The terminal is iTerm2 on macOS 10.13.6.

screen shot 2018-08-24 at 12 51 55

75lb commented Aug 24, 2018

Here's a screenshot to illustrate what I was talking about above. The terminal is iTerm2 on macOS 10.13.6.

screen shot 2018-08-24 at 12 51 55

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Aug 24, 2018

Member

i'm more surprised no-one tested this feature on a mac before it hit production.

I did. But I just happened to be using a terminal that supported that escape sequence.

But yes, I think we can draw the conclusion that not too many users are trying out non-release versions of curl ahead of time...

Member

bagder commented Aug 24, 2018

i'm more surprised no-one tested this feature on a mac before it hit production.

I did. But I just happened to be using a terminal that supported that escape sequence.

But yes, I think we can draw the conclusion that not too many users are trying out non-release versions of curl ahead of time...

@75lb

This comment has been minimized.

Show comment
Hide comment
@75lb

75lb Aug 24, 2018

@lotheac thanks for the useful tip!

@bagder Ok, thanks. I forgot to mention: i'm using the brew deployment of curl which is stable 7.61.0.

75lb commented Aug 24, 2018

@lotheac thanks for the useful tip!

@bagder Ok, thanks. I forgot to mention: i'm using the brew deployment of curl which is stable 7.61.0.

@kilobyte

This comment has been minimized.

Show comment
Hide comment
@kilobyte

kilobyte Sep 8, 2018

It's a regression in libvte 0.51.2; this was mindlessly changed following a piece of xterm's docs that even xterm itself doesn't support. I've filed a bug there.

Obviously, for programs that run in various terminals like curl, there's no way but to conform to the lowest common denomination and not use problematic sequences.

kilobyte commented Sep 8, 2018

It's a regression in libvte 0.51.2; this was mindlessly changed following a piece of xterm's docs that even xterm itself doesn't support. I've filed a bug there.

Obviously, for programs that run in various terminals like curl, there's no way but to conform to the lowest common denomination and not use problematic sequences.

falconindy added a commit to falconindy/curl that referenced this issue Sep 10, 2018

header output: switch off all styles, not just unbold
... the "unbold" sequence doesn't work on the mac Terminal.

Reported-by: Zero King
Fixes #2736
Closes #2738
@ilatypov

This comment has been minimized.

Show comment
Hide comment
@ilatypov

ilatypov Sep 12, 2018

Did not termcap and then terminfo allow to abstract away from terminal interpreters? This was available for at least 15 years but I don't know how hard it would be to use terminfo. Perhaps, ncurses uses it.

ilatypov commented Sep 12, 2018

Did not termcap and then terminfo allow to abstract away from terminal interpreters? This was available for at least 15 years but I don't know how hard it would be to use terminfo. Perhaps, ncurses uses it.

@kilobyte

This comment has been minimized.

Show comment
Hide comment
@kilobyte

kilobyte Sep 12, 2018

@ilatypov: nope, termcap stopped being really maintained in early 1980s — it was absolutely required in pre-vt100 days when terminals used completely incompatible codes, but has petered down once everything became vt100-compatible. With no urgent need, OS vendors stopped updating their databases. Case in point: Solaris still doesn't understand TERM=linux. For this reason, terminals can't have unique identifiers: any other than linux [console] and rxvt clones declare to be "xterm" or "xterm-256color" (eg. xterm, libvte (most GTK-using terminals), putty. osso-xterm, qterm, cool-retro-term, konsole, Windows 10 console, ...), despite having much different coverage of features. And then, serial console/IPMI/lxc console/etc have no means to pass the TERM variable to the programs inside. With meaningless or no $TERM, termcap/terminfo can't possibly be useful.

And in this particular case, it wouldn't help: beside knowing it's "vte", you'd also need version number as \e[21m got changed.

kilobyte commented Sep 12, 2018

@ilatypov: nope, termcap stopped being really maintained in early 1980s — it was absolutely required in pre-vt100 days when terminals used completely incompatible codes, but has petered down once everything became vt100-compatible. With no urgent need, OS vendors stopped updating their databases. Case in point: Solaris still doesn't understand TERM=linux. For this reason, terminals can't have unique identifiers: any other than linux [console] and rxvt clones declare to be "xterm" or "xterm-256color" (eg. xterm, libvte (most GTK-using terminals), putty. osso-xterm, qterm, cool-retro-term, konsole, Windows 10 console, ...), despite having much different coverage of features. And then, serial console/IPMI/lxc console/etc have no means to pass the TERM variable to the programs inside. With meaningless or no $TERM, termcap/terminfo can't possibly be useful.

And in this particular case, it wouldn't help: beside knowing it's "vte", you'd also need version number as \e[21m got changed.

@RalphCorderoy

This comment has been minimized.

Show comment
Hide comment
@RalphCorderoy

RalphCorderoy Sep 14, 2018

terminfo is still very useful and much used, e.g. screen(1) explains how to use it to coordinate the multiple terminal types involved. And personal terminfo entries can workaround buggy or unwanted features; I have ~/.terminfo/x/... and ~/.terminfo is in the default search path so it just works. IMO programs assuming the world is now all the same for terminal implementations are annoying, and stopping development of better terminal definitions with protocols that are faster to parse.

RalphCorderoy commented Sep 14, 2018

terminfo is still very useful and much used, e.g. screen(1) explains how to use it to coordinate the multiple terminal types involved. And personal terminfo entries can workaround buggy or unwanted features; I have ~/.terminfo/x/... and ~/.terminfo is in the default search path so it just works. IMO programs assuming the world is now all the same for terminal implementations are annoying, and stopping development of better terminal definitions with protocols that are faster to parse.

@kilobyte

This comment has been minimized.

Show comment
Hide comment
@kilobyte

kilobyte Sep 14, 2018

Then what would you propose to let terminfo assign different properties to xterm vs libvte vs putty vs qterm vs konsole vs Windows10 vs...? They all have an identical value of $TERM.

kilobyte commented Sep 14, 2018

Then what would you propose to let terminfo assign different properties to xterm vs libvte vs putty vs qterm vs konsole vs Windows10 vs...? They all have an identical value of $TERM.

@RalphCorderoy

This comment has been minimized.

Show comment
Hide comment
@RalphCorderoy

RalphCorderoy Sep 14, 2018

I would propose the long-held standard. That programs dutifully follow the user's choice of TERM and don't assume escape codes. If a bunch of different terminal implementations all strive to match one definition and set that as the default doesn't mean TERM can't be altered when the user knows they're incomplete, buggy, or annoying.

RalphCorderoy commented Sep 14, 2018

I would propose the long-held standard. That programs dutifully follow the user's choice of TERM and don't assume escape codes. If a bunch of different terminal implementations all strive to match one definition and set that as the default doesn't mean TERM can't be altered when the user knows they're incomplete, buggy, or annoying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment