Skip to content

Commit

Permalink
Fix empty files can't be selected in the 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 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 arxanas/scm-record#26 for the upstream bug.


Fixes #3016
  • Loading branch information
@emesterhazy
emesterhazy committed Apr 16, 2024
1 parent 0bbebaf commit 1b41780
Show file tree
Hide file tree
Showing 2 changed files with 302 additions and 18 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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` ([#3016](https://github.com/martinvonz/jj/issues/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
316 changes: 298 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> {
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.
///
Expand Down Expand Up @@ -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<Store>,
left_tree: &MergedTree,
Expand All @@ -245,29 +277,40 @@ 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 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 {
Expand Down Expand Up @@ -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,
});
}
Expand All @@ -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.
Expand Down Expand Up @@ -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();
Expand Down

0 comments on commit 1b41780

Please sign in to comment.