Unify FileId structs

We had two of these! Just use one.

src/tests/env_universal_common.rs

@@ -8,8 +8,7 @@ use crate::reader::{reader_pop, reader_push, ReaderConfig};
 use crate::tests::prelude::*;
 use crate::threads::{iothread_drain_all, iothread_perform};
 use crate::wchar::prelude::*;
-use crate::wutil::file_id_for_path;
-use crate::wutil::INVALID_FILE_ID;
+use crate::wutil::{file_id_for_path, INVALID_FILE_ID};
 
 const UVARS_PER_THREAD: usize = 8;
 const UVARS_TEST_PATH: &wstr = L!("test/fish_uvars_test/varsfile.txt");

src/wildcard.rs

@@ -741,7 +741,7 @@ mod expander {
                 };
 
                 let file_id = FileId::from_stat(&statbuf);
-                if !self.visited_files.insert(file_id.clone()) {
+                if !self.visited_files.insert(file_id) {
                     // Symlink loop! This directory was already visited, so skip it.
                     continue;
                 }

src/wutil/fileid.rs

@@ -1,13 +1,15 @@
-use crate::wutil::{wstat, wstr};
-use std::cmp::Ordering;
-use std::fs::{File, Metadata};
+use crate::common::wcs2zstring;
+use crate::wutil::wstr;
+use std::ffi::{CStr, OsStr};
+use std::fs::{self, File, Metadata};
+use std::os::fd::AsRawFd;
 use std::os::unix::prelude::*;
 
 /// Struct for representing a file's inode. We use this to detect and avoid symlink loops, among
 /// other things. While an inode / dev pair is sufficient to distinguish co-existing files, Linux  
 /// seems to aggressively re-use inodes, so it cannot determine if a file has been deleted (ABA  
 /// problem). Therefore we include richer information.
-#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)]
+#[derive(Debug, Copy, Clone, Hash, PartialEq, Eq, PartialOrd, Ord)]
 pub struct FileId {
     pub device: u64,
     pub inode: u64,
@@ -19,7 +21,7 @@ pub struct FileId {
 }
 
 impl FileId {
-    pub fn from_stat(buf: Metadata) -> Self {
+    pub fn from_md(buf: &Metadata) -> Self {
         // These "into()" calls are because the various fields have different types
         // on different platforms.
         #[allow(clippy::useless_conversion)]
@@ -34,13 +36,44 @@ impl FileId {
         }
     }
 
-    pub fn older_than(&self, rhs: &FileId) -> bool {
-        match (self.change_seconds, self.change_nanoseconds)
-            .cmp(&(rhs.change_seconds, rhs.change_nanoseconds))
+    #[allow(clippy::unnecessary_cast)] // platform-dependent
+    pub fn from_stat(buf: &libc::stat) -> FileId {
+        let device = buf.st_dev as _;
+        let inode = buf.st_ino as _;
+        let size = buf.st_size as _;
+        let change_seconds = buf.st_ctime as _;
+        let mod_seconds = buf.st_mtime as _;
+
+        let change_nanoseconds;
+        let mod_nanoseconds;
+
+        #[cfg(not(target_os = "netbsd"))]
         {
-            Ordering::Less => true,
-            Ordering::Equal | Ordering::Greater => false,
+            change_nanoseconds = buf.st_ctime_nsec as _;
+            mod_nanoseconds = buf.st_mtime_nsec as _;
         }
+        #[cfg(target_os = "netbsd")]
+        {
+            change_nanoseconds = buf.st_ctimensec as _;
+            mod_nanoseconds = buf.st_mtimensec as _;
+        }
+        FileId {
+            device,
+            inode,
+            size,
+            change_seconds,
+            change_nanoseconds,
+            mod_seconds,
+            mod_nanoseconds,
+        }
+    }
+
+    /// Return true if \param rhs has higher mtime seconds than this file_id_t.
+    /// If identical, nanoseconds are compared.
+    pub fn older_than(&self, rhs: &FileId) -> bool {
+        let lhs = (self.mod_seconds, self.mod_nanoseconds);
+        let rhs = (rhs.mod_seconds, rhs.mod_nanoseconds);
+        lhs.cmp(&rhs).is_lt()
     }
 }
 
@@ -49,20 +82,19 @@ pub const INVALID_FILE_ID: FileId = FileId {
     inode: u64::MAX,
     size: u64::MAX,
     change_seconds: i64::MIN,
-    change_nanoseconds: -1,
+    change_nanoseconds: i64::MIN,
     mod_seconds: i64::MIN,
-    mod_nanoseconds: -1,
+    mod_nanoseconds: i64::MIN,
 };
 
 /// Get a FileID corresponding to a raw file descriptor, or INVALID_FILE_ID if it fails.
-pub fn file_id_for_fd(fd: RawFd) -> FileId {
-    // Safety: we just want fstat(). Rust makes this stupidly hard.
-    // The only way to get fstat from an fd is to use a File as an intermediary,
-    // but File assumes ownership; so we have to use into_raw_fd() to release it.
+pub fn file_id_for_fd(fd: impl AsRawFd) -> FileId {
+    let fd = fd.as_raw_fd();
     let file = unsafe { File::from_raw_fd(fd) };
     let res = file
         .metadata()
-        .map(FileId::from_stat)
+        .as_ref()
+        .map(FileId::from_md)
         .unwrap_or(INVALID_FILE_ID);
     let fd2 = file.into_raw_fd();
     assert_eq!(fd, fd2);
@@ -71,7 +103,13 @@ pub fn file_id_for_fd(fd: RawFd) -> FileId {
 
 /// Get a FileID corresponding to a path, or INVALID_FILE_ID if it fails.
 pub fn file_id_for_path(path: &wstr) -> FileId {
-    wstat(path)
-        .map(FileId::from_stat)
+    file_id_for_path_narrow(&wcs2zstring(path))
+}
+
+pub fn file_id_for_path_narrow(path: &CStr) -> FileId {
+    let path = OsStr::from_bytes(path.to_bytes());
+    fs::metadata(path)
+        .as_ref()
+        .map(FileId::from_md)
         .unwrap_or(INVALID_FILE_ID)
 }

src/wutil/mod.rs

@@ -14,14 +14,13 @@ use crate::common::{
     cstr2wcstring, fish_reserved_codepoint, str2wcstring, wcs2osstring, wcs2string, wcs2zstring,
 };
 use crate::fallback;
-use crate::fds::AutoCloseFd;
 use crate::flog::FLOGF;
 use crate::wchar::{wstr, WString, L};
 use crate::wchar_ext::WExt;
 use crate::wcstringutil::{join_strings, wcs2string_callback};
 use errno::errno;
 pub use gettext::{wgettext, wgettext_fmt, wgettext_maybe_fmt, wgettext_str};
-use std::ffi::{CStr, OsStr};
+use std::ffi::OsStr;
 use std::fs::{self, canonicalize};
 use std::io::{self, Write};
 use std::os::unix::prelude::*;
@@ -29,6 +28,9 @@ use std::os::unix::prelude::*;
 extern crate fish_printf;
 pub use fish_printf::sprintf;
 
+pub use fileid::{
+    file_id_for_fd, file_id_for_path, file_id_for_path_narrow, FileId, INVALID_FILE_ID,
+};
 pub use wcstoi::*;
 
 /// Wide character version of opendir(). Note that opendir() is guaranteed to set close-on-exec by
@@ -592,90 +594,6 @@ pub fn fish_wcswidth(s: &wstr) -> isize {
     fallback::fish_wcswidth(s)
 }
 
-/// Class for representing a file's inode. We use this to detect and avoid symlink loops, among
-/// other things. While an inode / dev pair is sufficient to distinguish co-existing files, Linux
-/// seems to aggressively re-use inodes, so it cannot determine if a file has been deleted (ABA
-/// problem). Therefore we include richer information.
-#[derive(Debug, Clone, Eq, Hash, Ord, PartialEq, PartialOrd)]
-pub struct FileId {
-    pub device: libc::dev_t,
-    pub inode: libc::ino_t,
-    pub size: u64,
-    pub change_seconds: libc::time_t,
-    pub change_nanoseconds: i64,
-    pub mod_seconds: libc::time_t,
-    pub mod_nanoseconds: i64,
-}
-
-impl FileId {
-    pub const fn new() -> Self {
-        FileId {
-            device: -1 as _,
-            inode: -1 as _,
-            size: -1 as _,
-            change_seconds: libc::time_t::MIN,
-            change_nanoseconds: i64::MIN,
-            mod_seconds: libc::time_t::MIN,
-            mod_nanoseconds: -1 as _,
-        }
-    }
-    pub fn from_stat(buf: &libc::stat) -> FileId {
-        let mut result = FileId::new();
-        result.device = buf.st_dev;
-        result.inode = buf.st_ino;
-        result.size = buf.st_size as u64;
-        result.change_seconds = buf.st_ctime;
-        result.mod_seconds = buf.st_mtime;
-        #[allow(clippy::unnecessary_cast)] // platform-dependent
-        #[cfg(not(target_os = "netbsd"))]
-        {
-            result.change_nanoseconds = buf.st_ctime_nsec as _;
-            result.mod_nanoseconds = buf.st_mtime_nsec as _;
-        }
-        #[cfg(target_os = "netbsd")]
-        {
-            result.change_nanoseconds = buf.st_ctimensec as _;
-            result.mod_nanoseconds = buf.st_mtimensec as _;
-        }
-        result
-    }
-
-    /// Return true if \param rhs has higher mtime seconds than this file_id_t.
-    /// If identical, nanoseconds are compared.
-    pub fn older_than(&self, rhs: &FileId) -> bool {
-        let lhs = (self.mod_seconds, self.mod_nanoseconds);
-        let rhs = (rhs.mod_seconds, rhs.mod_nanoseconds);
-        lhs.cmp(&rhs).is_lt()
-    }
-}
-
-pub const INVALID_FILE_ID: FileId = FileId::new();
-
-pub fn file_id_for_fd(fd: BorrowedFd<'_>) -> FileId {
-    let mut result = INVALID_FILE_ID;
-    let mut buf: libc::stat = unsafe { std::mem::zeroed() };
-    if unsafe { libc::fstat(fd.as_raw_fd(), &mut buf) } == 0 {
-        result = FileId::from_stat(&buf);
-    }
-    result
-}
-
-pub fn file_id_for_autoclose_fd(fd: &AutoCloseFd) -> FileId {
-    file_id_for_fd(fd.as_fd())
-}
-
-pub fn file_id_for_path(path: &wstr) -> FileId {
-    file_id_for_path_narrow(&wcs2zstring(path))
-}
-
-pub fn file_id_for_path_narrow(path: &CStr) -> FileId {
-    let mut result = INVALID_FILE_ID;
-    let mut buf: libc::stat = unsafe { std::mem::zeroed() };
-    if unsafe { libc::stat(path.as_ptr(), &mut buf) } == 0 {
-        result = FileId::from_stat(&buf);
-    }
-    result
-}
 /// Given that `cursor` is a pointer into `base`, return the offset in characters.
 /// This emulates C pointer arithmetic:
 ///    `wstr_offset_in(cursor, base)` is equivalent to C++ `cursor - base`.

src/wutil/tests.rs

@@ -1,3 +1,4 @@
+use crate::fds::AutoCloseFd;
 use crate::tests::prelude::*;
 use libc::{c_void, O_CREAT, O_RDWR, O_TRUNC, SEEK_SET};
 use rand::random;