set_color with no arguments prints nothing

[faho]

This is essentially a re-run of #1335, which I assumed fixed just this but apparently didn't.

Apparently, a bare set_color will exit with no output.

Obviously, that's an issue if there's a cartesian product, like

echo (set_color $somevar)somestring

which in turn is a problem if a color happens to be undefined.

Instead, in this case it'd be nice if set_color always printed exactly one line. If not given any arguments, that means just a newline.

[ridiculousfish]

Makes sense to me!

[floam]

As mentioned on Gitter, the only potential problem would be stuff like (set_color $foo; set_color $bar), if perhaps they were colors that could be stacked ontop of each other. I think that's not really going to be a thing much with practical uses of our $fish_color_* variables. It could alternatively output some kind of escape or control character that is guaranteed to never take up any space on any kind of terminal (NUL?)

[faho]

Okay, I've now implemented this, and it broke the read.expect tests - they run with TERM=dumb (like all expect tests, to have fewer colors to match), because the default read prompt looks like:

#define DEFAULT_READ_PROMPT L"set_color green; echo -n read; set_color normal; echo -n \"> \""

And that idiom of doing set_color standalone shows up in a bunch of our sample prompts - https://github.com/fish-shell/fish-shell/search?p=2&q=set_color&unscoped_q=set_color.

So they'd break if we just changed this like I asked for.

So there's a few possibilities here:

• Explicitly separate the set_color output, with a special flag to not add a newline if it's not going into a command substitution (ugly)

• Always print a color sequence - e.g. sgr0 (which would then change color from the current version!) or a non-printable character? (Is there a noop terminfo sequence? My term seems to ignore \e\[m, but do all terms do that?)

One thing that seems to work is

streams.out.append(L"z\b");

i.e. print "a character" and then a backspace. This seems safe, since the user already is okay with us printing something, as long as it's not visible characters. So any character that always has a width of 1 should do. Also this is just completely invisible - it'll keep the color as it was, and AFAIK it shouldn't move to a newline and then not back.

Of course that still breaks assumptions about the "form" of the sequence - see e.g. 60f8eda.

[faho]

Oh, and since nobody commented on it:

control character that is guaranteed to never take up any space on any kind of terminal (NUL?)

@floam: The problem with NUL is that it would truncate arguments - echo (set_color $undef)bar would cause the argument to be \0bar, which means echo would read \0 and think the string is over.

Other control characters might work, but it seems prudent to restrict ourself to what's in ASCII, and that doesn't seem to have any fitting chars.

[floam]

There's actually a reverted commit that can help here. There used to be a ::ignore color and that would be a better noop for all these failure scenarios. The problem with exiting attributes mode is that sometimes the terminal can't even do that, and also resetting colors isn't right.

[faho]

There used to be a ::ignore color and that would be a better noop for all these failure scenarios

What did that do, though?

I mean we can make it so that set_color $undefined does set_color ::ignore, but what did that print?

If you did echo (set_color ::ignore)banana what was the output?

[floam]

Hm, actually, not sure about that, the 'ignore' color seems to have had no output.

[floam]

I think the DEL character would work.

[faho]

8f33b55 removed it, and if I'm reading it correctly it printed nothing, so we're back at the same problem.

I think the DEL character would work.

del and another char though?

If you did L"\x7f", then that'd remove a character that was supposed to show in the output, or am I missing something?

[floam]

I don't believe it will do anything. It should not affect output. It's only meaningful as input.

[floam]

printf foo"\x7f"foo

[floam]

Some research I did indicated that going back to before the vt100, there are two bare characters that are going to be treated as noops by the terminal: DEL and NUL. Since NUL is problematic, I'm leaning towards DEL. There are also a number of actual escapes that should be no-ops, but I worry about them causing garbage in funny situations.

[amiller27]

I'm coming here from this issue, which is caused by a closely related problem: set_color --help prints a lot of lines (as you'd expect), and using the output in a cartesian product a lot of times uses an exponential amount of memory that crashes the system. Not sure if that's a wontfix, or if it should be its own issue, but I wasn't sure if you were considering --help as part of the "it'd be nice if set_color always printed exactly one line". Even if this isn't something to be fixed by set_color, it seems like a problem that an unintended cartesian product can use all the memory on the system.

[floam]

I think the mistake there is builtin_print_help ought to be outputting to stderr.

[amiller27]

That would do it, but it doesn't seem like programs typically print --help to stderr; all the other programs I tried (non-fish-related) print --help to stdout.

[floam]

Most programs don't but I think a shell builtin doesn't want to have surprise output to stdout. So most shells do not have builtins that can take --help or -h at all. But ksh93 does, and they print to stderr.

$ ksh
$ echo $KSH_VERSION
Version AJM 93u+ 2012-08-01
$ typeset --help 2> /dev/null 
$ typeset --help > /dev/null
Usage: typeset [ options ] [name[=value]...]
   Or: typeset [ options ] -f [name...]
OPTIONS
  -a[type]        Indexed array. This is the default. If [type] is specified, each subscript is interpreted as a value of type type. The option value may be omitted.
  -b              Each name may contain binary data. Its value is the mime base64 encoding of the data. It can be used with -Z, to specify fixed sized fields.
  -f              Each of the options and names refers to a function.
...

Their builtins also take --man which prints out the formatted man page very much like fish does. Same thing, it's on stderr.

[IlanCosman]

I recently ran into this issue with IlanCosman/tide#21. The user had some program that set the TERM to dumb, which meant that set_color wouldn't print anything even if it's arguments were fine. Thus, there's no way for an author to guarantee that their command substitution set_color will work as expected. One must use the kludgy and annoying workaround of adding ; or echo to the end of every single command substitution set_color.

I've no idea as to the technical details of of which character to use, but fixing this would certainly improve quality of life for authors.

[clarfonthey]

Adding onto this, I ran into the issue with --bg being treated as an invalid argument and hence resulting in no output. Perhaps it should be added as an alias to --background?