From 3040bf99e51ef330dba24bffd8c5c4f5aea9e263 Mon Sep 17 00:00:00 2001
From: Brian Rutledge <bhrutledge@gmail.com>
Date: Fri, 17 Sep 2021 05:45:15 -0400
Subject: [PATCH] Improve redirect error message for trailing slash (#812)

* Reformat existing message

* Make existing test more specific

* Add failing test

* Add error message for trailing slash

* Reword existing message

* Add changelog entry
---
 changelog/812.bugfix.rst |  2 ++
 tests/test_register.py   | 15 ++++++---------
 tests/test_upload.py     | 38 +++++++++++++++++++++++++++++++-------
 twine/exceptions.py      | 21 +++++++++++----------
 4 files changed, 50 insertions(+), 26 deletions(-)
 create mode 100644 changelog/812.bugfix.rst

diff --git a/changelog/812.bugfix.rst b/changelog/812.bugfix.rst
new file mode 100644
index 00000000..1659d362
--- /dev/null
+++ b/changelog/812.bugfix.rst
@@ -0,0 +1,2 @@
+Add a helpful error message when an upload fails due to missing a trailing
+slash in the URL.
diff --git a/tests/test_register.py b/tests/test_register.py
index 7ceb5b15..90b38101 100644
--- a/tests/test_register.py
+++ b/tests/test_register.py
@@ -16,7 +16,7 @@ def register_settings(make_settings):
     return make_settings(
         """
         [pypi]
-        repository: https://test.pypi.org/legacy
+        repository: https://test.pypi.org/legacy/
         username:foo
         password:bar
         """
@@ -46,7 +46,7 @@ def test_successful_register(register_settings):
 def test_exception_for_redirect(register_settings):
     """Raise an exception when repository URL results in a redirect."""
     repository_url = register_settings.repository_config["repository"]
-    redirect_url = "https://malicious.website.org/danger"
+    redirect_url = "https://malicious.website.org/danger/"
 
     stub_response = pretend.stub(
         is_redirect=True,
@@ -60,13 +60,10 @@ def test_exception_for_redirect(register_settings):
 
     register_settings.create_repository = lambda: stub_repository
 
-    err_msg = (
-        f"{repository_url} attempted to redirect to {redirect_url}.\n"
-        f"If you trust these URLs, set {redirect_url} as your repository URL.\n"
-        f"Aborting."
-    )
-
-    with pytest.raises(exceptions.RedirectDetected, match=err_msg):
+    with pytest.raises(
+        exceptions.RedirectDetected,
+        match=rf"{repository_url}.+{redirect_url}.+\nIf you trust these URLs",
+    ):
         register.register(register_settings, helpers.WHEEL_FIXTURE)
 
 
diff --git a/tests/test_upload.py b/tests/test_upload.py
index 465a08cf..e6ff45e1 100644
--- a/tests/test_upload.py
+++ b/tests/test_upload.py
@@ -248,13 +248,39 @@ def test_deprecated_repo(make_settings):
     )
 
 
-def test_exception_for_redirect(make_settings):
+@pytest.mark.parametrize(
+    "repository_url, redirect_url, message_match",
+    [
+        (
+            "https://test.pypi.org/legacy",
+            "https://test.pypi.org/legacy/",
+            (
+                r"https://test.pypi.org/legacy.+https://test.pypi.org/legacy/"
+                r".+\nYour repository URL is missing a trailing slash"
+            ),
+        ),
+        (
+            "https://test.pypi.org/legacy/",
+            "https://malicious.website.org/danger/",
+            (
+                r"https://test.pypi.org/legacy/.+https://malicious.website.org/danger/"
+                r".+\nIf you trust these URLs"
+            ),
+        ),
+    ],
+)
+def test_exception_for_redirect(
+    repository_url,
+    redirect_url,
+    message_match,
+    make_settings,
+):
     # Not using fixtures because this setup is significantly different
 
     upload_settings = make_settings(
-        """
+        f"""
         [pypi]
-        repository: https://test.pypi.org/legacy
+        repository: {repository_url}
         username:foo
         password:bar
         """
@@ -263,7 +289,7 @@ def test_exception_for_redirect(make_settings):
     stub_response = pretend.stub(
         is_redirect=True,
         status_code=301,
-        headers={"location": "https://test.pypi.org/legacy/"},
+        headers={"location": redirect_url},
     )
 
     stub_repository = pretend.stub(
@@ -272,11 +298,9 @@ def test_exception_for_redirect(make_settings):
 
     upload_settings.create_repository = lambda: stub_repository
 
-    with pytest.raises(exceptions.RedirectDetected) as err:
+    with pytest.raises(exceptions.RedirectDetected, match=message_match):
         upload.upload(upload_settings, [helpers.WHEEL_FIXTURE])
 
-    assert "https://test.pypi.org/legacy/" in err.value.args[0]
-
 
 def test_prints_skip_message_for_uploaded_package(
     upload_settings, stub_repository, capsys
diff --git a/twine/exceptions.py b/twine/exceptions.py
index d90c70e3..02cb3708 100644
--- a/twine/exceptions.py
+++ b/twine/exceptions.py
@@ -31,17 +31,18 @@ class RedirectDetected(TwineException):
 
     @classmethod
     def from_args(cls, repository_url: str, redirect_url: str) -> "RedirectDetected":
-        msg = "\n".join(
-            [
-                "{} attempted to redirect to {}.".format(repository_url, redirect_url),
-                "If you trust these URLs, set {} as your repository URL.".format(
-                    redirect_url
-                ),
-                "Aborting.",
-            ]
-        )
+        if redirect_url == f"{repository_url}/":
+            return cls(
+                f"{repository_url} attempted to redirect to {redirect_url}.\n"
+                f"Your repository URL is missing a trailing slash. "
+                "Please add it and try again.",
+            )
 
-        return cls(msg)
+        return cls(
+            f"{repository_url} attempted to redirect to {redirect_url}.\n"
+            f"If you trust these URLs, set {redirect_url} as your repository URL "
+            "and try again.",
+        )
 
 
 class PackageNotFound(TwineException):