repl: Fixed node repl history edge case. Fixes #4102. by zeusdeux · Pull Request #4108 · nodejs/node · GitHub

If the deprecated NODE_REPL_HISTORY_FILE is set to default
node history file path ($HOME/.node_repl_history) and the file
doesn't exist, then node creates the file and then crashes when
it tries to parse that file as JSON thinking that it's an older
JSON formatted history file. This fixes that bug.

This patch also prevents node repl from throwing if the old
history file is empty or if $HOME/.node_repl_history is empty.

Also, in the test file, I moved the file paths above common
messages to make it easier to shift to template strings in the
future to build the messages.

Could this if/else be wrapped into the above if/else?

It could but that would give rise to else if branches with repeated checks. It would end up looking something like the following:

if (data) {
  repl.history = data.split(/[\n\r]+/, repl.historySize);
} else if (oldHistoryPath && oldHistoryPath === historyPath) {
  // If pre-v3.0, the user had set NODE_REPL_HISTORY_FILE to
  // ~/.node_repl_history. Warn the user about it and proceed.
  ...
} else if (oldHistoryPath && oldHistoryPath !== historyPath) {
  // Grab data from the older pre-v3.0 JSON NODE_REPL_HISTORY_FILE format.
  repl._writeToOutput(
    '\nConverting old JSON repl history to line-separated history.\n' +
    `The new repl history file can be found at ${historyPath}.\n`);
  repl._refreshLine();
  ...
}

I am not a fan tbh but if that is what works for the node project then I have no qualms @Fishrock123 :)

Correct me if I'm wrong, but I think it could be shortened to:

if (data) {
} else if (oldHistoryPath === historyPath) {
} else if (oldHistoryPath) {
}

Yupp that would. historyPath can never be empty. Boop!

@Fishrock123 Do you prefer squashed commits + force pushes or a verbose commit history?
I force pushed my changes based on your feedback for this iteration but can change how we go about this based on what work for this project.

@zeusdeux Either or. Squashing at the end is fine, and often what I do unless it's more convenient to squash inbetween.

@zeusdeux I'd just put the new message here. :)

Made changes based on feedback, rebased against master and sorted it all into one commit, 0ff9fb3.

Shouldn't this one be defaultHistoryPath?

In that case, this test would test the same path as the test on line 108 since oldHistoryPath === historyPath as both would be $HOME/.node_repl_history.
This test exists to test the the else if (oldHistoryPath) branch.

@zeusdeux could you make sure to run make -j$(getconf _NPROCESSORS_ONLN) test? That will run the tests and the linter. (Which you could also run with make jslint.

ditto, shouldn't need the else.

I run make -j4 test before every commit to make sure my changes aren't breaking stuff. :)

@zeusdeux The history is cleaned up and emptied by default in these tests, is it possible this one should be as follows?

env: { NODE_REPL_HISTORY_FILE: defaultHistoryPath },

If the deprecated NODE_REPL_HISTORY_FILE is set to default node history file path ($HOME/.node_repl_history) and the file doesn't exist, then node creates the file and then crashes when it tries to parse that file as JSON thinking that it's an older JSON formatted history file. This fixes that bug. This patch also prevents node repl from throwing if the old history file is empty or if $HOME/.node_repl_history is empty.

Included your suggestions and moved stuff into the parallel directory @Fishrock123 :)

Is there anything else you would want me to work on to get this PR ready @Fishrock123?

Probably should be something like "Only use old history if there was any"

@zeusdeux lgtm, you're free to fix that nit, otherwise I will on merging sometime later today/tomorrow.

If the deprecated NODE_REPL_HISTORY_FILE is set to default node history file path ($HOME/.node_repl_history) and the file doesn't exist, then node creates the file and then crashes when it tries to parse that file as JSON thinking that it's an older JSON formatted history file. This fixes that bug. This patch also prevents node repl from throwing if the old history file is empty or if $HOME/.node_repl_history is empty. Fixes: #4102 PR-URL: #4108 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

Thanks, landed in 29c4a2a with the following nits patched: (just comment-y stuff and position of a thing in a test)

diff --git a/lib/internal/repl.js b/lib/internal/repl.js
index fa357c1..4825d5d 100644
--- a/lib/internal/repl.js
+++ b/lib/internal/repl.js
@@ -122,7 +122,7 @@ function setupHistory(repl, historyPath, oldHistoryPath, ready) {
       repl.history = data.split(/[\n\r]+/, repl.historySize);
     } else if (oldHistoryPath === historyPath) {
       // If pre-v3.0, the user had set NODE_REPL_HISTORY_FILE to
-      // ~/.node_repl_history. Warn the user about it and proceed.
+      // ~/.node_repl_history, warn the user about it and proceed.
       repl._writeToOutput(
           '\nThe old repl history file has the same name and location as ' +
           `the new one i.e., ${historyPath} and is empty.\nUsing it as is.\n`);
@@ -136,11 +136,11 @@ function setupHistory(repl, historyPath, oldHistoryPath, ready) {
       repl._refreshLine();

       try {
-        // Pre-v3.0, repl history was stored as JSON. Try and convert it
-        // to line separated history.
-        var oldReplJSONHistory = fs.readFileSync(oldHistoryPath, 'utf8');
+        // Pre-v3.0, repl history was stored as JSON.
+        // Try and convert it to line separated history.
+        const oldReplJSONHistory = fs.readFileSync(oldHistoryPath, 'utf8');

-        // If there is NO old history, set repl history to an empty array.
+        // Only attempt to use the history if there was any.
         if (oldReplJSONHistory) repl.history = JSON.parse(oldReplJSONHistory);

         if (!Array.isArray(repl.history)) {
diff --git a/test/parallel/test-repl-persistent-history.js b/test/parallel/test-repl-persistent-history.js
index 8e6433c..9b4b389 100644
--- a/test/parallel/test-repl-persistent-history.js
+++ b/test/parallel/test-repl-persistent-history.js
@@ -66,6 +66,10 @@ const homedirErr = '\nError: Could not get the home directory.\n' +
                    'REPL session history will not be persisted.\n';
 const replFailedRead = '\nError: Could not open history file.\n' +
                        'REPL session history will not be persisted.\n';
+const sameHistoryFilePaths = '\nThe old repl history file has the same name ' +
+                             'and location as the new one i.e., ' +
+                             path.join(common.tmpDir, '.node_repl_history') +
+                             ' and is empty.\nUsing it as is.\n';
 // File paths
 const fixtures = path.join(common.testDir, 'fixtures');
 const historyFixturePath = path.join(fixtures, '.node_repl_history');
@@ -76,11 +80,6 @@ const enoentHistoryPath = path.join(fixtures, 'enoent-repl-history-file.json');
 const emptyHistoryPath = path.join(fixtures, '.empty-repl-history-file');
 const defaultHistoryPath = path.join(common.tmpDir, '.node_repl_history');

-const sameHistoryFilePaths = '\nThe old repl history file has the same name ' +
-                             'and location as the new one i.e., ' +
-                             path.join(common.tmpDir, '.node_repl_history') +
-                             ' and is empty.\nUsing it as is.\n';
-
 const tests = [{
   env: { NODE_REPL_HISTORY: '' },
   test: [UP],

(I guess I should have left comments on more for those, but eh, I didn't feel like it was worth it.)

@Fishrock123 should this be on lts watch? are the changes that broke repl in v4.x-staging?

If the deprecated NODE_REPL_HISTORY_FILE is set to default node history file path ($HOME/.node_repl_history) and the file doesn't exist, then node creates the file and then crashes when it tries to parse that file as JSON thinking that it's an older JSON formatted history file. This fixes that bug. This patch also prevents node repl from throwing if the old history file is empty or if $HOME/.node_repl_history is empty. Fixes: nodejs#4102 PR-URL: nodejs#4108 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

@thealphanerd Oh good call. Yes we should probably backport it.

@Fishrock123 Thanks for the clean up and merge. Sorry I couldn't get on it. Was travelling.

PS: How do you get the "commit with" message btw? Always been curious how y'all do that. ^_^

Edit: I think I got it. It's like this, right?

If the deprecated NODE_REPL_HISTORY_FILE is set to default node history file path ($HOME/.node_repl_history) and the file doesn't exist, then node creates the file and then crashes when it tries to parse that file as JSON thinking that it's an older JSON formatted history file. This fixes that bug. This patch also prevents node repl from throwing if the old history file is empty or if $HOME/.node_repl_history is empty. Fixes: nodejs#4102 PR-URL: nodejs#4108 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

If the deprecated NODE_REPL_HISTORY_FILE is set to default node history file path ($HOME/.node_repl_history) and the file doesn't exist, then node creates the file and then crashes when it tries to parse that file as JSON thinking that it's an older JSON formatted history file. This fixes that bug. This patch also prevents node repl from throwing if the old history file is empty or if $HOME/.node_repl_history is empty. Fixes: #4102 PR-URL: #4108 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

If the deprecated NODE_REPL_HISTORY_FILE is set to default node history file path ($HOME/.node_repl_history) and the file doesn't exist, then node creates the file and then crashes when it tries to parse that file as JSON thinking that it's an older JSON formatted history file. This fixes that bug. This patch also prevents node repl from throwing if the old history file is empty or if $HOME/.node_repl_history is empty. Fixes: nodejs#4102 PR-URL: nodejs#4108 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

Fishrock123 added the repl Issues and PRs related to the REPL subsystem. label Dec 1, 2015

Fishrock123 reviewed Dec 1, 2015
View reviewed changes

zeusdeux force-pushed the issue-4102 branch from 331770f to 0ea1b9c Compare December 2, 2015 06:04

Fishrock123 reviewed Dec 3, 2015
View reviewed changes

zeusdeux force-pushed the issue-4102 branch 2 times, most recently from 5c7f3f5 to 0ff9fb3 Compare December 4, 2015 17:45

Fishrock123 reviewed Dec 4, 2015
View reviewed changes

zeusdeux force-pushed the issue-4102 branch from 0ff9fb3 to a958d3b Compare December 7, 2015 15:18

Fishrock123 reviewed Dec 11, 2015
View reviewed changes

zeusdeux force-pushed the issue-4102 branch from a958d3b to 7bc64fd Compare December 16, 2015 19:49

Fishrock123 self-assigned this Dec 16, 2015

Fishrock123 reviewed Dec 21, 2015
View reviewed changes

Fishrock123 closed this Dec 22, 2015

Fishrock123 added the lts-watch-v4.x label Dec 23, 2015

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Jan 13, 2016

MylesBorins mentioned this pull request Jan 19, 2016

V4.2.5 proposal #4768

Merged

repl: Fixed node repl history edge case. Fixes #4102. by zeusdeux · Pull Request #4108 · nodejs/node · GitHub

Uh oh!

Uh oh!

repl: Fixed node repl history edge case. Fixes #4102. - #4108

repl: Fixed node repl history edge case. Fixes #4102.#4108zeusdeux wants to merge 1 commit intonodejs:masternodejs/node:masterfrom zeusdeux:issue-4102zeusdeux/node:issue-4102Copy head branch name to clipboard

Conversation

zeusdeux commented Dec 1, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeusdeux commented Dec 2, 2015

Uh oh!

Fishrock123 commented Dec 3, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeusdeux commented Dec 4, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Dec 4, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeusdeux commented Dec 7, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeusdeux commented Dec 16, 2015

Uh oh!

zeusdeux commented Dec 20, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Dec 21, 2015

Uh oh!

Fishrock123 commented Dec 22, 2015

Uh oh!

Fishrock123 commented Dec 22, 2015

Uh oh!

MylesBorins commented Dec 22, 2015

Uh oh!

Fishrock123 commented Dec 23, 2015

Uh oh!

zeusdeux commented Dec 23, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

repl: Fixed node repl history edge case. Fixes #4102.#4108
zeusdeux wants to merge 1 commit into
nodejs:masterfrom
zeusdeux:issue-4102