[RFC] Report apply_patch as a warning, not error, if matching commit found
diff mbox series

Message ID 20190606103912.26840-1-ajd@linux.ibm.com
State New
Delegated to: Russell Currey
Headers show
Series
  • [RFC] Report apply_patch as a warning, not error, if matching commit found
Related show

Commit Message

Andrew Donnellan June 6, 2019, 10:39 a.m. UTC
If a patch can't be applied, check to see if there's a commit already in
the repository that matches the title of the patch. If so, don't report
the fact that it failed to apply as an error, only a warning.

Closes: #24 ("Patch application can fail if snowpatch tries to apply patch after maintainer has merged")
Signed-off-by: Andrew Donnellan <ajd@linux.ibm.com>

---

like everything else i've done today, completely untested

---
 src/git.rs  | 15 +++++++++++++++
 src/main.rs | 49 +++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 56 insertions(+), 8 deletions(-)

Patch
diff mbox series

diff --git a/src/git.rs b/src/git.rs
index ad4bb781d23b..be744fdf1934 100644
--- a/src/git.rs
+++ b/src/git.rs
@@ -155,3 +155,18 @@  pub fn cred_from_settings(settings: &Git) -> Result<Cred, Error> {
         passphrase,
     )
 }
+
+pub fn find_commit_with_title(repo: &Repository, title: &str) -> bool {
+    let workdir = repo.workdir().unwrap();
+
+    // TODO: Perhaps limit this to a certain number of commits, etc
+    let result = Command::new("git")
+        .arg("log")
+        .arg("--oneline")
+        .arg("--grep")
+        .arg(title)
+        .current_dir(&workdir)
+        .output()
+        .expect("Couldn't run git");
+    !result.stdout.is_empty()
+}
diff --git a/src/main.rs b/src/main.rs
index 5ec0c37b2d7f..bbc773597cc5 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -191,6 +191,7 @@  fn test_patch(
     client: &Arc<Client>,
     project: &Project,
     path: &Path,
+    title: Option<&str>,
     hefty_tests: bool,
 ) -> Vec<TestResult> {
     let repo = project.get_repo().unwrap();
@@ -208,6 +209,8 @@  fn test_patch(
     push_opts.remote_callbacks(push_callbacks);
 
     let mut successfully_applied = false;
+    let mut possibly_applied = false;
+
     for branch_name in project.branches.clone() {
         let tag = format!("{}_{}", tag, branch_name);
         info!("Configuring local branch for {}.", tag);
@@ -261,19 +264,35 @@  fn test_patch(
             }
             Err(_) => {
                 // It didn't apply.  No need to bother testing.
+
+                // Check if there's a patch with an identical title already in the repository
+                let mut possibly_applied_to_branch = false;
+                if let Some(title) = title {
+                    possibly_applied_to_branch = git::find_commit_with_title(&repo, title);
+                }
+
                 results.push(TestResult {
                     state: TestState::Warning,
-                    description: Some(
+                    description: Some(if possibly_applied_to_branch {
+                        format!(
+                            "Failed to apply on branch {} ({}) (possibly already applied)",
+                            branch_name.to_string(),
+                            commit.id().to_string()
+                        )
+                        .to_string()
+                    } else {
                         format!(
                             "Failed to apply on branch {} ({})",
                             branch_name.to_string(),
                             commit.id().to_string()
                         )
-                        .to_string(),
-                    ),
+                        .to_string()
+                    }),
                     context: Some("apply_patch".to_string()),
                     ..Default::default()
                 });
+
+                possibly_applied = possibly_applied || possibly_applied_to_branch;
                 continue;
             }
         }
@@ -310,8 +329,13 @@  fn test_patch(
     }
 
     if !successfully_applied {
+        // If at least one branch found a commit with the same title, then use Warning rather than Fail
         results.push(TestResult {
-            state: TestState::Fail,
+            state: if possibly_applied {
+                TestState::Warning
+            } else {
+                TestState::Fail
+            },
             description: Some("Failed to apply to any branch".to_string()),
             context: Some("apply_patch".to_string()),
             ..Default::default()
@@ -398,7 +422,7 @@  fn run() -> Result<(), Box<Error>> {
                 } else {
                     patchwork.get_patch_mbox(&patch)
                 };
-                test_patch(&settings, &client, &project, &mbox, true);
+                test_patch(&settings, &client, &project, &mbox, Some(&patch.name), true);
             }
         }
         return Ok(());
@@ -416,7 +440,8 @@  fn run() -> Result<(), Box<Error>> {
             Some(project) => {
                 let dependencies = patchwork.get_patch_dependencies(&patch);
                 let mbox = patchwork.get_patches_mbox(dependencies);
-                let results = test_patch(&settings, &client, &project, &mbox, true);
+                let results =
+                    test_patch(&settings, &client, &project, &mbox, Some(&patch.name), true);
 
                 // Delete the temporary directory with the patch in it
                 fs::remove_dir_all(mbox.parent().unwrap())
@@ -437,7 +462,8 @@  fn run() -> Result<(), Box<Error>> {
     if args.flag_mbox != "" {
         info!("snowpatch is testing a local patch.");
         let patch = Path::new(&args.flag_mbox);
-        test_patch(&settings, &client, &project, patch, true);
+        // TODO: Can't match patch title
+        test_patch(&settings, &client, &project, patch, None, true);
 
         return Ok(());
     }
@@ -517,7 +543,14 @@  fn run() -> Result<(), Box<Error>> {
                 patchwork.get_patch_mbox(&patch)
             };
 
-            let results = test_patch(&settings, &client, &project, &mbox, hefty_tests);
+            let results = test_patch(
+                &settings,
+                &client,
+                &project,
+                &mbox,
+                Some(&patch.name),
+                hefty_tests,
+            );
 
             // Delete the temporary directory with the patch in it
             fs::remove_dir_all(mbox.parent().unwrap())