diff --git a/app/src/lib/components/CommitCard.svelte b/app/src/lib/components/CommitCard.svelte index b0684b2f7..d51dc0e34 100644 --- a/app/src/lib/components/CommitCard.svelte +++ b/app/src/lib/components/CommitCard.svelte @@ -144,11 +144,18 @@
{#if $advancedCommitOperations} - {#if !showFiles} -
- {commit.id.substring(0, 6)} -
- {/if} +
+ + {#if commit.isSigned} + 🔒 + {/if} + {#if commit.changeId} + {commit.changeId.split('-')[0]} + {:else} + {commit.id.substring(0, 6)} + {/if} + +
{/if}
{#if isUndoable} diff --git a/app/src/lib/vbranches/types.ts b/app/src/lib/vbranches/types.ts index c72fd40d1..64b039b0f 100644 --- a/app/src/lib/vbranches/types.ts +++ b/app/src/lib/vbranches/types.ts @@ -162,6 +162,8 @@ export class Commit { files!: LocalFile[]; parentIds!: string[]; branchId!: string; + changeId!: string; + isSigned!: boolean; get isLocal() { return !this.isRemote && !this.isIntegrated; @@ -196,6 +198,8 @@ export class RemoteCommit { description!: string; @Transform((obj) => new Date(obj.value * 1000)) createdAt!: Date; + changeId!: string; + isSigned!: boolean; get isLocal() { return false; diff --git a/app/src/routes/settings/git/+page.svelte b/app/src/routes/settings/git/+page.svelte index 270a81229..881ec8088 100644 --- a/app/src/routes/settings/git/+page.svelte +++ b/app/src/routes/settings/git/+page.svelte @@ -16,7 +16,6 @@ const gitConfig = getContext(GitConfigService); const authService = getContext(AuthService); - let signCommits = false; let annotateCommits = true; let sshKey = ''; @@ -25,15 +24,9 @@ gitConfig.set('gitbutler.gitbutlerCommitter', annotateCommits ? '1' : '0'); } - function toggleSigningSetting() { - signCommits = !signCommits; - gitConfig.set('gitbutler.signCommits', signCommits ? 'true' : 'false'); - } - onMount(async () => { sshKey = await authService.getPublicKey(); annotateCommits = (await gitConfig.get('gitbutler.gitbutlerCommitter')) == '1'; - signCommits = (await gitConfig.get('gitbutler.signCommits')) == 'true'; }); @@ -82,22 +75,4 @@
- - - Sign commits with the above SSH key - - If you want GitButler to sign your commits with the SSH key we generated, then you can add - that key to GitHub as a signing key to have those commits verified. - - Learn more - - - - - - diff --git a/crates/gitbutler-core/Cargo.toml b/crates/gitbutler-core/Cargo.toml index 17b2808c0..c37822fb7 100644 --- a/crates/gitbutler-core/Cargo.toml +++ b/crates/gitbutler-core/Cargo.toml @@ -8,9 +8,8 @@ publish = false [dev-dependencies] once_cell = "1.19" pretty_assertions = "1.4" -tempfile = "3.10" gitbutler-testsupport.workspace = true -gitbutler-git = { workspace = true, features = ["test-askpass-path" ]} +gitbutler-git = { workspace = true, features = ["test-askpass-path"] } [dependencies] toml = "0.8.12" @@ -42,6 +41,7 @@ ssh-key = { version = "0.6.6", features = [ "alloc", "ed25519" ] } ssh2 = { version = "0.9.4", features = ["vendored-openssl"] } strum = { version = "0.26", features = ["derive"] } log = "^0.4" +tempfile = "3.10" thiserror.workspace = true tokio = { workspace = true, features = [ "rt-multi-thread", "rt", "macros" ] } tracing = "0.1.40" diff --git a/crates/gitbutler-core/src/gb_repository/repository.rs b/crates/gitbutler-core/src/gb_repository/repository.rs new file mode 100644 index 000000000..e69de29bb diff --git a/crates/gitbutler-core/src/git/commit.rs b/crates/gitbutler-core/src/git/commit.rs index 67a6407da..665d468ce 100644 --- a/crates/gitbutler-core/src/git/commit.rs +++ b/crates/gitbutler-core/src/git/commit.rs @@ -71,6 +71,22 @@ impl<'repo> Commit<'repo> { self.commit.committer().into() } + pub fn change_id(&self) -> Option { + let cid = self.commit.header_field_bytes("change-id").ok()?; + if cid.is_empty() { + None + } else { + // convert the Buf to a string + let ch_id = std::str::from_utf8(&cid).ok()?.to_owned(); + Some(ch_id) + } + } + + pub fn is_signed(&self) -> bool { + let cid = self.commit.header_field_bytes("gpgsig").ok(); + cid.is_some() + } + pub fn raw_header(&self) -> Option<&str> { self.commit.raw_header() } diff --git a/crates/gitbutler-core/src/git/repository.rs b/crates/gitbutler-core/src/git/repository.rs index 7c9430941..2d8b8e5c5 100644 --- a/crates/gitbutler-core/src/git/repository.rs +++ b/crates/gitbutler-core/src/git/repository.rs @@ -1,13 +1,14 @@ -use std::{io::Write, path::Path, str}; - -use git2::{BlameOptions, Submodule}; -use git2_hooks::HookResult; - use super::{ Blob, Branch, Commit, Config, Index, Oid, Reference, Refname, Remote, Result, Signature, Tree, TreeBuilder, Url, }; -use crate::{keys, path::Normalize}; +use crate::path::Normalize; +use git2::{BlameOptions, Submodule}; +use git2_hooks::HookResult; +#[cfg(unix)] +use std::os::unix::fs::PermissionsExt; +use std::process::Stdio; +use std::{io::Write, path::Path, str}; // wrapper around git2::Repository to get control over how it's used. pub struct Repository(git2::Repository); @@ -242,6 +243,7 @@ impl Repository { self.0.blob(data).map(Into::into).map_err(Into::into) } + #[allow(clippy::too_many_arguments)] pub fn commit( &self, update_ref: Option<&Refname>, @@ -250,51 +252,183 @@ impl Repository { message: &str, tree: &Tree<'_>, parents: &[&Commit<'_>], + change_id: Option<&str>, ) -> Result { let parents: Vec<&git2::Commit> = parents .iter() .map(|c| c.to_owned().into()) .collect::>(); - self.0 - .commit( - update_ref.map(ToString::to_string).as_deref(), - author.into(), - committer.into(), - message, - tree.into(), - &parents, - ) - .map(Into::into) - .map_err(Into::into) - } - pub fn commit_signed( - &self, - author: &Signature<'_>, - message: &str, - tree: &Tree<'_>, - parents: &[&Commit<'_>], - key: &keys::PrivateKey, - ) -> Result { - let parents: Vec<&git2::Commit> = parents - .iter() - .map(|c| c.to_owned().into()) - .collect::>(); let commit_buffer = self.0.commit_create_buffer( author.into(), - // author and committer must be the same - // for signed commits - author.into(), + committer.into(), message, tree.into(), &parents, )?; - let commit_buffer = str::from_utf8(&commit_buffer).unwrap(); - let signature = key.sign(commit_buffer.as_bytes())?; - self.0 - .commit_signed(commit_buffer, &signature, None) - .map(Into::into) - .map_err(Into::into) + + let commit_buffer = Self::inject_change_id(&commit_buffer, change_id)?; + + let oid = self.commit_buffer(commit_buffer)?; + + // update reference + if let Some(refname) = update_ref { + self.0.reference(&refname.to_string(), oid, true, message)?; + } + Ok(oid.into()) + } + + /// takes raw commit data and commits it to the repository + /// - if the git config commit.gpgSign is set, it will sign the commit + /// returns an oid of the new commit object + pub fn commit_buffer(&self, buffer: String) -> Result { + // check git config for gpg.signingkey + let should_sign = self.0.config()?.get_string("commit.gpgSign"); + if should_sign.unwrap_or("false".to_string()) != "false" { + // TODO: support gpg.ssh.defaultKeyCommand to get the signing key if this value doesn't exist + let signing_key = self.0.config()?.get_string("user.signingkey"); + if let Ok(signing_key) = signing_key { + let sign_format = self.0.config()?.get_string("gpg.format"); + let is_ssh = if let Ok(sign_format) = sign_format { + sign_format == "ssh" + } else { + false + }; + + if is_ssh { + // write commit data to a temp file so we can sign it + let mut signature_storage = tempfile::NamedTempFile::new()?; + signature_storage.write_all(buffer.as_ref())?; + let buffer_file_to_sign_path = signature_storage.into_temp_path(); + + let gpg_program = self.0.config()?.get_string("gpg.ssh.program"); + let mut cmd = + std::process::Command::new(gpg_program.unwrap_or("ssh-keygen".to_string())); + cmd.args(["-Y", "sign", "-n", "git", "-f"]); + + let output; + // support literal ssh key + if let (true, signing_key) = Self::is_literal_ssh_key(&signing_key) { + // write the key to a temp file + let mut key_storage = tempfile::NamedTempFile::new()?; + key_storage.write_all(signing_key.as_bytes())?; + + // if on unix + #[cfg(unix)] + { + // make sure the tempfile permissions are acceptable for a private ssh key + let mut permissions = key_storage.as_file().metadata()?.permissions(); + permissions.set_mode(0o600); + key_storage.as_file().set_permissions(permissions)?; + } + + let key_file_path = key_storage.into_temp_path(); + + cmd.arg(&key_file_path); + cmd.arg("-U"); + cmd.arg(&buffer_file_to_sign_path); + cmd.stdout(Stdio::piped()); + + let child = cmd.spawn()?; + output = child.wait_with_output()?; + } else { + cmd.arg(signing_key); + cmd.arg(&buffer_file_to_sign_path); + cmd.stdout(Stdio::piped()); + + let child = cmd.spawn()?; + output = child.wait_with_output()?; + } + + if output.status.success() { + // read signed_storage path plus .sig + let signature_path = buffer_file_to_sign_path.with_extension("sig"); + let sig_data = std::fs::read(signature_path)?; + let signature = String::from_utf8_lossy(&sig_data); + let oid = self + .0 + .commit_signed(&buffer, &signature, None) + .map(Into::into) + .map_err(Into::into); + return oid; + } + } else { + // is gpg + let gpg_program = self.0.config()?.get_string("gpg.program"); + let mut cmd = + std::process::Command::new(gpg_program.unwrap_or("gpg".to_string())); + cmd.args(["--status-fd=2", "-bsau", &signing_key]) + //.arg(&signed_storage) + .arg("-") + .stdout(Stdio::piped()) + .stdin(Stdio::piped()); + + let mut child = cmd.spawn()?; + child + .stdin + .take() + .expect("configured") + .write_all(buffer.to_string().as_ref())?; + + let output = child.wait_with_output()?; + if output.status.success() { + // read stdout + let signature = String::from_utf8_lossy(&output.stdout); + let oid = self + .0 + .commit_signed(&buffer, &signature, None) + .map(Into::into) + .map_err(Into::into); + return oid; + } + } + } + } + + let oid = self + .0 + .odb()? + .write(git2::ObjectType::Commit, buffer.as_bytes())?; + + Ok(oid) + } + + fn is_literal_ssh_key(string: &str) -> (bool, &str) { + if let Some(key) = string.strip_prefix("key::") { + return (true, key); + } + if string.starts_with("ssh-") { + return (true, string); + } + (false, string) + } + + // in commit_buffer, inject a line right before the first `\n\n` that we see: + // `change-id: ` + fn inject_change_id(commit_buffer: &[u8], change_id: Option<&str>) -> Result { + // if no change id, generate one + let change_id = change_id + .map(|id| id.to_string()) + .unwrap_or_else(|| format!("{}", uuid::Uuid::new_v4())); + + let commit_ends_in_newline = commit_buffer.ends_with(b"\n"); + let commit_buffer = str::from_utf8(commit_buffer).unwrap(); + let lines = commit_buffer.lines(); + let mut new_buffer = String::new(); + let mut found = false; + for line in lines { + if line.is_empty() && !found { + new_buffer.push_str(&format!("change-id {}\n", change_id)); + found = true; + } + new_buffer.push_str(line); + new_buffer.push('\n'); + } + if !commit_ends_in_newline { + // strip last \n + new_buffer.pop(); + } + Ok(new_buffer) } pub fn config(&self) -> Result { diff --git a/crates/gitbutler-core/src/project_repository/repository.rs b/crates/gitbutler-core/src/project_repository/repository.rs index 5ce7a964f..d2f5d96b4 100644 --- a/crates/gitbutler-core/src/project_repository/repository.rs +++ b/crates/gitbutler-core/src/project_repository/repository.rs @@ -11,7 +11,6 @@ use crate::error::{AnyhowContextExt, Code, ErrorWithContext}; use crate::{ askpass, error, git::{self, credentials::HelpError, Url}, - keys, projects::{self, AuthKey}, ssh, users, virtual_branches::{Branch, BranchId}, @@ -335,18 +334,12 @@ impl Repository { message: &str, tree: &git::Tree, parents: &[&git::Commit], - signing_key: Option<&keys::PrivateKey>, + change_id: Option<&str>, ) -> Result { let (author, committer) = self.git_signatures(user)?; - if let Some(key) = signing_key { - self.git_repository - .commit_signed(&author, message, tree, parents, key) - .context("failed to commit signed") - } else { - self.git_repository - .commit(None, &author, &committer, message, tree, parents) - .context("failed to commit") - } + self.git_repository + .commit(None, &author, &committer, message, tree, parents, change_id) + .context("failed to commit") } pub fn push_to_gitbutler_server( diff --git a/crates/gitbutler-core/src/virtual_branches/base.rs b/crates/gitbutler-core/src/virtual_branches/base.rs index 7eeeea9a8..398f72efa 100644 --- a/crates/gitbutler-core/src/virtual_branches/base.rs +++ b/crates/gitbutler-core/src/virtual_branches/base.rs @@ -12,7 +12,6 @@ use super::{ }; use crate::{ git::{self, diff}, - keys, project_repository::{self, LogUntil}, projects::FetchResult, users, @@ -342,7 +341,6 @@ fn _print_tree(repo: &git2::Repository, tree: &git2::Tree) -> Result<()> { pub fn update_base_branch( project_repository: &project_repository::Repository, user: Option<&users::User>, - signing_key: Option<&keys::PrivateKey>, ) -> Result<(), errors::UpdateBaseBranchError> { if project_repository.is_resolving() { return Err(errors::UpdateBaseBranchError::Conflict( @@ -508,12 +506,12 @@ pub fn update_base_branch( "Merged {}/{} into {}", target.branch.remote(), target.branch.branch(), - branch.name + branch.name, ) .as_str(), &branch_head_merge_tree, &[&branch_head_commit, &new_target_commit], - signing_key, + None, ) .context("failed to commit merge")?; diff --git a/crates/gitbutler-core/src/virtual_branches/controller.rs b/crates/gitbutler-core/src/virtual_branches/controller.rs index 3eb932b2c..e1f415ced 100644 --- a/crates/gitbutler-core/src/virtual_branches/controller.rs +++ b/crates/gitbutler-core/src/virtual_branches/controller.rs @@ -17,7 +17,7 @@ use super::{ target, target_to_base_branch, BaseBranch, RemoteBranchFile, VirtualBranchesHandle, }; use crate::{ - git, keys, project_repository, + git, project_repository, projects::{self, ProjectId}, users, }; @@ -26,7 +26,6 @@ use crate::{ pub struct Controller { projects: projects::Controller, users: users::Controller, - keys: keys::Controller, helper: git::credentials::Helper, by_project_id: Arc>>, @@ -36,7 +35,6 @@ impl Controller { pub fn new( projects: projects::Controller, users: users::Controller, - keys: keys::Controller, helper: git::credentials::Helper, ) -> Self { Self { @@ -44,7 +42,6 @@ impl Controller { projects, users, - keys, helper, } } @@ -54,9 +51,7 @@ impl Controller { .lock() .await .entry(*project_id) - .or_insert_with(|| { - ControllerInner::new(&self.projects, &self.users, &self.keys, &self.helper) - }) + .or_insert_with(|| ControllerInner::new(&self.projects, &self.users, &self.helper)) .clone() } @@ -430,7 +425,6 @@ struct ControllerInner { projects: projects::Controller, users: users::Controller, - keys: keys::Controller, helper: git::credentials::Helper, } @@ -438,14 +432,12 @@ impl ControllerInner { pub fn new( projects: &projects::Controller, users: &users::Controller, - keys: &keys::Controller, helper: &git::credentials::Helper, ) -> Self { Self { semaphore: Arc::new(Semaphore::new(1)), projects: projects.clone(), users: users.clone(), - keys: keys.clone(), helper: helper.clone(), } } @@ -461,23 +453,11 @@ impl ControllerInner { let _permit = self.semaphore.acquire().await; self.with_verify_branch(project_id, |project_repository, user| { - let signing_key = project_repository - .config() - .sign_commits() - .context("failed to get sign commits option")? - .then(|| { - self.keys - .get_or_create() - .context("failed to get private key") - }) - .transpose()?; - let result = super::commit( project_repository, branch_id, message, ownership, - signing_key.as_ref(), user, run_hooks, ) @@ -546,22 +526,8 @@ impl ControllerInner { let _permit = self.semaphore.acquire().await; self.with_verify_branch(project_id, |project_repository, user| { - let signing_key = project_repository - .config() - .sign_commits() - .context("failed to get sign commits option")? - .then(|| { - self.keys - .get_or_create() - .context("failed to get private key") - }) - .transpose()?; - let result = super::create_virtual_branch_from_branch( - project_repository, - branch, - signing_key.as_ref(), - user, - )?; + let result = + super::create_virtual_branch_from_branch(project_repository, branch, user)?; Ok(result) }) } @@ -619,24 +585,8 @@ impl ControllerInner { let _permit = self.semaphore.acquire().await; self.with_verify_branch(project_id, |project_repository, user| { - let signing_key = project_repository - .config() - .sign_commits() - .context("failed to get sign commits option")? - .then(|| { - self.keys - .get_or_create() - .context("failed to get private key") - }) - .transpose()?; - - let result = super::merge_virtual_branch_upstream( - project_repository, - branch_id, - signing_key.as_ref(), - user, - ) - .map_err(Into::into); + let result = super::merge_virtual_branch_upstream(project_repository, branch_id, user) + .map_err(Into::into); let _ = project_repository .project() .create_snapshot(SnapshotDetails::new(OperationType::MergeUpstream)); @@ -648,19 +598,7 @@ impl ControllerInner { let _permit = self.semaphore.acquire().await; self.with_verify_branch(project_id, |project_repository, user| { - let signing_key = project_repository - .config() - .sign_commits() - .context("failed to get sign commits option")? - .then(|| { - self.keys - .get_or_create() - .context("failed to get private key") - }) - .transpose()?; - - let result = super::update_base_branch(project_repository, user, signing_key.as_ref()) - .map_err(Into::into); + let result = super::update_base_branch(project_repository, user).map_err(Into::into); let _ = project_repository .project() .create_snapshot(SnapshotDetails::new(OperationType::UpdateWorkspaceBase)); @@ -701,21 +639,7 @@ impl ControllerInner { let _permit = self.semaphore.acquire().await; self.with_verify_branch(project_id, |project_repository, user| { - let signing_key = project_repository - .config() - .sign_commits() - .context("failed to get sign commits option")? - .then(|| { - self.keys - .get_or_create() - .context("failed to get private key") - }) - .transpose()?; - - let result = - super::apply_branch(project_repository, branch_id, signing_key.as_ref(), user) - .map_err(Into::into); - result + super::apply_branch(project_repository, branch_id, user).map_err(Into::into) }) } @@ -1060,24 +984,8 @@ impl ControllerInner { let _permit = self.semaphore.acquire().await; self.with_verify_branch(project_id, |project_repository, user| { - let signing_key = project_repository - .config() - .sign_commits() - .context("failed to get sign commits option")? - .then(|| { - self.keys - .get_or_create() - .context("failed to get private key") - }) - .transpose()?; - let result = super::move_commit( - project_repository, - target_branch_id, - commit_oid, - user, - signing_key.as_ref(), - ) - .map_err(Into::into); + let result = super::move_commit(project_repository, target_branch_id, commit_oid, user) + .map_err(Into::into); let _ = project_repository .project() .create_snapshot(SnapshotDetails::new(OperationType::MoveCommit)); diff --git a/crates/gitbutler-core/src/virtual_branches/integration.rs b/crates/gitbutler-core/src/virtual_branches/integration.rs index 40632e07c..aaae9f410 100644 --- a/crates/gitbutler-core/src/virtual_branches/integration.rs +++ b/crates/gitbutler-core/src/virtual_branches/integration.rs @@ -89,6 +89,7 @@ pub fn get_workspace_head( WORKSPACE_HEAD, &workspace_tree, branch_head_refs.as_slice(), + None, )?; Ok(workspace_head_id) } @@ -219,6 +220,7 @@ pub fn update_gitbutler_integration( &message, &integration_commit.tree()?, &[&target_commit], + None, )?; // Create or replace the integration branch reference, then set as HEAD. @@ -257,6 +259,7 @@ pub fn update_gitbutler_integration( &message, &wip_tree, &[&branch_head], + None, )?; branch_head = repo.find_commit(branch_head_oid)?; } @@ -351,6 +354,7 @@ fn verify_head_is_clean( &commit.message().to_str_lossy(), &commit.tree().unwrap(), &[&new_branch_head], + None, ) .context(format!( "failed to rebase commit {} onto new branch", diff --git a/crates/gitbutler-core/src/virtual_branches/virtual.rs b/crates/gitbutler-core/src/virtual_branches/virtual.rs index e8f73d81f..c2fcfa74e 100644 --- a/crates/gitbutler-core/src/virtual_branches/virtual.rs +++ b/crates/gitbutler-core/src/virtual_branches/virtual.rs @@ -34,7 +34,6 @@ use crate::{ diff::{self}, Commit, Refname, RemoteRefname, }, - keys, project_repository::{self, conflicts, LogUntil}, reader, users, }; @@ -99,6 +98,8 @@ pub struct VirtualBranchCommit { pub is_integrated: bool, pub parent_ids: Vec, pub branch_id: BranchId, + pub change_id: Option, + pub is_signed: bool, } // this struct is a mapping to the view `File` type in Typescript @@ -213,7 +214,6 @@ pub fn normalize_branch_name(name: &str) -> String { pub fn apply_branch( project_repository: &project_repository::Repository, branch_id: &BranchId, - signing_key: Option<&keys::PrivateKey>, user: Option<&users::User>, ) -> Result<(), errors::ApplyBranchError> { if project_repository.is_resolving() { @@ -357,7 +357,7 @@ pub fn apply_branch( .as_str(), &merged_branch_tree, &[&head_commit, &target_commit], - signing_key, + None, )?; // ok, update the virtual branch @@ -426,7 +426,7 @@ pub fn apply_branch( .as_str(), &merge_tree, &[&head_commit, &target_commit], - signing_key, + None, ) .context("failed to commit merge")?; @@ -1044,6 +1044,8 @@ fn commit_to_vbranch_commit( is_integrated, parent_ids, branch_id: branch.id, + change_id: commit.change_id(), + is_signed: commit.is_signed(), }; Ok(commit) @@ -1163,7 +1165,6 @@ pub fn create_virtual_branch( pub fn merge_virtual_branch_upstream( project_repository: &project_repository::Repository, branch_id: &BranchId, - signing_key: Option<&keys::PrivateKey>, user: Option<&users::User>, ) -> Result<(), errors::MergeVirtualBranchUpstreamError> { if conflicts::is_conflicting::<&Path>(project_repository, None)? { @@ -1356,7 +1357,7 @@ pub fn merge_virtual_branch_upstream( .as_str(), &merge_tree, &[&head_commit, &upstream_commit], - signing_key, + None, )?; // checkout the merge tree @@ -2313,7 +2314,6 @@ pub fn commit( branch_id: &BranchId, message: &str, ownership: Option<&branch::BranchOwnershipClaims>, - signing_key: Option<&keys::PrivateKey>, user: Option<&users::User>, run_hooks: bool, ) -> Result { @@ -2416,12 +2416,12 @@ pub fn commit( message, &tree, &[&parent_commit, &merge_parent], - signing_key, + None, )?; conflicts::clear(project_repository).context("failed to clear conflicts")?; commit_oid } - None => project_repository.commit(user, message, &tree, &[&parent_commit], signing_key)?, + None => project_repository.commit(user, message, &tree, &[&parent_commit], None)?, }; if run_hooks { @@ -2879,6 +2879,7 @@ pub fn move_commit_file( let new_from_tree = &repo .find_tree(new_from_tree_oid) .map_err(|_error| errors::VirtualBranchError::GitObjectNotFound(new_from_tree_oid))?; + let change_id = from_commit.change_id(); let new_from_commit_oid = repo .commit( None, @@ -2887,6 +2888,7 @@ pub fn move_commit_file( &from_commit.message().to_str_lossy(), new_from_tree, &[&from_parent], + change_id.as_deref(), ) .map_err(|_error| errors::VirtualBranchError::CommitFailed)?; @@ -2957,6 +2959,7 @@ pub fn move_commit_file( let parents = amend_commit .parents() .context("failed to find head commit parents")?; + let change_id = amend_commit.change_id(); let commit_oid = project_repository .git_repository .commit( @@ -2966,6 +2969,7 @@ pub fn move_commit_file( &amend_commit.message().to_str_lossy(), &new_tree, &parents.iter().collect::>(), + change_id.as_deref(), ) .context("failed to create commit")?; @@ -3154,6 +3158,7 @@ pub fn amend( &amend_commit.message().to_str_lossy(), &new_tree, &parents.iter().collect::>(), + None, ) .context("failed to create commit")?; @@ -3501,6 +3506,8 @@ fn cherry_rebase_group( .find_tree(merge_tree_oid) .context("failed to find merge tree")?; + let change_id = to_rebase.change_id(); + let commit_oid = project_repository .git_repository .commit( @@ -3510,6 +3517,7 @@ fn cherry_rebase_group( &to_rebase.message().to_str_lossy(), &merge_tree, &[&head], + change_id.as_deref(), ) .context("failed to create commit")?; @@ -3524,60 +3532,6 @@ fn cherry_rebase_group( Ok(Some(new_head_id)) } -// runs a simple libgit2 based in-memory rebase on a commit range onto a target commit -// possibly not used in favor of cherry_rebase -pub fn simple_rebase( - project_repository: &project_repository::Repository, - target_commit_oid: git::Oid, - start_commit_oid: git::Oid, - end_commit_oid: git::Oid, -) -> Result, anyhow::Error> { - let repo = &project_repository.git_repository; - let (_, committer) = project_repository.git_signatures(None)?; - - let mut rebase_options = git2::RebaseOptions::new(); - rebase_options.quiet(true); - rebase_options.inmemory(true); - let mut rebase = repo - .rebase( - Some(end_commit_oid), - Some(start_commit_oid), - Some(target_commit_oid), - Some(&mut rebase_options), - ) - .context("failed to rebase")?; - - let mut rebase_success = true; - // check to see if these commits have already been pushed - let mut last_rebase_head = target_commit_oid; - while rebase.next().is_some() { - let index = rebase - .inmemory_index() - .context("failed to get inmemory index")?; - if index.has_conflicts() { - rebase_success = false; - break; - } - - if let Ok(commit_id) = rebase.commit(None, &committer.clone().into(), None) { - last_rebase_head = commit_id.into(); - } else { - rebase_success = false; - break; - } - } - - if rebase_success { - // rebase worked out, rewrite the branch head - rebase.finish(None).context("failed to finish rebase")?; - Ok(Some(last_rebase_head)) - } else { - // rebase failed, do a merge commit - rebase.abort().context("failed to abort rebase")?; - Ok(None) - } -} - pub fn cherry_pick( project_repository: &project_repository::Repository, branch_id: &BranchId, @@ -3661,6 +3615,7 @@ pub fn cherry_pick( "wip cherry picking commit", &wip_tree, &[&branch_head_commit], + None, ) .context("failed to commit wip work")?; project_repository @@ -3724,6 +3679,7 @@ pub fn cherry_pick( .find_commit(branch.head) .context("failed to find branch head commit")?; + let change_id = target_commit.change_id(); let commit_oid = project_repository .git_repository .commit( @@ -3733,6 +3689,7 @@ pub fn cherry_pick( &target_commit.message().to_str_lossy(), &merge_tree, &[&branch_head_commit], + change_id.as_deref(), ) .context("failed to create commit")?; @@ -3848,6 +3805,9 @@ pub fn squash( .parents() .context("failed to find head commit parents")?; + // use the squash commit's change id + let change_id = commit_to_squash.change_id(); + let new_commit_oid = project_repository .git_repository .commit( @@ -3861,6 +3821,7 @@ pub fn squash( ), &commit_to_squash.tree().context("failed to find tree")?, &parents.iter().collect::>(), + change_id.as_deref(), ) .context("failed to commit")?; @@ -3971,6 +3932,8 @@ pub fn update_commit_message( .parents() .context("failed to find head commit parents")?; + let change_id = target_commit.change_id(); + let new_commit_oid = project_repository .git_repository .commit( @@ -3980,6 +3943,7 @@ pub fn update_commit_message( message, &target_commit.tree().context("failed to find tree")?, &parents.iter().collect::>(), + change_id.as_deref(), ) .context("failed to commit")?; @@ -4016,7 +3980,6 @@ pub fn move_commit( target_branch_id: &BranchId, commit_oid: git::Oid, user: Option<&users::User>, - signing_key: Option<&keys::PrivateKey>, ) -> Result<(), errors::MoveCommitError> { if project_repository.is_resolving() { return Err(errors::MoveCommitError::Conflicted( @@ -4163,6 +4126,7 @@ pub fn move_commit( .find_tree(new_destination_tree_oid) .context("failed to find tree")?; + let change_id = source_branch_head.change_id(); let new_destination_head_oid = project_repository .commit( user, @@ -4172,7 +4136,7 @@ pub fn move_commit( .git_repository .find_commit(destination_branch.head) .context("failed to get dst branch head commit")?], - signing_key, + change_id.as_deref(), ) .context("failed to commit")?; @@ -4189,7 +4153,6 @@ pub fn move_commit( pub fn create_virtual_branch_from_branch( project_repository: &project_repository::Repository, upstream: &git::Refname, - signing_key: Option<&keys::PrivateKey>, user: Option<&users::User>, ) -> Result { if !matches!(upstream, git::Refname::Local(_) | git::Refname::Remote(_)) { @@ -4333,7 +4296,7 @@ pub fn create_virtual_branch_from_branch( .project() .snapshot_branch_creation(branch_name); - match apply_branch(project_repository, &branch.id, signing_key, user) { + match apply_branch(project_repository, &branch.id, user) { Ok(()) => Ok(branch.id), Err(errors::ApplyBranchError::BranchConflicts(_)) => { // if branch conflicts with the workspace, it's ok. keep it unapplied diff --git a/crates/gitbutler-core/tests/suite/virtual_branches/init.rs b/crates/gitbutler-core/tests/suite/virtual_branches/init.rs index ba39156fc..1be497e78 100644 --- a/crates/gitbutler-core/tests/suite/virtual_branches/init.rs +++ b/crates/gitbutler-core/tests/suite/virtual_branches/init.rs @@ -3,14 +3,13 @@ use super::*; #[tokio::test] async fn twice() { let data_dir = paths::data_dir(); - let keys = keys::Controller::from_path(data_dir.path()); let projects = projects::Controller::from_path(data_dir.path()); let users = users::Controller::from_path(data_dir.path()); let helper = git::credentials::Helper::from_path(data_dir.path()); let test_project = TestProject::default(); - let controller = Controller::new(projects.clone(), users, keys, helper); + let controller = Controller::new(projects.clone(), users, helper); { let project = projects diff --git a/crates/gitbutler-core/tests/suite/virtual_branches/mod.rs b/crates/gitbutler-core/tests/suite/virtual_branches/mod.rs index aee014ac0..663721cee 100644 --- a/crates/gitbutler-core/tests/suite/virtual_branches/mod.rs +++ b/crates/gitbutler-core/tests/suite/virtual_branches/mod.rs @@ -1,7 +1,7 @@ use std::{fs, path, str::FromStr}; use gitbutler_core::{ - git, keys, + git, projects::{self, ProjectId}, users, virtual_branches::{branch, errors, Controller}, @@ -29,7 +29,6 @@ impl Drop for Test { impl Default for Test { fn default() -> Self { let data_dir = paths::data_dir(); - let keys = keys::Controller::from_path(data_dir.path()); let projects = projects::Controller::from_path(data_dir.path()); let users = users::Controller::from_path(data_dir.path()); let helper = git::credentials::Helper::from_path(data_dir.path()); @@ -42,7 +41,7 @@ impl Default for Test { Self { repository: test_project, project_id: project.id, - controller: Controller::new(projects.clone(), users, keys, helper), + controller: Controller::new(projects.clone(), users, helper), projects, data_dir: Some(data_dir), } diff --git a/crates/gitbutler-core/tests/suite/virtual_branches/move_commit_file.rs b/crates/gitbutler-core/tests/suite/virtual_branches/move_commit_file.rs index bdb57a685..c1db30331 100644 --- a/crates/gitbutler-core/tests/suite/virtual_branches/move_commit_file.rs +++ b/crates/gitbutler-core/tests/suite/virtual_branches/move_commit_file.rs @@ -25,6 +25,7 @@ async fn move_file_down() { .create_commit(project_id, &branch_id, "commit one", None, false) .await .unwrap(); + let commit1 = repository.find_commit(commit1_id).unwrap(); // create commit fs::write(repository.path().join("file2.txt"), "content2").unwrap(); @@ -33,6 +34,7 @@ async fn move_file_down() { .create_commit(project_id, &branch_id, "commit two", None, false) .await .unwrap(); + let commit2 = repository.find_commit(commit2_id).unwrap(); // amend another hunk let to_amend: branch::BranchOwnershipClaims = "file2.txt:1-2".parse().unwrap(); @@ -50,6 +52,13 @@ async fn move_file_down() { .find(|b| b.id == branch_id) .unwrap(); + // shas changed but change_id is the same + assert_eq!(&commit1.change_id(), &branch.commits[1].change_id); + assert_ne!(&commit1.id(), &branch.commits[1].id); + assert_eq!(&commit2.change_id(), &branch.commits[0].change_id); + assert_ne!(&commit2.id(), &branch.commits[0].id); + + assert_eq!(branch.commits[0].files.len(), 1); assert_eq!(branch.commits.len(), 2); assert_eq!(branch.commits[0].files.len(), 1); assert_eq!(branch.commits[1].files.len(), 2); // this now has both file changes diff --git a/crates/gitbutler-core/tests/suite/virtual_branches/update_commit_message.rs b/crates/gitbutler-core/tests/suite/virtual_branches/update_commit_message.rs index 8b9169ff5..aa0794a0e 100644 --- a/crates/gitbutler-core/tests/suite/virtual_branches/update_commit_message.rs +++ b/crates/gitbutler-core/tests/suite/virtual_branches/update_commit_message.rs @@ -42,6 +42,8 @@ async fn head() { .await .unwrap() }; + let commit_three = repository.find_commit(commit_three_oid).unwrap(); + let before_change_id = &commit_three.change_id(); controller .update_commit_message( @@ -68,6 +70,13 @@ async fn head() { .map(|c| c.description.clone()) .collect::>(); + // get the last commit + let commit = repository.find_commit(branch.head).unwrap(); + + // make sure the SHA changed, but the change ID did not + assert_ne!(&commit_three.id(), &commit.id()); + assert_eq!(before_change_id, &commit.change_id()); + assert_eq!( descriptions, vec!["commit three updated", "commit two", "commit one"] diff --git a/crates/gitbutler-core/tests/virtual_branches/mod.rs b/crates/gitbutler-core/tests/virtual_branches/mod.rs index accca6185..327b1059e 100644 --- a/crates/gitbutler-core/tests/virtual_branches/mod.rs +++ b/crates/gitbutler-core/tests/virtual_branches/mod.rs @@ -64,7 +64,6 @@ fn commit_on_branch_then_change_file_then_get_status() -> Result<()> { "test commit", None, None, - None, false, )?; @@ -88,55 +87,6 @@ fn commit_on_branch_then_change_file_then_get_status() -> Result<()> { Ok(()) } -#[test] -fn signed_commit() -> Result<()> { - let suite = Suite::default(); - let Case { - project, - project_repository, - .. - } = &suite.new_case_with_files(HashMap::from([ - (PathBuf::from("test.txt"), "line1\nline2\nline3\nline4\n"), - (PathBuf::from("test2.txt"), "line5\nline6\nline7\nline8\n"), - ])); - - set_test_target(project_repository)?; - - let branch1_id = create_virtual_branch(project_repository, &BranchCreateRequest::default()) - .expect("failed to create virtual branch") - .id; - - std::fs::write( - Path::new(&project.path).join("test.txt"), - "line0\nline1\nline2\nline3\nline4\n", - )?; - - let mut config = project_repository - .git_repository - .config() - .with_context(|| "failed to get config")?; - config.set_str("gitbutler.signCommits", "true")?; - - // commit - commit( - project_repository, - &branch1_id, - "test commit", - None, - Some(suite.keys.get_or_create()?).as_ref(), - None, - false, - )?; - - let (branches, _) = virtual_branches::list_virtual_branches(project_repository).unwrap(); - let commit_id = &branches[0].commits[0].id; - let commit_obj = project_repository.git_repository.find_commit(*commit_id)?; - // check the raw_header contains the string "SSH SIGNATURE" - assert!(commit_obj.raw_header().unwrap().contains("SSH SIGNATURE")); - - Ok(()) -} - #[test] fn track_binary_files() -> Result<()> { let suite = Suite::default(); @@ -212,7 +162,6 @@ fn track_binary_files() -> Result<()> { "test commit", None, None, - None, false, )?; @@ -244,7 +193,6 @@ fn track_binary_files() -> Result<()> { "test commit", None, None, - None, false, )?; @@ -718,6 +666,89 @@ fn add_new_hunk_to_the_end() -> Result<()> { Ok(()) } +#[test] +fn commit_id_can_be_generated_or_specified() -> Result<()> { + let suite = Suite::default(); + let Case { + project_repository, + project, + .. + } = &suite.new_case(); + + let file_path = Path::new("test.txt"); + std::fs::write( + Path::new(&project.path).join(file_path), + "line1\nline2\nline3\nline4\n", + )?; + commit_all(&project_repository.git_repository); + + // lets make sure a change id is generated + let target_oid = project_repository + .git_repository + .head() + .unwrap() + .target() + .unwrap(); + let target = project_repository + .git_repository + .find_commit(target_oid) + .unwrap(); + let change_id = target.change_id(); + + // make sure we created a change-id + assert!(change_id.is_some()); + + // ok, make another change and specify a change-id + let file_path = Path::new("test.txt"); + std::fs::write( + Path::new(&project.path).join(file_path), + "line1\nline2\nline3\nline4\nline5\n", + )?; + + let repository = &project_repository.git_repository; + let mut index = repository.index().expect("failed to get index"); + index + .add_all(["."], git2::IndexAddOption::DEFAULT, None) + .expect("failed to add all"); + index.write().expect("failed to write index"); + let oid = index.write_tree().expect("failed to write tree"); + let signature = gitbutler_core::git::Signature::now("test", "test@email.com").unwrap(); + let head = repository.head().expect("failed to get head"); + repository + .commit( + Some(&head.name().unwrap()), + &signature, + &signature, + "some commit", + &repository.find_tree(oid).expect("failed to find tree"), + &[&repository + .find_commit( + repository + .refname_to_id("HEAD") + .expect("failed to get head"), + ) + .expect("failed to find commit")], + Some("my-change-id"), + ) + .expect("failed to commit"); + + let target_oid = project_repository + .git_repository + .head() + .unwrap() + .target() + .unwrap(); + let target = project_repository + .git_repository + .find_commit(target_oid) + .unwrap(); + let change_id = target.change_id(); + + // the change id should be what we specified, rather than randomly generated + assert_eq!(change_id, Some("my-change-id".to_string())); + Ok(()) +} + #[test] fn merge_vbranch_upstream_clean_rebase() -> Result<()> { let suite = Suite::default(); @@ -814,12 +845,7 @@ fn merge_vbranch_upstream_clean_rebase() -> Result<()> { assert_eq!(branch1.commits.len(), 1); // assert_eq!(branch1.upstream.as_ref().unwrap().commits.len(), 1); - merge_virtual_branch_upstream( - project_repository, - &branch1.id, - Some(suite.keys.get_or_create()?).as_ref(), - None, - )?; + merge_virtual_branch_upstream(project_repository, &branch1.id, None)?; let (branches, _) = virtual_branches::list_virtual_branches(project_repository)?; let branch1 = &branches[0]; @@ -934,7 +960,7 @@ fn merge_vbranch_upstream_conflict() -> Result<()> { assert_eq!(branch1.commits.len(), 1); // assert_eq!(branch1.upstream.as_ref().unwrap().commits.len(), 1); - merge_virtual_branch_upstream(project_repository, &branch1.id, None, None)?; + merge_virtual_branch_upstream(project_repository, &branch1.id, None)?; let (branches, _) = virtual_branches::list_virtual_branches(project_repository)?; let branch1 = &branches[0]; @@ -966,7 +992,6 @@ fn merge_vbranch_upstream_conflict() -> Result<()> { "fix merge conflict", None, None, - None, false, )?; @@ -1098,7 +1123,7 @@ fn unapply_branch() -> Result<()> { assert_eq!(branch.files.len(), 1); assert!(!branch.active); - apply_branch(project_repository, &branch1_id, None, None)?; + apply_branch(project_repository, &branch1_id, None)?; let contents = std::fs::read(Path::new(&project.path).join(file_path))?; assert_eq!( "line1\nline2\nline3\nline4\nbranch1\n", @@ -1171,11 +1196,11 @@ fn apply_unapply_added_deleted_files() -> Result<()> { // check that file3 is gone assert!(!Path::new(&project.path).join(file_path3).exists()); - apply_branch(project_repository, &branch2_id, None, None)?; + apply_branch(project_repository, &branch2_id, None)?; // check that file2 is gone assert!(!Path::new(&project.path).join(file_path2).exists()); - apply_branch(project_repository, &branch3_id, None, None)?; + apply_branch(project_repository, &branch3_id, None)?; // check that file3 is back let contents = std::fs::read(Path::new(&project.path).join(file_path3))?; assert_eq!("file3\n", String::from_utf8(contents)?); @@ -1464,7 +1489,6 @@ fn upstream_integrated_vbranch() -> Result<()> { "integrated commit", None, None, - None, false, )?; commit( @@ -1473,7 +1497,6 @@ fn upstream_integrated_vbranch() -> Result<()> { "non-integrated commit", None, None, - None, false, )?; @@ -1534,7 +1557,6 @@ fn commit_same_hunk_twice() -> Result<()> { "first commit to test.txt", None, None, - None, false, )?; @@ -1570,7 +1592,6 @@ fn commit_same_hunk_twice() -> Result<()> { "second commit to test.txt", None, None, - None, false, )?; @@ -1629,7 +1650,6 @@ fn commit_same_file_twice() -> Result<()> { "first commit to test.txt", None, None, - None, false, )?; @@ -1665,7 +1685,6 @@ fn commit_same_file_twice() -> Result<()> { "second commit to test.txt", None, None, - None, false, )?; @@ -1724,7 +1743,6 @@ fn commit_partial_by_hunk() -> Result<()> { "first commit to test.txt", Some(&"test.txt:1-6".parse::().unwrap()), None, - None, false, )?; @@ -1743,7 +1761,6 @@ fn commit_partial_by_hunk() -> Result<()> { "second commit to test.txt", Some(&"test.txt:16-22".parse::().unwrap()), None, - None, false, )?; @@ -1802,7 +1819,6 @@ fn commit_partial_by_file() -> Result<()> { "branch1 commit", None, None, - None, false, )?; @@ -1870,7 +1886,6 @@ fn commit_add_and_delete_files() -> Result<()> { "branch1 commit", None, None, - None, false, )?; @@ -1936,7 +1951,6 @@ fn commit_executable_and_symlinks() -> Result<()> { "branch1 commit", None, None, - None, false, )?; @@ -2113,7 +2127,6 @@ fn pre_commit_hook_rejection() -> Result<()> { &branch1_id, "test commit", None, - Some(suite.keys.get_or_create()?).as_ref(), None, true, ); @@ -2178,7 +2191,6 @@ fn post_commit_hook() -> Result<()> { &branch1_id, "test commit", None, - Some(suite.keys.get_or_create()?).as_ref(), None, true, )?; @@ -2227,7 +2239,6 @@ fn commit_msg_hook_rejection() -> Result<()> { &branch1_id, "test commit", None, - Some(suite.keys.get_or_create()?).as_ref(), None, true, ); diff --git a/crates/gitbutler-git/Cargo.toml b/crates/gitbutler-git/Cargo.toml index c0d023378..cacc758c2 100644 --- a/crates/gitbutler-git/Cargo.toml +++ b/crates/gitbutler-git/Cargo.toml @@ -28,8 +28,14 @@ test-askpass-path = [] [dependencies] thiserror.workspace = true serde = { workspace = true, optional = true } -tokio = { workspace = true, optional = true, features = ["process", "time", "io-util", "net", "fs"] } -uuid = { workspace = true, features = [ "v4", "fast-rng" ] } +tokio = { workspace = true, optional = true, features = [ + "process", + "time", + "io-util", + "net", + "fs", +] } +uuid = { workspace = true, features = ["v4", "fast-rng"] } rand = "0.8.5" futures = "0.3.30" sysinfo = "0.30.11" diff --git a/crates/gitbutler-tauri/src/main.rs b/crates/gitbutler-tauri/src/main.rs index 45efc5831..fd0fe9e1d 100644 --- a/crates/gitbutler-tauri/src/main.rs +++ b/crates/gitbutler-tauri/src/main.rs @@ -133,7 +133,6 @@ fn main() { app_handle.manage(gitbutler_core::virtual_branches::controller::Controller::new( projects_controller.clone(), users_controller.clone(), - keys_controller.clone(), git_credentials_controller.clone(), )); diff --git a/crates/gitbutler-testsupport/src/suite.rs b/crates/gitbutler-testsupport/src/suite.rs index e45cd91c5..088182653 100644 --- a/crates/gitbutler-testsupport/src/suite.rs +++ b/crates/gitbutler-testsupport/src/suite.rs @@ -173,6 +173,7 @@ pub fn test_repository() -> (gitbutler_core::git::Repository, TempDir) { "Initial commit", &repository.find_tree(oid).expect("failed to find tree"), &[], + None, ) .expect("failed to commit"); (repository, tmp) @@ -201,6 +202,7 @@ pub fn commit_all(repository: &gitbutler_core::git::Repository) -> gitbutler_cor .expect("failed to get head"), ) .expect("failed to find commit")], + None, ) .expect("failed to commit"); commit_oid diff --git a/crates/gitbutler-testsupport/src/test_project.rs b/crates/gitbutler-testsupport/src/test_project.rs index d65a5e421..90442f4e1 100644 --- a/crates/gitbutler-testsupport/src/test_project.rs +++ b/crates/gitbutler-testsupport/src/test_project.rs @@ -43,6 +43,7 @@ impl Default for TestProject { .find_tree(oid) .expect("failed to find tree"), &[], + None, ) .expect("failed to commit"); @@ -237,6 +238,7 @@ impl TestProject { &format!("Merge pull request from {}", branch_name), &merge_tree, &[&master_branch_commit, &branch_commit], + None, ) .unwrap(); } @@ -312,6 +314,7 @@ impl TestProject { .expect("failed to get head"), ) .expect("failed to find commit")], + None, ) .expect("failed to commit") }