Fix cargo add overwriting symlinked Cargo.toml files

[RaghavenderSingh]

What does this PR try to resolve?

This PR fixes a bug where cargo add breaks symlinks to Cargo.toml files. Currently, when Cargo.toml is a symlink and cargo add is used to add a dependency, the symlink is replaced with a regular file, breaking the link to the original target file.

This issue was reported in #15241 where a user who relies on symlinked Cargo.toml files found that cargo add breaks their workflow.

Fixes #15241

How should we test and review this PR?

I've modified LocalManifest::write() to check if the path is a symlink, and if so, follow it to get the actual target path. This ensures we write to the actual file rather than replacing the symlink.

I've also added a test in tests/testsuite/cargo_add/symlink.rs that:

1. Creates a symlinked Cargo.toml file
2. Runs cargo add to add a dependency
3. Verifies the symlink is preserved and the dependency is added to the target file

I've manually tested this fix and confirmed it works correctly.

[rustbot]

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[weihanglo]

nit: we prefer contributions split tests into the first commit, with them passing. That way the fix commit will show the change in behavior by how the test changes. This helps test the test and provides an illustration of the PR description for the reviewer and the community at large.

We linked to examples in the contrib docs explaining this

[RaghavenderSingh]

Thanks for the guidance. I've restructured the PR.

[weihanglo]

Note that the first commit is best to pass CI. That means you'll assert the problematic behavior. The "fix" commit then contains both the fix, and the change in test. See also this PR as an example.

In addition, your test is a functional test, but is placed along with other UI tests. Is it possible to make it a pure file-based UI test, and change the test name to case in order to follow the convention?

(It might not work because cross-platform symlinks are tricky, and git is not good at handling it.)

[weihanglo]

cc @epage, any thoughts on how to organize these?

[epage]

This mixing of functional with UI tests seems inline with how we do it elesewhere

[weihanglo]

Note that the first commit is best to pass CI. That means you'll assert the problematic behavior. The "fix" commit then contains both the fix, and the change in test. See also this PR as an example.

In addition, your test is a functional test, but is placed along with other UI tests. Is it possible to make it a pure file-based UI test, and change the test name to case in order to follow the convention?

(It might not work because cross-platform symlinks are tricky, and git is not good at handling it.)

[weihanglo]

This is pulling an exteranl dependency from crates.io. We avoid that in cargo's testsuite and instead host a local registry fixure, like

• cargo/tests/testsuite/rename_deps.rs

  Line 12 in ab1463d

  Package::new("bar", "0.2.0").publish();

• cargo/tests/testsuite/cargo_add/features/mod.rs

  Lines 10 to 16 in ab1463d

  	cargo_test_support::registry::init();
  	cargo_test_support::registry::Package::new("your-face", "99999.0.0+my-package")
  	.feature("nose", &[])
  	.feature("mouth", &[])
  	.feature("eyes", &[])
  	.feature("ears", &[])
  	.publish();

[weihanglo]

cc @epage, any thoughts on how to organize these?

[epage]

Why the double comment?

[epage]

Should this be put in write_atomic?

[epage]

This commit does not change the test. Does the previous commit fail cargo test or is it testing the wrong thing? Each commit should be atomic which includes passing tests. When we ask for a test commit to be split out first, it is to show the current behavior. The commit with the fix will also change the test and that diff shows how behavior changes.