diff --git a/app/src/lib/vbranches/tooltip.ts b/app/src/lib/vbranches/tooltip.ts index 605e5e8a2..1ebacae7c 100644 --- a/app/src/lib/vbranches/tooltip.ts +++ b/app/src/lib/vbranches/tooltip.ts @@ -1,15 +1,17 @@ +import { HunkLock, type Commit } from './types'; import { unique } from '$lib/utils/filters'; -import type { Commit } from './types'; -export function getLockText(commitId: string[] | string, commits: Commit[]): string { - if (!commitId || commits === undefined) return 'Depends on a committed change'; +export function getLockText(hunkLocks: HunkLock | HunkLock[] | string, commits: Commit[]): string { + if (!hunkLocks || commits === undefined) return 'Depends on a committed change'; - const lockedIds = typeof commitId == 'string' ? [commitId] : (commitId as string[]); + const locks = hunkLocks instanceof HunkLock ? [hunkLocks] : (hunkLocks as HunkLock[]); - const descriptions = lockedIds + const descriptions = locks .filter(unique) - .map((id) => { - const commit = commits.find((commit) => commit.id == id); + .map((lock) => { + const commit = commits.find((commit) => { + commit.id == lock.commitId; + }); const shortCommitId = commit?.id.slice(0, 7); if (commit) { const shortTitle = commit.descriptionTitle?.slice(0, 35) + '...'; @@ -19,5 +21,13 @@ export function getLockText(commitId: string[] | string, commits: Commit[]): str } }) .join('\n'); - return 'Locked due to dependency on:\n' + descriptions; + const branchCount = locks.map((lock) => lock.branchId).filter(unique).length; + if (branchCount > 1) { + return ( + 'Warning, undefined behavior due to lock on multiple branches!\n\n' + + 'Locked because changes depend on:\n' + + descriptions + ); + } + return 'Locked because changes depend on:\n' + descriptions; } diff --git a/app/src/lib/vbranches/types.ts b/app/src/lib/vbranches/types.ts index fb545399b..070eedbc2 100644 --- a/app/src/lib/vbranches/types.ts +++ b/app/src/lib/vbranches/types.ts @@ -22,10 +22,16 @@ export class Hunk { filePath!: string; hash?: string; locked!: boolean; - lockedTo!: string[] | undefined; + @Type(() => HunkLock) + lockedTo!: HunkLock[]; changeType!: ChangeType; } +export class HunkLock { + branchId!: string; + commitId!: string; +} + export type AnyFile = LocalFile | RemoteFile; export class LocalFile { @@ -63,7 +69,7 @@ export class LocalFile { : false; } - get lockedIds(): string[] { + get lockedIds(): HunkLock[] { return this.hunks .flatMap((hunk) => hunk.lockedTo) .filter(notNull) @@ -252,7 +258,7 @@ export class RemoteFile { return this.hunks.map((h) => h.id); } - get lockedIds(): string[] { + get lockedIds(): HunkLock[] { return []; } diff --git a/crates/gitbutler-core/src/git/diff.rs b/crates/gitbutler-core/src/git/diff.rs index 7dca0de77..40f9d36da 100644 --- a/crates/gitbutler-core/src/git/diff.rs +++ b/crates/gitbutler-core/src/git/diff.rs @@ -53,7 +53,7 @@ pub struct GitHunk { #[serde(rename = "diff", serialize_with = "crate::serde::as_string_lossy")] pub diff_lines: BString, pub binary: bool, - pub locked_to: Box<[git::Oid]>, + pub locked_to: Box<[HunkLock]>, pub change_type: ChangeType, } @@ -95,12 +95,22 @@ impl GitHunk { self.new_start <= line && self.new_start + self.new_lines >= line } - pub fn with_locks(mut self, locks: &[git::Oid]) -> Self { + pub fn with_locks(mut self, locks: &[HunkLock]) -> Self { self.locked_to = locks.to_owned().into(); self } } +// A hunk is locked when it depends on changes in commits that are in your +// workspace. A hunk can be locked to more than one branch if it overlaps +// with more than one committed hunk. +#[derive(Debug, PartialEq, Eq, Clone, Serialize, Copy)] +#[serde(rename_all = "camelCase")] +pub struct HunkLock { + pub branch_id: uuid::Uuid, + pub commit_id: git::Oid, +} + #[derive(Debug, PartialEq, Clone, Serialize, Default)] #[serde(rename_all = "camelCase")] pub struct FileDiff { diff --git a/crates/gitbutler-core/src/virtual_branches/branch/hunk.rs b/crates/gitbutler-core/src/virtual_branches/branch/hunk.rs index c818f7eb9..eb56576c5 100644 --- a/crates/gitbutler-core/src/virtual_branches/branch/hunk.rs +++ b/crates/gitbutler-core/src/virtual_branches/branch/hunk.rs @@ -3,7 +3,7 @@ use std::{fmt::Display, ops::RangeInclusive, str::FromStr}; use anyhow::{anyhow, Context, Result}; use bstr::{BStr, ByteSlice}; -use crate::git::{self, diff}; +use crate::git::diff; pub type HunkHash = md5::Digest; @@ -13,7 +13,7 @@ pub struct Hunk { pub timestamp_ms: Option, pub start: u32, pub end: u32, - pub locked_to: Vec, + pub locked_to: Vec, } impl From<&diff::GitHunk> for Hunk { diff --git a/crates/gitbutler-core/src/virtual_branches/virtual.rs b/crates/gitbutler-core/src/virtual_branches/virtual.rs index ccf1606a7..367a6efa6 100644 --- a/crates/gitbutler-core/src/virtual_branches/virtual.rs +++ b/crates/gitbutler-core/src/virtual_branches/virtual.rs @@ -24,6 +24,7 @@ use super::{ branch_to_remote_branch, errors, target, RemoteBranch, VirtualBranchesHandle, }; use crate::git::diff::{diff_files_into_hunks, trees, DiffByPathMap}; +use crate::id::Id; use crate::virtual_branches::branch::HunkHash; use crate::{ askpass::AskpassBroker, @@ -143,7 +144,7 @@ pub struct VirtualBranchHunk { pub end: u32, pub binary: bool, pub locked: bool, - pub locked_to: Option>, + pub locked_to: Option>, pub change_type: diff::ChangeType, } @@ -1734,13 +1735,20 @@ fn get_applied_status( ]; } + let mut commit_to_branch = HashMap::new(); + for branch in &mut virtual_branches { + for commit in project_repository.log(branch.head, LogUntil::Commit(*target_sha))? { + commit_to_branch.insert(commit.id(), branch.id); + } + } + let mut diffs_by_branch: HashMap = virtual_branches .iter() .map(|branch| (branch.id, HashMap::new())) .collect(); let mut mtimes = MTimeCache::default(); - let mut locked_hunk_map = HashMap::>::new(); + let mut locked_hunk_map = HashMap::>::new(); let merge_base = project_repository .git_repository @@ -1783,15 +1791,22 @@ fn get_applied_status( if let Ok(blame) = blame { for blame_hunk in blame.iter() { - let commit = git::Oid::from(blame_hunk.orig_commit_id()); - if commit != *target_sha && commit != *integration_commit { + let commit_id = git::Oid::from(blame_hunk.orig_commit_id()); + if commit_id != *target_sha && commit_id != *integration_commit { let hash = Hunk::hash_diff(hunk.diff_lines.as_ref()); + let branch_id = uuid::Uuid::parse_str( + &commit_to_branch.get(&commit_id).unwrap().to_string(), + )?; + let hunk_lock = diff::HunkLock { + branch_id, + commit_id, + }; locked_hunk_map .entry(hash) - .and_modify(|commits| { - commits.push(commit); + .and_modify(|locks| { + locks.push(hunk_lock); }) - .or_insert(vec![commit]); + .or_insert(vec![hunk_lock]); } } } @@ -1799,13 +1814,6 @@ fn get_applied_status( } } - let mut commit_to_branch = HashMap::new(); - for branch in &mut virtual_branches { - for commit in project_repository.log(branch.head, LogUntil::Commit(*target_sha))? { - commit_to_branch.insert(commit.id(), branch.id); - } - } - for branch in &mut virtual_branches { if !branch.applied { bail!("branch {} is not applied", branch.name); @@ -1902,9 +1910,11 @@ fn get_applied_status( let hash = Hunk::hash_diff(hunk.diff_lines.as_ref()); let locked_to = locked_hunk_map.get(&hash); - let vbranch_pos = if let Some(locked_to) = locked_to { - let branch_id = commit_to_branch.get(&locked_to[0]).unwrap(); - let p = virtual_branches.iter().position(|vb| vb.id == *branch_id); + let vbranch_pos = if let Some(locks) = locked_to { + let first_lock = &locks[0]; + let p = virtual_branches + .iter() + .position(|vb| vb.id == Id::::from(first_lock.branch_id)); match p { Some(p) => p, _ => default_vbranch_pos, diff --git a/crates/gitbutler-core/tests/suite/virtual_branches/create_commit.rs b/crates/gitbutler-core/tests/suite/virtual_branches/create_commit.rs index 2bdc46b70..82580aa8d 100644 --- a/crates/gitbutler-core/tests/suite/virtual_branches/create_commit.rs +++ b/crates/gitbutler-core/tests/suite/virtual_branches/create_commit.rs @@ -2,6 +2,7 @@ use gitbutler_core::{ id::Id, virtual_branches::{Branch, VirtualBranch}, }; +use uuid::Uuid; use super::*; @@ -275,8 +276,8 @@ async fn should_double_lock() { let locks = &branch.files[0].hunks[0].locked_to.clone().unwrap(); assert_eq!(locks.len(), 2); - assert_eq!(locks[0], commit_1); - assert_eq!(locks[1], commit_2); + assert_eq!(locks[0].commit_id, commit_1); + assert_eq!(locks[1].commit_id, commit_2); } // This test only validates that locks are detected across virtual branches, it does @@ -311,7 +312,11 @@ async fn should_double_lock_across_branches() { .await .unwrap(); - controller + // TODO(mg): Make `create_commit` clean up ownership of committed hunks. + // TODO(mg): Needed because next hunk overlaps with previous ownership. + get_virtual_branch(controller, project_id, branch_1_id).await; + + let branch_2_id = controller .create_virtual_branch( project_id, &branch::BranchCreateRequest { @@ -326,19 +331,26 @@ async fn should_double_lock_across_branches() { write_file(repository, "file.txt", &lines); let commit_2 = controller - .create_commit(project_id, &branch_1_id, "commit 2", None, false) + .create_commit(project_id, &branch_2_id, "commit 2", None, false) .await .unwrap(); lines[3] = "change3".to_string(); write_file(repository, "file.txt", &lines); - let branch = get_virtual_branch(controller, project_id, branch_1_id).await; - let locks = &branch.files[0].hunks[0].locked_to.clone().unwrap(); - + let branch_1 = get_virtual_branch(controller, project_id, branch_1_id).await; + let locks = &branch_1.files[0].hunks[0].locked_to.clone().unwrap(); assert_eq!(locks.len(), 2); - assert_eq!(locks[0], commit_1); - assert_eq!(locks[1], commit_2); + assert_eq!(locks[0].commit_id, commit_1); + assert_eq!(locks[1].commit_id, commit_2); + assert_eq!( + locks[0].branch_id, + Uuid::parse_str(&branch_1_id.to_string()).unwrap() + ); + assert_eq!( + locks[1].branch_id, + Uuid::parse_str(&branch_2_id.to_string()).unwrap() + ); } fn write_file(repository: &TestProject, path: &str, lines: &[String]) {