address review comments

Byron · EliahKagan · Byron · commit fcc3b69867db · 2024-05-20T21:50:23.000+02:00
- assure `con` is checked for, and that it's not overzealous. - reduce code duplication - improve documentation about more obscure parts of the code, based on the description in [this commit](git/git@e7cb0b4) - upper-case device names in comparisons as this is their canonical form, which also is more recognizable for people who are looking for them. - make clear why there is asymmetry between COM and LPT numbers. - Don't make a partial control-character check, but a complete one (i.e. *b < 32|0x20) - Add more variants for stream type tests (as regression protection, the code doesn't really care) - various clarifications in path-related tests on Windows Co-authored-by: Eliah Kagan <degeneracypressure@gmail.com>
diff --git a/gix-dir/src/walk/classify.rs b/gix-dir/src/walk/classify.rs
@@ -163,13 +163,7 @@ pub fn path(
                     stack
                         .at_entry(
                             rela_path.as_bstr(),
-                            disk_kind.map(|ft| {
-                                if ft.is_dir() {
-                                    gix_index::entry::Mode::DIR
-                                } else {
-                                    gix_index::entry::Mode::FILE
-                                }
-                            }),
+                            disk_kind.map(|ft| is_dir_to_mode(ft.is_dir())),
                             ctx.objects,
                         )
                         .map(|platform| platform.excluded_kind())
diff --git a/gix-fs/tests/stack/mod.rs b/gix-fs/tests/stack/mod.rs
@@ -37,12 +37,12 @@ fn path_join_handling() {
     let absolute = p("/absolute");
     assert!(
         absolute.is_relative(),
-        "on Windows, absolute linux paths are considered relative"
+        "on Windows, absolute Linux paths are considered relative (and relative to the current drive)"
     );
     let bs_absolute = p("\\absolute");
     assert!(
         absolute.is_relative(),
-        "on Windows, strange single-backslash paths are relative"
+        "on Windows, strange single-backslash paths are relative (and relative to the current drive)"
     );
     assert_eq!(
         p("relative").join(absolute),
@@ -58,7 +58,7 @@ fn path_join_handling() {
     assert_eq!(
         p("c:").join("relative"),
         p("c:relative"),
-        "absolute + relative = strange joined result with missing slash - but that shouldn't usually happen"
+        "absolute + relative = strange joined result with missing back-slash, but it's a valid path that works just like `c:\relative`"
     );
     assert_eq!(
         p("c:\\").join("relative"),
@@ -69,34 +69,52 @@ fn path_join_handling() {
     assert_eq!(
         p("\\\\?\\base").join(absolute),
         p("\\\\?\\base\\absolute"),
-        "absolute1 + absolute2 = joined result with backslash"
+        "absolute1 + unix-absolute2 = joined result with backslash"
+    );
+    assert_eq!(
+        p("\\\\.\\base").join(absolute),
+        p("\\\\.\\base\\absolute"),
+        "absolute1 + absolute2 = joined result with backslash (device relative)"
     );
     assert_eq!(
         p("\\\\?\\base").join(bs_absolute),
         p("\\\\?\\base\\absolute"),
         "absolute1 + absolute2 = joined result"
     );
+    assert_eq!(
+        p("\\\\.\\base").join(bs_absolute),
+        p("\\\\.\\base\\absolute"),
+        "absolute1 + absolute2 = joined result (device relative)"
+    );
 
+    assert_eq!(p("/").join("C:"), p("C:"), "unix-absolute + win-drive = win-drive");
     assert_eq!(
-        p("/").join("C:"),
+        p("d:/").join("C:"),
         p("C:"),
-        "unix-absolute + win-absolute = win-absolute"
+        "d-drive + c-drive = c-drive - interesting, as C: is supposed to be relative"
     );
     assert_eq!(
-        p("/").join("C:/"),
+        p("d:\\").join("C:\\"),
         p("C:\\"),
-        "unix-absolute + win-absolute = win-result, strangely enough it changed the trailing slash to backslash, so better not have trailing slashes"
+        "d-drive-with-bs + c-drive-with-bs = c-drive-with-bs - nothing special happens with backslashes"
     );
     assert_eq!(
-        p("/").join("C:\\"),
+        p("c:\\").join("\\\\.\\"),
+        p("\\\\.\\"),
+        "d-drive-with-bs + device-relative-unc = device-relative-unc"
+    );
+    assert_eq!(
+        p("/").join("C:/"),
         p("C:\\"),
-        "unix-absolute + win-absolute = win-result"
+        "unix-absolute + win-drive = win-drive, strangely enough it changed the trailing slash to backslash, so better not have trailing slashes"
     );
+    assert_eq!(p("/").join("C:\\"), p("C:\\"), "unix-absolute + win-drive = win-drive");
     assert_eq!(
-        p("relative").join("C:"),
+        p("\\\\.").join("C:"),
         p("C:"),
-        "relative + win-absolute = win-result"
+        "device-relative-unc + win-drive-relative = win-drive-relative - c: was supposed to be relative, but it's not acting like it."
     );
+    assert_eq!(p("relative").join("C:"), p("C:"), "relative + win-drive = win-drive");
 
     assert_eq!(
         p("/").join("\\\\localhost"),
@@ -202,6 +220,17 @@ fn relative_components_are_invalid() {
         if cfg!(windows) { ".\\a\\b" } else { "./a/b" },
         "dot is silently ignored"
     );
+    s.make_relative_path_current("a//b/".as_ref(), &mut r)
+        .expect("multiple-slashes are ignored");
+    assert_eq!(
+        r,
+        Record {
+            push_dir: 2,
+            dirs: vec![".".into(), "./a".into()],
+            push: 2,
+        },
+        "nothing changed"
+    );
 }
 
 #[test]
@@ -226,7 +255,7 @@ fn absolute_paths_are_invalid() -> crate::Result {
     assert_eq!(
         s.current(),
         p("./b\\"),
-        "trailing back-slashes are fine both on Windows and unix - on Unix it's part fo the filename"
+        "trailing backslashes are fine both on Windows and Unix - on Unix it's part fo the filename"
     );
 
     #[cfg(windows)]
@@ -235,22 +264,28 @@ fn absolute_paths_are_invalid() -> crate::Result {
         assert_eq!(
             err.to_string(),
             "Input path \"\\\" contains relative or absolute components",
-            "on windows, backslashes are considered absolute and replace the base if it is relative, \
+            "on Windows, backslashes are considered absolute and replace the base if it is relative, \
             hence they are forbidden."
         );
 
         let err = s.make_relative_path_current("c:".as_ref(), &mut r).unwrap_err();
         assert_eq!(
             err.to_string(),
             "Input path \"c:\" contains relative or absolute components",
-            "on windows, drive-letters are also absolute"
+            "on Windows, drive-letters without trailing backslash or slash are also absolute (even though they ought to be relative)"
+        );
+        let err = s.make_relative_path_current("c:\\".as_ref(), &mut r).unwrap_err();
+        assert_eq!(
+            err.to_string(),
+            "Input path \"c:\\\" contains relative or absolute components",
+            "on Windows, drive-letters are absolute, which is expected"
         );
 
         s.make_relative_path_current("֍:".as_ref(), &mut r)?;
         assert_eq!(
             s.current().to_string_lossy(),
             ".\\֍:",
-            "on windows, any unicode character will do as virtual drive-letter actually with `subst`, \
+            "on Windows, almost any unicode character will do as virtual drive-letter actually with `subst`, \
             but we just turn it into a presumably invalid path which is fine, i.e. we get a joined path"
         );
         let err = s
@@ -440,7 +475,7 @@ fn delegate_calls_are_consistent() -> crate::Result {
                 dirs: dirs.clone(),
                 push: 19,
             },
-            "a backslash is a normal character outside of windows, so it's fine to have it as component"
+            "a backslash is a normal character outside of Windows, so it's fine to have it as component"
         );
 
         s.make_relative_path_current("\\".as_ref(), &mut r)?;
diff --git a/gix-status/src/index_as_worktree/function.rs b/gix-status/src/index_as_worktree/function.rs
@@ -19,7 +19,7 @@ use crate::{
         types::{Error, Options},
         Change, Conflict, EntryStatus, Outcome, VisitEntry,
     },
-    SymlinkCheck,
+    is_dir_to_mode, SymlinkCheck,
 };
 
 /// Calculates the changes that need to be applied to an `index` to match the state of the `worktree` and makes them
@@ -276,15 +276,7 @@ impl<'index> State<'_, 'index> {
                 &mut |relative_path, case, is_dir, out| {
                     self.attr_stack
                         .set_case(case)
-                        .at_entry(
-                            relative_path,
-                            Some(if is_dir {
-                                gix_index::entry::Mode::DIR
-                            } else {
-                                gix_index::entry::Mode::FILE
-                            }),
-                            objects,
-                        )
+                        .at_entry(relative_path, Some(is_dir_to_mode(is_dir)), objects)
                         .map_or(false, |platform| platform.matching_attributes(out))
                 },
             )
diff --git a/gix-status/src/index_as_worktree_with_renames/mod.rs b/gix-status/src/index_as_worktree_with_renames/mod.rs
@@ -9,6 +9,7 @@ pub(super) mod function {
     use crate::index_as_worktree::traits::{CompareBlobs, SubmoduleStatus};
     use crate::index_as_worktree_with_renames::function::rewrite::ModificationOrDirwalkEntry;
     use crate::index_as_worktree_with_renames::{Context, Entry, Error, Options, Outcome, RewriteSource, VisitEntry};
+    use crate::is_dir_to_mode;
     use bstr::ByteSlice;
     use gix_worktree::stack::State;
     use std::borrow::Cow;
@@ -99,15 +100,7 @@ pub(super) mod function {
                                                 .expect("can only be called if attributes are used in patterns");
                                             stack
                                                 .set_case(case)
-                                                .at_entry(
-                                                    relative_path,
-                                                    Some(if is_dir {
-                                                        gix_index::entry::Mode::DIR
-                                                    } else {
-                                                        gix_index::entry::Mode::FILE
-                                                    }),
-                                                    &objects,
-                                                )
+                                                .at_entry(relative_path, Some(is_dir_to_mode(is_dir)), &objects)
                                                 .map_or(false, |platform| platform.matching_attributes(out))
                                         },
                                         excludes: excludes.as_mut(),
diff --git a/gix-status/src/lib.rs b/gix-status/src/lib.rs
@@ -32,3 +32,11 @@ pub struct SymlinkCheck {
 }
 
 mod stack;
+
+fn is_dir_to_mode(is_dir: bool) -> gix_index::entry::Mode {
+    if is_dir {
+        gix_index::entry::Mode::DIR
+    } else {
+        gix_index::entry::Mode::FILE
+    }
+}
diff --git a/gix-validate/src/path.rs b/gix-validate/src/path.rs
@@ -126,35 +126,41 @@ pub fn component(
 
 fn check_win_devices_and_illegal_characters(input: &BStr) -> Option<component::Error> {
     let in3 = input.get(..3)?;
-    if in3.eq_ignore_ascii_case(b"aux") && is_done_windows(input.get(3..)) {
+    if in3.eq_ignore_ascii_case(b"AUX") && is_done_windows(input.get(3..)) {
         return Some(component::Error::WindowsReservedName);
     }
-    if in3.eq_ignore_ascii_case(b"nul") && is_done_windows(input.get(3..)) {
+    if in3.eq_ignore_ascii_case(b"NUL") && is_done_windows(input.get(3..)) {
         return Some(component::Error::WindowsReservedName);
     }
-    if in3.eq_ignore_ascii_case(b"prn") && is_done_windows(input.get(3..)) {
+    if in3.eq_ignore_ascii_case(b"PRN") && is_done_windows(input.get(3..)) {
         return Some(component::Error::WindowsReservedName);
     }
-    if in3.eq_ignore_ascii_case(b"com")
+    // Note that the following allows `COM0`, even though `LPT0` is not allowed.
+    // Even though tests seem to indicate that neither `LPT0` nor `COM0` are valid
+    // device names, it's unclear this truly is the case in all possible versions and editions
+    // of Windows.
+    // Hence, justification for this asymmetry is merely to do exactly the same as Git does,
+    // and to have exactly the same behaviour during validation (for worktree-writes).
+    if in3.eq_ignore_ascii_case(b"COM")
         && input.get(3).map_or(false, |n| *n >= b'1' && *n <= b'9')
         && is_done_windows(input.get(4..))
     {
         return Some(component::Error::WindowsReservedName);
     }
-    if in3.eq_ignore_ascii_case(b"lpt")
+    if in3.eq_ignore_ascii_case(b"LPT")
         && input.get(3).map_or(false, u8::is_ascii_digit)
         && is_done_windows(input.get(4..))
     {
         return Some(component::Error::WindowsReservedName);
     }
-    if in3.eq_ignore_ascii_case(b"con")
+    if in3.eq_ignore_ascii_case(b"CON")
         && (is_done_windows(input.get(3..))
-            || (input.get(3..6).map_or(false, |n| n.eq_ignore_ascii_case(b"in$")) && is_done_windows(input.get(6..)))
-            || (input.get(3..7).map_or(false, |n| n.eq_ignore_ascii_case(b"out$")) && is_done_windows(input.get(7..))))
+            || (input.get(3..6).map_or(false, |n| n.eq_ignore_ascii_case(b"IN$")) && is_done_windows(input.get(6..)))
+            || (input.get(3..7).map_or(false, |n| n.eq_ignore_ascii_case(b"OUT$")) && is_done_windows(input.get(7..))))
     {
         return Some(component::Error::WindowsReservedName);
     }
-    if input.iter().any(|b| *b < 0x20 || b":<>\"|?*".contains(b)) {
+    if input.iter().any(|b| b.is_ascii_control() || b":<>\"|?*".contains(b)) {
         return Some(component::Error::WindowsIllegalCharacter);
     }
     if input.ends_with(b".") || input.ends_with(b" ") {
@@ -225,6 +231,10 @@ fn is_dot_git_ntfs(input: &BStr) -> bool {
     false
 }
 
+/// The `search_case_insensitive` name is the actual name to look for (in a case-insensitive way).
+/// Opposed to that there is the special `ntfs_shortname_prefix` which is derived from `search_case_insensitive`
+/// but looks more like a hash, one that NTFS uses to disambiguate things, for when there is a lot of files
+/// with the same prefix.
 fn is_dot_ntfs(input: &BStr, search_case_insensitive: &str, ntfs_shortname_prefix: &str) -> bool {
     if input.first() == Some(&b'.') {
         let end_pos = 1 + search_case_insensitive.len();
@@ -243,6 +253,8 @@ fn is_dot_ntfs(input: &BStr, search_case_insensitive: &str, ntfs_shortname_prefi
             .map_or(false, |(ntfs_prefix, first_6_of_input)| {
                 first_6_of_input.eq_ignore_ascii_case(ntfs_prefix)
                     && input.get(6) == Some(&b'~')
+                    // It's notable that only `~1` to `~4` are possible before the disambiguation algorithm
+                    // switches to using the `ntfs_shortname_prefix`, which is checked hereafter.
                     && input.get(7).map_or(false, |num| (b'1'..=b'4').contains(num))
             })
         {
diff --git a/gix-validate/tests/path/mod.rs b/gix-validate/tests/path/mod.rs
@@ -65,6 +65,8 @@ mod component {
         mktest!(conout_without_dollar_with_extension, b"conout.c");
         mktest!(conin_without_dollar_with_extension, b"conin.c");
         mktest!(conin_without_dollar, b"conin");
+        mktest!(not_con, b"com");
+        mktest!(also_not_con, b"co");
         mktest!(not_nul, b"null");
         mktest!(
             not_dot_gitmodules_shorter_hfs,
@@ -115,7 +117,7 @@ mod component {
         mktest!(empty, b"", Error::Empty);
         mktest!(dot_git_lower, b".git", Error::DotGitDir, NO_OPTS);
         mktest!(dot_git_lower_hfs, ".g\u{200c}it".as_bytes(), Error::DotGitDir);
-        mktest!(dot_git_lower_hfs_simple, ".Git".as_bytes(), Error::DotGitDir);
+        mktest!(dot_git_mixed_hfs_simple, b".Git", Error::DotGitDir);
         mktest!(dot_git_upper, b".GIT", Error::DotGitDir, NO_OPTS);
         mktest!(dot_git_upper_hfs, ".GIT\u{200e}".as_bytes(), Error::DotGitDir);
         mktest!(dot_git_upper_ntfs_8_3, b"GIT~1", Error::DotGitDir);
@@ -164,6 +166,30 @@ mod component {
             Symlink,
             ALL_OPTS
         );
+        mktest!(
+            dot_gitmodules_lower_ntfs_stream_default_implicit,
+            b".gitmodules::$DATA",
+            Error::SymlinkedGitModules,
+            Symlink,
+            ALL_OPTS
+        );
+        mktest!(
+            ntfs_stream_default_implicit,
+            b"file::$DATA",
+            Error::WindowsIllegalCharacter
+        );
+        mktest!(
+            ntfs_stream_default_explicit,
+            b"file:$ANYTHING_REALLY:$DATA",
+            Error::WindowsIllegalCharacter
+        );
+        mktest!(
+            dot_gitmodules_lower_ntfs_stream_default_explicit,
+            b".gitmodules:$DATA:$DATA",
+            Error::SymlinkedGitModules,
+            Symlink,
+            ALL_OPTS
+        );
         mktest!(
             not_gitmodules_trailing_space,
             b".gitmodules x ",
@@ -201,6 +227,7 @@ mod component {
         mktest!(nul_mixed, b"NuL", Error::WindowsReservedName);
         mktest!(prn_mixed_with_extension, b"PrN.abc", Error::WindowsReservedName);
         mktest!(con, b"CON", Error::WindowsReservedName);
+        mktest!(con_with_extension, b"CON.abc", Error::WindowsReservedName);
         mktest!(
             conout_mixed_with_extension,
             b"ConOut$  .xyz",
diff --git a/gix/src/lib.rs b/gix/src/lib.rs
@@ -345,3 +345,12 @@ pub mod shallow;
 pub mod discover;
 
 pub mod env;
+
+#[cfg(feature = "index")]
+fn is_dir_to_mode(is_dir: bool) -> gix_index::entry::Mode {
+    if is_dir {
+        gix_index::entry::Mode::DIR
+    } else {
+        gix_index::entry::Mode::FILE
+    }
+}
diff --git a/gix/src/pathspec.rs b/gix/src/pathspec.rs
diff --git a/gix/src/repository/dirwalk.rs b/gix/src/repository/dirwalk.rs
diff --git a/gix/src/submodule/mod.rs b/gix/src/submodule/mod.rs
