Skip to content

Commit

Permalink
Add fs::MTime newtype and fix assumptions of temporal ordering
Browse files Browse the repository at this point in the history
See ["mtime comparison considered
harmful"](https://apenwarr.ca/log/20181113) for details of why
comparators other than equality/inequality should not be used with
mtime.
  • Loading branch information
mgsloan committed Nov 18, 2024
1 parent f12981d commit 4d799dd
Show file tree
Hide file tree
Showing 15 changed files with 139 additions and 112 deletions.
4 changes: 4 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion crates/assistant/src/context_store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -770,7 +770,7 @@ impl ContextStore {
contexts.push(SavedContextMetadata {
title: title.to_string(),
path,
mtime: metadata.mtime.into(),
mtime: metadata.mtime.timestamp_for_user().into(),
});
}
}
Expand Down
2 changes: 1 addition & 1 deletion crates/copilot/src/copilot.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1231,7 +1231,7 @@ mod tests {

fn disk_state(&self) -> language::DiskState {
language::DiskState::Present {
mtime: std::time::UNIX_EPOCH,
mtime: ::fs::MTime::from_seconds_and_nanos(100, 42),
}
}

Expand Down
1 change: 1 addition & 0 deletions crates/editor/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ emojis.workspace = true
file_icons.workspace = true
futures.workspace = true
fuzzy.workspace = true
fs.workspace = true
git.workspace = true
gpui.workspace = true
http_client.workspace = true
Expand Down
14 changes: 4 additions & 10 deletions crates/editor/src/items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1620,13 +1620,11 @@ mod tests {
use crate::editor_tests::init_test;

use super::*;
use fs::MTime;
use gpui::{AppContext, VisualTestContext};
use language::{LanguageMatcher, TestFile};
use project::FakeFs;
use std::{
path::{Path, PathBuf},
time::SystemTime,
};
use std::path::{Path, PathBuf};

#[gpui::test]
fn test_path_for_file(cx: &mut AppContext) {
Expand Down Expand Up @@ -1679,9 +1677,7 @@ mod tests {
async fn test_deserialize(cx: &mut gpui::TestAppContext) {
init_test(cx, |_| {});

let now = SystemTime::now();
let fs = FakeFs::new(cx.executor());
fs.set_next_mtime(now);
fs.insert_file("/file.rs", Default::default()).await;

// Test case 1: Deserialize with path and contents
Expand All @@ -1695,7 +1691,7 @@ mod tests {
abs_path: Some(PathBuf::from("/file.rs")),
contents: Some("fn main() {}".to_string()),
language: Some("Rust".to_string()),
mtime: Some(now),
mtime: Some(fs.get_and_increment_mtime()),
};

DB.save_serialized_editor(item_id, workspace_id, serialized_editor.clone())
Expand Down Expand Up @@ -1792,9 +1788,7 @@ mod tests {
let workspace_id = workspace::WORKSPACE_DB.next_id().await.unwrap();

let item_id = 9345 as ItemId;
let old_mtime = now
.checked_sub(std::time::Duration::from_secs(60 * 60 * 24))
.unwrap();
let old_mtime = MTime::from_seconds_and_nanos(0, 50);
let serialized_editor = SerializedEditor {
abs_path: Some(PathBuf::from("/file.rs")),
contents: Some("fn main() {}".to_string()),
Expand Down
24 changes: 10 additions & 14 deletions crates/editor/src/persistence.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
use anyhow::Result;
use db::sqlez::bindable::{Bind, Column, StaticColumnCount};
use db::sqlez::statement::Statement;
use fs::MTime;
use std::path::PathBuf;
use std::time::{Duration, SystemTime, UNIX_EPOCH};

use db::sqlez_macros::sql;
use db::{define_connection, query};
Expand All @@ -14,7 +14,7 @@ pub(crate) struct SerializedEditor {
pub(crate) abs_path: Option<PathBuf>,
pub(crate) contents: Option<String>,
pub(crate) language: Option<String>,
pub(crate) mtime: Option<SystemTime>,
pub(crate) mtime: Option<MTime>,
}

impl StaticColumnCount for SerializedEditor {
Expand All @@ -29,16 +29,13 @@ impl Bind for SerializedEditor {
let start_index = statement.bind(&self.contents, start_index)?;
let start_index = statement.bind(&self.language, start_index)?;

let mtime = self.mtime.and_then(|mtime| {
mtime
.duration_since(UNIX_EPOCH)
.ok()
.map(|duration| (duration.as_secs() as i64, duration.subsec_nanos() as i32))
});
let start_index = match mtime {
let start_index = match self
.mtime
.and_then(|mtime| mtime.to_seconds_and_nanos_for_persistence())
{
Some((seconds, nanos)) => {
let start_index = statement.bind(&seconds, start_index)?;
statement.bind(&nanos, start_index)?
let start_index = statement.bind(&(seconds as i64), start_index)?;
statement.bind(&(nanos as i32), start_index)?
}
None => {
let start_index = statement.bind::<Option<i64>>(&None, start_index)?;
Expand All @@ -64,7 +61,7 @@ impl Column for SerializedEditor {

let mtime = mtime_seconds
.zip(mtime_nanos)
.map(|(seconds, nanos)| UNIX_EPOCH + Duration::new(seconds as u64, nanos as u32));
.map(|(seconds, nanos)| MTime::from_seconds_and_nanos(seconds as u64, nanos as u32));

let editor = Self {
abs_path,
Expand Down Expand Up @@ -280,12 +277,11 @@ mod tests {
assert_eq!(have, serialized_editor);

// Storing and retrieving mtime
let now = SystemTime::now();
let serialized_editor = SerializedEditor {
abs_path: None,
contents: None,
language: None,
mtime: Some(now),
mtime: Some(MTime::from_seconds_and_nanos(100, 42)),
};

DB.save_serialized_editor(1234, workspace_id, serialized_editor.clone())
Expand Down
2 changes: 1 addition & 1 deletion crates/extension_host/src/extension_host.rs
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@ impl ExtensionStore {
if let (Ok(Some(index_metadata)), Ok(Some(extensions_metadata))) =
(index_metadata, extensions_metadata)
{
if index_metadata.mtime > extensions_metadata.mtime {
if index_metadata.mtime != extensions_metadata.mtime {
extension_index_needs_rebuild = false;
}
}
Expand Down
2 changes: 2 additions & 0 deletions crates/fs/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,10 @@ libc.workspace = true
parking_lot.workspace = true
paths.workspace = true
rope.workspace = true
rpc.workspace = true
serde.workspace = true
serde_json.workspace = true
serde_derive.workspace = true
smol.workspace = true
tempfile.workspace = true
text.workspace = true
Expand Down
Loading

0 comments on commit 4d799dd

Please sign in to comment.