Skip to content

Commit

Permalink
fix: Trust the filesystem to move files
Browse files Browse the repository at this point in the history
The original lib/util/move-file.js has gone through many iterations over
the last several years.  It no longer represents anything close to the
original intent.

Most of the functionality it was originally trying to work around is now
something that `@npmcli/fs#moveFile` does, and better.  Trying to move a
file by creating a hard link and unlinking the old file is an
optimization best left to the operating system when you ask it to move a
file.  In fact the `@npmcli/fs#moveFile` method tries a *rename* first,
which is the best choice all around.  It also supports an "overwrite:
false" flag so we can still error out if the destination already exists.

This PR removes all of the old legacy code.
  • Loading branch information
wraithgar committed May 1, 2023
1 parent 8bc4897 commit 265e4ae
Show file tree
Hide file tree
Showing 4 changed files with 25 additions and 261 deletions.
12 changes: 9 additions & 3 deletions lib/content/write.js
Expand Up @@ -4,7 +4,7 @@ const events = require('events')

const contentPath = require('./path')
const fs = require('fs/promises')
const moveFile = require('../util/move-file')
const { moveFile } = require('@npmcli/fs')
const { Minipass } = require('minipass')
const Pipeline = require('minipass-pipeline')
const Flush = require('minipass-flush')
Expand Down Expand Up @@ -161,8 +161,14 @@ async function moveToDestination (tmp, cache, sri, opts) {
const destDir = path.dirname(destination)

await fs.mkdir(destDir, { recursive: true })
await moveFile(tmp.target, destination)
tmp.moved = true
try {
await moveFile(tmp.target, destination, { overwrite: false })
tmp.moved = true
} catch (err) {
if (!err.message.startsWith('The destination file exists')) {
throw Object.assign(err, { code: 'EEXIST' })
}
}
}

function sizeError (expected, found) {
Expand Down
56 changes: 0 additions & 56 deletions lib/util/move-file.js

This file was deleted.

16 changes: 16 additions & 0 deletions test/content/write.js
Expand Up @@ -233,6 +233,22 @@ t.test('cleans up tmp on successful completion', async t => {
})
})

t.test('Handles moveFile error other than EEXIST', async t => {
const write = t.mock('../../lib/content/write.js', {
'@npmcli/fs': {
moveFile: async () => {
throw new Error('Unknown error')
},
},
})
const CONTENT = 'foobarbaz'
const CACHE = t.testdir()
await t.rejects(
write.stream(CACHE).end(CONTENT).promise(),
{ message: 'Unknown error' }
)
})

t.test('cleans up tmp on streaming error', (t) => {
const CONTENT = 'foobarbaz'
const CACHE = t.testdir()
Expand Down
202 changes: 0 additions & 202 deletions test/util/move-file.js

This file was deleted.

0 comments on commit 265e4ae

Please sign in to comment.