fix: Follow submodule gitdir files when opening and perform basic validation (#2585)

codex · Byron · Byron · commit 82c483a98cfe · 2026-05-08T11:42:17.000+08:00
Modern submodules store a .git file in the worktree whose gitdir: value points
at the repository to open. The previous resolver treated every non-directory
.git path like an uninitialized submodule and fell back to .git/modules/&lt;name&gt;,
which can open the wrong repository after a submodule gitdir is relocated or
renamed. That shows up as phantom submodule HEAD changes in gix status.

Git baseline: Git setup.c::read_gitfile_gently() parses gitdir:
files and resolves relative targets against the .git file location;
submodule.c::submodule_to_gitdir() consults that gitfile before falling back to
the name-derived .git/modules path.

Update Submodule::git_dir_try_old_form() to validate the submodule name, then
follow worktree .git files when present while preserving the old-form directory
and uninitialized fallback behavior. Validate present gitdir file targets for
direct state/open/status queries so broken submodule checkouts are reported
if the gitlink doesn't point to a directory.
For status ignore=all, still parse valid gitdir files to keep state accurate,
but skip target validation and fall back without error if the gitdir file itself
is malformed or unreadable. Derive State::is_old_form directly from whether
the worktree .git path is a directory so a divergent modern gitlink is not
misclassified as old form.

Co-authored-by: Sebastian Thiel &lt;sebastian.thiel@icloud.com&gt;
diff --git a/gix/src/submodule/errors.rs b/gix/src/submodule/errors.rs
@@ -91,6 +91,13 @@ pub mod git_dir_try_old_form {
     pub enum Error {
         #[error(transparent)]
         PathConfiguration(#[from] gix_submodule::config::path::Error),
+        #[error("The gitdir file at '{}' contains an invalid gitdir target", .gitdir_file.display())]
+        InvalidGitDirFileTarget {
+            gitdir_file: std::path::PathBuf,
+            target: Option<std::path::PathBuf>,
+            #[source]
+            source: Option<gix_discover::path::from_gitdir_file::Error>,
+        },
         #[error(transparent)]
         GitDir(#[from] gix_validate::submodule::name::Error),
     }
diff --git a/gix/src/submodule/mod.rs b/gix/src/submodule/mod.rs
@@ -232,25 +232,56 @@ impl Submodule<'_> {
     /// the superproject's worktree where it actually *is* located if the submodule in the 'old-form', thus is a directory
     /// inside of the superproject's work-tree.
     ///
-    /// Note that 'old-form' paths returned aren't verified, i.e. the `.git` repository might be corrupt or otherwise
-    /// invalid - it's left to the caller to try to open it.
+    /// Note that paths returned aren't fully verified, i.e. the `.git` repository might be corrupt or otherwise
+    /// invalid - it's left to the caller to try to open it. `.git` files are parsed when present and their target
+    /// is required to be a directory though, as a malformed gitdir file means a broken submodule checkout.
     ///
-    /// Also note that the returned path may not actually exist.
+    /// Also note that the fallback path returned for uninitialized submodules may not actually exist.
     pub fn git_dir_try_old_form(&self) -> Result<PathBuf, git_dir_try_old_form::Error> {
+        self.git_dir_try_old_form_inner(true)
+    }
+
+    /// Return the best-known submodule repository path, optionally parsing a `.git` file target
+    /// and requiring it to be a directory if `validate_gitdir_file_target` is `true`.
+    ///
+    /// Validation may be skipped when callers only need coarse state, like `status_opts()` with
+    /// `Ignore::All`, which returns before opening or inspecting the submodule repository.
+    fn git_dir_try_old_form_inner(
+        &self,
+        validate_gitdir_file_target: bool,
+    ) -> Result<PathBuf, git_dir_try_old_form::Error> {
+        let git_dir = self.git_dir()?;
         let worktree_gitdir = self.worktree_gitdir()?;
-        let worktree_gitdir_or_modules_gitdir = if worktree_gitdir.is_dir() {
+        let git_dir = if worktree_gitdir.is_dir() {
             worktree_gitdir
+        } else if worktree_gitdir.is_file() {
+            if validate_gitdir_file_target {
+                let git_dir = gix_discover::path::from_gitdir_file(&worktree_gitdir).map_err(|source| {
+                    git_dir_try_old_form::Error::InvalidGitDirFileTarget {
+                        gitdir_file: worktree_gitdir.clone(),
+                        target: None,
+                        source: Some(source),
+                    }
+                })?;
+                if !git_dir.is_dir() {
+                    return Err(git_dir_try_old_form::Error::InvalidGitDirFileTarget {
+                        gitdir_file: worktree_gitdir,
+                        target: Some(git_dir),
+                        source: None,
+                    });
+                }
+                git_dir
+            } else {
+                gix_discover::path::from_gitdir_file(&worktree_gitdir).unwrap_or(git_dir)
+            }
         } else {
-            self.git_dir()?
+            git_dir
         };
-        Ok(worktree_gitdir_or_modules_gitdir)
+        Ok(git_dir)
     }
 
-    /// Query various parts of the submodule and assemble it into state information.
-    #[doc(alias = "status", alias = "git2")]
-    pub fn state(&self) -> Result<State, state::Error> {
-        let maybe_old_path = self.git_dir_try_old_form()?;
-        let git_dir = self.git_dir()?;
+    fn state_inner(&self, validate_gitdir_file_target: bool) -> Result<State, state::Error> {
+        let maybe_old_path = self.git_dir_try_old_form_inner(validate_gitdir_file_target)?;
         let worktree_git = self.worktree_gitdir()?;
         let superproject_configuration = self
             .state
@@ -263,12 +294,18 @@ impl Submodule<'_> {
             .any(|section| section.header().subsection_name() == Some(self.name.as_ref()));
         Ok(State {
             repository_exists: maybe_old_path.is_dir(),
-            is_old_form: maybe_old_path != git_dir,
+            is_old_form: worktree_git.is_dir(),
             worktree_checkout: worktree_git.exists(),
             superproject_configuration,
         })
     }
 
+    /// Query various parts of the submodule and assemble it into state information.
+    #[doc(alias = "status", alias = "git2")]
+    pub fn state(&self) -> Result<State, state::Error> {
+        self.state_inner(true)
+    }
+
     /// Open the submodule as repository, or `None` if the submodule wasn't initialized yet.
     ///
     /// More states can be derived here:
@@ -375,7 +412,7 @@ pub mod status {
             )
                 -> crate::status::Platform<'a, gix_features::progress::Discard>,
         ) -> Result<Status, Error> {
-            let mut state = self.state()?;
+            let mut state = self.state_inner(ignore != config::Ignore::All)?;
             if ignore == config::Ignore::All {
                 return Ok(Status {
                     state,
diff --git a/gix/tests/fixtures/generated-archives/make_submodules.tar b/gix/tests/fixtures/generated-archives/make_submodules.tar
diff --git a/gix/tests/fixtures/make_submodules.sh b/gix/tests/fixtures/make_submodules.sh
@@ -120,6 +120,34 @@ git init with-submodules
   git submodule add ../module1 dir/m1
 )
 
+git init submodule-with-divergent-gitlink
+(cd submodule-with-divergent-gitlink
+  git submodule add ../module1 outer/inner
+  git commit -m "add nested submodule"
+
+  mv .git/modules/outer/inner .git/modules/inner
+  rmdir .git/modules/outer
+  git config --file .git/modules/inner/config core.worktree ../../../outer/inner
+  printf 'gitdir: ../../.git/modules/inner\n' >outer/inner/.git
+
+  git clone --bare ../module1 .git/modules/outer/inner
+  git -C .git/modules/outer/inner update-ref HEAD "$(git -C ../module1 rev-parse @~1)"
+)
+
+git init submodule-with-missing-gitlink-target
+(cd submodule-with-missing-gitlink-target
+  git submodule add ../module1 m1
+  git commit -m "add submodule"
+  printf 'gitdir: ../missing\n' >m1/.git
+)
+
+git init submodule-with-malformed-gitlink
+(cd submodule-with-malformed-gitlink
+  git submodule add ../module1 m1
+  git commit -m "add submodule"
+  printf 'bogus\n' >m1/.git
+)
+
 cp -R with-submodules with-submodules-in-index
 (cd with-submodules-in-index
   git add .
@@ -167,4 +195,4 @@ git clone with-submodules not-a-submodule
   git add m1 && git commit -m "no submodule in index and commit, but in configuration"
 )
 
-git init unborn
+git init unborn
diff --git a/gix/tests/gix/submodule.rs b/gix/tests/gix/submodule.rs
@@ -157,6 +157,195 @@ mod open {
         Ok(())
     }
 
+    #[test]
+    fn gitlink_target_takes_precedence_over_name_in_git_dir_resolution() -> crate::Result {
+        let repo = repo("submodule-with-divergent-gitlink")?;
+        let sm = repo
+            .submodules()?
+            .expect("modules present")
+            .next()
+            .expect("one submodule");
+
+        let git_dir_from_name = sm.git_dir()?;
+        assert!(
+            git_dir_from_name.ends_with("modules/outer/inner"),
+            "the name-derived path is the fallback location, what it should be per submodule configuration/name"
+        );
+
+        let git_dir_from_gitlink = sm.git_dir_try_old_form()?;
+        assert!(
+            git_dir_from_gitlink.ends_with("modules/inner"),
+            "the worktree .git file points at the relocated repository"
+        );
+        assert_ne!(
+            git_dir_from_gitlink, git_dir_from_name,
+            "the gitlink target must be authoritative even when it differs from .git/modules/<name>"
+        );
+        assert_eq!(
+            sm.state()?,
+            submodule::State {
+                repository_exists: true,
+                is_old_form: false,
+                worktree_checkout: true,
+                superproject_configuration: true,
+            },
+            "a modern gitlink remains modern even when its target differs from the name-derived path"
+        );
+
+        #[cfg(feature = "status")]
+        {
+            let status = sm.status(gix::submodule::config::Ignore::None, false)?;
+            assert_eq!(
+                status.is_dirty(),
+                Some(false),
+                "opening the gitlink target avoids a 'phantom' submodule HEAD change"
+            );
+            assert_eq!(
+                status.checked_out_head_id, status.index_id,
+                "there are no HEAD changes even though the 'phantom' at modules/outer/inner as its HEAD at @~1"
+            );
+
+            let status = sm.status(gix::submodule::config::Ignore::All, false)?;
+            assert_eq!(
+                status.state,
+                submodule::State {
+                    repository_exists: true,
+                    is_old_form: false,
+                    worktree_checkout: true,
+                    superproject_configuration: true,
+                },
+                "ignore=all still follows a parseable divergent gitdir file"
+            );
+        }
+
+        Ok(())
+    }
+
+    #[test]
+    fn broken_gitlink_target_is_reported() -> crate::Result {
+        let repo = repo("submodule-with-missing-gitlink-target")?;
+        let sm = repo
+            .submodules()?
+            .expect("modules present")
+            .next()
+            .expect("one submodule");
+
+        assert!(matches!(
+            sm.git_dir_try_old_form(),
+            Err(submodule::git_dir_try_old_form::Error::InvalidGitDirFileTarget {
+                target: Some(target),
+                source: None,
+                ..
+            }) if target.ends_with("missing")
+        ));
+        assert!(matches!(
+            sm.state(),
+            Err(submodule::state::Error::GitDirTryOldForm(
+                submodule::git_dir_try_old_form::Error::InvalidGitDirFileTarget {
+                    target: Some(target),
+                    source: None,
+                    ..
+                }
+            )) if target.ends_with("missing")
+        ));
+        assert!(matches!(
+            sm.open(),
+            Err(submodule::open::Error::GitDir(
+                submodule::git_dir_try_old_form::Error::InvalidGitDirFileTarget {
+                    target: Some(target),
+                    source: None,
+                    ..
+                }
+            )) if target.ends_with("missing")
+        ));
+
+        #[cfg(feature = "status")]
+        assert!(
+            matches!(
+                sm.status(gix::submodule::config::Ignore::None, false),
+                Err(submodule::status::Error::State(
+                    submodule::state::Error::GitDirTryOldForm(
+                        submodule::git_dir_try_old_form::Error::InvalidGitDirFileTarget {
+                            target: Some(target),
+                            source: None,
+                            ..
+                        }
+                    )
+                )) if target.ends_with("missing")
+            ),
+            "ignore=none fails as some submodules can't be opened"
+        );
+
+        #[cfg(feature = "status")]
+        {
+            let status = sm.status(gix::submodule::config::Ignore::All, false)?;
+            assert_eq!(
+                status.state,
+                submodule::State {
+                    repository_exists: false,
+                    is_old_form: false,
+                    worktree_checkout: true,
+                    superproject_configuration: true,
+                },
+                "ignore=all does not inspect the broken gitdir target"
+            );
+        }
+
+        Ok(())
+    }
+
+    #[test]
+    fn malformed_gitlink_target_is_ignored_by_ignore_all_status() -> crate::Result {
+        let repo = repo("submodule-with-malformed-gitlink")?;
+        let sm = repo
+            .submodules()?
+            .expect("modules present")
+            .next()
+            .expect("one submodule");
+
+        assert!(matches!(
+            sm.git_dir_try_old_form(),
+            Err(submodule::git_dir_try_old_form::Error::InvalidGitDirFileTarget {
+                target: None,
+                source: Some(_),
+                ..
+            })
+        ));
+
+        #[cfg(feature = "status")]
+        {
+            assert!(
+                matches!(
+                    sm.status(gix::submodule::config::Ignore::None, false),
+                    Err(submodule::status::Error::State(
+                        submodule::state::Error::GitDirTryOldForm(
+                            submodule::git_dir_try_old_form::Error::InvalidGitDirFileTarget {
+                                target: None,
+                                source: Some(_),
+                                ..
+                            }
+                        )
+                    ))
+                ),
+                "ignore=none fails as some submodules can't be opened"
+            );
+
+            let status = sm.status(gix::submodule::config::Ignore::All, false)?;
+            assert_eq!(
+                status.state,
+                submodule::State {
+                    repository_exists: true,
+                    is_old_form: false,
+                    worktree_checkout: true,
+                    superproject_configuration: true,
+                },
+                "ignore=all does not parse the malformed gitdir file"
+            );
+        }
+
+        Ok(())
+    }
+
     #[cfg(feature = "status")]
     mod status {
         use crate::{submodule::repo, util::hex_to_id};
diff --git a/gix/tests/submodule_open.rs b/gix/tests/submodule_open.rs
@@ -24,7 +24,10 @@ fn on_nested_symlink() -> gix_testtools::Result {
         "the workdir remains relative and is available"
     );
     let sm_repo = sm.open()?.expect("repo is present and accessible");
-    assert_eq!(sm_repo.git_dir(), "../../.git/modules/m1");
+    assert!(
+        sm_repo.git_dir().ends_with(".git/modules/m1"),
+        "gitlink resolution may preserve symlink-sensitive parent components"
+    );
     assert_eq!(
         sm_repo.workdir().expect("worktree present as we have one"),
         p("../../m1")
