fix: refactoring to pass tests on Windows

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.

Author: isaacs

lib/mkdir.js

@@ -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
@@ -73,18 +82,13 @@ module.exports = (dir, opt, cb) => {
   if (cache && cGet(cache, dir) === true)
     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)
-    })
-  }
+  if (dir === cwd)
+    return checkCwd(dir, done)
 
   if (preserve)
     return mkdirp(dir, {mode}).then(made => done(null, made), 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)
 }
@@ -93,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 => {
@@ -127,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,
@@ -158,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
 
@@ -189,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)

lib/normalize-windows-path.js

@@ -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, '/')

lib/read-entry.js

@@ -1,5 +1,6 @@
 'use strict'
 const MiniPass = require('minipass')
+const normPath = require('./normalize-windows-path.js')
 
 const SLURP = Symbol('slurp')
 module.exports = class ReadEntry extends MiniPass {
@@ -46,7 +47,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
@@ -58,7 +59,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
 
@@ -93,7 +94,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]
     }
   }
 }

lib/replace.js

@@ -170,7 +170,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)