diff --git a/CHANGELOG.md b/CHANGELOG.md index 2009f6a0c6..8231ec36f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Breaking changes +* If `jj squash` receives paths or `--interactive`, then it will not abandon the + source commit even if it became empty. * Dropped support for `ui.default-revset` config (replaced by `revsets.log` in 0.8.0). @@ -37,6 +39,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### New features +* `jj squash` now accepts a `--no-abandon` option to keep the source commit. + * `jj branch list`/`tag list` now accept `-T`/`--template` option. The tag list prints commit summary along with the tag name by default. diff --git a/cli/src/commands/move.rs b/cli/src/commands/move.rs index 1bcea4670e..2ceed40f69 100644 --- a/cli/src/commands/move.rs +++ b/cli/src/commands/move.rs @@ -17,7 +17,7 @@ use jj_lib::object_id::ObjectId; use tracing::instrument; use super::squash::{move_diff, SquashedDescription}; -use crate::cli_util::{CommandHelper, RevisionArg}; +use crate::cli_util::{CommandHelper, DiffSelector, RevisionArg}; use crate::command_error::{user_error, CommandError}; use crate::ui::Ui; @@ -89,6 +89,13 @@ pub(crate) fn cmd_move( source.id().hex(), destination.id().hex() ); + + // If the user provided --interactive or files, the intent of the user was + // clearly to work on files rather than on commits, - it would be strange + // if the user wrote jj squash foo.rs and found that the branch pointer had + // moved to the parent, for example. + let abandon = !matches!(diff_selector, DiffSelector::Interactive(_)) && args.paths.is_empty(); + move_diff( ui, &mut tx, @@ -97,9 +104,14 @@ pub(crate) fn cmd_move( &destination, matcher.as_ref(), &diff_selector, - SquashedDescription::Combine, + if abandon { + SquashedDescription::Combine + } else { + SquashedDescription::UseDestination + }, false, &args.paths, + abandon, )?; tx.finish(ui, tx_description)?; Ok(()) diff --git a/cli/src/commands/squash.rs b/cli/src/commands/squash.rs index 58290fb8ff..52e7846464 100644 --- a/cli/src/commands/squash.rs +++ b/cli/src/commands/squash.rs @@ -37,13 +37,12 @@ use crate::ui::Ui; /// For example, `jj squash --into @--` moves changes from the working-copy /// commit to the grandparent. /// -/// If, after moving changes out, the source revision is empty compared to its -/// parent(s), it will be abandoned. Without `--interactive`, the source -/// revision will always be empty. +/// If `--interactive` is not set, no paths are provided, and `--keep` +/// is not set, the source will be abandoned. /// -/// If the source became empty and both the source and destination had a -/// non-empty description, you will be asked for the combined description. If -/// either was empty, then the other one will be used. +/// If the source was abandoned and both the source and destination +/// had a non-empty description, you will be asked for the combined +/// description. If either was empty, then the other one will be used. /// /// If a working-copy commit gets abandoned, it will be given a new, empty /// commit. This is true in general; it is not specific to this command. @@ -65,15 +64,19 @@ pub(crate) struct SquashArgs { /// description(s) of the source revision(s) #[arg(long, short, conflicts_with = "message_paragraphs")] use_destination_message: bool, - /// Interactively choose which parts to squash + /// Interactively choose which parts to squash (implies --no-abandon) #[arg(long, short)] interactive: bool, /// Specify diff editor to be used (implies --interactive) #[arg(long, value_name = "NAME")] tool: Option, - /// Move only changes to these paths (instead of all paths) + /// Move only changes to these paths instead of all paths (implies + /// --no-abandon) #[arg(conflicts_with_all = ["interactive", "tool"], value_hint = clap::ValueHint::AnyPath)] paths: Vec, + /// The source revision will not be abandoned + #[arg(long)] + no_abandon: bool, } #[instrument(skip_all)] @@ -119,6 +122,15 @@ pub(crate) fn cmd_squash( .to_matcher(); let diff_selector = workspace_command.diff_selector(ui, args.tool.as_deref(), args.interactive)?; + + // If the user provided --interactive or files, the intent of the user was + // clearly to work on files rather than on commits, - it would be strange + // if the user wrote jj squash foo.rs and found that the branch pointer had + // moved to the parent, for example. + let abandon = !args.no_abandon + && !matches!(diff_selector, DiffSelector::Interactive(_)) + && args.paths.is_empty(); + let mut tx = workspace_command.start_transaction(); let tx_description = format!("squash commits into {}", destination.id().hex()); move_diff( @@ -129,9 +141,10 @@ pub(crate) fn cmd_squash( &destination, matcher.as_ref(), &diff_selector, - SquashedDescription::from_args(args), + SquashedDescription::from_args(args, abandon), args.revision.is_none() && args.from.is_empty() && args.into.is_none(), &args.paths, + abandon, )?; tx.finish(ui, tx_description)?; Ok(()) @@ -150,14 +163,14 @@ pub(crate) enum SquashedDescription { // TODO(#2882): Remove public visibility once `jj move` is deleted. impl SquashedDescription { - pub(crate) fn from_args(args: &SquashArgs) -> Self { + pub(crate) fn from_args(args: &SquashArgs, abandon: bool) -> Self { // These options are incompatible and Clap is configured to prevent this. assert!(args.message_paragraphs.is_empty() || !args.use_destination_message); if !args.message_paragraphs.is_empty() { let desc = join_message_paragraphs(&args.message_paragraphs); SquashedDescription::Exact(desc) - } else if args.use_destination_message { + } else if args.use_destination_message || !abandon { SquashedDescription::UseDestination } else { SquashedDescription::Combine @@ -177,6 +190,7 @@ pub fn move_diff( description: SquashedDescription, no_rev_arg: bool, path_arg: &[String], + abandon: bool, ) -> Result<(), CommandError> { tx.base_workspace_helper() .check_rewritable(sources.iter().chain(std::iter::once(destination)).ids())?; @@ -185,7 +199,6 @@ pub fn move_diff( commit: &'a Commit, parent_tree: MergedTree, selected_tree: MergedTree, - abandon: bool, } let mut source_commits = vec![]; for source in sources { @@ -210,7 +223,6 @@ from the source will be moved into the destination. let selected_tree_id = diff_selector.select(&parent_tree, &source_tree, matcher, Some(&instructions))?; let selected_tree = tx.repo().store().get_root_tree(&selected_tree_id)?; - let abandon = selected_tree.id() == source_tree.id(); if !abandon && selected_tree_id == parent_tree.id() { // Nothing selected from this commit. If it's abandoned (i.e. already empty), we // still include it so `jj squash` can be used for abandoning an empty commit in @@ -223,7 +235,6 @@ from the source will be moved into the destination. commit: source, parent_tree, selected_tree, - abandon, }); } if source_commits.is_empty() { @@ -250,7 +261,7 @@ from the source will be moved into the destination. } for source in &source_commits { - if source.abandon { + if abandon { tx.mut_repo() .record_abandoned_commit(source.commit.id().clone()); } else { @@ -285,13 +296,12 @@ from the source will be moved into the destination. let description = match description { SquashedDescription::Exact(description) => description, SquashedDescription::UseDestination => destination.description().to_owned(), - SquashedDescription::Combine => { - let abandoned_commits = source_commits - .iter() - .filter_map(|source| source.abandon.then_some(source.commit)) - .collect_vec(); - combine_messages(tx.base_repo(), &abandoned_commits, destination, settings)? - } + SquashedDescription::Combine => combine_messages( + tx.base_repo(), + &sources.into_iter().collect_vec(), + destination, + settings, + )?, }; let mut predecessors = vec![destination.id().clone()]; predecessors.extend( diff --git a/cli/tests/cli-reference@.md.snap b/cli/tests/cli-reference@.md.snap index a2be897b00..1122712a9a 100644 --- a/cli/tests/cli-reference@.md.snap +++ b/cli/tests/cli-reference@.md.snap @@ -1781,9 +1781,9 @@ With the `-r` option, moves the changes from the specified revision to the paren With the `--from` and/or `--into` options, moves changes from/to the given revisions. If either is left out, it defaults to the working-copy commit. For example, `jj squash --into @--` moves changes from the working-copy commit to the grandparent. -If, after moving changes out, the source revision is empty compared to its parent(s), it will be abandoned. Without `--interactive`, the source revision will always be empty. +If `--interactive` is not set, no paths are provided, and `--keep` is not set, the source will be abandoned. -If the source became empty and both the source and destination had a non-empty description, you will be asked for the combined description. If either was empty, then the other one will be used. +If the source was abandoned and both the source and destination had a non-empty description, you will be asked for the combined description. If either was empty, then the other one will be used. If a working-copy commit gets abandoned, it will be given a new, empty commit. This is true in general; it is not specific to this command. @@ -1791,7 +1791,7 @@ If a working-copy commit gets abandoned, it will be given a new, empty commit. T ###### **Arguments:** -* `` — Move only changes to these paths (instead of all paths) +* `` — Move only changes to these paths instead of all paths (implies --no-abandon) ###### **Options:** @@ -1803,11 +1803,15 @@ If a working-copy commit gets abandoned, it will be given a new, empty commit. T Possible values: `true`, `false` -* `-i`, `--interactive` — Interactively choose which parts to squash +* `-i`, `--interactive` — Interactively choose which parts to squash (implies --no-abandon) Possible values: `true`, `false` * `--tool ` — Specify diff editor to be used (implies --interactive) +* `--no-abandon` — The source revision will not be abandoned + + Possible values: `true`, `false` + diff --git a/cli/tests/test_move_command.rs b/cli/tests/test_move_command.rs index 1e30470c13..21c5791aa5 100644 --- a/cli/tests/test_move_command.rs +++ b/cli/tests/test_move_command.rs @@ -221,7 +221,8 @@ fn test_move_partial() { "###); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ 71b69e433fbc d - │ ◉ 55171e33db26 b c + │ ◉ 9f21d0f664f3 c + │ ◉ 55171e33db26 b ├─╯ ◉ 3db0a2f5b535 a ◉ 000000000000 diff --git a/cli/tests/test_squash_command.rs b/cli/tests/test_squash_command.rs index da672d923a..427237d292 100644 --- a/cli/tests/test_squash_command.rs +++ b/cli/tests/test_squash_command.rs @@ -160,13 +160,14 @@ fn test_squash_partial() { let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["squash", "-r", "b", "-i"]); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" - Rebased 1 descendant commits - Working copy now at: mzvwutvl f03d5ce4 c | (no description set) - Parent commit : qpvuntsm c9f931cd a b | (no description set) + Rebased 2 descendant commits + Working copy now at: mzvwutvl 1fc0b9c1 c | (no description set) + Parent commit : kkmpptxz a54b88b5 b | (empty) (no description set) "###); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" - @ f03d5ce4a973 c - ◉ c9f931cd78af a b + @ 1fc0b9c19783 c + ◉ a54b88b5e186 b + ◉ c9f931cd78af a ◉ 000000000000 "###); let stdout = test_env.jj_cmd_success(&repo_path, &["print", "file1", "-r", "a"]); @@ -445,7 +446,8 @@ fn test_squash_from_to_partial() { "###); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ 71b69e433fbc d - │ ◉ 55171e33db26 b c + │ ◉ 9f21d0f664f3 c + │ ◉ 55171e33db26 b ├─╯ ◉ 3db0a2f5b535 a ◉ 000000000000 @@ -885,10 +887,8 @@ fn test_squash_from_multiple_partial_no_op() { "###); // Source commits that didn't match the paths are not rewritten - let (stdout, stderr) = test_env.jj_cmd_ok( - &repo_path, - &["squash", "--from=@-+ ~ @", "--into=@", "-m=d", "b"], - ); + let (stdout, stderr) = + test_env.jj_cmd_ok(&repo_path, &["squash", "--from=@-+ ~ @", "--into=@", "b"]); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" Working copy now at: mzvwutvl 9227d0d7 d @@ -897,6 +897,8 @@ fn test_squash_from_multiple_partial_no_op() { "###); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ 9227d0d780fa d + │ ◉ 8a4c9234c310 b + ├─╯ │ ◉ 5ad3ca4090a7 c ├─╯ ◉ 3df52ee1f8a9 a