builtin string: don't print final newline if it's missing from stdin

[krobelus]

A command like "printf nonewline | sed s/x/y/" does not print a
concluding newline, whereas "printf nnl | string replace x y" does.
This is an edge case -- usually the user input does have a newline at
the end -- but it seems still better for this command to just forward
the user's data.

Teach most string subcommands to check if stdin is missing the trailing
newline, and stop adding one in that case.
This does not apply when input is read from commandline arguments.

• Most subcommands stop adding the final newline, because they don't
  really care about newlines, so besides their normal processing,
  they just want to preserve user input. They are:

  • string collect
  • string escape/unescape
  • string join¹
  • string lower/upper
  • string pad
  • string replace
  • string repeat
  • string sub
  • string trim

• string match keeps adding the newline, following grep. Additionally,
  for string match --regex, it's important to output capture groups
  separated by newlines, resulting in multiple output lines for an
  input line. So it is not obvious where to leave out the newline.

• string split/split0 keep adding the newline for the same reason --
  they are meant to output a number of lines for each input line.

¹) string join0 is not changed because it already printed a zero byte
instead of the trailing newline. Maybe we should stop printing the
zero byte in this case?

Closes #3847

[faho]

string split/split0 keep adding the newline for the same reason --
they are meant to output a number of lines for each input line.

These are "explicitly split". The newline is added by the io logic - if it goes into a command substitution, the splits are used, if it goes elsewhere it adds newlines instead.

string join0 is not changed because it already printed a zero byte
instead of the trailing newline. Maybe we should stop printing the
zero byte in this case?

From my knowledge (and I did look into this back when I suggested automatically splitting on NULs) basically every tool that does NUL output adds a trailing NUL (the fancy way of saying this is it uses a NUL "terminator", not a "separator"). So this should probably be kept for interoperability.

---

(converted this to a draft because it is)

[krobelus]

string split/split0 keep adding the newline for the same reason -- they are meant to output a number of lines for each input line.

These are "explicitly split". The newline is added by the io logic - if it goes into a command substitution, the splits are used, if it goes elsewhere it adds newlines instead.

Right, maybe it's better to say "elements" instead of "lines".

string collect also uses explicit splitting but is changed here:

echo -n foo | string collect

will no longer print a trailing newline.

I'm happy with my changes now.
I assume it's fine to declare this as bug fix, only breaking questionable code.

[zanchey]

Do you want to take this for 3.4.0?

[krobelus]

if we agree on the behavior change, yes

[floam]

If changing a builtin, yesterday is always going to be the best time to do it; I'd want it for 3.4.0.

[zanchey]

Based on the discussion in #3847 I think it's worth doing

[faho]

Based on the discussion in #3847 I think it's worth doing

I'm too scared to. I think there's a potential for fallout here, considering that string is rather widely used.

I recommend punting this to after 3.4 if that's coming soon, because we've not had enough chance to test.

[krobelus]

Unfortunately there's no good way to roll out this change safely, whether
we wait or not. Adding a feature flag would work but only by pushing the
decision to users (and it's probably not worth the feature flag).

I personally mostly use string for interactive commands, but I guess many
users are writing scripts.

[faho]

Unfortunately there's no good way to roll out this change safely, whether
we wait or not.

Let's at least test it in master for a bit. As it appears to me 3.4 is imminent, so rushing this in now feels like a bad idea.

There is no real need to fast-track this, it's not an important bug fix for something that's bugging a lot of people, it's just somewhat nicer behavior for a builtin. It can wait for a cycle.

---

And yes, we could use more testing of master. It would be great if we had a lot of dedicated users who frequently ran it, especially with things like oh-my-fish and fisher, which the contributors typically don't (and the fisher developers don't run master, and the oh-my-fish developers are quite absent in general).

But we do find regressions in master, and this thing has a potential for regressions, especially with things like oh-my-fish and fisher plugins.

[faho]

Okay I've merged this now for 3.5.0 so we can get some feel for it.