Removed scan ID from repository interfaces

Mikayla Maki and Max created

co-authored-by: Max <max@zed.dev>

Change summary

crates/collab/src/db.rs        |   2 
crates/project/src/project.rs  |  19 +-
crates/project/src/worktree.rs | 304 ++++++++++++++++++------------------
crates/rpc/proto/zed.proto     |   5 
crates/rpc/src/proto.rs        |  29 +-
5 files changed, 178 insertions(+), 181 deletions(-)

Detailed changes

crates/collab/src/db.rs 🔗

@@ -1566,7 +1566,6 @@ impl Database {
                                     .push(db_repository.work_directory_id as u64);
                             } else {
                                 worktree.updated_repositories.push(proto::RepositoryEntry {
-                                    scan_id: db_repository.scan_id as u64,
                                     work_directory_id: db_repository.work_directory_id as u64,
                                     branch: db_repository.branch,
                                 });
@@ -2647,7 +2646,6 @@ impl Database {
                         worktrees.get_mut(&(db_repository_entry.worktree_id as u64))
                     {
                         worktree.repository_entries.push(proto::RepositoryEntry {
-                            scan_id: db_repository_entry.scan_id as u64,
                             work_directory_id: db_repository_entry.work_directory_id as u64,
                             branch: db_repository_entry.branch,
                         });

crates/project/src/project.rs 🔗

@@ -4697,7 +4697,7 @@ impl Project {
     fn update_local_worktree_buffers_git_repos(
         &mut self,
         worktree_handle: ModelHandle<Worktree>,
-        repos: &Vec<RepositoryEntry>,
+        repos: &HashMap<Arc<Path>, LocalRepositoryEntry>,
         cx: &mut ModelContext<Self>,
     ) {
         debug_assert!(worktree_handle.read(cx).is_local());
@@ -4715,15 +4715,16 @@ impl Project {
                 let path = file.path().clone();
 
                 let worktree = worktree_handle.read(cx);
-                let repo = match repos
+
+                let (work_directory, repo) = match repos
                     .iter()
-                    .find(|repository| repository.work_directory.contains(worktree, &path))
+                    .find(|(work_directory, _)| path.starts_with(work_directory))
                 {
                     Some(repo) => repo.clone(),
                     None => return,
                 };
 
-                let relative_repo = match repo.work_directory.relativize(worktree, &path) {
+                let relative_repo = match path.strip_prefix(work_directory).log_err() {
                     Some(relative_repo) => relative_repo.to_owned(),
                     None => return,
                 };
@@ -4732,12 +4733,10 @@ impl Project {
 
                 let remote_id = self.remote_id();
                 let client = self.client.clone();
-                let diff_base_task = worktree_handle.update(cx, move |worktree, cx| {
-                    worktree
-                        .as_local()
-                        .unwrap()
-                        .load_index_text(repo, relative_repo, cx)
-                });
+                let git_ptr = repo.repo_ptr.clone();
+                let diff_base_task = cx
+                    .background()
+                    .spawn(async move { git_ptr.lock().load_index_text(&relative_repo) });
 
                 cx.spawn(|_, mut cx| async move {
                     let diff_base = diff_base_task.await;

crates/project/src/worktree.rs 🔗

@@ -119,7 +119,6 @@ pub struct Snapshot {
 
 #[derive(Clone, Debug, Eq, PartialEq)]
 pub struct RepositoryEntry {
-    pub(crate) scan_id: usize,
     pub(crate) work_directory: WorkDirectoryEntry,
     pub(crate) branch: Option<Arc<str>>,
 }
@@ -147,7 +146,6 @@ impl RepositoryEntry {
 impl From<&RepositoryEntry> for proto::RepositoryEntry {
     fn from(value: &RepositoryEntry) -> Self {
         proto::RepositoryEntry {
-            scan_id: value.scan_id as u64,
             work_directory_id: value.work_directory.to_proto(),
             branch: value.branch.as_ref().map(|str| str.to_string()),
         }
@@ -235,6 +233,7 @@ pub struct LocalSnapshot {
 
 #[derive(Debug, Clone)]
 pub struct LocalRepositoryEntry {
+    pub(crate) scan_id: usize,
     pub(crate) repo_ptr: Arc<Mutex<dyn GitRepository>>,
     /// Path to the actual .git folder.
     /// Note: if .git is a file, this points to the folder indicated by the .git file
@@ -281,7 +280,7 @@ struct ShareState {
 
 pub enum Event {
     UpdatedEntries(HashMap<Arc<Path>, PathChange>),
-    UpdatedGitRepositories(Vec<RepositoryEntry>),
+    UpdatedGitRepositories(HashMap<Arc<Path>, LocalRepositoryEntry>),
 }
 
 impl Entity for Worktree {
@@ -690,10 +689,8 @@ impl LocalWorktree {
     }
 
     fn set_snapshot(&mut self, new_snapshot: LocalSnapshot, cx: &mut ModelContext<Worktree>) {
-        let updated_repos = Self::changed_repos(
-            &self.snapshot.repository_entries,
-            &new_snapshot.repository_entries,
-        );
+        let updated_repos =
+            self.changed_repos(&self.git_repositories, &new_snapshot.git_repositories);
         self.snapshot = new_snapshot;
 
         if let Some(share) = self.share.as_mut() {
@@ -706,32 +703,57 @@ impl LocalWorktree {
     }
 
     fn changed_repos(
-        old_repos: &TreeMap<RepositoryWorkDirectory, RepositoryEntry>,
-        new_repos: &TreeMap<RepositoryWorkDirectory, RepositoryEntry>,
-    ) -> Vec<RepositoryEntry> {
-        fn diff<'a>(
-            a: impl Iterator<Item = &'a RepositoryEntry>,
-            mut b: impl Iterator<Item = &'a RepositoryEntry>,
-            updated: &mut HashMap<ProjectEntryId, RepositoryEntry>,
-        ) {
-            for a_repo in a {
-                let matched = b.find(|b_repo| {
-                    a_repo.work_directory == b_repo.work_directory
-                        && a_repo.scan_id == b_repo.scan_id
-                });
+        &self,
+        old_repos: &TreeMap<ProjectEntryId, LocalRepositoryEntry>,
+        new_repos: &TreeMap<ProjectEntryId, LocalRepositoryEntry>,
+    ) -> HashMap<Arc<Path>, LocalRepositoryEntry> {
+        let mut diff = HashMap::default();
+        let mut old_repos = old_repos.iter().peekable();
+        let mut new_repos = new_repos.iter().peekable();
+        loop {
+            match (old_repos.peek(), new_repos.peek()) {
+                (Some((old_entry_id, old_repo)), Some((new_entry_id, new_repo))) => {
+                    match Ord::cmp(old_entry_id, new_entry_id) {
+                        Ordering::Less => {
+                            if let Some(entry) = self.entry_for_id(**old_entry_id) {
+                                diff.insert(entry.path.clone(), (*old_repo).clone());
+                            }
+                            old_repos.next();
+                        }
+                        Ordering::Equal => {
+                            if old_repo.scan_id != new_repo.scan_id {
+                                if let Some(entry) = self.entry_for_id(**new_entry_id) {
+                                    diff.insert(entry.path.clone(), (*new_repo).clone());
+                                }
+                            }
 
-                if matched.is_none() {
-                    updated.insert(*a_repo.work_directory, a_repo.clone());
+                            old_repos.next();
+                            new_repos.next();
+                        }
+                        Ordering::Greater => {
+                            if let Some(entry) = self.entry_for_id(**new_entry_id) {
+                                diff.insert(entry.path.clone(), (*new_repo).clone());
+                            }
+                            new_repos.next();
+                        }
+                    }
+                }
+                (Some((old_entry_id, old_repo)), None) => {
+                    if let Some(entry) = self.entry_for_id(**old_entry_id) {
+                        diff.insert(entry.path.clone(), (*old_repo).clone());
+                    }
+                    old_repos.next();
                 }
+                (None, Some((new_entry_id, new_repo))) => {
+                    if let Some(entry) = self.entry_for_id(**new_entry_id) {
+                        diff.insert(entry.path.clone(), (*new_repo).clone());
+                    }
+                    new_repos.next();
+                }
+                (None, None) => break,
             }
         }
-
-        let mut updated = HashMap::<ProjectEntryId, RepositoryEntry>::default();
-
-        diff(old_repos.values(), new_repos.values(), &mut updated);
-        diff(new_repos.values(), old_repos.values(), &mut updated);
-
-        updated.into_values().collect()
+        diff
     }
 
     pub fn scan_complete(&self) -> impl Future<Output = ()> {
@@ -1166,21 +1188,6 @@ impl LocalWorktree {
     pub fn is_shared(&self) -> bool {
         self.share.is_some()
     }
-
-    pub fn load_index_text(
-        &self,
-        repo: RepositoryEntry,
-        repo_path: RepoPath,
-        cx: &mut ModelContext<Worktree>,
-    ) -> Task<Option<String>> {
-        let Some(git_ptr) = self.git_repositories.get(&repo.work_directory).map(|git_ptr| git_ptr.to_owned()) else {
-            return Task::Ready(Some(None))
-        };
-        let git_ptr = git_ptr.repo_ptr;
-
-        cx.background()
-            .spawn(async move { git_ptr.lock().load_index_text(&repo_path) })
-    }
 }
 
 impl RemoteWorktree {
@@ -1419,7 +1426,6 @@ impl Snapshot {
         for repository in update.updated_repositories {
             let repository = RepositoryEntry {
                 work_directory: ProjectEntryId::from_proto(repository.work_directory_id).into(),
-                scan_id: repository.scan_id as usize,
                 branch: repository.branch.map(Into::into),
             };
             if let Some(entry) = self.entry_for_id(repository.work_directory_id()) {
@@ -1584,20 +1590,12 @@ impl LocalSnapshot {
     pub(crate) fn repo_for_metadata(
         &self,
         path: &Path,
-    ) -> Option<(RepositoryWorkDirectory, Arc<Mutex<dyn GitRepository>>)> {
+    ) -> Option<(ProjectEntryId, Arc<Mutex<dyn GitRepository>>)> {
         let (entry_id, local_repo) = self
             .git_repositories
             .iter()
             .find(|(_, repo)| repo.in_dot_git(path))?;
-
-        let work_dir = self
-            .snapshot
-            .repository_entries
-            .iter()
-            .find(|(_, entry)| *entry.work_directory == *entry_id)
-            .and_then(|(_, entry)| entry.work_directory(self))?;
-
-        Some((work_dir, local_repo.repo_ptr.to_owned()))
+        Some((*entry_id, local_repo.repo_ptr.to_owned()))
     }
 
     #[cfg(test)]
@@ -1686,7 +1684,7 @@ impl LocalSnapshot {
                             self_repos.next();
                         }
                         Ordering::Equal => {
-                            if self_repo.scan_id != other_repo.scan_id {
+                            if self_repo != other_repo {
                                 updated_repositories.push((*self_repo).into());
                             }
 
@@ -1811,21 +1809,19 @@ impl LocalSnapshot {
 
         if parent_path.file_name() == Some(&DOT_GIT) {
             let abs_path = self.abs_path.join(&parent_path);
-            let content_path: Arc<Path> = parent_path.parent().unwrap().into();
+            let work_dir: Arc<Path> = parent_path.parent().unwrap().into();
 
-            if let Some(work_dir_id) = self
-                .entry_for_path(content_path.clone())
-                .map(|entry| entry.id)
-            {
-                let key = RepositoryWorkDirectory(content_path.clone());
-                if self.repository_entries.get(&key).is_none() {
+            if let Some(work_dir_id) = self.entry_for_path(work_dir.clone()).map(|entry| entry.id) {
+                if self.git_repositories.get(&work_dir_id).is_none() {
                     if let Some(repo) = fs.open_repo(abs_path.as_path()) {
+                        let work_directory = RepositoryWorkDirectory(work_dir.clone());
+
                         let repo_lock = repo.lock();
+                        let scan_id = self.scan_id;
                         self.repository_entries.insert(
-                            key.clone(),
+                            work_directory,
                             RepositoryEntry {
                                 work_directory: work_dir_id.into(),
-                                scan_id: 0,
                                 branch: repo_lock.branch_name().map(Into::into),
                             },
                         );
@@ -1834,6 +1830,7 @@ impl LocalSnapshot {
                         self.git_repositories.insert(
                             work_dir_id,
                             LocalRepositoryEntry {
+                                scan_id,
                                 repo_ptr: repo,
                                 git_dir_path: parent_path.clone(),
                             },
@@ -1899,11 +1896,6 @@ impl LocalSnapshot {
             {
                 *scan_id = self.snapshot.scan_id;
             }
-        } else if path.file_name() == Some(&DOT_GIT) {
-            let repo_entry_key = RepositoryWorkDirectory(path.parent().unwrap().into());
-            self.snapshot
-                .repository_entries
-                .update(&repo_entry_key, |repo| repo.scan_id = self.snapshot.scan_id);
         }
     }
 
@@ -2836,15 +2828,22 @@ impl BackgroundScanner {
                     let scan_id = snapshot.scan_id;
 
                     let repo_with_path_in_dotgit = snapshot.repo_for_metadata(&path);
-                    if let Some((key, repo)) = repo_with_path_in_dotgit {
+                    if let Some((entry_id, repo)) = repo_with_path_in_dotgit {
+                        let work_dir = snapshot
+                            .entry_for_id(entry_id)
+                            .map(|entry| RepositoryWorkDirectory(entry.path.clone()))?;
+
                         let repo = repo.lock();
                         repo.reload_index();
                         let branch = repo.branch_name();
 
-                        snapshot.repository_entries.update(&key, |entry| {
+                        snapshot.git_repositories.update(&entry_id, |entry| {
                             entry.scan_id = scan_id;
-                            entry.branch = branch.map(Into::into)
                         });
+
+                        snapshot
+                            .repository_entries
+                            .update(&work_dir, |entry| entry.branch = branch.map(Into::into));
                     }
 
                     if let Some(scan_queue_tx) = &scan_queue_tx {
@@ -3640,26 +3639,27 @@ mod tests {
             );
         });
 
-        let original_scan_id = tree.read_with(cx, |tree, _cx| {
-            let tree = tree.as_local().unwrap();
-            let entry = tree.repo_for("dir1/src/b.txt".as_ref()).unwrap();
-            entry.scan_id
+        let repo_update_events = Arc::new(Mutex::new(vec![]));
+        tree.update(cx, |_, cx| {
+            let repo_update_events = repo_update_events.clone();
+            cx.subscribe(&tree, move |_, _, event, _| {
+                if let Event::UpdatedGitRepositories(update) = event {
+                    repo_update_events.lock().push(update.clone());
+                }
+            })
+            .detach();
         });
 
         std::fs::write(root.path().join("dir1/.git/random_new_file"), "hello").unwrap();
         tree.flush_fs_events(cx).await;
 
-        tree.read_with(cx, |tree, _cx| {
-            let tree = tree.as_local().unwrap();
-            let new_scan_id = {
-                let entry = tree.repo_for("dir1/src/b.txt".as_ref()).unwrap();
-                entry.scan_id
-            };
-            assert_ne!(
-                original_scan_id, new_scan_id,
-                "original {original_scan_id}, new {new_scan_id}"
-            );
-        });
+        assert_eq!(
+            repo_update_events.lock()[0]
+                .keys()
+                .cloned()
+                .collect::<Vec<Arc<Path>>>(),
+            vec![Path::new("dir1").into()]
+        );
 
         std::fs::remove_dir_all(root.path().join("dir1/.git")).unwrap();
         tree.flush_fs_events(cx).await;
@@ -3671,70 +3671,70 @@ mod tests {
         });
     }
 
-    #[test]
-    fn test_changed_repos() {
-        fn fake_entry(work_dir_id: usize, scan_id: usize) -> RepositoryEntry {
-            RepositoryEntry {
-                scan_id,
-                work_directory: ProjectEntryId(work_dir_id).into(),
-                branch: None,
-            }
-        }
-
-        let mut prev_repos = TreeMap::<RepositoryWorkDirectory, RepositoryEntry>::default();
-        prev_repos.insert(
-            RepositoryWorkDirectory(Path::new("don't-care-1").into()),
-            fake_entry(1, 0),
-        );
-        prev_repos.insert(
-            RepositoryWorkDirectory(Path::new("don't-care-2").into()),
-            fake_entry(2, 0),
-        );
-        prev_repos.insert(
-            RepositoryWorkDirectory(Path::new("don't-care-3").into()),
-            fake_entry(3, 0),
-        );
-
-        let mut new_repos = TreeMap::<RepositoryWorkDirectory, RepositoryEntry>::default();
-        new_repos.insert(
-            RepositoryWorkDirectory(Path::new("don't-care-4").into()),
-            fake_entry(2, 1),
-        );
-        new_repos.insert(
-            RepositoryWorkDirectory(Path::new("don't-care-5").into()),
-            fake_entry(3, 0),
-        );
-        new_repos.insert(
-            RepositoryWorkDirectory(Path::new("don't-care-6").into()),
-            fake_entry(4, 0),
-        );
-
-        let res = LocalWorktree::changed_repos(&prev_repos, &new_repos);
-
-        // Deletion retained
-        assert!(res
-            .iter()
-            .find(|repo| repo.work_directory.0 .0 == 1 && repo.scan_id == 0)
-            .is_some());
-
-        // Update retained
-        assert!(res
-            .iter()
-            .find(|repo| repo.work_directory.0 .0 == 2 && repo.scan_id == 1)
-            .is_some());
-
-        // Addition retained
-        assert!(res
-            .iter()
-            .find(|repo| repo.work_directory.0 .0 == 4 && repo.scan_id == 0)
-            .is_some());
-
-        // Nochange, not retained
-        assert!(res
-            .iter()
-            .find(|repo| repo.work_directory.0 .0 == 3 && repo.scan_id == 0)
-            .is_none());
-    }
+    // #[test]
+    // fn test_changed_repos() {
+    //     fn fake_entry(work_dir_id: usize, scan_id: usize) -> RepositoryEntry {
+    //         RepositoryEntry {
+    //             scan_id,
+    //             work_directory: ProjectEntryId(work_dir_id).into(),
+    //             branch: None,
+    //         }
+    //     }
+
+    //     let mut prev_repos = TreeMap::<RepositoryWorkDirectory, RepositoryEntry>::default();
+    //     prev_repos.insert(
+    //         RepositoryWorkDirectory(Path::new("don't-care-1").into()),
+    //         fake_entry(1, 0),
+    //     );
+    //     prev_repos.insert(
+    //         RepositoryWorkDirectory(Path::new("don't-care-2").into()),
+    //         fake_entry(2, 0),
+    //     );
+    //     prev_repos.insert(
+    //         RepositoryWorkDirectory(Path::new("don't-care-3").into()),
+    //         fake_entry(3, 0),
+    //     );
+
+    //     let mut new_repos = TreeMap::<RepositoryWorkDirectory, RepositoryEntry>::default();
+    //     new_repos.insert(
+    //         RepositoryWorkDirectory(Path::new("don't-care-4").into()),
+    //         fake_entry(2, 1),
+    //     );
+    //     new_repos.insert(
+    //         RepositoryWorkDirectory(Path::new("don't-care-5").into()),
+    //         fake_entry(3, 0),
+    //     );
+    //     new_repos.insert(
+    //         RepositoryWorkDirectory(Path::new("don't-care-6").into()),
+    //         fake_entry(4, 0),
+    //     );
+
+    //     let res = LocalWorktree::changed_repos(&prev_repos, &new_repos);
+
+    //     // Deletion retained
+    //     assert!(res
+    //         .iter()
+    //         .find(|repo| repo.work_directory.0 .0 == 1 && repo.scan_id == 0)
+    //         .is_some());
+
+    //     // Update retained
+    //     assert!(res
+    //         .iter()
+    //         .find(|repo| repo.work_directory.0 .0 == 2 && repo.scan_id == 1)
+    //         .is_some());
+
+    //     // Addition retained
+    //     assert!(res
+    //         .iter()
+    //         .find(|repo| repo.work_directory.0 .0 == 4 && repo.scan_id == 0)
+    //         .is_some());
+
+    //     // Nochange, not retained
+    //     assert!(res
+    //         .iter()
+    //         .find(|repo| repo.work_directory.0 .0 == 3 && repo.scan_id == 0)
+    //         .is_none());
+    // }
 
     #[gpui::test]
     async fn test_write_file(cx: &mut TestAppContext) {

crates/rpc/proto/zed.proto 🔗

@@ -982,9 +982,8 @@ message Entry {
 }
 
 message RepositoryEntry {
-    uint64 scan_id = 1;
-    uint64 work_directory_id = 2;
-    optional string branch = 3;
+    uint64 work_directory_id = 1;
+    optional string branch = 2;
 }
 
 message BufferState {

crates/rpc/src/proto.rs 🔗

@@ -5,13 +5,13 @@ use futures::{SinkExt as _, StreamExt as _};
 use prost::Message as _;
 use serde::Serialize;
 use std::any::{Any, TypeId};
-use std::fmt;
 use std::{
     cmp,
     fmt::Debug,
     io, iter,
     time::{Duration, SystemTime, UNIX_EPOCH},
 };
+use std::{fmt, mem};
 
 include!(concat!(env!("OUT_DIR"), "/zed.messages.rs"));
 
@@ -502,21 +502,22 @@ pub fn split_worktree_update(
             .drain(..removed_entries_chunk_size)
             .collect();
 
-        let updated_repositories_chunk_size =
-            cmp::min(message.updated_repositories.len(), max_chunk_size);
-        let updated_repositories = message
-            .updated_repositories
-            .drain(..updated_repositories_chunk_size)
-            .collect();
+        done = message.updated_entries.is_empty() && message.removed_entries.is_empty();
 
-        let removed_repositories_chunk_size =
-            cmp::min(message.removed_repositories.len(), max_chunk_size);
-        let removed_repositories = message
-            .removed_repositories
-            .drain(..removed_repositories_chunk_size)
-            .collect();
+        // Wait to send repositories until after we've guarnteed that their associated entries
+        // will be read
+        let updated_repositories = if done {
+            mem::take(&mut message.updated_repositories)
+        } else {
+            Default::default()
+        };
+
+        let removed_repositories = if done {
+            mem::take(&mut message.removed_repositories)
+        } else {
+            Default::default()
+        };
 
-        done = message.updated_entries.is_empty() && message.removed_entries.is_empty();
         Some(UpdateWorktree {
             project_id: message.project_id,
             worktree_id: message.worktree_id,