[Fix #1755] Cache failure to find project root (#1779)

When `projectile-project-root` was unable to find a project root for a
directory, it would not cache the negative result, so future invocations would
repeat the (often expensive) search for a project root every time.

With this change, `projectile-project-root` caches failure to find a project
root, when that failure is expected to be permanent, and consults that cache
for speed on subsequent invocations.  "Expected to be permanent" means that
either we're trying to find the project root of a local directory, or we're
successfully connected to a remote directory via TRAMP.  If the directory isn't
local, but we can't connect to it, we consider that a transient failure and
don't cache it.

Under the hood, this change uses the same `projectile-project-root-cache`
that's used to cache successful attempts to find a project root, but with a new
key `projectilerootless-{dir}`.  This should allow cache invalidation to work
as expected.

3 files changed, 78 insertions, 30 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index c6f5b55..041055a 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -23,6 +23,7 @@
 * [#1765](https://github.com/bbatsov/projectile/issues/1765): Fix `src-dir`/`test-dir` not defaulting to `"src/"` and `"test/"` with `projectile-toggle-between-implementation-and-test`.
 * Fix version extraction logic.
 * [1654](https://github.com/bbatsov/projectile/issues/1654) Fix consecutive duplicates appearing in command history
+* [#1755](https://github.com/bbatsov/projectile/issues/1755) Cache failure to find project root
 
 ### Changes
 
diff --git a/projectile.el b/projectile.el
index d1a71bf..aaa968d 100644
--- a/projectile.el
+++ b/projectile.el
@@ -1189,42 +1189,53 @@ topmost sequence of matched directories.  Nil otherwise."
 (defun projectile-project-root (&optional dir)
   "Retrieves the root directory of a project if available.
 If DIR is not supplied its set to the current directory by default."
-  ;; the cached value will be 'none in the case of no project root (this is to
-  ;; ensure it is not reevaluated each time when not inside a project) so use
-  ;; cl-subst to replace this 'none value with nil so a nil value is used
-  ;; instead
   (let ((dir (or dir default-directory)))
     ;; Back out of any archives, the project will live on the outside and
     ;; searching them is slow.
     (when (and (fboundp 'tramp-archive-file-name-archive)
                (tramp-archive-file-name-p dir))
       (setq dir (file-name-directory (tramp-archive-file-name-archive dir))))
+    ;; the cached value will be 'none in the case of no project root (this is to
+    ;; ensure it is not reevaluated each time when not inside a project) so use
+    ;; cl-subst to replace this 'none value with nil so a nil value is used
+    ;; instead
     (cl-subst nil 'none
-              ;; The `is-local' and `is-connected' variables are
-              ;; used to fix the behavior where Emacs hangs
-              ;; because of Projectile when you open a file over
-              ;; TRAMP. It basically prevents Projectile from
-              ;; trying to find information about files for which
-              ;; it's not possible to get that information right
-              ;; now.
-              (or (let ((is-local (not (file-remote-p dir)))      ;; `true' if the file is local
-                        (is-connected (file-remote-p dir nil t))) ;; `true' if the file is remote AND we are connected to the remote
-                    (when (or is-local is-connected)
-                      ;; Here is where all the magic happens.
-                      ;; We run the functions in `projectile-project-root-functions' until we find a project dir.
-                      (cl-some
-                       (lambda (func)
-                         (let* ((cache-key (format "%s-%s" func dir))
-                                (cache-value (gethash cache-key projectile-project-root-cache)))
-                           (if (and cache-value (file-exists-p cache-value))
-                               cache-value
-                             (let ((value (funcall func (file-truename dir))))
-                               (puthash cache-key value projectile-project-root-cache)
-                               value))))
-                       projectile-project-root-functions)))
-                  ;; set cached to none so is non-nil so we don't try
-                  ;; and look it up again
-                  'none))))
+      (or
+       ;; if we've already failed to find a project dir for this
+       ;; dir, and cached that failure, don't recompute
+       (let* ((cache-key (format "projectilerootless-%s" dir))
+              (cache-value (gethash cache-key projectile-project-root-cache)))
+         cache-value)
+       ;; if the file isn't local, and we're not connected, don't try to
+       ;; find a root now now, but don't cache failure, as we might
+       ;; re-connect.  The `is-local' and `is-connected' variables are
+       ;; used to fix the behavior where Emacs hangs because of
+       ;; Projectile when you open a file over TRAMP. It basically
+       ;; prevents Projectile from trying to find information about
+       ;; files for which it's not possible to get that information
+       ;; right now.
+       (let ((is-local (not (file-remote-p dir)))      ;; `true' if the file is local
+             (is-connected (file-remote-p dir nil t))) ;; `true' if the file is remote AND we are connected to the remote
+         (unless (or is-local is-connected)
+           'none))
+       ;; if the file is local or we're connected to it via TRAMP, run
+       ;; through the project root functions until we find a project dir
+       (cl-some
+        (lambda (func)
+          (let* ((cache-key (format "%s-%s" func dir))
+                 (cache-value (gethash cache-key projectile-project-root-cache)))
+            (if (and cache-value (file-exists-p cache-value))
+                cache-value
+              (let ((value (funcall func (file-truename dir))))
+                (puthash cache-key value projectile-project-root-cache)
+                value))))
+        projectile-project-root-functions)
+       ;; if we get here, we have failed to find a root by all
+       ;; conventional means, and we assume the failure isn't transient
+       ;; / network related, so cache the failure
+       (let ((cache-key (format "projectilerootless-%s" dir)))
+         (puthash cache-key 'none projectile-project-root-cache)
+         'none)))))
 
 (defun projectile-ensure-project (dir)
   "Ensure that DIR is non-nil.
diff --git a/test/projectile-test.el b/test/projectile-test.el
index a83268c..9760e78 100644
--- a/test/projectile-test.el
+++ b/test/projectile-test.el
@@ -691,7 +691,43 @@ Just delegates OPERATION and ARGS for all operations except for`shell-command`'.
                                                  projectile-root-top-down
                                                  projectile-root-top-down-recurring)))
         (projectile-test-should-root-in "projectA/src/" "projectA/src/")
-        (projectile-test-should-root-in "projectA/src/" "projectA/src/html"))))))
+        (projectile-test-should-root-in "projectA/src/" "projectA/src/html")))))
+
+  (it "caches permanent failure to find a project root"
+    (projectile-test-with-sandbox
+     (projectile-test-with-files
+      ("projectA/src/")
+      (let* ((projectile-project-root-functions '())
+             (dir "projectA/src")
+             (cache-key (format "projectilerootless-%s" dir))
+             (projectile-project-root-cache (make-hash-table :test 'equal)))
+        (expect (gethash cache-key projectile-project-root-cache) :to-be nil)
+        (expect (projectile-project-root dir) :to-be nil)
+        ;; now that this has run once, the cache should be populated with 'none
+        (expect (gethash cache-key projectile-project-root-cache) :to-be 'none)
+        ;; but projectile-project-root should still return nil
+        (expect (projectile-project-root dir) :to-be nil)))))
+
+  (it "does not cache transitory failure to find a project root"
+    (projectile-test-with-sandbox
+     (projectile-test-with-files
+      ("projectA/src/")
+      ;; hackish, but override file-remote-p for a moment, which is called in
+      ;; projectile-project-root with 1 argument to test if the file is remote,
+      ;; and 3 arguments to see if the file is connected.  We want to return t
+      ;; when checking if remote, and nil when checking if connected.
+      (cl-letf (((symbol-function 'file-remote-p)
+                 (lambda (&rest args) (eql 1 (length args)))))
+        (let* ((projectile-project-root-functions '())
+               (dir "projectA/src")
+               (cache-key (format "projectilerootless-%s" dir))
+               (projectile-project-root-cache (make-hash-table :test 'equal)))
+          (expect (gethash cache-key projectile-project-root-cache) :to-be nil)
+          (expect (projectile-project-root dir) :to-be nil)
+          ;; since the failure was transitory, there should be nothing cached
+          (expect (gethash cache-key projectile-project-root-cache) :to-be nil)
+          ;; and projectile-project-root should still return nil
+          (expect (projectile-project-root dir) :to-be nil)))))))
 
 (describe "projectile-file-exists-p"
   (it "returns t if file exists"