Codebase fixes round 2: TRAMP safety, safe-local-variable, hash table correctness#1987
Merged
Codebase fixes round 2: TRAMP safety, safe-local-variable, hash table correctness#1987
Conversation
When called with :precedence, the function recreated the type cache hash table without :test 'equal, defaulting to eql. Since project roots are strings, no cache key would ever match, causing perpetual re-detection of project types until Emacs was restarted.
The buffer-limiting function was called before the file-remote-p guard in projectile-find-file-hook-function, triggering expensive buffer enumeration and file-truename calls on every remote file open. Move it inside the existing TRAMP guard.
file-truename resolves symlinks via synchronous I/O, which can hang Emacs when the project is on a remote host via TRAMP. expand-file-name is sufficient here (it normalizes path components) and works safely with remote paths.
Variables documented as "should be set via .dir-locals.el" were missing safe-local-variable properties, causing Emacs to prompt for confirmation every time they were encountered. Add stringp for command and directory variables, booleanp for the caching toggle. Intentionally omit projectile-project-related-files-fn since it accepts functions, which cannot be safely evaluated from dir-locals.
The deleted file check used member inside seq-remove, which is O(n*m) where n is total files and m is deleted files. Convert the deleted list to a hash set for O(1) lookups, making the overall operation O(n).
nconc destructively modifies the list returned by projectile-get-immediate-sub-projects, which could affect any caller holding a reference to that list. Use append for a non-destructive alternative.
projectile-project-type already resolves the project root. Pass it through to projectile-detect-project-type so it doesn't resolve it a second time just for the cache key.
- Test that projectile-update-project-type resets cache with 'equal test - Test safe-local-variable predicates for project settings variables - Test that projectile-project-type passes project-root through to detect-project-type
bbatsov
force-pushed
the
feature/codebase-fixes-round2
branch
from
6dc3483 to
03a987b
Compare
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.
Second round of codebase fixes following the deep audit. Focuses on TRAMP robustness, correctness, and usability improvements.
Fixes:
projectile-update-project-typereset the type cache with wrong hash table:test(eqlinstead ofequal)projectile-find-file-hook-functionran buffer limiting on TRAMP buffers, risking hangsprojectile-compilation-dirusedfile-truename(resolves symlinks, slow on TRAMP) instead ofexpand-file-nameprojectile-get-all-sub-projectsused destructivenconcon returned datasafe-local-variablepredicates, causing prompts in.dir-locals.elPerformance:
projectile-dir-files-alien(O(n+m) vs O(n*m))projectile-detect-project-typeIncludes 8 new test specs (218 total, all passing).