{"thread":{"id":"15756","subject":"Files with colons under Cygwin","startedAt":"2008-10-02T14:02:23Z","lastAt":"2008-10-13T06:18:01Z","messageCount":19,"participants":["Giovanni Funchal","Dmitry Potapov","Alex Riesen","Johannes Sixt","Joshua Juran"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"92158","messageId":"c475e2e60810020702q573570dcp31a5dc18bf98ef30@mail.gmail.com","threadId":"15756","inReplyTo":null,"subject":"Files with colons under Cygwin","fromName":"Giovanni Funchal","fromEmail":"gafunchal@gmail.com","sentAt":"2008-10-02T14:02:23Z","receivedAt":"2008-10-02T14:02:23Z","isPatch":false,"sender":{"key":"gafunchal@gmail.com","avatar":"https://gravatar.com/avatar/96f8068bdfb134f61682ec4a9741b71b582efa9d102055028a88643507644e9c?d=mp&s=160"},"body":"Hello,\n\nCygwin does not allow files with colons, I think this is Windows stuff\none just can't avoid. If you have files with colons in a git\nrepository and try pulling them on cygwin, the file is empty, its name\nis truncated and the status is wrong.\n\nlinux $ date > a:b\nlinux $ git init\nlinux $ git add a:b\nlinux $ git commit -m test\nlinux $ git push\ncygwin $ git pull\ncygwin $ ls\n-rw-r--r-- 1 funchal funchal 0 Oct 2 15:15 a\ncygwin $ git status\n# On branch master\n# Untracked files:\n# (use \"git add ...\" to include in what will be committed)\n#\n# a\nnothing added to commit but untracked files present (use \"git add\" to track)\n\nAny ideas on what should be done? (for instance, warn when pulling\nthis kind of files on Cygwin)\n\nHas anyone noticed this before?\n\nRegards,\n-- Giovanni\n"},{"id":"92312","messageId":"20081004233945.GM21650@dpotapov.dyndns.org","threadId":"15756","inReplyTo":"c475e2e60810020702q573570dcp31a5dc18bf98ef30@mail.gmail.com","subject":"Re: Files with colons under Cygwin","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2008-10-04T23:39:45Z","receivedAt":"2008-10-04T23:39:45Z","isPatch":false,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Thu, Oct 02, 2008 at 04:02:23PM +0200, Giovanni Funchal wrote:\n> \n> Cygwin does not allow files with colons, I think this is Windows stuff\n> one just can't avoid. \n\nAt least, you cannot use colon in Win32 API. They say Windows \"native\"\nAPI has less restrictions over what symbols are not allowed in file\nnames, but I guess it is still not allowed.\n\n> If you have files with colons in a git\n> repository and try pulling them on cygwin, the file is empty, its name\n> is truncated and the status is wrong.\n> \n> linux $ date > a:b\n> linux $ git init\n> linux $ git add a:b\n> linux $ git commit -m test\n> linux $ git push\n> cygwin $ git pull\n\nStrange... What version of Cygwin did you use? When I tried this with\nCygwin 1.5.25, I got the following error:\n\n error: git checkout-index: unable to create file a:b (No medium found)\n\nApparently, Git tried to create 'b' file on the drive 'a', and creating\nfiles outside of the working tree is not a very good thing to do from\nthe security point of view, as it can easily overwrite anything in\nc:/windows/.\n\nSo, here is a patch. It basically disallow backslashes and colons in\nfile names on Windows (whether it is MinGW or Cygwin).\n\nI wonder if the problem exists on Mac OS X too. From what I heard, it\ndoes not treat ':' as a normal symbol. But I have no access to Mac OS X,\nso here is a patch for Windows only.\n\n-- >8 --\nFrom: Dmitry Potapov \nDate: Sat, 4 Oct 2008 22:57:19 +0400\nSubject: [PATCH] correct verify_path for Windows\n\nColon and backslash in names may be used on Windows to overwrite files\noutside of the working directory.\n\nSigned-off-by: Dmitry Potapov \n---\n read-cache.c | 10 ++++++++++\n 1 files changed, 10 insertions(+), 0 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 901064b..972592e 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -701,6 +701,16 @@ inside:\n \t\t\t}\n \t\t\treturn 0;\n \t\t}\n+#if defined(_WIN32) || defined(__CYGWIN__)\n+\t\t/*\n+\t\t * There is a bunch of other characters that are not allowed\n+\t\t * in Win32 API, but the following two create a security hole\n+\t\t * by allowing to overwrite files outside of the working tree,\n+\t\t * therefore they are explicitly prohibited.\n+\t\t */\n+\t\telse if (c == ':' || c == '\\\\')\n+\t\t\treturn 0;\n+#endif\n \t\tc = *path++;\n \t}\n }\n-- \n1.6.0.2.445.g1198\n\n-- >8 --\n"},{"id":"92325","messageId":"81b0412b0810050204y6bd91ddbn250ddf9051738969@mail.gmail.com","threadId":"15756","inReplyTo":"20081004233945.GM21650@dpotapov.dyndns.org","subject":"Re: Files with colons under Cygwin","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-10-05T09:04:25Z","receivedAt":"2008-10-05T09:04:25Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2008/10/5 Dmitry Potapov :\n> On Thu, Oct 02, 2008 at 04:02:23PM +0200, Giovanni Funchal wrote:\n>>\n>> Cygwin does not allow files with colons, I think this is Windows stuff\n>> one just can't avoid.\n>\n> At least, you cannot use colon in Win32 API. They say Windows \"native\"\n> API has less restrictions over what symbols are not allowed in file\n> names, but I guess it is still not allowed.\n\nThe part after colon in the file name specifies the identifier of an\nalternative data stream (so there can be multiple data sets under one name).\nJust another microsoft stupidity no one uses and knows about.\n"},{"id":"92326","messageId":"81b0412b0810050214w15a25e3axfb8bf3ca05ffc215@mail.gmail.com","threadId":"15756","inReplyTo":"20081004233945.GM21650@dpotapov.dyndns.org","subject":"Re: Files with colons under Cygwin","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-10-05T09:14:00Z","receivedAt":"2008-10-05T09:14:00Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2008/10/5 Dmitry Potapov :\n> So, here is a patch. It basically disallow backslashes and colons in\n> file names on Windows (whether it is MinGW or Cygwin).\n\nWith this and sparse checkout patch combined it maybe possible\nto make Git work on these backward filesystems in a saner way:\njust never checkout the names which the filesystems cannot support\non disk and mark them correspondingly in the index. If at all needed,\nthe user can fallback to git-show and git-update-index to see the data\nand update them in repo. Assumption is that the cases are rare...\n"},{"id":"92328","messageId":"c475e2e60810050228g64a53842i7ecf8e61d37bf9bf@mail.gmail.com","threadId":"15756","inReplyTo":"20081004233945.GM21650@dpotapov.dyndns.org","subject":"Re: Files with colons under Cygwin","fromName":"Giovanni Funchal","fromEmail":"gafunchal@gmail.com","sentAt":"2008-10-05T09:28:03Z","receivedAt":"2008-10-05T09:28:03Z","isPatch":false,"sender":{"key":"gafunchal@gmail.com","avatar":"https://gravatar.com/avatar/96f8068bdfb134f61682ec4a9741b71b582efa9d102055028a88643507644e9c?d=mp&s=160"},"body":"> Strange... What version of Cygwin did you use? When I tried this with\n> Cygwin 1.5.25, I got the following error:\n>\n> error: git checkout-index: unable to create file a:b (No medium found)\n\nI'm on 1.5.25-15 on WinXP over a mounted network file system, but no\nerrors/warnings here...\n\nThanks for the clarifications and the patch,\n-- Giovanni\n\nOn Sun, Oct 5, 2008 at 1:39 AM, Dmitry Potapov wrote:\n> On Thu, Oct 02, 2008 at 04:02:23PM +0200, Giovanni Funchal wrote:\n>>\n>> Cygwin does not allow files with colons, I think this is Windows stuff\n>> one just can't avoid.\n>\n> At least, you cannot use colon in Win32 API. They say Windows \"native\"\n> API has less restrictions over what symbols are not allowed in file\n> names, but I guess it is still not allowed.\n>\n>> If you have files with colons in a git\n>> repository and try pulling them on cygwin, the file is empty, its name\n>> is truncated and the status is wrong.\n>>\n>> linux $ date > a:b\n>> linux $ git init\n>> linux $ git add a:b\n>> linux $ git commit -m test\n>> linux $ git push\n>> cygwin $ git pull\n>\n> Strange... What version of Cygwin did you use? When I tried this with\n> Cygwin 1.5.25, I got the following error:\n>\n> error: git checkout-index: unable to create file a:b (No medium found)\n>\n> Apparently, Git tried to create 'b' file on the drive 'a', and creating\n> files outside of the working tree is not a very good thing to do from\n> the security point of view, as it can easily overwrite anything in\n> c:/windows/.\n>\n> So, here is a patch. It basically disallow backslashes and colons in\n> file names on Windows (whether it is MinGW or Cygwin).\n>\n> I wonder if the problem exists on Mac OS X too. From what I heard, it\n> does not treat ':' as a normal symbol. But I have no access to Mac OS X,\n> so here is a patch for Windows only.\n>\n> -- >8 --\n> From: Dmitry Potapov \n> Date: Sat, 4 Oct 2008 22:57:19 +0400\n> Subject: [PATCH] correct verify_path for Windows\n>\n> Colon and backslash in names may be used on Windows to overwrite files\n> outside of the working directory.\n>\n> Signed-off-by: Dmitry Potapov \n> ---\n> read-cache.c | 10 ++++++++++\n> 1 files changed, 10 insertions(+), 0 deletions(-)\n>\n> diff --git a/read-cache.c b/read-cache.c\n> index 901064b..972592e 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -701,6 +701,16 @@ inside:\n> }\n> return 0;\n> }\n> +#if defined(_WIN32) || defined(__CYGWIN__)\n> + /*\n> + * There is a bunch of other characters that are not allowed\n> + * in Win32 API, but the following two create a security hole\n> + * by allowing to overwrite files outside of the working tree,\n> + * therefore they are explicitly prohibited.\n> + */\n> + else if (c == ':' || c == '\\\\')\n> + return 0;\n> +#endif\n> c = *path++;\n> }\n> }\n> --\n> 1.6.0.2.445.g1198\n>\n> -- >8 --\n>\n"},{"id":"92356","messageId":"20081005195129.GP21650@dpotapov.dyndns.org","threadId":"15756","inReplyTo":"81b0412b0810050214w15a25e3axfb8bf3ca05ffc215@mail.gmail.com","subject":"Re: Files with colons under Cygwin","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2008-10-05T19:51:30Z","receivedAt":"2008-10-05T19:51:30Z","isPatch":false,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Sun, Oct 05, 2008 at 11:14:00AM +0200, Alex Riesen wrote:\n> 2008/10/5 Dmitry Potapov :\n> > So, here is a patch. It basically disallow backslashes and colons in\n> > file names on Windows (whether it is MinGW or Cygwin).\n> \n> With this and sparse checkout patch combined it maybe possible\n> to make Git work on these backward filesystems in a saner way:\n> just never checkout the names which the filesystems cannot support\n> on disk and mark them correspondingly in the index.\n\nPerhaps, using sparse checkout is a good idea for dealing with prohibit\ncharacters, but the goal of my patch was a bit different -- to close a\nsecurity hole in checkout when files outside of the working directory\ncan be overwritten. In fact, the whole point of having verify_path() is\nto prevent this from happening, and if it does not work properly on\nWindows then this function should be corrected.\n\nDmitry\n"},{"id":"92389","messageId":"48E9B634.6040909@viscovery.net","threadId":"15756","inReplyTo":"20081004233945.GM21650@dpotapov.dyndns.org","subject":"Re: Files with colons under Cygwin","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-10-06T06:54:44Z","receivedAt":"2008-10-06T06:54:44Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Dmitry Potapov schrieb:\n> Subject: [PATCH] correct verify_path for Windows\n> \n> Colon and backslash in names may be used on Windows to overwrite files\n> outside of the working directory.\n> \n> Signed-off-by: Dmitry Potapov \n> ---\n> read-cache.c | 10 ++++++++++\n> 1 files changed, 10 insertions(+), 0 deletions(-)\n> \n> diff --git a/read-cache.c b/read-cache.c\n> index 901064b..972592e 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -701,6 +701,16 @@ inside:\n> \t\t\t}\n> \t\t\treturn 0;\n> \t\t}\n> +#if defined(_WIN32) || defined(__CYGWIN__)\n> +\t\t/*\n> +\t\t * There is a bunch of other characters that are not allowed\n> +\t\t * in Win32 API, but the following two create a security hole\n> +\t\t * by allowing to overwrite files outside of the working tree,\n> +\t\t * therefore they are explicitly prohibited.\n> +\t\t */\n> +\t\telse if (c == ':' || c == '\\\\')\n> +\t\t\treturn 0;\n> +#endif\n> \t\tc = *path++;\n> \t}\n> }\n\nIIUC, verify_path() checks paths that were found in the database or the\nindex. As such, it checks for the integrity of the database. And paths\nwith backslashes or colons certainly do not violate the database integrity.\n\nMore precisely, the exchange of path names between the index and tree\nobjects (both directions) should not do this new check, nor if a path is\nadded to the index. The check is only meaningful[*] when a path is read\nfrom the index or a tree object and \"applied\" to the working directory.\nUnfortunately, I think there are lots of places where this happens.\n\n[*] I say \"meaningful\" and not \"necessary\" because the situation is just\nlike when you grab some random SoftwarePackage.tar.gz, and run ./configure\nwithout looking first what it is going to do.\n\n-- Hannes\n"},{"id":"92470","messageId":"20081007005327.GT21650@dpotapov.dyndns.org","threadId":"15756","inReplyTo":"48E9B634.6040909@viscovery.net","subject":"Re: Files with colons under Cygwin","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2008-10-07T00:53:27Z","receivedAt":"2008-10-07T00:53:27Z","isPatch":false,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Mon, Oct 06, 2008 at 08:54:44AM +0200, Johannes Sixt wrote:\n> \n> IIUC, verify_path() checks paths that were found in the database or the\n> index.\n\nIt also checks paths that was given to git add and other commands to\nprevent an invalid path to enter to the index. If I am not mistaken,\nif invalid path has entered in the index then it will be committed to\nthe database without any further checks.\n\n> As such, it checks for the integrity of the database. And paths\n> with backslashes or colons certainly do not violate the database integrity.\n\nNo, it has nothing to do with the database. You can run git fsck --full\non a repository that contains '..' or '.' or '.git', and there will be\nno error. Having those names does not violate the database integrity,\nas the database is concerned all names are just bytes separated by '/',\nso having name '.' is not a problem for it. However, names '.' and '..'\nhave special meaning for the filesystem, and paths starting with .git/\nhave special meaning for Git repository. If you work in a bare repo\nthen those names are not a problem, but once you have a working tree,\nyou want make sure that there is nothing wrong with it.\n\n> \n> More precisely, the exchange of path names between the index and tree\n> objects (both directions) should not do this new check, nor if a path is\n> added to the index. The check is only meaningful[*] when a path is read\n> from the index or a tree object and \"applied\" to the working directory.\n> Unfortunately, I think there are lots of places where this happens.\n> \n> [*] I say \"meaningful\" and not \"necessary\" because the situation is just\n> like when you grab some random SoftwarePackage.tar.gz, and run ./configure\n> without looking first what it is going to do.\n\nWhen I grab any tar, I can look at its context without myself of any\nrisk that some files can be overwritten on my file system. And when\nI want to look at some remote git repository, I usually do:\n\n git clone URL\n\nIf it can overwrite some files behind my back, it is security a hole.\n\nBTW, it was the reason why the idea of allowing .gitconfig to be stored\nin git repository (similar to .gitignore) was stroke down about a year ago\nthough it could help some clueless users...\n\nOn Linux (or other sane file systems), we have all required checks to\nprevent that from happening, and they are places in verify_path, which\nprevents malicious names entering into the index and thus to the file\nsystem too. So, we should do all required checks on Windows too.\n\nNow, I've realized that my checks were not sufficient (due to Windows\nbeing case-insensitive), so I will add more checks and resend this\npatch later.\n\n\nDmitry\n"},{"id":"92475","messageId":"B985AE98-F6E2-4C23-8D34-5A22A9F89FA7@gmail.com","threadId":"15756","inReplyTo":"20081004233945.GM21650@dpotapov.dyndns.org","subject":"Re: Files with colons under Cygwin","fromName":"Joshua Juran","fromEmail":"jjuran@gmail.com","sentAt":"2008-10-07T02:05:58Z","receivedAt":"2008-10-07T02:05:58Z","isPatch":false,"sender":{"key":"jjuran@gmail.com","avatar":null},"body":"On Oct 4, 2008, at 4:39 PM, Dmitry Potapov wrote:\n\n> On Thu, Oct 02, 2008 at 04:02:23PM +0200, Giovanni Funchal wrote:\n>>\n>> Cygwin does not allow files with colons, I think this is Windows \n>> stuff\n>> one just can't avoid.\n>\n> I wonder if the problem exists on Mac OS X too. From what I heard, it\n> does not treat ':' as a normal symbol.\n\nThe short answer is that Mac OS X's POSIX implementation works as \nexpected (internally replacing ':' with '/') without issue.\n\nFurthermore, my POSIX-like environment Lamp (Lamp ain't Mac POSIX) \nthat runs on classic Mac OS (much in the manner of Cygwin) provides \nthe same Unix filing behavior, so Mac filing syntax isn't an issue in \nrunning git on Mac OS 9. (I'm sure everybody's breathing a HUGE sigh \nof relief at this news...)\n\nJosh\n"},{"id":"92482","messageId":"20081007032623.GX21650@dpotapov.dyndns.org","threadId":"15756","inReplyTo":"B985AE98-F6E2-4C23-8D34-5A22A9F89FA7@gmail.com","subject":"[PATCH v2] correct verify_path for Windows","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2008-10-07T03:26:23Z","receivedAt":"2008-10-07T03:26:23Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"Colon and backslash in names may be used on Windows to overwrite files\noutside of the working directory. Due to the file-system being case-\ninsensitive, .git can be written as any combination of upper and lower\ncharacters, so we should check that too.\n\nSigned-off-by: Dmitry Potapov \n---\nIn this version, I have added the check that files in .git/ will not\nbe overwritten by checkout. Overwriting such files as .git/config is\npotentially exploitable.\n\nJosh,\n\nDoes OS X need the same check below? I believe it has case-insensitive\nfilesystem, so it needs that too, but I am not sure what is the right\ndefine should be used.\n\nThanks,\nDmitry\n\n read-cache.c | 19 +++++++++++++++++++\n 1 files changed, 19 insertions(+), 0 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex aff6390..7f855ee 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -668,10 +668,19 @@ static int verify_dotfile(const char *rest)\n \t * shares the path end test with the \"..\" case.\n \t */\n \tcase 'g':\n+#if defined(_WIN32) || defined(__CYGWIN__)\n+\t/* On Windows, file names are case-insensitive */\n+\tcase 'G':\n+\t\tif ((rest[1]|0x20) != 'i')\n+\t\t\tbreak;\n+\t\tif ((rest[2]|0x20) != 't')\n+\t\t\tbreak;\n+#else\n \t\tif (rest[1] != 'i')\n \t\t\tbreak;\n \t\tif (rest[2] != 't')\n \t\t\tbreak;\n+#endif\n \t\trest += 2;\n \t/* fallthrough */\n \tcase '.':\n@@ -703,6 +712,16 @@ inside:\n \t\t\t}\n \t\t\treturn 0;\n \t\t}\n+#if defined(_WIN32) || defined(__CYGWIN__)\n+\t\t/*\n+\t\t * There is a bunch of other characters that are not allowed\n+\t\t * in Win32 API, but the following two create a security hole\n+\t\t * by allowing to overwrite files outside of the working tree,\n+\t\t * therefore they are explicitly prohibited.\n+\t\t */\n+\t\telse if (c == ':' || c == '\\\\')\n+\t\t\treturn 0;\n+#endif\n \t\tc = *path++;\n \t}\n }\n-- \n1.6.0\n"},{"id":"92491","messageId":"48EAFE00.3040907@viscovery.net","threadId":"15756","inReplyTo":"20081007005327.GT21650@dpotapov.dyndns.org","subject":"Re: Files with colons under Cygwin","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-10-07T06:13:20Z","receivedAt":"2008-10-07T06:13:20Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Dmitry Potapov schrieb:\n> On Mon, Oct 06, 2008 at 08:54:44AM +0200, Johannes Sixt wrote:\n>> [*] I say \"meaningful\" and not \"necessary\" because the situation is just\n>> like when you grab some random SoftwarePackage.tar.gz, and run ./configure\n>> without looking first what it is going to do.\n> \n> When I grab any tar, I can look at its context without myself of any\n> risk that some files can be overwritten on my file system. And when\n> I want to look at some remote git repository, I usually do:\n> \n> git clone URL\n> \n> If it can overwrite some files behind my back, it is security a hole.\n\nFair enough.\n\n> On Linux (or other sane file systems), we have all required checks to\n> prevent that from happening, and they are places in verify_path, which\n> prevents malicious names entering into the index and thus to the file\n> system too. So, we should do all required checks on Windows too.\n\nI don't object the intention of your patch. But I cannot judge whether\nverify_path() is the correct location to put the checks because I don't\nknow this part of the code. I leave the final word to others.\n\n-- Hannes\n"},{"id":"92492","messageId":"48EAFF23.1020607@viscovery.net","threadId":"15756","inReplyTo":"20081007032623.GX21650@dpotapov.dyndns.org","subject":"Re: [PATCH v2] correct verify_path for Windows","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-10-07T06:18:11Z","receivedAt":"2008-10-07T06:18:11Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Dmitry Potapov schrieb:\n> +#if defined(_WIN32) || defined(__CYGWIN__)\n\nI think that for consistency you should use __MINGW32__ instead of _WIN32.\n\n> +\t/* On Windows, file names are case-insensitive */\n> +\tcase 'G':\n> +\t\tif ((rest[1]|0x20) != 'i')\n> +\t\t\tbreak;\n> +\t\tif ((rest[2]|0x20) != 't')\n> +\t\t\tbreak;\n\nWe have tolower().\n\n-- Hannes\n"},{"id":"92495","messageId":"81b0412b0810062325o818c2f0g65de4f7811d5b8ee@mail.gmail.com","threadId":"15756","inReplyTo":"20081007032623.GX21650@dpotapov.dyndns.org","subject":"Re: [PATCH v2] correct verify_path for Windows","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-10-07T06:25:05Z","receivedAt":"2008-10-07T06:25:05Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2008/10/7 Dmitry Potapov :\n> +#if defined(_WIN32) || defined(__CYGWIN__)\n> + /* On Windows, file names are case-insensitive */\n> + case 'G':\n> + if ((rest[1]|0x20) != 'i')\n> + break;\n> + if ((rest[2]|0x20) != 't')\n> + break;\n> +#else\n\nMaybe it is already time for FILESYSTEM_CASEINSENSITIVE?\n"},{"id":"92800","messageId":"20081011163310.GZ21650@dpotapov.dyndns.org","threadId":"15756","inReplyTo":"48EAFF23.1020607@viscovery.net","subject":"Re: [PATCH v2] correct verify_path for Windows","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2008-10-11T16:33:10Z","receivedAt":"2008-10-11T16:33:10Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Tue, Oct 07, 2008 at 08:18:11AM +0200, Johannes Sixt wrote:\n> Dmitry Potapov schrieb:\n> > +#if defined(_WIN32) || defined(__CYGWIN__)\n> \n> I think that for consistency you should use __MINGW32__ instead of _WIN32.\n\nI like Alex's suggestion more to use FILESYSTEM_CASEINSENSITIVE.\n\n> \n> > +\t/* On Windows, file names are case-insensitive */\n> > +\tcase 'G':\n> > +\t\tif ((rest[1]|0x20) != 'i')\n> > +\t\t\tbreak;\n> > +\t\tif ((rest[2]|0x20) != 't')\n> > +\t\t\tbreak;\n> \n> We have tolower().\n\nI am aware of that, but I am not sure what we gain by using it. It seems\nit makes only code bigger and slow. As to readability, I don't see much\nimprovement... Isn't obvious what this code does, especially with the\nabove comment?\n\nDmitry\n"},{"id":"92816","messageId":"81b0412b0810111558vb69be00if4842fa91d777c3b@mail.gmail.com","threadId":"15756","inReplyTo":"20081011163310.GZ21650@dpotapov.dyndns.org","subject":"Re: [PATCH v2] correct verify_path for Windows","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-10-11T22:58:52Z","receivedAt":"2008-10-11T22:58:52Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2008/10/11 Dmitry Potapov :\n>> > + /* On Windows, file names are case-insensitive */\n>> > + case 'G':\n>> > + if ((rest[1]|0x20) != 'i')\n>> > + break;\n>> > + if ((rest[2]|0x20) != 't')\n>> > + break;\n>>\n>> We have tolower().\n>\n> I am aware of that, but I am not sure what we gain by using it. It seems\n> it makes only code bigger and slow.\n\nIt does? Care to look into git-compat-util.h?\n\n> ... As to readability, I don't see much\n> improvement... Isn't obvious what this code does, especially with the\n> above comment?\n\nYou want to seriously argue that \"a | 0x20\" is as readable as \"tolower(a)\"?\nFor the years to come? With a person who does not even know what ASCII is?\nOk, I'm exaggerating. But the point is: it is not us who will be\nreading the code.\nAnd even if they do this just to remove Windows quirks it is well worth to\nuse a bit more of english language so that they don't need a second look.\nAs to comment: it is just additional info. It can't be checked by compiler\nif you make and accidental typo in your code (like, for example, accidentally\nputting an extra pipe in that expression, should happen to that emacs users\nfrom time to time).\n\nBTW, is it such a critical path? Can't the code be unified and do\nwithout #ifdef?\n"},{"id":"92839","messageId":"20081012135048.GC21650@dpotapov.dyndns.org","threadId":"15756","inReplyTo":"81b0412b0810111558vb69be00if4842fa91d777c3b@mail.gmail.com","subject":"Re: [PATCH v2] correct verify_path for Windows","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2008-10-12T13:50:48Z","receivedAt":"2008-10-12T13:50:48Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Sun, Oct 12, 2008 at 12:58:52AM +0200, Alex Riesen wrote:\n> 2008/10/11 Dmitry Potapov :\n> >> > + /* On Windows, file names are case-insensitive */\n> >> > + case 'G':\n> >> > + if ((rest[1]|0x20) != 'i')\n> >> > + break;\n> >> > + if ((rest[2]|0x20) != 't')\n> >> > + break;\n> >>\n> >> We have tolower().\n> >\n> > I am aware of that, but I am not sure what we gain by using it. It seems\n> > it makes only code bigger and slow.\n> \n> It does? Care to look into git-compat-util.h?\n\nAs a matter of fact, I did, and I see the following:\n\n #define sane_istest(x,mask) ((sane_ctype[(unsigned char)(x)] & (mask)) != 0)\n #define tolower(x) sane_case((unsigned char)(x), 0x20)\n\n static inline int sane_case(int x, int high)\n {\n \tif (sane_istest(x, GIT_ALPHA))\n \t\tx = (x & ~0x20) | high;\n \treturn x;\n }\n\nSo, it looks like an extra look up and an extra comparison here.\n\n> \n> > ... As to readability, I don't see much\n> > improvement... Isn't obvious what this code does, especially with the\n> > above comment?\n> \n> You want to seriously argue that \"a | 0x20\" is as readable as \"tolower(a)\"?\n> For the years to come? With a person who does not even know what ASCII is?\n> Ok, I'm exaggerating. But the point is: it is not us who will be\n> reading the code.\n\nObviously, for a person who don't know what ASCII is, tolower() will be\nmuch easier to understand, but the question is what I can reasonable to\nexpect for a person reading this code later. A similar argument can be\nmade about adding extra parenthesis, i.e. instead of writing\n if (a == b || c == d)\nyou should always write\n if ((a == b) || (c == d))\nbecause some people do not remember the priority of each operator.\n(And I have seen such programmers who claim to have many experience of\nwriting in C, yet, they do not remember operator priority.)\n\nFor me, using tolower() does not make it more readable, but maybe I am\ntoo old-fashion assuming that people are supposed to know at least basic\nthings about ASCII.\n\n> BTW, is it such a critical path?\n\nI am not sure whether it is critical or not. It is called for each\nname in path. So, if you have a long path, it may be called quite a\nfew times per a single path. Also, some operation such 'git add' can\ncall verify_path() more than once (IIRC, it was called thrice per each\nadded file). But I have no numbers to tell whether it is noticeable or\nnot.\n\n> Can't the code be unified and do without #ifdef?\n\nIt will impose a extra restriction on what file names people can use,\nand I don't like extra restrictions for those who use sane file systems.\n\n\nDmitry\n"},{"id":"92862","messageId":"20081012181836.GA10626@steel.home","threadId":"15756","inReplyTo":"20081012135048.GC21650@dpotapov.dyndns.org","subject":"Re: [PATCH v2] correct verify_path for Windows","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-10-12T18:18:36Z","receivedAt":"2008-10-12T18:18:36Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Dmitry Potapov, Sun, Oct 12, 2008 15:50:48 +0200:\n> On Sun, Oct 12, 2008 at 12:58:52AM +0200, Alex Riesen wrote:\n> > 2008/10/11 Dmitry Potapov :\n> > >> > + /* On Windows, file names are case-insensitive */\n> > >> > + case 'G':\n> > >> > + if ((rest[1]|0x20) != 'i')\n> > >> > + break;\n> > >> > + if ((rest[2]|0x20) != 't')\n> > >> > + break;\n> > >>\n> > >> We have tolower().\n> > >\n> > > I am aware of that, but I am not sure what we gain by using it. It seems\n> > > it makes only code bigger and slow.\n> > \n> > It does? Care to look into git-compat-util.h?\n> \n> As a matter of fact, I did, and I see the following:\n> \n> #define sane_istest(x,mask) ((sane_ctype[(unsigned char)(x)] & (mask)) != 0)\n> #define tolower(x) sane_case((unsigned char)(x), 0x20)\n> \n> static inline int sane_case(int x, int high)\n> {\n> \tif (sane_istest(x, GIT_ALPHA))\n> \t\tx = (x & ~0x20) | high;\n> \treturn x;\n> }\n> \n> So, it looks like an extra look up and an extra comparison here.\n\nDoes not look like much more code. But:\n\n> > BTW, is it such a critical path?\n> \n> I am not sure whether it is critical or not. It is called for each\n> name in path. So, if you have a long path, it may be called quite a\n> few times per a single path. Also, some operation such 'git add' can\n> call verify_path() more than once (IIRC, it was called thrice per each\n> added file). But I have no numbers to tell whether it is noticeable or\n> not.\n\nI looked at the callers (briefly). Performance could be a problem: add\nand checkout can work with real big file lists and long pathnames.\nSo ok, than. It is critical.\n"},{"id":"92902","messageId":"48F2E410.2080504@viscovery.net","threadId":"15756","inReplyTo":"20081012181836.GA10626@steel.home","subject":"Re: [PATCH v2] correct verify_path for Windows","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-10-13T06:00:48Z","receivedAt":"2008-10-13T06:00:48Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Alex Riesen schrieb:\n> I looked at the callers (briefly). Performance could be a problem: add\n> and checkout can work with real big file lists and long pathnames.\n> So ok, than. It is critical.\n\nYou are kidding, aren't you? What you win by a few CPU instructions here\nis dwarfed by the time that the stat() implementation requires. Dmitry,\nplease use the more readable tolower().\n\n-- Hannes\n"},{"id":"92905","messageId":"81b0412b0810122318h15e8f5bue9b8ee8da71a7c33@mail.gmail.com","threadId":"15756","inReplyTo":"48F2E410.2080504@viscovery.net","subject":"Re: [PATCH v2] correct verify_path for Windows","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-10-13T06:18:01Z","receivedAt":"2008-10-13T06:18:01Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2008/10/13 Johannes Sixt :\n> Alex Riesen schrieb:\n>> I looked at the callers (briefly). Performance could be a problem: add\n>> and checkout can work with real big file lists and long pathnames.\n>> So ok, than. It is critical.\n>\n> You are kidding, aren't you? What you win by a few CPU instructions here\n> is dwarfed by the time that the stat() implementation requires. Dmitry,\n> please use the more readable tolower().\n\nIt is called for every element in a path, not just for every filename.\nAnd maybe it is dwarfed, but they both are part of the same operation.\nAnd this code makes it more CPU intensive.\n"}]}