Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

repl: fix /dev/null history file regression #12762

Merged
merged 1 commit into from
May 2, 2017
Merged

repl: fix /dev/null history file regression #12762

merged 1 commit into from
May 2, 2017
+20 −16

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Apr 30, 2017 •
edited
Loading

This fixes a regression from bb041ea where ftruncate() fails on a file symlinked to /dev/null.

/cc @bzoz @jasnell

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

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

@mscdex mscdex added the repl Issues and PRs related to the REPL subsystem. label Apr 30, 2017
@nodejs-github-bot nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Apr 30, 2017
Fishrock123
Fishrock123 approved these changes Apr 30, 2017
View reviewed changes
Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

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

lgtm I suppose

test/parallel/test-repl-persistent-history.js Outdated
@@ -178,6 +179,21 @@ const tests = [
test: [UP],
expected: [prompt]
},
{
before: function before() {
if (!common.isWindows) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test case passes on Windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because like the test case before it, it will just act like an empty file on other platforms.

Fishrock123
Fishrock123 reviewed Apr 30, 2017
View reviewed changes
test/parallel/test-repl-persistent-history.js Outdated
}
}
},
env: { NODE_REPL_HISTORY: devNullHistoryPath },
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, you can't just set this to /dev/null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, because I believe Windows would then fail since /dev/null does not exists there and currently there is no way to conditionally skip a test from running, you can only make it fall back to the empty history file scenario.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, makes sense.

@mscdex mscdex force-pushed the repl-history-regression branch 2 times, most recently from 025b12c to 24326a0 Compare May 2, 2017 06:48
@mscdex
Copy link
Contributor Author

mscdex commented May 2, 2017 •
edited
Loading

I've made a slight tweak to the test so that it creates the symlink in a more appropriate place now (the temp dir. instead of fixtures).

CI once again: https://ci.nodejs.org/job/node-test-pull-request/7791/

jasnell
jasnell approved these changes May 2, 2017
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Ugh. Ok. Thanks for including the test.

@mscdex
repl: fix /dev/null history file regression
c20e87a 
This fixes a regression from 83887f3 where ftruncate() fails on
a file symlinked to /dev/null.

PR-URL: nodejs#12762
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@mscdex mscdex merged commit c20e87a into nodejs:master May 2, 2017
@mscdex mscdex deleted the repl-history-regression branch May 2, 2017 22:51
anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
@mscdex
repl: fix /dev/null history file regression
bb9663a 
This fixes a regression from 83887f3 where ftruncate() fails on
a file symlinked to /dev/null.

PR-URL: nodejs#12762
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell jasnell mentioned this pull request May 11, 2017
Closed
@mscdex
Copy link
Contributor Author

mscdex commented May 26, 2017 •
edited
Loading

I just noticed that the problematic commit made its way into v7.x without this fix :-(, so I've updated the tags accordingly to match that of the PR for that commit.

@mscdex mscdex mentioned this pull request May 26, 2017
Closed
3 tasks
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
gibfahn pushed a commit that referenced this pull request Jun 18, 2017
@mscdex @gibfahn
repl: fix /dev/null history file regression
7714f97 
This fixes a regression from 83887f3 where ftruncate() fails on
a file symlinked to /dev/null.

PR-URL: #12762
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
@mscdex @gibfahn
repl: fix /dev/null history file regression
a7129fb 
This fixes a regression from 83887f3 where ftruncate() fails on
a file symlinked to /dev/null.

PR-URL: #12762
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
@mscdex @MylesBorins
repl: fix /dev/null history file regression
bd323a7 
This fixes a regression from 83887f3 where ftruncate() fails on
a file symlinked to /dev/null.

PR-URL: #12762
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@Fishrock123 Fishrock123 Fishrock123 approved these changes

@cjihrig cjihrig cjihrig approved these changes

@bzoz bzoz bzoz approved these changes

Assignees
No one assigned
Labels
repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@mscdex @jasnell @addaleax @Fishrock123 @cjihrig @bzoz @gibfahn @nodejs-github-bot