Two fixes for add handling (#1578)

* 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.  Fixed #1178

* Improve ``porcelain.add()`` documentation to correctly describe
default
  behavior. Fixes #895

Author: jelmer

NEWS

@@ -6,6 +6,14 @@
    Now only resolves the parent directory for symlinks, matching Git's behavior.
    (Jelmer Vernooij, #789)
 
+ * 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
    files in the parent directory, matching Git's behavior. The ``is_ignored()`` method

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:
@@ -620,25 +625,50 @@ def add(repo=".", paths=None):
                 # Make relative paths relative to the repo directory
                 path = repo_path / path
 
-            try:
-                # Don't resolve symlinks completely - only resolve the parent directory
-                # to avoid issues when symlinks point outside the repository
-                if path.is_symlink():
-                    # For symlinks, resolve only the parent directory
-                    parent_resolved = path.parent.resolve()
-                    resolved_path = parent_resolved / path.name
-                else:
-                    # For regular files/dirs, resolve normally
-                    resolved_path = path.resolve()
+            # Don't resolve symlinks completely - only resolve the parent directory
+            # to avoid issues when symlinks point outside the repository
+            if path.is_symlink():
+                # For symlinks, resolve only the parent directory
+                parent_resolved = path.parent.resolve()
+                resolved_path = parent_resolved / path.name
+            else:
+                # For regular files/dirs, resolve normally
+                resolved_path = path.resolve()
 
-                relpath = str(resolved_path.relative_to(repo_path))
+            try:
+                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}")
 
+            # Handle directories by scanning their contents
+            if resolved_path.is_dir():
+                # Check if the directory itself is ignored
+                dir_relpath = posixpath.join(relpath, "") if relpath != "." else ""
+                if dir_relpath and ignore_manager.is_ignored(dir_relpath):
+                    ignored.add(dir_relpath)
+                    continue
+
+                # When adding a directory, add all untracked files within it
+                current_untracked = list(
+                    get_untracked_paths(
+                        str(resolved_path),
+                        str(repo_path),
+                        r.open_index(),
+                    )
+                )
+                for untracked_path in current_untracked:
+                    # If we're scanning a subdirectory, adjust the path
+                    if relpath != ".":
+                        untracked_path = posixpath.join(relpath, untracked_path)
+
+                    if not ignore_manager.is_ignored(untracked_path):
+                        relpaths.append(untracked_path)
+                    else:
+                        ignored.add(untracked_path)
+                continue
+
             # FIXME: Support patterns
-            if path.is_dir():
-                relpath = os.path.join(relpath, "")
             if ignore_manager.is_ignored(relpath):
                 ignored.add(relpath)
                 continue

tests/test_porcelain.py

@@ -920,9 +920,12 @@ def test_add_default_paths(self) -> None:
         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)
@@ -981,7 +984,7 @@ def test_add_ignored(self) -> None:
         )
         self.assertIn(b"bar", self.repo.open_index())
         self.assertEqual({"bar"}, set(added))
-        self.assertEqual({"foo", os.path.join("subdir", "")}, ignored)
+        self.assertEqual({"foo", "subdir/"}, ignored)
 
     def test_add_file_absolute_path(self) -> None:
         # Absolute paths are (not yet) supported
@@ -1044,6 +1047,199 @@ def test_add_symlink_outside_repo(self) -> None:
         index = self.repo.open_index()
         self.assertIn(b"symlink_to_nowhere", index)
 
+    def test_add_repo_path(self) -> None:
+        """Test adding the repository path itself should add all untracked files."""
+        # Create some untracked files
+        with open(os.path.join(self.repo.path, "file1.txt"), "w") as f:
+            f.write("content1")
+        with open(os.path.join(self.repo.path, "file2.txt"), "w") as f:
+            f.write("content2")
+
+        # Add the repository path itself
+        added, ignored = porcelain.add(self.repo.path, paths=[self.repo.path])
+
+        # Should add all untracked files, not stage './'
+        self.assertIn("file1.txt", added)
+        self.assertIn("file2.txt", added)
+        self.assertNotIn("./", added)
+
+        # Verify files are actually staged
+        index = self.repo.open_index()
+        self.assertIn(b"file1.txt", index)
+        self.assertIn(b"file2.txt", index)
+
+    def test_add_directory_contents(self) -> None:
+        """Test adding a directory adds all files within it."""
+        # Create a subdirectory with multiple files
+        subdir = os.path.join(self.repo.path, "subdir")
+        os.mkdir(subdir)
+        with open(os.path.join(subdir, "file1.txt"), "w") as f:
+            f.write("content1")
+        with open(os.path.join(subdir, "file2.txt"), "w") as f:
+            f.write("content2")
+        with open(os.path.join(subdir, "file3.txt"), "w") as f:
+            f.write("content3")
+
+        # Add the directory
+        added, ignored = porcelain.add(self.repo.path, paths=["subdir"])
+
+        # Should add all files in the directory
+        self.assertEqual(len(added), 3)
+        # 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()
+        self.assertIn(b"subdir/file1.txt", index)
+        self.assertIn(b"subdir/file2.txt", index)
+        self.assertIn(b"subdir/file3.txt", index)
+
+    def test_add_nested_directories(self) -> None:
+        """Test adding a directory with nested subdirectories."""
+        # Create nested directory structure
+        dir1 = os.path.join(self.repo.path, "dir1")
+        dir2 = os.path.join(dir1, "dir2")
+        dir3 = os.path.join(dir2, "dir3")
+        os.makedirs(dir3)
+
+        # Add files at each level
+        with open(os.path.join(dir1, "file1.txt"), "w") as f:
+            f.write("level1")
+        with open(os.path.join(dir2, "file2.txt"), "w") as f:
+            f.write("level2")
+        with open(os.path.join(dir3, "file3.txt"), "w") as f:
+            f.write("level3")
+
+        # Add the top-level directory
+        added, ignored = porcelain.add(self.repo.path, paths=["dir1"])
+
+        # Should add all files recursively
+        self.assertEqual(len(added), 3)
+        # 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()
+        self.assertIn(b"dir1/file1.txt", index)
+        self.assertIn(b"dir1/dir2/file2.txt", index)
+        self.assertIn(b"dir1/dir2/dir3/file3.txt", index)
+
+    def test_add_directory_with_tracked_files(self) -> None:
+        """Test adding a directory with some files already tracked."""
+        # Create a subdirectory with files
+        subdir = os.path.join(self.repo.path, "mixed")
+        os.mkdir(subdir)
+
+        # Create and commit one file
+        tracked_file = os.path.join(subdir, "tracked.txt")
+        with open(tracked_file, "w") as f:
+            f.write("already tracked")
+        porcelain.add(self.repo.path, paths=[tracked_file])
+        porcelain.commit(
+            repo=self.repo.path,
+            message=b"Add tracked file",
+            author=b"test <email>",
+            committer=b"test <email>",
+        )
+
+        # Add more untracked files
+        with open(os.path.join(subdir, "untracked1.txt"), "w") as f:
+            f.write("new file 1")
+        with open(os.path.join(subdir, "untracked2.txt"), "w") as f:
+            f.write("new file 2")
+
+        # Add the directory
+        added, ignored = porcelain.add(self.repo.path, paths=["mixed"])
+
+        # Should only add the untracked files
+        self.assertEqual(len(added), 2)
+        # 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
+        index = self.repo.open_index()
+        self.assertIn(b"mixed/tracked.txt", index)
+        self.assertIn(b"mixed/untracked1.txt", index)
+        self.assertIn(b"mixed/untracked2.txt", index)
+
+    def test_add_directory_with_gitignore(self) -> None:
+        """Test adding a directory respects .gitignore patterns."""
+        # Create .gitignore
+        with open(os.path.join(self.repo.path, ".gitignore"), "w") as f:
+            f.write("*.log\n*.tmp\nbuild/\n")
+
+        # Create directory with mixed files
+        testdir = os.path.join(self.repo.path, "testdir")
+        os.mkdir(testdir)
+
+        # Create various files
+        with open(os.path.join(testdir, "important.txt"), "w") as f:
+            f.write("keep this")
+        with open(os.path.join(testdir, "debug.log"), "w") as f:
+            f.write("ignore this")
+        with open(os.path.join(testdir, "temp.tmp"), "w") as f:
+            f.write("ignore this too")
+        with open(os.path.join(testdir, "readme.md"), "w") as f:
+            f.write("keep this too")
+
+        # Create a build directory that should be ignored
+        builddir = os.path.join(testdir, "build")
+        os.mkdir(builddir)
+        with open(os.path.join(builddir, "output.txt"), "w") as f:
+            f.write("ignore entire directory")
+
+        # Add the directory
+        added, ignored = porcelain.add(self.repo.path, paths=["testdir"])
+
+        # Should only add non-ignored files
+        # 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
+        # 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."""
+        # Create multiple directories
+        for dirname in ["dir1", "dir2", "dir3"]:
+            dirpath = os.path.join(self.repo.path, dirname)
+            os.mkdir(dirpath)
+            # Add files to each directory
+            for i in range(2):
+                with open(os.path.join(dirpath, f"file{i}.txt"), "w") as f:
+                    f.write(f"content {dirname} {i}")
+
+        # Add all directories at once
+        added, ignored = porcelain.add(self.repo.path, paths=["dir1", "dir2", "dir3"])
+
+        # 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_normalized)
+
+        # Verify all files are staged
+        index = self.repo.open_index()
+        self.assertEqual(len(index), 6)
+
 
 class RemoveTests(PorcelainTestCase):
     def test_remove_file(self) -> None: