Fix the classification for independent (virtual) branches in the workspace

Author: Byron

crates/but-graph/src/debug.rs

@@ -371,7 +371,7 @@ impl Graph {
                 .commit_id_by_index(e.dst)
                 .map(|c| c.to_hex_with_len(HEX).to_string())
                 .unwrap_or_else(|| "dst".into());
-            format!(", label = \"⚠️{src} → {dst} ({err})\", fontname = Courier")
+            format!(", label = \"⚠{src} → {dst} ({err})\", fontname = Courier")
         };
         let dot = petgraph::dot::Dot::with_attr_getters(&self.inner, &[], &edge_attrs, &node_attrs);
         format!("{dot:?}")

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

@@ -12,6 +12,7 @@ use gix::reference::Category;
 use petgraph::Direction;
 use petgraph::graph::NodeIndex;
 use petgraph::prelude::EdgeRef;
+use petgraph::visit::NodeRef;
 use std::collections::{BTreeMap, BTreeSet};
 use tracing::instrument;
 
@@ -439,6 +440,27 @@ impl Graph {
                         .map(|cidx| (sidx, &base_segment.commits[cidx]))
                 })
             })
+            .chain(
+                self.inner
+                    .neighbors_directed(ws_sidx, Direction::Outgoing)
+                    .filter_map(|s| {
+                        // This rule means that if there is no target, we'd want to put new independent stacks
+                        // onto segments which then are ambiguous so they get pulled out.
+                        if target.is_some() {
+                            return None;
+                        }
+                        // It's a very specialised filter… will that lead to strange behaviour later?
+                        let segment = &self[s];
+                        if segment.ref_name.is_some() {
+                            return None;
+                        }
+                        segment
+                            .commits
+                            .first()
+                            .filter(|c| !c.refs.is_empty())
+                            .map(|c| (s.id(), c))
+                    }),
+            )
             .collect();
         out.sort_by_key(|t| t.1.id);
         out.dedup_by_key(|t| t.1.id);
@@ -913,6 +935,37 @@ impl Graph {
         }
         for (remote_sidx, local_sidx) in links_from_remote_to_local {
             self[remote_sidx].sibling_segment_id = Some(local_sidx);
+
+            // If both remote and local point to the same commit, make sure that the remote points to the local segment.
+            if let Some((
+                (_remote_commit, _owner_of_commit_same_as_local),
+                (_local_commmit, owner_of_commit_same_as_remote),
+            )) = self
+                .first_commit_or_find_along_first_parent(remote_sidx)
+                .zip(self.first_commit_or_find_along_first_parent(local_sidx))
+                .filter(|((a, a_sidx), (b, b_sidx))| a.id == b.id && a_sidx == b_sidx)
+            {
+                let outgoing: Vec<_> = self
+                    .edges_directed_in_order_of_creation(remote_sidx, Direction::Outgoing)
+                    .into_iter()
+                    .map(EdgeOwned::from)
+                    .collect();
+                let remote_is_connected_to_local =
+                    outgoing.iter().any(|e| e.target.id() == local_sidx);
+                if !remote_is_connected_to_local
+                    && let Some(edge) = outgoing.iter().find(|e| {
+                        outgoing.len() == 1 || e.target.id() == owner_of_commit_same_as_remote
+                    })
+                {
+                    self.inner.remove_edge(edge.id);
+                    self.inner.add_edge(
+                        remote_sidx,
+                        local_sidx,
+                        edge.weight
+                            .adjusted_for(remote_sidx, local_sidx, &self.inner),
+                    );
+                }
+            }
         }
         Ok(())
     }

crates/but-graph/src/lib.rs

@@ -280,5 +280,32 @@ pub struct Edge {
     dst_id: Option<gix::ObjectId>,
 }
 
+impl Edge {
+    /// Useful when reusing an edge to assure it doesn't list commits that don't exist in `src_idx` and `dst_idx` anymore.
+    pub(crate) fn adjusted_for(
+        mut self,
+        src_sidx: SegmentIndex,
+        dst_sidx: SegmentIndex,
+        graph: &init::PetGraph,
+    ) -> Self {
+        let commits = &graph[src_sidx].commits;
+        let (id, idx) = commits
+            .last()
+            .map(|c| (Some(c.id), Some(commits.len() - 1)))
+            .unwrap_or_default();
+        self.src_id = id;
+        self.src = idx;
+
+        let commits = &graph[dst_sidx].commits;
+        let (id, idx) = commits
+            .first()
+            .map(|c| (Some(c.id), Some(0)))
+            .unwrap_or_default();
+        self.dst_id = id;
+        self.dst = idx;
+
+        self
+    }
+}
 /// An index into the [`Graph`].
 pub type SegmentIndex = petgraph::graph::NodeIndex;

crates/but-graph/src/projection/workspace.rs

@@ -789,7 +789,7 @@ impl Graph {
         out
     }
 
-    /// Return `(commit, start)` if `start` has a commit, or find the first commit downstream along the first parent.
+    /// Return `(commit, owner_sidx_of_commit)` if `start` has a commit, or find the first commit downstream along the first parent.
     pub(crate) fn first_commit_or_find_along_first_parent(
         &self,
         start: SegmentIndex,

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

@@ -1151,5 +1151,17 @@ EOF
     commit A3
     create_workspace_commit_once A
   )
+
+
+  git init no-ws-ref-no-ws-commit-two-branches
+  (cd no-ws-ref-no-ws-commit-two-branches
+    commit M1
+    commit M2
+
+    git branch A
+    git branch B
+
+    create_workspace_commit_once A B
+  )
 )
 

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

@@ -1,3 +1,4 @@
+use but_core::RefMetadata;
 use but_core::ref_metadata::StackId;
 use but_graph::VirtualBranchesTomlMetadata;
 use but_graph::virtual_branches_legacy_types::{Stack, StackBranch, Target};
@@ -53,6 +54,18 @@ pub fn add_workspace(meta: &mut VirtualBranchesTomlMetadata) {
     );
 }
 
+pub fn remove_target(meta: &mut VirtualBranchesTomlMetadata) {
+    let mut ws_md = meta
+        .workspace(
+            "refs/heads/gitbutler/workspace"
+                .try_into()
+                .expect("statically known to be valid"),
+        )
+        .unwrap();
+    ws_md.target_ref = None;
+    meta.set_workspace(&ws_md).unwrap();
+}
+
 pub fn add_workspace_without_target(meta: &mut VirtualBranchesTomlMetadata) {
     add_workspace(meta);
     meta.data_mut().default_target = None;

crates/but-graph/tests/graph/init/with_workspace.rs

@@ -1,4 +1,6 @@
-use crate::init::utils::{add_workspace_without_target, standard_options_with_extra_target};
+use crate::init::utils::{
+    add_workspace_without_target, remove_target, standard_options_with_extra_target,
+};
 use crate::init::{StackState, add_stack_with_segments, add_workspace, id_at, id_by_rev};
 use crate::init::{read_only_in_memory_scenario, standard_options};
 use but_graph::Graph;
@@ -2423,7 +2425,7 @@ fn three_branches_one_advanced_ws_commit_advanced_fully_pushed_empty_dependant()
     ├── ►:1[0]:origin/main →:2:
     │   └── →:2: (main →:1:)
     └── ►:4[0]:origin/advanced-lane →:6:
-        └── →:5: (dependant)
+        └── →:6: (advanced-lane →:4:)
     ");
 
     // When putting the dependent branch on top as empty segment, the frozen state is retained.
@@ -3241,7 +3243,7 @@ fn dependent_branch_insertion() -> anyhow::Result<()> {
     ├── ►:1[0]:origin/main →:2:
     │   └── →:2: (main →:1:)
     └── ►:4[0]:origin/advanced-lane →:6:
-        └── →:5: (dependant)
+        └── →:6: (advanced-lane →:4:)
     ");
 
     // The dependant branch is empty and on top of the one with the remote
@@ -3960,7 +3962,7 @@ fn without_target_ref_or_managed_commit_ambiguous_with_remotes() -> anyhow::Resu
     ├── ►:2[0]:origin/A →:5:
     │   └── →:5: (A →:2:)
     └── ►:3[0]:origin/B →:0:
-        └── →:5: (A →:2:)
+        └── →:0: (B →:3:)
     ");
     insta::assert_snapshot!(graph_workspace(&graph.to_workspace()?), @r"
     📕🏘️⚠️:1:gitbutler/workspace <> ✓!
@@ -5144,6 +5146,105 @@ fn unapplied_branch_on_base() -> anyhow::Result<()> {
     Ok(())
 }
 
+#[test]
+fn unapplied_branch_on_base_no_target() -> anyhow::Result<()> {
+    let (repo, mut meta) = read_only_in_memory_scenario("ws/unapplied-branch-on-base")?;
+    insta::assert_snapshot!(visualize_commit_graph_all(&repo)?, @r"
+    * a26ae77 (HEAD -> gitbutler/workspace) GitButler Workspace Commit
+    * fafd9d0 (origin/main, unapplied, main) init
+    ");
+    add_workspace(&mut meta);
+    remove_target(&mut meta);
+
+    let graph = Graph::from_head(&repo, &*meta, standard_options())?.validated()?;
+    insta::assert_snapshot!(graph_tree(&graph), @r"
+    ├── 👉📕►►►:0[0]:gitbutler/workspace
+    │   └── ·a26ae77 (⌂|🏘|1)
+    │       └── ►:2[1]:main <> origin/main →:1:
+    │           └── ·fafd9d0 (⌂|🏘|11) ►unapplied
+    └── ►:1[0]:origin/main →:2:
+        └── →:2: (main →:1:)
+    ");
+
+    // the main branch is disambiguated by its remote reference.
+    insta::assert_snapshot!(graph_workspace(&graph.to_workspace()?), @r"
+    📕🏘️:0:gitbutler/workspace <> ✓!
+    └── ≡:2:main <> origin/main →:1:
+        └── :2:main <> origin/main →:1:
+            └── ❄️fafd9d0 (🏘️) ►unapplied
+    ");
+
+    // The 'unapplied' branch can be added on top of that, and we make clear we want `main` as well.
+    add_stack_with_segments(&mut meta, 1, "unapplied", StackState::InWorkspace, &[]);
+    add_stack_with_segments(&mut meta, 2, "main", StackState::InWorkspace, &[]);
+
+    let graph = Graph::from_head(&repo, &*meta, standard_options())?.validated()?;
+    insta::assert_snapshot!(graph_tree(&graph), @r"
+    ├── 👉📕►►►:0[0]:gitbutler/workspace
+    │   └── ·a26ae77 (⌂|🏘|1)
+    │       ├── 📙►:3[1]:unapplied
+    │       │   └── ►:2[2]:anon:
+    │       │       └── ·fafd9d0 (⌂|🏘|✓|11)
+    │       └── 📙►:4[1]:main <> origin/main →:1:
+    │           └── →:2:
+    └── ►:1[0]:origin/main →:4:
+        └── →:4: (main →:1:)
+    ");
+
+    insta::assert_snapshot!(graph_workspace(&graph.to_workspace()?), @r"
+    📕🏘️:0:gitbutler/workspace <> ✓refs/remotes/origin/main on fafd9d0
+    ├── ≡📙:4:main <> origin/main →:1: on fafd9d0
+    │   └── 📙:4:main <> origin/main →:1:
+    └── ≡📙:3:unapplied on fafd9d0
+        └── 📙:3:unapplied
+    ");
+
+    // We simulate an unapplied branch on the base by giving it branch metadata, but not listing
+    // it in the workspace.
+    add_stack_with_segments(&mut meta, 1, "unapplied", StackState::Inactive, &[]);
+    let graph = Graph::from_head(&repo, &*meta, standard_options())?.validated()?;
+
+    // Now only `main` shows up.
+    insta::assert_snapshot!(graph_workspace(&graph.to_workspace()?), @r"
+    📕🏘️:0:gitbutler/workspace <> ✓refs/remotes/origin/main on fafd9d0
+    └── ≡📙:3:main <> origin/main →:1: on fafd9d0
+        └── 📙:3:main <> origin/main →:1:
+    ");
+
+    Ok(())
+}
+
+#[test]
+fn no_ws_commit_two_branches_no_target() -> anyhow::Result<()> {
+    let (repo, mut meta) = read_only_in_memory_scenario("ws/no-ws-ref-no-ws-commit-two-branches")?;
+    insta::assert_snapshot!(visualize_commit_graph_all(&repo)?, @r"
+    * bce0c5e (HEAD -> gitbutler/workspace, main, B, A) M2
+    * 3183e43 M1
+    ");
+    remove_target(&mut meta);
+    add_stack_with_segments(&mut meta, 0, "main", StackState::InWorkspace, &[]);
+    add_stack_with_segments(&mut meta, 1, "A", StackState::InWorkspace, &[]);
+
+    let graph = Graph::from_head(&repo, &*meta, standard_options())?.validated()?;
+    insta::assert_snapshot!(graph_tree(&graph), @r"
+    └── 👉📕►►►:0[0]:gitbutler/workspace
+        ├── 📙►:2[1]:main
+        │   └── ►:1[2]:anon: →:3:
+        │       ├── ·bce0c5e (⌂|🏘|1) ►B
+        │       └── ·3183e43 (⌂|🏘|1)
+        └── 📙►:3[1]:A
+            └── →:1:
+    ");
+    insta::assert_snapshot!(graph_workspace(&graph.to_workspace()?), @r"
+    📕🏘️⚠️:0:gitbutler/workspace <> ✓! on bce0c5e
+    ├── ≡📙:3:A on bce0c5e
+    │   └── 📙:3:A
+    └── ≡📙:2:main on bce0c5e
+        └── 📙:2:main
+    ");
+    Ok(())
+}
+
 mod edit_commit {
     use crate::init::{add_workspace, id_at, read_only_in_memory_scenario, standard_options};
     use but_graph::Graph;