Skip to content

Commit

Permalink
fix: refactoring to pass tests on Windows
Browse files Browse the repository at this point in the history
This is a larger refactoring than I tend to prefer to do in a single
commit, but here goes.

- The path normalization of \ to / is made more comprehensive.

- Checking to ensure we aren't overwriting the cwd is done earlier in
  the unpack process, and more thoroughly, so there is less need for
  repetitive checks later.

- The cwd is checked at the start in our recursive mkdir, saving an
  extra fs.mkdir call which would almost always result in an EEXIST.

- Many edge cases resulting in dangling file descriptors were found and
  addressed.  (Much as I complain about Windows stubbornly refusing to
  delete files currently open, it did come in handy here.)

- The Unpack[MAKEFS] methods are refactored for readability, and no
  longer rely on fall-through behavior which made the sync and async
  versions slightly different in some edge cases.

- Many of the tests were refactored to use async rimraf (the better to
  avoid Windows problems) and more modern tap affordances.

Note: coverage on Windows is not 100%, due to skipping many tests that
use symbolic links.  Given the value of having those code paths covered,
I believe that adding istanbul hints to skip coverage of those portions
of the code would be a bad idea.  And given the complexity and hazards
involved in mocking that much of the filesystem implementation, it's
probably best to just let Windows not have 100% coverage.
  • Loading branch information
isaacs committed Aug 9, 2021
1 parent 3f2e2da commit ce5148e
Show file tree
Hide file tree
Showing 14 changed files with 1,050 additions and 671 deletions.
62 changes: 32 additions & 30 deletions lib/mkdir.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,17 @@ class CwdError extends Error {
const cGet = (cache, key) => cache.get(normPath(key))
const cSet = (cache, key, val) => cache.set(normPath(key), val)

const checkCwd = (dir, cb) => {
fs.stat(dir, (er, st) => {
if (er || !st.isDirectory())
er = new CwdError(dir, er && er.code || 'ENOTDIR')
cb(er)
})
}

module.exports = (dir, opt, cb) => {
dir = normPath(dir)

// if there's any overlap between mask and mode,
// then we'll need an explicit chmod
const umask = opt.umask
Expand Down Expand Up @@ -74,16 +83,12 @@ module.exports = (dir, opt, cb) => {
return done()

if (dir === cwd)
return fs.stat(dir, (er, st) => {
if (er || !st.isDirectory())
er = new CwdError(dir, er && er.code || 'ENOTDIR')
done(er)
})
return checkCwd(dir, done)

if (preserve)
return mkdirp(dir, mode, done)

const sub = path.relative(cwd, dir)
const sub = normPath(path.relative(cwd, dir))
const parts = sub.split('/')
mkdir_(cwd, parts, mode, cache, unlink, cwd, null, done)
}
Expand All @@ -92,22 +97,19 @@ const mkdir_ = (base, parts, mode, cache, unlink, cwd, created, cb) => {
if (!parts.length)
return cb(null, created)
const p = parts.shift()
const part = base + '/' + p
const part = normPath(path.resolve(base + '/' + p))
if (cGet(cache, part))
return mkdir_(part, parts, mode, cache, unlink, cwd, created, cb)
fs.mkdir(part, mode, onmkdir(part, parts, mode, cache, unlink, cwd, created, cb))
}

const onmkdir = (part, parts, mode, cache, unlink, cwd, created, cb) => er => {
if (er) {
if (er.path && path.dirname(er.path) === cwd &&
(er.code === 'ENOTDIR' || er.code === 'ENOENT'))
return cb(new CwdError(cwd, er.code))

fs.lstat(part, (statEr, st) => {
if (statEr)
if (statEr) {
statEr.path = statEr.path && normPath(statEr.path)
cb(statEr)
else if (st.isDirectory())
} else if (st.isDirectory())
mkdir_(part, parts, mode, cache, unlink, cwd, created, cb)
else if (unlink)
fs.unlink(part, er => {
Expand All @@ -126,6 +128,19 @@ const onmkdir = (part, parts, mode, cache, unlink, cwd, created, cb) => er => {
}
}

const checkCwdSync = dir => {
let ok = false
let code = 'ENOTDIR'
try {
ok = fs.statSync(dir).isDirectory()
} catch (er) {
code = er.code
} finally {
if (!ok)
throw new CwdError(dir, code)
}
}

module.exports.sync = (dir, opt) => {
dir = normPath(dir)
// if there's any overlap between mask and mode,
Expand Down Expand Up @@ -157,29 +172,20 @@ module.exports.sync = (dir, opt) => {
return done()

if (dir === cwd) {
let ok = false
let code = 'ENOTDIR'
try {
ok = fs.statSync(dir).isDirectory()
} catch (er) {
code = er.code
} finally {
if (!ok)
throw new CwdError(dir, code)
}
done()
return
checkCwdSync(cwd)
return done()
}

if (preserve)
return done(mkdirp.sync(dir, mode))

const sub = path.relative(cwd, dir)
const sub = normPath(path.relative(cwd, dir))
const parts = sub.split('/')
let created = null
for (let p = parts.shift(), part = cwd;
p && (part += '/' + p);
p = parts.shift()) {
part = normPath(path.resolve(part))
if (cGet(cache, part))
continue

Expand All @@ -188,10 +194,6 @@ module.exports.sync = (dir, opt) => {
created = created || part
cSet(cache, part, true)
} catch (er) {
if (er.path && path.dirname(er.path) === cwd &&
(er.code === 'ENOTDIR' || er.code === 'ENOENT'))
return new CwdError(cwd, er.code)

const st = fs.lstatSync(part)
if (st.isDirectory()) {
cSet(cache, part, true)
Expand Down
2 changes: 1 addition & 1 deletion lib/normalize-windows-path.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,4 @@

const platform = process.env.TESTING_TAR_FAKE_PLATFORM || process.platform
module.exports = platform !== 'win32' ? p => p
: p => p.replace(/\\/g, '/')
: p => p && p.replace(/\\/g, '/')
7 changes: 4 additions & 3 deletions lib/read-entry.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
'use strict'
const types = require('./types.js')
const MiniPass = require('minipass')
const normPath = require('./normalize-windows-path.js')

const SLURP = Symbol('slurp')
module.exports = class ReadEntry extends MiniPass {
Expand Down Expand Up @@ -47,7 +48,7 @@ module.exports = class ReadEntry extends MiniPass {
this.ignore = true
}

this.path = header.path
this.path = normPath(header.path)
this.mode = header.mode
if (this.mode)
this.mode = this.mode & 0o7777
Expand All @@ -59,7 +60,7 @@ module.exports = class ReadEntry extends MiniPass {
this.mtime = header.mtime
this.atime = header.atime
this.ctime = header.ctime
this.linkpath = header.linkpath
this.linkpath = normPath(header.linkpath)
this.uname = header.uname
this.gname = header.gname

Expand Down Expand Up @@ -92,7 +93,7 @@ module.exports = class ReadEntry extends MiniPass {
// a global extended header, because that's weird.
if (ex[k] !== null && ex[k] !== undefined &&
!(global && k === 'path'))
this[k] = ex[k]
this[k] = k === 'path' || k === 'linkpath' ? normPath(ex[k]) : ex[k]
}
}
}
3 changes: 2 additions & 1 deletion lib/replace.js
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,8 @@ const replace = (opt, files, cb) => {

fs.fstat(fd, (er, st) => {
if (er)
return reject(er)
return fs.close(fd, () => reject(er))

getPos(fd, st.size, (er, position) => {
if (er)
return reject(er)
Expand Down
Loading

0 comments on commit ce5148e

Please sign in to comment.