Merge pull request #11405 from gitbutlerapp/but-describe

Allow inline message for but describe

Author: Byron

.github/copilot-instructions.md

@@ -347,3 +347,4 @@ corepack prepare pnpm@10.17.0 --activate
 8. **Test on target platform** if making platform-specific changes
 9. **Security**: Check dependencies for vulnerabilities before adding them
 10. **Code marked for refactoring**: Be extra careful with crates in the "Code Hitlist" section
+11. **but CLI happy path testing only**: CLI tests are expensive and should be limited to what really matters.

crates/but/src/args/mod.rs

@@ -345,6 +345,9 @@ pub enum Subcommands {
     Describe {
         /// Commit ID to edit the message for, or branch ID to rename
         target: String,
+        /// The new commit message or branch name. If not provided, opens an editor.
+        #[clap(short = 'm', long = "message")]
+        message: Option<String>,
     },
 
     /// Show operation history.

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

@@ -1,14 +1,16 @@
 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};
 
 pub(crate) fn describe_target(
     project: &Project,
     out: &mut OutputChannel,
     target: &str,
+    message: Option<&str>,
 ) -> Result<()> {
     let mut ctx = Context::new_from_legacy_project(project.clone())?;
 
@@ -31,10 +33,10 @@ pub(crate) fn describe_target(
 
     match cli_id {
         CliId::Branch { name, .. } => {
-            edit_branch_name(&ctx, project, name, out)?;
+            edit_branch_name(&ctx, project, name, out, message)?;
         }
         CliId::Commit { oid } => {
-            edit_commit_message_by_id(&ctx, project, *oid, out)?;
+            edit_commit_message_by_id(&ctx, project, *oid, out, message)?;
         }
         _ => {
             bail!("Target must be a commit ID, not {}", cli_id.kind());
@@ -49,6 +51,7 @@ fn edit_branch_name(
     project: &Project,
     branch_name: &str,
     out: &mut OutputChannel,
+    message: Option<&str>,
 ) -> Result<()> {
     // Find which stack this branch belongs to
     let stacks = but_api::legacy::workspace::stacks(
@@ -62,7 +65,8 @@ fn edit_branch_name(
         }
 
         if let Some(sid) = stack_entry.id {
-            let new_name = 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,
@@ -79,11 +83,22 @@ 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,
     commit_oid: gix::ObjectId,
     out: &mut OutputChannel,
+    message: Option<&str>,
 ) -> Result<()> {
     // Find which stack this commit belongs to
     let stacks = but_api::legacy::workspace::stacks(project.id, None)?;
@@ -132,18 +147,23 @@ fn edit_commit_message_by_id(
     let stack_id = stack_id
         .ok_or_else(|| anyhow::anyhow!("Could not find stack for commit {}", commit_oid))?;
 
-    // Get the files changed in this commit using but_api
-    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);
-
     // Get current commit message
     let current_message = commit_message.to_string();
 
-    // Open editor with current message and file list
-    let new_message = get_commit_message_from_editor(&current_message, &changed_files)?;
+    // Get new message from provided argument or editor
+    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)
+    })?;
 
     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
@@ -156,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()
         )?;
     }
 
@@ -236,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/src/lib.rs

@@ -364,9 +364,9 @@ async fn match_subcommand(
                 .emit_metrics(metrics_ctx)
         }
         #[cfg(feature = "legacy")]
-        Subcommands::Describe { target } => {
+        Subcommands::Describe { target, message } => {
             let project = legacy::get_or_init_non_bare_project(&args)?;
-            command::legacy::describe::describe_target(&project, out, &target)
+            command::legacy::describe::describe_target(&project, out, &target, message.as_deref())
                 .emit_metrics(metrics_ctx)
         }
         #[cfg(feature = "legacy")]

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

@@ -0,0 +1,98 @@
+use crate::utils::{Sandbox, setup_metadata};
+use gitbutler_commit::commit_ext::CommitExt;
+use snapbox::str;
+
+#[test]
+fn describe_commit_with_message_flag() -> 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"])?;
+
+    // Use describe with -m flag to change commit message (using commit ID)
+    env.but("describe 9477ae7 -m 'Updated commit message'")
+        .assert()
+        .success()
+        .stdout_eq(str![[r#"
+Updated commit message for [..] (now [..])
+
+"#]]);
+
+    insta::assert_snapshot!(env.git_log()?, @r"
+    * d84f3c4 (HEAD -> gitbutler/workspace) GitButler Workspace Commit
+    * 2f7c570 (A) Updated commit message
+    * 0dc3733 (origin/main, origin/HEAD, main, gitbutler/target) add M
+    ");
+
+    Ok(())
+}
+
+#[test]
+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
+    * 9477ae7 (A) add A
+    * 0dc3733 (origin/main, origin/HEAD, main) add M
+    ");
+
+    setup_metadata(&env, &["A"])?;
+
+    // Use describe with multiline message
+    env.but("describe 9477ae7 -m 'First line\n\n\tSecond paragraph with details'")
+        .assert()
+        .success()
+        .stdout_eq(str![[r#"
+Updated commit message for [..] (now [..])
+
+"#]]);
+
+    // Verify the commit message was updated with multiline content
+    insta::assert_snapshot!(env.git_log()?, @r"
+    * f2c2b50 (HEAD -> gitbutler/workspace) GitButler Workspace Commit
+    * cdf2c74 (A) First line
+    * 0dc3733 (origin/main, origin/HEAD, main, gitbutler/target) add M
+    ");
+
+    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(())
+}
+
+// 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_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
+    * 9477ae7 (A) add A
+    * 0dc3733 (origin/main, origin/HEAD, main) add M
+    ");
+
+    setup_metadata(&env, &["A"])?;
+
+    // Try to describe with the same message
+    env.but("describe 9477ae7 -m 'add A'")
+        .assert()
+        .success()
+        .stdout_eq(str![[r#"
+No changes to commit message - nothing to be done
+
+"#]]);
+
+    Ok(())
+}

crates/but/tests/but/command/mod.rs

@@ -1,8 +1,11 @@
 //! Put command-specific tests here. They should be focused on what's important for each command.
 //! Ideally they *show* the initial state, and the *post* state, to validate the commands actually do what they claim.
+//! **Only** test the *happy path* of a typical user journey, while keeping details in unit tests with private module access.
 mod branch;
 #[cfg(feature = "legacy")]
 mod commit;
+#[cfg(feature = "legacy")]
+mod describe;
 mod format;
 mod help;
 #[cfg(feature = "legacy")]