Re: [PATCH v4 3/3] fast-import: add mode to sign commits with invalid signatures
- From
Justin Tobler <jltobler@gmail.com>
- Date
- Mar 12, 2026, 14:08 UTC
- Message-ID
- <abLGgq-PXzdWs6kD@denethor>
- In-Reply-To
- <abKUBRRgRmbJ1hRA@pks.im>
On 26/03/12 11:23AM, Patrick Steinhardt wrote:
Show 33 quoted lines
> On Wed, Mar 11, 2026 at 12:31:47PM -0500, Justin Tobler wrote:
> > diff --git a/builtin/fast-import.c b/builtin/fast-import.c
> > index b8a7757cfd..d6281ff119 100644
> > --- a/builtin/fast-import.c
> > +++ b/builtin/fast-import.c
> > @@ -2865,6 +2855,66 @@ static void handle_strip_if_invalid(struct strbuf *new_data,
> > else
> > warning(_("stripping invalid signature for commit\n"
> > " allegedly by %s"), signer);
> > + break;
> > + case SIGN_SIGN_IF_INVALID:
> > + if (subject_len > 100)
> > + warning(_("signing commit with invalid signature for '%.100s...'\n"
> > + " allegedly by %s"), subject, signer);
> > + else if (subject_len > 0)
> > + warning(_("signing commit with invalid signature for '%.*s'\n"
> > + " allegedly by %s"), subject_len, subject, signer);
> > + else
> > + warning(_("signing commit with invalid signature\n"
> > + " allegedly by %s"), signer);
> > + break;
> > + default:
> > + BUG("unsupported signing mode");
> > + }
> > +}
>
> I'm still not convinced that it makes sense to warn about this case.
> After all the user has asked us to re-sign such commits, so they
> probably expect such cases. These warnings would thus result in a ton of
> noise in a repository where most commits are signed, drowning out the
> potentially-useful warnings.
>
> Anyway, I won't insist on a change here.I'm not really against removing these warning as I also agree it creates a bunch of noise. If we get rid of them for "sign-if-invalid" though, shouldn't we also get rid of them for "strip-if-invalid"? If the user asks to strip commits, I figure they would expect such cases as well. If we think removing the warning altogether is sensible, I can add another prepatory commit that simply removes the warning for the "strip-if-invalid" case.
Show 36 quoted lines
> > +static void handle_signature_if_invalid(struct strbuf *new_data,
> > + struct signature_data *sig_sha1,
> > + struct signature_data *sig_sha256,
> > + struct strbuf *msg,
> > + enum sign_mode mode)
> > +{
> > + struct strbuf tmp_buf = STRBUF_INIT;
> > + struct signature_check signature_check = { 0 };
> > + int ret;
> > +
> > + /* Check signature in a temporary commit buffer */
> > + strbuf_addbuf(&tmp_buf, new_data);
> > + finalize_commit_buffer(&tmp_buf, sig_sha1, sig_sha256, msg);
> > + ret = verify_commit_buffer(tmp_buf.buf, tmp_buf.len, &signature_check);
> > +
> > + if (ret) {
> > + warn_invalid_signature(&signature_check, msg->buf, mode);
> > +
> > + if (mode == SIGN_SIGN_IF_INVALID) {
> > + struct strbuf signature = STRBUF_INIT;
> > + struct strbuf payload = STRBUF_INIT;
> > +
> > + /*
> > + * NEEDSWORK: To properly support interoperability mode
> > + * when signing commit signatures, the commit buffer
> > + * must be provided in both the repository and
> > + * compatibility object formats. As currently
> > + * implemented, only the repository object format is
> > + * considered meaning compatibility signatures cannot be
> > + * generated. Thus, attempting to sign commit signatures
> > + * in interoperability mode is currently unsupported.
> > + */
> > + if (the_repository->compat_hash_algo)
> > + die(_("signing signatures in interoperability mode is unsupported"));
>
> "signing signatures"? You probably meant "signing commits"?Ah yes! Will fix in the next version. Thanks for reading closely :)
-Justin