Skip to content

repl: support hidden history file on Windows - #12207

Closed
bzoz wants to merge 1 commit into
nodejs:masterfrom
JaneaSystems:bartek-repl-init-hidden-file
Closed

repl: support hidden history file on Windows#12207
bzoz wants to merge 1 commit into
nodejs:masterfrom
JaneaSystems:bartek-repl-init-hidden-file

Conversation

@bzoz

@bzozbzoz commented Apr 4, 2017

Copy link
Copy Markdown
Contributor

On Windows when REPL history file has the hidden attribute node will fail when trying to open it in w mode. This changes the mode to r+. The file is guaranteed to exists because of earlier open call with a+.

Fixes: #5261

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows [commit guidelines][]
Affected core subsystem(s)

repl

@bzoz
repl: support hidden history file on Windows
83887f3
On Windows when REPL history file has the hidden attribute node will
fail when trying to open it in 'w' mode. This changes the mode to
'r+'. The file is guaranteed to exists because of earlier open call
with 'a+'.
@bzozbzoz added the repl Issues and PRs related to the REPL subsystem. label Apr 4, 2017
@vsemozhetbytvsemozhetbyt added the windows Issues and PRs related to the Windows platform. label Apr 4, 2017
Fishrock123
Fishrock123 previously requested changes Apr 4, 2017
View reviewed changes
Comment threadlib/internal/repl.js
}

fs.open(historyPath, 'w', onhandle);
fs.open(historyPath, 'r+', onhandle);

@Fishrock123Fishrock123Apr 4, 2017•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this break creating a new file if none exists? [1]

@Fishrock123
Fishrock123 dismissed their stale reviewApril 4, 2017 15:56

Oops, didn't see the a+.

@Fishrock123

Fishrock123 commented Apr 4, 2017

Copy link
Copy Markdown
Contributor

CI: https://ci.nodejs.org/job/node-test-pull-request/7196/

jasnell
jasnell approved these changes Apr 4, 2017
View reviewed changes

@jasnelljasnell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if CI is green

Fishrock123
Fishrock123 reviewed Apr 4, 2017
View reviewed changes
Comment threadlib/internal/repl.js
if (err) {
return ready(err);
}
fs.ftruncate(hnd, 0, (err) => {

@Fishrock123Fishrock123Apr 4, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I am gathering, the mode change no longer cause overwrites (appends instead) and so we need to "flush" the file?

From the docs though, it sounds like it does writes and not appends, so this shouldn't be necessary?

'r+' - Open file for reading and writing. An exception occurs if the file does not exist.

@bzozbzozApr 5, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previous implementation cleared the file once when opening it with w. This was the only place that would reset history file content - i.e. if one would set NODE_REPL_HISTORY_SIZE to value lower than 1000, then the history file would be trimmed when node starts. This ftruncate call here is to preserve this functionality.

@ghost

ghost commented Apr 5, 2017

Copy link
Copy Markdown

That's a good idea

@bzoz

bzoz commented Apr 20, 2017

Copy link
Copy Markdown
ContributorAuthor

Windows test fail unrelated. Landed in bb041ea

@bzozbzoz closed this Apr 20, 2017
bzoz added a commit that referenced this pull request Apr 20, 2017
@bzoz
repl: support hidden history file on Windows
bb041ea
On Windows when REPL history file has the hidden attribute node will
fail when trying to open it in 'w' mode. This changes the mode to
'r+'. The file is guaranteed to exists because of earlier open call
with 'a+'.
Fixes: #5261
PR-URL: #12207
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request Apr 25, 2017
@bzoz@evanlucas
repl: support hidden history file on Windows
63e957f
On Windows when REPL history file has the hidden attribute node will
fail when trying to open it in 'w' mode. This changes the mode to
'r+'. The file is guaranteed to exists because of earlier open call
with 'a+'.
Fixes: #5261
PR-URL: #12207
Reviewed-By: James M Snell <jasnell@gmail.com>
@evanlucasevanlucas mentioned this pull request May 1, 2017
Merged
evanlucas pushed a commit that referenced this pull request May 1, 2017
@bzoz@evanlucas
repl: support hidden history file on Windows
2e1ed73
On Windows when REPL history file has the hidden attribute node will
fail when trying to open it in 'w' mode. This changes the mode to
'r+'. The file is guaranteed to exists because of earlier open call
with 'a+'.
Fixes: #5261
PR-URL: #12207
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
@bzoz@evanlucas
repl: support hidden history file on Windows
b09bf51
On Windows when REPL history file has the hidden attribute node will
fail when trying to open it in 'w' mode. This changes the mode to
'r+'. The file is guaranteed to exists because of earlier open call
with 'a+'.
Fixes: #5261
PR-URL: #12207
Reviewed-By: James M Snell <jasnell@gmail.com>
@gibfahn

gibfahn commented May 16, 2017

Copy link
Copy Markdown
Member

Should this be backported to v6.x?

@mscdexmscdex mentioned this pull request May 26, 2017
Merged
3 tasks
@mscdex

mscdex commented May 26, 2017

Copy link
Copy Markdown
Contributor

Wherever this gets backported, we also need #12762.

@gibfahngibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
@gibfahn

gibfahn commented Jun 18, 2017

Copy link
Copy Markdown
Member

Should this be backported to v6.x?

Landed this with #12762, LMK if that wasn't a good idea.

gibfahn pushed a commit that referenced this pull request Jun 18, 2017
@bzoz@gibfahn
repl: support hidden history file on Windows
c2afcea
On Windows when REPL history file has the hidden attribute node will
fail when trying to open it in 'w' mode. This changes the mode to
'r+'. The file is guaranteed to exists because of earlier open call
with 'a+'.
Fixes: #5261
PR-URL: #12207
Reviewed-By: James M Snell <jasnell@gmail.com>
@bzoz

bzoz commented Jun 19, 2017

Copy link
Copy Markdown
ContributorAuthor

LGTM

gibfahn pushed a commit that referenced this pull request Jun 20, 2017
@bzoz@gibfahn
repl: support hidden history file on Windows
bd12580
On Windows when REPL history file has the hidden attribute node will
fail when trying to open it in 'w' mode. This changes the mode to
'r+'. The file is guaranteed to exists because of earlier open call
with 'a+'.
Fixes: #5261
PR-URL: #12207
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
@bzoz@MylesBorins
repl: support hidden history file on Windows
b2acb81
On Windows when REPL history file has the hidden attribute node will
fail when trying to open it in 'w' mode. This changes the mode to
'r+'. The file is guaranteed to exists because of earlier open call
with 'a+'.
Fixes: #5261
PR-URL: #12207
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorinsMylesBorins mentioned this pull request Jul 18, 2017
Merged
@seishunseishun mentioned this pull request Jul 31, 2017
Closed
@baby636baby636 mentioned this pull request Jun 22, 2022
Open
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes
+2 more reviewers
@Fishrock123Fishrock123Fishrock123 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

replIssues and PRs related to the REPL subsystem.windowsIssues and PRs related to the Windows platform.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Error: EPERM: operation not permitted, open 'C:\Users\username\.node_repl_history'

6 participants

@bzoz@Fishrock123@gibfahn@mscdex@jasnell@vsemozhetbyt