From 07559f24ece9dce290f06648018b3e74a485d9ff Mon Sep 17 00:00:00 2001 From: Alexander Potashev Date: Fri, 10 May 2024 16:44:49 +0200 Subject: [PATCH] Refuse to split an empty commit with `jj split`. Rationale: The user may be confused by the empty diff in the diff editor tool if they accidentally run `jj split` on a wrong (empty) commit. --- CHANGELOG.md | 2 ++ cli/src/commands/split.rs | 12 +++++++- cli/src/commit_templater.rs | 8 +---- cli/tests/cli-reference@.md.snap | 2 ++ cli/tests/test_immutable_commits.rs | 45 +++++++++++++++-------------- cli/tests/test_split_command.rs | 15 ++++++++++ lib/src/commit.rs | 10 +++++++ 7 files changed, 65 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3404c5467..7c660fde7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * The `commit_summary_no_branches` template is superseded by `templates.branch_list`. +* `jj split` will now refuse to split an empty commit. + ### Deprecations ### New features diff --git a/cli/src/commands/split.rs b/cli/src/commands/split.rs index 9661a50b4..ef4faa2af 100644 --- a/cli/src/commands/split.rs +++ b/cli/src/commands/split.rs @@ -18,7 +18,7 @@ use jj_lib::repo::Repo; use tracing::instrument; use crate::cli_util::{CommandHelper, RevisionArg}; -use crate::command_error::CommandError; +use crate::command_error::{user_error_with_hint, CommandError}; use crate::description_util::{description_template_for_commit, edit_description}; use crate::ui::Ui; @@ -36,6 +36,9 @@ use crate::ui::Ui; /// change description for each commit. If the change did not have a /// description, the second part will not get a description, and you will be /// asked for a description only for the first part. +/// +/// Splitting an empty commit is not supported because the same effect can be +/// achieved with `jj new`. #[derive(clap::Args, Clone, Debug)] pub(crate) struct SplitArgs { /// Interactively choose which parts to split. This is the default if no @@ -64,6 +67,13 @@ pub(crate) fn cmd_split( ) -> Result<(), CommandError> { let mut workspace_command = command.workspace_helper(ui)?; let commit = workspace_command.resolve_single_rev(&args.revision)?; + if commit.is_empty(workspace_command.repo().as_ref())? { + return Err(user_error_with_hint( + format!("Refusing to split empty commit {}.", commit.id().hex()), + "Use `jj new` if you want to create another empty commit.", + )); + } + workspace_command.check_rewritable([commit.id()])?; let matcher = workspace_command .parse_file_patterns(&args.paths)? diff --git a/cli/src/commit_templater.rs b/cli/src/commit_templater.rs index a6bd3ef0e..216e36746 100644 --- a/cli/src/commit_templater.rs +++ b/cli/src/commit_templater.rs @@ -641,13 +641,7 @@ fn builtin_commit_methods<'repo>() -> CommitTemplateBuildMethodFnMap<'repo, Comm map.insert("empty", |language, _build_ctx, self_property, function| { template_parser::expect_no_arguments(function)?; let repo = language.repo; - let out_property = self_property.and_then(|commit| { - if let [parent] = &commit.parents()[..] { - return Ok(parent.tree_id() == commit.tree_id()); - } - let parent_tree = commit.parent_tree(repo)?; - Ok(*commit.tree_id() == parent_tree.id()) - }); + let out_property = self_property.and_then(|commit| Ok(commit.is_empty(repo)?)); Ok(L::wrap_boolean(out_property)) }); map.insert("root", |language, _build_ctx, self_property, function| { diff --git a/cli/tests/cli-reference@.md.snap b/cli/tests/cli-reference@.md.snap index 7a199ef4c..04557b883 100644 --- a/cli/tests/cli-reference@.md.snap +++ b/cli/tests/cli-reference@.md.snap @@ -1732,6 +1732,8 @@ Starts a [diff editor] on the changes in the revision. Edit the right side of th If the change you split had a description, you will be asked to enter a change description for each commit. If the change did not have a description, the second part will not get a description, and you will be asked for a description only for the first part. +Splitting an empty commit is not supported because the same effect can be achieved with `jj new`. + **Usage:** `jj split [OPTIONS] [PATHS]...` ###### **Arguments:** diff --git a/cli/tests/test_immutable_commits.rs b/cli/tests/test_immutable_commits.rs index 5d4ac87ae..f9ad490fc 100644 --- a/cli/tests/test_immutable_commits.rs +++ b/cli/tests/test_immutable_commits.rs @@ -121,6 +121,9 @@ fn test_rewrite_immutable_commands() { test_env.jj_cmd_ok(&repo_path, &["new", "@-", "-m=c"]); std::fs::write(repo_path.join("file"), "c").unwrap(); test_env.jj_cmd_ok(&repo_path, &["new", "all:visible_heads()", "-m=merge"]); + // Create another file to make sure the merge commit isn't empty (to satisfy `jj + // split`) and still has a conflict (to satisfy `jj resolve`). + std::fs::write(repo_path.join("file2"), "merged").unwrap(); test_env.jj_cmd_ok(&repo_path, &["branch", "create", "main"]); test_env.jj_cmd_ok(&repo_path, &["new", "description(b)"]); test_env.add_config(r#"revset-aliases."immutable_heads()" = "main""#); @@ -131,8 +134,8 @@ fn test_rewrite_immutable_commands() { insta::assert_snapshot!(stdout, @r###" @ yqosqzyt test.user@example.com 2001-02-03 08:05:13 3f89addf │ (empty) (no description set) - │ ◉ mzvwutvl test.user@example.com 2001-02-03 08:05:11 main 406c181c conflict - ╭─┤ (empty) merge + │ ◉ mzvwutvl test.user@example.com 2001-02-03 08:05:12 main 3e025082 conflict + ╭─┤ merge │ │ │ ~ │ @@ -144,31 +147,31 @@ fn test_rewrite_immutable_commands() { // abandon let stderr = test_env.jj_cmd_failure(&repo_path, &["abandon", "main"]); insta::assert_snapshot!(stderr, @r###" - Error: Commit 406c181c04d8 is immutable + Error: Commit 3e0250828ca5 is immutable Hint: Pass `--ignore-immutable` or configure the set of immutable commits via `revset-aliases.immutable_heads()`. "###); // chmod let stderr = test_env.jj_cmd_failure(&repo_path, &["chmod", "-r=main", "x", "file"]); insta::assert_snapshot!(stderr, @r###" - Error: Commit 406c181c04d8 is immutable + Error: Commit 3e0250828ca5 is immutable Hint: Pass `--ignore-immutable` or configure the set of immutable commits via `revset-aliases.immutable_heads()`. "###); // describe let stderr = test_env.jj_cmd_failure(&repo_path, &["describe", "main"]); insta::assert_snapshot!(stderr, @r###" - Error: Commit 406c181c04d8 is immutable + Error: Commit 3e0250828ca5 is immutable Hint: Pass `--ignore-immutable` or configure the set of immutable commits via `revset-aliases.immutable_heads()`. "###); // diffedit let stderr = test_env.jj_cmd_failure(&repo_path, &["diffedit", "-r=main"]); insta::assert_snapshot!(stderr, @r###" - Error: Commit 406c181c04d8 is immutable + Error: Commit 3e0250828ca5 is immutable Hint: Pass `--ignore-immutable` or configure the set of immutable commits via `revset-aliases.immutable_heads()`. "###); // edit let stderr = test_env.jj_cmd_failure(&repo_path, &["edit", "main"]); insta::assert_snapshot!(stderr, @r###" - Error: Commit 406c181c04d8 is immutable + Error: Commit 3e0250828ca5 is immutable Hint: Pass `--ignore-immutable` or configure the set of immutable commits via `revset-aliases.immutable_heads()`. "###); // move --from @@ -176,7 +179,7 @@ fn test_rewrite_immutable_commands() { insta::assert_snapshot!(stderr, @r###" Warning: `jj move` is deprecated; use `jj squash` instead, which is equivalent Warning: `jj move` will be removed in a future version, and this will be a hard error - Error: Commit 406c181c04d8 is immutable + Error: Commit 3e0250828ca5 is immutable Hint: Pass `--ignore-immutable` or configure the set of immutable commits via `revset-aliases.immutable_heads()`. "###); // move --to @@ -184,31 +187,31 @@ fn test_rewrite_immutable_commands() { insta::assert_snapshot!(stderr, @r###" Warning: `jj move` is deprecated; use `jj squash` instead, which is equivalent Warning: `jj move` will be removed in a future version, and this will be a hard error - Error: Commit 406c181c04d8 is immutable + Error: Commit 3e0250828ca5 is immutable Hint: Pass `--ignore-immutable` or configure the set of immutable commits via `revset-aliases.immutable_heads()`. "###); // new --insert-before let stderr = test_env.jj_cmd_failure(&repo_path, &["new", "--insert-before", "main"]); insta::assert_snapshot!(stderr, @r###" - Error: Commit 406c181c04d8 is immutable + Error: Commit 3e0250828ca5 is immutable Hint: Pass `--ignore-immutable` or configure the set of immutable commits via `revset-aliases.immutable_heads()`. "###); // new --insert-after parent_of_main let stderr = test_env.jj_cmd_failure(&repo_path, &["new", "--insert-after", "description(b)"]); insta::assert_snapshot!(stderr, @r###" - Error: Commit 406c181c04d8 is immutable + Error: Commit 3e0250828ca5 is immutable Hint: Pass `--ignore-immutable` or configure the set of immutable commits via `revset-aliases.immutable_heads()`. "###); // parallelize let stderr = test_env.jj_cmd_failure(&repo_path, &["parallelize", "description(b)", "main"]); insta::assert_snapshot!(stderr, @r###" - Error: Commit 406c181c04d8 is immutable + Error: Commit 3e0250828ca5 is immutable Hint: Pass `--ignore-immutable` or configure the set of immutable commits via `revset-aliases.immutable_heads()`. "###); // rebase -s let stderr = test_env.jj_cmd_failure(&repo_path, &["rebase", "-s=main", "-d=@"]); insta::assert_snapshot!(stderr, @r###" - Error: Commit 406c181c04d8 is immutable + Error: Commit 3e0250828ca5 is immutable Hint: Pass `--ignore-immutable` or configure the set of immutable commits via `revset-aliases.immutable_heads()`. "###); // rebase -b @@ -220,31 +223,31 @@ fn test_rewrite_immutable_commands() { // rebase -r let stderr = test_env.jj_cmd_failure(&repo_path, &["rebase", "-r=main", "-d=@"]); insta::assert_snapshot!(stderr, @r###" - Error: Commit 406c181c04d8 is immutable + Error: Commit 3e0250828ca5 is immutable Hint: Pass `--ignore-immutable` or configure the set of immutable commits via `revset-aliases.immutable_heads()`. "###); // resolve let stderr = test_env.jj_cmd_failure(&repo_path, &["resolve", "-r=description(merge)", "file"]); insta::assert_snapshot!(stderr, @r###" - Error: Commit 406c181c04d8 is immutable + Error: Commit 3e0250828ca5 is immutable Hint: Pass `--ignore-immutable` or configure the set of immutable commits via `revset-aliases.immutable_heads()`. "###); // restore -c let stderr = test_env.jj_cmd_failure(&repo_path, &["restore", "-c=main"]); insta::assert_snapshot!(stderr, @r###" - Error: Commit 406c181c04d8 is immutable + Error: Commit 3e0250828ca5 is immutable Hint: Pass `--ignore-immutable` or configure the set of immutable commits via `revset-aliases.immutable_heads()`. "###); // restore --to let stderr = test_env.jj_cmd_failure(&repo_path, &["restore", "--to=main"]); insta::assert_snapshot!(stderr, @r###" - Error: Commit 406c181c04d8 is immutable + Error: Commit 3e0250828ca5 is immutable Hint: Pass `--ignore-immutable` or configure the set of immutable commits via `revset-aliases.immutable_heads()`. "###); // split let stderr = test_env.jj_cmd_failure(&repo_path, &["split", "-r=main"]); insta::assert_snapshot!(stderr, @r###" - Error: Commit 406c181c04d8 is immutable + Error: Commit 3e0250828ca5 is immutable Hint: Pass `--ignore-immutable` or configure the set of immutable commits via `revset-aliases.immutable_heads()`. "###); // squash -r @@ -256,19 +259,19 @@ fn test_rewrite_immutable_commands() { // squash --from let stderr = test_env.jj_cmd_failure(&repo_path, &["squash", "--from=main"]); insta::assert_snapshot!(stderr, @r###" - Error: Commit 406c181c04d8 is immutable + Error: Commit 3e0250828ca5 is immutable Hint: Pass `--ignore-immutable` or configure the set of immutable commits via `revset-aliases.immutable_heads()`. "###); // squash --into let stderr = test_env.jj_cmd_failure(&repo_path, &["squash", "--into=main"]); insta::assert_snapshot!(stderr, @r###" - Error: Commit 406c181c04d8 is immutable + Error: Commit 3e0250828ca5 is immutable Hint: Pass `--ignore-immutable` or configure the set of immutable commits via `revset-aliases.immutable_heads()`. "###); // unsquash let stderr = test_env.jj_cmd_failure(&repo_path, &["unsquash", "-r=main"]); insta::assert_snapshot!(stderr, @r###" - Error: Commit 406c181c04d8 is immutable + Error: Commit 3e0250828ca5 is immutable Hint: Pass `--ignore-immutable` or configure the set of immutable commits via `revset-aliases.immutable_heads()`. "###); } diff --git a/cli/tests/test_split_command.rs b/cli/tests/test_split_command.rs index 051fe24f7..8c9116f2c 100644 --- a/cli/tests/test_split_command.rs +++ b/cli/tests/test_split_command.rs @@ -523,3 +523,18 @@ fn test_split_siblings_with_merge_child() { ◉ zzzzzzzzzzzz true "###); } + +// Make sure `jj split` would refuse to split an empty commit. +#[test] +fn test_split_empty() { + let test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["init", "repo", "--git"]); + let workspace_path = test_env.env_root().join("repo"); + test_env.jj_cmd_ok(&workspace_path, &["describe", "--message", "abc"]); + + let stderr = test_env.jj_cmd_failure(&workspace_path, &["split"]); + insta::assert_snapshot!(stderr, @r###" + Error: Refusing to split empty commit 82b6292b775dc4e5c5e6f402faa599dad02d02a0. + Hint: Use `jj new` if you want to create another empty commit. + "###); +} diff --git a/lib/src/commit.rs b/lib/src/commit.rs index d1a8d202f..f8880471d 100644 --- a/lib/src/commit.rs +++ b/lib/src/commit.rs @@ -116,6 +116,16 @@ impl Commit { merge_commit_trees(repo, &self.parents()) } + /// Returns whether commit's content is empty. Commit description is not + /// taken into consideration. + pub fn is_empty(&self, repo: &dyn Repo) -> BackendResult { + if let [parent] = &self.parents()[..] { + return Ok(parent.tree_id() == self.tree_id()); + } + let parent_tree = self.parent_tree(repo)?; + Ok(*self.tree_id() == parent_tree.id()) + } + pub fn has_conflict(&self) -> BackendResult { if let MergedTreeId::Merge(tree_ids) = self.tree_id() { Ok(!tree_ids.is_resolved())