Skip to content

cli: warn when sign-on-push skips immutable commits #9963

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
baptiste0928 wants to merge 1 commit into
base: main
Choose a base branch
from
+41 −10
Open

cli: warn when sign-on-push skips immutable commits #9963

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 39 additions & 10 deletions cli/src/commands/git/push.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ use crate::cli_util::WorkspaceCommandHelper;
use crate::cli_util::WorkspaceCommandTransaction;
use crate::cli_util::has_tracked_remote_bookmarks;
use crate::cli_util::has_tracked_remote_tags;
use crate::cli_util::print_updated_commits;
use crate::cli_util::short_change_hash;
use crate::cli_util::short_commit_hash;
use crate::command_error::CommandError;
Expand Down Expand Up @@ -795,13 +796,13 @@ fn ready_to_push_revset_expression(
.flat_map(|(_, old_head)| old_head.target.added_ids())
.cloned()
.collect_vec();
RevsetExpression::commits(old_heads)
.union(workspace_helper.env().immutable_heads_expression())
.range(&RevsetExpression::commits(new_heads))
RevsetExpression::commits(old_heads).range(&RevsetExpression::commits(new_heads))
}

/// Signs commits before pushing.
///
/// Warns about commits that need a signature but are immutable.
///
/// Returns the updated list of bookmark names and corresponding
/// [`BookmarkPushUpdate`]s.
async fn sign_commits_before_push(
Expand All @@ -812,14 +813,42 @@ async fn sign_commits_before_push(
) -> Result<GitPushRefTargets, CommandError> {
let mut sign_settings = tx.settings().sign_settings();
sign_settings.behavior = SignBehavior::Own;
let commit_ids: IndexSet<CommitId> = tx
.base_workspace_helper()
.attach_revset_evaluator(commits_to_push)
// TODO: make filter condition configurable by revset?
let needs_signing = |commit: &Commit| {
future::ready(!commit.is_signed() && sign_settings.should_sign(commit.store_commit()))
};

let workspace_helper = tx.base_workspace_helper();
let immutable = workspace_helper.env().immutable_expression();
let skipped: Vec<Commit> = workspace_helper
.attach_revset_evaluator(commits_to_push.intersection(&immutable))
.evaluate_to_commits()?
// TODO: make filter condition configurable by revset?
.try_filter(|commit| {
future::ready(!commit.is_signed() && sign_settings.should_sign(commit.store_commit()))
})
.try_filter(needs_signing)
Comment thread
PhilipMetzger marked this conversation as resolved.
.take(11)
.try_collect()

@yuja yuja Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you restrict the number of commits to collect? There may be tons of unsigned immutable commits.

It seems also better to convert needs_signing to a revset expression, but that's a separate issue.

@baptiste0928 baptiste0928 Aug 11, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you restrict the number of commits to collect? There may be tons of unsigned immutable commits.

Done, I restricted to 11 to match the limit of print_updated_commits (it shows up to 10 commits then print ... ), and the warning message itself will print Skipped signing 10+ immutable commits.

It seems also better to convert needs_signing to a revset expression, but that's a separate issue.

I agree, I can do that in another commit in this PR (or a separate PR) if you want

.await?;
if !skipped.is_empty() {
let count = if skipped.len() > 10 {
"10+".to_owned()
} else {
skipped.len().to_string()
};
writeln!(
ui.warning_default(),
"Skipped signing {count} immutable commits:"
)?;
let mut formatter = ui.stderr_formatter();
print_updated_commits(
formatter.as_mut(),
&workspace_helper.commit_summary_template(),
&skipped,
)?;

@josephlou5 josephlou5 Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally I feel like the skipped commits should also be printed, if possible.

@baptiste0928 baptiste0928 Aug 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, I used the existing print_updated_commits helper (it doesn't quite match the rest of the command output style, but imo it's okay since it won't be shown often):

Image

}

let commit_ids: IndexSet<CommitId> = workspace_helper
.attach_revset_evaluator(commits_to_push.minus(&immutable))
.evaluate_to_commits()?
.try_filter(needs_signing)
.map_ok(|commit| commit.id().clone())
.try_collect()
.await?;
Expand Down
2 changes: 2 additions & 0 deletions cli/tests/test_git_push.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2728,6 +2728,8 @@ fn test_git_push_sign_on_push() {
let output = work_dir.run_jj(["git", "push"]);
insta::assert_snapshot!(output, @"
------- stderr -------
Warning: Skipped signing 1 immutable commits:
kpqxywon 48ea83e9 bookmark2* | (empty) commit which should not be signed 1
Changes to push to origin:
bookmark: bookmark2 [move forward from d45e2adce0ad to 48ea83e9499c]
[EOF]
Expand Down
Loading