Re: [PATCH] mailinfo: we parse fixed headers

From: Patrick Steinhardt <ps@pks.im>
Date: Aug 22, 2024, 08:24 UTC
Message-ID: <Zsb1rGQbglHMiBHI@tanuki>
In-Reply-To: <xmqq1q2i6gw7.fsf@gitster.g>

On Tue, Aug 20, 2024 at 02:09:12PM -0700, Junio C Hamano wrote:

Show 18 quoted lines

> diff --git c/mailinfo.c w/mailinfo.c
> index 94b9b0abf2..17d7c3b594 100644
> --- c/mailinfo.c
> +++ w/mailinfo.c
> @@ -346,10 +346,10 @@ static void cleanup_subject(struct mailinfo *mi, struct strbuf *subject)
>  	strbuf_trim(subject);
>  }
>  
> -#define MAX_HDR_PARSED 10
> -static const char *header[MAX_HDR_PARSED] = {
> -	"From","Subject","Date",
> +static const char * const header[] = {
> +	"From", "Subject", "Date",
>  };
> +#define MAX_HDR_PARSED (ARRAY_SIZE(header) + 1)
>  
>  static inline int skip_header(const struct strbuf *line, const char *hdr,
>  			      const char **outval)

I was briefly wondering whether changing MAX_HDR_PARSED is going to cause negative fallout. But the only place where it is used is to allocate both the `mi->p_hdr_data` and `mi->s_hdr_data` arrays. And these are only populated via non-NULL `header`s.

Show 9 quoted lines

> @@ -583,7 +583,7 @@ static int check_header(struct mailinfo *mi,
>  	struct strbuf sb = STRBUF_INIT;
>  
>  	/* search for the interesting parts */
> -	for (i = 0; header[i]; i++) {
> +	for (i = 0; i < ARRAY_SIZE(header); i++) {
>  		if ((!hdr_data[i] || overwrite) &&
>  		    parse_header(line, header[i], mi, &sb)) {
>  			handle_header(&hdr_data[i], &sb);

This is also a welcome change, as it makes it way easier to spot intent and to notice that `header` is indeed a static array of string constants.

So this change looks sensible to me, thanks!

Patrick