From 69e99b35024303f18a2d9efedd684cc3e3ae7d1b Mon Sep 17 00:00:00 2001
From: Charlie Marsh <charlie.r.marsh@gmail.com>
Date: Sat, 4 May 2024 08:44:25 -0400
Subject: [PATCH] Use canonical URLs in satisfaction check (#3373)

## Summary

Closes https://github.com/astral-sh/uv/issues/3367.
---
 Cargo.lock                           |  1 +
 crates/uv-installer/Cargo.toml       |  1 +
 crates/uv-installer/src/satisfies.rs | 43 ++++++++++++++++++++++------
 crates/uv/tests/pip_install.rs       | 25 ++++++++++++++++
 4 files changed, 62 insertions(+), 8 deletions(-)

diff --git a/Cargo.lock b/Cargo.lock
index 0452d45bd..d82a42156 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -4862,6 +4862,7 @@ version = "0.0.1"
 dependencies = [
  "anyhow",
  "async-channel",
+ "cache-key",
  "distribution-filename",
  "distribution-types",
  "fs-err",
diff --git a/crates/uv-installer/Cargo.toml b/crates/uv-installer/Cargo.toml
index 43b4e612b..83ff8b371 100644
--- a/crates/uv-installer/Cargo.toml
+++ b/crates/uv-installer/Cargo.toml
@@ -13,6 +13,7 @@ license = { workspace = true }
 workspace = true
 
 [dependencies]
+cache-key = { workspace = true }
 distribution-filename = { workspace = true }
 distribution-types = { workspace = true }
 install-wheel-rs = { workspace = true, default-features = false }
diff --git a/crates/uv-installer/src/satisfies.rs b/crates/uv-installer/src/satisfies.rs
index ccfebf034..e9c5634a8 100644
--- a/crates/uv-installer/src/satisfies.rs
+++ b/crates/uv-installer/src/satisfies.rs
@@ -1,5 +1,7 @@
 use anyhow::Result;
+use cache_key::{CanonicalUrl, RepositoryUrl};
 use std::fmt::Debug;
+use tracing::log::debug;
 use tracing::trace;
 
 use distribution_types::{InstalledDirectUrlDist, InstalledDist, RequirementSource};
@@ -61,8 +63,12 @@ impl RequirementSatisfaction {
                     return Ok(Self::Mismatch);
                 }
 
-                if &requested_url.to_string() != installed_url
-                    || requested_subdirectory != installed_subdirectory
+                if requested_subdirectory != installed_subdirectory {
+                    return Ok(Self::Mismatch);
+                }
+
+                if !CanonicalUrl::parse(installed_url)
+                    .is_ok_and(|installed_url| installed_url == CanonicalUrl::new(requested_url))
                 {
                     return Ok(Self::Mismatch);
                 }
@@ -105,12 +111,30 @@ impl RequirementSatisfaction {
                 else {
                     return Ok(Self::Mismatch);
                 };
-                if &requested_repository.to_string() != installed_url
-                    || requested_subdirectory != installed_subdirectory
-                {
+
+                if requested_subdirectory != installed_subdirectory {
+                    debug!(
+                        "Subdirectory mismatch: {:?} vs. {:?}",
+                        installed_subdirectory, requested_subdirectory
+                    );
                     return Ok(Self::Mismatch);
                 }
+
+                if !RepositoryUrl::parse(installed_url).is_ok_and(|installed_url| {
+                    installed_url == RepositoryUrl::new(requested_repository)
+                }) {
+                    debug!(
+                        "Repository mismatch: {:?} vs. {:?}",
+                        installed_url, requested_repository
+                    );
+                    return Ok(Self::Mismatch);
+                }
+
                 if installed_reference.as_deref() != requested_reference.as_str() {
+                    debug!(
+                        "Reference mismatch: {:?} vs. {:?}",
+                        installed_reference, requested_reference
+                    );
                     return Ok(Self::OutOfDate);
                 }
 
@@ -136,9 +160,12 @@ impl RequirementSatisfaction {
                     return Ok(Self::Mismatch);
                 };
 
-                if &requested_url.to_string() != installed_url
-                    || requested_editable.unwrap_or_default()
-                        != installed_editable.unwrap_or_default()
+                if requested_editable != installed_editable {
+                    return Ok(Self::Mismatch);
+                }
+
+                if !CanonicalUrl::parse(installed_url)
+                    .is_ok_and(|installed_url| installed_url == CanonicalUrl::new(requested_url))
                 {
                     return Ok(Self::Mismatch);
                 }
diff --git a/crates/uv/tests/pip_install.rs b/crates/uv/tests/pip_install.rs
index 00d025a45..069293652 100644
--- a/crates/uv/tests/pip_install.rs
+++ b/crates/uv/tests/pip_install.rs
@@ -4093,6 +4093,31 @@ fn already_installed_remote_url() {
 
     context.assert_installed("uv_public_pypackage", "0.1.0");
 
+    // Request installation again with a different URL, but the same _canonical_ URL. We should
+    // resolve the package (since we installed a specific commit, but are now requesting the default
+    // branch), but not reinstall the package.
+    uv_snapshot!(context.filters(), context.install().arg("uv-public-pypackage @ git+https://github.com/astral-test/uv-public-pypackage.git"), @r###"
+    success: true
+    exit_code: 0
+    ----- stdout -----
+
+    ----- stderr -----
+    Resolved 1 package in [TIME]
+    Audited 1 package in [TIME]
+    "###);
+
+    // Request installation again with a different URL, but the same _canonical_ URL and the same
+    // commit. We should neither resolve nor reinstall the package, since it's already installed
+    // at this precise commit.
+    uv_snapshot!(context.filters(), context.install().arg("uv-public-pypackage @ git+https://github.com/astral-test/uv-public-pypackage.git@b270df1a2fb5d012294e9aaf05e7e0bab1e6a389"), @r###"
+    success: true
+    exit_code: 0
+    ----- stdout -----
+
+    ----- stderr -----
+    Audited 1 package in [TIME]
+    "###);
+
     // Request installation again with just the name
     // We should just audit the URL package since it fulfills this requirement
     uv_snapshot!(