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

funced doesn't work if $EDITOR is more than a single word #5625

Closed
lilyball opened this issue Feb 7, 2019 · 9 comments
Closed

funced doesn't work if $EDITOR is more than a single word #5625

lilyball opened this issue Feb 7, 2019 · 9 comments
Labels
bug
Milestone

Comments

@lilyball
Copy link
Contributor

@lilyball lilyball commented Feb 7, 2019

My $EDITOR variable is a full job, not just a single word. This works in most contexts, but funced doesn't seem to support this.

> echo $EDITOR
mvim -f -c "au VimLeave * !open -a Terminal"
> funced foo
fish: Unknown command ''mvim -f -c "au VimLeave * !open -a Terminal"''
/usr/local/share/fish/functions/funced.fish (line 97): 
        if not $editor $tmpname
               ^
in function 'funced'
	called on standard input
	with parameter list 'foo'

Editor exited but the function was not modified

fish, version 3.0.0-21-g742fde0d

@lilyball lilyball added the bug label Feb 7, 2019
@lilyball
Copy link
Contributor Author

@lilyball lilyball commented Feb 7, 2019

I'm pretty sure this used to work. I'm guessing it broke when funced learned the -s flag.

@faho
Copy link
Member

@faho faho commented Feb 7, 2019

This works if you set $EDITOR as a list - set EDITOR nvim -f -c "au VimLeave * !open -a Terminal" should work.

The reason is that we now use the variable-as-a-command (if not $editor) instead of eval, which would of course expand the input again.

I feel like that's the right choice, especially since:

  • This allows the same $EDITOR to work on the commandline - $EDITOR $file works

  • This allows it to work with $EDITORs with special characters - here your $EDITOR has a * in it. Previously we would have globbed that.

@faho faho added question and removed bug labels Feb 7, 2019
@lilyball
Copy link
Contributor Author

@lilyball lilyball commented Feb 7, 2019

This new approach doesn't work given that one of the parameters has spaces in it. It would not have globbed the * because it was part of a double-quoted string.

Using a list will work for fishscript, but this is a variable that's exported to the environment, and it appears fish's approach towards exporting arbitrary list variables to the environment is to space-separate them. So if I convert my EDITOR into a list, it will work for funced, but will fail horribly if I invoke any command that itself uses EDITOR (such as git commit).

@floam
Copy link
Member

@floam floam commented Feb 7, 2019

I'm not sure this isn't a bug.

@faho
Copy link
Member

@faho faho commented Feb 7, 2019

This new approach doesn't work given that one of the parameters has spaces in it.

That one parameter then needs to be one element.

It would not have globbed the * because it was part of a double-quoted string.

Ah, that's right.

Using a list will work for fishscript, but this is a variable that's exported to the environment, and it appears fish's approach towards exporting arbitrary list variables to the environment is to space-separate them.

Yes.

will fail horribly if I invoke any command that itself uses EDITOR (such as git commit).

Because that last argument would then be split? Yikes. That's unfortunate.

Does git actually handle the quotes here? Because e.g. bash wouldn't. With your current $EDITOR, try $EDITOR somefile in bash. It'll split wrong, keep the quotes and perform the glob.

I'm not sure this isn't a bug.

Not really. It's a trade-off.

@lilyball
Copy link
Contributor Author

@lilyball lilyball commented Feb 7, 2019

I’ve been using this EDITOR for years. Every tool that uses it, either handles it correctly, or fails because it expects EDITOR to be a single command it can exec instead of pass to a shell. I’ve never seen a case where it interprets the multi-word EDITOR but splits the quoted string on spaces. I’m not at my computer right now so I can’t explicitly test bash, but git definitely handles this correctly.

FWIW most programs handle this correctly.

@lilyball
Copy link
Contributor Author

@lilyball lilyball commented Feb 7, 2019

I’d say this is definitively a bug. The primary use-case of EDITOR is as an environment variable, not a shell variable. The current approach makes it impossible to use EDITOR correctly when an argument contains spaces or shell-special variables.

@floam
Copy link
Member

@floam floam commented Feb 7, 2019

Not really. It's a trade-off.

I think if $EDITOR ran as a command can't just work, funced and the rest of our scripts simply need to do whatever they need to do to treat it like every other piece of software does, or it's a bug, because it's not working right.

@floam floam removed the question label Feb 7, 2019
faho added a commit that referenced this issue Feb 7, 2019
@floam floam added the bug label Feb 7, 2019
@faho
Copy link
Member

@faho faho commented Feb 7, 2019

I still disagree, but it's probably not worth fighting over.

8e60ebc restores the previous behavior.

@floam floam added this to the fish 3.1.0 milestone Feb 7, 2019
@faho faho removed the regression label Feb 7, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug
Projects
None yet
Development

No branches or pull requests

3 participants