Do not add or remove labels on PRs unnecessarily

src/bors/build_queue.rs

@@ -279,7 +279,7 @@ async fn maybe_complete_build(
 
     db.update_build_status(build, status).await?;
     if let Some(trigger) = trigger {
-        handle_label_trigger(repo, pr_num, trigger).await?;
+        handle_label_trigger(repo, pr_num, None, trigger).await?;
     }
 
     if let Some(check_run_id) = build.check_run_id {

src/bors/handlers/mod.rs

@@ -643,10 +643,17 @@ async fn has_permission(
 pub async fn unapprove_pr(
     repo_state: &RepositoryState,
     db: &PgDbClient,
-    pr: &PullRequestModel,
+    pr_db: &PullRequestModel,
+    pr_gh: &PullRequest,
 ) -> anyhow::Result<()> {
-    db.unapprove(pr).await?;
-    handle_label_trigger(repo_state, pr.number, LabelTrigger::Unapproved).await
+    db.unapprove(pr_db).await?;
+    handle_label_trigger(
+        repo_state,
+        pr_db.number,
+        Some(pr_gh),
+        LabelTrigger::Unapproved,
+    )
+    .await
 }
 
 /// Is this branch interesting for the bot?

src/bors/handlers/pr_events.rs

@@ -41,7 +41,7 @@ pub(super) async fn handle_pull_request_edited(
         return Ok(());
     }
 
-    unapprove_pr(&repo_state, &db, &pr_model).await?;
+    unapprove_pr(&repo_state, &db, &pr_model, pr).await?;
     notify_of_edited_pr(&repo_state, pr_number, &payload.pull_request.base.name).await
 }
 
@@ -76,7 +76,7 @@ pub(super) async fn handle_push_to_pull_request(
         .as_ref()
         .map(|b| b.status.is_failure())
         .unwrap_or(false);
-    unapprove_pr(&repo_state, &db, &pr_model).await?;
+    unapprove_pr(&repo_state, &db, &pr_model, pr).await?;
 
     // If we had an approved PR with a failed build, there's not much point in sending this warning
     if !had_failed_build {

src/bors/handlers/review.rs

@@ -63,7 +63,13 @@ pub(super) async fn command_approve(
     let priority = priority.or(pr.db.priority.map(|p| p as u32));
 
     merge_queue_tx.notify().await?;
-    handle_label_trigger(&repo_state, pr.number(), LabelTrigger::Approved).await?;
+    handle_label_trigger(
+        &repo_state,
+        pr.number(),
+        Some(pr.github),
+        LabelTrigger::Approved,
+    )
+    .await?;
 
     notify_of_approval(ctx, &repo_state, pr, priority, approver.as_str()).await
 }
@@ -145,7 +151,7 @@ pub(super) async fn command_unapprove(
         AutoBuildCancelReason::Unapproval,
     )
     .await?;
-    unapprove_pr(&repo_state, &db, pr.db).await?;
+    unapprove_pr(&repo_state, &db, pr.db, pr.github).await?;
     notify_of_unapproval(&repo_state, pr, auto_build_cancel_message).await?;
 
     Ok(())

src/bors/labels.rs

@@ -1,14 +1,16 @@
 use itertools::Itertools;
+use std::collections::HashSet;
 use tracing::log;
 
 use crate::bors::RepositoryState;
-use crate::github::{LabelModification, LabelTrigger, PullRequestNumber};
+use crate::github::{LabelModification, LabelTrigger, PullRequest, PullRequestNumber};
 
 /// If there are any label modifications that should be performed on the given PR when `trigger`
 /// happens, this function will perform them.
 pub async fn handle_label_trigger(
     repo: &RepositoryState,
-    pr: PullRequestNumber,
+    pr_number: PullRequestNumber,
+    pr: Option<&PullRequest>,
     trigger: LabelTrigger,
 ) -> anyhow::Result<()> {
     let mut add: Vec<String> = Vec::new();
@@ -21,14 +23,26 @@ pub async fn handle_label_trigger(
                 LabelModification::Add(label) => itertools::Either::Left(label.clone()),
                 LabelModification::Remove(label) => itertools::Either::Right(label.clone()),
             });
+
+        // If we know the GitHub state, only remove/add labels that will actually have any effect on the
+        // PR.
+        if let Some(pr) = pr {
+            let existing_labels: HashSet<&str> = pr.labels.iter().map(|s| s.as_str()).collect();
+            add.retain(|l| !existing_labels.contains(l.as_str()));
+            remove.retain(|l| existing_labels.contains(l.as_str()));
+            log::info!(
+                "Filtered labels: requested = {modifications:?}, pr = {existing_labels:?}, add = {add:?}, remove = {remove:?}"
+            );
+        }
     }
+
     if !add.is_empty() {
         log::info!("Adding label(s) {add:?}");
-        repo.client.add_labels(pr, &add).await?;
+        repo.client.add_labels(pr_number, &add).await?;
     }
     if !remove.is_empty() {
         log::info!("Removing label(s) {remove:?}");
-        repo.client.remove_labels(pr, &remove).await?;
+        repo.client.remove_labels(pr_number, &remove).await?;
     }
     Ok(())
 }

src/bors/mergeability_queue.rs

@@ -14,7 +14,7 @@ use crate::PgDbClient;
 use crate::bors::comment::conflict_comment;
 use crate::bors::handlers::unapprove_pr;
 use crate::database::{MergeableState, OctocrabMergeableState, PullRequestModel};
-use crate::github::{GithubRepoName, PullRequestNumber};
+use crate::github::{GithubRepoName, PullRequest, PullRequestNumber};
 use std::cmp::Reverse;
 use std::collections::{BTreeMap, BinaryHeap};
 use std::sync::atomic::AtomicBool;
@@ -390,11 +390,11 @@ pub async fn check_mergeability(
         .client
         .get_pull_request(pull_request.pr_number)
         .await?;
-    let new_mergeable_state = fetched_pr.mergeable_state;
+    let new_mergeable_state = fetched_pr.mergeable_state.clone();
 
     // We don't know the mergeability state yet. Retry the PR after some delay
     if new_mergeable_state == OctocrabMergeableState::Unknown {
-        match fetched_pr.status {
+        match &fetched_pr.status {
             PullRequestStatus::Open | PullRequestStatus::Draft => {
                 tracing::info!("Mergeability status unknown, scheduling retry.");
                 mq_tx.enqueue_retry(mq_item);
@@ -420,7 +420,7 @@ pub async fn check_mergeability(
         .await?;
 
     if new_mergeable_state == MergeableState::HasConflicts && was_previously_mergeable {
-        handle_pr_conflict(&repo_state, &ctx.db, pull_request, conflict_source).await?;
+        handle_pr_conflict(&repo_state, &ctx.db, &fetched_pr, conflict_source).await?;
     }
 
     Ok(())
@@ -429,7 +429,7 @@ pub async fn check_mergeability(
 async fn handle_pr_conflict(
     repo_state: &RepositoryState,
     db: &PgDbClient,
-    pr: &PullRequestData,
+    pr: &PullRequest,
     conflict_source: Option<PullRequestNumber>,
 ) -> anyhow::Result<()> {
     tracing::info!("Pull request {pr:?} was likely unmergeable (source: {conflict_source:?})");
@@ -439,20 +439,23 @@ async fn handle_pr_conflict(
         return Ok(());
     }
 
-    let Some(pr) = db.get_pull_request(&pr.repo, pr.pr_number).await? else {
+    let Some(pr_db) = db
+        .get_pull_request(repo_state.repository(), pr.number)
+        .await?
+    else {
         return Err(anyhow::anyhow!("Pull request {pr:?} was not found"));
     };
 
     // Unapprove PR
-    let unapproved = if pr.is_approved() {
-        unapprove_pr(repo_state, db, &pr).await?;
+    let unapproved = if pr_db.is_approved() {
+        unapprove_pr(repo_state, db, &pr_db, pr).await?;
         true
     } else {
         false
     };
     repo_state
         .client
-        .post_comment(pr.number, conflict_comment(conflict_source, unapproved))
+        .post_comment(pr_db.number, conflict_comment(conflict_source, unapproved))
         .await?;
     Ok(())
 }