Browse Source

Improve porcelain.add() documentation to clarify default behavior

The docstring incorrectly stated that when no paths are provided, the
function "stages all modified files". This has been corrected to
accurately describe that it "stages all untracked files from the
current working directory", which matches Git's behavior.

Added clarification that the function respects the current working
directory, so when called from a subdirectory, it only adds files
from that subdirectory and below, just like 'git add .' does.

Fixes #895
Jelmer Vernooij 2 months ago
parent
commit
3528f83685
3 changed files with 48 additions and 24 deletions
  1. 7 5
      NEWS
  2. 9 4
      dulwich/porcelain.py
  3. 32 15
      tests/test_porcelain.py

+ 7 - 5
NEWS

@@ -6,11 +6,13 @@
    Now only resolves the parent directory for symlinks, matching Git's behavior.
    (Jelmer Vernooij, #789)
 
- * Fix ``porcelain.add()`` when adding repository root path. Previously, adding
-   the repository path itself would incorrectly stage ``b'./'`` instead of the
-   actual untracked files, leading to repository corruption. Now correctly adds
-   all untracked files in the repository root when the root path is specified.
-   (Jelmer Vernooij, #1178)
+ * Fix ``porcelain.add()`` when adding repository root path or directories.
+   Previously, adding the repository path itself would incorrectly stage
+   ``b'./'`` instead of the actual untracked files, leading to
+   repository corruption.  (Jelmer Vernooij, #1178)
+
+ * Improve ``porcelain.add()`` documentation to correctly describe default
+   behavior. (Jelmer Vernooij, #895)
 
  * Fix gitignore pattern matching for directory negation patterns. Patterns like
    ``!data/*/`` now correctly unignore direct subdirectories while still ignoring

+ 9 - 4
dulwich/porcelain.py

@@ -587,12 +587,17 @@ def add(repo=".", paths=None):
 
     Args:
       repo: Repository for the files
-      paths: Paths to add.  No value passed stages all modified files.
+      paths: Paths to add. If None, stages all untracked files from the
+        current working directory (mimicking 'git add .' behavior).
     Returns: Tuple with set of added files and ignored files
 
     If the repository contains ignored directories, the returned set will
     contain the path to an ignored directory (with trailing slash). Individual
     files within ignored directories will not be returned.
+
+    Note: When paths=None, this function respects the current working directory,
+    so if called from a subdirectory, it will only add files from that
+    subdirectory and below, matching Git's behavior.
     """
     ignored = set()
     with open_repo_closing(repo) as r:
@@ -631,7 +636,7 @@ def add(repo=".", paths=None):
                 resolved_path = path.resolve()
 
             try:
-                relpath = str(resolved_path.relative_to(repo_path))
+                relpath = str(resolved_path.relative_to(repo_path)).replace(os.sep, "/")
             except ValueError:
                 # Path is not within the repository
                 raise ValueError(f"Path {p} is not within repository {repo_path}")
@@ -639,7 +644,7 @@ def add(repo=".", paths=None):
             # Handle directories by scanning their contents
             if resolved_path.is_dir():
                 # Check if the directory itself is ignored
-                dir_relpath = os.path.join(relpath, "") if relpath != "." else ""
+                dir_relpath = posixpath.join(relpath, "") if relpath != "." else ""
                 if dir_relpath and ignore_manager.is_ignored(dir_relpath):
                     ignored.add(dir_relpath)
                     continue
@@ -655,7 +660,7 @@ def add(repo=".", paths=None):
                 for untracked_path in current_untracked:
                     # If we're scanning a subdirectory, adjust the path
                     if relpath != ".":
-                        untracked_path = os.path.join(relpath, untracked_path)
+                        untracked_path = posixpath.join(relpath, untracked_path)
 
                     if not ignore_manager.is_ignored(untracked_path):
                         relpaths.append(untracked_path)

+ 32 - 15
tests/test_porcelain.py

@@ -920,9 +920,12 @@ class AddTests(PorcelainTestCase):
         try:
             os.chdir(self.repo.path)
             self.assertEqual({"foo", "blah", "adir", ".git"}, set(os.listdir(".")))
+            added, ignored = porcelain.add(self.repo.path)
+            # Normalize paths to use forward slashes for comparison
+            added_normalized = [path.replace(os.sep, "/") for path in added]
             self.assertEqual(
-                (["foo", os.path.join("adir", "afile")], set()),
-                porcelain.add(self.repo.path),
+                (added_normalized, ignored),
+                (["foo", "adir/afile"], set()),
             )
         finally:
             os.chdir(cwd)
@@ -1082,9 +1085,11 @@ class AddTests(PorcelainTestCase):
 
         # Should add all files in the directory
         self.assertEqual(len(added), 3)
-        self.assertIn("subdir/file1.txt", added)
-        self.assertIn("subdir/file2.txt", added)
-        self.assertIn("subdir/file3.txt", added)
+        # Normalize paths to use forward slashes for comparison
+        added_normalized = [path.replace(os.sep, "/") for path in added]
+        self.assertIn("subdir/file1.txt", added_normalized)
+        self.assertIn("subdir/file2.txt", added_normalized)
+        self.assertIn("subdir/file3.txt", added_normalized)
 
         # Verify files are actually staged
         index = self.repo.open_index()
@@ -1113,9 +1118,11 @@ class AddTests(PorcelainTestCase):
 
         # Should add all files recursively
         self.assertEqual(len(added), 3)
-        self.assertIn("dir1/file1.txt", added)
-        self.assertIn("dir1/dir2/file2.txt", added)
-        self.assertIn("dir1/dir2/dir3/file3.txt", added)
+        # Normalize paths to use forward slashes for comparison
+        added_normalized = [path.replace(os.sep, "/") for path in added]
+        self.assertIn("dir1/file1.txt", added_normalized)
+        self.assertIn("dir1/dir2/file2.txt", added_normalized)
+        self.assertIn("dir1/dir2/dir3/file3.txt", added_normalized)
 
         # Verify files are actually staged
         index = self.repo.open_index()
@@ -1152,8 +1159,10 @@ class AddTests(PorcelainTestCase):
 
         # Should only add the untracked files
         self.assertEqual(len(added), 2)
-        self.assertIn("mixed/untracked1.txt", added)
-        self.assertIn("mixed/untracked2.txt", added)
+        # Normalize paths to use forward slashes for comparison
+        added_normalized = [path.replace(os.sep, "/") for path in added]
+        self.assertIn("mixed/untracked1.txt", added_normalized)
+        self.assertIn("mixed/untracked2.txt", added_normalized)
         self.assertNotIn("mixed/tracked.txt", added)
 
         # Verify the index contains all files
@@ -1192,12 +1201,18 @@ class AddTests(PorcelainTestCase):
         added, ignored = porcelain.add(self.repo.path, paths=["testdir"])
 
         # Should only add non-ignored files
-        self.assertEqual(set(added), {"testdir/important.txt", "testdir/readme.md"})
+        # Normalize paths to use forward slashes for comparison
+        added_normalized = {path.replace(os.sep, "/") for path in added}
+        self.assertEqual(
+            added_normalized, {"testdir/important.txt", "testdir/readme.md"}
+        )
 
         # Check ignored files
-        self.assertIn("testdir/debug.log", ignored)
-        self.assertIn("testdir/temp.tmp", ignored)
-        self.assertIn("testdir/build/", ignored)
+        # Normalize paths to use forward slashes for comparison
+        ignored_normalized = {path.replace(os.sep, "/") for path in ignored}
+        self.assertIn("testdir/debug.log", ignored_normalized)
+        self.assertIn("testdir/temp.tmp", ignored_normalized)
+        self.assertIn("testdir/build/", ignored_normalized)
 
     def test_add_multiple_directories(self) -> None:
         """Test adding multiple directories in one call."""
@@ -1215,9 +1230,11 @@ class AddTests(PorcelainTestCase):
 
         # Should add all files from all directories
         self.assertEqual(len(added), 6)
+        # Normalize paths to use forward slashes for comparison
+        added_normalized = [path.replace(os.sep, "/") for path in added]
         for dirname in ["dir1", "dir2", "dir3"]:
             for i in range(2):
-                self.assertIn(f"{dirname}/file{i}.txt", added)
+                self.assertIn(f"{dirname}/file{i}.txt", added_normalized)
 
         # Verify all files are staged
         index = self.repo.open_index()