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

Why is the equal sign escaped in args to quote() #1

Closed
drok opened this issue Oct 30, 2023 · 4 comments
Closed

Why is the equal sign escaped in args to quote() #1

drok opened this issue Oct 30, 2023 · 4 comments

Comments

@drok
Copy link

drok commented Oct 30, 2023

Note: this issue is a copy of ljharb#11:

When converting an array or args into a string that might be saved to a file as a shell script, equal signs are escaped with a backslash. This is not necessary and makes the result script file harder to read. The superfluous escape is not incorrect, but unnecessary, and ugly.

I wonder why this escape is done, what purpose does it serve? Here's an example:

quote(["make", "CFLAGS=-DRELEASE"])

I expected the command line to be: make CFLAGS=-DRELEASE.

Instead, I got: make CFLAGS\=-DRELEASE (the \ is not necessary)

This behaviour seem to be implemented in quote.ts:

return String(s).replace(/([A-Za-z]:)?([#!"$&'()*,:;<=>?@[\\\]^`{|}])/g, '$1\\$2');

I see on this list that other characters which have no special meaning to the shell are also quoted: '@', '^', '{' '}'

I would like to note from the manpage of bash:

Note that unlike the metacharacters ( and ), { and } are reserved words and must occur where a reserved word is permitted to be recognized.

This means the curly brackets are not metacharacters ("A character that, when unquoted, separates words. One of
the following
: | & ; ( ) < > space tab newline"), they need not be escaped. Example:

$ echo {this-is-fine}
{this-is-fine}

$ echo "| this ( ) is ! also & fine > really ; and 'this' ... see ?"
| this ( ) is ! also & fine > really ; and 'this' ... see ?

$ echo "<quoted means not a metacharacter>"
<quoted means not a metacharacter>

Thanks!

@drok
Copy link
Author

drok commented Oct 30, 2023

The upstream maintainer's response indicates he is not interested in making the output of the library more readable:

Given that the purpose an escaping library like this is correctness, and not readability of the output, I'm inclined to leave it as-is, since "too much escaping" is a trivial problem but breakage is a very expensive one.

I need readable output, so it seems the way to accomplish this is to create a fork, and that is the reason why this repo exists.

@ljharb
Copy link

ljharb commented Oct 30, 2023

I said "inclined", not "not interested", so a fork seems like an extreme response :-)

@drok
Copy link
Author

drok commented Oct 30, 2023

I said "inclined", not "not interested", so a fork seems like an extreme response :-)

I am preparing the first patch that implements the readability I'm looking for and the associated test cases; I will propose it as a PR. Whether you like it or not will determine if the fork idea should be continued or abandoned.

I certainly wasn't looking forward to maintaining one more thing when I ran into the readability issue. I hope you will take on the battle for readability after reviewing the upcoming PR.

I think the project should also test the output of the "quote()" function against the supported shells; Do the targeted shells interpret the quoted strings as intended? I don't know if you're planning on any such testing. (no? fork is needed. yes? fork is not needed)

@ljharb
Copy link

ljharb commented Oct 30, 2023

That sort of testing would be great to have.

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.

2 participants