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

Added logic to preserve carriage returns when updating the manifest file #14983

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 32 additions & 1 deletion src/cargo/util/toml_mut/manifest.rs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this does what we want. However, I am not sure if this should be in toml_edit or in Cargo. Found one relevant comment from an issue: toml-rs/toml#752 (comment).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#14863 isn't marked as S-accepted. I would generally recommend discussing there, and leave this for implementation only dicussion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't marked as S-accepted. I would generally recommend discussing there, and leave this for implementation only dicussion.

ah my bad 😅 I was looking through C-bug issues for small bugs to work on to learn/get some experience with the Cargo codebase. I'll be sure to stick to S-accepted or at least discuss further on the issue before starting/opening a PR.

Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ impl LocalManifest {
/// Write changes back to the file.
pub fn write(&self) -> CargoResult<()> {
let mut manifest = self.manifest.data.to_string();
let raw = match self.embedded.as_ref() {
let mut raw = match self.embedded.as_ref() {
Some(Embedded::Implicit(start)) => {
if !manifest.ends_with("\n") {
manifest.push_str("\n");
Expand All @@ -321,6 +321,11 @@ impl LocalManifest {
}
None => manifest,
};

if self.is_crlf() {
raw = to_crlf_line_ending(&raw);
}

let new_contents_bytes = raw.as_bytes();

cargo_util::paths::write_atomic(&self.path, new_contents_bytes)
Expand Down Expand Up @@ -566,6 +571,10 @@ impl LocalManifest {
}
status
}

fn is_crlf(&self) -> bool {
return self.raw.contains("\r\n");
}
}

impl std::fmt::Display for LocalManifest {
Expand Down Expand Up @@ -753,3 +762,25 @@ fn remove_array_index(array: &mut toml_edit::Array, index: usize) {
array.set_trailing(merged_lines);
}
}

fn to_crlf_line_ending(input: &str) -> String {
let mut result = String::with_capacity(input.len());
let mut chars = input.chars().peekable();

while let Some(c) = chars.next() {
match c {
'\r' if chars.peek() == Some(&'\n') => {
chars.next();
result.push('\r');
result.push('\n');
}
'\n' => {
result.push('\r');
result.push('\n');
}
_ => result.push(c),
}
}

result
}
1 change: 1 addition & 0 deletions tests/testsuite/cargo_add/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ mod path_base_unstable;
mod path_dev;
mod path_inferred_name;
mod path_inferred_name_conflicts_full_feature;
mod preserve_carriage_returns;
mod preserve_dep_std_table;
mod preserve_features_sorted;
mod preserve_features_table;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
[workspace]

[package]
name = "cargo-list-test-fixture"
version = "0.0.0"
edition = "2015"
Empty file.
36 changes: 36 additions & 0 deletions tests/testsuite/cargo_add/preserve_carriage_returns/mod.rs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be auto-converted, if core.autocrlf = true is set in git configuration on Linux. So the test always passes even without this patch. I haven't tested it locally though. Maybe we should have a .gitattributes file and set the file eol=crlf?

Slightly related, in contributing doc we recommend splitting new tests into its own commit, and it still passes CI. Then followed by a fix and a test update. The test change in the second commit tell that that the bug is actually there, and the patch really fixes it.

Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
use cargo_test_support::compare::assert_ui;
use cargo_test_support::current_dir;
use cargo_test_support::file;
use cargo_test_support::prelude::*;
use cargo_test_support::str;
use cargo_test_support::Project;

#[cargo_test]
fn case() {
cargo_test_support::registry::init();
cargo_test_support::registry::Package::new("my-package", "0.1.0").publish();

let project = Project::from_template(current_dir!().join("in"));
let project_root = project.root();
let cwd = &project_root;

snapbox::cmd::Command::cargo_ui()
.arg("add")
.arg_line("my-package")
.current_dir(cwd)
.assert()
.success()
.stdout_eq(str![""])
.stderr_eq(file!["stderr.term.svg"]);

// Verify the content matches first (for nicer error output)
assert_ui().subset_matches(current_dir!().join("out"), &project_root);

// Snapbox normalizes lines so we also need to do a string comparision to verify line endings
let expected = current_dir!().join("out/Cargo.toml");
let actual = project_root.join("Cargo.toml");
assert_eq!(
std::fs::read_to_string(expected).unwrap(),
std::fs::read_to_string(actual).unwrap()
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
[workspace]

[package]
name = "cargo-list-test-fixture"
version = "0.0.0"
edition = "2015"

[dependencies]
my-package = "0.1.0"
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
1 change: 1 addition & 0 deletions tests/testsuite/cargo_remove/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ mod optional_dep_feature;
mod optional_dep_feature_formatting;
mod optional_feature;
mod package;
mod preserve_carriage_returns;
mod remove_basic;
mod script;
mod script_last;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
[workspace]

[package]
name = "cargo-list-test-fixture"
version = "0.0.0"
edition = "2015"

[dependencies]
my-package = "0.1.0"
Empty file.
36 changes: 36 additions & 0 deletions tests/testsuite/cargo_remove/preserve_carriage_returns/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
use cargo_test_support::compare::assert_ui;
use cargo_test_support::current_dir;
use cargo_test_support::file;
use cargo_test_support::prelude::*;
use cargo_test_support::str;
use cargo_test_support::Project;

#[cargo_test]
fn case() {
cargo_test_support::registry::init();
cargo_test_support::registry::Package::new("my-package", "0.1.0").publish();

let project = Project::from_template(current_dir!().join("in"));
let project_root = project.root();
let cwd = &project_root;

snapbox::cmd::Command::cargo_ui()
.arg("remove")
.arg_line("my-package")
.current_dir(cwd)
.assert()
.success()
.stdout_eq(str![""])
.stderr_eq(file!["stderr.term.svg"]);

// Verify the content matches first (for nicer error output)
assert_ui().subset_matches(current_dir!().join("out"), &project_root);

// Snapbox normalizes lines so we also need to do a string comparision to verify line endings
let expected = current_dir!().join("out/Cargo.toml");
let actual = project_root.join("Cargo.toml");
assert_eq!(
std::fs::read_to_string(expected).unwrap(),
std::fs::read_to_string(actual).unwrap()
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
[workspace]

[package]
name = "cargo-list-test-fixture"
version = "0.0.0"
edition = "2015"
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading