diff --git a/crates/but/skill/SKILL.md b/crates/but/skill/SKILL.md index 3cca9592905..2a0621cd4f6 100644 --- a/crates/but/skill/SKILL.md +++ b/crates/but/skill/SKILL.md @@ -41,7 +41,7 @@ but ... --status-after - Commit: `but commit -m "" --changes , --status-after` - Commit + create branch: `but commit -c -m "" --changes --status-after` -- Amend: `but amend --status-after` +- Amend: `but amend [,...] --status-after` - Reorder commits: `but move --status-after` (**commit IDs**, not branch names) - Stack branches: `but move --status-after` (**branch names or branch CLI IDs**) - Tear off a branch: `but move zz --status-after` (`zz` = unassigned; branch name or branch CLI ID) @@ -62,7 +62,7 @@ but ... --status-after 1. `but status -fv` (or `but show `) 2. Locate file ID and target commit ID. -3. `but amend --status-after` +3. `but amend [,...] --status-after` ### Reorder commits diff --git a/crates/but/skill/references/reference.md b/crates/but/skill/references/reference.md index 44aa8be442a..2d526fe81eb 100644 --- a/crates/but/skill/references/reference.md +++ b/crates/but/skill/references/reference.md @@ -288,8 +288,8 @@ but squash --status-after # Squash then show workspace status Amend file into a specific commit. Use when you know exactly which commit the change belongs to. ```bash -but amend # Amend file into specific commit -but amend --status-after # Amend then show workspace status +but amend [,...] # Amend file(s) into specific commit +but amend [,...] --status-after # Amend then show workspace status ``` **When to use `amend` vs `absorb`:** diff --git a/crates/but/src/command/legacy/rub/amend.rs b/crates/but/src/command/legacy/rub/amend.rs index df4337629ac..273f9872026 100644 --- a/crates/but/src/command/legacy/rub/amend.rs +++ b/crates/but/src/command/legacy/rub/amend.rs @@ -18,7 +18,7 @@ pub(crate) fn uncommitted_to_commit_with_perm( oid: ObjectId, out: &mut OutputChannel, perm: &mut RepoExclusive, -) -> anyhow::Result<()> { +) -> anyhow::Result> { let first_hunk_assignment = hunk_assignments.first(); let stack_id = first_hunk_assignment.stack_id; @@ -41,13 +41,8 @@ pub(crate) fn uncommitted_to_commit_with_perm( }) .unwrap_or_default(); writeln!(out, "Amended {description} → {new_commit}")?; - } else if let Some(out) = out.for_json() { - out.write_value(serde_json::json!({ - "ok": true, - "new_commit_id": outcome.new_commit.map(|c| c.to_string()), - }))?; } - Ok(()) + Ok(outcome.new_commit) } fn amend_diff_specs( diff --git a/crates/but/src/command/legacy/rub/mod.rs b/crates/but/src/command/legacy/rub/mod.rs index ac2910bf692..d23471a4f44 100644 --- a/crates/but/src/command/legacy/rub/mod.rs +++ b/crates/but/src/command/legacy/rub/mod.rs @@ -1444,6 +1444,8 @@ pub(crate) fn handle_amend( // Validate that commit is a commit match commit { CliId::Commit { commit_id, .. } => { + let mut commit_id = commit_id; + let mut amended_commit_ids = Vec::new(); // TODO(dp by st): This is a duplication of the UncommittedToCommitOperation which was previously // called through `handle()` after validation. The problem is that it does its own locking. // Since these are all mutations, it would have to be changed to take `perm` as well. @@ -1455,18 +1457,40 @@ pub(crate) fn handle_amend( OperationKind::AmendCommit, guard.write_permission(), ); - amend::uncommitted_to_commit_with_perm( + if let Some(new_commit_id) = amend::uncommitted_to_commit_with_perm( ctx, uncommitted.hunk_assignments.as_ref(), uncommitted.describe(), commit_id, out, guard.write_permission(), - )?; + )? { + commit_id = new_commit_id; + amended_commit_ids.push(Some(new_commit_id)); + } else { + amended_commit_ids.push(None); + } } _ => unreachable!("validated beforehand"), } } + if let Some(out) = out.for_json() { + let last_successful_commit_id = amended_commit_ids + .iter() + .rev() + .copied() + .flatten() + .next() + .map(|c| c.to_string()); + out.write_value(serde_json::json!({ + "ok": true, + "new_commit_id": last_successful_commit_id, + "new_commit_ids": amended_commit_ids + .into_iter() + .map(|commit_id| commit_id.map(|c| c.to_string())) + .collect::>(), + }))?; + } } other => { bail!( diff --git a/crates/but/tests/but/command/rub.rs b/crates/but/tests/but/command/rub.rs index f96fc5b87d8..3a03a75bfd7 100644 --- a/crates/but/tests/but/command/rub.rs +++ b/crates/but/tests/but/command/rub.rs @@ -1137,6 +1137,127 @@ Amended [..] → [..] Ok(()) } +#[test] +fn amend_multiple_uncommitted_files_to_commit() -> anyhow::Result<()> { + let env = Sandbox::init_scenario_with_target_and_default_settings("two-stacks")?; + env.setup_metadata(&["A", "B"])?; + + env.file("first-uncommitted.txt", "first\n"); + env.file("second-uncommitted.txt", "second\n"); + + let before = status_json(&env)?; + let first_file = unassigned_cli_id_for_file(&before, "first-uncommitted.txt") + .expect("first uncommitted file should be present"); + let second_file = unassigned_cli_id_for_file(&before, "second-uncommitted.txt") + .expect("second uncommitted file should be present"); + let target_commit = branch_commit_ids(&before, "A")[0].clone(); + + env.but(format!("amend {first_file},{second_file} {target_commit}")) + .assert() + .success() + .stdout_eq(str![[r#" +Amended the only hunk in first-uncommitted.txt in the unassigned area → [..] +Amended the only hunk in second-uncommitted.txt in the unassigned area → [..] + +"#]]) + .stderr_eq(str![""]); + + let after = status_json(&env)?; + assert!( + branch_commits_contain_file(&after, "A", "first-uncommitted.txt"), + "first file should be amended into the target commit" + ); + assert!( + branch_commits_contain_file(&after, "A", "second-uncommitted.txt"), + "second file should be amended into the target commit" + ); + assert!( + !unassigned_contains_file(&after, "first-uncommitted.txt"), + "first file should no longer be unassigned" + ); + assert!( + !unassigned_contains_file(&after, "second-uncommitted.txt"), + "second file should no longer be unassigned" + ); + + Ok(()) +} + +#[test] +fn amend_multiple_uncommitted_files_to_commit_status_after_json() -> anyhow::Result<()> { + let env = Sandbox::init_scenario_with_target_and_default_settings("two-stacks")?; + env.setup_metadata(&["A", "B"])?; + + env.file("first-uncommitted.txt", "first\n"); + env.file("second-uncommitted.txt", "second\n"); + + let before = status_json(&env)?; + let first_file = unassigned_cli_id_for_file(&before, "first-uncommitted.txt") + .expect("first uncommitted file should be present"); + let second_file = unassigned_cli_id_for_file(&before, "second-uncommitted.txt") + .expect("second uncommitted file should be present"); + let target_commit = branch_commit_ids(&before, "A")[0].clone(); + + let output = env + .but(format!( + "--json --status-after amend {first_file},{second_file} {target_commit}" + )) + .allow_json() + .output()?; + assert!(output.status.success()); + assert_eq!(String::from_utf8_lossy(&output.stderr), ""); + + let value: serde_json::Value = serde_json::from_slice(&output.stdout)?; + let amended_commits = value["result"]["new_commit_ids"] + .as_array() + .expect("amend result should contain all rewritten commit ids"); + assert_eq!(amended_commits.len(), 2); + assert!(value["result"]["new_commit_id"].as_str().is_some()); + assert!(value["status"]["stacks"].as_array().is_some()); + + let after = status_json(&env)?; + assert!( + branch_commits_contain_file(&after, "A", "first-uncommitted.txt"), + "first file should be amended into the target commit" + ); + assert!( + branch_commits_contain_file(&after, "A", "second-uncommitted.txt"), + "second file should be amended into the target commit" + ); + + Ok(()) +} + +#[test] +fn amend_duplicate_uncommitted_file_reports_last_successful_commit_json() -> anyhow::Result<()> { + let env = Sandbox::init_scenario_with_target_and_default_settings("two-stacks")?; + env.setup_metadata(&["A", "B"])?; + + env.file("duplicate-uncommitted.txt", "content\n"); + + let before = status_json(&env)?; + let file_id = unassigned_cli_id_for_file(&before, "duplicate-uncommitted.txt") + .expect("uncommitted file should be present"); + let target_commit = branch_commit_ids(&before, "A")[0].clone(); + + let output = env + .but(format!("--json amend {file_id},{file_id} {target_commit}")) + .allow_json() + .output()?; + assert!(output.status.success()); + assert_eq!(String::from_utf8_lossy(&output.stderr), ""); + + let value: serde_json::Value = serde_json::from_slice(&output.stdout)?; + let amended_commits = value["new_commit_ids"] + .as_array() + .expect("amend result should contain all attempted commit ids"); + assert_eq!(amended_commits.len(), 2); + assert_eq!(value["new_commit_id"], amended_commits[0]); + assert!(amended_commits[1].is_null()); + + Ok(()) +} + #[test] fn rub_matrix_uncommitted_to_stack_smoke() -> anyhow::Result<()> { let env = Sandbox::init_scenario_with_target_and_default_settings("two-stacks")?;