repl: Reverses order of .node_repl_history

Rationale nodejs#3928.
zeusdeux · Dec 16, 2015 · ae6e61f · ae6e61f
1 parent e5774c9
commit ae6e61f
Show file tree

Hide file tree

Showing 4 changed files with 45 additions and 29 deletions.
diff --git a/lib/internal/repl.js b/lib/internal/repl.js
@@ -119,7 +119,7 @@ function setupHistory(repl, historyPath, oldHistoryPath, ready) {
     }
 
     if (data) {
-      repl.history = data.split(/[\n\r]+/, repl.historySize);
+      repl.history = data.split(/[\n\r]+/).slice(repl.historySize * -1);
     } else if (oldHistoryPath) {
       // Grab data from the older pre-v3.0 JSON NODE_REPL_HISTORY_FILE format.
       repl._writeToOutput(
@@ -132,15 +132,16 @@ function setupHistory(repl, historyPath, oldHistoryPath, ready) {
         if (!Array.isArray(repl.history)) {
           throw new Error('Expected array, got ' + typeof repl.history);
         }
-        repl.history = repl.history.slice(0, repl.historySize);
+        // Pre-v3.0 latest item in history was at index 0
+        // Post #3928, latest item is now at history length - 1
+        repl.history = repl.history.slice(0, repl.historySize).reverse();
       } catch (err) {
         if (err.code !== 'ENOENT') {
           return ready(
             new Error(`Could not parse history data in ${oldHistoryPath}.`));
         }
       }
     }
-
     fs.open(historyPath, 'w', onhandle);
   }
 

diff --git a/lib/readline.js b/lib/readline.js
@@ -142,7 +142,7 @@ function Interface(input, output, completer, terminal) {
     this.cursor = 0;
 
     this.history = [];
-    this.historyIndex = -1;
+    this.historyIndex = null;
 
     if (output !== null && output !== undefined)
       output.on('resize', onresize);
@@ -225,17 +225,17 @@ Interface.prototype._writeToOutput = function _writeToOutput(stringToWrite) {
 };
 
 Interface.prototype._addHistory = function() {
-  if (this.line.length === 0) return '';
+  var historyLength = this.history.length;
 
-  if (this.history.length === 0 || this.history[0] !== this.line) {
-    this.history.unshift(this.line);
+  if (this.line.length === 0) return '';
 
-    // Only store so many
-    if (this.history.length > this.historySize) this.history.pop();
+  if (historyLength === 0 || this.history[historyLength - 1] !== this.line) {
+    // Store only `historySize` no., of records in the history file
+    if (!((historyLength + 1) > this.historySize))
+      historyLength = this.history.push(this.line);
   }
-
-  this.historyIndex = -1;
-  return this.history[0];
+  this.historyIndex = historyLength - 1;
+  return this.history[this.historyIndex];
 };
 
 
@@ -556,28 +556,40 @@ Interface.prototype._line = function() {
 
 
 Interface.prototype._historyNext = function() {
-  if (this.historyIndex > 0) {
-    this.historyIndex--;
-    this.line = this.history[this.historyIndex];
-    this.cursor = this.line.length; // set cursor to end of line.
-    this._refreshLine();
+  // Set historyIndex to the latest item in the history when repl loads
+  // and this is the first action that is invoked
+  if (this.historyIndex === null) this.historyIndex = this.history.length - 1;
 
-  } else if (this.historyIndex === 0) {
-    this.historyIndex = -1;
+  if ((this.historyIndex + 1) === this.history.length) {
     this.cursor = 0;
     this.line = '';
     this._refreshLine();
+  } else if (this.historyIndex < this.history.length) {
+    this.historyIndex++;
+    this.line = this.history[this.historyIndex];
+    this.cursor = this.line.length;
+    this._refreshLine();
   }
 };
 
 
 Interface.prototype._historyPrev = function() {
-  if (this.historyIndex + 1 < this.history.length) {
-    this.historyIndex++;
-    this.line = this.history[this.historyIndex];
-    this.cursor = this.line.length; // set cursor to end of line.
-
-    this._refreshLine();
+  // Set historyIndex to the latest item in the history when repl loads
+  // and this is the first action that is invoked
+  if (this.historyIndex === null) this.historyIndex = this.history.length - 1;
+
+  if (this.history.length) {
+    if (this.historyIndex > 0) {
+      this.line = this.history[this.historyIndex];
+      this.cursor = this.line.length; // set cursor to end of line
+      this.historyIndex--;
+      this._refreshLine();
+    }
+    else {
+      this.line = this.history[0];
+      this.cursor = this.line ? this.line.length : '';
+      this._refreshLine();
+    }
   }
 };
 

diff --git a/test/fixtures/.node_repl_history b/test/fixtures/.node_repl_history
@@ -1,2 +1,2 @@
-'you look fabulous today'
 'Stay Fresh~'
+'you look fabulous today'
diff --git a/test/parallel/test-repl-persistent-history.js b/test/parallel/test-repl-persistent-history.js
@@ -26,6 +26,7 @@ class ActionStream extends stream.Stream {
 
     function doAction() {
       const next = _iter.next();
+
       if (next.done) {
         // Close the repl. Note that it must have a clean prompt to do so.
         setImmediate(function() {
@@ -126,10 +127,10 @@ const tests = [{
     // Sometimes this test used to erase the fixture and I'm not sure why.
     const history = fs.readFileSync(historyFixturePath, 'utf8');
     assert.strictEqual(history,
-                       '\'you look fabulous today\'\n\'Stay Fresh~\'\n');
+                       '\'Stay Fresh~\'\n\'you look fabulous today\'');
     const historyCopy = fs.readFileSync(historyPath, 'utf8');
-    assert.strictEqual(historyCopy, '\'you look fabulous today\'' + os.EOL +
-                                    '\'Stay Fresh~\'' + os.EOL);
+    assert.strictEqual(historyCopy, '\'Stay Fresh~\'' + os.EOL +
+                                    '\'you look fabulous today\'');
   }
 },
 { // Requires the above testcase
@@ -138,6 +139,8 @@ const tests = [{
   expected: [prompt, prompt + '\'42\'', prompt + '\'=^.^=\'', '\'=^.^=\'\n',
              prompt]
 },
+// the test below fails with:
+// AssertionError: '> \'you look fabulous today\'' === '> '
 {
   env: { NODE_REPL_HISTORY: historyPath,
          NODE_REPL_HISTORY_SIZE: 1 },