refactor this to be a ChangeReference

it turns out that a reference to a change_id is all that we need
This commit is contained in:
Kiril Videlov 2024-08-26 17:02:38 +02:00
parent 2ddb032ebf
commit 89d1186b06
No known key found for this signature in database
GPG Key ID: A4C733025427C471
9 changed files with 113 additions and 185 deletions

View File

@ -1,6 +1,6 @@
use anyhow::{Context, Result};
use gitbutler_branch::{
BranchCreateRequest, BranchId, BranchOwnershipClaims, BranchReference, BranchUpdateRequest,
BranchCreateRequest, BranchId, BranchOwnershipClaims, BranchUpdateRequest, ChangeReference,
};
use gitbutler_command_context::CommandContext;
use gitbutler_operating_modes::assure_open_workspace_mode;
@ -363,10 +363,10 @@ impl VirtualBranchActions {
branch_id: BranchId,
reference: ReferenceName,
commit_oid: git2::Oid,
) -> Result<BranchReference> {
) -> Result<ChangeReference> {
let ctx = open_with_verify(project)?;
assure_open_workspace_mode(&ctx).context("Requires an open workspace mode")?;
gitbutler_repo::create_branch_reference(&ctx, branch_id, reference, commit_oid)
gitbutler_repo::create_change_reference(&ctx, branch_id, reference, commit_oid)
}
pub fn reorder_commit(

View File

@ -59,10 +59,14 @@ pub(crate) fn commit_to_vbranch_commit(
})
.collect::<Vec<_>>();
let remote_ref = list_branch_references(ctx, branch.id)
.map(|references| references.into_iter().find(|r| r.commit_id == commit.id()))
.map(|references| {
references
.into_iter()
.find(|r| Some(r.change_id.clone()) == commit.change_id())
})
.ok()
.flatten()
.map(|r| r.upstream);
.map(|r| r.name);
let commit = VirtualBranchCommit {
id: commit.id(),

View File

@ -5,7 +5,7 @@ use gitbutler_reference::{normalize_branch_name, Refname, RemoteRefname, Virtual
use serde::{Deserialize, Serialize, Serializer};
use std::ops::Deref;
use crate::{ownership::BranchOwnershipClaims, reference::BranchReference};
use crate::{ownership::BranchOwnershipClaims, reference::ChangeReference};
pub type BranchId = Id<Branch>;
@ -68,7 +68,7 @@ pub struct Branch {
#[serde(default)]
pub not_in_workspace_wip_change_id: Option<String>,
#[serde(default)]
pub references: Vec<BranchReference>,
pub references: Vec<ChangeReference>,
}
fn default_true() -> bool {

View File

@ -17,7 +17,7 @@ pub mod serde;
mod target;
pub use target::Target;
mod reference;
pub use reference::BranchReference;
pub use reference::ChangeReference;
mod state;
use lazy_static::lazy_static;

View File

@ -3,22 +3,18 @@ use serde::{Deserialize, Serialize};
use crate::BranchId;
/// GitButler reference associated with a virtual branch.
/// GitButler reference associated with a change (commit) on a virtual branch.
/// These are not the same as regular Git references, but rather app-managed refs.
/// Represent a deployable / reviewable part of a virtual branch that can be pushed to a remote
/// and have a "Pull Request" created for it.
// TODO(kv): There is a name collision with `VirtualBranchReference` in `gitbutler-branch-actions/src/branch.rs` where this name means something entirerly different.
#[derive(Serialize, Deserialize, Debug, PartialEq, Clone)]
pub struct BranchReference {
pub struct ChangeReference {
/// Branch id of the virtual branch this reference belongs to
/// Multiple references may belong to the same virtual branch, representing separate deployable / reviewable parts of the vbranch.
pub branch_id: BranchId,
/// Fully qualified reference name.
/// The reference must be a remote reference.
pub upstream: ReferenceName,
/// The commit this reference points to. The commit must be part of the virtual branch.
#[serde(with = "gitbutler_serde::oid")]
pub commit_id: git2::Oid,
/// The change id associated with the commit, if any.
pub change_id: Option<String>,
pub name: ReferenceName,
/// The change id this reference points to.
pub change_id: String,
}

View File

@ -22,6 +22,5 @@ mod conflicts;
mod reference;
pub use reference::{
create_branch_reference, list_branch_references, list_commit_references, push_branch_reference,
update_branch_reference,
create_change_reference, list_branch_references, push_change_reference, update_change_reference,
};

View File

@ -1,9 +1,6 @@
use std::collections::HashMap;
use anyhow::{anyhow, Context, Result};
use bstr::ByteSlice;
use git2::{build::TreeUpdateBuilder, Repository};
use gitbutler_branch::BranchReference;
use gitbutler_command_context::CommandContext;
use gitbutler_commit::{
commit_ext::CommitExt,
@ -13,10 +10,7 @@ use gitbutler_error::error::Marker;
use tempfile::tempdir;
use uuid::Uuid;
use crate::{
conflicts::ConflictedTreeKey, list_commit_references, update_branch_reference, LogUntil,
RepoActionsExt, RepositoryExt,
};
use crate::{conflicts::ConflictedTreeKey, LogUntil, RepoActionsExt, RepositoryExt};
/// cherry-pick based rebase, which handles empty commits
/// this function takes a commit range and generates a Vector of commit oids
@ -51,8 +45,6 @@ pub fn cherry_rebase_group(
ids_to_rebase: &mut [git2::Oid],
) -> Result<git2::Oid> {
ids_to_rebase.reverse();
// Attempt to list any GitButler references pointing to the commits to rebase, and default to an empty list if it fails
let references = list_commit_references(ctx, ids_to_rebase.to_vec()).unwrap_or_default();
// now, rebase unchanged commits onto the new commit
let commits_to_rebase = ids_to_rebase
.iter()
@ -75,7 +67,7 @@ pub fn cherry_rebase_group(
.cherry_pick_gitbutler(&head, &to_rebase)
.context("failed to cherry pick")?;
let result = if cherrypick_index.has_conflicts() {
if cherrypick_index.has_conflicts() {
if !ctx.project().succeeding_rebases {
return Err(anyhow!("failed to rebase")).context(Marker::BranchConflict);
}
@ -87,14 +79,7 @@ pub fn cherry_rebase_group(
to_rebase,
cherrypick_index,
)
};
if let Ok(commit) = &result {
// If the commit got successfully rebased and if there were any references pointiong to it,
// update them to point to the new commit. Ignore any errors that might occur during this process.
// TODO: some logging on error would be nice
let _ = update_existing_branch_references(ctx, &references, &head, commit);
}
result
},
)?
.id();
@ -102,26 +87,6 @@ pub fn cherry_rebase_group(
Ok(new_head_id)
}
fn update_existing_branch_references(
ctx: &CommandContext,
references: &HashMap<git2::Oid, Option<BranchReference>>,
old_commit: &git2::Commit,
new_commit: &git2::Commit,
) -> Result<Option<BranchReference>> {
references
.get(&old_commit.id())
.and_then(|reference| reference.as_ref())
.map_or(Ok(None), |reference| {
update_branch_reference(
ctx,
reference.branch_id,
reference.upstream.clone(),
new_commit.id(),
)
.map(Some)
})
}
fn commit_unconflicted_cherry_result<'repository>(
ctx: &'repository CommandContext,
head: git2::Commit<'repository>,

View File

@ -1,11 +1,10 @@
use std::collections::HashMap;
use std::str::FromStr;
use crate::credentials::Helper;
use crate::{LogUntil, RepoActionsExt};
use anyhow::Context;
use anyhow::{anyhow, Result};
use gitbutler_branch::BranchReference;
use gitbutler_branch::ChangeReference;
use gitbutler_branch::VirtualBranchesHandle;
use gitbutler_branch::{Branch, BranchId};
use gitbutler_command_context::CommandContext;
@ -18,45 +17,22 @@ use itertools::Itertools;
pub fn list_branch_references(
ctx: &CommandContext,
branch_id: BranchId,
) -> Result<Vec<BranchReference>> {
) -> Result<Vec<ChangeReference>> {
let handle = VirtualBranchesHandle::new(ctx.project().gb_dir());
let vbranch = handle.get_branch(branch_id)?;
Ok(vbranch.references)
}
/// Given a list of commits ids, returns a map of commit ids to the references that point to them or None
pub fn list_commit_references(
ctx: &CommandContext,
commits: Vec<git2::Oid>,
) -> Result<HashMap<git2::Oid, Option<BranchReference>>> {
let handle = VirtualBranchesHandle::new(ctx.project().gb_dir());
let all_references = handle
.list_all_branches()?
.into_iter()
.flat_map(|branch| branch.references)
.collect_vec();
Ok(commits
.into_iter()
.map(|commit_id| {
let reference = all_references
.iter()
.find(|r| r.commit_id == commit_id)
.cloned();
(commit_id, reference)
})
.collect())
}
/// Creates a new virtual branch reference and associates it with the branch.
/// However this will return an error if:
/// - a reference for the same commit already exists, an error is returned.
/// - the reference name already exists, an error is returned.
pub fn create_branch_reference(
pub fn create_change_reference(
ctx: &CommandContext,
branch_id: BranchId,
upstream: ReferenceName,
commit_id: git2::Oid,
) -> Result<BranchReference> {
) -> Result<ChangeReference> {
// The reference must be parseable as a remote reference
gitbutler_reference::RemoteRefname::from_str(&upstream)
.context("Failed to parse the provided reference")?;
@ -71,11 +47,14 @@ pub fn create_branch_reference(
.find_commit(commit_id)
.context(anyhow!("Commit {} does not exist", commit_id))?;
let branch_reference = BranchReference {
upstream,
let change_id = commit
.change_id()
.ok_or(anyhow!("Commit {} does not have a change id", commit_id))?;
let branch_reference = ChangeReference {
name: upstream,
branch_id,
commit_id,
change_id: commit.change_id(),
change_id: change_id.clone(),
};
let all_references = handle
.list_all_branches()?
@ -85,15 +64,15 @@ pub fn create_branch_reference(
// Ensure the reference name does not already exist
if all_references
.iter()
.any(|r| r.upstream == branch_reference.upstream)
.any(|r| r.name == branch_reference.name)
{
return Err(anyhow!(
"A reference {} already exists",
branch_reference.upstream
branch_reference.name
));
}
// Ensure the commit is not already referenced
if all_references.iter().any(|r| r.commit_id == commit_id) {
// Ensure the change is not already referenced
if all_references.iter().any(|r| r.change_id == change_id) {
return Err(anyhow!(
"A reference for commit {} already exists",
commit_id
@ -114,12 +93,12 @@ pub fn create_branch_reference(
/// - the reference exists, but the commit id is already associated with another reference
///
/// If the commit ID is the same as the current commit ID, the function is a no-op.
pub fn update_branch_reference(
pub fn update_change_reference(
ctx: &CommandContext,
branch_id: BranchId,
upstream: ReferenceName,
new_commit_id: git2::Oid,
) -> Result<BranchReference> {
new_change_id: String,
) -> Result<ChangeReference> {
// The reference must be parseable as a remote reference
gitbutler_reference::RemoteRefname::from_str(&upstream)
.context("Failed to parse the provided reference")?;
@ -128,32 +107,30 @@ pub fn update_branch_reference(
let mut vbranch = handle.get_branch(branch_id)?;
// Enusre that the commit acutally exists
let new_commit = ctx
.repository()
.find_commit(new_commit_id)
.context(anyhow!("Commit {} does not exist", new_commit_id))?;
let new_commit =
commit_by_branch_id_and_change_id(ctx, &vbranch, &handle, new_change_id.clone())
.context(anyhow!("Commit for change_id {} not found", new_change_id))?;
// Fail early if the commit is not valid
validate_commit(&vbranch, new_commit_id, ctx, &handle)?;
validate_commit(&vbranch, new_commit.id(), ctx, &handle)?;
let reference = vbranch
.references
.iter_mut()
.find(|r| r.upstream == upstream)
.find(|r| r.name == upstream)
.ok_or(anyhow!(
"Reference {} not found for branch {}",
upstream,
branch_id
))?;
reference.commit_id = new_commit_id;
reference.change_id = new_commit.change_id();
reference.change_id = new_change_id;
let new_reference = reference.clone();
handle.set_branch(vbranch)?;
Ok(new_reference)
}
/// Pushes a gitbutler branch reference to the remote repository.
pub fn push_branch_reference(
pub fn push_change_reference(
ctx: &CommandContext,
branch_id: BranchId,
upstream: ReferenceName,
@ -165,12 +142,14 @@ pub fn push_branch_reference(
let reference = vbranch
.references
.iter()
.find(|r| r.upstream == upstream)
.find(|r| r.name == upstream)
.ok_or_else(|| anyhow!("Reference {} not found", upstream))?;
let upstream_refname = gitbutler_reference::RemoteRefname::from_str(&reference.upstream)
let upstream_refname = gitbutler_reference::RemoteRefname::from_str(&reference.name)
.context("Failed to parse the provided reference")?;
let commit =
commit_by_branch_id_and_change_id(ctx, &vbranch, &handle, reference.change_id.clone())?;
ctx.push(
&reference.commit_id,
&commit.id(),
&upstream_refname,
with_force,
credentials,
@ -179,6 +158,32 @@ pub fn push_branch_reference(
)
}
/// Given a branch id and a change id, returns the commit associated with the change id.
// TODO: We need a more efficient way of getting a commit by change id.
fn commit_by_branch_id_and_change_id<'a>(
ctx: &'a CommandContext,
vbranch: &Branch,
handle: &VirtualBranchesHandle,
change_id: String,
) -> Result<git2::Commit<'a>> {
let target = handle.get_default_target()?;
let branch_commits = ctx
.log(vbranch.head, LogUntil::Commit(target.sha))?
.iter()
.map(|c| c.id())
.collect_vec();
// Find the commit with the change id
let commit = branch_commits
.iter()
.find(|c| {
let commit = ctx.repository().find_commit(**c).expect("Commit not found");
commit.change_id() == Some(change_id.clone())
})
.map(|c| ctx.repository().find_commit(*c).expect("Commit not found"))
.ok_or_else(|| anyhow!("Commit with change id {} not found", change_id))?;
Ok(commit)
}
/// Validates a commit in the following ways:
/// - The reference does not already exists for any other branch
/// - There is no other reference already pointing to the commit

View File

@ -3,8 +3,8 @@ use gitbutler_branch::VirtualBranchesHandle;
use gitbutler_command_context::CommandContext;
use gitbutler_commit::commit_ext::CommitExt;
use gitbutler_repo::{
create_branch_reference, credentials::Helper, list_branch_references, list_commit_references,
push_branch_reference, update_branch_reference, LogUntil, RepoActionsExt,
create_change_reference, credentials::Helper, list_branch_references, push_change_reference,
update_change_reference, LogUntil, RepoActionsExt,
};
use tempfile::TempDir;
@ -12,18 +12,17 @@ use tempfile::TempDir;
fn create_success() -> Result<()> {
let (ctx, _temp_dir) = command_ctx("multiple-commits")?;
let test_ctx = test_ctx(&ctx)?;
let reference = create_branch_reference(
let reference = create_change_reference(
&ctx,
test_ctx.branch.id,
"refs/remotes/origin/success".into(),
test_ctx.commits.first().unwrap().id(),
)?;
assert_eq!(reference.branch_id, test_ctx.branch.id);
assert_eq!(reference.upstream, "refs/remotes/origin/success".into());
assert_eq!(reference.commit_id, test_ctx.commits.first().unwrap().id());
assert_eq!(reference.name, "refs/remotes/origin/success".into());
assert_eq!(
reference.change_id,
test_ctx.commits.first().unwrap().change_id()
test_ctx.commits.first().unwrap().change_id().unwrap()
);
Ok(())
}
@ -32,29 +31,30 @@ fn create_success() -> Result<()> {
fn create_multiple() -> Result<()> {
let (ctx, _temp_dir) = command_ctx("multiple-commits")?;
let test_ctx = test_ctx(&ctx)?;
let first = create_branch_reference(
let first = create_change_reference(
&ctx,
test_ctx.branch.id,
"refs/remotes/origin/first".into(),
test_ctx.commits.first().unwrap().id(),
)?;
assert_eq!(first.branch_id, test_ctx.branch.id);
assert_eq!(first.upstream, "refs/remotes/origin/first".into());
assert_eq!(first.commit_id, test_ctx.commits.first().unwrap().id());
assert_eq!(first.name, "refs/remotes/origin/first".into());
assert_eq!(
first.change_id,
test_ctx.commits.first().unwrap().change_id()
test_ctx.commits.first().unwrap().change_id().unwrap()
);
let last = create_branch_reference(
let last = create_change_reference(
&ctx,
test_ctx.branch.id,
"refs/remotes/origin/last".into(),
test_ctx.commits.last().unwrap().id(),
)?;
assert_eq!(last.branch_id, test_ctx.branch.id);
assert_eq!(last.upstream, "refs/remotes/origin/last".into());
assert_eq!(last.commit_id, test_ctx.commits.last().unwrap().id());
assert_eq!(last.change_id, test_ctx.commits.last().unwrap().change_id());
assert_eq!(last.name, "refs/remotes/origin/last".into());
assert_eq!(
last.change_id,
test_ctx.commits.last().unwrap().change_id().unwrap()
);
Ok(())
}
@ -62,7 +62,7 @@ fn create_multiple() -> Result<()> {
fn create_fails_with_non_remote_reference() -> Result<()> {
let (ctx, _temp_dir) = command_ctx("multiple-commits")?;
let test_ctx = test_ctx(&ctx)?;
let result = create_branch_reference(
let result = create_change_reference(
&ctx,
test_ctx.branch.id,
"foo".into(),
@ -79,13 +79,13 @@ fn create_fails_with_non_remote_reference() -> Result<()> {
fn create_fails_when_branch_reference_with_name_exists() -> Result<()> {
let (ctx, _temp_dir) = command_ctx("multiple-commits")?;
let test_ctx = test_ctx(&ctx)?;
create_branch_reference(
create_change_reference(
&ctx,
test_ctx.branch.id,
"refs/remotes/origin/taken".into(),
test_ctx.commits.first().unwrap().id(),
)?;
let result = create_branch_reference(
let result = create_change_reference(
&ctx,
test_ctx.branch.id,
"refs/remotes/origin/taken".into(),
@ -103,13 +103,13 @@ fn create_fails_when_branch_reference_with_name_exists() -> Result<()> {
fn create_fails_when_commit_already_referenced() -> Result<()> {
let (ctx, _temp_dir) = command_ctx("multiple-commits")?;
let test_ctx = test_ctx(&ctx)?;
create_branch_reference(
create_change_reference(
&ctx,
test_ctx.branch.id,
"refs/remotes/origin/one".into(),
test_ctx.commits.first().unwrap().id(),
)?;
let result = create_branch_reference(
let result = create_change_reference(
&ctx,
test_ctx.branch.id,
"refs/remotes/origin/two".into(),
@ -131,7 +131,7 @@ fn create_fails_when_commit_in_anothe_branch() -> Result<()> {
let (ctx, _temp_dir) = command_ctx("multiple-commits")?;
let test_ctx = test_ctx(&ctx)?;
let wrong_commit = test_ctx.other_commits.first().unwrap().id();
let result = create_branch_reference(
let result = create_change_reference(
&ctx,
test_ctx.branch.id,
"refs/remotes/origin/asdf".into(),
@ -151,19 +151,13 @@ fn create_fails_when_commit_in_anothe_branch() -> Result<()> {
fn create_fails_when_commit_is_the_base() -> Result<()> {
let (ctx, _temp_dir) = command_ctx("multiple-commits")?;
let test_ctx = test_ctx(&ctx)?;
let result = create_branch_reference(
let result = create_change_reference(
&ctx,
test_ctx.branch.id,
"refs/remotes/origin/baz".into(),
test_ctx.branch_base,
);
assert_eq!(
result.unwrap_err().to_string(),
format!(
"The commit {} is not between the branch base and the branch head",
test_ctx.branch_base
),
);
assert!(result.is_err());
Ok(())
}
@ -171,13 +165,13 @@ fn create_fails_when_commit_is_the_base() -> Result<()> {
fn list_success() -> Result<()> {
let (ctx, _temp_dir) = command_ctx("multiple-commits")?;
let test_ctx = test_ctx(&ctx)?;
let first_ref = create_branch_reference(
let first_ref = create_change_reference(
&ctx,
test_ctx.branch.id,
"refs/remotes/origin/first".into(),
test_ctx.commits.first().unwrap().id(),
)?;
let second_ref = create_branch_reference(
let second_ref = create_change_reference(
&ctx,
test_ctx.branch.id,
"refs/remotes/origin/second".into(),
@ -194,26 +188,28 @@ fn list_success() -> Result<()> {
fn update_success() -> Result<()> {
let (ctx, _temp_dir) = command_ctx("multiple-commits")?;
let test_ctx = test_ctx(&ctx)?;
create_branch_reference(
create_change_reference(
&ctx,
test_ctx.branch.id,
"refs/remotes/origin/first".into(),
test_ctx.commits.first().unwrap().id(),
)?;
let updated = update_branch_reference(
let updated = update_change_reference(
&ctx,
test_ctx.branch.id,
"refs/remotes/origin/first".into(),
test_ctx.commits.last().unwrap().id(),
test_ctx.commits.last().unwrap().change_id().unwrap(),
)?;
assert_eq!(updated.commit_id, test_ctx.commits.last().unwrap().id());
assert_eq!(
updated.change_id,
test_ctx.commits.last().unwrap().change_id()
test_ctx.commits.last().unwrap().change_id().unwrap()
);
let list = list_branch_references(&ctx, test_ctx.branch.id)?;
assert_eq!(list.len(), 1);
assert_eq!(list[0].commit_id, test_ctx.commits.last().unwrap().id());
assert_eq!(
list[0].change_id,
test_ctx.commits.last().unwrap().change_id().unwrap()
);
Ok(())
}
@ -221,16 +217,16 @@ fn update_success() -> Result<()> {
fn push_success() -> Result<()> {
let (ctx, _temp_dir) = command_ctx("multiple-commits")?;
let test_ctx = test_ctx(&ctx)?;
let reference = create_branch_reference(
let reference = create_change_reference(
&ctx,
test_ctx.branch.id,
"refs/remotes/origin/first".into(),
test_ctx.commits.first().unwrap().id(),
)?;
let result = push_branch_reference(
let result = push_change_reference(
&ctx,
reference.branch_id,
reference.upstream,
reference.name,
false,
&Helper::default(),
);
@ -238,43 +234,6 @@ fn push_success() -> Result<()> {
Ok(())
}
#[test]
fn list_by_commits_success() -> Result<()> {
let (ctx, _temp_dir) = command_ctx("multiple-commits")?;
let test_ctx = test_ctx(&ctx)?;
let first = create_branch_reference(
&ctx,
test_ctx.branch.id,
"refs/remotes/origin/first".into(),
test_ctx.commits.first().unwrap().id(),
)?;
let second = create_branch_reference(
&ctx,
test_ctx.branch.id,
"refs/remotes/origin/second".into(),
test_ctx.commits.last().unwrap().id(),
)?;
let third = create_branch_reference(
&ctx,
test_ctx.other_branch.id,
"refs/remotes/origin/third".into(),
test_ctx.other_commits.first().unwrap().id(),
)?;
let commits = vec![
test_ctx.commits.first().unwrap().id(),
test_ctx.commits.get(1).unwrap().id(),
test_ctx.commits.last().unwrap().id(),
test_ctx.other_commits.first().unwrap().id(),
];
let result = list_commit_references(&ctx, commits.clone())?;
assert_eq!(result.len(), 4);
assert_eq!(result.get(&commits[0]).unwrap().clone().unwrap(), first);
assert_eq!(result.get(&commits[1]).unwrap().clone(), None);
assert_eq!(result.get(&commits[2]).unwrap().clone().unwrap(), second);
assert_eq!(result.get(&commits[3]).unwrap().clone().unwrap(), third);
Ok(())
}
fn command_ctx(name: &str) -> Result<(CommandContext, TempDir)> {
gitbutler_testsupport::writable::fixture("stacking.sh", name)
}
@ -292,7 +251,7 @@ fn test_ctx(ctx: &CommandContext) -> Result<TestContext> {
branch: branch.clone(),
branch_base,
commits: branch_commits,
other_branch: other_branch.clone(),
// other_branch: other_branch.clone(),
other_commits,
})
}
@ -300,6 +259,6 @@ struct TestContext<'a> {
branch: gitbutler_branch::Branch,
branch_base: git2::Oid,
commits: Vec<git2::Commit<'a>>,
other_branch: gitbutler_branch::Branch,
// other_branch: gitbutler_branch::Branch,
other_commits: Vec<git2::Commit<'a>>,
}