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

`source` reads from stdin (was: fish hangs up after command substitution failed during initialization) #2633

Closed
imcaizheng opened this Issue Dec 24, 2015 · 10 comments

Comments

Projects
None yet
4 participants
@imcaizheng
Copy link

imcaizheng commented Dec 24, 2015

I tried to put source (_fish_config_dir)alias.fish in my local configuration file.
After that fish complains an error and freezes every time I open a new terminal. Both Ctrl-D and Ctrl-C don't work.

I had copied the complained text as showed below:

fish: Unknown command '_fish_config_dir'
~/.config/fish/config.fish (line 1): _fish_config_dir
                                     ^
in command substitution
    called on line -1 of file ~/.config/fish/config.fish

from sourcing file ~/.config/fish/config.fish
    called during startup

I do have a function named __fish_config_dir in the functions directory and just made a typo. Fortunately I fixed the typo with a GUI text editor and everything goes well again.

Seems that it is a bug from the source command because fish would not be hanging up but complains error after I put echo (command_not_exists) in the local configuration file.

fish version: fish, version 2.2.0-473-g8a6f26f-dirty

@faho

This comment has been minimized.

Copy link
Member

faho commented Dec 24, 2015

Well, the issue here is that source waits for input because it was given no file. You'll get the same issue with cat.

I'm not sure how or if this should be fixed.

@faho

This comment has been minimized.

Copy link
Member

faho commented Dec 24, 2015

Out of curiosity - why a function? Why not a variable?

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Dec 24, 2015

This seems like it should be a bug. I know the documentation says that if no filename is specified stdin will be used. But wouldn't it be better to require the user be explicit about their desire to source stdin by requiring the use of the "-" pseudo-filename? This would also protect against this very scenario. Even if a var was being used instead of a function mistyping the var name will result in the entire argument being removed, without so much as an error being reported, leaving source reading unexpectedly from stdin.

The other question is whether source should be interruptible. I vote yes especially if we continue to allow a bare source command because someone who accidentally types that is going to expect [ctrl-C] to return them to a shell prompt. Also, if someone does type a bare cat then [ctrl-C] will kill it and return the user to a shell prompt.

@faho

This comment has been minimized.

Copy link
Member

faho commented Dec 24, 2015

Even if a var was being used instead of a function mistyping the var name will result in the entire argument being removed, without so much as an error being reported, leaving source reading unexpectedly from stdin.

Quite possibly source should only use stdin if it's not a tty - since I can't see a use case for that. In other, more code-y words:

> source
source: You can just enter the commands you wish to source directly since you are already typing on this terminal
> echo "echo banana" | source
banana
> source (_fish_NONEXISTENT_FUNCTION)
fish: Unknown command '_fish_NONEXISTENT_FUNCTION
source: You can just enter the commands you wish to source directly since you are already typing on this terminal

Or maybe tie it to interactivity?

I vote yes especially if we continue to allow a bare source command because someone who accidentally types that is going to expect [ctrl-C] to return them to a shell prompt.

Currently source will only exit on ctrl-D (standard keychord to denote EOF) by executing everything it's been given. I agree that it should also react to ctrl-C (by stopping immediately, not executing anything), since that's the more usual keychord to stop something and I can't see a use for it reading ctrl-C.

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Dec 24, 2015

Tying the ability to use a bare source to interactivity (by which I'm guessing you mean testing the is_interactive_shell var) wouldn't really help. But I like the idea of requiring that stdin not be a tty to use a bare source. Having said that I'm still think requiring an explicit "-" is preferable and safer. Consider the case of someone running a fish script with stdin redirected from a file and that script uses source with a typo of the sort being discussed. Oops! That bare source command will now unexpectedly gobble the rest of stdin. I think this feature is more dangerous than the utility of saving two characters merits.

@faho faho added the bug label Dec 24, 2015

@faho faho added this to the next-major milestone Dec 24, 2015

@faho

This comment has been minimized.

Copy link
Member

faho commented Dec 24, 2015

That bare source command will now unexpectedly gobble the rest of stdin.

Good point. This would also occur for fish functions, meaning the interactivity check wouldn't help.

I think this feature is more dangerous than the utility of saving two characters merits.

So you'd disallow bare source entirely, i.e. source without arguments fails instead of reading stdin?

One thing that would improve this is #1246 - by failing in this particular case before executing source. Of course the issue would still be hit if an expansion (comsub or variable) comes up empty or the user just forgets the argument.

So, there's three things to be done here:

  • Implement 1246
  • Make source abort on ctrl-c, since that's more common (and actually an option to abort without executing)
  • Make reading from stdin more explicit for source (via a "-" as filename or a "--stdin" flag) so this becomes harder to accidentally hit (a compatibility break, of course - at least eval.fish would need to be adjusted)
@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Dec 25, 2015

So you'd disallow bare source entirely, i.e. source without arguments fails instead of reading stdin?

Yes. For all the reasons I already mentioned plus two more: It doesn't even make an improvement to the interactive user experience, and it's redundant. I agree with your three action items. Whether or not a --stdin flag should be added in addition to supporting "-" is something I'm agnostic about. It's redundant but it does improve readability.

I dislike breaking backward compatibility but in this instance I'm betting (well, hoping) there are extremely few uses of the feature. If people believe this will affect more than a handful of users we could do it in two steps. Step 1: disallow a bare source if stdin is not a tty and update the documentation to make it clear that x months later a bare source won't be legal. Step 2: When that milestone is reached disable the feature completely.

@imcaizheng

This comment has been minimized.

Copy link

imcaizheng commented Dec 25, 2015

Out of curiosity - why a function? Why not a variable?

I found that If I wrapped a pathname in quotes, like:

set fish_config_dir "~/.config/fish"

the tilde wouldn't be expanded.

That's why I would have to make use of the echo command for tilde expansion.

> functions __fish_config_dir
function __fish_config_dir
    echo ~/.config/fish/
end

But a few hours ago after doing some searching on Google I knew that the proper pattern is without quotes:

set fish_config_dir ~/.config/fish

It is more succinct than defining a function. Great!

@faho faho changed the title fish hangs up after command substitution failed during initialization `source` reads from stdin (was: fish hangs up after command substitution failed during initialization) Jul 14, 2016

@krader1961 krader1961 added enhancement and removed bug labels Feb 9, 2017

@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented May 22, 2018

This can also happen if executing source $foo/bar where $foo is not defined (another good reason to fix #4961).

I think the discussion is missing the fact that there's an actual underlying bug wherein source can't read from stdin when called during startup. As @imcaizheng says, ctrl+d doesn't end source, either.

@faho faho closed this in 1d5e715 Oct 22, 2018

@faho

This comment has been minimized.

Copy link
Member

faho commented Oct 22, 2018

I have now decided to fix this by requiring an explicit "-" to read from the terminal.

The solution I went with has source exit with an error status, but no message.

@faho faho modified the milestones: next-major, fish-3.0 Oct 22, 2018

ridiculousfish added a commit to ridiculousfish/fish-shell that referenced this issue Nov 24, 2018

source: Return error instead of implicitly reading from tty
For things like

    source $undefined

or
    source (nooutput)

it was quite annoying that it read from tty.

Instead we now require a "-" as the filename to read from the tty.

This does not apply to reading from stdin if it's redirected, so

    something | source

still works.

Fixes fish-shell#2633.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment