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

kill the `IFS` variable #4156

Closed
krader1961 opened this Issue Jun 22, 2017 · 9 comments

Comments

Projects
None yet
2 participants
@krader1961
Copy link
Contributor

krader1961 commented Jun 22, 2017

The discussion about doing a major release of fish prompted someone to add a comment to an issue targeted for that major release. Specifically, they said that having to set IFS to get the desired behavior isn't a big deal. To which I replied:

Also, it should never be necessary to manipulate IFS in fish script. That var should die and removing it is something else we should consider for a major release. Whenever I have needed to, or seen it used to, change how strings are split into words in other shells (ksh, bash, zsh) it always made me want to vomit. It is the poster child for what is wrong with POSIX 1003 compatible shells.

So I'm opening this issue to ask that we consider doing so in the next fish major release.

@krader1961 krader1961 added this to the next-major milestone Jun 22, 2017

@faho

This comment has been minimized.

Copy link
Member

faho commented Jun 22, 2017

So, there's two ways we're using $IFS currently:

  • Set it to something for use with read - e.g. set IFS '=' and then while read -l key value to read from an INI-style file

  • Unset it or set it to empty to avoid splitting command substitutions (set IFS; set var (cat somefile)) - note that this only cares if IFS is unset or empty, not what it is set to (set IFS '=' does not split command substitutions on '=').

The former can currently already be replaced by just reading the line and then set line (string split '=' -- $line); set key $line[1]; set value $line[2]. It could be done easier if read had an option to specify the separator - something | while read -l --separator = key value.

The second could be solved by implementing "$()", but that still wouldn't make it easy to split on arbitrary separators but not newlines.

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Jun 22, 2017

Faho's first example, as they noted, can be handled by explicitly using something like string split on the data that has been read. Or, better, adding a read --separator flag. Manipulating IFS in that situation is dangerous since in POSIX 1003 shells it affects not just how read splits its input but how every string is tokenized into words.

There are only two places in fish where we unset IFS to get the second behavior noted by faho. One is __fish_urlencode which will die when I merge the fix for #4150 in the next day or two. The second is funced and that would be addressed by implementing "$()".

@faho

This comment has been minimized.

Copy link
Member

faho commented Jun 23, 2017

Or, better, adding a read --separator flag.

I have some initial work on that. I'm calling it "--delimiter" (and "-d"), though, both because "-s" is already taken and for symmetry with things like cut.

@krader1961 krader1961 modified the milestones: fish-3.0, next-major Jun 23, 2017

@faho faho referenced this issue Jun 23, 2017

Closed

Add `read --delimiter` #4160

2 of 3 tasks complete
@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Jun 26, 2017

In case it isn't obvious: When I say "kill the IFS variable" I only mean that no core fish C++ code or fish script will be affected by it. The user can still set it and export it to affect non-fish sub-shells. Although why you would ever do so is a mystery. This change is to make it even clearer that fish is not a POSIX 1003 shell with all the extremely weird string tokenization rules implied by that standard.

That fish currently uses IFS in a couple of situations (e.g., the read command) is unfortunate. It saddens me that the people working on fish long before I came on the scene recognized how awful POSIX 1003 was but didn't make a clean break from it. However, I may have made the same decision given the need to be practical rather than pure and given the available developer resources. Hindsight is 20-20.

@faho

This comment has been minimized.

Copy link
Member

faho commented Jun 26, 2017

In case it isn't obvious: When I say "kill the IFS variable" I only mean that no core fish C++ code or fish script will be affected by it. The user can still set it and export it to affect non-fish sub-shells.

Of course. Though since IFS isn't exported right now, we would actually remove it completely from fish.

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Jul 25, 2017

Set it to something for use with read - e.g. set IFS '=' and then while read -l key value to read from an INI-style file

Note that this is readily dealt with using our string builtin. For example, share/completions/dd.fish had that IFS pattern:

            set -l IFS =
            echo $operand_string | read -l operand value

I replaced that with this:

string replace = ' ' -- $operand_string | read -l operand complete
@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Jul 25, 2017

Also, unsetting IFS to avoid splitting command substitutions can be handled by replacing

set -e IFS
set var (command_sub)

with

command_sub | read -lz var

Since the command substitution should not be producing null delimited output that is equivalent to the first idiom. So while there are good reasons to introduce things like read --delim=x and $() they aren't necessary to eliminate our uses of IFS.

krader1961 added a commit that referenced this issue Jul 25, 2017

remove some uses of `$IFS`
This is a step towards resolving issue #4156. It replaces uses of `$IFS`
with other solutions.

krader1961 added a commit to krader1961/fish-shell that referenced this issue Jul 25, 2017

remove some uses of `$IFS`
This is a step towards resolving issue fish-shell#4156. It replaces uses of `$IFS`
with other solutions.

krader1961 added a commit to krader1961/fish-shell that referenced this issue Jul 25, 2017

Another step towards resolving issue fish-shell#4156
Remove another couple of references to `$IFS` in core fish script.

krader1961 added a commit that referenced this issue Jul 25, 2017

remove some uses of `$IFS`
This is a step towards resolving issue #4156. It replaces uses of `$IFS`
with other solutions.
@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Jul 25, 2017

I've removed all the IFS uses from our completions and functions. That just leaves modifying the read command, its tests, and documentation to make this a done deal.

faho added a commit to faho/fish-shell that referenced this issue Jul 27, 2017

Implement `read --delimiter`
This takes a string that is then split upon like `string split`.

Unlike $IFS, the string is used as one piece, not a set of characters.

There is still a fallback to IFS if no delimiter is given, that
behaves exactly as before.

Work towards fish-shell#4156.

@faho faho referenced this issue Jul 27, 2017

Closed

Add `read --delimiter` #4256

2 of 3 tasks complete

faho added a commit that referenced this issue Jul 28, 2017

Implement `read --delimiter`
This takes a string that is then split upon like `string split`.

Unlike $IFS, the string is used as one piece, not a set of characters.

There is still a fallback to IFS if no delimiter is given, that
behaves exactly as before.

Fixes #4156.
@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Aug 2, 2017

Okay, we've done 95% of the work. The remaining use in read is needed during a transition period. We can open a new issue in the 3.2 or 3.3 timeframe to discuss removing the remaining fallback to IFS.

@krader1961 krader1961 closed this Aug 2, 2017

jschank referenced this issue in oh-my-fish/theme-bobthefish Dec 24, 2017

@faho faho referenced this issue Feb 1, 2018

Closed

Split command substitutions on NUL if one is found #4624

1 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment