From: Patrick Steinhardt <ps@pks.im>
Date: Thu, 12 Mar 2026 14:22:03 GMT
Subject: Re: [PATCH v4 3/3] fast-import: add mode to sign commits with invalid signatures
Message-ID: <abLMCxWWNiCnqmp_@pks.im>
In-Reply-To: <abLGgq-PXzdWs6kD@denethor>

On Thu, Mar 12, 2026 at 09:08:46AM -0500, Justin Tobler wrote:
> 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.

Yeah, it kind of falls into the same space, agreed. As said, I won't
insist on changing this. Maybe the right way to approach this is to keep
it as-is for now and create a follow-up patch where you propose to strip
it from both sites?

Patrick