[PATCH] mailinfo: we parse fixed headers

[PATCH] mailinfo: we parse fixed headers

[Junio C Hamano]

The code was written as if we have a small room to add additional
headers to be parsed to the header[] array at runtime, but that is
not our intention at all.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * This will not be applied and merged down, as this will conflict
   with Patrick's leakfix part 5.  In this version, MAX_HDR_PARSED
   is still set to 1 larger than necessary, due to the use of
   strbuf_list_free(), but after the leakfix-5 topic fixes it,
   MAX_HDR_PARSED can be reduced to ARRAY_SIZE(header).

 mailinfo.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

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)
@@ -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);
@@ -625,7 +625,7 @@ static int is_inbody_header(const struct mailinfo *mi,
 {
 	int i;
 	const char *val;
-	for (i = 0; header[i]; i++)
+	for (i = 0; i < ARRAY_SIZE(header); i++)
 		if (!mi->s_hdr_data[i] && skip_header(line, header[i], &val))
 			return 1;
 	return 0;
@@ -772,7 +772,7 @@ static int check_inbody_header(struct mailinfo *mi, const struct strbuf *line)
 		return is_format_patch_separator(line->buf + 1, line->len - 1);
 	if (starts_with(line->buf, "[PATCH]") && isspace(line->buf[7])) {
 		int i;
-		for (i = 0; header[i]; i++)
+		for (i = 0; i < ARRAY_SIZE(header); i++)
 			if (!strcmp("Subject", header[i])) {
 				handle_header(&mi->s_hdr_data[i], line);
 				return 1;
@@ -824,7 +824,7 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line)
 		 * We may have already read "secondary headers"; purge
 		 * them to give ourselves a clean restart.
 		 */
-		for (i = 0; header[i]; i++) {
+		for (i = 0; i < ARRAY_SIZE(header); i++) {
 			if (mi->s_hdr_data[i])
 				strbuf_release(mi->s_hdr_data[i]);
 			FREE_AND_NULL(mi->s_hdr_data[i]);
@@ -1155,7 +1155,7 @@ static void handle_info(struct mailinfo *mi)
 	struct strbuf *hdr;
 	int i;
 
-	for (i = 0; header[i]; i++) {
+	for (i = 0; i < ARRAY_SIZE(header); i++) {
 		/* only print inbody headers if we output a patch file */
 		if (mi->patch_lines && mi->s_hdr_data[i])
 			hdr = mi->s_hdr_data[i];

Re: [PATCH] mailinfo: we parse fixed headers

[Patrick Steinhardt]

On Tue, Aug 20, 2024 at 02:09:12PM -0700, Junio C Hamano wrote:
> 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.

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