Skip to content

Conversation

@highmtworks
Copy link

A try to fix #19 .

I read https://gitlab.com/rhendric/puka/-/blob/v1.0.1/src/sh.js#L30
and understood that sh is equivalent to ShellString.sh(...).toString().

Although examples of https://gitlab.com/rhendric/puka/-/tree/v1.0.1#quoteforshell
applies quoteForShell only for sh template arguments ,

the format function of https://gitlab.com/rhendric/puka/-/blob/v1.0.1/src/ShellStringText.js
delegated by https://gitlab.com/rhendric/puka/-/blob/v1.0.1/src/ShellString.js#L49
also quotes parsed arguments of a sh template string .

The format function of ShellStringText uses formatter.quote
which actually is quoteForCmd registered in
https://gitlab.com/rhendric/puka/-/blob/v1.0.1/src/Formatter.js
and is equivalent to quoteForShell as seen in
https://gitlab.com/rhendric/puka/-/blob/v1.0.1/src/quoteForShell.js .

That means ShellString.sh(...).toString() virtually applies quoteForShell
to parsed arguments of a sh template string .

Although ShellString.sh(...).toString() does not have forceQuote parameter ,
escapeCmd in make-spawn-args passes false as forceQuote to quoteForShell and
the default of forceQuote is falsy as seen in
https://gitlab.com/rhendric/puka/-/blob/v1.0.1/src/Formatter.js#L72 and
https://gitlab.com/rhendric/puka/-/blob/v1.0.1/src/Formatter.js#L54 .
(But one of uses of formatter.quote in ShellStringText passes true as quoteForShell
as seee in
https://gitlab.com/rhendric/puka/-/blob/v1.0.1/src/ShellStringText.js#L46 .
This may cause incompatibility. )

Thus I think replacing escapeCmd in make-spawn-args with ShellString.sh(...).toString()
is safe .

One of the differences is that ShellString.sh(...).toString() replaces an empty string
with a quoted empty string as seen in
https://gitlab.com/rhendric/puka/-/blob/v1.0.1/src/ShellStringText.js#L54
and
https://gitlab.com/rhendric/puka/-/blob/v1.0.1/src/Formatter.js#L30 .

I added a test case for escaping empty arguments in windows environment
and run npm run test with node v15.8.0 and npm 7.5.2 on windows 10 cmd.exe .

References

Fixes #19

@highmtworks
Copy link
Author

I found one of incompatible behaviors when it is applied to npm.
package.json:

{
  "scripts": {
    "start": "node index.js \"|\"\"|\""
  }
}

index.js

console.log(process.argv.slice(2));

then

npm start

before (6e169a9)

[ '|"|' ]

after (f183ed1)

[ '||' ]

@darcyclarke darcyclarke added this to the OSS - Sprint 24 milestone Feb 9, 2021
@darcyclarke
Copy link

This should get closed by #22

@darcyclarke
Copy link

Closing based on the intent to have the other PR land

@darcyclarke darcyclarke closed this Mar 8, 2021
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] npm start ignores empty argument on windows

2 participants