Repository navigation
Conversation
KuSh
force-pushed
the
fix/4117-followup-log-probe
branch
from
September 20, 2026 18:50
c2ad379 to
7d14fcb
Compare
📊 Automated PR Analysis
SummaryFixes four bugs in rtk git log's cap-notice probe (walk_exceeds_limit): -z NUL-separated output silenced the count, the notice could print alongside git's own fatal error for a refused command, --check was misread as a refusal, and --binary/--cc/-c/--remerge-diff left patch output that got miscounted as commits due to indentation. Also refactors args_without_patch_shape into a generic args_without_flags helper and adds extensive new tests. Review Checklist
Linked issues: #4117 Analyzed automatically by wshm · This is an automated analysis, not a human review. |
KuSh
force-pushed
the
fix/4117-followup-log-probe
branch
from
September 20, 2026 20:28
7d14fcb to
4f70536
Compare
1 task done
Merged
…ight `walk_exceeds_limit` asks git whether `rtk git log`'s ten-commit cap actually cut anything. It was wrong in both directions: silent on walks the cap had gutted, and loud on walks it never touched. Two kinds of walk, two questions. Without a diff-based filter (`-S`, `-G`, `--diff-filter`, `--find-object`) the probe skips past the limit and asks whether anything is left. With one it has to count instead, because git applies `--skip` before such a filter and `--max-count` after it. Most of what follows is the counting branch getting an honest count, and the notice being owed at all. What the probe forwards. It may drop what only decides how git prints, and must keep what decides which commits come back -- a distinction the flags do not respect. `--line-prefix` and `--graph` are dropped because both put something in front of every record, and `--graph` draws its rail with the characters a patch begins its lines with (`|` before a diff body, `-` before a removed line), so no trimming rule serves both. But `--graph` also turns on parent rewriting, which decides which commits come back: one path here holds a single commit without it and fifteen with it, so the probe asks for `--parents` instead, which turns on the same rewriting and prints nothing beside a `%H` record. `--reverse` is dropped because it moves `--max-count`: git drains the whole walk before showing anything, so the count bounds the walk rather than what came through the filter, and eleven records became six against a selection of 836. `--check` joins `--exit-code`, whose non-zero exit reports on the diff rather than on the command. And `-c`/`--cc`/`--remerge-diff` and every `--diff-merges` format but `off` are kept whenever the walk selects by diff: they give merge commits a diff, and a diff-based filter selects on the diffs the walk produces. On a repository whose merges carry content from neither parent, `git log -S evil` selects nothing where `git log -c -S evil` selects 14. Off such a walk they are still dropped, which spares a configured `diff.external` the eleventh run of a ten-commit window. The user's own `--skip` is now reproduced in the counting branch too. RTK's has to win, so the user's is stripped and re-applied -- but only the other branch re-applied it, and the count covered commits they had already skipped past: `--skip=20 -S NEEDLE` leaves five commits and announced a cap of ten. What counts as a commit in the answer. Records are split on NUL as well as newline, because `-z` terminates them that way and the whole walk had been read as one line matching nothing. A record is a commit only at an object name's exact length -- `%H` does not abbreviate, for neither `--abbrev-commit` nor `core.abbrev` -- which keeps out the paths `--name-only -z` puts in a record of their own, where a file called `cafebabe01` had counted as a commit. And only at column 0, which keeps out the diff context of a probe that kept a merge-diff flag, in the case where that context is itself full-length hex: a file of checksums. Last, the notice is gated on git not having refused. The probe prunes `--pretty`/`--format`/`--output`, so it can succeed on arguments git rejected outright, and the notice landed under git's own `fatal:` line. Failure is not that test: git refuses with 128, 129 or 128 + n and keeps the small codes for what the diff machinery found in a walk it ran and printed in full -- 1 for `--exit-code`, 2 for `--check`. The strip these go through is the one the `diff`/`show` header already used, lifted behind a predicate so the probe can widen it without widening that header's own idea of patch shape. Going through it is what lets `-c` be dropped at all: a short flag shares its argument with whatever was clustered onto it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
KuSh
force-pushed
the
fix/4117-followup-log-probe
branch
from
October 7, 2026 00:02
4f70536 to
362c6f7
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.
walk_exceeds_limitasks git whetherrtk git log's ten-commit cap actually cut anything, so the notice can be printed only when it did. It was wrong in both directions: silent on walks the cap had gutted, and loud on walks it never touched.The first two items below are the reviewer's follow-ups on #4117. The rest are the same defect found elsewhere in the same probe while checking them, and all reproduce on
developtoday.Two kinds of walk, two questions
Without a diff-based filter (
-S,-G,--diff-filter,--find-object) the probe skips past the limit and asks whether anything is left. With one it has to count instead, because git applies--skipbefore such a filter and--max-countafter it. Most of what follows is the counting branch getting an honest count.What the probe forwards
It may drop what only decides how git prints, and must keep what decides which commits come back — a distinction the flags themselves do not respect.
Dropped.
--line-prefixand--graph, because both put something in front of every record;--graph's rail is drawn with the characters a patch begins its lines with (|before a diff body,-before a removed line), so no trimming rule serves both.--reverse, because it moves--max-count: git drains the whole walk before showing anything, so the count bounds the walk rather than what came through the filter — eleven records against a selection of 836.--check, joining--exit-code: its non-zero exit reports on the diff, not on the command, and the probe read it as git refusing.Kept.
--graphalso turns on parent rewriting, which decides which commits come back and not merely their order, so the probe asks for--parentsin its place — same rewriting, and it prints nothing beside a%Hrecord, not even on a merge. And-c/--cc/--remerge-diffand every--diff-mergesformat butoffare kept whenever the walk selects by diff: they give merge commits a diff, and a diff-based filter selects on the diffs the walk produces. Off such a walk they are dropped as before, which keeps the saving where it is safe.-cis the short spelling of--cc, and a short flag cannot be dropped by argument index the way the long ones are, because it shares its argument with whatever was clustered onto it. It goes through the cluster-aware strip thediff/showheader already used, lifted behind a predicate here so the probe can widen what it drops without widening that header's own idea of patch shape.The user's own
--skipRTK's
--skiphas to win, so the user's is stripped and re-applied — but only the non-counting branch re-applied it, and the count covered commits they had already skipped past.What counts as a commit in the answer
Records are split on NUL as well as newline, because
-zterminates them that way and the whole walk had been read as one line matching nothing.A record is a commit only at an object name's exact length —
%Hdoes not abbreviate, for neither--abbrev-commitnor--abbrev=<n>norcore.abbrev, unlike%h— which keeps out the paths--name-only -zputs in a record of their own at column 0, where a file calledcafebabe01had counted as a commit. And only at column 0, which keeps out the diff context of a probe that kept a merge-diff flag, in the one case where that context is itself full-length hex: a file of checksums.The notice is owed only when git ran the walk
The probe prunes
--pretty/--format/--output, so it can succeed on arguments git rejected outright, and the cap notice landed under git's ownfatal:line. Failure is not that test: git refuses alogwith 128 (fatal), 129 (usage) or 128 + n (a signal), and keeps the small codes for what the diff machinery found in a walk it ran and printed in full — 1 for--exit-code, 2 for--check.Measured
developat 727ee6e against this branch, same repo and same build flow per row.selis what git itself selects, counted with--no-patch --pretty=format:%H(plain--onelinedoes not suppress a combined patch, and counting its lines reads 14 selected commits as 149). The cap is 10, so the notice is owed exactly whensel > 10.-p -S LazyLock -z-p --pretty=bogusfatal:+ noticefatal:only-p --check-p --line-prefix=zz -S LazyLock-p --reverse -S LazyLock-p --skip=20 -S LazyLock--binary -S NEEDLE--graph --cc -S NEEDLE-p -c -S evil-p --diff-merges=c -S evil--graph --full-history --stat -- p.txt--name-only -z -S NEEDLE --pickaxe-all-p --exit-codeThe last five are the guards:
--exit-codemakes a successful walk exit 1, and the-c/--diff-merges/--graphrows are the walks whose selection depends on flags the probe now keeps.External diff program invocations for a ten-commit window,
GIT_EXTERNAL_DIFFset:git log -p --ext-diffgit log --binary --ext-diffgit log --cc --ext-diffgit log -c --ext-diffgit log --remerge-diff --ext-diffgit log -c -S LazyLock --ext-diffThe last row is the deliberate limit of that saving: once a diff-based filter is present the probe has to run the walk the user ran, merge diffs and all, so there is nothing to save.
Tests
tests/git_log_cap_notice_test.rsgrows one test per defect, plus fixtures for an evil-merge repo, a TREESAME-merge repo, a checksum file and a tree of hex-named paths:a_nul_separated_diff_selected_walk_announces_the_capa_walk_git_refused_carries_no_cap_noticea_check_run_still_announces_the_capa_merge_diff_walk_is_measured_with_its_merges_still_in_ita_graphed_diff_selected_walk_is_measured_like_any_othera_graphed_walk_keeps_the_commits_its_parent_rewriting_addsa_reversed_diff_selected_walk_is_still_measured_by_what_it_selectsa_diff_selected_walk_under_a_user_skip_is_measured_from_where_they_starteda_line_prefix_does_not_hide_a_diff_selected_capa_path_that_looks_like_an_object_name_is_not_counted_as_a_commita_file_of_bare_hex_words_is_not_counted_as_a_walkthe_probe_does_not_rerun_the_users_diff_program, extended with--binary,--ccand-cBeside the functions in
git_cmd.rs:a_commit_name_is_a_whole_object_name_at_column_zero, and four overlog_probe_argscovering the merge-diff family on filtered and unfiltered walks, both--diff-mergesspellings, cluster rebuilding, and a pathspec past--.Each fix was verified to fail exactly one of these tests when reverted alone, and
CAPPED_SHAPESlost three duplicate entries it had accumulated.cargo fmt --all --check,cargo clippy --all-targetsandcargo test --allare clean.🤖 Generated with Claude Code