oak-team/oak-gpui
3
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Optimistically update hunk states when staging and unstaging hunks (#25687)

Browse Source 
This PR adds an optimistic update when staging or unstaging diff hunks.
In the process, I've also refactored the logic for staging and unstaging
hunks, to consolidate more of it in the `buffer_diff` crate.

I've also changed the way that we treat untracked files. Previously, we
maintained an empty diff for them, so as not to show unwanted
entire-file diff hunks in a regular editor. But then in the project diff
view, we had to account for this, and replace these empty diffs with
entire-file diffs. This form of state management made it more difficult
to store the pending hunks, so now we always use the same
`BufferDiff`/`BufferDiffSnapshot` for untracked files (with a single
hunk spanning the entire buffer), but we just have a special case in
regular buffers, that avoids showing that entire-file hunk.

* [x] Avoid creating a long queue of `set_index` operations when
staging/unstaging rapidly
* [x] Keep pending hunks when diff is recalculated without base text
changes
* [x] Be optimistic even when staging the single hunk in added/deleted
files
* Testing

Release Notes:

- N/A

---------

Co-authored-by: Cole Miller <m@cole-miller.net>
This commit is contained in:
Max Brunsfeld
2025-02-28 20:55:29 +00:00
committed by GitHub
co-authored by Cole Miller
parent 9d8a163f5b
commit 0c2bbb3aa9
14 changed files with 756 additions and 600 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/project/src/buffer_store.rs
+57 -44
View File
@@ -816,20 +816,20 @@ impl LocalBufferStore {
.any(|(work_dir, _)| file.path.starts_with(work_dir))
{
let snapshot = buffer.text_snapshot();
let has_unstaged_diff = diff_state
.unstaged_diff
.as_ref()
.is_some_and(|diff| diff.is_upgradable());
let has_uncommitted_diff = diff_state
.uncommitted_diff
.as_ref()
.is_some_and(|set| set.is_upgradable());
diff_state_updates.push((
snapshot.clone(),
file.path.clone(),
diff_state
.unstaged_diff
.as_ref()
.and_then(|set| set.upgrade())
.is_some(),
diff_state
.uncommitted_diff
.as_ref()
.and_then(|set| set.upgrade())
.is_some(),
))
has_unstaged_diff.then(|| diff_state.index_text.clone()),
has_uncommitted_diff.then(|| diff_state.head_text.clone()),
));
}
}
@@ -845,37 +845,47 @@ impl LocalBufferStore {
diff_state_updates
.into_iter()
.filter_map(
|(buffer_snapshot, path, needs_staged_text, needs_committed_text)| {
|(buffer_snapshot, path, current_index_text, current_head_text)| {
let local_repo = snapshot.local_repo_for_path(&path)?;
let relative_path = local_repo.relativize(&path).ok()?;
let staged_text = if needs_staged_text {
let index_text = if current_index_text.is_some() {
local_repo.repo().load_index_text(&relative_path)
} else {
None
};
let committed_text = if needs_committed_text {
let head_text = if current_head_text.is_some() {
local_repo.repo().load_committed_text(&relative_path)
} else {
None
};
let diff_bases_change =
match (needs_staged_text, needs_committed_text) {
(true, true) => Some(if staged_text == committed_text {
DiffBasesChange::SetBoth(committed_text)
} else {
DiffBasesChange::SetEach {
index: staged_text,
head: committed_text,
}
}),
(true, false) => {
Some(DiffBasesChange::SetIndex(staged_text))
// Avoid triggering a diff update if the base text has not changed.
if let Some((current_index, current_head)) =
current_index_text.as_ref().zip(current_head_text.as_ref())
{
if current_index.as_deref() == index_text.as_ref()
&& current_head.as_deref() == head_text.as_ref()
{
return None;
}
}
let diff_bases_change = match (
current_index_text.is_some(),
current_head_text.is_some(),
) {
(true, true) => Some(if index_text == head_text {
DiffBasesChange::SetBoth(head_text)
} else {
DiffBasesChange::SetEach {
index: index_text,
head: head_text,
}
(false, true) => {
Some(DiffBasesChange::SetHead(committed_text))
}
(false, false) => None,
};
}),
(true, false) => Some(DiffBasesChange::SetIndex(index_text)),
(false, true) => Some(DiffBasesChange::SetHead(head_text)),
(false, false) => None,
};
Some((buffer_snapshot, diff_bases_change))
},
)
@@ -1476,14 +1486,15 @@ impl BufferStore {
diff_state.language = language;
diff_state.language_registry = language_registry;
let diff = cx.new(|_| BufferDiff::new(&text_snapshot));
let diff = cx.new(|cx| BufferDiff::new(&text_snapshot, cx));
match kind {
DiffKind::Unstaged => diff_state.unstaged_diff = Some(diff.downgrade()),
DiffKind::Uncommitted => {
let unstaged_diff = if let Some(diff) = diff_state.unstaged_diff() {
diff
} else {
let unstaged_diff = cx.new(|_| BufferDiff::new(&text_snapshot));
let unstaged_diff =
cx.new(|cx| BufferDiff::new(&text_snapshot, cx));
diff_state.unstaged_diff = Some(unstaged_diff.downgrade());
unstaged_diff
};
@@ -2384,8 +2395,7 @@ impl BufferStore {
shared.diff = Some(diff.clone());
}
})?;
let staged_text =
diff.read_with(&cx, |diff, _| diff.base_text().map(|buffer| buffer.text()))?;
let staged_text = diff.read_with(&cx, |diff, _| diff.base_text_string())?;
Ok(proto::OpenUnstagedDiffResponse { staged_text })
}
@@ -2415,22 +2425,25 @@ impl BufferStore {
diff.read_with(&cx, |diff, cx| {
use proto::open_uncommitted_diff_response::Mode;
let staged_buffer = diff
.secondary_diff()
.and_then(|diff| diff.read(cx).base_text());
let unstaged_diff = diff.secondary_diff();
let index_snapshot = unstaged_diff.and_then(|diff| {
let diff = diff.read(cx);
diff.base_text_exists().then(|| diff.base_text())
});
let mode;
let staged_text;
let committed_text;
if let Some(committed_buffer) = diff.base_text() {
committed_text = Some(committed_buffer.text());
if let Some(staged_buffer) = staged_buffer {
if staged_buffer.remote_id() == committed_buffer.remote_id() {
if diff.base_text_exists() {
let committed_snapshot = diff.base_text();
committed_text = Some(committed_snapshot.text());
if let Some(index_text) = index_snapshot {
if index_text.remote_id() == committed_snapshot.remote_id() {
mode = Mode::IndexMatchesHead;
staged_text = None;
} else {
mode = Mode::IndexAndHead;
staged_text = Some(staged_buffer.text());
staged_text = Some(index_text.text());
}
} else {
mode = Mode::IndexAndHead;
@@ -2439,7 +2452,7 @@ impl BufferStore {
} else {
mode = Mode::IndexAndHead;
committed_text = None;
staged_text = staged_buffer.as_ref().map(|buffer| buffer.text());
staged_text = index_snapshot.as_ref().map(|buffer| buffer.text());
}
proto::OpenUncommittedDiffResponse {
crates/project/src/git.rs
+76 -32
View File
@@ -18,6 +18,7 @@ use language::{Buffer, LanguageRegistry};
use rpc::proto::{git_reset, ToProto};
use rpc::{proto, AnyProtoClient, TypedEnvelope};
use settings::WorktreeId;
use std::collections::VecDeque;
use std::future::Future;
use std::path::{Path, PathBuf};
use std::sync::Arc;
@@ -25,8 +26,6 @@ use text::BufferId;
use util::{maybe, ResultExt};
use worktree::{ProjectEntryId, RepositoryEntry, StatusEntry};
type GitJob = Box<dyn FnOnce(&mut AsyncApp) -> Task<()>>;
pub struct GitStore {
buffer_store: Entity<BufferStore>,
pub(super) project_id: Option<ProjectId>,
@@ -64,6 +63,16 @@ pub enum GitEvent {
GitStateUpdated,
}
struct GitJob {
job: Box<dyn FnOnce(&mut AsyncApp) -> Task<()>>,
key: Option<GitJobKey>,
}
#[derive(PartialEq, Eq)]
enum GitJobKey {
WriteIndex(RepoPath),
}
impl EventEmitter<GitEvent> for GitStore {}
impl GitStore {
@@ -223,9 +232,29 @@ impl GitStore {
fn spawn_git_worker(cx: &mut Context<'_, GitStore>) -> mpsc::UnboundedSender<GitJob> {
let (job_tx, mut job_rx) = mpsc::unbounded::<GitJob>();
cx.spawn(|_, mut cx| async move {
while let Some(job) = job_rx.next().await {
job(&mut cx).await
let mut jobs = VecDeque::new();
loop {
while let Ok(Some(next_job)) = job_rx.try_next() {
jobs.push_back(next_job);
}
if let Some(job) = jobs.pop_front() {
if let Some(current_key) = &job.key {
if jobs
.iter()
.any(|other_job| other_job.key.as_ref() == Some(current_key))
{
continue;
}
}
(job.job)(&mut cx).await;
} else if let Some(job) = job_rx.next().await {
jobs.push_back(job);
} else {
break;
}
}
})
.detach();
@@ -567,6 +596,15 @@ impl Repository {
}
fn send_job<F, Fut, R>(&self, job: F) -> oneshot::Receiver<R>
where
F: FnOnce(GitRepo) -> Fut + 'static,
Fut: Future<Output = R> + Send + 'static,
R: Send + 'static,
{
self.send_keyed_job(None, job)
}
fn send_keyed_job<F, Fut, R>(&self, key: Option<GitJobKey>, job: F) -> oneshot::Receiver<R>
where
F: FnOnce(GitRepo) -> Fut + 'static,
Fut: Future<Output = R> + Send + 'static,
@@ -575,13 +613,16 @@ impl Repository {
let (result_tx, result_rx) = futures::channel::oneshot::channel();
let git_repo = self.git_repo.clone();
self.job_sender
.unbounded_send(Box::new(|cx: &mut AsyncApp| {
let job = job(git_repo);
cx.background_spawn(async move {
let result = job.await;
result_tx.send(result).ok();
})
}))
.unbounded_send(GitJob {
key,
job: Box::new(|cx: &mut AsyncApp| {
let job = job(git_repo);
cx.background_spawn(async move {
let result = job.await;
result_tx.send(result).ok();
})
}),
})
.ok();
result_rx
}
@@ -1144,28 +1185,31 @@ impl Repository {
content: Option<String>,
) -> oneshot::Receiver<anyhow::Result<()>> {
let path = path.clone();
self.send_job(|git_repo| async move {
match git_repo {
GitRepo::Local(repo) => repo.set_index_text(&path, content),
GitRepo::Remote {
project_id,
client,
worktree_id,
work_directory_id,
} => {
client
.request(proto::SetIndexText {
project_id: project_id.0,
worktree_id: worktree_id.to_proto(),
work_directory_id: work_directory_id.to_proto(),
path: path.as_ref().to_proto(),
text: content,
})
.await?;
Ok(())
self.send_keyed_job(
Some(GitJobKey::WriteIndex(path.clone())),
|git_repo| async move {
match git_repo {
GitRepo::Local(repo) => repo.set_index_text(&path, content),
GitRepo::Remote {
project_id,
client,
worktree_id,
work_directory_id,
} => {
client
.request(proto::SetIndexText {
project_id: project_id.0,
worktree_id: worktree_id.to_proto(),
work_directory_id: work_directory_id.to_proto(),
path: path.as_ref().to_proto(),
text: content,
})
.await?;
Ok(())
}
}
}
})
},
)
}
pub fn get_remotes(
crates/project/src/project_tests.rs
+101 -39
View File
@@ -1,5 +1,5 @@
use crate::{task_inventory::TaskContexts, Event, *};
use buffer_diff::{assert_hunks, DiffHunkSecondaryStatus, DiffHunkStatus};
use buffer_diff::{assert_hunks, DiffHunkSecondaryStatus, DiffHunkStatus, DiffHunkStatusKind};
use fs::FakeFs;
use futures::{future, StreamExt};
use gpui::{App, SemanticVersion, UpdateGlobal};
@@ -5819,7 +5819,7 @@ async fn test_unstaged_diff_for_buffer(cx: &mut gpui::TestAppContext) {
assert_hunks(
unstaged_diff.hunks_intersecting_range(Anchor::MIN..Anchor::MAX, &snapshot, cx),
&snapshot,
&unstaged_diff.base_text().unwrap().text(),
&unstaged_diff.base_text().text(),
&[(
2..3,
"",
@@ -5860,19 +5860,25 @@ async fn test_uncommitted_diff_for_buffer(cx: &mut gpui::TestAppContext) {
json!({
".git": {},
"src": {
"main.rs": file_contents,
"modification.rs": file_contents,
}
}),
)
.await;
fs.set_index_for_repo(
Path::new("/dir/.git"),
&[("src/main.rs".into(), staged_contents)],
);
fs.set_head_for_repo(
Path::new("/dir/.git"),
&[("src/main.rs".into(), committed_contents)],
&[
("src/modification.rs".into(), committed_contents),
("src/deletion.rs".into(), "// the-deleted-contents\n".into()),
],
);
fs.set_index_for_repo(
Path::new("/dir/.git"),
&[
("src/modification.rs".into(), staged_contents),
("src/deletion.rs".into(), "// the-deleted-contents\n".into()),
],
);
let project = Project::test(fs.clone(), ["/dir".as_ref()], cx).await;
@@ -5880,33 +5886,28 @@ async fn test_uncommitted_diff_for_buffer(cx: &mut gpui::TestAppContext) {
let language = rust_lang();
language_registry.add(language.clone());
let buffer = project
let buffer_1 = project
.update(cx, |project, cx| {
project.open_local_buffer("/dir/src/main.rs", cx)
project.open_local_buffer("/dir/src/modification.rs", cx)
})
.await
.unwrap();
let uncommitted_diff = project
let diff_1 = project
.update(cx, |project, cx| {
project.open_uncommitted_diff(buffer.clone(), cx)
project.open_uncommitted_diff(buffer_1.clone(), cx)
})
.await
.unwrap();
uncommitted_diff.read_with(cx, |diff, _| {
assert_eq!(
diff.base_text().and_then(|base| base.language().cloned()),
Some(language)
)
diff_1.read_with(cx, |diff, _| {
assert_eq!(diff.base_text().language().cloned(), Some(language))
});
cx.run_until_parked();
uncommitted_diff.update(cx, |uncommitted_diff, cx| {
let snapshot = buffer.read(cx).snapshot();
diff_1.update(cx, |diff, cx| {
let snapshot = buffer_1.read(cx).snapshot();
assert_hunks(
uncommitted_diff.hunks_intersecting_range(Anchor::MIN..Anchor::MAX, &snapshot, cx),
diff.hunks_intersecting_range(Anchor::MIN..Anchor::MAX, &snapshot, cx),
&snapshot,
&uncommitted_diff.base_text_string().unwrap(),
&diff.base_text_string().unwrap(),
&[
(
0..1,
@@ -5924,25 +5925,29 @@ async fn test_uncommitted_diff_for_buffer(cx: &mut gpui::TestAppContext) {
);
});
// Reset HEAD to a version that differs from both the buffer and the index.
let committed_contents = r#"
// print goodbye
fn main() {
}
"#
.unindent();
fs.set_head_for_repo(
Path::new("/dir/.git"),
&[("src/main.rs".into(), committed_contents)],
&[
("src/modification.rs".into(), committed_contents.clone()),
("src/deletion.rs".into(), "// the-deleted-contents\n".into()),
],
);
// Buffer now has an unstaged hunk.
cx.run_until_parked();
uncommitted_diff.update(cx, |uncommitted_diff, cx| {
let snapshot = buffer.read(cx).snapshot();
diff_1.update(cx, |diff, cx| {
let snapshot = buffer_1.read(cx).snapshot();
assert_hunks(
uncommitted_diff.hunks_intersecting_range(Anchor::MIN..Anchor::MAX, &snapshot, cx),
diff.hunks_intersecting_range(Anchor::MIN..Anchor::MAX, &snapshot, cx),
&snapshot,
&uncommitted_diff.base_text().unwrap().text(),
&diff.base_text().text(),
&[(
2..3,
"",
@@ -5951,6 +5956,56 @@ async fn test_uncommitted_diff_for_buffer(cx: &mut gpui::TestAppContext) {
)],
);
});
// Open a buffer for a file that's been deleted.
let buffer_2 = project
.update(cx, |project, cx| {
project.open_local_buffer("/dir/src/deletion.rs", cx)
})
.await
.unwrap();
let diff_2 = project
.update(cx, |project, cx| {
project.open_uncommitted_diff(buffer_2.clone(), cx)
})
.await
.unwrap();
cx.run_until_parked();
diff_2.update(cx, |diff, cx| {
let snapshot = buffer_2.read(cx).snapshot();
assert_hunks(
diff.hunks_intersecting_range(Anchor::MIN..Anchor::MAX, &snapshot, cx),
&snapshot,
&diff.base_text_string().unwrap(),
&[(
0..0,
"// the-deleted-contents\n",
"",
DiffHunkStatus::deleted(DiffHunkSecondaryStatus::HasSecondaryHunk),
)],
);
});
// Stage the deletion of this file
fs.set_index_for_repo(
Path::new("/dir/.git"),
&[("src/modification.rs".into(), committed_contents.clone())],
);
cx.run_until_parked();
diff_2.update(cx, |diff, cx| {
let snapshot = buffer_2.read(cx).snapshot();
assert_hunks(
diff.hunks_intersecting_range(Anchor::MIN..Anchor::MAX, &snapshot, cx),
&snapshot,
&diff.base_text_string().unwrap(),
&[(
0..0,
"// the-deleted-contents\n",
"",
DiffHunkStatus::deleted(DiffHunkSecondaryStatus::None),
)],
);
});
}
#[gpui::test]
@@ -5958,16 +6013,16 @@ async fn test_single_file_diffs(cx: &mut gpui::TestAppContext) {
init_test(cx);
let committed_contents = r#"
fn main() {
println!("hello from HEAD");
}
"#
fn main() {
println!("hello from HEAD");
}
"#
.unindent();
let file_contents = r#"
fn main() {
println!("hello from the working copy");
}
"#
fn main() {
println!("hello from the working copy");
}
"#
.unindent();
let fs = FakeFs::new(cx.background_executor.clone());
@@ -5984,7 +6039,11 @@ async fn test_single_file_diffs(cx: &mut gpui::TestAppContext) {
fs.set_head_for_repo(
Path::new("/dir/.git"),
&[("src/main.rs".into(), committed_contents)],
&[("src/main.rs".into(), committed_contents.clone())],
);
fs.set_index_for_repo(
Path::new("/dir/.git"),
&[("src/main.rs".into(), committed_contents.clone())],
);
let project = Project::test(fs.clone(), ["/dir/src/main.rs".as_ref()], cx).await;
@@ -6013,7 +6072,10 @@ async fn test_single_file_diffs(cx: &mut gpui::TestAppContext) {
1..2,
" println!(\"hello from HEAD\");\n",
" println!(\"hello from the working copy\");\n",
DiffHunkStatus::modified_none(),
DiffHunkStatus {
kind: DiffHunkStatusKind::Modified,
secondary: DiffHunkSecondaryStatus::HasSecondaryHunk,
},
)],
);
});