From: Christian Couder <christian.couder@gmail.com>
Date: Tue, 10 Mar 2026 09:27:14 GMT
Subject: Re: [PATCH v2 3/3] fast-import: add mode to re-sign invalid commit signatures
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:

> @@ -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>"));

[...]

> @@ -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.

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

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