Repository navigation
Conversation
|
Warning This organization's free trial has ended, so Indent couldn't start this review. Manage billing to resume reviews on this repository. |
d3cddfa to
9d19cd5
Compare
660cc7d to
0e5ad19
Compare
0e5ad19 to
dab87b4
Compare
![ultimator-ai[bot]](https://avatars.githubusercontent.com/u/340985799?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ultimator's review of dab87b4
Reviewed only the top commit, which this push rebased without changing its grep code. It skips .git in grep and find (and marks it as ignored under --no-ignore), makes grep -m N count files, and lets a file's matches carry on to the next page. The .git pruning and the CLI's file counting and dedupe look right. One problem blocks it: with no per-file cap, every page builds a record for every remaining match in a file, each at the cost of a scan from the file's start. So a large file with many matches takes quadratic time on each page, and the CLI hits this on every grep.
1 issue: 1 bug
The open issues and Fix are in the review of c29b044.
Reviewed in 28s · Session · @ultimator-ai review reviews again · @ultimator-ai fix fixes the open issues
| // Zero caps nothing: the page budget bounds each page and the cursor | ||
| // resumes inside a file, so no match is silently dropped. | ||
| let max_per_file = if request.max_per_file == 0 { | ||
| max_results | ||
| usize::MAX | ||
| } else { | ||
| request.max_per_file as usize | ||
| }; |
There was a problem hiding this comment.
🟠 Bug · Uncapped per-file matches make each grep page quadratic
With max_per_file = usize::MAX, the match loop (find_iter(..).take(max_per_file).skip(resume_match)) collects a record for every remaining match in the file on every page. Each record runs byte_line_column and line_display, and both scan the file from byte 0, so one page costs O(matches × file size). At most 256 of those records go out per page, and the next page redoes the work from the cursor. The CLI now always sends max_per_file: 0, so yas fs grep e on a 4 MiB log with ~10^5 hits does ~10^11 byte steps per page, over ~400 pages. Before, the default cap of max_results held the work to 256 matches per file per page.
Ultimator · issue #1 · reply @ultimator-ai fix 1 to have it fixed
|
Skipped this pull request: the review of |
A status snapshot can reflect an index written after its pass reconciled the watch set; the set catches up on the pass that index event triggers. Tests that asserted the set right after the snapshot raced that pass. The debug hook now signals each published set, and the tests wait on it.
Windows PowerShell 5.1 can take over 20 s to print its first line when the runner starts it cold under the parallel suite, which failed read_until's 20 s timeout. read_until now waits for the next event; an early exit still fails the test. The residue grace is measured from the first output, so a slow start no longer counts against it.
run_git_watch opened every subscription with an empty snapshot and only delivered repository state as a later Reset. One-shot `yas git status` printed that empty snapshot, so it always said clean. The first native snapshot now is the initial snapshot.
A PATCH_TEXT answer needs a content object and neither INDEX nor WORKTREE names one, so `yas git diff -p` failed with Unsupported. Use the null object ID for patches between two mutable endpoints.
`yas git show REV` sent a BLOB query for the commit and failed with Invalid. It now prints the commit in `git cat-file commit` layout (from the LOG record), and lists a directory for REV:DIR and REV:. --repo and `git discover PATH` were sent verbatim, so relative paths (including the default `.`) resolved against the server's cwd. They now resolve against the client's cwd, like fs and lsp --root.
The server family pairs a native LOCATION with the MARKUP that follows it into one HoverRecord, but translate::hover pushed the markup first, so every hover failed with "LSP markup has no target location" (surfaced as Internal: no detail). Push the location first, as one group, and fall back to the empty range at the queried position when the language server reports no range.
run_lsp_watch sent a buffer-only initial snapshot and delivered the engine's retained diagnostics only as a later Reset, so one-shot `yas lsp diagnostics` always printed nothing and exited 0. The initial snapshot of a diagnostics subscription now waits for the engine's first diagnostics replay. --wait now waits for indexing, then for the next published diagnostics, ending after 5 seconds without any server update so a clean workspace still returns. Help: lsp --root is relative to the client's cwd, not the server's; learn.md names the actual `Busy: Warming` error.
enumerate_paths walked with hidden(false), which keeps dotfiles as the design wants but also descended into .git, so grep and find searched repository metadata. .git is now pruned like an ignored directory, and with ignored paths included it is walked and marked ignored (docs/design/fs-grep.md). GREP's default per-file cap was the page record limit and counted from the first match, so a file with more matches than one page could hold lost the rest even though the cursor resumed inside it. Zero now caps nothing. `yas fs grep -m N` sent N as the page record limit and kept paging, so it never limited anything, -m 1 failed with Invalid, and small pages hit the dropped-match bug above. It now uses the default page size and stops after N matching files; a file split across pages counts and prints once, and multi-page searches no longer warn that results were truncated.
dab87b4 to
c29b044
Compare
There was a problem hiding this comment.
Ultimator's review of c29b044
This push rebased the stack. The top commit, the only one under review, is unchanged: its grep, find and CLI code is the same as at dab87b4. #1 is still open (with no per-file cap, each grep page takes quadratic time), and I found nothing new.
No new issues.
Open issues: check the ones to fix, then Fix.
- 🟠
#1Uncapped per-file matches make each grep page quadratic ·crates/server/src/yas_fs.rs:2141-2147 - 🔧 Fix the checked issues (all of them if none is checked)
Reviewed in 18s · Session · @ultimator-ai review reviews again · @ultimator-ai fix fixes the open issues
This branch has not been deployed
1 participant
Stacked on #46. Review only the top commit, "fs: prune .git from grep and find; make grep -m count files".
grepandfindskip.gitand still search dotfiles.--no-ignoreincludes.gitand marks it as ignored.grep -m Nstops after N matching files. Before this, it set the page size and-m 1failed with Invalid.Fork PR with the isolated diff: indent-com#29.
Tag
@indentto continue the conversation here.