repl: make sure historyPath is trimmed #4539
Conversation
| } | ||
|
|
||
| if (historyPath !== '') { | ||
| return setupHistory(repl, env.NODE_REPL_HISTORY, |
There was a problem hiding this comment.
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() !== '') {
...
}There was a problem hiding this comment.
Because then we would have to trim the string more than once before it is passed to setupHistory
There was a problem hiding this comment.
@evanlucas How so? We are anyway passing env.NODE_REPL_HISTORY as it is to setupHistory.
There was a problem hiding this comment.
Oh wow. Not what I meant to do. Will fix shortly. I was meaning to pass the trimmed historyPath to setupHistory
|
Hmmm, I still feel that the type checking would be better here. Because, users can tamper |
|
process.env gets coerced to a string when set though. |
|
TIL :-) Thanks :P In that case, we can simply do const historyPath = env.NODE_REPL_HISTORY.trim();
if (historyPath) {
return setupHistory(...);
}Right? |
|
We can't do that because the key may not exist at all: |
|
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? |
| if (opts.terminal) { | ||
| // Make sure we trim the history path. We don't want to create | ||
| // a file name " ". | ||
| let historyPath = env.NODE_REPL_HISTORY; |
There was a problem hiding this comment.
All of this logic, including the check for empty string might make more sense inside of setupHistory().
|
This probably warrants a tiny documentation update to mention that whitespace is stripped. Right now it only says:
After this change, that would only be partially correct. |
|
LGTM. What do y'all think about landing this in v4? |
|
@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) |
| return ready(null, repl); | ||
| } | ||
|
|
||
| if (historyPath) { |
There was a problem hiding this comment.
This condition is not necessary. If the control reaches this point, then historyPath is not an empty string.
There was a problem hiding this comment.
It could be undefined though
|
One comment, then LGTM. |
|
Ok, updated. PTAL @cjihrig |
|
Looking, sorry for the delay. |
| repl._historyPrev = _replHistoryMessage; | ||
| cb(null, repl); | ||
| } | ||
|
|
||
| function setupHistory(repl, historyPath, oldHistoryPath, ready) { | ||
| // Empty string disables persistent history. | ||
|
|
||
| if (typeof historyPath === 'string') |
There was a problem hiding this comment.
Wait what else would it be? We don't export setupHistory().
There was a problem hiding this comment.
if process.env.NODE_REPL_HISTORY is undefined, then it will be undefined
|
LGTM once @Fishrock123 is happy :-) |
|
Seems fine to me. |
|
Rebased after the eslint changes. @Fishrock123 @jasnell @cjihrig @thefourtheye LGTY? |
|
Yep, still LGTM. |
|
LGTM |
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
|
Landed in da550aa. Thanks! |
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
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?