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

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

git-branchless-submit/src/github.rs

@@ -289,6 +289,25 @@ impl Forge for GithubForge<'_> {
                 }),
                 options
             )?);
+            for (commit_oid, create_status) in created_branch.iter() {
+                match create_status {
+                    CreateStatus::Created { .. } => {}
+                    CreateStatus::Skipped | CreateStatus::Err => {
+                        // FIXME: surface the inner branch forge error somehow?
+                        writeln!(
+                            effects.get_output_stream(),
+                            "Could not create branch for commit: {}",
+                            effects.get_glyphs().render(
+                                self.repo.friendly_describe_commit_from_oid(
+                                    effects.get_glyphs(),
+                                    *commit_oid
+                                )?,
+                            )?
+                        )?;
+                        return Ok(Err(ExitCode(1)));
+                    }
+                }
+            }
             created_branches.extend(created_branch.into_iter());
         }
 
@@ -297,7 +316,7 @@ impl Forge for GithubForge<'_> {
             .copied()
             .map(|(commit_oid, commit_status)| {
                 let commit_status = match created_branches.get(&commit_oid) {
-                    Some(CreateStatus {
+                    Some(CreateStatus::Created {
                         final_commit_oid: _,
                         local_commit_name,
                     }) => CommitStatus {
@@ -308,11 +327,18 @@ impl Forge for GithubForge<'_> {
                         // Expecting this to be the same as the local branch name (for now):
                         remote_commit_name: Some(local_commit_name.clone()),
                     },
+
+                    Some(
+                        CreateStatus::Skipped  | CreateStatus::Err ,
+                    ) => {
+                        warn!(?commits_to_create, ?created_branches, ?commit_oid, ?commit_status, "commit failed to be created");
+                        eyre::bail!("BUG: should have been handled in previous call to branch_forge.create: {commit_oid:?} has status {commit_status:?}");
+                    }
                     None => commit_status.clone(),
                 };
-                (commit_oid, commit_status)
+                Ok((commit_oid, commit_status))
             })
-            .collect();
+            .try_collect()?;
 
         // Create the pull requests only after creating all the branches because
         // we rely on the presence of a branch on each commit in the stack to

git-branchless-submit/src/lib.rs

@@ -140,18 +140,28 @@ 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,
-
-    /// An identifier corresponding to the commit, for display purposes only.
-    /// This may be a branch name, a change ID, the commit summary, etc.
-    ///
-    /// This does not necessarily correspond to the commit's name/identifier in
-    /// the forge (e.g. not a code review link).
-    pub local_commit_name: String,
+pub enum CreateStatus {
+    /// The commit was successfully created.
+    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,
+
+        /// An identifier corresponding to the commit, for display purposes only.
+        /// This may be a branch name, a change ID, the commit summary, etc.
+        ///
+        /// This does not necessarily correspond to the commit's name/identifier in
+        /// the forge (e.g. not a code review link).
+        local_commit_name: String,
+    },
+
+    /// The commit was skipped. Possibly it had already been created, or it
+    /// couldn't be created due to a previous commit's error.
+    Skipped,
+
+    /// An error occurred while trying to create the commit.
+    Err,
 }
 
 /// "Forge" refers to a Git hosting provider, such as GitHub, GitLab, etc.
@@ -369,30 +379,52 @@ fn submit(
         (local, unsubmitted, to_update, to_skip)
     });
 
-    let (submitted_commit_names, unsubmitted_commit_names): (BTreeSet<String>, BTreeSet<String>) = {
+    let (submitted_commit_names, unsubmitted_commit_names, create_error_commits): (
+        BTreeSet<String>,
+        BTreeSet<String>,
+        BTreeSet<NonZeroOid>,
+    ) = {
         let unsubmitted_commit_names: BTreeSet<String> = unsubmitted_commits
             .values()
             .flat_map(|commit_status| commit_status.local_commit_name.clone())
             .collect();
         if create {
-            let created_commit_names = if dry_run {
-                unsubmitted_commit_names.clone()
+            if dry_run {
+                (
+                    unsubmitted_commit_names.clone(),
+                    Default::default(),
+                    Default::default(),
+                )
             } else {
                 let create_statuses =
                     try_exit_code!(forge.create(unsubmitted_commits, &submit_options)?);
-                create_statuses
-                    .into_values()
-                    .map(
-                        |CreateStatus {
-                             final_commit_oid: _,
-                             local_commit_name,
-                         }| local_commit_name,
-                    )
-                    .collect()
-            };
-            (created_commit_names, Default::default())
+                let mut submitted_commit_names = BTreeSet::new();
+                let mut error_commits = BTreeSet::new();
+                for (commit_oid, create_status) in create_statuses {
+                    match create_status {
+                        CreateStatus::Created {
+                            final_commit_oid: _, // currently not rendered
+                            local_commit_name,
+                        } => {
+                            submitted_commit_names.insert(local_commit_name);
+                        }
+                        // 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 | CreateStatus::Err => {
+                            error_commits.insert(commit_oid);
+                        }
+                    }
+                }
+                (submitted_commit_names, Default::default(), error_commits)
+            }
         } else {
-            (Default::default(), unsubmitted_commit_names)
+            (
+                Default::default(),
+                unsubmitted_commit_names,
+                Default::default(),
+            )
         }
     };
 
@@ -512,8 +544,33 @@ repository. To submit 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]