Gitattributes EOL conversion support #8266

06393993 · 2025-12-09T05:02:48Z

Checklist

If applicable:

I have updated CHANGELOG.md
I have updated the documentation (README.md, docs/, demos/)
I have updated the config schema (cli/src/config-schema.json)
I have added/updated tests to cover my changes

Hi Reviewers,

Before you start the actual code review, let's make sure we are on the same page on the scope and requirements of this PR:

The scope of this PR is local_working_copy. Therefore, external diff tools and external merge tools are out of scope. The jj diff, jj show and jj file show¹ are also out of scope, and will only show the contents directly retrieved from Store without EOL conversion.
While external diff tools and external merge tools, it's bad to not support EOL conversion for those 2 cases, where the user can modify files. I will file an issue to discuss how to implement such support in a separate issue.
For conflicts, on snapshot, the EOL conversion is applied before the conflict is parsed and written back to the store; on update, the conflict is materialized before the EOL conversion is applied, i.e., the EOL conversion is always applied to contents with conflict markers.²
Merge works on the original contents from Store without EOL conversion. This is an existing behavior, and this PR won't change that.³
We shouldn't design GitAttributes the same way as GitIgnore, because:
- The caller is responsible to build the tree graph by chain_with_file, which is fine in the snapshot path in local_working_copy, but is problematic for the update path in local_working_copy and when we implement external diff tools support and external merge tools support. This justifies the design that GitAttributes exposes one single GitAttributes::search API apart from initialization.
- The current GitIgnore design only reads from the disk. However, if the .gitattributes file is removed from the sparse pattern, we should read the .gitattributes from the Store. On update, we should read the gitattributes file from from the Store instead from the disk. On snpashot, we should read from the disk. This justifies the separate abstraction of FileLoader.
When the .gitattributes file doesn't exist on the disk, but exists in the Store, the .gitattributes from the Store will be used, and vice versa. In case the .gitattributes file exists in both the Store and the disk, on snapshot, the file on the disk takes the priority, and on update, the file from the Store takes the priority.⁴
We plan to implement the complete support for text, eol, crlf, and core.eol. Details can be found in the gitattributes doc, and my detailed design doc. You should just focus on the Goals/Requirements section of my verbose design doc. While it can be too complicated for the complete support, and we probably want to get rid of some of the features, please provide guidance on what features we want to keep, and what features we want to get rid of, so that I know how to proceed with this PR.
You don't have go through other minor details in the Context and Scope section of this doc and this doc, but I'd appreciate it if you can. Thanks.

The overview of the design can be found at here and here. You can just take a look at the graph if it's too verbose.

I leave the commit message almost empty so that I don't have to rewrite the commit message before we reach an agreement on the big picture of the design. The same to docs. I know I have introduced new settings and new features that need documentation, but I do want to write them in details after the UX is almost decided. Sorry for the incompleteness, and thanks for the review.

jj diff, jj show and jj file show are more closely related to the diff gitattributes support, which I will open a separate issue. git-lfs makes use of the diff gitattributes. ↩
This behavior exists when we first introduce the working-copy.eol-conversion setting, and this PR doesn't change that. The change to this behavior is out of scope of this PR. However, I will open an issue to discuss whether this behavior is expected, and do we want to change that. This can be more of an issue when we consider the filter gitattributes support: should we send contents with conflict markers to the filter process or should we send the different sides of contents to the filter process and materialize the conflict from the filter output? ↩
We may want to add conversion hooks when merge happens for jj. This is more related to the merge gitattributes support, which I will open a separate issue. git-lfs makes use of the merge gitattributes. ↩
While this follows the gitattributes document, this can result in weird behavior when we try to remove a .gitattributes file and snapshot: the removal won't take effect until another snapshot. ↩

Introduce an API internal to `jj-lib` that future git attributes related features could use to query the git attributes information associated with a file. The design can be found at jj-vcs#7164.

PhilipMetzger

leaving a small review with some nits

PhilipMetzger · 2025-12-11T22:28:52Z

lib/src/gitattributes.rs

+/// The error type for `gitattributes` operations.
+///
+/// Errors mostly originate from reading from the store, reading from the file
+/// system, and path conversion.


nit: move above the derive to be consistent with the rest of the codebase

PhilipMetzger · 2025-12-11T22:39:28Z

lib/src/gitattributes.rs

+ let attributes_names = expected_states
+ .iter()
+ .map(|(name, _)| *name)
+ .collect::<Vec<_>>();


nit: should be collect_vec()

PhilipMetzger · 2025-12-11T22:44:32Z

lib/src/gitattributes.rs

+ if !metadata.is_file() {
+ return Ok(None);
+ }
+ let file = match std::fs::File::open(&path) {


nit: import std::fs

PhilipMetzger · 2025-12-11T22:44:57Z

lib/src/gitattributes.rs

+ // link.
+ let metadata = match std::fs::symlink_metadata(&path) {
+ Ok(metadata) => metadata,
+ Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(None),


nit: import std::io to shorten this

PhilipMetzger · 2025-12-11T22:47:29Z

lib/tests/test_gitattributes.rs

+ tree_builder(&test_repo.repo, path)
+ .into_iter()
+ .map(|tree| tree.id().clone())
+ .collect::<Vec<_>>(),


nit: can also be collect_vec()

PhilipMetzger · 2025-12-11T22:47:49Z

lib/tests/test_gitattributes.rs

+ tree_builder(&test_repo.repo, path)
+ .into_iter()
+ .map(|tree| tree.id().clone())
+ .collect::<Vec<_>>(),


PhilipMetzger · 2025-12-11T22:48:06Z

lib/tests/test_gitattributes.rs

+ tree_builder(&test_repo.repo, path, contents)
+ .into_iter()
+ .map(|tree| tree.id().clone())
+ .collect::<Vec<_>>(),


PhilipMetzger · 2025-12-11T22:49:32Z

lib/src/eol.rs

+#[derive(PartialEq, Eq, Debug, Clone)]
+pub(crate) struct EolGitAttributes {
+ pub eol: State,
+ pub text: State,
+ pub crlf: State,
+}


nit: missing documentation even though parts are probably self-explanatory.

PhilipMetzger · 2025-12-11T23:00:28Z

lib/src/eol.rs

+enum ConvertMode {
+ /// Apply EOL conversion on both snapshot and update. Equivalent to
+ /// `eol=crlf`.
+ InputOutput,
+ /// Apply EOL conversion only on snapshot. Equivalent to `eol=lf`.
+ Input,
+}
+
+enum EffectiveEolMode {


nit: add some toplevel doc comment to both enums

PhilipMetzger · 2025-12-11T23:01:37Z

lib/src/eol.rs

+ let git_attributes = if self.settings.use_git_attributes {
+ // We only read the gitattributes file if necessary to save the cost for users
+ // who don't use gitattributes.
+ get_git_attributes().await?
+ } else {
+ EolGitAttributes {
+ eol: State::Unspecified,
+ text: State::Unspecified,
+ crlf: State::Unspecified,
+ }
+ };


nit: move this to a helper since we have two callers

06393993 requested a review from a team as a code owner December 9, 2025 05:02

06393993 force-pushed the gitattr-eol branch 3 times, most recently from 1770764 to e1a1da0 Compare December 9, 2025 05:15

06393993 added 2 commits December 8, 2025 21:35

gitattributes: read gitattributes files

22227d0

Introduce an API internal to `jj-lib` that future git attributes related features could use to query the git attributes information associated with a file. The design can be found at jj-vcs#7164.

eol: gitattributes support

aa6b698

06393993 force-pushed the gitattr-eol branch from e1a1da0 to aa6b698 Compare December 9, 2025 05:35

PhilipMetzger reviewed Dec 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Gitattributes EOL conversion support #8266

Gitattributes EOL conversion support #8266

Uh oh!

06393993 commented Dec 9, 2025 •

edited

Loading

PhilipMetzger left a comment

PhilipMetzger Dec 11, 2025 •

edited

Loading

PhilipMetzger Dec 11, 2025

PhilipMetzger Dec 11, 2025

PhilipMetzger Dec 11, 2025

PhilipMetzger Dec 11, 2025

PhilipMetzger Dec 11, 2025

PhilipMetzger Dec 11, 2025

PhilipMetzger Dec 11, 2025

PhilipMetzger Dec 11, 2025

PhilipMetzger Dec 11, 2025

Labels

2 participants

Gitattributes EOL conversion support #8266

Are you sure you want to change the base?

Gitattributes EOL conversion support #8266

Uh oh!

Conversation

06393993 commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Footnotes

PhilipMetzger left a comment

Choose a reason for hiding this comment

PhilipMetzger Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Labels

2 participants

06393993 commented Dec 9, 2025 •

edited

Loading

PhilipMetzger Dec 11, 2025 •

edited

Loading