Repository navigation
fix(read): stop the head window reading past the lines it was asked for - #4122
Conversation
`rtk read <file> --head-lines N` read the whole file before slicing, so on a source with no end there was nothing to slice: `head -n 5 /dev/urandom` returns five lines instantly, and the rewritten form never returned at all. That path is reachable now that `head -n N` and `head --lines N` rewrite to it. Read in chunks and stop at the Nth newline, as head does, retrying an interrupted read the way the `fs::read` this replaces did. Byte-for-byte the same answer as before on a regular file -- across chunk boundaries, on CRLF endings and on an unterminated last line; the savings baseline comes from the size on disk, and a source that has no meaningful size claims none. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📊 Automated PR Analysis
SummaryFixes a hang introduced by PR #3941, where rewriting Review Checklist
Analyzed automatically by wshm · This is an automated analysis, not a human review. |
pszymkowiak
left a comment
There was a problem hiding this comment.
Reviewed by building the branch and comparing rtk read <f> --head-lines N against native head -n N with cmp.
Verified
- develop:
rtk read /dev/urandom --head-lines 5never returns (killed after 3 s, 0 bytes). This PR: returns immediately with exactly 5 newline-terminated lines. A never-closing FIFO returns 3 lines with--head-lines 3. - Hook path:
rtk rewrite 'head -n 5 /dev/urandom'→rtk read /dev/urandom --head-lines 5; pipelines (cat f | head -n 5) are not rewritten, so the still-unbounded stdin path is not reachable from aheadrewrite. - Faithfulness: 44/44
cmp-identical tohead -n Nfor N ∈ {0, 1, 3, 99999} over: plain text, no trailing newline, CRLF, empty file, NUL bytes, newline exactly at byte 8191 and at 8192 (chunk boundary), a 20 000-byte line, a 4-byte UTF-8 char straddling the boundary,\rat 8191 /\nat 8192, 4000-line file. Same 44/44 on develop, so no behaviour change on regular files. - Only the head-only shape (
level == None && !line_numbers && head_lines.is_some(),read.rs:29-31) takes the chunked path; tail, line numbers and filter levels still read whole, as described. Error path and exit code unchanged (missing file → exit 1).
Follow-up, not blocking
read.rs:37-46: the description says regular_file_len returning None means nothing absurd is claimed, but the tracking row still gets output = String::from_utf8_lossy(&window), which inflates every invalid byte to 3 bytes. head -n 5 /dev/urandom records input 383 / output 690 / -80 %; rtk gain aggregates clamp to 0 so nothing user-visible, but the row is wrong. Passing window.len() for the output size fixes it.
Design note, no action needed: read_head_lines and head_window are two implementations of the same contract, guarded by test_read_head_lines_matches_head_window. Unifying them would also let the stdin path stop early.
Approving. CI green on all three OSes.
Fixes one regression from the release review on #3979, introduced by #3941.
Part of a set of six, one per originating PR: #3681, #3552, #3265, #3772, #3857, #3941 (this).
The regression
#3941 made
head -n Nandhead --lines Nrewrite tortk read <file> --head-lines N, and that path calledfs::read(file)— the whole file — before slicing the window out of it. On a source with no end there is nothing to slice:The hang itself predates #3941. What is new is that the hook now routes ordinary
headinvocations into it.The fix
Read in 8 KiB chunks and stop at the Nth newline, as
headdoes. An interrupted read is retried, which thefs::readthis replaces did for itself.The whole-file read still runs for every other shape.
--tail-linesgenuinely needs the end, and a filter level or--line-numbersneeds the file whole — none of those is reachable from aheadrewrite, which is what made this path the one that had to stop early.tail,cat,wcandgrepall read to the end natively too, so there is no asymmetry to fix there;headwas uniquely asymmetric because it stops.Savings accounting
The baseline can no longer be "the bytes we read", because not reading them is the point. It comes from the size on disk via
regular_file_len, which returnsNonefor anything whose size says nothing about what it will produce — a device node reports 0, which would book the window as pure cost — and falls back to the window's own length, claiming nothing.Known imprecision, not introduced here and not fixed here. The emitted side is still counted as
String::from_utf8_lossy(&window), as it was before this change, so a window of non-UTF-8 bytes counts three bytes for every invalid one.head -n 5 /dev/urandomrecordsinput 383 / output 690, a negative row. What is new is only that such a source now completes and therefore records a row at all — before, it hung and recorded nothing.rtk gainclamps aggregates at 0, so nothing user-visible changes; the stored row is still wrong. Passingwindow.len()as the emitted size fixes it, and is left for the follow-up below so this PR stays one behaviour change.Verification
read_head_linesmust be byte-for-byte equivalent to thehead_windowit replaces — that equivalence is the entire safety argument, so the test asserts it across inputs that span more than one chunk: a line twice the chunk size, newlines landing exactly on the boundary and on either side of it, 4000 short lines over several chunks, a CRLF straddling the boundary, an unterminated last line, and the degenerate cases.That matters because a set of inputs that all fit in the first chunk passes just as happily with the loop stopped after it. Confirmed by mutation: stopping after the first chunk leaves the narrow set green and fails the widened one at
content of 16384 bytes, n 1.Also verified against native
head -n Nfor n ∈ {0, 1, 5, 200, 99999} on a real source file, and that/dev/urandomand a FIFO nobody closes both return the right number of lines immediately.cargo fmt --all,cargo clippy --all-targetsandcargo test --allare green, rebased on currentdevelop.Follow-ups
Deliberately out of scope here, to keep this PR to the one behaviour change:
read_head_linesandhead_windoware two implementations of one contract, held together bytest_read_head_lines_matches_head_window. Unifying them removes the duplication and lets the stdin path stop early too.🤖 Generated with Claude Code