Skip to content

Commit

Permalink
Refuse to split an empty commit with jj split.
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
aspotashev committed May 10, 2024
1 parent 3a0929b commit c6a279f
Show file tree
Hide file tree
Showing 5 changed files with 39 additions and 8 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 11 additions & 1 deletion cli/src/commands/split.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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
Expand Down Expand Up @@ -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)?
Expand Down
8 changes: 1 addition & 7 deletions cli/src/commit_templater.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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| {
Expand Down
15 changes: 15 additions & 0 deletions cli/tests/test_split_command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
"###);
}
10 changes: 10 additions & 0 deletions lib/src/commit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<bool> {
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<bool> {
if let MergedTreeId::Merge(tree_ids) = self.tree_id() {
Ok(!tree_ids.is_resolved())
Expand Down

0 comments on commit c6a279f

Please sign in to comment.