FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

repl: make sure historyPath is trimmed by evanlucas · Pull Request #4539 · nodejs/node · GitHub

/ node Public

repl: make sure historyPath is trimmed - #4539

Merged
evanlucas merged 1 commit into
nodejs:masterfrom
evanlucas:replspace
Jan 16, 2016
ConversationCommits (1)ChecksFiles changed
Merged

repl: make sure historyPath is trimmed#4539
evanlucas merged 1 commit into
nodejs:masterfrom
evanlucas:replspace

Conversation

evanlucas commented Jan 5, 2016

Copy link
Copy Markdown
Contributor

If one were to set NODE_REPL_HISTORY to a string that contains only a
space (" "), then the history file would be created with that name
which can cause problems are certain systems.

Related: #4522

R= @Fishrock123?

evanlucas added the repl Issues and PRs related to the REPL subsystem. label Jan 5, 2016
thefourtheye reviewed Jan 5, 2016
View reviewed changes
Comment thread lib/internal/repl.js Outdated

thefourtheye Jan 5, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

If we are anyway going to use the actual value as it is, then why not just

if (opts.terminal &&
    typeof env.NODE_REPL_HISTORY === 'string' &&
    env.NODE_REPL_HISTORY.trim() !== '') {
...
}

evanlucas Jan 5, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Because then we would have to trim the string more than once before it is passed to setupHistory

thefourtheye Jan 5, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@evanlucas How so? We are anyway passing env.NODE_REPL_HISTORY as it is to setupHistory.

evanlucas Jan 5, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Oh wow. Not what I meant to do. Will fix shortly. I was meaning to pass the trimmed historyPath to setupHistory

evanlucas Jan 5, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Fixed

thefourtheye commented Jan 5, 2016

Copy link
Copy Markdown
Contributor

Hmmm, I still feel that the type checking would be better here. Because, users can tamper env programmatically and we will fail at runtime if trim is not defined on that object.

evanlucas commented Jan 5, 2016

Copy link
Copy Markdown
Contributor Author

process.env gets coerced to a string when set though.

> process.env.test = 1
1
> process.env.test
'1'
> process.env.test = { 'name': 'test' }
{ name: 'test' }
> process.env.test
'[object Object]'
> process.env.PATH
'/Users/evan/dev/code/depot_tools:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/opt/X11/bin:/usr/local/MacGPG2/bin'

thefourtheye commented Jan 5, 2016

Copy link
Copy Markdown
Contributor

TIL :-) Thanks :P

In that case, we can simply do

const historyPath = env.NODE_REPL_HISTORY.trim();
if (historyPath) {
  return setupHistory(...);
}

Right?

evanlucas commented Jan 5, 2016

Copy link
Copy Markdown
Contributor Author

We can't do that because the key may not exist at all:

> process.env.biscuits.trim()
TypeError: Cannot read property 'trim' of undefined
    at repl:1:21
    at REPLServer.defaultEval (repl.js:252:27)
    at bound (domain.js:287:14)
    at REPLServer.runBound [as eval] (domain.js:300:12)
    at REPLServer.<anonymous> (repl.js:417:12)
    at emitOne (events.js:82:20)
    at REPLServer.emit (events.js:169:7)
    at REPLServer.Interface._onLine (readline.js:210:10)
    at REPLServer.Interface._line (readline.js:549:8)
    at REPLServer.Interface._ttyWrite (readline.js:826:14)
>

thefourtheye commented Jan 5, 2016

Copy link
Copy Markdown
Contributor

Ah, right. Thanks :) Change LGTM. Let's see what @Fishrock123 thinks. Apart from this, should we warn the user that the file name is an empty string?

cjihrig reviewed Jan 5, 2016
View reviewed changes
Comment thread lib/internal/repl.js Outdated

cjihrig Jan 5, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

All of this logic, including the check for empty string might make more sense inside of setupHistory().

cjihrig commented Jan 5, 2016

Copy link
Copy Markdown
Contributor

This probably warrants a tiny documentation update to mention that whitespace is stripped. Right now it only says:

Setting this value to "" will disable persistent REPL history.

After this change, that would only be partially correct.

jasnell commented Jan 5, 2016

Copy link
Copy Markdown
Member

LGTM. What do y'all think about landing this in v4?

evanlucas commented Jan 6, 2016

Copy link
Copy Markdown
Contributor Author

@cjihrig nits addressed. PTAL.

@jasnell I think it should definitely be backported to LTS. Especially considering the problems it could cause on windows as stated in #4522 (comment)

thefourtheye reviewed Jan 6, 2016
View reviewed changes
Comment thread lib/internal/repl.js Outdated

thefourtheye Jan 6, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This condition is not necessary. If the control reaches this point, then historyPath is not an empty string.

evanlucas Jan 6, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It could be undefined though

cjihrig commented Jan 6, 2016

Copy link
Copy Markdown
Contributor

One comment, then LGTM.

evanlucas commented Jan 7, 2016

Copy link
Copy Markdown
Contributor Author

Ok, updated. PTAL @cjihrig

cjihrig commented Jan 7, 2016

Copy link
Copy Markdown
Contributor

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

Fishrock123 commented Jan 7, 2016

Copy link
Copy Markdown
Contributor

Looking, sorry for the delay.

Fishrock123 reviewed Jan 7, 2016
View reviewed changes
Comment thread lib/internal/repl.js

Fishrock123 Jan 7, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Wait what else would it be? We don't export setupHistory().

evanlucas Jan 7, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

if process.env.NODE_REPL_HISTORY is undefined, then it will be undefined

Fishrock123 Jan 7, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Oh right, carry on.

jasnell commented Jan 7, 2016

Copy link
Copy Markdown
Member

LGTM once @Fishrock123 is happy :-)

Fishrock123 commented Jan 8, 2016

Copy link
Copy Markdown
Contributor

Seems fine to me.

evanlucas commented Jan 14, 2016

Copy link
Copy Markdown
Contributor Author

Rebased after the eslint changes. @Fishrock123 @jasnell @cjihrig @thefourtheye LGTY?

cjihrig commented Jan 14, 2016

Copy link
Copy Markdown
Contributor

Yep, still LGTM.

Fishrock123 commented Jan 16, 2016

Copy link
Copy Markdown
Contributor

LGTM

repl: make sure historyPath is trimmed
da550aa 
If one were to set NODE_REPL_HISTORY to a string that contains only a
space (" "), then the history file would be created with that name
which can cause problems are certain systems.

PR-URL: nodejs#4539
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
evanlucas closed this Jan 16, 2016
evanlucas deleted the replspace branch January 16, 2016 19:25
evanlucas merged commit da550aa into nodejs:master Jan 16, 2016

evanlucas commented Jan 16, 2016

Copy link
Copy Markdown
Contributor Author

Landed in da550aa. Thanks!

evanlucas mentioned this pull request Jan 18, 2016
Merged
evanlucas added a commit that referenced this pull request Jan 18, 2016
repl: make sure historyPath is trimmed
8104d9d 
If one were to set NODE_REPL_HISTORY to a string that contains only a
space (" "), then the history file would be created with that name
which can cause problems are certain systems.

PR-URL: #4539
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 28, 2016
repl: make sure historyPath is trimmed
1070422 
If one were to set NODE_REPL_HISTORY to a string that contains only a
space (" "), then the history file would be created with that name
which can cause problems are certain systems.

PR-URL: #4539
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 11, 2016
repl: make sure historyPath is trimmed
2f481e1 
If one were to set NODE_REPL_HISTORY to a string that contains only a
space (" "), then the history file would be created with that name
which can cause problems are certain systems.

PR-URL: #4539
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 11, 2016
repl: make sure historyPath is trimmed
84ee8ae 
If one were to set NODE_REPL_HISTORY to a string that contains only a
space (" "), then the history file would be created with that name
which can cause problems are certain systems.

PR-URL: nodejs#4539
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins mentioned this pull request Feb 11, 2016
Merged
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 15, 2016
repl: make sure historyPath is trimmed
26f0240 
If one were to set NODE_REPL_HISTORY to a string that contains only a
space (" "), then the history file would be created with that name
which can cause problems are certain systems.

PR-URL: nodejs#4539
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
repl: make sure historyPath is trimmed
896dca9 
If one were to set NODE_REPL_HISTORY to a string that contains only a
space (" "), then the history file would be created with that name
which can cause problems are certain systems.

PR-URL: nodejs#4539
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

repl Issues and PRs related to the REPL subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL