Merge pull request #2160 from Urgau/gh-range-diff-misc-2

Misc changes for our range-diff

Author: Urgau

src/gh_range_diff.rs

@@ -315,6 +315,8 @@ fn process_old_new(
 "#
     )?;
 
+    let mut diff_displayed = 0;
+
     let mut process_diffs = |filename, old_patch, new_patch| -> anyhow::Result<()> {
         // Removes diff markers to avoid false-positives
         let new_marker = format!("@@ {filename}:");
@@ -346,6 +348,7 @@ fn process_old_new(
                 html,
                 r#"<details open=""><summary>{filename} <a href="{before_href}">before</a> <a href="{after_href}">after</a></summary><pre class="diff-content">{diff}</pre></details>"#
             )?;
+            diff_displayed += 1;
         }
         Ok(())
     };
@@ -379,6 +382,11 @@ fn process_old_new(
         process_diffs(filename, "", &*new_file.patch)?;
     }
 
+    // Print message when there aren't any differences
+    if diff_displayed == 0 {
+        writeln!(html, "<p>No differences</p>")?;
+    }
+
     writeln!(
         html,
         r#"

src/handlers/check_commits/force_push_range_diff.rs

@@ -1,11 +1,24 @@
 use anyhow::Context as _;
 
 use crate::config::RangeDiffConfig;
+use crate::db::issue_data::IssueData;
+use crate::github::CommitBase;
 use crate::github::GithubCompare;
 use crate::github::IssueRepository;
 use crate::github::IssuesEvent;
+use crate::github::ReportedContentClassifiers;
 use crate::handlers::Context;
 
+/// Key for the state in the database
+const RANGE_DIFF_LINK_KEY: &str = "range-diff-link";
+
+/// State stored in the database
+#[derive(Debug, Default, serde::Deserialize, serde::Serialize, Clone, PartialEq)]
+struct RangeDiffLinkState {
+    /// ID of the most recent range-diff comment.
+    last_comment: Option<String>,
+}
+
 pub(super) async fn handle_event(
     ctx: &Context,
     host: &str,
@@ -23,25 +36,45 @@ pub(super) async fn handle_event(
     };
 
     let base = event.issue.base.as_ref().context("no base ref")?;
-    let org = event.repository.owner();
-    let repo = event.repository.name();
+    let issue_repo = IssueRepository {
+        organization: event.repository.owner().to_string(),
+        repository: event.repository.name().to_string(),
+    };
 
     let compare_before = ctx
         .github
-        .compare(
-            &IssueRepository {
-                organization: org.to_string(),
-                repository: repo.to_string(),
-            },
-            &base.sha,
-            &before,
-        )
+        .compare(&issue_repo, &base.sha, &before)
         .await
         .context("failed to get the before compare")?;
 
+    if let Some(message) = changed_base_commit(
+        host,
+        &issue_repo,
+        base,
+        &compare_before,
+        compare_after,
+        before,
+        after,
+    ) {
+        // Rebase detected, post a comment linking to our range-diff.
+        post_new_comment(ctx, event, message).await?;
+    }
+
+    Ok(())
+}
+
+fn changed_base_commit(
+    host: &str,
+    issue_repo: &IssueRepository,
+    base: &CommitBase,
+    compare_before: &GithubCompare,
+    compare_after: &GithubCompare,
+    oldhead: &str,
+    newhead: &str,
+) -> Option<String> {
     // Does the merge_base_commits differs? No, not a force-push with rebase.
     if compare_before.merge_base_commit.sha == compare_after.merge_base_commit.sha {
-        return Ok(());
+        return None;
     }
 
     let protocol = if host.starts_with("localhost:") {
@@ -51,13 +84,137 @@ pub(super) async fn handle_event(
     };
 
     let branch = &base.git_ref;
-    let (oldbase, oldhead) = (&compare_before.merge_base_commit.sha, before);
-    let (newbase, newhead) = (&compare_after.merge_base_commit.sha, after);
+    let oldbase = &compare_before.merge_base_commit.sha;
+    let newbase = &compare_after.merge_base_commit.sha;
 
-    // Rebase detected, post a comment to our range-diff.
-    event.issue.post_comment(&ctx.github,
-        &format!(r#"This PR was rebased onto a different {branch} commit! Check out the changes with our [`range-diff`]({protocol}://{host}/gh-range-diff/{org}/{repo}/{oldbase}..{oldhead}/{newbase}..{newhead})."#)
-    ).await.context("failed to post range-diff comment")?;
+    let message = format!(
+        r#"This PR was rebased onto a different {branch} commit. Here's a [range-diff]({protocol}://{host}/gh-range-diff/{issue_repo}/{oldbase}..{oldhead}/{newbase}..{newhead}) highlighting what actually changed.
+
+*Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.*"#
+    );
+
+    Some(message)
+}
+
+async fn post_new_comment(
+    ctx: &Context,
+    event: &IssuesEvent,
+    message: String,
+) -> anyhow::Result<()> {
+    let mut db = ctx.db.get().await;
+    let mut state: IssueData<'_, RangeDiffLinkState> =
+        IssueData::load(&mut db, &event.issue, RANGE_DIFF_LINK_KEY).await?;
+
+    // Hide previous range-diff comment.
+    if let Some(last_comment) = state.data.last_comment {
+        event
+            .issue
+            .hide_comment(
+                &ctx.github,
+                &last_comment,
+                ReportedContentClassifiers::Outdated,
+            )
+            .await
+            .context("failed to hide previous range-diff comment")?;
+    }
+
+    // Post new range-diff comment and remember it.
+    state.data.last_comment = Some(
+        event
+            .issue
+            .post_comment(&ctx.github, &message)
+            .await
+            .context("failed to post range-diff comment")?
+            .node_id,
+    );
+
+    state.save().await?;
 
     Ok(())
 }
+
+#[cfg(test)]
+mod tests {
+    use crate::handlers::check_commits::dummy_commit_from_body;
+
+    use super::changed_base_commit;
+
+    #[test]
+    fn unchanged_base_commit() {
+        assert_eq!(
+            changed_base_commit(
+                "mytriagebot.com",
+                &crate::github::IssueRepository {
+                    organization: "rust-lang".to_string(),
+                    repository: "rust".to_string()
+                },
+                &crate::github::CommitBase {
+                    sha: "sha-base-commit".to_string(),
+                    git_ref: "master".to_string(),
+                    repo: None
+                },
+                &crate::github::GithubCompare {
+                    base_commit: dummy_commit_from_body("base-commit-sha", "base-commit-body"),
+                    merge_base_commit: dummy_commit_from_body(
+                        "same-merge-commit",
+                        "merge-commit-body"
+                    ),
+                    files: vec![]
+                },
+                &crate::github::GithubCompare {
+                    base_commit: dummy_commit_from_body("base-commit-sha", "base-commit-body"),
+                    merge_base_commit: dummy_commit_from_body(
+                        "same-merge-commit",
+                        "merge-commit-body"
+                    ),
+                    files: vec![]
+                },
+                "oldhead",
+                "newhead"
+            ),
+            None
+        );
+    }
+
+    #[test]
+    fn changed_base_commit_() {
+        assert_eq!(
+            changed_base_commit(
+                "mytriagebot.com",
+                &crate::github::IssueRepository {
+                    organization: "rust-lang".to_string(),
+                    repository: "rust".to_string()
+                },
+                &crate::github::CommitBase {
+                    sha: "sha-base-commit".to_string(),
+                    git_ref: "master".to_string(),
+                    repo: None
+                },
+                &crate::github::GithubCompare {
+                    base_commit: dummy_commit_from_body("base-commit-sha", "base-commit-body"),
+                    merge_base_commit: dummy_commit_from_body(
+                        "before-merge-commit",
+                        "merge-commit-body"
+                    ),
+                    files: vec![]
+                },
+                &crate::github::GithubCompare {
+                    base_commit: dummy_commit_from_body("base-commit-sha", "base-commit-body"),
+                    merge_base_commit: dummy_commit_from_body(
+                        "after-merge-commit",
+                        "merge-commit-body"
+                    ),
+                    files: vec![]
+                },
+                "oldhead",
+                "newhead"
+            ),
+            Some(
+                r#"This PR was rebased onto a different master commit. Here's a [range-diff](https://mytriagebot.com/gh-range-diff/rust-lang/rust/before-merge-commit..oldhead/after-merge-commit..newhead) highlighting what actually changed.
+
+*Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.*"#
+                .to_string()
+            )
+        );
+    }
+}