Repository navigation
Conversation
- Conflict list: drop rtk-ai#2579/rtk-ai#2590/rtk-ai#2615 (already flagged 07-17), add rtk-ai#2401/rtk-ai#2453 and the chunk-7 conflict cluster; note top-pick rtk-ai#2401 is a hand-port (issue #54). - Next-wave: restore recovered candidate rtk-ai#2256 (gh prompt-injection scan) dropped in distillation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
7be2a6e to
8b9e6b6
Compare
8b9e6b6 to
53146e4
Compare
53146e4 to
3f0776a
Compare
|
@KuSh rebased on current develop ( Wiring the two limits with the documented defaults (15 tracked / 10 untracked) means What changed since the earlier push: entries past a cap are Checked on a 25-modified + 15-untracked repo: develop lists 41 lines; this lists 5, and |
KuSh
left a comment
There was a problem hiding this comment.
Round 1 of 3 — CHANGES REQUESTED
Claim: [limits] status_max_files / status_max_untracked now bound the compact rtk git status listing (the rest stays recoverable through recall, conflicts never counted), and a partial [limits] section no longer drops the whole config.
Scope: accept if narrowed to "the two caps apply when set; unset by default, so rtk git status stays complete (#991)", plus the #[serde(default)] fix as is. (round 1, frozen)
Ran: rtk rewrite "git status" → rtk git status on real repos (25 modified + 15 untracked + an untracked dir; a 20-file merge with 3 conflicts; detached HEAD), develop merge-base 0c279f5 vs 3f0776a vs the merge into current develop, each binary built separately. Cases: no config, [limits] 2/1, 0/0, 100/100; -sb, -b, --short --branch, -s, --porcelain; rtk pipe -f git-status; git -C <other>; RTK_RECALL=0; sqlite and tee recovery modes, each hint followed through rtk rewrite. Invariants: under the hook | over-cap + accounting (shown + recalled = git status --porcelain, both modes) | non-default target (-C) | every decision (tracked / untracked / unmerged bucket, cap hit / exact / zero). Savings vs raw git status (964 B): develop 56%, this PR with defaults 67%, caps 2/1 91% — all above the 20% floor (CONTRIBUTING.md).
Matrix: never run before — executed this round
CI @3f0776a4: all green (ubuntu, macos, windows, clippy, fmt, benchmark)
Blocks merge (1, frozen at round 1)
src/core/config.rs:238— the documented defaults now apply to everyone, so a stock install hides dirty paths that develop lists. Evidence, no config.toml, 25 modified + 16 untracked: develop lists all 41; this PR lists 25 and prints... +16 more [+16 hidden: rtk recall …]. WithRTK_RECALL=0or[retriever] mode = "disabled"the note is just... +16 moreand the paths cannot be recovered from rtk at all. This is exactly what #991 removed (its test is kept here, but now run with caps of 100). The store write also costs time on every such status:hyperfine6.5 ms (develop) → 9.8 ms (this PR, defaults), and 6.6 ms for both when nothing is hidden. See "Your questions" for the change that clears this.
Optional — will not hold merge
- The listing tests that call
format_status_outputnow read the developer's own~/.config/rtk/config.toml: withstatus_max_files = 0set locally,test_format_status_output_{thai_filename,mixed_changes,preserves_rename_and_conflict_lines,preserves_nested_untracked_paths}fail. A clean CI runner never sees it. Routing those seven callers through a small helper that callsformat_status_innerwithusize::MAXcaps keeps them independent of local config. rtk pipe -f git-statusfed--porcelain=v2output putsu UU …into the tracked bucket, so under a cap a v2 conflict can be hidden (still recallable). The formatter only speaks v1, and nothing in rtk sends it v2, so this is only a note.- When
never_worsefalls back to raw (short headerless input throughrtk pipe), the recall row or tee file has already been written for a hint that is never printed. Harmless apart from one orphan row.
Filed as follow-ups
- #3431 (existing) — tee files are named
{epoch_seconds}_{slug}.log, so two capped statuses in the same second share one file and the first hint returns the second repo's entries. Reproduced withrtk git statusin repo A, thenrtk git -C B status: A'stail -n +3 …_git-status.logprintedB_f3…B_f6. Pre-existing for every tee caller (find,ls,search, …). This PR adds one caller, reached only with caps set and tee mode, so it's absorbed there (comment with the repro added) rather than held here. - Partial
[tracking]/[display]/[filters]sections still fail to parse and drop the whole config,[limits]included — pre-existing, covered by open #1483. Both PRs add the same#[serde(default)]line toLimitsConfig; whichever lands second rebases trivially.
Checked and correct — no need to re-verify
- Sqlite recall returns exactly the hidden entries; shown + recalled =
git status --porcelain, with no duplicates (38 hidden at 2/1, 16 at defaults). Tee mode'stail -n +N <file>is left native byrtk rewriteand returns the hidden tail. - A real merge with 20 staged files and 3
UUentries under caps 2/1: all three conflicts shown, only staged/untracked entries counted. -sb,-b,--short --branchtake the capped path;-sand--porcelainpass through untouched.git -C <other> status, detached HEAD andrtk pipe -f git-statusapply the same caps.#[serde(default)]onLimitsConfig: on develop,[limits]with only the two status keys makesrtk configfail (missing field … in 'limits') and drops the config; here it parses and keeps the other defaults.cargo fmt --check,cargo clippy --all-targets(0 warnings),cargo test --all(3936 passed) on this branch merged into current develop.
Hypotheses built and dropped
- "
config::limits()re-parses config.toml on the hot path": measured, develop 6.6 ms vs this PR 6.6 ms with caps set high enough that nothing is hidden. The time difference in the blocker comes from the store write, not the parse. - "One combined
+N morecount across both buckets hides which kind was cut": wording preference; recall shows it. - "
TEST_ENV_LOCK.lock().unwrap()in the new helper breaks the test rule": it's the established idiom in the existingtee.rsandretriever.rstests.
Your questions
- "I kept the documented defaults, but the alternatives are cheap: default both to unlimited (opt-in caps), or take option B from #2437 and delete the dead knobs. Which do you prefer?" → Opt-in: unset by default. Option B would throw away a feature this PR makes work, and the documented defaults undo #991 for everyone (blocker 1). One constraint I checked:
usize::MAXas the "unlimited" default breaksrtk configandrtk config --create(rtk: out-of-range value for u64 type, since TOML integers are i64), so the fields need to becomeOption<usize>withskip_serializing_if = "Option::is_none". The docs that name the defaults (src/core/README.md,src/cmds/git/README.md,docs/contributing/CODING_PRACTICES.md) change with them. I checked that shape locally: it passes fmt, clippy andcargo test --all, and on the same repo gives: no config → all 41 entries, byte-identical to develop; caps 2/1 → capped with a working recall;status_max_files = 0alone → honoured;rtk config/--createserialize and round-trip.
Next
Make the two caps opt-in (Option<usize>, unset by default). After that this merges.
Rounds: 1/3. Threads: 0 open, 0 resolved this round.
| format_status_inner( | ||
| porcelain, | ||
| None, | ||
| limits.status_max_files, |
There was a problem hiding this comment.
Blocks merge (1). With the documented defaults, this line caps every stock rtk git status at 15 tracked / 10 untracked: a repo with 25 modified + 16 untracked files goes from all 41 entries on develop to 25 plus ... +16 more, and with recall off the hidden paths cannot be recovered from rtk. That reverses #991. Making the caps Option<usize>, unset by default, clears it; details in the review body.
…mits `limits.status_max_files` and `limits.status_max_untracked` were parsed and documented, but nothing read them, so `rtk git status` listed every entry whatever the config said. The status formatter now applies both caps, one per bucket (tracked and untracked). Entries past a cap are counted as `... +N more`, followed by the recall hint so the hidden entries stay recoverable. Conflicted paths are never counted against a cap, so a merge that stages many files cannot push the conflicts off the listing. `LimitsConfig` also gains `#[serde(default)]`. A `[limits]` section that sets only the two status keys, as in the issue, used to fail to parse, and the failed load silently dropped the whole config (recall included). Fixes rtk-ai#2437
3f0776a to
28b13bd
Compare
|
Made both caps opt-in as you suggested — Verified the four cases you listed, on a real repo (5 tracked modified + 4 untracked):
Rebased onto current develop (45b0b4c) since this was 41 commits behind. |
KuSh
left a comment
There was a problem hiding this comment.
Round 2 of 3 — CHANGES REQUESTED
Claim: [limits] status_max_files / status_max_untracked now bound the compact rtk git status listing (the rest stays recoverable through recall, conflicts never counted), and a partial [limits] section no longer drops the whole config.
Scope: accept if narrowed to "the two caps apply when set; unset by default, so rtk git status stays complete (#991)", plus the #[serde(default)] fix as is. (round 1, frozen)
Ran: develop 45b0b4c vs 28b13bd (the head is on the develop tip, so it is also the merge check), each binary built separately. Same real repos as round 1 (25 modified + 15 untracked + an untracked dir; a 20-file merge with 3 conflicts; detached HEAD), each command through rtk rewrite first. Cases: no config file; a config.toml written by develop's rtk telemetry disable; [limits] 2/1, 0/0, status_max_files = 0 alone; -sb, -b, --short --branch, -s, --porcelain; rtk pipe -f git-status; git -C <other>; RTK_RECALL=0; sqlite and tee recovery; rtk config / rtk config --create. Invariants: under the hook | over-cap + accounting (shown + recalled = git status --porcelain) | non-default target (-C) | every decision (tracked / untracked / unmerged bucket, cap set / unset / zero). Mutation: defaults set back to Some(15) / Some(10) against the test suite. Savings unchanged from round 1 (no-config output is develop's).
Matrix: executed round 1 @3f0776a4; rerun this round on 28b13bd
CI @28b13bd6: all green (ubuntu, macos, windows, clippy, fmt, benchmark)
Thanks for the quick turnaround. The opt-in change does what it says for a fresh install. The remaining blocker is about configs rtk already wrote, and part of it is mine: in round 1 I said Option<usize> would clear it, and that was incomplete.
Blocks merge (1, frozen at round 1 — still open)
src/core/config.rs:228— rtk has been writing these two keys into users' config.toml since v0.30.1, so the caps switch on for existing installs.Config::save()writes the whole config. It runs onrtk config --create, and since v0.36.0 on every telemetry consent write too: the prompt duringrtk init, andrtk telemetry enable|disable|forget. On develop,rtk telemetry disablewith no prior config produces[limits] … status_max_files = 15 / status_max_untracked = 10. On that exact file, the 25 + 15 repo gives 42 lines on develop, but 27 lines ending... +16 more [+16 hidden: rtk recall …]on this head. So for anyone who answered the telemetry prompt, this is still the round-1 regression. The PR cannot tell those auto-written values from a 15/10 a user chose, so reading the same keys with a new meaning is what has to change.
What a fix has to satisfy (the rows I will rerun):- a config.toml written by develop (
rtk telemetry disableorrtk config --create) →rtk git statusoutput byte-identical to develop, with 25 + 15 entries and no... +N more - a cap the user sets on purpose still applies, and recall still returns exactly the hidden entries (the 2/1 row)
rtk config/rtk config --createstill round-trip- a test that fails if the defaults go back to 15/10 and one that fails on a develop-written config. Today, with the defaults set back to
Some(15)/Some(10), all 4 tests intests/git_status_limits_test.rsand thecore::configunit tests still pass: the sandbox has 5 tracked + 4 untracked files, under both caps.
One shape that meets all four: give the caps key names rtk has never written (for examplegit_status_max_files/git_status_max_untracked). The old keys then parse as unknown and are ignored, as other unknown keys are today. No user loses anything, because the old keys never did anything before this PR. Your call on the name, or on a different mechanism that meets the rows.
- a config.toml written by develop (
Optional — will not hold merge
- Still open from round 1: the listing tests that call
format_status_outputread the developer's own config.toml. Withstatus_max_files = 0set locally,test_format_status_output_{thai_filename,mixed_changes,preserves_rename_and_conflict_lines,preserves_nested_untracked_paths}fail on this head. Callingformat_status_innerwithusize::MAXcaps in those tests removes the dependency. - The commit message still describes the caps as applied without saying they are opt-in; one sentence saying the keys are unset by default would keep the changelog accurate.
Filed as follow-ups
Checked and correct — no need to re-verify
- No config file: output byte-identical to develop for plain
rtk git status,-sb,-b,--short --branch. rtk configwith no file andrtk config --create: noout-of-rangeerror, and neither status key is written. A set cap shows up inrtk config.- Caps 2/1: shown + recalled =
git status --porcelainexactly. Tee hinttail -n +Nstays native under the hook and returns the hidden tail.RTK_RECALL=0gives the bare count.-C, detached HEAD andrtk pipe -f git-statusapply the same caps.-s/--porcelainpass through. status_max_files = 0alone: tracked entries hidden, all 16 untracked shown. Real merge under 2/1: all 3UUshown.- Every reader of the two fields was updated (config.rs, both call sites in git_cmd.rs, the three docs). The old 15/10 is claimed nowhere else.
- Local gate on 28b13bd: fmt, clippy (0 warnings),
cargo test --all4043 passed.
Your questions
- none this round.
Next
Stop rtk-written configs from turning the caps on (the four rows under blocker 1); after that this merges. Round 3 is the last one, so if anything small is left then, I'll fix it on the branch rather than send another list.
Rounds: 2/3. Threads: 1 open (blocker 1, waiting on the fix above), 0 resolved this round.
Summary
Fixes #2437
LimitsConfig::status_max_filesandstatus_max_untrackedwere parsed from config but never read by the git status formatter, so setting them did nothing. This wires them up.rtk git status(and-sb/-b, andrtk pipe) now cap the listing at the configured limits. Entries past a cap are counted as... +N morewith the usual recall hint, so they stay recoverable:rtk recall <hash>returns exactly the hidden entries. With recall off it is just... +N more.0means summary only.DD AU UD UA DU AA UU) never count against a cap, so a large merge cannot push a conflict off the list.LimitsConfiggets#[serde(default)]: a[limits]section that sets only the two status keys (the repro in [config] status_max_files and status_max_untracked are declared in LimitsConfig but never read #2437) failed to parse at all, which silently dropped the whole config and turned recall off with it.--porcelain,-son its own, ...) still pass through untouched.Defaults are unchanged: 15 tracked / 10 untracked, as documented. That does mean
rtk git statusnow truncates by default, which is the part of #2437 worth a maintainer's call, since status completeness was made a goal in 6ebde6d and 7753e48. If you would rather default both to unlimited, or remove the knobs (option B in the issue), that is a small change.Verified
Repo with 25 modified + 15 untracked files and
[limits] status_max_files = 2, status_max_untracked = 1:* main, 2 tracked, 1 untracked,... +37 more [+37 hidden: rtk recall <hash>];rtk recall <hash>returns the 37 hidden entries, byte-identical togit status --porcelainminus the 3 shown... +15 more [...]Tests
tests/git_status_limits_test.rs, unix): a partial[limits]caps the listing, larger caps hide nothing, andrtk recallreturns exactly the hidden entriestest_format_status_output_shows_every_file_when_many_are_dirtyis kept (same name and assertions), now calling the formatter with explicit large caps#[serde(default)]is removedcargo fmt --all -- --check,cargo clippy --all-targets(0 warnings),cargo test --all(all pass)