From 64a398a1441ff83a73c610c9a399a2801b34c67e Mon Sep 17 00:00:00 2001
From: Nate Fischer <ntfschr@google.com>
Date: Thu, 8 Nov 2018 00:25:53 -0800
Subject: [PATCH] refactor: don't expose tempdir in common.state

Previously, the cached `tempdir` value was stored in `common.state`.
Unlike the other `common.state` values, this isn't immediately useful to
other commands (they can just call the tempdir API). So, this moves the
cached value into `tempdir.js`.

This also adds a unit test for the caching behavior, and exposes
test-only helpers to verify this behavior.

Finally, this adds a note to `common.state` that values should generally
be considered read-only, since this can be important for customized
behavior. Although, I recognize our code base has one exception to this
rule (`echo()`), we should strive to maintain this.

Fixes #902
Test: Added a unit test.
---
 src/common.js   |  2 +-
 src/exec.js     |  2 +-
 src/tempdir.js  | 26 +++++++++++++++++++++-----
 test/tempdir.js | 10 ++++++++++
 4 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/src/common.js b/src/common.js
index f873782b..14a49fc5 100644
--- a/src/common.js
+++ b/src/common.js
@@ -42,11 +42,11 @@ var config = {
 config.reset();
 exports.config = config;
 
+// Note: commands should generally consider these as read-only values.
 var state = {
   error: null,
   errorCode: 0,
   currentCmd: 'shell.js',
-  tempDir: null,
 };
 exports.state = state;
 
diff --git a/src/exec.js b/src/exec.js
index a73e2cd4..de3322c8 100644
--- a/src/exec.js
+++ b/src/exec.js
@@ -1,5 +1,5 @@
 var common = require('./common');
-var _tempDir = require('./tempdir');
+var _tempDir = require('./tempdir').tempDir;
 var _pwd = require('./pwd');
 var path = require('path');
 var fs = require('fs');
diff --git a/src/tempdir.js b/src/tempdir.js
index 6fe116fe..b62032d7 100644
--- a/src/tempdir.js
+++ b/src/tempdir.js
@@ -24,6 +24,8 @@ function writeableDir(dir) {
   }
 }
 
+// Variable to cache the tempdir value for successive lookups.
+var cachedTempDir;
 
 //@
 //@ ### tempdir()
@@ -37,10 +39,9 @@ function writeableDir(dir) {
 //@ Searches and returns string containing a writeable, platform-dependent temporary directory.
 //@ Follows Python's [tempfile algorithm](http://docs.python.org/library/tempfile.html#tempfile.tempdir).
 function _tempDir() {
-  var state = common.state;
-  if (state.tempDir) return state.tempDir; // from cache
+  if (cachedTempDir) return cachedTempDir;
 
-  state.tempDir = writeableDir(os.tmpdir()) ||
+  cachedTempDir = writeableDir(os.tmpdir()) ||
                   writeableDir(process.env.TMPDIR) ||
                   writeableDir(process.env.TEMP) ||
                   writeableDir(process.env.TMP) ||
@@ -54,6 +55,21 @@ function _tempDir() {
                   writeableDir('/usr/tmp') ||
                   writeableDir('.'); // last resort
 
-  return state.tempDir;
+  return cachedTempDir;
 }
-module.exports = _tempDir;
+
+// Indicates if the tempdir value is currently cached. This is exposed for tests
+// only. The return value should only be tested for truthiness.
+function isCached() {
+  return cachedTempDir;
+}
+
+// Clears the cached tempDir value, if one is cached. This is exposed for tests
+// only.
+function clearCache() {
+  cachedTempDir = undefined;
+}
+
+module.exports.tempDir = _tempDir;
+module.exports.isCached = isCached;
+module.exports.clearCache = clearCache;
diff --git a/test/tempdir.js b/test/tempdir.js
index ed7ec77f..c9c24754 100644
--- a/test/tempdir.js
+++ b/test/tempdir.js
@@ -3,6 +3,7 @@ import fs from 'fs';
 import test from 'ava';
 
 import shell from '..';
+import { isCached, clearCache } from '../src/tempdir';
 
 shell.config.silent = true;
 
@@ -19,3 +20,12 @@ test('basic usage', t => {
   // It's a directory
   t.truthy(shell.test('-d', tmp));
 });
+
+test('cache', t => {
+  clearCache(); // In case this runs after any test which relies on tempdir().
+  t.falsy(isCached());
+  const tmp1 = shell.tempdir();
+  t.truthy(isCached());
+  const tmp2 = shell.tempdir();
+  t.is(tmp1, tmp2);
+});