repl: Reverses order of .node_repl_history

[zeusdeux]

Rationale -> #3928.

A few tests are still broken for this and I am kinda confused as to why. Could use some help to ready this PR up.

Thanks!

[mscdex]

I'm not sure how large history files can get, but maybe it'd be more efficient to limit the input to the last repl.historySize lines first and then split on that? For example:

var reLastLines = new RegExp('(?:.*[\r\n]+){0,' + repl.historySize + '}$');
repl.history = reLastLines.exec(data)[0].match(/[^\r\n]+/g) || [];

[Fishrock123]

I doubt it would be an issue large enough to necessitate that unless some manually edited in a way-too-big history file.

[Fishrock123]

I think this code may not be correct though, I think it should probably keep the original logic, and then call .reverse()?

[zeusdeux]

@Fishrock123 Since the history file is going to be arranged from least recent to most (most being at the bottom), doing data.split(/[\n\r]+/, repl.historySize) will end up losing the latest history elements if data.length > repl.historySize. The latest history elements will end up getting dropped/truncated as they are at a higher index in data array than the less recent ones.

[Fishrock123]

Oooh good catch.

[Fishrock123]

(Actually, on second thought, this is semver-major anyways)

[Fishrock123]

If these are being reversed, something is probably not correct. :)

[Fishrock123]

@zeusdeux Instead of changing readline, I suggest changing how it is written out to file: https://github.com/nodejs/node/blob/master/lib/internal/repl.js#L173-L182

[rvagg]

Welcome @zeusdeux, I believe this would be your first commit to core if it gets in, thanks for contributing! Even if this doesn't get in, we appreciate the time you've spent discussing and working on this with us.

I'm labelling this one ctc-agenda as it's not clear to me that the pain of switching even in semver-minor is worth is and the rationale seems to be "most other apps that save history do it this way" which doesn't sound very compelling because there's no reason to be compatible with them as far as I can tell.

[zeusdeux]

@rvagg I do have a commit in actually. Got one in quite recently with help from @Fishrock123. I love this community! :)

As for this PR, should I continue working on this then or wait till I hear more?

[rvagg]

@zeusdeux we'll cover this in our CTC meeting tomorrow, no promises on actually getting anywhere with it. Will let you know.

[mscdex]

This is kind of a general idea: if we want to start making changes like this, perhaps we should consider versioning history files (e.g. add some version identifier at the top of the file)? That way we can more easily know how to parse a history file (in the future)?

[jasnell]

This was discussed on the CTC call today. Overall consensus is that there is currently no pressing reason to adopt this change currently and we'd rather keep the history ordering consistent with what's currently in v4 and v5. Unless there is a compelling new reason to make this change, we'd prefer to keep things as they are currently.

[zeusdeux]

@jasnell fair enough. Should I go ahead and just close this then?

[jasnell]

If that's what you wish. If you close but not delete the branch, we can at
least retain the changes so the wish is not lost should we decide later to
go ahead with the modification.
On Jan 7, 2016 5:46 AM, "Mudit Ameta" [email protected] wrote:

@jasnell https://github.com/jasnell fair enough. Should I go ahead and
just close this then?

—
Reply to this email directly or view it on GitHub
#4313 (comment).

[zeusdeux]

I don't mind leaving it open but I fear that might just add unnecessary cognitive load for anyone going through the PRs. On the flipside, the issue (#3928) that prompted this PR is still open so closing this might make for a weird history later on. So, yeah, I am not sure.

As for the branch, sure, we can keep it around.

[bnoordhuis]

Let's close it for now. Like you say, it unclutters the open PRs view.