repl: consider removeHistoryDuplicates when saving multiline history

Author: puskin

lib/internal/readline/interface.js

@@ -454,8 +454,9 @@ class Interface extends InterfaceConstructor {
 
     // If the trimmed line is empty then return the line
     if (StringPrototypeTrim(this.line).length === 0) return this.line;
+    const normalizedLine = StringPrototypeReplaceAll(this.line, '\n', '\r');
 
-    if (this.history.length === 0 || this.history[0] !== this.line) {
+    if (this.history.length === 0 || this.history[0] !== normalizedLine) {
       if (this.removeHistoryDuplicates) {
         // Remove older history line if identical to new one
         const dupIndex = ArrayPrototypeIndexOf(this.history, this.line);
@@ -936,14 +937,17 @@ class Interface extends InterfaceConstructor {
   [kMoveDownOrHistoryNext]() {
     const { cols, rows } = this.getCursorPos();
     const splitLine = StringPrototypeSplit(this.line, '\n');
+    if (!this.historyIndex && rows === splitLine.length) {
+      return;
+    }
     // Go to the next history only if the cursor is in the first line of the multiline input.
     // Otherwise treat the "arrow down" as a movement to the next row.
     if (splitLine.length > 1 && rows < splitLine.length - 1) {
       const currentLine = splitLine[rows];
       const nextLine = splitLine[rows + 1];
       const amountToMove = (cols > nextLine.length) ?
-        +cols + nextLine.length - 1 :
-        +currentLine.length + 1;
+        currentLine.length - cols + nextLine.length + 1 :
+        currentLine.length + 1;
       // Go to the same position on the next line, or the end of the next line
       // If the current position does not exist in the next line.
       this[kMoveCursor](amountToMove);
@@ -963,9 +967,7 @@ class Interface extends InterfaceConstructor {
   [kHistoryNext]() {
     if (this.historyIndex >= 0) {
       this[kBeforeEdit](this.line, this.cursor);
-      const isLineMultiline = StringPrototypeIndexOf(this.line, '\n') !== -1;
-
-      const search = isLineMultiline ? '' : this[kSubstringSearch] || '';
+      const search = this[kSubstringSearch] || '';
       let index = this.historyIndex - 1;
       while (
         index >= 0 &&
@@ -987,6 +989,9 @@ class Interface extends InterfaceConstructor {
 
   [kMoveUpOrHistoryPrev]() {
     const { cols, rows } = this.getCursorPos();
+    if (this.historyIndex === this.history.length && rows) {
+      return;
+    }
     const splitLine = StringPrototypeSplit(this.line, '\n');
     // Go to the previous history only if the cursor is in the first line of the multiline input.
     // Otherwise treat the "arrow up" as a movement to the previous row.
@@ -1007,9 +1012,7 @@ class Interface extends InterfaceConstructor {
   [kHistoryPrev]() {
     if (this.historyIndex < this.history.length && this.history.length) {
       this[kBeforeEdit](this.line, this.cursor);
-      const isLineMultiline = StringPrototypeIndexOf(this.line, '\n') !== -1;
-
-      const search = isLineMultiline ? '' : this[kSubstringSearch] || '';
+      const search = this[kSubstringSearch] || '';
       let index = this.historyIndex + 1;
       while (
         index < this.history.length &&
@@ -1122,7 +1125,8 @@ class Interface extends InterfaceConstructor {
       !key.meta &&
       !key.shift
     ) {
-      if (this[kSubstringSearch] === null) {
+      const isLineMultiline = StringPrototypeIndexOf(this.line, '\n') !== -1;
+      if (this[kSubstringSearch] === null && !isLineMultiline) {
         this[kSubstringSearch] = StringPrototypeSlice(
           this.line,
           0,

lib/repl.js

@@ -966,7 +966,10 @@ function REPLServer(prompt,
         ArrayPrototypePop(lines);
         // And replace them with the single command split by '\r'
         ArrayPrototypePush(lines, cmd);
-        ArrayPrototypeUnshift(self.history, ArrayPrototypeJoin(lines, '\r'));
+        const newHistoryLine = ArrayPrototypeJoin(lines, '\r');
+        if (self.history[0] !== newHistoryLine) {
+          ArrayPrototypeUnshift(self.history, newHistoryLine);
+        }
       }
 
       if (e) {

test/parallel/test-repl-history-navigation.js

@@ -767,6 +767,29 @@ const tests = [
     ],
     clean: true
   },
+  {
+    // Test that multiline history is not duplicated
+    env: { NODE_REPL_HISTORY: defaultHistoryPath },
+    skip: !process.features.inspector,
+    test: [
+      'let f = `multiline',
+      ENTER,
+      'string`',
+      ENTER,
+      UP, UP, UP,
+    ],
+    expected: [
+      prompt, ...'let f = `multiline',
+      '| ',
+      ...'string`',
+      'undefined\n',
+      prompt,
+      `${prompt}let f = \`multiline\nstring\``,
+      `${prompt}let f = \`multiline\nstring\``,
+      prompt,
+    ],
+    clean: true
+  },
 ];
 const numtests = tests.length;
 
@@ -814,7 +837,6 @@ function runTest() {
           try {
             assert.strictEqual(output, expected[i]);
           } catch (e) {
-            console.log({ output, expected: expected[i] });
             console.error(`Failed test # ${numtests - tests.length}`);
             console.error('Last outputs: ' + inspect(lastChunks, {
               breakLength: 5, colors: true

test/parallel/test-repl-multiline-navigation.js

@@ -0,0 +1,94 @@
+'use strict';
+
+// Flags: --expose-internals
+
+const common = require('../common');
+
+const assert = require('assert');
+const repl = require('internal/repl');
+const stream = require('stream');
+const fs = require('fs');
+
+class ActionStream extends stream.Stream {
+  run(data) {
+    const _iter = data[Symbol.iterator]();
+    const doAction = () => {
+      const next = _iter.next();
+      if (next.done) {
+        // Close the repl. Note that it must have a clean prompt to do so.
+        this.emit('keypress', '', { ctrl: true, name: 'd' });
+        return;
+      }
+      const action = next.value;
+
+      if (typeof action === 'object') {
+        this.emit('keypress', '', action);
+      }
+      setImmediate(doAction);
+    };
+    doAction();
+  }
+  resume() {}
+  pause() {}
+}
+ActionStream.prototype.readable = true;
+
+function cleanupTmpFile() {
+  try {
+    // Write over the file, clearing any history
+    fs.writeFileSync(defaultHistoryPath, '');
+  } catch (err) {
+    if (err.code === 'ENOENT') return true;
+    throw err;
+  }
+  return true;
+}
+
+const tmpdir = require('../common/tmpdir');
+tmpdir.refresh();
+const defaultHistoryPath = tmpdir.resolve('.node_repl_history');
+
+{
+  cleanupTmpFile();
+  // It moves the cursor at the right places
+  const checkResults = common.mustSucceed((r) => {
+    r.write('let str = `');
+    r.input.run([{ name: 'enter' }]);
+    r.write('111');
+    r.input.run([{ name: 'enter' }]);
+    r.write('22222222222222');
+    r.input.run([{ name: 'enter' }]);
+    r.write('3`');
+    r.input.run([{ name: 'enter' }]);
+    r.input.run([{ name: 'up' }]);
+    assert.strictEqual(r.cursor, 33);
+
+    r.input.run([{ name: 'up' }]);
+    assert.strictEqual(r.cursor, 18);
+
+    r.input.run([{ name: 'right' }]);
+    r.input.run([{ name: 'right' }]);
+    r.input.run([{ name: 'right' }]);
+    r.input.run([{ name: 'right' }]);
+    r.input.run([{ name: 'right' }]);
+    r.input.run([{ name: 'up' }]);
+    assert.strictEqual(r.cursor, 15);
+
+    r.input.run([{ name: 'down' }]);
+    assert.strictEqual(r.cursor, 19);
+  });
+
+  repl.createInternalRepl(
+    { NODE_REPL_HISTORY: defaultHistoryPath },
+    {
+      terminal: true,
+      input: new ActionStream(),
+      output: new stream.Writable({
+        write(chunk, _, next) {
+          next();
+        }
+      }),
+    },
+    checkResults
+  );
+}