From 8f4155f7dd0e32839912592386049a38de9d2a6f Mon Sep 17 00:00:00 2001 From: Federico Guerinoni Date: Tue, 26 May 2026 15:02:58 +0200 Subject: [PATCH] feat(reword): support gpg signing for non-HEAD commits Rewording any commit older than HEAD used to fail with SignRewordNonLastCommit as soon as commit.gpgsign was enabled, because git2's rebase API has no way to sign the commits it produces. The behaviour matched neither git CLI nor user expectation, so anyone relying on signing had to drop down to the shell to fix a typo a few commits back. --- CHANGELOG.md | 1 + asyncgit/src/error.rs | 4 - asyncgit/src/sync/commit.rs | 89 ++++++++--------- asyncgit/src/sync/mod.rs | 2 + asyncgit/src/sync/reword.rs | 187 ++++++++++++++++++++++++++++++++++-- asyncgit/src/sync/sign.rs | 32 ++++++ 6 files changed, 253 insertions(+), 62 deletions(-) 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;