From db02d093310ed1504df348ac2b38aff6417d4c4c Mon Sep 17 00:00:00 2001 From: Cole Miller Date: Mon, 2 Mar 2026 08:40:34 -0500 Subject: [PATCH] git: Fix conflicted paths not getting cleared (#50327) Before you mark this PR as ready for review, make sure that you have: - [ ] Added a solid test coverage and/or screenshots from doing manual testing - [ ] Done a self-review taking into account security and performance aspects - [ ] Aligned any UI changes with the [UI checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) Release Notes: - Fixed a bug where files would still be marked as having git conflicts after resolving them. --------- Co-authored-by: Bennet Bo Fenner --- crates/git_graph/src/git_graph.rs | 8 +- crates/git_ui/src/git_panel.rs | 4 +- crates/project/src/git_store.rs | 159 ++++++++--------- crates/project/tests/integration/git_store.rs | 164 +++++++++++++++++- .../tests/integration/project_tests.rs | 6 +- crates/sum_tree/src/tree_map.rs | 4 + 6 files changed, 245 insertions(+), 100 deletions(-) diff --git a/crates/git_graph/src/git_graph.rs b/crates/git_graph/src/git_graph.rs index 0052d58f5985a29f11043f0bd97edb76bb8d2124..90ccf94f5f91720972a52d85bc506d12c1a528cb 100644 --- a/crates/git_graph/src/git_graph.rs +++ b/crates/git_graph/src/git_graph.rs @@ -1024,7 +1024,7 @@ impl GitGraph { } } } - RepositoryEvent::BranchChanged | RepositoryEvent::MergeHeadsChanged => { + RepositoryEvent::BranchChanged => { self.pending_select_sha = None; // Only invalidate if we scanned atleast once, // meaning we are not inside the initial repo loading state @@ -3174,12 +3174,6 @@ mod tests { .any(|event| matches!(event, RepositoryEvent::BranchChanged)), "initial repository scan should emit BranchChanged" ); - assert!( - observed_repository_events - .iter() - .any(|event| matches!(event, RepositoryEvent::MergeHeadsChanged)), - "initial repository scan should emit MergeHeadsChanged" - ); let commit_count_after = repository.read_with(cx, |repo, _| { repo.get_graph_data(crate::LogSource::default(), crate::LogOrder::default()) .map(|data| data.commit_data.len()) diff --git a/crates/git_ui/src/git_panel.rs b/crates/git_ui/src/git_panel.rs index b042d66ce9ac5c45af2e5701da2d83db3c3ab907..5131e1d144e2cee0cbdbb32a062d3f9c4ea4a08b 100644 --- a/crates/git_ui/src/git_panel.rs +++ b/crates/git_ui/src/git_panel.rs @@ -769,9 +769,7 @@ impl GitPanel { move |this, _git_store, event, window, cx| match event { GitStoreEvent::RepositoryUpdated( _, - RepositoryEvent::StatusesChanged - | RepositoryEvent::BranchChanged - | RepositoryEvent::MergeHeadsChanged, + RepositoryEvent::StatusesChanged | RepositoryEvent::BranchChanged, true, ) | GitStoreEvent::RepositoryAdded diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index 3113163cbaec65d7b439e0cbf46603d60ac3fae0..45ba6817248929391dcc484b25879cf34e7506b9 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -72,7 +72,7 @@ use std::{ }, time::Instant, }; -use sum_tree::{Edit, SumTree, TreeSet}; +use sum_tree::{Edit, SumTree, TreeMap}; use task::Shell; use text::{Bias, BufferId}; use util::{ @@ -251,9 +251,8 @@ pub struct RepositoryId(pub u64); #[derive(Clone, Debug, Default, PartialEq, Eq)] pub struct MergeDetails { - pub conflicted_paths: TreeSet, + pub merge_heads_by_conflicted_path: TreeMap>>, pub message: Option, - pub heads: Vec>, } #[derive(Clone)] @@ -407,7 +406,6 @@ pub enum GitGraphEvent { #[derive(Clone, Debug, PartialEq, Eq)] pub enum RepositoryEvent { StatusesChanged, - MergeHeadsChanged, BranchChanged, StashEntriesChanged, PendingOpsChanged { pending_ops: SumTree }, @@ -3511,9 +3509,9 @@ impl RepositorySnapshot { removed_statuses: Default::default(), current_merge_conflicts: self .merge - .conflicted_paths + .merge_heads_by_conflicted_path .iter() - .map(|repo_path| repo_path.to_proto()) + .map(|(repo_path, _)| repo_path.to_proto()) .collect(), merge_message: self.merge.message.as_ref().map(|msg| msg.to_string()), project_id, @@ -3582,9 +3580,9 @@ impl RepositorySnapshot { removed_statuses, current_merge_conflicts: self .merge - .conflicted_paths + .merge_heads_by_conflicted_path .iter() - .map(|path| path.to_proto()) + .map(|(path, _)| path.to_proto()) .collect(), merge_message: self.merge.message.as_ref().map(|msg| msg.to_string()), project_id, @@ -3640,12 +3638,16 @@ impl RepositorySnapshot { } pub fn had_conflict_on_last_merge_head_change(&self, repo_path: &RepoPath) -> bool { - self.merge.conflicted_paths.contains(repo_path) + self.merge + .merge_heads_by_conflicted_path + .contains_key(repo_path) } pub fn has_conflict(&self, repo_path: &RepoPath) -> bool { - let had_conflict_on_last_merge_head_change = - self.merge.conflicted_paths.contains(repo_path); + let had_conflict_on_last_merge_head_change = self + .merge + .merge_heads_by_conflicted_path + .contains_key(repo_path); let has_conflict_currently = self .status_for_path(repo_path) .is_some_and(|entry| entry.status.is_conflicted()); @@ -3684,13 +3686,13 @@ pub fn proto_to_stash(entry: &proto::StashEntry) -> Result { } impl MergeDetails { - async fn load( + async fn update( + &mut self, backend: &Arc, - status: &SumTree, - prev_snapshot: &RepositorySnapshot, - ) -> Result<(MergeDetails, bool)> { + current_conflicted_paths: Vec, + ) -> Result { log::debug!("load merge details"); - let message = backend.merge_message().await; + self.message = backend.merge_message().await.map(SharedString::from); let heads = backend .revparse_batch(vec![ "MERGE_HEAD".into(), @@ -3705,44 +3707,31 @@ impl MergeDetails { .into_iter() .map(|opt| opt.map(SharedString::from)) .collect::>(); - let merge_heads_changed = heads != prev_snapshot.merge.heads; - let conflicted_paths = if merge_heads_changed { - let current_conflicted_paths = TreeSet::from_ordered_entries( - status - .iter() - .filter(|entry| entry.status.is_conflicted()) - .map(|entry| entry.repo_path.clone()), - ); - // It can happen that we run a scan while a lengthy merge is in progress - // that will eventually result in conflicts, but before those conflicts - // are reported by `git status`. Since for the moment we only care about - // the merge heads state for the purposes of tracking conflicts, don't update - // this state until we see some conflicts. - if heads.iter().any(Option::is_some) - && !prev_snapshot.merge.heads.iter().any(Option::is_some) - && current_conflicted_paths.is_empty() - { - log::debug!("not updating merge heads because no conflicts found"); - return Ok(( - MergeDetails { - message: message.map(SharedString::from), - ..prev_snapshot.merge.clone() - }, - false, - )); + let mut conflicts_changed = false; + + // Record the merge state for newly conflicted paths + for path in ¤t_conflicted_paths { + if self.merge_heads_by_conflicted_path.get(&path).is_none() { + conflicts_changed = true; + self.merge_heads_by_conflicted_path + .insert(path.clone(), heads.clone()); } + } - current_conflicted_paths - } else { - prev_snapshot.merge.conflicted_paths.clone() - }; - let details = MergeDetails { - conflicted_paths, - message: message.map(SharedString::from), - heads, - }; - Ok((details, merge_heads_changed)) + // Clear state for paths that are no longer conflicted and for which the merge heads have changed + self.merge_heads_by_conflicted_path + .retain(|path, old_merge_heads| { + let keep = current_conflicted_paths.contains(path) + || (old_merge_heads == &heads + && old_merge_heads.iter().any(|head| head.is_some())); + if !keep { + conflicts_changed = true; + } + keep + }); + + Ok(conflicts_changed) } } @@ -3798,7 +3787,7 @@ impl Repository { .shared(); cx.subscribe_self(move |this, event: &RepositoryEvent, _| match event { - RepositoryEvent::BranchChanged | RepositoryEvent::MergeHeadsChanged => { + RepositoryEvent::BranchChanged => { if this.scan_id > 1 { this.initial_graph_data.clear(); } @@ -6004,12 +5993,6 @@ impl Repository { update: proto::UpdateRepository, cx: &mut Context, ) -> Result<()> { - let conflicted_paths = TreeSet::from_ordered_entries( - update - .current_merge_conflicts - .into_iter() - .filter_map(|path| RepoPath::from_proto(&path).log_err()), - ); let new_branch = update.branch_summary.as_ref().map(proto_to_branch); let new_head_commit = update .head_commit_details @@ -6021,7 +6004,17 @@ impl Repository { self.snapshot.branch = new_branch; self.snapshot.head_commit = new_head_commit; - self.snapshot.merge.conflicted_paths = conflicted_paths; + // We don't store any merge head state for downstream projects; the upstream + // will track it and we will just get the updated conflicts + let new_merge_heads = TreeMap::from_ordered_entries( + update + .current_merge_conflicts + .into_iter() + .filter_map(|path| Some((RepoPath::from_proto(&path).ok()?, vec![]))), + ); + let conflicts_changed = + self.snapshot.merge.merge_heads_by_conflicted_path != new_merge_heads; + self.snapshot.merge.merge_heads_by_conflicted_path = new_merge_heads; self.snapshot.merge.message = update.merge_message.map(SharedString::from); let new_stash_entries = GitStash { entries: update @@ -6054,7 +6047,7 @@ impl Repository { }), ) .collect::>(); - if !edits.is_empty() { + if conflicts_changed || !edits.is_empty() { cx.emit(RepositoryEvent::StatusesChanged); } self.snapshot.statuses_by_path.edit(edits, ()); @@ -6141,17 +6134,16 @@ impl Repository { let RepositoryState::Local(LocalRepositoryState { backend, .. }) = state else { bail!("not a local repository") }; - let (snapshot, events) = this - .update(&mut cx, |this, _| { - this.paths_needing_status_update.clear(); - compute_snapshot( - this.id, - this.work_directory_abs_path.clone(), - this.snapshot.clone(), - backend.clone(), - ) - }) - .await?; + let compute_snapshot = this.update(&mut cx, |this, _| { + this.paths_needing_status_update.clear(); + compute_snapshot( + this.id, + this.work_directory_abs_path.clone(), + this.snapshot.clone(), + backend.clone(), + ) + }); + let (snapshot, events) = cx.background_spawn(compute_snapshot).await?; this.update(&mut cx, |this, cx| { this.snapshot = snapshot.clone(); this.clear_pending_ops(cx); @@ -6759,25 +6751,24 @@ async fn compute_snapshot( )]) .await?; let stash_entries = backend.stash_entries().await?; + let mut conflicted_paths = Vec::new(); let statuses_by_path = SumTree::from_iter( - statuses - .entries - .iter() - .map(|(repo_path, status)| StatusEntry { + statuses.entries.iter().map(|(repo_path, status)| { + if status.is_conflicted() { + conflicted_paths.push(repo_path.clone()); + } + StatusEntry { repo_path: repo_path.clone(), status: *status, - }), + } + }), (), ); - let (merge_details, merge_heads_changed) = - MergeDetails::load(&backend, &statuses_by_path, &prev_snapshot).await?; - log::debug!("new merge details (changed={merge_heads_changed:?}): {merge_details:?}"); - - if merge_heads_changed { - events.push(RepositoryEvent::MergeHeadsChanged); - } + let mut merge_details = prev_snapshot.merge; + let conflicts_changed = merge_details.update(&backend, conflicted_paths).await?; + log::debug!("new merge details: {merge_details:?}"); - if statuses_by_path != prev_snapshot.statuses_by_path { + if conflicts_changed || statuses_by_path != prev_snapshot.statuses_by_path { events.push(RepositoryEvent::StatusesChanged) } diff --git a/crates/project/tests/integration/git_store.rs b/crates/project/tests/integration/git_store.rs index 43704953e0d0bd3e81b9b63b5a797934970dcafa..802e0c072bf60466c32146d12cadd7c1e35c61ad 100644 --- a/crates/project/tests/integration/git_store.rs +++ b/crates/project/tests/integration/git_store.rs @@ -336,7 +336,7 @@ mod conflict_set_tests { second_head: UnmergedStatusCode::Updated, }, ); - // Cause the repository to emit MergeHeadsChanged. + // Cause the repository to update cached conflicts state.refs.insert("MERGE_HEAD".into(), "123".into()) }) .unwrap(); @@ -461,6 +461,168 @@ mod conflict_set_tests { assert_eq!(conflict_range, Point::new(1, 0)..Point::new(6, 0)); }); } + + #[gpui::test] + async fn test_conflict_updates_with_delayed_merge_head_conflicts( + executor: BackgroundExecutor, + cx: &mut TestAppContext, + ) { + zlog::init_test(); + cx.update(|cx| { + settings::init(cx); + }); + + let initial_text = " + one + two + three + four + " + .unindent(); + + let conflicted_text = " + one + <<<<<<< HEAD + two + ======= + TWO + >>>>>>> branch + three + four + " + .unindent(); + + let resolved_text = " + one + TWO + three + four + " + .unindent(); + + let fs = FakeFs::new(executor); + fs.insert_tree( + path!("/project"), + json!({ + ".git": {}, + "a.txt": initial_text, + }), + ) + .await; + + let project = Project::test(fs.clone(), [path!("/project").as_ref()], cx).await; + let (git_store, buffer) = project.update(cx, |project, cx| { + ( + project.git_store().clone(), + project.open_local_buffer(path!("/project/a.txt"), cx), + ) + }); + let buffer = buffer.await.unwrap(); + let conflict_set = git_store.update(cx, |git_store, cx| { + git_store.open_conflict_set(buffer.clone(), cx) + }); + + let (events_tx, events_rx) = mpsc::channel::(); + let _conflict_set_subscription = cx.update(|cx| { + cx.subscribe(&conflict_set, move |_, event, _| { + events_tx.send(event.clone()).ok(); + }) + }); + + cx.run_until_parked(); + events_rx + .try_recv() + .expect_err("conflict set should start empty"); + + fs.with_git_state(path!("/project/.git").as_ref(), true, |state| { + state.refs.insert("MERGE_HEAD".into(), "123".into()) + }) + .unwrap(); + + cx.run_until_parked(); + events_rx + .try_recv() + .expect_err("merge head without conflicted paths should not publish conflicts"); + conflict_set.update(cx, |conflict_set, _| { + assert!(!conflict_set.has_conflict); + assert_eq!(conflict_set.snapshot.conflicts.len(), 0); + }); + + buffer.update(cx, |buffer, cx| { + buffer.set_text(conflicted_text.clone(), cx); + }); + fs.with_git_state(path!("/project/.git").as_ref(), true, |state| { + state.unmerged_paths.insert( + repo_path("a.txt"), + UnmergedStatus { + first_head: UnmergedStatusCode::Updated, + second_head: UnmergedStatusCode::Updated, + }, + ); + }) + .unwrap(); + + cx.run_until_parked(); + let update = events_rx + .try_recv() + .expect("conflicts should appear once conflicted paths are visible"); + assert_eq!(update.old_range, 0..0); + assert_eq!(update.new_range, 0..1); + conflict_set.update(cx, |conflict_set, cx| { + assert!(conflict_set.has_conflict); + let conflict_range = conflict_set.snapshot().conflicts[0] + .range + .to_point(buffer.read(cx)); + assert_eq!(conflict_range, Point::new(1, 0)..Point::new(6, 0)); + }); + + buffer.update(cx, |buffer, cx| { + buffer.set_text(resolved_text.clone(), cx); + }); + + cx.run_until_parked(); + let update = events_rx + .try_recv() + .expect("resolved buffer text should clear visible conflict markers"); + assert_eq!(update.old_range, 0..1); + assert_eq!(update.new_range, 0..0); + conflict_set.update(cx, |conflict_set, _| { + assert!(conflict_set.has_conflict); + assert_eq!(conflict_set.snapshot.conflicts.len(), 0); + }); + + fs.with_git_state(path!("/project/.git").as_ref(), true, |state| { + state.refs.insert("MERGE_HEAD".into(), "456".into()); + }) + .unwrap(); + + cx.run_until_parked(); + events_rx.try_recv().expect_err( + "merge-head change without unmerged-path changes should not emit marker updates", + ); + conflict_set.update(cx, |conflict_set, _| { + assert!(conflict_set.has_conflict); + assert_eq!(conflict_set.snapshot.conflicts.len(), 0); + }); + + fs.with_git_state(path!("/project/.git").as_ref(), true, |state| { + state.unmerged_paths.remove(&repo_path("a.txt")); + state.refs.remove("MERGE_HEAD"); + }) + .unwrap(); + + cx.run_until_parked(); + let update = events_rx.try_recv().expect( + "status catch-up should emit a no-op update when clearing stale conflict state", + ); + assert_eq!(update.old_range, 0..0); + assert_eq!(update.new_range, 0..0); + assert!(update.buffer_range.is_none()); + conflict_set.update(cx, |conflict_set, _| { + assert!(!conflict_set.has_conflict); + assert_eq!(conflict_set.snapshot.conflicts.len(), 0); + }); + } } mod git_traversal { diff --git a/crates/project/tests/integration/project_tests.rs b/crates/project/tests/integration/project_tests.rs index 9bd0be45ae3fa1e66e8af2c43657ba039045ecef..6092836c19ef280aa2d13abcb32932f3b47703b6 100644 --- a/crates/project/tests/integration/project_tests.rs +++ b/crates/project/tests/integration/project_tests.rs @@ -10409,10 +10409,7 @@ async fn test_ignored_dirs_events(cx: &mut gpui::TestAppContext) { assert_eq!( repository_updates.lock().drain(..).collect::>(), - vec![ - RepositoryEvent::StatusesChanged, - RepositoryEvent::MergeHeadsChanged, - ], + vec![RepositoryEvent::StatusesChanged,], "Initial worktree scan should produce a repo update event" ); assert_eq!( @@ -10579,7 +10576,6 @@ async fn test_odd_events_for_ignored_dirs( assert_eq!( repository_updates.lock().drain(..).collect::>(), vec![ - RepositoryEvent::MergeHeadsChanged, RepositoryEvent::BranchChanged, RepositoryEvent::StatusesChanged, RepositoryEvent::StatusesChanged, diff --git a/crates/sum_tree/src/tree_map.rs b/crates/sum_tree/src/tree_map.rs index e58f7a65dd5d13ca67d4433bd25118ffb55d1169..004ec918514e0ad18b3c1e55178a6527866d1bb1 100644 --- a/crates/sum_tree/src/tree_map.rs +++ b/crates/sum_tree/src/tree_map.rs @@ -53,6 +53,10 @@ impl TreeMap { self.0.is_empty() } + pub fn contains_key(&self, key: &K) -> bool { + self.get(key).is_some() + } + pub fn get(&self, key: &K) -> Option<&V> { let (.., item) = self .0