Skip to content
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

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

Merged
merged 2 commits into from Mar 13, 2022

Conversation

krobelus
Copy link
Member

@krobelus krobelus commented Nov 22, 2021

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
Copy link
Member

faho commented Nov 23, 2021

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 krobelus force-pushed the string-preserve-missing-newline branch from cf9bacb to c1a1ee1 Compare November 27, 2021 18:08
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 multiple elements for a single input line.

¹) string join0 is not changed because it already printed a trailing
   zero byte instead of the trailing newline. This is consistent
   with other tools like "find -print0".

Closes fish-shell#3847
@krobelus krobelus force-pushed the string-preserve-missing-newline branch from c1a1ee1 to 745129e Compare November 27, 2021 18:12
@krobelus
Copy link
Member Author

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.

@krobelus krobelus marked this pull request as ready for review November 27, 2021 18:13
@krobelus krobelus changed the title Draft: builtin string: don't print final newline if it's missing from stdin builtin string: don't print final newline if it's missing from stdin Nov 29, 2021
@zanchey
Copy link
Member

zanchey commented Dec 11, 2021

Do you want to take this for 3.4.0?

@krobelus
Copy link
Member Author

if we agree on the behavior change, yes

@floam
Copy link
Member

floam commented Dec 11, 2021

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
Copy link
Member

zanchey commented Dec 15, 2021

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

@zanchey zanchey added this to the fish 3.4.0 milestone Dec 15, 2021
@zanchey zanchey added enhancement release notes Something that is or should be mentioned in the release notes labels Dec 15, 2021
@faho
Copy link
Member

faho commented Dec 15, 2021

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
Copy link
Member Author

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
Copy link
Member

faho commented Dec 15, 2021

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.

@zanchey zanchey modified the milestones: fish 3.4.0, fish-future Dec 16, 2021
@faho faho merged commit 9172ab5 into fish-shell:master Mar 13, 2022
@faho faho modified the milestones: fish-future, fish 3.5.0 Mar 13, 2022
@faho
Copy link
Member

faho commented Mar 13, 2022

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

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement release notes Something that is or should be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

should string be modified to not append a newline to its final line of output?
4 participants