On Wed, Mar 09, 2022 at 02:16:33PM +0100, Ævar Arnfjörð Bjarmason wrote:
> @@ -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.
> @@ -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.
> @@ -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
