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

Aperta
  • Qualcosa non va?
#587 4 commenti 0 reazioni 0 assegnatari Vedi su GitHub

I maintainer di solito rispondono entro 1 giorno

Nessuno ha ancora preso questa issue.

Valutazione

Difficoltà
4/5
Tempo stimato
3-5 giorni
Idoneità per principianti
48/100
Tipo di issue
Funzionalità
Chiarezza
Specificata chiaramente
Stato di attività
Attiva
Stack tecnologico
git, shell
Ambito
tooling

Direzione di ricerca

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.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Descrizione

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.

Lingua principale
Rust
Stelle
223
Fork
148
Merge medio
1g 11h
PR unite (30g)
224

Preparare l'ambiente

Come iniziare

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. Apri una pull request che faccia riferimento al numero della issue.

Altre issue di sysprog21/codetrial

Tutte le issue di sysprog21/codetrial

Issue simili

Altre issue su Rust

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.