patch_sync: Conduct unconditional cleanup

After any sort of modification, we have to conduct a full cleanup
of the workspace. While it's still possible to have a panic
cause no cleanup, this is an improvement over the previous work,
where cleanup could fail during the transpose stage.

Also add a --wip mode to prevent spamming people with emails
during testing.

BUG=b:209493133
TEST=Running patch_sync transpose locally

Change-Id: I01e8a5897ec8eeed8f90c528c567b2ba55613b23
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/3407914
Reviewed-by: George Burgess <gbiv@chromium.org>
Tested-by: Jordan Abrahams-Whitehead <ajordanr@google.com>
Commit-Queue: Jordan Abrahams-Whitehead <ajordanr@google.com>

4 files changed, 120 insertions, 36 deletions

diff --git a/llvm_tools/patch_sync/Cargo.lock b/llvm_tools/patch_sync/Cargo.lock
index 63a9fcf8..62db2220 100644
--- a/llvm_tools/patch_sync/Cargo.lock
+++ b/llvm_tools/patch_sync/Cargo.lock
@@ -167,6 +167,7 @@ dependencies = [
  "anyhow",
  "rand",
  "regex",
+ "scopeguard",
  "serde",
  "serde_json",
  "sha2",
@@ -286,6 +287,12 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "73b4b750c782965c211b42f022f59af1fbceabdd026623714f104152f1ec149f"
 
 [[package]]
+name = "scopeguard"
+version = "1.1.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "d29ab0c6d3fc0ee92fe66e2d99f700eab17a8d57d1c1d3b748380fb20baa78cd"
+
+[[package]]
 name = "serde"
 version = "1.0.132"
 source = "registry+https://github.com/rust-lang/crates.io-index"
diff --git a/llvm_tools/patch_sync/Cargo.toml b/llvm_tools/patch_sync/Cargo.toml
index 43082627..ba8817d5 100644
--- a/llvm_tools/patch_sync/Cargo.toml
+++ b/llvm_tools/patch_sync/Cargo.toml
@@ -15,6 +15,7 @@ serde_json = "1.0"
 sha2 = "0.9"
 structopt = "0.3"
 time = "0.3"
+scopeguard = "1.1.0"
 
 [dev-dependencies]
 rand = "0.8"
diff --git a/llvm_tools/patch_sync/src/main.rs b/llvm_tools/patch_sync/src/main.rs
index 033691e4..a8a957f9 100644
--- a/llvm_tools/patch_sync/src/main.rs
+++ b/llvm_tools/patch_sync/src/main.rs
@@ -10,6 +10,7 @@ use anyhow::{Context, Result};
 use structopt::StructOpt;
 
 use patch_parsing::{filter_patches_by_platform, PatchCollection, PatchDictSchema};
+use version_control::RepoSetupContext;
 
 fn main() -> Result<()> {
     match Opt::from_args() {
@@ -35,6 +36,7 @@ fn main() -> Result<()> {
             verbose,
             dry_run,
             no_commit,
+            wip,
         } => transpose_subcmd(TransposeOpt {
             cros_checkout_path,
             cros_reviewers: cros_reviewers
@@ -50,6 +52,7 @@ fn main() -> Result<()> {
             verbose,
             dry_run,
             no_commit,
+            wip,
         }),
     }
 }
@@ -68,10 +71,11 @@ fn show_subcmd(args: ShowOpt) -> Result<()> {
         keep_unmerged,
         sync,
     } = args;
-    let ctx = version_control::RepoSetupContext {
+    let ctx = RepoSetupContext {
         cros_checkout: cros_checkout_path,
         android_checkout: android_checkout_path,
         sync_before: sync,
+        wip_mode: true,
     };
     ctx.setup()?;
     let make_collection = |platform: &str, patches_fp: &Path| -> Result<PatchCollection> {
@@ -109,26 +113,28 @@ struct TransposeOpt {
     no_commit: bool,
     cros_reviewers: Vec<String>,
     android_reviewers: Vec<String>,
+    wip: bool,
 }
 
 fn transpose_subcmd(args: TransposeOpt) -> Result<()> {
-    let ctx = version_control::RepoSetupContext {
+    let ctx = RepoSetupContext {
         cros_checkout: args.cros_checkout_path,
         android_checkout: args.android_checkout_path,
         sync_before: args.sync,
+        wip_mode: args.wip,
     };
     ctx.setup()?;
     let cros_patches_path = ctx.cros_patches_path();
     let android_patches_path = ctx.android_patches_path();
 
     // Get new Patches -------------------------------------------------------
-    let (mut cur_cros_collection, new_cros_patches) = patch_parsing::new_patches(
+    let (cur_cros_collection, new_cros_patches) = patch_parsing::new_patches(
         &cros_patches_path,
         &ctx.old_cros_patch_contents(&args.old_cros_ref)?,
         "chromiumos",
     )
     .context("finding new patches for chromiumos")?;
-    let (mut cur_android_collection, new_android_patches) = patch_parsing::new_patches(
+    let (cur_android_collection, new_android_patches) = patch_parsing::new_patches(
         &android_patches_path,
         &ctx.old_android_patch_contents(&args.old_android_ref)?,
         "android",
@@ -170,27 +176,59 @@ fn transpose_subcmd(args: TransposeOpt) -> Result<()> {
         return Ok(());
     }
 
+    modify_repos(
+        &ctx,
+        args.no_commit,
+        ModifyOpt {
+            new_cros_patches,
+            cur_cros_collection,
+            cros_reviewers: args.cros_reviewers,
+            new_android_patches,
+            cur_android_collection,
+            android_reviewers: args.android_reviewers,
+        },
+    )
+}
+
+struct ModifyOpt {
+    new_cros_patches: PatchCollection,
+    cur_cros_collection: PatchCollection,
+    cros_reviewers: Vec<String>,
+    new_android_patches: PatchCollection,
+    cur_android_collection: PatchCollection,
+    android_reviewers: Vec<String>,
+}
+
+fn modify_repos(ctx: &RepoSetupContext, no_commit: bool, opt: ModifyOpt) -> Result<()> {
+    // Cleanup on scope exit.
+    scopeguard::defer! {
+        ctx.cleanup();
+    }
     // Transpose Patches -----------------------------------------------------
-    if !new_cros_patches.is_empty() {
-        new_cros_patches.transpose_write(&mut cur_android_collection)?;
+    let mut cur_android_collection = opt.cur_android_collection;
+    let mut cur_cros_collection = opt.cur_cros_collection;
+    if !opt.new_cros_patches.is_empty() {
+        opt.new_cros_patches
+            .transpose_write(&mut cur_android_collection)?;
     }
-    if !new_android_patches.is_empty() {
-        new_android_patches.transpose_write(&mut cur_cros_collection)?;
+    if !opt.new_android_patches.is_empty() {
+        opt.new_android_patches
+            .transpose_write(&mut cur_cros_collection)?;
     }
 
-    if args.no_commit {
+    if no_commit {
         println!("--no-commit specified; not committing or uploading");
         return Ok(());
     }
     // Commit and upload for review ------------------------------------------
     // Note we want to check if the android patches are empty for CrOS, and
     // vice versa. This is a little counterintuitive.
-    if !new_android_patches.is_empty() {
-        ctx.cros_repo_upload(&args.cros_reviewers)
+    if !opt.new_android_patches.is_empty() {
+        ctx.cros_repo_upload(&opt.cros_reviewers)
             .context("uploading chromiumos changes")?;
     }
-    if !new_cros_patches.is_empty() {
-        ctx.android_repo_upload(&args.android_reviewers)
+    if !opt.new_cros_patches.is_empty() {
+        ctx.android_repo_upload(&opt.android_reviewers)
             .context("uploading android changes")?;
     }
     Ok(())
@@ -269,5 +307,10 @@ enum Opt {
         /// Do not commit or upload any changes made.
         #[structopt(long)]
         no_commit: bool,
+
+        /// Upload and send things for review, but mark as WIP and send no
+        /// emails.
+        #[structopt(long)]
+        wip: bool,
     },
 }
diff --git a/llvm_tools/patch_sync/src/version_control.rs b/llvm_tools/patch_sync/src/version_control.rs
index b45177f7..dc43523e 100644
--- a/llvm_tools/patch_sync/src/version_control.rs
+++ b/llvm_tools/patch_sync/src/version_control.rs
@@ -10,6 +10,7 @@ const ANDROID_LLVM_REL_PATH: &str = "toolchain/llvm_android";
 
 const CROS_MAIN_BRANCH: &str = "main";
 const ANDROID_MAIN_BRANCH: &str = "master"; // nocheck
+const WORK_BRANCH_NAME: &str = "__patch_sync_tmp";
 
 /// Context struct to keep track of both Chromium OS and Android checkouts.
 #[derive(Debug)]
@@ -18,6 +19,7 @@ pub struct RepoSetupContext {
     pub android_checkout: PathBuf,
     /// Run `repo sync` before doing any comparisons.
     pub sync_before: bool,
+    pub wip_mode: bool,
 }
 
 impl RepoSetupContext {
@@ -55,9 +57,12 @@ impl RepoSetupContext {
             extra_args.push("--re");
             extra_args.push(reviewer.as_ref());
         }
+        if self.wip_mode {
+            extra_args.push("--wip");
+            extra_args.push("--no-emails");
+        }
         Self::repo_upload(
             &self.cros_checkout,
-            CROS_MAIN_BRANCH,
             CHROMIUMOS_OVERLAY_REL_PATH,
             &Self::build_commit_msg(
                 "llvm: Synchronize patches from android",
@@ -78,9 +83,12 @@ impl RepoSetupContext {
             extra_args.push("--re");
             extra_args.push(reviewer.as_ref());
         }
+        if self.wip_mode {
+            extra_args.push("--wip");
+            extra_args.push("--no-emails");
+        }
         Self::repo_upload(
             &self.android_checkout,
-            ANDROID_MAIN_BRANCH,
             ANDROID_LLVM_REL_PATH,
             &Self::build_commit_msg(
                 "Synchronize patches from chromiumos",
@@ -92,6 +100,30 @@ impl RepoSetupContext {
         )
     }
 
+    fn cros_cleanup(&self) -> Result<()> {
+        let git_path = self.cros_checkout.join(CHROMIUMOS_OVERLAY_REL_PATH);
+        Self::cleanup_branch(&git_path, CROS_MAIN_BRANCH, WORK_BRANCH_NAME)
+            .with_context(|| format!("cleaning up branch {}", WORK_BRANCH_NAME))?;
+        Ok(())
+    }
+
+    fn android_cleanup(&self) -> Result<()> {
+        let git_path = self.android_checkout.join(ANDROID_LLVM_REL_PATH);
+        Self::cleanup_branch(&git_path, ANDROID_MAIN_BRANCH, WORK_BRANCH_NAME)
+            .with_context(|| format!("cleaning up branch {}", WORK_BRANCH_NAME))?;
+        Ok(())
+    }
+
+    /// Wrapper around cleanups to ensure both get run, even if errors appear.
+    pub fn cleanup(&self) {
+        if let Err(e) = self.cros_cleanup() {
+            eprintln!("Failed to clean up chromiumos, continuing: {}", e);
+        }
+        if let Err(e) = self.android_cleanup() {
+            eprintln!("Failed to clean up android, continuing: {}", e);
+        }
+    }
+
     /// Get the Android path to the PATCHES.json file
     pub fn android_patches_path(&self) -> PathBuf {
         self.android_checkout
@@ -125,34 +157,31 @@ impl RepoSetupContext {
     }
 
     fn repo_upload<'a, I: IntoIterator<Item = &'a str>>(
-        path: &Path,
-        base_branch: &str,
-        git_wd: &'a str,
+        checkout_path: &Path,
+        subproject_git_wd: &'a str,
         commit_msg: &str,
         extra_flags: I,
     ) -> Result<()> {
-        let git_path = &path.join(&git_wd);
+        let git_path = &checkout_path.join(&subproject_git_wd);
         ensure!(
             git_path.is_dir(),
             "git_path {} is not a directory",
             git_path.display()
         );
-        let branch_name = "__patch_sync_tmp_branch";
-        repo_cd_cmd(path, &["start", branch_name, git_wd])?;
-        let res: Result<()> = {
-            let base_args = ["upload", "--br", branch_name, "-y", "--verify"];
-            let new_args = base_args
-                .iter()
-                .copied()
-                .chain(extra_flags)
-                .chain(["--", git_wd]);
-            git_cd_cmd(git_path, &["add", "."])
-                .and_then(|_| git_cd_cmd(git_path, &["commit", "-m", commit_msg]))
-                .and_then(|_| repo_cd_cmd(path, new_args))
-        };
-        Self::cleanup_branch(git_path, base_branch, branch_name)
-            .with_context(|| format!("cleaning up branch {}", branch_name))?;
-        res
+        repo_cd_cmd(
+            checkout_path,
+            &["start", WORK_BRANCH_NAME, subproject_git_wd],
+        )?;
+        let base_args = ["upload", "--br", WORK_BRANCH_NAME, "-y", "--verify"];
+        let new_args = base_args
+            .iter()
+            .copied()
+            .chain(extra_flags)
+            .chain(["--", subproject_git_wd]);
+        git_cd_cmd(git_path, &["add", "."])
+            .and_then(|_| git_cd_cmd(git_path, &["commit", "-m", commit_msg]))
+            .and_then(|_| repo_cd_cmd(checkout_path, new_args))?;
+        Ok(())
     }
 
     /// Clean up the git repo after we're done with it.
@@ -160,7 +189,11 @@ impl RepoSetupContext {
         git_cd_cmd(git_path, ["restore", "."])?;
         git_cd_cmd(git_path, ["clean", "-fd"])?;
         git_cd_cmd(git_path, ["checkout", base_branch])?;
-        git_cd_cmd(git_path, ["branch", "-D", rm_branch])?;
+        // It's acceptable to be able to not delete the branch. This may be
+        // because the branch does not exist, which is an expected result.
+        // Since this is a very common case, we won't report any failures related
+        // to this command failure as it'll pollute the stderr logs.
+        let _ = git_cd_cmd(git_path, ["branch", "-D", rm_branch]);
         Ok(())
     }