From b6edc79e5d14f194fbbe06f13decd71382058c1b Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Sun, 5 Nov 2023 10:40:55 +0900 Subject: [PATCH 1/5] merge: rewrite bottom half of trivial_merge() for non-copyable types The input values of trivial_merge() will be changed to Iterator where T: Eq + Hash. It could be , but it doesn't have to be. --- lib/src/merge.rs | 46 ++++++++++++++++++++++------------------------ 1 file changed, 22 insertions(+), 24 deletions(-) diff --git a/lib/src/merge.rs b/lib/src/merge.rs index 5e51f44a0b..256e96459c 100644 --- a/lib/src/merge.rs +++ b/lib/src/merge.rs @@ -72,31 +72,29 @@ where // Collect non-zero value. Values with a count of 0 means that they have // cancelled out. - let counts = counts - .into_iter() - .filter(|&(_, count)| count != 0) - .collect_vec(); - match counts[..] { - [(value, 1)] => { - // If there is a single value with a count of 1 left, then that is the result. - Some(value) - } - [(value1, count1), (value2, count2)] => { - // All sides made the same change. - // This matches what Git and Mercurial do (in the 3-way case at least), but not - // what Darcs and Pijul do. It means that repeated 3-way merging of multiple - // trees may give different results depending on the order of merging. - // TODO: Consider removing this special case, making the algorithm more strict, - // and maybe add a more lenient version that is used when the user explicitly - // asks for conflict resolution. - assert_eq!(count1 + count2, 1); - if count1 > 0 { - Some(value1) - } else { - Some(value2) - } + counts.retain(|_, count| *count != 0); + if counts.len() == 1 { + // If there is a single value with a count of 1 left, then that is the result. + let (value, count) = counts.into_iter().next().unwrap(); + assert_eq!(count, 1); + Some(value) + } else if counts.len() == 2 { + // All sides made the same change. + // This matches what Git and Mercurial do (in the 3-way case at least), but not + // what Darcs and Pijul do. It means that repeated 3-way merging of multiple + // trees may give different results depending on the order of merging. + // TODO: Consider removing this special case, making the algorithm more strict, + // and maybe add a more lenient version that is used when the user explicitly + // asks for conflict resolution. + let ((value1, count1), (value2, count2)) = counts.into_iter().next_tuple().unwrap(); + assert_eq!(count1 + count2, 1); + if count1 > 0 { + Some(value1) + } else { + Some(value2) } - _ => None, + } else { + None } } From 8f9e6cb4876487df07003434a8027f3f74196bad Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Fri, 3 Nov 2023 11:11:39 +0900 Subject: [PATCH 2/5] merge: extract trivial_merge() that takes interleaved adds/removes iterator The Merge type will store interleaved terms instead of separate adds/removes vecs. --- lib/src/merge.rs | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/lib/src/merge.rs b/lib/src/merge.rs index 256e96459c..24a1f2829e 100644 --- a/lib/src/merge.rs +++ b/lib/src/merge.rs @@ -43,17 +43,28 @@ where removes.len() + 1, "trivial_merge() requires exactly one more adds than removes" ); + trivial_merge_inner( + itertools::interleave(adds, removes), + adds.len() + removes.len(), + ) +} +fn trivial_merge_inner(mut values: impl Iterator, values_len: usize) -> Option +where + T: Eq + Hash, +{ // Optimize the common cases of 3-way merge and 1-way (non-)merge - if adds.len() == 1 { - return Some(&adds[0]); - } else if adds.len() == 2 { - return if adds[0] == adds[1] { - Some(&adds[0]) - } else if adds[0] == removes[0] { - Some(&adds[1]) - } else if adds[1] == removes[0] { - Some(&adds[0]) + if values_len == 1 { + let add = values.next().unwrap(); + return Some(add); + } else if values_len == 3 { + let (add0, remove, add1) = values.next_tuple().unwrap(); + return if add0 == add1 { + Some(add0) + } else if add0 == remove { + Some(add1) + } else if add1 == remove { + Some(add0) } else { None }; @@ -62,12 +73,9 @@ where // Number of occurrences of each value, with positive indexes counted as +1 and // negative as -1, thereby letting positive and negative terms with the same // value (i.e. key in the map) cancel each other. - let mut counts: HashMap<&T, i32> = HashMap::new(); - for value in adds.iter() { - counts.entry(value).and_modify(|e| *e += 1).or_insert(1); - } - for value in removes.iter() { - counts.entry(value).and_modify(|e| *e -= 1).or_insert(-1); + let mut counts: HashMap = HashMap::new(); + for (value, n) in zip(values, [1, -1].into_iter().cycle()) { + counts.entry(value).and_modify(|e| *e += n).or_insert(n); } // Collect non-zero value. Values with a count of 0 means that they have From c315e1bd6b63cc5edb3cbf04c724fbf600af72e4 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Wed, 18 Oct 2023 03:55:02 +0900 Subject: [PATCH 3/5] merge: store negative/positive terms internally in an interleaved Vec Many callers use interleaved iterators, and recently-added serialization code is built on top of that, so I think it's better to store terms in that format. map() functions no longer use MergeBuilder as we know the mapped values are ordered properly. flatten() and simplify() are reimplemented to work with the interleaved values. The other changes are trivial. --- lib/src/merge.rs | 180 ++++++++++++++++++++++++++--------------------- 1 file changed, 100 insertions(+), 80 deletions(-) diff --git a/lib/src/merge.rs b/lib/src/merge.rs index 24a1f2829e..d4084ccb67 100644 --- a/lib/src/merge.rs +++ b/lib/src/merge.rs @@ -22,6 +22,7 @@ use std::hash::Hash; use std::io::Write; use std::iter::zip; use std::sync::Arc; +use std::{slice, vec}; use itertools::Itertools; @@ -113,20 +114,21 @@ where /// remove. The zeroth add is considered a diff from the non-existent state. #[derive(PartialEq, Eq, Hash, Clone)] pub struct Merge { - removes: Vec, - adds: Vec, + /// Alternates between positive and negative terms, starting with positive. + values: Vec, } impl Debug for Merge { fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), std::fmt::Error> { // Format like an enum with two variants to make it less verbose in the common // case of a resolved state. - if self.removes.is_empty() { - f.debug_tuple("Resolved").field(&self.adds[0]).finish() + if let Some(value) = self.as_resolved() { + f.debug_tuple("Resolved").field(value).finish() } else { + // TODO: just print values? f.debug_struct("Conflicted") - .field("removes", &self.removes) - .field("adds", &self.adds) + .field("removes", &self.removes().collect_vec()) + .field("adds", &self.adds().collect_vec()) .finish() } } @@ -135,13 +137,18 @@ impl Debug for Merge { impl Merge { /// Creates a new merge object from the given removes and adds. pub fn new(removes: Vec, adds: Vec) -> Self { + // TODO: removes and adds can be just IntoIterator. + // TODO: maybe add constructor that takes single values vec, and rename this assert_eq!(adds.len(), removes.len() + 1); - Merge { removes, adds } + let values = itertools::interleave(adds, removes).collect(); + Merge { values } } /// Creates a `Merge` with a single resolved value. pub fn resolved(value: T) -> Self { - Merge::new(vec![], vec![value]) + Merge { + values: vec![value], + } } /// Create a `Merge` from a `removes` and `adds`, padding with `None` to @@ -150,6 +157,7 @@ impl Merge { removes: impl IntoIterator, adds: impl IntoIterator, ) -> Merge> { + // TODO: no need to build intermediate removes/adds vecs let mut removes = removes.into_iter().map(Some).collect_vec(); let mut adds = adds.into_iter().map(Some).collect_vec(); while removes.len() + 1 < adds.len() { @@ -163,51 +171,59 @@ impl Merge { /// Returns the removes and adds as a pair. pub fn take(self) -> (Vec, Vec) { - (self.removes, self.adds) + let mut removes = Vec::with_capacity(self.values.len() / 2); + let mut adds = Vec::with_capacity(self.values.len() / 2 + 1); + let mut values = self.values.into_iter(); + adds.push(values.next().unwrap()); + while let Some(remove) = values.next() { + removes.push(remove); + adds.push(values.next().unwrap()); + } + (removes, adds) } /// The removed values, also called negative terms. pub fn removes(&self) -> impl ExactSizeIterator { - self.removes.iter() + self.values[1..].iter().step_by(2) } /// The added values, also called positive terms. pub fn adds(&self) -> impl ExactSizeIterator { - self.adds.iter() + self.values.iter().step_by(2) } /// Returns the zeroth added value, which is guaranteed to exist. pub fn first(&self) -> &T { - &self.adds[0] + &self.values[0] } /// Returns the `index`-th removed value, which is considered belonging to /// the `index`-th diff pair. pub fn get_remove(&self, index: usize) -> Option<&T> { - self.removes.get(index) + self.values.get(index * 2 + 1) } /// Returns the `index`-th added value, which is considered belonging to the /// `index-1`-th diff pair. The zeroth add is a diff from the non-existent /// state. pub fn get_add(&self, index: usize) -> Option<&T> { - self.adds.get(index) + self.values.get(index * 2) } /// The number of positive terms in the conflict. pub fn num_sides(&self) -> usize { - self.adds.len() + self.values.len() / 2 + 1 } /// Whether this merge is resolved. Does not resolve trivial merges. pub fn is_resolved(&self) -> bool { - self.removes.is_empty() + self.values.len() == 1 } /// Returns the resolved value, if this merge is resolved. Does not /// resolve trivial merges. pub fn as_resolved(&self) -> Option<&T> { - if let [value] = &self.adds[..] { + if let [value] = &self.values[..] { Some(value) } else { None @@ -217,8 +233,8 @@ impl Merge { /// Returns the resolved value, if this merge is resolved. Otherwise returns /// the merge itself as an `Err`. Does not resolve trivial merges. pub fn into_resolved(mut self) -> Result> { - if self.removes.is_empty() { - Ok(self.adds.pop().unwrap()) + if self.values.len() == 1 { + Ok(self.values.pop().unwrap()) } else { Err(self) } @@ -231,16 +247,16 @@ impl Merge { T: PartialEq, { let mut add_index = 0; - while add_index < self.adds.len() { - let add = &self.adds[add_index]; - if let Some(remove_index) = self.removes.iter().position(|remove| remove == add) { - // Move the value to the `add_index-1`th diff, then delete the `remove_index`th - // diff. - self.adds.swap(remove_index + 1, add_index); - self.removes.remove(remove_index); - self.adds.remove(remove_index + 1); + while add_index < self.values.len() { + let add = &self.values[add_index]; + let mut removes = self.values.iter().enumerate().skip(1).step_by(2); + if let Some((remove_index, _)) = removes.find(|&(_, remove)| remove == add) { + // Align the current "add" value to the `remove_index/2`-th diff, then + // delete the diff pair. + self.values.swap(remove_index + 1, add_index); + self.values.drain(remove_index..remove_index + 2); } else { - add_index += 1; + add_index += 2; } } self @@ -252,7 +268,7 @@ impl Merge { where T: Eq + Hash, { - trivial_merge(&self.removes, &self.adds) + trivial_merge_inner(self.values.iter(), self.values.len()) } /// Pads this merge with to the specified number of sides with the specified @@ -261,35 +277,35 @@ impl Merge { where T: Clone, { - for _ in self.num_sides()..num_sides { - self.removes.push(value.clone()); - self.adds.push(value.clone()); + if num_sides <= self.num_sides() { + return; } + self.values.resize(num_sides * 2 - 1, value.clone()); } /// Returns an iterator over references to the terms. The items will /// alternate between positive and negative terms, starting with /// positive (since there's one more of those). - pub fn iter(&self) -> impl Iterator { - itertools::interleave(&self.adds, &self.removes) + pub fn iter(&self) -> slice::Iter<'_, T> { + self.values.iter() } /// A version of `Merge::iter()` that iterates over mutable references. - pub fn iter_mut(&mut self) -> impl Iterator { - itertools::interleave(self.adds.iter_mut(), self.removes.iter_mut()) + pub fn iter_mut(&mut self) -> slice::IterMut<'_, T> { + self.values.iter_mut() } /// Creates a new merge by applying `f` to each remove and add. pub fn map<'a, U>(&'a self, f: impl FnMut(&'a T) -> U) -> Merge { - let builder: MergeBuilder = self.iter().map(f).collect(); - builder.build() + let values = self.values.iter().map(f).collect(); + Merge { values } } /// Creates a new merge by applying `f` to each remove and add, returning /// `None if `f` returns `None` for any of them. pub fn maybe_map<'a, U>(&'a self, f: impl FnMut(&'a T) -> Option) -> Option> { - let builder: Option> = self.iter().map(f).collect(); - builder.map(MergeBuilder::build) + let values = self.values.iter().map(f).collect::>()?; + Some(Merge { values }) } /// Creates a new merge by applying `f` to each remove and add, returning @@ -298,8 +314,8 @@ impl Merge { &'a self, f: impl FnMut(&'a T) -> Result, ) -> Result, E> { - let builder: MergeBuilder = self.iter().map(f).try_collect()?; - Ok(builder.build()) + let values = self.values.iter().map(f).try_collect()?; + Ok(Merge { values }) } } @@ -313,15 +329,13 @@ impl Merge { /// the checking until after `from_iter()` (to `MergeBuilder::build()`). #[derive(Clone, Debug, PartialEq, Eq)] pub struct MergeBuilder { - removes: Vec, - adds: Vec, + values: Vec, } impl Default for MergeBuilder { fn default() -> Self { Self { - removes: Default::default(), - adds: Default::default(), + values: Default::default(), } } } @@ -330,16 +344,19 @@ impl MergeBuilder { /// Requires that exactly one more "adds" than "removes" have been added to /// this builder. pub fn build(self) -> Merge { - Merge::new(self.removes, self.adds) + assert!(self.values.len() & 1 != 0); + Merge { + values: self.values, + } } } impl IntoIterator for Merge { type Item = T; - type IntoIter = itertools::Interleave, std::vec::IntoIter>; + type IntoIter = vec::IntoIter; fn into_iter(self) -> Self::IntoIter { - itertools::interleave(self.adds, self.removes) + self.values.into_iter() } } @@ -353,15 +370,7 @@ impl FromIterator for MergeBuilder { impl Extend for MergeBuilder { fn extend>(&mut self, iter: I) { - let (mut curr, mut next) = if self.adds.len() != self.removes.len() { - (&mut self.removes, &mut self.adds) - } else { - (&mut self.adds, &mut self.removes) - }; - for item in iter { - curr.push(item); - std::mem::swap(&mut curr, &mut next); - } + self.values.extend(iter) } } @@ -390,10 +399,16 @@ impl Merge> { /// `None` values. Note that the conversion is lossy: the order of `None` /// values is not preserved when converting back to a `Merge`. pub fn into_legacy_form(self) -> (Vec, Vec) { - ( - self.removes.into_iter().flatten().collect(), - self.adds.into_iter().flatten().collect(), - ) + // Allocate the maximum size assuming there would be few `None`s. + let mut removes = Vec::with_capacity(self.values.len() / 2); + let mut adds = Vec::with_capacity(self.values.len() / 2 + 1); + let mut values = self.values.into_iter(); + adds.extend(values.next().unwrap()); + while let Some(remove) = values.next() { + removes.extend(remove); + adds.extend(values.next().unwrap()); + } + (removes, adds) } } @@ -411,30 +426,35 @@ impl Merge> { /// /// 4 5 0 7 8 /// 3 2 1 6 - pub fn flatten(mut self) -> Merge { - self.removes.reverse(); - self.adds.reverse(); - let mut result = self.adds.pop().unwrap(); - while let Some(mut remove) = self.removes.pop() { + pub fn flatten(self) -> Merge { + let mut outer_values = self.values.into_iter(); + let mut result = outer_values.next().unwrap(); + while let Some(mut remove) = outer_values.next() { // Add removes reversed, and with the first element moved last, so we preserve // the diffs - let first_add = remove.adds.remove(0); - result.removes.extend(remove.adds); - result.removes.push(first_add); - result.adds.extend(remove.removes); - let add = self.adds.pop().unwrap(); - result.removes.extend(add.removes); - result.adds.extend(add.adds); + remove.values.rotate_left(1); + for i in 0..remove.values.len() / 2 { + remove.values.swap(i * 2, i * 2 + 1); + } + result.values.extend(remove.values); + let add = outer_values.next().unwrap(); + result.values.extend(add.values); } - assert!(self.adds.is_empty()); result } } impl ContentHash for Merge { fn hash(&self, state: &mut impl digest::Update) { - self.removes.hash(state); - self.adds.hash(state); + // TODO: just hash values + state.update(&(self.removes().len() as u64).to_le_bytes()); + for value in self.removes() { + value.hash(state); + } + state.update(&(self.adds().len() as u64).to_le_bytes()); + for value in self.adds() { + value.hash(state); + } } } @@ -492,8 +512,8 @@ impl MergedTreeValue { /// 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>) -> Self { - assert_eq!(self.removes.len(), file_ids.removes.len()); - let builder: MergeBuilder> = zip(self.iter(), file_ids.iter()) + assert_eq!(self.values.len(), file_ids.values.len()); + let values = zip(self.iter(), file_ids.iter()) .map(|(tree_value, file_id)| { if let Some(TreeValue::File { id: _, executable }) = tree_value { Some(TreeValue::File { @@ -507,7 +527,7 @@ impl MergedTreeValue { } }) .collect(); - builder.build() + Merge { values } } /// Give a summary description of the conflict's "removes" and "adds" From f53346f22163e84dc8f7ec4519f3b1be6053d094 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Fri, 3 Nov 2023 14:19:59 +0900 Subject: [PATCH 4/5] merge: simply generate content hash from interleaved values --- cli/tests/test_concurrent_operations.rs | 20 +++++------ cli/tests/test_debug_command.rs | 2 +- cli/tests/test_operations.rs | 44 ++++++++++++------------- cli/tests/test_workspaces.rs | 8 ++--- lib/src/merge.rs | 10 +----- lib/src/simple_op_store.rs | 2 +- 6 files changed, 39 insertions(+), 47 deletions(-) diff --git a/cli/tests/test_concurrent_operations.rs b/cli/tests/test_concurrent_operations.rs index ee52b90499..2e5a7a307e 100644 --- a/cli/tests/test_concurrent_operations.rs +++ b/cli/tests/test_concurrent_operations.rs @@ -55,15 +55,15 @@ fn test_concurrent_operations_auto_rebase() { test_env.jj_cmd_ok(&repo_path, &["describe", "-m", "initial"]); let stdout = test_env.jj_cmd_success(&repo_path, &["op", "log"]); insta::assert_snapshot!(stdout, @r###" - @ 8626e1f87784 test-username@host.example.com 2001-02-03 04:05:08.000 +07:00 - 2001-02-03 04:05:08.000 +07:00 + @ cfc96ff553b9 test-username@host.example.com 2001-02-03 04:05:08.000 +07:00 - 2001-02-03 04:05:08.000 +07:00 │ describe commit 123ed18e4c4c0d77428df41112bc02ffc83fb935 │ args: jj describe -m initial - ◉ 80b17428029f test-username@host.example.com 2001-02-03 04:05:08.000 +07:00 - 2001-02-03 04:05:08.000 +07:00 + ◉ 65a6c90b9544 test-username@host.example.com 2001-02-03 04:05:08.000 +07:00 - 2001-02-03 04:05:08.000 +07:00 │ snapshot working copy │ args: jj describe -m initial - ◉ d50e0e495b10 test-username@host.example.com 2001-02-03 04:05:07.000 +07:00 - 2001-02-03 04:05:07.000 +07:00 + ◉ 19b8089fc78b test-username@host.example.com 2001-02-03 04:05:07.000 +07:00 - 2001-02-03 04:05:07.000 +07:00 │ add workspace 'default' - ◉ 23b83cc0392f test-username@host.example.com 2001-02-03 04:05:07.000 +07:00 - 2001-02-03 04:05:07.000 +07:00 + ◉ f1c462c494be test-username@host.example.com 2001-02-03 04:05:07.000 +07:00 - 2001-02-03 04:05:07.000 +07:00 initialize repo "###); let op_id_hex = stdout[3..15].to_string(); @@ -168,21 +168,21 @@ fn test_concurrent_snapshot_wc_reloadable() { let template = r#"id ++ "\n" ++ description ++ "\n" ++ tags"#; let op_log_stdout = test_env.jj_cmd_success(&repo_path, &["op", "log", "-T", template]); insta::assert_snapshot!(op_log_stdout, @r###" - @ 98aae8ac6dabe0f4e97b0708afd5848f22f0831d8f5be3169063120efe0769b27d123b038f505f2a78d1cd92a0b8345eee2bd201ba6d687fb8c9326dac1fcfaa + @ 9be517934aaabc351597e88ed4119aa9454ae3588ab7f28646a810272c82f3dafb1deb20b3c978dbb58ba9abc8f08fe870fe3c7ce5f682411991e83eee40a77f │ commit 323b414dd255b51375d7f4392b7b2641ffe4289f │ args: jj commit -m 'new child1' - ◉ 13ce1be149ff5fe4386896a753f9321e1826cb6603a6310e50eb778aacbaf7080ca50e7eb3c3ddcacb3e433c619e222fe0f7637dec14551350a03b8a6959f739 + ◉ d967c09eb12b38dad2065a0bc9e251824247f9f84ba406a7356f5405e4c93c21562178a3f00cafedfa1df1435ba496265f39da9d1ccebaccb78bdcb4bd7031e1 │ snapshot working copy │ args: jj commit -m 'new child1' - ◉ 21126efd140dc442d7663ac963912f664dcde62b397e71bcc1628d914f454c3767a55ae16756a47793303b68099150d4a86344129e32e561f4853f283e745814 + ◉ b6d168ba4fb4534257b6e58d53eb407582567342358eab07cf5a01a7e4d797313b692f27664c2fb7935b2380d398d0298233c9732f821b8c687e35607ea08a55 │ commit 3d918700494a9895696e955b85fa05eb0d314cc6 │ args: jj commit -m initial - ◉ f29c2903a0a18deefc8323c6e1ac8c1165c238f74ed508180a7ae77b46ba1c1be0ff13d7112d59368d56a3552564476b8595f7c2b3a6dc26aec1e926d0f280e6 + ◉ 5e9e3f82fc14750ff985c5a39f1935ed8876b973b8800b56bc03d1c9754795e724956d862d1fcb2c533d06ca36abc9fa9f7cb7d3b2b64e993e9a87f80d5af670 │ snapshot working copy │ args: jj commit -m initial - ◉ d50e0e495b10b2575e2a9b914fed7045e427254baec051bc769d22f607f2f55f09788e6f6c9e12a4e56ec994ac7a63decb1fa7e353b2c8fd20ce77760777283d + ◉ 19b8089fc78b7c49171f3c8934248be6f89f52311005e961cab5780f9f138b142456d77b27d223d7ee84d21d8c30c4a80100eaf6735b548b1acd0da688f94c80 │ add workspace 'default' - ◉ 23b83cc0392f51ef6b548af289e54efbe8661fe88ab5d8c036444d8c4ec798fb9d4d6063f02c4db768b17bde4608bcae2d75ba79da91ddd59dc57a23d45b5c57 + ◉ f1c462c494be39f6690928603c5393f908866bc8d81d8cd1ae0bb2ea02cb4f78cafa47165fa5b7cda258e2178f846881de199066991960a80954ba6066ba0821 initialize repo "###); let op_log_lines = op_log_stdout.lines().collect_vec(); diff --git a/cli/tests/test_debug_command.rs b/cli/tests/test_debug_command.rs index 3d383042dd..f6e6cccd3b 100644 --- a/cli/tests/test_debug_command.rs +++ b/cli/tests/test_debug_command.rs @@ -127,7 +127,7 @@ fn test_debug_operation_id() { let stdout = test_env.jj_cmd_success(&workspace_path, &["debug", "operation", "--display", "id"]); assert_snapshot!(filter_index_stats(&stdout), @r###" - d50e0e495b10b2575e2a9b914fed7045e427254baec051bc769d22f607f2f55f09788e6f6c9e12a4e56ec994ac7a63decb1fa7e353b2c8fd20ce77760777283d + 19b8089fc78b7c49171f3c8934248be6f89f52311005e961cab5780f9f138b142456d77b27d223d7ee84d21d8c30c4a80100eaf6735b548b1acd0da688f94c80 "### ); } diff --git a/cli/tests/test_operations.rs b/cli/tests/test_operations.rs index 63f95020ef..d9a63871fd 100644 --- a/cli/tests/test_operations.rs +++ b/cli/tests/test_operations.rs @@ -38,12 +38,12 @@ fn test_op_log() { ], ); insta::assert_snapshot!(&stdout, @r###" - @ 745ab9998f2f test-username@host.example.com 2001-02-03 04:05:08.000 +07:00 - 2001-02-03 04:05:08.000 +07:00 + @ 98f7262e4a06 test-username@host.example.com 2001-02-03 04:05:08.000 +07:00 - 2001-02-03 04:05:08.000 +07:00 │ describe commit 230dd059e1b059aefc0da06a2e5a7dbf22362f22 │ args: jj describe -m 'description 0' - ◉ d50e0e495b10 test-username@host.example.com 2001-02-03 04:05:07.000 +07:00 - 2001-02-03 04:05:07.000 +07:00 + ◉ 19b8089fc78b test-username@host.example.com 2001-02-03 04:05:07.000 +07:00 - 2001-02-03 04:05:07.000 +07:00 │ add workspace 'default' - ◉ 23b83cc0392f test-username@host.example.com 2001-02-03 04:05:07.000 +07:00 - 2001-02-03 04:05:07.000 +07:00 + ◉ f1c462c494be test-username@host.example.com 2001-02-03 04:05:07.000 +07:00 - 2001-02-03 04:05:07.000 +07:00 initialize repo "###); let op_log_lines = stdout.lines().collect_vec(); @@ -139,9 +139,9 @@ fn test_op_log_no_graph() { let stdout = test_env.jj_cmd_success(&repo_path, &["op", "log", "--no-graph", "--color=always"]); insta::assert_snapshot!(stdout, @r###" - d50e0e495b10 test-username@host.example.com 2001-02-03 04:05:07.000 +07:00 - 2001-02-03 04:05:07.000 +07:00 + 19b8089fc78b test-username@host.example.com 2001-02-03 04:05:07.000 +07:00 - 2001-02-03 04:05:07.000 +07:00 add workspace 'default' - 23b83cc0392f test-username@host.example.com 2001-02-03 04:05:07.000 +07:00 - 2001-02-03 04:05:07.000 +07:00 + f1c462c494be test-username@host.example.com 2001-02-03 04:05:07.000 +07:00 - 2001-02-03 04:05:07.000 +07:00 initialize repo "###); } @@ -164,7 +164,7 @@ fn test_op_log_no_graph_null_terminated() { r#"id.short(4) ++ "\0""#, ], ); - insta::assert_debug_snapshot!(stdout, @r###""16e6\0fb05\0d50e\023b8\0""###); + insta::assert_debug_snapshot!(stdout, @r###""c8b0\07277\019b8\0f1c4\0""###); } #[test] @@ -175,14 +175,14 @@ fn test_op_log_template() { let render = |template| test_env.jj_cmd_success(&repo_path, &["op", "log", "-T", template]); insta::assert_snapshot!(render(r#"id ++ "\n""#), @r###" - @ d50e0e495b10b2575e2a9b914fed7045e427254baec051bc769d22f607f2f55f09788e6f6c9e12a4e56ec994ac7a63decb1fa7e353b2c8fd20ce77760777283d - ◉ 23b83cc0392f51ef6b548af289e54efbe8661fe88ab5d8c036444d8c4ec798fb9d4d6063f02c4db768b17bde4608bcae2d75ba79da91ddd59dc57a23d45b5c57 + @ 19b8089fc78b7c49171f3c8934248be6f89f52311005e961cab5780f9f138b142456d77b27d223d7ee84d21d8c30c4a80100eaf6735b548b1acd0da688f94c80 + ◉ f1c462c494be39f6690928603c5393f908866bc8d81d8cd1ae0bb2ea02cb4f78cafa47165fa5b7cda258e2178f846881de199066991960a80954ba6066ba0821 "###); insta::assert_snapshot!( render(r#"separate(" ", id.short(5), current_operation, user, time.start(), time.end(), time.duration()) ++ "\n""#), @r###" - @ d50e0 true test-username@host.example.com 2001-02-03 04:05:07.000 +07:00 2001-02-03 04:05:07.000 +07:00 less than a microsecond - ◉ 23b83 false test-username@host.example.com 2001-02-03 04:05:07.000 +07:00 2001-02-03 04:05:07.000 +07:00 less than a microsecond + @ 19b80 true test-username@host.example.com 2001-02-03 04:05:07.000 +07:00 2001-02-03 04:05:07.000 +07:00 less than a microsecond + ◉ f1c46 false test-username@host.example.com 2001-02-03 04:05:07.000 +07:00 2001-02-03 04:05:07.000 +07:00 less than a microsecond "###); // Negative length shouldn't cause panic (and is clamped.) @@ -204,9 +204,9 @@ fn test_op_log_template() { let regex = Regex::new(r"\d\d years").unwrap(); let stdout = test_env.jj_cmd_success(&repo_path, &["op", "log"]); insta::assert_snapshot!(regex.replace_all(&stdout, "NN years"), @r###" - @ d50e0e495b10 test-username@host.example.com NN years ago, lasted less than a microsecond + @ 19b8089fc78b test-username@host.example.com NN years ago, lasted less than a microsecond │ add workspace 'default' - ◉ 23b83cc0392f test-username@host.example.com NN years ago, lasted less than a microsecond + ◉ f1c462c494be test-username@host.example.com NN years ago, lasted less than a microsecond initialize repo "###); } @@ -220,24 +220,24 @@ fn test_op_log_builtin_templates() { test_env.jj_cmd_ok(&repo_path, &["describe", "-m", "description 0"]); insta::assert_snapshot!(render(r#"builtin_op_log_compact"#), @r###" - @ 745ab9998f2f test-username@host.example.com 2001-02-03 04:05:08.000 +07:00 - 2001-02-03 04:05:08.000 +07:00 + @ 98f7262e4a06 test-username@host.example.com 2001-02-03 04:05:08.000 +07:00 - 2001-02-03 04:05:08.000 +07:00 │ describe commit 230dd059e1b059aefc0da06a2e5a7dbf22362f22 │ args: jj describe -m 'description 0' - ◉ d50e0e495b10 test-username@host.example.com 2001-02-03 04:05:07.000 +07:00 - 2001-02-03 04:05:07.000 +07:00 + ◉ 19b8089fc78b test-username@host.example.com 2001-02-03 04:05:07.000 +07:00 - 2001-02-03 04:05:07.000 +07:00 │ add workspace 'default' - ◉ 23b83cc0392f test-username@host.example.com 2001-02-03 04:05:07.000 +07:00 - 2001-02-03 04:05:07.000 +07:00 + ◉ f1c462c494be test-username@host.example.com 2001-02-03 04:05:07.000 +07:00 - 2001-02-03 04:05:07.000 +07:00 initialize repo "###); insta::assert_snapshot!(render(r#"builtin_op_log_comfortable"#), @r###" - @ 745ab9998f2f test-username@host.example.com 2001-02-03 04:05:08.000 +07:00 - 2001-02-03 04:05:08.000 +07:00 + @ 98f7262e4a06 test-username@host.example.com 2001-02-03 04:05:08.000 +07:00 - 2001-02-03 04:05:08.000 +07:00 │ describe commit 230dd059e1b059aefc0da06a2e5a7dbf22362f22 │ args: jj describe -m 'description 0' │ - ◉ d50e0e495b10 test-username@host.example.com 2001-02-03 04:05:07.000 +07:00 - 2001-02-03 04:05:07.000 +07:00 + ◉ 19b8089fc78b test-username@host.example.com 2001-02-03 04:05:07.000 +07:00 - 2001-02-03 04:05:07.000 +07:00 │ add workspace 'default' │ - ◉ 23b83cc0392f test-username@host.example.com 2001-02-03 04:05:07.000 +07:00 - 2001-02-03 04:05:07.000 +07:00 + ◉ f1c462c494be test-username@host.example.com 2001-02-03 04:05:07.000 +07:00 - 2001-02-03 04:05:07.000 +07:00 initialize repo "###); @@ -264,18 +264,18 @@ fn test_op_log_word_wrap() { // ui.log-word-wrap option works insta::assert_snapshot!(render(&["op", "log"], 40, false), @r###" - @ d50e0e495b10 test-username@host.example.com 2001-02-03 04:05:07.000 +07:00 - 2001-02-03 04:05:07.000 +07:00 + @ 19b8089fc78b test-username@host.example.com 2001-02-03 04:05:07.000 +07:00 - 2001-02-03 04:05:07.000 +07:00 │ add workspace 'default' - ◉ 23b83cc0392f test-username@host.example.com 2001-02-03 04:05:07.000 +07:00 - 2001-02-03 04:05:07.000 +07:00 + ◉ f1c462c494be test-username@host.example.com 2001-02-03 04:05:07.000 +07:00 - 2001-02-03 04:05:07.000 +07:00 initialize repo "###); insta::assert_snapshot!(render(&["op", "log"], 40, true), @r###" - @ d50e0e495b10 + @ 19b8089fc78b │ test-username@host.example.com │ 2001-02-03 04:05:07.000 +07:00 - │ 2001-02-03 04:05:07.000 +07:00 │ add workspace 'default' - ◉ 23b83cc0392f + ◉ f1c462c494be test-username@host.example.com 2001-02-03 04:05:07.000 +07:00 - 2001-02-03 04:05:07.000 +07:00 diff --git a/cli/tests/test_workspaces.rs b/cli/tests/test_workspaces.rs index b2445bbce1..423f9343ab 100644 --- a/cli/tests/test_workspaces.rs +++ b/cli/tests/test_workspaces.rs @@ -275,14 +275,14 @@ fn test_workspaces_conflicting_edits() { "###); let stderr = test_env.jj_cmd_failure(&secondary_path, &["st"]); insta::assert_snapshot!(stderr, @r###" - Error: The working copy is stale (not updated since operation d47ba8bebf5d). + Error: The working copy is stale (not updated since operation 5c95db542ebd). Hint: Run `jj workspace update-stale` to update it. See https://github.com/martinvonz/jj/blob/main/docs/working-copy.md#stale-working-copy for more information. "###); // Same error on second run, and from another command let stderr = test_env.jj_cmd_failure(&secondary_path, &["log"]); insta::assert_snapshot!(stderr, @r###" - Error: The working copy is stale (not updated since operation d47ba8bebf5d). + Error: The working copy is stale (not updated since operation 5c95db542ebd). Hint: Run `jj workspace update-stale` to update it. See https://github.com/martinvonz/jj/blob/main/docs/working-copy.md#stale-working-copy for more information. "###); @@ -362,7 +362,7 @@ fn test_workspaces_updated_by_other() { "###); let stderr = test_env.jj_cmd_failure(&secondary_path, &["st"]); insta::assert_snapshot!(stderr, @r###" - Error: The working copy is stale (not updated since operation d47ba8bebf5d). + Error: The working copy is stale (not updated since operation 5c95db542ebd). Hint: Run `jj workspace update-stale` to update it. See https://github.com/martinvonz/jj/blob/main/docs/working-copy.md#stale-working-copy for more information. "###); @@ -543,7 +543,7 @@ fn test_workspaces_forget_multi_transaction() { // the op log should have multiple workspaces forgotten in a single tx let stdout = test_env.jj_cmd_success(&main_path, &["op", "log", "--limit", "1"]); insta::assert_snapshot!(stdout, @r###" - @ dd31c8d01c8b test-username@host.example.com 2001-02-03 04:05:12.000 +07:00 - 2001-02-03 04:05:12.000 +07:00 + @ e18f223ba3d3 test-username@host.example.com 2001-02-03 04:05:12.000 +07:00 - 2001-02-03 04:05:12.000 +07:00 │ forget workspaces second, third │ args: jj workspace forget second third "###); diff --git a/lib/src/merge.rs b/lib/src/merge.rs index d4084ccb67..dde8be11b3 100644 --- a/lib/src/merge.rs +++ b/lib/src/merge.rs @@ -446,15 +446,7 @@ impl Merge> { impl ContentHash for Merge { fn hash(&self, state: &mut impl digest::Update) { - // TODO: just hash values - state.update(&(self.removes().len() as u64).to_le_bytes()); - for value in self.removes() { - value.hash(state); - } - state.update(&(self.adds().len() as u64).to_le_bytes()); - for value in self.adds() { - value.hash(state); - } + self.values.hash(state) } } diff --git a/lib/src/simple_op_store.rs b/lib/src/simple_op_store.rs index 6ca185fde5..429c350fc1 100644 --- a/lib/src/simple_op_store.rs +++ b/lib/src/simple_op_store.rs @@ -625,7 +625,7 @@ mod tests { // Test exact output so we detect regressions in compatibility assert_snapshot!( ViewId::new(blake2b_hash(&create_view()).to_vec()).hex(), - @"7a7d8e33aff631bc3a8a281358e818f3c962d539ec2ced78a40b8221a42a707d51e546c5a6644c435b5764d8a51b29e63c3c107d5a8926d4be74288ea8ac879d" + @"d6d19f3edcb3b2fed6104801b7938e6be1147ab036e9fa81b7624fd5ff0149a6c221c3abb6fb7380c3f37e077a03b234313b44cbb6faca0b8c76f68f24ea7174" ); } From 3dc4ab45e3d6b38417859653448719bcba7438a9 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Fri, 3 Nov 2023 16:20:39 +0900 Subject: [PATCH 5/5] merge: simply print interleaved conflict values in debug output We could apply that for the resolved case, but Resolved/Conflicted label seems more useful than just printing Merge([value]). --- cli/tests/test_chmod_command.rs | 14 +++++------ lib/src/merge.rs | 6 +---- lib/tests/test_conflicts.rs | 42 +++++++++++++-------------------- 3 files changed, 25 insertions(+), 37 deletions(-) diff --git a/cli/tests/test_chmod_command.rs b/cli/tests/test_chmod_command.rs index 21d39a5787..2d523142a8 100644 --- a/cli/tests/test_chmod_command.rs +++ b/cli/tests/test_chmod_command.rs @@ -68,7 +68,7 @@ fn test_chmod_regular_conflict() { let stdout = test_env.jj_cmd_success(&repo_path, &["debug", "tree"]); insta::assert_snapshot!(stdout, @r###" - file: Conflicted { removes: [Some(File { id: FileId("df967b96a579e45a18b8251732d16804b2e56a55"), executable: false })], adds: [Some(File { id: FileId("587be6b4c3f93f93c489c0111bba5596147a26cb"), executable: true }), Some(File { id: FileId("8ba3a16384aacc37d01564b28401755ce8053f51"), executable: false })] } + file: Conflicted([Some(File { id: FileId("587be6b4c3f93f93c489c0111bba5596147a26cb"), executable: true }), Some(File { id: FileId("df967b96a579e45a18b8251732d16804b2e56a55"), executable: false }), Some(File { id: FileId("8ba3a16384aacc37d01564b28401755ce8053f51"), executable: false })]) "###); let stdout = test_env.jj_cmd_success(&repo_path, &["cat", "file"]); insta::assert_snapshot!(stdout, @@ -87,7 +87,7 @@ fn test_chmod_regular_conflict() { let stdout = test_env.jj_cmd_success(&repo_path, &["debug", "tree"]); insta::assert_snapshot!(stdout, @r###" - file: Conflicted { removes: [Some(File { id: FileId("df967b96a579e45a18b8251732d16804b2e56a55"), executable: true })], adds: [Some(File { id: FileId("587be6b4c3f93f93c489c0111bba5596147a26cb"), executable: true }), Some(File { id: FileId("8ba3a16384aacc37d01564b28401755ce8053f51"), executable: true })] } + file: Conflicted([Some(File { id: FileId("587be6b4c3f93f93c489c0111bba5596147a26cb"), executable: true }), Some(File { id: FileId("df967b96a579e45a18b8251732d16804b2e56a55"), executable: true }), Some(File { id: FileId("8ba3a16384aacc37d01564b28401755ce8053f51"), executable: true })]) "###); let stdout = test_env.jj_cmd_success(&repo_path, &["cat", "file"]); insta::assert_snapshot!(stdout, @@ -104,7 +104,7 @@ fn test_chmod_regular_conflict() { let stdout = test_env.jj_cmd_success(&repo_path, &["debug", "tree"]); insta::assert_snapshot!(stdout, @r###" - file: Conflicted { removes: [Some(File { id: FileId("df967b96a579e45a18b8251732d16804b2e56a55"), executable: false })], adds: [Some(File { id: FileId("587be6b4c3f93f93c489c0111bba5596147a26cb"), executable: false }), Some(File { id: FileId("8ba3a16384aacc37d01564b28401755ce8053f51"), executable: false })] } + file: Conflicted([Some(File { id: FileId("587be6b4c3f93f93c489c0111bba5596147a26cb"), executable: false }), Some(File { id: FileId("df967b96a579e45a18b8251732d16804b2e56a55"), executable: false }), Some(File { id: FileId("8ba3a16384aacc37d01564b28401755ce8053f51"), executable: false })]) "###); let stdout = test_env.jj_cmd_success(&repo_path, &["cat", "file"]); insta::assert_snapshot!(stdout, @@ -127,7 +127,7 @@ fn test_chmod_regular_conflict() { let stdout = test_env.jj_cmd_success(&repo_path, &["debug", "tree"]); insta::assert_snapshot!(stdout, @r###" - file: Conflicted { removes: [Some(File { id: FileId("df967b96a579e45a18b8251732d16804b2e56a55"), executable: false })], adds: [Some(File { id: FileId("587be6b4c3f93f93c489c0111bba5596147a26cb"), executable: false }), Some(File { id: FileId("8ba3a16384aacc37d01564b28401755ce8053f51"), executable: false })] } + file: Conflicted([Some(File { id: FileId("587be6b4c3f93f93c489c0111bba5596147a26cb"), executable: false }), Some(File { id: FileId("df967b96a579e45a18b8251732d16804b2e56a55"), executable: false }), Some(File { id: FileId("8ba3a16384aacc37d01564b28401755ce8053f51"), executable: false })]) "###); let stdout = test_env.jj_cmd_success(&repo_path, &["cat", "file"]); insta::assert_snapshot!(stdout, @@ -190,7 +190,7 @@ fn test_chmod_file_dir_deletion_conflicts() { let stdout = test_env.jj_cmd_success(&repo_path, &["debug", "tree", "-r=file_dir"]); insta::assert_snapshot!(stdout, @r###" - file: Conflicted { removes: [Some(File { id: FileId("df967b96a579e45a18b8251732d16804b2e56a55"), executable: false })], adds: [Some(File { id: FileId("78981922613b2afb6025042ff6bd878ac1994e85"), executable: false }), Some(Tree(TreeId("133bb38fc4e4bf6b551f1f04db7e48f04cac2877")))] } + file: Conflicted([Some(File { id: FileId("78981922613b2afb6025042ff6bd878ac1994e85"), executable: false }), Some(File { id: FileId("df967b96a579e45a18b8251732d16804b2e56a55"), executable: false }), Some(Tree(TreeId("133bb38fc4e4bf6b551f1f04db7e48f04cac2877")))]) "###); let stdout = test_env.jj_cmd_success(&repo_path, &["cat", "-r=file_dir", "file"]); insta::assert_snapshot!(stdout, @@ -209,7 +209,7 @@ fn test_chmod_file_dir_deletion_conflicts() { let stdout = test_env.jj_cmd_success(&repo_path, &["debug", "tree", "-r=file_deletion"]); insta::assert_snapshot!(stdout, @r###" - file: Conflicted { removes: [Some(File { id: FileId("df967b96a579e45a18b8251732d16804b2e56a55"), executable: false })], adds: [Some(File { id: FileId("78981922613b2afb6025042ff6bd878ac1994e85"), executable: false }), None] } + file: Conflicted([Some(File { id: FileId("78981922613b2afb6025042ff6bd878ac1994e85"), executable: false }), Some(File { id: FileId("df967b96a579e45a18b8251732d16804b2e56a55"), executable: false }), None]) "###); let stdout = test_env.jj_cmd_success(&repo_path, &["cat", "-r=file_deletion", "file"]); insta::assert_snapshot!(stdout, @@ -233,7 +233,7 @@ fn test_chmod_file_dir_deletion_conflicts() { let stdout = test_env.jj_cmd_success(&repo_path, &["debug", "tree", "-r=file_deletion"]); insta::assert_snapshot!(stdout, @r###" - file: Conflicted { removes: [Some(File { id: FileId("df967b96a579e45a18b8251732d16804b2e56a55"), executable: true })], adds: [Some(File { id: FileId("78981922613b2afb6025042ff6bd878ac1994e85"), executable: true }), None] } + file: Conflicted([Some(File { id: FileId("78981922613b2afb6025042ff6bd878ac1994e85"), executable: true }), Some(File { id: FileId("df967b96a579e45a18b8251732d16804b2e56a55"), executable: true }), None]) "###); let stdout = test_env.jj_cmd_success(&repo_path, &["cat", "-r=file_deletion", "file"]); insta::assert_snapshot!(stdout, diff --git a/lib/src/merge.rs b/lib/src/merge.rs index dde8be11b3..b4702d260d 100644 --- a/lib/src/merge.rs +++ b/lib/src/merge.rs @@ -125,11 +125,7 @@ impl Debug for Merge { if let Some(value) = self.as_resolved() { f.debug_tuple("Resolved").field(value).finish() } else { - // TODO: just print values? - f.debug_struct("Conflicted") - .field("removes", &self.removes().collect_vec()) - .field("adds", &self.adds().collect_vec()) - .finish() + f.debug_tuple("Conflicted").field(&self.values).finish() } } } diff --git a/lib/tests/test_conflicts.rs b/lib/tests/test_conflicts.rs index b39acb2f5e..0837d63a6e 100644 --- a/lib/tests/test_conflicts.rs +++ b/lib/tests/test_conflicts.rs @@ -304,27 +304,23 @@ line 5 right @r###" Some( [ - Conflicted { - removes: [ - "line 1\nline 2\n", - ], - adds: [ + Conflicted( + [ "line 1 left\nline 2 left\n", + "line 1\nline 2\n", "line 1 right\nline 2\n", ], - }, + ), Resolved( "line 3\n", ), - Conflicted { - removes: [ - "line 4\nline 5\n", - ], - adds: [ + Conflicted( + [ "line 4\nline 5 left\n", + "line 4\nline 5\n", "line 4 right\nline 5 right\n", ], - }, + ), ], ) "###); @@ -464,15 +460,13 @@ line 5 Resolved( "line 1\n", ), - Conflicted { - removes: [ - "line 2\nline 3\nline 4\n", - ], - adds: [ + Conflicted( + [ "line 2\nleft\nline 4\n", + "line 2\nline 3\nline 4\n", "right\n", ], - }, + ), Resolved( "line 5\n", ), @@ -511,17 +505,15 @@ line 5 Resolved( "line 1\n", ), - Conflicted { - removes: [ - "line 2\nline 3\nline 4\n", - "line 2\nline 3\nline 4\n", - ], - adds: [ + Conflicted( + [ "line 2\nleft\nline 4\n", + "line 2\nline 3\nline 4\n", "right\n", + "line 2\nline 3\nline 4\n", "line 2\nforward\nline 3\nline 4\n", ], - }, + ), Resolved( "line 5\n", ),