Merge pull request #9450 from Byron/graph-segmentation

integration-checks (testing)

Author: Byron

crates/but-workspace/src/changeset.rs

@@ -148,12 +148,13 @@ impl RefInfo {
                 else {
                     continue;
                 };
-                let identity_of_tip_to_base = Identifier::ChangesetId(id_for_tree_diff(
-                    repo,
-                    base_commit_id,
-                    topmost_unintegrated_commit.tree_id,
-                )?);
+                let Some(changeset_id) =
+                    id_for_tree_diff(repo, base_commit_id, topmost_unintegrated_commit.tree_id)?
+                else {
+                    continue;
+                };
 
+                let identity_of_tip_to_base = Identifier::ChangesetId(changeset_id);
                 let Some(squashed_commit_id) = upstream_lut.get(&identity_of_tip_to_base).cloned()
                 else {
                     continue;
@@ -209,15 +210,17 @@ impl PushStatus {
         let first_commit = commits.first();
         let everything_integrated_locally =
             first_commit.is_some_and(|c| matches!(c.relation, LocalCommitRelation::Integrated(_)));
+        let first_commit_is_local =
+            first_commit.is_some_and(|c| matches!(c.relation, LocalCommitRelation::LocalOnly));
         if everything_integrated_locally {
             PushStatus::Integrated
-        } else if commits
-            .iter()
-            .any(|c| matches!(c.relation, LocalCommitRelation::LocalAndRemote(id) if c.id != id))
-        {
+        } else if commits.iter().any(|c| {
+            matches!(c.relation, LocalCommitRelation::LocalAndRemote(id) if c.id != id)
+                || (first_commit_is_local
+                    && matches!(c.relation, LocalCommitRelation::Integrated(_)))
+        }) {
             PushStatus::UnpushedCommitsRequiringForce
-        } else if first_commit.is_some_and(|c| matches!(c.relation, LocalCommitRelation::LocalOnly))
-        {
+        } else if first_commit_is_local {
             if remote_has_commits {
                 PushStatus::UnpushedCommitsRequiringForce
             } else {
@@ -236,11 +239,10 @@ fn changeset_identifier(
     let Some(commit) = commit else {
         return Ok(None);
     };
-    Ok(Some(Identifier::ChangesetId(id_for_tree_diff(
-        repo,
-        commit.parent_ids.first().cloned(),
-        commit.id,
-    )?)))
+    Ok(
+        id_for_tree_diff(repo, commit.parent_ids.first().cloned(), commit.id)?
+            .map(Identifier::ChangesetId),
+    )
 }
 
 fn lookup_similar<'a>(
@@ -276,6 +278,11 @@ fn create_similarity_lut(
             }
             match similarity_lut.entry(k) {
                 Entry::Occupied(ambiguous) => {
+                    if matches!(ambiguous.key(), Identifier::ChangesetId(_)) {
+                        // the most expensive option should never be ambiguous (which can happen with merges),
+                        // so just keep the (typically top-most/first) commit with a changeset ID instead.
+                        return;
+                    }
                     ambiguous_commits.insert(ambiguous.key().clone());
                     ambiguous.remove();
                 }
@@ -297,18 +304,12 @@ fn create_similarity_lut(
                 commit.id,
             );
             if expensive {
-                // Just like with rename-tracking, let's not use 'empty' commits as specific unique value.
-                if commit.tree_id.is_empty_tree() {
+                let Some(changeset_id) =
+                    id_for_tree_diff(repo, commit.parent_ids.first().cloned(), commit.id)?
+                else {
                     continue;
-                }
-                insert_or_expell_ambiguous(
-                    Identifier::ChangesetId(id_for_tree_diff(
-                        repo,
-                        commit.parent_ids.first().cloned(),
-                        commit.id,
-                    )?),
-                    commit.id,
-                );
+                };
+                insert_or_expell_ambiguous(Identifier::ChangesetId(changeset_id), commit.id);
             }
         }
     }
@@ -322,10 +323,21 @@ fn id_for_tree_diff(
     repo: &gix::Repository,
     lhs: Option<gix::ObjectId>,
     rhs: gix::ObjectId,
-) -> anyhow::Result<ChangesetID> {
+) -> anyhow::Result<Option<ChangesetID>> {
     let lhs_tree = lhs.map(|id| id_to_tree(repo, id)).transpose()?;
     let rhs_tree = id_to_tree(repo, rhs)?;
 
+    let no_changes = lhs_tree
+        .as_ref()
+        .map_or(rhs_tree.id.is_empty_tree(), |lhs_tree| {
+            lhs_tree.id == rhs_tree.id
+        });
+    if no_changes {
+        return Ok(None);
+    }
+    // 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,
@@ -335,6 +347,9 @@ fn id_for_tree_diff(
             // but would cost time, making it useless.
             .track_rewrites(None),
     )?;
+    if changes.is_empty() {
+        return Ok(None);
+    }
 
     let mut hash = gix::hash::hasher(gix::hash::Kind::Sha1);
     hash.update(&[CURRENT_VERSION as u8]);
@@ -376,7 +391,7 @@ fn id_for_tree_diff(
             }
         }
     }
-    Ok(hash.try_finalize()?)
+    Ok(Some(hash.try_finalize()?))
 }
 
 // TODO: use `peel_to_tree()` once special conflict markers aren't needed anymore.

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

@@ -16,7 +16,7 @@ We change the name of the first commit and also need the similarity to be detect
   git checkout -b A
     git branch soon-A-remote
     echo A-new >file && git add file && git commit -m A1
-    commit A2
+    echo A-new >other && git add other && git commit -m A2
   git checkout soon-A-remote
     echo A-new >file && git add file && git commit -m "A1 (same but different)"
     setup_remote_tracking soon-A-remote A "move"
@@ -25,6 +25,16 @@ We change the name of the first commit and also need the similarity to be detect
   create_workspace_commit_once A
 )
 
+cp -R 01-one-rewritten-one-local-after-push 01.5-one-rewritten-one-local-after-push-merge
+(cd 01.5-one-rewritten-one-local-after-push-merge
+  echo "On the remote, a rewritten/rebased commit we have locally is merged back into target." >.git/description
+  git checkout -b soon-main-remote origin/main
+    git merge --no-ff origin/A -m "merge origin/A"
+    setup_remote_tracking soon-main-remote main "move"
+
+  git checkout gitbutler/workspace
+)
+
 git init 02-diverged-remote
 (cd 02-diverged-remote
   echo "A setup that demands for a force-push
@@ -43,6 +53,18 @@ The tip of the local branch isn't in the ancestry of the remote anymore." >.git/
   create_workspace_commit_once A
 )
 
+cp -R 02-diverged-remote 02.5-diverged-remote-merge
+(cd 02.5-diverged-remote-merge
+  echo "A remote sharing a commit with a stack and its own commit gets merged.
+
+We'd not want to see the remote unique commit anymore as it's also considered integrated." >.git/description
+  git checkout main
+    git merge --no-ff origin/A
+    git rev-parse @ >.git/refs/remotes/origin/main
+
+  git checkout gitbutler/workspace
+)
+
 git init 03-remote-one-behind
 (cd 03-remote-one-behind
   echo "A can be pushed as it has local, unpushed commits" >.git/description
@@ -55,6 +77,16 @@ git init 03-remote-one-behind
   create_workspace_commit_once A
 )
 
+cp -R 03-remote-one-behind 03.5-remote-one-behind-merge-no-ff
+(cd 03.5-remote-one-behind-merge-no-ff
+  echo "Remote origin/A is merged back (with forceful merge commit) while there are still local commits." >.git/description
+  git checkout main
+    git merge --no-ff origin/A
+    git rev-parse @ >.git/refs/remotes/origin/main
+
+  git checkout gitbutler/workspace
+)
+
 git init 04-remote-one-ahead-ff
 (cd 04-remote-one-ahead-ff
   echo "There are no unpushed local commits, the remote is one ahead (FF)" >.git/description
@@ -68,3 +100,13 @@ git init 04-remote-one-ahead-ff
   git checkout A
   create_workspace_commit_once A
 )
+
+cp -R 04-remote-one-ahead-ff 04.5-remote-one-ahead-ff-merge
+(cd 04.5-remote-one-ahead-ff-merge
+  echo "Remote origin/A is merged back (fast-forward), bringing all into the target branch" >.git/description
+  git checkout main
+    git merge origin/A
+    git rev-parse @ >.git/refs/remotes/origin/main
+
+  git checkout gitbutler/workspace
+)

crates/but-workspace/tests/workspace/ref_info/with_workspace_commit/journey/exhaustive_with_squash_merges.rs

@@ -806,6 +806,8 @@ A new multi-segment stack is created without remote and squash merged locally.
     * fafd9d0 init
     ");
 
+    // TODO: if the user now puts another dependent branch, it's breaking down in many ways.
+    //       We should be smarter about that and flesh out additional steps on top.
     add_stack_with_segments(&mut meta, 0, "S1", StackState::InWorkspace, &[]);
     let info = but_workspace::head_info(&repo, &*meta, standard_options());
     insta::assert_debug_snapshot!(info, @r#"