chore: do rust more idiomatically, clean up language still referencing the non-rollback behaviour

Author: notactuallytreyanastasio

src/cmd/rebase.rs

@@ -96,12 +96,15 @@ fn make() -> clap::Command {
                 .long_help(
                     "Execute the given shell command after each patch is successfully \
                      applied during the rebase operation. If the command fails (exits with \
-                     non-zero status), the rebase will halt.\n\
+                     non-zero status), the entire transaction is rolled back and no patches \
+                     remain applied.\n\
                      \n\
                      This option may be specified multiple times to run multiple commands \
                      in sequence after each patch.\n\
                      \n\
-                     This is similar to `git rebase --exec`.",
+                     This is similar to `git rebase --exec`, but note that stgit rolls back \
+                     the entire operation on failure rather than leaving you at the failing \
+                     point.",
                 )
                 .action(clap::ArgAction::Append)
                 .value_name("cmd")
@@ -256,9 +259,9 @@ fn run(matches: &ArgMatches) -> Result<()> {
     } else if !matches.get_flag("nopush") {
         stack.check_head_top_mismatch()?;
         let check_merged = matches.get_flag("merged");
-        let exec_cmds: Vec<String> = matches
+        let exec_cmds: Vec<&str> = matches
             .get_many::<String>("exec")
-            .map(|vals| vals.cloned().collect())
+            .map(|vals| vals.map(String::as_str).collect())
             .unwrap_or_default();
 
         stack

src/stack/transaction/mod.rs

@@ -969,49 +969,36 @@ impl<'repo> StackTransaction<'repo> {
     where
         P: AsRef<PatchName>,
     {
-        let stupid = self.stack.repo.stupid();
-        stupid.with_temp_index(|stupid_temp| {
-            let mut temp_index_tree_id: Option<gix::ObjectId> = None;
-
-            let merged = if check_merged {
-                Some(self.check_merged(patchnames, stupid_temp, &mut temp_index_tree_id)?)
-            } else {
-                None
-            };
-
-            for (i, patchname) in patchnames.iter().enumerate() {
-                let patchname = patchname.as_ref();
-                let is_last = i + 1 == patchnames.len();
-                let already_merged = merged
-                    .as_ref()
-                    .is_some_and(|merged| merged.contains(&patchname));
-                self.push_patch(
-                    patchname,
-                    already_merged,
-                    is_last,
-                    stupid_temp,
-                    &mut temp_index_tree_id,
-                )?;
-            }
-
-            Ok(())
-        })
+        self.push_patches_impl(patchnames, check_merged, &[])
     }
 
     /// Push unapplied patches, running exec commands after each successful push.
     ///
-    /// This is similar to `push_patches`, but after each patch is pushed successfully,
-    /// all provided exec commands are run in sequence. If any exec command fails, the
-    /// entire transaction is rolled back (no patches remain applied).
+    /// After each patch is successfully pushed, all provided exec commands are run
+    /// in sequence. If any exec command fails, the entire transaction is rolled back
+    /// (no patches remain applied).
     ///
     /// This supports the `stg rebase --exec` functionality. Note that this differs from
-    /// `git rebase --exec` which leaves you at the failing point; stgit's behavior is
-    /// safer and consistent with how stgit transactions work.
+    /// `git rebase --exec` which leaves you at the failing point; stgit's rollback
+    /// behavior is safer and consistent with how stgit transactions work.
     pub(crate) fn push_patches_with_exec<P>(
         &mut self,
         patchnames: &[P],
         check_merged: bool,
-        exec_cmds: &[String],
+        exec_cmds: &[&str],
+    ) -> Result<()>
+    where
+        P: AsRef<PatchName>,
+    {
+        self.push_patches_impl(patchnames, check_merged, exec_cmds)
+    }
+
+    /// Core implementation for pushing patches with optional exec commands.
+    fn push_patches_impl<P>(
+        &mut self,
+        patchnames: &[P],
+        check_merged: bool,
+        exec_cmds: &[&str],
     ) -> Result<()>
     where
         P: AsRef<PatchName>,
@@ -1028,22 +1015,26 @@ impl<'repo> StackTransaction<'repo> {
 
             for (i, patchname) in patchnames.iter().enumerate() {
                 let patchname = patchname.as_ref();
-                let is_last = i + 1 == patchnames.len() && exec_cmds.is_empty();
+                // When exec commands are provided, we can't optimize the final checkout
+                // because the commands may modify the working tree. Only treat the last
+                // patch as "last" (enabling checkout optimization) when there are no
+                // exec commands.
+                let should_optimize_final_checkout =
+                    i + 1 == patchnames.len() && exec_cmds.is_empty();
                 let already_merged = merged
                     .as_ref()
                     .is_some_and(|merged| merged.contains(&patchname));
                 self.push_patch(
                     patchname,
                     already_merged,
-                    is_last,
+                    should_optimize_final_checkout,
                     stupid_temp,
                     &mut temp_index_tree_id,
                 )?;
 
-                // Run exec commands after each successful push
-                for exec_cmd in exec_cmds {
-                    self.ui.print_exec(exec_cmd)?;
-                    stupid.exec_cmd(exec_cmd)?;
+                for cmd in exec_cmds {
+                    self.ui.print_exec(cmd)?;
+                    stupid.exec_cmd(cmd)?;
                 }
             }
 

src/stupid/context.rs

@@ -1335,14 +1335,21 @@ impl StupidContext<'_, '_> {
             .stdout(Stdio::inherit())
             .stderr(Stdio::inherit())
             .status()
-            .with_context(|| format!("could not execute `{cmd}`"))?;
+            .with_context(|| format!("could not execute command: {cmd}"))?;
 
         if status.success() {
             Ok(())
         } else if let Some(code) = status.code() {
-            Err(anyhow!("`{cmd}` exited with code {code}"))
+            Err(anyhow!("command exited with code {code}: {cmd}"))
         } else {
-            Err(anyhow!("`{cmd}` failed"))
+            #[cfg(unix)]
+            {
+                use std::os::unix::process::ExitStatusExt;
+                if let Some(signal) = status.signal() {
+                    return Err(anyhow!("command killed by signal {signal}: {cmd}"));
+                }
+            }
+            Err(anyhow!("command failed: {cmd}"))
         }
     }
 

t/t2206-rebase-exec.sh

@@ -38,10 +38,10 @@ test_expect_success 'Rebase with multiple --exec options' '
     test $(wc -l <exec2.log) = 3
 '
 
-test_expect_success 'Rebase with --exec halts on command failure and rolls back' '
+test_expect_success 'Rebase with --exec rolls back on command failure' '
     rm -f exec.log out &&
     test_must_fail stg rebase --exec "echo EXEC >>exec.log && false" master >out 2>&1 &&
-    grep -q "exited with code" out &&
+    grep -q "command exited with code" out &&
     test_line_count = 1 exec.log &&
     test "$(stg series --applied -c)" = "0"
 '