feat(reword): support gpg signing for non-HEAD commits

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))

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,

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<CommitId> {
 
 	let parents = parents.iter().collect::<Vec<_>>();
 
-	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())
 }

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))

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<CommitId> {
+	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"));
+	}
 }