From e1074840428c2f8e4a25344df019196e8b73a4f6 Mon Sep 17 00:00:00 2001 From: Don Garrett Date: Sat, 17 Oct 2026 07:47:48 +0000 Subject: [PATCH] nisse-review: a commit diff Gitea can't give falls back to the whole diff From nisse-review's review of #156: only a failed listing fell back to the PR's whole diff; one commit's diff failing (a force-push dropped it) failed the run. Tests also pin down its other three points, which the code already handled: a command is taken in once, a second review at the same head has its own marker, and `review` beside a comment is one fresh review. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk --- nisse-review/src/nisse_review/review.py | 13 +++--- nisse-review/tests/test_review_rounds.py | 54 +++++++++++++++++++++++- 2 files changed, 60 insertions(+), 7 deletions(-) diff --git a/nisse-review/src/nisse_review/review.py b/nisse-review/src/nisse_review/review.py index f93f9c2..4afcb1b 100644 --- a/nisse-review/src/nisse_review/review.py +++ b/nisse-review/src/nisse_review/review.py @@ -384,23 +384,24 @@ class Reviewer: def _changes(self, repo: str, pr: PullRequest, since: str) -> str: """The commits since ``since``, each with its diff; the whole PR's - diff if Gitea can't list them (``since`` gone, say).""" + diff if Gitea can't list them or give one's diff (``since`` gone + after a force-push, say).""" try: commits = self._gitea.commits_between(repo, since, pr.head_sha) + diffs = "\n".join( + f"commit {line}\n{self._gitea.commit_diff(repo, line.split()[0])}" + for line in commits + ) except GiteaError as exc: self._oplog.emit("review.compare_failed", cause=str(exc)[:300]) commits = [] if not commits: return ( - "Gitea couldn't list the commits since then (a force-push?): " + "Gitea couldn't give the commits since then (a force-push?): " "the whole PR's diff instead.\n" + self._cut(self._gitea.pull_request_diff(repo, pr.number)) + "\n\n" ) - diffs = "\n".join( - f"commit {line}\n{self._gitea.commit_diff(repo, line.split()[0])}" - for line in commits - ) return ( "The commits since:\n" + "\n".join(commits) diff --git a/nisse-review/tests/test_review_rounds.py b/nisse-review/tests/test_review_rounds.py index 4e6f870..513a9fc 100644 --- a/nisse-review/tests/test_review_rounds.py +++ b/nisse-review/tests/test_review_rounds.py @@ -136,6 +136,22 @@ def test_without_the_commits_since_it_reads_the_whole_diff(tmp_path: Path) -> No assert "the whole PR's diff instead" in last_brief(w) +def test_a_commit_whose_diff_cannot_be_read_falls_back_too(tmp_path: Path) -> None: + # nisse-review on #156: only a failed listing fell back; a failed diff + # cost a run, in the force-push case the fallback is for. + class Gone(FakeGitea): + def commit_diff(self, repo: str, sha: str) -> str: + raise GiteaError("404: no such commit") + + gitea = Gone(repos={REPO: [pr()]}) + w = world(tmp_path, Scripted(*REVIEW, [call("post_review", summary="ok")]), gitea) + w["scheduler"].round() + push(w) + w["scheduler"].round() + assert "the whole PR's diff instead" in last_brief(w) + assert item(w).position == REVIEWED + + def test_a_sign_off_is_said_once_while_it_stays_happy(tmp_path: Path) -> None: gitea = FakeGitea(repos={REPO: [pr()]}) model = Scripted( @@ -218,7 +234,9 @@ def test_stop_quiets_it_until_review_asks_again(tmp_path: Path) -> None: push(w) assert w["scheduler"].round() is False assert item(w).position == STOPPED - assert (stop, "+1") in w["gitea"].reactions + assert w["gitea"].reactions == [(stop, "+1")] + w["scheduler"].round() + assert w["gitea"].reactions == [(stop, "+1")] # taken in: said once w["gitea"].say("don", "Review.") assert w["scheduler"].round() is True assert "Review o/demo#7" in last_brief(w) # the whole PR, afresh @@ -357,3 +375,37 @@ def test_a_summary_answers_nothing_and_a_first_review_answers_no_points( w["scheduler"].round() assert "reply" not in model.offered[-1] and "post_summary" in model.offered[-1] assert item(w).position == CAPPED + + +def test_a_second_review_at_the_same_head_has_its_own_marker(tmp_path: Path) -> None: + # nisse-review on #156 feared the first review's unnumbered marker would + # pass for a crashed second one at the same head. + head = pr().head_sha + w = reviewed(tmp_path) + reviewer = w["scheduler"]._reviewer + assert reviewer.posted(REPO, 7, head, 1) + assert not reviewer.posted(REPO, 7, head, 2) # `review` again: not yet + w["gitea"].posted.append( + { + "repo": REPO, + "pr": 7, + "body": f"again\n\n{marker(head, 2)}", + "commit": head, + "comments": [], + } + ) + assert reviewer.posted(REPO, 7, head, 2) + assert marker(head, 1) not in marker(head, 2) + + +def test_review_with_a_comment_beside_it_is_one_fresh_review(tmp_path: Path) -> None: + # nisse-review on #156 feared a replies run, then a second, pointless + # whole-PR review: `review` makes the run a first one, which clears it. + w = reviewed(tmp_path, [call("post_review", summary="Fresh look.")]) + w["gitea"].say("don", "review") + w["gitea"].say("nisse-dev", "Also, a question.") + assert w["scheduler"].round() is True + assert "Review o/demo#7" in last_brief(w) # a first review, not replies + known = pr_of(item(w)) + assert (known.full, known.fresh, item(w).position) == (False, (), REVIEWED) + assert w["scheduler"].round() is False # nothing more due