From: Patrick Steinhardt <ps@pks.im>
Date: Wed, 08 Oct 2025 07:14:34 GMT
Subject: Re: [PATCH 4/5] fast-export: handle all kinds of tag signatures
Message-ID: <aOYPWvdE4VnL8T7z@pks.im>
In-Reply-To: <20251007122958.1089680-5-christian.couder@gmail.com>

On Tue, Oct 07, 2025 at 02:29:57PM +0200, Christian Couder wrote:
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> index dc2486f9a8..7adbc55f0d 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -931,9 +931,8 @@ static void handle_tag(const char *name, struct tag *tag)
>  
>  	/* handle signed tags */
>  	if (message) {
> -		const char *signature = strstr(message,
> -					       "\n-----BEGIN PGP SIGNATURE-----\n");
> -		if (signature)
> +		size_t sig_offset = parse_signed_buffer(message, message_size);
> +		if (sig_offset < message_size)

Yup. The function either returns `message_size` in case there is no
signature, or it returns the offset at which the signature starts.

>  			switch (signed_tag_mode) {
>  			case SIGN_ABORT:
>  				die("encountered signed tag %s; use "

I was afraid at first that we're now open-coding all these different
signature formats. But this implementation makes me quite happy, as we
even remove the existing check instead of using a central function.
Nice.

> @@ -950,7 +949,7 @@ static void handle_tag(const char *name, struct tag *tag)
>  					oid_to_hex(&tag->object.oid));
>  				/* fallthru */
>  			case SIGN_STRIP:
> -				message_size = signature + 1 - message;
> +				message_size = sig_offset;
>  				break;
>  			}
>  	}

Makes sense.

> diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
> index 21ff26939c..5a46608f65 100755
> --- a/t/t9350-fast-export.sh
> +++ b/t/t9350-fast-export.sh
> @@ -279,6 +279,54 @@ test_expect_success 'signed-tags=warn-strip' '
>  	test -s err
>  '
>  
> +test_expect_success GPGSM 'setup X.509 signed tag' '
> +
> +	test_config gpg.format x509 &&
> +	test_config user.signingkey $GIT_COMMITTER_EMAIL &&
> +
> +	git tag -s -m "X.509 signed tag" x509-signed $(git rev-parse HEAD) &&
> +	ANNOTATED_TAG_COUNT=$((ANNOTATED_TAG_COUNT + 1))
> +
> +'

Nit for this test and all of the below ones: our modern style does not
have empty lines at the beginning and end of a test case.

Patrick