Updates from review

Author: stsewd

Diff for: AUTHORS

@@ -50,4 +50,5 @@ Contributors are:
 -Patrick Gerard
 -Luke Twist <[email protected]>
 -Joseph Hale <me _at_ jhale.dev>
+-Santos Gallegos <stsewd _at_ proton.me>
 Portions derived from other open source works and are clearly marked.

Diff for: test/test_remote.py

@@ -694,91 +694,115 @@ def test_push_error(self, repo):
 
     @with_rw_repo("HEAD")
     def test_set_unsafe_url(self, rw_repo):
+        tmp_dir = Path(tempfile.mkdtemp())
+        tmp_file = tmp_dir / "pwn"
         remote = rw_repo.remote("origin")
         urls = [
-            "ext::sh -c touch% /tmp/pwn",
+            f"ext::sh -c touch% {tmp_file}",
             "fd::17/foo",
         ]
         for url in urls:
             with self.assertRaises(UnsafeProtocolError):
                 remote.set_url(url)
+            assert not tmp_file.exists()
 
     @with_rw_repo("HEAD")
     def test_set_unsafe_url_allowed(self, rw_repo):
+        tmp_dir = Path(tempfile.mkdtemp())
+        tmp_file = tmp_dir / "pwn"
         remote = rw_repo.remote("origin")
         urls = [
-            "ext::sh -c touch% /tmp/pwn",
+            f"ext::sh -c touch% {tmp_file}",
             "fd::17/foo",
         ]
         for url in urls:
             remote.set_url(url, allow_unsafe_protocols=True)
             assert list(remote.urls)[-1] == url
+            assert not tmp_file.exists()
 
     @with_rw_repo("HEAD")
     def test_add_unsafe_url(self, rw_repo):
+        tmp_dir = Path(tempfile.mkdtemp())
+        tmp_file = tmp_dir / "pwn"
         remote = rw_repo.remote("origin")
         urls = [
-            "ext::sh -c touch% /tmp/pwn",
+            f"ext::sh -c touch% {tmp_file}",
             "fd::17/foo",
         ]
         for url in urls:
             with self.assertRaises(UnsafeProtocolError):
                 remote.add_url(url)
+            assert not tmp_file.exists()
 
     @with_rw_repo("HEAD")
     def test_add_unsafe_url_allowed(self, rw_repo):
+        tmp_dir = Path(tempfile.mkdtemp())
+        tmp_file = tmp_dir / "pwn"
         remote = rw_repo.remote("origin")
         urls = [
-            "ext::sh -c touch% /tmp/pwn",
+            f"ext::sh -c touch% {tmp_file}",
             "fd::17/foo",
         ]
         for url in urls:
             remote.add_url(url, allow_unsafe_protocols=True)
             assert list(remote.urls)[-1] == url
+            assert not tmp_file.exists()
 
     @with_rw_repo("HEAD")
     def test_create_remote_unsafe_url(self, rw_repo):
+        tmp_dir = Path(tempfile.mkdtemp())
+        tmp_file = tmp_dir / "pwn"
         urls = [
-            "ext::sh -c touch% /tmp/pwn",
+            f"ext::sh -c touch% {tmp_file}",
             "fd::17/foo",
         ]
         for url in urls:
             with self.assertRaises(UnsafeProtocolError):
                 Remote.create(rw_repo, "origin", url)
+            assert not tmp_file.exists()
 
     @with_rw_repo("HEAD")
     def test_create_remote_unsafe_url_allowed(self, rw_repo):
+        tmp_dir = Path(tempfile.mkdtemp())
+        tmp_file = tmp_dir / "pwn"
         urls = [
-            "ext::sh -c touch% /tmp/pwn",
+            f"ext::sh -c touch% {tmp_file}",
             "fd::17/foo",
         ]
         for i, url in enumerate(urls):
             remote = Remote.create(rw_repo, f"origin{i}", url, allow_unsafe_protocols=True)
             assert remote.url == url
+            assert not tmp_file.exists()
 
     @with_rw_repo("HEAD")
     def test_fetch_unsafe_url(self, rw_repo):
+        tmp_dir = Path(tempfile.mkdtemp())
+        tmp_file = tmp_dir / "pwn"
         remote = rw_repo.remote("origin")
         urls = [
-            "ext::sh -c touch% /tmp/pwn",
+            f"ext::sh -c touch% {tmp_file}",
             "fd::17/foo",
         ]
         for url in urls:
             with self.assertRaises(UnsafeProtocolError):
                 remote.fetch(url)
+            assert not tmp_file.exists()
 
     @with_rw_repo("HEAD")
     def test_fetch_unsafe_url_allowed(self, rw_repo):
+        tmp_dir = Path(tempfile.mkdtemp())
+        tmp_file = tmp_dir / "pwn"
         remote = rw_repo.remote("origin")
         urls = [
-            "ext::sh -c touch% /tmp/pwn",
+            f"ext::sh -c touch% {tmp_file}",
             "fd::17/foo",
         ]
         for url in urls:
             # The URL will be allowed into the command, but the command will
             # fail since we don't have that protocol enabled in the Git config file.
             with self.assertRaises(GitCommandError):
                 remote.fetch(url, allow_unsafe_protocols=True)
+            assert not tmp_file.exists()
 
     @with_rw_repo("HEAD")
     def test_fetch_unsafe_options(self, rw_repo):
@@ -789,6 +813,7 @@ def test_fetch_unsafe_options(self, rw_repo):
         for unsafe_option in unsafe_options:
             with self.assertRaises(UnsafeOptionError):
                 remote.fetch(**unsafe_option)
+            assert not tmp_file.exists()
 
     @with_rw_repo("HEAD")
     def test_fetch_unsafe_options_allowed(self, rw_repo):
@@ -798,32 +823,40 @@ def test_fetch_unsafe_options_allowed(self, rw_repo):
         unsafe_options = [{"upload-pack": f"touch {tmp_file}"}]
         for unsafe_option in unsafe_options:
             # The options will be allowed, but the command will fail.
+            assert not tmp_file.exists()
             with self.assertRaises(GitCommandError):
                 remote.fetch(**unsafe_option, allow_unsafe_options=True)
+            assert tmp_file.exists()
 
     @with_rw_repo("HEAD")
     def test_pull_unsafe_url(self, rw_repo):
+        tmp_dir = Path(tempfile.mkdtemp())
+        tmp_file = tmp_dir / "pwn"
         remote = rw_repo.remote("origin")
         urls = [
-            "ext::sh -c touch% /tmp/pwn",
+            f"ext::sh -c touch% {tmp_file}",
             "fd::17/foo",
         ]
         for url in urls:
             with self.assertRaises(UnsafeProtocolError):
                 remote.pull(url)
+            assert not tmp_file.exists()
 
     @with_rw_repo("HEAD")
     def test_pull_unsafe_url_allowed(self, rw_repo):
+        tmp_dir = Path(tempfile.mkdtemp())
+        tmp_file = tmp_dir / "pwn"
         remote = rw_repo.remote("origin")
         urls = [
-            "ext::sh -c touch% /tmp/pwn",
+            f"ext::sh -c touch% {tmp_file}",
             "fd::17/foo",
         ]
         for url in urls:
             # The URL will be allowed into the command, but the command will
             # fail since we don't have that protocol enabled in the Git config file.
             with self.assertRaises(GitCommandError):
                 remote.pull(url, allow_unsafe_protocols=True)
+            assert not tmp_file.exists()
 
     @with_rw_repo("HEAD")
     def test_pull_unsafe_options(self, rw_repo):
@@ -834,6 +867,7 @@ def test_pull_unsafe_options(self, rw_repo):
         for unsafe_option in unsafe_options:
             with self.assertRaises(UnsafeOptionError):
                 remote.pull(**unsafe_option)
+            assert not tmp_file.exists()
 
     @with_rw_repo("HEAD")
     def test_pull_unsafe_options_allowed(self, rw_repo):
@@ -843,32 +877,40 @@ def test_pull_unsafe_options_allowed(self, rw_repo):
         unsafe_options = [{"upload-pack": f"touch {tmp_file}"}]
         for unsafe_option in unsafe_options:
             # The options will be allowed, but the command will fail.
+            assert not tmp_file.exists()
             with self.assertRaises(GitCommandError):
                 remote.pull(**unsafe_option, allow_unsafe_options=True)
+            assert tmp_file.exists()
 
     @with_rw_repo("HEAD")
     def test_push_unsafe_url(self, rw_repo):
+        tmp_dir = Path(tempfile.mkdtemp())
+        tmp_file = tmp_dir / "pwn"
         remote = rw_repo.remote("origin")
         urls = [
-            "ext::sh -c touch% /tmp/pwn",
+            f"ext::sh -c touch% {tmp_file}",
             "fd::17/foo",
         ]
         for url in urls:
             with self.assertRaises(UnsafeProtocolError):
                 remote.push(url)
+            assert not tmp_file.exists()
 
     @with_rw_repo("HEAD")
     def test_push_unsafe_url_allowed(self, rw_repo):
+        tmp_dir = Path(tempfile.mkdtemp())
+        tmp_file = tmp_dir / "pwn"
         remote = rw_repo.remote("origin")
         urls = [
-            "ext::sh -c touch% /tmp/pwn",
+            f"ext::sh -c touch% {tmp_file}",
             "fd::17/foo",
         ]
         for url in urls:
             # The URL will be allowed into the command, but the command will
             # fail since we don't have that protocol enabled in the Git config file.
             with self.assertRaises(GitCommandError):
                 remote.push(url, allow_unsafe_protocols=True)
+            assert not tmp_file.exists()
 
     @with_rw_repo("HEAD")
     def test_push_unsafe_options(self, rw_repo):
@@ -882,8 +924,10 @@ def test_push_unsafe_options(self, rw_repo):
             }
         ]
         for unsafe_option in unsafe_options:
+            assert not tmp_file.exists()
             with self.assertRaises(UnsafeOptionError):
                 remote.push(**unsafe_option)
+            assert not tmp_file.exists()
 
     @with_rw_repo("HEAD")
     def test_push_unsafe_options_allowed(self, rw_repo):
@@ -898,8 +942,11 @@ def test_push_unsafe_options_allowed(self, rw_repo):
         ]
         for unsafe_option in unsafe_options:
             # The options will be allowed, but the command will fail.
+            assert not tmp_file.exists()
             with self.assertRaises(GitCommandError):
                 remote.push(**unsafe_option, allow_unsafe_options=True)
+            assert tmp_file.exists()
+            tmp_file.unlink()
 
 
 class TestTimeouts(TestBase):

Diff for: test/test_repo.py

@@ -279,6 +279,7 @@ def test_clone_unsafe_options(self, rw_repo):
         for unsafe_option in unsafe_options:
             with self.assertRaises(UnsafeOptionError):
                 rw_repo.clone(tmp_dir, multi_options=[unsafe_option])
+            assert not tmp_file.exists()
 
     @with_rw_repo("HEAD")
     def test_clone_unsafe_options_allowed(self, rw_repo):
@@ -290,9 +291,12 @@ def test_clone_unsafe_options_allowed(self, rw_repo):
         ]
         for i, unsafe_option in enumerate(unsafe_options):
             destination = tmp_dir / str(i)
+            assert not tmp_file.exists()
             # The options will be allowed, but the command will fail.
             with self.assertRaises(GitCommandError):
                 rw_repo.clone(destination, multi_options=[unsafe_option], allow_unsafe_options=True)
+            assert tmp_file.exists()
+            tmp_file.unlink()
 
         unsafe_options = [
             "--config=protocol.ext.allow=always",
@@ -331,6 +335,7 @@ def test_clone_from_unsafe_options(self, rw_repo):
         for unsafe_option in unsafe_options:
             with self.assertRaises(UnsafeOptionError):
                 Repo.clone_from(rw_repo.working_dir, tmp_dir, multi_options=[unsafe_option])
+            assert not tmp_file.exists()
 
     @with_rw_repo("HEAD")
     def test_clone_from_unsafe_options_allowed(self, rw_repo):
@@ -342,11 +347,14 @@ def test_clone_from_unsafe_options_allowed(self, rw_repo):
         ]
         for i, unsafe_option in enumerate(unsafe_options):
             destination = tmp_dir / str(i)
+            assert not tmp_file.exists()
             # The options will be allowed, but the command will fail.
             with self.assertRaises(GitCommandError):
                 Repo.clone_from(
                     rw_repo.working_dir, destination, multi_options=[unsafe_option], allow_unsafe_options=True
                 )
+            assert tmp_file.exists()
+            tmp_file.unlink()
 
         unsafe_options = [
             "--config=protocol.ext.allow=always",
@@ -374,16 +382,19 @@ def test_clone_from_safe_options(self, rw_repo):
 
     def test_clone_from_unsafe_procol(self):
         tmp_dir = pathlib.Path(tempfile.mkdtemp())
+        tmp_file = tmp_dir / "pwn"
         urls = [
-            "ext::sh -c touch% /tmp/pwn",
+            f"ext::sh -c touch% {tmp_file}",
             "fd::17/foo",
         ]
         for url in urls:
             with self.assertRaises(UnsafeProtocolError):
                 Repo.clone_from(url, tmp_dir)
+            assert not tmp_file.exists()
 
     def test_clone_from_unsafe_procol_allowed(self):
         tmp_dir = pathlib.Path(tempfile.mkdtemp())
+        tmp_file = tmp_dir / "pwn"
         urls = [
             "ext::sh -c touch% /tmp/pwn",
             "fd::/foo",
@@ -393,6 +404,7 @@ def test_clone_from_unsafe_procol_allowed(self):
             # fail since we don't have that protocol enabled in the Git config file.
             with self.assertRaises(GitCommandError):
                 Repo.clone_from(url, tmp_dir, allow_unsafe_protocols=True)
+            assert not tmp_file.exists()
 
     @with_rw_repo("HEAD")
     def test_max_chunk_size(self, repo):