Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cargo add, etc do not preserve \r\n line endings #14863

Open
epage opened this issue Nov 27, 2024 · 3 comments
Open

cargo add, etc do not preserve \r\n line endings #14863

epage opened this issue Nov 27, 2024 · 3 comments
Labels
C-bug Category: bug Command-add Command-fix Command-remove S-triage Status: This issue is waiting on initial triage.

Comments

@epage
Copy link
Contributor

epage commented Nov 27, 2024

Problem

Generally people want line endings to be consistent. When Cargo modifies a Cargo.toml, it may insert \n independent of what line ending type is used

Steps

  1. Have a Cargo.toml file with \r\n line endings
  2. Run cargo add serde
  3. See that \n line endings are mixed in

Possible Solution(s)

Check if \r\n is present and then convert any stray \ns to \r\n

Notes

See also #14857 as cargo-script has another area where this can be hit

Version


@epage epage added C-bug Category: bug Command-add Command-fix Command-remove S-triage Status: This issue is waiting on initial triage. labels Nov 27, 2024
@epage
Copy link
Contributor Author

epage commented Nov 27, 2024

Somewhat related: #12897

@ranger-ross
Copy link
Contributor

I took a look at how file decides if a file has CRLF line terminators and it also appears to be using the simple approach of just checking the number /r/n in the file is > 0.

I think the proposed solution if checking if the file contains at least one \r\n should work.
A small downside of this is that if a file has mixed line terminates (both \n and \r\n) Cargo would not be able to preserve both.
But this is probably a rare scenario and I don't see a usecase for having both.

Fix no concerns with this approach, I could probably take a stab at implementing this

@weihanglo

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug Command-add Command-fix Command-remove S-triage Status: This issue is waiting on initial triage.
Projects
None yet
Development

No branches or pull requests

3 participants