clone: unify porcelain/Repo rmtree behavior on failure

dulwich/clone.py

@@ -21,6 +21,7 @@
 """Repository clone handling."""
 
 import os
+import shutil
 from typing import TYPE_CHECKING, Callable, Tuple
 
 from dulwich.objects import (
@@ -67,16 +68,20 @@ def do_clone(
     if not clone_refs:
         raise ValueError("clone_refs callback is required")
 
-    if not bare:
-        target = Repo.init(target_path, mkdir=mkdir)
-        if checkout is None:
-            checkout = True
-    else:
-        if checkout:
-            raise ValueError("checkout and bare are incompatible")
-        target = Repo.init_bare(target_path, mkdir=mkdir)
+    if mkdir:
+        os.mkdir(target_path)
 
     try:
+        target = None
+        if not bare:
+            target = Repo.init(target_path)
+            if checkout is None:
+                checkout = True
+        else:
+            if checkout:
+                raise ValueError("checkout and bare are incompatible")
+            target = Repo.init_bare(target_path)
+
         target_config = target.get_config()
         target_config.set((b"remote", origin), b"url", source_path)
         target_config.set(
@@ -108,7 +113,10 @@ def do_clone(
                 errstream.write(b"Checking out " + head + b"\n")
             target.reset_index()
     except BaseException:
-        target.close()
+        if target is not None:
+            target.close()
+        if mkdir:
+            shutil.rmtree(target_path)
         raise
 
     return target

dulwich/porcelain.py

@@ -70,7 +70,6 @@ import datetime
 import os
 from pathlib import Path
 import posixpath
-import shutil
 import stat
 import sys
 import time
@@ -443,8 +442,7 @@ def clone(
     if target is None:
         target = source.split("/")[-1]
 
-    if not os.path.exists(target):
-        os.mkdir(target)
+    mkdir = not os.path.exists(target)
 
     if not isinstance(source, bytes):
         source = source.encode(DEFAULT_ENCODING)
@@ -465,21 +463,17 @@ def clone(
             head_sha = None
         return head_ref, head_sha
 
-    try:
-        return do_clone(
-            source,
-            target,
-            clone_refs=clone_refs,
-            mkdir=False,
-            bare=bare,
-            origin=origin,
-            checkout=checkout,
-            errstream=errstream,
-            branch=branch,
-        )
-    except BaseException:
-        shutil.rmtree(target)
-        raise
+    return do_clone(
+        source,
+        target,
+        clone_refs=clone_refs,
+        mkdir=mkdir,
+        bare=bare,
+        origin=origin,
+        checkout=checkout,
+        errstream=errstream,
+        branch=branch,
+    )
 
 
 def add(repo=".", paths=None):

dulwich/tests/test_porcelain.py

@@ -630,9 +630,12 @@ class CloneTests(PorcelainTestCase):
         r.close()
 
     def test_source_broken(self):
-        target_path = tempfile.mkdtemp()
-        self.assertRaises(Exception, porcelain.clone, "/nonexistant/repo", target_path)
-        self.assertFalse(os.path.exists(target_path))
+        with tempfile.TemporaryDirectory() as parent:
+            target_path = os.path.join(parent, "target")
+            self.assertRaises(
+                Exception, porcelain.clone, "/nonexistant/repo", target_path
+            )
+            self.assertFalse(os.path.exists(target_path))
 
     def test_fetch_symref(self):
         f1_1 = make_object(Blob, data=b"f1")