Skip to content

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

Open
baptiste0928 wants to merge 1 commit into
jj-vcs:mainfrom
baptiste0928:baptiste/push-tznwrsylspsk
Open

cli: warn when sign-on-push skips immutable commits#9963
baptiste0928 wants to merge 1 commit into
jj-vcs:mainfrom
baptiste0928:baptiste/push-tznwrsylspsk

Conversation

@baptiste0928

@baptiste0928 baptiste0928 commented Aug 10, 2026

Copy link
Copy Markdown
Member

Small QoL improvement to warn when a commit that should have been signed is skipped during jj git push (when using sign-on-push). This can easily happen when tagging a commit while it is unsigned, then pushing it (since the tag makes the commit immutable).

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added/updated tests to cover my changes
  • I fully understand the code that I am submitting (what it does,
    how it works, how it's organized), including any code drafted by an LLM.
  • For any prose generated by an LLM, I have proof-read and copy-edited with
    an eye towards deleting anything that is irrelevant, clarifying anything
    that is confusing, and adding details that are relevant. This includes,
    for example, commit descriptions, PR descriptions, and code comments.

@baptiste0928
baptiste0928 requested a review from a team as a code owner August 10, 2026 19:20
writeln!(
ui.warning_default(),
"Skipped signing {num_immutable} immutable commits."
)?;

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

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

@baptiste0928
baptiste0928 force-pushed the baptiste/push-tznwrsylspsk branch from 3358f35 to e9d574c Compare August 11, 2026 08:17
future::ready(!commit.is_signed() && sign_settings.should_sign(commit.store_commit()))
})
.try_filter(needs_signing)
.try_collect()

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.

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

Comment thread CHANGELOG.md Outdated
Comment thread cli/src/commands/git/push.rs
Commits that need a signature but are immutable cannot be rewritten to
add one, so they were silently pushed unsigned. This is easy to hit
when, for example, tagging unpushed commits.

`jj git push` now warns if any commit that should have been signed was
skipped because it is immutable.
@baptiste0928
baptiste0928 force-pushed the baptiste/push-tznwrsylspsk branch from e9d574c to 69154d5 Compare August 11, 2026 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants