Various minor cleanups

dulwich/hooks.py

@@ -105,7 +105,7 @@ class ShellHook(Hook):
                 raise HookError(f"Hook {self.name} exited with non-zero status {ret}")
             if self.post_exec_callback is not None:
                 return self.post_exec_callback(1, *args)
-        except OSError:  # no file. silent failure.
+        except FileNotFoundError:  # no file. silent failure.
             if self.post_exec_callback is not None:
                 self.post_exec_callback(0, *args)
 

dulwich/ignore.py

@@ -518,8 +518,15 @@ class IgnoreFilterManager:
         p = os.path.join(self._top_path, path, ".gitignore")
         try:
             self._path_filters[path] = IgnoreFilter.from_path(p, self._ignorecase)
-        except OSError:
+        except (FileNotFoundError, NotADirectoryError):
             self._path_filters[path] = None
+        except OSError as e:
+            # On Windows, opening a path that contains a symlink can fail with
+            # errno 22 (Invalid argument) when the symlink points outside the repo
+            if e.errno == 22:
+                self._path_filters[path] = None
+            else:
+                raise
         return self._path_filters[path]
 
     def find_matching(self, path: str) -> Iterable[Pattern]:

dulwich/index.py

@@ -1934,8 +1934,13 @@ def update_working_tree(
             ):
                 try:
                     os.rmdir(root)
-                except OSError:
+                except FileNotFoundError:
+                    # Directory was already removed
                     pass
+                except OSError as e:
+                    if e.errno != errno.ENOTEMPTY:
+                        # Only ignore "directory not empty" errors
+                        raise
 
     index.write()
 

dulwich/pack.py

@@ -389,8 +389,8 @@ def _load_file_contents(f, size=None):
         if has_mmap:
             try:
                 contents = mmap.mmap(fd, size, access=mmap.ACCESS_READ)
-            except OSError:
-                # Perhaps a socket?
+            except (OSError, ValueError):
+                # Can't mmap - perhaps a socket or invalid file descriptor
                 pass
             else:
                 return contents, size

tests/test_attrs.py

@@ -320,21 +320,20 @@ class FileOperationsTests(TestCase):
             f.write(b"*.jpg -text binary\n")
             temp_path = f.name
 
-        try:
-            patterns = parse_gitattributes_file(temp_path)
-            self.assertEqual(len(patterns), 2)
+        self.addCleanup(os.unlink, temp_path)
 
-            # Check first pattern
-            pattern, attrs = patterns[0]
-            self.assertEqual(pattern.pattern, b"*.txt")
-            self.assertEqual(attrs, {b"text": True})
+        patterns = parse_gitattributes_file(temp_path)
+        self.assertEqual(len(patterns), 2)
 
-            # Check second pattern
-            pattern, attrs = patterns[1]
-            self.assertEqual(pattern.pattern, b"*.jpg")
-            self.assertEqual(attrs, {b"text": False, b"binary": True})
-        finally:
-            os.unlink(temp_path)
+        # Check first pattern
+        pattern, attrs = patterns[0]
+        self.assertEqual(pattern.pattern, b"*.txt")
+        self.assertEqual(attrs, {b"text": True})
+
+        # Check second pattern
+        pattern, attrs = patterns[1]
+        self.assertEqual(pattern.pattern, b"*.jpg")
+        self.assertEqual(attrs, {b"text": False, b"binary": True})
 
     def test_read_gitattributes(self):
         """Test reading gitattributes from a directory."""
@@ -427,17 +426,16 @@ class GitAttributesTests(TestCase):
             f.write(b"*.bin -text binary\n")
             temp_path = f.name
 
-        try:
-            ga = GitAttributes.from_file(temp_path)
-            self.assertEqual(len(ga), 2)
+        self.addCleanup(os.unlink, temp_path)
 
-            attrs = ga.match_path(b"file.txt")
-            self.assertEqual(attrs, {b"text": True})
+        ga = GitAttributes.from_file(temp_path)
+        self.assertEqual(len(ga), 2)
 
-            attrs = ga.match_path(b"file.bin")
-            self.assertEqual(attrs, {b"text": False, b"binary": True})
-        finally:
-            os.unlink(temp_path)
+        attrs = ga.match_path(b"file.txt")
+        self.assertEqual(attrs, {b"text": True})
+
+        attrs = ga.match_path(b"file.bin")
+        self.assertEqual(attrs, {b"text": False, b"binary": True})
 
     def test_from_path(self):
         """Test creating GitAttributes from directory path."""

tests/test_config.py

@@ -341,14 +341,12 @@ who\"
             f.write("[core]\n    filemode = true\n")
             temp_path = f.name
 
-        try:
-            # Test with pathlib.Path
-            path_obj = Path(temp_path)
-            cf = ConfigFile.from_path(path_obj)
-            self.assertEqual(cf.get((b"core",), b"filemode"), b"true")
-        finally:
-            # Clean up
-            os.unlink(temp_path)
+        self.addCleanup(os.unlink, temp_path)
+
+        # Test with pathlib.Path
+        path_obj = Path(temp_path)
+        cf = ConfigFile.from_path(path_obj)
+        self.assertEqual(cf.get((b"core",), b"filemode"), b"true")
 
     def test_write_to_path_pathlib(self) -> None:
         import tempfile

tests/test_ignore.py

@@ -204,18 +204,16 @@ class IgnoreFilterTests(TestCase):
             f.write("*.pyc\n__pycache__/\n")
             temp_path = f.name
 
-        try:
-            # Test with pathlib.Path
-            path_obj = Path(temp_path)
-            ignore_filter = IgnoreFilter.from_path(path_obj)
-
-            # Test that it loaded the patterns correctly
-            self.assertTrue(ignore_filter.is_ignored("test.pyc"))
-            self.assertTrue(ignore_filter.is_ignored("__pycache__/"))
-            self.assertFalse(ignore_filter.is_ignored("test.py"))
-        finally:
-            # Clean up
-            os.unlink(temp_path)
+        self.addCleanup(os.unlink, temp_path)
+
+        # Test with pathlib.Path
+        path_obj = Path(temp_path)
+        ignore_filter = IgnoreFilter.from_path(path_obj)
+
+        # Test that it loaded the patterns correctly
+        self.assertTrue(ignore_filter.is_ignored("test.pyc"))
+        self.assertTrue(ignore_filter.is_ignored("__pycache__/"))
+        self.assertFalse(ignore_filter.is_ignored("test.py"))
 
 
 class IgnoreFilterStackTests(TestCase):

tests/test_index.py

@@ -140,32 +140,30 @@ class SimpleIndexTestCase(IndexTestCase):
         with tempfile.NamedTemporaryFile(suffix=".index", delete=False) as f:
             temp_path = f.name
 
-        try:
-            # Test creating Index with pathlib.Path
-            path_obj = Path(temp_path)
-            index = Index(path_obj, read=False)
-            self.assertEqual(str(path_obj), index.path)
-
-            # Add an entry and write
-            index[b"test"] = IndexEntry(
-                ctime=(0, 0),
-                mtime=(0, 0),
-                dev=0,
-                ino=0,
-                mode=33188,
-                uid=0,
-                gid=0,
-                size=0,
-                sha=b"e69de29bb2d1d6434b8b29ae775ad8c2e48c5391",
-            )
-            index.write()
+        self.addCleanup(os.unlink, temp_path)
+
+        # Test creating Index with pathlib.Path
+        path_obj = Path(temp_path)
+        index = Index(path_obj, read=False)
+        self.assertEqual(str(path_obj), index.path)
+
+        # Add an entry and write
+        index[b"test"] = IndexEntry(
+            ctime=(0, 0),
+            mtime=(0, 0),
+            dev=0,
+            ino=0,
+            mode=33188,
+            uid=0,
+            gid=0,
+            size=0,
+            sha=b"e69de29bb2d1d6434b8b29ae775ad8c2e48c5391",
+        )
+        index.write()
 
-            # Read it back with pathlib.Path
-            index2 = Index(path_obj)
-            self.assertIn(b"test", index2)
-        finally:
-            # Clean up
-            os.unlink(temp_path)
+        # Read it back with pathlib.Path
+        index2 = Index(path_obj)
+        self.assertIn(b"test", index2)
 
 
 class SimpleIndexWriterTestCase(IndexTestCase):
@@ -875,23 +873,20 @@ class TestTreeFSPathConversion(TestCase):
     def test_tree_to_fs_path_windows_separator(self) -> None:
         tree_path = b"path/with/slash"
         original_sep = os.sep.encode("ascii")
-        try:
-            # Temporarily modify os_sep_bytes to test Windows path conversion
-            # This simulates Windows behavior on all platforms for testing
-            import dulwich.index
+        # Temporarily modify os_sep_bytes to test Windows path conversion
+        # This simulates Windows behavior on all platforms for testing
+        import dulwich.index
 
-            dulwich.index.os_sep_bytes = b"\\"
+        dulwich.index.os_sep_bytes = b"\\"
+        self.addCleanup(setattr, dulwich.index, "os_sep_bytes", original_sep)
 
-            fs_path = _tree_to_fs_path(b"/prefix/path", tree_path)
+        fs_path = _tree_to_fs_path(b"/prefix/path", tree_path)
 
-            # The function should join the prefix path with the converted tree path
-            # The expected behavior is that the path separators in the tree_path are
-            # converted to the platform-specific separator (which we've set to backslash)
-            expected_path = os.path.join(b"/prefix/path", b"path\\with\\slash")
-            self.assertEqual(fs_path, expected_path)
-        finally:
-            # Restore original value
-            dulwich.index.os_sep_bytes = original_sep
+        # The function should join the prefix path with the converted tree path
+        # The expected behavior is that the path separators in the tree_path are
+        # converted to the platform-specific separator (which we've set to backslash)
+        expected_path = os.path.join(b"/prefix/path", b"path\\with\\slash")
+        self.assertEqual(fs_path, expected_path)
 
     def test_fs_to_tree_path_str(self) -> None:
         fs_path = os.path.join(os.path.join("délwíçh", "foo"))
@@ -907,17 +902,14 @@ class TestTreeFSPathConversion(TestCase):
         # Test conversion of Windows paths to tree paths
         fs_path = b"path\\with\\backslash"
         original_sep = os.sep.encode("ascii")
-        try:
-            # Temporarily modify os_sep_bytes to test Windows path conversion
-            import dulwich.index
+        # Temporarily modify os_sep_bytes to test Windows path conversion
+        import dulwich.index
 
-            dulwich.index.os_sep_bytes = b"\\"
+        dulwich.index.os_sep_bytes = b"\\"
+        self.addCleanup(setattr, dulwich.index, "os_sep_bytes", original_sep)
 
-            tree_path = _fs_to_tree_path(fs_path)
-            self.assertEqual(tree_path, b"path/with/backslash")
-        finally:
-            # Restore original value
-            dulwich.index.os_sep_bytes = original_sep
+        tree_path = _fs_to_tree_path(fs_path)
+        self.assertEqual(tree_path, b"path/with/backslash")
 
 
 class TestIndexEntryFromPath(TestCase):

tests/test_object_store.py

@@ -308,31 +308,30 @@ class DiskObjectStoreTests(PackBasedObjectStoreTests, TestCase):
 
     def test_add_thin_pack(self) -> None:
         o = DiskObjectStore(self.store_dir)
-        try:
-            blob = make_object(Blob, data=b"yummy data")
-            o.add_object(blob)
+        self.addCleanup(o.close)
 
-            f = BytesIO()
-            entries = build_pack(
-                f,
-                [
-                    (REF_DELTA, (blob.id, b"more yummy data")),
-                ],
-                store=o,
-            )
+        blob = make_object(Blob, data=b"yummy data")
+        o.add_object(blob)
 
-            with o.add_thin_pack(f.read, None) as pack:
-                packed_blob_sha = sha_to_hex(entries[0][3])
-                pack.check_length_and_checksum()
-                self.assertEqual(sorted([blob.id, packed_blob_sha]), list(pack))
-                self.assertTrue(o.contains_packed(packed_blob_sha))
-                self.assertTrue(o.contains_packed(blob.id))
-                self.assertEqual(
-                    (Blob.type_num, b"more yummy data"),
-                    o.get_raw(packed_blob_sha),
-                )
-        finally:
-            o.close()
+        f = BytesIO()
+        entries = build_pack(
+            f,
+            [
+                (REF_DELTA, (blob.id, b"more yummy data")),
+            ],
+            store=o,
+        )
+
+        with o.add_thin_pack(f.read, None) as pack:
+            packed_blob_sha = sha_to_hex(entries[0][3])
+            pack.check_length_and_checksum()
+            self.assertEqual(sorted([blob.id, packed_blob_sha]), list(pack))
+            self.assertTrue(o.contains_packed(packed_blob_sha))
+            self.assertTrue(o.contains_packed(blob.id))
+            self.assertEqual(
+                (Blob.type_num, b"more yummy data"),
+                o.get_raw(packed_blob_sha),
+            )
 
     def test_add_thin_pack_empty(self) -> None:
         with closing(DiskObjectStore(self.store_dir)) as o:

tests/test_porcelain.py

@@ -1826,16 +1826,18 @@ class RemoveTests(PorcelainTestCase):
         # Change to a different directory
         cwd = os.getcwd()
         tempdir = tempfile.mkdtemp()
-        try:
-            os.chdir(tempdir)
-            # Remove the file using relative path from repository root
-            porcelain.remove(self.repo.path, paths=["mydir/myfile"])
-
-            # Verify file was removed
-            self.assertFalse(os.path.exists(fullpath))
-        finally:
+
+        def cleanup():
             os.chdir(cwd)
-            shutil.rmtree(tempdir, ignore_errors=True)
+            shutil.rmtree(tempdir)
+
+        self.addCleanup(cleanup)
+        os.chdir(tempdir)
+        # Remove the file using relative path from repository root
+        porcelain.remove(self.repo.path, paths=["mydir/myfile"])
+
+        # Verify file was removed
+        self.assertFalse(os.path.exists(fullpath))
 
     def test_remove_with_absolute_path(self) -> None:
         # Create a file
@@ -1855,16 +1857,18 @@ class RemoveTests(PorcelainTestCase):
         # Change to a different directory
         cwd = os.getcwd()
         tempdir = tempfile.mkdtemp()
-        try:
-            os.chdir(tempdir)
-            # Remove the file using absolute path
-            porcelain.remove(self.repo.path, paths=[fullpath])
-
-            # Verify file was removed
-            self.assertFalse(os.path.exists(fullpath))
-        finally:
+
+        def cleanup():
             os.chdir(cwd)
-            shutil.rmtree(tempdir, ignore_errors=True)
+            shutil.rmtree(tempdir)
+
+        self.addCleanup(cleanup)
+        os.chdir(tempdir)
+        # Remove the file using absolute path
+        porcelain.remove(self.repo.path, paths=[fullpath])
+
+        # Verify file was removed
+        self.assertFalse(os.path.exists(fullpath))
 
     def test_remove_with_filter_normalization(self) -> None:
         # Enable autocrlf to normalize line endings
@@ -1896,11 +1900,9 @@ class RemoveTests(PorcelainTestCase):
         # Remove the file - this should not fail even though working tree has CRLF
         # and index has LF (thanks to the normalization in the commit)
         cwd = os.getcwd()
-        try:
-            os.chdir(self.repo.path)
-            porcelain.remove(self.repo.path, paths=["foo.txt"])
-        finally:
-            os.chdir(cwd)
+        os.chdir(self.repo.path)
+        self.addCleanup(os.chdir, cwd)
+        porcelain.remove(self.repo.path, paths=["foo.txt"])
 
         # Verify file was removed
         self.assertFalse(os.path.exists(fullpath))
@@ -2054,17 +2056,19 @@ class MvTests(PorcelainTestCase):
         # Change to a different directory and move the file
         cwd = os.getcwd()
         tempdir = tempfile.mkdtemp()
-        try:
-            os.chdir(tempdir)
-            # Move the file using relative path from repository root
-            porcelain.mv(self.repo.path, "mydir/myfile", "renamed")
-
-            # Verify file was moved
-            self.assertFalse(os.path.exists(fullpath))
-            self.assertTrue(os.path.exists(os.path.join(self.repo.path, "renamed")))
-        finally:
+
+        def cleanup():
             os.chdir(cwd)
-            shutil.rmtree(tempdir, ignore_errors=True)
+            shutil.rmtree(tempdir)
+
+        self.addCleanup(cleanup)
+        os.chdir(tempdir)
+        # Move the file using relative path from repository root
+        porcelain.mv(self.repo.path, "mydir/myfile", "renamed")
+
+        # Verify file was moved
+        self.assertFalse(os.path.exists(fullpath))
+        self.assertTrue(os.path.exists(os.path.join(self.repo.path, "renamed")))
 
 
 class LogTests(PorcelainTestCase):
@@ -3629,10 +3633,8 @@ class CheckoutTests(PorcelainTestCase):
         target_repo = porcelain.clone(
             self.repo.path, target=clone_path, errstream=errstream
         )
-        try:
-            self.assertEqual(target_repo[b"HEAD"], self.repo[b"HEAD"])
-        finally:
-            target_repo.close()
+        self.addCleanup(target_repo.close)
+        self.assertEqual(target_repo[b"HEAD"], self.repo[b"HEAD"])
 
         # create a second file to be pushed back to origin
         handle, fullpath = tempfile.mkstemp(dir=clone_path)
@@ -4112,10 +4114,8 @@ class PushTests(PorcelainTestCase):
         target_repo = porcelain.clone(
             self.repo.path, target=clone_path, errstream=errstream
         )
-        try:
-            self.assertEqual(target_repo[b"HEAD"], self.repo[b"HEAD"])
-        finally:
-            target_repo.close()
+        self.addCleanup(target_repo.close)
+        self.assertEqual(target_repo[b"HEAD"], self.repo[b"HEAD"])
 
         # create a second file to be pushed back to origin
         handle, fullpath = tempfile.mkstemp(dir=clone_path)
@@ -5007,11 +5007,9 @@ class StatusTests(PorcelainTestCase):
             committer=b"committer <email>",
         )
         cwd = os.getcwd()
-        try:
-            os.chdir(self.repo.path)
-            porcelain.remove(repo=self.repo.path, paths=[filename])
-        finally:
-            os.chdir(cwd)
+        self.addCleanup(os.chdir, cwd)
+        os.chdir(self.repo.path)
+        porcelain.remove(repo=self.repo.path, paths=[filename])
         changes = porcelain.get_tree_changes(self.repo.path)
 
         self.assertEqual(changes["delete"][0], filename.encode("ascii"))
@@ -5820,16 +5818,14 @@ class CheckIgnoreTests(PorcelainTestCase):
         with open(os.path.join(self.repo.path, ".gitignore"), "w") as f:
             f.write("foo\n")
         cwd = os.getcwd()
+        self.addCleanup(os.chdir, cwd)
         os.mkdir(os.path.join(self.repo.path, "bar"))
         os.chdir(os.path.join(self.repo.path, "bar"))
-        try:
-            self.assertEqual(list(porcelain.check_ignore(self.repo, ["../foo"])), [])
-            self.assertEqual(
-                ["../foo"],
-                list(porcelain.check_ignore(self.repo, ["../foo"], no_index=True)),
-            )
-        finally:
-            os.chdir(cwd)
+        self.assertEqual(list(porcelain.check_ignore(self.repo, ["../foo"])), [])
+        self.assertEqual(
+            ["../foo"],
+            list(porcelain.check_ignore(self.repo, ["../foo"], no_index=True)),
+        )
 
 
 class UpdateHeadTests(PorcelainTestCase):
@@ -6046,30 +6042,28 @@ class PathToTreeTests(PorcelainTestCase):
 
     def test_path_to_tree_path_rel(self) -> None:
         cwd = os.getcwd()
+        self.addCleanup(os.chdir, cwd)
         os.mkdir(os.path.join(self.repo.path, "foo"))
         os.mkdir(os.path.join(self.repo.path, "foo/bar"))
-        try:
-            os.chdir(os.path.join(self.repo.path, "foo/bar"))
-            with open("baz", "w") as f:
-                f.write("contents")
-            self.assertEqual(b"bar/baz", porcelain.path_to_tree_path("..", "baz"))
-            self.assertEqual(
-                b"bar/baz",
-                porcelain.path_to_tree_path(
-                    os.path.join(os.getcwd(), ".."),
-                    os.path.join(os.getcwd(), "baz"),
-                ),
-            )
-            self.assertEqual(
-                b"bar/baz",
-                porcelain.path_to_tree_path("..", os.path.join(os.getcwd(), "baz")),
-            )
-            self.assertEqual(
-                b"bar/baz",
-                porcelain.path_to_tree_path(os.path.join(os.getcwd(), ".."), "baz"),
-            )
-        finally:
-            os.chdir(cwd)
+        os.chdir(os.path.join(self.repo.path, "foo/bar"))
+        with open("baz", "w") as f:
+            f.write("contents")
+        self.assertEqual(b"bar/baz", porcelain.path_to_tree_path("..", "baz"))
+        self.assertEqual(
+            b"bar/baz",
+            porcelain.path_to_tree_path(
+                os.path.join(os.getcwd(), ".."),
+                os.path.join(os.getcwd(), "baz"),
+            ),
+        )
+        self.assertEqual(
+            b"bar/baz",
+            porcelain.path_to_tree_path("..", os.path.join(os.getcwd(), "baz")),
+        )
+        self.assertEqual(
+            b"bar/baz",
+            porcelain.path_to_tree_path(os.path.join(os.getcwd(), ".."), "baz"),
+        )
 
 
 class GetObjectByPathTests(PorcelainTestCase):