From e22b6f09f56a31f13363f7d4f7fdad977448a2d3 Mon Sep 17 00:00:00 2001 From: Jonathan Gilchrist Date: Sun, 26 Jan 2025 18:10:12 +0000 Subject: [PATCH] Allow file deletion in diffedit and split This uses some code from https://github.com/jj-vcs/jj/pull/4078 but has a slightly different underlying approach based on https://github.com/arxanas/scm-record/pull/62#issuecomment-2241768677. --- cli/src/merge_tools/builtin.rs | 480 +++++++++++++++++++++++++++------ cli/src/merge_tools/mod.rs | 4 +- 2 files changed, 394 insertions(+), 90 deletions(-) diff --git a/cli/src/merge_tools/builtin.rs b/cli/src/merge_tools/builtin.rs index b545965d6a..0152a960f2 100644 --- a/cli/src/merge_tools/builtin.rs +++ b/cli/src/merge_tools/builtin.rs @@ -440,7 +440,6 @@ pub fn make_diff_files( old_path: None, // Path for displaying purposes, not for file access. path: Cow::Owned(changed_path.to_fs_path_unchecked(Path::new(""))), - file_mode: Some(left_info.file_mode), sections, }); } @@ -453,6 +452,7 @@ pub fn apply_diff_builtin( right_tree: &MergedTree, changed_files: Vec, files: &[scm_record::File], + conflict_marker_style: ConflictMarkerStyle, ) -> BackendResult { let mut tree_builder = MergedTreeBuilder::new(left_tree.id().clone()); assert_eq!( @@ -464,57 +464,117 @@ pub fn apply_diff_builtin( for (path, file) in changed_files.into_iter().zip(files) { let (selected, _unselected) = file.get_selected_contents(); match selected { - scm_record::SelectedContents::Absent => { + scm_record::FileState::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 { + // If we get Absent back from scm-record, in most cases we can just keep the + // changes in the left tree. + // However, there are two cases in which a user can select changes in scm-record + // but we get Absent back. We need to determine if we're in either of these two + // cases to make sure we use the contents from the right tree. + + // First, we determine if we're in one of these cases. + + let left_info = + read_file_contents(store, left_tree, &path, conflict_marker_style).unwrap(); + let file_existed_previously = left_info.file_mode != scm_record::FileMode::absent(); + let file_was_empty_previously = file_existed_previously + && matches!(left_info.contents, FileContents::Text { num_bytes: 0, .. }); + + let non_empty_file_was_modified = + file_existed_previously && !file_was_empty_previously; + + // If a non-empty file was not made to look empty, then we're OK to use the + // state from the left tree as the changes have been discarded. + if !non_empty_file_was_modified { + continue; + } + + // If we got this far, we had a non-empty file previously which now looks empty + // and the user selected those changes. + // To know what to do, we need to look at whether the file exists now or not. + + let right_info = + read_file_contents(store, right_tree, &path, conflict_marker_style).unwrap(); + let file_exists_now = right_info.file_mode != scm_record::FileMode::absent(); + + // If the file looks empty, but it still exists, then it is empty. + if file_exists_now { let value = right_tree.path_value(&path)?; tree_builder.set_or_remove(path, value); - } else if file_deleted { + } else { + // If the file looks empty and doesn't exist, then it was deleted. tree_builder.set_or_remove(path, Merge::absent()); } - // Else: the file is empty and nothing changed. } - scm_record::SelectedContents::Unchanged => { - // Do nothing. - } - scm_record::SelectedContents::Binary { - old_description: _, - new_description: _, + scm_record::FileState::Present { + contents, + mode_transition, } => { - let value = right_tree.path_value(&path)?; - tree_builder.set_or_remove(path, value); - } - scm_record::SelectedContents::Present { contents } => { - let file_id = store - .write_file(&path, &mut contents.as_bytes()) - .block_on()?; - tree_builder.set_or_remove( - path, - Merge::normal(TreeValue::File { - id: file_id, - executable: file.get_file_mode() - == Some(scm_record::FileMode(mode::EXECUTABLE)), - }), - ); + // If we operated on an empty file, then we don't show the user any file + // contents, only a mode transition (e.g. from 0 to x) for a + // creation. + // + // If one of these mode transitions was selected, we need to handle the + // corresponding creation or deletion. + if let Some(ref mode_transition) = mode_transition { + // If an empty file was deleted, remove it. + if mode_transition.is_deletion() { + tree_builder.set_or_remove(path, Merge::absent()); + continue; + } + + // If an empty file was created, create it. scm-record refers to an empty file + // as contents == Unchanged. A created file with text is + // handled below as contents == Text. + if mode_transition.is_creation() + && contents == scm_record::SelectedContents::Unchanged + { + let value = right_tree.path_value(&path)?; + tree_builder.set_or_remove(path, value); + continue; + } + } + + match contents { + scm_record::SelectedContents::Unchanged => { + // Do nothing. + } + scm_record::SelectedContents::Binary { + old_description: _, + new_description: _, + } => { + let value = right_tree.path_value(&path)?; + tree_builder.set_or_remove(path, value); + } + scm_record::SelectedContents::Text { contents } => { + let file_id = store + .write_file(&path, &mut contents.as_bytes()) + .block_on()?; + + let executable = match mode_transition { + None => match right_tree.path_value(&path)?.as_resolved() { + Some(Some(TreeValue::File { executable, .. })) => *executable, + _ => false, + }, + Some(transition) => { + transition.after == scm_record::FileMode(mode::EXECUTABLE) + } + }; + + tree_builder.set_or_remove( + path, + Merge::normal(TreeValue::File { + id: file_id, + executable, + }), + ); + } + } } } } @@ -553,8 +613,15 @@ pub fn edit_diff_builtin( &mut input, ); let result = recorder.run().map_err(BuiltinToolError::Record)?; - let tree_id = apply_diff_builtin(&store, left_tree, right_tree, changed_files, &result.files) - .map_err(BuiltinToolError::BackendError)?; + let tree_id = apply_diff_builtin( + &store, + left_tree, + right_tree, + changed_files, + &result.files, + conflict_marker_style, + ) + .map_err(BuiltinToolError::BackendError)?; Ok(tree_id) } @@ -651,7 +718,6 @@ fn make_merge_file( .repo_path .to_fs_path_unchecked(Path::new("")), ), - file_mode: None, sections, }) } @@ -659,6 +725,7 @@ fn make_merge_file( pub fn edit_merge_builtin( tree: &MergedTree, merge_tool_files: &[MergeToolFile], + conflict_marker_style: ConflictMarkerStyle, ) -> Result { let mut input = scm_record::helpers::CrosstermInput; let recorder = scm_record::Recorder::new( @@ -680,6 +747,7 @@ pub fn edit_merge_builtin( .map(|file| file.repo_path.clone()) .collect_vec(), &state.files, + conflict_marker_style, ) .map_err(BuiltinToolError::BackendError) } @@ -733,16 +801,11 @@ mod tests { ConflictMarkerStyle::Diff, ) .unwrap(); - insta::assert_debug_snapshot!(files, @r###" + insta::assert_debug_snapshot!(files, @r#" [ File { old_path: None, path: "unchanged", - file_mode: Some( - FileMode( - 33188, - ), - ), sections: [ Unchanged { lines: [ @@ -754,11 +817,6 @@ mod tests { File { old_path: None, path: "changed", - file_mode: Some( - FileMode( - 33188, - ), - ), sections: [ Unchanged { lines: [ @@ -803,11 +861,6 @@ mod tests { File { old_path: None, path: "added", - file_mode: Some( - FileMode( - 0, - ), - ), sections: [ Changed { lines: [ @@ -821,7 +874,7 @@ mod tests { ], }, ] - "###); + "#); let no_changes_tree_id = apply_diff_builtin( store, @@ -829,6 +882,7 @@ mod tests { &right_tree, changed_files.clone(), &files, + ConflictMarkerStyle::Diff, ) .unwrap(); let no_changes_tree = store.get_root_tree(&no_changes_tree_id).unwrap(); @@ -842,8 +896,15 @@ mod tests { for file in &mut files { file.toggle_all(); } - let all_changes_tree_id = - apply_diff_builtin(store, &left_tree, &right_tree, changed_files, &files).unwrap(); + let all_changes_tree_id = apply_diff_builtin( + store, + &left_tree, + &right_tree, + changed_files, + &files, + ConflictMarkerStyle::Diff, + ) + .unwrap(); let all_changes_tree = store.get_root_tree(&all_changes_tree_id).unwrap(); assert_eq!( all_changes_tree.id(), @@ -870,16 +931,11 @@ mod tests { ConflictMarkerStyle::Diff, ) .unwrap(); - insta::assert_debug_snapshot!(files, @r###" + insta::assert_debug_snapshot!(files, @r#" [ File { old_path: None, path: "empty_file", - file_mode: Some( - FileMode( - 0, - ), - ), sections: [ FileMode { is_checked: false, @@ -893,13 +949,98 @@ mod tests { ], }, ] - "###); + "#); + let no_changes_tree_id = apply_diff_builtin( + store, + &left_tree, + &right_tree, + changed_files.clone(), + &files, + ConflictMarkerStyle::Diff, + ) + .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 &mut files { + file.toggle_all(); + } + let all_changes_tree_id = apply_diff_builtin( + store, + &left_tree, + &right_tree, + changed_files, + &files, + ConflictMarkerStyle::Diff, + ) + .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_add_executable_file() { + let test_repo = TestRepo::init(); + let store = test_repo.repo.store(); + + let added_executable_file_path = RepoPath::from_internal_string("executable_file"); + let left_tree = testutils::create_tree(&test_repo.repo, &[]); + let right_tree = { + let store = test_repo.repo.store(); + let mut tree_builder = store.tree_builder(store.empty_tree_id().clone()); + testutils::write_executable_file( + &mut tree_builder, + added_executable_file_path, + "executable", + ); + let id = tree_builder.write_tree().unwrap(); + MergedTree::resolved(store.get_tree(RepoPathBuf::root(), &id).unwrap()) + }; + + let changed_files = vec![added_executable_file_path.to_owned()]; + let files = make_diff_files( + store, + &left_tree, + &right_tree, + &changed_files, + ConflictMarkerStyle::Diff, + ) + .unwrap(); + insta::assert_debug_snapshot!(files, @r#" + [ + File { + old_path: None, + path: "executable_file", + sections: [ + Changed { + lines: [ + SectionChangedLine { + is_checked: false, + change_type: Added, + line: "executable", + }, + ], + }, + ], + }, + ] + "#); let no_changes_tree_id = apply_diff_builtin( store, &left_tree, &right_tree, changed_files.clone(), &files, + ConflictMarkerStyle::Diff, ) .unwrap(); let no_changes_tree = store.get_root_tree(&no_changes_tree_id).unwrap(); @@ -913,8 +1054,89 @@ mod tests { for file in &mut files { file.toggle_all(); } - let all_changes_tree_id = - apply_diff_builtin(store, &left_tree, &right_tree, changed_files, &files).unwrap(); + let all_changes_tree_id = apply_diff_builtin( + store, + &left_tree, + &right_tree, + changed_files, + &files, + ConflictMarkerStyle::Diff, + ) + .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_file() { + let test_repo = TestRepo::init(); + let store = test_repo.repo.store(); + + let file_path = RepoPath::from_internal_string("file_with_content"); + let left_tree = testutils::create_tree(&test_repo.repo, &[(file_path, "content\n")]); + let right_tree = testutils::create_tree(&test_repo.repo, &[]); + + let changed_files = vec![file_path.to_owned()]; + let files = make_diff_files( + store, + &left_tree, + &right_tree, + &changed_files, + ConflictMarkerStyle::Diff, + ) + .unwrap(); + insta::assert_debug_snapshot!(files, @r#" + [ + File { + old_path: None, + path: "file_with_content", + sections: [ + Changed { + lines: [ + SectionChangedLine { + is_checked: false, + change_type: Removed, + line: "content\n", + }, + ], + }, + ], + }, + ] + "#); + let no_changes_tree_id = apply_diff_builtin( + store, + &left_tree, + &right_tree, + changed_files.clone(), + &files, + ConflictMarkerStyle::Diff, + ) + .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 &mut files { + file.toggle_all(); + } + let all_changes_tree_id = apply_diff_builtin( + store, + &left_tree, + &right_tree, + changed_files, + &files, + ConflictMarkerStyle::Diff, + ) + .unwrap(); let all_changes_tree = store.get_root_tree(&all_changes_tree_id).unwrap(); assert_eq!( all_changes_tree.id(), @@ -941,16 +1163,11 @@ mod tests { ConflictMarkerStyle::Diff, ) .unwrap(); - insta::assert_debug_snapshot!(files, @r###" + insta::assert_debug_snapshot!(files, @r#" [ File { old_path: None, path: "empty_file", - file_mode: Some( - FileMode( - 33188, - ), - ), sections: [ FileMode { is_checked: false, @@ -964,13 +1181,14 @@ mod tests { ], }, ] - "###); + "#); let no_changes_tree_id = apply_diff_builtin( store, &left_tree, &right_tree, changed_files.clone(), &files, + ConflictMarkerStyle::Diff, ) .unwrap(); let no_changes_tree = store.get_root_tree(&no_changes_tree_id).unwrap(); @@ -984,8 +1202,15 @@ mod tests { for file in &mut files { file.toggle_all(); } - let all_changes_tree_id = - apply_diff_builtin(store, &left_tree, &right_tree, changed_files, &files).unwrap(); + let all_changes_tree_id = apply_diff_builtin( + store, + &left_tree, + &right_tree, + changed_files, + &files, + ConflictMarkerStyle::Diff, + ) + .unwrap(); let all_changes_tree = store.get_root_tree(&all_changes_tree_id).unwrap(); assert_eq!( all_changes_tree.id(), @@ -1013,16 +1238,11 @@ mod tests { ConflictMarkerStyle::Diff, ) .unwrap(); - insta::assert_debug_snapshot!(files, @r###" + insta::assert_debug_snapshot!(files, @r#" [ File { old_path: None, path: "empty_file", - file_mode: Some( - FileMode( - 33188, - ), - ), sections: [ Changed { lines: [ @@ -1036,13 +1256,14 @@ mod tests { ], }, ] - "###); + "#); let no_changes_tree_id = apply_diff_builtin( store, &left_tree, &right_tree, changed_files.clone(), &files, + ConflictMarkerStyle::Diff, ) .unwrap(); let no_changes_tree = store.get_root_tree(&no_changes_tree_id).unwrap(); @@ -1056,8 +1277,89 @@ mod tests { for file in &mut files { file.toggle_all(); } - let all_changes_tree_id = - apply_diff_builtin(store, &left_tree, &right_tree, changed_files, &files).unwrap(); + let all_changes_tree_id = apply_diff_builtin( + store, + &left_tree, + &right_tree, + changed_files, + &files, + ConflictMarkerStyle::Diff, + ) + .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_make_file_empty() { + let test_repo = TestRepo::init(); + let store = test_repo.repo.store(); + + let file_path = RepoPath::from_internal_string("file_with_content"); + let left_tree = testutils::create_tree(&test_repo.repo, &[(file_path, "content\n")]); + let right_tree = testutils::create_tree(&test_repo.repo, &[(file_path, "")]); + + let changed_files = vec![file_path.to_owned()]; + let files = make_diff_files( + store, + &left_tree, + &right_tree, + &changed_files, + ConflictMarkerStyle::Diff, + ) + .unwrap(); + insta::assert_debug_snapshot!(files, @r#" + [ + File { + old_path: None, + path: "file_with_content", + sections: [ + Changed { + lines: [ + SectionChangedLine { + is_checked: false, + change_type: Removed, + line: "content\n", + }, + ], + }, + ], + }, + ] + "#); + let no_changes_tree_id = apply_diff_builtin( + store, + &left_tree, + &right_tree, + changed_files.clone(), + &files, + ConflictMarkerStyle::Diff, + ) + .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 &mut files { + file.toggle_all(); + } + let all_changes_tree_id = apply_diff_builtin( + store, + &left_tree, + &right_tree, + changed_files, + &files, + ConflictMarkerStyle::Diff, + ) + .unwrap(); let all_changes_tree = store.get_root_tree(&all_changes_tree_id).unwrap(); assert_eq!( all_changes_tree.id(), diff --git a/cli/src/merge_tools/mod.rs b/cli/src/merge_tools/mod.rs index d017bf90bb..2928c9e604 100644 --- a/cli/src/merge_tools/mod.rs +++ b/cli/src/merge_tools/mod.rs @@ -391,7 +391,9 @@ impl MergeEditor { match &self.tool { MergeTool::Builtin => { - let tree_id = edit_merge_builtin(tree, &merge_tool_files).map_err(Box::new)?; + let tree_id = + edit_merge_builtin(tree, &merge_tool_files, self.conflict_marker_style) + .map_err(Box::new)?; Ok((tree_id, None)) } MergeTool::External(editor) => external::run_mergetool_external(