{"thread":{"id":"47307","subject":"RFC: Native clean/smudge filter for UTF-16 files","startedAt":"2017-11-23T15:19:07Z","lastAt":"2017-12-04T17:23:47Z","messageCount":6,"participants":["Lars Schneider","Torsten Bögershausen","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"333385","messageId":"BDB9B884-6D17-4BE3-A83C-F67E2AFA2B46@gmail.com","threadId":"47307","inReplyTo":null,"subject":"RFC: Native clean/smudge filter for UTF-16 files","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-11-23T15:18:59Z","receivedAt":"2017-11-23T15:19:07Z","isPatch":false,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"Hi,\n\nI am working with a team that owns a repository with lots of UTF-16 files.\nConverting these files to UTF-8 is no good option as downstream applications\nrequire the UTF-16 encoding. Keeping the files in UTF-16 is no good option\neither as Git and Git related tools (e.g. GitHub) consider the files binary\nand consequently do not render diffs.\n\nThe obvious solution is to setup a clean/smudge filter like this [1]:\n [filter \"winutf16\"]\n clean = iconv -f utf-16 -t utf-8\n smudge = iconv -f utf-8 -t utf-16\n\nIn general this works well but the \"per-file\" clean/smudge process invocation \ncan be slow for many files. I could apply the same trick that we used for Git\nLFS and write a iconv that processes all files with a single invocation (see\n\"edcc85814c convert: add filter..process option\" [2]). \n\nAlternatively, I could add a native attribute to Git that translates UTF-16 \nto UTF-8 and back. A conversion function is already available in \"mingw.h\" [3]\non Windows. Limiting this feature to Windows wouldn't be a problem from my\npoint of view as UTF-16 is only relevant on Windows anyways. The attribute \ncould look like this:\n\n *.txt text encoding=utf-16\n\nThere was a previous discussion on the topic and Jonathan already suggested\na \"native\" clean/smudge filter in 2010 [4]. Also the \"encoding\" attribute\nis already present but, as far as I can tell, is only used by the git gui\nfor viewing [5].\n\nDo you think a patch that converts UTF-16 files to UTF-8 via an attribute\n\"encoding=utf-16\" on Windows would have a chance to get accepted?\n\nThanks,\nLars\n\n[1] https://github.com/msysgit/msysgit/issues/113#issue-13142846\n[2] https://github.com/git/git/commit/edcc85814c87ebd7f3b1b7d3979fac3dfb84d308\n[3] https://github.com/git/git/blob/14c63a9dc093d6738454f6369a4f5663ca732cf7/compat/mingw.h#L501-L533\n[4] https://public-inbox.org/git/20101022195331.GA12014@burratino/\n[5] https://github.com/git/git/commit/1ffca60f0b0395e1e593e64d66e7ed3c47d8517e"},{"id":"333390","messageId":"20171123200913.GA29990@tor.lan","threadId":"47307","inReplyTo":"BDB9B884-6D17-4BE3-A83C-F67E2AFA2B46@gmail.com","subject":"Re: RFC: Native clean/smudge filter for UTF-16 files","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2017-11-23T20:09:13Z","receivedAt":"2017-11-23T20:09:37Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Thu, Nov 23, 2017 at 04:18:59PM +0100, Lars Schneider wrote:\n> Hi,\n> \n> I am working with a team that owns a repository with lots of UTF-16 files.\n> Converting these files to UTF-8 is no good option as downstream applications\n> require the UTF-16 encoding. Keeping the files in UTF-16 is no good option\n> either as Git and Git related tools (e.g. GitHub) consider the files binary\n> and consequently do not render diffs.\n> \n> The obvious solution is to setup a clean/smudge filter like this [1]:\n> [filter \"winutf16\"]\n> clean = iconv -f utf-16 -t utf-8\n> smudge = iconv -f utf-8 -t utf-16\n> \n> In general this works well but the \"per-file\" clean/smudge process invocation \n> can be slow for many files. I could apply the same trick that we used for Git\n> LFS and write a iconv that processes all files with a single invocation (see\n> \"edcc85814c convert: add filter..process option\" [2]). \n> \n> Alternatively, I could add a native attribute to Git that translates UTF-16 \n> to UTF-8 and back. A conversion function is already available in \"mingw.h\" [3]\n> on Windows. Limiting this feature to Windows wouldn't be a problem from my\n> point of view as UTF-16 is only relevant on Windows anyways. The attribute \n> could look like this:\n> \n> *.txt text encoding=utf-16\n> \n\nI would probably vote for UTF-16LE.\nBut we can specify whatever iconv allows, see below.\n\n[]\n> Do you think a patch that converts UTF-16 files to UTF-8 via an attribute\n> \"encoding=utf-16\" on Windows would have a chance to get accepted?\n\nYes.\nThe thing is that we have convert.c and utf8.c (reencode_string_iconv()\nand possible strbuf.c, which feels that they are in charge here.\n\nHaving a path using mingw.h would mean that this is Windows only,\nand that is not a good solution.\n(I sometimes host files on a Linux box, export them via SAMBA to\n Mac and Windows. Having that kind of setup allows to commit\n directly on the Linux fileserver)\n\nBut even if you don't have that setup, cross platform is a must, I would say.\nThere may be possible optimizations using xutftowcsn() and friends under\nWindows, but they may come later into the picture (or are not needed)\nWhat file sizes are we talking about ?\n100K ?\n100M ?\n\nIt is even possible to hook into the streaming interface.\n\n> \n> Thanks,\n> Lars\n> \n"},{"id":"333450","messageId":"20171124180401.GB29190@sigill","threadId":"47307","inReplyTo":"BDB9B884-6D17-4BE3-A83C-F67E2AFA2B46@gmail.com","subject":"Re: RFC: Native clean/smudge filter for UTF-16 files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-24T18:04:01Z","receivedAt":"2017-11-24T18:04:08Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 23, 2017 at 04:18:59PM +0100, Lars Schneider wrote:\n\n> Alternatively, I could add a native attribute to Git that translates UTF-16 \n> to UTF-8 and back. A conversion function is already available in \"mingw.h\" [3]\n> on Windows. Limiting this feature to Windows wouldn't be a problem from my\n> point of view as UTF-16 is only relevant on Windows anyways. The attribute \n> could look like this:\n> \n> *.txt text encoding=utf-16\n>\n> There was a previous discussion on the topic and Jonathan already suggested\n> a \"native\" clean/smudge filter in 2010 [4]. Also the \"encoding\" attribute\n> is already present but, as far as I can tell, is only used by the git gui\n> for viewing [5].\n\nI would not want to see a proliferation of built-in filters, but it\nreally seems like text-encoding conversion is a broad and practical one\nthat many people might benefit from. So just like line-ending\nconversion, which _could_ be done by generic filters, it makes sense to\nme to support it natively for speed and simplicity.\n\n> Do you think a patch that converts UTF-16 files to UTF-8 via an attribute\n> \"encoding=utf-16\" on Windows would have a chance to get accepted?\n\nYou haven't fully specified the semantics here, so let me sketch out\nwhat I think it ought to look like:\n\n - declare utf8 the \"canonical\" in-repo representation, just as we have\n declared LF for line endings (alternatively this could be\n configurable, but if we can get away with declaring utf8 the one true\n encoding, that cuts out a lot of corner cases).\n\n - if core.convertEncoding is true, then for any file with an\n encoding=foo attribute, internally run iconv(foo, utf8) in\n convert_to_git(), and likewise iconv(utf8, foo) in\n convert_to_working_tree.\n\n - I'm not sure if core.convertEncoding should be enabled by default. If\n it's a noop as long as there's no encoding attribute, then it's\n probably fine. But I would not want accidental conversion or any\n slowdown for the common case that the user wants no conversion.\n\n - I doubt we'd want a \"core.autoEncoding\" similar to \"core.autocrlf\". I\n don't think people consistently have all utf-16 files (the way they\n might have all CRLF files) rather a few files that must be utf-16.\n\n - I have actually seen two types of utf-16 in git repos in the wild:\n files which really must be utf-16 (because some tool demands it) and\n files which happen to be utf-16, but could just as easily be utf-8\n (and the user simply does not notice and commits utf-16, but doesn't\n realize it until much later when their diffs are unreadable).\n\n For the first case, the \"encoding\" thing above would work fine. For\n the second case, in theory we could have an option that takes any\n file with a \"text\" attribute and no \"encoding\" attribute, and\n converts it to utf-8.\n\n I suspect that's opening a can of worms for false positives similar\n to core.autocrlf. And performance drops as we try to guess the\n encoding and convert all incoming data.\n\n So I mention it mostly as a direction I think we probably _don't_\n want to go. Anybody with the \"this could have been utf-8 all along\"\n type of file can remedy it by converting and committing the result.\n\nOmitting all of the \"we shouldn't do this\" bullet points, it seems\npretty simple and sane to me.\n\nThere is one other approach, which is to really store utf-16 in the\nrepository and better teach the diff tools to handle it (which are\nreally the main thing in git that cares about looking into the blob\ncontents). You can do this already with a textconv filter, but:\n\n 1. It's slow (though cacheable).\n\n 2. It doesn't work unless each repo configures the filter (so not on\n sites like GitHub, unless we define a micro-format that diff=utf16\n should be textconv'd on display, and get all implementations to\n respect that).\n\n 3. Textconv patches look good, but can't be applied. This occasionally\n makes things awkward, depending on your workflow.\n\n 4. You have to actually mark each file with an attribute, which is\n slightly annoying and more thing to remember to do (I see this from\n the server side, since people often commit utf-16 without any\n attributes, and then get annoyed when they see the file marked as\n binary).\n\nWe've toyed with the idea at GitHub of auto-detecting UTF-16 BOMs and\ndoing an \"auto-textconv\" to utf-8 (for our human-readable diffs only, of\ncourse). That solves (1), (2), and (4), but leaves (3). I actually\nlooked into using libicu to do it not just for UTF-16, but to detect any\nencoding. It turned out to be really slow, though. :)\n\nSo anyway, that is an alternate strategy, but I think I like \"canonical\nin-repo text is utf-8\" approach a lot more, since then git operations\nwork consistently. There are still a few rough edges (e.g., I'm not sure\nif you could apply a utf-8 patch directly to a utf-16 working tree file.\nCertainly not using \"patch\", but I'm not sure how well \"git apply\" would\nhandle that case either). But I think it would mostly Just Work as long\nas people were willing to set their encoding attributes.\n\n-Peff\n"},{"id":"333466","messageId":"xmqqy3mvks0v.fsf@gitster.mtv.corp.google.com","threadId":"47307","inReplyTo":"20171124180401.GB29190@sigill","subject":"Re: RFC: Native clean/smudge filter for UTF-16 files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-25T02:32:00Z","receivedAt":"2017-11-25T02:32:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So anyway, that is an alternate strategy, but I think I like \"canonical\n> in-repo text is utf-8\" approach a lot more, since then git operations\n> work consistently. There are still a few rough edges (e.g., I'm not sure\n\nSounds like a good way forward.\n\n> if you could apply a utf-8 patch directly to a utf-16 working tree file.\n> Certainly not using \"patch\", but I'm not sure how well \"git apply\" would\n> handle that case either). But I think it would mostly Just Work as long\n> as people were willing to set their encoding attributes.\n\nIt should work (or fail) just like applying LF patch to CRLF working\ntree, so I wouldn't worry too much about it.\n\nThanks.\n\n"},{"id":"334003","messageId":"759F0C3A-8C46-4685-BB27-6D508B26BB49@gmail.com","threadId":"47307","inReplyTo":"20171124180401.GB29190@sigill","subject":"Re: RFC: Native clean/smudge filter for UTF-16 files","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-12-03T18:48:01Z","receivedAt":"2017-12-03T18:48:11Z","isPatch":false,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 24 Nov 2017, at 19:04, Jeff King wrote:\n> \n> On Thu, Nov 23, 2017 at 04:18:59PM +0100, Lars Schneider wrote:\n> \n>> Alternatively, I could add a native attribute to Git that translates UTF-16 \n>> to UTF-8 and back. A conversion function is already available in \"mingw.h\" [3]\n>> on Windows. Limiting this feature to Windows wouldn't be a problem from my\n>> point of view as UTF-16 is only relevant on Windows anyways. The attribute \n>> could look like this:\n>> \n>> *.txt text encoding=utf-16\n>> \n>> There was a previous discussion on the topic and Jonathan already suggested\n>> a \"native\" clean/smudge filter in 2010 [4]. Also the \"encoding\" attribute\n>> is already present but, as far as I can tell, is only used by the git gui\n>> for viewing [5].\n> \n> I would not want to see a proliferation of built-in filters, but it\n> really seems like text-encoding conversion is a broad and practical one\n> that many people might benefit from. So just like line-ending\n> conversion, which _could_ be done by generic filters, it makes sense to\n> me to support it natively for speed and simplicity.\n> \n>> Do you think a patch that converts UTF-16 files to UTF-8 via an attribute\n>> \"encoding=utf-16\" on Windows would have a chance to get accepted?\n> \n> You haven't fully specified the semantics here, so let me sketch out\n> what I think it ought to look like:\n\nThank you :-)\n\n\n> - declare utf8 the \"canonical\" in-repo representation, just as we have\n> declared LF for line endings (alternatively this could be\n> configurable, but if we can get away with declaring utf8 the one true\n> encoding, that cuts out a lot of corner cases).\n\n100% agree on UTF-8 as canonical representation and I wouldn't make \nthat configurable as it would open a can of worms.\n\n\n> - if core.convertEncoding is true, then for any file with an\n> encoding=foo attribute, internally run iconv(foo, utf8) in\n> convert_to_git(), and likewise iconv(utf8, foo) in\n> convert_to_working_tree.\n> \n> - I'm not sure if core.convertEncoding should be enabled by default. If\n> it's a noop as long as there's no encoding attribute, then it's\n> probably fine. But I would not want accidental conversion or any\n> slowdown for the common case that the user wants no conversion.\n\nI think we should mimic the behavior of \"eol=crlf/lf\" attribute.\n\nAFAIK whenever I set \"*.ext text eol=crlf\", then I can be sure the \nfile is checked out with CRLF independent of any of my local config\nsettings. Isn't that correct? I would expect a similar behavior if\n\"*.ext text encoding=utf16\" is set. Wouldn't that mean that we do\nnot need a \"core.convertEncoding\" config?\n\nI do 100% agree that it must be a noop if there is no encoding \nattribute present.\n\n\n> - I doubt we'd want a \"core.autoEncoding\" similar to \"core.autocrlf\". I\n> don't think people consistently have all utf-16 files (the way they\n> might have all CRLF files) rather a few files that must be utf-16.\n\nAgreed!\n\n\n> - I have actually seen two types of utf-16 in git repos in the wild:\n> files which really must be utf-16 (because some tool demands it) and\n> files which happen to be utf-16, but could just as easily be utf-8\n> (and the user simply does not notice and commits utf-16, but doesn't\n> realize it until much later when their diffs are unreadable).\n> \n> For the first case, the \"encoding\" thing above would work fine. For\n> the second case, in theory we could have an option that takes any\n> file with a \"text\" attribute and no \"encoding\" attribute, and\n> converts it to utf-8.\n\nTBH I would label that a \"non-goal\". Because AFAIK we cannot reliability\ndetect the encoding automatically.\n\n\n> I suspect that's opening a can of worms for false positives similar\n> to core.autocrlf. And performance drops as we try to guess the\n> encoding and convert all incoming data.\n> \n> So I mention it mostly as a direction I think we probably _don't_\n> want to go. Anybody with the \"this could have been utf-8 all along\"\n> type of file can remedy it by converting and committing the result.\n\n100 % agree.\n\n\n> Omitting all of the \"we shouldn't do this\" bullet points, it seems\n> pretty simple and sane to me.\n> \n> There is one other approach, which is to really store utf-16 in the\n> repository and better teach the diff tools to handle it (which are\n> really the main thing in git that cares about looking into the blob\n> contents). You can do this already with a textconv filter, but:\n> \n> 1. It's slow (though cacheable).\n> \n> 2. It doesn't work unless each repo configures the filter (so not on\n> sites like GitHub, unless we define a micro-format that diff=utf16\n> should be textconv'd on display, and get all implementations to\n> respect that).\n\nActually, rendering diffs on Git hosting sites such as GitHub is one\nof my goals. Therefore, storing content as UTF-16 wouldn't be a solution\nfor me.\n\n\n> 3. Textconv patches look good, but can't be applied. This occasionally\n> makes things awkward, depending on your workflow.\n\nTBH I dont't understand what you mean here. What do you mean with\n\"textconv patches\"?\n\n\n> 4. You have to actually mark each file with an attribute, which is\n> slightly annoying and more thing to remember to do (I see this from\n> the server side, since people often commit utf-16 without any\n> attributes, and then get annoyed when they see the file marked as\n> binary).\n> \n> We've toyed with the idea at GitHub of auto-detecting UTF-16 BOMs and\n> doing an \"auto-textconv\" to utf-8 (for our human-readable diffs only, of\n> course). That solves (1), (2), and (4), but leaves (3). I actually\n> looked into using libicu to do it not just for UTF-16, but to detect any\n> encoding. It turned out to be really slow, though. :)\n> \n> So anyway, that is an alternate strategy, but I think I like \"canonical\n> in-repo text is utf-8\" approach a lot more, since then git operations\n> work consistently. There are still a few rough edges (e.g., I'm not sure\n> if you could apply a utf-8 patch directly to a utf-16 working tree file.\n> Certainly not using \"patch\", but I'm not sure how well \"git apply\" would\n> handle that case either). But I think it would mostly Just Work as long\n> as people were willing to set their encoding attributes.\n\nAgreed!\n\nThanks for your thoughts! I'll try to come up with a v1 of this idea.\n\n- Lars"},{"id":"334068","messageId":"20171204172340.GB13332@sigill.intra.peff.net","threadId":"47307","inReplyTo":"759F0C3A-8C46-4685-BB27-6D508B26BB49@gmail.com","subject":"Re: RFC: Native clean/smudge filter for UTF-16 files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-04T17:23:40Z","receivedAt":"2017-12-04T17:23:47Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Dec 03, 2017 at 07:48:01PM +0100, Lars Schneider wrote:\n\n> > - if core.convertEncoding is true, then for any file with an\n> > encoding=foo attribute, internally run iconv(foo, utf8) in\n> > convert_to_git(), and likewise iconv(utf8, foo) in\n> > convert_to_working_tree.\n> > \n> > - I'm not sure if core.convertEncoding should be enabled by default. If\n> > it's a noop as long as there's no encoding attribute, then it's\n> > probably fine. But I would not want accidental conversion or any\n> > slowdown for the common case that the user wants no conversion.\n> \n> I think we should mimic the behavior of \"eol=crlf/lf\" attribute.\n> \n> AFAIK whenever I set \"*.ext text eol=crlf\", then I can be sure the \n> file is checked out with CRLF independent of any of my local config\n> settings. Isn't that correct? I would expect a similar behavior if\n> \"*.ext text encoding=utf16\" is set. Wouldn't that mean that we do\n> not need a \"core.convertEncoding\" config?\n\nYeah, on further thought, that's probably the right thing. Both \"eol\"\nand \"encoding\" attributes are definite indications of what should happen\n(unlike \"text\", which is just saying you _could_ convert line endings if\nyou wished to, and therefore has to be used in conjunction with a config\nsetting).\n\nI like the name \"encoding\" for the attribute, but I do wonder if this\nwould bite anybody using it already for other purposes (like gitk).\n\n> > There is one other approach, which is to really store utf-16 in the\n> > repository and better teach the diff tools to handle it (which are\n> > really the main thing in git that cares about looking into the blob\n> > contents). You can do this already with a textconv filter, but:\n> > \n> > 1. It's slow (though cacheable).\n> > \n> > 2. It doesn't work unless each repo configures the filter (so not on\n> > sites like GitHub, unless we define a micro-format that diff=utf16\n> > should be textconv'd on display, and get all implementations to\n> > respect that).\n> \n> Actually, rendering diffs on Git hosting sites such as GitHub is one\n> of my goals. Therefore, storing content as UTF-16 wouldn't be a solution\n> for me.\n\nIf there were a convention for specifying the attribute, I think sites\nlike GitHub would start respecting it in the server-side diffs (though\nlike I said, we could also just auto-detect via BOM without even\nrequiring any attributes to be set up).\n\n> > 3. Textconv patches look good, but can't be applied. This occasionally\n> > makes things awkward, depending on your workflow.\n> \n> TBH I dont't understand what you mean here. What do you mean with\n> \"textconv patches\"?\n\nI mean the patch produced by \"git diff\" when textconv is in effect. That\npatch cannot be applied to the original content. E.g.:\n\n git init\n echo \"* diff=foo\" >.git/info/attributes\n git config diff.foo.textconv \"sed s/^/foo:/\"\n\n echo base >file\n git add file\n git commit -m base\n\n echo change >file\n git diff >patch\n\n git reset --hard\n git apply patch\n\nThat works in the absence of the textconv, but not with it. (For a real\nbinary file, you'd probably need \"diff --binary\" to produce a usable\npatch, but the principle is the same).\n\n-Peff\n"}]}