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.

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

Create file recovery directory if it doesn't already exist #17255

Merged
merged 4 commits into from May 1, 2018

Conversation

Projects
None yet
3 participants
@byronigoe
Contributor

byronigoe commented May 1, 2018

Fixes #17013 by creating the recovery directory

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

From v1.24.0 to v1.25.0-beta0 a regression issue was created by replacing fs-plus.copyFileSync() with a local function that does a stream copy. The problem is that the new function does not create the directory first, but fs-plus.copyFileSync() did.

I've added the same mkdirp.sync() call that fs-plus uses in copyFileSync().

Alternate Designs

It's also possible to revert to using copyFileSync() from fs-plus, but I presume the change was for a good reason.

Why Should This Be In Core?

The FileRecoveryService is core functionality.

Benefits

Fixes very significant problems that folks have been experiencing with git

Possible Drawbacks

None that I can see.

Verification Process

I was not able to reproduce the issue with this change in place.

Applicable Issues

#17013
atom/github#1418
#17223
#17235
atom/github#1283
atom/github#1375
#11384
#16420
#17179
#17187

@daviwil daviwil changed the title from Create recovery directory to Create file recovery directory if it doesn't already exist May 1, 2018

@maxbrunsfeld

This comment has been minimized.

Contributor

maxbrunsfeld commented May 1, 2018

It looks like the regression was caused by me in #16592.

@@ -146,6 +147,7 @@ async function tryStatFile (path) {
}
async function copyFile (source, destination, mode) {
mkdirp.sync(Path.dirname(destination))

This comment has been minimized.

@maxbrunsfeld

maxbrunsfeld May 1, 2018

Contributor

Can you use the async mkdirp function instead?

@maxbrunsfeld maxbrunsfeld self-assigned this May 1, 2018

@@ -146,6 +147,7 @@ async function tryStatFile (path) {
}
async function copyFile (source, destination, mode) {
await mkdirp(Path.dirname(destination))

This comment has been minimized.

@maxbrunsfeld

maxbrunsfeld May 1, 2018

Contributor

Does this work? From the mkdirp docs, I didn't think it returned a promise; I thought you had to pass a callback.

This comment has been minimized.

@Arcanemagus

Arcanemagus May 1, 2018

Contributor

The current code at least requires the use of a callback, so this isn't going to actually await properly.

This comment has been minimized.

@byronigoe

byronigoe May 1, 2018

Contributor

Can someone help me rework the code? I am having trouble understanding how to use the callback and return a promise.

@maxbrunsfeld

This comment has been minimized.

Contributor

maxbrunsfeld commented May 1, 2018

@byronigoe It looks like there's a pre-existing problem with CI on master. I'm looking into that now.

Update - I think the CI problem is fixed now.

@maxbrunsfeld

This comment has been minimized.

Contributor

maxbrunsfeld commented May 1, 2018

I've pushed a fix to your branch. Here's the whitespace-insensitive diff.

@byronigoe

This comment has been minimized.

Contributor

byronigoe commented May 1, 2018

Thanks. Is this all set to merge? This PR is from my "master" branch, which is bad form. Will I be able to continue committing to my master after this PR is merged/closed without screwing things up?

@maxbrunsfeld

This comment has been minimized.

Contributor

maxbrunsfeld commented May 1, 2018

Yeah, thanks so much for tracking this bug down. After we merge this, I'd suggest just resetting your master branch back to atom/atom's master.

@maxbrunsfeld maxbrunsfeld merged commit 3dfacaf into atom:master May 1, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment