From: Justin Tobler <jltobler@gmail.com>
Date: Thu, 12 Mar 2026 14:08:46 GMT
Subject: Re: [PATCH v4 3/3] fast-import: add mode to sign commits with invalid signatures
Message-ID: <abLGgq-PXzdWs6kD@denethor>
In-Reply-To: <abKUBRRgRmbJ1hRA@pks.im>

On 26/03/12 11:23AM, Patrick Steinhardt wrote:
> 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.

> > +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