{"thread":{"id":"40440","subject":"[PATCH] Fix \"inside work tree\" detection on case-insensitive filesystems","startedAt":"2015-09-28T16:12:18Z","lastAt":"2016-01-01T16:24:13Z","messageCount":4,"participants":["Johannes Schindelin","Michael Haggerty"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"270817","messageId":"ac9733a6b922572ec10f09f89e07cde37ba43f13.1443456630.git.johannes.schindelin@gmx.de","threadId":"40440","inReplyTo":null,"subject":"[PATCH] Fix \"inside work tree\" detection on case-insensitive filesystems","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-09-28T16:12:18Z","receivedAt":"2015-09-28T16:12:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Git has a config variable to indicate that it is operating on a file\nsystem that is case-insensitive: core.ignoreCase. But the\n`dir_inside_of()` function did not respect that. As a result, if Git's\nidea of the current working directory disagreed in its upper/lower case\nwith the `GIT_WORK_TREE` variable (e.g. `C:\\test` vs `c:\\test`) the\nuser would be greeted by the error message\n\n\tfatal: git-am cannot be used without a working tree.\n\nwhen trying to run a rebase.\n\nThis fixes https://github.com/git-for-windows/git/issues/402 (reported by\nDaniel Harding).\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n dir.c | 11 ++++++++++-\n 1 file changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/dir.c b/dir.c\nindex b90484a..fba938b 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2107,6 +2107,15 @@ int file_exists(const char *f)\n \treturn lstat(f, &sb) == 0;\n }\n \n+static int cmp_icase(char a, char b)\n+{\n+\tif (a == b)\n+\t\treturn 0;\n+\tif (ignore_case)\n+\t\treturn toupper(a) - toupper(b);\n+\treturn a - b;\n+}\n+\n /*\n  * Given two normalized paths (a trailing slash is ok), if subdir is\n  * outside dir, return -1.  Otherwise return the offset in subdir that\n@@ -2118,7 +2127,7 @@ int dir_inside_of(const char *subdir, const char *dir)\n \n \tassert(dir && subdir && *dir && *subdir);\n \n-\twhile (*dir && *subdir && *dir == *subdir) {\n+\twhile (*dir && *subdir && !cmp_icase(*dir, *subdir)) {\n \t\tdir++;\n \t\tsubdir++;\n \t\toffset++;\n-- \n2.5.3.windows.1.3.gc322723\n"},{"id":"275136","messageId":"56829D08.1060107@alum.mit.edu","threadId":"40440","inReplyTo":"ac9733a6b922572ec10f09f89e07cde37ba43f13.1443456630.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] Fix \"inside work tree\" detection on case-insensitive filesystems","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-12-29T14:47:36Z","receivedAt":"2015-12-29T14:47:36Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/28/2015 06:12 PM, Johannes Schindelin wrote:\n> Git has a config variable to indicate that it is operating on a file\n> system that is case-insensitive: core.ignoreCase. But the\n> `dir_inside_of()` function did not respect that. As a result, if Git's\n> idea of the current working directory disagreed in its upper/lower case\n> with the `GIT_WORK_TREE` variable (e.g. `C:\\test` vs `c:\\test`) the\n> user would be greeted by the error message\n> \n> \tfatal: git-am cannot be used without a working tree.\n> \n> when trying to run a rebase.\n> \n> This fixes https://github.com/git-for-windows/git/issues/402 (reported by\n> Daniel Harding).\n\nI was just going through the 2.7 release notes when I saw this patch. My\nunderstanding was that many of the case-insensitive filesystems also\nsupport Unicode. Is the byte-oriented code in this patch adequate? I\nwould have thought it necessary to use a Unicode-aware algorithm here,\nthat knows:\n\n* that bytes != characters\n* how to do a case-insensitive comparison of strings that include\nnon-ASCII characters\n* (possibly) insensitivity to NFC vs. NFD vs. non-normalized forms\n\nI suppose that such OSs have built-in functions for deciding whether two\npaths are equivalent. Possibly these could be used?\n\nMichael\n\n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> ---\n>  dir.c | 11 ++++++++++-\n>  1 file changed, 10 insertions(+), 1 deletion(-)\n> \n> diff --git a/dir.c b/dir.c\n> index b90484a..fba938b 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -2107,6 +2107,15 @@ int file_exists(const char *f)\n>  \treturn lstat(f, &sb) == 0;\n>  }\n>  \n> +static int cmp_icase(char a, char b)\n> +{\n> +\tif (a == b)\n> +\t\treturn 0;\n> +\tif (ignore_case)\n> +\t\treturn toupper(a) - toupper(b);\n> +\treturn a - b;\n> +}\n> +\n>  /*\n>   * Given two normalized paths (a trailing slash is ok), if subdir is\n>   * outside dir, return -1.  Otherwise return the offset in subdir that\n> @@ -2118,7 +2127,7 @@ int dir_inside_of(const char *subdir, const char *dir)\n>  \n>  \tassert(dir && subdir && *dir && *subdir);\n>  \n> -\twhile (*dir && *subdir && *dir == *subdir) {\n> +\twhile (*dir && *subdir && !cmp_icase(*dir, *subdir)) {\n>  \t\tdir++;\n>  \t\tsubdir++;\n>  \t\toffset++;\n> \n\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"275254","messageId":"alpine.DEB.2.20.1601011554240.14434@virtualbox","threadId":"40440","inReplyTo":"56829D08.1060107@alum.mit.edu","subject":"Re: [PATCH] Fix \"inside work tree\" detection on case-insensitive filesystems","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-01T14:58:53Z","receivedAt":"2016-01-01T14:58:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Michael,\n\nOn Tue, 29 Dec 2015, Michael Haggerty wrote:\n\n> On 09/28/2015 06:12 PM, Johannes Schindelin wrote:\n> > Git has a config variable to indicate that it is operating on a file\n> > system that is case-insensitive: core.ignoreCase. But the\n> > `dir_inside_of()` function did not respect that. As a result, if Git's\n> > idea of the current working directory disagreed in its upper/lower case\n> > with the `GIT_WORK_TREE` variable (e.g. `C:\\test` vs `c:\\test`) the\n> > user would be greeted by the error message\n> > \n> > \tfatal: git-am cannot be used without a working tree.\n> > \n> > when trying to run a rebase.\n> > \n> > This fixes https://github.com/git-for-windows/git/issues/402 (reported by\n> > Daniel Harding).\n> \n> I was just going through the 2.7 release notes when I saw this patch.\n\nThanks for your diligence!\n\n> My understanding was that many of the case-insensitive filesystems also\n> support Unicode. Is the byte-oriented code in this patch adequate? I\n> would have thought it necessary to use a Unicode-aware algorithm here,\n> that knows:\n> \n> * that bytes != characters\n\nI am not sure that we can in general assume that the file name is UTF-8...\nOr does Git always assume that?\n\n> * how to do a case-insensitive comparison of strings that include\n> non-ASCII characters\n\nI was worrying about that, too, but decided to punt on it when I realized\nthat no other case-insensitive code in Git bothers about those characters.\n\n> * (possibly) insensitivity to NFC vs. NFD vs. non-normalized forms\n\nWhoa... I really would like to stay away from that collection of\npotholes...\n\n> I suppose that such OSs have built-in functions for deciding whether two\n> paths are equivalent. Possibly these could be used?\n\nWe could, in theory, try to do that, but what about the OSes that do *not*\nhave those functions? We would need our own fallback anyway, so why not\nguarantee consistency and use our own functions only?\n\nCiao,\nDscho\n"},{"id":"275259","messageId":"5686A82D.6020105@alum.mit.edu","threadId":"40440","inReplyTo":"alpine.DEB.2.20.1601011554240.14434@virtualbox","subject":"Re: [PATCH] Fix \"inside work tree\" detection on case-insensitive filesystems","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-01-01T16:24:13Z","receivedAt":"2016-01-01T16:24:13Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 01/01/2016 03:58 PM, Johannes Schindelin wrote:\n> Hi Michael,\n> \n> On Tue, 29 Dec 2015, Michael Haggerty wrote:\n> \n>> On 09/28/2015 06:12 PM, Johannes Schindelin wrote:\n>>> Git has a config variable to indicate that it is operating on a file\n>>> system that is case-insensitive: core.ignoreCase. But the\n>>> `dir_inside_of()` function did not respect that. As a result, if Git's\n>>> idea of the current working directory disagreed in its upper/lower case\n>>> with the `GIT_WORK_TREE` variable (e.g. `C:\\test` vs `c:\\test`) the\n>>> user would be greeted by the error message\n>>>\n>>> \tfatal: git-am cannot be used without a working tree.\n>>>\n>>> when trying to run a rebase.\n>>>\n>>> This fixes https://github.com/git-for-windows/git/issues/402 (reported by\n>>> Daniel Harding).\n>>\n>> I was just going through the 2.7 release notes when I saw this patch.\n> \n> Thanks for your diligence!\n> \n>> My understanding was that many of the case-insensitive filesystems also\n>> support Unicode. Is the byte-oriented code in this patch adequate? I\n>> would have thought it necessary to use a Unicode-aware algorithm here,\n>> that knows:\n>>\n>> * that bytes != characters\n> \n> I am not sure that we can in general assume that the file name is UTF-8...\n> Or does Git always assume that?\n\nNo, it does not. I would say that Git resolutely refuses to take a stand\non whether/what encoding is used for filenames within its object database.\n\nBut as far as I understand, here we have a slightly different situation:\nwe have a string from an environment variable, and we have the current\nworking directory. Neither of these things are paths from the Git ODB,\nand we don't have to take a stand about whether two paths from the ODB\nare equivalent.\n\n>> * how to do a case-insensitive comparison of strings that include\n>> non-ASCII characters\n> \n> I was worrying about that, too, but decided to punt on it when I realized\n> that no other case-insensitive code in Git bothers about those characters.\n> \n>> * (possibly) insensitivity to NFC vs. NFD vs. non-normalized forms\n> \n> Whoa... I really would like to stay away from that collection of\n> potholes...\n> \n>> I suppose that such OSs have built-in functions for deciding whether two\n>> paths are equivalent. Possibly these could be used?\n> \n> We could, in theory, try to do that, but what about the OSes that do *not*\n> have those functions? We would need our own fallback anyway, so why not\n> guarantee consistency and use our own functions only?\n\nI don't see why consistency across OSes is an issue here. Whenever we\nare talking about paths on a particular filesystem, it is the\nfilesystem's idea of equivalence that is relevant.\n\nSuppose we had a wrapper `filesystem_paths_equivalent(path1, path2)`. If\nan OS provides a function that can be used for such a comparison, we can\nuse that. If not, but `ignore_case` is set, we can use your\n`cmp_icase()`-based function. Otherwise, the default implementation\nwould be `!strcmp()`.\n\nI don't think it would be worth going to all this trouble for this one\ncallsite (at least under my assumption that this has no security\nimplications). But I would have expected this issue to come up in enough\nplaces that such an abstraction would be helpful.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"}]}