Re: [PATCH v4 1/4] mailinfo.c: convert_to_utf8(): added a target_charset parameter

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 29, 2010, 20:23 UTC
Message-ID: <7vsjyj3mmr.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1290971417-4474-2-git-send-email-r0bertz@gentoo.org>

"ZHANG, Le" <r0bertz@gentoo.org> writes:

> This is required for my recode-patch patch which needs a seperate patch_charset variable.
>
> Signed-off-by: ZHANG, Le <r0bertz@gentoo.org>
> ---

Thanks.

Please describe what the new parameter means. Is it used to convert the contents in "line" from "charset" to "target_charset"? Perhaps it is a good time to rename the function to "convert_to()", its "charset" parameter to "from_charset", and name the new parameter "to_charset", in order to reduce confusion?

It is not just _your_ patch but will help other/later patches, so you may want to phrase the proposed commit log message a bit differently (and with a narrower page width, like 68-74 chars per line). Perhaps like...

    mailinfo.c: Allow convert_to_utf8() to specify both src/dst charset

    The convert_to_utf8() function actually converts to whatever charset
    "metainfo_charset" variable contains, which is not necessarily UTF-8.
    Rename it to convert_to(), and give an extra parameter "to_charset" to
    specify what charset to re-encode to.  Also rename its "charset"
    parameter to "from_charset" to clarify which is which.