Skip to content

Commit

Permalink
merge: materialize conflicts in executable files like regular files
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
martinvonz committed Oct 24, 2023
1 parent 1257416 commit 92f7b1b
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 17 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
31 changes: 20 additions & 11 deletions cli/tests/test_chmod_command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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"]);
Expand Down Expand Up @@ -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
>>>>>>>
"###);
}
11 changes: 5 additions & 6 deletions lib/src/merge.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
})
}
Expand Down

0 comments on commit 92f7b1b

Please sign in to comment.