conflicts: compare content hash to detect unchanged materialized conflicts

It seemed odd that we had to pass ConflictMarkerStyle to snapshot functions.
Since we just need to check if the on-disk data has changed, we can instead
store and compare content hashes. Conflict files checked out by old jj might be
updated on mtime change, which seems okay. It's less likely to leave conflict
files in the working copy for long and touch them.

updated_from_content() is called by merge tools, and unchanged conflicts may be
updated there. I think that's okay since "jj resolve" is an explicit operation.

test_materialize_parse_roundtrip_different_markers() is simplified as there
should no longer be "pair" of marker styles.

Author: yuja

CHANGELOG.md

@@ -39,6 +39,11 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 * The `conflict` label used for coloring log graph nodes was renamed to
   `conflicted`.
 
+* `jj` now records content hash of materialized conflict files in addition to
+  file modification time, and relies on them to detect changes. Unchanged
+  conflicts checked out by old `jj` may be snapshotted by new `jj`. Run `jj new
+  <REVISION-WITHOUT-CONFLICTS> && jj undo` to check out fresh conflict files.
+
 ### Deprecations
 
 * The on-disk index format has changed. `jj` will write index files in both old

cli/src/cli_util.rs

@@ -1383,14 +1383,12 @@ to the current parents may contain changes from multiple commits.
         if max_new_file_size == 0 {
             max_new_file_size = u64::MAX;
         }
-        let conflict_marker_style = self.env.conflict_marker_style();
         Ok(SnapshotOptions {
             base_ignores,
             fsmonitor_settings,
             progress: None,
             start_tracking_matcher,
             max_new_file_size,
-            conflict_marker_style,
         })
     }
 

cli/src/merge_tools/builtin.rs

@@ -382,7 +382,6 @@ fn apply_diff_builtin(
     right_tree: &MergedTree,
     changed_files: Vec<RepoPathBuf>,
     files: &[scm_record::File],
-    conflict_marker_style: ConflictMarkerStyle,
 ) -> BackendResult<MergedTreeId> {
     // Start with the right tree to match external tool behavior.
     // This ensures unmatched paths keep their values from the right tree.
@@ -417,7 +416,6 @@ fn apply_diff_builtin(
                     store,
                     path,
                     contents,
-                    conflict_marker_style,
                     MIN_CONFLICT_MARKER_LEN, // TODO: use the materialization parameter
                 )
                 .block_on()?;
@@ -555,15 +553,8 @@ 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,
-        conflict_marker_style,
-    )
-    .map_err(BuiltinToolError::BackendError)?;
+    let tree_id = apply_diff_builtin(&store, left_tree, right_tree, changed_files, &result.files)
+        .map_err(BuiltinToolError::BackendError)?;
     Ok(tree_id)
 }
 
@@ -767,15 +758,7 @@ mod tests {
         changed_files: &[RepoPathBuf],
         files: &[scm_record::File],
     ) -> MergedTreeId {
-        apply_diff_builtin(
-            store,
-            left_tree,
-            right_tree,
-            changed_files.to_vec(),
-            files,
-            ConflictMarkerStyle::Diff,
-        )
-        .unwrap()
+        apply_diff_builtin(store, left_tree, right_tree, changed_files.to_vec(), files).unwrap()
     }
 
     #[test]
@@ -1805,15 +1788,8 @@ mod tests {
 
         assert_eq!(changed_files, vec![matched_path.to_owned()]);
 
-        let result_tree_id = apply_diff_builtin(
-            store,
-            &left_tree,
-            &right_tree,
-            changed_files,
-            &files,
-            ConflictMarkerStyle::Diff,
-        )
-        .unwrap();
+        let result_tree_id =
+            apply_diff_builtin(store, &left_tree, &right_tree, changed_files, &files).unwrap();
         let result_tree = store.get_root_tree(&result_tree_id).unwrap();
 
         assert_eq!(

cli/src/merge_tools/diff_working_copies.rs

@@ -8,7 +8,6 @@ use std::sync::Arc;
 
 use futures::StreamExt as _;
 use jj_lib::backend::MergedTreeId;
-use jj_lib::conflicts::ConflictMarkerStyle;
 use jj_lib::fsmonitor::FsmonitorSettings;
 use jj_lib::gitignore::GitIgnoreFile;
 use jj_lib::local_working_copy::TreeState;
@@ -274,7 +273,6 @@ diff editing in mind and be a little inaccurate.
     pub fn snapshot_results(
         self,
         base_ignores: Arc<GitIgnoreFile>,
-        conflict_marker_style: ConflictMarkerStyle,
     ) -> Result<MergedTreeId, DiffEditError> {
         if let Some(path) = self.instructions_path_to_cleanup {
             std::fs::remove_file(path).ok();
@@ -289,7 +287,6 @@ diff editing in mind and be a little inaccurate.
             progress: None,
             start_tracking_matcher: &EverythingMatcher,
             max_new_file_size: u64::MAX,
-            conflict_marker_style,
         })?;
         Ok(output_tree_state.current_tree_id().clone())
     }

cli/src/merge_tools/external.rs

@@ -294,7 +294,6 @@ fn run_mergetool_external_single_file(
             store,
             repo_path,
             output_file_contents.as_slice(),
-            conflict_marker_style,
             conflict_marker_len,
         )
         .block_on()?
@@ -425,7 +424,7 @@ pub fn edit_diff_external(
         }));
     }
 
-    diffedit_wc.snapshot_results(base_ignores, options.conflict_marker_style)
+    diffedit_wc.snapshot_results(base_ignores)
 }
 
 /// Generates textual diff by the specified `tool` and writes into `writer`.

lib/src/conflicts.rs

@@ -954,29 +954,10 @@ pub async fn update_from_content(
     store: &Store,
     path: &RepoPath,
     content: &[u8],
-    conflict_marker_style: ConflictMarkerStyle,
     conflict_marker_len: usize,
 ) -> BackendResult<Merge<Option<FileId>>> {
     let simplified_file_ids = file_ids.simplify();
 
-    // 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());
-    let merge_hunk = extract_as_single_hunk(&simplified_file_ids, store, path).await?;
-    materialize_merge_result_with_marker_len(
-        &merge_hunk,
-        conflict_marker_style,
-        conflict_marker_len,
-        &mut old_content,
-    )
-    .unwrap();
-    if content == old_content {
-        return Ok(file_ids.clone());
-    }
-
     // Parse conflicts from the new content using the arity of the simplified
     // conflicts.
     let Some(mut hunks) = parse_conflict(
@@ -993,6 +974,7 @@ pub async fn update_from_content(
     // check whether the original term ended with a newline. If it didn't, then
     // remove the newline since it was added automatically when materializing.
     if let Some(last_hunk) = hunks.last_mut().filter(|hunk| !hunk.is_resolved()) {
+        let merge_hunk = extract_as_single_hunk(&simplified_file_ids, store, path).await?;
         for (original_content, term) in merge_hunk.iter().zip_eq(last_hunk.iter_mut()) {
             if term.last() == Some(&b'\n') && has_no_eol(original_content) {
                 term.pop();

lib/src/local_working_copy.rs

@@ -1037,7 +1037,6 @@ impl TreeState {
             progress,
             start_tracking_matcher,
             max_new_file_size,
-            conflict_marker_style,
         } = options;
 
         let sparse_matcher = self.sparse_matcher();
@@ -1079,7 +1078,6 @@ impl TreeState {
                 error: OnceLock::new(),
                 progress,
                 max_new_file_size,
-                conflict_marker_style,
                 target_eol_strategy: self.target_eol_strategy.clone(),
             };
             let directory_to_visit = DirectoryToVisit {
@@ -1228,7 +1226,6 @@ struct FileSnapshotter<'a> {
     error: OnceLock<SnapshotError>,
     progress: Option<&'a SnapshotProgress<'a>>,
     max_new_file_size: u64,
-    conflict_marker_style: ConflictMarkerStyle,
     target_eol_strategy: TargetEolStrategy,
 }
 
@@ -1619,7 +1616,14 @@ impl FileSnapshotter<'_> {
                     message: "Failed to read the EOL converted contents".to_string(),
                     err: err.into(),
                 })?;
+            // Check if the new content is unchanged. This makes sure that
+            // unchanged conflicts are not updated.
+            let old_content_hash =
+                current_conflict_data.and_then(|data| data.content_hash.as_deref());
             let new_content_hash = blake2b_hash(&contents);
+            if old_content_hash == Some(&*new_content_hash) {
+                return Ok((current_tree_values.clone(), current_conflict_data.cloned()));
+            }
             // If the file contained a conflict before and is a normal file on
             // disk, we try to parse any conflict markers in the file into a
             // conflict.
@@ -1632,7 +1636,6 @@ impl FileSnapshotter<'_> {
                 self.store(),
                 repo_path,
                 &contents,
-                self.conflict_marker_style,
                 conflict_marker_len as usize,
             )
             .await?;

lib/src/working_copy.rs

@@ -223,8 +223,6 @@ pub struct SnapshotOptions<'a> {
     /// (depending on implementation)
     /// return `SnapshotError::NewFileTooLarge`.
     pub max_new_file_size: u64,
-    /// Expected conflict marker style for checking for changed files.
-    pub conflict_marker_style: ConflictMarkerStyle,
 }
 
 impl SnapshotOptions<'_> {
@@ -236,7 +234,6 @@ impl SnapshotOptions<'_> {
             progress: None,
             start_tracking_matcher: &EverythingMatcher,
             max_new_file_size: u64::MAX,
-            conflict_marker_style: ConflictMarkerStyle::default(),
         }
     }
 }