Re: [PATCH 5/5] fast-import: add '--signed-tags=<mode>' option

From: Christian Couder <christian.couder@gmail.com>
Date: Oct 8, 2025, 10:50 UTC
Message-ID: <CAP8UFD0E+5K1yL1rj5jXVMX9hQyoA_sH0f=fUP6aCj==TtfAbQ@mail.gmail.com>
In-Reply-To: <aOYPYEk5sT6b1kuS@pks.im>

On Wed, Oct 8, 2025 at 11:21 AM Patrick Steinhardt <ps@pks.im> wrote:

Show 18 quoted lines

>
> On Tue, Oct 07, 2025 at 02:29:58PM +0200, Christian Couder wrote:
> > diff --git a/Documentation/git-fast-import.adoc b/Documentation/git-fast-import.adoc
> > index 85ed7a7270..b74179a6c8 100644
> > --- a/Documentation/git-fast-import.adoc
> > +++ b/Documentation/git-fast-import.adoc
> > @@ -66,6 +66,11 @@ fast-import stream! This option is enabled automatically for
> >  remote-helpers that use the `import` capability, as they are
> >  already trusted to run their own code.
> >
> > +--signed-tags=(verbatim|warn-verbatim|warn-strip|strip|abort)::
> > +     Specify how to handle signed tags.  Behaves in the same way
> > +     as the same option in linkgit:git-fast-export[1], except that
> > +     default is 'verbatim' (instead of 'abort').
> > +
>
> Nit: I would've ordered this after "--signed-commits", mostly so that
> these two are ordered alphabetically.

In the fast-export doc --signed-tags is before --signed-commits. Also in the previous patch series Junio mentioned that historically signed tags came before signed commits. And the other options are not sorted alphabetically.

Show 27 quoted lines

> >  --signed-commits=(verbatim|warn-verbatim|warn-strip|strip|abort)::
> >       Specify how to handle signed commits.  Behaves in the same way
> >       as the same option in linkgit:git-fast-export[1], except that
> > diff --git a/builtin/fast-import.c b/builtin/fast-import.c
> > index 2010e78475..668c926db5 100644
> > --- a/builtin/fast-import.c
> > +++ b/builtin/fast-import.c
> > @@ -2961,6 +2962,43 @@ static void parse_new_commit(const char *arg)
> >       b->last_commit = object_count_by_type[OBJ_COMMIT];
> >  }
> >
> > +static void handle_tag_signature(struct strbuf *msg, const char *name)
> > +{
> > +     size_t sig_offset = parse_signed_buffer(msg->buf, msg->len);
> > +
> > +     /* If there is no signature, there is nothing to do. */
> > +     if (sig_offset >= msg->len)
> > +             return;
> > +
> > +     switch (signed_tag_mode) {
> > +
> > +     /* First, modes that don't change anything */
> > +     case SIGN_ABORT:
> > +             die("encountered signed tag; use "
> > +                 "--signed-tags=<mode> to handle it");
>
> This message needs to be marked for translation.

Yeah, I will fix it in V2.

Show 5 quoted lines

> > +     case SIGN_WARN_VERBATIM:
> > +             warning(_("importing a tag signature verbatim for tag '%s'"), name);
> > +                     /* fallthru */
>
> This comment is misindented.

Will fix it in V2. Same with other misindented comments.

Show 8 quoted lines

> > +     case SIGN_STRIP:
> > +             /* Truncate the buffer to remove the signature */
> > +             strbuf_setlen(msg, sig_offset);
> > +             break;
>
> I'm not familiar with the signature format, so it's probably a dumb
> question: does the signature always extend until the end of the tag
> message? Doesn't the tag message come after it?

Users can add anything in a tag message, including signatures created however they want and copy-pasted there, followed by whatever content they want. I don't think we need to take care of those signatures, except perhaps to warn in our docs that Git could mistake them with the one Git creates.

When Git itself signs a tag, it appends the signature to the tag message. See do_sign() in "builtin/tag.c" for more details. It looks like 2 signatures can be created in "compat" mode, but the compat signature is added into an object header, not appended to the tag message.

So I think this is the right thing to do and relatively safe.