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

feat(preview-tui): handle quoting in start_preview more robustly #1630

Merged
merged 2 commits into from
Apr 21, 2023

Conversation

musjj
Copy link
Contributor

@musjj musjj commented Apr 19, 2023

This PR makes the script more resistant to naughty filenames.
The script now depends on bash for the following features:

  • Arrays
    Correctly creating and passing argument lists is now simple
  • Parameter transformation
    ${parameter@Q} makes it easy to correctly quote a string so that it can be safely re-evaluated by the interpreter later.

On iTerm, the shell command used to render the preview is now passed to osascript via a named pipe: $FIFO_OSASCRIPT.
By not embedding the shell command directly, we now no longer need to worry about osascript's quoting rules. It's not perfect, because $SHELL and $TMPDIR might contain naughty characters, but it's quite unlikely to happen.

Fix #1614

@luukvbaal
Copy link
Collaborator

Thanks! I had a similar commit laying around but there were problems and hadn't gotten around to fixing them yet.

I can check windows terminal tomorrow and iterm through a VM but other testers are welcome.

@N-R-K
Copy link
Collaborator

N-R-K commented Apr 19, 2023

Looks OK to me (lightly tested). A couple comments:

  • The $FIFO_OSASCRIPT thing is new. I can guess what it's doing and why but the commit body should explain it too.
  • Additionally the PR description should be included in the commit body. It's much more convenient to have the rationale behind a change documented in the commit message so that it can be viewed directly in git instead of hunting down PR/issues in some web ui.

plugins/preview-tui Outdated Show resolved Hide resolved
This commit makes the script more resistant to naughty filenames.
The script now depends on bash for the following features:
- Arrays
Correctly creating and passing argument lists is now simple
- Parameter transformation
`${parameter@Q}` makes it easy to correctly quote a string so that it
can be safely re-evaluated by the interpreter later.

On iTerm, the shell command used to render the preview is now passed to
osascript via a named pipe: `$FIFO_OSASCRIPT`. By not embedding the
shell command directly, we now no longer need to worry about osascript's
quoting rules. It's not perfect, because $SHELL and $TMPDIR might
contain naughty characters, but it's quite unlikely to happen.
@musjj musjj force-pushed the preview-tui-escape branch from 6695ea0 to 432b075 Compare April 19, 2023 01:21
@luukvbaal luukvbaal marked this pull request as draft April 19, 2023 23:10
@luukvbaal
Copy link
Collaborator

luukvbaal commented Apr 19, 2023

I pushed a commit that is noticeably faster when toggling preview-tui (comparable to pre env refactor). I confirmed that it works with xterm, tmux, kitty and wezterm as previewer.

I have yet to test iTerm and Windows Terminal.

@luukvbaal
Copy link
Collaborator

luukvbaal commented Apr 20, 2023

iTerm does work, but not with the bash version that comes by default on (my VM's) MacOS. It does not support the @Q expansion.
I was able to make it work by reverting back to escaping ourselves, perhaps we want to do that to avoid bash version requirements?

@jarun
Copy link
Owner

jarun commented Apr 20, 2023

We can document if it's straightforward to upgrade the bash.

@musjj
Copy link
Contributor Author

musjj commented Apr 20, 2023

Nice, that's probably faster than a manual loop. It works pretty well for me.

@luukvbaal luukvbaal force-pushed the preview-tui-escape branch from af9dffb to 7078f36 Compare April 20, 2023 23:54
@luukvbaal
Copy link
Collaborator

luukvbaal commented Apr 20, 2023

I must say I am not a big fan of writing the osascript command to a file. We already had a working solution by properly escaping that can now be replaced by two simple bash parameter expansions. The latest commit does that, what do you guys think?
Besides, it is not really a FIFO, just a file and another (unnecessary IMO) one that we must clean up after ourselves at that.

It works on iTerm (with the shipped bash version) and Windows Terminal. Although I made some minor changes after testing on windows so I will test once more tomorrow.

EDIT: All working well in my testing.

@luukvbaal luukvbaal force-pushed the preview-tui-escape branch from 7078f36 to d3b5d0e Compare April 21, 2023 09:04
@luukvbaal luukvbaal marked this pull request as ready for review April 21, 2023 09:04
@jarun jarun merged commit 6a8d74a into jarun:master Apr 21, 2023
@jarun
Copy link
Owner

jarun commented Apr 21, 2023

Thank you!

@github-actions github-actions bot locked and limited conversation to collaborators May 22, 2023
@N-R-K N-R-K linked an issue Jun 13, 2024 that may be closed by this pull request
9 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants