Merge pull request #1620 from Byron/fix-discovery

Byron · web-flow · commit 64872690e60e · 2024-10-11T18:13:25.000+02:00
fix discovery
diff --git a/gix-discover/src/is.rs b/gix-discover/src/is.rs
@@ -9,107 +9,6 @@ pub fn bare(git_dir_candidate: &Path) -> bool {
     !(git_dir_candidate.join("index").exists() || (git_dir_candidate.file_name() == Some(OsStr::new(DOT_GIT_DIR))))
 }
 
-/// Parse `<git_dir_candidate>/config` quickly to evaluate the value of the `bare` line, or return `true` if the file doesn't exist
-/// similar to what`guess_repository_type` seems to be doing.
-/// Return `None` if the `bare` line can't be found or the value of `bare` can't be determined.
-fn bare_by_config(git_dir_candidate: &Path) -> std::io::Result<Option<bool>> {
-    match std::fs::read(git_dir_candidate.join("config")) {
-        Ok(buf) => Ok(config::parse_bare(&buf)),
-        Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(Some(true)),
-        Err(err) => Err(err),
-    }
-}
-
-// Copied and adapted from `gix-config-value::boolean`.
-mod config {
-    use bstr::{BStr, ByteSlice};
-
-    /// Note that we intentionally turn repositories that have a worktree configuration into bare repos,
-    /// as we don't actually parse the worktree from the config file and expect the caller to do the right
-    /// think when seemingly seeing bare repository.
-    /// The reason we do this is to not incorrectly pretend this is a worktree.
-    pub(crate) fn parse_bare(buf: &[u8]) -> Option<bool> {
-        let mut is_bare = None;
-        let mut has_worktree_configuration = false;
-        for line in buf.lines() {
-            if is_bare.is_none() {
-                if let Some(line) = line.trim().strip_prefix(b"bare") {
-                    is_bare = match line.first() {
-                        None => Some(true),
-                        Some(c) if *c == b'=' => parse_bool(line.get(1..)?.trim_start().as_bstr()),
-                        Some(c) if c.is_ascii_whitespace() => match line.split_once_str(b"=") {
-                            Some((_left, right)) => parse_bool(right.trim_start().as_bstr()),
-                            None => Some(true),
-                        },
-                        Some(_other_char_) => None,
-                    };
-                    continue;
-                }
-            }
-            if line.trim().strip_prefix(b"worktree").is_some() {
-                has_worktree_configuration = true;
-                break;
-            }
-        }
-        is_bare.map(|bare| bare || has_worktree_configuration)
-    }
-
-    fn parse_bool(value: &BStr) -> Option<bool> {
-        Some(if parse_true(value) {
-            true
-        } else if parse_false(value) {
-            false
-        } else {
-            use std::str::FromStr;
-            if let Some(integer) = value.to_str().ok().and_then(|s| i64::from_str(s).ok()) {
-                integer != 0
-            } else {
-                return None;
-            }
-        })
-    }
-
-    fn parse_true(value: &BStr) -> bool {
-        value.eq_ignore_ascii_case(b"yes") || value.eq_ignore_ascii_case(b"on") || value.eq_ignore_ascii_case(b"true")
-    }
-
-    fn parse_false(value: &BStr) -> bool {
-        value.eq_ignore_ascii_case(b"no")
-            || value.eq_ignore_ascii_case(b"off")
-            || value.eq_ignore_ascii_case(b"false")
-            || value.is_empty()
-    }
-
-    #[cfg(test)]
-    mod tests {
-        use super::*;
-
-        #[test]
-        fn various() {
-            for (input, expected) in [
-                ("bare=true", Some(true)),
-                ("bare=1", Some(true)),
-                ("bare =1", Some(true)),
-                ("bare= yes", Some(true)),
-                ("bare=false", Some(false)),
-                ("bare=0", Some(false)),
-                ("bare=blah", None),
-                ("bare=", Some(false)),
-                ("bare=  \n", Some(false)),
-                ("bare = true \n", Some(true)),
-                ("\t bare = false \n", Some(false)),
-                ("\n\tbare=true", Some(true)),
-                ("\n\tbare=true\n\tfoo", Some(true)),
-                ("\n\tbare ", Some(true)),
-                ("\n\tbare", Some(true)),
-                ("not found\nreally", None),
-            ] {
-                assert_eq!(parse_bare(input.as_bytes()), expected, "{input:?}");
-            }
-        }
-    }
-}
-
 /// Returns true if `git_dir` is located within a `.git/modules` directory, indicating it's a submodule clone.
 pub fn submodule_git_dir(git_dir: &Path) -> bool {
     let mut last_comp = None;
@@ -141,12 +40,15 @@ pub fn git(git_dir: &Path) -> Result<crate::repository::Kind, crate::is_git::Err
         source: err,
         path: git_dir.into(),
     })?;
-    git_with_metadata(git_dir, git_dir_metadata)
+    // precompose-unicode can't be known here, so we just default it to false, hoping it won't matter.
+    let cwd = gix_fs::current_dir(false)?;
+    git_with_metadata(git_dir, git_dir_metadata, &cwd)
 }
 
 pub(crate) fn git_with_metadata(
     git_dir: &Path,
     git_dir_metadata: std::fs::Metadata,
+    cwd: &Path,
 ) -> Result<crate::repository::Kind, crate::is_git::Error> {
     #[derive(Eq, PartialEq)]
     enum Kind {
@@ -166,6 +68,8 @@ pub(crate) fn git_with_metadata(
     {
         // Fast-path: avoid doing the complete search if HEAD is already not there.
         // TODO(reftable): use a ref-store to lookup HEAD if ref-tables should be supported, or detect ref-tables beforehand.
+        //                 Actually ref-tables still keep a specially marked `HEAD` around, so nothing might be needed here
+        //                 Even though our head-check later would fail without supporting it.
         if !dot_git.join("HEAD").exists() {
             return Err(crate::is_git::Error::MissingHead);
         }
@@ -236,25 +140,26 @@ pub(crate) fn git_with_metadata(
         },
         Kind::MaybeRepo => {
             let conformed_git_dir = if git_dir == Path::new(".") {
-                gix_path::realpath(git_dir)
+                gix_path::realpath_opts(git_dir, cwd, gix_path::realpath::MAX_SYMLINKS)
                     .map(Cow::Owned)
                     .unwrap_or(Cow::Borrowed(git_dir))
             } else {
-                Cow::Borrowed(git_dir)
+                gix_path::normalize(git_dir.into(), cwd).unwrap_or(Cow::Borrowed(git_dir))
             };
             if bare(conformed_git_dir.as_ref()) || conformed_git_dir.extension() == Some(OsStr::new("git")) {
                 crate::repository::Kind::PossiblyBare
             } else if submodule_git_dir(conformed_git_dir.as_ref()) {
                 crate::repository::Kind::SubmoduleGitDir
-            } else if conformed_git_dir.file_name() == Some(OsStr::new(DOT_GIT_DIR))
-                || !bare_by_config(conformed_git_dir.as_ref())
-                    .map_err(|err| crate::is_git::Error::Metadata {
-                        source: err,
-                        path: conformed_git_dir.join("config"),
-                    })?
-                    .ok_or(crate::is_git::Error::Inconclusive)?
-            {
+            } else if conformed_git_dir.file_name() == Some(OsStr::new(DOT_GIT_DIR)) {
                 crate::repository::Kind::WorkTree { linked_git_dir: None }
+            // } else if !bare_by_config(conformed_git_dir.as_ref())
+            //     .map_err(|err| crate::is_git::Error::Metadata {
+            //         source: err,
+            //         path: conformed_git_dir.join("config"),
+            //     })?
+            //     .ok_or(crate::is_git::Error::Inconclusive)?
+            // {
+            //     crate::repository::Kind::WorktreePossiblyInConfiguration
             } else {
                 crate::repository::Kind::PossiblyBare
             }
diff --git a/gix-discover/src/lib.rs b/gix-discover/src/lib.rs
@@ -39,6 +39,8 @@ pub mod is_git {
         Metadata { source: std::io::Error, path: PathBuf },
         #[error("The repository's config file doesn't exist or didn't have a 'bare' configuration or contained core.worktree without value")]
         Inconclusive,
+        #[error("Could not obtain current directory when conforming repository path")]
+        CurrentDir(#[from] std::io::Error),
     }
 }
 
diff --git a/gix-discover/src/repository.rs b/gix-discover/src/repository.rs
@@ -14,8 +14,10 @@ pub enum Path {
     /// The currently checked out or nascent work tree of a git repository
     WorkTree(PathBuf),
     /// The git repository itself, typically bare and without known worktree.
+    /// It could also be non-bare with a worktree configured using git configuration, or no worktree at all despite
+    /// not being bare (due to mis-configuration for example).
     ///
-    /// Note that it might still have linked work-trees which can be accessed later, weather bare or not, or it might be a
+    /// Note that it might still have linked work-trees which can be accessed later, bare or not, or it might be a
     /// submodule git directory in the `.git/modules/**/<name>` directory of the parent repository.
     Repository(PathBuf),
 }
@@ -112,8 +114,11 @@ pub enum Kind {
     /// Note that this is merely a guess at this point as we didn't read the configuration yet.
     ///
     /// Also note that due to optimizing for performance and *just* making an educated *guess in some situations*,
-    /// we may consider a non-bare repository bare if it it doesn't have an index yet due to be freshly initialized.
-    /// The caller is has to handle this, typically by reading the configuration.
+    /// we may consider a non-bare repository bare if it doesn't have an index yet due to be freshly initialized.
+    /// The caller has to handle this, typically by reading the configuration.
+    ///
+    /// It could also be a directory which is non-bare by configuration, but is *not* named `.git`.
+    /// Unusual, but it's possible that a worktree is configured via `core.worktree`.
     PossiblyBare,
     /// A `git` repository along with checked out files in a work tree.
     WorkTree {
diff --git a/gix-discover/src/upwards/mod.rs b/gix-discover/src/upwards/mod.rs
@@ -38,11 +38,11 @@ pub(crate) mod function {
         // Normalize the path so that `Path::parent()` _actually_ gives
         // us the parent directory. (`Path::parent` just strips off the last
         // path component, which means it will not do what you expect when
-        // working with paths paths that contain '..'.)
+        // working with paths that contain '..'.)
         let cwd = current_dir.map_or_else(
             || {
                 // The paths we return are relevant to the repository, but at this time it's impossible to know
-                // what `core.precomposeUnicode` is going to be. Hence the one using these paths will have to
+                // what `core.precomposeUnicode` is going to be. Hence, the one using these paths will have to
                 // transform the paths as needed, because we can't. `false` means to leave the obtained path as is.
                 gix_fs::current_dir(false).map(Cow::Owned)
             },
@@ -130,7 +130,7 @@ pub(crate) mod function {
                     cursor_metadata_backup = cursor_metadata.take();
                 }
                 if let Ok(kind) = match cursor_metadata.take() {
-                    Some(metadata) => is_git_with_metadata(&cursor, metadata),
+                    Some(metadata) => is_git_with_metadata(&cursor, metadata, &cwd),
                     None => is_git(&cursor),
                 } {
                     match filter_by_trust(&cursor)? {
diff --git a/gix-discover/tests/discover/is_git/mod.rs b/gix-discover/tests/discover/is_git/mod.rs
@@ -58,6 +58,30 @@ fn bare_repo_with_index_file_looks_still_looks_like_bare() -> crate::Result {
     Ok(())
 }
 
+#[test]
+fn non_bare_repo_without_workdir() -> crate::Result {
+    let repo = repo_path()?.join("non-bare-without-worktree");
+    let kind = gix_discover::is_git(&repo)?;
+    assert_eq!(
+        kind,
+        gix_discover::repository::Kind::PossiblyBare,
+        "typically due to misconfiguration, but worktrees could also be configured in Git configuration"
+    );
+    Ok(())
+}
+
+#[test]
+fn non_bare_repo_without_workdir_with_index() -> crate::Result {
+    let repo = repo_path()?.join("non-bare-without-worktree-with-index");
+    let kind = gix_discover::is_git(&repo)?;
+    assert_eq!(
+        kind,
+        gix_discover::repository::Kind::PossiblyBare,
+        "this means it has to be validated later"
+    );
+    Ok(())
+}
+
 #[test]
 fn bare_repo_with_index_file_looks_still_looks_like_bare_if_it_was_renamed() -> crate::Result {
     for repo_name in ["bare-with-index-bare", "bare-with-index-no-config-bare"] {
diff --git a/gix-discover/tests/discover/main.rs b/gix-discover/tests/discover/main.rs
diff --git a/gix-discover/tests/discover/parse/mod.rs b/gix-discover/tests/discover/parse/mod.rs
diff --git a/gix-discover/tests/discover/path/mod.rs b/gix-discover/tests/discover/path/mod.rs
diff --git a/gix-discover/tests/discover/upwards/ceiling_dirs.rs b/gix-discover/tests/discover/upwards/ceiling_dirs.rs
diff --git a/gix-discover/tests/discover/upwards/mod.rs b/gix-discover/tests/discover/upwards/mod.rs
diff --git a/gix-discover/tests/fixtures/make_basic_repo.sh b/gix-discover/tests/fixtures/make_basic_repo.sh
@@ -15,6 +15,17 @@ mkdir -p some/very/deeply/nested/subdir
 
 git clone --bare --shared . bare.git
 
+git clone --bare --shared . non-bare-without-worktree
+(cd non-bare-without-worktree
+  git config core.bare false
+)
+
+git clone --bare --shared . non-bare-without-worktree-with-index
+(cd non-bare-without-worktree
+  git config core.bare false
+  cp ../.git/index .
+)
+
 git worktree add worktrees/a
 git worktree add worktrees/b-private-dir-deleted
 rm -R .git/worktrees/b-private-dir-deleted
diff --git a/gix/src/object/tree/diff/change.rs b/gix/src/object/tree/diff/change.rs
@@ -1,8 +1,8 @@
+use super::ChangeDetached;
 use crate::bstr::{BStr, ByteSlice};
 use crate::ext::ObjectIdExt;
 use crate::object::tree::diff::Change;
 use crate::Repository;
-use gix_diff::tree_with_rewrites::Change as ChangeDetached;
 
 impl Change<'_, '_, '_> {
     /// Produce a platform for performing a line-diff no matter whether the underlying [Change] is an addition, modification,
diff --git a/gix/src/object/tree/diff/mod.rs b/gix/src/object/tree/diff/mod.rs
@@ -12,6 +12,8 @@ pub enum Action {
     Cancel,
 }
 
+pub use gix_diff::tree_with_rewrites::Change as ChangeDetached;
+
 /// Represents any possible change in order to turn one tree into another.
 #[derive(Debug, Clone, Copy)]
 pub enum Change<'a, 'old, 'new> {
diff --git a/gix/src/open/repository.rs b/gix/src/open/repository.rs
@@ -1,7 +1,7 @@
 #![allow(clippy::result_large_err)]
-use std::{borrow::Cow, path::PathBuf};
-
 use gix_features::threading::OwnShared;
+use std::ffi::OsStr;
+use std::{borrow::Cow, path::PathBuf};
 
 use super::{Error, Options};
 use crate::{
@@ -86,7 +86,7 @@ impl ThreadSafeRepository {
             }
         };
 
-        // The be altered later based on `core.precomposeUnicode`.
+        // To be altered later based on `core.precomposeUnicode`.
         let cwd = gix_fs::current_dir(false)?;
         let (git_dir, worktree_dir) = gix_discover::repository::Path::from_dot_git_dir(path, kind, &cwd)
             .expect("we have sanitized path with is_git()")
@@ -284,7 +284,7 @@ impl ThreadSafeRepository {
         }
 
         match worktree_dir {
-            None if !config.is_bare => {
+            None if !config.is_bare && refs.git_dir().extension() == Some(OsStr::new(gix_discover::DOT_GIT_DIR)) => {
                 worktree_dir = Some(git_dir.parent().expect("parent is always available").to_owned());
             }
             Some(_) => {
diff --git a/gix/tests/fixtures/generated-archives/make_basic_repo.tar b/gix/tests/fixtures/generated-archives/make_basic_repo.tar
diff --git a/gix/tests/fixtures/make_basic_repo.sh b/gix/tests/fixtures/make_basic_repo.sh
@@ -36,4 +36,9 @@ git init all-untracked
 git init empty-core-excludes
 (cd empty-core-excludes
   echo $'[core]\n\texcludesFile = ' >> .git/config
+)
+
+git clone --bare . non-bare-without-worktree
+(cd non-bare-without-worktree
+  git config core.bare false
 )
diff --git a/gix/tests/gix/repository/mod.rs b/gix/tests/gix/repository/mod.rs
@@ -42,6 +42,7 @@ mod dirwalk {
             ("bare.git".into(), Directory),
             ("empty-core-excludes".into(), Repository),
             ("non-bare-repo-without-index".into(), Repository),
+            ("non-bare-without-worktree".into(), Directory),
             ("some".into(), Directory),
         ];
         assert_eq!(
diff --git a/gix/tests/gix/repository/open.rs b/gix/tests/gix/repository/open.rs
@@ -62,6 +62,30 @@ fn bare_repo_with_index() -> crate::Result {
         repo.is_bare(),
         "it's properly classified as it reads the configuration (and has no worktree)"
     );
+    assert_eq!(repo.work_dir(), None);
+    Ok(())
+}
+
+#[test]
+fn non_bare_non_git_repo_without_worktree() -> crate::Result {
+    let repo = named_subrepo_opts(
+        "make_basic_repo.sh",
+        "non-bare-without-worktree",
+        gix::open::Options::isolated(),
+    )?;
+    assert!(!repo.is_bare());
+    assert_eq!(repo.work_dir(), None, "it doesn't assume that workdir exists");
+
+    let repo = gix::open_opts(
+        repo.git_dir().join("objects").join(".."),
+        gix::open::Options::isolated(),
+    )?;
+    assert!(!repo.is_bare());
+    assert_eq!(
+        repo.work_dir(),
+        None,
+        "it figures this out even if a non-normalized gitdir is used"
+    );
     Ok(())
 }
 

Original file line number	Diff line number	Diff line change
`@@ -39,6 +39,8 @@ pub mod is_git {`
`39`	`39`	`Metadata { source: std::io::Error, path: PathBuf },`
`40`	`40`	`#[error("The repository's config file doesn't exist or didn't have a 'bare' configuration or contained core.worktree without value")]`
`41`	`41`	`Inconclusive,`
	`42`	`+ #[error("Could not obtain current directory when conforming repository path")]`
	`43`	`+ CurrentDir(#[from] std::io::Error),`
`42`	`44`	`}`
`43`	`45`	`}`
`44`	`46`
Original file line number	Diff line number	Diff line change
`@@ -12,6 +12,8 @@ pub enum Action {`
`12`	`12`	`Cancel,`
`13`	`13`	`}`
`14`	`14`
	`15`	`+pub use gix_diff::tree_with_rewrites::Change as ChangeDetached;`
	`16`	`+`
`15`	`17`	`/// Represents any possible change in order to turn one tree into another.`
`16`	`18`	`#[derive(Debug, Clone, Copy)]`
`17`	`19`	`pub enum Change<'a, 'old, 'new> {`