Auto merge of #13793 - ijackson:symlink, r=epage

Fix warning suppression for config.toml vs config compat symlinks

### What does this PR try to resolve?

Background: the cargo config file is being renamed from `.cargo/config` to `.cargo/config.toml`.  There's code in new cargo to look for both files (for compatibility), to issue a warning when onliy the old filename is found, and also to issue a warning if both files are found.  The warning suggests making a symlink if compatibility with old cargo is wanted.

An attempt was made to detect when both the old and new files exists, but one is a symlink to the other, but as reported in #13667, this code is not effective.  (It would work only if the symlink had the precise absolute pathname that cargo has decided to use for the lookup, which would be an unnatural way to make the link.)

Logically, the warning should appear when both files exist *but are different*.  That is the anomalous situation that will generate confusing behaviour.   By "different" we ought to mean "aren't the very same file".

That's what this MR implements, where possible.  On Unix, we use the information from stat(2).  That's not available on other platforms; on those, we arrange to also tolerate a symlink referring to precisely `config.toml` as a relative pathname, which is also fine, since by definition the target is then in the same directrory as `config`.

Fixes #13667.

### How should we test and review this PR?

I have interleaved the new tests with the commits that support them.  In each case, a functional commit is followed by a test which fails just beforehand.

(This can be observed by experimentally reordering the branch.)

I have also done ad-hoc testing.

### Additional information

I'm making the assumption that a symlink containing a relative path does something sane on Windows.  This assumption may be unwarranted.  If so, "Handle `config` -> `config.toml` (without full path)" needs to be dropped, and the test case needs to be `#[cfg(unix)]`.

But also, in this case, we should probably put some warnings in the stdlib docs!

Author: bors

Cargo.toml

@@ -183,6 +183,7 @@ rand.workspace = true
 regex.workspace = true
 rusqlite.workspace = true
 rustfix.workspace = true
+same-file.workspace = true
 semver.workspace = true
 serde = { workspace = true, features = ["derive"] }
 serde-untagged.workspace = true

src/cargo/util/context/mod.rs

@@ -1537,36 +1537,32 @@ impl GlobalContext {
         let possible = dir.join(filename_without_extension);
         let possible_with_extension = dir.join(format!("{}.toml", filename_without_extension));
 
-        if possible.exists() {
+        if let Ok(possible_handle) = same_file::Handle::from_path(&possible) {
             if warn {
-                // We don't want to print a warning if the version
-                // without the extension is just a symlink to the version
-                // WITH an extension, which people may want to do to
-                // support multiple Cargo versions at once and not
-                // get a warning.
-                let skip_warning = if let Ok(target_path) = fs::read_link(&possible) {
-                    target_path == possible_with_extension
-                } else {
-                    false
-                };
-
-                if !skip_warning {
-                    if possible_with_extension.exists() {
+                if let Ok(possible_with_extension_handle) =
+                    same_file::Handle::from_path(&possible_with_extension)
+                {
+                    // We don't want to print a warning if the version
+                    // without the extension is just a symlink to the version
+                    // WITH an extension, which people may want to do to
+                    // support multiple Cargo versions at once and not
+                    // get a warning.
+                    if possible_handle != possible_with_extension_handle {
                         self.shell().warn(format!(
                             "both `{}` and `{}` exist. Using `{}`",
                             possible.display(),
                             possible_with_extension.display(),
                             possible.display()
                         ))?;
-                    } else {
-                        self.shell().warn(format!(
-                            "`{}` is deprecated in favor of `{filename_without_extension}.toml`",
-                            possible.display(),
-                        ))?;
-                        self.shell().note(
-                            format!("if you need to support cargo 1.38 or earlier, you can symlink `{filename_without_extension}` to `{filename_without_extension}.toml`"),
-                        )?;
                     }
+                } else {
+                    self.shell().warn(format!(
+                        "`{}` is deprecated in favor of `{filename_without_extension}.toml`",
+                        possible.display(),
+                    ))?;
+                    self.shell().note(
+                        format!("if you need to support cargo 1.38 or earlier, you can symlink `{filename_without_extension}` to `{filename_without_extension}.toml`"),
+                    )?;
                 }
             }
 

tests/testsuite/config.rs

@@ -184,12 +184,29 @@ fn symlink_file(target: &Path, link: &Path) -> io::Result<()> {
     os::windows::fs::symlink_file(target, link)
 }
 
-fn symlink_config_to_config_toml() {
+fn make_config_symlink_to_config_toml_absolute() {
     let toml_path = paths::root().join(".cargo/config.toml");
     let symlink_path = paths::root().join(".cargo/config");
     t!(symlink_file(&toml_path, &symlink_path));
 }
 
+fn make_config_symlink_to_config_toml_relative() {
+    let symlink_path = paths::root().join(".cargo/config");
+    t!(symlink_file(Path::new("config.toml"), &symlink_path));
+}
+
+fn rename_config_toml_to_config_replacing_with_symlink() {
+    let root = paths::root();
+    t!(fs::rename(
+        root.join(".cargo/config.toml"),
+        root.join(".cargo/config")
+    ));
+    t!(symlink_file(
+        Path::new("config"),
+        &root.join(".cargo/config.toml")
+    ));
+}
+
 #[track_caller]
 pub fn assert_error<E: Borrow<anyhow::Error>>(error: E, msgs: &str) {
     let causes = error
@@ -298,7 +315,33 @@ f1 = 1
 ",
     );
 
-    symlink_config_to_config_toml();
+    make_config_symlink_to_config_toml_absolute();
+
+    let gctx = new_gctx();
+
+    assert_eq!(gctx.get::<Option<i32>>("foo.f1").unwrap(), Some(1));
+
+    // It should NOT have warned for the symlink.
+    let output = read_output(gctx);
+    assert_match("", &output);
+}
+
+#[cargo_test]
+fn config_ambiguous_filename_symlink_doesnt_warn_relative() {
+    // Windows requires special permissions to create symlinks.
+    // If we don't have permission, just skip this test.
+    if !symlink_supported() {
+        return;
+    };
+
+    write_config_toml(
+        "\
+[foo]
+f1 = 1
+",
+    );
+
+    make_config_symlink_to_config_toml_relative();
 
     let gctx = new_gctx();
 
@@ -309,6 +352,32 @@ f1 = 1
     assert_match("", &output);
 }
 
+#[cargo_test]
+fn config_ambiguous_filename_symlink_doesnt_warn_backward() {
+    // Windows requires special permissions to create symlinks.
+    // If we don't have permission, just skip this test.
+    if !symlink_supported() {
+        return;
+    };
+
+    write_config_toml(
+        "\
+[foo]
+f1 = 1
+",
+    );
+
+    rename_config_toml_to_config_replacing_with_symlink();
+
+    let gctx = new_gctx();
+
+    assert_eq!(gctx.get::<Option<i32>>("foo.f1").unwrap(), Some(1));
+
+    // It should NOT have warned for this situation.
+    let output = read_output(gctx);
+    assert_match("", &output);
+}
+
 #[cargo_test]
 fn config_ambiguous_filename() {
     write_config_extless(