diff --git a/macher.el b/macher.el index f3de71f..6d09d0b 100644 --- a/macher.el +++ b/macher.el @@ -1345,14 +1345,15 @@ To enable, add to `macher-workspace-functions': ;; Built-in workspace type functions (defun macher--project-root (project-id) - "Get the project root for PROJECT-ID, validating it's a real project root." - (require 'project) - ;; Verify that project-id is actually a valid project root directory. - (unless (and (stringp project-id) - (file-name-absolute-p project-id) - (file-directory-p project-id) - (project-current nil project-id)) - (error "Project ID '%s' is not a valid project root directory" project-id)) + "Get the project root for PROJECT-ID. + +Validates only the shape of the ID (absolute path string). A full +`project-current' check is deliberately avoided here: it triggers a +cascade of remote I/O (VC discovery, `.gitmodules' probing, etc.) that +becomes very expensive over TRAMP, and `project-files' will re-validate +the project when it's actually needed." + (unless (and (stringp project-id) (file-name-absolute-p project-id)) + (error "Project ID '%s' is not a valid project root" project-id)) project-id) (defun macher--project-name (project-id) @@ -1429,11 +1430,11 @@ absolute path to a real directory." (error "Root function for workspace type %s failed to return a root" workspace-type)) (error "No root function configured for workspace type %s" workspace-type)))) - ;; Verify that the root is a real directory. + ;; Only validate the shape of the root. A `file-directory-p' check would be a remote + ;; round-trip over TRAMP on every call; any real file operation downstream will fail with a + ;; reasonable error anyway if the root doesn't exist. (unless (and root (file-name-absolute-p root)) (error "Workspace root '%s' is not an absolute path" root)) - (unless (and root (file-directory-p root)) - (error "Workspace root '%s' is not a valid directory" root)) root)) (defun macher--workspace-name (workspace) @@ -1943,7 +1944,7 @@ Ensures paths are consistently handled throughout the codebase." (expand-file-name path) (error "PATH must be an absolute file path"))) -(defun macher--resolve-workspace-path (workspace rel-path) +(defun macher--resolve-workspace-path (workspace rel-path &optional workspace-files) "Get the full path for REL-PATH within the WORKSPACE. The path will be resolved relative to the workspace root. '.' and '..' @@ -1974,7 +1975,12 @@ them in. Note also that paths outside the workspace root are allowed _if_ they appear in the workspace's files list. This won't be the case for the built-in workspace types, but might be relevant for custom workspace -types." +types. + +If WORKSPACE-FILES is provided, it is used as the workspace files list +instead of calling `macher--workspace-files'. This lets callers that +already have the list avoid a redundant computation, which can be +expensive for remote workspaces." (let* ( ;; We don't really want to deal with the `file-truename', as this would resolve symlinks ;; and might mess up the path structure when dealing with relative paths like @@ -1994,22 +2000,32 @@ types." (path-components (file-name-split relative-path)) (current-path workspace-root)) ;; Check each component except the last one for files or symlinks (only below workspace - ;; root). + ;; root). For performance (especially over remote connections), use a single + ;; file-attributes call per component instead of separate + ;; file-exists-p/file-symlink-p/file-directory-p calls. (when (> (length path-components) 1) (dolist (component (butlast path-components)) (unless ;; Skip empty components. (string-empty-p component) (setq current-path (expand-file-name component current-path)) - (when (file-exists-p current-path) - (cond - ((file-symlink-p current-path) - (error "Path '%s' contains a symbolic link in a non-final component" rel-path)) - ((not (file-directory-p current-path)) - (error "Path '%s' contains a file in a non-final component" rel-path))))))))) - - ;; Validate access permissions. - (let* ((raw-workspace-files (macher--workspace-files workspace)) + (let ((attrs (file-attributes current-path))) + ;; attrs is nil when the path doesn't exist. + (when attrs + (let ((type (file-attribute-type attrs))) + (cond + ;; type is a string when the path is a symlink (the string is the target). + ((stringp type) + (error + "Path '%s' contains a symbolic link in a non-final component" rel-path)) + ;; type is t for directories, nil for regular files. + ((not (eq t type)) + (error + "Path '%s' contains a file in a non-final component" rel-path))))))))))) + + ;; Validate access permissions. If WORKSPACE-FILES was passed in, use it instead of calling + ;; `macher--workspace-files' (which can trigger `project-current' over TRAMP). + (let* ((raw-workspace-files (or workspace-files (macher--workspace-files workspace))) ;; Process workspace files by expanding them relative to workspace root (workspace-files (when raw-workspace-files @@ -2026,14 +2042,18 @@ types." (string= (directory-file-name workspace-root) full-path) ;; Check whether the path begins with the workspace root + the path separator. (string-prefix-p (file-name-as-directory workspace-root) full-path)))) - (file-exists (file-exists-p full-path))) + ;; Use file-attributes once instead of separate file-exists-p and + ;; file-directory-p calls, since each is a TRAMP round-trip. + (path-attrs (file-attributes full-path)) + (file-exists path-attrs) + (is-directory (eq t (file-attribute-type path-attrs)))) (when (and is-outside-workspace (not (member full-path workspace-files))) (error "Path '%s' resolves outside the workspace" rel-path)) (when (and file-exists ;; Allow directories, but only within the workspace. - (or is-outside-workspace (not (file-directory-p full-path))) + (or is-outside-workspace (not is-directory)) (not (member full-path workspace-files))) (error "File '%s' is not in the workspace files list" rel-path))) @@ -2152,7 +2172,7 @@ Returns the processed content as a string." (t (string-join selected-lines "\n")))))) -(defun macher--with-workspace-file (context path callback &optional set-dirty-p) +(defun macher--with-workspace-file (context path callback &optional set-dirty-p workspace-files) "Helper function to execute CALLBACK with workspace file content. CONTEXT is a `macher-context' struct containing workspace information. @@ -2163,20 +2183,15 @@ CALLBACK is called with arguments (full-path new-content) where: - full-path is the absolute path to the file - new-content is the current content string of the file -If SET-DIRTY-P is non-nil, sets the dirty-p flag on the context." +If SET-DIRTY-P is non-nil, sets the dirty-p flag on the context. + +WORKSPACE-FILES, if provided, is passed through to +`macher--resolve-workspace-path' instead of having it compute the +list itself. Useful for callers that already have the list, to avoid +a redundant computation over a remote connection." (let* ((workspace (macher-context-workspace context)) - (resolve-workspace-path (apply-partially #'macher--resolve-workspace-path workspace)) - (get-or-create-file-contents - (lambda (file-path) - "Get or create contents for FILE-PATH in the current context. - -Returns a cons cell (orig-content . new-content) of strings for the -file. Also updates the context's :contents alist." - (let ((full-path (funcall resolve-workspace-path file-path))) - (macher-context--contents-for-file full-path context)))) - (full-path (funcall resolve-workspace-path path)) - ;; Get implementation contents for this file. - (contents (funcall get-or-create-file-contents path)) + (full-path (macher--resolve-workspace-path workspace path workspace-files)) + (contents (macher-context--contents-for-file full-path context)) (new-content (cdr contents))) ;; Check if the file exists for editing. (if (not new-content) @@ -2209,40 +2224,40 @@ offset/limit/show-line-numbers processing. For symlinks, returns the target path instead of following the link. Signals an error if the file is not found in the workspace." (let* ((workspace (macher-context-workspace context)) - (resolve-workspace-path (apply-partially #'macher--resolve-workspace-path workspace)) - (full-path (funcall resolve-workspace-path path))) + (full-path (macher--resolve-workspace-path workspace path))) ;; Check if this is a symlink first (only for existing files). - (if (file-symlink-p full-path) - ;; For symlinks, return the target path instead of following the link. - (let ((target (file-symlink-p full-path))) - (format "Symlink target: %s" target)) - - ;; Normal/non-symlink handling. - (macher--with-workspace-file - context - path - (lambda (_full-path new-content) - ;; Some LLMs (for example qwen3-coder at time of writing) seem to have trouble invoking - ;; tools with integer inputs - they'll always pass e.g. '1.0' instead of '1'. Therefore we - ;; need to support float inputs, which in general we handle by rounding to the nearest - ;; integer. - (let* ((parsed-offset - (when offset - (round offset))) - (parsed-limit - (when limit - (round limit))) - (processed-content - (macher--read-string new-content parsed-offset parsed-limit show-line-numbers))) - ;; Check if the processed content exceeds the maximum read length. - (when (> (length processed-content) macher--max-read-length) - (error - "File content too large: %d bytes exceeds maximum read length of %d bytes" - (length processed-content) - macher--max-read-length)) - processed-content)) - nil)))) + (let ((symlink-target (file-symlink-p full-path))) + (if symlink-target + ;; For symlinks, return the target path instead of following the link. + (format "Symlink target: %s" symlink-target) + + ;; Fetch file contents directly using the already-resolved path, + ;; rather than going through macher--with-workspace-file which + ;; would resolve the path a second time. + (let* ((contents (macher-context--contents-for-file full-path context)) + (new-content (cdr contents))) + (if (not new-content) + (error "File '%s' not found in workspace" path) + ;; Some LLMs (for example qwen3-coder at time of writing) seem to have trouble + ;; invoking tools with integer inputs - they'll always pass e.g. '1.0' instead of + ;; '1'. Therefore we need to support float inputs, which in general we handle by + ;; rounding to the nearest integer. + (let* ((parsed-offset + (when offset + (round offset))) + (parsed-limit + (when limit + (round limit))) + (processed-content + (macher--read-string new-content parsed-offset parsed-limit show-line-numbers))) + ;; Check if the processed content exceeds the maximum read length. + (when (> (length processed-content) macher--max-read-length) + (error + "File content too large: %d bytes exceeds maximum read length of %d bytes" + (length processed-content) + macher--max-read-length)) + processed-content))))))) (defun macher--tool-list-directory (context path &optional recursive sizes) "List directory contents at PATH within the workspace. @@ -2261,9 +2276,14 @@ LLMs. Signals an error if the directory is not found in the workspace." (let* ((workspace (macher-context-workspace context)) - (resolve-workspace-path (apply-partially #'macher--resolve-workspace-path workspace)) - (full-path (funcall resolve-workspace-path path)) (workspace-root (macher--workspace-root workspace)) + ;; Compute workspace-files once and reuse it for path resolution and entry collection + ;; below. Each call transitively triggers `project-current', which is expensive over + ;; TRAMP (walks the directory tree probing for a VC root). + (workspace-files (macher--workspace-files workspace)) + (resolve-workspace-path + (lambda (rel-path) (macher--resolve-workspace-path workspace rel-path workspace-files))) + (full-path (funcall resolve-workspace-path path)) (results '()) (context-contents (macher-context-contents context))) @@ -2364,9 +2384,10 @@ Signals an error if the directory is not found in the workspace." (collect-entries (current-path current-rel-path depth) - ;; Build entries list by iterating through workspace files. - (let* ((workspace-files (macher--workspace-files workspace)) - (current-path-as-dir (file-name-as-directory current-path)) + ;; Build entries list by iterating through workspace files. Uses the + ;; already-computed `workspace-files' from the enclosing let to avoid another remote + ;; `project-current' walk. + (let* ((current-path-as-dir (file-name-as-directory current-path)) ;; Hash table to collect unique entries (both files and directories). (entries-hash (make-hash-table :test 'equal)) @@ -2418,9 +2439,13 @@ Signals an error if the directory is not found in the workspace." entry (concat current-rel-path "/" entry))) (entry-deleted-p (file-deleted-in-context-p entry-full-path)) - (entry-exists-on-disk-p (file-exists-p entry-full-path)) - (entry-is-symlink-p - (and (not entry-deleted-p) (file-symlink-p entry-full-path))) + ;; Use a single file-attributes call per entry instead of + ;; separate file-exists-p / file-symlink-p / file-directory-p, + ;; since each is a TRAMP round-trip. + (entry-attrs (file-attributes entry-full-path)) + (entry-exists-on-disk-p entry-attrs) + (entry-disk-type (and entry-attrs (file-attribute-type entry-attrs))) + (entry-is-symlink-p (and (not entry-deleted-p) (stringp entry-disk-type))) (entry-exists-in-context-p (when-let ((entry (assoc @@ -2438,8 +2463,7 @@ Signals an error if the directory is not found in the workspace." (not entry-deleted-p) (not entry-is-symlink-p) ;; A path is a directory if it exists on disk as a directory OR if it's ;; in our context-new-directories list. - (or (file-directory-p entry-full-path) - (member entry context-new-directories)))) + (or (eq t entry-disk-type) (member entry context-new-directories)))) (size-info "") (indent (make-string (* depth 2) ?\s))) @@ -2451,9 +2475,10 @@ Signals an error if the directory is not found in the workspace." (setq size-info (format " (%s)" (macher--format-size file-size))))) ;; Add symlink target info if it's a symlink. + ;; + ;; entry-disk-type is the symlink target string. (when entry-is-symlink-p - (let ((target (file-symlink-p entry-full-path))) - (setq size-info (format " -> %s" target)))) + (setq size-info (format " -> %s" entry-disk-type))) ;; Add entry to results. (push (format "%s%s: %s%s" @@ -2619,8 +2644,11 @@ Returns nil on success. Signals an error if the source file is not found or if the destination already exists. Sets the dirty-p flag on the context to indicate changes." (let* ((workspace (macher-context-workspace context)) - (resolve-workspace-path (apply-partially #'macher--resolve-workspace-path workspace)) - (dest-full-path (funcall resolve-workspace-path destination-path))) + ;; Compute workspace-files once and share it with both resolve calls + ;; below, to avoid a redundant `project-current' walk over TRAMP. + (workspace-files (macher--workspace-files workspace)) + (dest-full-path + (macher--resolve-workspace-path workspace destination-path workspace-files))) ;; Check if destination already exists. (let ((dest-contents (macher-context--contents-for-file dest-full-path context))) (when (cdr dest-contents) @@ -2636,7 +2664,7 @@ indicate changes." source-full-path nil context) ;; Return nil to indicate success. nil) - t))) + t workspace-files))) (defun macher--tool-delete-file (context rel-path) "Delete a file specified by REL-PATH within the workspace. @@ -2691,10 +2719,14 @@ the `xref-search-program' to perform the search." (case-fold-search case-insensitive) (workspace (macher-context-workspace context)) (workspace-root (macher--workspace-root workspace)) - (resolve-workspace-path (apply-partially #'macher--resolve-workspace-path workspace)) + ;; Compute workspace-files once and reuse it for path resolution below. Each call + ;; triggers `project-current' which is expensive over TRAMP (walks the directory tree + ;; looking for a VC root). + (workspace-files (macher--workspace-files workspace)) + (resolve-workspace-path + (lambda (rel-path) (macher--resolve-workspace-path workspace rel-path workspace-files))) (search-path (funcall resolve-workspace-path (or path "."))) (context-contents (macher-context-contents context)) - (workspace-files (macher--workspace-files workspace)) (path (when path (expand-file-name path workspace-root))) @@ -2713,6 +2745,9 @@ the `xref-search-program' to perform the search." (cl-remove-if-not (lambda (file-path) (let ((full-path (expand-file-name file-path workspace-root))) + ;; Note - we don't check explicitly that `file-exists-p', since this is expensive + ;; for remote files or large projects. The search backend (grep/rg) should simply + ;; be able to skip files that don't exist. (and ;; File is under the search path. (string-prefix-p search-path full-path) @@ -2723,11 +2758,7 @@ the `xref-search-program' to perform the search." (file-relative-name full-path search-path) (file-relative-name full-path workspace-root)))) (string-match-p file-regexp rel-path)) - t) - ;; File exists or has content in context. - (or (file-exists-p full-path) - (let ((entry (assoc (macher--normalize-path full-path) context-contents))) - (and entry (cdr (cdr entry)))))))) + t)))) workspace-files)) ;; Add any context-only files that match our criteria. (context-only-files @@ -2815,7 +2846,15 @@ the `xref-search-program' to perform the search." ;; Path is a single file, use the original path parameter. path ;; Otherwise, always relative to workspace root. - (file-relative-name original-file workspace-root)))) + ;; Strip any remote prefix from both paths so + ;; file-relative-name can compare them even when + ;; one has a TRAMP prefix and the other does not + ;; (e.g. xref may return local paths while + ;; workspace-root is remote). + (file-relative-name (or (file-remote-p original-file 'localname) + original-file) + (or (file-remote-p workspace-root 'localname) + workspace-root))))) ;; Group results by file for proper formatting. (let ((file-entry (assoc rel-path results))) @@ -3386,30 +3425,25 @@ otherwise returns (nil . nil)." (if existing-contents ;; Return the existing contents. (cdr existing-contents) - ;; Handle file existence check. - (if (not (file-exists-p normalized-path)) - ;; For non-existent files, store (nil . nil) in context and return it. - (let ((context-contents (macher-context-contents context)) - (content-pair (cons nil nil))) - ;; Add to context. - (push (cons normalized-path content-pair) context-contents) - (setf (macher-context-contents context) context-contents) - content-pair) - ;; For existing files, load the file content. - (let* ((file-content - (with-temp-buffer - (insert-file-contents normalized-path) - (buffer-substring-no-properties (point-min) (point-max)))) - (context-contents (macher-context-contents context)) - ;; Both original and new content start as the same. - (content-pair (cons file-content file-content))) - - ;; Add to context. - (push (cons normalized-path content-pair) context-contents) - (setf (macher-context-contents context) context-contents) - - ;; Return the content pair. - content-pair))))) + ;; Try to load the file content, treating a missing file as (nil . nil) rather than + ;; doing a separate `file-exists-p' round-trip over TRAMP before the actual read. Only + ;; catch `file-missing' — broader `file-error' signals (permission denied, TRAMP + ;; connection failure, etc.) should propagate so the caller sees the real problem. + (let* ((file-content + (condition-case nil + (with-temp-buffer + (insert-file-contents normalized-path) + (buffer-substring-no-properties (point-min) (point-max))) + (file-missing + nil))) + ;; For non-existent files, store (nil . nil); for existing files, the original and + ;; new content start as the same. + (content-pair + (if file-content + (cons file-content file-content) + (cons nil nil)))) + (push (cons normalized-path content-pair) (macher-context-contents context)) + content-pair)))) ;;; Default Prompt Functions (defun macher--focus-string-default () @@ -3878,7 +3912,7 @@ CALLBACK and FSM are as described in the `gptel-prompt-transform-functions' documentation." (when-let* ((info (gptel-fsm-info fsm)) (buffer (plist-get info :buffer)) - (_ (buffer-live-p buffer))) + ((buffer-live-p buffer))) ;; The system message needs to be set in the temporary buffer where this prompt transform is ;; being invoked, but the context string needs to be generated in the buffer where the request ;; is actually being sent. Pass the request buffer to the replace function. @@ -4475,7 +4509,7 @@ BUF defaults to the current buffer if not specified." (interactive) (with-current-buffer (or buf (current-buffer)) (when-let* ((action-buffer (macher-action-buffer)) - (_ (buffer-live-p action-buffer))) + ((buffer-live-p action-buffer))) (gptel-abort action-buffer)))) ;;;###autoload diff --git a/tests/test-functional.el b/tests/test-functional.el index ce3a275..978de27 100644 --- a/tests/test-functional.el +++ b/tests/test-functional.el @@ -375,7 +375,10 @@ CALLBACK-TEST is a function that verifies the result." (describe "inline presets" :var* - ((original-gptel--known-presets gptel--known-presets) + ( ;; `gptel-make-preset' uses `nconc' to append to `gptel--known-presets', which mutates the + ;; existing list. Capture a copy of the list head so the after-each restoration actually + ;; reverts what `macher-install' does. + (original-gptel--known-presets (copy-sequence gptel--known-presets)) (original-gptel-post-response-functions gptel-post-response-functions) (verify-delete-file diff --git a/tests/test-unit.el b/tests/test-unit.el index 26dce2d..dfc0d64 100644 --- a/tests/test-unit.el +++ b/tests/test-unit.el @@ -105,7 +105,15 @@ (expect (cdr non-existent-contents) :to-be nil) ;; New content should be nil. ;; Should be added to context's contents list. (expect (assoc (macher--normalize-path non-existent) (macher-context-contents context)) - :to-be-truthy)))) + :to-be-truthy))) + + (it "propagates non-file-missing read errors (e.g. permission denied)" + ;; `contents-for-file' should catch `file-missing' to avoid a separate existence + ;; probe, but broader `file-error' signals must propagate so real problems + ;; (permission denied, TRAMP connection failure, etc.) aren't silently hidden. + (cl-letf (((symbol-function 'insert-file-contents) + (lambda (&rest _) (signal 'file-error '("Permission denied"))))) + (expect (macher-context--contents-for-file temp-file context) :to-throw 'file-error)))) (describe "macher-context--set-new-content-for-file" :var (context temp-file original-contents) @@ -235,7 +243,22 @@ :to-equal '("hello world hello universe" . "hi world hi universe")))) (it "sets the dirty-p flag" (macher--tool-edit-file context temp-file "original" "modified" nil) - (expect (macher-context-dirty-p context) :to-be-truthy)))) + (expect (macher-context-dirty-p context) :to-be-truthy)) + (it "errors when workspace-files includes a file not on disk" + (let ((proj-dir (make-temp-file "macher-test-edit-proj" t))) + (write-region "" nil (expand-file-name ".project" proj-dir)) + (write-region "real" nil (expand-file-name "real.txt" proj-dir)) + (let ((ctx + (macher--make-context + :workspace (cons 'project (file-name-as-directory proj-dir))))) + (spy-on 'macher--workspace-files + :and-return-value + (list + (expand-file-name "real.txt" proj-dir) + (expand-file-name "ghost.txt" proj-dir))) + (unwind-protect + (expect (macher--tool-edit-file ctx "ghost.txt" "old" "new" nil) :to-throw) + (delete-directory proj-dir t))))))) (describe "macher--process-request" :var (context fsm temp-file build-patch-called) @@ -1045,7 +1068,7 @@ (append (macher--project-files (cdr workspace)) (list symlink-path)))) (let ((result (macher--tool-list-directory context "."))) - (expect result :to-match "link: file-symlink ->") + (expect result :to-match (format "link: file-symlink -> %s" target-path)) (expect result :to-match "file: file1.txt") (expect result :to-match "file: file2.el")) @@ -1064,7 +1087,7 @@ (append (macher--project-files (cdr workspace)) (list dir-symlink-path)))) (let ((result (macher--tool-list-directory context "."))) - (expect result :to-match "link: dir-symlink ->") + (expect result :to-match (format "link: dir-symlink -> %s" target-dir)) (expect result :to-match "dir: subdir")) ;; Clean up. @@ -1407,6 +1430,20 @@ (let ((result (macher--search-get-xref-matches context "modified"))) (expect (assoc "file1.txt" result) :to-be-truthy))) + (it "tolerates stale workspace-files entries pointing at missing files" + ;; `macher--search-get-xref-matches' no longer filters workspace-files through + ;; `file-exists-p' up-front (that was expensive over TRAMP); it relies on the + ;; search backend to skip missing files. Simulate a stale project cache by + ;; adding a non-existent path to the workspace-files list and ensure search + ;; doesn't crash and still returns matches for the real files. + (let ((stale-path (expand-file-name "ghost.txt" temp-dir))) + (spy-on + 'macher--workspace-files + :and-call-fake (lambda (workspace) (cons stale-path (macher--project-files (cdr workspace))))) + (let ((result (macher--search-get-xref-matches context "hello"))) + (expect (assoc "file1.txt" result) :to-be-truthy) + (expect (assoc "ghost.txt" result) :to-be nil)))) + (it "shows paths relative to workspace root even when path is specified" (let ((result (macher--search-get-xref-matches context "hello" :path "subdir"))) ;; When searching in "subdir", results should still be relative to workspace root (like grep). @@ -2785,7 +2822,18 @@ ;; 0.4 should round to 0 (no extra lines). (expect result-0.4 :to-match "hello") ;; 0.6 should round to 1 (1 extra line). - (expect result-0.6 :to-match "hello"))))) + (expect result-0.6 :to-match "hello")))) + + (it "search handles nonexistent files in workspace gracefully" + ;; When workspace-files includes a file that doesn't exist on disk, + ;; search should still work, treating the missing file as empty. + (spy-on 'macher--workspace-files + :and-return-value + (list + (expand-file-name "file1.txt" temp-dir) (expand-file-name "ghost.txt" temp-dir))) + (let ((result (macher--tool-search-helper context "hello"))) + (expect result :to-match "file1.txt") + (expect result :not :to-match "ghost.txt")))) (describe "macher--tool-read-file" :var (context temp-dir) @@ -2820,8 +2868,7 @@ (append (macher--project-files (cdr workspace)) (list symlink-path)))) (let ((result (macher--tool-read-file context "test-symlink"))) - (expect result :to-match "Symlink target:") - (expect result :to-match target-path)) + (expect result :to-equal (format "Symlink target: %s" target-path))) ;; Clean up. (delete-file symlink-path)) @@ -2837,8 +2884,7 @@ (append (macher--project-files (cdr workspace)) (list broken-symlink-path)))) (let ((result (macher--tool-read-file context "broken-symlink"))) - (expect result :to-match "Symlink target:") - (expect result :to-match "/nonexistent/target")) + (expect result :to-equal "Symlink target: /nonexistent/target")) ;; Clean up. (delete-file broken-symlink-path)) @@ -2854,8 +2900,7 @@ (append (macher--project-files (cdr workspace)) (list rel-symlink-path)))) (let ((result (macher--tool-read-file context "rel-symlink"))) - (expect result :to-match "Symlink target:") - (expect result :to-match "./test-file.txt")) + (expect result :to-equal "Symlink target: ./test-file.txt")) ;; Clean up. (delete-file rel-symlink-path))) @@ -2863,6 +2908,15 @@ (it "signals error for non-existent files" (expect (macher--tool-read-file context "nonexistent.txt") :to-throw)) + (it "errors when workspace-files includes a file not on disk" + ;; When a custom workspace type's :get-files returns stale entries, + ;; read-file should error rather than silently returning nil. + (spy-on 'macher--workspace-files + :and-return-value + (list + (expand-file-name "test-file.txt" temp-dir) (expand-file-name "ghost.txt" temp-dir))) + (expect (macher--tool-read-file context "ghost.txt") :to-throw)) + (describe "float parameter handling" (it "handles float offset values by rounding them" ;; Create test file for this test @@ -3444,33 +3498,6 @@ (test-workspace '(test-type . "/some/path"))) (expect (macher--workspace-root test-workspace) :to-throw 'error))) - (it "throws error when root function returns path to non-existent directory" - (let ((macher-workspace-types-alist - '((test-type - . - (:get-root - (lambda (id) "/nonexistent/directory") - :get-name (lambda (id) "test") - :get-files (lambda (id) nil))))) - (test-workspace '(test-type . "/some/path"))) - (expect (macher--workspace-root test-workspace) :to-throw 'error))) - - (it "throws error when root function returns path to a file instead of directory" - (let* ((temp-file (make-temp-file "macher-test-file")) - (macher-workspace-types-alist - `((test-type - . - (:get-root - (lambda (id) ,temp-file) - :get-name (lambda (id) "test") - :get-files (lambda (id) nil))))) - (test-workspace '(test-type . "/some/path"))) - (unwind-protect - (expect (macher--workspace-root test-workspace) :to-throw 'error) - ;; Clean up - (when (file-exists-p temp-file) - (delete-file temp-file))))) - (it "works correctly with valid workspace type configuration" (let* ((temp-dir (make-temp-file "macher-test-workspace" t)) (macher-workspace-types-alist @@ -6867,7 +6894,148 @@ (goto-char (point-min)) (while (search-forward macher-context-string-marker-start nil t) (setq marker-count (1+ marker-count)))) - (expect marker-count :to-equal 1)))))))))) + (expect marker-count :to-equal 1))))))))) + + (describe "remote workspace compatibility" + :var (temp-dir remote-root) + + (before-all + ;; Register a custom TRAMP method that uses a local /bin/sh as its "login program". This + ;; exercises real TRAMP code paths — path parsing, handler dispatch, caching, xref temp-file + ;; management — without needing a network connection. The same trick is used by TRAMP's own + ;; test suite. + (require 'tramp) + (require 'tramp-sh) + ;; Suppress chatty "File is missing: .gitmodules" log lines that TRAMP emits when project.el + ;; probes for submodules. + (setq tramp-verbose 0) + (unless (assoc "mock" tramp-methods) + (add-to-list + 'tramp-methods + `("mock" + (tramp-login-program ,(executable-find "sh")) + (tramp-login-args (("-i"))) + (tramp-remote-shell "/bin/sh") + (tramp-remote-shell-args ("-c")) + (tramp-connection-timeout 10))))) + + (before-each + (setq temp-dir (make-temp-file "macher-test-remote" t)) + + ;; Create test files and initialize a git repo so project-vc discovers the project through + ;; TRAMP. + (make-directory (expand-file-name "src" temp-dir)) + (write-region "hello world" nil (expand-file-name "src/main.py" temp-dir)) + (write-region "hello again" nil (expand-file-name "src/util.py" temp-dir)) + (let ((default-directory temp-dir)) + (call-process "git" nil nil nil "init") + (call-process "git" nil nil nil "add" ".") + (call-process "git" + nil + nil + nil + "-c" + "user.name=test" + "-c" + "user.email=test@test" + "commit" + "-m" + "init")) + + ;; Construct the remote root in TRAMP's normalized form (/mock:HOST:PATH/) and prime the + ;; connection so that the first real operation doesn't have to initialize it. + (setq remote-root (format "/mock:%s:%s" (system-name) (file-name-as-directory temp-dir))) + (file-directory-p remote-root) + + ;; Spy on tramp-send-command to count remote round-trips. Every actual command sent to the + ;; remote shell goes through this function, so it gives a uniform, backend-agnostic measure of + ;; "remote I/O" regardless of which primitive (file-attributes, file-exists-p, process-file, + ;; etc.) triggered it. Installed after warm-up so connection-setup commands aren't counted. + (spy-on 'tramp-send-command :and-call-through)) + + (after-each + (tramp-cleanup-all-connections) + (when (and temp-dir (file-exists-p temp-dir)) + (delete-directory temp-dir t))) + + (it "search produces correct relative paths over a remote connection" + ;; The search function must produce correct relative paths when the + ;; workspace root is a remote (TRAMP) path. + (let* ((context (macher--make-context :workspace (cons 'project remote-root))) + (result (macher--search-get-xref-matches context "hello"))) + (expect result :to-be-truthy) + (expect (assoc "src/main.py" result) :to-be-truthy) + (expect (assoc "src/util.py" result) :to-be-truthy) + (dolist (entry result) + (expect (file-name-absolute-p (car entry)) :to-be nil)))) + + (it "search makes sub-linear remote calls in N workspace files" + ;; A regression that triggers per-file I/O (e.g. a `file-exists-p' check on each workspace + ;; file) would push the total remote-call count above the file count. Create more than 100 + ;; files and assert that the total stays under 100 — that directly proves the operation is + ;; sub-linear. Note we expect a somewhat large baseline of remote operations, for + ;; e.g. project.el project detection; but we want to make sure it doesn't grow linearly with + ;; the number of files in the project. + (dotimes (i 120) + (write-region + (format "hello from file %d" i) nil (expand-file-name (format "file_%d.txt" i) temp-dir))) + (let ((default-directory temp-dir)) + (call-process "git" nil nil nil "add" ".") + (call-process "git" + nil + nil + nil + "-c" + "user.name=test" + "-c" + "user.email=test@test" + "commit" + "-m" + "add test files")) + + (let ((context (macher--make-context :workspace (cons 'project remote-root)))) + (macher--tool-search context "hello" nil nil "files")) + (expect (spy-calls-count 'tramp-send-command) :to-be-less-than 100)) + + (it "macher--workspace-root does not trigger remote I/O" + ;; `macher--workspace-root' is a pure resolver: it should return the configured root without + ;; validating it, since any downstream file operation will fail naturally if the root is bad. + ;; Over TRAMP, a validation like file-directory-p would be a remote round-trip every time the + ;; function is called (which may be many times per tool invocation). + (let ((workspace (cons 'project remote-root))) + (expect (macher--workspace-root workspace) :not :to-be nil)) + (expect (spy-calls-count 'tramp-send-command) :to-equal 0)) + + (it "read-file does not trigger redundant project discovery" + ;; `project-current' (called transitively via `macher--project-files') triggers project-try-vc + ;; which walks the directory tree probing for .git/.gitmodules. It's expensive over TRAMP and + ;; should be called at most once per tool invocation. + (spy-on 'project-current :and-call-through) + (ignore-errors + (let ((context (macher--make-context :workspace (cons 'project remote-root)))) + (macher--tool-read-file context "src/main.py"))) + (expect (spy-calls-count 'project-current) :to-equal 1)) + + (it "search does not trigger redundant project discovery" + ;; The search tool resolves the search path via `macher--resolve-workspace-path' and also reads + ;; workspace-files directly. Both paths call `macher--workspace-files' internally, but + ;; project-current (and its directory-walk probes) should only happen once per tool + ;; invocation. + (spy-on 'project-current :and-call-through) + (ignore-errors + (let ((context (macher--make-context :workspace (cons 'project remote-root)))) + (macher--tool-search context "hello" nil nil "files"))) + (expect (spy-calls-count 'project-current) :to-equal 1)) + + (it "list-directory does not trigger redundant project discovery" + ;; Like search, list-directory resolves a path and separately reads workspace-files for entry + ;; collection. Both should share the same workspace-files computation so project-current only + ;; runs once. + (spy-on 'project-current :and-call-through) + (ignore-errors + (let ((context (macher--make-context :workspace (cons 'project remote-root)))) + (macher--tool-list-directory context "."))) + (expect (spy-calls-count 'project-current) :to-equal 1)))) (provide 'test-unit) ;;; test-unit.el ends here