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/merge_tools/builtin.rs

@@ -386,7 +386,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.
@@ -421,7 +420,6 @@ fn apply_diff_builtin(
                     store,
                     path,
                     contents,
-                    conflict_marker_style,
                     MIN_CONFLICT_MARKER_LEN, // TODO: use the materialization parameter
                 )
                 .block_on()?;
@@ -559,15 +557,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)
 }
 
@@ -771,15 +762,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]
@@ -1809,15 +1792,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/external.rs

@@ -192,10 +192,6 @@ fn run_mergetool_external_single_file(
         file,
     } = merge_tool_file;
 
-    let conflict_marker_style = editor
-        .conflict_marker_style
-        .unwrap_or(default_conflict_marker_style);
-
     let uses_marker_length = find_all_variables(&editor.merge_args).contains(&"marker_length");
 
     // If the merge tool doesn't get conflict markers pre-populated in the output
@@ -209,7 +205,9 @@ fn run_mergetool_external_single_file(
     };
     let initial_output_content = if editor.merge_tool_edits_conflict_markers {
         let options = ConflictMaterializeOptions {
-            marker_style: conflict_marker_style,
+            marker_style: editor
+                .conflict_marker_style
+                .unwrap_or(default_conflict_marker_style),
             marker_len: Some(conflict_marker_len),
         };
         materialize_merge_result_to_bytes(&file.contents, &options)
@@ -294,7 +292,6 @@ fn run_mergetool_external_single_file(
             store,
             repo_path,
             output_file_contents.as_slice(),
-            conflict_marker_style,
             conflict_marker_len,
         )
         .block_on()?

lib/src/conflicts.rs

@@ -923,27 +923,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?;
-    let materialize_options = ConflictMaterializeOptions {
-        marker_style: conflict_marker_style,
-        marker_len: Some(conflict_marker_len),
-    };
-    materialize_merge_result(&merge_hunk, &mut old_content, &materialize_options).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(
@@ -960,6 +943,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

@@ -1621,7 +1621,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.
@@ -1634,7 +1641,6 @@ impl FileSnapshotter<'_> {
                 self.store(),
                 repo_path,
                 &contents,
-                self.tree_state.conflict_marker_style,
                 conflict_marker_len as usize,
             )
             .await?;