fix: address CodeRabbit findings on orphan-gitlink stripping

Three correctness issues found in review of PR #1385:

- declared_submodule_paths() swallowed every SubprocessCommandError
  from `git config --file .gitmodules --get-regexp`, including exit
  128 for a malformed .gitmodules -- indistinguishable from the
  documented exit 1 for "no matching lines". A malformed .gitmodules
  would make drop_orphan_gitlinks() strip every gitlink, including
  legitimately declared ones. Now only exit 1 is treated as empty;
  anything else propagates.

- declared_submodule_paths() only checked for a `path` entry, so a
  stanza declaring `path` but no `url` was treated as "declared" and
  kept, even though `git submodule update --init --recursive` still
  fails on it with the same "No url found for submodule path" error
  the fix exists to avoid. Now a path only counts as declared when a
  matching `url` entry exists too.

- The `git update-index --force-remove <path>` call had no `--`
  terminator, so a gitlink path starting with `-` (e.g. a repo
  containing a path literally named `--cacheinfo`) could be parsed as
  an option instead of a path and fail. Added `--`.

Added tests/test_git_gitlinks.py covering all three; confirmed each
fails against the pre-fix code and passes against the fix.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DziAso23sxMgeTSUmzkus8

Author: claude

dfetch/vcs/git_gitlinks.py

@@ -32,25 +32,45 @@ def gitlink_paths() -> list[str]:
     return paths
 
 
-def declared_submodule_paths() -> set[str]:
-    """Return the submodule paths declared in .gitmodules, if any."""
-    if not os.path.isfile(GIT_MODULES_FILE):
-        return set()
+def _submodule_config_values(key: str) -> dict[str, str]:
+    """Return {submodule name: value} for a .gitmodules key ("path" or "url").
+
+    Args:
+        key: The per-submodule config key to read, e.g. "path" or "url".
+    """
     try:
         result = run_on_cmdline(
             logger,
-            ["git", "config", "--file", GIT_MODULES_FILE, "--get-regexp", "path"],
+            ["git", "config", "--file", GIT_MODULES_FILE, "--get-regexp", key],
         )
-    except SubprocessCommandError:
-        return set()
+    except SubprocessCommandError as exc:
+        # get-regexp documents exit status 1 for "no matching lines"; anything
+        # else (e.g. a malformed .gitmodules) must not be mistaken for that.
+        if exc.returncode == 1:
+            return {}
+        raise
     return {
-        match.group(1)
+        match.group(1): match.group(2)
         for match in re.finditer(
-            r"submodule\.(?:.*)\.path\s+(.*)", result.stdout.decode()
+            rf"submodule\.(.*)\.{key}\s+(.*)", result.stdout.decode()
         )
     }
 
 
+def declared_submodule_paths() -> set[str]:
+    """Return the paths of .gitmodules submodules that also declare a url.
+
+    A stanza with a ``path`` but no ``url`` cannot be initialized by
+    ``git submodule update`` either, so it is treated the same as an orphan
+    gitlink rather than as "declared".
+    """
+    if not os.path.isfile(GIT_MODULES_FILE):
+        return set()
+    paths = _submodule_config_values("path")
+    urls = _submodule_config_values("url")
+    return {path for name, path in paths.items() if name in urls}
+
+
 def drop_orphan_gitlinks() -> None:
     """Strip gitlinks with no .gitmodules entry from the index.
 
@@ -67,4 +87,6 @@ def drop_orphan_gitlinks() -> None:
                 "Gitlink '%s' has no '.gitmodules' entry; skipping it as a submodule",
                 path,
             )
-            run_on_cmdline(logger, ["git", "update-index", "--force-remove", path])
+            run_on_cmdline(
+                logger, ["git", "update-index", "--force-remove", "--", path]
+            )

tests/test_git_gitlinks.py

@@ -0,0 +1,101 @@
+"""Test dfetch.vcs.git_gitlinks."""
+
+# mypy: ignore-errors
+# flake8: noqa
+
+import subprocess
+
+import pytest
+
+from dfetch.util.cmdline import SubprocessCommandError
+from dfetch.vcs import git_gitlinks
+
+
+def _init_git_repo(path):
+    """Initialize a real git repo at *path* with a committer identity set.
+
+    Args:
+        path: Directory to initialize as a git repo.
+    """
+    subprocess.check_call(["git", "init", "--quiet"], cwd=path)
+    subprocess.check_call(["git", "config", "user.email", "you@example.com"], cwd=path)
+    subprocess.check_call(["git", "config", "user.name", "John Doe"], cwd=path)
+    subprocess.check_call(["git", "config", "commit.gpgsign", "false"], cwd=path)
+
+
+def _add_gitlink(path, gitlink_path):
+    """Stage a gitlink (mode 160000) at *gitlink_path* using the repo's own HEAD sha."""
+    sha = (
+        subprocess.check_output(["git", "rev-parse", "HEAD"], cwd=path).decode().strip()
+    )
+    subprocess.check_call(
+        [
+            "git",
+            "update-index",
+            "--add",
+            "--cacheinfo",
+            f"160000,{sha},{gitlink_path}",
+        ],
+        cwd=path,
+    )
+
+
+def test_declared_submodule_paths_requires_a_url(tmp_path, monkeypatch):
+    """A .gitmodules stanza with a path but no url must not count as declared.
+
+    git submodule update --init --recursive fails on such a stanza with the
+    same "No url found for submodule path" error as a fully undeclared
+    gitlink, so it must be treated the same way: as an orphan to drop.
+    """
+    _init_git_repo(tmp_path)
+    (tmp_path / "root").write_text("root")
+    subprocess.check_call(["git", "add", "root"], cwd=tmp_path)
+    subprocess.check_call(["git", "commit", "-qm", "initial"], cwd=tmp_path)
+    _add_gitlink(tmp_path, "orphan")
+    (tmp_path / ".gitmodules").write_text('[submodule "orphan"]\n\tpath = orphan\n')
+    subprocess.check_call(["git", "add", ".gitmodules"], cwd=tmp_path)
+    subprocess.check_call(
+        ["git", "commit", "-qm", "path-only declaration"], cwd=tmp_path
+    )
+
+    monkeypatch.chdir(tmp_path)
+    assert git_gitlinks.declared_submodule_paths() == set()
+
+
+def test_declared_submodule_paths_propagates_malformed_gitmodules(
+    tmp_path, monkeypatch
+):
+    """A malformed .gitmodules must raise, not be silently treated as 'no submodules'.
+
+    Swallowing every SubprocessCommandError here would make
+    drop_orphan_gitlinks() strip every gitlink -- including legitimately
+    declared ones -- whenever .gitmodules simply fails to parse.
+    """
+    _init_git_repo(tmp_path)
+    (tmp_path / ".gitmodules").write_text('[submodule "broken"\n')
+
+    monkeypatch.chdir(tmp_path)
+    with pytest.raises(SubprocessCommandError):
+        git_gitlinks.declared_submodule_paths()
+
+
+def test_drop_orphan_gitlinks_handles_leading_dash_path(tmp_path, monkeypatch):
+    """A gitlink path starting with '-' must not be parsed as a git option.
+
+    Without a '--' terminator before the path, `git update-index
+    --force-remove --cacheinfo` would try to parse '--cacheinfo' as an
+    option instead of a path and fail.
+    """
+    _init_git_repo(tmp_path)
+    (tmp_path / "root").write_text("root")
+    subprocess.check_call(["git", "add", "root"], cwd=tmp_path)
+    subprocess.check_call(["git", "commit", "-qm", "initial"], cwd=tmp_path)
+    _add_gitlink(tmp_path, "--cacheinfo")
+
+    monkeypatch.chdir(tmp_path)
+    git_gitlinks.drop_orphan_gitlinks()
+
+    remaining = subprocess.check_output(
+        ["git", "ls-files", "-s"], cwd=tmp_path
+    ).decode()
+    assert "--cacheinfo" not in remaining

tests/test_git_vcs.py

@@ -226,6 +226,11 @@ def test_filter_submodules_sibling_of_src_not_removed(tmp_path, monkeypatch):
 
 
 def _init_git_repo(path):
+    """Initialize a real git repo at *path* with a committer identity set.
+
+    Args:
+        path: Directory to initialize as a git repo.
+    """
     subprocess.check_call(["git", "init", "--initial-branch=main", "--quiet"], cwd=path)
     subprocess.check_call(["git", "config", "user.email", "you@example.com"], cwd=path)
     subprocess.check_call(["git", "config", "user.name", "John Doe"], cwd=path)