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

Fix handling of paths with special characters / wildcards #652

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

karlch
Copy link
Owner

@karlch karlch commented Jul 5, 2023

Escapes the characters []?* properly when expanding wildcards such as % and %m.

fixes #553

@pglira @TeaWhyDee @jcjgraf could one of you give this a quick check?

In addition, I was not able to find out what the issue with image.jpg~ should be, all playing around I did with tilde worked just fine. Could someone elaborate, so I can potentially add this to the PR?

@jcjgraf
Copy link
Contributor

jcjgraf commented Jul 5, 2023

I have tested the reported cases and your PR seems to fix them. I was also not able to observe anything unusual when using tiles in the file name.

@TeaWhyDee
Copy link

TeaWhyDee commented Jul 6, 2023

I have tested it, the ~ is still broken. Reproducible example command:

xt : !notify-send "%%"

Running this on "IMG.jpg~":
image

The issue seems to persist with other special symbols too in case of external commands.

@karlch
Copy link
Owner Author

karlch commented Jul 6, 2023

Thanks, I can reproduce it with external commands, especially together with quoting this is complete havoc currently.

I believe we will have to clarify and change some of the requirements here.

In principle we have a matrix of combinations here, with different wildcards as the rows (internal like %, the tilde, python globbing, shell expansion) and different command types as the columns (regular command, path command such as :open, external without sub-shell using !, external with sub-shell using :spawn). Summarized here quick and dirty:

scrot-2023-07-06_21:35:58

yes / no indicates whether this combination should be supported. I am unsure on the ones colored, any feedback or discussion would be great. Here green means this is the way it is currently, red is a deviation.

Removing globbing from external commands would save us a lot of escaping-havoc, and we could strictly split shell escaping and glob escaping, as they never overlap. I am honestly not 100 % sold on changing this, yes it could be useful to run something along !rm **/*.png, but really this should probably be done without vimiv then. And one could still do :mark **/*.png followed by !rm %m.

tl;dr different wildcards meet different command types, can we remove globbing from external commands? Should we add some further expansions to internal (non-path) commands?

@TeaWhyDee
Copy link

I think globbing could be useful in external commands. I don't think it's currently possible to escape the globbing symbols? I think either it should be removed or \ escaping added.
Something like :mark IMG*.jpg and :mark IMG\*.jpg still marks all glob matching images (and this auto-completes).

As for the external commands issue, tilde and globbing expansion could probably be done before internal symbol expansion so the issue doesn't occur?

@karlch
Copy link
Owner Author

karlch commented Jul 7, 2023

Works with two backslashes, i.e. :mark IMG\\*.jpg, don't ask why 😆
Now I agree it should escape in the completion, and one backslash should be enough of course.

I am not really sold on globbing in external commands though. Mainly to avoid actually using a sub-shell with :spawn? Or for the additional ** feature which not all shells have?

@karlch karlch mentioned this pull request Jul 12, 2023
4 tasks
This has various issues, and no trivial fix. We can rely on the shell
with spawn for globbing, e.g.,
:spawn "!rm Image*.jpg"
as long as the shell supports * and friends.

See #744 and #652 for some more details.
* Aliases first, as these can contain additional wildcards
* Then "path-like" expansion of the tilde
* Finally internal expansions, to avoid extra expansions of the true
  path names
No need to expand internal wildcards already here as it is done in the
next step.
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 this pull request may close these issues.

Bug: mark does not work if path contains "[" and "]"
3 participants