refactor(dag): unify definition of commit visibility

Previously, there were a lot of different, inconsistent ways that we checked to see if a commit was "visible". Most often, we would take the set of observed commits and subtract the set of obsolete commits. However, it's possible for a commit to be obsolete and visible, when it has a non-obsolete descendant commit.

This commit centralizes the definition of commit visibility and changes the APIs to make it easier to do the right thing.

git-branchless-lib/src/core/dag.rs

@@ -11,6 +11,7 @@ use eden_dag::ops::DagPersistent;
 use eden_dag::DagAlgorithm;
 use eyre::Context;
 use itertools::Itertools;
+use once_cell::sync::OnceCell;
 use tracing::{instrument, trace, warn};
 
 use crate::core::effects::{Effects, OperationType};
@@ -106,11 +107,16 @@ pub struct Dag {
 
     /// A set containing all commits that have been observed by the
     /// `EventReplayer`.
-    pub observed_commits: CommitSet,
+    observed_commits: CommitSet,
 
     /// A set containing all commits that have been determined to be obsolete by
     /// the `EventReplayer`.
-    pub obsolete_commits: CommitSet,
+    obsolete_commits: CommitSet,
+
+    public_commits: OnceCell<CommitSet>,
+    visible_commits: OnceCell<CommitSet>,
+    draft_commits: OnceCell<CommitSet>,
+    default_smartlog_commits: OnceCell<CommitSet>,
 }
 
 impl Dag {
@@ -187,6 +193,10 @@ impl Dag {
             branch_commits,
             observed_commits,
             obsolete_commits,
+            public_commits: Default::default(),
+            visible_commits: Default::default(),
+            draft_commits: Default::default(),
+            default_smartlog_commits: Default::default(),
         })
     }
 
@@ -362,36 +372,61 @@ impl Dag {
     }
 
     /// Return the set of commits which are public (checked into the main branch).
-    pub fn query_public_commits(&self) -> eyre::Result<CommitSet> {
-        let public_commits = self.query().ancestors(self.main_branch_commit.clone())?;
-        Ok(public_commits)
+    pub fn query_public_commits(&self) -> eyre::Result<&CommitSet> {
+        self.public_commits.get_or_try_init(|| {
+            let public_commits = self.query().ancestors(self.main_branch_commit.clone())?;
+            Ok(public_commits)
+        })
     }
 
-    /// Query the set of active heads. This includes the heads of the set of
-    /// visible commits, plus any other commits which would be rendered in the
-    /// smartlog.
-    ///
-    /// This query includes heads which may be ancestor of other commits in the
-    /// set. For example, if `HEAD` points to a commit which is the ancestor of
-    /// a visible commit, both commits are included in the resulting set. This
-    /// is so that they can be explicitly rendered in the smartlog. To get the
-    /// set of visible commits, simply query for the ancestors of the set
-    /// resulting from this function.
-    pub fn query_active_heads(
-        &self,
-        public_commits: &CommitSet,
-        observed_commits: &CommitSet,
-    ) -> eyre::Result<CommitSet> {
-        let active_commits = observed_commits.clone();
-        let active_heads = self.query().heads(active_commits)?;
-        let active_heads = active_heads.difference(public_commits);
-
-        let active_heads = active_heads
-            .union(&self.head_commit)
-            .union(&self.branch_commits)
-            .union(&self.main_branch_commit);
+    /// Determine the set of commits which are considered to be "visible". A
+    /// commit is "visible" if it is not obsolete or has a non-obsolete
+    /// descendant.
+    pub fn query_visible_commits(&self) -> eyre::Result<&CommitSet> {
+        self.visible_commits.get_or_try_init(|| {
+            let public_commits = self.query_public_commits()?;
+            let visible_heads = public_commits
+                .union(&self.observed_commits.difference(&self.obsolete_commits))
+                .union(&self.head_commit)
+                .union(&self.main_branch_commit)
+                .union(&self.branch_commits);
+            let visible_commits = self.query().ancestors(visible_heads)?;
+            Ok(visible_commits)
+        })
+    }
 
-        Ok(active_heads)
+    /// Determine the set of obsolete commits. These commits have been rewritten
+    /// or explicitly hidden by the user.
+    pub fn query_obsolete_commits(&self) -> CommitSet {
+        self.obsolete_commits.clone()
+    }
+
+    /// Determine the set of "draft" commits. The draft commits are all visible
+    /// commits which aren't public.
+    pub fn query_draft_commits(&self) -> eyre::Result<&CommitSet> {
+        self.draft_commits.get_or_try_init(|| {
+            let visible_commits = self.query_visible_commits()?;
+            let public_commits = self.query_public_commits()?;
+            Ok(visible_commits.difference(public_commits))
+        })
+    }
+
+    /// Determine the default set of commits that is shown in the smartlog when
+    /// no revset is passed.
+    pub fn query_default_smartlog_commits(&self) -> eyre::Result<&CommitSet> {
+        self.default_smartlog_commits.get_or_try_init(|| {
+            let public_commits = self.query_public_commits()?;
+            let active_commits = self.observed_commits.clone();
+            let active_heads = self.query().heads(active_commits)?;
+            let active_heads = active_heads.difference(public_commits);
+
+            let active_heads = active_heads
+                .union(&self.head_commit)
+                .union(&self.branch_commits)
+                .union(&self.main_branch_commit);
+
+            Ok(active_heads)
+        })
     }
 
     /// Find a path from the provided head to its merge-base with the main

git-branchless-lib/src/core/rewrite/evolve.rs

@@ -84,8 +84,8 @@ pub fn find_abandoned_children(
     };
 
     let children = dag.query().children(CommitSet::from(oid))?;
-    let children = children.intersection(draft_commits);
-    let non_obsolete_children = children.difference(&dag.obsolete_commits);
+    let children = children.intersection(dag.query_visible_commits()?);
+    let non_obsolete_children = children.difference(&dag.query_obsolete_commits());
     let non_obsolete_children_oids: Vec<NonZeroOid> = non_obsolete_children
         .iter()?
         .map(|x| -> eyre::Result<NonZeroOid> { NonZeroOid::try_from(x?) })

git-branchless-lib/src/core/rewrite/plan.rs

@@ -202,13 +202,9 @@ impl<'a> RebasePlanPermissions<'a> {
             }
         }
 
-        let allowed_commits = dag.query().range(
-            commits.clone(),
-            commits.union(&dag.query_active_heads(&public_commits, &dag.observed_commits)?),
-        )?;
         Ok(Ok(RebasePlanPermissions {
             build_options,
-            allowed_commits,
+            allowed_commits: commits,
         }))
     }
 
@@ -307,8 +303,8 @@ impl<'a> ConstraintGraph<'a> {
                         .dag
                         .query()
                         .children(CommitSet::from(*children_of_oid))?
-                        .difference(&self.dag.obsolete_commits)
-                        .difference(&commits_to_move);
+                        .difference(&commits_to_move)
+                        .intersection(self.dag.query_visible_commits()?);
 
                     for child_oid in commit_set_to_vec(&source_children)? {
                         self.inner.entry(parent_oid).or_default().insert(child_oid);
@@ -362,21 +358,13 @@ impl<'a> ConstraintGraph<'a> {
         let _effects = effects;
 
         let all_descendants_of_constrained_nodes = {
-            let public_commits = self.dag.query_public_commits()?;
-            let active_heads = self.dag.query_active_heads(
-                &public_commits,
-                &self
-                    .dag
-                    .observed_commits
-                    .difference(&self.dag.obsolete_commits),
-            )?;
-            let visible_commits = self.dag.query().ancestors(active_heads)?;
+            let visible_commits = self.dag.query_visible_commits()?;
 
             let mut acc = Vec::new();
             let parents = self.commits_to_move();
             progress.notify_progress(0, parents.len());
             for parent_oid in parents {
-                self.collect_descendants(&visible_commits, &mut acc, parent_oid)?;
+                self.collect_descendants(visible_commits, &mut acc, parent_oid)?;
                 progress.notify_progress_inc(1);
             }
             acc

git-branchless-lib/src/core/rewrite/rewrite_hooks.rs

@@ -9,7 +9,6 @@ use std::path::Path;
 use std::time::SystemTime;
 
 use console::style;
-use eden_dag::DagAlgorithm;
 use eyre::Context;
 use itertools::Itertools;
 use tempfile::NamedTempFile;
@@ -214,20 +213,15 @@ fn warn_abandoned(
         &references_snapshot,
     )?;
 
-    let public_commits = dag.query_public_commits()?;
-    let active_heads = dag.query_active_heads(
-        &public_commits,
-        &dag.observed_commits.difference(&dag.obsolete_commits),
-    )?;
-    let draft_commits = dag.query().range(public_commits, active_heads)?;
+    let draft_commits = dag.query_draft_commits()?;
 
     let (all_abandoned_children, all_abandoned_branches) = {
         let mut all_abandoned_children: HashSet<NonZeroOid> = HashSet::new();
         let mut all_abandoned_branches: HashSet<&str> = HashSet::new();
         for old_commit_oid in old_commit_oids {
             let abandoned_result = find_abandoned_children(
                 &dag,
-                &draft_commits,
+                draft_commits,
                 &event_replayer,
                 event_cursor,
                 old_commit_oid,

git-branchless/src/commands/amend.rs

@@ -14,7 +14,7 @@ use lib::util::ExitCode;
 use tracing::instrument;
 
 use crate::commands::restack;
-use crate::opts::{MoveOptions, Revset};
+use crate::opts::{MoveOptions, ResolveRevsetOptions, Revset};
 use lib::core::config::get_restack_preserve_timestamps;
 use lib::core::effects::Effects;
 use lib::core::eventlog::{Event, EventLogDb};
@@ -27,6 +27,7 @@ use lib::git::{AmendFastOptions, GitRunInfo, MaybeZeroOid, Repo, ResolvedReferen
 pub fn amend(
     effects: &Effects,
     git_run_info: &GitRunInfo,
+    resolve_revset_options: &ResolveRevsetOptions,
     move_options: &MoveOptions,
 ) -> eyre::Result<ExitCode> {
     let now = SystemTime::now();
@@ -147,6 +148,7 @@ pub fn amend(
         effects,
         git_run_info,
         vec![Revset(head_oid.to_string())],
+        resolve_revset_options,
         move_options,
         MergeConflictRemediation::Restack,
     )?;

git-branchless/src/commands/bug_report.rs

@@ -136,8 +136,7 @@ fn describe_event_cursor(
         dag,
         event_replayer,
         event_cursor,
-        &dag.observed_commits,
-        true,
+        dag.query_default_smartlog_commits()?,
     )?;
     let graph_lines = render_graph(
         &effects,

git-branchless/src/commands/hide.rs

@@ -18,7 +18,7 @@ use lib::core::formatting::{Glyphs, Pluralize};
 use lib::core::rewrite::move_branches;
 use lib::git::{CategorizedReferenceName, GitRunInfo, MaybeZeroOid, NonZeroOid, Repo};
 
-use crate::opts::Revset;
+use crate::opts::{ResolveRevsetOptions, Revset};
 use crate::revset::resolve_commits;
 
 /// Hide the hashes provided on the command-line.
@@ -27,6 +27,7 @@ pub fn hide(
     effects: &Effects,
     git_run_info: &GitRunInfo,
     revsets: Vec<Revset>,
+    resolve_revset_options: &ResolveRevsetOptions,
     delete_branches: bool,
     recursive: bool,
 ) -> eyre::Result<ExitCode> {
@@ -46,19 +47,20 @@ pub fn hide(
         &references_snapshot,
     )?;
 
-    let commit_sets = match resolve_commits(effects, &repo, &mut dag, &revsets) {
-        Ok(commit_sets) => commit_sets,
-        Err(err) => {
-            err.describe(effects)?;
-            return Ok(ExitCode(1));
-        }
-    };
+    let commit_sets =
+        match resolve_commits(effects, &repo, &mut dag, &revsets, resolve_revset_options) {
+            Ok(commit_sets) => commit_sets,
+            Err(err) => {
+                err.describe(effects)?;
+                return Ok(ExitCode(1));
+            }
+        };
 
     let commits = union_all(&commit_sets);
     let commits = if recursive {
         dag.query()
             .descendants(commits)?
-            .difference(&dag.obsolete_commits)
+            .intersection(dag.query_visible_commits()?)
     } else {
         commits
     };
@@ -181,7 +183,12 @@ pub fn hide(
 
 /// Unhide the hashes provided on the command-line.
 #[instrument]
-pub fn unhide(effects: &Effects, revsets: Vec<Revset>, recursive: bool) -> eyre::Result<ExitCode> {
+pub fn unhide(
+    effects: &Effects,
+    revsets: Vec<Revset>,
+    resolve_revset_options: &ResolveRevsetOptions,
+    recursive: bool,
+) -> eyre::Result<ExitCode> {
     let now = SystemTime::now();
     let glyphs = Glyphs::detect();
     let repo = Repo::from_current_dir()?;
@@ -198,19 +205,20 @@ pub fn unhide(effects: &Effects, revsets: Vec<Revset>, recursive: bool) -> eyre:
         &references_snapshot,
     )?;
 
-    let commit_sets = match resolve_commits(effects, &repo, &mut dag, &revsets) {
-        Ok(commit_sets) => commit_sets,
-        Err(err) => {
-            err.describe(effects)?;
-            return Ok(ExitCode(1));
-        }
-    };
+    let commit_sets =
+        match resolve_commits(effects, &repo, &mut dag, &revsets, resolve_revset_options) {
+            Ok(commit_sets) => commit_sets,
+            Err(err) => {
+                err.describe(effects)?;
+                return Ok(ExitCode(1));
+            }
+        };
 
     let commits = union_all(&commit_sets);
     let commits = if recursive {
         dag.query()
             .descendants(commits)?
-            .intersection(&dag.obsolete_commits)
+            .intersection(&dag.query_obsolete_commits())
     } else {
         commits
     };