Bug: vim-slime creates an empty directory when using shell environment variables in config file

[maisevector]

First of all, thanks for all your work on vim-slime, it is one of my favorite vim plugins!

Description:

I have noticed a subtle bug: When specifying an alternative g:slime_paste_file in the vimrc file which contains a shell environment variable, e.g. $HOME, vim-slime does not resolve it and creates a directory called $HOME in the directory from which vim currently operates.

Desired behavior:

The directory should not be created, i.e. the variable should be resolved.

Steps to reproduce:

• Put this minimal example in your vimrc file:

  let g:slime_paste_file = "$HOME/.other_file_name"
  nmap <c-c><c-c> <Plug>SlimeParagraphSend

• Open a file and use SlimeParagraphSend to send something to the REPL
• There will be an empty directory $HOME in the current directory.

I will submit a pull request later which should resolve the issue.

[FreedomBen]

I've been seeing this for days (weeks?) and have been baffled as to how this was happening :laughing:

Finally figured it out and saw this issue and rejoiced that I wasn't the only one.

[jpalardy]

Hi @maisevector , @FreedomBen

Thanks for opening this ⬆️

I had to double check, the relevant code uses expand

if !exists("g:slime_paste_file")
  let g:slime_paste_file = expand("$HOME/.slime_paste")
end

but the documentation is misleading. I think the "fix" is to change the examples in the documentation to use expand

How does that sound? (or were you thinking about something else?)

[jpalardy]

Hmmm… I'm on PR behind 😆

[maisevector]

Hi @jpalardy,

thanks for your quick response! Let us continue the thread here and not on the PR to discuss all in one place.

Sorry for not being specific enough initially, because from what I understand from the code, the current problem is that there is a mixture between shell commands and vim-internal commands that cause this behavior.

The problem is that $HOME is not expanded during the creation of the directory (since it is created by the vim-internal mkdir(paste_dir, "p") function) in this line. But when writing the file, system("cat > " . g:slime_paste_file, a:text) in this line will call cat in the system's shell. And the shell does expand the variable automatically. So the created directory and the paste file are not related as soon as there is an environment variable there. In this case, the directory name will contain a literal environment variable and be relative to the current directory, and for writing the file, the shell expands it either way, so the file will use the resolved environment variable.

Here is an example:

If someone wants to use a different file in their vimrc, e.g. let g:slime_paste_file = $HOME/.my_other_file (without specifically using vim's expand), mkdir will create the literal directory /path/to/current/dir/$HOME, but will write to the file $HOME/.my_other_file because during writing, the shell expands the variable regardless.

So, basically there will be an empty directory created, but it will not even be used by vim-slime. Since the directory is created in the wrong place the directory in which the paste file is supposed to reside might not even exist. In this case, vim-slime somehow falls back to expand($HOME/.slime_paste) (although I did not find the relevant code). This only occurs, if someone puts an environment variable without explicit expanding in their vimrc.

Regarding your response:

though VERY unlikely, as user MIGHT want to use a $ in their path, without it being auto-expanded

I generally agree, but I think that is currently not possible due to using vim's system() the shell will expand it when writing the file regardless.

So I guess you are right that the principal problem is the user's variable definition, but given that the environment variable is expanded when writing the file, I think it would be best to also do that during creating the directory.

I agree that the relevant documentation should be updated and non-expanded environment variables should not be placed in the vimrc in the first place. But in my opinion, it would make sense to 1. either handle directory creation and paste-file writing both by vim so that environment variables can be allowed in file names and directories, or 2. to make both of them expand (which would not allow environment variables and what is what I suggested).

Sorry, this was lengthier than I thought! If you still find that updating the documentation is enough, I could also try to add the relevant description there. Please let me know your thoughts!

[jpalardy]

Hi @maisevector

Thank for the detailed explanation ⬆️

Looking back at:

let output = system("cat > " . g:slime_paste_file, a:text)

I'm a bit horrified I forgot the shellescape… (meaning, I'll fix this one and other similar places while I'm at it)

…

Plan, so far:

• fix documentation, recommend expand(…)
• add shellescape to all system(…) calls

As for creating the directory … this is not something that was originally part of the code (it felt out of scope) 😩
It seems like recommending expand(…) would ensure that mkdir is fixed (for most use cases).

[maisevector]

Hi @jpalardy,

Yes, I think that should work!

I honestly think that there will be more users who (incorrectly) put e.g. $HOME to the paste file than users who want to have a literal $HOME somewhere in their directory, but regardless of how you would like to have it in the code, I could offer to update the relevant docs and/or update the system(...) calls and submit a new pull request if you like.

Cheers!

[jpalardy]

@maisevector

PR opened ⬆️

I would also be very surprised by a literal $HOME 😃

[jpalardy]

@maisevector can you checkout that branch and tell me if that works for your use-cases?

[maisevector]

Yes it does, thanks for all!