From fc49d35daa61a8429520ca6e9c9982160473ea03 Mon Sep 17 00:00:00 2001 From: Evan Mesterhazy Date: Tue, 9 Apr 2024 13:53:21 -0400 Subject: [PATCH] Fix empty files can't be selected in the builtin diff editor This fixes several issues that made working with empty files difficult using the builtin diff editor. 1. The scm-record library used for the editor expects each file to have at least one section. For new empty files this should be a file mode section. jj wasn't rendering this mode section, which prevented empty files from being selected at all. 2. The scm-record library returns `SelectedContents::Absent` if the file has no contents after the diff editor is invoked. This can be because of several reasons: 1) the file is new and empty; 2) the file was empty before and is still empty; 3) the file has been deleted. Perhaps this is a bug in scm-record and it should return `SelectedContents::Unchanged` or `SelectedContents::Present` if the file hasn't been deleted. Until this is patched upstream, we can work around it by disambiguating these cases. See https://github.com/arxanas/scm-record/issues/26 for the upstream bug. Fixes #3016 --- CHANGELOG.md | 4 + cli/src/merge_tools/builtin.rs | 316 +++++++++++++++++++++++++++++++-- 2 files changed, 302 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ccef85851..f1a673d3d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,10 +52,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * Revsets now support `\`-escapes in string literal. +* The builtin diff editor now allows empty files to be selected during + `jj split`. + * Fixed a bug with `jj split` introduced in 0.16.0 that caused it to incorrectly rebase the children of the revision being split if they had other parents (i.e. if the child was a merge). + ## [0.16.0] - 2024-04-03 ### Deprecations diff --git a/cli/src/merge_tools/builtin.rs b/cli/src/merge_tools/builtin.rs index 2c98abce1..673fc7e0f 100644 --- a/cli/src/merge_tools/builtin.rs +++ b/cli/src/merge_tools/builtin.rs @@ -64,6 +64,27 @@ pub struct FileInfo { contents: FileContents, } +impl FileInfo { + // Returns `Some(true)` if the file exists and is empty. + // Returns `None` if the file is absent and does not exist. + fn is_empty(&self) -> Option { + match self.contents { + FileContents::Absent => { + // If the contents are `Absent` the mode should be `Absent` as + // well since the file doesn't exist. + debug_assert!(self.file_mode == scm_record::FileMode::absent()); + None + } + FileContents::Text { + contents: _, + hash: _, + num_bytes, + } + | FileContents::Binary { hash: _, num_bytes } => Some(num_bytes == 0), + } + } +} + /// File modes according to the Git file mode conventions. used for display /// purposes and equality comparison. /// @@ -237,6 +258,17 @@ fn make_diff_sections( Ok(sections) } +fn should_render_mode_section(left: &FileInfo, right: &FileInfo) -> bool { + match (left.is_empty(), right.is_empty()) { + // The file only exists on one side, but it's not empty on the other + // side. We'll render a section for the change in content, so the mode + // change is implied and mandatory if the user selects the change in + // content. + (Some(false), None) | (None, Some(false)) => false, + _ => left.file_mode != right.file_mode, + } +} + pub fn make_diff_files( store: &Arc, left_tree: &MergedTree, @@ -245,29 +277,40 @@ pub fn make_diff_files( ) -> Result>, BuiltinToolError> { let mut files = Vec::new(); for changed_path in changed_files { - let FileInfo { - file_mode: left_file_mode, - contents: left_contents, - } = read_file_contents(store, left_tree, changed_path)?; - let FileInfo { - file_mode: right_file_mode, - contents: right_contents, - } = read_file_contents(store, right_tree, changed_path)?; - + let left_info = read_file_contents(store, left_tree, changed_path)?; + let right_info = read_file_contents(store, right_tree, changed_path)?; let mut sections = Vec::new(); - if left_file_mode != right_file_mode - && left_file_mode != scm_record::FileMode::absent() - && right_file_mode != scm_record::FileMode::absent() - { + + if should_render_mode_section(&left_info, &right_info) { sections.push(scm_record::Section::FileMode { is_checked: false, - before: left_file_mode, - after: right_file_mode, + before: left_info.file_mode, + after: right_info.file_mode, }); } - match (left_contents, right_contents) { + match (left_info.contents, right_info.contents) { (FileContents::Absent, FileContents::Absent) => {} + // In this context, `Absent` means the file doesn't exist. If it only + // exists on one side, we will render a mode change section above. + // The next two patterns are to avoid also rendering an empty + // changed lines section that clutters the UI. + ( + FileContents::Absent, + FileContents::Text { + contents: _, + hash: _, + num_bytes: 0, + }, + ) => {} + ( + FileContents::Text { + contents: _, + hash: _, + num_bytes: 0, + }, + FileContents::Absent, + ) => {} ( FileContents::Absent, FileContents::Text { @@ -356,7 +399,7 @@ pub fn make_diff_files( files.push(scm_record::File { old_path: None, path: Cow::Owned(changed_path.to_fs_path(Path::new(""))), - file_mode: Some(left_file_mode), + file_mode: Some(left_info.file_mode), sections, }); } @@ -380,7 +423,33 @@ pub fn apply_diff_builtin( let (selected, _unselected) = file.get_selected_contents(); match selected { scm_record::SelectedContents::Absent => { - tree_builder.set_or_remove(path, Merge::absent()); + // TODO(https://github.com/arxanas/scm-record/issues/26): This + // is probably an upstream bug in scm-record. If the file exists + // but is empty, `get_selected_contents` should probably return + // `Unchanged` or `Present`. When this is fixed upstream we can + // simplify this logic. + + // Currently, `Absent` means the file is either empty or + // deleted. We need to disambiguate three cases: + // 1. The file is new and empty. + // 2. The file existed before, is empty, and nothing changed. + // 3. The file does not exist (it's been deleted). + let old_mode = file.file_mode; + let new_mode = file.get_file_mode(); + let file_existed_previously = + old_mode.is_some() && old_mode != Some(scm_record::FileMode::absent()); + let file_exists_now = + new_mode.is_some() && new_mode != Some(scm_record::FileMode::absent()); + let new_empty_file = !file_existed_previously && file_exists_now; + let file_deleted = file_existed_previously && !file_exists_now; + + if new_empty_file { + let value = right_tree.path_value(&path); + tree_builder.set_or_remove(path, value); + } else if file_deleted { + tree_builder.set_or_remove(path, Merge::absent()); + } + // Else: the file is empty and nothing changed. } scm_record::SelectedContents::Unchanged => { // Do nothing. @@ -720,6 +789,217 @@ mod tests { ); } + #[test] + fn test_edit_diff_builtin_add_empty_file() { + let test_repo = TestRepo::init(); + let store = test_repo.repo.store(); + + let added_empty_file_path = RepoPath::from_internal_string("empty_file"); + let left_tree = testutils::create_tree(&test_repo.repo, &[]); + let right_tree = testutils::create_tree(&test_repo.repo, &[(added_empty_file_path, "")]); + + let changed_files = vec![added_empty_file_path.to_owned()]; + let files = make_diff_files(store, &left_tree, &right_tree, &changed_files).unwrap(); + insta::assert_debug_snapshot!(files, @r###" + [ + File { + old_path: None, + path: "empty_file", + file_mode: Some( + FileMode( + 0, + ), + ), + sections: [ + FileMode { + is_checked: false, + before: FileMode( + 0, + ), + after: FileMode( + 33188, + ), + }, + ], + }, + ] + "###); + let no_changes_tree_id = apply_diff_builtin( + store.clone(), + &left_tree, + &right_tree, + changed_files.clone(), + &files, + ) + .unwrap(); + let no_changes_tree = store.get_root_tree(&no_changes_tree_id).unwrap(); + assert_eq!( + no_changes_tree.id(), + left_tree.id(), + "no-changes tree was different", + ); + + let mut files = files; + for file in files.iter_mut() { + file.toggle_all(); + } + let all_changes_tree_id = apply_diff_builtin( + store.clone(), + &left_tree, + &right_tree, + changed_files, + &files, + ) + .unwrap(); + let all_changes_tree = store.get_root_tree(&all_changes_tree_id).unwrap(); + assert_eq!( + all_changes_tree.id(), + right_tree.id(), + "all-changes tree was different", + ); + } + + #[test] + fn test_edit_diff_builtin_delete_empty_file() { + let test_repo = TestRepo::init(); + let store = test_repo.repo.store(); + + let added_empty_file_path = RepoPath::from_internal_string("empty_file"); + let left_tree = testutils::create_tree(&test_repo.repo, &[(added_empty_file_path, "")]); + let right_tree = testutils::create_tree(&test_repo.repo, &[]); + + let changed_files = vec![added_empty_file_path.to_owned()]; + let files = make_diff_files(store, &left_tree, &right_tree, &changed_files).unwrap(); + insta::assert_debug_snapshot!(files, @r###" + [ + File { + old_path: None, + path: "empty_file", + file_mode: Some( + FileMode( + 33188, + ), + ), + sections: [ + FileMode { + is_checked: false, + before: FileMode( + 33188, + ), + after: FileMode( + 0, + ), + }, + ], + }, + ] + "###); + let no_changes_tree_id = apply_diff_builtin( + store.clone(), + &left_tree, + &right_tree, + changed_files.clone(), + &files, + ) + .unwrap(); + let no_changes_tree = store.get_root_tree(&no_changes_tree_id).unwrap(); + assert_eq!( + no_changes_tree.id(), + left_tree.id(), + "no-changes tree was different", + ); + + let mut files = files; + for file in files.iter_mut() { + file.toggle_all(); + } + let all_changes_tree_id = apply_diff_builtin( + store.clone(), + &left_tree, + &right_tree, + changed_files, + &files, + ) + .unwrap(); + let all_changes_tree = store.get_root_tree(&all_changes_tree_id).unwrap(); + assert_eq!( + all_changes_tree.id(), + right_tree.id(), + "all-changes tree was different", + ); + } + + #[test] + fn test_edit_diff_builtin_modify_empty_file() { + let test_repo = TestRepo::init(); + let store = test_repo.repo.store(); + + let empty_file_path = RepoPath::from_internal_string("empty_file"); + let left_tree = testutils::create_tree(&test_repo.repo, &[(empty_file_path, "")]); + let right_tree = + testutils::create_tree(&test_repo.repo, &[(empty_file_path, "modified\n")]); + + let changed_files = vec![empty_file_path.to_owned()]; + let files = make_diff_files(store, &left_tree, &right_tree, &changed_files).unwrap(); + insta::assert_debug_snapshot!(files, @r###" + [ + File { + old_path: None, + path: "empty_file", + file_mode: Some( + FileMode( + 33188, + ), + ), + sections: [ + Changed { + lines: [ + SectionChangedLine { + is_checked: false, + change_type: Added, + line: "modified\n", + }, + ], + }, + ], + }, + ] + "###); + let no_changes_tree_id = apply_diff_builtin( + store.clone(), + &left_tree, + &right_tree, + changed_files.clone(), + &files, + ) + .unwrap(); + let no_changes_tree = store.get_root_tree(&no_changes_tree_id).unwrap(); + assert_eq!( + no_changes_tree.id(), + left_tree.id(), + "no-changes tree was different", + ); + + let mut files = files; + for file in files.iter_mut() { + file.toggle_all(); + } + let all_changes_tree_id = apply_diff_builtin( + store.clone(), + &left_tree, + &right_tree, + changed_files, + &files, + ) + .unwrap(); + let all_changes_tree = store.get_root_tree(&all_changes_tree_id).unwrap(); + assert_eq!( + all_changes_tree.id(), + right_tree.id(), + "all-changes tree was different", + ); + } + #[test] fn test_make_merge_sections() { let test_repo = TestRepo::init();