Merge pull request #9468 from Byron/graph-segmentation

integration-checks (testing)

Author: Byron

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

@@ -473,12 +473,12 @@ impl Graph {
         (out, stopped_at)
     }
 
-    /// Visit the ancestry of `start` along the first parents, itself included, until `stop` returns `true`.
+    /// Visit the ancestry of `start` along the first parents, itself excluded, until `stop` returns `true`.
     /// Also return the segment that we stopped at.
     /// **Important**: `stop` is not called with `start`, this is a feature.
     ///
     /// Note that the traversal assumes as well-segmented graph without cycles.
-    fn visit_segments_along_first_parent_until(
+    fn visit_segments_downward_along_first_parent_until(
         &self,
         start: SegmentIndex,
         mut stop: impl FnMut(&Segment) -> bool,
@@ -507,7 +507,7 @@ impl Graph {
         mut find: impl FnMut(&Segment) -> Option<T>,
     ) -> Option<T> {
         let mut out = None;
-        self.visit_segments_along_first_parent_until(start, |s| {
+        self.visit_segments_downward_along_first_parent_until(start, |s| {
             if let Some(res) = find(s) {
                 out = Some(res);
                 true

crates/but-testing/src/command/mod.rs

@@ -466,7 +466,7 @@ pub fn operating_mode(args: &super::Args) -> anyhow::Result<()> {
 }
 
 pub fn ref_info(args: &super::Args, ref_name: Option<&str>, expensive: bool) -> anyhow::Result<()> {
-    let (repo, project) = repo_and_maybe_project(args, RepositoryOpenMode::General)?;
+    let (repo, project) = repo_and_maybe_project(args, RepositoryOpenMode::Merge)?;
     let opts = but_workspace::ref_info::Options {
         stack_commit_limit: 0,
         expensive_commit_info: expensive,

crates/but-workspace/src/changeset.rs

@@ -4,11 +4,6 @@
 //! This property allows changeset IDs to be used to determine if two different commits, or sets of commits,
 //! represent the same change.
 
-use std::{
-    borrow::Borrow,
-    collections::{HashMap, HashSet, hash_map::Entry},
-};
-
 use crate::{
     RefInfo,
     ref_info::{
@@ -18,9 +13,15 @@ use crate::{
     ui::PushStatus,
 };
 use bstr::BString;
-use but_core::{ChangeState, TreeChange, TreeStatus, commit::TreeKind};
+use but_core::{ChangeState, commit::TreeKind};
+use gix::diff::tree::recorder::Change;
 use gix::{ObjectId, Repository, object::tree::EntryKind, prelude::ObjectIdExt};
 use itertools::Itertools;
+use std::ops::Deref;
+use std::{
+    borrow::Borrow,
+    collections::{HashMap, HashSet, hash_map::Entry},
+};
 
 /// The ID of a changeset, calculated as Git hash for convenience.
 type ChangesetID = gix::ObjectId;
@@ -68,9 +69,12 @@ impl RefInfo {
             self.compute_pushstatus();
             return Ok(());
         };
+        let lower_bound_generation = self.lower_bound.map(|sidx| graph[sidx].generation);
         graph.visit_all_segments_until(target_tip, but_graph::petgraph::Direction::Outgoing, |s| {
             let prune = true;
-            if Some(s.id) == self.lower_bound {
+            if Some(s.id) == self.lower_bound
+                || lower_bound_generation.is_some_and(|generation| s.generation > generation)
+            {
                 return prune;
             }
             for c in &s.commits {
@@ -101,7 +105,17 @@ impl RefInfo {
                     // top-to-bottom
                     .commits
                     .iter_mut()
-                    .take_while(|c| c.relation == LocalCommitRelation::LocalOnly)
+                    .take_while(|c| {
+                        matches!(
+                            c.relation,
+                            // This happens when the identity match with the remote didn't work.
+                            LocalCommitRelation::LocalOnly |
+                            // This would be expected to be a remote-match by identity (we don't check for this),
+                            // something that is determined during graph traversal time. But we want ot see
+                            // if any of these is also integrated.
+                            LocalCommitRelation::LocalAndRemote(_)
+                        )
+                    })
                 {
                     let expensive = changeset_identifier(repo, expensive.then_some(local))?;
                     if let Some(upstream_commit_id) =
@@ -338,15 +352,19 @@ fn id_for_tree_diff(
     // TODO(perf): use plumbing directly to avoid resource-cache overhead.
     //             consider parallelization
     //             really needs caching to be practical, in-memory might suffice for now.
-    let changes = repo.diff_tree_to_tree(
-        lhs_tree.as_ref(),
-        &rhs_tree,
-        *gix::diff::Options::default()
-            .track_path()
-            // Rewrite tracking isn't needed for unique IDs and doesn't alter the validity,
-            // but would cost time, making it useless.
-            .track_rewrites(None),
+
+    let empty_tree = repo.empty_tree();
+    let mut state = Default::default();
+    let mut recorder = gix::diff::tree::Recorder::default()
+        .track_location(Some(gix::diff::tree::recorder::Location::Path));
+    gix::diff::tree(
+        gix::objs::TreeRefIter::from_bytes(&lhs_tree.unwrap_or(empty_tree).data),
+        gix::objs::TreeRefIter::from_bytes(&rhs_tree.data),
+        &mut state,
+        repo.objects.deref(),
+        &mut recorder,
     )?;
+    let changes = recorder.records;
     if changes.is_empty() {
         return Ok(None);
     }
@@ -356,41 +374,73 @@ fn id_for_tree_diff(
 
     // We rely on the diff order, it's consistent as rewrites are disabled.
     for c in changes {
-        if c.entry_mode().is_tree() {
+        let (entry_mode, location) = match &c {
+            Change::Addition {
+                entry_mode, path, ..
+            }
+            | Change::Deletion {
+                entry_mode, path, ..
+            }
+            | Change::Modification {
+                entry_mode, path, ..
+            } => (*entry_mode, path),
+        };
+        if entry_mode.is_tree() {
             continue;
         }
-        // For simplicity, use this type.
-        let c = TreeChange::from(c);
         // must hash all fields, even if None for unambiguous hashes.
-        hash.update(&c.path);
-        match c.status {
-            TreeStatus::Addition {
-                state,
-                // Ignore as untracked files can't happen with tree/tree diffs
-                is_untracked: _,
+        hash.update(location);
+        match c {
+            Change::Addition {
+                entry_mode, oid, ..
             } => {
                 hash.update(b"A");
-                hash_change_state(&mut hash, state)
+                hash_change_state(
+                    &mut hash,
+                    ChangeState {
+                        id: oid,
+                        kind: entry_mode.kind(),
+                    },
+                )
             }
-            TreeStatus::Deletion { previous_state } => {
+            Change::Deletion {
+                entry_mode, oid, ..
+            } => {
                 hash.update(b"D");
-                hash_change_state(&mut hash, previous_state)
+                hash_change_state(
+                    &mut hash,
+                    ChangeState {
+                        id: oid,
+                        kind: entry_mode.kind(),
+                    },
+                );
             }
-            TreeStatus::Modification {
-                previous_state,
-                state,
-                // Ignore as it's derived
-                flags: _,
+            Change::Modification {
+                previous_entry_mode,
+                previous_oid,
+                entry_mode,
+                oid,
+                ..
             } => {
                 hash.update(b"M");
-                hash_change_state(&mut hash, previous_state);
-                hash_change_state(&mut hash, state);
-            }
-            TreeStatus::Rename { .. } => {
-                unreachable!("disabled in prior configuration")
+                hash_change_state(
+                    &mut hash,
+                    ChangeState {
+                        id: previous_oid,
+                        kind: previous_entry_mode.kind(),
+                    },
+                );
+                hash_change_state(
+                    &mut hash,
+                    ChangeState {
+                        id: oid,
+                        kind: entry_mode.kind(),
+                    },
+                );
             }
         }
     }
+
     Ok(Some(hash.try_finalize()?))
 }
 

crates/but-workspace/src/ref_info.rs

@@ -160,13 +160,14 @@ pub mod ui {
         /// Convert this relation into something displaying, mainly for debugging.
         pub fn display(&self, id: gix::ObjectId) -> Cow<'static, str> {
             match self {
-                LocalCommitRelation::LocalOnly => "local".into(),
-                LocalCommitRelation::LocalAndRemote(remote_id) => if *remote_id == id {
-                    "local/remote(identity)"
-                } else {
-                    "local/remote(similarity)"
+                LocalCommitRelation::LocalOnly => Cow::Borrowed("local"),
+                LocalCommitRelation::LocalAndRemote(remote_id) => {
+                    if *remote_id == id {
+                        "local/remote(identity)".into()
+                    } else {
+                        format!("local/remote({})", remote_id.to_hex_with_len(7)).into()
+                    }
                 }
-                .into(),
                 LocalCommitRelation::Integrated(id) => {
                     format!("integrated({})", id.to_hex_with_len(7)).into()
                 }

crates/but-workspace/tests/fixtures/scenario/journey02.sh

@@ -2,7 +2,7 @@
 
 ### General Description
 
-# Various stages through a typical user journey. Ideally complete enough to test everything that matters to us.
+# Disjoint scenarios related to remote branches that are merged in.
 set -eu -o pipefail
 
 source "${BASH_SOURCE[0]%/*}/shared.sh"

crates/but-workspace/tests/fixtures/scenario/journey03.sh

@@ -0,0 +1,56 @@
+#!/usr/bin/env bash
+
+### General Description
+
+# Disjoint scenarios related to remote branches that have been rebased and merged.
+set -eu -o pipefail
+
+source "${BASH_SOURCE[0]%/*}/shared.sh"
+
+git init 01-one-rewritten-one-local-after-push
+(cd 01-one-rewritten-one-local-after-push
+  echo "two local commits pushed to a remote, then rebased onto target.
+
+The branch should then be considered integrated" >.git/description
+  commit init && setup_target_to_match_main
+  git checkout -b A
+    echo A-new >file && git add file && git commit -m A1
+    echo A-new >other && git add other && git commit -m A2
+    git rev-parse @ >.git/refs/remotes/origin/A
+
+  git checkout main
+    commit M1
+    git cherry-pick :/A1
+    git cherry-pick :/A2
+    echo M-change >>file && git commit -am M2
+    echo M-change >>other && git commit -am M3
+    git rev-parse @ >.git/refs/remotes/origin/main
+
+  git checkout A
+  create_workspace_commit_once A
+)
+
+git init 01-one-rewritten-one-local-after-push-author-date-change
+(cd 01-one-rewritten-one-local-after-push-author-date-change
+  echo "two local commits pushed to a remote, then rebased onto target, but with the author date adjusted.
+
+This prevents quick-checks to work." >.git/description
+  commit init && setup_target_to_match_main
+  git checkout -b A
+    echo A-new >file && git add file && git commit -m A1
+    echo A-new >other && git add other && git commit -m A2
+    git rev-parse @ >.git/refs/remotes/origin/A
+
+  git checkout main
+    commit M1
+    tick
+    git cherry-pick :/A1
+    tick
+    git cherry-pick :/A2
+    echo M-change >>file && git commit -am M2
+    echo M-change >>other && git commit -am M3
+    git rev-parse @ >.git/refs/remotes/origin/main
+
+  git checkout A
+  create_workspace_commit_once A
+)