Review a force-pushed pull request incrementally instead of falling back to the whole diff

Offen
  • Stimmt etwas nicht?
#587 4 Kommentare 0 Reaktionen 0 zugewiesene Personen Auf GitHub ansehen

Maintainer antworten meist innerhalb von 1 Tag

Dieses Issue hat noch niemand übernommen.

Bewertung

Schwierigkeit
4/5
Geschätzter Aufwand
3-5 Tage
Anfängerfreundlichkeit
48/100
Issue-Typ
Feature
Klarheit
Klar beschrieben
Aktivitätsstatus
Aktiv
Tech-Stack
git, shell
Bereich
tooling

Rechercherichtung

Start with .claude/skills/codetrial-review/review-bundle.sh around lines 338–363, then read the related SKILL.md and lenses.md guidance. Trace how --since resolves heads and writes the manifest, and inspect the existing shell test setup before implementing the four described cases. Done means the bundle reports rewritten or rebased updates as specified, falls back for an unresolved head, and ./scripts/test.sh passes.

Vom Indexierungsmodell aus dem Issue-Text verfasst.

Beschreibung

Problem

review-bundle.sh --since <sha> gives a reviewer only the work added since the head an earlier review read, but only when that head is an ancestor of the new head and no merge commit lies between them (.claude/skills/codetrial-review/review-bundle.sh, lines 338-363). Otherwise the manifest records not an ancestor of the head, so the whole diff is reviewed, and no since.diff is written.

Maintainers routinely ask for a rebase or for commits to be squashed and the author answers with a force push, which is exactly the case where the old head stops being an ancestor. So the update that happens most often during review is the one that gets no incremental range, and the reviewer rereads a diff they have already read.

Current behavior

I ran review-bundle.sh 585 <dir> sysprog21/codetrial --since <sha> on main b2c2bc9 for three earlier heads of #585, which was force pushed before it merged: 9bfab6c, d8d4fe8 and 533bc5a (taken from the commit ids of its reviews and review comments). Each run records status: "diverged" with not an ancestor of the head, so the whole diff is reviewed, and writes no since.diff. These heads can still be fetched by full SHA (git fetch rejects the short form), and all four heads share the base efdd342.

For that pull request the whole diff is 1130 lines. What the author changed between d8d4fe8 and the final head is a 22-line diff that deletes 6 lines in one file (git diff d8d4fe8 6bf2003).

git range-diff efdd342..d8d4fe8 efdd342..6bf2003 is 133 lines for the same change, because every rewritten commit also gets a new Change-Id trailer and the commit message hunks show it. So range-diff alone is not the right first tool when the base did not move.

Desired outcome

When --since is not an ancestor, the bundle still tells the reviewer what the author changed since the head they read:

  • If the old head and the new head have the same merge base with the target branch (a squash, an amend, a reorder), the answer is the plain tree diff git diff <old head> <new head>.
  • If the base moved (a rebase), the answer is git range-diff <old base>..<old head> <new base>..<new head>, so that a conflict-free rebase shows no change in any commit and a conflict resolution or an amended commit shows up as a change to read first.

Proposed scope

One pull request, limited to .claude/skills/codetrial-review/:

  • review-bundle.sh takes an optional --since-base <sha> next to --since. When --since is not an ancestor and the old head can be resolved locally or fetched by SHA, it writes since.diff for the same-merge-base case and since.range-diff.txt for the moved-base case, and records which one in the manifest as status: "rewritten" or status: "rebased". A short --since is expanded to the full SHA first, because git fetch does not accept the short form.
  • In the range-diff output, lines that only differ by a Change-Id: trailer are not shown, so a hook-driven rechain is not reported as a change.
  • When the old head cannot be resolved, the behavior is unchanged: the manifest says why and the whole diff is reviewed.
  • SKILL.md and lenses.md say that these files are a pointer to what to read first, not a replacement for the full diff, because the pairing is heuristic and merge commits are ignored by default.
  • A shell test that builds a small repository and checks four outcomes: a squash on an unchanged base, a conflict-free rebase, a rebase with a conflict resolved, and an unresolvable old head falling back.

Out of scope: a merge of the base into the branch (the whole diff is still reviewed), Change-Id tracking, and any change to memory.py or the findings ledger.

Open question

The old base is not recorded anywhere today; the manifest stores only the base of the current bundle. Should the caller pass --since-base explicitly, or should the manifest of the previous bundle be read when it is present? I would start with the explicit flag, which needs no new state.

Acceptance criteria

  • For #585, --since d8d4fe8 writes a since.diff containing the 22-line diff of the author's change instead of recording not an ancestor, and the manifest says rewritten.
  • After a conflict-free rebase in the new test, since.range-diff.txt shows every commit as unchanged.
  • After a rebase with a conflict resolved, it shows that change and nothing else.
  • With an unresolvable old head, the output is the same as today.
  • ./scripts/test.sh passes, and the new test fails if the new branch is removed.

Related

#585 added a findings ledger that places findings again after a push; it does not change which diff a reviewer reads after a force push. I searched open and closed issues and pull requests for range-diff and found none.

I would like to implement this myself. I read review-bundle.sh and SKILL.md on main b2c2bc9. I will wait for the issue to be assigned to me before opening a pull request, and I will keep the change to one pull request with the test above.

Vorherrschende Sprache
Rust
Sterne
223
Forks
148
Ø Merge
1 T. 11 Std.
Gemergte PRs (30 T.)
224

Entwicklungsumgebung

Erste Schritte

  1. Lesen Sie das ganze Issue und danach den Beitragsleitfaden des Projekts.
  2. Schreiben Sie ins Issue, dass Sie es übernehmen — das erspart doppelte Arbeit.
  3. Forken Sie das Repository und arbeiten Sie in einem Branch.
  4. Öffnen Sie einen Pull Request, der die Issue-Nummer nennt.

Mehr aus sysprog21/codetrial

Alle Issues in sysprog21/codetrial

Ähnliche Issues

Weitere Issues zu Rust

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.