From: Justin Tobler Date: Tue, 24 Feb 2026 18:33:52 GMT Subject: Re: [PATCH 2/2] fast-import: add mode to re-sign invalid commit signatures Message-ID: In-Reply-To: On 26/02/24 10:33AM, Patrick Steinhardt wrote: > On Mon, Feb 23, 2026 at 01:41:46PM -0600, Justin Tobler wrote: > > With git-fast-import(1), handling of signed commits is controlled via > > the `--signed-commits=` option. When an invalid signature is > > encountered, a user may want the option to re-sign the commit as opposed > > to just stripping the signature. To faciliate this, introduce a > > "re-sign-if-invalid" mode for the `--signed-commits` option. > > > > Note that commits are re-signed using only the repository object format > > hash algorithm. If a commit has an additional signature due to the > > `compatObjectFormat` repository extension being set, the other signature > > is stripped. > > This part here might use some explanation why this part is not done so > that a future reader that ends up here doesn't have to wonder whether > this is done with intent, or whether this was done because it was hard > to do. Good point. I'll expand the explaination here in the next version. > > diff --git a/Documentation/git-fast-import.adoc b/Documentation/git-fast-import.adoc > > index 479c4081da..b902a6e2b0 100644 > > --- a/Documentation/git-fast-import.adoc > > +++ b/Documentation/git-fast-import.adoc > > @@ -86,6 +86,9 @@ already trusted to run their own code. > > * `strip-if-invalid` will check signatures and, if they are invalid, > > will strip them and display a warning. The validation is performed > > in the same way as linkgit:git-verify-commit[1] does it. > > +* `re-sign-if-invalid` is the same as `strip-if-invalid`, but additionally the > > + commits with invalid signatures are signed again, so that old invalid > > + signatures are replaced with new valid ones. > > Okay. It's a bit curious to say it's the "same as `strip-if-invalid`", > but I get what you mean by this, and I think a user would, too. Ya, maybe it would be better to say that it is "similar to `strip-if-invalid`". I'll try to rework the documentation here a little bit in the next version. > > diff --git a/builtin/fast-import.c b/builtin/fast-import.c > > index b8a7757cfd..e34a373d2f 100644 > > --- a/builtin/fast-import.c > > +++ b/builtin/fast-import.c > > @@ -2836,10 +2836,11 @@ static void finalize_commit_buffer(struct strbuf *new_data, > > strbuf_addbuf(new_data, msg); > > } > > > > -static void handle_strip_if_invalid(struct strbuf *new_data, > > - struct signature_data *sig_sha1, > > - struct signature_data *sig_sha256, > > - struct strbuf *msg) > > +static void handle_invalid_signature(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 }; > > Should we maybe call this `handle_signature_if_invalid()`? Otherwise it > sounds as if we already know the signature was invalid. That sounds better. Will adapt. > > > @@ -2866,6 +2867,30 @@ static void handle_strip_if_invalid(struct strbuf *new_data, > > warning(_("stripping invalid signature for commit\n" > > " allegedly by %s"), signer); > > I wonder: does it still make sense to warn about those stripped > signatures in case we re-sign anyway? Ya, good point. I was originally thinking it would still make sense to keep these messages since we are still stripping the signatures, but it might be misleading if are also re-signing them. We could keep these, but add an additional message if re-signing. That might be a little noisy though. Maybe we just adapt the warning message when re-signing. > > + if (mode == SIGN_RESIGN_IF_INVALID) { > > + struct strbuf signature = STRBUF_INIT; > > + struct strbuf payload = STRBUF_INIT; > > + char *key = get_signing_key(); > > + > > + /* > > + * Commits are resigned using the repository object > > Poor commits. Maybe s/resigned/re-signed/? Poor commits indeed, will change. XD > > + * format hash algorithm only. Consequently if > > + * extensions.compatObjectFormat is set, the > > + * compatability hash is not currently used to > > + * additionally sign the commit. If the commit payload > > + * were reconstructed in the compatability format, it > > + * would be possible to generate the other signature > > + * accordingly though. > > + */ > > Same as in the commit message, we should document whether this is done > intentionally, or whether it may require more work going forward. If the > latter, it might make sense to add a NEEDSWORK comment. Ya, I think it should be possible to support compatibility hashes in the future. I'll explain this better in a NEEDSWORK comment. > I think meanwhile though it's okay that we don't handle compatibility > hashes yet. > > > diff --git a/gpg-interface.c b/gpg-interface.c > > index 87fb6605fb..e7eb42d9d6 100644 > > --- a/gpg-interface.c > > +++ b/gpg-interface.c > > @@ -1156,6 +1156,8 @@ int parse_sign_mode(const char *arg, enum sign_mode *mode) > > *mode = SIGN_STRIP; > > else if (!strcmp(arg, "strip-if-invalid")) > > *mode = SIGN_STRIP_IF_INVALID; > > + else if (!strcmp(arg, "re-sign-if-invalid")) > > + *mode = SIGN_RESIGN_IF_INVALID; > > else > > return -1; > > return 0; > > One thing I wonder here is which signing key is actually in use, and how > the user would specify it. In git-commit(1) you can for example pass > "--gpg-sign=" to specify the key. Do we want to allow the same > here, where you can pass "--signed-commits=re-sign-if-invalid[=]"? This seems sensible. I'll explore this in the next version. Thanks for the review. :) -Justin