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, 14:22 UTC
- Message-ID
- <abLMCxWWNiCnqmp_@pks.im>
- In-Reply-To
- <abLGgq-PXzdWs6kD@denethor>
On Thu, Mar 12, 2026 at 09:08:46AM -0500, Justin Tobler wrote:
Show 42 quoted lines
> 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