From fdd9a0fe45f675a4f26e435d8f3c8474a1b48af4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dawid=20Ci=C4=99=C5=BCarkiewicz?= <dpc@dpc.pw>
Date: Thu, 25 Aug 2022 12:15:34 -0700
Subject: [PATCH] Do not add home bin path to PATH if it's already there

This is to allow users to control the order via PATH if they so desire.

Tested by preparing two different `cargo-foo` scripts in `$HOME/.cargo/bin` and `$HOME/bin`:

```
> env PATH="/usr/bin/:$HOME/bin:$HOME/.cargo/bin" ./target/debug/cargo foo
Inside ~/bin/
> env PATH="$HOME/.cargo/bin:/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
> env PATH="/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
```

and trailing slash:

```
> env PATH="$HOME/.cargo/bin/:/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
> env PATH="/usr/bin/:$HOME/bin:$HOME/.cargo/bin/" ./target/debug/cargo foo
Inside ~/bin/
```

Fix #11020
---
 src/bin/cargo/main.rs            | 27 +++++++++--
 tests/testsuite/cargo_command.rs | 80 ++++++++++++++++++++++++++++++++
 2 files changed, 102 insertions(+), 5 deletions(-)

diff --git a/src/bin/cargo/main.rs b/src/bin/cargo/main.rs
index 1619b487b2ff..609c8e122b38 100644
--- a/src/bin/cargo/main.rs
+++ b/src/bin/cargo/main.rs
@@ -220,11 +220,28 @@ fn is_executable<P: AsRef<Path>>(path: P) -> bool {
 }
 
 fn search_directories(config: &Config) -> Vec<PathBuf> {
-    let mut dirs = vec![config.home().clone().into_path_unlocked().join("bin")];
-    if let Some(val) = env::var_os("PATH") {
-        dirs.extend(env::split_paths(&val));
-    }
-    dirs
+    let mut path_dirs = if let Some(val) = env::var_os("PATH") {
+        env::split_paths(&val).collect()
+    } else {
+        vec![]
+    };
+
+    let home_bin = config.home().clone().into_path_unlocked().join("bin");
+
+    // If any of that PATH elements contains `home_bin`, do not
+    // add it again. This is so that the users can control priority
+    // of it using PATH, while preserving the historical
+    // behavior of preferring it over system global directories even
+    // when not in PATH at all.
+    // See https://github.com/rust-lang/cargo/issues/11020 for details.
+    //
+    // Note: `p == home_bin` will ignore trailing slash, but we don't
+    // `canonicalize` the paths.
+    if !path_dirs.iter().any(|p| p == &home_bin) {
+        path_dirs.insert(0, home_bin);
+    };
+
+    path_dirs
 }
 
 fn init_git_transports(config: &Config) {
diff --git a/tests/testsuite/cargo_command.rs b/tests/testsuite/cargo_command.rs
index a49f56a41539..694b9a1090b4 100644
--- a/tests/testsuite/cargo_command.rs
+++ b/tests/testsuite/cargo_command.rs
@@ -7,11 +7,14 @@ use std::path::{Path, PathBuf};
 use std::process::Stdio;
 use std::str;
 
+use cargo_test_support::basic_manifest;
+use cargo_test_support::paths::CargoPathExt;
 use cargo_test_support::registry::Package;
 use cargo_test_support::tools::echo_subcommand;
 use cargo_test_support::{
     basic_bin_manifest, cargo_exe, cargo_process, paths, project, project_in_home,
 };
+use cargo_util::paths::join_paths;
 
 fn path() -> Vec<PathBuf> {
     env::split_paths(&env::var_os("PATH").unwrap_or_default()).collect()
@@ -341,6 +344,83 @@ fn cargo_subcommand_env() {
         .run();
 }
 
+#[cargo_test]
+fn cargo_cmd_bins_vs_explicit_path() {
+    // Set up `cargo-foo` binary in two places: inside `$HOME/.cargo/bin` and outside of it
+    //
+    // Return paths to both places
+    fn set_up_cargo_foo() -> (PathBuf, PathBuf) {
+        let p = project()
+            .at("cargo-foo")
+            .file("Cargo.toml", &basic_manifest("cargo-foo", "1.0.0"))
+            .file(
+                "src/bin/cargo-foo.rs",
+                r#"fn main() { println!("INSIDE"); }"#,
+            )
+            .file(
+                "src/bin/cargo-foo2.rs",
+                r#"fn main() { println!("OUTSIDE"); }"#,
+            )
+            .build();
+        p.cargo("build").run();
+        let cargo_bin_dir = paths::home().join(".cargo/bin");
+        cargo_bin_dir.mkdir_p();
+        let root_bin_dir = paths::root().join("bin");
+        root_bin_dir.mkdir_p();
+        let exe_name = format!("cargo-foo{}", env::consts::EXE_SUFFIX);
+        fs::rename(p.bin("cargo-foo"), cargo_bin_dir.join(&exe_name)).unwrap();
+        fs::rename(p.bin("cargo-foo2"), root_bin_dir.join(&exe_name)).unwrap();
+
+        (root_bin_dir, cargo_bin_dir)
+    }
+
+    let (outside_dir, inside_dir) = set_up_cargo_foo();
+
+    // If `$CARGO_HOME/bin` is not in a path, prefer it over anything in `$PATH`.
+    //
+    // This is the historical behavior we don't want to break.
+    cargo_process("foo").with_stdout_contains("INSIDE").run();
+
+    // When `$CARGO_HOME/bin` is in the `$PATH`
+    // use only `$PATH` so the user-defined ordering is respected.
+    {
+        cargo_process("foo")
+            .env(
+                "PATH",
+                join_paths(&[&inside_dir, &outside_dir], "PATH").unwrap(),
+            )
+            .with_stdout_contains("INSIDE")
+            .run();
+
+        cargo_process("foo")
+            // Note: trailing slash
+            .env(
+                "PATH",
+                join_paths(&[inside_dir.join(""), outside_dir.join("")], "PATH").unwrap(),
+            )
+            .with_stdout_contains("INSIDE")
+            .run();
+
+        cargo_process("foo")
+            .env(
+                "PATH",
+                join_paths(&[&outside_dir, &inside_dir], "PATH").unwrap(),
+            )
+            .with_stdout_contains("OUTSIDE")
+            .run();
+
+        cargo_process("foo")
+            // Note: trailing slash
+            .env(
+                "PATH",
+                join_paths(&[outside_dir.join(""), inside_dir.join("")], "PATH").unwrap(),
+            )
+            .with_stdout_contains("OUTSIDE")
+            .run();
+    }
+}
+
+#[test]
 #[cargo_test]
 fn cargo_subcommand_args() {
     let p = echo_subcommand();