feat(but-core): add programmatic signing control to commit creation#12999
feat(but-core): add programmatic signing control to commit creation#12999
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
slarse
force-pushed
the
GB-1121/sign-on-push-groundwork
branch
2 times, most recently
from
1af2176 to
60ffe7c
Compare
slarse
force-pushed
the
GB-1121/sign-on-push-groundwork
branch
2 times, most recently
from
e3a1aaa to
58dc1c0
Compare
| let target = but_core::Commit::from_id(target.attach(repo))?; | ||
|
|
||
| if ontos == target.parents.as_slice() { | ||
| if ontos == target.parents.as_slice() && sign_mode != PickSignMode::Force { |
There was a problem hiding this comment.
This is the only place where PickSignMode::Force differs from PickSignMode::IfChanged.
There was a problem hiding this comment.
Pull request overview
Lays the groundwork for sign-on-push by replacing the old boolean “sign if configured” flow with explicit signing modes, and by adding but-workspace actions to (a) list unsigned commits and (b) force signing of selected commits via the graph rebase engine—while keeping legacy gitbutler.signCommits behavior compatible.
Changes:
- Introduce
but_core::commit::SignCommitand thread it through commit creation paths to replacesign_if_configured: bool. - Add graph-rebase signing control via
PickSignModeandGraphEditorOptions, enabling forced signing and “if changed” cascades. - Add but-workspace APIs + fixtures/tests for listing unsigned commits and signing commits in-place via the editor.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/gitbutler-repo/src/repository_ext.rs | Switch gix commit creation to SignCommit (legacy/no) instead of bool. |
| crates/gitbutler-edit-mode/src/lib.rs | Update edit-mode rebase commit creation to use legacy signing mode enum. |
| crates/but-workspace/tests/workspace/commit/mod.rs | Register new workspace commit tests for unsigned listing and signing. |
| crates/but-workspace/tests/workspace/commit/commits_sign.rs | Add tests covering force-signing and cascade signing behaviors. |
| crates/but-workspace/tests/workspace/commit/commits_list_unsigned.rs | Add tests for unsigned-commit enumeration with optional stack filtering. |
| crates/but-workspace/tests/fixtures/scenario/ws-with-mixed-signed-unsigned.sh | Add fixture workspace with mixed signed/unsigned commits for new tests. |
| crates/but-workspace/src/commit_engine/mod.rs | Route commit creation through SignCommit (legacy compatibility). |
| crates/but-workspace/src/commit/mod.rs | Export new commit actions commits_sign and commits_list_unsigned. |
| crates/but-workspace/src/commit/commits_sign.rs | Implement editor-driven commit signing via forced-pick steps. |
| crates/but-workspace/src/commit/commits_list_unsigned.rs | Implement unsigned commit enumeration by inspecting commit headers. |
| crates/but-rebase/tests/rebase/graph_rebase/signing_preferences.rs | Update tests to use PickSignMode instead of legacy bool. |
| crates/but-rebase/tests/rebase/graph_rebase/cherry_pick.rs | Update cherry-pick tests to pass explicit sign mode. |
| crates/but-rebase/src/merge.rs | Update merge commit creation to use legacy signing enum. |
| crates/but-rebase/src/lib.rs | Update rebase paths that create commits to use legacy signing enum. |
| crates/but-rebase/src/graph_rebase/rebase.rs | Pass PickSignMode through to cherry-pick during graph rebase. |
| crates/but-rebase/src/graph_rebase/mod.rs | Replace pick bool with PickSignMode; add new_pick_with_sign_mode. |
| crates/but-rebase/src/graph_rebase/creation.rs | Add GraphEditorOptions to set default pick signing behavior. |
| crates/but-rebase/src/graph_rebase/commit.rs | Update editor commit creation to use SignCommit enum. |
| crates/but-rebase/src/graph_rebase/cherry_pick.rs | Introduce PickSignMode and map it into SignCommit; adjust identity behavior for forced signing. |
| crates/but-rebase/src/commit.rs | Switch commit creation API to accept SignCommit. |
| crates/but-rebase/src/cherry_pick.rs | Update cherry-pick implementation to use SignCommit enum. |
| crates/but-core/tests/core/commit.rs | Update commit signing tests to use SignCommit enum values. |
| crates/but-core/src/commit.rs | Add SignCommit enum and implement new signing decision logic in commit creation. |
Comments suppressed due to low confidence (1)
crates/but-rebase/src/graph_rebase/mod.rs:49
- Spelling: duplicated word in doc comment ("since the the mappings").
/// creating a new commit since the the mappings will be non-sensical to the
/// frontend consumers.
| use gix::{ObjectId, hashtable::HashSet}; | ||
|
|
||
| /// The result of listing unsigned commits in the workspace. | ||
| #[derive(Debug)] | ||
| pub struct CommitsListUnsignedOutcome { | ||
| /// IDs of all unsigned commits in the workspace | ||
| pub unsigned_commits: HashSet<ObjectId>, | ||
| } |
There was a problem hiding this comment.
CommitsListUnsignedOutcome exposes gix::hashtable::HashSet in a public API. This leaks an internal gix container type to API consumers and makes the API harder to use/stabilize; prefer returning std::collections::HashSet<gix::ObjectId> (or a Vec/BTreeSet if ordering is desired).
There was a problem hiding this comment.
I feel like this is fine. This is an efficient HashSet implementation for already hashed things, which is something we have a lot of in GitButler.
slarse
force-pushed
the
GB-1121/sign-on-push-groundwork
branch
from
58dc1c0 to
d905572
Compare
There was a problem hiding this comment.
Thanks, I really wanted to approve it but ultimately couldn't. I looked at but-core, and but-workspace in detail, but also glimpsed at but-rebase.
What I'd want to be merged immediately is the improvements to the signing logic via SignCommits, as the enum offers a place to spell out behaviour previously hidden behind true/false. From that point of view it would have made sense to split the PR into distinct independent commits as it's possible.
Here is patch with changes that I hope you can apply. I would have pushed it into the branch, but since it's a GitButler stack I think it would be quite unpleasant to deal with right now.
My suggestion is to turn this PR into something without but-rebase logic changes (but with the new enum), and with commit_signed so it can be merged. More details follow.
Patch
commit a290f4c1df51a1e3241f5283f72595610374c15c
Author: Byron <sebastian.thiel@icloud.com>
Date: Tue Mar 24 12:02:53 2026 +0800
review
diff --git a/crates/but-core/src/commit.rs b/crates/but-core/src/commit.rs
index 8648c869ab..d024e685a6 100644
--- a/crates/but-core/src/commit.rs
+++ b/crates/but-core/src/commit.rs
@@ -149,20 +149,29 @@ impl From<&Headers> for Vec<(BString, BString)> {
}
/// Determines how to sign the commit.
-#[derive(PartialEq, Copy, Clone, Debug)]
+#[derive(Default, PartialEq, Copy, Clone, Debug)]
pub enum SignCommit {
- /// Sign
+ /// Sign the commit.
Yes,
- /// Do not sign
+ /// Do not sign the commit.
No,
- /// **Deprecated:** Legacy behavior, signs the commit if gitbutler.signCommits=true
- LegacyIfSignCommitsEnabled,
+ /// Signs the commit if `gitbutler.signCommits=true` *or* if `commit.gpgSign=true`.
+ /// or do *not* sign if neither of these are set.
+ /// `gitbutler.signCommits` takes precedence, so if it is set, it overrules `commit.gpgSign`.
+ /// The reason for this is that some clients cannot ensure that they can execute GPG/SSH in such
+ /// a way that it will work correctly or be in the `PATH` at all, not to talk about the various
+ /// ways a password for a key can be requested.
+ /// This is the safest option, as the other options represent a programmatic override of
+ /// user preferences.
+ #[default]
+ IfSignCommitsEnabled,
}
/// Write `commit` into `repo`, removing any existing commit signature first, optionally creating a
/// new one based on repository configuration, and optionally updating `update_ref` to the new ID.
///
/// Apply any desired message/header mutations, such as Gerrit trailers, before calling this helper.
+/// `sign_commit` defines under which [circumstances](SignCommit) a signature for `commit` should be created.
pub fn create(
repo: &gix::Repository,
mut commit: gix::objs::Commit,
@@ -176,7 +185,7 @@ pub fn create(
commit.extra_headers.remove(pos);
}
- if (sign_commit == SignCommit::LegacyIfSignCommitsEnabled
+ if (sign_commit == SignCommit::IfSignCommitsEnabled
&& repo.git_settings()?.gitbutler_sign_commits.unwrap_or(false))
|| sign_commit == SignCommit::Yes
{
@@ -190,7 +199,7 @@ pub fn create(
}
Err(err) => {
tracing::warn!("Commit signing failed with sign_commit={sign_commit:?}");
- if sign_commit == SignCommit::LegacyIfSignCommitsEnabled {
+ if sign_commit == SignCommit::IfSignCommitsEnabled {
if repo
.config_snapshot()
.boolean_filter("gitbutler.signCommits", |md| {
diff --git a/crates/but-core/tests/core/commit.rs b/crates/but-core/tests/core/commit.rs
index b366b60977..5faef42900 100644
--- a/crates/but-core/tests/core/commit.rs
+++ b/crates/but-core/tests/core/commit.rs
@@ -174,7 +174,7 @@ mod create {
&repo,
commit_from_head(&repo, "signed again")?,
None,
- SignCommit::LegacyIfSignCommitsEnabled,
+ SignCommit::IfSignCommitsEnabled,
)?;
let commit = repo.find_commit(oid)?;
let commit = commit.decode()?;
@@ -186,6 +186,30 @@ mod create {
Ok(())
}
+ #[test]
+ fn forcefully_signs_commits() -> anyhow::Result<()> {
+ let (mut repo, _tmp) = writable_scenario_with_ssh_key("single-signed");
+ repo.config_snapshot_mut()
+ .set_raw_value(&"gitbutler.signCommits", "false")?;
+ repo.config_snapshot_mut()
+ .set_raw_value(&"commit.gpgSign", "false")?;
+
+ let oid = commit::create(
+ &repo,
+ commit_from_head(&repo, "should not sign")?,
+ None,
+ SignCommit::Yes,
+ )?;
+ let commit = repo.find_commit(oid)?;
+ let commit = commit.decode()?;
+
+ assert!(
+ commit.extra_headers().find(SIGNATURE_FIELD_NAME).is_some(),
+ "despite the settings saying everything is disabled, it still signs the commit"
+ );
+ Ok(())
+ }
+
#[test]
fn removes_existing_signature_when_not_signing() -> anyhow::Result<()> {
let (repo, _tmp) = writable_scenario_with_ssh_key("single-signed");
diff --git a/crates/but-rebase/src/cherry_pick.rs b/crates/but-rebase/src/cherry_pick.rs
index 060a1f0b58..c38ca99e3d 100644
--- a/crates/but-rebase/src/cherry_pick.rs
+++ b/crates/but-rebase/src/cherry_pick.rs
@@ -192,7 +192,7 @@ pub(crate) mod function {
repo,
new_commit,
DateMode::CommitterUpdateAuthorKeep,
- SignCommit::LegacyIfSignCommitsEnabled,
+ SignCommit::IfSignCommitsEnabled,
)?
.attach(repo))
}
@@ -257,7 +257,7 @@ pub(crate) mod function {
repo,
to_rebase.inner,
DateMode::CommitterUpdateAuthorKeep,
- SignCommit::LegacyIfSignCommitsEnabled,
+ SignCommit::IfSignCommitsEnabled,
)?
.attach(repo))
}
diff --git a/crates/but-rebase/src/graph_rebase/cherry_pick.rs b/crates/but-rebase/src/graph_rebase/cherry_pick.rs
index 4f2c8d85cd..28aab79b2c 100644
--- a/crates/but-rebase/src/graph_rebase/cherry_pick.rs
+++ b/crates/but-rebase/src/graph_rebase/cherry_pick.rs
@@ -51,13 +51,11 @@ pub enum CherryPickOutcome {
pub enum PickSignMode {
/// The picked commit is never signed.
Never,
- /// **Deprecated:** This is the legacy behavior, where a commit is signed if
- /// gitbutler.signCommits is enabled. It's here as a compatibility mode only and will be
- /// removed in the future.
+ /// A commit is signed according to the logic of [`SignCommit::IfSignCommitsEnabled`].
///
/// This mode is generally unwieldy to use as it causes us to sign commits _all the time_ when
/// rebasing.
- LegacyIfSignCommitsEnabled,
+ IfSignCommitsEnabled,
/// Signs the commit only if something has changed. If the Pick is otherwise a noop, this has
/// no effect, even if the picked commit is currently unsigned.
IfChanged,
@@ -72,7 +70,7 @@ impl From<PickSignMode> for SignCommit {
match value {
PickSignMode::Never => SignCommit::No,
PickSignMode::Force | PickSignMode::IfChanged => SignCommit::Yes,
- PickSignMode::LegacyIfSignCommitsEnabled => SignCommit::LegacyIfSignCommitsEnabled,
+ PickSignMode::IfSignCommitsEnabled => SignCommit::IfSignCommitsEnabled,
}
}
}
diff --git a/crates/but-rebase/src/graph_rebase/commit.rs b/crates/but-rebase/src/graph_rebase/commit.rs
index 0220086d90..ecc61ae517 100644
--- a/crates/but-rebase/src/graph_rebase/commit.rs
+++ b/crates/but-rebase/src/graph_rebase/commit.rs
@@ -79,7 +79,7 @@ impl<M: RefMetadata> Editor<'_, '_, M> {
&self.repo,
commit.inner,
date_mode,
- SignCommit::LegacyIfSignCommitsEnabled,
+ SignCommit::IfSignCommitsEnabled,
)
}
diff --git a/crates/but-rebase/src/graph_rebase/creation.rs b/crates/but-rebase/src/graph_rebase/creation.rs
index 354ed1696e..e498c34f12 100644
--- a/crates/but-rebase/src/graph_rebase/creation.rs
+++ b/crates/but-rebase/src/graph_rebase/creation.rs
@@ -20,7 +20,7 @@ pub struct GraphEditorOptions {
impl Default for GraphEditorOptions {
fn default() -> Self {
Self {
- sign_mode: PickSignMode::LegacyIfSignCommitsEnabled,
+ sign_mode: PickSignMode::IfSignCommitsEnabled,
}
}
}
diff --git a/crates/but-rebase/src/graph_rebase/mod.rs b/crates/but-rebase/src/graph_rebase/mod.rs
index 359ca33e6c..672611bce9 100644
--- a/crates/but-rebase/src/graph_rebase/mod.rs
+++ b/crates/but-rebase/src/graph_rebase/mod.rs
@@ -58,7 +58,7 @@ impl Pick {
preserved_parents: None,
conflictable: true,
parents_must_be_references: false,
- sign_mode: PickSignMode::LegacyIfSignCommitsEnabled,
+ sign_mode: PickSignMode::IfSignCommitsEnabled,
exclude_from_tracking: false,
}
}
@@ -395,7 +395,7 @@ mod test {
preserved_parents: None,
conflictable: true,
parents_must_be_references: false,
- sign_mode: PickSignMode::LegacyIfSignCommitsEnabled,
+ sign_mode: PickSignMode::IfSignCommitsEnabled,
exclude_from_tracking: false
}
);
diff --git a/crates/but-rebase/src/lib.rs b/crates/but-rebase/src/lib.rs
index 917691d5ce..13e61ce297 100644
--- a/crates/but-rebase/src/lib.rs
+++ b/crates/but-rebase/src/lib.rs
@@ -295,7 +295,7 @@ fn rebase(
repo,
new_commit,
DateMode::CommitterUpdateAuthorKeep,
- SignCommit::LegacyIfSignCommitsEnabled,
+ SignCommit::IfSignCommitsEnabled,
)?);
}
None => {
@@ -334,7 +334,7 @@ fn rebase(
repo,
new_commit,
DateMode::CommitterUpdateAuthorKeep,
- SignCommit::LegacyIfSignCommitsEnabled,
+ SignCommit::IfSignCommitsEnabled,
)?;
}
RebaseStep::Reference(reference) => {
@@ -379,7 +379,7 @@ fn reword_commit(
repo,
new_commit,
DateMode::CommitterUpdateAuthorKeep,
- SignCommit::LegacyIfSignCommitsEnabled,
+ SignCommit::IfSignCommitsEnabled,
)?)
}
@@ -395,7 +395,7 @@ pub fn replace_commit_tree(
repo,
new_commit,
DateMode::CommitterUpdateAuthorKeep,
- SignCommit::LegacyIfSignCommitsEnabled,
+ SignCommit::IfSignCommitsEnabled,
)?)
}
diff --git a/crates/but-rebase/src/merge.rs b/crates/but-rebase/src/merge.rs
index 26e98706f4..b987077308 100644
--- a/crates/but-rebase/src/merge.rs
+++ b/crates/but-rebase/src/merge.rs
@@ -98,7 +98,7 @@ pub fn octopus(
repo,
target_merge_commit,
DateMode::CommitterUpdateAuthorKeep,
- SignCommit::LegacyIfSignCommitsEnabled,
+ SignCommit::IfSignCommitsEnabled,
)
} else {
crate::commit::update_committer(repo, &mut target_merge_commit)?;
diff --git a/crates/but-rebase/tests/rebase/graph_rebase/cherry_pick.rs b/crates/but-rebase/tests/rebase/graph_rebase/cherry_pick.rs
index fb7c4cdf77..b06643dda5 100644
--- a/crates/but-rebase/tests/rebase/graph_rebase/cherry_pick.rs
+++ b/crates/but-rebase/tests/rebase/graph_rebase/cherry_pick.rs
@@ -21,12 +21,7 @@ fn basic_cherry_pick_clean() -> Result<()> {
let target = repo.rev_parse_single("single-clean-commit")?.detach();
let onto = repo.rev_parse_single("single-target")?.detach();
- let result = cherry_pick(
- &repo,
- target,
- &[onto],
- PickSignMode::LegacyIfSignCommitsEnabled,
- )?;
+ let result = cherry_pick(&repo, target, &[onto], PickSignMode::IfSignCommitsEnabled)?;
insta::assert_debug_snapshot!(result, @"
Commit(
@@ -57,12 +52,7 @@ fn basic_cherry_pick_cp_conflicts() -> Result<()> {
let target = repo.rev_parse_single("single-conflicting-commit")?.detach();
let onto = repo.rev_parse_single("single-target")?.detach();
- let result = cherry_pick(
- &repo,
- target,
- &[onto],
- PickSignMode::LegacyIfSignCommitsEnabled,
- )?;
+ let result = cherry_pick(&repo, target, &[onto], PickSignMode::IfSignCommitsEnabled)?;
insta::assert_debug_snapshot!(result, @"
ConflictedCommit(
@@ -110,7 +100,7 @@ fn basic_cherry_pick_identity() -> Result<()> {
&repo,
target.detach(),
&parents,
- PickSignMode::LegacyIfSignCommitsEnabled,
+ PickSignMode::IfSignCommitsEnabled,
)?;
insta::assert_debug_snapshot!(result, @"
@@ -134,7 +124,7 @@ fn single_parent_to_multiple_parents_clean() -> Result<()> {
&repo,
target,
&[onto, onto2],
- PickSignMode::LegacyIfSignCommitsEnabled,
+ PickSignMode::IfSignCommitsEnabled,
)?;
insta::assert_debug_snapshot!(result, @"
@@ -173,7 +163,7 @@ fn single_parent_to_multiple_parents_cp_conflicts() -> Result<()> {
&repo,
target,
&[onto, onto2],
- PickSignMode::LegacyIfSignCommitsEnabled,
+ PickSignMode::IfSignCommitsEnabled,
)?;
insta::assert_debug_snapshot!(result, @"
@@ -228,7 +218,7 @@ fn single_parent_to_multiple_parents_parents_conflict() -> Result<()> {
&repo,
target,
&[onto, onto2],
- PickSignMode::LegacyIfSignCommitsEnabled,
+ PickSignMode::IfSignCommitsEnabled,
)?;
insta::assert_debug_snapshot!(result, @"
@@ -256,12 +246,7 @@ fn multiple_parents_to_single_parent_clean() -> Result<()> {
let target = repo.rev_parse_single("merge-clean-commit")?.detach();
let onto = repo.rev_parse_single("single-target")?.detach();
- let result = cherry_pick(
- &repo,
- target,
- &[onto],
- PickSignMode::LegacyIfSignCommitsEnabled,
- )?;
+ let result = cherry_pick(&repo, target, &[onto], PickSignMode::IfSignCommitsEnabled)?;
insta::assert_debug_snapshot!(result, @"
Commit(
@@ -293,12 +278,7 @@ fn multiple_parents_to_single_parent_cp_conflicts() -> Result<()> {
let target = repo.rev_parse_single("merge-conflicting-commit")?.detach();
let onto = repo.rev_parse_single("single-target")?.detach();
- let result = cherry_pick(
- &repo,
- target,
- &[onto],
- PickSignMode::LegacyIfSignCommitsEnabled,
- )?;
+ let result = cherry_pick(&repo, target, &[onto], PickSignMode::IfSignCommitsEnabled)?;
insta::assert_debug_snapshot!(result, @"
ConflictedCommit(
@@ -348,12 +328,7 @@ fn multiple_parents_to_single_parent_parents_conflict() -> Result<()> {
.detach();
let onto = repo.rev_parse_single("single-target")?.detach();
- let result = cherry_pick(
- &repo,
- target,
- &[onto],
- PickSignMode::LegacyIfSignCommitsEnabled,
- )?;
+ let result = cherry_pick(&repo, target, &[onto], PickSignMode::IfSignCommitsEnabled)?;
insta::assert_debug_snapshot!(result, @"
FailedToMergeBases {
@@ -385,7 +360,7 @@ fn multiple_parents_to_multiple_parents_clean() -> Result<()> {
&repo,
target,
&[onto, onto2],
- PickSignMode::LegacyIfSignCommitsEnabled,
+ PickSignMode::IfSignCommitsEnabled,
)?;
insta::assert_debug_snapshot!(result, @"
@@ -424,7 +399,7 @@ fn multiple_parents_to_multiple_parents_cp_conflicts() -> Result<()> {
&repo,
target,
&[onto, onto2],
- PickSignMode::LegacyIfSignCommitsEnabled,
+ PickSignMode::IfSignCommitsEnabled,
)?;
insta::assert_debug_snapshot!(result, @"
@@ -483,7 +458,7 @@ fn multiple_parents_to_multiple_parents_base_parents_conflict() -> Result<()> {
&repo,
target,
&[onto, onto2],
- PickSignMode::LegacyIfSignCommitsEnabled,
+ PickSignMode::IfSignCommitsEnabled,
)?;
insta::assert_debug_snapshot!(result, @"
@@ -515,7 +490,7 @@ fn multiple_parents_to_multiple_parents_target_parents_conflict() -> Result<()>
&repo,
target,
&[onto, onto2],
- PickSignMode::LegacyIfSignCommitsEnabled,
+ PickSignMode::IfSignCommitsEnabled,
)?;
insta::assert_debug_snapshot!(result, @"
@@ -547,7 +522,7 @@ fn multiple_parents_to_multiple_parents_identity() -> Result<()> {
&repo,
target.detach(),
&parents,
- PickSignMode::LegacyIfSignCommitsEnabled,
+ PickSignMode::IfSignCommitsEnabled,
)?;
insta::assert_debug_snapshot!(result, @"
@@ -570,7 +545,7 @@ fn no_parents_identity() -> Result<()> {
&repo,
target.detach(),
&[],
- PickSignMode::LegacyIfSignCommitsEnabled,
+ PickSignMode::IfSignCommitsEnabled,
)?;
insta::assert_debug_snapshot!(result, @"
@@ -589,7 +564,7 @@ fn single_parent_to_no_parents_clean() -> Result<()> {
let target = repo.rev_parse_single("single-clean-commit")?.detach();
- let result = cherry_pick(&repo, target, &[], PickSignMode::LegacyIfSignCommitsEnabled)?;
+ let result = cherry_pick(&repo, target, &[], PickSignMode::IfSignCommitsEnabled)?;
insta::assert_debug_snapshot!(result, @"
Commit(
@@ -619,12 +594,7 @@ fn no_parents_to_single_parent_clean() -> Result<()> {
let target = repo.rev_parse_single("base")?.detach();
let onto = repo.rev_parse_single("single-target")?.detach();
- let result = cherry_pick(
- &repo,
- target,
- &[onto],
- PickSignMode::LegacyIfSignCommitsEnabled,
- )?;
+ let result = cherry_pick(&repo, target, &[onto], PickSignMode::IfSignCommitsEnabled)?;
insta::assert_debug_snapshot!(result, @"
Commit(
@@ -655,12 +625,7 @@ fn no_parents_to_single_parent_cp_conflicts() -> Result<()> {
let target = repo.rev_parse_single("base-conflicting")?.detach();
let onto = repo.rev_parse_single("single-target")?.detach();
- let result = cherry_pick(
- &repo,
- target,
- &[onto],
- PickSignMode::LegacyIfSignCommitsEnabled,
- )?;
+ let result = cherry_pick(&repo, target, &[onto], PickSignMode::IfSignCommitsEnabled)?;
insta::assert_debug_snapshot!(result, @"
ConflictedCommit(
@@ -708,7 +673,7 @@ fn cherry_pick_back_to_original_parents_unconflicts() -> Result<()> {
&repo,
target.detach(),
&[onto, onto2],
- PickSignMode::LegacyIfSignCommitsEnabled,
+ PickSignMode::IfSignCommitsEnabled,
)?;
insta::assert_debug_snapshot!(result, @"
@@ -723,12 +688,7 @@ fn cherry_pick_back_to_original_parents_unconflicts() -> Result<()> {
assert_eq!(&get_parents(&id.attach(&repo))?, &[onto, onto2]);
- let result = cherry_pick(
- &repo,
- id,
- &parents,
- PickSignMode::LegacyIfSignCommitsEnabled,
- )?;
+ let result = cherry_pick(&repo, id, &parents, PickSignMode::IfSignCommitsEnabled)?;
insta::assert_debug_snapshot!(result, @"
Commit(
@@ -777,7 +737,7 @@ fn cherry_pick_recursive_merge() -> Result<()> {
&repo,
target.detach(),
&[onto, onto2, onto3],
- PickSignMode::LegacyIfSignCommitsEnabled,
+ PickSignMode::IfSignCommitsEnabled,
)?;
insta::assert_debug_snapshot!(result, @"
diff --git a/crates/but-workspace/src/commit/commits_list_unsigned.rs b/crates/but-workspace/src/commit/commits_list_unsigned.rs
deleted file mode 100644
index 2e1d418f84..0000000000
--- a/crates/but-workspace/src/commit/commits_list_unsigned.rs
+++ /dev/null
@@ -1,49 +0,0 @@
-//! An action to list unsigned commit IDs in the workspace.
-
-pub(crate) mod function {
- use anyhow::Result;
- use but_core::ref_metadata::StackId;
- use but_graph::projection::Workspace;
- use gix::{ObjectId, hashtable::HashSet};
-
- /// The result of listing unsigned commits in the workspace.
- #[derive(Debug)]
- pub struct CommitsListUnsignedOutcome {
- /// IDs of all unsigned commits in the workspace
- pub unsigned_commits: HashSet<ObjectId>,
- }
-
- /// List unsigned commits in the workspace.
- ///
- /// `ws` - the workspace to operate in.
- ///
- /// `repo` - associated repository so signature information can be extracted for commits.
- ///
- /// `stack_id` - if provided, only commits for this stack are fetched.
- pub fn commits_list_unsigned(
- ws: &Workspace,
- repo: &gix::Repository,
- stack_id: Option<StackId>,
- ) -> Result<CommitsListUnsignedOutcome> {
- let commits = ws
- .stacks
- .iter()
- .filter(|stack| stack_id.is_none() || stack_id == stack.id)
- .flat_map(|stack| stack.segments.iter())
- .flat_map(|segment| segment.commits.iter());
-
- let mut unsigned_commits = HashSet::default();
- for commit in commits {
- if commit
- .attach(repo)?
- .extra_headers()
- .pgp_signature()
- .is_none()
- {
- unsigned_commits.insert(commit.id);
- }
- }
-
- Ok(CommitsListUnsignedOutcome { unsigned_commits })
- }
-}
diff --git a/crates/but-workspace/src/commit/mod.rs b/crates/but-workspace/src/commit/mod.rs
index 98c1cca15d..5e8d2535b1 100644
--- a/crates/but-workspace/src/commit/mod.rs
+++ b/crates/but-workspace/src/commit/mod.rs
@@ -12,8 +12,6 @@ pub mod commit_amend;
pub use commit_amend::function::{CommitAmendOutcome, commit_amend};
pub mod commits_sign;
pub use commits_sign::function::{CommitsSignOutcome, commits_sign};
-pub mod commits_list_unsigned;
-pub use commits_list_unsigned::function::{CommitsListUnsignedOutcome, commits_list_unsigned};
pub mod insert_blank_commit;
pub use insert_blank_commit::function::insert_blank_commit;
pub mod move_changes;
diff --git a/crates/but-workspace/src/commit_engine/mod.rs b/crates/but-workspace/src/commit_engine/mod.rs
index 86d3313ec2..e76650f4f9 100644
--- a/crates/but-workspace/src/commit_engine/mod.rs
+++ b/crates/but-workspace/src/commit_engine/mod.rs
@@ -195,7 +195,7 @@ pub fn create_commit(
repo,
commit.inner,
DateMode::CommitterUpdateAuthorKeep,
- SignCommit::LegacyIfSignCommitsEnabled,
+ SignCommit::IfSignCommitsEnabled,
)?)
}
}
@@ -237,6 +237,6 @@ fn create_possibly_signed_commit(
repo,
commit,
DateMode::CommitterKeepAuthorKeep,
- SignCommit::LegacyIfSignCommitsEnabled,
+ SignCommit::IfSignCommitsEnabled,
)
}
diff --git a/crates/but-workspace/tests/fixtures/scenario/ws-with-mixed-signed-unsigned.sh b/crates/but-workspace/tests/fixtures/scenario/ws-with-mixed-signed-unsigned.sh
index 4bcf2ff25b..d852124b9b 100644
--- a/crates/but-workspace/tests/fixtures/scenario/ws-with-mixed-signed-unsigned.sh
+++ b/crates/but-workspace/tests/fixtures/scenario/ws-with-mixed-signed-unsigned.sh
@@ -6,7 +6,7 @@
# Stack unsigned-*: A two-branch stack with all commits unsigned.
# Stack mixed: A two-branch stack with a mix of signed and unsigned commits.
#
-# This is useful for testing commits_list_unsigned and commits_sign.
+# This is useful for testing commits_sign.
set -eu -o pipefail
diff --git a/crates/but-workspace/tests/workspace/commit/commits_list_unsigned.rs b/crates/but-workspace/tests/workspace/commit/commits_list_unsigned.rs
deleted file mode 100644
index dd87eaa5c2..0000000000
--- a/crates/but-workspace/tests/workspace/commit/commits_list_unsigned.rs
+++ /dev/null
@@ -1,116 +0,0 @@
-use anyhow::Result;
-use but_workspace::commit::commits_list_unsigned;
-
-use crate::ref_info::with_workspace_commit::utils::{
- StackState, add_stack_with_segments, named_writable_scenario_with_description_and_graph,
-};
-
-#[test]
-fn filter_by_stack_id() -> Result<()> {
- let (_tmp, graph, repo, _meta, _description) =
- named_writable_scenario_with_description_and_graph(
- "ws-with-mixed-signed-unsigned",
- |meta| {
- add_stack_with_segments(
- meta,
- 1,
- "unsigned-top",
- StackState::InWorkspace,
- &["unsigned-bottom"],
- );
- add_stack_with_segments(
- meta,
- 2,
- "mixed-top",
- StackState::InWorkspace,
- &["mixed-bottom"],
- );
- },
- )?;
-
- let unsigned_stack_id = gitbutler_stack::StackId::from_number_for_testing(1);
- let mixed_stack_id = gitbutler_stack::StackId::from_number_for_testing(2);
-
- let ws = graph.into_workspace()?;
- let outcome_unsigned_stack = commits_list_unsigned(&ws, &repo, Some(unsigned_stack_id))?;
- let outcome_mixed_stack = commits_list_unsigned(&ws, &repo, Some(mixed_stack_id))?;
-
- assert_eq!(
- outcome_unsigned_stack.unsigned_commits.len(),
- 2,
- "expected 2 unsigned commits for unsigned stack, got {}: {:?}",
- outcome_unsigned_stack.unsigned_commits.len(),
- outcome_unsigned_stack.unsigned_commits,
- );
-
- assert_eq!(
- outcome_mixed_stack.unsigned_commits.len(),
- 1,
- "expected 1 unsigned commits for mixed stack, got {}: {:?}",
- outcome_mixed_stack.unsigned_commits.len(),
- outcome_mixed_stack.unsigned_commits,
- );
-
- Ok(())
-}
-
-#[test]
-fn all_unsigned_commits_are_listed() -> Result<()> {
- let (_tmp, graph, repo, _meta, _description) =
- named_writable_scenario_with_description_and_graph(
- "ws-with-mixed-signed-unsigned",
- |meta| {
- add_stack_with_segments(
- meta,
- 1,
- "unsigned-top",
- StackState::InWorkspace,
- &["unsigned-bottom"],
- );
- add_stack_with_segments(
- meta,
- 2,
- "mixed-top",
- StackState::InWorkspace,
- &["mixed-bottom"],
- );
- },
- )?;
-
- let ws = graph.into_workspace()?;
- let outcome = commits_list_unsigned(&ws, &repo, None)?;
-
- assert_eq!(
- outcome.unsigned_commits.len(),
- 3,
- "expected 3 unsigned commits (signed mixed excluded), got {}: {:?}",
- outcome.unsigned_commits.len(),
- outcome.unsigned_commits
- );
-
- let unsigned_bottom = repo.rev_parse_single("unsigned-bottom")?.detach();
- let unsigned_top = repo.rev_parse_single("unsigned-top")?.detach();
- let mixed_bottom = repo.rev_parse_single("mixed-bottom")?.detach();
- assert!(
- outcome.unsigned_commits.contains(&unsigned_bottom),
- "unsigned-bottom commit should be unsigned"
- );
- assert!(
- outcome.unsigned_commits.contains(&unsigned_top),
- "unsigned-top commit should be unsigned"
- );
- assert!(
- outcome.unsigned_commits.contains(&mixed_bottom),
- "mixed bottom commit should be unsigned",
- );
-
- // just to be on the safe side, we'll also explicitly check that the signed top-commit of the
- // mixed stack is _not_ in the result set.
- let mixed_top = repo.rev_parse_single("mixed-top")?.detach();
- assert!(
- !outcome.unsigned_commits.contains(&mixed_top),
- "signed mixed-top commit should not be in the unsigned set"
- );
-
- Ok(())
-}
diff --git a/crates/but-workspace/tests/workspace/commit/commits_sign.rs b/crates/but-workspace/tests/workspace/commit/commits_sign.rs
index 10ca56e576..77b0385701 100644
--- a/crates/but-workspace/tests/workspace/commit/commits_sign.rs
+++ b/crates/but-workspace/tests/workspace/commit/commits_sign.rs
@@ -1,6 +1,6 @@
use anyhow::Result;
use but_rebase::graph_rebase::{Editor, GraphEditorOptions, cherry_pick::PickSignMode};
-use but_workspace::commit::{commits_list_unsigned, commits_sign};
+use but_workspace::commit::commits_sign;
use crate::ref_info::with_workspace_commit::utils::{
StackState, add_stack_with_segments,
@@ -44,9 +44,9 @@ fn sign_all_unsigned_commits_in_workspace() -> Result<()> {
})?;
let mut ws = graph.into_workspace()?;
-
- let unsigned = commits_list_unsigned(&ws, &repo, None)?;
- assert_eq!(unsigned.unsigned_commits.len(), 3);
+ let unsigned_bottom = repo.rev_parse_single("unsigned-bottom")?.detach();
+ let unsigned_top = repo.rev_parse_single("unsigned-top")?.detach();
+ let mixed_bottom = repo.rev_parse_single("mixed-bottom")?.detach();
let editor = Editor::create_with_opts(
&mut ws,
@@ -56,7 +56,7 @@ fn sign_all_unsigned_commits_in_workspace() -> Result<()> {
sign_mode: PickSignMode::IfChanged,
},
)?;
- let outcome = commits_sign(editor, unsigned.unsigned_commits)?;
+ let outcome = commits_sign(editor, [unsigned_bottom, unsigned_top, mixed_bottom])?;
let materialize_outcome = outcome.rebase.materialize()?;
let workspace_commit_id = repo.head_id()?.detach();
@@ -72,16 +72,8 @@ fn sign_all_unsigned_commits_in_workspace() -> Result<()> {
.map(|new_id| commit_is_signed(&repo, *new_id) as i32)
.sum();
- let num_unsigned_commits_in_ws = commits_list_unsigned(&ws, &repo, None)?
- .unsigned_commits
- .len();
-
assert!(!commit_is_signed(&repo, workspace_commit_id));
assert_eq!(num_signed_commits, 4, "expected all 4 commits to be signed");
- assert_eq!(
- num_unsigned_commits_in_ws, 0,
- "expected no unsigned commits to be left in the workspace"
- );
Ok(())
}
@@ -116,9 +108,6 @@ fn sign_entire_stack_by_signing_only_oldest_unsigned_commit() -> Result<()> {
&["mixed-bottom"],
);
})?;
- let unsigned_stack_id = gitbutler_stack::StackId::from_number_for_testing(1);
- let mixed_stack_id = gitbutler_stack::StackId::from_number_for_testing(2);
-
let mut ws = graph.into_workspace()?;
let unsigned_bottom = repo.rev_parse_single("unsigned-bottom")?.detach();
@@ -146,25 +135,14 @@ fn sign_entire_stack_by_signing_only_oldest_unsigned_commit() -> Result<()> {
.values()
.map(|new_id| commit_is_signed(&repo, *new_id) as i32)
.sum();
- let num_unsigned_commits_in_unsigned_stack =
- commits_list_unsigned(&ws, &repo, Some(unsigned_stack_id))?
- .unsigned_commits
- .len();
- let num_unsigned_commits_in_mixed_stack =
- commits_list_unsigned(&ws, &repo, Some(mixed_stack_id))?
- .unsigned_commits
- .len();
+ let mixed_bottom = repo.rev_parse_single("mixed-bottom")?.detach();
assert_eq!(
num_signed_commits, 2,
"expected all 2 commits in the unsigned stack to be signed"
);
- assert_eq!(
- num_unsigned_commits_in_unsigned_stack, 0,
- "expected no unsigned commits in unsigned stack"
- );
- assert_eq!(
- num_unsigned_commits_in_mixed_stack, 1,
+ assert!(
+ !commit_is_signed(&repo, mixed_bottom),
"expected to still have unsigned commit in mixed stack as we were not signing it",
);
diff --git a/crates/but-workspace/tests/workspace/commit/mod.rs b/crates/but-workspace/tests/workspace/commit/mod.rs
index 752fe2eaf0..505d172399 100644
--- a/crates/but-workspace/tests/workspace/commit/mod.rs
+++ b/crates/but-workspace/tests/workspace/commit/mod.rs
@@ -1,6 +1,5 @@
mod commit_amend;
mod commit_create;
-mod commits_list_unsigned;
mod commits_sign;
mod insert_blank_commit;
mod move_changes;
diff --git a/crates/gitbutler-edit-mode/src/lib.rs b/crates/gitbutler-edit-mode/src/lib.rs
index 3d3cf2064a..4c242a072c 100644
--- a/crates/gitbutler-edit-mode/src/lib.rs
+++ b/crates/gitbutler-edit-mode/src/lib.rs
@@ -329,7 +329,7 @@ pub(crate) fn save_and_return_to_workspace(ctx: &Context, perm: &mut RepoExclusi
..commit.inner
},
but_rebase::commit::DateMode::CommitterUpdateAuthorKeep,
- SignCommit::LegacyIfSignCommitsEnabled,
+ SignCommit::IfSignCommitsEnabled,
)?;
editor.replace(target_selector, Step::new_pick(new_commit_oid))?;
diff --git a/crates/gitbutler-repo/src/repository_ext.rs b/crates/gitbutler-repo/src/repository_ext.rs
index e6c8d5c540..1d069dd98b 100644
--- a/crates/gitbutler-repo/src/repository_ext.rs
+++ b/crates/gitbutler-repo/src/repository_ext.rs
@@ -115,7 +115,7 @@ pub fn commit_with_signature_gix(
tree,
parents,
commit_headers,
- SignCommit::LegacyIfSignCommitsEnabled,
+ SignCommit::IfSignCommitsEnabled,
)
}Takeaway
This PR effectively deprecates the special gitbutler.signCommits flag, and I am torn. While on the one hand it shouldn't be needed because Git knows when to sign and we just pick that up, on the other hand the GUI doesn't always pickup all the configuration that's needed to do that correctly.
The flag was introduced to have a way to say: Do not sign in GitButler because it doesn't work, and we set that flag per repository to prevent GitButler from failing permanently when trying to create commits. So after a failed signing attempt, we set the flag, surface a descriptive error via .context(Code::CommitSigningFailed));, and the user can try again.
For that reason, I don't think we can remove that flag or even call it legacy without a better way to solve it.
commit_list_unsigned
I don't know what the plan is for it or how it will be used.
My thinking here is that instead of adding a new special method, rather extract that bit of information directly in RefInfo. It already parses the commits and it's trivial to add while universally useful In future, we might want to extend this to show more about the signature as well, and this function here just doesn't fit.
If it is supposed to be used as input for commits_sign I think it's not suitable to get that list using the workspace projection. What has to be done is to get it through the graph so no commits are missed.
I feel so strongly about this as I removed that function entirely 😅.
Edit: After making a round, I realize this was probably the meant to given an initial selection of commits which should be signed, and the rebase engine will take care of signing the right affected commits according to its rules. Nonetheless, I think it's fine to bring that back once it's clear how it's used and we don't have to make a big deal out of a commit traversal, while seeing where this functionality fits in to make it not so special anymore. My intuition is that the rebase engine can help with that more.
Thoughts on PickSignMode::LegacyIfSignCommitsEnabled
Interestingly, if a smarter signing logic is supposed to be implemented, I also think that but-rebase will need to play along. However, that logic would also have to respect Git signing settings or it's clearly something very special that deserves its own mode. For the same reasons as at the beginning of this text, we can't remove it the IfSignCommitsEnabled mode.
Also, it's great to spell out these modes, so that alone would be something to keep and merge right away.
Auto-review: but-rebase
I asked my agent to check for more and it came up with these reasonable sounding points about but-rebase. Posting here in case it's helpful, while thinking that @Caleb-T-Owens should thoroughly review this as well.
Agent review with one high-prio item
I’m converting the review notes to GitHub-ready markdown with stable blob links pinned to the current HEAD, so the line references won’t drift when you paste them into a PR or issue.
Pinned to commit a290f4c1df51a1e3241f5283f72595610374c15c.
-
High:
commits_sign()replaces the entirePicknode instead of mutating onlysign_mode. That resetspreserved_parents,conflictable,parents_must_be_references, andexclude_from_trackingback to the defaults fromPick::new_pick()/Step::new_pick_with_sign_mode()andStep::new_pick_with_sign_mode(). That is behavior-changing, not cosmetic: partial traversals explicitly preserve the real parents increation.rs, and rebase execution consumespreserved_parentsinrebase.rs. As written, signing one of those commits can silently rewrite it onto graph-derived parents instead of its original preserved parents. -
Medium:
PickSignMode::Forceis documented as signing even otherwise-noop picks, and the first identity fast-path was updated accordingly incherry_pick(). But the later(MergeOutcome::NoCommit, MergeOutcome::NoCommit)arm still returnsIdentityunconditionally here. That means a parentless commit selected throughcommits_sign()still cannot be force-signed. -
Medium:
commits_sign()only cascades signatures if the caller constructs the editor withPickSignMode::IfChanged. The default editor path still seeds picks withPickSignMode::IfSignCommitsEnabled, whilecommits_sign()itself only force-signs the explicitly selected commits. So if a caller usesEditor::create(...)and repo signing is disabled, descendants rewritten because their parent changed will still be recreated unsigned. The current tests avoid that path by explicitly usingEditor::create_with_opts(... PickSignMode::IfChanged ...).
Wow, I clock in at 115 minutes for this review, and am sorry that I don't think it should be merged in its current form.
|
@Byron Thanks so much for the detailed review! I think this review suffered a fate worse than big PR - it suffered big PR hat still didn't have enough context to the changes. I thought I made a sensible split in the stack to make this make sense standalone but I think I clearly failed (more on that below).
I'll try to make that split make sense.
I will fight tooth and nail to deprecate the The key takeaway I had in designing this was that we can't have a boolean for signing - sometimes we want the rebase engine to sign, and sometimes we don't want it to sign. For example, with sign-on-push, we want to sign only if we're just about to push. At all other times, doing all the same things, we don't want to sign. This is not something that we can figure out at the point of creating the commit, we need to resolve that state way further up the stack and make the sign decisions up there.
This is where I think I excluded important information and should've just had the entire big-ass prototype be the PR. This is not to be removed outright, but replaced. The replacement for the boolean This PR does nothing with settings, it only preemptively names the legacy mode legacy, because I truly don't think "sign all the time if we've set this thing in the config" is useful. But I realize the context for that was completely lost in this PR, and only @Caleb-T-Owens had that from our discussion on how this should work. The outright deprecation of
It's just to get unsigned commits as input for
I don't really understand what you mean by this. Do you mean to say that it's an important property of the rebase engine that it, mid rebase, inspects signing settings in Git? I don't agree on that design decision, I think it makes more sense to inspect the settings before starting the rebase, make the decisions about how to sign when building the editor, and then do the rebase with those settings. It might blow up if someone changes the settings at the very same time as the rebase is ongoing, but that race condition exists regardless of where you put the "read" of the settings. Is I noted above, it's very inconvenient to decide on signing at the point of creating the commit. It makes sign-on-push kind of functionality impossible to implement. The decision of whether a commit is eligible for signing must be made much earlier.
I'm sorry for taking up so much of your time 😓. It was pretty tough figuring out where to put what to realize this functionality. I'll put in some work to split this up further and then we can take it from there! |
|
Just as a tiny reply: instead of writing a huge reply here we had a call and discovered interesting and previously unknown aspects of this topic that will change its trajectory, and to the better we think. Big picture stuff I took away from our meeting:
|
|
Had a chat with @Byron and we came to the following conclusions:
Re-stacking this to be easier to review and incorporate the above changes. |
slarse
force-pushed
the
GB-1121/sign-on-push-groundwork
branch
from
d905572 to
2a0f2ed
Compare
slarse
changed the title
slarse
force-pushed
the
GB-1121/sign-on-push-groundwork
branch
from
2a0f2ed to
647a01c
Compare
slarse
left a comment
slarse
left a comment
There was a problem hiding this comment.
@Byron Okay, so I've reworked this completely to only encompass the parts that programmatically control signing behavior internally inside of GitButler. That's really the core concept that paves the way for sign-on-push: we need to be able to control signing independently of configuration.
I think that this might still not be ready to merge, there are some parts I'm not happy with and not confident in (see comments in code). I think we should take our time, anything that goes into but-core is... well, core. And the graph engine as well.
Ironically, this is the same size as the PR was before as I went ahead and added almost 400 lines of test code to cement the behavior. This can easily be split further into one PR to introduce SignCommit, and one to introduce the PickSignMode thing, but I think that kind of split detracts from understanding the purpose of it all. Instead I split the commits.
crates/but-core/src/commit.rs
Outdated
| /// Sign the commit only if `gitbutler.signCommits=true`, *or* `gitbutler.signCommits` is unset | ||
| /// but `commit.gpgSign=true`. In other words, `gitbutler.signCommits` takes precedence over | ||
| /// `commit.gpgSign`, the latter only being checked if the former is not at all configured. | ||
| /// | ||
| /// If signing fails, `gitbutler.signCommits` is set to false locally, preventing further | ||
| /// signing when this variant is supplied. This step is however skipped if | ||
| /// `gitbutler.signCommits` has been explicitly configured in non-local Git Config. | ||
| /// | ||
| /// The need for `gitbutler.signCommits` stems from the fact that it can be difficult to | ||
| /// impossible to validate before hand that signing is properly configured. Signing may also | ||
| /// break after validation has been performed. If signing is enabled for *all* committing but | ||
| /// fails, GitButler basically can't do anything, so we flip `gitbutler.signCommits=false` as a | ||
| /// kill switch to disable signing for GitButler. `gitbutler.signCommits` taking precedence | ||
| /// over `commit.gpgSign` means we can honor Git's signing settings by default, but disable it | ||
| /// in the event that we fail to sign without affecting Git. | ||
| /// | ||
| /// This is really only necessary for the use case where GitButler acts in a Git-compatible | ||
| /// fashion to sign commits all the time. When signing more conservatively, for example just | ||
| /// before pushing, the need for this kind of kill-switching is greatly reduced, perhaps to the | ||
| /// point where we can remove it. But as long as we want to smoothly interoperate with Git, it | ||
| /// should remain. |
There was a problem hiding this comment.
Please help me double check that this makes sense. It reads right to me and I've double and triple checked it, but... It's a lot of text.
There was a problem hiding this comment.
My main modification here is to not claim we ever want to not be Git compatible.
As such, I think this kill-switch is valuable even if sign-on-push is enabled, while agreeing that it's less necessary then.
| /// This enum allows us to control when a pick is cherry-picked in a rebase. | ||
| /// | ||
| /// At present, this is here to be able to control signing behavior, but could be extended for | ||
| /// other purposes in the future. | ||
| /// | ||
| /// For a picked commit to be signed, it needs to: | ||
| /// | ||
| /// 1. Be included in the rebase s.t. it is rewritten, and potentially get signed in the process. | ||
| /// This is determined by the [`PickDecisionMode`] variants. | ||
| /// 2. Actually get signed when the pick is made into a commit. This is determined by combination | ||
| /// of configuration and the [`SignCommit`] variant, see its docs for details. | ||
| /// | ||
| /// Note that signing a parent commit only causes descendants to be signed if they are also picked | ||
| /// with appropriate [`PickDecisionMode`] and [`SignCommit`] variants. For example, in a commit chain A | ||
| /// <- B, where A is the root, picking A with `PickDecisionMode::Force(SignCommit::Yes)` and B with | ||
| /// `PickDecisionMode::IfChanged(SignCommit::No)` will cause A to be signed, but not B. However, if B | ||
| /// is picked with `PickDecisionMode::IfChanged(SignCommit::Yes)` in the same scenario, then B also | ||
| /// gets signed as its parent A has changed from the signature being applied to it. | ||
| /// | ||
| /// An intentional pattern to cause a cascade of signatures is to make all picks with | ||
| /// `PickDecisionMode::IfChanged(SignCommit::Yes)`, and then re-pick the intended root(s) of the | ||
| /// signature cascade with `PickDecisionMode::Force(SignCommit::Yes)`. | ||
| #[derive(Debug, Clone, Copy, PartialEq)] | ||
| pub enum PickDecisionMode { | ||
| /// Cherry-picks the commit only if it's necessary because of changes to the commit or its | ||
| /// parents. | ||
| /// | ||
| /// [`SignCommit`] controls the signing behavior of the pick, if it gets cherry-picked into the | ||
| /// rebase. | ||
| IfChanged(SignCommit), | ||
| /// Forces a cherry-pick on a commit. This is useful for example to sign or unsign commits. | ||
| /// | ||
| /// [`SignCommit`] controls the signing behavior of the forced pick. | ||
| Force(SignCommit), | ||
| } |
There was a problem hiding this comment.
I really struggled with getting this down the way I want it. I wasn't happy with the 4 PickSignMode variants I initially had here (Never, IfChanged, IfChangedAndSignCommitsEnabled, Force), because to the rebase engine the choice was still binary: Either the pick is forced, or it's not and we only cherry-pick it if something has changed.
What I finally realized was bugging me was that this single PickSignMode variant was controlling two things: whether the commit gets cherry-picked at all (the rebase engine's direct concern), and whether the commit gets signed (the commit machinery's concern, the rebase engine doesn't really care).
It would possibly be even cleaner to completely separate these concerns, s.t. we don't nest the SignCommit enum inside the PickDecisionMode variants. Right now I chose not to do that because I do think these are tightly related in terms of functionality.
I'm still uncertain about this design but I think this is the least bad design I've come up with, and it cleanly separates the "when to pick" and "how to commit" parts of the cherry-picking.
| DateMode::CommitterUpdateAuthorKeep, | ||
| pick_decision_mode.into(), | ||
| )?; | ||
| Ok(CherryPickOutcome::Commit(commit)) |
There was a problem hiding this comment.
Unsure about this implementation. What I want to do is just to re-write the commit that was picked with PickDecisionMode::Force, which should be the target here.
slarse
force-pushed
the
GB-1121/sign-on-push-groundwork
branch
from
defcd56 to
24d46ee
Compare
| /// | ||
| /// [`SignCommit`] controls the signing behavior of the pick, if it gets cherry-picked into the | ||
| /// rebase. | ||
| IfChanged(SignCommit), |
There was a problem hiding this comment.
Question: How do we feel about exposing parts of crate X in the API of crate Y? I was in the process of creating a but_rebase::SignCommit wrapper that maps 1-to-1 with but_core::SignCommit just to not mix-and-match crate components. But then I stopped myself.
It has the downside of a potential explosion of such logic-less adapters, but it helps keep crate boundaries clean. Thoughts?
There was a problem hiding this comment.
That's a good call!
If it's 1:1 the same type, we have been sharing them, something I believe is practical and fine among friendly crates.
The but-api crate would be more interesting in that regard, but even there we have types from multiple crates (among which is gix) mixed in.
As I don't think we we ever had a problem with that, I don't think it's an effort worth introducing now either.
| export "GIT_CONFIG_COUNT=1" | ||
| export "GIT_CONFIG_KEY_0=init.defaultBranch" | ||
| export "GIT_CONFIG_VALUE_0=main" | ||
|
|
There was a problem hiding this comment.
This fixture exports GIT_CONFIG_COUNT/KEY/VALUE after git init, so it won’t influence the initial branch name and it overrides the test harness’ isolated Git config (but-testsupport sets multiple GIT_CONFIG_* keys, including protocol.file.allow=always). Consider removing these exports (they appear redundant) or applying the desired config without clobbering the harness’ GIT_CONFIG_* environment.
| export "GIT_CONFIG_COUNT=1" | |
| export "GIT_CONFIG_KEY_0=init.defaultBranch" | |
| export "GIT_CONFIG_VALUE_0=main" |
Byron
left a comment
Byron
left a comment
There was a problem hiding this comment.
Great, I will take a closer look soon. From what I could tell at a glimpse, but-core should be fine.
| export "GIT_CONFIG_COUNT=1" | ||
| export "GIT_CONFIG_KEY_0=init.defaultBranch" | ||
| export "GIT_CONFIG_VALUE_0=main" |
There was a problem hiding this comment.
This isn't needed as the fixture is executed through gix-testtools, which isolates Git with this and more.
slarse
force-pushed
the
GB-1121/sign-on-push-groundwork
branch
from
24d46ee to
dc38712
Compare
| fn commit_picked_with_force_sign_is_signed_when_otherwise_unchanged_and_signing_config_is_not_enabled() | ||
| -> Result<()> { |
There was a problem hiding this comment.
The function signature formatting here (line break before -> Result<()>) doesn’t match rustfmt output / surrounding tests. Please run rustfmt (or adjust formatting) to keep the return type placement consistent.
| fn commit_picked_with_force_sign_is_signed_when_otherwise_unchanged_and_signing_config_is_not_enabled() | |
| -> Result<()> { | |
| fn commit_picked_with_force_sign_is_signed_when_otherwise_unchanged_and_signing_config_is_not_enabled() -> Result<()> { |
| Ok(()) | ||
| } | ||
|
|
||
| /// Picking with with [`PickDecisionMode::Force`] _should_ cause a cascade of signatures when |
There was a problem hiding this comment.
Minor typo in the test docstring: "Picking with with" has a duplicated "with".
| /// Picking with with [`PickDecisionMode::Force`] _should_ cause a cascade of signatures when | |
| /// Picking with [`PickDecisionMode::Force`] _should_ cause a cascade of signatures when |
| fn force_signing_ancestor_does_not_sign_descendants_that_are_picked_with_sign_commit_no() | ||
| -> Result<()> { |
There was a problem hiding this comment.
This function signature formatting (line break before -> Result<()>) is inconsistent with rustfmt/surrounding tests. Please run rustfmt (or adjust formatting) to keep signatures consistent.
| fn force_signing_ancestor_triggers_cascading_signatures_of_descendants_that_are_picked_with_sign_commit_yes() | ||
| -> Result<()> { |
There was a problem hiding this comment.
This function signature formatting (line break before -> Result<()>) is inconsistent with rustfmt/surrounding tests. Please run rustfmt (or adjust formatting) to keep signatures consistent.
| fn force_signing_ancestor_triggers_cascading_signatures_of_descendants_that_are_picked_with_sign_commit_yes() | |
| -> Result<()> { | |
| fn force_signing_ancestor_triggers_cascading_signatures_of_descendants_that_are_picked_with_sign_commit_yes() -> Result<()> { |
| fn commit_picked_with_ifchange_and_configuration_guard_is_not_signed_when_signing_config_is_not_enabled() | ||
| -> Result<()> { |
There was a problem hiding this comment.
This function signature formatting (line break before -> Result<()>) is inconsistent with rustfmt/surrounding tests. Please run rustfmt (or adjust formatting) to keep signatures consistent.
| fn commit_picked_with_ifchange_and_configuration_guard_is_not_signed_when_signing_config_is_not_enabled() | |
| -> Result<()> { | |
| fn commit_picked_with_ifchange_and_configuration_guard_is_not_signed_when_signing_config_is_not_enabled() -> Result<()> { |
|
@Caleb-T-Owens The Let me know if you want to have a call about it. |
There was a problem hiding this comment.
Thanks a lot, now the parts I looked at, everything but but-rebase look good to me!
If you ask me, they could be merged first.
Maybe restructure this PR to make that possible and the but-rebase review can then stand on its own.
PS: I pushed directly in here because it seemed the PR isn't stacked anymore. But maybe that's wrong. Sorry for that.
Byron
force-pushed
the
GB-1121/sign-on-push-groundwork
branch
from
26200b3 to
3cdc404
Compare
Sounds good, I'll split out everything
This was a correct assessment, so that's fine! |
slarse
force-pushed
the
GB-1121/sign-on-push-groundwork
branch
from
3cdc404 to
cdf1046
Compare
slarse
changed the title
This introduces a SignCommit enum to control how the core commit creation handles signing. It allows a caller to override the Git configuration and decide to sign independently of the settings `gitbutler.signCommits` and `commit.gpgSign`, allowing for greater flexibility in signing behavior. This will be used in upcoming changes to the graph rebase engine to allow for sign-on-push functionality.
slarse
force-pushed
the
GB-1121/sign-on-push-groundwork
branch
from
cdf1046 to
3dfc22e
Compare
slarse
changed the title
This PR introduces programmatic overrides to signing behavior for the core commit flow. This will later be used to allow the graph rebase engine to decide when to sign programmatically to be able to implement sign-on-push where we never sign for "local" operations.
but-rebasechanges coming up in a separate PR.Old description kept for historical reasons
This is big, but I couldn't find any super reasonable way to cut it down any more than I've done with this stack. It turns out that overhauling how we sign commits touches a lot of things :)This PR puts down the groundwork for enabling sign-on-push functionality. Importantly, it contributes but-workspace abstractions to list unsigned commits and forcibly sign commits, as well as the underlying graph rebase engine work to actually accomplish the signing.
Note that there is still full backwards compatibility with the current sign-all-the-flippin-time functionality, and this PR alone should result in zero changes to end users as the new functionality isn't used anywhere.
See the tests in
crates/but-workspace/tests/workspace/commit/commits_list_unsigned.rsandcrates/but-workspace/tests/workspace/commit/commits_sign.rsfor details about the new behavior.Integration into the
butCLI is done in a separate PR. I feel that the UX there still needs some polish, but I really need to land this PR as I've now had to fix two sets of merge conflicts just today :sThis is part 1 of 2 in a stack made with GitButler: