Merge pull request #13046 from gitbutlerapp/GB-1121/but-rebase-sign-on-push

feat(but-rebase): add programmatic signing control for graph engine

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,7 +1,7 @@
 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 _};
 
@@ -10,13 +10,38 @@ use crate::graph_rebase::{
     SuccessfulRebase, util,
 };
 
+#[derive(Clone, Copy)]
+/// Options for the editor.
+pub struct GraphEditorOptions {
+    /// Determines how cherry-picked commits are signed.
+    pub default_sign_commit: SignCommit,
+}
+
+impl Default for GraphEditorOptions {
+    fn default() -> Self {
+        Self {
+            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 +116,9 @@ 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.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