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

Abierto
  • ¿Algo no está bien?
#587 4 comentarios 0 reacciones 0 asignados Ver en GitHub

Los mantenedores suelen responder en 1 día

Nadie ha tomado este issue todavía.

Evaluación

Dificultad
4/5
Tiempo estimado
3-5 días
Aptitud para principiantes
48/100
Tipo de issue
Nueva funcionalidad
Claridad
Bien especificado
Estado de actividad
Activo
Stack tecnológico
git, shell
Área
tooling

Línea de trabajo

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.

Escrito por el modelo de indexación a partir del texto del issue.

Descripción

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.

Lenguaje dominante
Rust
Estrellas
223
Forks
148
Merge medio
1 d 11 h
PR fusionados (30 d)
224

Preparar el entorno

Primeros pasos

  1. Lee el issue completo y luego la guía de contribución del proyecto.
  2. Comenta en el issue que vas a ocuparte — evita que dos personas hagan lo mismo.
  3. Haz un fork del repositorio y trabaja en una rama.
  4. Abre un pull request que haga referencia al número del issue.

Más de sysprog21/codetrial

Todos los issues de sysprog21/codetrial

Issues similares

Más issues de Rust

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.