Re: [PATCH 03/24] format-patch: don't leak "extra_headers" or "ref_message_ids"

From: Taylor Blau <me@ttaylorr.com>
Date: Mar 9, 2022, 20:34 UTC
Message-ID: <YikPXN5xobn+Y/Jn@nand.local>
In-Reply-To: <patch-03.24-ab1afe6c0c8-20220309T123321Z-avarab@gmail.com>

On Wed, Mar 09, 2022 at 02:16:33PM +0100, Ævar Arnfjörð Bjarmason wrote:

Show 7 quoted lines

> @@ -1946,7 +1947,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  		strbuf_addch(&buf, '\n');
>  	}
>
> -	rev.extra_headers = strbuf_detach(&buf, NULL);
> +	extra_headers = strbuf_detach(&buf, NULL);
> +	rev.extra_headers = extra_headers;

Small nit, these two assignments could be combined on the same line without wrapping. But obviously not a big deal.

Show 10 quoted lines

> @@ -2173,8 +2175,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  		prepare_bases(&bases, base, list, nr);
>  	}
>
> -	if (in_reply_to || thread || cover_letter)
> -		rev.ref_message_ids = xcalloc(1, sizeof(struct string_list));
> +	if (in_reply_to || thread || cover_letter) {
> +		rev.ref_message_ids = xmalloc(sizeof(*rev.ref_message_ids));
> +		string_list_init_nodup(rev.ref_message_ids);
> +	}

OK, and this is the "while we're at it..." part of your commit message. I did find this a little confusing to read at first, so wouldn't have minded to see this and the rest of the change split across two patches, but I think this is fine, too.

Show 7 quoted lines

> @@ -2281,6 +2285,11 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  	strbuf_release(&rdiff1);
>  	strbuf_release(&rdiff2);
>  	strbuf_release(&rdiff_title);
> +	free(extra_headers);
> +	if (rev.ref_message_ids) {
> +		string_list_clear(rev.ref_message_ids, 0);

I was surprised to learn that string_list_clear() is not a noop when its first argument is NULL.

Looking around, it probably wouldn't help that much. E.g., running something like:

    git grep -B1 'string_list_clear(' | grep if -A1

doesn't turn up many results like the above. So maybe some good #leftoverbits there, although maybe not.

> +		free(rev.ref_message_ids);

This could go in or out of the if-statement.

Thanks, Taylor