Don't let-bind default-directory across switch-project-action

CHANGELOG.md

@@ -29,6 +29,8 @@
 
 ### Bugs fixed
 
+* Stop mutating the calling buffer's `default-directory` for the duration of `projectile-switch-project-action`. The wrapper let-bound `default-directory` in the caller, but `default-directory` is auto-buffer-local, so the binding leaked into the caller's buffer-local value for the duration of the action. Switch-project actions that iterate `(buffer-list)` and read each buffer's `default-directory` (e.g. `projectile-switch-to-buffer` as a switch-project-action) misclassified the originating buffer as belonging to the target project. The directory is now set on the temporary buffer the action runs in, not the caller.
+
 * `projectile-discard-root-cache' and `projectile-invalidate-cache' now also clear `projectile-file-exists-cache'. Without this, after creating a new project marker (`.projectile', `.git', etc.) over TRAMP, the negative entries cached during earlier root-walks would keep reporting "not found" for up to `projectile-file-exists-remote-cache-expire' seconds even after the user explicitly invalidated the root cache.
 * `projectile-find-file-hook-function` no longer disables *all* of Projectile for remote buffers - the cheap operations (file caching, known-projects tracking, project buffer-count cap) now run regardless of remoteness, and only the genuinely slow ones (mode-line update, tags-table visit) stay gated. Previously the entire hook was a single `(unless (file-remote-p ...))', which meant remote projects never showed up in known-projects auto-tracking and never had their files cached on visit.
 * Stop stat'ing remote known projects in `projectile-keep-project-p`. The "remote and connected" branch used `file-readable-p`, which is a remote round-trip per project; with `projectile-auto-cleanup-known-projects' enabled, that turned every project switch into a serial network walk over all remote known projects. Remote projects are now always kept; users can drop dead ones with `projectile-remove-known-project'.

projectile.el

@@ -6404,23 +6404,33 @@ With a prefix ARG invokes `projectile-commander' instead of
                                    'projectile-commander
                                  projectile-switch-project-action)))
     (run-hooks 'projectile-before-switch-project-hook)
-    (let* ((default-directory project-to-switch)
-           (switched-buffer
-            ;; use a temporary buffer to load PROJECT-TO-SWITCH's dir-locals
-            ;; before calling SWITCH-PROJECT-ACTION
-            (with-temp-buffer
-              (hack-dir-local-variables-non-file-buffer)
-              ;; Normally the project name is determined from the current
-              ;; buffer. However, when we're switching projects, we want to
-              ;; show the name of the project being switched to, rather than
-              ;; the current project, in the minibuffer. This is a simple hack
-              ;; to tell the `projectile-project-name' function to ignore the
-              ;; current buffer and the caching mechanism, and just return the
-              ;; value of the `projectile-project-name' variable.
-              (let ((projectile-project-name (funcall projectile-project-name-function
-                                                      project-to-switch)))
-                (funcall switch-project-action)
-                (current-buffer)))))
+    (let ((switched-buffer
+           ;; use a temporary buffer to load PROJECT-TO-SWITCH's dir-locals
+           ;; before calling SWITCH-PROJECT-ACTION
+           (with-temp-buffer
+             ;; Set `default-directory' on the temporary buffer rather than
+             ;; let-binding it in the caller.  `default-directory' is
+             ;; auto-buffer-local, so a `let' in the caller mutates the
+             ;; caller's local value for the duration of the action.  When
+             ;; the action iterates `(buffer-list)' (e.g.
+             ;; `projectile-switch-to-buffer' as a switch-project-action),
+             ;; `projectile-project-buffer-p' reads each buffer's
+             ;; `default-directory' via `with-current-buffer' and the caller's
+             ;; transiently-mutated value misclassifies it as a project
+             ;; buffer of the target project.
+             (setq-local default-directory project-to-switch)
+             (hack-dir-local-variables-non-file-buffer)
+             ;; Normally the project name is determined from the current
+             ;; buffer. However, when we're switching projects, we want to
+             ;; show the name of the project being switched to, rather than
+             ;; the current project, in the minibuffer. This is a simple hack
+             ;; to tell the `projectile-project-name' function to ignore the
+             ;; current buffer and the caching mechanism, and just return the
+             ;; value of the `projectile-project-name' variable.
+             (let ((projectile-project-name (funcall projectile-project-name-function
+                                                     project-to-switch)))
+               (funcall switch-project-action)
+               (current-buffer)))))
       ;; If switch-project-action switched buffers then with-temp-buffer will
       ;; have lost that change, so switch back to the correct buffer.
       (when (buffer-live-p switched-buffer)

test/projectile-test.el

@@ -2267,7 +2267,38 @@ by `projectile-files-via-ext-command')."
             (projectile-switch-project-by-name project-dir)
             (expect switch-project-root :to-equal project-dir)
             (expect switch-project-vcs :to-equal 'hg)
-            (expect switch-project-root :not :to-equal (file-name-as-directory (expand-file-name "~")))))))))
+            (expect switch-project-root :not :to-equal (file-name-as-directory (expand-file-name "~"))))))))
+
+  (it "does not mutate the calling buffer's default-directory while running the action"
+      ;; Regression test: a previous implementation wrapped the action in
+      ;; (let* ((default-directory project-to-switch)) ...).  Because
+      ;; `default-directory' is auto-buffer-local, that let-binding mutated
+      ;; the calling buffer's local value for the duration of the action.
+      ;; Actions that iterate (buffer-list) and read each buffer's
+      ;; `default-directory' (e.g. `projectile-switch-to-buffer' as a
+      ;; switch-project-action) would then misclassify the calling buffer
+      ;; as belonging to the target project.
+      (let* ((origin-buffer nil)
+             (origin-dd nil)
+             (observed nil)
+             (projectile-switch-project-action
+              (lambda ()
+                (setq observed (with-current-buffer origin-buffer
+                                 default-directory)))))
+        (projectile-test-with-sandbox
+         (projectile-test-with-files
+          ("origin/" "project/" "project/.projectile")
+          (setq origin-buffer (get-buffer-create "*projectile-test-origin*"))
+          (setq origin-dd (file-name-as-directory (expand-file-name "origin")))
+          (with-current-buffer origin-buffer
+            (setq-local default-directory origin-dd))
+          (let ((project-dir (file-name-as-directory (expand-file-name "project"))))
+            (projectile-add-known-project project-dir)
+            (unwind-protect
+                (with-current-buffer origin-buffer
+                  (projectile-switch-project-by-name project-dir))
+              (kill-buffer origin-buffer)))
+          (expect observed :to-equal origin-dd))))))
 
 (describe "projectile-ignored-buffer-p"
   (it "checks if buffer should be ignored"