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

Shell expansions passing to commands #1175

Closed
avidseeker opened this issue Mar 25, 2023 · 23 comments · Fixed by #1246
Closed

Shell expansions passing to commands #1175

avidseeker opened this issue Mar 25, 2023 · 23 comments · Fixed by #1246

Comments

@avidseeker
Copy link

cmd e $nvim $1 is defined in lfrc.

Entering :e /home/user/somefile works. But :e ~/somefile doesn't (although tab completion works).

@zSnails
Copy link

zSnails commented Mar 31, 2023

I don't really understand your question, but you can use $f so that lf takes care of passing the selected file to nvim

cmd e $nvim $f should do the trick.

@avidseeker
Copy link
Author

avidseeker commented Mar 31, 2023

This edits the currently selected file. I want to edit a file in another directory.

@gokcehan
Copy link
Owner

gokcehan commented Apr 2, 2023

@avidseeker You can use map e :push $nvim<space> instead to be able to use shell expansions.

@avidseeker
Copy link
Author

This is map instead of cmd.

I'm basically trying to mimic :e /some/path/to/file behavior in vim.

@gokcehan
Copy link
Owner

gokcehan commented Apr 2, 2023

@avidseeker I forgot to comment on the expansion part. I agree it is unusual for completion to work in this case. Either we should expand the parameters or completion should not work. On the top of my head, expanding all parameters might be problematic if there is no way to avoid the expansion. For example, shell expands ~ but you can also avoid expansion with quotes. I think we currently do not have a separate mechanism for quoted and unquoted strings. Removing the expansion could be better but it is used in some builtin commands (e.g. :source) and I think we don't have a separate completion mechanism for builtin and custom commands.

@gokcehan
Copy link
Owner

gokcehan commented Apr 2, 2023

This is map instead of cmd.

Sorry, you are right. I'm a little confused because I have been chain commenting on multiple issues and PRs at the same time.

@joelim-work
Copy link
Collaborator

@avidseeker The reason why cmd e $nvim $1 doesn't work is because the shell will substitute the $1 variable with literally ~/somefile, but then not expand the ~ afterwards. You can refer to this Stack Overflow post for more details about this kind of issue.

I guess it would be possible to have lf forcibly perform tilde expansion on every argument before passing it to the shell, but this will also prevent the user from passing ~ directly to the shell if they don't want the expansion to happen. I'm not sure how likely this scenario is though.

Anyway even if lf doesn't expand the tilde, you can just manually expand it yourself:

# Substituting text via ${parameter/pattern/string} is specific to bash, using sed probably works too
cmd e $nvim ${1/#~/$HOME}

@avidseeker
Copy link
Author

cmd e $nvim ${1/#~/$HOME}

I'm getting --: 2: Bad substitution for some reason..

possible to have lf forcibly perform tilde expansion

The reason for proposing this is that typing $nvim ~/somefile directly in lf works, but binding the same line to a command doesn't work. It seems reasonable to expect cmd to be executed just as it's being typed. And in case user needs to pass literal tilde, it's possible to do: $nvim '~'

@joelim-work
Copy link
Collaborator

I'm getting --: 2: Bad substitution for some reason..

I think that might be because you don't use bash, or at least your shell doesn't know how to evaluate ${1/#~/$HOME}. Could you try this instead?

cmd e $nvim $(echo $1 | sed "s|^~|$HOME|")

The reason for proposing this is that typing $nvim ~/somefile directly in lf works, but binding the same line to a command doesn't work. It seems reasonable to expect cmd to be executed just as it's being typed. And in case user needs to pass literal tilde, it's possible to do: $nvim '~'

When you type $nvim ~/somefile directly, the entire string nvim ~/somefile is passed to the shell and executed. In this case the tilde will be expanded since it appears in the original string, and it is very much like running sh -c 'nvim ~/somefile' directly in the shell.

When you define cmd e $nvim $1 and type :e ~/somefile, then the string nvim $1 is the resulting string that is executed, with ~/somefile being passed as an argument. In this case the shell has to substitute $1 with the value ~/somefile, but that also means that the tilde will not be expanded afterwards. This would be similar to running sh -c 'nvim $1' -- '~/somefile', which also doesn't work. I believe this is just simply because of the order in which the shell performs various expansions., that is to say tilde expansion happens before parameter expansion and not after.

Regarding the part about quoting the tilde, the quotes only apply on lf's internal command parsing rules, and since lf itself doesn't treat ~ as a special character, there is no difference between ~ and '~'. Perhaps it might be possible to change lf's parsing rules to expand the tilde, I have not given it much thought. At the very the unit tests in eval_test.go will have to be changed, and I'm not sure how simple it is to mock the home directory into the parser.

@joelim-work
Copy link
Collaborator

I think it's worth mentioning the issue #1219 here as well. There seems to be a desire for lf command parsing to be able to expand ~ and variable names similar to how a shell would. While lf can forward commands to a shell, I think it's important to remember that it is not a shell itself.

Currently lf doesn't treat ~ specially when parsing, but for certain situations it will manually expand it, for instance the source command:

lf/eval.go

Lines 2081 to 2087 in f04401b

case "source":
if len(e.args) != 1 {
app.ui.echoerr("source: requires an argument")
return
}
app.readFile(replaceTilde(e.args[0]))
app.ui.loadFileInfo(app.nav)

While I think it's possible to have lf manually expand ~ just before passing it to a shell, changing the parsing logic to treat ~ as a special character and expand it is going too far against the original intentions, but this is just my interpretation after going through the code.

@joelim-work
Copy link
Collaborator

I was playing around with the parser, and it turns out it's not too difficult to expand the tilde during the parsing stage after all. I have submitted a PR: #1246

I am not a huge fan of having the parser depend on the HOME environment variable to expand the tilde, but I don't see any other good solution.

@joelim-work
Copy link
Collaborator

I am not a huge fan of having the parser depend on the HOME environment variable to expand the tilde

Turns out @gokcehan isn't a fan of this either 😂. Instead we have agreed on an alternative approach where the tilde is expanded during the autocompletion stage (e.g. :echo ~/lon<tab> would result in the text :echo /home/foo/long_name in the prompt), so that the following parsing and command handling stages don't have to worry about it.

@ilyagr
Copy link
Collaborator

ilyagr commented May 17, 2023

I think #1254 is related. Though, the auto-completion mechanism doesn't immediately solve that.

We could, in principle, have some special syntax for expanding environment variables and ~ in the command line. At the moment, I don't have a great idea of what that would look like exactly.

The simplest option would be to have a special prefix from shell commands - so $expand $EDITOR ~/.config/lf/lfrc would expand both the shell variable and the ~ - but this is not very flexible. I'd prefer something that is more local as opposed to changing the expansion rules for the whole line.

@ilyagr
Copy link
Collaborator

ilyagr commented May 17, 2023

If we go with auto-completion, an awkward way to use it inside a config would be

:push $$EDITOR<tab><space>~/.config/lf/lfrc<tab><ret>

@KaitlynEthylia
Copy link

I think #1254 is related. Though, the auto-completion mechanism doesn't immediately solve that.

i don't thank that that is related, as that issue is due to the % signs being hardcoded in the os_windows.go file

@joelim-work
Copy link
Collaborator

I think what @ilyagr meant was that even if the value %EDITOR% %f% is a hard-coded string, it would still theoretically be possible for lf to scan it for variables and substitute them before passing it to the shell. There are a couple of potential problems though:

  1. It is possible for the user to set the shell and also define custom maps at any time, even after startup. This means that lf would somehow have to magically know what a variable looks like (e.g. %var% or $var) in order to substitute them.
  2. The user may sometimes want to pass variable strings literally to the shell in which case lf would have to detect quotes/escapes in order to not substitute them.

Because the shell is already capable of substituting variables, lf takes the simple and modular approach of just forwarding the command string to the shell to evaluate, instead of duplicating the variable substitution functionality. I believe this is an intentional design decision.

Regarding #1254, the default settings are catered towards users with a default setup (i.e. CMD as the shell). If you have gone to the trouble of installing a different shell on Windows then I guess it's reasonable to expect that the default settings may no longer work out of the box? At the end of the day they are just defaults, so you can override them in your config file, that is what config files are there for.

Of course, this is just my own opinion, but I'm not really convinced that the default Windows shell syntax is much of an issue if it can be solved by configuration.

@ilyagr
Copy link
Collaborator

ilyagr commented May 18, 2023

This means that lf would somehow have to magically know what a variable looks like (e.g. %var% or $var) in order to substitute them.

My suggestion was that lf could have its own syntax for environment variable expansion that wold be independent of any shell, similar to the ~ expansion you were discussing here. Then, we could use that in the definition of the e mapping.

+1 to your other points. I'm quite unsure about this being a good direction to go in.

@avidseeker
Copy link
Author

Thank you for your work.

The PR works great, but one minor nitpick: it doesn't seem to cover home user expansion. E.g: :cd ~john expands as :cd /home/userjohn.

@joelim-work
Copy link
Collaborator

joelim-work commented Jun 4, 2023

@avidseeker What is your use case exactly? Are you saying that you have a directory that is literally called ~john and you want to cd into it?

And is this related to autocompletion at all?

@joelim-work
Copy link
Collaborator

BTW I think the culprit might be this function:

lf/misc.go

Lines 18 to 23 in a9d90bc

func replaceTilde(s string) string {
if strings.HasPrefix(s, "~") {
s = strings.Replace(s, "~", gUser.HomeDir, 1)
}
return s
}

And perhaps the condition should be changed to if s == "~" || strings.HasPrefix(s, "~/").

@avidseeker
Copy link
Author

And is this related to autocompletion at all?

Not all people know this type of autocompletion, that's why I said it's a minor nitpick, and so it's ok if not implemented. See: https://www.gnu.org/software/bash/manual/html_node/Tilde-Expansion.html

E.g: cd ~james goes to the home of user named james (which might not necessarily be /home/james).

@ilyagr
Copy link
Collaborator

ilyagr commented Jun 4, 2023

It's a really weird kind of expansion; it seems to check whether the user exists:

$ echo ~james
~james
$ echo ~ilyagr
/home/ilyagr

@joelim-work
Copy link
Collaborator

Oh OK I actually had no idea that there was more to tilde expansion.

I am not really sure whether it's worth implementing since the lf command line isn't really supposed to be a shell and the current handling of tilde was just provided as a convenience so users wouldn't have to type out /home/<username>.

You're welcome to create a separate new issue for this though, if you think it warrants further discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants