From affd456d082ef739095304d9cdb3495aa84c316e Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Mon, 4 Apr 2022 15:10:53 +0000 Subject: [PATCH 1/3] File Cache is valid if checkout or contents hasn't changed --- src/cargo/sources/registry/remote.rs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/cargo/sources/registry/remote.rs b/src/cargo/sources/registry/remote.rs index d3ce8864d70..7ed5550caad 100644 --- a/src/cargo/sources/registry/remote.rs +++ b/src/cargo/sources/registry/remote.rs @@ -170,8 +170,10 @@ impl<'cfg> RegistryData for RemoteRegistry<'cfg> { } // Check if the cache is valid. let current_version = self.current_version(); - if current_version.is_some() && current_version.as_deref() == index_version { - return Poll::Ready(Ok(LoadResponse::CacheValid)); + if let (Some(c), Some(i)) = (current_version, index_version) { + if c.ends_with(i) { + return Poll::Ready(Ok(LoadResponse::CacheValid)); + } } // Note that the index calls this method and the filesystem is locked // in the index, so we don't need to worry about an `update_index` @@ -185,6 +187,13 @@ impl<'cfg> RegistryData for RemoteRegistry<'cfg> { let tree = registry.tree()?; let entry = tree.get_path(path); let entry = entry?; + let file_version = entry.id().to_string(); + if let Some(c) = current_version { + if c.starts_with(&file_version) { + return Ok(LoadResponse::CacheValid); + } + } + let object = entry.to_object(repo)?; let blob = match object.as_blob() { Some(blob) => blob, @@ -194,6 +203,8 @@ impl<'cfg> RegistryData for RemoteRegistry<'cfg> { Ok(LoadResponse::Data { raw_data: blob.content().to_vec(), index_version: current_version.map(String::from), + // TODO: When the reading code has been stable for long enough (Say 7/2022) + // change to `file_version + ":" + current_version` }) } From 269033d08a579f8145534cab97ff35d14af7a1e8 Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Mon, 4 Apr 2022 15:10:53 +0000 Subject: [PATCH 2/3] backwards ends_with Co-authored-by: Weihang Lo --- src/cargo/sources/registry/remote.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cargo/sources/registry/remote.rs b/src/cargo/sources/registry/remote.rs index 7ed5550caad..bce1e6469de 100644 --- a/src/cargo/sources/registry/remote.rs +++ b/src/cargo/sources/registry/remote.rs @@ -171,7 +171,7 @@ impl<'cfg> RegistryData for RemoteRegistry<'cfg> { // Check if the cache is valid. let current_version = self.current_version(); if let (Some(c), Some(i)) = (current_version, index_version) { - if c.ends_with(i) { + if i.ends_with(c.as_str()) { return Poll::Ready(Ok(LoadResponse::CacheValid)); } } From 8058c3de3d3ba9cddf3289caf9d3d351a0e64247 Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Mon, 4 Apr 2022 21:26:45 +0000 Subject: [PATCH 3/3] Fix code and add comment --- src/cargo/sources/registry/remote.rs | 30 ++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/src/cargo/sources/registry/remote.rs b/src/cargo/sources/registry/remote.rs index bce1e6469de..514f45a2545 100644 --- a/src/cargo/sources/registry/remote.rs +++ b/src/cargo/sources/registry/remote.rs @@ -159,6 +159,14 @@ impl<'cfg> RegistryData for RemoteRegistry<'cfg> { self.config.assert_package_cache_locked(path) } + // `index_version` Is a string representing the version of the file used to construct the cached copy. + // Older versions of Cargo used the single value of the hash of the HEAD commit as a `index_version`. + // This is technically correct but a little too conservative. If a new commit is fetched all cached + // files need to be regenerated even if a particular file was not changed. + // Cargo now reads the `index_version` in two parts the cache file is considered valid if `index_version` + // ends with the hash of the HEAD commit OR if it starts with the hash of the file's contents. + // In the future cargo can write cached files with `index_version` = `git_file_hash + ":" + `git_commit_hash`, + // but for now it still uses `git_commit_hash` to be compatible with older Cargoes. fn load( &mut self, _root: &Path, @@ -169,8 +177,8 @@ impl<'cfg> RegistryData for RemoteRegistry<'cfg> { return Poll::Pending; } // Check if the cache is valid. - let current_version = self.current_version(); - if let (Some(c), Some(i)) = (current_version, index_version) { + let git_commit_hash = self.current_version(); + if let (Some(c), Some(i)) = (git_commit_hash, index_version) { if i.ends_with(c.as_str()) { return Poll::Ready(Ok(LoadResponse::CacheValid)); } @@ -181,15 +189,17 @@ impl<'cfg> RegistryData for RemoteRegistry<'cfg> { fn load_helper( registry: &RemoteRegistry<'_>, path: &Path, - current_version: Option<&str>, + index_version: Option<&str>, + git_commit_hash: Option<&str>, ) -> CargoResult { let repo = registry.repo()?; let tree = registry.tree()?; let entry = tree.get_path(path); let entry = entry?; - let file_version = entry.id().to_string(); - if let Some(c) = current_version { - if c.starts_with(&file_version) { + let git_file_hash = entry.id().to_string(); + + if let Some(i) = index_version { + if i.starts_with(git_file_hash.as_str()) { return Ok(LoadResponse::CacheValid); } } @@ -202,13 +212,13 @@ impl<'cfg> RegistryData for RemoteRegistry<'cfg> { Ok(LoadResponse::Data { raw_data: blob.content().to_vec(), - index_version: current_version.map(String::from), - // TODO: When the reading code has been stable for long enough (Say 7/2022) - // change to `file_version + ":" + current_version` + index_version: git_commit_hash.map(String::from), + // TODO: When the reading code has been stable for long enough (Say 8/2022) + // change to `git_file_hash + ":" + git_commit_hash` }) } - match load_helper(&self, path, current_version.as_deref()) { + match load_helper(&self, path, index_version, git_commit_hash.as_deref()) { Ok(result) => Poll::Ready(Ok(result)), Err(_) if !self.updated => { // If git returns an error and we haven't updated the repo, return