From 55a2641b3287a7d169d06c3b31465a90b35c1606 Mon Sep 17 00:00:00 2001 From: Nikita Galaiko Date: Tue, 30 Jan 2024 11:31:33 +0100 Subject: [PATCH] fix going back to existing default target --- gitbutler-app/src/git/repository.rs | 12 +++++ gitbutler-app/src/virtual_branches/base.rs | 53 +++++++++++++--------- gitbutler-app/tests/common.rs | 30 ++++++++---- gitbutler-app/tests/virtual_branches.rs | 40 ++++++++++++++++ 4 files changed, 104 insertions(+), 31 deletions(-) diff --git a/gitbutler-app/src/git/repository.rs b/gitbutler-app/src/git/repository.rs index dedef26a0..e1770717d 100644 --- a/gitbutler-app/src/git/repository.rs +++ b/gitbutler-app/src/git/repository.rs @@ -101,6 +101,12 @@ impl Repository { .map_err(Into::into) } + pub fn is_descendant_of(&self, a: Oid, b: Oid) -> Result { + self.0 + .graph_descendant_of(a.into(), b.into()) + .map_err(Into::into) + } + pub fn merge_base(&self, one: Oid, two: Oid) -> Result { self.0 .merge_base(one.into(), two.into()) @@ -389,6 +395,12 @@ impl Repository { self.0.set_head(&refname.to_string()).map_err(Into::into) } + pub fn set_head_detached(&self, commitish: Oid) -> Result<()> { + self.0 + .set_head_detached(commitish.into()) + .map_err(Into::into) + } + pub fn reference( &self, name: &Refname, diff --git a/gitbutler-app/src/virtual_branches/base.rs b/gitbutler-app/src/virtual_branches/base.rs index e17060625..f62418578 100644 --- a/gitbutler-app/src/virtual_branches/base.rs +++ b/gitbutler-app/src/virtual_branches/base.rs @@ -67,22 +67,17 @@ pub fn set_base_branch( Err(error) => Err(errors::SetBaseBranchError::Other(error.into())), }?; - let remote_name = repo - .branch_remote_name(target_branch.refname().unwrap()) + let remote = repo + .find_remote(target_branch_ref.remote()) .context(format!( - "failed to get remote name for branch {}", + "failed to find remote for branch {}", target_branch.name().unwrap() ))?; - let remote = repo.find_remote(&remote_name).context(format!( - "failed to find remote {} for branch {}", - remote_name, - target_branch.name().unwrap() - ))?; let remote_url = remote .url() .context(format!( - "failed to get remote url for remote {}", - remote_name + "failed to get remote url for {}", + target_branch_ref.remote() ))? .unwrap(); @@ -91,23 +86,35 @@ pub fn set_base_branch( target_branch.name().unwrap() ))?; - let head_ref = repo.head().context("Failed to get HEAD reference")?; - let head_name: git::Refname = head_ref - .name() - .context("Failed to get HEAD reference name")?; - let head_commit = head_ref + let current_head = repo.head().context("Failed to get HEAD reference")?; + let current_head_commit = current_head .peel_to_commit() .context("Failed to peel HEAD reference to commit")?; // calculate the commit as the merge-base between HEAD in project_repository and this target commit let commit_oid = repo - .merge_base(head_commit.id(), target_branch_head.id()) + .merge_base(current_head_commit.id(), target_branch_head.id()) .context(format!( "Failed to calculate merge base between {} and {}", - head_commit.id(), + current_head_commit.id(), target_branch_head.id() ))?; + // if default target was already set, and the new target is a descendant of the current head, then we want to + // keep the current target to avoid unnecessary rebases + let commit_oid = if let Some(current_target) = gb_repository.default_target()? { + if repo + .is_descendant_of(current_target.sha, commit_oid) + .context("failed to check if target branch is descendant of current head")? + { + current_target.sha + } else { + commit_oid + } + } else { + commit_oid + }; + let target = target::Target { branch: target_branch_ref.clone(), remote_url: remote_url.to_string(), @@ -118,6 +125,9 @@ pub fn set_base_branch( target::Writer::new(gb_repository).context("failed to create target writer")?; target_writer.write_default(&target)?; + let head_name: git::Refname = current_head + .name() + .context("Failed to get HEAD reference name")?; if !head_name .to_string() .eq(&GITBUTLER_INTEGRATION_REFERENCE.to_string()) @@ -125,9 +135,8 @@ pub fn set_base_branch( // if there are any commits on the head branch or uncommitted changes in the working directory, we need to // put them into a virtual branch - let wd_diff = diff::workdir(repo, &head_commit.id())?; - - if !wd_diff.is_empty() || head_commit.id() != commit_oid { + let wd_diff = diff::workdir(repo, ¤t_head_commit.id())?; + if !wd_diff.is_empty() || current_head_commit.id() != target.sha { let hunks_by_filepath = super::virtual_hunks_by_filepath(&project_repository.project().path, &wd_diff); @@ -183,10 +192,10 @@ pub fn set_base_branch( upstream_head, created_timestamp_ms: now_ms, updated_timestamp_ms: now_ms, - head: head_commit.id(), + head: current_head_commit.id(), tree: super::write_tree_onto_commit( project_repository, - head_commit.id(), + current_head_commit.id(), &wd_diff, )?, ownership, diff --git a/gitbutler-app/tests/common.rs b/gitbutler-app/tests/common.rs index 2fb8cd008..79a74f192 100644 --- a/gitbutler-app/tests/common.rs +++ b/gitbutler-app/tests/common.rs @@ -85,7 +85,7 @@ impl TestProject { } /// git add -A - /// git reset --hard + /// git reset --hard pub fn reset_hard(&self, oid: Option) { let mut index = self.local_repository.index().expect("failed to get index"); index @@ -93,14 +93,14 @@ impl TestProject { .expect("failed to add all"); index.write().expect("failed to write index"); - let commit = oid.map_or( - self.local_repository - .head() - .unwrap() - .peel_to_commit() - .unwrap(), - |oid| self.local_repository.find_commit(oid).unwrap(), - ); + let head = self.local_repository.head().unwrap(); + let commit = oid.map_or(head.peel_to_commit().unwrap(), |oid| { + self.local_repository.find_commit(oid).unwrap() + }); + + let head_ref = head.name().unwrap(); + let head_ref = self.local_repository.find_reference(&head_ref).unwrap(); + self.local_repository .reset(&commit, git2::ResetType::Hard, None) .unwrap(); @@ -231,6 +231,18 @@ impl TestProject { self.local_repository.find_commit(oid) } + pub fn checkout_commit(&self, commit_oid: git::Oid) { + let commit = self.local_repository.find_commit(commit_oid).unwrap(); + let commit_tree = commit.tree().unwrap(); + + self.local_repository.set_head_detached(commit_oid).unwrap(); + self.local_repository + .checkout_tree(&commit_tree) + .force() + .checkout() + .unwrap(); + } + pub fn checkout(&self, branch: &git::LocalRefname) { let branch: git::Refname = branch.into(); let tree = match self.local_repository.find_branch(&branch) { diff --git a/gitbutler-app/tests/virtual_branches.rs b/gitbutler-app/tests/virtual_branches.rs index 849081a5e..d3cdf8fad 100644 --- a/gitbutler-app/tests/virtual_branches.rs +++ b/gitbutler-app/tests/virtual_branches.rs @@ -692,6 +692,8 @@ mod delete_virtual_branch { mod set_base_branch { use super::*; + use pretty_assertions::assert_eq; + #[tokio::test] async fn success() { let Test { @@ -728,6 +730,44 @@ mod set_base_branch { )); } } + + #[tokio::test] + async fn go_back_to_integration() { + let Test { + repository, + project_id, + controller, + .. + } = Test::default(); + + std::fs::write(repository.path().join("file.txt"), "one").unwrap(); + let oid_one = repository.commit_all("one"); + std::fs::write(repository.path().join("file.txt"), "two").unwrap(); + repository.commit_all("two"); + repository.push(); + + println!("{}", repository.path().display()); + + let base = controller + .set_base_branch(&project_id, &"refs/remotes/origin/master".parse().unwrap()) + .await + .unwrap(); + + let branches = controller.list_virtual_branches(&project_id).await.unwrap(); + assert!(branches.is_empty()); + + repository.checkout_commit(oid_one); + + let base_two = controller + .set_base_branch(&project_id, &"refs/remotes/origin/master".parse().unwrap()) + .await + .unwrap(); + + let branches = controller.list_virtual_branches(&project_id).await.unwrap(); + assert_eq!(branches.len(), 1); + + assert_eq!(base_two, base); + } } mod unapply {