Skip to content

Commit

Permalink
Fix empty files can't be selected in builtin diff editor
Browse files Browse the repository at this point in the history 
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 arxanas/scm-record#26 for the upstream bug.


Fixes #3016
  • Loading branch information
@emesterhazy
emesterhazy committed Apr 14, 2024
1 parent aaa2025 commit ef95706
Show file tree
Hide file tree
Showing 2 changed files with 314 additions and 18 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,13 @@ 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).

* 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
Expand Down
329 changes: 311 additions & 18 deletions cli/src/merge_tools/builtin.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<bool> {
if let FileContents::Absent = self.contents {
// If the contents are absent the mode should be absent as well.
debug_assert!(self.file_mode == scm_record::FileMode::absent())
}
match self.contents {
// Being absent is different from being present and empty.
FileContents::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.
///
Expand Down Expand Up @@ -237,6 +258,29 @@ 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 should exist on one side.
debug_assert!(left.file_mode != right.file_mode);
// 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 and the mode change is implied.
// See https://github.com/martinvonz/jj/issues/3016.
return matches!(
(left.is_empty(), right.is_empty()),
(Some(true), None) | (None, Some(true))
);
}
left.file_mode != right.file_mode
}

pub fn make_diff_files(
store: &Arc<Store>,
left_tree: &MergedTree,
Expand All @@ -245,29 +289,41 @@ pub fn make_diff_files(
) -> Result<Vec<scm_record::File<'static>>, 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
// doesn't exist on one side and is empty on the other, 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 {
Expand Down Expand Up @@ -356,7 +412,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,
});
}
Expand All @@ -380,7 +436,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.
// 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.
Expand Down Expand Up @@ -720,6 +802,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();
Expand Down

0 comments on commit ef95706

Please sign in to comment.