local_working_copy: skip EOL conversion if the file in Store has CRLF
#7031
Conversation
06393993
force-pushed
the
skip_crlf_in_store
branch
2 times, most recently
from
6cfae6e to
3dd1993
Compare
| match byte { | ||
| b'\0' => res.null += 1, | ||
| b'\r' => { | ||
| if bytes.peek() == Some(&&b'\n') { |
There was a problem hiding this comment.
nit: bytes.next_if_eq() to not re-read \n?
lib/src/eol.rs
Outdated
| read_old_contents: impl AsyncFnOnce() -> Result<Merge<Option<R>>, E>, | ||
| ) -> Result< | ||
| Box<dyn AsyncRead + Send + Unpin + 'a>, | ||
| impl std::error::Error + Send + Sync + 'static, |
There was a problem hiding this comment.
nit: simply return Box<dyn std::error::Error + Send + Sync>.
lib/src/eol.rs
Outdated
| &self, | ||
| mut contents: impl AsyncRead + Send + Unpin + 'a, | ||
| ) -> Result<Box<dyn AsyncRead + Send + Unpin + 'a>, std::io::Error> { | ||
| read_old_contents: impl AsyncFnOnce() -> Result<Merge<Option<R>>, E>, |
There was a problem hiding this comment.
nit: maybe you might expect, and I know you would disagree, but I would let the caller check contents of the old/new files because this function is getting complicated.
lib/src/local_working_copy.rs
Outdated
| old_file_ids | ||
| .try_map_async(async |file_id| { | ||
| let Some(file_id) = file_id else { | ||
| return Ok(None); | ||
| }; | ||
| self.store() | ||
| .read_file(repo_path, file_id) | ||
| .await | ||
| .map(Option::Some) | ||
| }) | ||
| .await |
There was a problem hiding this comment.
Perhaps, we should also update the materialization function to test conflict contents one by one, instead of converting a materialized conflict data to CRLF.
btw, it seems that we don't apply EOL conversion to the conflict content written by write_conflict(). That's probably a bug.
There was a problem hiding this comment.
Perhaps, we should also update the materialization function to test conflict contents one by one, instead of converting a materialized conflict data to CRLF.
Will do.
it seems that we don't apply EOL conversion to the conflict content written by write_conflict(). That's probably a bug.
Created #7132.
There was a problem hiding this comment.
Actually, I think we should probably just convert EOL for the materialized conflict data on snapshot - when working-copy.eol-conversion = "input-output" the conflict marker lines are also likely to end with CRLF, and we should convert them to LF EOL on snapshot. Besides, I don't think it's possible to track the old EOL data for every side or am I missing anything?
There was a problem hiding this comment.
Actually, I think we should probably just convert EOL for the materialized conflict data on snapshot
If the writer side does that, yes, I think that's good.
I don't think it's possible to track the old EOL data for every side or am I missing anything?
If we didn't convert line endings, the materialized content would preserve the original line endings, and LF<->CRLF changes would be rendered as hunks. I don't think that would be useful as a textual representation, though.
| }), | ||
| disk_path, | ||
| ) | ||
| .await?; |
There was a problem hiding this comment.
Another idea is to remember whether we applied EOL translation to the checked-out content. I'm not sure if the problem can be fully addressed, but the implementation would be simpler.
pub struct FileState {
...
pub eol: bool, // or enum?
}
There was a problem hiding this comment.
Changed to this implementation. I don't think the implementation is necessarily simpler, because the logic to correctly update FileState::has_crlf_in_store scatters in 6 different places in local_working_copy.rs:
- 2 different locations in
FileSnapshotter::write_path_to_store, one for files without conflict(which calls intoFileSnapshotter::write_file_to_store), one for conflicts. TreeState::write_fileTreeState::write_conflict- 2 different locations in
TreeState::reset, one for files without conflict, one for conflicts.
However, the performance for snapshot should be better, because we don't have to read the contents from Store on snapshot at the cost of the performance of TreeState::reset, which should be rarely used anyway.
WDYT? Do we prefer the design that write the information to FileState or do we prefer the old design where we read the contents from the Store on EOL conversion when necessary?
| // First we create a file with CRLF EOL in the Store by using the none EOL | ||
| // conversion setting. | ||
| let no_eol_conversion_settings = | ||
| base_user_settings_with_extra_configs("working-copy.eol-conversion = \"none\"\n"); |
There was a problem hiding this comment.
nit: use a raw string here to avoid the needless escaping
lib/src/eol.rs
Outdated
| #[derive(Default)] | ||
| struct Stats { | ||
| crlf: usize, | ||
| null: usize, | ||
| } |
There was a problem hiding this comment.
nit: missing documentation
06393993
force-pushed
the
skip_crlf_in_store
branch
6 times, most recently
from
2d73ba8 to
cd152bb
Compare
| /// Creates a future which will calculate the [`Stats`]. | ||
| #[derive(Debug)] | ||
| #[must_use = "streams do nothing unless polled"] | ||
| pub(crate) struct WithStats<'a, T> { |
There was a problem hiding this comment.
Can't we instead make convert_eol_for_*() functions return whether line endings were transformed? If I understand the problem correctly, we shouldn't apply EOL conversion if the stored content includes mixed line endings, and if EOL wasn't converted when checking out, we shouldn't apply the reverse transformation when snapshotting the working-copy content.
There was a problem hiding this comment.
Can't we instead make convert_eol_for_*() functions return whether line endings were transformed?
We can, but I think that will complicate the lifetime of the future returned, because even if we don't convert EOL on checkout, it doesn't necesarrily mean we don't apply EOL conversion on next snapshot. See below for why.
And in case we don't apply EOL conversion on checkout, whether the file in store contains CRLF is only known after the contents from the Store are read when writing the file to the disk later.
we shouldn't apply EOL conversion if the stored content includes mixed line endings
Yes in general, but precisely, if the stored content includes only CRLF EOL, we also won't apply EOL conversion on checkout.
and if EOL wasn't converted when checking out, we shouldn't apply the reverse transformation when snapshotting the working-copy content.
That's not the case. In the following situations, we don't apply EOL conversion on checkout, but should apply EOL conversion on next snapshot:
- The EOL conversion setting is set to "input-output". The contents in the Store is binary and doesn't contain CRLF. We modify the contents to text and uses CRLF EOL.
- The contents in the Store are text, and contain only LF EOL. The EOL conversion setting is set to "input". On checkout, we don't apply the EOL conversion. We add lines ending with CRLF to the contents. On snapshot, we should apply the CRLF -> LF EOL conversion.
And we have some corner case situations:
- On checkout, the EOL conversion setting is set to "none", and the content doesn't include CRLF. We add some CRLF to the contents. On snapshot, the EOL conversion setting is changed to "input-output", the EOL conversion should be applied.
- This is more of future thing. The contents in Store doesn't have CRLF. On checkout, the
gitattributesassociated with the file are-text, which means no EOL conversion should be applied. We add CRLF to the contents. On snapshot, thegitattributesassociated are changed totext, and the EOL conversion should be applied.
This actually reminds me, if the old file is binary, but the file is then changed to text, we should probably always apply EOL conversion regardless of the existence of CRLF, but that will be another issue/PR, and needs separate investigation.
There was a problem hiding this comment.
That's not the case. In the following situations, we don't apply EOL conversion on checkout, but should apply EOL conversion on next snapshot:
The EOL conversion setting is set to "input-output". The contents in the Store is binary and doesn't contain CRLF. We modify the contents to text and uses CRLF EOL.
The contents in the Store are text, and contain only LF EOL. The EOL conversion setting is set to "input". On checkout, we don't apply the EOL conversion. We add lines ending with CRLF to the contents. On snapshot, we should apply the CRLF -> LF EOL conversion.
I personally don't think (2) should convert line endings back because the working-copy content has mixed line endings. If the user converted the checked-out content to all in CRLF, then, we should convert them back to LF, though.
I'm not sure about (1). If the stored content was binary, my feeling is that it would be safer to not assume the working-copy content were replaced with a text file. If we were to assume the file was replaced, I think we should ignore the line endings in the original binary file, btw.
And we have some corner case situations:
On checkout, the EOL conversion setting is set to "none", and the content doesn't include CRLF. We add some CRLF to the contents. On snapshot, the EOL conversion setting is changed to "input-output", the EOL conversion should be applied.
This is more of future thing. The contents in Store doesn't have CRLF. On checkout, the
gitattributesassociated with the file are-text, which means no EOL conversion should be applied. We add CRLF to the contents. On snapshot, thegitattributesassociated are changed totext, and the EOL conversion should be applied.
In (3) and (4), the last checked-out state would be eol = none (of none|converted|could-not-convert), so we'll determine the snapshot operation based on the current working-copy content.
FWIW, if file mode is explicitly specified by .gitattributes, I think we should apply EOL conversion unconditionally. I don't check the Git's behavior, so I might be wrong, though.
There was a problem hiding this comment.
I make the 4 examples to prove that there are situations where we need to apply EOL conversion on snapshot right after a checkout that doesn't apply EOL conversion. It seems that at least we all agree that at least (3) is such a situation.
Then even in (3), we shouldn't just perform the EOL conversion blindly(and this is the key issue that this PR is all about):
Example a.
- Set the EOL conversion setting to
none - Checkout a change with a
test-file, whose contents are\r\r\r\n. The contents in store are the same, as we are using thenoneEOL setting. - Change the EOL conversion setting to
input-output. And prepend 1 byte0totest-file, resulting1\r\r\r\n. The issue starts. - We snapshot now. The EOL conversion is applied, and the content in store becomes
1\r\r\n. 1CRis eatten unexpectedly. Let's proceed. - Let's prepend 2. The file on disk becomes
21\r\r\n. Let's snapshot. The content in store becomes21\r\n. AnotherCRis eaten unexpectedly.
Of course, the above is not a practical example, but the following one is a little more practical(and is the example described in the assocuated issue #7010):
Example b.
- Set the EOL conversion setting to
none - Checkout a change with a
test-file, whose contents are1\r\n2\r\n3\r\n. The contents in store are the same, as we are using thenoneEOL setting. - Change the EOL conversion setting to
input-output. And prepend4\r\ntotest-fileresulting1\r\n2\r\n3\r\n4\r\n. - We snapshot now. The EOL conversion is applied. All 4 lines of the file are changed, while we really expect a 1 line change.
The solution to the above issue is to always check the content of the file in the store before applying EOL conversion in snapshot, and this is the git behavior. Neither of the following design is as good:
- if we see mixed EOLs on the disk we don't apply EOL conversion on snapshot(neither of the previous examples have files with mixed EOLs)
- if we didn't apply EOL conversion on last checkout, we don't apply EOL conversion on snapshot(while this solves the original issue, it breaks (3), where we all agree that we should apply EOL conversion on snapshot after the user changes the EOL conversion settings)
I am wondering if we all agree that we should not apply EOL conversion if the file in store has CRLF? As of how to obtain that information, we can either read from store(the old implementation) or cache the information on last checkout.
And if so, it becomes awkward to let convert_eol_for_snapshot to return whether CRLF exists in the file in store.
Below are some comments about your thought on (1), (2), and (4) but they should only be remotely related to this PR and the original issue.
I personally don't think (2) should convert line endings back ...
I am sure (2) should convert the EOL. working-copy.eol-conversion = "input" is supposed to repliacate the behavior of the git autocrlf = input config, and convert EOL is what it should do:
This variable can be set to input, in which case no output conversion is performed.
The "input" option is introduced so that if you accidentally introduce CRLFs to your files(e.g., copy paste from a weird web page), git fixes the EOL from CRLFs to LFs for you. Some unofficial sources.
I'm not sure about (1). If the stored content was binary, my feeling is that it would be safer to not assume the working-copy content were replaced with a text file.
I probably should check git's behavior for this case.
We could make the assumption, but it could bring the following confusion. Let's assume the "input-output" EOL conversion setting for the example:
- Create change
awith a binarytestfile. - Create change
bwhich modifiestestfileto besome_command\n. And let's assume that we shouldn't do the EOL conversion when changebis created. I guess in your opinion, after changebis createdFileStatefor thistestfileshould prevent next snapshot from applying EOL conversion. - Append another
more_command\r\nline totestfile, and create changec. Now changecwon't do the EOL conversion, sotestfileissome_command\nmore_command\r\nin store. - We checkout the
root()change, which removes thetestfileand itsFileStatecache from the current working copy. - We checkout back to
b. NowFileStateoftestfileshould be generated based on the contents, which issome_command\n. It seems that there is no reason to generate aFileStatefortestfileto prevent next snapshot from applying EOL conversion. - Append another
more_command\r\nline totestfile, and create changed. When snapshotting EOL conversion is applied, resultingsome_command\nmore_command\n.
And we end up with change c and change d with only EOL difference.
Alternatively, when creating change b(step 2.), the FileState could allow the next snapshot to perform EOL conversion Then, when the user modifies testfile and snapshot again to modify the change b, EOL conversion just happens. The conservative protection only works for one snapshot, which seems to be too short.
If we were to assume the file was replaced, I think we should ignore the line endings in the original binary file, btw.
On that, I somehow agree. But it adds some complexity, and brings an extra questions1. I should also check git's behavior and report.
FWIW, if file mode is explicitly specified by .gitattributes, I think we should apply EOL conversion unconditionally. I don't check the Git's behavior, so I might be wrong, though.
You are right. Example (4) is not good. But if we change the git attributes from Set(text) to Unspecified(!text or just remove the old text attribute). (4) is very similar to (3).
Footnotes
-
Since we probably will add a
FileState::is_binaryfield, should it be influenced by thetextgitattributes, i.e., if our guessing algorithm doesn't think a file is binary, but the file is associated with the-textattributes, what the value ofFileState::is_binaryshould be. ↩
There was a problem hiding this comment.
Example a.
- Set the EOL conversion setting to
none- Checkout a change with a
test-file, whose contents are\r\r\r\n. The contents in store are the same, as we are using thenoneEOL setting.- Change the EOL conversion setting to
input-output. And prepend 1 byte0totest-file, resulting1\r\r\r\n. The issue starts.- We snapshot now. The EOL conversion is applied, and the content in store becomes
1\r\r\n. 1CRis eatten unexpectedly. Let's proceed.- Let's prepend 2. The file on disk becomes
21\r\r\n. Let's snapshot. The content in store becomes21\r\n. AnotherCRis eaten unexpectedly.
I thought \r\r\r\n would be detected as a variant of mixed line endings (CR + CR + CRLF), so EOL conversion would be disabled.
Example b.
- Set the EOL conversion setting to
none- Checkout a change with a
test-file, whose contents are1\r\n2\r\n3\r\n. The contents in store are the same, as we are using thenoneEOL setting.- Change the EOL conversion setting to
input-output. And prepend4\r\ntotest-fileresulting1\r\n2\r\n3\r\n4\r\n.- We snapshot now. The EOL conversion is applied. All 4 lines of the file are changed, while we really expect a 1 line change.
Suppose eol-conversion = input-output is the option to normalize the stored content, I think this behavior is correct. If the file should be kept in CRLF, maybe the path should be listed in .gitattributes?
That said, I don't feel strongly about (3) in the previous discussion. If we want to turn our EOL conversion into safer (or less-lossy) side, I think it's okay to require clean checkout after eol-conversion option is changed.
I am wondering if we all agree that we should not apply EOL conversion if the file in store has CRLF? As of how to obtain that information, we can either read from store(the old implementation) or cache the information on last checkout.
Good question. I thought the stored content would be updated to LF on snapshot. If we have to treat stored CRLF files as binary, and if we need to handle the case (3), I agree we need to rely on the line endings of the stored files.
I personally don't think (2) should convert line endings back ...
I am sure (2) should convert the EOL.
working-copy.eol-conversion = "input"is supposed to repliacate the behavior of the gitautocrlf = inputconfig, and convert EOL is what it should do:
My reasoning is that a file with mixed line endings is no different from a binary file, so any text-based transformation would be turned off (unless explicitly specified as text file.) We can of course treat it as text, and normalize line endings in both input/output directions.
There was a problem hiding this comment.
Suppose eol-conversion = input-output is the option to normalize the stored content, I think this behavior is correct. If the file should be kept in CRLF, maybe the path should be listed in .gitattributes?
Example b. is the original issue this PR tries to fix. If you think Example b. is the correct behavior, can you leave comments on #7010 to confirm that it's indeed the expected behavior? I can add explanation to your comments in the issue and close this PR. Thanks.
I am Ok that we have subtle difference with git on EOL conversion, but I would prefer we document that difference(details in github issues, general description on the doc website) to avoid repeating the same discussion in the future, and add tests to ensure jj does behave like that. We should also continue the discussion on other behaviors we disagree in this comment thread, and move to an issue if that leads to different behaviors from git.
There was a problem hiding this comment.
Before I close this PR, do we want to open new issues on the following 2 questions?
- Should we treat files with mixed EOL as binary file and won't convert the EOL for such files in checkout and snapshot?
- If we don't apply EOL conversion for a file on checkout, should we avoid EOL conversion on snapshot for the same file?
There was a problem hiding this comment.
I don't think we'll need separate issue for each edge case.
06393993
force-pushed
the
skip_crlf_in_store
branch
from
cd152bb to
ed792ad
Compare
* On snapshot, this prevents unexpected modification due to EOL conversion. This aligns with the [git behavior](https://github.com/git/git/blob/1a793261c53507f7c46f748cc76378a9c5bb05cf/convert.c#L536-L538). * On checkout, this prevents the next snapshot from unexpected modification due to EOL conversion. This aligns with the [git behavior](https://github.com/git/git/blob/1a793261c53507f7c46f748cc76378a9c5bb05cf/convert.c#L259-L260). For example, if we have a file whose contents are `a\r\n` in the `Store`. If we checkout the file and append a new line `b\r\n` to the file and snapshot, no matter what the EOL conversion setting is, we don't want the first `a\r\n` line to be changed in the `Store`, because our current change doesn't modify that line at all. On a Windows machine, this change can make snapshot somewhat slower if EOL conversion is enabled(the time spent is 0.96-1.21 times of that without this change): ``` PS C:\Users\x\repo\jj-test-repo\firefox-autocrlf-true> C:\Users\x\repo\jj\target\release\jj.exe config get working-copy.eol-conversion input-output PS C:\Users\x\repo\jj-test-repo\firefox-autocrlf-true> hyperfine --warmup 20 --prepare 'del C:\Users\x\repo\jj-test-repo\firefox-autocrlf-true\.jj\working_copy\tree_state' 'cd C:\Users\x\repo\jj-test-repo\firefox-autocrlf-true && C:\Users\x\repo\jj\target\release\jj.exe debug snapshot' --prepare 'del C:\Users\x\repo\jj-test-repo\firefox-autocrlf-true\.jj\working_copy\tree_state' 'cd C:\Users\x\repo\jj-test-repo\firefox-autocrlf-true && C:\Users\x\repo\jj-head\target\release\jj.exe debug snapshot' --min-runs 30 Benchmark 1: cd C:\Users\x\repo\jj-test-repo\firefox-autocrlf-true && C:\Users\x\repo\jj\target\release\jj.exe debug snapshot Time (mean ± σ): 11.725 s ± 0.660 s [User: 21.133 s, System: 21.815 s] Range (min … max): 10.046 s … 12.286 s 30 runs Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. Benchmark 2: cd C:\Users\x\repo\jj-test-repo\firefox-autocrlf-true && C:\Users\x\repo\jj-head\target\release\jj.exe debug snapshot Time (mean ± σ): 10.643 s ± 0.905 s [User: 17.835 s, System: 18.265 s] Range (min … max): 10.000 s … 12.170 s 30 runs Warning: The first benchmarking run for this command was significantly slower than the rest (12.155 s). This could be caused by (filesystem) caches that were not filled until after the first run. You are already using both the '--warmup' option as well as the '--prepare' option. Consider re-running the benchmark on a quiet system. Maybe it was a random outlier. Alternatively, consider increasing the warmup count. Summary cd C:\Users\x\repo\jj-test-repo\firefox-autocrlf-true && C:\Users\x\repo\jj-head\target\release\jj.exe debug snapshot ran 1.10 ± 0.11 times faster than cd C:\Users\x\repo\jj-test-repo\firefox-autocrlf-true && C:\Users\x\repo\jj\target\release\jj.exe debug snapshot PS C:\Users\x\repo\jj-test-repo\firefox-autocrlf-true> hyperfine --warmup 20 --prepare 'del C:\Users\x\repo\jj-test-repo\firefox-autocrlf-true\.jj\working_copy\tree_state' 'cd C:\Users\x\repo\jj-test-repo\firefox-autocrlf-true && C:\Users\x\repo\jj-head\target\release\jj.exe debug snapshot' --prepare 'del C:\Users\x\repo\jj-test-repo\firefox-autocrlf-true\.jj\working_copy\tree_state' 'cd C:\Users\x\repo\jj-test-repo\firefox-autocrlf-true && C:\Users\x\repo\jj\target\release\jj.exe debug snapshot' --min-runs 30 Benchmark 1: cd C:\Users\x\repo\jj-test-repo\firefox-autocrlf-true && C:\Users\x\repo\jj-head\target\release\jj.exe debug snapshot Time (mean ± σ): 10.086 s ± 0.051 s [User: 16.937 s, System: 16.527 s] Range (min … max): 9.965 s … 10.180 s 30 runs Benchmark 2: cd C:\Users\x\repo\jj-test-repo\firefox-autocrlf-true && C:\Users\x\repo\jj\target\release\jj.exe debug snapshot Time (mean ± σ): 10.407 s ± 0.722 s [User: 17.705 s, System: 18.312 s] Range (min … max): 10.025 s … 12.465 s 30 runs Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. Summary cd C:\Users\x\repo\jj-test-repo\firefox-autocrlf-true && C:\Users\x\repo\jj-head\target\release\jj.exe debug snapshot ran 1.03 ± 0.07 times faster than cd C:\Users\x\repo\jj-test-repo\firefox-autocrlf-true && C:\Users\x\repo\jj\target\release\jj.exe debug snapshot ``` Closes jj-vcs#7010.
06393993
force-pushed
the
skip_crlf_in_store
branch
from
ed792ad to
58f5e1b
Compare
|
Close this PR, because we don't want to fix the original problem in this way. See the original issue for details. |
For example, if we have a file whose contents are
a\r\nin theStore. If we checkout the file and append a new lineb\r\nto the file and snapshot, no matter what the EOL conversion setting is, we don't want the firsta\r\nline to be changed in theStore, because our current change doesn't modify that line at all.On a Windows machine, this change can make snapshot somewhat slower if EOL conversion is enabled(the time spent is 0.96-1.21 times of that without this change):
Closes #7010.
Checklist
If applicable:
CHANGELOG.mdREADME.md,docs/,demos/)cli/src/config-schema.json)