Re: [PATCH v1] convert: add support for 'encoding' attribute

On 12 Dec 2017, at 00:58, Eric Sunshine [off-list ref] wrote:

On Mon, Dec 11, 2017 at 6:47 PM, Lars Schneider
[off-list ref] wrote:

quoted

On 11 Dec 2017, at 19:39, Eric Sunshine [off-list ref] wrote:

quoted

On Mon, Dec 11, 2017 at 10:50 AM,  [off-list ref] wrote:

quoted

From: Lars Schneider <redacted>

Git and its tools (e.g. git diff) expect all text files in UTF-8
encoding. Git will happily accept content in all other encodings, too,
but it might not be able to process the text (e.g. viewing diffs or
changing line endings).

Add an attribute to tell Git what encoding the user has defined for a
given file. If the content is added to the index, then Git converts the
content to a canonical UTF-8 representation. On checkout Git will
reverse the conversion.

Reviewed-by: Patrick Lühne <redacted>
Signed-off-by: Lars Schneider <redacted>
---
+static int encode_to_git(const char *path, const char *src, size_t src_len,
+                        struct strbuf *buf, struct encoding *enc)
+{
+       if (enc->to_git == invalid_conversion) {
+               enc->to_git = iconv_open(default_encoding, encoding->name);
+               if (enc->to_git == invalid_conversion)
+                       warning(_("unsupported encoding %s"), encoding->name);
+       }
+
+       if (enc->to_worktree == invalid_conversion)
+               enc->to_worktree = iconv_open(encoding->name, default_encoding);

Do you need to be calling iconv_close() somewhere on the result of the
iconv_open() calls? [Answering myself after reading the rest of the
patch: You're caching these opened 'iconv' descriptors, so you don't
plan on closing them.]

Should this information go into the commit message to avoid confusing
future readers? I think, yes.

Maybe. However, the code which does the actual caching is so distant
from these iconv_open() invocations that it might be more helpful to
have an in-code comment here saying that the "missing" iconv_close()
invocations is intentional.

Agreed. I will add that in v2.

Thanks,
Lars