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

DOCS/input: simpler solution to handle sh metacharacters with run #9647

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

emanuele6
Copy link
Contributor

@emanuele6 emanuele6 commented Dec 27, 2021

Also rephrase "To get the old behavior" as "If you want to use shell features".

Screenshot:
file

@emanuele6 emanuele6 force-pushed the runinlinesh branch 3 times, most recently from 509375d to 9b72e47 Compare December 27, 2021 09:02
@avih
Copy link
Member

avih commented Dec 27, 2021

Not sure about this. While technically correct, it's a bit misleading in two ways:

  • The first argument to sh is - but it's not clear why it's there (I know why, but it's not clear at the docs).
  • The example where you use "$1" makes it look as if (mpv) properties are not expanded due to single quotes - but that's not true. Single quotes doesn't prevent mpv properties expansion, and it's not expanded because it's not ${ or $> or $$, but if you used "${1}" then mpv would try to expand it and error out. So (mpv argument) single quotes are not a general solution for a shell line - but your example makes it look like it is.

Also, the text is a bit too verbose for my taste with the metacharacters etc. It's suffice IMHO to say that it avoid the need for escaping the actual value.

Overall, the text should be shorter and without gotchas.

@emanuele6
Copy link
Contributor Author

emanuele6 commented Dec 27, 2021

Yeah, I am aware I should have used $$ instead of just $ (I use that in my personal bindings even when not strictly necessary because I don't like to have special characters unescaped).

There I was just trying to shorten the line as much as possible since that line is indented a lot and it is hard to read on a 80x24 terminal window. But, you are right that that could be misleading. I will add it back since the line won't fit on one line in a small terminal anyway.

I also would not have mentioned the "potential code injection attack", but I did since it was already mentioned in the original text, I will happily remove it.

I will make the note less verbose and add an addendum for - (that I changed to _ since some shells ignore - instead of using it to set $0).


EDIT: Screenshot
file

@emanuele6 emanuele6 force-pushed the runinlinesh branch 3 times, most recently from a0898c8 to 2eab3a5 Compare December 27, 2021 10:52
@avih
Copy link
Member

avih commented Dec 27, 2021

Yeah, I am aware I should have used $$ instead of just $

That would work, but looks clumsy IMHO (it is a matter of taste though), and if more arguments are used then it becomes messier.

Maybe something like run sh -c '$> SHELL-CONTENT' xx "${title}" ? (with an actual shell code).

The single quotes prevent string escpes interpretation (backslash), the $> at the begining of the argument prevents mpv expansion, and the quotes for "${title}" because officially all arguments should be quoted.

Also, you should ignore the terminal width. The manpage is aligned it to the terminal width automatically, and except for example strings which preferably fit to 60 chars or so, all other text should be arbitrary. Don't "design" for manpage in 80 cols term.

@emanuele6
Copy link
Contributor Author

emanuele6 commented Apr 14, 2022

I rebased the commit onto master. Is this PR ready to be merged or should I change something else?

Also rephrase "To get the old behavior" as "If you want to use shell
features".
@emanuele6 emanuele6 changed the title man: suggest a simpler solution to handle special sh character with run DOCS/input: simpler solution to handle sh metacharacters with run May 2, 2022
@emanuele6
Copy link
Contributor Author

I changed the commit message to DOCS/input: ... as suggested in #10153 (comment)

@zDEFz
Copy link

zDEFz commented Jun 1, 2024

Relevant: Had to find out
c run "/usr/bin/bash" "-c" "echo ${filename/no-ext} | xsel -ib" doesnt really work.
However,
c run "/usr/bin/bash" "-c" "printf '%s' \"${filename/no-ext}\" | xsel -ib" does.

Test string 20221110 「死にたいわけじゃなくて」⧸ 可不(KAFU) - l3UvRTpHqKY

Please consider a update.

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

Successfully merging this pull request may close these issues.

4 participants