fix: improve performance for large/remote workspaces#60
Merged
Conversation
Add a mock remote file name handler (registered via file-name-handler-alist) that simulates TRAMP behavior without requiring sudo, SSH, or external infrastructure. Uses a custom project-find-functions entry instead of spying on macher internals. Remote workspace tests (currently FAIL, will pass after fix): - search produces correct relative paths over a remote connection - search does not make O(N) remote calls for N workspace files Nonexistent workspace file tests (currently PASS, regression guards): - macher--tool-edit-file: errors for file in workspace-files not on disk - macher--tool-read-file: errors for file in workspace-files not on disk - macher--tool-search-helper: search ignores missing files gracefully
…tests Replace the custom test-remote-project type (which used cl-defmethod to define project-root and project-files specializations) with a real git repo discovered by project-vc through the mock file handler. This tests the actual project.el code path that TRAMP-based projects use. Changes: - Remove cl-defmethod definitions for test-remote-project - Initialize a git repo in the temp directory instead - Remove project-find-functions manipulation - Let project-vc discover the project naturally through the mock handler - In the O(N) test, git-add the extra files instead of overriding project-find-functions
Three fixes for poor performance and broken behavior when the workspace is a remote TRAMP directory: 1. macher--project-files: use the local part of project-id for file-relative-name, since project-files returns paths without the TRAMP remote prefix. 2. macher--search-get-xref-matches: remove per-file file-exists-p checks that cause O(N) remote round-trips. Workspace files come from project-files and are expected to exist; non-existent files produce no grep matches and context-deleted files are filtered later. 3. macher--search-get-xref-matches: relativize xref result paths against the local part of workspace-root, since xref returns paths without the remote prefix.
…lve-workspace-path Consolidate redundant file I/O calls that each become a remote round-trip over TRAMP: - resolve-workspace-path: use a single file-attributes call per path component instead of separate file-exists-p + file-symlink-p + file-directory-p (3 round-trips → 1). - tool-read-file: inline content fetching with the already-resolved path instead of going through with-workspace-file which resolves the path a second time. Also avoid calling file-symlink-p twice. - with-workspace-file: remove redundant resolve-workspace-path call from the get-or-create-file-contents lambda. - tool-list-directory: use a single file-attributes call per directory entry instead of separate file-exists-p + file-symlink-p + file-directory-p. Reuse the attrs for symlink target info. Benchmarked with 50ms simulated latency over SSH to localhost: read-file: 6.3s → 3.1s (51% faster, 92 → 34 I/O calls) list-directory: 2.0s → 1.7s (18% faster, 43 → 35 I/O calls)
The previous approach used file-remote-p 'localname on workspace-root to compensate for xref returning local paths. This doesn't match real TRAMP behavior (where xref returns fully-prefixed remote paths) and was only needed for the mock test handler. Fix the search result relativization to strip remote prefixes from BOTH the original-file and workspace-root before calling file-relative-name. This handles all cases correctly: - Real TRAMP: both have prefix → both stripped → correct relative path - Mock: only workspace-root has prefix → workspace-root stripped → correct - Local: neither has prefix → unchanged → correct Also remove the incorrect localname workaround from project-files, fix the mock handler's file-relative-name to not special-case mismatched prefixes, and update test comments.
- read-file: use :to-equal with exact 'Symlink target: <path>' string instead of two separate partial :to-match checks - list-directory: check full 'link: <name> -> <target>' including the symlink target path, not just the prefix
Previously macher--workspace-root called file-directory-p and macher--project-root called project-current + file-directory-p on every invocation. Each of these is a remote round-trip over TRAMP, and project-current also triggers project-try-vc which walks the directory tree probing for .git and .gitmodules. These validations are dropped: any real file operation downstream will fail with a reasonable error if the root doesn't exist, and project-files re-validates the project when it's actually needed. Also adds an optional ROOT-PATH argument to macher--workspace-files so that callers with an already-resolved root can avoid a redundant macher--workspace-root call, and passes it through from macher--resolve-workspace-path.
With file-directory-p removed from macher--workspace-root and project-current removed from macher--project-root, workspace-root is now free of remote I/O. Calling it multiple times in one tool invocation is no longer a concern, so the optional ROOT-PATH argument added to avoid redundant calls is not worth the signature change. Also updates the workspace-root test to verify the real property of interest (no remote I/O during resolution) rather than an implementation-detail call count.
Two independent optimizations: - macher--tool-search: compute workspace-files once and pass it to macher--resolve-workspace-path instead of letting both sites call macher--workspace-files separately. Each call transitively triggers project-current (via macher--project-files), which walks the directory tree probing for .git and .gitmodules — expensive over TRAMP. - macher-context--contents-for-file: drop the file-exists-p check before insert-file-contents. The two operations are separate remote round-trips; instead, try to read and treat a read failure as a non-existent file. Adds regression tests for both properties.
Same pattern as the search fix: macher--tool-list-directory called macher--workspace-files twice (once transitively via macher--resolve-workspace-path, once directly in collect-entries). Each call transitively triggers project-current, which walks the directory tree probing for a VC root — expensive over TRAMP. Now the workspace-files list is computed once at the top of list-directory and reused for both path resolution and entry collection. Adds a regression test.
Catching the broader file-error signal in macher-context--contents-for-file would silently convert permission-denied / TRAMP connection failures into "file doesn't exist", hiding real errors from callers. Narrow the catch to file-missing (still avoids the extra existence-probe round-trip) and let other file-error subtypes propagate. Also add unit tests for: - the file-error propagation behavior, and - search tolerating stale workspace-files entries that point at files no longer present on disk.
Drop the broader file-error arm so real errors (permission denied, TRAMP connection failure, etc.) propagate to callers instead of being silently converted to "file doesn't exist".
`gptel-make-preset` mutates `gptel--known-presets` via `nconc`. The inline-presets describe in test-functional.el captured the list head into `original-gptel--known-presets` by reference, so when its `before-each` ran `macher-install` (which calls `gptel-make-preset`), the captured "original" was mutated alongside the live list. The `after-each` `(setq gptel--known-presets original-gptel--known-presets)` was then a no-op, leaving macher presets registered globally for subsequent test files. The leak surfaces in the integration tests' default-before-action suite, where `macher--before-action-insert-prompt` finds `macher-ro` in `gptel--known-presets` and inserts `@macher-ro` into the action buffer prompt, breaking the expected buffer content. Capture a copy of the list head so the restoration actually undoes `macher-install`. Change-Id: I56f00283caa40bcc7f843ae6c962c3daf19aee90
* macher.el (macher--transform-system-replace-placeholder) (macher-abort): Use the no-binding form of `when-let*' for buffer-live-p gates instead of binding to a placeholder, which recent byte compilers flag as 'variable not left unused'. * macher.el (macher--resolve-workspace-path) (macher--tool-list-directory): Drop double-negation when checking for non-nil `file-attributes' results. Change-Id: Ifdf8c88bfea20b56df9f9c731275e301c56ba39e
kmontag
added a commit
to yantar92/macher
that referenced
this pull request
May 6, 2026
Brings in the directory workspace type (kmontag#48), no-workspace user-error fix (kmontag#48), and the large/remote workspace perf fixes (kmontag#60). Resolved conflicts: - macher.el (macher--tool-read-file): kept main's perf-improved body that skips the redundant path resolution in macher--with-workspace-file, but used the new `macher--check-output-length' helper from this branch in place of main's inline length check. - tests/test-unit.el (macher--tool-search-helper): kept both `it' blocks added at the same spot (one from each side). Change-Id: I713522bc98a021f55cf186806c2f34de3d5592a3
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cleans up unnecessary filesystem I/O and round trips to TRAMP remotes when using macher tools.
The biggest improvement is in the search and list-file tools, which now skip some defensive file-exists-p checks to avoid unnecessary O(n) filesystem (or network) access. These checks were only really relevant as a sanity check that macher--workspace-files is returning real files.
Redundant checks of file attributes, repeated calls to macher-workspace (which may run remote project-detection, a somewhat expensive operation) have also been cleaned up, and the performance of the other tools has been improved, for example the read-file tool has about 50% fewer remote interactions.
An issue with relative-path computation when receiving xref results for remote files (which come in with "local" filenames instead of TRAMP-style names) has also been fixed, and general behavior over TRAMP is now better tested.
Re-PR of #59, which was accidentally merged without squashing.