From 63e566e56e94bea885dbfdcb216dc88dffee1019 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Mon, 1 Apr 2024 17:22:25 +0200 Subject: [PATCH] Remove git diff base from symlinked files (#10037) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes https://github.com/zed-industries/zed/issues/4730 ![image](https://github.com/zed-industries/zed/assets/2690773/d3c5317f-8120-45b5-b57c-c0fb5d8c066d) To the left is a symlink, to the right — the real file. The issue was due to the fact, that symlinks files contain the file path to the real file, and git (properly) treats that symlink file contents as diff base, returning in `load_index_text` (via `let content = repo.find_blob(oid)?.content().to_owned();`) the contents of that symlink file — the path. The fix checks for FS metadata before fetching the git diff base, and skips it entirely for symlinks: Zed opens the symlink file contents instead, fully obscuring the git symlink diff hunks. Interesting, that VSCode behaves as Zed before the fix; while the fix makes Zed behave like Intellij* IDEs now. Release Notes: - Fixed git diff hunks appearing in the symlinked files ([4730](https://github.com/zed-industries/zed/issues/4730)) --- crates/fs/src/repository.rs | 3 +++ crates/project/src/project.rs | 47 ++++++++++++++++++++++++++------- crates/worktree/src/worktree.rs | 23 +++++++++++++--- 3 files changed, 60 insertions(+), 13 deletions(-) diff --git a/crates/fs/src/repository.rs b/crates/fs/src/repository.rs index 3eca321d66..806b3a0784 100644 --- a/crates/fs/src/repository.rs +++ b/crates/fs/src/repository.rs @@ -25,6 +25,9 @@ pub struct Branch { pub trait GitRepository: Send { fn reload_index(&self); + + /// Loads a git repository entry's contents. + /// Note that for symlink entries, this will return the contents of the symlink, not the target. fn load_index_text(&self, relative_file_path: &Path) -> Option; /// Returns the URL of the remote with the given name. diff --git a/crates/project/src/project.rs b/crates/project/src/project.rs index 6b8087d960..b39fb423b1 100644 --- a/crates/project/src/project.rs +++ b/crates/project/src/project.rs @@ -7107,11 +7107,12 @@ impl Project { .find(|(work_dir, _)| path.starts_with(work_dir))?; let receiver = receiver.clone(); let path = path.clone(); + let abs_path = worktree_handle.read(cx).absolutize(&path).ok()?; Some(async move { wait_for_loading_buffer(receiver) .await .ok() - .map(|buffer| (buffer, path)) + .map(|buffer| (buffer, path, abs_path)) }) }) .collect::>(); @@ -7130,7 +7131,7 @@ impl Project { changed_repos .iter() .find(|(work_dir, _)| path.starts_with(work_dir))?; - Some((buffer, path.clone())) + Some((buffer, path.clone(), file.abs_path(cx))) }) .collect::>(); @@ -7140,6 +7141,7 @@ impl Project { let remote_id = self.remote_id(); let client = self.client.clone(); + let fs = self.fs.clone(); cx.spawn(move |_, mut cx| async move { // Wait for all of the buffers to load. let future_buffers = future_buffers.collect::>().await; @@ -7150,20 +7152,47 @@ impl Project { let diff_bases_by_buffer = cx .background_executor() .spawn(async move { - future_buffers + let mut diff_base_tasks = future_buffers .into_iter() .flatten() .chain(current_buffers) - .filter_map(|(buffer, path)| { + .filter_map(|(buffer, path, abs_path)| { let (work_directory, repo) = snapshot.repository_and_work_directory_for_path(&path)?; let repo_entry = snapshot.get_local_repo(&repo)?; - let relative_path = path.strip_prefix(&work_directory).ok()?; - let base_text = repo_entry.repo().lock().load_index_text(relative_path); - - Some((buffer, base_text)) + Some((buffer, path, abs_path, work_directory, repo_entry)) }) - .collect::>() + .map(|(buffer, path, abs_path, work_directory, repo_entry)| { + let fs = fs.clone(); + async move { + let abs_path_metadata = fs + .metadata(&abs_path) + .await + .with_context(|| { + format!("loading file and FS metadata for {path:?}") + }) + .log_err() + .flatten()?; + let base_text = if abs_path_metadata.is_dir + || abs_path_metadata.is_symlink + { + None + } else { + let relative_path = path.strip_prefix(&work_directory).ok()?; + repo_entry.repo().lock().load_index_text(relative_path) + }; + Some((buffer, base_text)) + } + }) + .collect::>(); + + let mut diff_bases = Vec::with_capacity(diff_base_tasks.len()); + while let Some(diff_base) = diff_base_tasks.next().await { + if let Some(diff_base) = diff_base { + diff_bases.push(diff_base); + } + } + diff_bases }) .await; diff --git a/crates/worktree/src/worktree.rs b/crates/worktree/src/worktree.rs index 399084d15e..82abf17bb4 100644 --- a/crates/worktree/src/worktree.rs +++ b/crates/worktree/src/worktree.rs @@ -1081,10 +1081,25 @@ impl LocalWorktree { { if let Some(git_repo) = snapshot.git_repositories.get(&*repo.work_directory) { let git_repo = git_repo.repo_ptr.clone(); - index_task = Some( - cx.background_executor() - .spawn(async move { git_repo.lock().load_index_text(&repo_path) }), - ); + index_task = Some(cx.background_executor().spawn({ + let fs = fs.clone(); + let abs_path = abs_path.clone(); + async move { + let abs_path_metadata = fs + .metadata(&abs_path) + .await + .with_context(|| { + format!("loading file and FS metadata for {abs_path:?}") + }) + .log_err() + .flatten()?; + if abs_path_metadata.is_dir || abs_path_metadata.is_symlink { + None + } else { + git_repo.lock().load_index_text(&repo_path) + } + } + })); } } }