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

zeusdeux · 2015-12-01T21:03:57Z

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.

Fishrock123 · 2015-12-01T21:12:36Z

lib/internal/repl.js

@@ -121,22 +121,43 @@ function setupHistory(repl, historyPath, oldHistoryPath, ready) {
    if (data) {
      repl.history = data.split(/[\n\r]+/, repl.historySize);
    } else if (oldHistoryPath) {
-      // Grab data from the older pre-v3.0 JSON NODE_REPL_HISTORY_FILE format.
-      repl._writeToOutput(
+      if (oldHistoryPath === historyPath) {


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!

zeusdeux · 2015-12-02T06:10:40Z

@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.

Fishrock123 · 2015-12-03T15:22:12Z

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

Fishrock123 · 2015-12-03T15:25:09Z

test/sequential/test-repl-persistent-history.js

@@ -66,17 +77,24 @@ 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';


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

zeusdeux · 2015-12-04T17:48:12Z

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

Fishrock123 · 2015-12-04T18:46:28Z

test/sequential/test-repl-persistent-history.js

@@ -94,6 +100,17 @@ const tests = [{
  expected: [prompt, replDisabled, prompt]
 },
 {
+  env: { NODE_REPL_HISTORY_FILE: emptyHistoryPath },


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.

Fishrock123 · 2015-12-04T18:52:58Z

@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.

Fishrock123 · 2015-12-04T18:55:36Z

lib/internal/repl.js

+
+        // If there is NO old history, set repl history to an empty array.
+        if (oldReplJSONHistory) repl.history = JSON.parse(oldReplJSONHistory);
+        else repl.history = [];


ditto, shouldn't need the else.

zeusdeux · 2015-12-07T15:16:04Z

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

Fishrock123 · 2015-12-11T16:30:17Z

test/sequential/test-repl-persistent-history.js

+},
+{
+  env: { NODE_REPL_HISTORY: emptyHistoryPath,
+         NODE_REPL_HISTORY_FILE: emptyHistoryPath },


@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.

zeusdeux · 2015-12-16T19:49:49Z

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

zeusdeux · 2015-12-20T11:29:23Z

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

Fishrock123 · 2015-12-21T19:37:51Z

lib/internal/repl.js

+        // to line separated history.
+        var oldReplJSONHistory = fs.readFileSync(oldHistoryPath, 'utf8');
+
+        // If there is NO old history, set repl history to an empty array.


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

Fishrock123 · 2015-12-21T19:39:36Z

@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>

Fishrock123 · 2015-12-22T18:35:18Z

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],

Fishrock123 · 2015-12-22T18:35:58Z

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

MylesBorins · 2015-12-22T18:37:06Z

@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>

Fishrock123 · 2015-12-23T18:51:15Z

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

zeusdeux · 2015-12-23T19:14:47Z

@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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

zeusdeux commented Dec 1, 2015

Fishrock123 Dec 1, 2015

zeusdeux Dec 2, 2015

Fishrock123 Dec 3, 2015

zeusdeux Dec 4, 2015

zeusdeux commented Dec 2, 2015

Fishrock123 commented Dec 3, 2015

Fishrock123 Dec 3, 2015

zeusdeux commented Dec 4, 2015

Fishrock123 Dec 4, 2015

zeusdeux Dec 7, 2015

Fishrock123 commented Dec 4, 2015

Fishrock123 Dec 4, 2015

zeusdeux commented Dec 7, 2015

Fishrock123 Dec 11, 2015

zeusdeux commented Dec 16, 2015

zeusdeux commented Dec 20, 2015

Fishrock123 Dec 21, 2015

Fishrock123 commented Dec 21, 2015

Fishrock123 commented Dec 22, 2015

Fishrock123 commented Dec 22, 2015

MylesBorins commented Dec 22, 2015

Fishrock123 commented Dec 23, 2015

zeusdeux commented Dec 23, 2015

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

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

Conversation

zeusdeux commented Dec 1, 2015

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

zeusdeux commented Dec 2, 2015

Fishrock123 commented Dec 3, 2015

Choose a reason for hiding this comment

zeusdeux commented Dec 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Dec 4, 2015

Choose a reason for hiding this comment

zeusdeux commented Dec 7, 2015

Choose a reason for hiding this comment

zeusdeux commented Dec 16, 2015

zeusdeux commented Dec 20, 2015

Choose a reason for hiding this comment

Fishrock123 commented Dec 21, 2015

Fishrock123 commented Dec 22, 2015

Fishrock123 commented Dec 22, 2015

MylesBorins commented Dec 22, 2015

Fishrock123 commented Dec 23, 2015

zeusdeux commented Dec 23, 2015