refactor

- deduplicate
- remove unhappy tests in favor of unit-testing the respective function.

Author: Byron

crates/but/src/command/legacy/describe.rs

@@ -1,7 +1,8 @@
 use anyhow::{Result, bail};
 use but_ctx::Context;
-use but_oxidize::{ObjectIdExt, OidExt};
+use but_oxidize::{ObjectIdExt as _, OidExt};
 use gitbutler_project::Project;
+use gix::prelude::ObjectIdExt;
 
 use crate::{legacy::id::CliId, tui, utils::OutputChannel};
 
@@ -64,17 +65,8 @@ fn edit_branch_name(
         }
 
         if let Some(sid) = stack_entry.id {
-            let new_name = if let Some(msg) = message {
-                // Use provided message, trim and validate
-                let trimmed = msg.trim();
-                if trimmed.is_empty() {
-                    bail!("Aborting due to empty branch name");
-                }
-                trimmed.to_string()
-            } else {
-                // Fall back to editor
-                get_branch_name_from_editor(branch_name)?
-            };
+            let new_name = prepare_provided_message(message, "branch name")
+                .unwrap_or_else(|| get_branch_name_from_editor(branch_name))?;
             but_api::legacy::stack::update_branch_name(
                 project.id,
                 sid,
@@ -91,6 +83,16 @@ fn edit_branch_name(
     bail!("Branch '{}' not found in any stack", branch_name)
 }
 
+fn prepare_provided_message(msg: Option<&str>, entity: &str) -> Option<Result<String>> {
+    msg.map(|msg| {
+        let trimmed = msg.trim();
+        if trimmed.is_empty() {
+            bail!("Aborting due to empty {entity}");
+        }
+        Ok(trimmed.to_string())
+    })
+}
+
 fn edit_commit_message_by_id(
     ctx: &Context,
     project: &Project,
@@ -149,24 +151,19 @@ fn edit_commit_message_by_id(
     let current_message = commit_message.to_string();
 
     // Get new message from provided argument or editor
-    let new_message = if let Some(msg) = message {
-        // Use provided message, trim and validate
-        let trimmed = msg.trim();
-        if trimmed.is_empty() {
-            bail!("Aborting due to empty commit message");
-        }
-        trimmed.to_string()
-    } else {
-        // Get the files changed in this commit using but_api
+    let new_message = prepare_provided_message(message, "commit message").unwrap_or_else(|| {
         let commit_details = but_api::legacy::diff::commit_details(project.id, commit_oid.into())?;
         let changed_files = get_changed_files_from_commit_details(&commit_details);
 
         // Open editor with current message and file list
-        get_commit_message_from_editor(&current_message, &changed_files)?
-    };
+        get_commit_message_from_editor(&current_message, &changed_files)
+    })?;
 
     if new_message.trim() == current_message.trim() {
-        bail!("No changes to commit message.");
+        if let Some(out) = out.for_human() {
+            writeln!(out, "No changes to commit message - nothing to be done")?;
+        }
+        return Ok(());
     }
 
     // Use gitbutler_branch_actions::update_commit_message instead of low-level primitives
@@ -179,11 +176,12 @@ fn edit_commit_message_by_id(
     )?;
 
     if let Some(out) = out.for_human() {
+        let repo = ctx.repo.get()?;
         writeln!(
             out,
             "Updated commit message for {} (now {})",
-            commit_oid.to_hex_with_len(7),
-            new_commit_oid.to_gix().to_hex_with_len(7)
+            commit_oid.attach(&repo).shorten_or_id(),
+            new_commit_oid.to_gix().attach(&repo).shorten_or_id()
         )?;
     }
 
@@ -259,3 +257,33 @@ fn get_branch_name_from_editor(current_name: &str) -> Result<String> {
 
     Ok(branch_name)
 }
+
+#[cfg(test)]
+mod tests {
+
+    mod prepare_provided_message {
+        use super::super::*;
+
+        #[test]
+        fn empty_is_fails() {
+            assert_eq!(
+                prepare_provided_message(Some(""), "message")
+                    .unwrap()
+                    .unwrap_err()
+                    .to_string(),
+                "Aborting due to empty message"
+            );
+        }
+
+        #[test]
+        fn empty_is_after_trimming_fails() {
+            assert_eq!(
+                prepare_provided_message(Some("    "), "message")
+                    .unwrap()
+                    .unwrap_err()
+                    .to_string(),
+                "Aborting due to empty message"
+            );
+        }
+    }
+}

crates/but/tests/but/command/describe.rs

@@ -1,6 +1,6 @@
-use snapbox::str;
-
 use crate::utils::{Sandbox, setup_metadata};
+use gitbutler_commit::commit_ext::CommitExt;
+use snapbox::str;
 
 #[test]
 fn describe_commit_with_message_flag() -> anyhow::Result<()> {
@@ -22,43 +22,17 @@ Updated commit message for [..] (now [..])
 
 "#]]);
 
-    // Verify the commit message was updated
-    let log = env.git_log()?;
-    assert!(log.contains("Updated commit message"));
-    assert!(!log.contains("add A"));
-
-    Ok(())
-}
-
-// Note: Branch rename test is omitted because the test scenario uses single-character
-// branch names ("A") which don't meet the 2-character minimum requirement for CLI IDs.
-// The branch rename functionality with -m flag is tested manually and works correctly.
-
-#[test]
-fn describe_with_empty_message_fails() -> anyhow::Result<()> {
-    let env = Sandbox::init_scenario_with_target_and_default_settings("one-stack")?;
     insta::assert_snapshot!(env.git_log()?, @r"
-    * edd3eb7 (HEAD -> gitbutler/workspace) GitButler Workspace Commit
-    * 9477ae7 (A) add A
-    * 0dc3733 (origin/main, origin/HEAD, main) add M
+    * d84f3c4 (HEAD -> gitbutler/workspace) GitButler Workspace Commit
+    * 2f7c570 (A) Updated commit message
+    * 0dc3733 (origin/main, origin/HEAD, main, gitbutler/target) add M
     ");
 
-    setup_metadata(&env, &["A"])?;
-
-    // Try to describe commit with empty message
-    env.but("describe 9477ae7 -m ''")
-        .assert()
-        .failure()
-        .stderr_eq(str![[r#"
-Error: Aborting due to empty commit message
-
-"#]]);
-
     Ok(())
 }
 
 #[test]
-fn describe_with_whitespace_only_message_fails() -> anyhow::Result<()> {
+fn describe_commit_with_multiline_message() -> anyhow::Result<()> {
     let env = Sandbox::init_scenario_with_target_and_default_settings("one-stack")?;
     insta::assert_snapshot!(env.git_log()?, @r"
     * edd3eb7 (HEAD -> gitbutler/workspace) GitButler Workspace Commit
@@ -68,66 +42,40 @@ fn describe_with_whitespace_only_message_fails() -> anyhow::Result<()> {
 
     setup_metadata(&env, &["A"])?;
 
-    // Try to describe commit with whitespace-only message
-    env.but("describe 9477ae7 -m '   '")
+    // Use describe with multiline message
+    env.but("describe 9477ae7 -m 'First line\n\n\tSecond paragraph with details'")
         .assert()
-        .failure()
-        .stderr_eq(str![[r#"
-Error: Aborting due to empty commit message
+        .success()
+        .stdout_eq(str![[r#"
+Updated commit message for [..] (now [..])
 
 "#]]);
 
-    Ok(())
-}
-
-#[test]
-fn describe_commit_with_same_message_fails() -> anyhow::Result<()> {
-    let env = Sandbox::init_scenario_with_target_and_default_settings("one-stack")?;
+    // Verify the commit message was updated with multiline content
     insta::assert_snapshot!(env.git_log()?, @r"
-    * edd3eb7 (HEAD -> gitbutler/workspace) GitButler Workspace Commit
-    * 9477ae7 (A) add A
-    * 0dc3733 (origin/main, origin/HEAD, main) add M
+    * f2c2b50 (HEAD -> gitbutler/workspace) GitButler Workspace Commit
+    * cdf2c74 (A) First line
+    * 0dc3733 (origin/main, origin/HEAD, main, gitbutler/target) add M
     ");
 
-    setup_metadata(&env, &["A"])?;
-
-    // Try to describe with the same message
-    env.but("describe 9477ae7 -m 'add A'")
-        .assert()
-        .failure()
-        .stderr_eq(str![[r#"
-Error: No changes to commit message.
-
-"#]]);
+    let repo = env.open_repo()?;
+    assert_eq!(
+        repo.rev_parse_single(":/First line")?
+            .object()?
+            .into_commit()
+            .message_bstr(),
+        "First line\n\n\tSecond paragraph with details"
+    );
 
     Ok(())
 }
 
-#[test]
-fn describe_nonexistent_target_fails() -> anyhow::Result<()> {
-    let env = Sandbox::init_scenario_with_target_and_default_settings("one-stack")?;
-    insta::assert_snapshot!(env.git_log()?, @r"
-    * edd3eb7 (HEAD -> gitbutler/workspace) GitButler Workspace Commit
-    * 9477ae7 (A) add A
-    * 0dc3733 (origin/main, origin/HEAD, main) add M
-    ");
-
-    setup_metadata(&env, &["A"])?;
-
-    // Try to describe a nonexistent target
-    env.but("describe nonexistent -m 'new message'")
-        .assert()
-        .failure()
-        .stderr_eq(str![[r#"
-Error: ID 'nonexistent' not found
-
-"#]]);
-
-    Ok(())
-}
+// Note: Branch rename test is omitted because the test scenario uses single-character
+// branch names ("A") which don't meet the 2-character minimum requirement for CLI IDs.
+// The branch rename functionality with -m flag is tested manually and works correctly.
 
 #[test]
-fn describe_commit_with_multiline_message() -> anyhow::Result<()> {
+fn describe_commit_with_same_message_succeeds_as_noop() -> anyhow::Result<()> {
     let env = Sandbox::init_scenario_with_target_and_default_settings("one-stack")?;
     insta::assert_snapshot!(env.git_log()?, @r"
     * edd3eb7 (HEAD -> gitbutler/workspace) GitButler Workspace Commit
@@ -137,18 +85,14 @@ fn describe_commit_with_multiline_message() -> anyhow::Result<()> {
 
     setup_metadata(&env, &["A"])?;
 
-    // Use describe with multiline message
-    env.but("describe 9477ae7 -m 'First line\n\nSecond paragraph with details'")
+    // Try to describe with the same message
+    env.but("describe 9477ae7 -m 'add A'")
         .assert()
         .success()
         .stdout_eq(str![[r#"
-Updated commit message for [..] (now [..])
+No changes to commit message - nothing to be done
 
 "#]]);
 
-    // Verify the commit message was updated with multiline content
-    let log = env.git_log()?;
-    assert!(log.contains("First line"));
-
     Ok(())
 }