From 6b7cf298745b918632c04fe1de1881502dad4e8e 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 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 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 | 2 + cli/src/merge_tools/builtin.rs | 313 +++++++++++++++++++++++++++++++-- 2 files changed, 297 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a703a6dfe2e..7074ad8b660 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,8 @@ 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 works for correctly for empty files (#3016). + ## [0.16.0] - 2024-04-03 diff --git a/cli/src/merge_tools/builtin.rs b/cli/src/merge_tools/builtin.rs index 2c98abce160..3773a92cc5b 100644 --- a/cli/src/merge_tools/builtin.rs +++ b/cli/src/merge_tools/builtin.rs @@ -56,6 +56,21 @@ enum FileContents { }, } +impl FileContents { + fn is_empty(&self) -> bool { + match self { + // Being absent is different from being present and empty. + FileContents::Absent => false, + FileContents::Text { + contents: _, + hash: _, + num_bytes, + } => *num_bytes == 0, + FileContents::Binary { hash: _, num_bytes } => *num_bytes == 0, + } + } +} + /// Information about a file that was read from disk. Note that the file may not /// have existed, in which case its contents will be marked as absent. #[derive(Clone, Debug)] @@ -237,6 +252,24 @@ fn make_diff_sections( Ok(sections) } +// We should render the mode section if: +// 1. The file exists on only one side and is empty. +// 2. The file existed on both the left and right and the mode actually changed. +fn should_render_mode_section(left: &FileInfo, right: &FileInfo) -> bool { + if left.file_mode == scm_record::FileMode::absent() + || right.file_mode == scm_record::FileMode::absent() + { + // The file is absent on one side. If it's empty on the other side, then + // a content diff section won't be added later. If we don't add the mode + // section now, then the file won't be selectable at all. If the file + // isn't empty, then we don't need to add a mode section since the user + // can select the contents instead. + // See https://github.com/martinvonz/jj/issues/3016. + return left.contents.is_empty() || right.contents.is_empty(); + } + left.file_mode != right.file_mode +} + pub fn make_diff_files( store: &Arc, left_tree: &MergedTree, @@ -245,29 +278,36 @@ 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) => {} + ( + FileContents::Absent, + FileContents::Text { + contents: _, + hash: _, + num_bytes: 0, + }, + ) => {} + ( + FileContents::Text { + contents: _, + hash: _, + num_bytes: 0, + }, + FileContents::Absent, + ) => {} ( FileContents::Absent, FileContents::Text { @@ -356,7 +396,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 +420,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 empty. We need to + // disambiguate three cases: + // 1. The file is new. + // 2. The file existed 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 +786,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();