Re: [PATCH 2/5] log: Refactor duplicated code to headerize recipient lists

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 25, 2023, 02:11 UTC
Message-ID: <xmqqo7qnfjnf.fsf@gitster.g>
In-Reply-To: <20230119223858.29262-3-zev@bewilderbeest.net>

Zev Weiss <zev@bewilderbeest.net> writes:

> Subject: Re: [PATCH 2/5] log: Refactor duplicated code to headerize recipient lists

Style: "log: Refactor" -> "log: refactor"

cf. Documentation/SubmittingPatches[[summary-section]]

> The To and Cc headers are handled identically (the only difference is
> the header name tag), so we might as well reuse the same code for both
> instead of duplicating it.

Makes tons of sense.  But seeing this one ...

> +	recipients_to_header_buf("To", &buf, &extra_to);
> +	recipients_to_header_buf("Cc", &buf, &extra_cc);

... the parameters to the function probably should be ...

> +void recipients_to_header_buf(const char *hdr, struct strbuf *buf,
> +			      const struct string_list *recipients);

... in this order, instead:

    format_recipients(&buf, "To", &extra_to);

That is, "To" and &extra_to are much closely related to each other than they are to &buf in the sense that they are both input to the helper function to work on, while &buf is an output buffer, and we tend to place closer things together, next to each other.

Other than that, removal of repetition is very good.

Thanks.