Re: [PATCH v4 3/3] fast-import: add mode to sign commits with invalid signatures
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Mar 12, 2026, 10:23 UTC
- Message-ID
- <abKUBRRgRmbJ1hRA@pks.im>
- In-Reply-To
- <20260311173147.2336432-4-jltobler@gmail.com>
On Wed, Mar 11, 2026 at 12:31:47PM -0500, Justin Tobler wrote:
Show 24 quoted lines
> 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.
Show 34 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"?
Patrick