diff --git a/CHANGELOG.md b/CHANGELOG.md index 6db667675c..6a7b330f1d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * use [tombi](https://github.com/tombi-toml/tombi) for all toml file formatting * open the external editor from the status diff view [[@WaterWhisperer](https://github.com/WaterWhisperer)] ([#2805](https://github.com/gitui-org/gitui/issues/2805)) * automatically convert spaces to dashes when creating or renaming a branch [[@pbouillon]](https//pbouillon.github.io)] ([#2916](https://github.com/gitui-org/gitui/pull/2916)) +* support rewording non-HEAD commits when `commit.gpgsign` is enabled, by rebuilding the chain manually and signing each rewritten commit ### Fixes * crash when opening submodule ([#2895](https://github.com/gitui-org/gitui/issues/2895)) diff --git a/asyncgit/src/error.rs b/asyncgit/src/error.rs index e36040d814..e89e4a56f8 100644 --- a/asyncgit/src/error.rs +++ b/asyncgit/src/error.rs @@ -177,10 +177,6 @@ pub enum Error { #[error("amend error: config commit.gpgsign=true detected.\ngpg signing is not supported for amending non-last commits")] SignAmendNonLastCommit, - /// - #[error("reword error: config commit.gpgsign=true detected.\ngpg signing is not supported for rewording non-last commits")] - SignRewordNonLastCommit, - /// #[error("reword error: config commit.gpgsign=true detected.\ngpg signing is not supported for rewording commits with staged changes\ntry unstaging or stashing your changes")] SignRewordLastCommitStaged, diff --git a/asyncgit/src/sync/commit.rs b/asyncgit/src/sync/commit.rs index 1a9ffd11aa..aaf8874521 100644 --- a/asyncgit/src/sync/commit.rs +++ b/asyncgit/src/sync/commit.rs @@ -1,6 +1,6 @@ //! Git Api for Commits use super::{CommitId, RepoPath}; -use crate::sync::sign::{SignBuilder, SignError}; +use crate::sync::sign::{create_signed_commit, SignBuilder}; use crate::{ error::{Error, Result}, sync::{repository::repo, utils::get_head_repo}, @@ -99,58 +99,47 @@ pub fn commit(repo_path: &RepoPath, msg: &str) -> Result { let parents = parents.iter().collect::>(); - let commit_id = if config - .get_bool("commit.gpgsign") - .unwrap_or(false) - { - let buffer = repo.commit_create_buffer( - &signature, - &signature, - msg, - &tree, - parents.as_slice(), - )?; - - let commit = std::str::from_utf8(&buffer).map_err(|_e| { - SignError::Shellout("utf8 conversion error".to_string()) - })?; - - let signer = SignBuilder::from_gitconfig(&repo, &config)?; - let (signature, signature_field) = signer.sign(&buffer)?; - let commit_id = repo.commit_signed( - commit, - &signature, - signature_field.as_deref(), - )?; - - // manually advance to the new commit ID - // repo.commit does that on its own, repo.commit_signed does not - // if there is no head, read default branch or default to "master" - if let Ok(mut head) = repo.head() { - head.set_target(commit_id, msg)?; - } else { - let default_branch_name = config - .get_str("init.defaultBranch") - .unwrap_or("master"); - repo.reference( - &format!("refs/heads/{default_branch_name}"), - commit_id, - true, + let commit_id = + if config.get_bool("commit.gpgsign").unwrap_or(false) { + let signer = SignBuilder::from_gitconfig(&repo, &config)?; + let commit_id = create_signed_commit( + &repo, + signer.as_ref(), + &signature, + &signature, msg, + &tree, + parents.as_slice(), )?; - } - commit_id - } else { - repo.commit( - Some("HEAD"), - &signature, - &signature, - msg, - &tree, - parents.as_slice(), - )? - }; + // manually advance to the new commit ID + // repo.commit does that on its own, repo.commit_signed does not + // if there is no head, read default branch or default to "master" + if let Ok(mut head) = repo.head() { + head.set_target(commit_id, msg)?; + } else { + let default_branch_name = config + .get_str("init.defaultBranch") + .unwrap_or("master"); + repo.reference( + &format!("refs/heads/{default_branch_name}"), + commit_id, + true, + msg, + )?; + } + + commit_id + } else { + repo.commit( + Some("HEAD"), + &signature, + &signature, + msg, + &tree, + parents.as_slice(), + )? + }; Ok(commit_id.into()) } diff --git a/asyncgit/src/sync/mod.rs b/asyncgit/src/sync/mod.rs index 2a5f413e8f..2cd358d065 100644 --- a/asyncgit/src/sync/mod.rs +++ b/asyncgit/src/sync/mod.rs @@ -274,6 +274,8 @@ pub mod tests { pub fn repo_init_bare() -> Result<(TempDir, Repository)> { init_log(); + sandbox_config_files(); + let tmp_repo_dir = TempDir::new()?; let bare_repo = Repository::init_bare(tmp_repo_dir.path())?; Ok((tmp_repo_dir, bare_repo)) diff --git a/asyncgit/src/sync/reword.rs b/asyncgit/src/sync/reword.rs index a503686321..a3c2829aea 100644 --- a/asyncgit/src/sync/reword.rs +++ b/asyncgit/src/sync/reword.rs @@ -3,6 +3,7 @@ use git2::{Oid, RebaseOptions, Repository}; use super::{ commit::signature_allow_undefined_name, repo, + sign::{create_signed_commit, SignBuilder}, utils::{bytes2string, get_head_refname, get_head_repo}, CommitId, RepoPath, }; @@ -17,12 +18,14 @@ pub fn reword( let repo = repo(repo_path)?; let config = repo.config()?; - if config.get_bool("commit.gpgsign").unwrap_or(false) { - // HACK: we undo the last commit and create a new one - use crate::sync::utils::undo_last_commit; + let sign = config.get_bool("commit.gpgsign").unwrap_or(false); + if sign { let head = get_head_repo(&repo)?; if head == commit { + // HACK: we undo the last commit and create a new one + use crate::sync::utils::undo_last_commit; + // Check if there are any staged changes let parent = repo.find_commit(head.into())?; let tree = parent.tree()?; @@ -37,14 +40,19 @@ pub fn reword( return Err(Error::SignRewordLastCommitStaged); } - - return Err(Error::SignRewordNonLastCommit); } let cur_branch_ref = get_head_refname(&repo)?; - match reword_internal(&repo, commit.get_oid(), message) { - Ok(id) => Ok(id.into()), + let result = if sign { + reword_signed(&repo, commit.get_oid(), message) + } else { + reword_unsigned(&repo, commit.get_oid(), message) + .map(Into::into) + }; + + match result { + Ok(id) => Ok(id), // Something went wrong, checkout the previous branch then error Err(e) => { if let Ok(mut rebase) = repo.open_rebase(None) { @@ -80,7 +88,7 @@ fn get_current_branch( /// /// This is dangerous if it errors, as the head will be detached so this should /// always be wrapped by another function which aborts the rebase if something goes wrong -fn reword_internal( +fn reword_unsigned( repo: &Repository, commit: Oid, message: &str, @@ -147,6 +155,89 @@ fn reword_internal( Err(Error::NoBranch) } +/// Reword a non-HEAD commit while honoring `commit.gpgsign`. +/// +/// `git2`'s rebase API cannot sign commits, so we rebuild the chain +/// from `target` up to HEAD manually and sign each rewritten commit with `commit_signed`. +/// A pure reword does not alter trees, so the original tree of every commit in the chain is reused, +/// only parents, committer and (for `target`) the message are rewritten. +fn reword_signed( + repo: &Repository, + target: Oid, + new_message: &str, +) -> Result { + let config = repo.config()?; + let signer = SignBuilder::from_gitconfig(repo, &config)?; + let committer = signature_allow_undefined_name(repo)?; + let signer = signer.as_ref(); + + let cur_branch_ref = get_head_refname(repo)?; + + // collect commits from HEAD down to (and including) target. + let mut chain = Vec::new(); + let mut cur = repo.head()?.peel_to_commit()?; + loop { + let id = cur.id(); + let parent_count = cur.parent_count(); + chain.push(cur); + if id == target { + break; + } + if parent_count != 1 { + return Err(Error::Generic( + "reword across merge commits is not supported" + .to_string(), + )); + } + cur = repo.find_commit(id)?.parent(0)?; + } + + // target first, then its descendants up to HEAD. + chain.reverse(); + + let (target_commit, descendants) = + chain.split_first().ok_or_else(|| { + Error::Generic("empty reword chain".to_string()) + })?; + + let target_parent = + target_commit.parent(0).map_err(|_| Error::NoParent)?; + let parents = [&target_parent]; + let new_target_oid = create_signed_commit( + repo, + signer, + &target_commit.author(), + &committer, + new_message, + &target_commit.tree()?, + &parents, + )?; + + let mut last_new_oid = new_target_oid; + for original in descendants { + let new_parent = repo.find_commit(last_new_oid)?; + let parents = [&new_parent]; + let msg = original.message_raw().unwrap_or(""); + last_new_oid = create_signed_commit( + repo, + signer, + &original.author(), + &committer, + msg, + &original.tree()?, + &parents, + )?; + } + + // move the branch to the rewritten tip and refresh the worktree. + let mut branch_ref = repo.find_reference(&cur_branch_ref)?; + branch_ref.set_target(last_new_oid, "gitui: reword (signed)")?; + repo.set_head(&cur_branch_ref)?; + repo.checkout_head(None)?; + + Ok(new_target_oid.into()) +} + #[cfg(test)] mod tests { use super::*; @@ -192,4 +283,84 @@ mod tests { get_commit_info(repo_path, &reworded).unwrap().message ); } + + #[cfg(unix)] + #[test] + fn test_reword_signed_non_head() { + use std::os::unix::fs::PermissionsExt; + + let (td, repo) = repo_init_empty().unwrap(); + let root = repo.path().parent().unwrap(); + let repo_path: &RepoPath = + &root.as_os_str().to_str().unwrap().into(); + + // fake gpg program: drain stdin, emit a fixed PGP block on stdout and the GNUPG status line gitui's signer expects on stderr. + let script_path = td.path().join("fake_gpg.sh"); + let script = "#!/bin/sh\n\ + cat > /dev/null\n\ + printf -- '-----BEGIN PGP SIGNATURE-----\\n\\nfake\\n-----END PGP SIGNATURE-----\\n'\n\ + printf '[GNUPG:] BEGIN_SIGNING\\n[GNUPG:] SIG_CREATED fake\\n' 1>&2\n"; + std::fs::write(&script_path, script).unwrap(); + std::fs::set_permissions( + &script_path, + std::fs::Permissions::from_mode(0o755), + ) + .unwrap(); + + { + let mut cfg = repo.config().unwrap(); + cfg.set_bool("commit.gpgsign", true).unwrap(); + cfg.set_str("gpg.format", "openpgp").unwrap(); + cfg.set_str("gpg.program", script_path.to_str().unwrap()) + .unwrap(); + cfg.set_str("user.signingKey", "TEST_KEY").unwrap(); + } + + // build a 3-commit history with signing disabled so we can use + // `write_commit_file`, then enable signing for the reword. + repo.config() + .unwrap() + .set_bool("commit.gpgsign", false) + .unwrap(); + write_commit_file(&repo, "foo", "a", "commit1"); + let oid2 = write_commit_file(&repo, "foo", "ab", "commit2"); + write_commit_file(&repo, "foo", "abc", "commit3"); + repo.config() + .unwrap() + .set_bool("commit.gpgsign", true) + .unwrap(); + + let reworded = + reword(repo_path, oid2, "RewordedMiddle").unwrap(); + + // reworded commit carries the new message. + assert_eq!( + get_commit_info(repo_path, &reworded).unwrap().message, + "RewordedMiddle" + ); + + // HEAD still points to the descendant ("commit3") and the chain length is unchanged. + let head = repo.head().unwrap().peel_to_commit().unwrap(); + assert_eq!(head.message().unwrap().trim_end(), "commit3"); + assert_eq!(head.parent_count(), 1); + let middle = head.parent(0).unwrap(); + assert_eq!(middle.id(), reworded.get_oid()); + let first = middle.parent(0).unwrap(); + assert_eq!(first.message().unwrap().trim_end(), "commit1"); + assert!(first.parent(0).is_err()); + + // both rewritten commits carry the fake PGP signature. + let (sig_middle, _) = + repo.extract_signature(&middle.id(), None).unwrap(); + assert!(sig_middle + .as_str() + .unwrap() + .contains("BEGIN PGP SIGNATURE")); + let (sig_head, _) = + repo.extract_signature(&head.id(), None).unwrap(); + assert!(sig_head + .as_str() + .unwrap() + .contains("BEGIN PGP SIGNATURE")); + } } diff --git a/asyncgit/src/sync/sign.rs b/asyncgit/src/sync/sign.rs index bde0fc53d2..fe050e4ff3 100644 --- a/asyncgit/src/sync/sign.rs +++ b/asyncgit/src/sync/sign.rs @@ -79,6 +79,38 @@ pub trait Sign { fn signing_key(&self) -> &String; } +/// Build a signed commit object and return its [`git2::Oid`]. +/// +/// Creates the commit buffer, signs it with `signer` and writes the +/// signed commit. It does not move any reference, the caller is +/// responsible for advancing the relevant head or branch to the +/// returned id. +pub fn create_signed_commit( + repo: &git2::Repository, + signer: &dyn Sign, + author: &git2::Signature<'_>, + committer: &git2::Signature<'_>, + message: &str, + tree: &git2::Tree<'_>, + parents: &[&git2::Commit<'_>], +) -> crate::error::Result { + let buffer = repo.commit_create_buffer( + author, committer, message, tree, parents, + )?; + + let contents = std::str::from_utf8(&buffer).map_err(|_| { + SignError::Shellout("utf8 conversion error".to_string()) + })?; + + let (signature, signature_field) = signer.sign(&buffer)?; + + Ok(repo.commit_signed( + contents, + &signature, + signature_field.as_deref(), + )?) +} + /// A builder to facilitate the creation of a signing method ([`Sign`]) by examining the git configuration. pub struct SignBuilder;