Skip to content

Commit 8826993

Browse files
committed
Allow traversal to be aborted when there is only 'integrated' tips left.
We should, however, also make sure that we finish dedicated segments, if possible.
1 parent 03e0aec commit 8826993

File tree

8 files changed

+365
-31
lines changed

8 files changed

+365
-31
lines changed

crates/but-graph/src/api.rs

Lines changed: 59 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,14 @@ impl Graph {
4242
) -> SegmentIndex {
4343
let dst = self.inner.add_node(dst);
4444
self.inner[dst].id = dst.index();
45-
self.connect_segments_with_dst_id(src, src_commit, dst, dst_commit, dst_commit_id.into());
45+
self.connect_segments_with_ids(
46+
src,
47+
src_commit,
48+
None,
49+
dst,
50+
dst_commit,
51+
dst_commit_id.into(),
52+
);
4653
dst
4754
}
4855
}
@@ -56,13 +63,14 @@ impl Graph {
5663
dst: SegmentIndex,
5764
dst_commit: impl Into<Option<CommitIndex>>,
5865
) {
59-
self.connect_segments_with_dst_id(src, src_commit, dst, dst_commit, None)
66+
self.connect_segments_with_ids(src, src_commit, None, dst, dst_commit, None)
6067
}
6168

62-
pub(crate) fn connect_segments_with_dst_id(
69+
pub(crate) fn connect_segments_with_ids(
6370
&mut self,
6471
src: SegmentIndex,
6572
src_commit: impl Into<Option<CommitIndex>>,
73+
src_id: Option<gix::ObjectId>,
6674
dst: SegmentIndex,
6775
dst_commit: impl Into<Option<CommitIndex>>,
6876
dst_id: Option<gix::ObjectId>,
@@ -74,7 +82,7 @@ impl Graph {
7482
dst,
7583
Edge {
7684
src: src_commit,
77-
src_id: self[src].commit_id_by_index(src_commit),
85+
src_id: src_id.or_else(|| self[src].commit_id_by_index(src_commit)),
7886
dst: dst_commit,
7987
dst_id: dst_id.or_else(|| self[dst].commit_id_by_index(dst_commit)),
8088
},
@@ -116,6 +124,26 @@ impl Graph {
116124
self.inner.externals(Direction::Outgoing)
117125
}
118126

127+
/// Return all segments that are both [base segments](Self::base_segments) and which
128+
/// aren't fully defined as traversal stopped due to some abort condition being met.
129+
/// Valid partial segments always have at least one commit.
130+
pub fn partial_segments(&self) -> impl Iterator<Item = SegmentIndex> {
131+
self.base_segments().filter(|s| {
132+
let has_outgoing = self
133+
.inner
134+
.edges_directed(*s, Direction::Outgoing)
135+
.next()
136+
.is_some();
137+
if has_outgoing {
138+
return false;
139+
}
140+
self[*s]
141+
.commits
142+
.first()
143+
.is_none_or(|c| !c.parent_ids.is_empty())
144+
})
145+
}
146+
119147
/// Return all segments that sit on top of the `sidx` segment as `(source_commit_index(of sidx), destination_segment_index)`,
120148
/// along with the exact commit at which the segment branches off as seen from `sidx`, usually the last one.
121149
/// Also, **this will only return those segments where the incoming connection points to their first commit**.
@@ -174,11 +202,13 @@ impl Graph {
174202
has_conflicts: bool,
175203
is_entrypoint: bool,
176204
show_message: bool,
205+
is_early_end: bool,
177206
) -> String {
178207
let extra = extra.into();
179208
format!(
180-
"{ep}{kind}{conflict}{hex}{extra}{flags}{msg}{refs}",
209+
"{ep}{end}{kind}{conflict}{hex}{extra}{flags}{msg}{refs}",
181210
ep = if is_entrypoint { "👉" } else { "" },
211+
end = if is_early_end { "✂️" } else { "" },
182212
kind = if commit.flags.contains(CommitFlags::NotInRemote) {
183213
"·"
184214
} else {
@@ -234,15 +264,15 @@ impl Graph {
234264

235265
/// Validate the graph for consistency and fail loudly when an issue was found, after printing the dot graph.
236266
/// Mostly useful for debugging to stop early when a connection wasn't created correctly.
237-
pub(crate) fn validate_or_eprint_dot(&mut self) -> anyhow::Result<()> {
267+
pub fn validated_or_open_as_svg(self) -> anyhow::Result<Self> {
238268
for edge in self.inner.edge_references() {
239269
let res = check_edge(&self.inner, edge);
240270
if res.is_err() {
241-
self.eprint_dot_graph();
271+
self.open_as_svg();
242272
}
243273
res?;
244274
}
245-
Ok(())
275+
Ok(self)
246276
}
247277

248278
/// Output this graph in dot-format to stderr to allow copying it, and using like this for visualization:
@@ -298,6 +328,26 @@ impl Graph {
298328
);
299329
}
300330

331+
/// Return `true` if commit `cidx` in `sidx` is 'cut off', i.e. the traversal finished at this
332+
/// commit due to an abort condition.
333+
pub fn is_early_end_of_traversal(&self, sidx: SegmentIndex, cidx: CommitIndex) -> bool {
334+
if cidx + 1 == self[sidx].commits.len() {
335+
!self[sidx]
336+
.commits
337+
.last()
338+
.expect("length check above works")
339+
.parent_ids
340+
.is_empty()
341+
&& self
342+
.inner
343+
.edges_directed(sidx, Direction::Outgoing)
344+
.next()
345+
.is_none()
346+
} else {
347+
false
348+
}
349+
}
350+
301351
/// Produces a dot-version of the graph.
302352
pub fn dot_graph(&self) -> String {
303353
const HEX: usize = 7;
@@ -325,6 +375,7 @@ impl Graph {
325375
c.has_conflicts,
326376
!show_segment_entrypoint && Some((sidx, Some(cidx))) == entrypoint,
327377
false,
378+
self.is_early_end_of_traversal(sidx, cidx),
328379
)
329380
})
330381
.collect::<Vec<_>>()

crates/but-graph/src/init/mod.rs

Lines changed: 27 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -79,9 +79,10 @@ impl Graph {
7979
/// - It maintains information about the intended connections, so modifications afterwards will show
8080
/// in debugging output if edges are now in violation of this constraint.
8181
///
82-
/// ### (Arbitrary) Rules
82+
/// ### Rules
8383
///
84-
/// These rules should help to create graphs and segmentations that feel natural and are desirable to the user.
84+
/// These rules should help to create graphs and segmentations that feel natural and are desirable to the user,
85+
/// while avoiding traversing the entire commit-graph all the time.
8586
/// Change the rules as you see fit to accomplish this.
8687
///
8788
/// * a commit can be governed by multiple workspaces
@@ -98,6 +99,9 @@ impl Graph {
9899
/// - This implies that remotes aren't relevant for segments added during post-processing, which would typically
99100
/// be empty anyway.
100101
/// - Remotes never take commits that are already owned.
102+
/// * The traversal is cut short when there is only tips which are integrated, even though named segments that are
103+
/// supposed to be in the workspace will be fully traversed (implying they will stop at the first anon segment
104+
/// as will happen at merge commits).
101105
pub fn from_commit_traversal(
102106
tip: gix::Id<'_>,
103107
ref_name: impl Into<Option<gix::refs::FullName>>,
@@ -106,8 +110,6 @@ impl Graph {
106110
) -> anyhow::Result<Self> {
107111
// TODO: also traverse (outside)-branches that ought to be in the workspace. That way we have the desired ones
108112
// automatically and just have to find a way to prune the undesired ones.
109-
// TODO: pickup ref-names and see if some simple logic can avoid messes, like lot's of refs pointing to a single commit.
110-
// while at it: make tags work.
111113
let repo = tip.repo;
112114
let ref_name = ref_name.into();
113115
if ref_name
@@ -133,7 +135,7 @@ impl Graph {
133135
None
134136
}),
135137
)?;
136-
let (workspaces, target_refs) =
138+
let (workspaces, target_refs, desired_refs) =
137139
obtain_workspace_infos(ref_name.as_ref().map(|rn| rn.as_ref()), meta)?;
138140
let mut seen = gix::revwalk::graph::IdMap::<SegmentIndex>::default();
139141
let tip_flags = CommitFlags::NotInRemote;
@@ -299,8 +301,6 @@ impl Graph {
299301
bottom_sidx,
300302
Some(bottom_cidx),
301303
);
302-
graph.validate_or_eprint_dot().unwrap();
303-
304304
continue;
305305
}
306306
Entry::Vacant(e) => {
@@ -320,8 +320,24 @@ impl Graph {
320320
parent_above,
321321
at_commit,
322322
} => match seen.entry(id) {
323-
Entry::Occupied(_) => {
324-
todo!("handle previously existing segment when connecting a new one")
323+
Entry::Occupied(existing_sidx) => {
324+
let bottom_sidx = *existing_sidx.get();
325+
let bottom = &graph[bottom_sidx];
326+
let bottom_cidx = bottom.commit_index_of(id).context(
327+
"BUG: bottom segment must contain ID, `seen` seems out of date",
328+
)?;
329+
if bottom_cidx != 0 {
330+
todo!("split bottom segment at `at_commit`");
331+
}
332+
let bottom_flags = bottom.commits[bottom_cidx].flags;
333+
graph.connect_segments(parent_above, at_commit, bottom_sidx, bottom_cidx);
334+
propagate_flags_downward(
335+
&mut graph.inner,
336+
propagated_flags | bottom_flags,
337+
bottom_sidx,
338+
Some(bottom_cidx),
339+
);
340+
continue;
325341
}
326342
Entry::Vacant(e) => {
327343
let segment_below =
@@ -377,6 +393,8 @@ impl Graph {
377393
&target_refs,
378394
meta,
379395
)?;
396+
397+
prune_integrated_tips(&mut graph.inner, &mut next, &desired_refs);
380398
}
381399

382400
graph.post_processed(

crates/but-graph/src/init/post.rs

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,10 +35,34 @@ impl Graph {
3535
configured_remote_tracking_branches,
3636
)?;
3737
self.workspace_upgrades(meta)?;
38+
self.fill_flags_at_border_segments()?;
3839

3940
Ok(self)
4041
}
4142

43+
/// Borders are special as these are not fully traversed segments, so their flags might be partial.
44+
/// Here we want to assure that segments with local branch names have `NotInRemote` set
45+
/// (just to indicate they are local first).
46+
fn fill_flags_at_border_segments(&mut self) -> anyhow::Result<()> {
47+
for segment in self.partial_segments().collect::<Vec<_>>() {
48+
let segment = &mut self[segment];
49+
// Partial segments are naturally the end, so no need to propagate flags.
50+
// Note that this is usually not relevant except for yielding more correct looking graphs.
51+
let is_named_and_local = segment
52+
.ref_name
53+
.as_ref()
54+
.is_some_and(|rn| rn.category() == Some(Category::LocalBranch));
55+
if is_named_and_local {
56+
for commit in segment.commits.iter_mut() {
57+
// We set this flag as the *lack* of it means it's in a remote, which is definitely wrong
58+
// knowing that we know what's purely remote by looking at the absence of this flag.
59+
commit.flags |= CommitFlags::NotInRemote;
60+
}
61+
}
62+
}
63+
Ok(())
64+
}
65+
4266
/// Perform operations on segments that can reach a workspace segment when searching upwards.
4367
///
4468
/// * insert empty segments as defined by the workspace that affects its downstream.

crates/but-graph/src/init/utils.rs

Lines changed: 43 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -443,7 +443,7 @@ pub fn collect_ref_mapping_by_prefix<'a>(
443443
Ok(all_refs_by_id)
444444
}
445445

446-
/// Returns `([(workspace_ref_name, workspace_info)], target_refs)` for all available workspace,
446+
/// Returns `([(workspace_ref_name, workspace_info)], target_refs, desired_refs)` for all available workspace,
447447
/// or exactly one workspace if `maybe_ref_name`.
448448
/// already points to a workspace. That way we can discover the workspace containing any starting point, but only if needed.
449449
///
@@ -457,6 +457,7 @@ pub fn obtain_workspace_infos(
457457
) -> anyhow::Result<(
458458
Vec<(gix::refs::FullName, ref_metadata::Workspace)>,
459459
Vec<gix::refs::FullName>,
460+
BTreeSet<gix::refs::FullName>,
460461
)> {
461462
let mut workspaces = if let Some((ref_name, ws_data)) = maybe_ref_name
462463
.and_then(|ref_name| {
@@ -483,6 +484,14 @@ pub fn obtain_workspace_infos(
483484
.iter()
484485
.filter_map(|(_, data)| data.target_ref.clone())
485486
.collect();
487+
let desired_refs = workspaces
488+
.iter()
489+
.flat_map(|(_, data): &(_, ref_metadata::Workspace)| {
490+
data.stacks
491+
.iter()
492+
.flat_map(|stacks| stacks.branches.iter().map(|b| b.ref_name.clone()))
493+
})
494+
.collect();
486495
// defensive pruning
487496
workspaces.retain(|(rn, data)| {
488497
if rn.category() != Some(Category::LocalBranch) {
@@ -510,7 +519,7 @@ pub fn obtain_workspace_infos(
510519
true
511520
});
512521

513-
Ok((workspaces, target_refs))
522+
Ok((workspaces, target_refs, desired_refs))
514523
}
515524

516525
pub fn try_refname_to_id(
@@ -591,3 +600,35 @@ pub fn try_queue_remote_tracking_branches(
591600
}
592601
Ok(())
593602
}
603+
604+
/// Remove if there are only tips with integrated commits…
605+
///
606+
/// - keep tips that are adding segments that are or contain a workspace ref
607+
/// - prune the rest
608+
/// - delete empty segments of pruned tips.
609+
pub fn prune_integrated_tips(
610+
graph: &mut PetGraph,
611+
next: &mut VecDeque<QueueItem>,
612+
workspace_refs: &BTreeSet<gix::refs::FullName>,
613+
) {
614+
let all_integated = next
615+
.iter()
616+
.all(|tip| tip.1.contains(CommitFlags::Integrated));
617+
if !all_integated {
618+
return;
619+
}
620+
next.retain(|(_id, _flags, instruction)| {
621+
let sidx = instruction.segment_idx();
622+
let s = &graph[sidx];
623+
let any_segment_ref_is_contained_in_workspace = s
624+
.ref_name
625+
.as_ref()
626+
.into_iter()
627+
.chain(s.commits.iter().flat_map(|c| c.refs.iter()))
628+
.any(|segment_rn| workspace_refs.contains(segment_rn));
629+
if !any_segment_ref_is_contained_in_workspace && s.commits.is_empty() {
630+
graph.remove_node(sidx);
631+
}
632+
any_segment_ref_is_contained_in_workspace
633+
});
634+
}

crates/but-graph/tests/fixtures/scenarios.sh

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -330,5 +330,47 @@ EOF
330330

331331
git checkout gitbutler/workspace
332332
)
333+
334+
git init two-segments-one-integrated
335+
(cd two-segments-one-integrated
336+
for c in $(seq 3); do
337+
commit "$c"
338+
done
339+
git checkout -b A
340+
commit 4
341+
git checkout -b A-feat
342+
commit "A-feat-1"
343+
commit "A-feat-2"
344+
git checkout A
345+
git merge --no-ff A-feat
346+
for c in $(seq 5 8); do
347+
commit "$c"
348+
done
349+
git checkout -b B
350+
commit "B1"
351+
commit "B2"
352+
353+
create_workspace_commit_once B
354+
355+
tick
356+
git checkout -b soon-origin-main main
357+
git merge --no-ff A
358+
setup_remote_tracking soon-origin-main main "move"
359+
git checkout gitbutler/workspace
360+
)
361+
362+
git init on-top-of-target-with-history
363+
(cd on-top-of-target-with-history
364+
commit outdated-main
365+
git checkout -b soon-origin-main
366+
for c in $(seq 5); do
367+
commit "$c"
368+
done
369+
for name in A B C D E F gitbutler/workspace; do
370+
git branch "$name"
371+
done
372+
setup_remote_tracking soon-origin-main main "move"
373+
git checkout gitbutler/workspace
374+
)
333375
)
334376

0 commit comments

Comments
 (0)