Merge pull request #3310 from gitbutlerapp/reconcile-ownership-claims

refactor: extract ownership calculation for update

gitbutler-app/src/git/oid.rs

@@ -7,6 +7,12 @@ pub struct Oid {
     oid: git2::Oid,
 }
 
+impl Default for Oid {
+    fn default() -> Self {
+        git2::Oid::zero().into()
+    }
+}
+
 impl Serialize for Oid {
     fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
     where

gitbutler-app/src/virtual_branches/branch.rs

@@ -6,6 +6,7 @@ mod writer;
 
 pub use file_ownership::OwnershipClaim;
 pub use hunk::Hunk;
+pub use ownership::reconcile_claims;
 pub use ownership::BranchOwnershipClaims;
 pub use reader::BranchReader as Reader;
 pub use writer::BranchWriter as Writer;
@@ -22,7 +23,7 @@ pub type BranchId = Id<Branch>;
 // store. it is more or less equivalent to a git branch reference, but it is not
 // stored or accessible from the git repository itself. it is stored in our
 // session storage under the branches/ directory.
-#[derive(Serialize, Deserialize, Debug, PartialEq, Clone)]
+#[derive(Serialize, Deserialize, Debug, PartialEq, Clone, Default)]
 pub struct Branch {
     pub id: BranchId,
     pub name: String,

gitbutler-app/src/virtual_branches/branch/ownership.rs

@@ -1,8 +1,9 @@
-use std::{fmt, str::FromStr};
+use std::{collections::HashSet, fmt, str::FromStr};
 
 use serde::{Deserialize, Serialize, Serializer};
 
-use super::OwnershipClaim;
+use super::{Branch, OwnershipClaim};
+use anyhow::Result;
 
 #[derive(Debug, Clone, Default, PartialEq, Eq)]
 pub struct BranchOwnershipClaims {
@@ -120,12 +121,177 @@ impl BranchOwnershipClaims {
     }
 }
 
+#[derive(Debug, Clone)]
+pub struct ClaimOutcome {
+    pub updated_branch: Branch,
+    pub removed_claims: Vec<OwnershipClaim>,
+}
+pub fn reconcile_claims(
+    all_branches: Vec<Branch>,
+    claiming_branch: &Branch,
+    new_claims: &Vec<OwnershipClaim>,
+) -> Result<Vec<ClaimOutcome>> {
+    let mut other_branches = all_branches
+        .into_iter()
+        .filter(|branch| branch.applied)
+        .filter(|branch| branch.id != claiming_branch.id)
+        .collect::<Vec<_>>();
+
+    let mut claim_outcomes: Vec<ClaimOutcome> = Vec::new();
+
+    for file_ownership in new_claims {
+        for branch in &mut other_branches {
+            let taken = branch.ownership.take(file_ownership);
+            claim_outcomes.push(ClaimOutcome {
+                updated_branch: branch.clone(),
+                removed_claims: taken,
+            });
+        }
+    }
+
+    // Add the claiming branch to the list of outcomes
+    claim_outcomes.push(ClaimOutcome {
+        updated_branch: Branch {
+            ownership: BranchOwnershipClaims {
+                claims: new_claims.clone(),
+            },
+            ..claiming_branch.clone()
+        },
+        removed_claims: Vec::new(),
+    });
+
+    // Check the outcomes consistency and error out if they would result in a hunk being claimed by multiple branches
+    let mut seen = HashSet::new();
+    for outcome in claim_outcomes.clone() {
+        for claim in outcome.updated_branch.ownership.claims {
+            for hunk in claim.hunks {
+                if !seen.insert(format!(
+                    "{}-{}-{}",
+                    claim.file_path.to_str().unwrap_or_default(),
+                    hunk.start,
+                    hunk.end
+                )) {
+                    return Err(anyhow::anyhow!("inconsistent ownership claims"));
+                }
+            }
+        }
+    }
+
+    Ok(claim_outcomes)
+}
+
 #[cfg(test)]
 mod tests {
-    use std::vec;
+    use std::{path::PathBuf, vec};
+
+    use crate::virtual_branches::branch::Hunk;
 
     use super::*;
 
+    #[test]
+    fn test_reconcile_ownership_simple() {
+        let branch_a = Branch {
+            name: "a".to_string(),
+            ownership: BranchOwnershipClaims {
+                claims: vec![OwnershipClaim {
+                    file_path: PathBuf::from("foo"),
+                    hunks: vec![
+                        Hunk {
+                            start: 1,
+                            end: 3,
+                            hash: Some("1,3".to_string()),
+                            timestamp_ms: None,
+                        },
+                        Hunk {
+                            start: 4,
+                            end: 6,
+                            hash: Some("4,6".to_string()),
+                            timestamp_ms: None,
+                        },
+                    ],
+                }],
+            },
+            applied: true,
+            ..Default::default()
+        };
+        let branch_b = Branch {
+            name: "b".to_string(),
+            ownership: BranchOwnershipClaims {
+                claims: vec![OwnershipClaim {
+                    file_path: PathBuf::from("foo"),
+                    hunks: vec![Hunk {
+                        start: 7,
+                        end: 9,
+                        hash: Some("7,9".to_string()),
+                        timestamp_ms: None,
+                    }],
+                }],
+            },
+            applied: true,
+            ..Default::default()
+        };
+        let all_branches: Vec<Branch> = vec![branch_a.clone(), branch_b.clone()];
+        let claim: Vec<OwnershipClaim> = vec![OwnershipClaim {
+            file_path: PathBuf::from("foo"),
+            hunks: vec![
+                Hunk {
+                    start: 4,
+                    end: 6,
+                    hash: Some("4,6".to_string()),
+                    timestamp_ms: None,
+                },
+                Hunk {
+                    start: 7,
+                    end: 9,
+                    hash: Some("9,7".to_string()),
+                    timestamp_ms: None,
+                },
+            ],
+        }];
+        let claim_outcomes = reconcile_claims(all_branches.clone(), &branch_b, &claim).unwrap();
+        assert_eq!(claim_outcomes.len(), all_branches.len());
+        assert_eq!(claim_outcomes[0].updated_branch.id, branch_a.id);
+        assert_eq!(claim_outcomes[1].updated_branch.id, branch_b.id);
+
+        assert_eq!(
+            claim_outcomes[0].updated_branch.ownership,
+            BranchOwnershipClaims {
+                claims: vec![OwnershipClaim {
+                    file_path: PathBuf::from("foo"),
+                    hunks: vec![Hunk {
+                        start: 1,
+                        end: 3,
+                        hash: Some("1,3".to_string()),
+                        timestamp_ms: None,
+                    },],
+                }],
+            }
+        );
+
+        assert_eq!(
+            claim_outcomes[1].updated_branch.ownership,
+            BranchOwnershipClaims {
+                claims: vec![OwnershipClaim {
+                    file_path: PathBuf::from("foo"),
+                    hunks: vec![
+                        Hunk {
+                            start: 4,
+                            end: 6,
+                            hash: Some("4,6".to_string()),
+                            timestamp_ms: None,
+                        },
+                        Hunk {
+                            start: 7,
+                            end: 9,
+                            hash: Some("9,7".to_string()),
+                            timestamp_ms: None,
+                        },
+                    ],
+                }],
+            }
+        );
+    }
+
     #[test]
     fn test_ownership() {
         let ownership = "src/main.rs:0-100\nsrc/main2.rs:200-300".parse::<BranchOwnershipClaims>();

gitbutler-app/src/virtual_branches/virtual.rs

@@ -1,5 +1,5 @@
 use std::{
-    collections::{HashMap, HashSet},
+    collections::HashMap,
     hash::Hash,
     path::{Path, PathBuf},
     time, vec,
@@ -1791,48 +1791,23 @@ fn set_ownership(
         return Ok(());
     }
 
-    let mut virtual_branches = Iterator::new(session_reader)
+    let virtual_branches = Iterator::new(session_reader)
         .context("failed to create branch iterator")?
         .collect::<Result<Vec<branch::Branch>, reader::Error>>()
-        .context("failed to read virtual branches")?
-        .into_iter()
-        .filter(|branch| branch.applied)
-        .filter(|branch| branch.id != target_branch.id)
-        .collect::<Vec<_>>();
+        .context("failed to read virtual branches")?;
 
-    let mut branch_claims: Vec<(branch::Branch, Vec<OwnershipClaim>)> = Vec::new();
-    for file_ownership in &ownership.claims {
-        for branch in &mut virtual_branches {
-            let taken = branch.ownership.take(file_ownership);
-            branch_claims.push((branch.clone(), taken));
-        }
-    }
-
-    // Check consistency and reject changes if they would result in a hunk being claimed by multiple branches
-    let mut seen = HashSet::new();
-    for (_, claims) in branch_claims.clone() {
-        for claim in claims {
-            for hunk in claim.hunks {
-                if !seen.insert(format!(
-                    "{}-{}-{}",
-                    claim.file_path.to_str().unwrap_or_default(),
-                    hunk.start,
-                    hunk.end
-                )) {
-                    return Err(anyhow::anyhow!("inconsistent ownership claims"));
-                }
-            }
-        }
-    }
-
-    for (branch, taken) in &mut branch_claims {
-        if !taken.is_empty() {
+    let mut claim_outcomes =
+        branch::reconcile_claims(virtual_branches, target_branch, &ownership.claims)?;
+    for claim_outcome in &mut claim_outcomes {
+        if !claim_outcome.removed_claims.is_empty() {
             branch_writer
-                .write(branch)
+                .write(&mut claim_outcome.updated_branch)
                 .context("failed to write ownership for branch".to_string())?;
         }
     }
 
+    // Updates the claiming branch that was passed as mutable state with the new ownership claims
+    // TODO: remove mutable reference to target_branch
     target_branch.ownership = ownership.clone();
 
     Ok(())