More add fixes (#1580)

NEWS

@@ -1,5 +1,10 @@
 0.22.9	UNRELEASED
 
+ * Fix ``porcelain.add()`` to stage both untracked and modified files when no
+   paths are specified. Previously, only untracked files were staged, inconsistent
+   with Git's behavior. Now behaves like ``git add -A`` when called without paths.
+   (Jelmer Vernooij, #746)
+
  * Fix ``porcelain.add()`` symlink handling to allow adding symlinks that point
    outside the repository. Previously, the function would fail when trying to
    add a symlink pointing outside the repo due to aggressive path resolution.
@@ -9,7 +14,7 @@
  * 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)
+   repository corruption.  (Jelmer Vernooij, #1178, #655)
 
  * Improve ``porcelain.add()`` documentation to correctly describe default
    behavior. (Jelmer Vernooij, #895)

dulwich/porcelain.py

@@ -587,7 +587,7 @@ def add(repo=".", paths=None):
 
     Args:
       repo: Repository for the files
-      paths: Paths to add. If None, stages all untracked files from the
+      paths: Paths to add. If None, stages all untracked and modified files from the
         current working directory (mimicking 'git add .' behavior).
     Returns: Tuple with set of added files and ignored files
 
@@ -595,27 +595,23 @@ def add(repo=".", paths=None):
     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.
+    Note: When paths=None, this function adds all untracked and modified files
+    from the entire repository, mimicking 'git add -A' behavior.
     """
     ignored = set()
     with open_repo_closing(repo) as r:
         repo_path = Path(r.path).resolve()
         ignore_manager = IgnoreFilterManager.from_repo(r)
+
+        # Get unstaged changes once for the entire operation
+        index = r.open_index()
+        normalizer = r.get_blob_normalizer()
+        filter_callback = normalizer.checkin_normalize
+        all_unstaged_paths = list(get_unstaged_changes(index, r.path, filter_callback))
+
         if not paths:
-            cwd = Path(os.getcwd()).resolve()
-            paths = list(
-                get_untracked_paths(
-                    str(cwd),
-                    str(repo_path),
-                    r.open_index(),
-                )
-            )
-            # If we're in a subdirectory, adjust paths to be relative to repo root
-            if cwd != repo_path:
-                cwd_relative_to_repo = cwd.relative_to(repo_path)
-                paths = [str(cwd_relative_to_repo / p) for p in paths]
+            # When no paths specified, add all untracked and modified files from repo root
+            paths = [str(repo_path)]
         relpaths = []
         if not isinstance(paths, list):
             paths = [paths]
@@ -654,7 +650,7 @@ def add(repo=".", paths=None):
                     get_untracked_paths(
                         str(resolved_path),
                         str(repo_path),
-                        r.open_index(),
+                        index,
                     )
                 )
                 for untracked_path in current_untracked:
@@ -666,6 +662,24 @@ def add(repo=".", paths=None):
                         relpaths.append(untracked_path)
                     else:
                         ignored.add(untracked_path)
+
+                # Also add unstaged (modified) files within this directory
+                for unstaged_path in all_unstaged_paths:
+                    if isinstance(unstaged_path, bytes):
+                        unstaged_path = unstaged_path.decode("utf-8")
+
+                    # Check if this unstaged file is within the directory we're processing
+                    unstaged_full_path = repo_path / unstaged_path
+                    try:
+                        unstaged_full_path.relative_to(resolved_path)
+                        # File is within this directory, add it
+                        if not ignore_manager.is_ignored(unstaged_path):
+                            relpaths.append(unstaged_path)
+                        else:
+                            ignored.add(unstaged_path)
+                    except ValueError:
+                        # File is not within this directory, skip it
+                        continue
                 continue
 
             # FIXME: Support patterns

tests/test_porcelain.py

@@ -955,7 +955,8 @@ class AddTests(PorcelainTestCase):
             os.chdir(cwd)
 
         index = self.repo.open_index()
-        self.assertEqual(sorted(index), [b"foo/blie"])
+        # After fix: add() with no paths should behave like git add -A (add everything)
+        self.assertEqual(sorted(index), [b"blah", b"foo/blie"])
 
     def test_add_file(self) -> None:
         fullpath = os.path.join(self.repo.path, "foo")
@@ -1047,6 +1048,182 @@ class AddTests(PorcelainTestCase):
         index = self.repo.open_index()
         self.assertIn(b"symlink_to_nowhere", index)
 
+    def test_add_symlink_to_file_inside_repo(self) -> None:
+        """Test adding a symlink that points to a file inside the repository."""
+        # Create a regular file
+        target_file = os.path.join(self.repo.path, "target.txt")
+        with open(target_file, "w") as f:
+            f.write("target content")
+
+        # Create a symlink to the file
+        symlink_path = os.path.join(self.repo.path, "link_to_target")
+        os.symlink("target.txt", symlink_path)
+
+        # Add both the target and the symlink
+        added, ignored = porcelain.add(
+            self.repo.path, paths=[target_file, symlink_path]
+        )
+
+        # Both should be added successfully
+        self.assertIn("target.txt", added)
+        self.assertIn("link_to_target", added)
+        self.assertEqual(len(ignored), 0)
+
+        # Verify both are in the index
+        index = self.repo.open_index()
+        self.assertIn(b"target.txt", index)
+        self.assertIn(b"link_to_target", index)
+
+    def test_add_symlink_to_directory_inside_repo(self) -> None:
+        """Test adding a symlink that points to a directory inside the repository."""
+        # Create a directory with some files
+        target_dir = os.path.join(self.repo.path, "target_dir")
+        os.mkdir(target_dir)
+        with open(os.path.join(target_dir, "file1.txt"), "w") as f:
+            f.write("content1")
+        with open(os.path.join(target_dir, "file2.txt"), "w") as f:
+            f.write("content2")
+
+        # Create a symlink to the directory
+        symlink_path = os.path.join(self.repo.path, "link_to_dir")
+        os.symlink("target_dir", symlink_path)
+
+        # Add the symlink
+        added, ignored = porcelain.add(self.repo.path, paths=[symlink_path])
+
+        # When adding a symlink to a directory, it follows the symlink and adds contents
+        self.assertEqual(len(added), 2)
+        self.assertIn("link_to_dir/file1.txt", added)
+        self.assertIn("link_to_dir/file2.txt", added)
+        self.assertEqual(len(ignored), 0)
+
+        # Verify files are added through the symlink path
+        index = self.repo.open_index()
+        self.assertIn(b"link_to_dir/file1.txt", index)
+        self.assertIn(b"link_to_dir/file2.txt", index)
+        # The original target directory files are not added
+        self.assertNotIn(b"target_dir/file1.txt", index)
+        self.assertNotIn(b"target_dir/file2.txt", index)
+
+    def test_add_symlink_chain(self) -> None:
+        """Test adding a chain of symlinks (symlink to symlink)."""
+        # Create a regular file
+        target_file = os.path.join(self.repo.path, "original.txt")
+        with open(target_file, "w") as f:
+            f.write("original content")
+
+        # Create first symlink
+        first_link = os.path.join(self.repo.path, "link1")
+        os.symlink("original.txt", first_link)
+
+        # Create second symlink pointing to first
+        second_link = os.path.join(self.repo.path, "link2")
+        os.symlink("link1", second_link)
+
+        # Add all files
+        added, ignored = porcelain.add(
+            self.repo.path, paths=[target_file, first_link, second_link]
+        )
+
+        # All should be added
+        self.assertEqual(len(added), 3)
+        self.assertIn("original.txt", added)
+        self.assertIn("link1", added)
+        self.assertIn("link2", added)
+
+        # Verify all are in the index
+        index = self.repo.open_index()
+        self.assertIn(b"original.txt", index)
+        self.assertIn(b"link1", index)
+        self.assertIn(b"link2", index)
+
+    def test_add_broken_symlink(self) -> None:
+        """Test adding a broken symlink (points to non-existent target)."""
+        # Create a symlink to a non-existent file
+        broken_link = os.path.join(self.repo.path, "broken_link")
+        os.symlink("does_not_exist.txt", broken_link)
+
+        # Add the broken symlink
+        added, ignored = porcelain.add(self.repo.path, paths=[broken_link])
+
+        # Should be added successfully (Git tracks the symlink, not its target)
+        self.assertIn("broken_link", added)
+        self.assertEqual(len(ignored), 0)
+
+        # Verify it's in the index
+        index = self.repo.open_index()
+        self.assertIn(b"broken_link", index)
+
+    def test_add_symlink_relative_outside_repo(self) -> None:
+        """Test adding a symlink that uses '..' to point outside the repository."""
+        # Create a file outside the repo
+        outside_file = os.path.join(self.test_dir, "outside.txt")
+        with open(outside_file, "w") as f:
+            f.write("outside content")
+
+        # Create a symlink using relative path to go outside
+        symlink_path = os.path.join(self.repo.path, "link_outside")
+        os.symlink("../outside.txt", symlink_path)
+
+        # Add the symlink
+        added, ignored = porcelain.add(self.repo.path, paths=[symlink_path])
+
+        # Should be added successfully
+        self.assertIn("link_outside", added)
+        self.assertEqual(len(ignored), 0)
+
+        # Verify it's in the index
+        index = self.repo.open_index()
+        self.assertIn(b"link_outside", index)
+
+    def test_add_symlink_absolute_to_system(self) -> None:
+        """Test adding a symlink with absolute path to system directory."""
+        # Create a symlink to a system directory
+        symlink_path = os.path.join(self.repo.path, "link_to_tmp")
+        if os.name == "nt":
+            # On Windows, use a system directory like TEMP
+            symlink_target = os.environ["TEMP"]
+        else:
+            # On Unix-like systems, use /tmp
+            symlink_target = "/tmp"
+        os.symlink(symlink_target, symlink_path)
+
+        # Adding a symlink to a directory outside the repo should raise ValueError
+        with self.assertRaises(ValueError) as cm:
+            porcelain.add(self.repo.path, paths=[symlink_path])
+
+        # Check that the error indicates the path is outside the repository
+        self.assertIn("is not in the subpath of", str(cm.exception))
+
+    def test_add_file_through_symlink(self) -> None:
+        """Test adding a file through a symlinked directory."""
+        # Create a directory with a file
+        real_dir = os.path.join(self.repo.path, "real_dir")
+        os.mkdir(real_dir)
+        real_file = os.path.join(real_dir, "file.txt")
+        with open(real_file, "w") as f:
+            f.write("content")
+
+        # Create a symlink to the directory
+        link_dir = os.path.join(self.repo.path, "link_dir")
+        os.symlink("real_dir", link_dir)
+
+        # Try to add the file through the symlink path
+        symlink_file_path = os.path.join(link_dir, "file.txt")
+
+        # This should add the real file, not create a new entry
+        added, ignored = porcelain.add(self.repo.path, paths=[symlink_file_path])
+
+        # The real file should be added
+        self.assertIn("real_dir/file.txt", added)
+        self.assertEqual(len(added), 1)
+
+        # Verify correct path in index
+        index = self.repo.open_index()
+        self.assertIn(b"real_dir/file.txt", index)
+        # Should not create a separate entry for the symlink path
+        self.assertNotIn(b"link_dir/file.txt", index)
+
     def test_add_repo_path(self) -> None:
         """Test adding the repository path itself should add all untracked files."""
         # Create some untracked files
@@ -1240,6 +1417,42 @@ class AddTests(PorcelainTestCase):
         index = self.repo.open_index()
         self.assertEqual(len(index), 6)
 
+    def test_add_default_paths_includes_modified_files(self) -> None:
+        """Test that add() with no paths includes both untracked and modified files."""
+        # Create and commit initial file
+        initial_file = os.path.join(self.repo.path, "existing.txt")
+        with open(initial_file, "w") as f:
+            f.write("initial content\n")
+        porcelain.add(repo=self.repo.path, paths=[initial_file])
+        porcelain.commit(
+            repo=self.repo.path,
+            message=b"initial commit",
+            author=b"test <email>",
+            committer=b"test <email>",
+        )
+
+        # Modify the existing file (this creates an unstaged change)
+        with open(initial_file, "w") as f:
+            f.write("modified content\n")
+
+        # Create a new untracked file
+        new_file = os.path.join(self.repo.path, "new.txt")
+        with open(new_file, "w") as f:
+            f.write("new file content\n")
+
+        # Call add() with no paths - should stage both modified and untracked files
+        added_files, ignored_files = porcelain.add(repo=self.repo.path)
+
+        # Verify both files were added
+        self.assertIn("existing.txt", added_files)
+        self.assertIn("new.txt", added_files)
+        self.assertEqual(len(ignored_files), 0)
+
+        # Verify both files are now staged
+        index = self.repo.open_index()
+        self.assertIn(b"existing.txt", index)
+        self.assertIn(b"new.txt", index)
+
 
 class RemoveTests(PorcelainTestCase):
     def test_remove_file(self) -> None: