Repository: MarcoIeni/release-plz
Pull request #470 (2023-01-18 20:58:53)

feat: edit pr
<!-- Please explain the changes you made -->

<!--
Please, make sure:
- you have read the contributing guidelines:
  https://github.com/MarcoIeni/release-plz/blob/main/CONTRIBUTING.md
- you have formatted the code using rustfmt:
  https://github.com/rust-lang/rustfmt
- you have checked that all tests pass, by running `cargo test --all`
-->
Solves #280 for github. Gitea is todo


Patch:
diff --git a/Cargo.lock b/Cargo.lock
index dec467f0..61da4c27 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -572,7 +572,7 @@ dependencies = [
  "nom",
  "pathdiff",
  "serde",
- "toml",
+ "toml 0.5.11",
  "yaml-rust",
 ]
 
@@ -613,9 +613,9 @@ dependencies = [
 
 [[package]]
 name = "crates-index"
-version = "0.18.12"
+version = "0.19.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "b655ece026c6f8088979ec131e922243801f0b52d2e8ecae805f56519511e69c"
+checksum = "75096e85e0ce07c430da88f21b404734f81c873d107177004f2ca97fc4baa267"
 dependencies = [
  "git2",
  "hex 0.4.3",
@@ -629,7 +629,7 @@ dependencies = [
  "serde_derive",
  "serde_json",
  "smartstring",
- "toml",
+ "toml 0.6.0",
 ]
 
 [[package]]
@@ -1255,7 +1255,7 @@ dependencies = [
  "serde_regex",
  "tera",
  "thiserror",
- "toml",
+ "toml 0.5.11",
 ]
 
 [[package]]
@@ -2819,6 +2819,15 @@ dependencies = [
  "serde",
 ]
 
+[[package]]
+name = "serde_spanned"
+version = "0.6.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "2c68e921cef53841b8925c2abadd27c9b891d9613bdc43d6b823062866df38e8"
+dependencies = [
+ "serde",
+]
+
 [[package]]
 name = "serde_urlencoded"
 version = "0.7.1"
@@ -3183,6 +3192,18 @@ dependencies = [
  "serde",
 ]
 
+[[package]]
+name = "toml"
+version = "0.6.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "4fb9d890e4dc9298b70f740f615f2e05b9db37dce531f6b24fb77ac993f9f217"
+dependencies = [
+ "serde",
+ "serde_spanned",
+ "toml_datetime",
+ "toml_edit 0.18.0",
+]
+
 [[package]]
 name = "toml_datetime"
 version = "0.5.1"
@@ -3220,6 +3241,19 @@ dependencies = [
  "toml_datetime",
 ]
 
+[[package]]
+name = "toml_edit"
+version = "0.18.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "729bfd096e40da9c001f778f5cdecbd2957929a24e10e5883d9392220a751581"
+dependencies = [
+ "indexmap",
+ "nom8",
+ "serde",
+ "serde_spanned",
+ "toml_datetime",
+]
+
 [[package]]
 name = "tower-service"
 version = "0.3.2"
diff --git a/Cargo.toml b/Cargo.toml
index 4a95a59f..edf28c81 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -9,7 +9,7 @@ chrono = { version = "0.4.23", default-features = false }
 clap = "4.1.1"
 clap_complete = "4.1.0"
 conventional_commit_parser = "0.9.4"
-crates-index = "0.18.12"
+crates-index = "0.19.0"
 dirs = "4.0.0"
 dunce = "1.0.3"
 expect-test = "1.4.0"
diff --git a/crates/git_cmd/src/lib.rs b/crates/git_cmd/src/lib.rs
index 3906adc9..ba05433f 100644
--- a/crates/git_cmd/src/lib.rs
+++ b/crates/git_cmd/src/lib.rs
@@ -92,6 +92,11 @@ impl Repo {
         Ok(())
     }
 
+    pub fn force_push(&self, obj: &str) -> anyhow::Result<()> {
+        self.git(&["push", "origin", obj, "--force"])?;
+        Ok(())
+    }
+
     pub fn checkout_head(&self) -> anyhow::Result<()> {
         self.git(&["checkout", &self.default_branch])?;
         Ok(())
@@ -130,7 +135,7 @@ impl Repo {
     }
 
     /// Run a git command in the repository git directory
-    fn git(&self, args: &[&str]) -> anyhow::Result<String> {
+    pub fn git(&self, args: &[&str]) -> anyhow::Result<String> {
         git_in_dir(&self.directory, args)
     }
 
@@ -156,7 +161,7 @@ impl Repo {
     }
 
     #[instrument(skip(self))]
-    fn checkout(&self, object: &str) -> anyhow::Result<()> {
+    pub fn checkout(&self, object: &str) -> anyhow::Result<()> {
         self.git(&["checkout", object])?;
         Ok(())
     }
@@ -229,7 +234,7 @@ pub fn git_in_dir(dir: &Path, args: &[&str]) -> anyhow::Result<String> {
         .with_context(|| {
             format!("error while running git in directory `{dir:?}` with args `{args:?}`")
         })?;
-    trace!("git output = {:?}", output);
+    trace!("git {:?}: output = {:?}", args, output);
     let stdout = cmd::string_from_bytes(output.stdout)?;
     if output.status.success() {
         Ok(stdout)
diff --git a/crates/release_plz_core/src/github_client.rs b/crates/release_plz_core/src/github_client.rs
index e0cb7dc3..cc7f45be 100644
--- a/crates/release_plz_core/src/github_client.rs
+++ b/crates/release_plz_core/src/github_client.rs
@@ -38,16 +38,23 @@ pub struct CreateReleaseOption<'a> {
 }
 
 #[derive(Deserialize)]
-struct GitHubPr {
-    number: u64,
-    html_url: Url,
-    head: Commit,
+pub struct GitHubPr {
+    pub number: u64,
+    pub html_url: Url,
+    pub head: Commit,
+}
+
+impl GitHubPr {
+    pub fn branch(&self) -> &str {
+        self.head.ref_field.as_str()
+    }
 }
 
 #[derive(Deserialize)]
-struct Commit {
+pub struct Commit {
     #[serde(rename = "ref")]
     pub ref_field: String,
+    pub sha: String,
 }
 
 impl<'a> GitHubClient<'a> {
@@ -99,10 +106,11 @@ impl<'a> GitHubClient<'a> {
         )
     }
 
-    /// Close all Prs which branch starts with the given `branch_prefix`.
-    pub async fn close_prs_on_branches(&self, branch_prefix: &str) -> anyhow::Result<()> {
+    /// Get all opened Prs which branch starts with the given `branch_prefix`.
+    pub async fn opened_prs(&self, branch_prefix: &str) -> anyhow::Result<Vec<GitHubPr>> {
         let mut page = 1;
         let page_size = 30;
+        let mut release_prs: Vec<GitHubPr> = vec![];
         loop {
             let prs: Vec<GitHubPr> = self
                 .client
@@ -117,21 +125,30 @@ impl<'a> GitHubClient<'a> {
                 .json()
                 .await
                 .context("failed to parse pr")?;
-            for pr in &prs {
-                if pr.head.ref_field.starts_with(branch_prefix) {
-                    debug!("closing pr {}", pr.number);
-                    self.close_pr(pr.number).await?;
-                }
-            }
-            if prs.len() < page_size as usize {
+            let prs_len = prs.len();
+            let current_release_prs: Vec<GitHubPr> = prs
+                .into_iter()
+                .filter(|pr| pr.head.ref_field.starts_with(branch_prefix))
+                .collect();
+            release_prs.extend(current_release_prs);
+            if prs_len < page_size {
                 break;
             }
             page += 1;
         }
+        Ok(release_prs)
+    }
+
+    /// Close all Prs which branch starts with the given `branch_prefix`.
+    pub async fn close_prs_on_branches(&self, branch_prefix: &str) -> anyhow::Result<()> {
+        for pr in self.opened_prs(branch_prefix).await? {
+            debug!("closing pr {}", pr.number);
+            self.close_pr(pr.number).await?;
+        }
         Ok(())
     }
 
-    async fn close_pr(&self, pr_number: u64) -> anyhow::Result<()> {
+    pub async fn close_pr(&self, pr_number: u64) -> anyhow::Result<()> {
         self.client
             .patch(format!("{}/{}", self.pulls_url(), pr_number))
             .json(&json!({
@@ -164,6 +181,46 @@ impl<'a> GitHubClient<'a> {
 
         Ok(pr.html_url)
     }
+
+    pub async fn pr_commits(&self, pr_number: u64) -> anyhow::Result<Vec<PrCommit>> {
+        self.client
+            .get(format!("{}/{}/commits", self.pulls_url(), pr_number))
+            .send()
+            .await?
+            .error_for_status()?
+            .json()
+            .await
+            .context("can't parse commits")
+    }
+}
+
+/// Returns the list of contributors for the given commits,
+/// excluding the PR author and bots.
+pub fn contributors_from_commits(commits: &[PrCommit]) -> Vec<String> {
+    let mut contributors = commits
+        .iter()
+        .skip(1) // skip pr author
+        .flat_map(|commit| &commit.author)
+        .filter(|author| !author.login.ends_with("[bot]")) // ignore bots
+        .map(|author| author.login.clone())
+        .collect::<Vec<_>>();
+    contributors.dedup();
+    contributors
+}
+
+#[derive(Deserialize)]
+pub struct PrCommit {
+    pub author: Option<Author>,
+}
+
+#[derive(Deserialize)]
+pub struct CommitParent {
+    pub sha: String,
+}
+
+#[derive(Deserialize)]
+pub struct Author {
+    login: String,
 }
 
 #[derive(Debug)]
@@ -191,3 +248,32 @@ impl GitHub {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn contributors_are_extracted_from_commits() {
+        let commits = vec![
+            PrCommit {
+                author: Some(Author {
+                    login: "bob".to_string(),
+                }),
+            },
+            PrCommit {
+                author: Some(Author {
+                    login: "marco".to_string(),
+                }),
+            },
+            PrCommit {
+                author: Some(Author {
+                    login: "release[bot]".to_string(),
+                }),
+            },
+            PrCommit { author: None },
+        ];
+        let contributors = contributors_from_commits(&commits);
+        assert_eq!(contributors, vec!["marco"]);
+    }
+}
diff --git a/crates/release_plz_core/src/release_pr.rs b/crates/release_plz_core/src/release_pr.rs
index c94ea1fe..6fc1d808 100644
--- a/crates/release_plz_core/src/release_pr.rs
+++ b/crates/release_plz_core/src/release_pr.rs
@@ -1,14 +1,17 @@
-use std::path::PathBuf;
+use std::path::{Path, PathBuf};
 
+use cargo_metadata::Package;
 use git_cmd::Repo;
 
 use anyhow::{anyhow, Context};
-use tracing::instrument;
+use tracing::{info, instrument};
 
 use crate::backend::GitClient;
+use crate::github_client::{contributors_from_commits, GitHubPr};
 use crate::pr::{Pr, BRANCH_PREFIX};
 use crate::{
-    copy_to_temp_dir, publishable_packages, update, GitBackend, UpdateRequest, CARGO_TOML,
+    copy_to_temp_dir, publishable_packages, update, GitBackend, UpdateRequest, UpdateResult,
+    CARGO_TOML,
 };
 
 #[derive(Debug)]
@@ -35,27 +38,144 @@ pub async fn release_pr(input: &ReleasePrRequest) -> anyhow::Result<()> {
         .set_local_manifest(&local_manifest)
         .context("can't find temporary project")?;
     let (packages_to_update, _temp_repository) = update(&new_update_request)?;
-    let gh_client = GitClient::new(&input.git)?;
-    gh_client
-        .close_prs_on_branches(BRANCH_PREFIX)
-        .await
-        .context("cannot close old release-plz prs")?;
+    let git_client = GitClient::new(&input.git)?;
     if !packages_to_update.is_empty() {
         let repo = Repo::new(new_manifest_dir)?;
         let there_are_commits_to_push = repo.is_clean().is_err();
         if there_are_commits_to_push {
-            let project_contains_multiple_pub_packages =
-                publishable_packages(local_manifest)?.len() > 1;
-            let pr = Pr::new(
-                repo.default_branch(),
-                packages_to_update.as_ref(),
-                project_contains_multiple_pub_packages,
-            );
-            create_release_branch(&repo, &pr.branch)?;
-            gh_client.open_pr(&pr).await?;
+            open_or_update_release_pr(&local_manifest, &packages_to_update, &git_client, &repo)
+                .await?;
+        }
+    }
+
+    Ok(())
+}
+
+async fn open_or_update_release_pr<'a>(
+    local_manifest: &Path,
+    packages_to_update: &[(Package, UpdateResult)],
+    git_client: &GitClient<'a>,
+    repo: &Repo,
+) -> anyhow::Result<()> {
+    match &git_client {
+        GitClient::GitHub(gh_client) => {
+            let opened_release_prs = gh_client
+                .opened_prs(BRANCH_PREFIX)
+                .await
+                .context("cannot get opened release-plz prs")?;
+            // Close all release-plz prs, except one.
+            let old_release_prs = opened_release_prs.iter().skip(1);
+            for pr in old_release_prs {
+                gh_client
+                    .close_pr(pr.number)
+                    .await
+                    .context("cannot close old release-plz prs")?;
+            }
+
+            match opened_release_prs.first() {
+                Some(pr) => {
+                    let pr_commits = gh_client
+                        .pr_commits(pr.number)
+                        .await
+                        .context("cannot get commits of release-plz pr")?;
+                    let pr_contributors = contributors_from_commits(&pr_commits);
+                    if pr_contributors.is_empty() {
+                        // There are no contributors, so we can force-push
+                        // in this PR, because we don't care about the git history.
+                        let update_outcome = update_pr(pr, pr_commits.len(), repo);
+                        if let Err(e) = update_outcome {
+                            tracing::error!("cannot update release pr {}: {}. I'm closing it and opening a new one", pr.number, e);
+                            gh_client
+                                .close_pr(pr.number)
+                                .await
+                                .context("cannot close old release-plz prs")?;
+                            create_pr(git_client, repo, packages_to_update, local_manifest).await?
+                        }
+                    } else {
+                        // There's a contributor, so we don't want to force-push in this PR.
+                        // We close it because we want to save the contributor's work.
+                        // TODO improvement: check how many lines the commit added, if no lines (for example a merge to update the branch),
+                        //      then don't count it as a contributor.
+                        info!("closing pr {} to preserve git history", pr.html_url);
+                        gh_client
+                            .close_pr(pr.number)
+                            .await
+                            .context("cannot close old release-plz prs")?;
+                        create_pr(git_client, repo, packages_to_update, local_manifest).await?
+                    }
+                }
+                None => create_pr(git_client, repo, packages_to_update, local_manifest).await?,
+            }
+        }
+        GitClient::Gitea(_) => {
+            close_old_prs(git_client).await?;
+            create_pr(git_client, repo, packages_to_update, local_manifest).await?;
         }
     }
+    Ok(())
+}
 
+async fn close_old_prs(git_client: &GitClient<'_>) -> anyhow::Result<()> {
+    git_client
+        .close_prs_on_branches(BRANCH_PREFIX)
+        .await
+        .context("cannot close old release-plz prs")?;
+    Ok(())
+}
+
+async fn create_pr(
+    git_client: &GitClient<'_>,
+    repo: &Repo,
+    packages_to_update: &[(Package, UpdateResult)],
+    local_manifest: &Path,
+) -> anyhow::Result<()> {
+    let project_contains_multiple_pub_packages = publishable_packages(local_manifest)?.len() > 1;
+    let pr = Pr::new(
+        repo.default_branch(),
+        packages_to_update,
+        project_contains_multiple_pub_packages,
+    );
+    create_release_branch(repo, &pr.branch)?;
+    git_client.open_pr(&pr).await?;
+    Ok(())
+}
+
+fn update_pr(pr: &GitHubPr, commits_number: usize, repository: &Repo) -> anyhow::Result<()> {
+    // save local work
+    repository.git(&["stash"])?;
+
+    reset_branch(pr, commits_number, repository).map_err(|e| {
+        // restore local work
+        if let Err(e) = repository.git(&["stash", "pop"]) {
+            tracing::error!("cannot restore local work: {}", e);
+        }
+        e
+    })?;
+    repository.git(&["stash", "pop"])?;
+    force_push(pr, repository)?;
+    info!("updated pr {}", pr.html_url);
+    Ok(())
+}
+
+fn reset_branch(pr: &GitHubPr, commits_number: usize, repository: &Repo) -> anyhow::Result<()> {
+    // sanity check to avoid doing bad things on non-release-plz branches
+    anyhow::ensure!(pr.branch().starts_with(BRANCH_PREFIX), "wrong branch name");
+
+    if repository.checkout(pr.branch()).is_err() {
+        repository.git(&["pull"])?;
+    };
+    repository.checkout(pr.branch())?;
+
+    let head = format!("HEAD~{}", commits_number);
+    repository.git(&["reset", "--hard", &head])?;
+    Ok(())
+}
+
+fn force_push(pr: &GitHubPr, repository: &Repo) -> anyhow::Result<()> {
+    let changes_expect_typechanges = repository.changes_except_typechanges()?;
+    repository.add(&changes_expect_typechanges)?;
+    repository.commit("chore: release")?;
+    repository.force_push(pr.branch())?;
     Ok(())
 }
 


Test patch:
diff --git a/crates/release_plz_core/tests/all/helpers/gitea_mock_server.rs b/crates/release_plz_core/tests/all/helpers/gitea_mock_server.rs
index 303a3416..ce386688 100644
--- a/crates/release_plz_core/tests/all/helpers/gitea_mock_server.rs
+++ b/crates/release_plz_core/tests/all/helpers/gitea_mock_server.rs
@@ -34,7 +34,7 @@ impl GiteaMockServer {
     }
 
     /// Return an empty list of PRs.
-    pub async fn no_prs(&self) {
+    pub async fn _no_prs(&self) {
         let no_prs = {
             let empty_body = serde_json::Value::Array(vec![]);
             ResponseTemplate::new(200).set_body_json(empty_body)
diff --git a/crates/release_plz_core/tests/all/helpers/github_mock_server.rs b/crates/release_plz_core/tests/all/helpers/github_mock_server.rs
index 646b965b..d8554759 100644
--- a/crates/release_plz_core/tests/all/helpers/github_mock_server.rs
+++ b/crates/release_plz_core/tests/all/helpers/github_mock_server.rs
@@ -34,7 +34,7 @@ impl GitHubMockServer {
     }
 
     /// Return an empty list of PRs.
-    pub async fn no_prs(&self) {
+    pub async fn _no_prs(&self) {
         let no_prs = {
             let empty_body = serde_json::Value::Array(vec![]);
             ResponseTemplate::new(200).set_body_json(empty_body)
diff --git a/crates/release_plz_core/tests/all/release_pr.rs b/crates/release_plz_core/tests/all/release_pr.rs
index 042fee8a..11e1b92a 100644
--- a/crates/release_plz_core/tests/all/release_pr.rs
+++ b/crates/release_plz_core/tests/all/release_pr.rs
@@ -7,7 +7,6 @@ async fn github_up_to_date_project_should_not_raise_pr() {
         .github_mock_server()
         .expect_no_created_prs()
         .await;
-    comparison_test.github_mock_server().no_prs().await;
     comparison_test.github_open_release_pr().await.unwrap();
 }
 
@@ -18,6 +17,5 @@ async fn gitea_up_to_date_project_should_not_raise_pr() {
         .gitea_mock_server()
         .expect_no_created_prs()
         .await;
-    comparison_test.gitea_mock_server().no_prs().await;
     comparison_test.gitea_open_release_pr().await.unwrap();
 }