Re-index project rules on startup.#10377
Conversation
szgupta
force-pushed
the
suraj/fix-project-rule-loading-based-on-persisted-rules
branch
2 times, most recently
from
2ab78ec to
4729a5d
Compare
szgupta
force-pushed
the
suraj/fix-project-rule-loading-based-on-persisted-rules
branch
from
4729a5d to
991af4a
Compare
szgupta
changed the title
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This change re-scans project rules instead of treating persisted rule paths as proof that a repository is already indexed, and it persists added or modified rule paths from repository watcher updates.
Concerns
- Re-indexing now also registers an additional repository subscriber each time
index_and_store_rulesruns for a root that is already loaded, which can duplicate update processing and leak subscribers.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| root_path: PathBuf, | ||
| ctx: &mut ModelContext<Self>, | ||
| ) -> Result<()> { | ||
| if self.path_to_rules.contains_key(&root_path) { |
There was a problem hiding this comment.
register_watcher_for_path again; startup loading already registers watchers for persisted roots, and repeated indexing will leave duplicate repository subscribers processing each update. Split re-scan from watcher registration or dedupe registrations per root.
szgupta
deleted the
suraj/fix-project-rule-loading-based-on-persisted-rules
branch
Caching rule-paths for indexed repos is problematic because the entries could go stale and prevent re-indexing of the repo. For example, suppose we index a repo that has a root `WARP.md` file. Later on, the user adds a `dir/WARP.md` file. Currently, we would not detect the new sub-dir WARP.md file because we wouldn't re-index the repo (because we already have a rule-file for the repo). If we think this is the correct solution, we should probably get rid of the `project_rules` sqlite table.
Caching rule-paths for indexed repos is problematic because the entries could go stale and prevent re-indexing of the repo. For example, suppose we index a repo that has a root `WARP.md` file. Later on, the user adds a `dir/WARP.md` file. Currently, we would not detect the new sub-dir WARP.md file because we wouldn't re-index the repo (because we already have a rule-file for the repo). If we think this is the correct solution, we should probably get rid of the `project_rules` sqlite table. (cherry picked from commit 3019671)
Caching rule-paths for indexed repos is problematic because the entries could go stale and prevent re-indexing of the repo. For example, suppose we index a repo that has a root `WARP.md` file. Later on, the user adds a `dir/WARP.md` file. Currently, we would not detect the new sub-dir WARP.md file because we wouldn't re-index the repo (because we already have a rule-file for the repo). If we think this is the correct solution, we should probably get rid of the `project_rules` sqlite table.
2 participants
Caching rule-paths for indexed repos is problematic because the entries could go stale and prevent re-indexing of the repo. For example, suppose we index a repo that has a root
WARP.mdfile. Later on, the user adds adir/WARP.mdfile. Currently, we would not detect the new sub-dir WARP.md file because we wouldn't re-index the repo (because we already have a rule-file for the repo).If we think this is the correct solution, we should probably get rid of the
project_rulessqlite table.