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)

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