Merge pull request GitoxideLabs#2382 from GitoxideLabs/improvements

improvements

Author: Byron

gix-dir/src/walk/classify.rs

@@ -388,20 +388,27 @@ fn resolve_file_type_with_index(
     // TODO(perf): multi-threaded hash-table so it's always used, even for case-sensitive lookups, just like Git does it.
     let (uptodate_kind, index_kind) = if let Some(accelerate) = ignore_case {
         match index.entry_by_path_icase(rela_path.as_bstr(), true, accelerate) {
-            None => {
-                icase_directory_to_kinds(index.entry_closest_to_directory_icase(rela_path.as_bstr(), true, accelerate))
-            }
+            None => icase_directory_to_kinds(index.entry_closest_to_directory_or_directory_icase(
+                rela_path.as_bstr(),
+                true,
+                accelerate,
+            )),
             Some(entry) => {
-                let icase_dir = index.entry_closest_to_directory_icase(rela_path.as_bstr(), true, accelerate);
-                let directory_matches_exactly = icase_dir.is_some_and(|dir| {
-                    let path = dir.path(index);
-                    let slash_idx = path.rfind_byte(b'/').expect("dir");
-                    path[..slash_idx].as_bstr() == rela_path
-                });
-                if directory_matches_exactly {
-                    icase_directory_to_kinds(icase_dir)
-                } else {
+                if entry.mode.is_submodule() || entry.mode.is_sparse() {
                     entry_to_kinds(entry)
+                } else {
+                    let icase_dir =
+                        index.entry_closest_to_directory_or_directory_icase(rela_path.as_bstr(), true, accelerate);
+                    let directory_matches_exactly = icase_dir.is_some_and(|dir| {
+                        let path = dir.path(index);
+                        let slash_idx = path.rfind_byte(b'/').expect("dir");
+                        path[..slash_idx].as_bstr() == rela_path
+                    });
+                    if directory_matches_exactly {
+                        icase_directory_to_kinds(icase_dir)
+                    } else {
+                        entry_to_kinds(entry)
+                    }
                 }
             }
         }

gix-index/src/access/mod.rs

@@ -149,6 +149,16 @@ impl State {
             let hash = AccelerateLookup::icase_hash(entry_path);
             out.icase_entries
                 .insert_unique(hash, entry, |e| AccelerateLookup::icase_hash(e.path(self)));
+            if entry_is_dir(entry) {
+                out.icase_dirs.insert_unique(
+                    hash,
+                    crate::DirEntry {
+                        entry,
+                        dir_end: entry.path.end,
+                    },
+                    |dir| AccelerateLookup::icase_hash(dir.path(self)),
+                );
+            }
 
             let mut last_pos = entry_path.len();
             while let Some(slash_idx) = entry_path[..last_pos].rfind_byte(b'/') {
@@ -206,14 +216,11 @@ impl State {
             .copied()
     }
 
-    /// Return the entry (at any stage) that is inside of `directory`, or `None`,
-    /// using `lookup` for acceleration.
-    /// Note that submodules are not detected as directories and the user should
-    /// make another call to [`entry_by_path_icase()`](Self::entry_by_path_icase) to check for this
-    /// possibility. Doing so might also reveal a sparse directory.
+    /// Return the entry (at any stage) that is inside `directory`, or `None`,
+    /// or a directory itself like a submodule or sparse directory, using `lookup` for acceleration.
     ///
     /// If `ignore_case` is set, a case-insensitive (ASCII-folding only) search will be performed.
-    pub fn entry_closest_to_directory_icase<'a>(
+    pub fn entry_closest_to_directory_or_directory_icase<'a>(
         &'a self,
         directory: &BStr,
         ignore_case: bool,
@@ -234,14 +241,18 @@ impl State {
             .map(|dir| dir.entry)
     }
 
-    /// Return the entry (at any stage) that is inside of `directory`, or `None`.
-    /// Note that submodules are not detected as directories and the user should
-    /// make another call to [`entry_by_path_icase()`](Self::entry_by_path_icase) to check for this
-    /// possibility. Doing so might also reveal a sparse directory.
+    /// Return the entry (at any stage) that is inside `directory`, or `None`,
+    /// or that is a directory itself like a submodule or sparse directory.
     ///
-    /// Note that this is a case-sensitive search.
-    pub fn entry_closest_to_directory(&self, directory: &BStr) -> Option<&Entry> {
-        let idx = self.entry_index_by_path(directory).err()?;
+    /// Note that this is a *case-sensitive* search.
+    pub fn entry_closest_to_directory_or_directory(&self, directory: &BStr) -> Option<&Entry> {
+        let idx = match self.entry_index_by_path(directory) {
+            Ok(idx) => {
+                let entry = &self.entries[idx];
+                return entry_is_dir(entry).then_some(entry);
+            }
+            Err(closest_idx) => closest_idx,
+        };
         for entry in &self.entries[idx..] {
             let path = entry.path(self);
             if path.get(..directory.len())? != directory {
@@ -259,7 +270,7 @@ impl State {
         None
     }
 
-    /// Check if `path` is a directory that contains entries in the index.
+    /// Check if `path` is a directory that contains entries in the index, or is a submodule.
     ///
     /// Returns `true` if there is at least one entry in the index whose path starts with `path/`,
     /// indicating that `path` is a directory containing indexed files.
@@ -269,10 +280,11 @@ impl State {
     ///
     /// Note that this is a case-sensitive search.
     pub fn path_is_directory(&self, path: &BStr) -> bool {
-        self.entry_closest_to_directory(path).is_some()
+        self.entry_closest_to_directory_or_directory(path).is_some()
     }
 
-    /// Check if `path` is a directory that contains entries in the index, with optional case-insensitive matching.
+    /// Check if `path` is a directory that contains entries in the index or is a submodule,
+    /// with optional case-insensitive matching.
     ///
     /// Returns `true` if there is at least one entry in the index whose path starts with `path/`,
     /// indicating that `path` is a directory containing indexed files.
@@ -288,7 +300,7 @@ impl State {
         ignore_case: bool,
         lookup: &AccelerateLookup<'a>,
     ) -> bool {
-        self.entry_closest_to_directory_icase(path, ignore_case, lookup)
+        self.entry_closest_to_directory_or_directory_icase(path, ignore_case, lookup)
             .is_some()
     }
 
@@ -623,6 +635,10 @@ impl State {
     }
 }
 
+fn entry_is_dir(entry: &Entry) -> bool {
+    entry.mode.is_sparse() || entry.mode.is_submodule()
+}
+
 #[cfg(test)]
 mod tests {
     use std::path::{Path, PathBuf};

gix-index/tests/fixtures/generated-archives/v2_all_file_kinds.tar

@@ -19,6 +19,8 @@ ln -s a c
 mkdir d
 (cd d && touch a b c)
 
+git submodule add ./sub sub-worktree
+
 git add .
 git update-index --chmod=+x b  # For Windows.
 git commit -m "init"

…fixtures/generated-archives/V2_empty.tar …fixtures/generated-archives/v2_empty.targix-index/tests/fixtures/generated-archives/V2_empty.tar renamed to gix-index/tests/fixtures/generated-archives/v2_empty.tar gix-index/tests/fixtures/generated-archives/V2_empty.tar renamed to gix-index/tests/fixtures/generated-archives/v2_empty.tar

@@ -48,7 +48,7 @@ fn dirwalk_api_and_icase_support() {
             last_pos += 1;
 
             let entry = file
-                .entry_closest_to_directory(dir)
+                .entry_closest_to_directory_or_directory(dir)
                 .unwrap_or_else(|| panic!("didn't find {dir}"));
             assert!(
                 entry.path(&file).starts_with(dir),
@@ -57,13 +57,75 @@ fn dirwalk_api_and_icase_support() {
 
             let dir_upper: BString = dir.to_ascii_uppercase().into();
             let other_entry = file
-                .entry_closest_to_directory_icase(dir_upper.as_bstr(), true, &icase)
+                .entry_closest_to_directory_or_directory_icase(dir_upper.as_bstr(), true, &icase)
                 .unwrap_or_else(|| panic!("didn't find upper-cased {dir_upper}"));
             assert_eq!(other_entry, entry, "the first entry is always the same, no matter what kind of search is conducted (as there are no clashes/ambiguities here)");
         }
     }
 }
 
+#[test]
+fn entry_closest_to_directory_or_directory_with_submodule() {
+    let file = Fixture::Generated("v2_all_file_kinds").open();
+
+    assert!(
+        file.entry_closest_to_directory_or_directory("d".into()).is_some(),
+        "this is a directory"
+    );
+    assert!(
+        file.entry_closest_to_directory_or_directory("sub".into()).is_some(),
+        "this is a checked in repository, a directory itself"
+    );
+    assert!(
+        file.entry_closest_to_directory_or_directory("sub-worktree".into())
+            .is_some(),
+        "a submodule that is officially registered, absolutely the same as 'sub' in the index."
+    );
+    assert!(
+        file.entry_closest_to_directory_or_directory("a".into()).is_none(),
+        "'a' is a file, and we ask for a directory"
+    );
+}
+
+#[test]
+fn entry_closest_to_directory_or_directory_icase_with_submodule() {
+    let file = Fixture::Generated("v2_all_file_kinds").open();
+    let icase = file.prepare_icase_backing();
+
+    assert!(
+        file.entry_closest_to_directory_or_directory_icase("D".into(), true, &icase)
+            .is_some(),
+        "this is a directory"
+    );
+    assert!(file
+        .entry_closest_to_directory_or_directory_icase("D".into(), false, &icase)
+        .is_none());
+
+    assert!(
+        file.entry_closest_to_directory_or_directory_icase("SuB".into(), true, &icase)
+            .is_some(),
+        "this is a checked in repository, a directory itself"
+    );
+    assert!(file
+        .entry_closest_to_directory_or_directory_icase("SuB".into(), false, &icase)
+        .is_none());
+
+    assert!(
+        file.entry_closest_to_directory_or_directory_icase("SUB-worktree".into(), true, &icase)
+            .is_some(),
+        "a submodule that is officially registered, absolutely the same as 'sub' in the index."
+    );
+    assert!(file
+        .entry_closest_to_directory_or_directory_icase("SUB-worktree".into(), false, &icase)
+        .is_none());
+
+    assert!(
+        file.entry_closest_to_directory_or_directory_icase("A".into(), true, &icase)
+            .is_none(),
+        "'a' is a file, and we ask for a directory"
+    );
+}
+
 #[test]
 fn ignorecase_clashes_and_order() {
     let file = icase_fixture();
@@ -85,7 +147,7 @@ fn ignorecase_clashes_and_order() {
             last_pos += 1;
 
             let entry = file
-                .entry_closest_to_directory(dir)
+                .entry_closest_to_directory_or_directory(dir)
                 .unwrap_or_else(|| panic!("didn't find {dir}"));
             assert!(
                 entry.path(&file).starts_with(dir),
@@ -110,16 +172,16 @@ fn ignorecase_clashes_and_order() {
     );
 
     assert!(
-        file.entry_closest_to_directory("d".into()).is_none(),
+        file.entry_closest_to_directory_or_directory("d".into()).is_none(),
         "this is a file, and this directory search isn't case-sensitive"
     );
-    let entry = file.entry_closest_to_directory("D".into());
+    let entry = file.entry_closest_to_directory_or_directory("D".into());
     assert_eq!(
         entry.map(|e| e.path(&file)).expect("present"),
         "D/B",
         "this is a directory, indeed, we find the first file in it"
     );
-    let entry_icase = file.entry_closest_to_directory_icase("d".into(), true, &icase);
+    let entry_icase = file.entry_closest_to_directory_or_directory_icase("d".into(), true, &icase);
     assert_eq!(
         entry_icase, entry,
         "case-insensitive searches don't confuse directories and files, so `d` finds `D`, the directory."
@@ -325,6 +387,26 @@ fn check_prefix(index: &gix_index::State, prefix: &str, expected: &[&str]) {
     );
 }
 
+#[test]
+fn path_is_directory_with_submodule() {
+    let file = Fixture::Generated("v2_all_file_kinds").open();
+
+    assert!(file.path_is_directory("sub-worktree".into()), "a submodule worktree");
+    assert!(file.path_is_directory("d".into()), "a single-letter directory");
+    assert!(
+        file.path_is_directory("sub".into()),
+        "this is the parent repository, and it was added as well"
+    );
+    assert!(
+        !file.path_is_directory("su".into()),
+        "just a sub-string of the directory which doesn't match"
+    );
+    assert!(
+        !file.path_is_directory("a".into()),
+        "a one-letter file isn't a directory"
+    );
+}
+
 #[test]
 fn path_is_directory() {
     let file = Fixture::Loose("ignore-case-realistic").open();
@@ -408,6 +490,43 @@ fn path_is_directory_icase() {
     );
 }
 
+#[test]
+fn path_is_directory_icase_with_submodule() {
+    let file = Fixture::Generated("v2_all_file_kinds").open();
+    let icase = file.prepare_icase_backing();
+
+    assert!(
+        file.path_is_directory_icase("SUB-worktree".into(), true, &icase),
+        "a submodule worktree"
+    );
+    assert!(!file.path_is_directory_icase("SUB-worktree".into(), false, &icase));
+
+    assert!(
+        file.path_is_directory_icase("D".into(), true, &icase),
+        "a single-letter directory"
+    );
+    assert!(!file.path_is_directory_icase("D".into(), false, &icase));
+
+    assert!(
+        file.path_is_directory_icase("SuB".into(), true, &icase),
+        "this is the parent repository, and it was added as well"
+    );
+    assert!(!file.path_is_directory_icase("SuB".into(), false, &icase));
+
+    assert!(
+        !file.path_is_directory_icase("Su".into(), true, &icase),
+        "just a sub-string of the directory which doesn't match"
+    );
+    assert!(
+        !file.path_is_directory_icase("A".into(), true, &icase),
+        "a one-letter file isn't a directory"
+    );
+    assert!(
+        !file.path_is_directory_icase("a".into(), true, &icase),
+        "a one-letter file isn't a directory, even with correct case"
+    );
+}
+
 #[test]
 fn path_is_directory_icase_with_clashes() {
     let file = icase_fixture();

gix-index/tests/fixtures/make_index/v2_all_file_kinds.sh

@@ -37,7 +37,7 @@ mod from_state {
         let fixtures = [
             (Loose("extended-flags"), V3),
             (Generated("v2"), V2),
-            (Generated("V2_empty"), V2),
+            (Generated("v2_empty"), V2),
             (Generated("v2_more_files"), V2),
             (Generated("v2_all_file_kinds"), V2),
             (Generated("v4_more_files_IEOT"), V2),

…ex/tests/fixtures/make_index/V2_empty.sh …ex/tests/fixtures/make_index/v2_empty.shgix-index/tests/fixtures/make_index/V2_empty.sh renamed to gix-index/tests/fixtures/make_index/v2_empty.sh gix-index/tests/fixtures/make_index/V2_empty.sh renamed to gix-index/tests/fixtures/make_index/v2_empty.sh

@@ -66,7 +66,7 @@ fn v2_with_single_entry_tree_and_eoie_ext() {
 }
 #[test]
 fn v2_empty() {
-    let file = file("V2_empty");
+    let file = file("v2_empty");
     assert_eq!(file.version(), Version::V2);
     assert_eq!(file.entries().len(), 0);
     let tree = file.tree().unwrap();

gix-index/tests/index/access.rs

@@ -17,7 +17,7 @@ fn roundtrips() -> crate::Result {
                 end_of_index_entry: true,
             }),
         ),
-        (Generated("V2_empty"), only_tree_ext()),
+        (Generated("v2_empty"), only_tree_ext()),
         (Generated("v2_more_files"), only_tree_ext()),
         (Generated("v2_all_file_kinds"), only_tree_ext()),
     ];
@@ -128,7 +128,7 @@ fn state_comparisons_with_various_extension_configurations() {
         Loose("REUC"),
         Loose("UNTR-with-oids"),
         Loose("UNTR"),
-        Generated("V2_empty"),
+        Generated("v2_empty"),
         Generated("v2"),
         Generated("v2_more_files"),
         Generated("v2_all_file_kinds"),