feat(rebase): support GPG-signing for all rebase-based commands

Author: tommyip

git-branchless-lib/src/core/rewrite/execute.rs

@@ -15,7 +15,7 @@ use crate::core::formatting::Pluralize;
 use crate::core::repo_ext::RepoExt;
 use crate::git::{
     BranchType, CategorizedReferenceName, GitRunInfo, MaybeZeroOid, NonZeroOid, ReferenceName,
-    Repo, ResolvedReferenceInfo,
+    Repo, ResolvedReferenceInfo, SignOption,
 };
 use crate::util::{ExitCode, EyreExitOr};
 
@@ -435,7 +435,8 @@ mod in_memory {
     use crate::core::rewrite::move_branches;
     use crate::core::rewrite::plan::{OidOrLabel, RebaseCommand, RebasePlan};
     use crate::git::{
-        CherryPickFastError, CherryPickFastOptions, GitRunInfo, MaybeZeroOid, NonZeroOid, Repo,
+        self, CherryPickFastError, CherryPickFastOptions, GitRunInfo, MaybeZeroOid, NonZeroOid,
+        Repo,
     };
     use crate::util::EyreExitOr;
 
@@ -498,6 +499,7 @@ mod in_memory {
             force_on_disk: _,
             resolve_merge_conflicts: _, // May be needed once we can resolve merge conflicts in memory.
             check_out_commit_options: _, // Caller is responsible for checking out to new HEAD.
+            sign_option,
         } = options;
 
         let mut current_oid = rebase_plan.first_dest_oid;
@@ -535,6 +537,8 @@ mod in_memory {
             .count();
         let (effects, progress) = effects.start_operation(OperationType::RebaseCommits);
 
+        let signer = git::get_signer(&repo, sign_option)?;
+
         for command in rebase_plan.commands.iter() {
             match command {
                 RebaseCommand::CreateLabel { label_name } => {
@@ -633,7 +637,7 @@ mod in_memory {
                             commit_message,
                             &commit_tree,
                             vec![&current_commit],
-                            None,
+                            signer.as_deref(),
                         )
                         .wrap_err("Applying rebased commit")?;
 
@@ -753,7 +757,7 @@ mod in_memory {
                             replacement_commit_message,
                             &replacement_tree,
                             parents.iter().collect(),
-                            None,
+                            signer.as_deref(),
                         )
                         .wrap_err("Applying rebased commit")?;
 
@@ -864,6 +868,7 @@ mod in_memory {
             force_on_disk: _,
             resolve_merge_conflicts: _,
             check_out_commit_options,
+            sign_option: _,
         } = options;
 
         // Note that if an OID has been mapped to multiple other OIDs, then the last
@@ -959,6 +964,7 @@ mod on_disk {
             force_on_disk: _,
             resolve_merge_conflicts: _,
             check_out_commit_options: _, // Checkout happens after rebase has concluded.
+            sign_option,
         } = options;
 
         let (effects, _progress) = effects.start_operation(OperationType::InitializeRebase);
@@ -1073,6 +1079,16 @@ mod on_disk {
             )
         })?;
 
+        let gpg_sign_opt_file = rebase_state_dir.join("gpg_sign_opt");
+        if let Some(sign_flag) = sign_option.as_rebase_flag(repo)? {
+            std::fs::write(&gpg_sign_opt_file, sign_flag).wrap_err_with(|| {
+                format!(
+                    "Writing `gpg_sign_opt` to: {:?}",
+                    gpg_sign_opt_file.as_path()
+                )
+            })?;
+        }
+
         let end_file_path = rebase_state_dir.join("end");
         std::fs::write(
             end_file_path.as_path(),
@@ -1132,6 +1148,7 @@ mod on_disk {
             force_on_disk: _,
             resolve_merge_conflicts: _,
             check_out_commit_options: _, // Checkout happens after rebase has concluded.
+            sign_option: _,
         } = options;
 
         match write_rebase_state_to_disk(effects, git_run_info, repo, rebase_plan, options)? {
@@ -1176,6 +1193,9 @@ pub struct ExecuteRebasePlanOptions {
 
     /// If `HEAD` was moved, the options for checking out the new `HEAD` commit.
     pub check_out_commit_options: CheckOutCommitOptions,
+
+    /// GPG-sign commits.
+    pub sign_option: SignOption,
 }
 
 /// The result of executing a rebase plan.
@@ -1221,6 +1241,7 @@ pub fn execute_rebase_plan(
         force_on_disk,
         resolve_merge_conflicts,
         check_out_commit_options: _,
+        sign_option: _,
     } = options;
 
     if !force_on_disk {

git-branchless-lib/src/git/sign.rs

@@ -3,7 +3,7 @@ use tracing::instrument;
 use super::{repo::Result, Repo, RepoError};
 
 /// GPG-signing option.
-#[derive(Debug, PartialEq, Eq)]
+#[derive(Debug, Clone, PartialEq, Eq)]
 pub enum SignOption {
     /// Sign commits conditionally based on the `commit.gpgsign` configuration and
     /// and the key `user.signingkey`.
@@ -26,6 +26,22 @@ impl SignOption {
             Self::Disable => Some("--no-gpg-sign".to_string()),
         }
     }
+
+    /// GPG-signing flag to use for interactive rebase
+    pub fn as_rebase_flag(&self, repo: &Repo) -> Result<Option<String>> {
+        Ok(match self {
+            Self::UseConfig => {
+                let config = repo.inner.config().map_err(RepoError::ReadConfig)?;
+                match config.get_bool("commit.gpgsign").ok() {
+                    Some(true) => Some("-S".to_string()),
+                    Some(false) | None => None,
+                }
+            }
+            Self::UseConfigKey => Some("-S".to_string()),
+            Self::KeyOverride(keyid) => Some(format!("-S{}", keyid)),
+            Self::Disable => None,
+        })
+    }
 }
 
 /// Get commit signer configured from CLI arguments and repository configurations.

git-branchless-lib/tests/test_rewrite_plan.rs

@@ -15,6 +15,7 @@ use branchless::core::rewrite::{
     execute_rebase_plan, BuildRebasePlanOptions, ExecuteRebasePlanOptions, ExecuteRebasePlanResult,
     RebasePlan, RebasePlanBuilder, RepoResource,
 };
+use branchless::git::SignOption;
 use git_branchless_testing::{make_git, Git};
 
 #[test]
@@ -713,6 +714,7 @@ fn create_and_execute_plan(
             reset: false,
             render_smartlog: false,
         },
+        sign_option: SignOption::Disable,
     };
     let git_run_info = git.get_git_run_info();
     let result = execute_rebase_plan(

git-branchless-move/src/lib.rs

@@ -261,6 +261,7 @@ pub fn r#move(
         resolve_merge_conflicts,
         dump_rebase_constraints,
         dump_rebase_plan,
+        ref sign_options,
     } = *move_options;
     let now = SystemTime::now();
     let event_tx_id = event_log_db.make_transaction_id(now, "move")?;
@@ -471,6 +472,7 @@ pub fn r#move(
                 force_on_disk,
                 resolve_merge_conflicts,
                 check_out_commit_options: Default::default(),
+                sign_option: sign_options.to_owned().into(),
             };
             execute_rebase_plan(
                 effects,

git-branchless-opts/src/lib.rs

@@ -91,6 +91,10 @@ pub struct MoveOptions {
     /// executing it.
     #[clap(action, long = "debug-dump-rebase-plan")]
     pub dump_rebase_plan: bool,
+
+    /// GPG-sign commits.
+    #[clap(flatten)]
+    pub sign_options: SignOptions,
 }
 
 /// Options for traversing commits.
@@ -179,7 +183,7 @@ pub struct SwitchOptions {
 }
 
 /// Options for signing commits
-#[derive(Args, Debug)]
+#[derive(Args, Debug, Clone)]
 pub struct SignOptions {
     /// GPG-sign commits. The `keyid` argument is optional and defaults to the committer
     /// identity.
@@ -464,10 +468,6 @@ pub enum Command {
         /// formatting or refactoring changes.
         #[clap(long)]
         reparent: bool,
-
-        /// Options for signing commits.
-        #[clap(flatten)]
-        sign_options: SignOptions,
     },
 
     /// Gather information about recent operations to upload as part of a bug
@@ -650,6 +650,10 @@ pub enum Command {
         /// use with `git rebase --autosquash`) targeting the supplied commit.
         #[clap(value_parser, long = "fixup", conflicts_with_all(&["messages", "discard"]))]
         commit_to_fixup: Option<Revset>,
+
+        /// GPG-sign commits.
+        #[clap(flatten)]
+        sign_options: SignOptions,
     },
 
     /// `smartlog` command.

git-branchless-record/src/lib.rs

@@ -215,7 +215,8 @@ fn record(
             effects,
             git_run_info,
             now,
-            event_tx_id
+            event_tx_id,
+            sign_option,
         )?);
     }
 
@@ -357,6 +358,7 @@ fn insert_before_siblings(
     git_run_info: &GitRunInfo,
     now: SystemTime,
     event_tx_id: EventTransactionId,
+    sign_option: &SignOption,
 ) -> EyreExitOr<()> {
     // Reopen the repository since references may have changed.
     let repo = Repo::from_dir(&git_run_info.working_directory)?;
@@ -484,6 +486,7 @@ To proceed anyways, run: git move -f -s 'siblings(.)",
         force_on_disk: false,
         resolve_merge_conflicts: false,
         check_out_commit_options: Default::default(),
+        sign_option: sign_option.to_owned(),
     };
     let result = execute_rebase_plan(
         effects,

git-branchless-reword/src/lib.rs

@@ -42,7 +42,7 @@ use lib::core::rewrite::{
 };
 use lib::git::{message_prettify, Commit, GitRunInfo, MaybeZeroOid, NonZeroOid, Repo};
 
-use git_branchless_opts::{ResolveRevsetOptions, Revset};
+use git_branchless_opts::{ResolveRevsetOptions, Revset, SignOptions};
 use git_branchless_revset::resolve_commits;
 
 /// The commit message(s) provided by the user.
@@ -67,6 +67,7 @@ pub fn reword(
     messages: InitialCommitMessages,
     git_run_info: &GitRunInfo,
     force_rewrite_public_commits: bool,
+    sign_options: SignOptions,
 ) -> EyreExitOr<()> {
     let repo = Repo::from_current_dir()?;
     let references_snapshot = repo.get_references_snapshot()?;
@@ -290,6 +291,7 @@ pub fn reword(
             reset: false,
             render_smartlog: false,
         },
+        sign_option: sign_options.into(),
     };
     let result = execute_rebase_plan(
         effects,

git-branchless-submit/src/phabricator.rs

@@ -26,7 +26,9 @@ use lib::core::rewrite::{
     execute_rebase_plan, BuildRebasePlanError, BuildRebasePlanOptions, ExecuteRebasePlanOptions,
     ExecuteRebasePlanResult, RebasePlanBuilder, RebasePlanPermissions, RepoResource,
 };
-use lib::git::{Commit, GitRunInfo, MaybeZeroOid, NonZeroOid, Repo, RepoError, TestCommand};
+use lib::git::{
+    Commit, GitRunInfo, MaybeZeroOid, NonZeroOid, Repo, RepoError, SignOption, TestCommand,
+};
 use lib::try_exit_code;
 use lib::util::{ExitCode, EyreExitOr};
 use rayon::ThreadPoolBuilder;
@@ -359,6 +361,7 @@ impl Forge for PhabricatorForge<'_> {
                 render_smartlog: false,
                 ..Default::default()
             },
+            sign_option: SignOption::Disable,
         };
         let permissions =
             RebasePlanPermissions::verify_rewrite_set(self.dag, build_options, &commit_set)
@@ -608,6 +611,7 @@ Differential Revision: https://phabricator.example.com/D000$(git rev-list --coun
                 render_smartlog: false,
                 ..Default::default()
             },
+            sign_option: SignOption::Disable,
         };
         let permissions =
             RebasePlanPermissions::verify_rewrite_set(self.dag, build_options, &commit_set)

git-branchless-test/src/lib.rs

@@ -51,8 +51,9 @@ use lib::core::rewrite::{
 use lib::git::{
     get_latest_test_command_path, get_test_locks_dir, get_test_tree_dir, get_test_worktrees_dir,
     make_test_command_slug, Commit, ConfigRead, GitRunInfo, GitRunResult, MaybeZeroOid, NonZeroOid,
-    Repo, SerializedNonZeroOid, SerializedTestResult, TestCommand, WorkingCopyChangesType,
-    TEST_ABORT_EXIT_CODE, TEST_INDETERMINATE_EXIT_CODE, TEST_SUCCESS_EXIT_CODE,
+    Repo, SerializedNonZeroOid, SerializedTestResult, SignOption, TestCommand,
+    WorkingCopyChangesType, TEST_ABORT_EXIT_CODE, TEST_INDETERMINATE_EXIT_CODE,
+    TEST_SUCCESS_EXIT_CODE,
 };
 use lib::try_exit_code;
 use lib::util::{get_sh, ExitCode, EyreExitOr};
@@ -386,6 +387,7 @@ BUG: Expected resolved_interactive ({resolved_interactive:?}) to match interacti
                 resolve_merge_conflicts,
                 dump_rebase_constraints,
                 dump_rebase_plan,
+                sign_options,
             } = move_options;
 
             let force_in_memory = true;
@@ -414,6 +416,7 @@ BUG: Expected resolved_interactive ({resolved_interactive:?}) to match interacti
                     render_smartlog: false,
                     ..Default::default()
                 },
+                sign_option: sign_options.to_owned().into(),
             };
             let permissions =
                 match RebasePlanPermissions::verify_rewrite_set(dag, build_options, commits)? {
@@ -730,6 +733,7 @@ fn set_abort_trap(
                 render_smartlog: false,
                 ..Default::default()
             },
+            sign_option: SignOption::Disable,
         },
     )? {
         ExecuteRebasePlanResult::Succeeded { rewritten_oids: _ } => {

git-branchless/src/commands/amend.rs

@@ -26,7 +26,7 @@ use lib::core::rewrite::{
     execute_rebase_plan, move_branches, BuildRebasePlanOptions, ExecuteRebasePlanOptions,
     ExecuteRebasePlanResult, RebasePlanBuilder, RebasePlanPermissions, RepoResource,
 };
-use lib::git::{get_signer, SignOption};
+use lib::git::get_signer;
 use lib::git::{AmendFastOptions, GitRunInfo, MaybeZeroOid, Repo, ResolvedReferenceInfo};
 use lib::try_exit_code;
 use lib::util::{ExitCode, EyreExitOr};
@@ -41,7 +41,6 @@ pub fn amend(
     resolve_revset_options: &ResolveRevsetOptions,
     move_options: &MoveOptions,
     reparent: bool,
-    sign_option: SignOption,
 ) -> EyreExitOr<()> {
     let now = SystemTime::now();
     let timestamp = now.duration_since(SystemTime::UNIX_EPOCH)?.as_secs_f64();
@@ -157,6 +156,7 @@ pub fn amend(
         )
     };
 
+    let sign_option = move_options.sign_options.to_owned().into();
     let signer = get_signer(&repo, &sign_option)?;
 
     let amended_commit_oid = repo.amend_commit(
@@ -305,6 +305,7 @@ pub fn amend(
                 reset: true,
                 render_smartlog: false,
             },
+            sign_option,
         };
         match execute_rebase_plan(
             effects,