From 8157d4ff636e0621a92a74317be1d56fa74e5254 Mon Sep 17 00:00:00 2001
From: Martin von Zweigbergk <martinvonz@google.com>
Date: Mon, 23 Oct 2023 20:01:00 -0700
Subject: [PATCH] merge: materialize conflicts in executable files like regular
 files

AFAICT, all callers of `Merge::to_file_merge()` are already well
prepared for working with executable files. It's called from these
places:

* `local_working_copy.rs`: Materialized conflicts are correctly
  updated using `Merge::with_new_file_ids()`.

* `merge_tools/`: Same as above.

* `cmd_cat()`: We already ignore the executable bit when we print
  non-conflicted files, so it makes sense to also ignore it for
  conflicted files.

* `git_diff_part()`: We print all conflicts with mode "100644" (the
  mode for regular files). Maybe it's best to use "100755" for
  conflicts that are unambiguously executable, or maybe it's better to
  use a fake mode like "000000" for all conflicts. Either way, the
  current behavior seems fine.

* `diff_content()`: We use the diff content in various diff
  formats. We could add more detail about the executable bits in some
  of them, but I think the current output is fine. For example,
  instead of our current "Created conflict in my-file", we could say
  "Created conflict in executable file my-file" or "Created conflict
  in ambiguously executable file my-file". That's getting verbose,
  though.

So, I think all we need to do is to make `Merge::to_file_merge()` not
require its inputs to be non-executable.

Closes #1279.
---
 CHANGELOG.md                    |  3 +++
 cli/tests/test_chmod_command.rs | 31 ++++++++++++++++++++-----------
 lib/src/merge.rs                | 11 +++++------
 3 files changed, 28 insertions(+), 17 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index b030545ec..b3ad1cd41 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -66,6 +66,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   in the working copy no longer leads to a crash
   ([#976](https://github.com/martinvonz/jj/issues/976)).
 
+* Conflicts in executable files can now be resolved just like conflicts in
+  non-executable files ([#1279](https://github.com/martinvonz/jj/issues/1279)).
+
 ## [0.10.0] - 2023-10-04
 
 ### Breaking changes
diff --git a/cli/tests/test_chmod_command.rs b/cli/tests/test_chmod_command.rs
index 8ba47e5a8..21d39a578 100644
--- a/cli/tests/test_chmod_command.rs
+++ b/cli/tests/test_chmod_command.rs
@@ -73,10 +73,13 @@ fn test_chmod_regular_conflict() {
     let stdout = test_env.jj_cmd_success(&repo_path, &["cat", "file"]);
     insta::assert_snapshot!(stdout, 
     @r###"
-    Conflict:
-      Removing file with id df967b96a579e45a18b8251732d16804b2e56a55
-      Adding executable file with id 587be6b4c3f93f93c489c0111bba5596147a26cb
-      Adding file with id 8ba3a16384aacc37d01564b28401755ce8053f51
+    <<<<<<<
+    %%%%%%%
+    -base
+    +x
+    +++++++
+    n
+    >>>>>>>
     "###);
 
     // Test chmodding a conflict
@@ -89,10 +92,13 @@ fn test_chmod_regular_conflict() {
     let stdout = test_env.jj_cmd_success(&repo_path, &["cat", "file"]);
     insta::assert_snapshot!(stdout, 
     @r###"
-    Conflict:
-      Removing executable file with id df967b96a579e45a18b8251732d16804b2e56a55
-      Adding executable file with id 587be6b4c3f93f93c489c0111bba5596147a26cb
-      Adding executable file with id 8ba3a16384aacc37d01564b28401755ce8053f51
+    <<<<<<<
+    %%%%%%%
+    -base
+    +x
+    +++++++
+    n
+    >>>>>>>
     "###);
     test_env.jj_cmd_ok(&repo_path, &["chmod", "n", "file"]);
     let stdout = test_env.jj_cmd_success(&repo_path, &["debug", "tree"]);
@@ -232,8 +238,11 @@ fn test_chmod_file_dir_deletion_conflicts() {
     let stdout = test_env.jj_cmd_success(&repo_path, &["cat", "-r=file_deletion", "file"]);
     insta::assert_snapshot!(stdout,
     @r###"
-    Conflict:
-      Removing executable file with id df967b96a579e45a18b8251732d16804b2e56a55
-      Adding executable file with id 78981922613b2afb6025042ff6bd878ac1994e85
+    <<<<<<<
+    +++++++
+    a
+    %%%%%%%
+    -base
+    >>>>>>>
     "###);
 }
diff --git a/lib/src/merge.rs b/lib/src/merge.rs
index 7c77ffe53..159172090 100644
--- a/lib/src/merge.rs
+++ b/lib/src/merge.rs
@@ -447,15 +447,14 @@ impl Merge<Option<TreeValue>> {
                 .all(|value| matches!(value, Some(TreeValue::Tree(_)) | None))
     }
 
-    /// If this merge contains only non-executable files or absent entries,
-    /// returns a merge of the `FileId`s`.
+    /// If this merge contains only files or absent entries, returns a merge of
+    /// the `FileId`s`. The executable bits will be ignored. Use
+    /// `Merge::with_new_file_ids()` to produce a new merge with the original
+    /// executable bits preserved.
     pub fn to_file_merge(&self) -> Option<Merge<Option<FileId>>> {
         self.maybe_map(|term| match term {
             None => Some(None),
-            Some(TreeValue::File {
-                id,
-                executable: false,
-            }) => Some(Some(id.clone())),
+            Some(TreeValue::File { id, executable: _ }) => Some(Some(id.clone())),
             _ => None,
         })
     }