{"thread":{"id":"31154","subject":"[PATCH] macos: lazily initialize iconv","startedAt":"2012-07-31T17:52:11Z","lastAt":"2012-08-10T21:47:22Z","messageCount":15,"participants":["Junio C Hamano","Ralf Thielow","Linus Torvalds","Torsten Bögershausen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"196242","messageId":"7vk3xjked0.fsf@alter.siamese.dyndns.org","threadId":"31154","inReplyTo":null,"subject":"[PATCH] macos: lazily initialize iconv","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-31T17:52:11Z","receivedAt":"2012-07-31T17:52:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"In practice, the majority of paths do not have any utf8 character\nthat needs the canonicalization. Lazily call iconv_open() and\niconv_close() to avoid unnecessary overhead.\n\nSigned-off-by: Junio C Hamano \n---\n\n * This is not even compile tested, so it needs testing and\n benchmarking, as I do not even know how costly the calls to\n open/close are when we do not have to call iconv() itself.\n\n This was brought up by Linus (Cc'ed) in http://goo.gl/INWVc\n\n compat/precompose_utf8.c | 24 ++++++++++++++++++------\n compat/precompose_utf8.h | 1 +\n 2 files changed, 19 insertions(+), 6 deletions(-)\n\ndiff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c\nindex d40d1b3..63ce89f 100644\n--- a/compat/precompose_utf8.c\n+++ b/compat/precompose_utf8.c\n@@ -67,7 +67,7 @@ void probe_utf8_pathname_composition(char *path, int len)\n \n void precompose_argv(int argc, const char **argv)\n {\n-\tint i = 0;\n+\tint i;\n \tconst char *oldarg;\n \tchar *newarg;\n \ticonv_t ic_precompose;\n@@ -75,11 +75,19 @@ void precompose_argv(int argc, const char **argv)\n \tif (precomposed_unicode != 1)\n \t\treturn;\n \n+\t/* Avoid iconv_open()/iconv_close() if there is nothing to convert */\n+\tfor (i = 0; i < argc; i++) {\n+\t\tif (has_utf8(argv[i], (size_t)-1, NULL))\n+\t\t\tbreak;\n+\t}\n+\tif (i < argc)\n+\t\treturn;\n+\n \tic_precompose = iconv_open(repo_encoding, path_encoding);\n \tif (ic_precompose == (iconv_t) -1)\n \t\treturn;\n \n-\twhile (i < argc) {\n+\tfor (i = 0; i < argc; i++) {\n \t\tsize_t namelen;\n \t\toldarg = argv[i];\n \t\tif (has_utf8(oldarg, (size_t)-1, &namelen)) {\n@@ -87,7 +95,6 @@ void precompose_argv(int argc, const char **argv)\n \t\t\tif (newarg)\n \t\t\t\targv[i] = newarg;\n \t\t}\n-\t\ti++;\n \t}\n \ticonv_close(ic_precompose);\n }\n@@ -106,8 +113,7 @@ PREC_DIR *precompose_utf8_opendir(const char *dirname)\n \t\treturn NULL;\n \t} else {\n \t\tint ret_errno = errno;\n-\t\tprec_dir->ic_precompose = iconv_open(repo_encoding, path_encoding);\n-\t\t/* if iconv_open() fails, die() in readdir() if needed */\n+\t\tprec_dir->iconv_opened = 0;\n \t\terrno = ret_errno;\n \t}\n \n@@ -136,6 +142,11 @@ struct dirent_prec_psx *precompose_utf8_readdir(PREC_DIR *prec_dir)\n \t\tprec_dir->dirent_nfc->d_type = res->d_type;\n \n \t\tif ((precomposed_unicode == 1) && has_utf8(res->d_name, (size_t)-1, NULL)) {\n+\t\t\tif (!prec_dir->iconv_opened) {\n+\t\t\t\tprec_dir->ic_precompose =\n+\t\t\t\t\ticonv_open(repo_encoding, path_encoding);\n+\t\t\t\tprec_dir->iconv_opened = 1;\n+\t\t\t}\n \t\t\tif (prec_dir->ic_precompose == (iconv_t)-1) {\n \t\t\t\tdie(\"iconv_open(%s,%s) failed, but needed:\\n\"\n \t\t\t\t\t\t\" precomposed unicode is not supported.\\n\"\n@@ -181,7 +192,8 @@ int precompose_utf8_closedir(PREC_DIR *prec_dir)\n \tint ret_errno;\n \tret_value = closedir(prec_dir->dirp);\n \tret_errno = errno;\n-\tif (prec_dir->ic_precompose != (iconv_t)-1)\n+\tif (prec->dir->iconv_opened &&\n+\t (prec_dir->ic_precompose != (iconv_t)-1))\n \t\ticonv_close(prec_dir->ic_precompose);\n \tfree(prec_dir->dirent_nfc);\n \tfree(prec_dir);\ndiff --git a/compat/precompose_utf8.h b/compat/precompose_utf8.h\nindex 3b73585..8de485e 100644\n--- a/compat/precompose_utf8.h\n+++ b/compat/precompose_utf8.h\n@@ -22,6 +22,7 @@ typedef struct dirent_prec_psx {\n \n typedef struct {\n \ticonv_t ic_precompose;\n+\tint iconv_opened;\n \tDIR *dirp;\n \tstruct dirent_prec_psx *dirent_nfc;\n } PREC_DIR;\n-- \n1.7.12.rc1.43.g3fa3e7e\n"},{"id":"196244","messageId":"7va9yfkdo7.fsf@alter.siamese.dyndns.org","threadId":"31154","inReplyTo":"7vk3xjked0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] macos: lazily initialize iconv","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-31T18:07:04Z","receivedAt":"2012-07-31T18:07:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> In practice, the majority of paths do not have any utf8 character\n> that needs the canonicalization. Lazily call iconv_open() and\n> iconv_close() to avoid unnecessary overhead.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n>\n> * This is not even compile tested, so it needs testing and\n> benchmarking, as I do not even know how costly the calls to\n> open/close are when we do not have to call iconv() itself.\n>\n> This was brought up by Linus (Cc'ed) in http://goo.gl/INWVc\n\nEven though I also think that per-DIR iconv may not be the optimal\nway to organize this (I think iconv_t should be a per-thread thing\nat most), it would be a more involved change that needs to be done\nby somebody who actually works on Mac, so the patch I sent is kept\ndeliberately minimum.\n\nJust FYI.\n"},{"id":"196245","messageId":"CAN0XMOL6m0B0bLwdp9mviOLqYORGcnf9LMS5=+WkXhGinBrzgA@mail.gmail.com","threadId":"31154","inReplyTo":"7vk3xjked0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] macos: lazily initialize iconv","fromName":"Ralf Thielow","fromEmail":"ralf.thielow@gmail.com","sentAt":"2012-07-31T18:10:49Z","receivedAt":"2012-07-31T18:10:49Z","isPatch":true,"sender":{"key":"ralf.thielow@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1275832?v=4"},"body":"On Tue, Jul 31, 2012 at 7:52 PM, Junio C Hamano wrote:\n> + /* Avoid iconv_open()/iconv_close() if there is nothing to convert */\n> + for (i = 0; i < argc; i++) {\n> + if (has_utf8(argv[i], (size_t)-1, NULL))\n> + break;\n> + }\n> + if (i < argc)\n> + return;\n> +\n\nI'm not very familiar with this code but:\n\nbefore: it reencodes everything which is utf-8 in argv\nafter: it reencodes *nothing* if one string in argv is not in utf-8\n\nam i wrong?\n"},{"id":"196246","messageId":"7v6293kd2y.fsf@alter.siamese.dyndns.org","threadId":"31154","inReplyTo":"CAN0XMOL6m0B0bLwdp9mviOLqYORGcnf9LMS5=+WkXhGinBrzgA@mail.gmail.com","subject":"Re: [PATCH] macos: lazily initialize iconv","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-31T18:19:49Z","receivedAt":"2012-07-31T18:19:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ralf Thielow writes:\n\n> On Tue, Jul 31, 2012 at 7:52 PM, Junio C Hamano wrote:\n>> + /* Avoid iconv_open()/iconv_close() if there is nothing to convert */\n>> + for (i = 0; i < argc; i++) {\n>> + if (has_utf8(argv[i], (size_t)-1, NULL))\n>> + break;\n>> + }\n>> + if (i < argc)\n>> + return;\n>> +\n>\n> I'm not very familiar with this code but:\n>\n> before: it reencodes everything which is utf-8 in argv\n> after: it reencodes *nothing* if one string in argv is not in utf-8\n>\n> am i wrong?\n\nYou are right. It should avoid the whole iconv thing if we saw _no_\nutf8, i.e. the last two should be:\n\n\tif (argc <= i)\n \treturn;\n\nThanks.\n"},{"id":"196248","messageId":"7v1ujrkc9p.fsf@alter.siamese.dyndns.org","threadId":"31154","inReplyTo":"7vk3xjked0.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2] macos: lazily initialize iconv","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-31T18:37:22Z","receivedAt":"2012-07-31T18:37:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"In practice, the majority of paths do not have utf8 that needs\nthe canonicalization. Lazily call iconv_open()/iconv_close() to\navoid unnecessary overhead.\n\nSigned-off-by: Junio C Hamano \nHelped-by: Ralf Thielow \nHelped-by: Linus Torvalds \n---\n\n * This is not even compile tested, so it needs testing and\n benchmarking, as I do not even know how costly the calls to\n open/close are when we do not have to call iconv() itself.\n\n This reroll corrects an obvious thinko pointed out by Ralf, and\n gets rid of an extra iconv_opened field added unnecessarily in\n the previous round.\n\n This was brought up by Linus in http://goo.gl/INWVc\n\n compat/precompose_utf8.c | 19 ++++++++++++++-----\n 1 file changed, 14 insertions(+), 5 deletions(-)\n\ndiff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c\nindex d40d1b3..79b5528 100644\n--- a/compat/precompose_utf8.c\n+++ b/compat/precompose_utf8.c\n@@ -67,7 +67,7 @@ void probe_utf8_pathname_composition(char *path, int len)\n \n void precompose_argv(int argc, const char **argv)\n {\n-\tint i = 0;\n+\tint i;\n \tconst char *oldarg;\n \tchar *newarg;\n \ticonv_t ic_precompose;\n@@ -75,11 +75,19 @@ void precompose_argv(int argc, const char **argv)\n \tif (precomposed_unicode != 1)\n \t\treturn;\n \n+\t/* Avoid iconv_open()/iconv_close() if there is nothing to convert */\n+\tfor (i = 0; i < argc; i++) {\n+\t\tif (has_utf8(argv[i], (size_t)-1, NULL))\n+\t\t\tbreak;\n+\t}\n+\tif (argc <= i)\n+\t\treturn; /* no utf8 found */\n+\n \tic_precompose = iconv_open(repo_encoding, path_encoding);\n \tif (ic_precompose == (iconv_t) -1)\n \t\treturn;\n \n-\twhile (i < argc) {\n+\tfor (i = 0; i < argc; i++) {\n \t\tsize_t namelen;\n \t\toldarg = argv[i];\n \t\tif (has_utf8(oldarg, (size_t)-1, &namelen)) {\n@@ -87,7 +95,6 @@ void precompose_argv(int argc, const char **argv)\n \t\t\tif (newarg)\n \t\t\t\targv[i] = newarg;\n \t\t}\n-\t\ti++;\n \t}\n \ticonv_close(ic_precompose);\n }\n@@ -106,8 +113,7 @@ PREC_DIR *precompose_utf8_opendir(const char *dirname)\n \t\treturn NULL;\n \t} else {\n \t\tint ret_errno = errno;\n-\t\tprec_dir->ic_precompose = iconv_open(repo_encoding, path_encoding);\n-\t\t/* if iconv_open() fails, die() in readdir() if needed */\n+\t\tprec_dir->ic_precompose = (iconv_t)-1;\n \t\terrno = ret_errno;\n \t}\n \n@@ -136,6 +142,9 @@ struct dirent_prec_psx *precompose_utf8_readdir(PREC_DIR *prec_dir)\n \t\tprec_dir->dirent_nfc->d_type = res->d_type;\n \n \t\tif ((precomposed_unicode == 1) && has_utf8(res->d_name, (size_t)-1, NULL)) {\n+\t\t\tif (prec_dir->ic_precompose == (iconv_t)-1)\n+\t\t\t\tprec_dir->ic_precompose =\n+\t\t\t\t\ticonv_open(repo_encoding, path_encoding);\n \t\t\tif (prec_dir->ic_precompose == (iconv_t)-1) {\n \t\t\t\tdie(\"iconv_open(%s,%s) failed, but needed:\\n\"\n \t\t\t\t\t\t\" precomposed unicode is not supported.\\n\"\n-- \n1.7.12.rc1.43.g3fa3e7e\n"},{"id":"196249","messageId":"CAN0XMOKBPirJFzHM_QgDC8TGimoEhjWoAVA+JhaLzto4-SVvmw@mail.gmail.com","threadId":"31154","inReplyTo":"7v1ujrkc9p.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] macos: lazily initialize iconv","fromName":"Ralf Thielow","fromEmail":"ralf.thielow@gmail.com","sentAt":"2012-07-31T18:55:04Z","receivedAt":"2012-07-31T18:55:04Z","isPatch":true,"sender":{"key":"ralf.thielow@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1275832?v=4"},"body":"On Tue, Jul 31, 2012 at 8:37 PM, Junio C Hamano wrote:\n> + /* Avoid iconv_open()/iconv_close() if there is nothing to convert */\n> + for (i = 0; i < argc; i++) {\n> + if (has_utf8(argv[i], (size_t)-1, NULL))\n> + break;\n> + }\n> + if (argc <= i)\n> + return; /* no utf8 found */\n\nsorry, but \"argc\" can never be smaller than \"i\", right?\n"},{"id":"196250","messageId":"7vwr1jiwn8.fsf@alter.siamese.dyndns.org","threadId":"31154","inReplyTo":"CAN0XMOKBPirJFzHM_QgDC8TGimoEhjWoAVA+JhaLzto4-SVvmw@mail.gmail.com","subject":"Re: [PATCH v2] macos: lazily initialize iconv","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-31T19:00:11Z","receivedAt":"2012-07-31T19:00:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ralf Thielow writes:\n\n> On Tue, Jul 31, 2012 at 8:37 PM, Junio C Hamano wrote:\n>> + /* Avoid iconv_open()/iconv_close() if there is nothing to convert */\n>> + for (i = 0; i < argc; i++) {\n>> + if (has_utf8(argv[i], (size_t)-1, NULL))\n>> + break;\n>> + }\n>> + if (argc <= i)\n>> + return; /* no utf8 found */\n>\n> sorry, but \"argc\" can never be smaller than \"i\", right?\n\nYeah, but it is idiomatic to have an inverse of the exit condition\nof the preceding for loop here to catch an early exit, and writing\nit as \"if (i == argc)\", while technically correct, would break the\npattern.\n"},{"id":"196257","messageId":"CA+55aFwE93YeVjZp9VLhRvbxFJNonafmUE6rHzPer5hv-hON5Q@mail.gmail.com","threadId":"31154","inReplyTo":"7v1ujrkc9p.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] macos: lazily initialize iconv","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2012-07-31T19:51:30Z","receivedAt":"2012-07-31T19:51:30Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Tue, Jul 31, 2012 at 11:37 AM, Junio C Hamano wrote:\n>\n> * This is not even compile tested, so it needs testing and\n> benchmarking, as I do not even know how costly the calls to\n> open/close are when we do not have to call iconv() itself.\n\nOk, so it's easily compile-tested: just add\n\n+ COMPAT_OBJS += compat/precompose_utf8.o\n+ BASIC_CFLAGS += -DPRECOMPOSE_UNICODE\n\nto the makefile for Linux too.\n\nActually testing how well it *works* is hard, since I don't really\nhave a mac (well, I do, but it no longer has OS X on it ;), and the\nwhole \"utf-8-mac\" thing does not make sense.\n\nHOWEVER. I actually tested it with the conversion being from Latin1 to\nUTF-8 instead, and it does interesting things, and kind of works. I\nsay \"kind of\", because for the case of the filesystem being in Latin1,\nwe actually have to convert things back to the filesystem character\nset when doing \"open()\" and \"lstat()\", and this patch obviously\ndoesn't do that, because OS X does the conversion back to NFD on its\nown.\n\nBut ACK on the patch.\n\nIf I had more time, I'd actually be interested to do the generic case\nof namespace conversion, and we could make this a generic git feature\n- it's something I wanted to do long ago. However, right now I'm in\nthe merge window and will go on a vacation to Finland after that, so I\nprobably won't get around to it.\n\nI do have one suggestion: please rename the \"has_utf8()\" function to\n\"has_nonascii()\" too. Why? Because that's what the function actually\ndoes. It has nothing to do with testing for UTF-8 (the utf-8 rules are\nmore complex than just \"high bit set\"), and *if* I ever get around to\ndoing a more generic character set conversion for the filenames, the\ndecision really would be about non-ASCII, not about non-UTF8.\n\n Linus\n"},{"id":"196260","messageId":"7vk3xjit4h.fsf@alter.siamese.dyndns.org","threadId":"31154","inReplyTo":"CA+55aFwE93YeVjZp9VLhRvbxFJNonafmUE6rHzPer5hv-hON5Q@mail.gmail.com","subject":"Re: [PATCH v2] macos: lazily initialize iconv","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-31T20:16:14Z","receivedAt":"2012-07-31T20:16:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> On Tue, Jul 31, 2012 at 11:37 AM, Junio C Hamano wrote:\n>>\n>> * This is not even compile tested, so it needs testing and\n>> benchmarking, as I do not even know how costly the calls to\n>> open/close are when we do not have to call iconv() itself.\n>\n> Ok, so it's easily compile-tested: just add\n>\n> + COMPAT_OBJS += compat/precompose_utf8.o\n> + BASIC_CFLAGS += -DPRECOMPOSE_UNICODE\n>\n> to the makefile for Linux too.\n>\n> Actually testing how well it *works* is hard, since I don't really\n> have a mac (well, I do, but it no longer has OS X on it ;), and the\n> whole \"utf-8-mac\" thing does not make sense.\n\nAlso the motivation for this change (not the original utf-8-mac one,\nwhich is not my code) is about not paying unnecessary iconv_open()\noverhead when we do not have to, so the measurement has to happen on\nMac, not on Linux.\n\n> HOWEVER. I actually tested it with the conversion being from Latin1 to\n> UTF-8 instead, and it does interesting things, and kind of works. I\n> say \"kind of\", because for the case of the filesystem being in Latin1,\n> we actually have to convert things back to the filesystem character\n> set ...\n\nEek.\n\nNot just write_entry() codepath that creates the final paths on the\nfilesystem, you would need to touch lstat() calls that check the\nexistence and freshness of the path, once you go that route. I am\nsure such a change can be made to work, but I am not sure how much\nwe would gain from one.\n"},{"id":"196266","messageId":"CA+55aFzhgTsHKhL599k7M6NzD5WUm72v3V+NYuCKs4uCPbnFzg@mail.gmail.com","threadId":"31154","inReplyTo":"7vk3xjit4h.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] macos: lazily initialize iconv","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2012-07-31T20:39:44Z","receivedAt":"2012-07-31T20:39:44Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Tue, Jul 31, 2012 at 1:16 PM, Junio C Hamano wrote:\n>\n> Eek.\n\nOh, I agree. Doing a full character set conversion both ways is quite\na bit more work.\n\n> Not just write_entry() codepath that creates the final paths on the\n> filesystem, you would need to touch lstat() calls that check the\n> existence and freshness of the path, once you go that route. I am\n> sure such a change can be made to work, but I am not sure how much\n> we would gain from one.\n\nI think it might be interesting. I doubt it matters all that much any\nmore in Western Europe (Unicode really does seem to have largely taken\nover), but I think Japan still uses Shift-JIS a lot.\n\nAlthough maybe that is starting to fade too.\n\nAnd it really is just a generalization of the OS X filesystem damage.\n\n Linus\n"},{"id":"196299","messageId":"5019829F.7050606@web.de","threadId":"31154","inReplyTo":"CA+55aFzhgTsHKhL599k7M6NzD5WUm72v3V+NYuCKs4uCPbnFzg@mail.gmail.com","subject":"Re: [PATCH v2] macos: lazily initialize iconv","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2012-08-01T19:25:19Z","receivedAt":"2012-08-01T19:25:19Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"Am 2012-07-31 22:39, schrieb Linus Torvalds:\n> On Tue, Jul 31, 2012 at 1:16 PM, Junio C Hamano wrote:\n>>\n>> Eek.\n>\n> Oh, I agree. Doing a full character set conversion both ways is quite\n> a bit more work.\n>\n>> Not just write_entry() codepath that creates the final paths on the\n>> filesystem, you would need to touch lstat() calls that check the\n>> existence and freshness of the path, once you go that route. I am\n>> sure such a change can be made to work, but I am not sure how much\n>> we would gain from one.\n>\n> I think it might be interesting. I doubt it matters all that much any\n> more in Western Europe (Unicode really does seem to have largely taken\n> over), but I think Japan still uses Shift-JIS a lot.\n>\n> Although maybe that is starting to fade too.\n>\n> And it really is just a generalization of the OS X filesystem damage.\n>\n> Linus\n>\nHi,\n(I'm on vacation myself, so I migth apologize for answering very late.)\n\nI had done a fully fledges conversion back-and-force of file names.\n\nHaving e.g. ISO-8859-1 on disk and UTF-8 in the repo.\n\nThe basic idea came from Linus, and it needs conversion for\nopen() fopen(), unlink(), readdir(), stat(), lstat(), rename()\n(and may be one or two more, I even added support in readlink()).\n\nAfter doing the whole thing ready, I realized that UTF-8 became more and \nmore standard.\n\nAt the same time people started to enhance msysgit to use UTF-8 as well. \n(Thanks to the contributors)\n\nMy feeling was that the \"market window\" for such a generic file name \nconversion might be closed.\n\nSo, is there still a need for such a feature in git?\n\nI wouldn't mind to re-base my pathch to master and post it within a \ncouple of days/weeks.\n\nThanks for git and all enhancements.\n/Torsten\n"},{"id":"196808","messageId":"50253A1E.20706@web.de","threadId":"31154","inReplyTo":"7v1ujrkc9p.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] macos: lazily initialize iconv","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2012-08-10T16:43:10Z","receivedAt":"2012-08-10T16:43:10Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 31.07.12 20:37, Junio C Hamano wrote:\n> In practice, the majority of paths do not have utf8 that needs\n> the canonicalization. Lazily call iconv_open()/iconv_close() to\n> avoid unnecessary overhead.\n>\n> Signed-off-by: Junio C Hamano \n> Helped-by: Ralf Thielow \n> Helped-by: Linus Torvalds \n> ---\n>\n> * This is not even compile tested, so it needs testing and\n> benchmarking, as I do not even know how costly the calls to\n> open/close are when we do not have to call iconv() itself.\n>\n> This reroll corrects an obvious thinko pointed out by Ralf, and\n> gets rid of an extra iconv_opened field added unnecessarily in\n> the previous round.\n>\n> This was brought up by Linus in http://goo.gl/INWVc\n>\n> compat/precompose_utf8.c | 19 ++++++++++++++-----\n> 1 file changed, 14 insertions(+), 5 deletions(-)\n>\n> diff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c\n> index d40d1b3..79b5528 100644\n> --- a/compat/precompose_utf8.c\n> +++ b/compat/precompose_utf8.c\n> @@ -67,7 +67,7 @@ void probe_utf8_pathname_composition(char *path, int len)\n> \n> void precompose_argv(int argc, const char **argv)\n> {\n> -\tint i = 0;\n> +\tint i;\n> \tconst char *oldarg;\n> \tchar *newarg;\n> \ticonv_t ic_precompose;\n> @@ -75,11 +75,19 @@ void precompose_argv(int argc, const char **argv)\n> \tif (precomposed_unicode != 1)\n> \t\treturn;\n> \n> +\t/* Avoid iconv_open()/iconv_close() if there is nothing to convert */\n> +\tfor (i = 0; i < argc; i++) {\n> +\t\tif (has_utf8(argv[i], (size_t)-1, NULL))\n> +\t\t\tbreak;\n> +\t}\n> +\tif (argc <= i)\n> +\t\treturn; /* no utf8 found */\n> +\n> \tic_precompose = iconv_open(repo_encoding, path_encoding);\n> \tif (ic_precompose == (iconv_t) -1)\n> \t\treturn;\n> \n> -\twhile (i < argc) {\n> +\tfor (i = 0; i < argc; i++) {\n> \t\tsize_t namelen;\n> \t\toldarg = argv[i];\n> \t\tif (has_utf8(oldarg, (size_t)-1, &namelen)) {\n> @@ -87,7 +95,6 @@ void precompose_argv(int argc, const char **argv)\n> \t\t\tif (newarg)\n> \t\t\t\targv[i] = newarg;\n> \t\t}\n> -\t\ti++;\n> \t}\n> \ticonv_close(ic_precompose);\n> }\n> @@ -106,8 +113,7 @@ PREC_DIR *precompose_utf8_opendir(const char *dirname)\n> \t\treturn NULL;\n> \t} else {\n> \t\tint ret_errno = errno;\n> -\t\tprec_dir->ic_precompose = iconv_open(repo_encoding, path_encoding);\n> -\t\t/* if iconv_open() fails, die() in readdir() if needed */\n> +\t\tprec_dir->ic_precompose = (iconv_t)-1;\n> \t\terrno = ret_errno;\n> \t}\n> \n> @@ -136,6 +142,9 @@ struct dirent_prec_psx *precompose_utf8_readdir(PREC_DIR *prec_dir)\n> \t\tprec_dir->dirent_nfc->d_type = res->d_type;\n> \n> \t\tif ((precomposed_unicode == 1) && has_utf8(res->d_name, (size_t)-1, NULL)) {\n> +\t\t\tif (prec_dir->ic_precompose == (iconv_t)-1)\n> +\t\t\t\tprec_dir->ic_precompose =\n> +\t\t\t\t\ticonv_open(repo_encoding, path_encoding);\n> \t\t\tif (prec_dir->ic_precompose == (iconv_t)-1) {\n> \t\t\t\tdie(\"iconv_open(%s,%s) failed, but needed:\\n\"\n> \t\t\t\t\t\t\" precomposed unicode is not supported.\\n\"\nHi Junio,\n\nthanks for the optimization.\nTested-by: Torsten Bögershausen \n\nWe can optimize the optimization with another 0.01 % ;-)\n\ndiff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c\nindex 79b5528..93ae5de 100644\n--- a/compat/precompose_utf8.c\n+++ b/compat/precompose_utf8.c\n@@ -112,9 +112,7 @@ PREC_DIR *precompose_utf8_opendir(const char *dirname)\n free(prec_dir);\n return NULL;\n } else {\n- int ret_errno = errno;\n prec_dir->ic_precompose = (iconv_t)-1;\n- errno = ret_errno;\n }\n"},{"id":"196818","messageId":"7vsjbuk3v3.fsf@alter.siamese.dyndns.org","threadId":"31154","inReplyTo":"50253A1E.20706@web.de","subject":"Re: [PATCH v2] macos: lazily initialize iconv","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-10T18:18:24Z","receivedAt":"2012-08-10T18:18:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> On 31.07.12 20:37, Junio C Hamano wrote:\n>> In practice, the majority of paths do not have utf8 that needs\n>> the canonicalization. Lazily call iconv_open()/iconv_close() to\n>> avoid unnecessary overhead.\n>>\n>> Signed-off-by: Junio C Hamano \n>> Helped-by: Ralf Thielow \n>> Helped-by: Linus Torvalds \n>> ---\n>>\n>> * This is not even compile tested, so it needs testing and\n>> benchmarking, as I do not even know how costly the calls to\n>> open/close are when we do not have to call iconv() itself.\n>> ...\n> Hi Junio,\n>\n> thanks for the optimization.\n> Tested-by: Torsten Bögershausen \n\nWell, I didn't mean the correctness testing without numbers. The\ncorrectness of the patch after a couple of people eyeballed it was\nno longer a question.\n\nIf the patch does not give any measuable performance difference to\npeople who exercise this codepath, it is not worth merging. And\nthat is not something I can't do myself without a Mac (nor I wish to\nhave one to be able to do so myself).\n\nThanks.\n"},{"id":"196835","messageId":"50257906.1010606@web.de","threadId":"31154","inReplyTo":"7vsjbuk3v3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] macos: lazily initialize iconv","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2012-08-10T21:11:34Z","receivedAt":"2012-08-10T21:11:34Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 10.08.12 20:18, Junio C Hamano wrote:\n> Torsten Bögershausen writes:\n>\n>> On 31.07.12 20:37, Junio C Hamano wrote:\n>>> In practice, the majority of paths do not have utf8 that needs\n>>> the canonicalization. Lazily call iconv_open()/iconv_close() to\n>>> avoid unnecessary overhead.\n>>>\n>>> Signed-off-by: Junio C Hamano \n>>> Helped-by: Ralf Thielow \n>>> Helped-by: Linus Torvalds \n>>> ---\n>>>\n>>> * This is not even compile tested, so it needs testing and\n>>> benchmarking, as I do not even know how costly the calls to\n>>> open/close are when we do not have to call iconv() itself.\n>>> ...\n>> Hi Junio,\n>>\n>> thanks for the optimization.\n>> Tested-by: Torsten Bögershausen \n> Well, I didn't mean the correctness testing without numbers. The\n> correctness of the patch after a couple of people eyeballed it was\n> no longer a question.\n>\n> If the patch does not give any measuable performance difference to\n> people who exercise this codepath, it is not worth merging. And\n> that is not something I can't do myself without a Mac (nor I wish to\n> have one to be able to do so myself).\n>\n> Thanks.\nReally sorry for the confusion with the percentage, I should have written:\nThe patch is tested OK, and we can even remove 2 lines of code,\nwhere we save and restore errno.\nThey are no longer needed.\n\nHere some benchmarks on my system\n(Intel Core 2 Duo @ 3,06 GHz, L2-Cache: 3 MB, Memory: 4 GB)\nrunning git status on the linux source tree.\n\nAfter cloning, I set \"git config core.precomposeunicode true\".\n\ngit is v1.7.12.rc2,\ngit_git/git is with your patch.\n\ngit v1.7.12.rc2:\ntb@birne:~/projects/git/linux-2.6> for f in 1 2 3 4 5; do time git status ; done 2>&1 | egrep \"^user|^real|^sys\"\nreal 0m0.469s\nuser 0m0.283s\nsys 0m0.175s\n\nreal 0m0.461s\nuser 0m0.283s\nsys 0m0.170s\n\nreal 0m0.460s\nuser 0m0.283s\nsys 0m0.170s\n\nreal 0m0.461s\nuser 0m0.283s\nsys 0m0.177s\n\nreal 0m0.460s\nuser 0m0.283s\nsys 0m0.176s\n\nWith lazy init of iconv:\ntb@birne:~/projects/git/linux-2.6> for f in 1 2 3 4 5; do time ~/projects/git/git_git/git status ; done 2>&1 | egrep \"^user|^real|^sys\"\nreal 0m0.463s\nuser 0m0.282s\nsys 0m0.173s\n\nreal 0m0.460s\nuser 0m0.281s\nsys 0m0.172s\n\nreal 0m0.460s\nuser 0m0.281s\nsys 0m0.172s\n\nreal 0m0.463s\nuser 0m0.281s\nsys 0m0.175s\n\nreal 0m0.462s\nuser 0m0.281s\nsys 0m0.177s\n"},{"id":"196843","messageId":"7v7gt6ifmd.fsf@alter.siamese.dyndns.org","threadId":"31154","inReplyTo":"50257906.1010606@web.de","subject":"Re: [PATCH v2] macos: lazily initialize iconv","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-10T21:47:22Z","receivedAt":"2012-08-10T21:47:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> After cloning, I set \"git config core.precomposeunicode true\".\n>\n> git is v1.7.12.rc2,\n> git_git/git is with your patch.\n> \n> git v1.7.12.rc2:\n> tb@birne:~/projects/git/linux-2.6> for f in 1 2 3 4 5; do time git status ; done 2>&1 | egrep \"^user|^real|^sys\"\n> real 0m0.460s\n> user 0m0.283s\n> sys 0m0.176s\n>\n> With lazy init of iconv:\n> tb@birne:~/projects/git/linux-2.6> for f in 1 2 3 4 5; do time ~/projects/git/git_git/git status ; done 2>&1 | egrep \"^user|^real|^sys\"\n>\n> real 0m0.462s\n> user 0m0.281s\n> sys 0m0.177s\n\nThanks.\n\nSo in short, even in a tree that theoretically should benefit from\nlazy initialization, it does not make things better in any measuable\nway.\n"}]}