From 4668a1ec3d75951c3b583e7f27b10cdd970ced2a Mon Sep 17 00:00:00 2001 From: Kiril Videlov Date: Fri, 23 Aug 2024 14:32:31 +0200 Subject: [PATCH] update create and update reference apis No need to provide the change_id as it can be obtained from the commit itself --- crates/gitbutler-repo/src/rebase.rs | 1 - crates/gitbutler-repo/src/reference.rs | 27 ++++++++++-------- crates/gitbutler-repo/tests/reference.rs | 35 ++++++++++-------------- 3 files changed, 30 insertions(+), 33 deletions(-) diff --git a/crates/gitbutler-repo/src/rebase.rs b/crates/gitbutler-repo/src/rebase.rs index 0e8514763..bf5eaca08 100644 --- a/crates/gitbutler-repo/src/rebase.rs +++ b/crates/gitbutler-repo/src/rebase.rs @@ -117,7 +117,6 @@ fn update_existing_branch_references( reference.branch_id, reference.upstream.clone(), new_commit.id(), - new_commit.change_id(), ) .map(Some) }) diff --git a/crates/gitbutler-repo/src/reference.rs b/crates/gitbutler-repo/src/reference.rs index ae54b76d0..2bf1e1532 100644 --- a/crates/gitbutler-repo/src/reference.rs +++ b/crates/gitbutler-repo/src/reference.rs @@ -9,6 +9,7 @@ use gitbutler_branch::BranchReference; use gitbutler_branch::VirtualBranchesHandle; use gitbutler_branch::{Branch, BranchId}; use gitbutler_command_context::CommandContext; +use gitbutler_commit::commit_ext::CommitExt; use gitbutler_reference::ReferenceName; use itertools::Itertools; @@ -55,7 +56,6 @@ pub fn create_branch_reference( branch_id: BranchId, upstream: ReferenceName, commit_id: git2::Oid, - change_id: Option, ) -> Result { // The reference must be parseable as a remote reference gitbutler_reference::RemoteRefname::from_str(&upstream) @@ -64,11 +64,18 @@ pub fn create_branch_reference( // The branch must exist let mut vbranch = handle.get_branch(branch_id)?; + + // Enusre that the commit acutally exists + let commit = ctx + .repository() + .find_commit(commit_id) + .context(anyhow!("Commit {} does not exist", commit_id))?; + let branch_reference = BranchReference { upstream, branch_id, commit_id, - change_id, + change_id: commit.change_id(), }; let all_references = handle .list_all_branches()? @@ -107,13 +114,11 @@ 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. -/// If the change ID is provided, it will be updated, otherwise it will be left unchanged. pub fn update_branch_reference( ctx: &CommandContext, branch_id: BranchId, upstream: ReferenceName, new_commit_id: git2::Oid, - new_change_id: Option, ) -> Result { // The reference must be parseable as a remote reference gitbutler_reference::RemoteRefname::from_str(&upstream) @@ -122,6 +127,12 @@ pub fn update_branch_reference( // The branch must exist 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))?; + // Fail early if the commit is not valid validate_commit(&vbranch, new_commit_id, ctx, &handle)?; @@ -135,7 +146,7 @@ pub fn update_branch_reference( branch_id ))?; reference.commit_id = new_commit_id; - reference.change_id = new_change_id.or(reference.change_id.clone()); + reference.change_id = new_commit.change_id(); let new_reference = reference.clone(); handle.set_branch(vbranch)?; Ok(new_reference) @@ -171,7 +182,6 @@ pub fn push_branch_reference( /// 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 -/// - The commit actually exists /// - The commit is between the branch base and the branch head fn validate_commit( vbranch: &Branch, @@ -179,11 +189,6 @@ fn validate_commit( ctx: &CommandContext, handle: &VirtualBranchesHandle, ) -> Result<()> { - // Enusre that the commit acutally exists - ctx.repository() - .find_commit(commit_id) - .context(anyhow!("Commit {} does not exist", commit_id))?; - let target = handle.get_default_target()?; let branch_commits = ctx .log(vbranch.head, LogUntil::Commit(target.sha))? diff --git a/crates/gitbutler-repo/tests/reference.rs b/crates/gitbutler-repo/tests/reference.rs index e35aa1f6b..d9862cdba 100644 --- a/crates/gitbutler-repo/tests/reference.rs +++ b/crates/gitbutler-repo/tests/reference.rs @@ -1,6 +1,7 @@ use anyhow::Result; 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, @@ -16,12 +17,14 @@ fn create_success() -> Result<()> { test_ctx.branch.id, "refs/remotes/origin/success".into(), test_ctx.commits.first().unwrap().id(), - Some("change-id".into()), )?; 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.change_id, Some("change-id".into())); + assert_eq!( + reference.change_id, + test_ctx.commits.first().unwrap().change_id() + ); Ok(()) } @@ -34,23 +37,24 @@ fn create_multiple() -> Result<()> { test_ctx.branch.id, "refs/remotes/origin/first".into(), test_ctx.commits.first().unwrap().id(), - None, )?; 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.change_id, None); + assert_eq!( + first.change_id, + test_ctx.commits.first().unwrap().change_id() + ); let last = create_branch_reference( &ctx, test_ctx.branch.id, "refs/remotes/origin/last".into(), test_ctx.commits.last().unwrap().id(), - None, )?; 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, None); + assert_eq!(last.change_id, test_ctx.commits.last().unwrap().change_id()); Ok(()) } @@ -63,7 +67,6 @@ fn create_fails_with_non_remote_reference() -> Result<()> { test_ctx.branch.id, "foo".into(), test_ctx.commits.first().unwrap().id(), - None, ); assert_eq!( result.unwrap_err().to_string(), @@ -81,14 +84,12 @@ fn create_fails_when_branch_reference_with_name_exists() -> Result<()> { test_ctx.branch.id, "refs/remotes/origin/taken".into(), test_ctx.commits.first().unwrap().id(), - None, )?; let result = create_branch_reference( &ctx, test_ctx.branch.id, "refs/remotes/origin/taken".into(), test_ctx.commits.last().unwrap().id(), - None, ); assert_eq!( result.unwrap_err().to_string(), @@ -107,14 +108,12 @@ fn create_fails_when_commit_already_referenced() -> Result<()> { test_ctx.branch.id, "refs/remotes/origin/one".into(), test_ctx.commits.first().unwrap().id(), - None, )?; let result = create_branch_reference( &ctx, test_ctx.branch.id, "refs/remotes/origin/two".into(), test_ctx.commits.first().unwrap().id(), - None, ); assert_eq!( result.unwrap_err().to_string(), @@ -137,7 +136,6 @@ fn create_fails_when_commit_in_anothe_branch() -> Result<()> { test_ctx.branch.id, "refs/remotes/origin/asdf".into(), wrong_commit, - None, ); assert_eq!( result.unwrap_err().to_string(), @@ -158,7 +156,6 @@ fn create_fails_when_commit_is_the_base() -> Result<()> { test_ctx.branch.id, "refs/remotes/origin/baz".into(), test_ctx.branch_base, - None, ); assert_eq!( result.unwrap_err().to_string(), @@ -179,14 +176,12 @@ fn list_success() -> Result<()> { test_ctx.branch.id, "refs/remotes/origin/first".into(), test_ctx.commits.first().unwrap().id(), - Some("change-id".into()), )?; let second_ref = create_branch_reference( &ctx, test_ctx.branch.id, "refs/remotes/origin/second".into(), test_ctx.commits.last().unwrap().id(), - Some("change-id".into()), )?; let result = list_branch_references(&ctx, test_ctx.branch.id)?; assert_eq!(result.len(), 2); @@ -204,16 +199,18 @@ fn update_success() -> Result<()> { test_ctx.branch.id, "refs/remotes/origin/first".into(), test_ctx.commits.first().unwrap().id(), - Some("change-id".into()), )?; let updated = update_branch_reference( &ctx, test_ctx.branch.id, "refs/remotes/origin/first".into(), test_ctx.commits.last().unwrap().id(), - None, )?; assert_eq!(updated.commit_id, test_ctx.commits.last().unwrap().id()); + assert_eq!( + updated.change_id, + test_ctx.commits.last().unwrap().change_id() + ); 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()); @@ -229,7 +226,6 @@ fn push_success() -> Result<()> { test_ctx.branch.id, "refs/remotes/origin/first".into(), test_ctx.commits.first().unwrap().id(), - Some("change-id".into()), )?; let result = push_branch_reference( &ctx, @@ -251,21 +247,18 @@ fn list_by_commits_success() -> Result<()> { test_ctx.branch.id, "refs/remotes/origin/first".into(), test_ctx.commits.first().unwrap().id(), - None, )?; let second = create_branch_reference( &ctx, test_ctx.branch.id, "refs/remotes/origin/second".into(), test_ctx.commits.last().unwrap().id(), - None, )?; let third = create_branch_reference( &ctx, test_ctx.other_branch.id, "refs/remotes/origin/third".into(), test_ctx.other_commits.first().unwrap().id(), - None, )?; let commits = vec![ test_ctx.commits.first().unwrap().id(),