ok/jj
1
0
Fork 0
forked from mirrors/jj
Code Pull requests Activity

conflicts: make update_from_content() work with only FileIds

Browse source 
Since `update_from_contents()` only works with file contents and not
the executable or other kinds of paths, I think it makes more sense
for it to deal with `FileId`s instead of `TreeValue`s.
This commit is contained in:
Martin von Zweigbergk 2023-08-10 22:06:38 -07:00 committed by Martin von Zweigbergk
parent 94c14d454a
commit 0b85f06e3d
5 changed files with 102 additions and 102 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

5
cli/src/merge_tools.rs
View file

@ -325,12 +325,13 @@ pub fn run_mergetool(
let mut new_tree_value: Option<TreeValue> = None;
if editor.merge_tool_edits_conflict_markers {
if let Some(new_conflict) = conflicts::update_from_content(
&conflict,
if let Some(new_file_ids) = conflicts::update_from_content(
&file_merge,
tree.store(),
repo_path,
output_file_contents.as_slice(),
)? {
let new_conflict = conflict.with_new_file_ids(&new_file_ids);
let new_conflict_id = tree.store().write_conflict(repo_path, &new_conflict)?;
new_tree_value = Some(TreeValue::Conflict(new_conflict_id));
}

47
lib/src/conflicts.rs
View file

@ -296,29 +296,26 @@ fn parse_conflict_hunk(input: &[u8]) -> Merge<ContentHunk> {
/// Returns `None` if there are no conflict markers in `content`.
pub fn update_from_content(
conflict: &Merge<Option<TreeValue>>,
file_ids: &Merge<Option<FileId>>,
store: &Store,
path: &RepoPath,
content: &[u8],
) -> BackendResult<Option<Merge<Option<TreeValue>>>> {
// TODO: Check that the conflict only involves files and convert it to a
// `Merge<Option<FileId>>` so we can remove the wildcard pattern in the loops
// further down.
) -> BackendResult<Option<Merge<Option<FileId>>>> {
// First check if the new content is unchanged compared to the old content. If
// it is, we don't need parse the content or write any new objects to the
// store. This is also a way of making sure that unchanged tree/file
// conflicts (for example) are not converted to regular files in the working
// copy.
let mut old_content = Vec::with_capacity(content.len());
materialize(conflict, store, path, &mut old_content).unwrap();
let merge_hunk = extract_as_single_hunk(file_ids, store, path);
materialize_merge_result(&merge_hunk, &mut old_content).unwrap();
if content == old_content {
return Ok(Some(conflict.clone()));
return Ok(Some(file_ids.clone()));
}
let mut removed_content = vec![vec![]; conflict.removes().len()];
let mut added_content = vec![vec![]; conflict.adds().len()];
let Some(hunks) = parse_conflict(content, conflict.removes().len(), conflict.adds().len())
let mut removed_content = vec![vec![]; file_ids.removes().len()];
let mut added_content = vec![vec![]; file_ids.adds().len()];
let Some(hunks) = parse_conflict(content, file_ids.removes().len(), file_ids.adds().len())
else {
// Either there are no self markers of they don't have the expected arity
return Ok(None);
@ -346,14 +343,10 @@ pub fn update_from_content(
// FileIds.
let mut new_removes = vec![];
for (i, buf) in removed_content.iter().enumerate() {
match &conflict.removes()[i] {
Some(TreeValue::File { id: _, executable }) => {
match &file_ids.removes()[i] {
Some(_) => {
let file_id = store.write_file(path, &mut buf.as_slice())?;
let new_value = TreeValue::File {
id: file_id,
executable: *executable,
};
new_removes.push(Some(new_value));
new_removes.push(Some(file_id));
}
None if buf.is_empty() => {
// The missing side of a conflict is still represented by
@ -361,22 +354,18 @@ pub fn update_from_content(
new_removes.push(None);
}
_ => {
// The user edited a non-file side. This should never happen. We consider the
// conflict resolved for now.
// The user edited the empty placeholder for an absent side. We consider the
// conflict resolved.
return Ok(None);
}
}
}
let mut new_adds = vec![];
for (i, buf) in added_content.iter().enumerate() {
match &conflict.adds()[i] {
Some(TreeValue::File { id: _, executable }) => {
match &file_ids.adds()[i] {
Some(_) => {
let file_id = store.write_file(path, &mut buf.as_slice())?;
let new_value = TreeValue::File {
id: file_id,
executable: *executable,
};
new_adds.push(Some(new_value));
new_adds.push(Some(file_id));
}
None if buf.is_empty() => {
// The missing side of a conflict is still represented by
@ -384,8 +373,8 @@ pub fn update_from_content(
new_adds.push(None);
}
_ => {
// The user edited a non-file side. This should never happen. We consider the
// conflict resolved for now.
// The user edited the empty placeholder for an absent side. We consider the
// conflict resolved.
return Ok(None);
}
}

29
lib/src/merge.rs
View file

@ -18,6 +18,7 @@ use std::borrow::Borrow;
use std::collections::HashMap;
use std::hash::Hash;
use std::io::Write;
use std::iter::zip;
use std::sync::Arc;
use itertools::Itertools;
@ -346,6 +347,34 @@ impl Merge<Option<TreeValue>> {
})
}
/// Creates a new merge with the file ids from the given merge. In other
/// words, only the executable bits from `self` will be preserved.
pub fn with_new_file_ids(&self, file_ids: &Merge<Option<FileId>>) -> Self {
assert_eq!(self.removes.len(), file_ids.removes.len());
fn updated_terms(
old_tree_values: &[Option<TreeValue>],
file_ids: &[Option<FileId>],
) -> Vec<Option<TreeValue>> {
let mut new_tree_values = vec![];
for (tree_value, file_id) in zip(old_tree_values, file_ids) {
if let Some(TreeValue::File { id: _, executable }) = tree_value {
new_tree_values.push(Some(TreeValue::File {
id: file_id.as_ref().unwrap().clone(),
executable: *executable,
}));
} else {
assert!(tree_value.is_none());
assert!(file_id.is_none());
new_tree_values.push(None);
}
}
new_tree_values
}
let removes = updated_terms(&self.removes, &file_ids.removes);
let adds = updated_terms(&self.adds, &file_ids.adds);
Merge { removes, adds }
}
/// Give a summary description of the conflict's "removes" and "adds"
pub fn describe(&self, file: &mut dyn Write) -> std::io::Result<()> {
file.write_all(b"Conflict:\n")?;

29
lib/src/working_copy.rs
View file

@ -968,19 +968,28 @@ impl TreeState {
let mut content = vec![];
file.read_to_end(&mut content).unwrap();
let conflict = self.store.read_conflict(repo_path, &conflict_id)?;
if let Some(new_conflict) =
conflicts::update_from_content(&conflict, self.store.as_ref(), repo_path, &content)
.unwrap()
{
if new_conflict != conflict {
let new_conflict_id = self.store.write_conflict(repo_path, &new_conflict)?;
Ok(TreeValue::Conflict(new_conflict_id))
if let Some(old_file_ids) = conflict.to_file_merge() {
if let Some(new_file_ids) = conflicts::update_from_content(
&old_file_ids,
self.store.as_ref(),
repo_path,
&content,
)
.unwrap()
{
if new_file_ids != old_file_ids {
let new_conflict = conflict.with_new_file_ids(&new_file_ids);
let new_conflict_id = self.store.write_conflict(repo_path, &new_conflict)?;
Ok(TreeValue::Conflict(new_conflict_id))
} else {
Ok(TreeValue::Conflict(conflict_id))
}
} else {
Ok(TreeValue::Conflict(conflict_id))
let id = self.store.write_file(repo_path, &mut content.as_slice())?;
Ok(TreeValue::File { id, executable })
}
} else {
let id = self.store.write_file(repo_path, &mut content.as_slice())?;
Ok(TreeValue::File { id, executable })
Ok(TreeValue::Conflict(conflict_id))
}
}

94
lib/tests/test_conflicts.rs
View file

@ -12,21 +12,16 @@
// See the License for the specific language governing permissions and
// limitations under the License.
use jj_lib::backend::{FileId, TreeValue};
use jj_lib::conflicts::{materialize, parse_conflict, update_from_content};
use jj_lib::backend::FileId;
use jj_lib::conflicts::{
extract_as_single_hunk, materialize_merge_result, parse_conflict, update_from_content,
};
use jj_lib::merge::Merge;
use jj_lib::repo::Repo;
use jj_lib::repo_path::RepoPath;
use jj_lib::store::Store;
use testutils::TestRepo;
fn file_value(file_id: &FileId) -> TreeValue {
TreeValue::File {
id: file_id.clone(),
executable: false,
}
}
#[test]
fn test_materialize_conflict_basic() {
let test_repo = TestRepo::init(false);
@ -69,8 +64,8 @@ line 5
// The left side should come first. The diff should be use the smaller (right)
// side, and the left side should be a snapshot.
let conflict = Merge::new(
vec![Some(file_value(&base_id))],
vec![Some(file_value(&left_id)), Some(file_value(&right_id))],
vec![Some(base_id.clone())],
vec![Some(left_id.clone()), Some(right_id.clone())],
);
insta::assert_snapshot!(
&materialize_conflict_string(store, &path, &conflict),
@ -93,8 +88,8 @@ line 5
// Swap the positive terms in the conflict. The diff should still use the right
// side, but now the right side should come first.
let conflict = Merge::new(
vec![Some(file_value(&base_id))],
vec![Some(file_value(&right_id)), Some(file_value(&left_id))],
vec![Some(base_id.clone())],
vec![Some(right_id.clone()), Some(left_id.clone())],
);
insta::assert_snapshot!(
&materialize_conflict_string(store, &path, &conflict),
@ -162,12 +157,8 @@ line 3
// The order of (a, b, c) should be preserved. For all cases, the "a" side
// should be a snapshot.
let conflict = Merge::new(
vec![Some(file_value(&base_id)), Some(file_value(&base_id))],
vec![
Some(file_value(&a_id)),
Some(file_value(&b_id)),
Some(file_value(&c_id)),
],
vec![Some(base_id.clone()), Some(base_id.clone())],
vec![Some(a_id.clone()), Some(b_id.clone()), Some(c_id.clone())],
);
insta::assert_snapshot!(
&materialize_conflict_string(store, &path, &conflict),
@ -190,12 +181,8 @@ line 3
"###
);
let conflict = Merge::new(
vec![Some(file_value(&base_id)), Some(file_value(&base_id))],
vec![
Some(file_value(&c_id)),
Some(file_value(&b_id)),
Some(file_value(&a_id)),
],
vec![Some(base_id.clone()), Some(base_id.clone())],
vec![Some(c_id.clone()), Some(b_id.clone()), Some(a_id.clone())],
);
insta::assert_snapshot!(
&materialize_conflict_string(store, &path, &conflict),
@ -218,12 +205,8 @@ line 3
"###
);
let conflict = Merge::new(
vec![Some(file_value(&base_id)), Some(file_value(&base_id))],
vec![
Some(file_value(&c_id)),
Some(file_value(&a_id)),
Some(file_value(&b_id)),
],
vec![Some(base_id.clone()), Some(base_id.clone())],
vec![Some(c_id.clone()), Some(a_id.clone()), Some(b_id.clone())],
);
insta::assert_snapshot!(
&materialize_conflict_string(store, &path, &conflict),
@ -285,8 +268,8 @@ line 5 right
);
let conflict = Merge::new(
vec![Some(file_value(&base_id))],
vec![Some(file_value(&left_id)), Some(file_value(&right_id))],
vec![Some(base_id.clone())],
vec![Some(left_id.clone()), Some(right_id.clone())],
);
let materialized = materialize_conflict_string(store, &path, &conflict);
insta::assert_snapshot!(
@ -387,11 +370,8 @@ line 5
// left modifies a line, right deletes the same line.
let conflict = Merge::new(
vec![Some(file_value(&base_id))],
vec![
Some(file_value(&modified_id)),
Some(file_value(&deleted_id)),
],
vec![Some(base_id.clone())],
vec![Some(modified_id.clone()), Some(deleted_id.clone())],
);
insta::assert_snapshot!(&materialize_conflict_string(store, &path, &conflict), @r###"
line 1
@ -409,11 +389,8 @@ line 5
// right modifies a line, left deletes the same line.
let conflict = Merge::new(
vec![Some(file_value(&base_id))],
vec![
Some(file_value(&deleted_id)),
Some(file_value(&modified_id)),
],
vec![Some(base_id.clone())],
vec![Some(deleted_id.clone()), Some(modified_id.clone())],
);
insta::assert_snapshot!(&materialize_conflict_string(store, &path, &conflict), @r###"
line 1
@ -431,8 +408,8 @@ line 5
// modify/delete conflict at the file level
let conflict = Merge::new(
vec![Some(file_value(&base_id))],
vec![Some(file_value(&modified_id)), None],
vec![Some(base_id.clone())],
vec![Some(modified_id.clone()), None],
);
insta::assert_snapshot!(&materialize_conflict_string(store, &path, &conflict), @r###"
<<<<<<<
@ -651,11 +628,8 @@ fn test_update_conflict_from_content() {
let left_file_id = testutils::write_file(store, &path, "left 1\nline 2\nleft 3\n");
let right_file_id = testutils::write_file(store, &path, "right 1\nline 2\nright 3\n");
let conflict = Merge::new(
vec![Some(file_value(&base_file_id))],
vec![
Some(file_value(&left_file_id)),
Some(file_value(&right_file_id)),
],
vec![Some(base_file_id.clone())],
vec![Some(left_file_id.clone()), Some(right_file_id.clone())],
);
// If the content is unchanged compared to the materialized value, we get the
@ -686,10 +660,10 @@ fn test_update_conflict_from_content() {
assert_eq!(
new_conflict,
Merge::new(
vec![Some(file_value(&new_base_file_id))],
vec![Some(new_base_file_id.clone())],
vec![
Some(file_value(&new_left_file_id)),
Some(file_value(&new_right_file_id))
Some(new_left_file_id.clone()),
Some(new_right_file_id.clone())
]
)
);
@ -703,10 +677,7 @@ fn test_update_conflict_from_content_modify_delete() {
let path = RepoPath::from_internal_string("dir/file");
let before_file_id = testutils::write_file(store, &path, "line 1\nline 2 before\nline 3\n");
let after_file_id = testutils::write_file(store, &path, "line 1\nline 2 after\nline 3\n");
let conflict = Merge::new(
vec![Some(file_value(&before_file_id))],
vec![Some(file_value(&after_file_id)), None],
);
let conflict = Merge::new(vec![Some(before_file_id)], vec![Some(after_file_id), None]);
// If the content is unchanged compared to the materialized value, we get the
// old conflict id back.
@ -734,8 +705,8 @@ fn test_update_conflict_from_content_modify_delete() {
assert_eq!(
new_conflict,
Merge::new(
vec![Some(file_value(&new_base_file_id))],
vec![Some(file_value(&new_left_file_id)), None]
vec![Some(new_base_file_id.clone())],
vec![Some(new_left_file_id.clone()), None]
)
);
}
@ -743,9 +714,10 @@ fn test_update_conflict_from_content_modify_delete() {
fn materialize_conflict_string(
store: &Store,
path: &RepoPath,
conflict: &Merge<Option<TreeValue>>,
conflict: &Merge<Option<FileId>>,
) -> String {
let mut result: Vec<u8> = vec![];
materialize(conflict, store, path, &mut result).unwrap();
let contents = extract_as_single_hunk(conflict, store, path);
materialize_merge_result(&contents, &mut result).unwrap();
String::from_utf8(result).unwrap()
}