feat(submit:phabricator): do not abort entire process on failure

Currently, `git submit` for Phabricator will abort the entire operation if any commit fails to be submitted. This means that if `arc diff` succeeds on one commit and then fails on its child, the entire operation is aborted. However, the first `arc diff` had side effects, so the user gets diffs uploaded to Phabricator that are not reflected locally. Instead, we should confirm any passing commits and abort after we get a failing commit. This commit updates the Phabricator forge to handle the error case better and not produce garbage commits on Phabricator.

Author: arxanas

git-branchless-lib/src/core/eventlog.rs

@@ -580,7 +580,6 @@ INSERT INTO event_log VALUES (
     ///
     /// Returns: All the events in the database, ordered from oldest to newest.
     #[instrument]
-
     pub fn get_events(&self) -> eyre::Result<Vec<Event>> {
         let mut stmt = self.conn.prepare(
             "

git-branchless-submit/src/branch_forge.rs

@@ -247,7 +247,7 @@ These remotes are available: {}",
                     commit_status.local_branch_name.map(|local_branch_name| {
                         (
                             commit_oid,
-                            CreateStatus {
+                            CreateStatus::Created {
                                 final_commit_oid: commit_oid,
                                 local_branch_name,
                             },

git-branchless-submit/src/lib.rs

@@ -109,15 +109,23 @@ pub struct SubmitOptions {
 
 /// The result of creating a commit.
 #[derive(Clone, Debug)]
-pub struct CreateStatus {
-    /// The commit OID after carrying out the creation process. Usually, this
-    /// will be the same as the original commit OID, unless the forge amends it
-    /// (e.g. to include a change ID).
-    pub final_commit_oid: NonZeroOid,
-
-    /// The local branch name to use. The caller will try to create the branch
-    /// pointing to that commit (assuming that it doesn't already exist).
-    pub local_branch_name: String,
+pub enum CreateStatus {
+    Created {
+        /// The commit OID after carrying out the creation process. Usually, this
+        /// will be the same as the original commit OID, unless the forge amends it
+        /// (e.g. to include a change ID).
+        final_commit_oid: NonZeroOid,
+
+        /// The local branch name to use. The caller will try to create the branch
+        /// pointing to that commit (assuming that it doesn't already exist).
+        local_branch_name: String,
+    },
+    Skipped {
+        commit_oid: NonZeroOid,
+    },
+    Err {
+        commit_oid: NonZeroOid,
+    },
 }
 
 /// "Forge" refers to a Git hosting provider, such as GitHub, GitLab, etc.
@@ -325,15 +333,19 @@ fn submit(
         (unsubmitted, to_update, to_skip)
     });
 
-    let (created_branches, uncreated_branches): (BTreeSet<String>, BTreeSet<String>) = {
+    let (created_branches, uncreated_branches, create_error_commits): (
+        BTreeSet<String>,
+        BTreeSet<String>,
+        BTreeSet<NonZeroOid>,
+    ) = {
         let unsubmitted_branches = unsubmitted_commits
             .values()
             .flat_map(|commit_status| commit_status.local_branch_name.clone())
             .collect();
         if unsubmitted_commits.is_empty() {
             Default::default()
         } else if create && dry_run {
-            (unsubmitted_branches, Default::default())
+            (unsubmitted_branches, Default::default(), Default::default())
         } else if create {
             let create_statuses =
                 try_exit_code!(forge.create(unsubmitted_commits, &submit_options)?);
@@ -343,26 +355,37 @@ fn submit(
                 .flatten()
                 .collect();
             let mut created_branches = BTreeSet::new();
+            let mut error_commits = BTreeSet::new();
             for (_commit_oid, create_status) in create_statuses {
-                let CreateStatus {
-                    final_commit_oid,
-                    local_branch_name,
-                } = create_status;
-                let branch_reference_name =
-                    ReferenceName::from(format!("refs/heads/{local_branch_name}"));
-                created_branches.insert(local_branch_name);
-                if !all_branches.contains(&branch_reference_name) {
-                    repo.create_reference(
-                        &branch_reference_name,
+                match create_status {
+                    CreateStatus::Created {
                         final_commit_oid,
-                        false,
-                        "submit",
-                    )?;
+                        local_branch_name,
+                    } => {
+                        let branch_reference_name =
+                            ReferenceName::from(format!("refs/heads/{local_branch_name}"));
+                        created_branches.insert(local_branch_name);
+                        if !all_branches.contains(&branch_reference_name) {
+                            repo.create_reference(
+                                &branch_reference_name,
+                                final_commit_oid,
+                                false,
+                                "submit",
+                            )?;
+                        }
+                    }
+                    // For now, treat `Skipped` the same as `Err` as it would be
+                    // a lot of work to render it differently in the output, and
+                    // we may want to rethink the data structures before doing
+                    // that.
+                    CreateStatus::Skipped { commit_oid } | CreateStatus::Err { commit_oid } => {
+                        error_commits.insert(commit_oid);
+                    }
                 }
             }
-            (created_branches, Default::default())
+            (created_branches, Default::default(), error_commits)
         } else {
-            (Default::default(), unsubmitted_branches)
+            (Default::default(), unsubmitted_branches, Default::default())
         }
     };
 
@@ -482,8 +505,33 @@ create and push them, retry this operation with the --create option.",
             if dry_run { "are" } else { "were" },
         )?;
     }
+    if !create_error_commits.is_empty() {
+        writeln!(
+            effects.get_output_stream(),
+            "Failed to create {}:",
+            Pluralize {
+                determiner: None,
+                amount: create_error_commits.len(),
+                unit: ("commit", "commits")
+            },
+        )?;
+        for error_commit_oid in &create_error_commits {
+            let error_commit = repo.find_commit_or_fail(*error_commit_oid)?;
+            writeln!(
+                effects.get_output_stream(),
+                "{}",
+                effects
+                    .get_glyphs()
+                    .render(error_commit.friendly_describe(effects.get_glyphs())?)?
+            )?;
+        }
+    }
 
-    Ok(Ok(()))
+    if !create_error_commits.is_empty() {
+        Ok(Err(ExitCode(1)))
+    } else {
+        Ok(Ok(()))
+    }
 }
 
 #[instrument]

git-branchless-submit/src/phabricator.rs

@@ -10,7 +10,7 @@ use std::time::SystemTime;
 
 use cursive_core::theme::Effect;
 use cursive_core::utils::markup::StyledString;
-use git_branchless_opts::Revset;
+use git_branchless_opts::{Revset, TestSearchStrategy};
 use git_branchless_test::{
     run_tests, FixInfo, ResolvedTestOptions, TestOutput, TestResults, TestStatus,
     TestingAbortedError, Verbosity,
@@ -373,7 +373,7 @@ impl Forge for PhabricatorForge<'_> {
             TestCommand::Args(args.into_iter().map(ToString::to_string).collect())
         } else {
             TestCommand::String(
-                r#"git commit --amend --message "$(git show --no-patch --format=%B HEAD)
+                r#"(git show | grep 'BROKEN') && exit 1 || git commit --amend --message "$(git show --no-patch --format=%B HEAD)
 
 Differential Revision: https://phabricator.example.com/D000$(git rev-list --count HEAD)
             "
@@ -394,7 +394,7 @@ Differential Revision: https://phabricator.example.com/D000$(git rev-list --coun
             &ResolvedTestOptions {
                 command,
                 execution_strategy: *execution_strategy,
-                search_strategy: None,
+                search_strategy: Some(TestSearchStrategy::Linear),
                 is_dry_run: false,
                 use_cache: false,
                 is_interactive: false,
@@ -429,10 +429,23 @@ Differential Revision: https://phabricator.example.com/D000$(git rev-list --coun
             return Ok(Err(ExitCode(1)));
         }
 
-        let rebase_plan = {
+        enum ErrorReason {
+            NotTested,
+            TestFailed,
+        }
+        let (maybe_rebase_plan, error_commits) = {
             let mut builder = RebasePlanBuilder::new(self.dag, permissions);
-            for (commit_oid, test_output) in test_outputs {
-                let head_commit_oid = match test_output.test_status {
+            let mut error_commits = HashMap::new();
+            for commit_oid in commit_oids.iter().copied() {
+                let test_output = match test_outputs.get(&commit_oid) {
+                    Some(test_output) => test_output,
+                    None => {
+                        // Testing was aborted due to a previous commit failing.
+                        error_commits.insert(commit_oid, ErrorReason::NotTested);
+                        continue;
+                    }
+                };
+                match test_output.test_status {
                     TestStatus::CheckoutFailed
                     | TestStatus::SpawnTestFailed(_)
                     | TestStatus::TerminatedBySignal
@@ -442,7 +455,7 @@ Differential Revision: https://phabricator.example.com/D000$(git rev-list --coun
                     | TestStatus::Abort { .. }
                     | TestStatus::Failed { .. } => {
                         self.render_failed_test(commit_oid, &test_output)?;
-                        return Ok(Err(ExitCode(1)));
+                        error_commits.insert(commit_oid, ErrorReason::TestFailed);
                     }
                     TestStatus::Passed {
                         cached: _,
@@ -452,66 +465,86 @@ Differential Revision: https://phabricator.example.com/D000$(git rev-list --coun
                                 snapshot_tree_oid: _,
                             },
                         interactive: _,
-                    } => head_commit_oid,
-                };
-
-                let commit = self.repo.find_commit_or_fail(commit_oid)?;
-                builder.move_subtree(commit.get_oid(), commit.get_parent_oids())?;
-                builder.replace_commit(commit.get_oid(), head_commit_oid.unwrap_or(commit_oid))?;
+                    } => {
+                        let commit = self.repo.find_commit_or_fail(commit_oid)?;
+                        builder.move_subtree(commit.get_oid(), commit.get_parent_oids())?;
+                        builder.replace_commit(
+                            commit.get_oid(),
+                            head_commit_oid.unwrap_or(commit_oid),
+                        )?;
+                    }
+                }
             }
 
             let pool = ThreadPoolBuilder::new().build()?;
             let repo_pool = RepoResource::new_pool(self.repo)?;
-            match builder.build(self.effects, &pool, &repo_pool)? {
-                Ok(Some(rebase_plan)) => rebase_plan,
-                Ok(None) => return Ok(Ok(Default::default())),
+            let maybe_rebase_plan = match builder.build(self.effects, &pool, &repo_pool)? {
+                Ok(maybe_rebase_plan) => maybe_rebase_plan,
                 Err(err) => {
                     err.describe(self.effects, self.repo, self.dag)?;
                     return Ok(Err(ExitCode(1)));
                 }
-            }
+            };
+            (maybe_rebase_plan, error_commits)
         };
 
-        let rewritten_oids = match execute_rebase_plan(
-            self.effects,
-            self.git_run_info,
-            self.repo,
-            self.event_log_db,
-            &rebase_plan,
-            &execute_options,
-        )? {
-            ExecuteRebasePlanResult::Succeeded {
-                rewritten_oids: Some(rewritten_oids),
-            } => rewritten_oids,
-            ExecuteRebasePlanResult::Succeeded {
-                rewritten_oids: None,
-            } => {
-                warn!("No rewritten commit OIDs were produced by rebase plan execution");
-                Default::default()
-            }
-            ExecuteRebasePlanResult::DeclinedToMerge {
-                failed_merge_info: _,
-            } => {
-                writeln!(
-                    self.effects.get_error_stream(),
-                    "BUG: Merge failed, but rewording shouldn't cause any merge failures."
-                )?;
-                return Ok(Err(ExitCode(1)));
-            }
-            ExecuteRebasePlanResult::Failed { exit_code } => {
-                return Ok(Err(exit_code));
-            }
+        let rewritten_oids = match maybe_rebase_plan {
+            None => Default::default(),
+            Some(rebase_plan) => match execute_rebase_plan(
+                self.effects,
+                self.git_run_info,
+                self.repo,
+                self.event_log_db,
+                &rebase_plan,
+                &execute_options,
+            )? {
+                ExecuteRebasePlanResult::Succeeded {
+                    rewritten_oids: Some(rewritten_oids),
+                } => rewritten_oids,
+                ExecuteRebasePlanResult::Succeeded {
+                    rewritten_oids: None,
+                } => {
+                    warn!("No rewritten commit OIDs were produced by rebase plan execution");
+                    Default::default()
+                }
+                ExecuteRebasePlanResult::DeclinedToMerge {
+                    failed_merge_info: _,
+                } => {
+                    writeln!(
+                        self.effects.get_error_stream(),
+                        "BUG: Merge failed, but rewording shouldn't cause any merge failures."
+                    )?;
+                    return Ok(Err(ExitCode(1)));
+                }
+                ExecuteRebasePlanResult::Failed { exit_code } => {
+                    return Ok(Err(exit_code));
+                }
+            },
         };
 
         let mut create_statuses = HashMap::new();
         for commit_oid in commit_oids {
+            if let Some(error_reason) = error_commits.get(&commit_oid) {
+                create_statuses.insert(
+                    commit_oid,
+                    match error_reason {
+                        ErrorReason::NotTested => CreateStatus::Skipped { commit_oid },
+                        ErrorReason::TestFailed => CreateStatus::Err { commit_oid },
+                    },
+                );
+                continue;
+            }
+
             let final_commit_oid = match rewritten_oids.get(&commit_oid) {
                 Some(MaybeZeroOid::NonZero(commit_oid)) => *commit_oid,
                 Some(MaybeZeroOid::Zero) => {
                     warn!(?commit_oid, "Commit was rewritten to the zero OID",);
                     commit_oid
                 }
-                None => commit_oid,
+                None => {
+                    create_statuses.insert(commit_oid, CreateStatus::Skipped { commit_oid });
+                    continue;
+                }
             };
             let local_branch_name = {
                 match self.get_revision_id(final_commit_oid)? {
@@ -527,13 +560,14 @@ Differential Revision: https://phabricator.example.com/D000$(git rev-list --coun
                                 )?
                             )?,
                         )?;
-                        return Ok(Err(ExitCode(1)));
+                        create_statuses.insert(commit_oid, CreateStatus::Err { commit_oid });
+                        continue;
                     }
                 }
             };
             create_statuses.insert(
                 commit_oid,
-                CreateStatus {
+                CreateStatus::Created {
                     final_commit_oid,
                     local_branch_name,
                 },
@@ -542,12 +576,12 @@ Differential Revision: https://phabricator.example.com/D000$(git rev-list --coun
 
         let final_commit_oids: CommitSet = create_statuses
             .values()
-            .map(|create_status| {
-                let CreateStatus {
+            .filter_map(|create_status| match create_status {
+                CreateStatus::Created {
                     final_commit_oid,
                     local_branch_name: _,
-                } = create_status;
-                *final_commit_oid
+                } => Some(*final_commit_oid),
+                CreateStatus::Skipped { .. } | CreateStatus::Err { .. } => None,
             })
             .collect();
         self.dag.sync_from_oids(