doc: child_process exec string is processed by shell #10921

codeman869 · 2017-01-20T19:11:33Z

Updates the Child Process Exec function documentation to provide more
information regarding how to deal with strings containing spaces
so that those spaces are not interpreted as separate arguments.
Fixes: #6803

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

sam-github · 2017-01-20T20:46:57Z

doc/api/child_process.md

@@ -147,7 +147,9 @@ added: v0.1.90
 * Returns: {ChildProcess}

 Spawns a shell then executes the `command` within that shell, buffering any
-generated output.
+generated output. When specifying paths, spaces need to be dealt with 


This isn't path specific, what about $? Or args that are not paths? Can you change to make clear that the command line is being passed to the shell, and will be processed as a shell command, so beware of anything significant: white space, shell meta chars, etc.

That's a good point! For some reason I was just focused on the path aspect 🤷‍♂️

What about something like:
...The command string passed to the exec function is processed directly by the shell and special characters need to be dealt with
accordingly, eg. exec("'/path/to/test file/test.sh' arg1 arg2") or
exec("echo \"Hello \$\\ World\"").

looks good, but mention that the shell specials depend on the shell, and maybe link to where it docs what shell is used on what system?

@sam-github I'll send another commit to update this PR to include these changes as well as those proposed by @Trott so that everyone can see a clean start point.

sam-github · 2017-01-20T20:48:23Z

also commit message should say what it s doing "note that exec string is processed by shell", perhaps?

Trott · 2017-01-20T22:44:26Z

doc/api/child_process.md

@@ -147,7 +147,9 @@ added: v0.1.90
 * Returns: {ChildProcess}

 Spawns a shell then executes the `command` within that shell, buffering any
-generated output.
+generated output. When specifying paths, spaces need to be dealt with 
+accordingly, eg. `exec("'/path/to/test file/test.sh' arg1 arg2")` or 


Our code standards use single quotes instead of double quotes and I think all (or at least most?) of our examples follow that style too (except perhaps in cases where double quote avoids escaping).

I would recommend avoiding e.g. and just spelling out for example. Even better perhaps, just remove it entirely and put a colon after accordingly. Then put the examples on separate lines by themselves.

...dealt with accordingly:

exec('"path...arg2'); exec('"path/to/test...arg2');

Ping @nodejs/documentation: Is using a code block that way OK when providing examples like that? I mean, it's not really a single block of code, but I'm not sure how else to do it? (Each one in its own code block?_

sam-github · 2017-01-23T21:35:47Z

@codeman869 can you rebase, squash, and rewrite the commit message?

codeman869 · 2017-01-25T15:52:41Z

@sam-github This has been squashed into 1 commit and the commit message was updated.

sam-github · 2017-01-25T17:21:46Z

@codeman869 It is difficult to find commits in logs when they don't try to describe the change. Did you see my suggestion?

doc: child_process exec string is processed by shell

You commit message is

doc: update child_process exec documentation

In this commit message, "documentation" adds no value, the prefix already makes clear its a commit to the docs, and "update" adds no value, of course a git commit is updating something, so all we know is this commit touches some aspect of child_process.exec, a git commit message that could appear over and over for any kind of change to the child_process.exec docs, so not so helpful.

Remove those words, and you have lots of space to give a hint as to what is being updated. See my examples.

sam-github · 2017-01-25T17:23:21Z

If you don't have time, I can rewrite the commit message when I land this, but if you are going to help out more on the project (as I hope you do), then it saves work if you produce better commit messages.

codeman869 · 2017-01-25T17:26:31Z

@sam-github no, I want to do it right, so I'll update the message. Thanks for the input! 😄

codeman869 · 2017-01-26T22:39:08Z

@sam-github I've amended my commit message per your suggestions, thank you!

sam-github · 2017-01-27T15:56:03Z

@Trott PTAL

Trott · 2017-01-27T19:15:14Z

@Trott PTAL

No objections from me on this.

thefourtheye · 2017-01-27T19:21:15Z

doc/api/child_process.md

+[shell](https://en.wikipedia.org/wiki/List_of_command-line_interpreters))
+need to be dealt with accordingly:
+```js
+exec('"/path/to/test file/test.sh" arg1 arg2'); 


It might be a good idea to explain the space thing in a comment.

@thefourtheye - I'm sorry, can't believe I didn't see this. I'm not quite sure I'm following what you're saying. Can you explain a little bit more about what you mean by ? Thanks!

It might be a good idea to explain the space thing in a comment.

@thefourtheye Do you want to clarify your comment? Or, @codeman869, do you want to take a stab at addressing the comment?

@fhinkel I'll take a stab at it

codeman869 · 2017-02-17T20:04:14Z

@sam-github Is this waiting for anything further before being landed ? Also do I need to do anything about the test/arm and test/osx failures ?
Thanks!

jasnell · 2017-02-17T20:25:26Z

@codeman869 ... there is one comment from @thefourtheye that should be looked at, otherwise we'd just need to run CI again.

jasnell · 2017-02-17T20:27:20Z

CI: https://ci.nodejs.org/job/node-test-pull-request/6475/

Updates the Child Process Exec function documentation providing further information with regards to treating special characters in the command string function argument. Note that the exec string is processed by the shell. Updates description to show that special shell characters vary based on the shell with a link to the Wikipedia page regarding different command line interpreters and their associated operating systems. Updates example code to reside in separate lines using a codeblock and utilizes examples to comply with code standards by using single quotes. Fixes: nodejs#6803

codeman869 · 2017-03-27T23:35:34Z

Okay, took a stab at @thefourtheye's recommendation and amended my commit, and rebased off the current master branch

jasnell · 2017-04-04T18:57:16Z

Ping @thefourtheye

thefourtheye

Sorry for the delay people. Glad to see the helpful comments. Thanks @codeman869

benjamingr · 2017-04-16T00:22:27Z

Seeing as this is all approved - landed in fe1be39

Thanks for your contribution @codeman869 !

Updates the Child Process Exec function documentation providing further information with regards to treating special characters in the command string function argument. Note that the exec string is processed by the shell. Updates description to show that special shell characters vary based on the shell with a link to the Wikipedia page regarding different command line interpreters and their associated operating systems. Updates example code to reside in separate lines using a codeblock and utilizes examples to comply with code standards by using single quotes. PR-URL: #10921 Fixes: #6803 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations. labels Jan 20, 2017

sam-github suggested changes Jan 20, 2017

View reviewed changes

Trott reviewed Jan 20, 2017

View reviewed changes

nodejs-github-bot added lts-watch-v6.x labels Jan 23, 2017

codeman869 force-pushed the doc-child_process-exec branch from 17bc05d to 54d60a2 Compare January 23, 2017 21:58

nodejs-github-bot added lts-watch-v6.x labels Jan 23, 2017

codeman869 force-pushed the doc-child_process-exec branch from 54d60a2 to b63555b Compare January 25, 2017 17:32

nodejs-github-bot added lts-watch-v6.x labels Jan 25, 2017

codeman869 changed the title ~~doc: update child_process exec documentation~~ doc: child_process exec string is processed by shell Jan 26, 2017

sam-github approved these changes Jan 27, 2017

View reviewed changes

thefourtheye reviewed Jan 27, 2017

View reviewed changes

jasnell approved these changes Jan 27, 2017

View reviewed changes

codeman869 force-pushed the doc-child_process-exec branch from b63555b to 3ac049a Compare March 27, 2017 23:26

nodejs-github-bot removed lts-watch-v6.x labels Mar 27, 2017

thefourtheye approved these changes Apr 5, 2017

View reviewed changes

lpinca approved these changes Apr 13, 2017

View reviewed changes

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

benjamingr closed this Apr 16, 2017

codeman869 deleted the doc-child_process-exec branch April 16, 2017 23:52

MylesBorins added the dont-land-on-v6.x label Jul 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: child_process exec string is processed by shell #10921

doc: child_process exec string is processed by shell #10921

codeman869 commented Jan 20, 2017

sam-github Jan 20, 2017

codeman869 Jan 20, 2017

sam-github Jan 23, 2017

codeman869 Jan 23, 2017

sam-github commented Jan 20, 2017

Trott Jan 20, 2017 •

edited

Loading

sam-github commented Jan 23, 2017

codeman869 commented Jan 25, 2017

sam-github commented Jan 25, 2017

sam-github commented Jan 25, 2017

codeman869 commented Jan 25, 2017

codeman869 commented Jan 26, 2017

sam-github commented Jan 27, 2017

Trott commented Jan 27, 2017

thefourtheye Jan 27, 2017

codeman869 Feb 17, 2017

fhinkel Mar 26, 2017

codeman869 Mar 27, 2017

codeman869 commented Feb 17, 2017

jasnell commented Feb 17, 2017

jasnell commented Feb 17, 2017

codeman869 commented Mar 27, 2017 •

edited

Loading

jasnell commented Apr 4, 2017

thefourtheye left a comment

benjamingr commented Apr 16, 2017

doc: child_process exec string is processed by shell #10921

doc: child_process exec string is processed by shell #10921

Conversation

codeman869 commented Jan 20, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-github commented Jan 20, 2017

Trott Jan 20, 2017 • edited Loading

Choose a reason for hiding this comment

sam-github commented Jan 23, 2017

codeman869 commented Jan 25, 2017

sam-github commented Jan 25, 2017

sam-github commented Jan 25, 2017

codeman869 commented Jan 25, 2017

codeman869 commented Jan 26, 2017

sam-github commented Jan 27, 2017

Trott commented Jan 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeman869 commented Feb 17, 2017

jasnell commented Feb 17, 2017

jasnell commented Feb 17, 2017

codeman869 commented Mar 27, 2017 • edited Loading

jasnell commented Apr 4, 2017

thefourtheye left a comment

Choose a reason for hiding this comment

benjamingr commented Apr 16, 2017

Trott Jan 20, 2017 •

edited

Loading

codeman869 commented Mar 27, 2017 •

edited

Loading