From 7362bab7aa8298bd58d5907d6aaf957e4c0d699d Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 11 Aug 2024 20:40:41 +0200 Subject: [PATCH] Assure we can use different merge bases when showing branch details. Alternatively, correct the assumption that there is a difference. --- crates/gitbutler-branch-actions/src/base.rs | 11 +++++----- crates/gitbutler-branch-actions/src/branch.rs | 8 ++++---- .../tests/fixtures/for-details.sh | 3 +++ .../tests/virtual_branches/list_details.rs | 20 +++++++++++-------- crates/gitbutler-branch/src/target.rs | 13 +++++++++--- crates/gitbutler-cli/src/args.rs | 2 ++ crates/gitbutler-cli/src/command/vbranch.rs | 5 +++++ crates/gitbutler-cli/src/main.rs | 3 +++ 8 files changed, 45 insertions(+), 20 deletions(-) diff --git a/crates/gitbutler-branch-actions/src/base.rs b/crates/gitbutler-branch-actions/src/base.rs index 7b3656fd3..f824aa97e 100644 --- a/crates/gitbutler-branch-actions/src/base.rs +++ b/crates/gitbutler-branch-actions/src/base.rs @@ -314,11 +314,12 @@ fn _print_tree(repo: &git2::Repository, tree: &git2::Tree) -> Result<()> { Ok(()) } -// try to update the target branch -// this means that we need to: -// determine if what the target branch is now pointing to is mergeable with our current working directory -// merge the target branch into our current working directory -// update the target sha +/// try to update the target branch +/// this means that we need to: +/// - determine if what the target branch is now pointing to is mergeable with our current working directory, +/// - merge the target branch into our current working directory +/// - update the target sha +/// - return all conflicting references that were unapplied to avoid the conflict pub(crate) fn update_base_branch( ctx: &CommandContext, perm: &mut WorktreeWritePermission, diff --git a/crates/gitbutler-branch-actions/src/branch.rs b/crates/gitbutler-branch-actions/src/branch.rs index c3e6a2557..4c33bab5e 100644 --- a/crates/gitbutler-branch-actions/src/branch.rs +++ b/crates/gitbutler-branch-actions/src/branch.rs @@ -428,7 +428,7 @@ pub fn get_branch_listing_details( let repo = ctx.repository(); let branches = list_branches(ctx, None, Some(branch_names))?; - let (default_target_upstream_commit_id, default_target_merge_base) = { + let (default_target_current_upstream_commit_id, default_target_seen_at_last_update) = { let target = ctx .project() .virtual_branches() @@ -446,12 +446,12 @@ pub fn get_branch_listing_details( for branch in branches { let other_branch_commit_id = if let Some(virtual_branch) = branch.virtual_branch { if virtual_branch.in_workspace { - default_target_merge_base + default_target_seen_at_last_update } else { - default_target_upstream_commit_id + default_target_current_upstream_commit_id } } else { - default_target_upstream_commit_id + default_target_current_upstream_commit_id }; let Ok(base) = repo.merge_base(other_branch_commit_id, branch.head) else { continue; diff --git a/crates/gitbutler-branch-actions/tests/fixtures/for-details.sh b/crates/gitbutler-branch-actions/tests/fixtures/for-details.sh index 66ac11525..fb137be3d 100644 --- a/crates/gitbutler-branch-actions/tests/fixtures/for-details.sh +++ b/crates/gitbutler-branch-actions/tests/fixtures/for-details.sh @@ -49,4 +49,7 @@ git clone remote complex-repo echo non-virtual-feature >> file git commit -am "non-virtual-feat-$round" done + + # pretend the remote is at the same state as our local `main` + git update-ref refs/remotes/origin/main main ) diff --git a/crates/gitbutler-branch-actions/tests/virtual_branches/list_details.rs b/crates/gitbutler-branch-actions/tests/virtual_branches/list_details.rs index 678aba4c3..35b7cbf18 100644 --- a/crates/gitbutler-branch-actions/tests/virtual_branches/list_details.rs +++ b/crates/gitbutler-branch-actions/tests/virtual_branches/list_details.rs @@ -52,12 +52,13 @@ fn many_commits_in_all_branch_types() -> anyhow::Result<()> { list[0], BranchListingDetails { name: "feature".into(), - lines_added: 100 + 5, + lines_added: 100, lines_removed: 0, number_of_files: 1, - number_of_commits: 100 + 5, /* local tracking branch is merge base */ + number_of_commits: 100, authors: vec![default_author()], - } + }, + "local branches use the *current* local tracking branch…" ); assert_eq!( list[1], @@ -66,21 +67,24 @@ fn many_commits_in_all_branch_types() -> anyhow::Result<()> { lines_added: 15, lines_removed: 0, number_of_files: 1, - // TODO(ST): why is it also going against the local tracking branch instead of the local `main`? number_of_commits: 10 + 5, authors: vec![default_author()], - } + }, + "…while virtual branches use the 'target' stored when the workspace was last updated.\ + That way the 'update' of the workspace performs the calculation to put it back on top of \ + the local tracking branch." ); assert_eq!( list[2], BranchListingDetails { name: "non-virtual-feature".into(), - lines_added: 55, + lines_added: 50, lines_removed: 0, number_of_files: 1, - number_of_commits: 50 + 5, + number_of_commits: 50, authors: vec![default_author()], - } + }, + "This is a non-virtual brnach, so it sees the local tracking branch as well" ); Ok(()) } diff --git a/crates/gitbutler-branch/src/target.rs b/crates/gitbutler-branch/src/target.rs index 9d9800d97..a005df585 100644 --- a/crates/gitbutler-branch/src/target.rs +++ b/crates/gitbutler-branch/src/target.rs @@ -5,12 +5,19 @@ use serde::{ser::SerializeStruct, Deserialize, Deserializer, Serialize, Serializ pub struct Target { /// The combination of remote name and branch name, i.e. `origin` and `main`. /// The remote name is the one used to fetch from. + /// It's equivalent to e.g. `refs/remotes/origin/main` , and the type `RemoteRefName` + /// stores it as `` and `` so that finding references named `/` + /// will typically find the local tracking branch unambiguously. pub branch: RemoteRefname, /// The URL of the remote behind the symbolic name. pub remote_url: String, - /// The merge-base between `branch` and the current worktree `HEAD`. - // TODO(ST): is it safe/correct to rename this to `merge_base_commit_id`? - // It seems like it, but why was it named just `sha` in the first place? + /// The merge-base between `branch` and the current worktree `HEAD` upon first creation, + /// but then it's the set to the new destination of e.g. `refs/remotes/origin/main` after + /// the remote was fetched. This value is used to determine if there was a change, + /// and if the *workspace* needs to be recalculated/rebased against the new commit. + // TODO(ST): is it safe/correct to rename this to `branch_target_id`? Should be! + // It's just a bit strange it starts life as merge-base, but maybe it ends + // up the same anyway? Definitely could use a test then. pub sha: git2::Oid, /// The name of the remote to push to. pub push_remote_name: Option, diff --git a/crates/gitbutler-cli/src/args.rs b/crates/gitbutler-cli/src/args.rs index b57e6fa65..4c3d2a683 100644 --- a/crates/gitbutler-cli/src/args.rs +++ b/crates/gitbutler-cli/src/args.rs @@ -71,6 +71,8 @@ pub mod vbranch { }, /// List all branches that can be relevant. ListAll, + /// After fetching the target (i.e. local tracking branch), recompute our workspace against it. + UpdateTarget, } } diff --git a/crates/gitbutler-cli/src/command/vbranch.rs b/crates/gitbutler-cli/src/command/vbranch.rs index 167eb8476..685ab7886 100644 --- a/crates/gitbutler-cli/src/command/vbranch.rs +++ b/crates/gitbutler-cli/src/command/vbranch.rs @@ -8,6 +8,11 @@ use gitbutler_project::Project; use crate::command::debug_print; +pub fn update_target(project: Project) -> Result<()> { + let unapplied = VirtualBranchActions.update_base_branch(&project)?; + debug_print(unapplied) +} + pub fn list_all(project: Project) -> Result<()> { let ctx = CommandContext::open(&project)?; debug_print(list_branches(&ctx, None, None)?) diff --git a/crates/gitbutler-cli/src/main.rs b/crates/gitbutler-cli/src/main.rs index 2381de9f1..a7585db56 100644 --- a/crates/gitbutler-cli/src/main.rs +++ b/crates/gitbutler-cli/src/main.rs @@ -36,6 +36,9 @@ fn main() -> Result<()> { command::vbranch::details(project, names) } Some(vbranch::SubCommands::ListAll) => command::vbranch::list_all(project), + Some(vbranch::SubCommands::UpdateTarget) => { + command::vbranch::update_target(project) + } None => command::vbranch::list(project), } }