Repository navigation
Conversation
📊 Automated PR Analysis
SummaryThis PR unifies two divergent head-window implementations (for files and stdin) into a single Review Checklist
Linked issues: #4122 Analyzed automatically by wshm · This is an automated analysis, not a human review. |
KuSh
force-pushed
the
fix/4122-followup-unify-head-window
branch
5 times, most recently
from
September 20, 2026 21:01
ef0f071 to
57f031a
Compare
Merged
KuSh
force-pushed
the
fix/4122-followup-unify-head-window
branch
2 times, most recently
from
October 7, 2026 16:45
bc42d35 to
978bd61
Compare
`read_head_lines` and `head_window` each carried their own scan for the end of the `n`th line, agreeing only by test. `head_prefix` now holds that rule once: where the first `n` newline-terminated lines end in a slice, and how many terminators are there. `head_window` applies it to bytes already in memory; `copy_head_lines` applies it per chunk with a running count, which is what lets it stop at the `n`th newline. `copy_head_lines` takes any `Read` and writes each chunk on as it comes, so two more things follow. `rtk read - --head-lines N` uses it instead of draining stdin first, and returns on a producer that never ends -- `yes | rtk read - --head-lines 3` used to read until memory ran out where `head -n 3` returns. And a window costs one chunk of memory rather than its own length: `rtk read /dev/zero --head-lines 1` holds steady where it grew past 440 MB in three seconds before, which matters on any file whose first line is enormous. What it prints is byte-identical to `head -n N`, from a file and from stdin, and on Unix a closed downstream now ends the process at the chunk it is writing, as `head` is ended. The producer feeding stdin then gets SIGPIPE as it would from `head`, so under `set -o pipefail` `seq 100000 | rtk read - --head-lines 5` exits 141 where it used to exit 0. A window the user asked for is now measured against the command it stands in for. `rtk read <f> --head-lines N` is what the hook rewrites `head -n N <f>` to, and it prints the window that command prints, so measuring it against the whole file credited RTK with the user's own choice of command: on a 20000-line file `--head-lines 5` booked 100%, `--head-lines 5000` booked 78%, `--tail-lines 5` booked 100%, while the plain read beside them -- the one place RTK really does emit everything -- booked 0%. The baseline is the window, the output is what was printed of it, and printing it whole is worth nothing. Those rows now name `head -n 5 <f>` instead of `cat <f>`, the command they stand in for. `--line-numbers` renders the window rather than printing it, but the window is still all the user asked for, so it is measured the same way against what was shown. A filter level, `--max-lines` and a plain read keep the whole input as their baseline, because there RTK is the one choosing to emit less than was asked for. `track_bytes` takes the emitted size as a byte count as well, so a lossy decode can no longer inflate it: each invalid sequence counted as the replacement character's three bytes, and `rtk read /dev/urandom --head-lines 5` booked -84%. With no window measured against a size on disk any more, a pseudo-file's claim stops being read as a measurement too -- `head -n 5 /proc/cpuinfo` booked -29 tokens against a reported size of 0, and `head -n 5 /sys/kernel/mm/transparent_hugepage/enabled` booked 99.4% against a reported page for a 23-byte attribute. All of them book 0% now, which is what RTK saved. `rtk gain` clamps a negative saving out of its headline, but nothing clamped the over-credited rows, which moved it directly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
KuSh
force-pushed
the
fix/4122-followup-unify-head-window
branch
from
October 9, 2026 20:43
978bd61 to
0324935
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
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.
Follow-up to #4122, addressing both review notes on it: unify the two head-window implementations, and stop the tracking row inflating the emitted size.
One rule for the head window
read_head_lines(chunked, added in #4122) andhead_window(a slice over bytes in memory) each carried their own scan for the end of thenth line. They agreed only because a test said so.head_prefixnow holds that rule once — where the firstnnewline-terminated lines end in a slice, and how many terminators are there.head_windowapplies it to bytes it already has;copy_head_linesapplies it to each chunk with a running count, which is what lets it stop at thenth newline instead of draining the source.copy_head_linestakes anyReadand writes each chunk on as it comes, so two things follow from the unification:rtk read - --head-lines Nused toread_to_endfirst.yes | rtk read - --head-lines 3kept reading until memory ran out; it now prints three lines and exits, likehead -n 3. (The hook only rewriteshead -n N <file>, never a pipeline, so this path is reached by typing it.)rtk read /dev/zero --head-lines 1held 440 MB and climbing after three seconds; it now holds flat — 17.3 MB for the debug build, 7.2 MB for the release build, steady across three seconds. It still blocks, ashead -n 1 /dev/zerodoes, but at bounded memory. The same applies to any file whose first line is enormous — a minified bundle, say — which the hook does rewrite.Output is unchanged. Windows from files and from stdin were compared byte-for-byte against native
head -n Novernin {0, 1, 2, 3, 10, 4999, 100000} on binary data, CRLF text, an unterminated last line, a 200 KB random file and a 5000-line file; exit codes match too (1 on a missing file, SIGPIPE on a closed stdout).What the equivalence test covers
test_head_windows_match_the_referencepinscopy_head_linesandhead_windowagainstreference_head_window, a frozen copy of the pre-refactor slice kept in the test module. Both are compared to that third implementation rather than to each other, so a shared rule that drifts is still caught rather than agreeing with itself.Its inputs deliberately span several
READ_CHUNKs, because reading across chunks is the only thing the chunked reader adds: a line twice the chunk size, a newline exactly on the boundary and on either side, 4000 short lines, a CRLF straddling the boundary, an unterminated last line, empty input; each atnin {0, 1, 2, 3, 10, 1000, 4000}. I checked it catches regressions by mutation: stopping after the first chunk, off-by-one in the shared rule, scanning each chunk forninstead of the lines still wanted, and writing the whole chunk instead of its accepted prefix all fail it; a narrow input set that fits in one chunk leaves the first of those green.test_copy_head_lines_stops_on_a_trickling_endless_sourcecovers what a pipe does and a file does not: five bytes per read, no end, a CRLF split across two reads. It counts the bytes the source hands out, so a reader that pulls one chunk past thenth newline fails it (verified by mutation) — the class this path exists to prevent.read_stdin_head_stops_without_draining_an_endless_producerruns the binary against a producer that prints three lines and keeps the pipe open, under a 20 s deadline, so a draining regression fails without growing memory; the unit tests on an endless FIFO and on the trickling source are bounded the same way.A closed downstream now ends the process at the chunk it is writing, where the buffered path wrote everything in one go after the reader had finished. Over 25 runs of
… --head-lines 5000 | head -1, this branch exited 141 every time, nativehead17 times, the buffered path never — closer tohead, and enough to make a pipeline underset -o pipefailfail where it used to pass. The producer side changes the same way:seq 100000 | rtk read - --head-lines 5underpipefailexits 141, as withhead, wheredevelopexits 0.One divergence from
headstays, deliberately: a seekable stdin is not rewound.{ rtk read - --head-lines 1 >/dev/null; wc -c; } < fleaves 8192 bytes consumed wherehead -n 1leaves 2, because the chunk that satisfied the count is not seeked back. The previous behaviour left nothing at all, having drained the stream.What a window is measured against
rtk read <f> --head-lines Nexists because the hook rewriteshead -n N <f>to it, and it prints the window that command prints. The rows measured it against the whole file anyway, so RTK took credit for the user's own choice of command — and the labels saidcat <f>, which is not what ran. Measured on a 20000-line file:read big.txt --head-lines 5cat big.txt27224 / 3 / 100%head -n 5 big.txt3 / 3 / 0%read big.txt --head-lines 5 -ncat big.txt77224 / 10 / 100%head -n 5 big.txt10 / 10 / 0%read big.txt --head-lines 5000cat big.txt27224 / 5974 / 78.1%head -n 5000 big.txt5974 / 5974 / 0%read big.txt --tail-lines 5cat big.txt27224 / 8 / 100%tail -n 5 big.txt8 / 8 / 0%read big.txt(a realcat)read read.rs --level aggressiveEach row carries two numbers, the window and what RTK printed of it; neither is a constant in the code, and every path today prints the window whole, so every such row reads 0%. That is the true figure, not a floor: output is byte-identical to native
head -n Nat n ∈ {5, 500, 5000, 19999, 99999} and totail -n Nat {5, 5000}, from a file and from stdin, and--head-linescannot combine with--max-lines, so nothing trims a requested window anywhere.--line-numbersis measured the same way: it renders the window rather than printing it, but the window is still all the user asked RTK to do, so the row is measured against what was shown.rtk read big.txt --head-lines 5 -nbooked 77224 / 10 / 100% ondevelopand books 10 / 10 / 0% here.Only a window the user asked for is measured this way.
--max-lines, the filter levels and a plain read keep the whole input as their baseline, because there RTK is the one deciding to emit less than was asked for — the--level aggressiverow above is untouched. One combination is left over-credited: a filter level together with a window.--level aggressive --head-lines 3books 99.6% where the filter alone earns 75.4%, and--level aggressive --tail-lines 5books 100.0%, because there the user and RTK each cut part of the output and there is no single command the row stands in for. The hook never produces either spelling.This also settles what a pipe is worth:
cat big.txt | rtk read - --head-lines 5books 3 / 3 / 0%, the same as every other spelling of the same window, because that is what the window is worth and not because the producer's size is unknowable.Emitted size in the tracking row
track_bytestook the emitted side as a&str, so a window was measured afterString::from_utf8_lossyhad replaced each invalid sequence with the three-byte replacement character. Both sides are byte counts now.With no window measured against a size on disk any more, a size that lies stops being read as a measurement too. Rows recorded before and after, against a binary built from
develop:read /dev/urandom --head-lines 5read /proc/cpuinfo --head-lines 5read /sys/kernel/mm/transparent_hugepage/enabled --head-lines 5read <non-UTF-8 file> --tail-lines 1rtk gainclamps a negative saving out of its headline, so those two never credited RTK there; nothing clamped the over-credited rows, which moved it directly — that is what the window baseline above is for.🤖 Generated with Claude Code