Merge pull request #1993 from bbatsov/dirconfig-improvements

Improvements to .projectile (dirconfig) parsing and ergonomics

4 files changed, 323 insertions, 18 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 654862e..40a5c62 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,12 +4,15 @@
 
 ### New features
 
+* Warn once per session when `projectile-indexing-method' is `alien' but the project has a non-empty `.projectile' file, so users notice their dirconfig rules are being bypassed. Controlled by the new `projectile-warn-when-dirconfig-is-ignored' option.
+* Warn when a `+' keep entry in `.projectile' contains glob metacharacters. The `+' prefix is for subdirectory paths only and globs are silently coerced into a non-matching directory name; the warning surfaces the misuse rather than letting it fail silently.
 * [#1964](https://github.com/bbatsov/projectile/issues/1964): Implement `project-name` and `project-buffers` methods for the `project.el` integration, so that code using `project.el` APIs returns correct results for Projectile-managed projects.
 * [#1837](https://github.com/bbatsov/projectile/issues/1837): Add `eat` project terminal commands with keybindings `x x` and `x 4 x`.
 * Add keybinding `A` (in the projectile command map) and a menu entry for `projectile-add-known-project`.
 
 ### Bugs fixed
 
+* [#1508](https://github.com/bbatsov/projectile/issues/1508): Fix dirconfig parser silently treating lines as ignore patterns when the `+`/`-`/`!` prefix or the comment character is preceded by whitespace; leading spaces and tabs are now skipped before prefix dispatch.
 * Fix `projectile-files-via-ext-command` executing empty string as shell command for non-git VCS sub-projects.
 * Fix `projectile-select-files` crashing on filenames with regexp metacharacters by using `string-search` instead of `string-match`.
 * Fix `projectile-find-references` using internal `xref--show-xrefs` API whose signature changed across Emacs versions.
diff --git a/doc/modules/ROOT/pages/projects.adoc b/doc/modules/ROOT/pages/projects.adoc
index 16b38f8..f1f4a5f 100644
--- a/doc/modules/ROOT/pages/projects.adoc
+++ b/doc/modules/ROOT/pages/projects.adoc
@@ -866,11 +866,35 @@ When a path is overridden, its contents are still subject to ignore
 patterns. To override those files as well, specify their full path
 with a bang prefix.
 
+==== Path entries vs. glob patterns
+
+The two ignore examples above look similar but go through different
+matchers. An entry that begins with a slash (e.g. `-/log`,
+`+/src/foo`, `!/src/foo`) is treated as a *path* relative to the
+project root and is expanded literally. An entry without a leading
+slash (e.g. `-tmp`, `-*.rb`) is treated as a *glob pattern* applied
+to every file's path. As a consequence:
+
+* `-/log` ignores only the top-level `log` directory.
+* `-log` ignores anything called `log` at any depth, but only matches
+  full path components — it will not match `xlog` or `log.txt`.
+* `-*.rb` ignores any file whose path matches the glob `*.rb`.
+
+If a glob pattern doesn't behave the way you'd expect — particularly
+across nested directories — try the explicit path form first to
+confirm whether the file is being indexed at all.
+
+==== Comments
+
 If you would like to include comment lines in your .projectile file,
 you can customize the variable `projectile-dirconfig-comment-prefix`.
 Assigning it a non-nil character value, e.g. `#`, will cause lines in
-the `.projectile` file starting with that character to be treated as
-comments instead of patterns.
+the `.projectile` file whose first non-whitespace character matches
+that character to be treated as comments instead of patterns.
+
+The same is true of the `+`, `-`, and `!` prefixes: leading spaces
+and tabs before the prefix are skipped, so accidental indentation
+won't silently turn the entry into a literal ignore pattern.
 
 === Ignored files using the project indexing tools
 
diff --git a/projectile.el b/projectile.el
index 0584f66..7f2b754 100644
--- a/projectile.el
+++ b/projectile.el
@@ -404,6 +404,17 @@ Similar to '#' in .gitignore files."
   :type 'character
   :package-version '(projectile . "2.2.0"))
 
+(defcustom projectile-warn-when-dirconfig-is-ignored t
+  "Whether to warn when a non-empty .projectile is bypassed by alien indexing.
+Under the `alien' indexing method, Projectile does not consult the
+project's dirconfig file at indexing time.  When this option is
+non-nil, a one-time warning is shown for each project where a
+non-empty dirconfig is present alongside alien indexing, since the
+silent bypass is a frequent source of confusion."
+  :group 'projectile
+  :type 'boolean
+  :package-version '(projectile . "2.10.0"))
+
 (defcustom projectile-globally-ignored-files
   (list projectile-tags-file-name projectile-cache-file)
   "A list of files globally ignored by projectile.
@@ -653,6 +664,9 @@ project."
   "Cache for parsed dirconfig files, keyed by project root.
 Each value is a cons of (MTIME . PARSED-RESULT).")
 
+(defvar projectile--alien-dirconfig-warned-projects (make-hash-table :test 'equal)
+  "Set of project roots already warned about alien indexing skipping the dirconfig.")
+
 (defvar projectile-known-projects nil
   "List of locations where we have previously seen projects.
 The list of projects is ordered by the time they have been accessed.
@@ -2162,6 +2176,18 @@ Unignored files/directories are not included."
   "Return the absolute path to the project's dirconfig file."
   (expand-file-name projectile-dirconfig-file (projectile-project-root)))
 
+(defun projectile--warn-glob-in-keep-entry (entry dirconfig)
+  "Warn that ENTRY in DIRCONFIG looks like a glob pattern after a `+'.
+The `+' prefix is for subdirectories only; the parser silently coerces
+each entry to a directory, so a glob pattern would never match."
+  (display-warning
+   'projectile
+   (format "%s contains `+%s', but `+' entries are treated as \
+subdirectory paths and globs are not expanded.  Use a plain directory \
+or move the pattern to a `-'/`!' rule."
+           dirconfig entry)
+   :warning))
+
 (defun projectile--parse-dirconfig-file-uncached ()
   "Parse the dirconfig file without caching.
 Returns a list of (KEEP IGNORE ENSURE) or nil if the file doesn't exist."
@@ -2170,6 +2196,10 @@ Returns a list of (KEEP IGNORE ENSURE) or nil if the file doesn't exist."
       (with-temp-buffer
         (insert-file-contents dirconfig)
         (while (not (eobp))
+          ;; Skip leading whitespace so prefix dispatch isn't defeated by
+          ;; an accidental space or tab before the +/-/! marker or the
+          ;; configured comment character.
+          (skip-chars-forward " \t")
           (pcase (char-after)
             ;; ignore comment lines if prefix char has been set
             ((pred (lambda (leading-char)
@@ -2182,25 +2212,35 @@ Returns a list of (KEEP IGNORE ENSURE) or nil if the file doesn't exist."
             (?! (push (buffer-substring (1+ (point)) (line-end-position)) ensure))
             (_ (push (buffer-substring (point) (line-end-position)) ignore)))
           (forward-line)))
-      (list (mapcar (lambda (f) (file-name-as-directory (string-trim f)))
-                    (delete "" (reverse keep)))
-            (mapcar #'string-trim
-                    (delete "" (reverse ignore)))
-            (mapcar #'string-trim
-                    (delete "" (reverse ensure)))))))
+      (let ((trimmed-keep (mapcar #'string-trim (delete "" (reverse keep)))))
+        (dolist (entry trimmed-keep)
+          (when (string-match-p "[*?[]" entry)
+            (projectile--warn-glob-in-keep-entry entry dirconfig)))
+        (list (mapcar #'file-name-as-directory trimmed-keep)
+              (mapcar #'string-trim
+                      (delete "" (reverse ignore)))
+              (mapcar #'string-trim
+                      (delete "" (reverse ensure))))))))
 
 (defun projectile-parse-dirconfig-file ()
   "Parse project ignore file and return directories to ignore and keep.
 
-The return value will be a list of three elements, the car being
-the list of directories to keep, the cadr being the list of files
-or directories to ignore, and the caddr being the list of files
-or directories to ensure.
+The return value is a list of three elements: the car is the list
+of directories to keep, the cadr is the list of files or
+directories to ignore, and the caddr is the list of files or
+directories to ensure (i.e. forcibly include even when otherwise
+ignored).
+
+Lines are dispatched on their first non-whitespace character:
+
+  +  add to the keep list
+  -  add to the ignore list
+  !  add to the ensure list
 
-Strings starting with + will be added to the list of directories
-to keep, and strings starting with - will be added to the list of
-directories to ignore.  For backward compatibility, without a
-prefix the string will be assumed to be an ignore string.
+Without a prefix, the line is assumed to be an ignore pattern, for
+backward compatibility.  When `projectile-dirconfig-comment-prefix'
+is non-nil, lines whose first non-whitespace character matches it
+are treated as comments.
 
 Results are cached per project root and invalidated when the
 dirconfig file's modification time changes."
@@ -2277,6 +2317,30 @@ project-root for every file."
         (funcall action res)
       res)))
 
+(defun projectile--dirconfig-non-empty-p ()
+  "Return non-nil if the current project's dirconfig file has any content."
+  (let* ((dirconfig (projectile-dirconfig-file))
+         (attrs (and (projectile-file-exists-p dirconfig)
+                     (file-attributes dirconfig))))
+    (and attrs (> (file-attribute-size attrs) 0))))
+
+(defun projectile--maybe-warn-dirconfig-ignored (project-root)
+  "Warn once per session that PROJECT-ROOT's dirconfig is bypassed by alien mode."
+  (when (and projectile-warn-when-dirconfig-is-ignored
+             (eq projectile-indexing-method 'alien)
+             (not (gethash project-root
+                           projectile--alien-dirconfig-warned-projects))
+             (projectile--dirconfig-non-empty-p))
+    (puthash project-root t projectile--alien-dirconfig-warned-projects)
+    (display-warning
+     'projectile
+     (format "Project %s has a non-empty %s but `projectile-indexing-method' \
+is `alien', which bypasses dirconfig filtering.  Switch to `hybrid' or \
+`native' if you need those rules to apply, or set \
+`projectile-warn-when-dirconfig-is-ignored' to nil to silence this warning."
+             project-root projectile-dirconfig-file)
+     :warning)))
+
 (defun projectile-project-files (project-root)
   "Return a list of files for the PROJECT-ROOT."
   (let (files)
@@ -2306,7 +2370,9 @@ project-root for every file."
             (if (eq projectile-indexing-method 'alien)
                 ;; In alien mode we can just skip reading
                 ;; .projectile and find all files in the root dir.
-                (projectile-dir-files-alien project-root)
+                (progn
+                  (projectile--maybe-warn-dirconfig-ignored project-root)
+                  (projectile-dir-files-alien project-root))
               ;; If a project is defined as a list of subfolders
               ;; then we'll have the files returned for each subfolder,
               ;; so they are relative to the project root.
diff --git a/test/projectile-test.el b/test/projectile-test.el
index 3b67734..7e1e2a1 100644
--- a/test/projectile-test.el
+++ b/test/projectile-test.el
@@ -549,7 +549,219 @@ Just delegates OPERATION and ARGS for all operations except for`shell-command`'.
 							     "left-wspace"
 							     "right-wspace")
 							    nil)))
-    ))
+    )
+  (it "skips leading whitespace before dispatching on the prefix"
+    (spy-on 'file-exists-p :and-return-value t)
+    (spy-on 'insert-file-contents :and-call-fake
+            (lambda (_filename)
+              (save-excursion
+                (insert "  -indented-exclude\n"
+                        "\t+indented-include\n"
+                        " !indented-ensure\n"
+                        "  no-prefix-indented\n"))))
+    (expect (projectile-parse-dirconfig-file)
+            :to-equal '(("indented-include/")
+                        ("indented-exclude" "no-prefix-indented")
+                        ("indented-ensure"))))
+  (it "treats indented comment-prefix lines as comments"
+    (spy-on 'file-exists-p :and-return-value t)
+    (spy-on 'insert-file-contents :and-call-fake
+            (lambda (_filename)
+              (save-excursion
+                (insert "  # indented comment\n"
+                        "-keep-this\n"))))
+    (let ((projectile-dirconfig-comment-prefix ?#))
+      (expect (projectile-parse-dirconfig-file)
+              :to-equal '(nil ("keep-this") nil))))
+  (it "warns when a + keep entry contains glob metacharacters"
+    (spy-on 'file-exists-p :and-return-value t)
+    (spy-on 'insert-file-contents :and-call-fake
+            (lambda (_filename)
+              (save-excursion (insert "+/*.json\n+/src\n"))))
+    (spy-on 'display-warning)
+    (projectile-parse-dirconfig-file)
+    (expect 'display-warning :to-have-been-called-times 1))
+  (it "does not warn for plain + subdirectory entries"
+    (spy-on 'file-exists-p :and-return-value t)
+    (spy-on 'insert-file-contents :and-call-fake
+            (lambda (_filename)
+              (save-excursion (insert "+/src\n+/tests/foo\n"))))
+    (spy-on 'display-warning)
+    (projectile-parse-dirconfig-file)
+    (expect 'display-warning :not :to-have-been-called))
+  (it "does not warn for - ignore entries that contain globs"
+    (spy-on 'file-exists-p :and-return-value t)
+    (spy-on 'insert-file-contents :and-call-fake
+            (lambda (_filename)
+              (save-excursion (insert "-*.json\n-build/*.tmp\n"))))
+    (spy-on 'display-warning)
+    (projectile-parse-dirconfig-file)
+    (expect 'display-warning :not :to-have-been-called)))
+
+(describe "projectile-parse-dirconfig-file with a real file"
+  (before-each
+    (clrhash projectile--dirconfig-cache))
+  (it "parses a mix of keep, ignore, ensure and unprefixed entries from disk"
+    (projectile-test-with-sandbox
+     (projectile-test-with-files
+      ("project/.projectile")
+      (let ((root (file-truename (expand-file-name "project/"))))
+        (with-temp-file (expand-file-name ".projectile" root)
+          (insert "+/src\n"
+                  "-/build\n"
+                  "!/build/keepme\n"
+                  "stale-pattern\n"))
+        (spy-on 'projectile-project-root :and-return-value root)
+        (expect (projectile-parse-dirconfig-file)
+                :to-equal '(("/src/")
+                            ("/build" "stale-pattern")
+                            ("/build/keepme")))))))
+  (it "round-trips non-ASCII paths through the parser"
+    (projectile-test-with-sandbox
+     (projectile-test-with-files
+      ("project/.projectile")
+      (let ((root (file-truename (expand-file-name "project/")))
+            (coding-system-for-write 'utf-8-unix))
+        (with-temp-file (expand-file-name ".projectile" root)
+          (insert "-héllo/wörld\n"
+                  "+/プロジェクト\n"))
+        (spy-on 'projectile-project-root :and-return-value root)
+        (expect (projectile-parse-dirconfig-file)
+                :to-equal '(("/プロジェクト/")
+                            ("héllo/wörld")
+                            nil))))))
+  (it "tolerates a trailing line without a final newline"
+    (projectile-test-with-sandbox
+     (projectile-test-with-files
+      ("project/.projectile")
+      (let ((root (file-truename (expand-file-name "project/"))))
+        (with-temp-file (expand-file-name ".projectile" root)
+          (insert "-foo\n-bar"))
+        (spy-on 'projectile-project-root :and-return-value root)
+        (expect (cadr (projectile-parse-dirconfig-file))
+                :to-equal '("foo" "bar")))))))
+
+(describe "dirconfig cache"
+  (before-each
+    (clrhash projectile--dirconfig-cache))
+  (it "memoizes the parsed result while the file is unchanged"
+    (projectile-test-with-sandbox
+     (projectile-test-with-files
+      ("project/.projectile")
+      (let ((root (file-truename (expand-file-name "project/"))))
+        (with-temp-file (expand-file-name ".projectile" root)
+          (insert "-foo\n"))
+        (spy-on 'projectile-project-root :and-return-value root)
+        (spy-on 'projectile--parse-dirconfig-file-uncached
+                :and-call-through)
+        (projectile-parse-dirconfig-file)
+        (projectile-parse-dirconfig-file)
+        (projectile-parse-dirconfig-file)
+        (expect 'projectile--parse-dirconfig-file-uncached
+                :to-have-been-called-times 1)))))
+  (it "re-parses when the dirconfig file's mtime changes"
+    (projectile-test-with-sandbox
+     (projectile-test-with-files
+      ("project/.projectile")
+      (let* ((root (file-truename (expand-file-name "project/")))
+             (dirconfig (expand-file-name ".projectile" root)))
+        (with-temp-file dirconfig (insert "-foo\n"))
+        (spy-on 'projectile-project-root :and-return-value root)
+        (expect (cadr (projectile-parse-dirconfig-file))
+                :to-equal '("foo"))
+        ;; Force a distinct mtime — file-attribute-modification-time has
+        ;; second-level resolution on some filesystems.
+        (set-file-times dirconfig (time-add (current-time) 5))
+        (with-temp-file dirconfig (insert "-bar\n"))
+        (set-file-times dirconfig (time-add (current-time) 5))
+        (expect (cadr (projectile-parse-dirconfig-file))
+                :to-equal '("bar"))))))
+  (it "returns nil and does not cache when the dirconfig file is absent"
+    (projectile-test-with-sandbox
+     (projectile-test-with-files
+      ("project/")
+      (let ((root (file-truename (expand-file-name "project/"))))
+        (spy-on 'projectile-project-root :and-return-value root)
+        (expect (projectile-parse-dirconfig-file) :to-be nil)
+        (expect (gethash root projectile--dirconfig-cache) :to-be nil)))))
+  (it "is cleared for the project by projectile-invalidate-cache"
+    (projectile-test-with-sandbox
+     (projectile-test-with-files
+      ("project/.projectile")
+      (let ((root (file-truename (expand-file-name "project/"))))
+        (with-temp-file (expand-file-name ".projectile" root)
+          (insert "-foo\n"))
+        (spy-on 'projectile-project-root :and-return-value root)
+        ;; Avoid touching the on-disk cache file or recentf during the test.
+        (spy-on 'projectile-persistent-cache-p :and-return-value nil)
+        (spy-on 'recentf-cleanup)
+        (projectile-parse-dirconfig-file)
+        (expect (gethash root projectile--dirconfig-cache) :not :to-be nil)
+        (projectile-invalidate-cache nil)
+        (expect (gethash root projectile--dirconfig-cache) :to-be nil))))))
+
+(describe "alien-mode dirconfig warning"
+  (before-each
+    (clrhash projectile--alien-dirconfig-warned-projects))
+  (it "warns once when alien indexing skips a populated .projectile"
+    (projectile-test-with-sandbox
+     (projectile-test-with-files
+      ("project/.projectile")
+      (let ((root (file-truename (expand-file-name "project/"))))
+        (with-temp-file (expand-file-name ".projectile" root)
+          (insert "-foo\n"))
+        (spy-on 'projectile-project-root :and-return-value root)
+        (spy-on 'projectile-dir-files-alien :and-return-value '("a"))
+        (spy-on 'display-warning)
+        (let ((projectile-indexing-method 'alien)
+              (projectile-enable-caching nil)
+              (projectile-warn-when-dirconfig-is-ignored t))
+          (projectile-project-files root)
+          (projectile-project-files root))
+        (expect 'display-warning :to-have-been-called-times 1)))))
+  (it "does not warn for an empty .projectile"
+    (projectile-test-with-sandbox
+     (projectile-test-with-files
+      ("project/.projectile")
+      (let ((root (file-truename (expand-file-name "project/"))))
+        (spy-on 'projectile-project-root :and-return-value root)
+        (spy-on 'projectile-dir-files-alien :and-return-value '("a"))
+        (spy-on 'display-warning)
+        (let ((projectile-indexing-method 'alien)
+              (projectile-enable-caching nil)
+              (projectile-warn-when-dirconfig-is-ignored t))
+          (projectile-project-files root))
+        (expect 'display-warning :not :to-have-been-called)))))
+  (it "does not warn when the warning is disabled"
+    (projectile-test-with-sandbox
+     (projectile-test-with-files
+      ("project/.projectile")
+      (let ((root (file-truename (expand-file-name "project/"))))
+        (with-temp-file (expand-file-name ".projectile" root)
+          (insert "-foo\n"))
+        (spy-on 'projectile-project-root :and-return-value root)
+        (spy-on 'projectile-dir-files-alien :and-return-value '("a"))
+        (spy-on 'display-warning)
+        (let ((projectile-indexing-method 'alien)
+              (projectile-enable-caching nil)
+              (projectile-warn-when-dirconfig-is-ignored nil))
+          (projectile-project-files root))
+        (expect 'display-warning :not :to-have-been-called)))))
+  (it "does not warn under non-alien indexing"
+    (projectile-test-with-sandbox
+     (projectile-test-with-files
+      ("project/.projectile")
+      (let ((root (file-truename (expand-file-name "project/"))))
+        (with-temp-file (expand-file-name ".projectile" root)
+          (insert "-foo\n"))
+        (spy-on 'projectile-project-root :and-return-value root)
+        (spy-on 'projectile-get-project-directories :and-return-value '())
+        (spy-on 'display-warning)
+        (let ((projectile-indexing-method 'native)
+              (projectile-enable-caching nil)
+              (projectile-warn-when-dirconfig-is-ignored t))
+          (projectile-project-files root))
+        (expect 'display-warning :not :to-have-been-called))))))
 
 (describe "projectile-get-project-directories"
   (it "gets the list of project directories"