Skip worktree trust checks on Zed invisible worktrees (#46563)

Kirill Bulatov created

Follow-up of https://github.com/zed-industries/zed/pull/46256

Release Notes:

- Fixed worktree trust pop-up appearing on a keymap editor invication

Change summary

crates/project/src/trusted_worktrees.rs | 149 +++++++++++++++++++++++---
1 file changed, 131 insertions(+), 18 deletions(-)

Detailed changes

crates/project/src/trusted_worktrees.rs 🔗

@@ -11,7 +11,7 @@
 //! Unless `trust_all_worktrees` auto trust is enabled, does not trust anything that was not persisted before.
 //! When dealing with "restricted" and other related concepts in the API, it means all explicitly restricted, after any of the [`TrustedWorktreesStore::can_trust`] and [`TrustedWorktreesStore::can_trust_global`] calls.
 //!
-//!
+//! Zed does not consider invisible, `worktree.is_visible() == false` worktrees in Zed, as those are programmatically created inside Zed for internal needs, e.g. a tmp dir for `keymap_editor.rs` needs.
 //!
 //!
 //! Path rust hierarchy.
@@ -474,6 +474,12 @@ impl TrustedWorktreesStore {
             return false;
         };
         let worktree_path = worktree.read(cx).abs_path();
+        // Zed opened an "internal" directory: e.g. a tmp dir for `keymap_editor.rs` needs.
+        if !worktree.read(cx).is_visible() {
+            log::debug!("Skipping worktree trust checks for not visible {worktree_path:?}");
+            return true;
+        }
+
         let is_file = worktree.read(cx).is_single_file();
         if self
             .restricted
@@ -491,30 +497,35 @@ impl TrustedWorktreesStore {
             return true;
         }
 
+        // * Single files are auto-approved when something else (not a single file) was approved on this host already.
+        // * If parent path is trusted already, this worktree is stusted also.
+        //
         // See module documentation for details on trust level.
-        if is_file && self.trusted_paths.contains_key(&weak_worktree_store) {
-            return true;
-        }
-
-        let parent_path_trusted =
-            self.trusted_paths
-                .get(&weak_worktree_store)
-                .is_some_and(|trusted_paths| {
-                    trusted_paths.iter().any(|trusted_path| {
-                        let PathTrust::AbsPath(trusted_path) = trusted_path else {
-                            return false;
-                        };
-                        worktree_path.starts_with(trusted_path)
-                    })
-                });
-        if parent_path_trusted {
-            return true;
+        if let Some(trusted_paths) = self.trusted_paths.get(&weak_worktree_store) {
+            let auto_trusted = worktree_store.read_with(cx, |worktree_store, cx| {
+                trusted_paths.iter().any(|trusted_path| match trusted_path {
+                    PathTrust::Worktree(worktree_id) => worktree_store
+                        .worktree_for_id(*worktree_id, cx)
+                        .is_some_and(|worktree| {
+                            let worktree = worktree.read(cx);
+                            worktree_path.starts_with(&worktree.abs_path())
+                                || (is_file && !worktree.is_single_file())
+                        }),
+                    PathTrust::AbsPath(trusted_path) => {
+                        is_file || worktree_path.starts_with(trusted_path)
+                    }
+                })
+            });
+            if auto_trusted {
+                return true;
+            }
         }
 
         self.restricted
             .entry(weak_worktree_store.clone())
             .or_default()
             .insert(worktree_id);
+        log::info!("Worktree {worktree_path:?} is not trusted");
         cx.emit(TrustedWorktreesEvent::Restricted(
             weak_worktree_store,
             HashSet::from_iter([PathTrust::Worktree(worktree_id)]),
@@ -1545,4 +1556,106 @@ mod tests {
             "local worktree should be trusted on local host"
         );
     }
+
+    #[gpui::test]
+    async fn test_invisible_worktree_stores_do_not_affect_trust(cx: &mut TestAppContext) {
+        init_test(cx);
+
+        let fs = FakeFs::new(cx.executor());
+        fs.insert_tree(
+            path!("/"),
+            json!({
+                "visible": { "main.rs": "fn main() {}" },
+                "other": { "a.rs": "fn other() {}" },
+                "invisible": { "b.rs": "fn invisible() {}" }
+            }),
+        )
+        .await;
+
+        let project = Project::test(fs, [path!("/visible").as_ref()], cx).await;
+        let worktree_store = project.read_with(cx, |project, _| project.worktree_store());
+        let visible_worktree_id = worktree_store.read_with(cx, |store, cx| {
+            store
+                .worktrees()
+                .find(|worktree| worktree.read(cx).root_dir().unwrap().ends_with("visible"))
+                .expect("visible worktree")
+                .read(cx)
+                .id()
+        });
+        let trusted_worktrees = init_trust_global(worktree_store.clone(), cx);
+
+        let events: Rc<RefCell<Vec<TrustedWorktreesEvent>>> = Rc::default();
+        cx.update({
+            let events = events.clone();
+            |cx| {
+                cx.subscribe(&trusted_worktrees, move |_, event, _| {
+                    events.borrow_mut().push(match event {
+                        TrustedWorktreesEvent::Trusted(host, paths) => {
+                            TrustedWorktreesEvent::Trusted(host.clone(), paths.clone())
+                        }
+                        TrustedWorktreesEvent::Restricted(host, paths) => {
+                            TrustedWorktreesEvent::Restricted(host.clone(), paths.clone())
+                        }
+                    });
+                })
+            }
+        })
+        .detach();
+
+        assert!(
+            !trusted_worktrees.update(cx, |store, cx| {
+                store.can_trust(&worktree_store, visible_worktree_id, cx)
+            }),
+            "visible worktree should be restricted initially"
+        );
+        assert_eq!(
+            HashSet::from_iter([(visible_worktree_id)]),
+            trusted_worktrees.read_with(cx, |store, _| {
+                store
+                    .restricted
+                    .get(&worktree_store.downgrade())
+                    .unwrap()
+                    .clone()
+            }),
+            "only visible worktree should be restricted",
+        );
+
+        let (new_visible_worktree, new_invisible_worktree) =
+            worktree_store.update(cx, |worktree_store, cx| {
+                let new_visible_worktree = worktree_store.create_worktree("/other", true, cx);
+                let new_invisible_worktree =
+                    worktree_store.create_worktree("/invisible", false, cx);
+                (new_visible_worktree, new_invisible_worktree)
+            });
+        let (new_visible_worktree, new_invisible_worktree) = (
+            new_visible_worktree.await.unwrap(),
+            new_invisible_worktree.await.unwrap(),
+        );
+
+        let new_visible_worktree_id =
+            new_visible_worktree.read_with(cx, |new_visible_worktree, _| new_visible_worktree.id());
+        assert!(
+            !trusted_worktrees.update(cx, |store, cx| {
+                store.can_trust(&worktree_store, new_visible_worktree_id, cx)
+            }),
+            "new visible worktree should be restricted initially",
+        );
+        assert!(
+            trusted_worktrees.update(cx, |store, cx| {
+                store.can_trust(&worktree_store, new_invisible_worktree.read(cx).id(), cx)
+            }),
+            "invisible worktree should be skipped",
+        );
+        assert_eq!(
+            HashSet::from_iter([visible_worktree_id, new_visible_worktree_id]),
+            trusted_worktrees.read_with(cx, |store, _| {
+                store
+                    .restricted
+                    .get(&worktree_store.downgrade())
+                    .unwrap()
+                    .clone()
+            }),
+            "only visible worktrees should be restricted"
+        );
+    }
 }