Return regular strings from dulwich.porcelain.status() (#2059)

Author: jelmer

NEWS

@@ -1,5 +1,8 @@
 0.25.1	UNRELEASED
 
+ * ``dulwich.porcelain.status`` now returns regular strings.
+   (Jelmer Vernooij, #889)
+
  * Fix AssertionError when accessing ref names with length matching binary
    hash length (e.g., 32 bytes for SHA-256). (Jelmer Vernooij, #2040)
 

dulwich/cli.py

@@ -3311,25 +3311,23 @@ def run(self, args: Sequence[str]) -> None:
             sys.stdout.write("Changes to be committed:\n\n")
             for kind, names in status.staged.items():
                 for name in names:
-                    sys.stdout.write(
-                        f"\t{kind}: {name.decode(sys.getfilesystemencoding())}\n"
-                    )
+                    sys.stdout.write(f"\t{kind}: {os.fsdecode(name)}\n")
             sys.stdout.write("\n")
         if status.unstaged:
             sys.stdout.write("Changes not staged for commit:\n\n")
             for name in status.unstaged:
-                sys.stdout.write(f"\t{name.decode(sys.getfilesystemencoding())}\n")
+                sys.stdout.write(f"\t{os.fsdecode(name)}\n")
             sys.stdout.write("\n")
         if status.untracked:
             sys.stdout.write("Untracked files:\n\n")
             if parsed_args.column:
                 # Format untracked files in columns
-                untracked_names = [name for name in status.untracked]
+                untracked_names = [os.fsdecode(name) for name in status.untracked]
                 output = format_columns(untracked_names, mode="column", indent="\t")
                 sys.stdout.write(output)
             else:
                 for name in status.untracked:
-                    sys.stdout.write(f"\t{name}\n")
+                    sys.stdout.write(f"\t{os.fsdecode(name)}\n")
             sys.stdout.write("\n")
 
 

dulwich/porcelain/__init__.py

@@ -749,6 +749,28 @@ def path_to_tree_path(
         return bytes(relpath)
 
 
+def tree_path_to_fs_path(
+    tree_path: bytes,
+    tree_encoding: str = DEFAULT_ENCODING,
+) -> bytes:
+    """Convert a git tree path to a filesystem path (relative).
+
+    Args:
+      tree_path: Path from git tree (bytes with "/" separators, UTF-8 encoded)
+      tree_encoding: Encoding used for tree paths (default: utf-8)
+    Returns: Filesystem path as bytes (with os.sep, filesystem encoding)
+    """
+    # Decode from tree encoding
+    path_str = tree_path.decode(tree_encoding)
+
+    # Replace / with OS separator if needed
+    if os.sep != "/":
+        path_str = path_str.replace("/", os.sep)
+
+    # Encode for filesystem
+    return os.fsencode(path_str)
+
+
 class DivergedBranches(Error):
     """Branches have diverged and fast-forward is not possible."""
 
@@ -2963,9 +2985,9 @@ def status(
           directories that are entirely untracked without listing all their contents.
 
     Returns: GitStatus tuple,
-        staged -  dict with lists of staged paths (diff index/HEAD)
-        unstaged -  list of unstaged paths (diff index/working-tree)
-        untracked - list of untracked, un-ignored & non-.git paths
+        staged -  dict with lists of staged paths (filesystem paths as bytes)
+        unstaged -  list of unstaged paths (filesystem paths as bytes)
+        untracked - list of untracked paths (filesystem paths as bytes)
     """
     with open_repo_closing(repo) as r:
         # Open the index once and reuse it for both staged and unstaged checks
@@ -2985,7 +3007,7 @@ def status(
         config = r.get_config_stack()
         preload_index = config.get_boolean(b"core", b"preloadIndex", False)
 
-        unstaged_changes = list(
+        unstaged_changes_tree = list(
             get_unstaged_changes(index, r.path, filter_callback, preload_index)
         )
 
@@ -2996,14 +3018,23 @@ def status(
             exclude_ignored=not ignored,
             untracked_files=untracked_files,
         )
-        if sys.platform == "win32":
-            untracked_changes = [
-                path.replace(os.path.sep, "/") for path in untracked_paths
+
+        # Convert all paths to filesystem encoding
+        # Convert staged changes (dict with lists of tree paths)
+        staged_fs = {}
+        for change_type, paths in tracked_changes.items():
+            staged_fs[change_type] = [
+                tree_path_to_fs_path(p) if isinstance(p, bytes) else os.fsencode(p)
+                for p in paths
             ]
-        else:
-            untracked_changes = list(untracked_paths)
 
-        return GitStatus(tracked_changes, unstaged_changes, untracked_changes)
+        # Convert unstaged changes (list of tree paths)
+        unstaged_fs = [tree_path_to_fs_path(p) for p in unstaged_changes_tree]
+
+        # Convert untracked changes (list of strings)
+        untracked_fs = [os.fsencode(p) for p in untracked_paths]
+
+        return GitStatus(staged_fs, unstaged_fs, untracked_fs)
 
 
 def shortlog(

tests/porcelain/__init__.py

@@ -4403,12 +4403,16 @@ def test_checkout_to_branch_with_untracked_files(self) -> None:
             f.write("new message\n")
 
         status = list(porcelain.status(self.repo))
-        self.assertEqual([{"add": [], "delete": [], "modify": []}, [], ["neu"]], status)
+        self.assertEqual(
+            [{"add": [], "delete": [], "modify": []}, [], [os.fsencode("neu")]], status
+        )
 
         porcelain.checkout(self.repo, b"uni")
 
         status = list(porcelain.status(self.repo))
-        self.assertEqual([{"add": [], "delete": [], "modify": []}, [], ["neu"]], status)
+        self.assertEqual(
+            [{"add": [], "delete": [], "modify": []}, [], [os.fsencode("neu")]], status
+        )
 
     def test_checkout_to_branch_with_new_files(self) -> None:
         porcelain.checkout(self.repo, b"uni")
@@ -6099,8 +6103,8 @@ def test_status_base(self) -> None:
 
         results = porcelain.status(self.repo)
 
-        self.assertEqual(results.staged["add"][0], filename_add.encode("ascii"))
-        self.assertEqual(results.unstaged, [b"foo"])
+        self.assertEqual(results.staged["add"][0], os.fsencode(filename_add))
+        self.assertEqual(results.unstaged, [os.fsencode("foo")])
 
     def test_status_with_core_preloadindex(self) -> None:
         """Test status with core.preloadIndex enabled."""
@@ -6139,7 +6143,7 @@ def test_status_with_core_preloadindex(self) -> None:
 
         # Check that we detected the correct unstaged changes
         unstaged_sorted = sorted(results.unstaged)
-        expected_sorted = sorted([f.encode("ascii") for f in modified_files])
+        expected_sorted = sorted([os.fsencode(f) for f in modified_files])
         self.assertEqual(unstaged_sorted, expected_sorted)
 
     def test_status_all(self) -> None:
@@ -6175,10 +6179,14 @@ def test_status_all(self) -> None:
             f.write("origstuff")
         results = porcelain.status(self.repo.path)
         self.assertDictEqual(
-            {"add": [b"baz"], "delete": [b"foo"], "modify": [b"bar"]},
+            {
+                "add": [os.fsencode("baz")],
+                "delete": [os.fsencode("foo")],
+                "modify": [os.fsencode("bar")],
+            },
             results.staged,
         )
-        self.assertListEqual(results.unstaged, [b"blye"])
+        self.assertListEqual(results.unstaged, [os.fsencode("blye")])
         results_no_untracked = porcelain.status(self.repo.path, untracked_files="no")
         self.assertListEqual(results_no_untracked.untracked, [])
 
@@ -6194,7 +6202,9 @@ def test_status_untracked_path(self) -> None:
             fh.write("untracked")
 
         _, _, untracked = porcelain.status(self.repo.path, untracked_files="all")
-        self.assertEqual(untracked, ["untracked_dir/untracked_file"])
+        self.assertEqual(
+            untracked, [os.fsencode(os.path.join("untracked_dir", "untracked_file"))]
+        )
 
     def test_status_untracked_path_normal(self) -> None:
         # Create an untracked directory with multiple files
@@ -6216,16 +6226,16 @@ def test_status_untracked_path_normal(self) -> None:
 
         # Test "normal" mode - should only show the directory, not individual files
         _, _, untracked = porcelain.status(self.repo.path, untracked_files="normal")
-        self.assertEqual(untracked, ["untracked_dir/"])
+        self.assertEqual(untracked, [os.fsencode("untracked_dir" + os.sep)])
 
         # Test "all" mode - should show all files
         _, _, untracked_all = porcelain.status(self.repo.path, untracked_files="all")
         self.assertEqual(
             sorted(untracked_all),
             [
-                "untracked_dir/file1",
-                "untracked_dir/file2",
-                "untracked_dir/nested/file3",
+                os.fsencode(os.path.join("untracked_dir", "file1")),
+                os.fsencode(os.path.join("untracked_dir", "file2")),
+                os.fsencode(os.path.join("untracked_dir", "nested", "file3")),
             ],
         )
 
@@ -6253,11 +6263,15 @@ def test_status_mixed_tracked_untracked(self) -> None:
 
         # In "normal" mode, should show individual untracked files in mixed dirs
         _, _, untracked = porcelain.status(self.repo.path, untracked_files="normal")
-        self.assertEqual(untracked, ["mixed_dir/untracked.txt"])
+        self.assertEqual(
+            untracked, [os.fsencode(os.path.join("mixed_dir", "untracked.txt"))]
+        )
 
         # In "all" mode, should be the same for mixed directories
         _, _, untracked_all = porcelain.status(self.repo.path, untracked_files="all")
-        self.assertEqual(untracked_all, ["mixed_dir/untracked.txt"])
+        self.assertEqual(
+            untracked_all, [os.fsencode(os.path.join("mixed_dir", "untracked.txt"))]
+        )
 
     def test_status_crlf_mismatch(self) -> None:
         # First make a commit as if the file has been added on a Linux system
@@ -6280,7 +6294,7 @@ def test_status_crlf_mismatch(self) -> None:
 
         results = porcelain.status(self.repo)
         self.assertDictEqual({"add": [], "delete": [], "modify": []}, results.staged)
-        self.assertListEqual(results.unstaged, [b"crlf"])
+        self.assertListEqual(results.unstaged, [os.fsencode("crlf")])
         self.assertListEqual(results.untracked, [])
 
     def test_status_autocrlf_true(self) -> None:
@@ -6348,7 +6362,8 @@ def test_status_autocrlf_input(self) -> None:
 
         results = porcelain.status(self.repo)
         self.assertDictEqual(
-            {"add": [b"crlf-new"], "delete": [], "modify": []}, results.staged
+            {"add": [os.fsencode("crlf-new")], "delete": [], "modify": []},
+            results.staged,
         )
         # File committed with CRLF before autocrlf=input was enabled
         # will NOT appear as unstaged because stat matching optimization
@@ -6382,7 +6397,7 @@ def test_status_autocrlf_input_modified(self) -> None:
 
         results = porcelain.status(self.repo)
         # Modified file should be detected as unstaged
-        self.assertListEqual(results.unstaged, [b"crlf-file.txt"])
+        self.assertListEqual(results.unstaged, [os.fsencode("crlf-file.txt")])
 
     def test_status_autocrlf_input_binary(self) -> None:
         """Test that binary files are not affected by autocrlf=input."""
@@ -6554,11 +6569,16 @@ def test_get_untracked_paths(self) -> None:
             ),
         )
         self.assertEqual(
-            {".gitignore", "notignored", "link"},
+            {os.fsencode(".gitignore"), os.fsencode("notignored"), os.fsencode("link")},
             set(porcelain.status(self.repo).untracked),
         )
         self.assertEqual(
-            {".gitignore", "notignored", "ignored", "link"},
+            {
+                os.fsencode(".gitignore"),
+                os.fsencode("notignored"),
+                os.fsencode("ignored"),
+                os.fsencode("link"),
+            },
             set(porcelain.status(self.repo, ignored=True).untracked),
         )
 
@@ -6679,14 +6699,16 @@ def test_get_untracked_paths_nested_gitignore(self) -> None:
 
         # Test status() which uses exclude_ignored=True by default
         status = porcelain.status(self.repo)
-        self.assertEqual(["untracked.txt"], status.untracked)
+        self.assertEqual([os.fsencode("untracked.txt")], status.untracked)
 
         # Test status() with ignored=True which uses exclude_ignored=False
         status_with_ignored = porcelain.status(self.repo, ignored=True)
         # Should include cache directories
-        self.assertIn("untracked.txt", status_with_ignored.untracked)
+        self.assertIn(os.fsencode("untracked.txt"), status_with_ignored.untracked)
         for cache_dir in cache_dirs:
-            self.assertIn(cache_dir + "/", status_with_ignored.untracked)
+            self.assertIn(
+                os.fsencode(cache_dir + os.sep), status_with_ignored.untracked
+            )
 
     def test_get_untracked_paths_mixed_directory(self) -> None:
         """Test directory with both ignored and non-ignored files."""
@@ -6932,7 +6954,7 @@ def test_get_untracked_paths_normal(self) -> None:
         _, _, untracked = porcelain.status(
             repo=self.repo.path, untracked_files="normal"
         )
-        self.assertEqual(untracked, ["untracked_dir/"])
+        self.assertEqual(untracked, [os.fsencode("untracked_dir" + os.sep)])
 
     def test_get_untracked_paths_top_level_issue_1247(self) -> None:
         """Test for issue #1247: ensure top-level untracked files are detected."""
@@ -6955,7 +6977,7 @@ def test_get_untracked_paths_top_level_issue_1247(self) -> None:
         # Test via status
         status = porcelain.status(self.repo)
         self.assertIn(
-            "sample.txt",
+            os.fsencode("sample.txt"),
             status.untracked,
             "Top-level file 'sample.txt' should be in status.untracked",
         )

tests/test_repository.py

@@ -1717,7 +1717,12 @@ def test_unstage_midify_file_with_dir(self) -> None:
         self._repo.get_worktree().unstage(["new_dir/foo"])
         status = list(porcelain.status(self._repo))
         self.assertEqual(
-            [{"add": [], "delete": [], "modify": []}, [b"new_dir/foo"], []], status
+            [
+                {"add": [], "delete": [], "modify": []},
+                [os.fsencode(os.path.join("new_dir", "foo"))],
+                [],
+            ],
+            status,
         )
 
     def test_unstage_while_no_commit(self) -> None:
@@ -1728,7 +1733,9 @@ def test_unstage_while_no_commit(self) -> None:
         porcelain.add(self._repo, paths=[full_path])
         self._repo.get_worktree().unstage([file])
         status = list(porcelain.status(self._repo))
-        self.assertEqual([{"add": [], "delete": [], "modify": []}, [], ["foo"]], status)
+        self.assertEqual(
+            [{"add": [], "delete": [], "modify": []}, [], [os.fsencode("foo")]], status
+        )
 
     def test_unstage_add_file(self) -> None:
         file = "foo"
@@ -1744,7 +1751,9 @@ def test_unstage_add_file(self) -> None:
         porcelain.add(self._repo, paths=[full_path])
         self._repo.get_worktree().unstage([file])
         status = list(porcelain.status(self._repo))
-        self.assertEqual([{"add": [], "delete": [], "modify": []}, [], ["foo"]], status)
+        self.assertEqual(
+            [{"add": [], "delete": [], "modify": []}, [], [os.fsencode("foo")]], status
+        )
 
     def test_unstage_modify_file(self) -> None:
         file = "foo"
@@ -1765,7 +1774,7 @@ def test_unstage_modify_file(self) -> None:
         status = list(porcelain.status(self._repo))
 
         self.assertEqual(
-            [{"add": [], "delete": [], "modify": []}, [b"foo"], []], status
+            [{"add": [], "delete": [], "modify": []}, [os.fsencode("foo")], []], status
         )
 
     def test_unstage_remove_file(self) -> None:
@@ -1784,7 +1793,7 @@ def test_unstage_remove_file(self) -> None:
         self._repo.get_worktree().unstage([file])
         status = list(porcelain.status(self._repo))
         self.assertEqual(
-            [{"add": [], "delete": [], "modify": []}, [b"foo"], []], status
+            [{"add": [], "delete": [], "modify": []}, [os.fsencode("foo")], []], status
         )
 
     def test_reset_index(self) -> None:
@@ -1796,11 +1805,18 @@ def test_reset_index(self) -> None:
         r.get_worktree().stage(["a", "b"])
         status = list(porcelain.status(self._repo))
         self.assertEqual(
-            [{"add": [b"b"], "delete": [], "modify": [b"a"]}, [], []], status
+            [
+                {"add": [os.fsencode("b")], "delete": [], "modify": [os.fsencode("a")]},
+                [],
+                [],
+            ],
+            status,
         )
         r.get_worktree().reset_index()
         status = list(porcelain.status(self._repo))
-        self.assertEqual([{"add": [], "delete": [], "modify": []}, [], ["b"]], status)
+        self.assertEqual(
+            [{"add": [], "delete": [], "modify": []}, [], [os.fsencode("b")]], status
+        )
 
     @skipIf(
         sys.platform in ("win32", "darwin"),