From c81b14079044104e711e43bb23b8d2a4abab5fc3 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Thu, 29 Aug 2024 14:33:30 +0200 Subject: [PATCH 1/6] Assure the most-recent workspace head is used when emitting changed files. The reason for this is that now the outcome of this can be re-used in `list_virtual_branches`, which is sensitive to seeing the latest vbranch state which is not a given. Calculating it makes sure there is no mismatch, and probably "can't be wrong". Also note that `head_commit()` is still kept around as it's the idea to eventually use it more. --- crates/gitbutler-branch-actions/src/base.rs | 2 +- crates/gitbutler-branch-actions/src/branch.rs | 9 ++++----- crates/gitbutler-branch-actions/src/status.rs | 2 +- crates/gitbutler-branch-actions/src/virtual.rs | 2 +- crates/gitbutler-diff/src/diff.rs | 4 ++-- crates/gitbutler-repo/src/repository_ext.rs | 4 ++++ 6 files changed, 13 insertions(+), 10 deletions(-) diff --git a/crates/gitbutler-branch-actions/src/base.rs b/crates/gitbutler-branch-actions/src/base.rs index fce18420d9..69755eaf74 100644 --- a/crates/gitbutler-branch-actions/src/base.rs +++ b/crates/gitbutler-branch-actions/src/base.rs @@ -181,7 +181,7 @@ pub(crate) 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 = gitbutler_diff::workdir(repo, ¤t_head_commit.id())?; + let wd_diff = gitbutler_diff::workdir(repo, current_head_commit.id())?; if !wd_diff.is_empty() || current_head_commit.id() != target.sha { // assign ownership to the branch let ownership = wd_diff.iter().fold( diff --git a/crates/gitbutler-branch-actions/src/branch.rs b/crates/gitbutler-branch-actions/src/branch.rs index dc45cd93ee..67bd1b6512 100644 --- a/crates/gitbutler-branch-actions/src/branch.rs +++ b/crates/gitbutler-branch-actions/src/branch.rs @@ -1,3 +1,4 @@ +use crate::integration::get_workspace_head; use crate::{RemoteBranchFile, VirtualBranchesExt}; use anyhow::{bail, Context, Result}; use bstr::{BStr, ByteSlice}; @@ -9,7 +10,7 @@ use gitbutler_command_context::CommandContext; use gitbutler_diff::DiffByPathMap; use gitbutler_project::access::WorktreeReadPermission; use gitbutler_reference::normalize_branch_name; -use gitbutler_repo::{GixRepositoryExt, RepositoryExt}; +use gitbutler_repo::GixRepositoryExt; use gitbutler_serde::BStringForFrontend; use gix::object::tree::diff::Action; use gix::prelude::ObjectIdExt; @@ -25,12 +26,10 @@ use std::{ }; pub(crate) fn get_uncommited_files_raw( - context: &CommandContext, + ctx: &CommandContext, _permission: &WorktreeReadPermission, ) -> Result { - let repository = context.repository(); - let head_commit = repository.head_commit()?; - gitbutler_diff::workdir(repository, &head_commit.id()) + gitbutler_diff::workdir(ctx.repository(), get_workspace_head(ctx)?) .context("Failed to list uncommited files") } diff --git a/crates/gitbutler-branch-actions/src/status.rs b/crates/gitbutler-branch-actions/src/status.rs index fc93857a9f..2a4d4c4b84 100644 --- a/crates/gitbutler-branch-actions/src/status.rs +++ b/crates/gitbutler-branch-actions/src/status.rs @@ -56,7 +56,7 @@ pub fn get_applied_status_cached( // any of its inputs will update the intragration commit right away. // It's for another day though - right now the integration commit may be slightly stale. let integration_commit_id = get_workspace_head(ctx)?; - gitbutler_diff::workdir(ctx.repository(), &integration_commit_id.to_owned()) + gitbutler_diff::workdir(ctx.repository(), integration_commit_id.to_owned()) .context("failed to diff workdir") })?; diff --git a/crates/gitbutler-branch-actions/src/virtual.rs b/crates/gitbutler-branch-actions/src/virtual.rs index ebba075f33..167ddefb1c 100644 --- a/crates/gitbutler-branch-actions/src/virtual.rs +++ b/crates/gitbutler-branch-actions/src/virtual.rs @@ -1215,7 +1215,7 @@ pub(crate) fn move_commit_file( let mut upstream_commits = ctx.l(target_branch.head, LogUntil::Commit(amend_commit.id()))?; // get a list of all the diffs across all the virtual branches - let base_file_diffs = gitbutler_diff::workdir(ctx.repository(), &default_target.sha) + let base_file_diffs = gitbutler_diff::workdir(ctx.repository(), default_target.sha) .context("failed to diff workdir")?; // filter base_file_diffs to HashMap> only for hunks in target_ownership diff --git a/crates/gitbutler-diff/src/diff.rs b/crates/gitbutler-diff/src/diff.rs index 0315d8cdb0..2e9b4e4d34 100644 --- a/crates/gitbutler-diff/src/diff.rs +++ b/crates/gitbutler-diff/src/diff.rs @@ -123,9 +123,9 @@ pub struct FileDiff { } #[instrument(level = tracing::Level::DEBUG, skip(repo))] -pub fn workdir(repo: &git2::Repository, commit_oid: &git2::Oid) -> Result { +pub fn workdir(repo: &git2::Repository, commit_oid: git2::Oid) -> Result { let commit = repo - .find_commit(*commit_oid) + .find_commit(commit_oid) .context("failed to find commit")?; let old_tree = repo.find_real_tree(&commit, Default::default())?; diff --git a/crates/gitbutler-repo/src/repository_ext.rs b/crates/gitbutler-repo/src/repository_ext.rs index 4317126a42..567ec1d5bf 100644 --- a/crates/gitbutler-repo/src/repository_ext.rs +++ b/crates/gitbutler-repo/src/repository_ext.rs @@ -17,6 +17,10 @@ use tracing::instrument; /// /// For now, it collects useful methods from `gitbutler-core::git::Repository` pub trait RepositoryExt { + /// Return `HEAD^{commit}` - ideal for obtaining the integration branch commit in open-workspace mode + /// when it's clear that it's representing the current state. + /// + /// Ideally, this is used in places of `get_workspace_head()`. fn head_commit(&self) -> Result>; fn remote_branches(&self) -> Result>; fn remotes_as_string(&self) -> Result>; From 6a4849d09096040d911530ee23a25cfb68366c08 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Thu, 29 Aug 2024 14:41:33 +0200 Subject: [PATCH 2/6] refactor: avoid borrowed `git2::Oid` as it's a copy type. Usually this makes the affected code simpler. --- .../src/branch_manager/branch_removal.rs | 2 +- crates/gitbutler-branch-actions/src/status.rs | 2 +- crates/gitbutler-branch-actions/src/virtual.rs | 4 ++-- crates/gitbutler-diff/src/write.rs | 4 ++-- crates/gitbutler-repo/src/change_reference.rs | 2 +- crates/gitbutler-repo/src/repository.rs | 8 ++++---- 6 files changed, 11 insertions(+), 11 deletions(-) diff --git a/crates/gitbutler-branch-actions/src/branch_manager/branch_removal.rs b/crates/gitbutler-branch-actions/src/branch_manager/branch_removal.rs index 0f75118131..bba1cb0ef7 100644 --- a/crates/gitbutler-branch-actions/src/branch_manager/branch_removal.rs +++ b/crates/gitbutler-branch-actions/src/branch_manager/branch_removal.rs @@ -102,7 +102,7 @@ impl BranchManager<'_> { .map(|file| (file.path, file.hunks)) .collect::)>>(); let tree_oid = - gitbutler_diff::write::hunks_onto_oid(self.ctx, &branch.head, files)?; + gitbutler_diff::write::hunks_onto_oid(self.ctx, branch.head, files)?; let branch_tree = repo.find_tree(tree_oid)?; let mut result = repo.merge_trees(&base_tree, &final_tree, &branch_tree, None)?; diff --git a/crates/gitbutler-branch-actions/src/status.rs b/crates/gitbutler-branch-actions/src/status.rs index 2a4d4c4b84..c390f750dc 100644 --- a/crates/gitbutler-branch-actions/src/status.rs +++ b/crates/gitbutler-branch-actions/src/status.rs @@ -213,7 +213,7 @@ pub fn get_applied_status_cached( // write updated state if not resolving if !ctx.is_resolving() { for (vbranch, files) in &mut hunks_by_branch { - vbranch.tree = gitbutler_diff::write::hunks_onto_oid(ctx, &vbranch.head, files)?; + vbranch.tree = gitbutler_diff::write::hunks_onto_oid(ctx, vbranch.head, files)?; vb_state .set_branch(vbranch.clone()) .context(format!("failed to write virtual branch {}", vbranch.name))?; diff --git a/crates/gitbutler-branch-actions/src/virtual.rs b/crates/gitbutler-branch-actions/src/virtual.rs index 167ddefb1c..a2359e24fd 100644 --- a/crates/gitbutler-branch-actions/src/virtual.rs +++ b/crates/gitbutler-branch-actions/src/virtual.rs @@ -158,7 +158,7 @@ pub fn unapply_ownership( .map(|file| (file.path, file.hunks)) .collect::)>>(); let tree_oid = - gitbutler_diff::write::hunks_onto_oid(ctx, &integration_commit_id, files)?; + gitbutler_diff::write::hunks_onto_oid(ctx, integration_commit_id, files)?; let branch_tree = repo.find_tree(tree_oid)?; let mut result = repo.merge_trees(&base_tree, &final_tree, &branch_tree, None)?; let final_tree_oid = result.write_tree_to(ctx.repository())?; @@ -1043,7 +1043,7 @@ pub(crate) fn push( }; ctx.push( - &vbranch.head, + vbranch.head, &remote_branch, with_force, credentials, diff --git a/crates/gitbutler-diff/src/write.rs b/crates/gitbutler-diff/src/write.rs index 7098e54c89..53e7276ff9 100644 --- a/crates/gitbutler-diff/src/write.rs +++ b/crates/gitbutler-diff/src/write.rs @@ -15,13 +15,13 @@ use crate::GitHunk; // and writes it as a new tree for storage pub fn hunks_onto_oid( ctx: &CommandContext, - target: &git2::Oid, + target: git2::Oid, files: impl IntoIterator, impl Borrow>)>, ) -> Result where T: Into + Clone, { - hunks_onto_commit(ctx, *target, files) + hunks_onto_commit(ctx, target, files) } pub fn hunks_onto_commit( diff --git a/crates/gitbutler-repo/src/change_reference.rs b/crates/gitbutler-repo/src/change_reference.rs index cd923fe772..305b4e8586 100644 --- a/crates/gitbutler-repo/src/change_reference.rs +++ b/crates/gitbutler-repo/src/change_reference.rs @@ -146,7 +146,7 @@ pub fn push_change_reference( let commit = commit_by_branch_id_and_change_id(ctx, &vbranch, &handle, reference.change_id.clone())?; ctx.push( - &commit.id(), + commit.id(), &upstream_refname, with_force, credentials, diff --git a/crates/gitbutler-repo/src/repository.rs b/crates/gitbutler-repo/src/repository.rs index 42e3f4feac..ea7792edea 100644 --- a/crates/gitbutler-repo/src/repository.rs +++ b/crates/gitbutler-repo/src/repository.rs @@ -14,7 +14,7 @@ pub trait RepoActionsExt { -> Result<()>; fn push( &self, - head: &git2::Oid, + head: git2::Oid, branch: &RemoteRefname, with_force: bool, credentials: &Helper, @@ -67,14 +67,14 @@ impl RepoActionsExt for CommandContext { let refname = RemoteRefname::from_str(&format!("refs/remotes/{remote_name}/{branch_name}",))?; - match self.push(&commit_id, &refname, false, credentials, None, askpass) { + match self.push(commit_id, &refname, false, credentials, None, askpass) { Ok(()) => Ok(()), Err(e) => Err(anyhow::anyhow!(e.to_string())), }?; let empty_refspec = Some(format!(":refs/heads/{}", branch_name)); match self.push( - &commit_id, + commit_id, &refname, false, credentials, @@ -254,7 +254,7 @@ impl RepoActionsExt for CommandContext { fn push( &self, - head: &git2::Oid, + head: git2::Oid, branch: &RemoteRefname, with_force: bool, credentials: &Helper, From 36f23d529f929e9707688ff56269e7ee60ec3109 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Thu, 29 Aug 2024 15:16:01 +0200 Subject: [PATCH 3/6] CLI with `branch list-remotes` to list remote branches --- apps/desktop/src/lib/stores/remoteBranches.ts | 2 +- .../gitbutler-branch-actions/src/actions.rs | 6 ++-- crates/gitbutler-branch-actions/src/lib.rs | 2 +- crates/gitbutler-branch-actions/src/remote.rs | 29 +++++++++++-------- .../tests/extra/mod.rs | 2 +- crates/gitbutler-cli/src/args.rs | 2 ++ crates/gitbutler-cli/src/command/vbranch.rs | 4 +++ crates/gitbutler-cli/src/main.rs | 1 + crates/gitbutler-tauri/src/main.rs | 2 +- .../gitbutler-tauri/src/virtual_branches.rs | 4 +-- 10 files changed, 33 insertions(+), 21 deletions(-) diff --git a/apps/desktop/src/lib/stores/remoteBranches.ts b/apps/desktop/src/lib/stores/remoteBranches.ts index 0cfec61979..14fc5bd498 100644 --- a/apps/desktop/src/lib/stores/remoteBranches.ts +++ b/apps/desktop/src/lib/stores/remoteBranches.ts @@ -21,7 +21,7 @@ export class RemoteBranchService { try { const remoteBranches = plainToInstance( Branch, - await invoke('list_remote_branches', { projectId: this.projectId }) + await invoke('list_local_branches', { projectId: this.projectId }) ); this.projectMetrics?.setMetric('normal_branch_count', remoteBranches.length); this.branches.set(remoteBranches); diff --git a/crates/gitbutler-branch-actions/src/actions.rs b/crates/gitbutler-branch-actions/src/actions.rs index 4629682228..801a7a4216 100644 --- a/crates/gitbutler-branch-actions/src/actions.rs +++ b/crates/gitbutler-branch-actions/src/actions.rs @@ -8,7 +8,7 @@ use crate::{ branch::get_uncommited_files, branch_manager::BranchManagerExt, file::RemoteBranchFile, - remote::{get_branch_data, list_remote_branches, RemoteBranch, RemoteBranchData}, + remote::{get_branch_data, list_local_branches, RemoteBranch, RemoteBranchData}, VirtualBranchesExt, }; use anyhow::{Context, Result}; @@ -474,9 +474,9 @@ impl VirtualBranchActions { branch::push(&ctx, branch_id, with_force, &helper, askpass) } - pub fn list_remote_branches(project: Project) -> Result> { + pub fn list_local_branches(project: Project) -> Result> { let ctx = CommandContext::open(&project)?; - list_remote_branches(&ctx) + list_local_branches(&ctx) } pub fn get_remote_branch_data( diff --git a/crates/gitbutler-branch-actions/src/lib.rs b/crates/gitbutler-branch-actions/src/lib.rs index 8ffae0891a..a22d26bd02 100644 --- a/crates/gitbutler-branch-actions/src/lib.rs +++ b/crates/gitbutler-branch-actions/src/lib.rs @@ -18,7 +18,7 @@ mod file; pub use file::{Get, RemoteBranchFile}; mod remote; -pub use remote::{list_remote_branches, RemoteBranch, RemoteBranchData, RemoteCommit}; +pub use remote::{list_local_branches, RemoteBranch, RemoteBranchData, RemoteCommit}; pub mod conflicts; diff --git a/crates/gitbutler-branch-actions/src/remote.rs b/crates/gitbutler-branch-actions/src/remote.rs index b6c49a4d2a..0e4b887ce2 100644 --- a/crates/gitbutler-branch-actions/src/remote.rs +++ b/crates/gitbutler-branch-actions/src/remote.rs @@ -10,15 +10,15 @@ use gitbutler_repo::{LogUntil, RepoActionsExt, RepositoryExt}; use gitbutler_serde::BStringForFrontend; use serde::Serialize; -// this struct is a mapping to the view `RemoteBranch` type in Typescript -// found in src-tauri/src/routes/repo/[project_id]/types.ts -// -// it holds data calculated for presentation purposes of one Git branch -// with comparison data to the Target commit, determining if it is mergeable, -// and how far ahead or behind the Target it is. -// an array of them can be requested from the frontend to show in the sidebar -// Tray and should only contain branches that have not been converted into -// virtual branches yet (ie, we have no `Branch` struct persisted in our data. +/// this struct is a mapping to the view `RemoteBranch` type in Typescript +/// found in src-tauri/src/routes/repo/[project_id]/types.ts +/// +/// it holds data calculated for presentation purposes of one Git branch +/// with comparison data to the Target commit, determining if it is mergeable, +/// and how far ahead or behind the Target it is. +/// an array of them can be requested from the frontend to show in the sidebar +/// Tray and should only contain branches that have not been converted into +/// virtual branches yet (ie, we have no `Branch` struct persisted in our data. #[derive(Debug, Clone, Serialize, PartialEq)] #[serde(rename_all = "camelCase")] pub struct RemoteBranch { @@ -57,9 +57,14 @@ pub struct RemoteCommit { pub parent_ids: Vec, } -// for legacy purposes, this is still named "remote" branches, but it's actually -// a list of all the normal (non-gitbutler) git branches. -pub fn list_remote_branches(ctx: &CommandContext) -> Result> { +/// Return information on all local branches, while skipping gitbutler-specific branches in `refs/heads`. +/// +/// Note to be confused with `list_branches()`, which is used for the new branch listing. +/// +/// # Previous notes +/// For legacy purposes, this is still named "remote" branches, but it's actually +/// a list of all the normal (non-gitbutler) git branches. +pub fn list_local_branches(ctx: &CommandContext) -> Result> { let default_target = default_target(&ctx.project().gb_dir())?; let mut remote_branches = vec![]; diff --git a/crates/gitbutler-branch-actions/tests/extra/mod.rs b/crates/gitbutler-branch-actions/tests/extra/mod.rs index 857adb5949..22407d0583 100644 --- a/crates/gitbutler-branch-actions/tests/extra/mod.rs +++ b/crates/gitbutler-branch-actions/tests/extra/mod.rs @@ -1345,7 +1345,7 @@ fn detect_mergeable_branch() -> Result<()> { vb_state.set_branch(branch4.clone())?; let remotes = - gitbutler_branch_actions::list_remote_branches(ctx).expect("failed to list remotes"); + gitbutler_branch_actions::list_local_branches(ctx).expect("failed to list remotes"); let _remote1 = &remotes .iter() .find(|b| b.name.to_string() == "refs/remotes/origin/remote_branch") diff --git a/crates/gitbutler-cli/src/args.rs b/crates/gitbutler-cli/src/args.rs index 9b94fc0c5f..ece070c8b7 100644 --- a/crates/gitbutler-cli/src/args.rs +++ b/crates/gitbutler-cli/src/args.rs @@ -38,6 +38,8 @@ pub mod vbranch { #[derive(Debug, clap::Subcommand)] pub enum SubCommands { + /// List all local branches that aren't GitButler specific. + ListLocal, /// Provide the current state of all applied virtual branches. Status, /// Make the named branch the default so all worktree or index changes are associated with it automatically. diff --git a/crates/gitbutler-cli/src/command/vbranch.rs b/crates/gitbutler-cli/src/command/vbranch.rs index 4dd7e75d6d..425780e65b 100644 --- a/crates/gitbutler-cli/src/command/vbranch.rs +++ b/crates/gitbutler-cli/src/command/vbranch.rs @@ -18,6 +18,10 @@ pub fn list_all(project: Project) -> Result<()> { debug_print(list_branches(&ctx, None, None)?) } +pub fn list_local(project: Project) -> Result<()> { + debug_print(VirtualBranchActions::list_local_branches(project)?) +} + pub fn details(project: Project, branch_names: Vec) -> Result<()> { let ctx = CommandContext::open(&project)?; debug_print(get_branch_listing_details(&ctx, branch_names)?) diff --git a/crates/gitbutler-cli/src/main.rs b/crates/gitbutler-cli/src/main.rs index a2c1413229..6e7e38d268 100644 --- a/crates/gitbutler-cli/src/main.rs +++ b/crates/gitbutler-cli/src/main.rs @@ -20,6 +20,7 @@ fn main() -> Result<()> { args::Subcommands::Branch(vbranch::Platform { cmd }) => { let project = command::prepare::project_from_path(args.current_dir)?; match cmd { + Some(vbranch::SubCommands::ListLocal) => command::vbranch::list_local(project), Some(vbranch::SubCommands::Status) => command::vbranch::status(project), Some(vbranch::SubCommands::Unapply { name }) => { command::vbranch::unapply(project, name) diff --git a/crates/gitbutler-tauri/src/main.rs b/crates/gitbutler-tauri/src/main.rs index ea8a323e58..df161ead9d 100644 --- a/crates/gitbutler-tauri/src/main.rs +++ b/crates/gitbutler-tauri/src/main.rs @@ -177,7 +177,7 @@ fn main() { virtual_branches::commands::push_change_reference, virtual_branches::commands::reorder_commit, virtual_branches::commands::update_commit_message, - virtual_branches::commands::list_remote_branches, + virtual_branches::commands::list_local_branches, virtual_branches::commands::list_branches, virtual_branches::commands::get_branch_listing_details, virtual_branches::commands::get_remote_branch_data, diff --git a/crates/gitbutler-tauri/src/virtual_branches.rs b/crates/gitbutler-tauri/src/virtual_branches.rs index caf346fd92..2191d93844 100644 --- a/crates/gitbutler-tauri/src/virtual_branches.rs +++ b/crates/gitbutler-tauri/src/virtual_branches.rs @@ -447,12 +447,12 @@ pub mod commands { #[tauri::command(async)] #[instrument(skip(projects), err(Debug))] - pub fn list_remote_branches( + pub fn list_local_branches( projects: State<'_, projects::Controller>, project_id: ProjectId, ) -> Result, Error> { let project = projects.get(project_id)?; - let branches = VirtualBranchActions::list_remote_branches(project)?; + let branches = VirtualBranchActions::list_local_branches(project)?; Ok(branches) } From 4ae354392b2b12e59011f06b14dc1371dd079b0e Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Thu, 29 Aug 2024 15:35:18 +0200 Subject: [PATCH 4/6] Improve performance of `list-local` using `git2`. --- crates/gitbutler-branch-actions/src/remote.rs | 57 ++++++++----------- .../gitbutler-branch-actions/src/virtual.rs | 6 +- 2 files changed, 28 insertions(+), 35 deletions(-) diff --git a/crates/gitbutler-branch-actions/src/remote.rs b/crates/gitbutler-branch-actions/src/remote.rs index 0e4b887ce2..3c21ee265b 100644 --- a/crates/gitbutler-branch-actions/src/remote.rs +++ b/crates/gitbutler-branch-actions/src/remote.rs @@ -68,24 +68,30 @@ pub fn list_local_branches(ctx: &CommandContext) -> Result> { let default_target = default_target(&ctx.project().gb_dir())?; let mut remote_branches = vec![]; + let remotes = ctx.repository().remotes()?; for (branch, _) in ctx .repository() .branches(None) .context("failed to list remote branches")? .flatten() { - let branch = branch_to_remote_branch(ctx, &branch); + let branch = match branch_to_remote_branch(&branch, &remotes) { + Ok(Some(b)) => b, + Ok(None) => continue, + Err(err) => { + tracing::warn!(?err, "Ignoring branch"); + continue; + } + }; - if let Some(branch) = branch { - let branch_is_trunk = branch.name.branch() == Some(default_target.branch.branch()) - && branch.name.remote() == Some(default_target.branch.remote()); + let branch_is_trunk = branch.name.branch() == Some(default_target.branch.branch()) + && branch.name.remote() == Some(default_target.branch.remote()); - if !branch_is_trunk - && branch.name.branch() != Some("gitbutler/integration") - && branch.name.branch() != Some("gitbutler/target") - { - remote_branches.push(branch); - } + if !branch_is_trunk + && branch.name.branch() != Some("gitbutler/integration") + && branch.name.branch() != Some("gitbutler/target") + { + remote_branches.push(branch); } } Ok(remote_branches) @@ -104,30 +110,15 @@ pub(crate) fn get_branch_data(ctx: &CommandContext, refname: &Refname) -> Result } pub(crate) fn branch_to_remote_branch( - ctx: &CommandContext, - branch: &git2::Branch, -) -> Option { - let commit = match branch.get().peel_to_commit() { - Ok(c) => c, - Err(err) => { - tracing::warn!( - ?err, - "ignoring branch {:?} as peeling failed", - branch.name() - ); - return None; - } - }; - let name = Refname::try_from(branch) - .context("could not get branch name") - .ok()?; + branch: &git2::Branch<'_>, + remotes: &git2::string_array::StringArray, +) -> Result> { + let commit = branch.get().peel_to_commit()?; + let name = Refname::try_from(branch).context("could not get branch name")?; - let given_name = branch - .get() - .given_name(&ctx.repository().remotes().ok()?) - .ok()?; + let given_name = branch.get().given_name(remotes)?; - branch.get().target().map(|sha| RemoteBranch { + Ok(branch.get().target().map(|sha| RemoteBranch { sha, upstream: if let Refname::Local(local_name) = &name { local_name.remote().cloned() @@ -144,7 +135,7 @@ pub(crate) fn branch_to_remote_branch( .ok(), last_commit_author: commit.author().name().map(std::string::ToString::to_string), is_remote: branch.get().is_remote(), - }) + })) } pub(crate) fn branch_to_remote_branch_data( diff --git a/crates/gitbutler-branch-actions/src/virtual.rs b/crates/gitbutler-branch-actions/src/virtual.rs index a2359e24fd..dd7beb5f35 100644 --- a/crates/gitbutler-branch-actions/src/virtual.rs +++ b/crates/gitbutler-branch-actions/src/virtual.rs @@ -354,8 +354,10 @@ pub fn list_virtual_branches_cached( .context("failed to find merge base")?; let base_current = true; - let upstream = upstream_branch - .and_then(|upstream_branch| branch_to_remote_branch(ctx, &upstream_branch)); + let upstream = upstream_branch.and_then(|upstream_branch| { + let remotes = repo.remotes().ok()?; + branch_to_remote_branch(&upstream_branch, &remotes).ok()? + }); let path_claim_positions: HashMap<&PathBuf, usize> = branch .ownership From 4cafc909b0a4d4a9f55c40008ca755cceaf14220 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Thu, 29 Aug 2024 14:46:58 +0200 Subject: [PATCH 5/6] Improve the clarity of performance logs --- .../gitbutler-branch-actions/src/actions.rs | 1 + crates/gitbutler-branch-actions/src/branch.rs | 2 + .../src/branch_manager/branch_creation.rs | 6 ++ .../src/branch_manager/branch_removal.rs | 58 +++++++++++-------- .../src/integration.rs | 1 + .../gitbutler-branch-actions/src/virtual.rs | 51 +++++++++------- crates/gitbutler-diff/src/diff.rs | 7 +-- crates/gitbutler-oplog/src/oplog.rs | 3 +- 8 files changed, 80 insertions(+), 49 deletions(-) diff --git a/crates/gitbutler-branch-actions/src/actions.rs b/crates/gitbutler-branch-actions/src/actions.rs index 801a7a4216..943286371e 100644 --- a/crates/gitbutler-branch-actions/src/actions.rs +++ b/crates/gitbutler-branch-actions/src/actions.rs @@ -570,6 +570,7 @@ impl VirtualBranchActions { branch::move_commit(&ctx, target_branch_id, commit_oid).map_err(Into::into) } + #[instrument(level = tracing::Level::DEBUG, skip(self, project), err(Debug))] pub fn create_virtual_branch_from_branch( &self, project: &Project, diff --git a/crates/gitbutler-branch-actions/src/branch.rs b/crates/gitbutler-branch-actions/src/branch.rs index 67bd1b6512..b7d12c4e09 100644 --- a/crates/gitbutler-branch-actions/src/branch.rs +++ b/crates/gitbutler-branch-actions/src/branch.rs @@ -24,7 +24,9 @@ use std::{ fmt::Debug, vec, }; +use tracing::instrument; +#[instrument(level = tracing::Level::DEBUG, skip(ctx, _permission))] pub(crate) fn get_uncommited_files_raw( ctx: &CommandContext, _permission: &WorktreeReadPermission, diff --git a/crates/gitbutler-branch-actions/src/branch_manager/branch_creation.rs b/crates/gitbutler-branch-actions/src/branch_manager/branch_creation.rs index ba491b9226..f588a51d89 100644 --- a/crates/gitbutler-branch-actions/src/branch_manager/branch_creation.rs +++ b/crates/gitbutler-branch-actions/src/branch_manager/branch_creation.rs @@ -9,6 +9,7 @@ use gitbutler_project::access::WorktreeWritePermission; use gitbutler_reference::{Refname, RemoteRefname}; use gitbutler_repo::{rebase::cherry_rebase, RepoActionsExt, RepositoryExt}; use gitbutler_time::time::now_since_unix_epoch_ms; +use tracing::instrument; use super::BranchManager; use crate::{ @@ -20,6 +21,7 @@ use crate::{ }; impl BranchManager<'_> { + #[instrument(level = tracing::Level::DEBUG, skip(self, perm), err(Debug))] pub fn create_virtual_branch( &self, create: &BranchCreateRequest, @@ -281,6 +283,7 @@ impl BranchManager<'_> { /// Holding private methods associated to branch creation impl BranchManager<'_> { + #[instrument(level = tracing::Level::DEBUG, skip(self, perm), err(Debug))] fn apply_branch( &self, branch_id: BranchId, @@ -310,6 +313,7 @@ impl BranchManager<'_> { default_target.sha, branch.head ))?; if merge_base != default_target.sha { + let _span = tracing::debug_span!("merge-base isn't default-target").entered(); // Branch is out of date, merge or rebase it let merge_base_tree = repo .find_commit(merge_base) @@ -454,6 +458,8 @@ impl BranchManager<'_> { vb_state.set_branch(branch.clone())?; } + let _span = tracing::debug_span!("finalize").entered(); + let wd_tree = self.ctx.repository().create_wd_tree()?; let branch_tree = repo diff --git a/crates/gitbutler-branch-actions/src/branch_manager/branch_removal.rs b/crates/gitbutler-branch-actions/src/branch_manager/branch_removal.rs index bba1cb0ef7..3bc14859a8 100644 --- a/crates/gitbutler-branch-actions/src/branch_manager/branch_removal.rs +++ b/crates/gitbutler-branch-actions/src/branch_manager/branch_removal.rs @@ -8,6 +8,7 @@ use gitbutler_oplog::SnapshotExt; use gitbutler_project::access::WorktreeWritePermission; use gitbutler_reference::{normalize_branch_name, ReferenceName, Refname}; use gitbutler_repo::{RepoActionsExt, RepositoryExt}; +use tracing::instrument; use super::BranchManager; use crate::{ @@ -19,6 +20,7 @@ use crate::{ impl BranchManager<'_> { // to unapply a branch, we need to write the current tree out, then remove those file changes from the wd + #[instrument(level = tracing::Level::DEBUG, skip(self, perm), err(Debug))] pub fn convert_to_real_branch( &self, branch_id: BranchId, @@ -52,6 +54,7 @@ impl BranchManager<'_> { real_branch.reference_name() } + #[instrument(level = tracing::Level::DEBUG, skip(self, perm), err(Debug))] pub(crate) fn delete_branch( &self, branch_id: BranchId, @@ -88,30 +91,38 @@ impl BranchManager<'_> { // go through the other applied branches and merge them into the final tree // then check that out into the working directory - let final_tree = applied_statuses - .into_iter() - .filter(|(branch, _)| branch.id != branch_id) - .fold( - target_commit.tree().context("failed to get target tree"), - |final_tree, status| { - let final_tree = final_tree?; - let branch = status.0; - let files = status - .1 - .into_iter() - .map(|file| (file.path, file.hunks)) - .collect::)>>(); - let tree_oid = - gitbutler_diff::write::hunks_onto_oid(self.ctx, branch.head, files)?; - let branch_tree = repo.find_tree(tree_oid)?; - let mut result = - repo.merge_trees(&base_tree, &final_tree, &branch_tree, None)?; - let final_tree_oid = result.write_tree_to(repo)?; - repo.find_tree(final_tree_oid) - .context("failed to find tree") - }, - )?; + let final_tree = { + let _span = tracing::debug_span!( + "new tree without deleted branch", + num_branches = applied_statuses.len() - 1 + ) + .entered(); + applied_statuses + .into_iter() + .filter(|(branch, _)| branch.id != branch_id) + .fold( + target_commit.tree().context("failed to get target tree"), + |final_tree, status| { + let final_tree = final_tree?; + let branch = status.0; + let files = status + .1 + .into_iter() + .map(|file| (file.path, file.hunks)) + .collect::)>>(); + let tree_oid = + gitbutler_diff::write::hunks_onto_oid(self.ctx, branch.head, files)?; + let branch_tree = repo.find_tree(tree_oid)?; + let mut result = + repo.merge_trees(&base_tree, &final_tree, &branch_tree, None)?; + let final_tree_oid = result.write_tree_to(repo)?; + repo.find_tree(final_tree_oid) + .context("failed to find tree") + }, + )? + }; + let _span = tracing::debug_span!("checkout final tree").entered(); // checkout final_tree into the working directory repo.checkout_tree_builder(&final_tree) .force() @@ -128,6 +139,7 @@ impl BranchManager<'_> { } impl BranchManager<'_> { + #[instrument(level = tracing::Level::DEBUG, skip(self, vbranch), err(Debug))] fn build_real_branch(&self, vbranch: &mut Branch) -> Result> { let repo = self.ctx.repository(); let target_commit = repo.find_commit(vbranch.head)?; diff --git a/crates/gitbutler-branch-actions/src/integration.rs b/crates/gitbutler-branch-actions/src/integration.rs index d1d8f8f397..a507180826 100644 --- a/crates/gitbutler-branch-actions/src/integration.rs +++ b/crates/gitbutler-branch-actions/src/integration.rs @@ -122,6 +122,7 @@ fn write_integration_file(head: &git2::Reference, path: PathBuf) -> Result<()> { std::fs::write(path, format!(":{}", sha))?; Ok(()) } +#[instrument(level = tracing::Level::DEBUG, skip(vb_state, ctx), err(Debug))] pub fn update_gitbutler_integration( vb_state: &VirtualBranchesHandle, ctx: &CommandContext, diff --git a/crates/gitbutler-branch-actions/src/virtual.rs b/crates/gitbutler-branch-actions/src/virtual.rs index dd7beb5f35..02069ac12b 100644 --- a/crates/gitbutler-branch-actions/src/virtual.rs +++ b/crates/gitbutler-branch-actions/src/virtual.rs @@ -232,9 +232,9 @@ fn find_base_tree<'a>( /// This should only ever be called by `list_virtual_branches /// /// This checks for the case where !branch.old_applied && branch.in_workspace -/// If this is the case, we ought to unapply the branch as its been carried +/// If this is the case, we ought to unapply the branch as it has been carried /// over from the old style of unapplying -fn resolve_old_applied_state( +fn fixup_old_applied_state( ctx: &CommandContext, vb_state: &VirtualBranchesHandle, perm: &mut WorktreeWritePermission, @@ -278,7 +278,7 @@ pub fn list_virtual_branches_cached( let vb_state = ctx.project().virtual_branches(); - resolve_old_applied_state(ctx, &vb_state, perm)?; + fixup_old_applied_state(ctx, &vb_state, perm)?; let default_target = vb_state .get_default_target() @@ -292,6 +292,8 @@ pub fn list_virtual_branches_cached( .max() .unwrap_or(-1); + let branches_span = + tracing::debug_span!("handle branches", num_branches = status.branches.len()).entered(); for (branch, mut files) in status.branches { let repo = ctx.repository(); update_conflict_markers(ctx, files.clone())?; @@ -331,23 +333,31 @@ pub fn list_virtual_branches_cached( // find all commits on head that are not on target.sha let commits = ctx.log(branch.head, LogUntil::Commit(default_target.sha))?; let check_commit = IsCommitIntegrated::new(ctx, &default_target)?; - let vbranch_commits = commits - .iter() - .map(|commit| { - is_remote = if is_remote { - is_remote - } else { - pushed_commits.contains_key(&commit.id()) - }; - - // only check for integration if we haven't already found an integration - if !is_integrated { - is_integrated = check_commit.is_integrated(commit)? - }; - - commit_to_vbranch_commit(ctx, &branch, commit, is_integrated, is_remote) - }) - .collect::>>()?; + let vbranch_commits = { + let _span = tracing::debug_span!( + "is-commit-integrated", + given_name = branch.name, + commits_to_check = commits.len() + ) + .entered(); + commits + .iter() + .map(|commit| { + is_remote = if is_remote { + is_remote + } else { + pushed_commits.contains_key(&commit.id()) + }; + + // only check for integration if we haven't already found an integration + if !is_integrated { + is_integrated = check_commit.is_integrated(commit)? + }; + + commit_to_vbranch_commit(ctx, &branch, commit, is_integrated, is_remote) + }) + .collect::>>()? + }; let merge_base = repo .merge_base(default_target.sha, branch.head) @@ -409,6 +419,7 @@ pub fn list_virtual_branches_cached( }; branches.push(branch); } + drop(branches_span); let mut branches = branches_with_large_files_abridged(branches); branches.sort_by(|a, b| a.order.cmp(&b.order)); diff --git a/crates/gitbutler-diff/src/diff.rs b/crates/gitbutler-diff/src/diff.rs index 2e9b4e4d34..dc36a1c23d 100644 --- a/crates/gitbutler-diff/src/diff.rs +++ b/crates/gitbutler-diff/src/diff.rs @@ -162,7 +162,7 @@ pub fn workdir(repo: &git2::Repository, commit_oid: git2::Oid) -> Result Result { @@ -175,10 +175,7 @@ pub fn trees( .context_lines(3) .show_untracked_content(true); - // This is not a content-based diff, but also considers modification times apparently, - // maybe related to racy-git. This is why empty diffs have ot be filtered. - let diff = - repository.diff_tree_to_tree(Some(old_tree), Some(new_tree), Some(&mut diff_opts))?; + let diff = repo.diff_tree_to_tree(Some(old_tree), Some(new_tree), Some(&mut diff_opts))?; hunks_by_filepath(None, &diff) } diff --git a/crates/gitbutler-oplog/src/oplog.rs b/crates/gitbutler-oplog/src/oplog.rs index 2a4050b96e..0bf35c3c84 100644 --- a/crates/gitbutler-oplog/src/oplog.rs +++ b/crates/gitbutler-oplog/src/oplog.rs @@ -148,7 +148,7 @@ impl OplogExt for Project { commit_snapshot(self, snapshot_tree_id, details, perm) } - #[instrument(skip(details, perm), err(Debug))] + #[instrument(skip(self, details, perm), err(Debug))] fn create_snapshot( &self, details: SnapshotDetails, @@ -158,6 +158,7 @@ impl OplogExt for Project { commit_snapshot(self, tree_id, details, perm) } + #[instrument(skip(self), err(Debug))] fn list_snapshots( &self, limit: usize, From 4d495cb3bc0862d47f09aef6456c28b31ea518a9 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Thu, 29 Aug 2024 20:44:03 +0200 Subject: [PATCH 6/6] Adjust integration check to also recognize by matching tree. This is relevant when all commits are equal by tree, but seem changed due to the added GitButler headers. For added safety, we also compare by commit message, date and authors, basically everything that isn't the headers. --- apps/desktop/src/lib/vbranches/types.ts | 11 +- crates/gitbutler-branch-actions/src/commit.rs | 7 ++ .../gitbutler-branch-actions/src/virtual.rs | 108 ++++++++++++++---- .../virtual_branches/update_base_branch.rs | 5 + 4 files changed, 106 insertions(+), 25 deletions(-) diff --git a/apps/desktop/src/lib/vbranches/types.ts b/apps/desktop/src/lib/vbranches/types.ts index 53ab7fd6ff..8cb784f1af 100644 --- a/apps/desktop/src/lib/vbranches/types.ts +++ b/apps/desktop/src/lib/vbranches/types.ts @@ -179,13 +179,19 @@ export class DetailedCommit { conflicted!: boolean; // Set if a GitButler branch reference pointing to this commit exists. In the format of "refs/remotes/origin/my-branch" remoteRef?: string | undefined; + // Identifies the remote commit id from which this local commit was copied. The backend figured this out by comparing + // author, commit and message. + copiedFromRemoteId?: string; prev?: DetailedCommit; next?: DetailedCommit; get status(): CommitStatus { if (this.isIntegrated) return 'integrated'; - if (this.isRemote && (!this.relatedTo || this.id === this.relatedTo.id)) + if ( + (this.isRemote && (!this.relatedTo || this.id === this.relatedTo.id)) || + (this.copiedFromRemoteId && this.relatedTo && this.copiedFromRemoteId === this.relatedTo.id) + ) return 'localAndRemote'; return 'local'; } @@ -240,9 +246,10 @@ export class Commit { export type AnyCommit = DetailedCommit | Commit; -export function commitCompare(left: AnyCommit, right: AnyCommit): boolean { +export function commitCompare(left: AnyCommit, right: DetailedCommit): boolean { if (left.id === right.id) return true; if (left.changeId && right.changeId && left.changeId === right.changeId) return true; + if (right.copiedFromRemoteId && right.copiedFromRemoteId === left.id) return true; return false; } diff --git a/crates/gitbutler-branch-actions/src/commit.rs b/crates/gitbutler-branch-actions/src/commit.rs index 8ff3f3e9b9..df0b75d8d7 100644 --- a/crates/gitbutler-branch-actions/src/commit.rs +++ b/crates/gitbutler-branch-actions/src/commit.rs @@ -36,6 +36,11 @@ pub struct VirtualBranchCommit { pub change_id: Option, pub is_signed: bool, pub conflicted: bool, + /// The id of the remote commit from which this one was copied, as identified by + /// having equal author, committer, and commit message. + /// This is used by the frontend similar to the `change_id` to group matching commits. + #[serde(with = "gitbutler_serde::oid_opt")] + pub copied_from_remote_id: Option, pub remote_ref: Option, } @@ -45,6 +50,7 @@ pub(crate) fn commit_to_vbranch_commit( commit: &git2::Commit, is_integrated: bool, is_remote: bool, + copied_from_remote_id: Option, ) -> Result { let timestamp = u128::try_from(commit.time().seconds())?; let message = commit.message_bstr().to_owned(); @@ -81,6 +87,7 @@ pub(crate) fn commit_to_vbranch_commit( change_id: commit.change_id(), is_signed: commit.is_signed(), conflicted: commit.is_conflicted(), + copied_from_remote_id, remote_ref, }; diff --git a/crates/gitbutler-branch-actions/src/virtual.rs b/crates/gitbutler-branch-actions/src/virtual.rs index 02069ac12b..f599f70fe5 100644 --- a/crates/gitbutler-branch-actions/src/virtual.rs +++ b/crates/gitbutler-branch-actions/src/virtual.rs @@ -1,10 +1,3 @@ -use std::{ - borrow::Cow, - collections::HashMap, - path::{Path, PathBuf}, - vec, -}; - use crate::{ branch_manager::BranchManagerExt, commit::{commit_to_vbranch_commit, VirtualBranchCommit}, @@ -17,7 +10,7 @@ use crate::{ Get, VirtualBranchesExt, }; use anyhow::{anyhow, bail, Context, Result}; -use bstr::ByteSlice; +use bstr::{BString, ByteSlice}; use git2_hooks::HookResult; use gitbutler_branch::{ dedup, dedup_fmt, reconcile_claims, signature, Branch, BranchId, BranchOwnershipClaims, @@ -38,6 +31,13 @@ use gitbutler_repo::{ }; use gitbutler_time::time::now_since_unix_epoch_ms; use serde::Serialize; +use std::collections::HashSet; +use std::{ + borrow::Cow, + collections::HashMap, + path::{Path, PathBuf}, + vec, +}; use tracing::instrument; // this struct is a mapping to the view `Branch` type in Typescript @@ -313,19 +313,34 @@ pub fn list_virtual_branches_cached( ))?; // find upstream commits if we found an upstream reference - let mut pushed_commits = HashMap::new(); - if let Some(upstream) = &upstram_branch_commit { - let merge_base = - repo.merge_base(upstream.id(), default_target.sha) - .context(format!( - "failed to find merge base between {} and {}", - upstream.id(), - default_target.sha - ))?; - for oid in ctx.l(upstream.id(), LogUntil::Commit(merge_base))? { - pushed_commits.insert(oid, true); - } - } + let (remote_commit_ids, remote_commit_data) = upstram_branch_commit + .as_ref() + .map( + |upstream| -> Result<(HashSet, HashMap)> { + let merge_base = + repo.merge_base(upstream.id(), default_target.sha) + .context(format!( + "failed to find merge base between {} and {}", + upstream.id(), + default_target.sha + ))?; + let remote_commit_ids = + HashSet::from_iter(ctx.l(upstream.id(), LogUntil::Commit(merge_base))?); + let remote_commit_data: HashMap<_, _> = remote_commit_ids + .iter() + .copied() + .filter_map(|id| repo.find_commit(id).ok()) + .filter_map(|commit| { + CommitData::try_from(&commit) + .ok() + .map(|key| (key, commit.id())) + }) + .collect(); + Ok((remote_commit_ids, remote_commit_data)) + }, + ) + .transpose()? + .unwrap_or_default(); let mut is_integrated = false; let mut is_remote = false; @@ -346,7 +361,9 @@ pub fn list_virtual_branches_cached( is_remote = if is_remote { is_remote } else { - pushed_commits.contains_key(&commit.id()) + // This can only work once we have pushed our commits to the remote. + // Otherwise, even local commits created from a remote commit will look different. + remote_commit_ids.contains(&commit.id()) }; // only check for integration if we haven't already found an integration @@ -354,7 +371,18 @@ pub fn list_virtual_branches_cached( is_integrated = check_commit.is_integrated(commit)? }; - commit_to_vbranch_commit(ctx, &branch, commit, is_integrated, is_remote) + let copied_from_remote_id = CommitData::try_from(commit) + .ok() + .and_then(|data| remote_commit_data.get(&data).copied()); + + commit_to_vbranch_commit( + ctx, + &branch, + commit, + is_integrated, + is_remote, + copied_from_remote_id, + ) }) .collect::>>()? }; @@ -427,6 +455,40 @@ pub fn list_virtual_branches_cached( Ok((branches, status.skipped_files)) } +/// The commit-data we can use for comparison to see which remote-commit was used to craete +/// a local commit from. +/// Note that trees can't be used for comparison as these are typically rebased. +#[derive(Hash, Eq, PartialEq)] +struct CommitData { + message: BString, + author: gix::actor::Signature, + committer: gix::actor::Signature, +} + +impl TryFrom<&git2::Commit<'_>> for CommitData { + type Error = anyhow::Error; + + fn try_from(commit: &git2::Commit<'_>) -> std::result::Result { + Ok(CommitData { + message: commit.message_raw_bytes().into(), + author: git2_signature_to_gix_signature(commit.author()), + committer: git2_signature_to_gix_signature(commit.committer()), + }) + } +} + +fn git2_signature_to_gix_signature(input: git2::Signature<'_>) -> gix::actor::Signature { + gix::actor::Signature { + name: input.name_bytes().into(), + email: input.email_bytes().into(), + time: gix::date::Time { + seconds: input.when().seconds(), + offset: input.when().offset_minutes() * 60, + sign: input.when().offset_minutes().into(), + }, + } +} + fn branches_with_large_files_abridged(mut branches: Vec) -> Vec { for branch in &mut branches { for file in &mut branch.files { diff --git a/crates/gitbutler-branch-actions/tests/virtual_branches/update_base_branch.rs b/crates/gitbutler-branch-actions/tests/virtual_branches/update_base_branch.rs index fe2214437b..1d67a2740e 100644 --- a/crates/gitbutler-branch-actions/tests/virtual_branches/update_base_branch.rs +++ b/crates/gitbutler-branch-actions/tests/virtual_branches/update_base_branch.rs @@ -415,6 +415,11 @@ mod applied_branch { assert_eq!(branches[0].files.len(), 1); assert_eq!(branches[0].commits.len(), 1); assert!(!branches[0].commits[0].is_remote); + assert!( + branches[0].commits[0].copied_from_remote_id.is_some(), + "it's copied, which displays things differently in the \ + UI which knows what remote commit it relates to" + ); assert!(!branches[0].commits[0].is_integrated); } }