diff --git a/crates/git_graph/src/git_graph.rs b/crates/git_graph/src/git_graph.rs index 90ccf94f5f91720972a52d85bc506d12c1a528cb..d9c3620959e371e160fcbcbf53c124d53f3ac4c0 100644 --- a/crates/git_graph/src/git_graph.rs +++ b/crates/git_graph/src/git_graph.rs @@ -274,8 +274,8 @@ fn accent_colors_count(accents: &AccentColors) -> usize { accents.0.len() } -#[derive(Copy, Clone, Debug)] -struct BranchColor(u8); +#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] +struct BranchId(usize); #[derive(Debug)] enum LaneState { @@ -283,7 +283,7 @@ enum LaneState { Active { child: Oid, parent: Oid, - color: Option, + branch_id: BranchId, starting_row: usize, starting_col: usize, destination_column: Option, @@ -297,7 +297,6 @@ impl LaneState { ending_row: usize, lane_column: usize, parent_column: usize, - parent_color: BranchColor, ) -> Option { let state = std::mem::replace(self, LaneState::Empty); @@ -307,14 +306,13 @@ impl LaneState { parent, #[cfg_attr(not(test), allow(unused_variables))] child, - color, + branch_id, starting_row, starting_col, destination_column, mut segments, } => { let final_destination = destination_column.unwrap_or(parent_column); - let final_color = color.unwrap_or(parent_color); Some(CommitLine { #[cfg(test)] @@ -323,7 +321,7 @@ impl LaneState { parent, child_column: starting_col, full_interval: starting_row..ending_row, - color_idx: final_color.0 as usize, + branch_id, segments: { match segments.last_mut() { Some(CommitLineSegment::Straight { to_row }) @@ -431,6 +429,13 @@ impl LaneState { } } + fn branch_id(&self) -> Option { + match self { + LaneState::Active { branch_id, .. } => Some(*branch_id), + LaneState::Empty => None, + } + } + fn is_empty(&self) -> bool { match self { LaneState::Empty => true, @@ -442,7 +447,7 @@ impl LaneState { struct CommitEntry { data: Arc, lane: usize, - color_idx: usize, + branch_id: BranchId, } type ActiveLaneIdx = usize; @@ -478,7 +483,7 @@ struct CommitLine { parent: Oid, child_column: usize, full_interval: Range, - color_idx: usize, + branch_id: BranchId, segments: SmallVec<[CommitLineSegment; 1]>, } @@ -522,10 +527,8 @@ struct CommitLineKey { struct GraphData { lane_states: SmallVec<[LaneState; 8]>, - lane_colors: HashMap, parent_to_lanes: HashMap>, - next_color: BranchColor, - accent_colors_count: usize, + next_branch_id: BranchId, commits: Vec>, max_commit_count: AllCommitCount, max_lanes: usize, @@ -535,13 +538,11 @@ struct GraphData { } impl GraphData { - fn new(accent_colors_count: usize) -> Self { + fn new(_accent_colors_count: usize) -> Self { GraphData { lane_states: SmallVec::default(), - lane_colors: HashMap::default(), parent_to_lanes: HashMap::default(), - next_color: BranchColor(0), - accent_colors_count, + next_branch_id: BranchId(0), commits: Vec::default(), max_commit_count: AllCommitCount::NotLoaded, max_lanes: 0, @@ -553,13 +554,12 @@ impl GraphData { fn clear(&mut self) { self.lane_states.clear(); - self.lane_colors.clear(); self.parent_to_lanes.clear(); self.commits.clear(); self.lines.clear(); self.active_commit_lines.clear(); self.active_commit_lines_by_parent.clear(); - self.next_color = BranchColor(0); + self.next_branch_id = BranchId(0); self.max_commit_count = AllCommitCount::NotLoaded; self.max_lanes = 0; } @@ -574,13 +574,23 @@ impl GraphData { }) } - fn get_lane_color(&mut self, lane_idx: ActiveLaneIdx) -> BranchColor { - let accent_colors_count = self.accent_colors_count; - *self.lane_colors.entry(lane_idx).or_insert_with(|| { - let color_idx = self.next_color; - self.next_color = BranchColor((self.next_color.0 + 1) % accent_colors_count as u8); - color_idx - }) + fn allocate_branch_id(&mut self) -> BranchId { + let branch_id = self.next_branch_id; + self.next_branch_id = BranchId(self.next_branch_id.0 + 1); + branch_id + } + + fn branch_id_for_lane(&self, lane_idx: ActiveLaneIdx) -> Option { + self.lane_states + .get(lane_idx) + .and_then(LaneState::branch_id) + } + + fn origin_branch_id_for_parent(&self, parent: Oid) -> Option { + self.parent_to_lanes + .get(&parent) + .and_then(|lane_indices| lane_indices.first().copied()) + .and_then(|lane_idx| self.branch_id_for_lane(lane_idx)) } fn add_commits(&mut self, commits: &[Arc]) { @@ -596,8 +606,9 @@ impl GraphData { .and_then(|lanes| lanes.first().copied()); let commit_lane = commit_lane.unwrap_or_else(|| self.first_empty_lane_idx()); - - let commit_color = self.get_lane_color(commit_lane); + let commit_branch_id = self + .branch_id_for_lane(commit_lane) + .unwrap_or_else(|| self.allocate_branch_id()); if let Some(lanes) = self.parent_to_lanes.remove(&commit.sha) { for lane_column in lanes { @@ -632,7 +643,7 @@ impl GraphData { } if let Some(commit_line) = - state.to_commit_lines(commit_row, lane_column, commit_lane, commit_color) + state.to_commit_lines(commit_row, lane_column, commit_lane) { self.lines.push(Rc::new(commit_line)); } @@ -648,7 +659,7 @@ impl GraphData { self.lane_states[commit_lane] = LaneState::Active { parent: *parent, child: commit.sha, - color: Some(commit_color), + branch_id: commit_branch_id, starting_col: commit_lane, starting_row: commit_row, destination_column: None, @@ -661,11 +672,14 @@ impl GraphData { .push(commit_lane); } else { let new_lane = self.first_empty_lane_idx(); + let branch_id = self + .origin_branch_id_for_parent(*parent) + .unwrap_or_else(|| self.allocate_branch_id()); self.lane_states[new_lane] = LaneState::Active { parent: *parent, child: commit.sha, - color: None, + branch_id, starting_col: commit_lane, starting_row: commit_row, destination_column: None, @@ -688,7 +702,7 @@ impl GraphData { self.commits.push(Rc::new(CommitEntry { data: commit.clone(), lane: commit_lane, - color_idx: commit_color.0 as usize, + branch_id: commit_branch_id, })); } @@ -1121,11 +1135,7 @@ impl GitGraph { } let accent_colors = cx.theme().accents(); - let accent_color = accent_colors - .0 - .get(commit.color_idx) - .copied() - .unwrap_or_else(|| accent_colors.0.first().copied().unwrap_or_default()); + let accent_color = accent_colors.color_for_index(commit.branch_id.0 as u32); let is_selected = self.selected_entry_idx == Some(idx); let column_label = |label: SharedString| { @@ -1341,11 +1351,7 @@ impl GitGraph { let ref_names = commit_entry.data.ref_names.clone(); let accent_colors = cx.theme().accents(); - let accent_color = accent_colors - .0 - .get(commit_entry.color_idx) - .copied() - .unwrap_or_else(|| accent_colors.0.first().copied().unwrap_or_default()); + let accent_color = accent_colors.color_for_index(commit_entry.branch_id.0 as u32); let (author_name, author_email, commit_timestamp, subject) = match &data { CommitDataState::Loaded(data) => ( @@ -1761,7 +1767,7 @@ impl GitGraph { } for (row_idx, row) in rows.into_iter().enumerate() { - let row_color = accent_colors.color_for_index(row.color_idx as u32); + let row_color = accent_colors.color_for_index(row.branch_id.0 as u32); let row_y_center = bounds.origin.y + row_idx as f32 * row_height + row_height / 2.0 - vertical_scroll_offset; @@ -1899,11 +1905,11 @@ impl GitGraph { } builder.close(); - lines.entry(line.color_idx).or_default().push(builder); + lines.entry(line.branch_id.0).or_default().push(builder); } - for (color_idx, builders) in lines { - let line_color = accent_colors.color_for_index(color_idx as u32); + for (branch_id, builders) in lines { + let line_color = accent_colors.color_for_index(branch_id as u32); for builder in builders { if let Ok(path) = builder.build() { @@ -2828,6 +2834,157 @@ mod tests { Ok(()) } + fn expected_branch_assignments( + commits: &[Arc], + ) -> (HashMap, HashMap<(Oid, Oid), BranchId>) { + let mut parent_to_branch_ids: HashMap> = HashMap::default(); + let mut commit_branch_ids = HashMap::default(); + let mut line_branch_ids = HashMap::default(); + let mut next_branch_id = 0usize; + + let mut allocate_branch_id = || { + let branch_id = BranchId(next_branch_id); + next_branch_id += 1; + branch_id + }; + + for commit in commits { + let commit_branch_id = parent_to_branch_ids + .remove(&commit.sha) + .and_then(|branch_ids| branch_ids.first().copied()) + .unwrap_or_else(|| allocate_branch_id()); + + commit_branch_ids.insert(commit.sha, commit_branch_id); + + for (parent_idx, parent) in commit.parents.iter().enumerate() { + let branch_id = if parent_idx == 0 { + commit_branch_id + } else { + parent_to_branch_ids + .get(parent) + .and_then(|branch_ids| branch_ids.first().copied()) + .unwrap_or_else(|| allocate_branch_id()) + }; + + line_branch_ids.insert((commit.sha, *parent), branch_id); + parent_to_branch_ids + .entry(*parent) + .or_default() + .push(branch_id); + } + } + + (commit_branch_ids, line_branch_ids) + } + + fn verify_branch_ids(graph: &GraphData, commits: &[Arc]) -> Result<()> { + let (expected_commit_branch_ids, expected_line_branch_ids) = + expected_branch_assignments(commits); + let mut seen_branch_ids = HashSet::default(); + let actual_commit_branch_ids: HashMap = graph + .commits + .iter() + .map(|commit| (commit.data.sha, commit.branch_id)) + .collect(); + let actual_line_branch_ids: HashMap<(Oid, Oid), BranchId> = graph + .lines + .iter() + .map(|line| ((line.child, line.parent), line.branch_id)) + .collect(); + let mut parent_to_seen_branch_ids: HashMap> = + HashMap::default(); + + for commit in &graph.commits { + let expected_branch_id = expected_commit_branch_ids + .get(&commit.data.sha) + .context("Commit is missing an expected branch id")?; + + if &commit.branch_id != expected_branch_id { + bail!( + "Commit {:?} has branch_id {:?}, expected {:?}", + commit.data.sha, + commit.branch_id, + expected_branch_id + ); + } + + seen_branch_ids.insert(commit.branch_id.0); + } + + for line in &graph.lines { + let expected_branch_id = expected_line_branch_ids + .get(&(line.child, line.parent)) + .context("Line is missing an expected branch id")?; + + if &line.branch_id != expected_branch_id { + bail!( + "Line {:?} -> {:?} has branch_id {:?}, expected {:?}", + line.child, + line.parent, + line.branch_id, + expected_branch_id + ); + } + + seen_branch_ids.insert(line.branch_id.0); + } + + for commit in commits { + let commit_branch_id = *actual_commit_branch_ids + .get(&commit.sha) + .context("Commit is missing an actual branch id")?; + + for (parent_idx, parent) in commit.parents.iter().enumerate() { + let line_branch_id = *actual_line_branch_ids + .get(&(commit.sha, *parent)) + .context("Line is missing an actual branch id")?; + + if parent_idx > 0 + && let Some(origin_branch_id) = parent_to_seen_branch_ids + .get(parent) + .and_then(|branch_ids| branch_ids.first().copied()) + { + if line_branch_id != origin_branch_id { + bail!( + "Line {:?} -> {:?} has branch_id {:?}, expected origin branch id {:?}", + commit.sha, + parent, + line_branch_id, + origin_branch_id + ); + } + + if line_branch_id == commit_branch_id { + bail!( + "Line {:?} -> {:?} reused merged-into branch id {:?} instead of origin branch id {:?}", + commit.sha, + parent, + commit_branch_id, + origin_branch_id + ); + } + } + + parent_to_seen_branch_ids + .entry(*parent) + .or_default() + .push(line_branch_id); + } + } + + let Some(max_branch_id) = seen_branch_ids.iter().max().copied() else { + return Ok(()); + }; + + for expected_branch_id in 0..=max_branch_id { + if !seen_branch_ids.contains(&expected_branch_id) { + bail!("Missing branch id {}", expected_branch_id); + } + } + + Ok(()) + } + fn verify_merge_line_optimality( graph: &GraphData, oid_to_row: &HashMap, @@ -2928,6 +3085,7 @@ mod tests { let oid_to_row = build_oid_to_row_map(graph); verify_commit_order(graph, commits).context("commit order")?; + verify_branch_ids(graph, commits).context("branch ids")?; verify_line_endpoints(graph, &oid_to_row).context("line endpoints")?; verify_column_correctness(graph, &oid_to_row).context("column correctness")?; verify_segment_continuity(graph).context("segment continuity")?; @@ -3044,6 +3202,31 @@ mod tests { } } + #[test] + fn test_git_graph_random_branch_ids() { + for seed in 0..100 { + let mut rng = StdRng::seed_from_u64(seed); + + let adversarial = rng.random_bool(0.2); + let num_commits = if adversarial { + rng.random_range(10..100) + } else { + rng.random_range(5..50) + }; + + let commits = generate_random_commit_dag(&mut rng, num_commits, adversarial); + let mut graph_data = GraphData::new(8); + graph_data.add_commits(&commits); + + if let Err(error) = verify_branch_ids(&graph_data, &commits) { + panic!( + "Branch id invariant violation (seed={}, adversarial={}, num_commits={}):\n{:#}", + seed, adversarial, num_commits, error + ); + } + } + } + // The full integration test has less iterations because it's significantly slower // than the random commit test #[gpui::test(iterations = 10)]