Re: [PATCH v2 3/3] fast-import: add mode to re-sign invalid commit signatures
- From
Christian Couder <christian.couder@gmail.com>
- Date
- Mar 10, 2026, 09:27 UTC
- Message-ID
- <CAP8UFD3p84U0FhjGXNqagtDi=Cd3+QBHqGb3_ceWy-tdeLc43g@mail.gmail.com>
- In-Reply-To
- <20260306205359.1723254-4-jltobler@gmail.com>
On Fri, Mar 6, 2026 at 9:54 PM Justin Tobler <jltobler@gmail.com> wrote:
Show 5 quoted lines
> @@ -825,6 +825,9 @@ static void handle_commit(struct commit *commit, struct rev_info *rev,
> case SIGN_STRIP_IF_INVALID:
> die(_("'strip-if-invalid' is not a valid mode for "
> "git fast-export with --signed-commits=<mode>"));
> + case SIGN_RESIGN_IF_INVALID:Everywhere in this patch, I think "RE_SIGN" might be more consistent than "RESIGN" for this name.
> + die(_("'re-sign-if-invalid' is not a valid mode for "
> + "git fast-export with --signed-commits=<mode>"));[...]
Show 36 quoted lines
> @@ -2856,15 +2858,52 @@ static void handle_strip_if_invalid(struct strbuf *new_data,
> const char *subject;
> int subject_len = find_commit_subject(msg->buf, &subject);
>
> - if (subject_len > 100)
> - warning(_("stripping invalid signature for commit '%.100s...'\n"
> - " allegedly by %s"), subject, signer);
> - else if (subject_len > 0)
> - warning(_("stripping invalid signature for commit '%.*s'\n"
> - " allegedly by %s"), subject_len, subject, signer);
> - else
> - warning(_("stripping invalid signature for commit\n"
> - " allegedly by %s"), signer);
> + if (mode == SIGN_STRIP_IF_INVALID) {
> + if (subject_len > 100)
> + warning(_("stripping invalid signature for commit '%.100s...'\n"
> + " allegedly by %s"), subject, signer);
> + else if (subject_len > 0)
> + warning(_("stripping invalid signature for commit '%.*s'\n"
> + " allegedly by %s"), subject_len, subject, signer);
> + else
> + warning(_("stripping invalid signature for commit\n"
> + " allegedly by %s"), signer);
> + } else if (mode == SIGN_RESIGN_IF_INVALID) {
> + struct strbuf signature = STRBUF_INIT;
> + struct strbuf payload = STRBUF_INIT;
> +
> + if (subject_len > 100)
> + warning(_("re-signing invalid signature for commit '%.100s...'\n"
> + " allegedly by %s"), subject, signer);
> + else if (subject_len > 0)
> + warning(_("re-signing invalid signature for commit '%.*s'\n"
> + " allegedly by %s"), subject_len, subject, signer);
> + else
> + warning(_("re-signing invalid signature for commit\n"
> + " allegedly by %s"), signer);Maybe a helper function could be used to avoid duplicating the warning logic.
Show 5 quoted lines
> + /* > + * NEEDSWORK: To properly support interoperability mode > + * when re-signing commit signatures, the commit buffer > + * must be provided in both the repository and > + * compatability object formats. As currently
s/compatability/compatibility/
> + * implemented, only the repository object format is > + * considered meaning compatability signatures cannot be
s/compatability/compatibility/
Show 15 quoted lines
> + * generated. Thus, attempting to re-sign commit
> + * signatures in interoperability mode is currently
> + * unsupported.
> + */
> + if (the_repository->compat_hash_algo)
> + die(_("re-signing signatures in interoperability mode is unsupported"));
> +
> + strbuf_addstr(&payload, signature_check.payload);
> + if (sign_buffer_with_key(&payload, &signature, signed_commit_keyid))
> + die(_("failed to sign commit object"));
> + add_header_signature(new_data, &signature, the_hash_algo);
> +
> + strbuf_release(&signature);
> + strbuf_release(&payload);
> + }Except for these small issues and the few nits in the previous patch, this looks good to me. Thanks for working on it.