feat: enable programmatic signing in graph rebase engine

This commit puts down the groundwork for enabling sign-on-push
functionality by introducing new facilities to tweak sign behavior for the
graph rebase engine. The key design goal is to allow for programmatically
driven signing, where we can decide at runtime if a certain rebase operation
should cause signing (e.g. just before a push), while another one should not.

At the same time, we want to retain the old gitbutler.signCommits behavior for
the sake of Git interoperability - you might do some work with GitButler
and then decide to push with Git, and that should still work, respecting
signing settings.

The two concepts introduced to the graph rebase engine in this commit
is a `PickMode`, allowing the caller to tune when a commit is
cherry-picked, and `SignCommit`, which allows for tuning signing
behavior beyond what was previously possible. Setting `PickMode::Force`
and `SignCommit::Yes` or `SignCommit::IfSignCommitsEnabled` allows one
to sign a commit that is otherwise unchanged. For descendants to also be
signed, they need to be picked with an appropriate `SignCommit` setting.

None of the new behavior is as of yet exposed in any of the GitButler UIs.
That will come later down the road with sign-on-push functionality.

Author: slarse

crates/but-rebase/src/cherry_pick.rs

@@ -27,7 +27,7 @@ pub(crate) mod function {
 
     use anyhow::{Context as _, bail};
     use bstr::BString;
-    use but_core::commit::{HEADERS_CONFLICTED_FIELD, Headers, TreeKind};
+    use but_core::commit::{HEADERS_CONFLICTED_FIELD, Headers, SignCommit, TreeKind};
     use gix::{object::tree::EntryKind, prelude::ObjectIdExt};
     use serde::Serialize;
 
@@ -188,10 +188,13 @@ pub(crate) mod function {
                 )));
         }
         set_parent(&mut new_commit, head.id.detach())?;
-        Ok(
-            crate::commit::create(repo, new_commit, DateMode::CommitterUpdateAuthorKeep, true)?
-                .attach(repo),
-        )
+        Ok(crate::commit::create(
+            repo,
+            new_commit,
+            DateMode::CommitterUpdateAuthorKeep,
+            SignCommit::IfSignCommitsEnabled,
+        )?
+        .attach(repo))
     }
 
     fn commit_from_conflicted_tree<'repo>(
@@ -254,7 +257,7 @@ pub(crate) mod function {
             repo,
             to_rebase.inner,
             DateMode::CommitterUpdateAuthorKeep,
-            true,
+            SignCommit::IfSignCommitsEnabled,
         )?
         .attach(repo))
     }

crates/but-rebase/src/commit.rs

@@ -86,7 +86,7 @@ pub fn create(
     repo: &gix::Repository,
     mut commit: gix::objs::Commit,
     committer: DateMode,
-    sign_if_configured: bool,
+    sign_commit: SignCommit,
 ) -> anyhow::Result<gix::ObjectId> {
     match committer {
         DateMode::CommitterUpdateAuthorKeep => {
@@ -102,16 +102,7 @@ pub fn create(
     if settings.gitbutler_gerrit_mode.unwrap_or(false) {
         but_gerrit::set_trailers(&mut commit);
     }
-    but_core::commit::create(
-        repo,
-        commit,
-        None,
-        if sign_if_configured {
-            SignCommit::IfSignCommitsEnabled
-        } else {
-            SignCommit::No
-        },
-    )
+    but_core::commit::create(repo, commit, None, sign_commit)
 }
 
 /// Update the committer of `commit` to be the current one.

crates/but-rebase/src/graph_rebase/cherry_pick.rs

@@ -6,7 +6,7 @@ use anyhow::{Context as _, Result, bail};
 use bstr::BString;
 use but_core::{
     RepositoryExt as _,
-    commit::{HEADERS_CONFLICTED_FIELD, Headers, TreeKind},
+    commit::{HEADERS_CONFLICTED_FIELD, Headers, SignCommit, TreeKind},
 };
 use gix::{objs::tree::EntryKind, prelude::ObjectIdExt as _};
 
@@ -35,7 +35,18 @@ pub enum CherryPickOutcome {
     },
 }
 
-/// Cherry pick, but supports supports cherry-picking merge commits.
+/// Controls when a commit is cherry-picked during a rebase.
+#[derive(Debug, Clone, Copy, PartialEq)]
+pub enum PickMode {
+    /// Cherry-picks the commit only if it's necessary because of changes to the commit or its
+    /// parents.
+    IfChanged,
+    /// Forces a cherry-pick on a commit. This is for example useful in combination with
+    /// [`SignCommit`] to sign/unsign commits that are otherwise unchanged.
+    Force,
+}
+
+/// Cherry pick, but supports cherry-picking merge commits.
 ///
 /// When cherry-picking a commit onto two or more commits, we first find the
 /// merge of the two "onto" commits, and then cherry-pick onto that.
@@ -52,15 +63,19 @@ pub enum CherryPickOutcome {
 ///
 /// Except in the case where X is conflicted. In that case we then make use of
 /// X's "base" sub-tree as the base.
+///
+/// `pick_mode` - controls how to determine if a commit should be cherry-picked.
+/// `sign_commit` - controls how the resulting commit is signed.
 pub fn cherry_pick(
     repo: &gix::Repository,
     target: gix::ObjectId,
     ontos: &[gix::ObjectId],
-    sign_if_configured: bool,
+    pick_mode: PickMode,
+    sign_commit: SignCommit,
 ) -> Result<CherryPickOutcome> {
     let target = but_core::Commit::from_id(target.attach(repo))?;
 
-    if ontos == target.parents.as_slice() {
+    if ontos == target.parents.as_slice() && pick_mode != PickMode::Force {
         // We don't need to rebase
         return Ok(CherryPickOutcome::Identity(target.id.detach()));
     }
@@ -72,9 +87,22 @@ pub fn cherry_pick(
     let onto_t = tree_from_merging_commits(repo, ontos, TreeKind::AutoResolution)?;
 
     match (&base_t, &onto_t) {
+        (MergeOutcome::NoCommit, MergeOutcome::NoCommit) if pick_mode == PickMode::Force => {
+            // We should only end up here when trying to force-pick a parentless commit. At that
+            // point, it's safe to simply recreate that commit outright.
+            //
+            // Currently, the only known use case for this is to forcibly sign/unsign root commits.
+            let commit = crate::commit::create(
+                repo,
+                target.inner,
+                DateMode::CommitterUpdateAuthorKeep,
+                sign_commit,
+            )?;
+            Ok(CherryPickOutcome::Commit(commit))
+        }
         (MergeOutcome::NoCommit, MergeOutcome::NoCommit) => {
             // We shouldn't actually ever hit this because it should be handled
-            // by the ontos & parents comparison.
+            // by the ontos & parents comparison or the PickMode::Force case.
             Ok(CherryPickOutcome::Identity(target.id.detach()))
         }
         // TODO(cto): We can handle the specific case where (the base is
@@ -130,15 +158,14 @@ pub fn cherry_pick(
                     base_t,
                     onto_t,
                     target_t.detach(),
-                    sign_if_configured,
+                    sign_commit,
                 )?;
                 Ok(CherryPickOutcome::ConflictedCommit(
                     conflicted_commit.detach(),
                 ))
             } else {
                 Ok(CherryPickOutcome::Commit(
-                    commit_from_unconflicted_tree(ontos, target, tree_id, sign_if_configured)?
-                        .detach(),
+                    commit_from_unconflicted_tree(ontos, target, tree_id, sign_commit)?.detach(),
                 ))
             }
         }
@@ -273,7 +300,7 @@ fn commit_from_unconflicted_tree<'repo>(
     parents: &[gix::ObjectId],
     to_rebase: but_core::Commit<'repo>,
     resolved_tree_id: gix::Id<'repo>,
-    sign_if_configured: bool,
+    sign_commit: SignCommit,
 ) -> anyhow::Result<gix::Id<'repo>> {
     let repo = to_rebase.id.repo;
 
@@ -303,7 +330,7 @@ fn commit_from_unconflicted_tree<'repo>(
         repo,
         new_commit,
         DateMode::CommitterUpdateAuthorKeep,
-        sign_if_configured,
+        sign_commit,
     )?
     .attach(repo))
 }
@@ -318,7 +345,7 @@ fn commit_from_conflicted_tree<'repo>(
     base_tree_id: gix::ObjectId,
     ours_tree_id: gix::ObjectId,
     theirs_tree_id: gix::ObjectId,
-    sign_if_configured: bool,
+    sign_commit: SignCommit,
 ) -> anyhow::Result<gix::Id<'repo>> {
     let repo = resolved_tree_id.repo;
     // in case someone checks this out with vanilla Git, we should warn why it looks like this
@@ -370,7 +397,7 @@ fn commit_from_conflicted_tree<'repo>(
         repo,
         to_rebase.inner,
         DateMode::CommitterUpdateAuthorKeep,
-        sign_if_configured,
+        sign_commit,
     )?
     .attach(repo))
 }

crates/but-rebase/src/graph_rebase/commit.rs

@@ -2,6 +2,7 @@
 
 use anyhow::{Context, Result, bail};
 use but_core::RefMetadata;
+use but_core::commit::SignCommit;
 use gix::prelude::ObjectIdExt;
 
 use crate::{
@@ -74,9 +75,12 @@ impl<M: RefMetadata> Editor<'_, '_, M> {
         commit: but_core::CommitOwned,
         date_mode: DateMode,
     ) -> Result<gix::ObjectId> {
-        // TODO(GB-983): As part of moving to only signing at the materializing
-        // step, this should have sign_if_configured false here.
-        create(&self.repo, commit.inner, date_mode, true)
+        create(
+            &self.repo,
+            commit.inner,
+            date_mode,
+            SignCommit::IfSignCommitsEnabled,
+        )
     }
 
     /// Writes a commit with correct signing to the in memory repository.

crates/but-rebase/src/graph_rebase/creation.rs

@@ -1,22 +1,50 @@
 use std::collections::{HashMap, HashSet};
 
 use anyhow::{Result, bail};
-use but_core::RefMetadata;
+use but_core::{RefMetadata, commit::SignCommit};
 use but_graph::{Commit, SegmentIndex};
 use petgraph::{Direction, visit::EdgeRef as _};
 
 use crate::graph_rebase::{
     Checkout, Edge, Editor, Pick, RevisionHistory, Selector, Step, StepGraph, StepGraphIndex,
-    SuccessfulRebase, util,
+    SuccessfulRebase, cherry_pick::PickMode, util,
 };
 
+#[derive(Clone, Copy)]
+/// Options for the editor.
+pub struct GraphEditorOptions {
+    /// Determines when picks are cherry-picked during a rebase.
+    pub default_pick_mode: PickMode,
+    /// Determines how resulting commits are signed.
+    pub default_sign_commit: SignCommit,
+}
+
+impl Default for GraphEditorOptions {
+    fn default() -> Self {
+        Self {
+            default_pick_mode: PickMode::IfChanged,
+            default_sign_commit: SignCommit::IfSignCommitsEnabled,
+        }
+    }
+}
+
 /// Creates an editor out of the workspace graph.
 impl<'ws, 'meta, M: RefMetadata> Editor<'ws, 'meta, M> {
-    /// Creates an editor out of the workspace graph.
+    /// Creates an editor out of the workspace graph with the default options.
     pub fn create(
         workspace: &'ws mut but_graph::projection::Workspace,
         meta: &'meta mut M,
         repo: &gix::Repository,
+    ) -> Result<Self> {
+        Self::create_with_opts(workspace, meta, repo, &GraphEditorOptions::default())
+    }
+
+    /// Creates an editor out of the workspace graph with the specified options.
+    pub fn create_with_opts(
+        workspace: &'ws mut but_graph::projection::Workspace,
+        meta: &'meta mut M,
+        repo: &gix::Repository,
+        options: &GraphEditorOptions,
     ) -> Result<Self> {
         // This first creates runs of nodes and associates them with the
         // but-graph segments. We then do a second pass over all the segments
@@ -91,7 +119,10 @@ impl<'ws, 'meta, M: RefMetadata> Editor<'ws, 'meta, M> {
                     let pick = if workspace_commit_id == Some(commit.id) {
                         Pick::new_workspace_pick(commit.id)
                     } else {
-                        Pick::new_pick(commit.id)
+                        let mut pick = Pick::new_pick(commit.id);
+                        pick.pick_mode = options.default_pick_mode;
+                        pick.sign_commit = options.default_sign_commit;
+                        pick
                     };
                     let ix = graph.add_node(Step::Pick(pick));
                     commit_to_pick_ix.insert(commit.id, ix);

crates/but-rebase/src/graph_rebase/mod.rs

@@ -9,11 +9,14 @@ pub mod rebase;
 use std::collections::{BTreeMap, HashMap};
 
 use anyhow::{Context, Result, bail};
-use but_core::RefMetadata;
+use but_core::{RefMetadata, commit::SignCommit};
 use but_graph::init::Overlay;
+pub use creation::GraphEditorOptions;
 use gix::refs::transaction::RefEdit;
 
 use crate::graph_rebase::util::collect_ordered_parents;
+
+use crate::graph_rebase::cherry_pick::PickMode;
 pub mod cherry_pick;
 pub mod commit;
 pub mod materialize;
@@ -41,9 +44,14 @@ pub struct Pick {
     /// If set to true, a rebase will fail if not all of the parents (outgoing
     /// nodes) are references.
     pub parents_must_be_references: bool,
-    /// If set to true, the rebase engine will try to sign the commit if it
-    /// gets cherry-picked and the user has configured signing.
-    pub sign_if_configured: bool,
+    /// Controls under what circumstances the commit is cherry-picked.
+    pub pick_mode: PickMode,
+    /// Controls whether the resulting commit is signed.
+    ///
+    /// Note that signing a parent commit only causes descendants to be signed if those descendants
+    /// are also picked with a `sign_commit` value that enables signing (e.g. [`SignCommit::Yes`]
+    /// or [`SignCommit::IfSignCommitsEnabled`] with config enabled).
+    pub sign_commit: SignCommit,
     /// Exclude the commit from being included in the
     /// [`RevisionHistory::commit_mappings()`]. This is helpful if we are
     /// creating a new commit since the the mappings will be non-sensical to the
@@ -59,7 +67,8 @@ impl Pick {
             preserved_parents: None,
             conflictable: true,
             parents_must_be_references: false,
-            sign_if_configured: true,
+            pick_mode: PickMode::IfChanged,
+            sign_commit: SignCommit::IfSignCommitsEnabled,
             exclude_from_tracking: false,
         }
     }
@@ -81,7 +90,8 @@ impl Pick {
             preserved_parents: None,
             conflictable: false,
             parents_must_be_references: true,
-            sign_if_configured: false,
+            pick_mode: PickMode::IfChanged,
+            sign_commit: SignCommit::No,
             exclude_from_tracking: false,
         }
     }
@@ -419,7 +429,9 @@ impl RevisionHistory {
 mod test {
     use std::str::FromStr;
 
-    use crate::graph_rebase::Pick;
+    use but_core::commit::SignCommit;
+
+    use crate::graph_rebase::{Pick, cherry_pick::PickMode};
 
     #[test]
     fn workspace_commit_defaults() -> anyhow::Result<()> {
@@ -432,7 +444,8 @@ mod test {
                 preserved_parents: None,
                 conflictable: false,
                 parents_must_be_references: true,
-                sign_if_configured: false,
+                pick_mode: PickMode::IfChanged,
+                sign_commit: SignCommit::No,
                 exclude_from_tracking: false
             }
         );
@@ -451,7 +464,8 @@ mod test {
                 preserved_parents: None,
                 conflictable: true,
                 parents_must_be_references: false,
-                sign_if_configured: true,
+                pick_mode: PickMode::IfChanged,
+                sign_commit: SignCommit::IfSignCommitsEnabled,
                 exclude_from_tracking: false
             }
         );

crates/but-rebase/src/graph_rebase/rebase.rs

@@ -70,8 +70,13 @@ impl<'ws, 'graph, M: RefMetadata> Editor<'ws, 'graph, M> {
                             .collect::<Result<Vec<_>>>()?,
                     };
 
-                    let outcome =
-                        cherry_pick(&self.repo, pick.id, &ontos, pick.sign_if_configured)?;
+                    let outcome = cherry_pick(
+                        &self.repo,
+                        pick.id,
+                        &ontos,
+                        pick.pick_mode,
+                        pick.sign_commit,
+                    )?;
 
                     if matches!(outcome, CherryPickOutcome::ConflictedCommit(_))
                         && !pick.conflictable