{"thread":{"id":"48963","subject":"Git clone and case sensitivity","startedAt":"2018-07-27T09:59:38Z","lastAt":"2018-11-23T11:24:46Z","messageCount":96,"participants":["Paweł Paruzel","brian m. carlson","Duy Nguyen","Jeff King","Simon Ruderich","Nguyễn Thái Ngọc Duy","Torsten Bögershausen","Elijah Newren","Junio C Hamano","Jeff Hostetler","SZEDER Gábor","Carlo Marcelo Arenas Belón","Carlo Arenas","Ramsay Jones","tboegi@web.de","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"353693","messageId":"24A09B73-B4D4-4C22-BC1B-41B22CB59FE6@gmail.com","threadId":"48963","inReplyTo":null,"subject":"Git clone and case sensitivity","fromName":"Paweł Paruzel","fromEmail":"pawelparuzel95@gmail.com","sentAt":"2018-07-27T09:59:33Z","receivedAt":"2018-07-27T09:59:38Z","isPatch":false,"sender":{"key":"pawelparuzel95@gmail.com","avatar":null},"body":"Hi,\n\nLately, I have been wondering why my test files in repo are modified after I clone it. It turned out to be two files: boolStyle_t_f and boolStyle_T_F.\nThe system that pushed those files was case sensitive while my mac after High Sierra update had APFS which is by default case-insensitive. I highly suggest that git clone threw an exception when files are case sensitive and being cloned to a case insensitive system. This has caused problems with overriding files for test cases without any warning.\n\nThanks in advance.\nRegards,\nPawel Paruzel"},{"id":"353753","messageId":"20180727205909.GC376343@genre.crustytoothpaste.net","threadId":"48963","inReplyTo":"24A09B73-B4D4-4C22-BC1B-41B22CB59FE6@gmail.com","subject":"Re: Git clone and case sensitivity","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-07-27T20:59:09Z","receivedAt":"2018-07-27T20:59:17Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Fri, Jul 27, 2018 at 11:59:33AM +0200, Paweł Paruzel wrote:\n> Hi,\n> \n> Lately, I have been wondering why my test files in repo are modified\n> after I clone it. It turned out to be two files: boolStyle_t_f and\n> boolStyle_T_F.\n> The system that pushed those files was case sensitive while my mac\n> after High Sierra update had APFS which is by default\n> case-insensitive. I highly suggest that git clone threw an exception\n> when files are case sensitive and being cloned to a case insensitive\n> system. This has caused problems with overriding files for test cases\n> without any warning.\n\nIf we did what you proposed, it would be impossible to clone such a\nrepository on a case-insensitive system. While this might be fine for a\nclosed system such as inside a company, this would make many open source\nrepositories unusable, even when the files differing in case are\nnonfunctional (like README and readme).\n\nThis is actually one of a few ways people can make repositories that\nwill show as modified on Windows or macOS systems, due to limitations in\nthose OSes. If you want to be sure that your repository is unmodified\nafter clone, you can ensure that the output of git status --porcelain is\nempty, such as by checking for a zero exit from\n\"test $(git status --porcelain | wc -l) -eq 0\".\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"353783","messageId":"20180728043559.GA29185@duynguyen.home","threadId":"48963","inReplyTo":"20180727205909.GC376343@genre.crustytoothpaste.net","subject":"Re: Git clone and case sensitivity","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-07-28T04:36:00Z","receivedAt":"2018-07-28T04:41:33Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Jul 27, 2018 at 08:59:09PM +0000, brian m. carlson wrote:\n> On Fri, Jul 27, 2018 at 11:59:33AM +0200, Paweł Paruzel wrote:\n> > Hi,\n> > \n> > Lately, I have been wondering why my test files in repo are modified\n> > after I clone it. It turned out to be two files: boolStyle_t_f and\n> > boolStyle_T_F.\n> > The system that pushed those files was case sensitive while my mac\n> > after High Sierra update had APFS which is by default\n> > case-insensitive. I highly suggest that git clone threw an exception\n> > when files are case sensitive and being cloned to a case insensitive\n> > system. This has caused problems with overriding files for test cases\n> > without any warning.\n> \n> If we did what you proposed, it would be impossible to clone such a\n> repository on a case-insensitive system.\n\nI agree throwing a real exception would be bad. But how about detecting\nthe problem and trying our best to keep the repo in somewhat usable\nstate like this?\n\nThis patch uses sparse checkout to hide all those paths that we fail\nto checkout, so you can still have a clean worktree to do things, as\nlong as you don't touch those paths.\n\n-- 8< --\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 1d939af9d8..a6b5e2c948 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -711,6 +711,30 @@ static void update_head(const struct ref *our, const struct ref *remote,\n \t}\n }\n \n+static int enable_sparse_checkout_on_icase_fs(struct index_state *istate)\n+{\n+\tint i;\n+\tint skip_count = 0;\n+\tFILE *fp = fopen(git_path(\"info/sparse\"), \"a+\");\n+\n+\tfor (i = 0; i < istate->cache_nr; i++) {\n+\t\tstruct cache_entry *ce = istate->cache[i];\n+\t\tif (!ce_skip_worktree(ce))\n+\t\t\tcontinue;\n+\t\tif (!skip_count) {\n+\t\t\tgit_config_set_multivar_gently(\"core.sparseCheckout\",\n+\t\t\t\t\t\t \"true\",\n+\t\t\t\t\t\t CONFIG_REGEX_NONE, 0);\n+\t\t\tfprintf(fp, \"# List of paths hidden by 'git clone'\\n\");\n+\t\t}\n+\t\tfprintf(fp, \"/%s\\n\", ce->name);\n+\t\tskip_count++;\n+\t}\n+\tfclose(fp);\n+\n+\treturn skip_count;\n+}\n+\n static int checkout(int submodule_progress)\n {\n \tstruct object_id oid;\n@@ -751,6 +775,7 @@ static int checkout(int submodule_progress)\n \topts.verbose_update = (option_verbosity >= 0);\n \topts.src_index = &the_index;\n \topts.dst_index = &the_index;\n+\topts.clone_checkout = 1;\n \n \ttree = parse_tree_indirect(&oid);\n \tparse_tree(tree);\n@@ -761,6 +786,12 @@ static int checkout(int submodule_progress)\n \tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(_(\"unable to write new index file\"));\n \n+\tif (enable_sparse_checkout_on_icase_fs(&the_index))\n+\t\twarning(\"Paths that differ only in case are detected \"\n+\t\t\t\"and will not work correctly on this case-insensitive \"\n+\t\t\t\"filesystem. Sparse checkout has been enabled to hide \"\n+\t\t\t\"these paths.\");\n+\n \terr |= run_hook_le(NULL, \"post-checkout\", sha1_to_hex(null_sha1),\n \t\t\t oid_to_hex(&oid), \"1\", NULL);\n \ndiff --git a/cache.h b/cache.h\nindex 8b447652a7..9ecf7ad952 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1455,6 +1455,7 @@ struct checkout {\n \tunsigned force:1,\n \t\t quiet:1,\n \t\t not_new:1,\n+\t\t set_skipworktree_on_updated:1,\n \t\t refresh_cache:1;\n };\n #define CHECKOUT_INIT { NULL, \"\" }\ndiff --git a/entry.c b/entry.c\nindex b5d1d3cf23..ba21db63e7 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -447,6 +447,11 @@ int checkout_entry(struct cache_entry *ce,\n \n \t\tif (!changed)\n \t\t\treturn 0;\n+\t\tif (state->set_skipworktree_on_updated) {\n+\t\t\tce->ce_flags |= CE_SKIP_WORKTREE;\n+\t\t\tstate->istate->cache_changed |= CE_ENTRY_CHANGED;\n+\t\t\treturn 0;\n+\t\t}\n \t\tif (!state->force) {\n \t\t\tif (!state->quiet)\n \t\t\t\tfprintf(stderr,\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 66741130ae..a8a24e0b13 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -358,6 +358,7 @@ static int check_updates(struct unpack_trees_options *o)\n \tstate.quiet = 1;\n \tstate.refresh_cache = 1;\n \tstate.istate = index;\n+\tstate.set_skipworktree_on_updated = o->clone_checkout;\n \n \tprogress = get_progress(o);\n \ndiff --git a/unpack-trees.h b/unpack-trees.h\nindex c2b434c606..8ebe2e2ec5 100644\n--- a/unpack-trees.h\n+++ b/unpack-trees.h\n@@ -49,6 +49,7 @@ struct unpack_trees_options {\n \t\t aggressive,\n \t\t skip_unmerged,\n \t\t initial_checkout,\n+\t\t clone_checkout,\n \t\t diff_index_cached,\n \t\t debug_unpack,\n \t\t skip_sparse_checkout,\n-- 8< --\n\n> While this might be fine for a closed system such as inside a\n> company, this would make many open source repositories unusable,\n> even when the files differing in case are nonfunctional (like README\n> and readme).\n> \n> This is actually one of a few ways people can make repositories that\n> will show as modified on Windows or macOS systems, due to limitations in\n> those OSes. If you want to be sure that your repository is unmodified\n> after clone, you can ensure that the output of git status --porcelain is\n> empty, such as by checking for a zero exit from\n> \"test $(git status --porcelain | wc -l) -eq 0\".\n> -- \n> brian m. carlson: Houston, Texas, US\n> OpenPGP: https://keybase.io/bk2204\n\n\n"},{"id":"353784","messageId":"CACsJy8A3pd85fDrbak8TCnmkMb_FDmmpaNd5tBSCKBGkGswKCg@mail.gmail.com","threadId":"48963","inReplyTo":"20180728043559.GA29185@duynguyen.home","subject":"Re: Git clone and case sensitivity","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-07-28T04:45:43Z","receivedAt":"2018-07-28T04:46:13Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jul 28, 2018 at 6:36 AM Duy Nguyen wrote:\n>\n> On Fri, Jul 27, 2018 at 08:59:09PM +0000, brian m. carlson wrote:\n> > On Fri, Jul 27, 2018 at 11:59:33AM +0200, Paweł Paruzel wrote:\n> > > Hi,\n> > >\n> > > Lately, I have been wondering why my test files in repo are modified\n> > > after I clone it. It turned out to be two files: boolStyle_t_f and\n> > > boolStyle_T_F.\n> > > The system that pushed those files was case sensitive while my mac\n> > > after High Sierra update had APFS which is by default\n> > > case-insensitive. I highly suggest that git clone threw an exception\n> > > when files are case sensitive and being cloned to a case insensitive\n> > > system. This has caused problems with overriding files for test cases\n> > > without any warning.\n> >\n> > If we did what you proposed, it would be impossible to clone such a\n> > repository on a case-insensitive system.\n>\n> I agree throwing a real exception would be bad. But how about detecting\n> the problem and trying our best to keep the repo in somewhat usable\n> state like this?\n>\n> This patch uses sparse checkout to hide all those paths that we fail\n> to checkout, so you can still have a clean worktree to do things, as\n> long as you don't touch those paths.\n\nSide note. There may still be problems with this patch. Let's use\nvim-colorschemes.git as an example, which has darkBlue.vim and\ndarkblue.vim.\n\nSay we have checked out darkBlue.vim and hidden darkblue.vim. When you\nupdate darkBlue.vim on worktree and then update the index, are we sure\nwe will update darkBlue.vim entry and not (hidden) darkblue.vim? I am\nnot sure. I don't think our lookup function is prepared to deal with\nthis. Maybe it's best to hide both of them.\n-- \nDuy\n"},{"id":"353785","messageId":"20180728044857.GA10444@sigill.intra.peff.net","threadId":"48963","inReplyTo":"CACsJy8A3pd85fDrbak8TCnmkMb_FDmmpaNd5tBSCKBGkGswKCg@mail.gmail.com","subject":"Re: Git clone and case sensitivity","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-28T04:48:57Z","receivedAt":"2018-07-28T04:49:00Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 28, 2018 at 06:45:43AM +0200, Duy Nguyen wrote:\n\n> > I agree throwing a real exception would be bad. But how about detecting\n> > the problem and trying our best to keep the repo in somewhat usable\n> > state like this?\n> >\n> > This patch uses sparse checkout to hide all those paths that we fail\n> > to checkout, so you can still have a clean worktree to do things, as\n> > long as you don't touch those paths.\n> \n> Side note. There may still be problems with this patch. Let's use\n> vim-colorschemes.git as an example, which has darkBlue.vim and\n> darkblue.vim.\n> \n> Say we have checked out darkBlue.vim and hidden darkblue.vim. When you\n> update darkBlue.vim on worktree and then update the index, are we sure\n> we will update darkBlue.vim entry and not (hidden) darkblue.vim? I am\n> not sure. I don't think our lookup function is prepared to deal with\n> this. Maybe it's best to hide both of them.\n\nIt might be enough to just issue a warning and give an advise() hint\nthat tells the user what's going on. Then they can decide what to do\n(hide both paths, or just work in the index, or move to a different fs,\nor complain to upstream).\n\n-Peff\n"},{"id":"353786","messageId":"20180728051105.GA32243@duynguyen.home","threadId":"48963","inReplyTo":"20180728044857.GA10444@sigill.intra.peff.net","subject":"Re: Git clone and case sensitivity","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-07-28T05:11:05Z","receivedAt":"2018-07-28T05:11:13Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jul 28, 2018 at 12:48:57AM -0400, Jeff King wrote:\n> On Sat, Jul 28, 2018 at 06:45:43AM +0200, Duy Nguyen wrote:\n> \n> > > I agree throwing a real exception would be bad. But how about detecting\n> > > the problem and trying our best to keep the repo in somewhat usable\n> > > state like this?\n> > >\n> > > This patch uses sparse checkout to hide all those paths that we fail\n> > > to checkout, so you can still have a clean worktree to do things, as\n> > > long as you don't touch those paths.\n> > \n> > Side note. There may still be problems with this patch. Let's use\n> > vim-colorschemes.git as an example, which has darkBlue.vim and\n> > darkblue.vim.\n> > \n> > Say we have checked out darkBlue.vim and hidden darkblue.vim. When you\n> > update darkBlue.vim on worktree and then update the index, are we sure\n> > we will update darkBlue.vim entry and not (hidden) darkblue.vim? I am\n> > not sure. I don't think our lookup function is prepared to deal with\n> > this. Maybe it's best to hide both of them.\n> \n> It might be enough to just issue a warning and give an advise() hint\n> that tells the user what's going on. Then they can decide what to do\n> (hide both paths, or just work in the index, or move to a different fs,\n> or complain to upstream).\n\nYeah that may be the best option. Something like this perhaps? Not\nsure how much detail the advice should be here.\n\n-- 8< --\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 1d939af9d8..b47ad5877b 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -711,6 +711,30 @@ static void update_head(const struct ref *our, const struct ref *remote,\n \t}\n }\n \n+static int has_duplicate_icase_entries(struct index_state *istate)\n+{\n+\tstruct string_list list = STRING_LIST_INIT_NODUP;\n+\tint i;\n+\tint found = 0;\n+\n+\tfor (i = 0; i < istate->cache_nr; i++)\n+\t\tstring_list_append(&list, istate->cache[i]->name);\n+\n+\tlist.cmp = strcasecmp;\n+\tstring_list_sort(&list);\n+\n+\tfor (i = 1; i < list.nr; i++) {\n+\t\tif (strcasecmp(list.items[i-1].string,\n+\t\t\t list.items[i].string))\n+\t\t\tcontinue;\n+\t\tfound = 1;\n+\t\tbreak;\n+\t}\n+\tstring_list_clear(&list, 0);\n+\n+\treturn found;\n+}\n+\n static int checkout(int submodule_progress)\n {\n \tstruct object_id oid;\n@@ -761,6 +785,11 @@ static int checkout(int submodule_progress)\n \tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(_(\"unable to write new index file\"));\n \n+\tif (ignore_case && has_duplicate_icase_entries(&the_index))\n+\t\twarning(_(\"This repository has paths that only differ in case\\n\"\n+\t\t\t \"and you have a case-insenitive filesystem which will\\n\"\n+\t\t\t \"cause problems.\"));\n+\n \terr |= run_hook_le(NULL, \"post-checkout\", sha1_to_hex(null_sha1),\n \t\t\t oid_to_hex(&oid), \"1\", NULL);\n \n-- 8< --\n"},{"id":"353793","messageId":"20180728094804.GA12770@ruderich.org","threadId":"48963","inReplyTo":"20180728051105.GA32243@duynguyen.home","subject":"Re: Git clone and case sensitivity","fromName":"Simon Ruderich","fromEmail":"simon@ruderich.org","sentAt":"2018-07-28T09:48:04Z","receivedAt":"2018-07-28T09:48:20Z","isPatch":false,"sender":{"key":"simon@ruderich.org","avatar":"https://avatars.githubusercontent.com/u/390994?v=4"},"body":"On Sat, Jul 28, 2018 at 07:11:05AM +0200, Duy Nguyen wrote:\n> static int checkout(int submodule_progress)\n> {\n> \tstruct object_id oid;\n> @@ -761,6 +785,11 @@ static int checkout(int submodule_progress)\n> \tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n> \t\tdie(_(\"unable to write new index file\"));\n>\n> +\tif (ignore_case && has_duplicate_icase_entries(&the_index))\n> +\t\twarning(_(\"This repository has paths that only differ in case\\n\"\n> +\t\t\t \"and you have a case-insenitive filesystem which will\\n\"\n> +\t\t\t \"cause problems.\"));\n> +\n> \terr |= run_hook_le(NULL, \"post-checkout\", sha1_to_hex(null_sha1),\n> \t\t\t oid_to_hex(&oid), \"1\", NULL);\n\nI think the advice message should list the problematic file\nnames. Even though this might be quite verbose it will help those\naffected to quickly find the problematic files to either fix this\non their own or report to upstream (unless there's already an\neasy way to find those files - if so it should be mentioned in\nthe message).\n\nRegards\nSimon\n-- \n+ privacy is necessary\n+ using gnupg http://gnupg.org\n+ public key id: 0x92FEFDB7E44C32F9\n"},{"id":"353794","messageId":"20180728095659.GA21450@sigill.intra.peff.net","threadId":"48963","inReplyTo":"20180728051105.GA32243@duynguyen.home","subject":"Re: Git clone and case sensitivity","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-28T09:56:59Z","receivedAt":"2018-07-28T09:57:02Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 28, 2018 at 07:11:05AM +0200, Duy Nguyen wrote:\n\n> > It might be enough to just issue a warning and give an advise() hint\n> > that tells the user what's going on. Then they can decide what to do\n> > (hide both paths, or just work in the index, or move to a different fs,\n> > or complain to upstream).\n> \n> Yeah that may be the best option. Something like this perhaps? Not\n> sure how much detail the advice should be here.\n\nYeah, something along these lines. I agree with Simon's comment\nelsewhere that this should probably mention the names. I don't know if\nwe'd want to offer advice pointing them to using the sparse feature to\nwork around it.\n\n> +static int has_duplicate_icase_entries(struct index_state *istate)\n> +{\n> +\tstruct string_list list = STRING_LIST_INIT_NODUP;\n> +\tint i;\n> +\tint found = 0;\n> +\n> +\tfor (i = 0; i < istate->cache_nr; i++)\n> +\t\tstring_list_append(&list, istate->cache[i]->name);\n> +\n> +\tlist.cmp = strcasecmp;\n> +\tstring_list_sort(&list);\n> +\n> +\tfor (i = 1; i < list.nr; i++) {\n> +\t\tif (strcasecmp(list.items[i-1].string,\n> +\t\t\t list.items[i].string))\n> +\t\t\tcontinue;\n> +\t\tfound = 1;\n> +\t\tbreak;\n> +\t}\n> +\tstring_list_clear(&list, 0);\n> +\n> +\treturn found;\n> +}\n\nstrcasecmp() will only catch a subset of the cases. We really need to\nfollow the same folding rules that the filesystem would.\n\nFor the case of clone, I actually wonder if we could detect during the\ncheckout step that a file already exists. Since we know that the\ndirectory we started with was empty, then if it does, either:\n\n - there's some funny case-folding going on that means two paths in the\n repository map to the same name in the filesystem; or\n\n - somebody else is writing to the directory at the same time as us\n\nEither of which I think would be worth warning about. I'm not sure if we\nalready lstat() the paths we're writing anyway as part of the checkout,\nso we might even get the feature \"for free\".\n\n-Peff\n"},{"id":"353801","messageId":"20180728180514.GA945730@genre.crustytoothpaste.net","threadId":"48963","inReplyTo":"20180728095659.GA21450@sigill.intra.peff.net","subject":"Re: Git clone and case sensitivity","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-07-28T18:05:14Z","receivedAt":"2018-07-28T18:05:23Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Sat, Jul 28, 2018 at 05:56:59AM -0400, Jeff King wrote:\n> strcasecmp() will only catch a subset of the cases. We really need to\n> follow the same folding rules that the filesystem would.\n> \n> For the case of clone, I actually wonder if we could detect during the\n> checkout step that a file already exists. Since we know that the\n> directory we started with was empty, then if it does, either:\n> \n> - there's some funny case-folding going on that means two paths in the\n> repository map to the same name in the filesystem; or\n> \n> - somebody else is writing to the directory at the same time as us\n> \n> Either of which I think would be worth warning about. I'm not sure if we\n> already lstat() the paths we're writing anyway as part of the checkout,\n> so we might even get the feature \"for free\".\n\nThis is possible to do. From the bug I accidentally introduced in 2.16,\nwe know that on clone, there is a code path that is only traversed when\nwe hit this case and only on case-insensitive file systems.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"353812","messageId":"CACsJy8DTQhinpLOhojnrpFt3_2tVo3mo1Dwv-x4aF3mZJ2Rhgg@mail.gmail.com","threadId":"48963","inReplyTo":"20180728095659.GA21450@sigill.intra.peff.net","subject":"Re: Git clone and case sensitivity","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-07-29T05:26:41Z","receivedAt":"2018-07-29T05:29:26Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jul 28, 2018 at 11:57 AM Jeff King wrote:\n> > +static int has_duplicate_icase_entries(struct index_state *istate)\n> > +{\n> > + struct string_list list = STRING_LIST_INIT_NODUP;\n> > + int i;\n> > + int found = 0;\n> > +\n> > + for (i = 0; i < istate->cache_nr; i++)\n> > + string_list_append(&list, istate->cache[i]->name);\n> > +\n> > + list.cmp = strcasecmp;\n> > + string_list_sort(&list);\n> > +\n> > + for (i = 1; i < list.nr; i++) {\n> > + if (strcasecmp(list.items[i-1].string,\n> > + list.items[i].string))\n> > + continue;\n> > + found = 1;\n> > + break;\n> > + }\n> > + string_list_clear(&list, 0);\n> > +\n> > + return found;\n> > +}\n>\n> strcasecmp() will only catch a subset of the cases. We really need to\n> follow the same folding rules that the filesystem would.\n\nTrue. But that's how we handle case insensitivity internally. If a\nfilesytem has more sophisticated folding rules then git will not work\nwell on that one anyway.\n\n> For the case of clone, I actually wonder if we could detect during the\n> checkout step that a file already exists. Since we know that the\n> directory we started with was empty, then if it does, either:\n>\n> - there's some funny case-folding going on that means two paths in the\n> repository map to the same name in the filesystem; or\n>\n> - somebody else is writing to the directory at the same time as us\n\nThis is exactly what my first patch does (minus the sparse checkout\npart). But without knowing the exact folding rules, I don't think we\ncan locate this \"somebody else\" who wrote the first path. So if N\npaths are treated the same by this filesystem, we could only report\nN-1 of them.\n\nIf we want to report just one path when this happens though, then this\nworks quite well.\n-- \nDuy\n"},{"id":"353814","messageId":"20180729092759.GA14484@sigill.intra.peff.net","threadId":"48963","inReplyTo":"CACsJy8DTQhinpLOhojnrpFt3_2tVo3mo1Dwv-x4aF3mZJ2Rhgg@mail.gmail.com","subject":"Re: Git clone and case sensitivity","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-29T09:28:00Z","receivedAt":"2018-07-29T09:28:03Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 29, 2018 at 07:26:41AM +0200, Duy Nguyen wrote:\n\n> > strcasecmp() will only catch a subset of the cases. We really need to\n> > follow the same folding rules that the filesystem would.\n> \n> True. But that's how we handle case insensitivity internally. If a\n> filesytem has more sophisticated folding rules then git will not work\n> well on that one anyway.\n\nHrm. Yeah, I guess that's the best we can do for the actual in-memory\nchecks. Everything else depends on doing an actual filesystem operation,\nand our icase stuff kicks in way before then. I was mostly thinking of\nHFS+ utf8 normalization weirdness, but I guess people are accustomed to\nthat by now.\n\n> > For the case of clone, I actually wonder if we could detect during the\n> > checkout step that a file already exists. Since we know that the\n> > directory we started with was empty, then if it does, either:\n> >\n> > - there's some funny case-folding going on that means two paths in the\n> > repository map to the same name in the filesystem; or\n> >\n> > - somebody else is writing to the directory at the same time as us\n> \n> This is exactly what my first patch does (minus the sparse checkout\n> part).\n\nRight, sorry, I should have read that one more carefully.\n\n> But without knowing the exact folding rules, I don't think we can\n> locate this \"somebody else\" who wrote the first path. So if N paths\n> are treated the same by this filesystem, we could only report N-1 of\n> them.\n> \n> If we want to report just one path when this happens though, then this\n> works quite well.\n\nHmm. Since most such systems are case-preserving, would it be possible\nto report the name of the existing file? Doing it via opendir/readdir is\nhacky, and anyway puts the burden on us to find the matching name. Doing\nit via fstat() on the opened file doesn't work because at that the\nfilesystem has resolved the name to an inode.\n\nSo yeah, perhaps strcasecmp() is the best we can do (I do agree that\nbeing able to mention all of the conflicting names is a benefit).\n\nI guess we should be using fspathcmp(), though, in case it later learns\nto be smarter.\n\n-Peff\n"},{"id":"353885","messageId":"20180730152756.15012-1-pclouds@gmail.com","threadId":"48963","inReplyTo":"20180729092759.GA14484@sigill.intra.peff.net","subject":"[PATCH/RFC] clone: report duplicate entries on case-insensitive filesystems","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-07-30T15:27:55Z","receivedAt":"2018-07-30T15:28:04Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Paths that only differ in case work fine in a case-sensitive\nfilesystems, but if those repos are cloned in a case-insensitive one,\nyou'll get problems. The first thing to notice is \"git status\" will\nnever be clean with no indication what's exactly is \"dirty\".\n\nThis patch helps the situation a bit by pointing out the problem at\nclone time. I have not suggested any way to work around or fix this\nproblem. But I guess we could probably have a section in\nDocumentation/ dedicated to this problem and point there instead of\na long advice in this warning.\n\nAnother thing we probably should do is catch in \"git checkout\" too,\nnot just \"git clone\" since your linux/unix colleage colleague may\naccidentally add some files that your mac/windows machine is not very\nhappy with. But then there's another problem, once the problem is\nknown, we probably should stop spamming this warning at every\ncheckout, but how?\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/clone.c | 41 +++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 41 insertions(+)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 5c439f1394..32738c2737 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -711,6 +711,33 @@ static void update_head(const struct ref *our, const struct ref *remote,\n \t}\n }\n \n+static void find_duplicate_icase_entries(struct index_state *istate,\n+\t\t\t\t\t struct string_list *dup)\n+{\n+\tstruct string_list list = STRING_LIST_INIT_NODUP;\n+\tint i;\n+\n+\tfor (i = 0; i < istate->cache_nr; i++)\n+\t\tstring_list_append(&list, istate->cache[i]->name);\n+\n+\tlist.cmp = fspathcmp;\n+\tstring_list_sort(&list);\n+\n+\tfor (i = 1; i < list.nr; i++) {\n+\t\tconst char *cur = list.items[i].string;\n+\t\tconst char *prev = list.items[i - 1].string;\n+\n+\t\tif (dup->nr &&\n+\t\t !fspathcmp(cur, dup->items[dup->nr - 1].string)) {\n+\t\t\tstring_list_append(dup, cur);\n+\t\t} else if (!fspathcmp(cur, prev)) {\n+\t\t\tstring_list_append(dup, prev);\n+\t\t\tstring_list_append(dup, cur);\n+\t\t}\n+\t}\n+\tstring_list_clear(&list, 0);\n+}\n+\n static int checkout(int submodule_progress)\n {\n \tstruct object_id oid;\n@@ -761,6 +788,20 @@ static int checkout(int submodule_progress)\n \tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(_(\"unable to write new index file\"));\n \n+\tif (ignore_case) {\n+\t\tstruct string_list dup = STRING_LIST_INIT_DUP;\n+\t\tint i;\n+\n+\t\tfind_duplicate_icase_entries(&the_index, &dup);\n+\t\tif (dup.nr) {\n+\t\t\twarning(_(\"the following paths in this repository only differ in case and will\\n\"\n+\t\t\t\t \"cause problems because you have cloned it on an case-insensitive filesytem:\\n\"));\n+\t\t\tfor (i = 0; i < dup.nr; i++)\n+\t\t\t\tfprintf(stderr, \"\\t%s\\n\", dup.items[i].string);\n+\t\t}\n+\t\tstring_list_clear(&dup, 0);\n+\t}\n+\n \terr |= run_hook_le(NULL, \"post-checkout\", sha1_to_hex(null_sha1),\n \t\t\t oid_to_hex(&oid), \"1\", NULL);\n \n-- \n2.18.0.656.gda699b98b3\n\n"},{"id":"354099","messageId":"20180731182344.GA3286@tor.lan","threadId":"48963","inReplyTo":"20180730152756.15012-1-pclouds@gmail.com","subject":"Re: [PATCH/RFC] clone: report duplicate entries on case-insensitive filesystems","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2018-07-31T18:23:44Z","receivedAt":"2018-07-31T18:24:01Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Mon, Jul 30, 2018 at 05:27:55PM +0200, Nguyễn Thái Ngọc Duy wrote:\n> Paths that only differ in case work fine in a case-sensitive\n> filesystems, but if those repos are cloned in a case-insensitive one,\n> you'll get problems. The first thing to notice is \"git status\" will\n> never be clean with no indication what's exactly is \"dirty\".\n> \n> This patch helps the situation a bit by pointing out the problem at\n> clone time. I have not suggested any way to work around or fix this\n> problem. But I guess we could probably have a section in\n> Documentation/ dedicated to this problem and point there instead of\n> a long advice in this warning.\n> \n> Another thing we probably should do is catch in \"git checkout\" too,\n> not just \"git clone\" since your linux/unix colleage colleague may\n> accidentally add some files that your mac/windows machine is not very\n> happy with. But then there's another problem, once the problem is\n> known, we probably should stop spamming this warning at every\n> checkout, but how?\n> \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> builtin/clone.c | 41 +++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 41 insertions(+)\n> \n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 5c439f1394..32738c2737 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -711,6 +711,33 @@ static void update_head(const struct ref *our, const struct ref *remote,\n> \t}\n> }\n> \n> +static void find_duplicate_icase_entries(struct index_state *istate,\n> +\t\t\t\t\t struct string_list *dup)\n> +{\n> +\tstruct string_list list = STRING_LIST_INIT_NODUP;\n> +\tint i;\n> +\n> +\tfor (i = 0; i < istate->cache_nr; i++)\n> +\t\tstring_list_append(&list, istate->cache[i]->name);\n> +\n> +\tlist.cmp = fspathcmp;\n> +\tstring_list_sort(&list);\n> +\n> +\tfor (i = 1; i < list.nr; i++) {\n> +\t\tconst char *cur = list.items[i].string;\n> +\t\tconst char *prev = list.items[i - 1].string;\n> +\n> +\t\tif (dup->nr &&\n> +\t\t !fspathcmp(cur, dup->items[dup->nr - 1].string)) {\n> +\t\t\tstring_list_append(dup, cur);\n> +\t\t} else if (!fspathcmp(cur, prev)) {\n> +\t\t\tstring_list_append(dup, prev);\n> +\t\t\tstring_list_append(dup, cur);\n> +\t\t}\n> +\t}\n> +\tstring_list_clear(&list, 0);\n> +}\n> +\n> static int checkout(int submodule_progress)\n> {\n> \tstruct object_id oid;\n> @@ -761,6 +788,20 @@ static int checkout(int submodule_progress)\n> \tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n> \t\tdie(_(\"unable to write new index file\"));\n> \n> +\tif (ignore_case) {\n> +\t\tstruct string_list dup = STRING_LIST_INIT_DUP;\n> +\t\tint i;\n> +\n> +\t\tfind_duplicate_icase_entries(&the_index, &dup);\n> +\t\tif (dup.nr) {\n> +\t\t\twarning(_(\"the following paths in this repository only differ in case and will\\n\"\n> +\t\t\t\t \"cause problems because you have cloned it on an case-insensitive filesytem:\\n\"));\n\nThanks for the patch.\nI wonder if we can tell the users more about the \"problems\"\nand how to avoid them, or to live with them.\n\nThis is more loud thinking:\n\n\"The following paths only differ in case\\n\"\n\"One a case-insensitive file system only one at a time can be present\\n\"\n\"You may rename one like this:\\n\"\n\"git checkout && git mv .1\\n\"\n\n> +\t\t\t\tfprintf(stderr, \"\\t%s\\n\", dup.items[i].string);\n\nAnother question:\nDo we need any quote_path() here ?\n(This may be overkill, since typically the repos with conflicting names\nonly use ASCII.)\n\n> +\t\t}\n> +\t\tstring_list_clear(&dup, 0);\n> +\t}\n> +\n> \terr |= run_hook_le(NULL, \"post-checkout\", sha1_to_hex(null_sha1),\n> \t\t\t oid_to_hex(&oid), \"1\", NULL);\n> \n> -- \n> 2.18.0.656.gda699b98b3\n> \n"},{"id":"354100","messageId":"CABPp-BG+nB+ifRbCdMpXnnxQ+rzhM8W-=sfQf8TYmXvuPy5WXg@mail.gmail.com","threadId":"48963","inReplyTo":"20180730152756.15012-1-pclouds@gmail.com","subject":"Re: [PATCH/RFC] clone: report duplicate entries on case-insensitive filesystems","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-31T18:44:27Z","receivedAt":"2018-07-31T18:44:30Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Jul 30, 2018 at 8:27 AM, Nguyễn Thái Ngọc Duy wrote:\n> Paths that only differ in case work fine in a case-sensitive\n> filesystems, but if those repos are cloned in a case-insensitive one,\n> you'll get problems. The first thing to notice is \"git status\" will\n> never be clean with no indication what's exactly is \"dirty\".\n\n\"what\" rather than \"what's\"?\n\n> This patch helps the situation a bit by pointing out the problem at\n> clone time. I have not suggested any way to work around or fix this\n> problem. But I guess we could probably have a section in\n> Documentation/ dedicated to this problem and point there instead of\n> a long advice in this warning.\n>\n> Another thing we probably should do is catch in \"git checkout\" too,\n> not just \"git clone\" since your linux/unix colleage colleague may\n\ndrop \"colleage\", keep \"colleague\"?\n\n> accidentally add some files that your mac/windows machine is not very\n> happy with. But then there's another problem, once the problem is\n> known, we probably should stop spamming this warning at every\n> checkout, but how?\n\nGood questions. I have no answers.\n\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> builtin/clone.c | 41 +++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 41 insertions(+)\n>\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 5c439f1394..32738c2737 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -711,6 +711,33 @@ static void update_head(const struct ref *our, const struct ref *remote,\n> }\n> }\n>\n> +static void find_duplicate_icase_entries(struct index_state *istate,\n> + struct string_list *dup)\n> +{\n> + struct string_list list = STRING_LIST_INIT_NODUP;\n> + int i;\n> +\n> + for (i = 0; i < istate->cache_nr; i++)\n> + string_list_append(&list, istate->cache[i]->name);\n> +\n> + list.cmp = fspathcmp;\n> + string_list_sort(&list);\n\nSo, you sort the list by fspathcmp to get the entries that differ in\ncase only next to each other. Makes sense...\n\n> +\n> + for (i = 1; i < list.nr; i++) {\n> + const char *cur = list.items[i].string;\n> + const char *prev = list.items[i - 1].string;\n> +\n> + if (dup->nr &&\n> + !fspathcmp(cur, dup->items[dup->nr - 1].string)) {\n> + string_list_append(dup, cur);\n\nIf we have at least one duplicate in dup (and currently we'd have to\nhave at least two), and we now hit yet another (i.e. a third or\nfourth...) way of spelling the same path, then we add it.\n\n> + } else if (!fspathcmp(cur, prev)) {\n> + string_list_append(dup, prev);\n> + string_list_append(dup, cur);\n> + }\n\n...otherwise, if we find a duplicate, we add both spellings to the dup list.\n\n> + }\n> + string_list_clear(&list, 0);\n> +}\n> +\n> static int checkout(int submodule_progress)\n> {\n> struct object_id oid;\n> @@ -761,6 +788,20 @@ static int checkout(int submodule_progress)\n> if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n> die(_(\"unable to write new index file\"));\n>\n> + if (ignore_case) {\n> + struct string_list dup = STRING_LIST_INIT_DUP;\n> + int i;\n> +\n> + find_duplicate_icase_entries(&the_index, &dup);\n> + if (dup.nr) {\n> + warning(_(\"the following paths in this repository only differ in case and will\\n\"\n\nPerhaps I'm being excessively pedantic, but what if there are multiple\npairs of paths differing in case? E.g. if someone has readme.txt,\nREADME.txt, foo.txt, and FOO.txt, you'll list all four files but\nreadme.txt and foo.txt do not only differ in case.\n\nMaybe something like \"...only differ in case from another path and\nwill... \" or is that too verbose and annoying?\n\n> + \"cause problems because you have cloned it on an case-insensitive filesytem:\\n\"));\n> + for (i = 0; i < dup.nr; i++)\n> + fprintf(stderr, \"\\t%s\\n\", dup.items[i].string);\n> + }\n> + string_list_clear(&dup, 0);\n> + }\n> +\n> err |= run_hook_le(NULL, \"post-checkout\", sha1_to_hex(null_sha1),\n> oid_to_hex(&oid), \"1\", NULL);\n\nIs it worth attempting to also warn about paths that only differ in\nUTF-normalization on relevant MacOS systems?\n"},{"id":"354104","messageId":"xmqqo9enb4n9.fsf@gitster-ct.c.googlers.com","threadId":"48963","inReplyTo":"CABPp-BG+nB+ifRbCdMpXnnxQ+rzhM8W-=sfQf8TYmXvuPy5WXg@mail.gmail.com","subject":"Re: [PATCH/RFC] clone: report duplicate entries on case-insensitive filesystems","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-31T19:12:58Z","receivedAt":"2018-07-31T19:13:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> Is it worth attempting to also warn about paths that only differ in\n> UTF-normalization on relevant MacOS systems?\n\nI hate to bring up a totally different approach this late in the\nparty, but I wonder if it makes more sense to take advantage of\n\"clone\" being a command that starts from an empty working tree.\n\nbuiltin/clone.c::checkout() drives a single-tree unpack_trees(),\nusing oneway_merge() as its callback and at the end, eventually\nunpack_trees.c:check_updates() will call into checkout_entry()\nto perform the usual \"unlink and then create\" dance.\n\nI wonder if it makes sense to introduce a new option to tell the\nmachinery to report when the final checkout_entry() notices that it\nneeded to remove the working tree file to make room (perhaps that\nbit would go in \"struct unpack_trees_options\"). In the initial\ncheckout codepath for a freshly cloned repository, that would only\nhappen when your tree has two (or more) paths that gets smashed\nby case insensitive or UTF-normalizing filesystem, and the code we\nwould maintain do not have to care how exactly the filesystem\ncollapses two (or more) paths if we go that way. We only need to\nreport \"we tried to check out X but it seems your filesystem equates\nsomething else that is also in the project to X\".\n\n\n\n"},{"id":"354105","messageId":"xmqqk1pbb4m2.fsf@gitster-ct.c.googlers.com","threadId":"48963","inReplyTo":"20180730152756.15012-1-pclouds@gmail.com","subject":"Re: [PATCH/RFC] clone: report duplicate entries on case-insensitive filesystems","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-31T19:13:41Z","receivedAt":"2018-07-31T19:13:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> Another thing we probably should do is catch in \"git checkout\" too,\n> not just \"git clone\" since your linux/unix colleage colleague may\n> accidentally add some files that your mac/windows machine is not very\n> happy with.\n\nThen you would catch it not in checkout but in add, no?\n"},{"id":"354108","messageId":"20180731192931.GD3372@sigill.intra.peff.net","threadId":"48963","inReplyTo":"xmqqo9enb4n9.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH/RFC] clone: report duplicate entries on case-insensitive filesystems","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-31T19:29:31Z","receivedAt":"2018-07-31T19:29:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 31, 2018 at 12:12:58PM -0700, Junio C Hamano wrote:\n\n> Elijah Newren writes:\n> \n> > Is it worth attempting to also warn about paths that only differ in\n> > UTF-normalization on relevant MacOS systems?\n> \n> I hate to bring up a totally different approach this late in the\n> party, but I wonder if it makes more sense to take advantage of\n> \"clone\" being a command that starts from an empty working tree.\n> \n> builtin/clone.c::checkout() drives a single-tree unpack_trees(),\n> using oneway_merge() as its callback and at the end, eventually\n> unpack_trees.c:check_updates() will call into checkout_entry()\n> to perform the usual \"unlink and then create\" dance.\n> \n> I wonder if it makes sense to introduce a new option to tell the\n> machinery to report when the final checkout_entry() notices that it\n> needed to remove the working tree file to make room (perhaps that\n> bit would go in \"struct unpack_trees_options\"). In the initial\n> checkout codepath for a freshly cloned repository, that would only\n> happen when your tree has two (or more) paths that gets smashed\n> by case insensitive or UTF-normalizing filesystem, and the code we\n> would maintain do not have to care how exactly the filesystem\n> collapses two (or more) paths if we go that way. We only need to\n> report \"we tried to check out X but it seems your filesystem equates\n> something else that is also in the project to X\".\n\nHeh. See my similar suggestion in:\n\n https://public-inbox.org/git/20180728095659.GA21450@sigill.intra.peff.net/\n\nand the response from Duy.\n\n-Peff\n"},{"id":"354109","messageId":"c174d972-fc5b-88e7-b437-89b9dd29e579@jeffhostetler.com","threadId":"48963","inReplyTo":"20180729092759.GA14484@sigill.intra.peff.net","subject":"Re: Git clone and case sensitivity","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2018-07-31T19:39:41Z","receivedAt":"2018-07-31T19:39:45Z","isPatch":false,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 7/29/2018 5:28 AM, Jeff King wrote:\n> On Sun, Jul 29, 2018 at 07:26:41AM +0200, Duy Nguyen wrote:\n> \n>>> strcasecmp() will only catch a subset of the cases. We really need to\n>>> follow the same folding rules that the filesystem would.\n>>\n>> True. But that's how we handle case insensitivity internally. If a\n>> filesytem has more sophisticated folding rules then git will not work\n>> well on that one anyway.\n> \n> Hrm. Yeah, I guess that's the best we can do for the actual in-memory\n> checks. Everything else depends on doing an actual filesystem operation,\n> and our icase stuff kicks in way before then. I was mostly thinking of\n> HFS+ utf8 normalization weirdness, but I guess people are accustomed to\n> that by now.\n> \n>>> For the case of clone, I actually wonder if we could detect during the\n>>> checkout step that a file already exists. Since we know that the\n>>> directory we started with was empty, then if it does, either:\n>>>\n>>> - there's some funny case-folding going on that means two paths in the\n>>> repository map to the same name in the filesystem; or\n>>>\n>>> - somebody else is writing to the directory at the same time as us\n>>\n>> This is exactly what my first patch does (minus the sparse checkout\n>> part).\n> \n> Right, sorry, I should have read that one more carefully.\n> \n>> But without knowing the exact folding rules, I don't think we can\n>> locate this \"somebody else\" who wrote the first path. So if N paths\n>> are treated the same by this filesystem, we could only report N-1 of\n>> them.\n>>\n>> If we want to report just one path when this happens though, then this\n>> works quite well.\n> \n> Hmm. Since most such systems are case-preserving, would it be possible\n> to report the name of the existing file? Doing it via opendir/readdir is\n> hacky, and anyway puts the burden on us to find the matching name. Doing\n> it via fstat() on the opened file doesn't work because at that the\n> filesystem has resolved the name to an inode.\n> \n> So yeah, perhaps strcasecmp() is the best we can do (I do agree that\n> being able to mention all of the conflicting names is a benefit).\n> \n> I guess we should be using fspathcmp(), though, in case it later learns\n> to be smarter.\n> \n> -Peff\n> \n\nAs has already been mentioned, this gets into weird territory really\nfast, between case folding, final space/dot on windows, utf8 NFC/NFD\nweirdness on the mac, utf8 invisible chars on the mac, long/short names\non windows, and etc.\n\nAnd that's just for filenames. Things really get weird if directory\nnames have these ambiguities.\n\nPerhaps just print the problematic paths (where the collision is\ndetected) and let the user decide how to correct them.\n\nPerhaps we could have a separate tool that could scan the index or\ncommit for potential conflicts and warn them in advance (granted, it\nmight not be perfect and may report a few false positives).\n\nForcing them into a sparse-checkout situation might be over their\nskill level.\n\nJeff\n"},{"id":"354116","messageId":"xmqqva8v9nc1.fsf@gitster-ct.c.googlers.com","threadId":"48963","inReplyTo":"20180731192931.GD3372@sigill.intra.peff.net","subject":"Re: [PATCH/RFC] clone: report duplicate entries on case-insensitive filesystems","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-31T20:12:14Z","receivedAt":"2018-07-31T20:12:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Jul 31, 2018 at 12:12:58PM -0700, Junio C Hamano wrote:\n> ...\n>> collapses two (or more) paths if we go that way. We only need to\n>> report \"we tried to check out X but it seems your filesystem equates\n>> something else that is also in the project to X\".\n>\n> Heh. See my similar suggestion in:\n>\n> https://public-inbox.org/git/20180728095659.GA21450@sigill.intra.peff.net/\n>\n> and the response from Duy.\n\nYes, but is there a reason why we need to report what that\n\"something else\" is?\n\nPresumably we are already in an error codepath, so if it is\nabsolutely necessary, then we can issue a lstat() to grab the inum\nfor the path we are about to create, iterate over the previously\nchecked out paths issuing lstat() and see which one yields the same\ninum, to find the one who is the culprit.\n\n"},{"id":"354120","messageId":"20180731203746.GA9442@sigill.intra.peff.net","threadId":"48963","inReplyTo":"xmqqva8v9nc1.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH/RFC] clone: report duplicate entries on case-insensitive filesystems","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-31T20:37:47Z","receivedAt":"2018-07-31T20:37:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 31, 2018 at 01:12:14PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Tue, Jul 31, 2018 at 12:12:58PM -0700, Junio C Hamano wrote:\n> > ...\n> >> collapses two (or more) paths if we go that way. We only need to\n> >> report \"we tried to check out X but it seems your filesystem equates\n> >> something else that is also in the project to X\".\n> >\n> > Heh. See my similar suggestion in:\n> >\n> > https://public-inbox.org/git/20180728095659.GA21450@sigill.intra.peff.net/\n> >\n> > and the response from Duy.\n> \n> Yes, but is there a reason why we need to report what that\n> \"something else\" is?\n\nI don't think it's strictly necessary, but it probably makes things\neasier for the user. That said...\n\n> Presumably we are already in an error codepath, so if it is\n> absolutely necessary, then we can issue a lstat() to grab the inum\n> for the path we are about to create, iterate over the previously\n> checked out paths issuing lstat() and see which one yields the same\n> inum, to find the one who is the culprit.\n\nYes, this is the cleverness I was missing in my earlier response.\n\nSo it seems do-able, and I like that this incurs no cost in the\nnon-error case.\n\n-Peff\n"},{"id":"354124","messageId":"xmqqin4v9l7u.fsf@gitster-ct.c.googlers.com","threadId":"48963","inReplyTo":"20180731203746.GA9442@sigill.intra.peff.net","subject":"Re: [PATCH/RFC] clone: report duplicate entries on case-insensitive filesystems","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-31T20:57:57Z","receivedAt":"2018-07-31T20:58:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> Presumably we are already in an error codepath, so if it is\n>> absolutely necessary, then we can issue a lstat() to grab the inum\n>> for the path we are about to create, iterate over the previously\n>> checked out paths issuing lstat() and see which one yields the same\n>> inum, to find the one who is the culprit.\n>\n> Yes, this is the cleverness I was missing in my earlier response.\n>\n> So it seems do-able, and I like that this incurs no cost in the\n> non-error case.\n\nNot so fast, unfortunately. \n\nI suspect that some filesystems do not give us inum that we can use\nfor that \"identity\" purpose, and they tend to be the ones with the\ncase smashing characteristics where we need this code in the error\npath the most X-<.\n"},{"id":"354157","messageId":"CACsJy8A_uZM7nUmyERNHJMya0EyRQYTV7Dp2ikLznxnbOQU6tw@mail.gmail.com","threadId":"48963","inReplyTo":"xmqqk1pbb4m2.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH/RFC] clone: report duplicate entries on case-insensitive filesystems","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-08-01T15:16:56Z","receivedAt":"2018-08-01T15:17:25Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jul 31, 2018 at 9:13 PM Junio C Hamano wrote:\n>\n> Nguyễn Thái Ngọc Duy writes:\n>\n> > Another thing we probably should do is catch in \"git checkout\" too,\n> > not just \"git clone\" since your linux/unix colleage colleague may\n> > accidentally add some files that your mac/windows machine is not very\n> > happy with.\n>\n> Then you would catch it not in checkout but in add, no?\n\nNo because the guy who added these may have done it on a\ncase-sensitive filesystem. Only later when his friend fetches new\nchanges on a case-insensitive filesytem, the problem becomes real. If\nin this scenario, core.ignore is enforced on all machines, then yes we\ncould catch it at \"git add\" (and we should already do that or we have\na bug). At least in open source project setting, I think enforcing\ncore.ignore will not work.\n-- \nDuy\n"},{"id":"354161","messageId":"CACsJy8C4znRAiQMUt_t85EUCad3HJUeeHd0z2g4uKZDfjWs1OA@mail.gmail.com","threadId":"48963","inReplyTo":"CABPp-BG+nB+ifRbCdMpXnnxQ+rzhM8W-=sfQf8TYmXvuPy5WXg@mail.gmail.com","subject":"Re: [PATCH/RFC] clone: report duplicate entries on case-insensitive filesystems","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-08-01T15:21:17Z","receivedAt":"2018-08-01T15:21:46Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jul 31, 2018 at 8:44 PM Elijah Newren wrote:\n> Is it worth attempting to also warn about paths that only differ in\n> UTF-normalization on relevant MacOS systems?\n\nDown this thread, Jeff Hostetler drew a scarier picture of \"case\"\nhandling on MacOS and Windows. I think we should start with support\nthings like utf normalization... in core.ignore first before doing the\nwarning stuff. At that point we know well how to detect and warn, or\nany other pitfalls.\n-- \nDuy\n"},{"id":"354163","messageId":"CACsJy8CBEFkbgoiTrsmsVy8KzU+KUPD+XN3weTrnUJUMAQn9xg@mail.gmail.com","threadId":"48963","inReplyTo":"20180731182344.GA3286@tor.lan","subject":"Re: [PATCH/RFC] clone: report duplicate entries on case-insensitive filesystems","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-08-01T15:25:16Z","receivedAt":"2018-08-01T15:25:45Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jul 31, 2018 at 8:23 PM Torsten Bögershausen wrote:\n> I wonder if we can tell the users more about the \"problems\"\n> and how to avoid them, or to live with them.\n>\n> This is more loud thinking:\n>\n> \"The following paths only differ in case\\n\"\n> \"One a case-insensitive file system only one at a time can be present\\n\"\n> \"You may rename one like this:\\n\"\n> \"git checkout && git mv .1\\n\"\n\nJeff gave a couple more options [1] to fix or workaround this. I think\nthe problem is there is no single recommended way to deal with it. If\nthere is, we can describe in this warning. But listing multiple\noptions in this warning may be too much (the wall of text could easily\ntake half a screen).\n\nOr if people agree on _one_ suggestion, I will gladly put it in.\n\n[1] https://public-inbox.org/git/CACsJy8A_uZM7nUmyERNHJMya0EyRQYTV7Dp2ikLznxnbOQU6tw@mail.gmail.com/T/#m60fedd7dc928a4d52eb5919811f84556f391a7b3\n\n> > + fprintf(stderr, \"\\t%s\\n\", dup.items[i].string);\n>\n> Another question:\n> Do we need any quote_path() here ?\n> (This may be overkill, since typically the repos with conflicting names\n> only use ASCII.)\n\nWould be good to show trailing spaces in path names, so yes.\n-- \nDuy\n"},{"id":"354196","messageId":"xmqq1sbh7phx.fsf@gitster-ct.c.googlers.com","threadId":"48963","inReplyTo":"xmqqin4v9l7u.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH/RFC] clone: report duplicate entries on case-insensitive filesystems","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-01T21:20:42Z","receivedAt":"2018-08-01T21:20:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>>> Presumably we are already in an error codepath, so if it is\n>>> absolutely necessary, then we can issue a lstat() to grab the inum\n>>> for the path we are about to create, iterate over the previously\n>>> checked out paths issuing lstat() and see which one yields the same\n>>> inum, to find the one who is the culprit.\n>>\n>> Yes, this is the cleverness I was missing in my earlier response.\n>>\n>> So it seems do-able, and I like that this incurs no cost in the\n>> non-error case.\n>\n> Not so fast, unfortunately. \n>\n> I suspect that some filesystems do not give us inum that we can use\n> for that \"identity\" purpose, and they tend to be the ones with the\n> case smashing characteristics where we need this code in the error\n> path the most X-<.\n\nBut even if inum is unreliable, we should be able to use other\nclues, perhaps the same set of fields we use for cached stat\nmatching optimization we use for \"diff\" plumbing commands, to\nimplement the error report. The more important part of the idea is\nthat we already need to notice that we need to remove a path that is\nin the working tree while doing the checkout, so the alternative\napproach won't incur any extra cost for normal cases where the\nproject being checked out does not have two files whose pathnames\nare only different in case (or checking out such an offending\nproject to a case sensitive filesytem, of course).\n\nSo I guess it still _is_ workable. Any takers?\n"},{"id":"354261","messageId":"CACsJy8DFX2=CaTomc33uuHQ-nBvgfutVbaQ2DxT_p8-hzj6PsA@mail.gmail.com","threadId":"48963","inReplyTo":"xmqq1sbh7phx.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH/RFC] clone: report duplicate entries on case-insensitive filesystems","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-08-02T14:43:53Z","receivedAt":"2018-08-02T14:44:22Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Aug 1, 2018 at 11:20 PM Junio C Hamano wrote:\n>\n> Junio C Hamano writes:\n>\n> > Jeff King writes:\n> >\n> >>> Presumably we are already in an error codepath, so if it is\n> >>> absolutely necessary, then we can issue a lstat() to grab the inum\n> >>> for the path we are about to create, iterate over the previously\n> >>> checked out paths issuing lstat() and see which one yields the same\n> >>> inum, to find the one who is the culprit.\n> >>\n> >> Yes, this is the cleverness I was missing in my earlier response.\n> >>\n> >> So it seems do-able, and I like that this incurs no cost in the\n> >> non-error case.\n> >\n> > Not so fast, unfortunately.\n> >\n> > I suspect that some filesystems do not give us inum that we can use\n> > for that \"identity\" purpose, and they tend to be the ones with the\n> > case smashing characteristics where we need this code in the error\n> > path the most X-<.\n>\n> But even if inum is unreliable, we should be able to use other\n> clues, perhaps the same set of fields we use for cached stat\n> matching optimization we use for \"diff\" plumbing commands, to\n> implement the error report. The more important part of the idea is\n> that we already need to notice that we need to remove a path that is\n> in the working tree while doing the checkout, so the alternative\n> approach won't incur any extra cost for normal cases where the\n> project being checked out does not have two files whose pathnames\n> are only different in case (or checking out such an offending\n> project to a case sensitive filesytem, of course).\n>\n> So I guess it still _is_ workable. Any takers?\n\nOK so we're going back to the original way of checking that we check\nout the different files on the same place (because fs is icase) and\ntry to collect all paths for reporting, yes? I can give it another go\n(but of course if anybody else steps up, I'd very gladly hand this\nover)\n-- \nDuy\n"},{"id":"354271","messageId":"xmqqpnz03f9o.fsf@gitster-ct.c.googlers.com","threadId":"48963","inReplyTo":"CACsJy8DFX2=CaTomc33uuHQ-nBvgfutVbaQ2DxT_p8-hzj6PsA@mail.gmail.com","subject":"Re: [PATCH/RFC] clone: report duplicate entries on case-insensitive filesystems","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-02T16:27:31Z","receivedAt":"2018-08-02T16:27:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n>> But even if inum is unreliable, we should be able to use other\n>> clues, perhaps the same set of fields we use for cached stat\n>> matching optimization we use for \"diff\" plumbing commands, to\n>> implement the error report. The more important part of the idea is\n>> that we already need to notice that we need to remove a path that is\n>> in the working tree while doing the checkout, so the alternative\n>> approach won't incur any extra cost for normal cases where the\n>> project being checked out does not have two files whose pathnames\n>> are only different in case (or checking out such an offending\n>> project to a case sensitive filesytem, of course).\n>>\n>> So I guess it still _is_ workable. Any takers?\n>\n> OK so we're going back to the original way of checking that we check\n> out the different files on the same place (because fs is icase) and\n> try to collect all paths for reporting, yes? I can give it another go\n> (but of course if anybody else steps up, I'd very gladly hand this\n> over)\n\nDetect and report, definitely yes; I am not sure about collect all\n(personally I am OK if we stopped at reporting \"I tried to check out\nX but your project tree has something else that is turned to X by\nyour pathname-smashing filesystem\" without making it a requirement\nto report what the other one that conflict with X is. Of course,\nreporting the other side _is_ nicer and I'd be happier if we can do\nso without too much ugly code, but I do not think it is a hard\nrequirement.\n\nThanks.\n"},{"id":"354300","messageId":"20180802190644.GE23690@sigill.intra.peff.net","threadId":"48963","inReplyTo":"xmqqpnz03f9o.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH/RFC] clone: report duplicate entries on case-insensitive filesystems","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-02T19:06:44Z","receivedAt":"2018-08-02T19:06:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 02, 2018 at 09:27:31AM -0700, Junio C Hamano wrote:\n\n> > OK so we're going back to the original way of checking that we check\n> > out the different files on the same place (because fs is icase) and\n> > try to collect all paths for reporting, yes? I can give it another go\n> > (but of course if anybody else steps up, I'd very gladly hand this\n> > over)\n> \n> Detect and report, definitely yes; I am not sure about collect all\n> (personally I am OK if we stopped at reporting \"I tried to check out\n> X but your project tree has something else that is turned to X by\n> your pathname-smashing filesystem\" without making it a requirement\n> to report what the other one that conflict with X is. Of course,\n> reporting the other side _is_ nicer and I'd be happier if we can do\n> so without too much ugly code, but I do not think it is a hard\n> requirement.\n\nYeah, I think it would be OK to issue the warning for the conflicted\npath, and then if we _can_ produce the secondary list of colliding\npaths, do so. Even on a system with working inodes, we may not come up\nwith a match (e.g., if it wasn't us who wrote the file, but rather we\nraced with some other process).\n\nI also wonder if Windows could return some other file-unique identifier\nthat would work in place of an inode here. That would be pretty easy to\nswap in via an #ifdef's helper function. I'd be OK shipping without that\nand letting Windows folks fill it in later (as long as we do not do\nanything too stupid until then, like claim all of the inode==0 files are\nthe same).\n\n-Peff\n\nPS It occurs to me that doing this naively (re-scan the entries already\n checked out when we see a collision) ends up quadratic over the\n number of entries in the worst case. That may not matter. You'd only\n have a handful of collisions normally, and anybody malicious can\n already git-bomb your checkout anyway. If we care, an alternative\n would be to set a flag for \"I saw some collisions\", and then follow\n up with a single pass putting entries into a hashmap of\n inode->filename.\n"},{"id":"354326","messageId":"xmqqmuu4zd1l.fsf@gitster-ct.c.googlers.com","threadId":"48963","inReplyTo":"20180802190644.GE23690@sigill.intra.peff.net","subject":"Re: [PATCH/RFC] clone: report duplicate entries on case-insensitive filesystems","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-02T21:14:30Z","receivedAt":"2018-08-02T21:14:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I also wonder if Windows could return some other file-unique identifier\n> that would work in place of an inode here. That would be pretty easy to\n> swap in via an #ifdef's helper function. I'd be OK shipping without that\n> and letting Windows folks fill it in later (as long as we do not do\n> anything too stupid until then, like claim all of the inode==0 files are\n> the same).\n\nYeah, but such a useful file-unique identifier would probably be\nused in place of inum in their (l)stat emulation already, if exists,\nno?\n\n> PS It occurs to me that doing this naively (re-scan the entries already\n> checked out when we see a collision) ends up quadratic over the\n> number of entries in the worst case. That may not matter. You'd only\n> have a handful of collisions normally, and anybody malicious can\n> already git-bomb your checkout anyway. If we care, an alternative\n> would be to set a flag for \"I saw some collisions\", and then follow\n> up with a single pass putting entries into a hashmap of\n> inode->filename.\n\nYeah, that makes sense.\n"},{"id":"354330","messageId":"20180802212819.GA32538@sigill.intra.peff.net","threadId":"48963","inReplyTo":"xmqqmuu4zd1l.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH/RFC] clone: report duplicate entries on case-insensitive filesystems","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-02T21:28:19Z","receivedAt":"2018-08-02T21:28:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 02, 2018 at 02:14:30PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I also wonder if Windows could return some other file-unique identifier\n> > that would work in place of an inode here. That would be pretty easy to\n> > swap in via an #ifdef's helper function. I'd be OK shipping without that\n> > and letting Windows folks fill it in later (as long as we do not do\n> > anything too stupid until then, like claim all of the inode==0 files are\n> > the same).\n> \n> Yeah, but such a useful file-unique identifier would probably be\n> used in place of inum in their (l)stat emulation already, if exists,\n> no?\n\nMaybe. It might not work as ino_t. Or it might be expensive to get. Or\nmaybe it's simply impossible. I don't know much about Windows. Some\nsearching implies that NTFS does have a \"file index\" concept which is\nsupposed to be unique.\n\nAt any rate, until we have an actual plan for Windows, I think it would\nmake sense only to split the cases into \"has working inodes\" and\n\"other\", and make sure \"other\" does something sensible in the meantime\n(like mention the conflict, but skip trying to list duplicates).\n\nWhen somebody wants to work on Windows support, then we can figure out\nif it just needs to wrap the \"get unique identifier\" operation, or if it\nwould use a totally different algorithm.\n\n-Peff\n"},{"id":"354391","messageId":"c4ce4d55-ad10-55b0-0cb0-89025102210c@web.de","threadId":"48963","inReplyTo":"CACsJy8DFX2=CaTomc33uuHQ-nBvgfutVbaQ2DxT_p8-hzj6PsA@mail.gmail.com","subject":"Re: [PATCH/RFC] clone: report duplicate entries on case-insensitive filesystems","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2018-08-03T14:28:38Z","receivedAt":"2018-08-03T14:29:09Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2018-08-02 15:43, Duy Nguyen wrote:\n> On Wed, Aug 1, 2018 at 11:20 PM Junio C Hamano wrote:\n>>\n>> Junio C Hamano writes:\n>>\n>>> Jeff King writes:\n>>>\n>>>>> Presumably we are already in an error codepath, so if it is\n>>>>> absolutely necessary, then we can issue a lstat() to grab the inum\n>>>>> for the path we are about to create, iterate over the previously\n>>>>> checked out paths issuing lstat() and see which one yields the same\n>>>>> inum, to find the one who is the culprit.\n>>>>\n>>>> Yes, this is the cleverness I was missing in my earlier response.\n>>>>\n>>>> So it seems do-able, and I like that this incurs no cost in the\n>>>> non-error case.\n>>>\n>>> Not so fast, unfortunately.\n>>>\n>>> I suspect that some filesystems do not give us inum that we can use\n>>> for that \"identity\" purpose, and they tend to be the ones with the\n>>> case smashing characteristics where we need this code in the error\n>>> path the most X-<.\n>>\n>> But even if inum is unreliable, we should be able to use other\n>> clues, perhaps the same set of fields we use for cached stat\n>> matching optimization we use for \"diff\" plumbing commands, to\n>> implement the error report. The more important part of the idea is\n>> that we already need to notice that we need to remove a path that is\n>> in the working tree while doing the checkout, so the alternative\n>> approach won't incur any extra cost for normal cases where the\n>> project being checked out does not have two files whose pathnames\n>> are only different in case (or checking out such an offending\n>> project to a case sensitive filesytem, of course).\n>>\n>> So I guess it still _is_ workable. Any takers?\n> \n> OK so we're going back to the original way of checking that we check\n> out the different files on the same place (because fs is icase) and\n> try to collect all paths for reporting, yes?\n\nI would say: Yes.\n\n> I can give it another go\n> (but of course if anybody else steps up, I'd very gladly hand this\n> over)\n> \n\nNot at the moment.\n\n\n"},{"id":"354422","messageId":"5b17454b-7fa7-7a9c-92d9-214e6e697785@jeffhostetler.com","threadId":"48963","inReplyTo":"20180802212819.GA32538@sigill.intra.peff.net","subject":"Re: [PATCH/RFC] clone: report duplicate entries on case-insensitive filesystems","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2018-08-03T18:23:17Z","receivedAt":"2018-08-03T18:23:23Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 8/2/2018 5:28 PM, Jeff King wrote:\n> On Thu, Aug 02, 2018 at 02:14:30PM -0700, Junio C Hamano wrote:\n> \n>> Jeff King writes:\n>>\n>>> I also wonder if Windows could return some other file-unique identifier\n>>> that would work in place of an inode here. That would be pretty easy to\n>>> swap in via an #ifdef's helper function. I'd be OK shipping without that\n>>> and letting Windows folks fill it in later (as long as we do not do\n>>> anything too stupid until then, like claim all of the inode==0 files are\n>>> the same).\n>>\n>> Yeah, but such a useful file-unique identifier would probably be\n>> used in place of inum in their (l)stat emulation already, if exists,\n>> no?\n> \n> Maybe. It might not work as ino_t. Or it might be expensive to get. Or\n> maybe it's simply impossible. I don't know much about Windows. Some\n> searching implies that NTFS does have a \"file index\" concept which is\n> supposed to be unique.\n\nThis is hard and/or expensive on Windows. Yes, you can get the\n\"file index\" values for an open file handle with a cost similar to\nan fstat(). Unfortunately, the FindFirst/FindNext routines (equivalent\nto the opendir/readdir routines), don't give you that data. So we'd\nhave to scan the directory and then open and stat each file. This is\nterribly expensive on Windows -- and the reason we have the fscache\nlayer (in the GfW version) to intercept the lstat() calls whenever\npossible.\n\nIt might be possible to use the NTFS Master File Table to discover\nthis (very big handwave), but I would need to do a little digging.\n\nThis would all be NTFS specific. FAT and other volume types would not\nbe covered.\n\nAnother thing to keep in mind is that the collision could be because\nof case folding (or other such nonsense) on a directory in the path.\nI mean, if someone on Linux builds a commit containing:\n\n a/b/c/D/e/foo.txt\n a/b/c/d/e/foo.txt\n\nwe'll get a similar collision as if one of them were spelled \"FOO.txt\".\n\nAlso, do we need to worry about hard-links or symlinks here?\nIf checkout populates symlinks, then you might have another collision\nopportunity. For example:\n\n a/b/c/D/e/foo.txt\n a/link -> ./b/c/d\n a/link/e/foo.txt\n\nAlso, some platforms (like the Mac) allow directory hard-links.\nGranted, Git doesn't create hard-links during checkout, but the\nuser might.\n\nI'm sure there are other edge cases here that make reporting\ndifficult; these are just a few I thought of. I guess what I'm\ntrying to say is that as a first step just report that you found\na collision -- without trying to identify the set existing objects\nthat it collided with.\n\n> \n> At any rate, until we have an actual plan for Windows, I think it would\n> make sense only to split the cases into \"has working inodes\" and\n> \"other\", and make sure \"other\" does something sensible in the meantime\n> (like mention the conflict, but skip trying to list duplicates).\n\nYes, this should be split. Do the \"easy\" Linux version first.\nKeep in mind that there may also be a different solution for the Mac.\n\n> When somebody wants to work on Windows support, then we can figure out\n> if it just needs to wrap the \"get unique identifier\" operation, or if it\n> would use a totally different algorithm.\n> \n> -Peff\n> \n\nJeff\n"},{"id":"354427","messageId":"xmqqsh3vwaj9.fsf@gitster-ct.c.googlers.com","threadId":"48963","inReplyTo":"5b17454b-7fa7-7a9c-92d9-214e6e697785@jeffhostetler.com","subject":"Re: [PATCH/RFC] clone: report duplicate entries on case-insensitive filesystems","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-03T18:49:14Z","receivedAt":"2018-08-03T18:49:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff Hostetler writes:\n\n> Another thing to keep in mind is that the collision could be because\n> of case folding (or other such nonsense) on a directory in the path.\n> I mean, if someone on Linux builds a commit containing:\n>\n> a/b/c/D/e/foo.txt\n> a/b/c/d/e/foo.txt\n>\n> we'll get a similar collision as if one of them were spelled \"FOO.txt\".\n\nI'd think the approach to teach checkout_entry() codepath to notice\nit needed to unlink the existing file in order to check out the\nentry it wanted to check out would cover this equally well.\n\n> Also, do we need to worry about hard-links or symlinks here?\n\nI do not think so. You do not get a file with multiple hardlinks\nin a \"git clone\" or \"git checkout\" result, and we do not check\nthings out beyond a symbolic link in the first place.\n\n> If checkout populates symlinks, then you might have another collision\n> opportunity. For example:\n>\n> a/b/c/D/e/foo.txt\n> a/link -> ./b/c/d\n> a/link/e/foo.txt\n\nIn other words, a tree with a/link (symlink) and a/link/\nthat requires a/link to be a symlink and a directory at the same\ntime cannot be created, so you won't get one with \"git clone\"\n\n> Also, some platforms (like the Mac) allow directory hard-links.\n> Granted, Git doesn't create hard-links during checkout, but the\n> user might.\n\nAnd we'd report \"we are doing a fresh checkout immediately after a\nclone and saw some file we haven't created, which may indicate a\ncase smashing filesystem glitch (or a competing third-party process\ncreating random files)\", so noticing that would be a good thing, I\nwould think.\n\n> I'm sure there are other edge cases here that make reporting\n> difficult; these are just a few I thought of. I guess what I'm\n> trying to say is that as a first step just report that you found\n> a collision -- without trying to identify the set existing objects\n> that it collided with.\n\nYup, I think that is sensible. If it can be done cheaply, i.e. on a\nfilesystem with trustable and cheap inum, after noticing such a\ncollision, go back and lstat() all paths in the index we have\nchecked out so far to see which ones are colliding, it adds useful\nclue to the report, but noticing the collision in the first place\nobviously has more value.\n"},{"id":"354429","messageId":"20180803185325.GA27977@sigill.intra.peff.net","threadId":"48963","inReplyTo":"5b17454b-7fa7-7a9c-92d9-214e6e697785@jeffhostetler.com","subject":"Re: [PATCH/RFC] clone: report duplicate entries on case-insensitive filesystems","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-03T18:53:26Z","receivedAt":"2018-08-03T18:53:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 03, 2018 at 02:23:17PM -0400, Jeff Hostetler wrote:\n\n> > Maybe. It might not work as ino_t. Or it might be expensive to get. Or\n> > maybe it's simply impossible. I don't know much about Windows. Some\n> > searching implies that NTFS does have a \"file index\" concept which is\n> > supposed to be unique.\n> \n> This is hard and/or expensive on Windows. Yes, you can get the\n> \"file index\" values for an open file handle with a cost similar to\n> an fstat(). Unfortunately, the FindFirst/FindNext routines (equivalent\n> to the opendir/readdir routines), don't give you that data. So we'd\n> have to scan the directory and then open and stat each file. This is\n> terribly expensive on Windows -- and the reason we have the fscache\n> layer (in the GfW version) to intercept the lstat() calls whenever\n> possible.\n\nI think that high cost might be OK for our purposes here. This code\nwould _only_ kick in during a clone, and then only on the error path\nonce we knew we had a collision during the checkout step.\n\n> Another thing to keep in mind is that the collision could be because\n> of case folding (or other such nonsense) on a directory in the path.\n> I mean, if someone on Linux builds a commit containing:\n> \n> a/b/c/D/e/foo.txt\n> a/b/c/d/e/foo.txt\n> \n> we'll get a similar collision as if one of them were spelled \"FOO.txt\".\n\nTrue, though I think that may be OK. If you had conflicting directories\nyou'd get a _ton_ of duplicates listed, but that makes sense: you\nactually have a ton of duplicates.\n\n> Also, do we need to worry about hard-links or symlinks here?\n\nI think we can ignore hardlinks. Git never creates them, and we know the\ndirectory was empty when we started. Symlinks should be handled by using\nlstat(). (Obviously that's for a Unix-ish platform).\n\n> I'm sure there are other edge cases here that make reporting\n> difficult; these are just a few I thought of. I guess what I'm\n> trying to say is that as a first step just report that you found\n> a collision -- without trying to identify the set existing objects\n> that it collided with.\n\nI certainly don't disagree with that. :)\n\n> > At any rate, until we have an actual plan for Windows, I think it would\n> > make sense only to split the cases into \"has working inodes\" and\n> > \"other\", and make sure \"other\" does something sensible in the meantime\n> > (like mention the conflict, but skip trying to list duplicates).\n> \n> Yes, this should be split. Do the \"easy\" Linux version first.\n> Keep in mind that there may also be a different solution for the Mac.\n\nI assumed that an inode-based solution would work for Mac, since it's\nmostly BSD under the hood. There may be subtleties I don't know about,\nthough.\n\n-Peff\n"},{"id":"354549","messageId":"9eb6caf1-b4aa-766d-922e-b2b24b391b33@jeffhostetler.com","threadId":"48963","inReplyTo":"20180803185325.GA27977@sigill.intra.peff.net","subject":"Re: [PATCH/RFC] clone: report duplicate entries on case-insensitive filesystems","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2018-08-05T14:01:49Z","receivedAt":"2018-08-05T14:03:08Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 8/3/2018 2:53 PM, Jeff King wrote:\n> On Fri, Aug 03, 2018 at 02:23:17PM -0400, Jeff Hostetler wrote:\n> \n>>> Maybe. It might not work as ino_t. Or it might be expensive to get. Or\n>>> maybe it's simply impossible. I don't know much about Windows. Some\n>>> searching implies that NTFS does have a \"file index\" concept which is\n>>> supposed to be unique.\n>>\n>> This is hard and/or expensive on Windows. Yes, you can get the\n>> \"file index\" values for an open file handle with a cost similar to\n>> an fstat(). Unfortunately, the FindFirst/FindNext routines (equivalent\n>> to the opendir/readdir routines), don't give you that data. So we'd\n>> have to scan the directory and then open and stat each file. This is\n>> terribly expensive on Windows -- and the reason we have the fscache\n>> layer (in the GfW version) to intercept the lstat() calls whenever\n>> possible.\n> \n> I think that high cost might be OK for our purposes here. This code\n> would _only_ kick in during a clone, and then only on the error path\n> once we knew we had a collision during the checkout step.\n> \n\nGood point.\n\nI've confirmed that the \"file index\" values can be used to determine\nwhether 2 path names are equivalent under NTFS for case variation,\nfinal-dot/space, and short-names vs long-names.\n\nI ran out of time this morning to search the directory for equivalent \npaths. I'll look at that shortly.\n\nJeff\n"},{"id":"354762","messageId":"20180807190110.16216-1-pclouds@gmail.com","threadId":"48963","inReplyTo":"20180730152756.15012-1-pclouds@gmail.com","subject":"[PATCH v2] clone: report duplicate entries on case-insensitive filesystems","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-08-07T19:01:10Z","receivedAt":"2018-08-07T19:01:25Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Paths that only differ in case work fine in a case-sensitive\nfilesystems, but if those repos are cloned in a case-insensitive one,\nyou'll get problems. The first thing to notice is \"git status\" will\nnever be clean with no indication what exactly is \"dirty\".\n\nThis patch helps the situation a bit by pointing out the problem at\nclone time. Even though this patch talks about case sensitivity, the\npatch makes no assumption about folding rules by the filesystem. It\nsimply observes that if an entry has been already checked out at clone\ntime when we're about to write a new path, some folding rules are\nbehind this.\n\nI do not make any suggestions to fix or workaround the problem because\nthere are many different options, especially when the problem comes\nfrom folding rules other than case (e.g. UTF-8 normalization, Windows\nspecial paths...)\n\nIn the previous iteration, inode has been suggested to find the\nmatching entry. But it is platform specific, and because we already\nhave a common function for matching stat, the function is used here\neven if it's more expensive. Bonus point is we don't need some \"#ifdef\nplatform\" around this code.\n\nThe cost goes higher when we find duplicated entries at the bottom of\nthe index, but the number of these entries should be very small that\ntotal extra cost should not be really noticeable.\n\nThis patch is tested with vim-colorschemes repository on a JFS partion\nwith case insensitive support on Linux. This repository has two files\ndarkBlue.vim and darkblue.vim.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n v2 has completely different approach so no point in sending\n interdiff.\n\n One nice thing about this is we don't need platform specific code for\n detecting the duplicate entries. I think ce_match_stat() works even\n on Windows. And it's now equally expensive on all platforms :D\n\n builtin/clone.c | 1 +\n cache.h | 2 ++\n entry.c | 44 ++++++++++++++++++++++++++++++++++++++++++++\n unpack-trees.c | 23 +++++++++++++++++++++++\n unpack-trees.h | 1 +\n 5 files changed, 71 insertions(+)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 9ebb5acf56..38d5609282 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -748,6 +748,7 @@ static int checkout(int submodule_progress)\n \tmemset(&opts, 0, sizeof opts);\n \topts.update = 1;\n \topts.merge = 1;\n+\topts.clone = 1;\n \topts.fn = oneway_merge;\n \topts.verbose_update = (option_verbosity >= 0);\n \topts.src_index = &the_index;\ndiff --git a/cache.h b/cache.h\nindex 8dc7134f00..cdf0984707 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1515,9 +1515,11 @@ struct checkout {\n \tconst char *base_dir;\n \tint base_dir_len;\n \tstruct delayed_checkout *delayed_checkout;\n+\tint *nr_duplicates;\n \tunsigned force:1,\n \t\t quiet:1,\n \t\t not_new:1,\n+\t\t clone:1,\n \t\t refresh_cache:1;\n };\n #define CHECKOUT_INIT { NULL, \"\" }\ndiff --git a/entry.c b/entry.c\nindex b5d1d3cf23..3917bfc874 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -399,6 +399,47 @@ static int check_path(const char *path, int len, struct stat *st, int skiplen)\n \treturn lstat(path, st);\n }\n \n+static void mark_duplicate_entries(const struct checkout *state,\n+\t\t\t\t struct cache_entry *ce, struct stat *st)\n+{\n+\tint i;\n+\tint *count = state->nr_duplicates;\n+\n+\tif (!count)\n+\t\tBUG(\"state->nr_duplicates must not be NULL\");\n+\n+\tce->ce_flags |= CE_MATCHED;\n+\t(*count)++;\n+\n+\tif (!state->refresh_cache)\n+\t\tBUG(\"We need this to narrow down the set of updated entries\");\n+\n+\tfor (i = 0; i < state->istate->cache_nr; i++) {\n+\t\tstruct cache_entry *dup = state->istate->cache[i];\n+\n+\t\t/*\n+\t\t * This entry has not been checked out yet, otherwise\n+\t\t * its stat info must have been updated. And since we\n+\t\t * check out from top to bottom, the rest is guaranteed\n+\t\t * not checked out. Stop now.\n+\t\t */\n+\t\tif (!ce_uptodate(dup))\n+\t\t\tbreak;\n+\n+\t\tif (dup->ce_flags & CE_MATCHED)\n+\t\t\tcontinue;\n+\n+\t\tif (ce_match_stat(dup, st,\n+\t\t\t\t CE_MATCH_IGNORE_VALID |\n+\t\t\t\t CE_MATCH_IGNORE_SKIP_WORKTREE))\n+\t\t\tcontinue;\n+\n+\t\tdup->ce_flags |= CE_MATCHED;\n+\t\t(*count)++;\n+\t\tbreak;\n+\t}\n+}\n+\n /*\n * Write the contents from ce out to the working tree.\n *\n@@ -455,6 +496,9 @@ int checkout_entry(struct cache_entry *ce,\n \t\t\treturn -1;\n \t\t}\n \n+\t\tif (state->clone)\n+\t\t\tmark_duplicate_entries(state, ce, &st);\n+\n \t\t/*\n \t\t * We unlink the old file, to get the new one with the\n \t\t * right permissions (including umask, which is nasty\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex f9efee0836..1b0c11142a 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -344,12 +344,20 @@ static int check_updates(struct unpack_trees_options *o)\n \tstruct index_state *index = &o->result;\n \tstruct checkout state = CHECKOUT_INIT;\n \tint i;\n+\tint nr_duplicates = 0;\n \n \tstate.force = 1;\n \tstate.quiet = 1;\n \tstate.refresh_cache = 1;\n \tstate.istate = index;\n \n+\tif (o->clone) {\n+\t\tstate.clone = 1;\n+\t\tstate.nr_duplicates = &nr_duplicates;\n+\t\tfor (i = 0; i < index->cache_nr; i++)\n+\t\t\tindex->cache[i]->ce_flags &= ~CE_MATCHED;\n+\t}\n+\n \tprogress = get_progress(o);\n \n \tif (o->update)\n@@ -414,6 +422,21 @@ static int check_updates(struct unpack_trees_options *o)\n \terrs |= finish_delayed_checkout(&state);\n \tif (o->update)\n \t\tgit_attr_set_direction(GIT_ATTR_CHECKIN, NULL);\n+\n+\tif (o->clone && state.nr_duplicates) {\n+\t\twarning(_(\"the following paths in this repository only differ in case\\n\"\n+\t\t\t \"from another path and will cause problems because you have cloned\\n\"\n+\t\t\t \"it on an case-insensitive filesytem:\\n\"));\n+\t\tfor (i = 0; i < index->cache_nr; i++) {\n+\t\t\tstruct cache_entry *ce = index->cache[i];\n+\n+\t\t\tif (!(ce->ce_flags & CE_MATCHED))\n+\t\t\t\tcontinue;\n+\t\t\tfprintf(stderr, \" '%s'\\n\", ce->name);\n+\t\t\tce->ce_flags &= ~CE_MATCHED;\n+\t\t}\n+\t}\n+\n \treturn errs != 0;\n }\n \ndiff --git a/unpack-trees.h b/unpack-trees.h\nindex c2b434c606..d940f1c5c2 100644\n--- a/unpack-trees.h\n+++ b/unpack-trees.h\n@@ -42,6 +42,7 @@ struct unpack_trees_options {\n \tunsigned int reset,\n \t\t merge,\n \t\t update,\n+\t\t clone,\n \t\t index_only,\n \t\t nontrivial_merge,\n \t\t trivial_merges_only,\n-- \n2.18.0.915.gd571298aae\n\n"},{"id":"354766","messageId":"xmqq7el2km82.fsf@gitster-ct.c.googlers.com","threadId":"48963","inReplyTo":"20180807190110.16216-1-pclouds@gmail.com","subject":"Re: [PATCH v2] clone: report duplicate entries on case-insensitive filesystems","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-07T19:31:09Z","receivedAt":"2018-08-07T19:31:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> One nice thing about this is we don't need platform specific code for\n> detecting the duplicate entries. I think ce_match_stat() works even\n> on Windows. And it's now equally expensive on all platforms :D\n\nce_match_stat() may not be a very good measure to see if two paths\nrefer to the same file, though. After a fresh checkout, I would not\nbe surprised if two completely unrelated paths have the same size\nand have same mtime/ctime. In its original use case, i.e. \"I have\none specific path in mind and took a snapshot of its metadata\nearlier. Is it still the same, or has it been touched?\", that may\nbe sufficient to detect that the path has been _modified_, but\nwithout reliable inum, it may be a poor measure to say two paths\nrefer to the same.\n\n> builtin/clone.c | 1 +\n> cache.h | 2 ++\n> entry.c | 44 ++++++++++++++++++++++++++++++++++++++++++++\n> unpack-trees.c | 23 +++++++++++++++++++++++\n> unpack-trees.h | 1 +\n> 5 files changed, 71 insertions(+)\n\nHaving said that, it is pleasing to see that this can be achieved\nwith so little additional code.\n\n> +static void mark_duplicate_entries(const struct checkout *state,\n> +\t\t\t\t struct cache_entry *ce, struct stat *st)\n> +{\n> +\tint i;\n> +\tint *count = state->nr_duplicates;\n> +\n> +\tif (!count)\n> +\t\tBUG(\"state->nr_duplicates must not be NULL\");\n> +\n> +\tce->ce_flags |= CE_MATCHED;\n> +\t(*count)++;\n> +\n> +\tif (!state->refresh_cache)\n> +\t\tBUG(\"We need this to narrow down the set of updated entries\");\n> +\n> +\tfor (i = 0; i < state->istate->cache_nr; i++) {\n> +\t\tstruct cache_entry *dup = state->istate->cache[i];\n> +\n> +\t\t/*\n> +\t\t * This entry has not been checked out yet, otherwise\n> +\t\t * its stat info must have been updated. And since we\n> +\t\t * check out from top to bottom, the rest is guaranteed\n> +\t\t * not checked out. Stop now.\n> +\t\t */\n> +\t\tif (!ce_uptodate(dup))\n> +\t\t\tbreak;\n> +\n> +\t\tif (dup->ce_flags & CE_MATCHED)\n> +\t\t\tcontinue;\n> +\n> +\t\tif (ce_match_stat(dup, st,\n> +\t\t\t\t CE_MATCH_IGNORE_VALID |\n> +\t\t\t\t CE_MATCH_IGNORE_SKIP_WORKTREE))\n> +\t\t\tcontinue;\n> +\n> +\t\tdup->ce_flags |= CE_MATCHED;\n> +\t\t(*count)++;\n> +\t\tbreak;\n> +\t}\n> +}\n> +\n\nHmph. If there is only one true collision, then all its aliases\nwill be marked with CE_MATCHED bit every time the second and the\nsubsequent alias is checked out (as the caller calls this function\nwhen it noticed that something already is at the path ce wants to\ngo). But if there are two sets of colliding paths, because there is\nonly one bit used, we do not group the paths into these two sets and\nreport, e.g. \"blue.txt, BLUE.txt and BLUE.TXT collide. red.txt and\nRED.txt also collide.\" I am not sure if computing that is too much\nwork for too little gain, but because this is in an error codepath,\nit may be worth doing. I dunno.\n\n> +\n> +\tif (o->clone && state.nr_duplicates) {\n> +\t\twarning(_(\"the following paths in this repository only differ in case\\n\"\n> +\t\t\t \"from another path and will cause problems because you have cloned\\n\"\n> +\t\t\t \"it on an case-insensitive filesytem:\\n\"));\n\nWith the new approach, we no longer preemptively detect that the\nproject will be harmed by a case smashing filesystems before it\nhappens. This instead reports that the project has already been\nharmed on _this_ system by such a filesystem after the fact.\n\nSo from the end-user's point of view, \"will cause problems\" may be a\nmessage that came a bit too late. \"have collided and only one from\nthe same colliding group is in the working tree; others failed to be\nchecked out\" is probably closer to the truth.\n\n"},{"id":"354919","messageId":"fc56d572-e333-2e05-2130-71b53e251a13@jeffhostetler.com","threadId":"48963","inReplyTo":"xmqq7el2km82.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] clone: report duplicate entries on case-insensitive filesystems","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2018-08-08T19:48:04Z","receivedAt":"2018-08-08T19:48:10Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 8/7/2018 3:31 PM, Junio C Hamano wrote:\n> Nguyễn Thái Ngọc Duy writes:\n> \n>> One nice thing about this is we don't need platform specific code for\n>> detecting the duplicate entries. I think ce_match_stat() works even\n>> on Windows. And it's now equally expensive on all platforms :D\n> \n> ce_match_stat() may not be a very good measure to see if two paths\n> refer to the same file, though. After a fresh checkout, I would not\n> be surprised if two completely unrelated paths have the same size\n> and have same mtime/ctime. In its original use case, i.e. \"I have\n> one specific path in mind and took a snapshot of its metadata\n> earlier. Is it still the same, or has it been touched?\", that may\n> be sufficient to detect that the path has been _modified_, but\n> without reliable inum, it may be a poor measure to say two paths\n> refer to the same.\n\nI agree with Junio on this one. The mtime values are sloppy at best.\nOn FAT file systems, they have 2 second resolution. Even NTFS IIRC\nhas only 100ns resolution, so we might get a lot of false matches\nusing this technique, right?\n\nIt might be better to build an equivalence-class hash-map for the\ncolliding entries. Compute a \"normalized\" version of the pathname\n(such as convert to lowercase, strip final-dots/spaces, strip the\ndigits following tilda of a shortname, and etc for the MAC's UTF-isms).\nThen when you rescan the index entries to find the matches, apply the\nequivalence operator on the pathname and do the hashmap lookup.\nWhen you find a match, you have a \"potential\" collider pair (I say\npotential only because of the ambiguity of shortnames). Then we\ncan use inum/file-index/whatever to see if they actually collide.\n\n\nJeff\n"},{"id":"354944","messageId":"20180808223139.GA3902@sigill.intra.peff.net","threadId":"48963","inReplyTo":"fc56d572-e333-2e05-2130-71b53e251a13@jeffhostetler.com","subject":"Re: [PATCH v2] clone: report duplicate entries on case-insensitive filesystems","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-08T22:31:39Z","receivedAt":"2018-08-08T22:31:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 08, 2018 at 03:48:04PM -0400, Jeff Hostetler wrote:\n\n> > ce_match_stat() may not be a very good measure to see if two paths\n> > refer to the same file, though. After a fresh checkout, I would not\n> > be surprised if two completely unrelated paths have the same size\n> > and have same mtime/ctime. In its original use case, i.e. \"I have\n> > one specific path in mind and took a snapshot of its metadata\n> > earlier. Is it still the same, or has it been touched?\", that may\n> > be sufficient to detect that the path has been _modified_, but\n> > without reliable inum, it may be a poor measure to say two paths\n> > refer to the same.\n> \n> I agree with Junio on this one. The mtime values are sloppy at best.\n> On FAT file systems, they have 2 second resolution. Even NTFS IIRC\n> has only 100ns resolution, so we might get a lot of false matches\n> using this technique, right?\n\nYeah, I think anything less than inode (or some system equivalent) is\ngoing to be too flaky.\n\n> It might be better to build an equivalence-class hash-map for the\n> colliding entries. Compute a \"normalized\" version of the pathname\n> (such as convert to lowercase, strip final-dots/spaces, strip the\n> digits following tilda of a shortname, and etc for the MAC's UTF-isms).\n> Then when you rescan the index entries to find the matches, apply the\n> equivalence operator on the pathname and do the hashmap lookup.\n> When you find a match, you have a \"potential\" collider pair (I say\n> potential only because of the ambiguity of shortnames). Then we\n> can use inum/file-index/whatever to see if they actually collide.\n\nI think we really want to avoid doing that normalization ourselves if we\ncan. There are just too many filesystem-specific rules.\n\nIf we have an equivalence-class hashmap and feed it inodes (or again,\nsome system equivalent) as the keys, we should get buckets of\ncollisions. I started to write a \"something like this...\" earlier, but\ngot bogged down in boilerplate around the C hashmap.\n\nBut here it is in perl. ;)\n\n-- >8 --\n# pretend we have these paths in our index\npaths='foo FOO and some other paths'\n\n# create them; this will make a single path on a case-insensitive system\nfor i in $paths; do\n echo $i >$i\ndone\n\n# now find the duplicates\nperl -le '\n for my $path (@ARGV) {\n # this would be an ntfs unique-id on Windows\n my $inode = (lstat($path))[1];\n push @{$h{$inode}}, $path;\n }\n\n for my $group (grep { @$_ > 1 } values(%h)) {\n print \"group:\";\n print \" \", $_ for (@$group);\n }\n' $paths\n-- >8 --\n\nwhich should show the obvious pair (it does for me on vfat-on-linux,\nthough where it gets those inodes from, I have no idea ;) ).\n\n-Peff\n"},{"id":"354961","messageId":"xmqqbmace5i1.fsf@gitster-ct.c.googlers.com","threadId":"48963","inReplyTo":"20180808223139.GA3902@sigill.intra.peff.net","subject":"Re: [PATCH v2] clone: report duplicate entries on case-insensitive filesystems","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-09T00:41:10Z","receivedAt":"2018-08-09T00:41:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I think we really want to avoid doing that normalization ourselves if we\n> can. There are just too many filesystem-specific rules.\n\nExactly; not having to learn these rules is the major (if not whole)\npoint of the \"let checkout notice the collision and then deal with\nit\" approach. Let's not forget that.\n\n> If we have an equivalence-class hashmap and feed it inodes (or again,\n> some system equivalent) as the keys, we should get buckets of\n> collisions.\n\nI guess one way to get \"some system equivalent\" that can be used as\nthe last resort, when there absolutely is no inum equivalent, is to\nrehash the working tree file that shouldn't be there when we detect\na collision.\n\nIf we found that there is something when we tried to write out\n\"Foo.txt\", if we open \"Foo.txt\" on the working tree and hash-object\nit, we should find the matching blob somewhere in the index _before_\n\"Foo.txt\". On a case-insensitive filesytem, it may well be\n\"foo.txt\", but we do not even have to know \"foo.txt\" and \"Foo.txt\"\nonly differ in case.\n\nOf course, that's really the last resort, as it would be costly, but\nthis is something that only need to happen on the \"unusual\" case in\nthe error codepath, so...\n"},{"id":"354991","messageId":"20180809142333.GB1439@sigill.intra.peff.net","threadId":"48963","inReplyTo":"xmqqbmace5i1.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] clone: report duplicate entries on case-insensitive filesystems","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-09T14:23:34Z","receivedAt":"2018-08-09T14:23:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 08, 2018 at 05:41:10PM -0700, Junio C Hamano wrote:\n\n> > If we have an equivalence-class hashmap and feed it inodes (or again,\n> > some system equivalent) as the keys, we should get buckets of\n> > collisions.\n> \n> I guess one way to get \"some system equivalent\" that can be used as\n> the last resort, when there absolutely is no inum equivalent, is to\n> rehash the working tree file that shouldn't be there when we detect\n> a collision.\n> \n> If we found that there is something when we tried to write out\n> \"Foo.txt\", if we open \"Foo.txt\" on the working tree and hash-object\n> it, we should find the matching blob somewhere in the index _before_\n> \"Foo.txt\". On a case-insensitive filesytem, it may well be\n> \"foo.txt\", but we do not even have to know \"foo.txt\" and \"Foo.txt\"\n> only differ in case.\n\nClever. You might still run into false positives when there is\nduplicated content in the repository (especially, say, zero-length\nfiles). But the fact that you only do the hashing on known duplicates\nhelps with that.\n\nOne of the things I did like about the equivalence-class approach is\nthat it can be done in a single linear pass in the worst case. Whereas\nanything that searches when we see a collision is quite likely to be\nquadratic. But as I said before, it may not be worth worrying too much\nabout that for an error code path where we expect the number of\ncollisions to be small.\n\n-Peff\n"},{"id":"355038","messageId":"34b22185-a0bc-f712-b5e5-fc5e2697dcc2@jeffhostetler.com","threadId":"48963","inReplyTo":"20180809142333.GB1439@sigill.intra.peff.net","subject":"Re: [PATCH v2] clone: report duplicate entries on case-insensitive filesystems","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2018-08-09T21:14:16Z","receivedAt":"2018-08-09T21:14:20Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 8/9/2018 10:23 AM, Jeff King wrote:\n> On Wed, Aug 08, 2018 at 05:41:10PM -0700, Junio C Hamano wrote:\n> \n>>> If we have an equivalence-class hashmap and feed it inodes (or again,\n>>> some system equivalent) as the keys, we should get buckets of\n>>> collisions.\n>>\n>> I guess one way to get \"some system equivalent\" that can be used as\n>> the last resort, when there absolutely is no inum equivalent, is to\n>> rehash the working tree file that shouldn't be there when we detect\n>> a collision.\n>>\n>> If we found that there is something when we tried to write out\n>> \"Foo.txt\", if we open \"Foo.txt\" on the working tree and hash-object\n>> it, we should find the matching blob somewhere in the index _before_\n>> \"Foo.txt\". On a case-insensitive filesytem, it may well be\n>> \"foo.txt\", but we do not even have to know \"foo.txt\" and \"Foo.txt\"\n>> only differ in case.\n> \n> Clever. You might still run into false positives when there is\n> duplicated content in the repository (especially, say, zero-length\n> files). But the fact that you only do the hashing on known duplicates\n> helps with that.\n> \n\nI worry that the false positives make this a non-starter. I mean, if\nclone creates files 'A' and 'B' (both equal) and then tries to create\n'b', would the collision code reports that 'b' collided with 'A' because\nthat was the first OID match? Ideally with this scheme we'd have to\nsearch the entire index prior to 'b' and then report that 'b' collided\nwith either 'A' or 'B'. Neither message instills confidence. And\nthere's no way to prefer answer 'B' over 'A' without using knowledge\nof the FS name mangling/aliasing rules -- unless we want to just assume\nignore-case for this iteration.\n\n> One of the things I did like about the equivalence-class approach is\n> that it can be done in a single linear pass in the worst case. Whereas\n> anything that searches when we see a collision is quite likely to be\n> quadratic. But as I said before, it may not be worth worrying too much\n> about that for an error code path where we expect the number of\n> collisions to be small.\n> \n> -Peff\n> \n\nSorry to be paranoid, but I have an index with 3.5M entries, the word\n\"quadratic\" rings all kinds of alarm bells for me. :-)\n\nGranted, we expect the number of collisions to be small, but searching\nback for each collision over the already-populated portion of the index\ncould be expensive.\n\nJeff\n\n"},{"id":"355044","messageId":"20180809213409.GC11342@sigill.intra.peff.net","threadId":"48963","inReplyTo":"34b22185-a0bc-f712-b5e5-fc5e2697dcc2@jeffhostetler.com","subject":"Re: [PATCH v2] clone: report duplicate entries on case-insensitive filesystems","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-09T21:34:09Z","receivedAt":"2018-08-09T21:34:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 09, 2018 at 05:14:16PM -0400, Jeff Hostetler wrote:\n\n> > Clever. You might still run into false positives when there is\n> > duplicated content in the repository (especially, say, zero-length\n> > files). But the fact that you only do the hashing on known duplicates\n> > helps with that.\n> > \n> \n> I worry that the false positives make this a non-starter. I mean, if\n> clone creates files 'A' and 'B' (both equal) and then tries to create\n> 'b', would the collision code reports that 'b' collided with 'A' because\n> that was the first OID match? Ideally with this scheme we'd have to\n> search the entire index prior to 'b' and then report that 'b' collided\n> with either 'A' or 'B'. Neither message instills confidence. And\n> there's no way to prefer answer 'B' over 'A' without using knowledge\n> of the FS name mangling/aliasing rules -- unless we want to just assume\n> ignore-case for this iteration.\n\nYeah. If we can get usable unique ids (inode or otherwise) of some form\non each system, I think I prefer that. It's much easier to reason about.\n\n> Sorry to be paranoid, but I have an index with 3.5M entries, the word\n> \"quadratic\" rings all kinds of alarm bells for me. :-)\n> \n> Granted, we expect the number of collisions to be small, but searching\n> back for each collision over the already-populated portion of the index\n> could be expensive.\n\nHeh. Yeah, it's really O(n*m), and we expect a small \"m\". But of course\nthe two are equal in the worst case.\n\n-Peff\n"},{"id":"355046","messageId":"CABPp-BHiB_gR-dQbpJtSBYPJ5Om4Mv0ymnZFNocyTfbUotyBgw@mail.gmail.com","threadId":"48963","inReplyTo":"34b22185-a0bc-f712-b5e5-fc5e2697dcc2@jeffhostetler.com","subject":"Re: [PATCH v2] clone: report duplicate entries on case-insensitive filesystems","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-09T21:40:58Z","receivedAt":"2018-08-09T21:41:12Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Aug 9, 2018 at 2:14 PM Jeff Hostetler wrote:\n> On 8/9/2018 10:23 AM, Jeff King wrote:\n> > On Wed, Aug 08, 2018 at 05:41:10PM -0700, Junio C Hamano wrote:\n> >> If we found that there is something when we tried to write out\n> >> \"Foo.txt\", if we open \"Foo.txt\" on the working tree and hash-object\n> >> it, we should find the matching blob somewhere in the index _before_\n> >> \"Foo.txt\". On a case-insensitive filesytem, it may well be\n> >> \"foo.txt\", but we do not even have to know \"foo.txt\" and \"Foo.txt\"\n> >> only differ in case.\n> >\n> > Clever. You might still run into false positives when there is\n> > duplicated content in the repository (especially, say, zero-length\n> > files). But the fact that you only do the hashing on known duplicates\n> > helps with that.\n>\n> I worry that the false positives make this a non-starter. I mean, if\n> clone creates files 'A' and 'B' (both equal) and then tries to create\n> 'b', would the collision code reports that 'b' collided with 'A' because\n> that was the first OID match? Ideally with this scheme we'd have to\n> search the entire index prior to 'b' and then report that 'b' collided\n> with either 'A' or 'B'. Neither message instills confidence. And\n> there's no way to prefer answer 'B' over 'A' without using knowledge\n> of the FS name mangling/aliasing rules -- unless we want to just assume\n> ignore-case for this iteration.\n\nA possibly crazy idea: Don't bother reporting the other filename; just\nreport the OID instead.\n\n\"Error: Foo.txt cannot be checked out because another file with hash\n is in the way.\" Maybe even add a hint for the user: \"Run\n`git ls-files -s` to see see all files and their hash\".\n\nWhatever the exact wording for the error message, just create a nice\npost on stackoverflow.com explaining the various weird filesystems out\nthere (VFAT, NTFS, HFS, APFS, etc) and how they cause differing\nfilenames to be written to the same location. Have a bunch of folks\nvote it up so it has some nice search-engine juice.\n\n\nThe error message isn't quite as good, but does the user really need\nall the names of the file? If so, we gave them enough information to\nfigure it out, and this is a really unusual case anyway, right?\nBesides, now we're back to linear performance....\n"},{"id":"355049","messageId":"20180809214430.GE11342@sigill.intra.peff.net","threadId":"48963","inReplyTo":"CABPp-BHiB_gR-dQbpJtSBYPJ5Om4Mv0ymnZFNocyTfbUotyBgw@mail.gmail.com","subject":"Re: [PATCH v2] clone: report duplicate entries on case-insensitive filesystems","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-09T21:44:30Z","receivedAt":"2018-08-09T21:44:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 09, 2018 at 02:40:58PM -0700, Elijah Newren wrote:\n\n> > I worry that the false positives make this a non-starter. I mean, if\n> > clone creates files 'A' and 'B' (both equal) and then tries to create\n> > 'b', would the collision code reports that 'b' collided with 'A' because\n> > that was the first OID match? Ideally with this scheme we'd have to\n> > search the entire index prior to 'b' and then report that 'b' collided\n> > with either 'A' or 'B'. Neither message instills confidence. And\n> > there's no way to prefer answer 'B' over 'A' without using knowledge\n> > of the FS name mangling/aliasing rules -- unless we want to just assume\n> > ignore-case for this iteration.\n> \n> A possibly crazy idea: Don't bother reporting the other filename; just\n> report the OID instead.\n> \n> \"Error: Foo.txt cannot be checked out because another file with hash\n> is in the way.\" Maybe even add a hint for the user: \"Run\n> `git ls-files -s` to see see all files and their hash\".\n> \n> Whatever the exact wording for the error message, just create a nice\n> post on stackoverflow.com explaining the various weird filesystems out\n> there (VFAT, NTFS, HFS, APFS, etc) and how they cause differing\n> filenames to be written to the same location. Have a bunch of folks\n> vote it up so it has some nice search-engine juice.\n\nActually, I kind of like the simplicity of that. It puts the human brain\nin the loop.\n\n> The error message isn't quite as good, but does the user really need\n> all the names of the file? If so, we gave them enough information to\n> figure it out, and this is a really unusual case anyway, right?\n> Besides, now we're back to linear performance....\n\nWell, it's still quadratic when they run O(n) iterations of \"git\nls-files -s | grep $colliding_oid\". You've just pushed the second linear\nsearch onto the user. ;)\n\n-Peff\n"},{"id":"355052","messageId":"CABPp-BEAybfJ8sojRwDbDjhcwk4VyQ26F1LnKyNLsg1fYS1fNA@mail.gmail.com","threadId":"48963","inReplyTo":"20180809214430.GE11342@sigill.intra.peff.net","subject":"Re: [PATCH v2] clone: report duplicate entries on case-insensitive filesystems","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-09T21:53:42Z","receivedAt":"2018-08-09T21:53:56Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Aug 9, 2018 at 2:44 PM Jeff King wrote:\n> > The error message isn't quite as good, but does the user really need\n> > all the names of the file? If so, we gave them enough information to\n> > figure it out, and this is a really unusual case anyway, right?\n> > Besides, now we're back to linear performance....\n>\n> Well, it's still quadratic when they run O(n) iterations of \"git\n> ls-files -s | grep $colliding_oid\". You've just pushed the second linear\n> search onto the user. ;)\n\nWouldn't that be their own fault for not running\n git ls-files -s | grep -e $colliding_oid_1 ... -e $colliding_oid_n | sort -k 2\n? ;-)\n"},{"id":"355054","messageId":"20180809215913.GB12441@sigill.intra.peff.net","threadId":"48963","inReplyTo":"CABPp-BEAybfJ8sojRwDbDjhcwk4VyQ26F1LnKyNLsg1fYS1fNA@mail.gmail.com","subject":"Re: [PATCH v2] clone: report duplicate entries on case-insensitive filesystems","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-09T21:59:13Z","receivedAt":"2018-08-09T21:59:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 09, 2018 at 02:53:42PM -0700, Elijah Newren wrote:\n\n> On Thu, Aug 9, 2018 at 2:44 PM Jeff King wrote:\n> > > The error message isn't quite as good, but does the user really need\n> > > all the names of the file? If so, we gave them enough information to\n> > > figure it out, and this is a really unusual case anyway, right?\n> > > Besides, now we're back to linear performance....\n> >\n> > Well, it's still quadratic when they run O(n) iterations of \"git\n> > ls-files -s | grep $colliding_oid\". You've just pushed the second linear\n> > search onto the user. ;)\n> \n> Wouldn't that be their own fault for not running\n> git ls-files -s | grep -e $colliding_oid_1 ... -e $colliding_oid_n | sort -k 2\n> ? ;-)\n\nMan, this thread is the gift that keeps on giving. :)\n\nThat's still quadratic, isn't it? You've just hidden the second\ndimension in the single grep call.\n\nNow since these are all going to be constant strings, in theory an\nintelligent grep could stick them all in a search trie, and match each\nline with complexity k, the length of the matched strings. And since\nk=40, that's technically still linear overall.\n\n-Peff\n"},{"id":"355057","messageId":"xmqqftzn9ot4.fsf@gitster-ct.c.googlers.com","threadId":"48963","inReplyTo":"CABPp-BHiB_gR-dQbpJtSBYPJ5Om4Mv0ymnZFNocyTfbUotyBgw@mail.gmail.com","subject":"Re: [PATCH v2] clone: report duplicate entries on case-insensitive filesystems","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-09T22:07:35Z","receivedAt":"2018-08-09T22:07:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> A possibly crazy idea: Don't bother reporting the other filename; just\n> report the OID instead.\n>\n> \"Error: Foo.txt cannot be checked out because another file with hash\n> is in the way.\" Maybe even add a hint for the user: \"Run\n> `git ls-files -s` to see see all files and their hash\".\n\nOnce we start using OID to talk to humans, we are already lost. At\nthat point, it would be 1000% better to\n\n - not check out Foo.txt to unlink and overwrite; instead leave the\n content that is already in the working tree as-is; and\n\n - report that Foo.txt was not checked out as something else that\n also claims to deserve that path was checked out already.\n\nThen the user can inspect Foo.txt and realize it is actually the\ncontents that should be in foo.txt or whatever.\n\n\n"},{"id":"355066","messageId":"CABPp-BE+QRpxq1-r_ORhLi3KqaX3EjfpzswzmyP2BUV7uYPiuQ@mail.gmail.com","threadId":"48963","inReplyTo":"20180809215913.GB12441@sigill.intra.peff.net","subject":"Re: [PATCH v2] clone: report duplicate entries on case-insensitive filesystems","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-09T23:05:49Z","receivedAt":"2018-08-09T23:06:04Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Aug 9, 2018 at 2:59 PM Jeff King wrote:\n> On Thu, Aug 09, 2018 at 02:53:42PM -0700, Elijah Newren wrote:\n>\n> > On Thu, Aug 9, 2018 at 2:44 PM Jeff King wrote:\n> > > > The error message isn't quite as good, but does the user really need\n> > > > all the names of the file? If so, we gave them enough information to\n> > > > figure it out, and this is a really unusual case anyway, right?\n> > > > Besides, now we're back to linear performance....\n> > >\n> > > Well, it's still quadratic when they run O(n) iterations of \"git\n> > > ls-files -s | grep $colliding_oid\". You've just pushed the second linear\n> > > search onto the user. ;)\n> >\n> > Wouldn't that be their own fault for not running\n> > git ls-files -s | grep -e $colliding_oid_1 ... -e $colliding_oid_n | sort -k 2\n> > ? ;-)\n>\n> Man, this thread is the gift that keeps on giving. :)\n>\n> That's still quadratic, isn't it? You've just hidden the second\n> dimension in the single grep call.\n\nIt may depend on the implementation within grep. If I had remembered\nto pass -F, and if wikipedia's claims[1] about the Aho-Corasick\nalgotihrm being the basis of the original Unix command fgrep, and it's\nclaims that this algorithm is \"linear in the length of the strings\nplus the length of the searched text plus the number of output\nmatches\", and I didn't just misunderstand something there, then it\nlooks like it could be linear.\n\n[1] https://en.wikipedia.org/wiki/Aho%E2%80%93Corasick_algorithm\n\nOf course, the grep implementation could also do something stupid and\nprovide dramatically worse behavior than running the command N times\nspecifying one pattern each time.[2]\n\n[2] http://savannah.gnu.org/bugs/?16305\n\n> Now since these are all going to be constant strings, in theory an\n> intelligent grep could stick them all in a search trie, and match each\n> line with complexity k, the length of the matched strings. And since\n> k=40, that's technically still linear overall.\n\nLooks like Aho-Corasick uses \"a finite-state machine that resembles a\ntrie\", so definitely along those lines.\n"},{"id":"355081","messageId":"20180810153608.30051-1-pclouds@gmail.com","threadId":"48963","inReplyTo":"20180807190110.16216-1-pclouds@gmail.com","subject":"[PATCH v3 0/1] clone: warn on colidding entries on checkout","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-08-10T15:36:07Z","receivedAt":"2018-08-10T15:37:21Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"There are lots of suggestions on optimizing this stuff, but since this\nproblem does not affect me to begin with, I'm reluctant to make more\nchanges and going to stay simple, stupid and slow. I could continue to\ndo small updates if needed. But for bigger changes, consider this\npatch dropped by me.\n\nv3 now uses inode on UNIXy platforms for checking colliding items. I\nstill don't try to separate colliding groups because it should be\nquite obvious once you look at the colliding list (and most of the\ntime I suspect we only have one or two groups).\n\nSince on Windows we can't really have colliding groups (no inode to\ncheck) so the warning message is to me a bit misleading. But I frankly\ndon't want to put more effort in this.\n\nNguyễn Thái Ngọc Duy (1):\n clone: report duplicate entries on case-insensitive filesystems\n\n builtin/clone.c | 1 +\n cache.h | 2 ++\n entry.c | 32 ++++++++++++++++++++++++++++++++\n unpack-trees.c | 22 ++++++++++++++++++++++\n unpack-trees.h | 1 +\n 5 files changed, 58 insertions(+)\n\n-- \n2.18.0.915.gd571298aae\n\n"},{"id":"355082","messageId":"20180810153608.30051-2-pclouds@gmail.com","threadId":"48963","inReplyTo":"20180810153608.30051-1-pclouds@gmail.com","subject":"[PATCH v3 1/1] clone: report duplicate entries on case-insensitive filesystems","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-08-10T15:36:08Z","receivedAt":"2018-08-10T15:37:23Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Paths that only differ in case work fine in a case-sensitive\nfilesystems, but if those repos are cloned in a case-insensitive one,\nyou'll get problems. The first thing to notice is \"git status\" will\nnever be clean with no indication what exactly is \"dirty\".\n\nThis patch helps the situation a bit by pointing out the problem at\nclone time. Even though this patch talks about case sensitivity, the\npatch makes no assumption about folding rules by the filesystem. It\nsimply observes that if an entry has been already checked out at clone\ntime when we're about to write a new path, some folding rules are\nbehind this.\n\nThis patch is tested with vim-colorschemes repository on a JFS partition\nwith case insensitive support on Linux. This repository has two files\ndarkBlue.vim and darkblue.vim.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/clone.c | 1 +\n cache.h | 2 ++\n entry.c | 32 ++++++++++++++++++++++++++++++++\n unpack-trees.c | 22 ++++++++++++++++++++++\n unpack-trees.h | 1 +\n 5 files changed, 58 insertions(+)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 9ebb5acf56..38d5609282 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -748,6 +748,7 @@ static int checkout(int submodule_progress)\n \tmemset(&opts, 0, sizeof opts);\n \topts.update = 1;\n \topts.merge = 1;\n+\topts.clone = 1;\n \topts.fn = oneway_merge;\n \topts.verbose_update = (option_verbosity >= 0);\n \topts.src_index = &the_index;\ndiff --git a/cache.h b/cache.h\nindex 8dc7134f00..cdf0984707 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1515,9 +1515,11 @@ struct checkout {\n \tconst char *base_dir;\n \tint base_dir_len;\n \tstruct delayed_checkout *delayed_checkout;\n+\tint *nr_duplicates;\n \tunsigned force:1,\n \t\t quiet:1,\n \t\t not_new:1,\n+\t\t clone:1,\n \t\t refresh_cache:1;\n };\n #define CHECKOUT_INIT { NULL, \"\" }\ndiff --git a/entry.c b/entry.c\nindex b5d1d3cf23..f2d73e6255 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -399,6 +399,35 @@ static int check_path(const char *path, int len, struct stat *st, int skiplen)\n \treturn lstat(path, st);\n }\n \n+static void mark_duplicate_entries(const struct checkout *state,\n+\t\t\t\t struct cache_entry *ce, struct stat *st)\n+{\n+\tint i;\n+\tint *count = state->nr_duplicates;\n+\n+\tif (!count)\n+\t\tBUG(\"state->nr_duplicates must not be NULL\");\n+\n+\tce->ce_flags |= CE_MATCHED;\n+\t(*count)++;\n+\n+#if !defined(GIT_WINDOWS_NATIVE) /* inode is always zero on Windows */\n+\tfor (i = 0; i < state->istate->cache_nr; i++) {\n+\t\tstruct cache_entry *dup = state->istate->cache[i];\n+\n+\t\tif (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE))\n+\t\t\tcontinue;\n+\n+\t\tif (ce_uptodate(dup) &&\n+\t\t dup->ce_stat_data.sd_ino == st->st_ino) {\n+\t\t\tdup->ce_flags |= CE_MATCHED;\n+\t\t\t(*count)++;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+#endif\n+}\n+\n /*\n * Write the contents from ce out to the working tree.\n *\n@@ -455,6 +484,9 @@ int checkout_entry(struct cache_entry *ce,\n \t\t\treturn -1;\n \t\t}\n \n+\t\tif (state->clone)\n+\t\t\tmark_duplicate_entries(state, ce, &st);\n+\n \t\t/*\n \t\t * We unlink the old file, to get the new one with the\n \t\t * right permissions (including umask, which is nasty\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex f9efee0836..d4fece913c 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -344,12 +344,20 @@ static int check_updates(struct unpack_trees_options *o)\n \tstruct index_state *index = &o->result;\n \tstruct checkout state = CHECKOUT_INIT;\n \tint i;\n+\tint nr_duplicates = 0;\n \n \tstate.force = 1;\n \tstate.quiet = 1;\n \tstate.refresh_cache = 1;\n \tstate.istate = index;\n \n+\tif (o->clone) {\n+\t\tstate.clone = 1;\n+\t\tstate.nr_duplicates = &nr_duplicates;\n+\t\tfor (i = 0; i < index->cache_nr; i++)\n+\t\t\tindex->cache[i]->ce_flags &= ~CE_MATCHED;\n+\t}\n+\n \tprogress = get_progress(o);\n \n \tif (o->update)\n@@ -414,6 +422,20 @@ static int check_updates(struct unpack_trees_options *o)\n \terrs |= finish_delayed_checkout(&state);\n \tif (o->update)\n \t\tgit_attr_set_direction(GIT_ATTR_CHECKIN, NULL);\n+\n+\tif (o->clone && state.nr_duplicates) {\n+\t\twarning(_(\"the following paths have collided and only one from the same\\n\"\n+\t\t\t \"colliding group is in the working tree:\\n\"));\n+\t\tfor (i = 0; i < index->cache_nr; i++) {\n+\t\t\tstruct cache_entry *ce = index->cache[i];\n+\n+\t\t\tif (!(ce->ce_flags & CE_MATCHED))\n+\t\t\t\tcontinue;\n+\t\t\tfprintf(stderr, \" '%s'\\n\", ce->name);\n+\t\t\tce->ce_flags &= ~CE_MATCHED;\n+\t\t}\n+\t}\n+\n \treturn errs != 0;\n }\n \ndiff --git a/unpack-trees.h b/unpack-trees.h\nindex c2b434c606..d940f1c5c2 100644\n--- a/unpack-trees.h\n+++ b/unpack-trees.h\n@@ -42,6 +42,7 @@ struct unpack_trees_options {\n \tunsigned int reset,\n \t\t merge,\n \t\t update,\n+\t\t clone,\n \t\t index_only,\n \t\t nontrivial_merge,\n \t\t trivial_merges_only,\n-- \n2.18.0.915.gd571298aae\n\n"},{"id":"355087","messageId":"xmqqr2j68ake.fsf@gitster-ct.c.googlers.com","threadId":"48963","inReplyTo":"20180810153608.30051-1-pclouds@gmail.com","subject":"Re: [PATCH v3 0/1] clone: warn on colidding entries on checkout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-10T16:12:49Z","receivedAt":"2018-08-10T16:12:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> There are lots of suggestions on optimizing this stuff, but since this\n> problem does not affect me to begin with, I'm reluctant to make more\n> changes and going to stay simple, stupid and slow. I could continue to\n> do small updates if needed. But for bigger changes, consider this\n> patch dropped by me.\n>\n> v3 now uses inode on UNIXy platforms for checking colliding items. I\n> still don't try to separate colliding groups because it should be\n> quite obvious once you look at the colliding list (and most of the\n> time I suspect we only have one or two groups).\n\nI think that design decision is fine. We can extend it later if\nneeded, but I would not be surprised if what you have here is\nsufficient.\n\nAnother possible follow-up in the future may be to encapsulate the\n\"I have a cache-entry 'dup', and stat data 'st' taken for a path\nin the working tree. Does it look likely that the latter is the\nresult of checking out the former?\" logic, which you currently has a\nhard-coded if() statement condition, into a helper function and\nmake its implementation platform dependent.\n\n"},{"id":"355091","messageId":"xmqqmutu896n.fsf@gitster-ct.c.googlers.com","threadId":"48963","inReplyTo":"20180810153608.30051-2-pclouds@gmail.com","subject":"Re: [PATCH v3 1/1] clone: report duplicate entries on case-insensitive filesystems","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-10T16:42:40Z","receivedAt":"2018-08-10T16:42:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> +static void mark_duplicate_entries(const struct checkout *state,\n> +\t\t\t\t struct cache_entry *ce, struct stat *st)\n> +{\n> +\tint i;\n> +\tint *count = state->nr_duplicates;\n> +\n> +\tif (!count)\n> +\t\tBUG(\"state->nr_duplicates must not be NULL\");\n> +\n> +\tce->ce_flags |= CE_MATCHED;\n> +\t(*count)++;\n\nWe tried to check out ce and found out that the path is already\noccupied, whose stat data is in st. We increment (*count)++ to\nsignal the fact that ce itself had trouble.\n\n> +#if !defined(GIT_WINDOWS_NATIVE) /* inode is always zero on Windows */\n> +\tfor (i = 0; i < state->istate->cache_nr; i++) {\n\nDo we want to count up all the way? IOW, don't we want to stop when\nwe see the ce itself?\n\n> +\t\tstruct cache_entry *dup = state->istate->cache[i];\n> +\n> +\t\tif (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE))\n> +\t\t\tcontinue;\n> +\n> +\t\tif (ce_uptodate(dup) &&\n> +\t\t dup->ce_stat_data.sd_ino == st->st_ino) {\n> +\t\t\tdup->ce_flags |= CE_MATCHED;\n\nIs checking ce_uptodate(dup) done to check that the 'dup' is a\nfreshly checked out entry? I'd like to understand why it is\nnecessary, especially beause you know you only call this function in\nthe o->clone codepath, so everything ought to be fresh.\n\nI agree that the check you have above is sufficient on inum capable\nsystems. I also suspect that Windows folks, if they really wish to\nreport which other path(s) are colliding with ce, would want to\nreplace this whole loop with a platform specific helper function,\nnot just the condition in the above \"if\" statement [*1*], so what we\nsee here should be sufficient for now.\n\n\tSide note #1. It is sufficient for inum capable systems to\n\tlook at 'dup' and 'st' to cheaply identify collision, but it\n\tmay be more efficient for other systems to look at ce and\n\tthe previous entries of the index, which allows them to use\n\tplatform specific API that knows case-folding and other\n\tpathname munging specifics. Going that way lets them\n\tminimally emulate 'struct stat', which may be expensive to\n\tfill.\n\n> +\t\t\t(*count)++;\n\nAnd then we increment (*count)++ for this _other_ one that\ncollided with ce.\n\n> +\t\t\tbreak;\n\nAnd we leave the loop. There may be somebody else that collided\nwhen 'dup' was checked out, making this triple collision, but in\nsuch a case, we won't be coming this far in this loop (i.e. we\nwould have continued on this 'dup' as it is marked as CE_MATCHED),\nso there is no reason to continue looking for further collision with\n'ce' here. So nr_duplicates kept track by these (*count)++ will\nindicate number of paths involved in any collision correctly, I\nthink.\n\nMakes sense.\n\n> +\t\t}\n> +\t}\n> +#endif\n> +}\n> +\n> /*\n> * Write the contents from ce out to the working tree.\n> *\n> @@ -455,6 +484,9 @@ int checkout_entry(struct cache_entry *ce,\n> \t\t\treturn -1;\n> \t\t}\n> \n> +\t\tif (state->clone)\n> +\t\t\tmark_duplicate_entries(state, ce, &st);\n> +\n> \t\t/*\n> \t\t * We unlink the old file, to get the new one with the\n> \t\t * right permissions (including umask, which is nasty\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index f9efee0836..d4fece913c 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -344,12 +344,20 @@ static int check_updates(struct unpack_trees_options *o)\n> \tstruct index_state *index = &o->result;\n> \tstruct checkout state = CHECKOUT_INIT;\n> \tint i;\n> +\tint nr_duplicates = 0;\n> \n> \tstate.force = 1;\n> \tstate.quiet = 1;\n> \tstate.refresh_cache = 1;\n> \tstate.istate = index;\n> \n> +\tif (o->clone) {\n> +\t\tstate.clone = 1;\n> +\t\tstate.nr_duplicates = &nr_duplicates;\n> +\t\tfor (i = 0; i < index->cache_nr; i++)\n> +\t\t\tindex->cache[i]->ce_flags &= ~CE_MATCHED;\n> +\t}\n> +\n> \tprogress = get_progress(o);\n> \n> \tif (o->update)\n> @@ -414,6 +422,20 @@ static int check_updates(struct unpack_trees_options *o)\n> \terrs |= finish_delayed_checkout(&state);\n> \tif (o->update)\n> \t\tgit_attr_set_direction(GIT_ATTR_CHECKIN, NULL);\n> +\n> +\tif (o->clone && state.nr_duplicates) {\n\nDoes state.nr_duplicates have to be a pointer to int? I am\nwondering if state.clone bit is sufficient to protect codepaths that\nneeds to spend cycles to keep track of that number, in which case we\ncan do without local variable nr_duplicates here, and the set-up\ncode in the previous hunk not to store the pointer to it in\nstate.nr_duplicates (instead, that field itself can become an int).\n\n> +\t\twarning(_(\"the following paths have collided and only one from the same\\n\"\n> +\t\t\t \"colliding group is in the working tree:\\n\"));\n\nAs we are not grouping (and do not particularly see the need to\ngroup), perhaps we can do without mentioning group in the warning?\ne.g.\n\n\tsome of the following may have been checked out incorrectly\n\tdue to limitation of the filesystem like case insensitivity.\n\n\n> +\t\tfor (i = 0; i < index->cache_nr; i++) {\n> +\t\t\tstruct cache_entry *ce = index->cache[i];\n> +\n> +\t\t\tif (!(ce->ce_flags & CE_MATCHED))\n> +\t\t\t\tcontinue;\n> +\t\t\tfprintf(stderr, \" '%s'\\n\", ce->name);\n> +\t\t\tce->ce_flags &= ~CE_MATCHED;\n> +\t\t}\n> +\t}\n"},{"id":"355243","messageId":"20180811100905.1511-1-szeder.dev@gmail.com","threadId":"48963","inReplyTo":"20180810153608.30051-2-pclouds@gmail.com","subject":"Re: [PATCH v3 1/1] clone: report duplicate entries on case-insensitive filesystems","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-08-11T10:09:05Z","receivedAt":"2018-08-11T10:09:16Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\n> Paths that only differ in case work fine in a case-sensitive\n> filesystems, but if those repos are cloned in a case-insensitive one,\n> you'll get problems. The first thing to notice is \"git status\" will\n> never be clean with no indication what exactly is \"dirty\".\n> \n> This patch helps the situation a bit by pointing out the problem at\n> clone time. Even though this patch talks about case sensitivity, the\n> patch makes no assumption about folding rules by the filesystem. It\n> simply observes that if an entry has been already checked out at clone\n> time when we're about to write a new path, some folding rules are\n> behind this.\n> \n> This patch is tested with vim-colorschemes repository on a JFS partition\n> with case insensitive support on Linux. This repository has two files\n> darkBlue.vim and darkblue.vim.\n> \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n\nThis patch makes 'clone http repository' in\n't5551-http-fetch-smart.sh' fail with:\n\n --- exp 2018-08-11 02:29:45.216641851 +0000\n +++ actual.smudged 2018-08-11 02:29:45.264642318 +0000\n @@ -15,3 +15,5 @@\n < Pragma: no-cache\n < Cache-Control: no-cache, max-age=0, must-revalidate\n < Content-Type: application/x-git-upload-pack-result\n +> warning: the following paths have collided and only one from the same\n +> colliding group is in the working tree:\n\n\nThis highlights a few issues:\n\n - This test runs\n\n GIT_TRACE_CURL=true git clone --quiet 2>err\n\n i.e. the curl trace and any errors or warnings from 'git clone'\n end up in the same file. This test then removes a lot of\n uninteresting headers before the comparison with 'test_cmp', but\n the warning remains and then triggers the test failure.\n\n Several other tests run a command like this, but those don't use\n 'test_cmp', but only grep the output to verify the presence or\n absence of certain headers.\n\n I'm inclined to think that it would be prudent to change all these\n tests to send the curl trace to a dedicated file (and then\n '--quiet' can be removed as well). Though, arguably, had that\n been already the case, this test wouldn't have failed, and we\n probably wouldn't have noticed that something is wrong.\n\n - But what triggered this warning in the first place? 'git clone'\n didn't print anything after the that warning, even when I re-run\n the test with that '--quiet' option removed. Furthermore, the\n cloned repository contains a single file, so there could be no\n case/folding collision among multiple files.\n\n I also notice that this patch doesn't add any tests... :)\n\n - I didn't understand this warning, I had to read the corresponding\n commit message to figure out what it's all about.\n\n\n"},{"id":"355246","messageId":"CACsJy8BeRYVvWvTQU+bj+hSQ3DFw0mHtSjtOg9zVSsXznpU=Xw@mail.gmail.com","threadId":"48963","inReplyTo":"20180811100905.1511-1-szeder.dev@gmail.com","subject":"Re: [PATCH v3 1/1] clone: report duplicate entries on case-insensitive filesystems","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-08-11T13:16:12Z","receivedAt":"2018-08-11T13:16:41Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Aug 11, 2018 at 12:09 PM SZEDER Gábor wrote:\n>\n>\n> > Paths that only differ in case work fine in a case-sensitive\n> > filesystems, but if those repos are cloned in a case-insensitive one,\n> > you'll get problems. The first thing to notice is \"git status\" will\n> > never be clean with no indication what exactly is \"dirty\".\n> >\n> > This patch helps the situation a bit by pointing out the problem at\n> > clone time. Even though this patch talks about case sensitivity, the\n> > patch makes no assumption about folding rules by the filesystem. It\n> > simply observes that if an entry has been already checked out at clone\n> > time when we're about to write a new path, some folding rules are\n> > behind this.\n> >\n> > This patch is tested with vim-colorschemes repository on a JFS partition\n> > with case insensitive support on Linux. This repository has two files\n> > darkBlue.vim and darkblue.vim.\n> >\n> > Signed-off-by: Nguyễn Thái Ngọc Duy \n>\n> This patch makes 'clone http repository' in\n> 't5551-http-fetch-smart.sh' fail with:\n>\n> --- exp 2018-08-11 02:29:45.216641851 +0000\n> +++ actual.smudged 2018-08-11 02:29:45.264642318 +0000\n> @@ -15,3 +15,5 @@\n> < Pragma: no-cache\n> < Cache-Control: no-cache, max-age=0, must-revalidate\n> < Content-Type: application/x-git-upload-pack-result\n> +> warning: the following paths have collided and only one from the same\n> +> colliding group is in the working tree:\n\nI was careless and checked the wrong variable (should have checked\nnr_duplicates not state.nr_duplicates; the second is a pointer). So we\nalways get this warning (and with no following list of files)\n\n> I also notice that this patch doesn't add any tests... :)\n\nThis is platform specific and I was to be frank a bit lazy. Will\nconsider adding a test with CASE_INSENSITIVE_FS after this.\n-- \nDuy\n"},{"id":"355297","messageId":"20180812090714.19060-1-pclouds@gmail.com","threadId":"48963","inReplyTo":"20180810153608.30051-1-pclouds@gmail.com","subject":"[PATCH v4] clone: report duplicate entries on case-insensitive filesystems","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-08-12T09:07:14Z","receivedAt":"2018-08-12T09:07:35Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Paths that only differ in case work fine in a case-sensitive\nfilesystems, but if those repos are cloned in a case-insensitive one,\nyou'll get problems. The first thing to notice is \"git status\" will\nnever be clean with no indication what exactly is \"dirty\".\n\nThis patch helps the situation a bit by pointing out the problem at\nclone time. Even though this patch talks about case sensitivity, the\npatch makes no assumption about folding rules by the filesystem. It\nsimply observes that if an entry has been already checked out at clone\ntime when we're about to write a new path, some folding rules are\nbehind this.\n\nThis patch is tested with vim-colorschemes repository on a JFS partition\nwith case insensitive support on Linux. This repository has two files\ndarkBlue.vim and darkblue.vim.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n v4 removes nr_duplicates (and fixes that false warning Szeder\n reported). It also hints about case insensitivity as a cause of\n problem because it's most likely the case when this warning shows up.\n\n builtin/clone.c | 1 +\n cache.h | 1 +\n entry.c | 28 ++++++++++++++++++++++++++++\n t/t5601-clone.sh | 8 +++++++-\n unpack-trees.c | 28 ++++++++++++++++++++++++++++\n unpack-trees.h | 1 +\n 6 files changed, 66 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 5c439f1394..0702b0e9d0 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -747,6 +747,7 @@ static int checkout(int submodule_progress)\n \tmemset(&opts, 0, sizeof opts);\n \topts.update = 1;\n \topts.merge = 1;\n+\topts.clone = 1;\n \topts.fn = oneway_merge;\n \topts.verbose_update = (option_verbosity >= 0);\n \topts.src_index = &the_index;\ndiff --git a/cache.h b/cache.h\nindex 8b447652a7..6d6138f4f1 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1455,6 +1455,7 @@ struct checkout {\n \tunsigned force:1,\n \t\t quiet:1,\n \t\t not_new:1,\n+\t\t clone:1,\n \t\t refresh_cache:1;\n };\n #define CHECKOUT_INIT { NULL, \"\" }\ndiff --git a/entry.c b/entry.c\nindex b5d1d3cf23..c70340df8e 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -399,6 +399,31 @@ static int check_path(const char *path, int len, struct stat *st, int skiplen)\n \treturn lstat(path, st);\n }\n \n+static void mark_colliding_entries(const struct checkout *state,\n+\t\t\t\t struct cache_entry *ce, struct stat *st)\n+{\n+\tint i;\n+\n+\tce->ce_flags |= CE_MATCHED;\n+\n+#if !defined(GIT_WINDOWS_NATIVE) /* inode is always zero on Windows */\n+\tfor (i = 0; i < state->istate->cache_nr; i++) {\n+\t\tstruct cache_entry *dup = state->istate->cache[i];\n+\n+\t\tif (dup == ce)\n+\t\t\tbreak;\n+\n+\t\tif (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE))\n+\t\t\tcontinue;\n+\n+\t\tif (dup->ce_stat_data.sd_ino == st->st_ino) {\n+\t\t\tdup->ce_flags |= CE_MATCHED;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+#endif\n+}\n+\n /*\n * Write the contents from ce out to the working tree.\n *\n@@ -455,6 +480,9 @@ int checkout_entry(struct cache_entry *ce,\n \t\t\treturn -1;\n \t\t}\n \n+\t\tif (state->clone)\n+\t\t\tmark_colliding_entries(state, ce, &st);\n+\n \t\t/*\n \t\t * We unlink the old file, to get the new one with the\n \t\t * right permissions (including umask, which is nasty\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 0b62037744..f2eb73bc74 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -624,10 +624,16 @@ test_expect_success 'clone on case-insensitive fs' '\n \t\t\tgit hash-object -w -t tree --stdin) &&\n \t\tc=$(git commit-tree -m bogus $t) &&\n \t\tgit update-ref refs/heads/bogus $c &&\n-\t\tgit clone -b bogus . bogus\n+\t\tgit clone -b bogus . bogus 2>warning\n \t)\n '\n \n+test_expect_success !MINGW,!CYGWIN,CASE_INSENSITIVE_FS 'colliding file detection' '\n+\tgrep X icasefs/warning &&\n+\tgrep x icasefs/warning &&\n+\ttest_i18ngrep \"the following paths have collided\" icasefs/warning\n+'\n+\n partial_clone () {\n \t SERVER=\"$1\" &&\n \t URL=\"$2\" &&\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex cd0680f11e..443df048ef 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -359,6 +359,12 @@ static int check_updates(struct unpack_trees_options *o)\n \tstate.refresh_cache = 1;\n \tstate.istate = index;\n \n+\tif (o->clone) {\n+\t\tstate.clone = 1;\n+\t\tfor (i = 0; i < index->cache_nr; i++)\n+\t\t\tindex->cache[i]->ce_flags &= ~CE_MATCHED;\n+\t}\n+\n \tprogress = get_progress(o);\n \n \tif (o->update)\n@@ -423,6 +429,28 @@ static int check_updates(struct unpack_trees_options *o)\n \terrs |= finish_delayed_checkout(&state);\n \tif (o->update)\n \t\tgit_attr_set_direction(GIT_ATTR_CHECKIN, NULL);\n+\n+\tif (o->clone) {\n+\t\tint printed_warning = 0;\n+\n+\t\tfor (i = 0; i < index->cache_nr; i++) {\n+\t\t\tstruct cache_entry *ce = index->cache[i];\n+\n+\t\t\tif (!(ce->ce_flags & CE_MATCHED))\n+\t\t\t\tcontinue;\n+\n+\t\t\tif (!printed_warning) {\n+\t\t\t\twarning(_(\"the following paths have collided (e.g. case-sensitive paths\\n\"\n+\t\t\t\t\t \"on a case-insensitive filesystem) and only one from the same\\n\"\n+\t\t\t\t\t \"colliding group is in the working tree:\\n\"));\n+\t\t\t\tprinted_warning = 1;\n+\t\t\t}\n+\n+\t\t\tfprintf(stderr, \" '%s'\\n\", ce->name);\n+\t\t\tce->ce_flags &= ~CE_MATCHED;\n+\t\t}\n+\t}\n+\n \treturn errs != 0;\n }\n \ndiff --git a/unpack-trees.h b/unpack-trees.h\nindex c2b434c606..d940f1c5c2 100644\n--- a/unpack-trees.h\n+++ b/unpack-trees.h\n@@ -42,6 +42,7 @@ struct unpack_trees_options {\n \tunsigned int reset,\n \t\t merge,\n \t\t update,\n+\t\t clone,\n \t\t index_only,\n \t\t nontrivial_merge,\n \t\t trivial_merges_only,\n-- \n2.18.0.1004.g6639190530\n\n"},{"id":"355364","messageId":"1c0c0ff0-0005-a5a8-5aed-d39ce94373ba@jeffhostetler.com","threadId":"48963","inReplyTo":"20180812090714.19060-1-pclouds@gmail.com","subject":"Re: [PATCH v4] clone: report duplicate entries on case-insensitive filesystems","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2018-08-13T15:32:51Z","receivedAt":"2018-08-13T15:32:56Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 8/12/2018 5:07 AM, Nguyễn Thái Ngọc Duy wrote:\n> Paths that only differ in case work fine in a case-sensitive\n> filesystems, but if those repos are cloned in a case-insensitive one,\n> you'll get problems. The first thing to notice is \"git status\" will\n> never be clean with no indication what exactly is \"dirty\".\n> \n> This patch helps the situation a bit by pointing out the problem at\n> clone time. Even though this patch talks about case sensitivity, the\n> patch makes no assumption about folding rules by the filesystem. It\n> simply observes that if an entry has been already checked out at clone\n> time when we're about to write a new path, some folding rules are\n> behind this.\n> \n> This patch is tested with vim-colorschemes repository on a JFS partition\n> with case insensitive support on Linux. This repository has two files\n> darkBlue.vim and darkblue.vim.\n> \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> v4 removes nr_duplicates (and fixes that false warning Szeder\n> reported). It also hints about case insensitivity as a cause of\n> problem because it's most likely the case when this warning shows up.\n> \n> builtin/clone.c | 1 +\n> cache.h | 1 +\n> entry.c | 28 ++++++++++++++++++++++++++++\n> t/t5601-clone.sh | 8 +++++++-\n> unpack-trees.c | 28 ++++++++++++++++++++++++++++\n> unpack-trees.h | 1 +\n> 6 files changed, 66 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 5c439f1394..0702b0e9d0 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -747,6 +747,7 @@ static int checkout(int submodule_progress)\n> \tmemset(&opts, 0, sizeof opts);\n> \topts.update = 1;\n> \topts.merge = 1;\n> +\topts.clone = 1;\n> \topts.fn = oneway_merge;\n> \topts.verbose_update = (option_verbosity >= 0);\n> \topts.src_index = &the_index;\n> diff --git a/cache.h b/cache.h\n> index 8b447652a7..6d6138f4f1 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1455,6 +1455,7 @@ struct checkout {\n> \tunsigned force:1,\n> \t\t quiet:1,\n> \t\t not_new:1,\n> +\t\t clone:1,\n> \t\t refresh_cache:1;\n> };\n> #define CHECKOUT_INIT { NULL, \"\" }\n> diff --git a/entry.c b/entry.c\n> index b5d1d3cf23..c70340df8e 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -399,6 +399,31 @@ static int check_path(const char *path, int len, struct stat *st, int skiplen)\n> \treturn lstat(path, st);\n> }\n> \n> +static void mark_colliding_entries(const struct checkout *state,\n> +\t\t\t\t struct cache_entry *ce, struct stat *st)\n> +{\n> +\tint i;\n> +\n> +\tce->ce_flags |= CE_MATCHED;\n> +\n> +#if !defined(GIT_WINDOWS_NATIVE) /* inode is always zero on Windows */\n> +\tfor (i = 0; i < state->istate->cache_nr; i++) {\n> +\t\tstruct cache_entry *dup = state->istate->cache[i];\n> +\n> +\t\tif (dup == ce)\n> +\t\t\tbreak;\n> +\n> +\t\tif (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE))\n> +\t\t\tcontinue;\n> +\n> +\t\tif (dup->ce_stat_data.sd_ino == st->st_ino) {\n> +\t\t\tdup->ce_flags |= CE_MATCHED;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +#endif\n> +}\n> +\n> /*\n> * Write the contents from ce out to the working tree.\n> *\n> @@ -455,6 +480,9 @@ int checkout_entry(struct cache_entry *ce,\n> \t\t\treturn -1;\n> \t\t}\n> \n> +\t\tif (state->clone)\n> +\t\t\tmark_colliding_entries(state, ce, &st);\n> +\n> \t\t/*\n> \t\t * We unlink the old file, to get the new one with the\n> \t\t * right permissions (including umask, which is nasty\n> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\n> index 0b62037744..f2eb73bc74 100755\n> --- a/t/t5601-clone.sh\n> +++ b/t/t5601-clone.sh\n> @@ -624,10 +624,16 @@ test_expect_success 'clone on case-insensitive fs' '\n> \t\t\tgit hash-object -w -t tree --stdin) &&\n> \t\tc=$(git commit-tree -m bogus $t) &&\n> \t\tgit update-ref refs/heads/bogus $c &&\n> -\t\tgit clone -b bogus . bogus\n> +\t\tgit clone -b bogus . bogus 2>warning\n> \t)\n> '\n> \n> +test_expect_success !MINGW,!CYGWIN,CASE_INSENSITIVE_FS 'colliding file detection' '\n> +\tgrep X icasefs/warning &&\n> +\tgrep x icasefs/warning &&\n> +\ttest_i18ngrep \"the following paths have collided\" icasefs/warning\n> +'\n> +\n> partial_clone () {\n> \t SERVER=\"$1\" &&\n> \t URL=\"$2\" &&\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index cd0680f11e..443df048ef 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -359,6 +359,12 @@ static int check_updates(struct unpack_trees_options *o)\n> \tstate.refresh_cache = 1;\n> \tstate.istate = index;\n> \n> +\tif (o->clone) {\n> +\t\tstate.clone = 1;\n> +\t\tfor (i = 0; i < index->cache_nr; i++)\n> +\t\t\tindex->cache[i]->ce_flags &= ~CE_MATCHED;\n> +\t}\n> +\n> \tprogress = get_progress(o);\n> \n> \tif (o->update)\n> @@ -423,6 +429,28 @@ static int check_updates(struct unpack_trees_options *o)\n> \terrs |= finish_delayed_checkout(&state);\n> \tif (o->update)\n> \t\tgit_attr_set_direction(GIT_ATTR_CHECKIN, NULL);\n> +\n> +\tif (o->clone) {\n> +\t\tint printed_warning = 0;\n> +\n> +\t\tfor (i = 0; i < index->cache_nr; i++) {\n> +\t\t\tstruct cache_entry *ce = index->cache[i];\n> +\n> +\t\t\tif (!(ce->ce_flags & CE_MATCHED))\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tif (!printed_warning) {\n> +\t\t\t\twarning(_(\"the following paths have collided (e.g. case-sensitive paths\\n\"\n> +\t\t\t\t\t \"on a case-insensitive filesystem) and only one from the same\\n\"\n> +\t\t\t\t\t \"colliding group is in the working tree:\\n\"));\n> +\t\t\t\tprinted_warning = 1;\n> +\t\t\t}\n> +\n> +\t\t\tfprintf(stderr, \" '%s'\\n\", ce->name);\n> +\t\t\tce->ce_flags &= ~CE_MATCHED;\n> +\t\t}\n> +\t}\n> +\n\nIf I'm reading this correctly, on Linux and friends, you'll print the\nnames of the files where the collision was detected and the paths of\nany peers found from the inum matching. And because of the #ifdef'ing\non Windows, we'll just get the former (at least for now).\n\nThat sounds fine.\nThanks\nJeff\n\n\n> \treturn errs != 0;\n> }\n> \n> diff --git a/unpack-trees.h b/unpack-trees.h\n> index c2b434c606..d940f1c5c2 100644\n> --- a/unpack-trees.h\n> +++ b/unpack-trees.h\n> @@ -42,6 +42,7 @@ struct unpack_trees_options {\n> \tunsigned int reset,\n> \t\t merge,\n> \t\t update,\n> +\t\t clone,\n> \t\t index_only,\n> \t\t nontrivial_merge,\n> \t\t trivial_merges_only,\n> \n"},{"id":"355399","messageId":"xmqq8t5a6wbc.fsf@gitster-ct.c.googlers.com","threadId":"48963","inReplyTo":"CACsJy8BeRYVvWvTQU+bj+hSQ3DFw0mHtSjtOg9zVSsXznpU=Xw@mail.gmail.com","subject":"Re: [PATCH v3 1/1] clone: report duplicate entries on case-insensitive filesystems","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-13T16:55:03Z","receivedAt":"2018-08-13T16:55:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> I was careless and checked the wrong variable (should have checked\n> nr_duplicates not state.nr_duplicates; the second is a pointer). So we\n> always get this warning (and with no following list of files)\n\nHeh, does that bug go away if you got rid of the pointer-ness of the\nfield and store the value directly in there?\n\n>> I also notice that this patch doesn't add any tests... :)\n>\n> This is platform specific and I was to be frank a bit lazy. Will\n> consider adding a test with CASE_INSENSITIVE_FS after this.\n"},{"id":"355402","messageId":"CACsJy8DudzY7n3XJ4tLKaLWEcA3ctwkzW8amny=KdhP4u2p_8g@mail.gmail.com","threadId":"48963","inReplyTo":"xmqq8t5a6wbc.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 1/1] clone: report duplicate entries on case-insensitive filesystems","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-08-13T17:12:10Z","receivedAt":"2018-08-13T17:12:39Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Aug 13, 2018 at 6:55 PM Junio C Hamano wrote:\n>\n> Duy Nguyen writes:\n>\n> > I was careless and checked the wrong variable (should have checked\n> > nr_duplicates not state.nr_duplicates; the second is a pointer). So we\n> > always get this warning (and with no following list of files)\n>\n> Heh, does that bug go away if you got rid of the pointer-ness of the\n> field and store the value directly in there?\n\nYou mean replacing the pointer with a real counter in struct checkout?\nThat would not work (it was my first option) because struct checkout\nis passed around as a const struct. entry.c code is not allowed to\nmake any updates there. So I got rid of both \"nr_duplicates\" and just\ncount again at the bottom of check_updates(). It's not that expensive\nand it simplifies the code.\n-- \nDuy\n"},{"id":"355408","messageId":"xmqq4lfy6v88.fsf@gitster-ct.c.googlers.com","threadId":"48963","inReplyTo":"20180812090714.19060-1-pclouds@gmail.com","subject":"Re: [PATCH v4] clone: report duplicate entries on case-insensitive filesystems","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-13T17:18:31Z","receivedAt":"2018-08-13T17:18:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> Paths that only differ in case work fine in a case-sensitive\n> filesystems, but if those repos are cloned in a case-insensitive one,\n> you'll get problems. The first thing to notice is \"git status\" will\n> never be clean with no indication what exactly is \"dirty\".\n>\n> This patch helps the situation a bit by pointing out the problem at\n> clone time. Even though this patch talks about case sensitivity, the\n> patch makes no assumption about folding rules by the filesystem. It\n> simply observes that if an entry has been already checked out at clone\n> time when we're about to write a new path, some folding rules are\n> behind this.\n>\n> This patch is tested with vim-colorschemes repository on a JFS partition\n> with case insensitive support on Linux. This repository has two files\n> darkBlue.vim and darkblue.vim.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> v4 removes nr_duplicates (and fixes that false warning Szeder\n> reported). It also hints about case insensitivity as a cause of\n> problem because it's most likely the case when this warning shows up.\n\nAh, you no longer have that counter and the pointer to the counter,\nas you do not even report how many paths collide ;-) Makes sense.\n\n> diff --git a/entry.c b/entry.c\n> index b5d1d3cf23..c70340df8e 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -399,6 +399,31 @@ static int check_path(const char *path, int len, struct stat *st, int skiplen)\n> \treturn lstat(path, st);\n> }\n> \n> +static void mark_colliding_entries(const struct checkout *state,\n> +\t\t\t\t struct cache_entry *ce, struct stat *st)\n> +{\n> +\tint i;\n> +\n> +\tce->ce_flags |= CE_MATCHED;\n> +\n> +#if !defined(GIT_WINDOWS_NATIVE) /* inode is always zero on Windows */\n> +\tfor (i = 0; i < state->istate->cache_nr; i++) {\n> +\t\tstruct cache_entry *dup = state->istate->cache[i];\n> +\n> +\t\tif (dup == ce)\n> +\t\t\tbreak;\n> +\n> +\t\tif (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE))\n> +\t\t\tcontinue;\n> +\n> +\t\tif (dup->ce_stat_data.sd_ino == st->st_ino) {\n> +\t\t\tdup->ce_flags |= CE_MATCHED;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +#endif\n> +}\n\nOK. The whole loop might want to become a call to a helper function\nwhose implementation is platform dependent in the future, but that\nshould be kept outside the topic and left for a future enhancement.\n\n> @@ -455,6 +480,9 @@ int checkout_entry(struct cache_entry *ce,\n> \t\t\treturn -1;\n> \t\t}\n> \n> +\t\tif (state->clone)\n> +\t\t\tmark_colliding_entries(state, ce, &st);\n\nOK. I haven't carefully looked at the codepath but is it more\ninvolved to instead *not* check out this ce (and leave the working\ntree file that is already there for another path in the index\nalone)? I suspect it won't be as simple as\n\n\t\tif (state->clone) {\n\t\t\tmark_colliding_entries(state, ce, &st);\n\t\t\treturn -1;\n\t\t}\n\nbut I think it would give much more pleasant end-user experience if\nwe can do so, especially on GIT_WINDOWS_NATIVE. I would imagine\nthat the first thing those who see the message \"foo.txt have\ncollided with something else we are not telling you\" would want to\ndo is to see what \"foo.txt\" contains---and it may be obvious to\nhuman that it contains the contents intended for \"Foo.txt\" instead,\nif we somehow refrained from overwriting it here, which would\ncompensate for the lack of \"this is the other path that collided\nwith your file.\"\n\nIt is perfectly an acceptable answer if it is \"I looked at it, and\nit is a lot more involved as there are these fallouts from the\ncodepaths that the control flows later from this point you haven't\nchecked and considered---let's keep overwriting, it is much safer.\"\n\n> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\n> index 0b62037744..f2eb73bc74 100755\n> --- a/t/t5601-clone.sh\n> +++ b/t/t5601-clone.sh\n> @@ -624,10 +624,16 @@ test_expect_success 'clone on case-insensitive fs' '\n> \t\t\tgit hash-object -w -t tree --stdin) &&\n> \t\tc=$(git commit-tree -m bogus $t) &&\n> \t\tgit update-ref refs/heads/bogus $c &&\n> -\t\tgit clone -b bogus . bogus\n> +\t\tgit clone -b bogus . bogus 2>warning\n> \t)\n> '\n> \n> +test_expect_success !MINGW,!CYGWIN,CASE_INSENSITIVE_FS 'colliding file detection' '\n> +\tgrep X icasefs/warning &&\n> +\tgrep x icasefs/warning &&\n> +\ttest_i18ngrep \"the following paths have collided\" icasefs/warning\n> +'\n> +\n\nAh, I was wondering why possible error message needs to be hidden in\nthe previous test---it is not hiding; it is capturing to look for\nthe paths in the message. Makes sense.\n\n"},{"id":"355743","messageId":"20180815190816.GA26521@tor.lan","threadId":"48963","inReplyTo":"20180812090714.19060-1-pclouds@gmail.com","subject":"Re: [PATCH v4] clone: report duplicate entries on case-insensitive filesystems","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2018-08-15T19:08:16Z","receivedAt":"2018-08-15T19:08:44Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Sun, Aug 12, 2018 at 11:07:14AM +0200, Nguyễn Thái Ngọc Duy wrote:\n> Paths that only differ in case work fine in a case-sensitive\n> filesystems, but if those repos are cloned in a case-insensitive one,\n> you'll get problems. The first thing to notice is \"git status\" will\n> never be clean with no indication what exactly is \"dirty\".\n> \n> This patch helps the situation a bit by pointing out the problem at\n> clone time. Even though this patch talks about case sensitivity, the\n> patch makes no assumption about folding rules by the filesystem. It\n> simply observes that if an entry has been already checked out at clone\n> time when we're about to write a new path, some folding rules are\n> behind this.\n> \n> This patch is tested with vim-colorschemes repository on a JFS partition\n> with case insensitive support on Linux. This repository has two files\n> darkBlue.vim and darkblue.vim.\n> \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> v4 removes nr_duplicates (and fixes that false warning Szeder\n> reported). It also hints about case insensitivity as a cause of\n> problem because it's most likely the case when this warning shows up.\n> \n> builtin/clone.c | 1 +\n> cache.h | 1 +\n> entry.c | 28 ++++++++++++++++++++++++++++\n> t/t5601-clone.sh | 8 +++++++-\n> unpack-trees.c | 28 ++++++++++++++++++++++++++++\n> unpack-trees.h | 1 +\n> 6 files changed, 66 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 5c439f1394..0702b0e9d0 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -747,6 +747,7 @@ static int checkout(int submodule_progress)\n> \tmemset(&opts, 0, sizeof opts);\n> \topts.update = 1;\n> \topts.merge = 1;\n> +\topts.clone = 1;\n> \topts.fn = oneway_merge;\n> \topts.verbose_update = (option_verbosity >= 0);\n> \topts.src_index = &the_index;\n> diff --git a/cache.h b/cache.h\n> index 8b447652a7..6d6138f4f1 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1455,6 +1455,7 @@ struct checkout {\n> \tunsigned force:1,\n> \t\t quiet:1,\n> \t\t not_new:1,\n> +\t\t clone:1,\n> \t\t refresh_cache:1;\n> };\n> #define CHECKOUT_INIT { NULL, \"\" }\n> diff --git a/entry.c b/entry.c\n> index b5d1d3cf23..c70340df8e 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -399,6 +399,31 @@ static int check_path(const char *path, int len, struct stat *st, int skiplen)\n> \treturn lstat(path, st);\n> }\n> \n> +static void mark_colliding_entries(const struct checkout *state,\n> +\t\t\t\t struct cache_entry *ce, struct stat *st)\n> +{\n> +\tint i;\n> +\n> +\tce->ce_flags |= CE_MATCHED;\n> +\n> +#if !defined(GIT_WINDOWS_NATIVE) /* inode is always zero on Windows */\n> +\tfor (i = 0; i < state->istate->cache_nr; i++) {\n> +\t\tstruct cache_entry *dup = state->istate->cache[i];\n> +\n> +\t\tif (dup == ce)\n> +\t\t\tbreak;\n> +\n> +\t\tif (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE))\n> +\t\t\tcontinue;\n> +\n\nShould the following be protected by core.checkstat ? \n\tif (check_stat) {\n\n\n> +\t\tif (dup->ce_stat_data.sd_ino == st->st_ino) {\n> +\t\t\tdup->ce_flags |= CE_MATCHED;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +#endif\n\nAnother thing is that we switch of the ASCII case-folding-detection-logic\noff for Windows users, even if we otherwise rely on icase.\nI think we can use fspathcmp() as a fallback. when inodes fail,\nbecause we may be on a network file system.\n(I don't have a test setup at the moment, but what happens with inodes\nwhen a Windows machine exports a share to Linux or Mac ?)\n\nIs there a chance to get the fspathcmp() back, like this ?\n\nstatic void mark_colliding_entries(const struct checkout *state,\n\t\t\t\t struct cache_entry *ce, struct stat *st)\n{\n\tint i;\n\tce->ce_flags |= CE_MATCHED;\n\n\tfor (i = 0; i < state->istate->cache_nr; i++) {\n\t\tstruct cache_entry *dup = state->istate->cache[i];\n\t\tint folded = 0;\n\n\t\tif (dup == ce)\n\t\t\tbreak;\n\n\t\tif (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE))\n\t\t\tcontinue;\n\n\t\tif (!fspathcmp(dup->name, ce->name))\n\t\t\tfolded = 1;\n\n#if !defined(GIT_WINDOWS_NATIVE) /* inode is always zero on Windows */\n\t\tif (check_stat && (dup->ce_stat_data.sd_ino == st->st_ino))\n\t\t\tfolded = 1;\n#endif\n\t\tif (folded) {\n\t\t\tdup->ce_flags |= CE_MATCHED;\n\t\t\tbreak;\n\t\t}\n\t}\n}\n\n"},{"id":"355749","messageId":"CACsJy8AYQL3oDLyt14eJ1emynngqKQv9GXju56gU9u4mHrFHOg@mail.gmail.com","threadId":"48963","inReplyTo":"20180815190816.GA26521@tor.lan","subject":"Re: [PATCH v4] clone: report duplicate entries on case-insensitive filesystems","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-08-15T19:35:41Z","receivedAt":"2018-08-15T19:36:10Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Aug 15, 2018 at 9:08 PM Torsten Bögershausen wrote:\n> > +#if !defined(GIT_WINDOWS_NATIVE) /* inode is always zero on Windows */\n> > + for (i = 0; i < state->istate->cache_nr; i++) {\n> > + struct cache_entry *dup = state->istate->cache[i];\n> > +\n> > + if (dup == ce)\n> > + break;\n> > +\n> > + if (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE))\n> > + continue;\n> > +\n>\n> Should the following be protected by core.checkstat ?\n> if (check_stat) {\n\nGood catch! st_ino is ignored if core.checkStat is false. I will\nprobably send a separate patch to add more details to config.txt about\nthis key.\n\n> > + if (dup->ce_stat_data.sd_ino == st->st_ino) {\n> > + dup->ce_flags |= CE_MATCHED;\n> > + break;\n> > + }\n> > + }\n> > +#endif\n>\n> Another thing is that we switch of the ASCII case-folding-detection-logic\n> off for Windows users, even if we otherwise rely on icase.\n> I think we can use fspathcmp() as a fallback. when inodes fail,\n> because we may be on a network file system.\n\nI admit I did not think about network file system. Will spend some\ntime (and hopefully not on nfs kernel code) on it.\n\nFor falling back on fspathcmp even on Windows, is it really safe? I'm\non Linux and never have to deal with this issue to have any\nexperience. It does sound good though because it should be a subset\nfor any \"weird\" filesystems out there.\n\n> (I don't have a test setup at the moment, but what happens with inodes\n> when a Windows machine exports a share to Linux or Mac ?)\n>\n> Is there a chance to get the fspathcmp() back, like this ?\n>\n> static void mark_colliding_entries(const struct checkout *state,\n> struct cache_entry *ce, struct stat *st)\n> {\n> int i;\n> ce->ce_flags |= CE_MATCHED;\n>\n> for (i = 0; i < state->istate->cache_nr; i++) {\n> struct cache_entry *dup = state->istate->cache[i];\n> int folded = 0;\n>\n> if (dup == ce)\n> break;\n>\n> if (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE))\n> continue;\n>\n> if (!fspathcmp(dup->name, ce->name))\n> folded = 1;\n>\n> #if !defined(GIT_WINDOWS_NATIVE) /* inode is always zero on Windows */\n> if (check_stat && (dup->ce_stat_data.sd_ino == st->st_ino))\n> folded = 1;\n> #endif\n> if (folded) {\n> dup->ce_flags |= CE_MATCHED;\n> break;\n> }\n> }\n> }\n>\n\n\n-- \nDuy\n"},{"id":"355750","messageId":"xmqqtvnvh12u.fsf@gitster-ct.c.googlers.com","threadId":"48963","inReplyTo":"20180815190816.GA26521@tor.lan","subject":"Re: [PATCH v4] clone: report duplicate entries on case-insensitive filesystems","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-15T19:38:49Z","receivedAt":"2018-08-15T19:38:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n>> +\n>> +#if !defined(GIT_WINDOWS_NATIVE) /* inode is always zero on Windows */\n>> +\tfor (i = 0; i < state->istate->cache_nr; i++) {\n>> +\t\tstruct cache_entry *dup = state->istate->cache[i];\n>> +\n>> +\t\tif (dup == ce)\n>> +\t\t\tbreak;\n>> +\n>> +\t\tif (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE))\n>> +\t\t\tcontinue;\n>> +\n>\n> Should the following be protected by core.checkstat ? \n> \tif (check_stat) {\n\nI do not think such a if statement is strictly necessary.\n\nEven if check_stat tells us \"when checking if a cached stat\ninformation tells us that the path may have modified, use minimum\nset of fields from the 'struct stat'\", we still capture and update\nthe values from the same \"full\" set of fields when we mark a cache\nentry up-to-date. So it all depends on why you are limiting with\ncheck_stat. Is it because stdev is unusable? Is it because nsec is\nunusable? Is it because ino is unusable? Only in the last case,\npaying attention to check_stat will reduce the false positive.\n\nBut then you made me wonder what value check_stat has on Windows.\nIf it is false, perhaps we do not even need the conditional\ncompilation, which is a huge plus.\n\n>> +\t\tif (dup->ce_stat_data.sd_ino == st->st_ino) {\n>> +\t\t\tdup->ce_flags |= CE_MATCHED;\n>> +\t\t\tbreak;\n>> +\t\t}\n>> +\t}\n>> +#endif\n>\n> Another thing is that we switch of the ASCII case-folding-detection-logic\n> off for Windows users, even if we otherwise rely on icase.\n> I think we can use fspathcmp() as a fallback. when inodes fail,\n> because we may be on a network file system.\n>\n> (I don't have a test setup at the moment, but what happens with inodes\n> when a Windows machine exports a share to Linux or Mac ?)\n>\n> Is there a chance to get the fspathcmp() back, like this ?\n\nIf fspathcmp() never gives false positives, I do not think we would\nmind using it like your update. False negatives are fine, as that\nis better than just punting the whole thing when there is no usable\ninum. And we do not care all that much if it is more expensive;\nthis is an error codepath after all.\n\nAnd from code structure's point of view, I think it makes sense. It\nwould be even better if we can lose the conditional compilation.\n\nAnother thing we maybe want to see is if we can update the caller of\nthis function so that we do not overwrite the earlier checkout with\nthe data for this path. When two paths collide, we check out one of\nthe paths without reporting (because we cannot notice), then attempt\nto check out the other path and report (because we do notice the\nprevious one with lstat()). The current code then goes on and overwrites\nthe file with the contents from the \"other\" path.\n\nEven if we had false negative in this loop, if we leave the contents\nfor the earlier path while reporting the \"other\" path, then the user\ncan get curious, inspect what contents the \"other\" path has on the\nfilesystem, and can notice that it belongs to the (unreported--due\nto false negative) earlier path.\n\n> static void mark_colliding_entries(const struct checkout *state,\n> \t\t\t\t struct cache_entry *ce, struct stat *st)\n> {\n> \tint i;\n> \tce->ce_flags |= CE_MATCHED;\n>\n> \tfor (i = 0; i < state->istate->cache_nr; i++) {\n> \t\tstruct cache_entry *dup = state->istate->cache[i];\n> \t\tint folded = 0;\n>\n> \t\tif (dup == ce)\n> \t\t\tbreak;\n>\n> \t\tif (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE))\n> \t\t\tcontinue;\n>\n> \t\tif (!fspathcmp(dup->name, ce->name))\n> \t\t\tfolded = 1;\n>\n> #if !defined(GIT_WINDOWS_NATIVE) /* inode is always zero on Windows */\n> \t\tif (check_stat && (dup->ce_stat_data.sd_ino == st->st_ino))\n> \t\t\tfolded = 1;\n> #endif\n> \t\tif (folded) {\n> \t\t\tdup->ce_flags |= CE_MATCHED;\n> \t\t\tbreak;\n> \t\t}\n> \t}\n> }\n"},{"id":"355832","messageId":"20180816140312.GA6102@tor.lan","threadId":"48963","inReplyTo":"xmqqtvnvh12u.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v4] clone: report duplicate entries on case-insensitive filesystems","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2018-08-16T14:03:12Z","receivedAt":"2018-08-16T14:03:38Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Wed, Aug 15, 2018 at 12:38:49PM -0700, Junio C Hamano wrote:\n\nThis should answer Duys comments as well.\n> Torsten Bögershausen writes:\n> \n[snip]\n> > Should the following be protected by core.checkstat ? \n> > \tif (check_stat) {\n> \n> I do not think such a if statement is strictly necessary.\n> \n> Even if check_stat tells us \"when checking if a cached stat\n> information tells us that the path may have modified, use minimum\n> set of fields from the 'struct stat'\", we still capture and update\n> the values from the same \"full\" set of fields when we mark a cache\n> entry up-to-date. So it all depends on why you are limiting with\n> check_stat. Is it because stdev is unusable? Is it because nsec is\n> unusable? Is it because ino is unusable? Only in the last case,\n> paying attention to check_stat will reduce the false positive.\n> \n> But then you made me wonder what value check_stat has on Windows.\n> If it is false, perhaps we do not even need the conditional\n> compilation, which is a huge plus.\n\nAgreed:\ncheck_stat is 0 on Windows, and inum is allways 0 in lstat().\nI was thinking about systems which don't have inodes and inum,\nand then generate an inum in memory, sometimes random.\nAfter a reboot or a re-mount of the file systems those ino values\nchange.\nHowever, for the initial clone we are fine in any case.\n\n> \n> >> +\t\tif (dup->ce_stat_data.sd_ino == st->st_ino) {\n> >> +\t\t\tdup->ce_flags |= CE_MATCHED;\n> >> +\t\t\tbreak;\n> >> +\t\t}\n> >> +\t}\n> >> +#endif\n> >\n> > Another thing is that we switch of the ASCII case-folding-detection-logic\n> > off for Windows users, even if we otherwise rely on icase.\n> > I think we can use fspathcmp() as a fallback. when inodes fail,\n> > because we may be on a network file system.\n> >\n> > (I don't have a test setup at the moment, but what happens with inodes\n> > when a Windows machine exports a share to Linux or Mac ?)\n> >\n> > Is there a chance to get the fspathcmp() back, like this ?\n> \n> If fspathcmp() never gives false positives, I do not think we would\n> mind using it like your update. False negatives are fine, as that\n> is better than just punting the whole thing when there is no usable\n> inum. And we do not care all that much if it is more expensive;\n> this is an error codepath after all.\n> \n> And from code structure's point of view, I think it makes sense. It\n> would be even better if we can lose the conditional compilation.\n\nThe current implementation of fspathcmp() does not give false positvies,\nand future versions should not either.\nAll case-insentive file systems have always treated 'a-z' equal to 'A-Z'.\nIn FAT MS/DOS there had only been uppercase letters as file names,\nand `type file.txt` (the equivilant to ´cat file.txt´ in *nix)\nsimply resultet in `type FILE.TXT`\nLater, with VFAT and later with HPFS/NTFS a file could be stored on\ndisk as \"File.txt\".\nFrom now on ´type FILE.TXT´ still worked, (and all other upper-lowercase\ncombinations).\nThis all is probably nothing new.\nThe main point should be that fspathcmp() should never return a false positive,\nand I think we all agree on that. \n\n\nNow back to the compiler switch:\nWindows always set inum to 0 and I can't think about a situation where\na file in a working tree gets inum = 0, can we use the following:\n\nstatic void mark_colliding_entries(const struct checkout *state,\n\t\t\t\t struct cache_entry *ce, struct stat *st)\n{\n\tint i;\n\tce->ce_flags |= CE_MATCHED;\n\n\tfor (i = 0; i < state->istate->cache_nr; i++) {\n\t\tstruct cache_entry *dup = state->istate->cache[i];\n\t\tint folded = 0;\n\n\t\tif (dup == ce)\n\t\t\tbreak;\n\n\t\tif (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE))\n\t\t\tcontinue;\n\t\t/*\n\t\t * Windows sets ino to 0. On other FS ino = 0 will already be\n\t\t * used, so we don't see it for a file in a Git working tree\n\t\t */\n\t\tif (st->st_ino && (dup->ce_stat_data.sd_ino == st->st_ino))\n\t\t\tfolded = 1;\n\n\t\t/*\n\t\t * Fallback for NTFS and other case insenstive FS,\n\t\t * which don't use POSIX inums\n\t\t */\n\t\tif (!fspathcmp(dup->name, ce->name))\n\t\t\tfolded = 1;\n\n\t\tif (folded) {\n\t\t\tdup->ce_flags |= CE_MATCHED;\n\t\t\tbreak;\n\t\t}\n\t}\n}\n\n\n> \n> Another thing we maybe want to see is if we can update the caller of\n> this function so that we do not overwrite the earlier checkout with\n> the data for this path. When two paths collide, we check out one of\n> the paths without reporting (because we cannot notice), then attempt\n> to check out the other path and report (because we do notice the\n> previous one with lstat()). The current code then goes on and overwrites\n> the file with the contents from the \"other\" path.\n> \n> Even if we had false negative in this loop, if we leave the contents\n> for the earlier path while reporting the \"other\" path, then the user\n> can get curious, inspect what contents the \"other\" path has on the\n> filesystem, and can notice that it belongs to the (unreported--due\n> to false negative) earlier path.\n> \n[snip]\n"},{"id":"355841","messageId":"CACsJy8AwfFNJp56rdmGe8ZqZhjCeOZ16i8YX6AhSwJPHg=1EFQ@mail.gmail.com","threadId":"48963","inReplyTo":"20180816140312.GA6102@tor.lan","subject":"Re: [PATCH v4] clone: report duplicate entries on case-insensitive filesystems","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-08-16T15:42:57Z","receivedAt":"2018-08-16T15:43:26Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Aug 16, 2018 at 4:03 PM Torsten Bögershausen wrote:\n> check_stat is 0 on Windows,\n\nHow? check_stat is 1 by default. And \"git init\" does not add this key\non new repos.\n\n> Now back to the compiler switch:\n> Windows always set inum to 0 and I can't think about a situation where\n> a file in a working tree gets inum = 0, can we use the following:\n\nI did consider using zero inum, but dropped it when thinking about\nchecking all file systems. Are you sure zero inode can't be valid?\n-- \nDuy\n"},{"id":"355844","messageId":"20180816155647.10459-1-pclouds@gmail.com","threadId":"48963","inReplyTo":"CACsJy8AYQL3oDLyt14eJ1emynngqKQv9GXju56gU9u4mHrFHOg@mail.gmail.com","subject":"[PATCH] config.txt: clarify core.checkStat = minimal","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-08-16T15:56:47Z","receivedAt":"2018-08-16T15:57:05Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The description of this key does not really tell what 'minimal' mode\nchecks exactly. More information about this mode can be found in the\ncommit message of c08e4d5b5c (Enable minimal stat checking -\n2013-01-22).\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Documentation/config.txt | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex fd8d27e761..5c41314dd5 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -466,6 +466,8 @@ core.checkStat::\n \tand work tree. The user can set this to 'default' or\n \t'minimal'. Default (or explicitly 'default'), is to check\n \tall fields, including the sub-second part of mtime and ctime.\n+\t'minimal' only checks size and the whole second part of mtime\n+\tand ctime.\n \n core.quotePath::\n \tCommands that output paths (e.g. 'ls-files', 'diff'), will\n-- \n2.18.0.1004.g6639190530\n\n"},{"id":"355850","messageId":"xmqqpnyiffgo.fsf@gitster-ct.c.googlers.com","threadId":"48963","inReplyTo":"20180816140312.GA6102@tor.lan","subject":"Re: [PATCH v4] clone: report duplicate entries on case-insensitive filesystems","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-16T16:23:19Z","receivedAt":"2018-08-16T16:23:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> check_stat is 0 on Windows, and inum is allways 0 in lstat().\n> I was thinking about systems which don't have inodes and inum,\n> and then generate an inum in memory, sometimes random.\n> After a reboot or a re-mount of the file systems those ino values\n> change.\n> However, for the initial clone we are fine in any case.\n\nYup.\n\n> Now back to the compiler switch:\n> Windows always set inum to 0 and I can't think about a situation where\n> a file in a working tree gets inum = 0, can we use the following:\n>\n> static void mark_colliding_entries(const struct checkout *state,\n> \t\t\t\t struct cache_entry *ce, struct stat *st)\n> {\n> \tint i;\n> \tce->ce_flags |= CE_MATCHED;\n>\n> \tfor (i = 0; i < state->istate->cache_nr; i++) {\n> \t\tstruct cache_entry *dup = state->istate->cache[i];\n> \t\tint folded = 0;\n>\n> \t\tif (dup == ce)\n> \t\t\tbreak;\n>\n> \t\tif (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE))\n> \t\t\tcontinue;\n> \t\t/*\n> \t\t * Windows sets ino to 0. On other FS ino = 0 will already be\n> \t\t * used, so we don't see it for a file in a Git working tree\n> \t\t */\n> \t\tif (st->st_ino && (dup->ce_stat_data.sd_ino == st->st_ino))\n> \t\t\tfolded = 1;\n\nHmm, that is tempting but feels slightly too magical to my taste.\nOthers may easily be able to persuade me to change my mind in this\ncase, though.\n\n> \t\t/*\n> \t\t * Fallback for NTFS and other case insenstive FS,\n> \t\t * which don't use POSIX inums\n> \t\t */\n> \t\tif (!fspathcmp(dup->name, ce->name))\n> \t\t\tfolded = 1;\n>\n> \t\tif (folded) {\n> \t\t\tdup->ce_flags |= CE_MATCHED;\n> \t\t\tbreak;\n> \t\t}\n> \t}\n> }\n>\n>\n>> \n>> Another thing we maybe want to see is if we can update the caller of\n>> this function so that we do not overwrite the earlier checkout with\n>> the data for this path. When two paths collide, we check out one of\n>> the paths without reporting (because we cannot notice), then attempt\n>> to check out the other path and report (because we do notice the\n>> previous one with lstat()). The current code then goes on and overwrites\n>> the file with the contents from the \"other\" path.\n>> \n>> Even if we had false negative in this loop, if we leave the contents\n>> for the earlier path while reporting the \"other\" path, then the user\n>> can get curious, inspect what contents the \"other\" path has on the\n>> filesystem, and can notice that it belongs to the (unreported--due\n>> to false negative) earlier path.\n>> \n> [snip]\n"},{"id":"355852","messageId":"xmqqin4afdoj.fsf@gitster-ct.c.googlers.com","threadId":"48963","inReplyTo":"20180816155647.10459-1-pclouds@gmail.com","subject":"Re: [PATCH] config.txt: clarify core.checkStat = minimal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-16T17:01:48Z","receivedAt":"2018-08-16T17:01:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> The description of this key does not really tell what 'minimal' mode\n> checks exactly. More information about this mode can be found in the\n> commit message of c08e4d5b5c (Enable minimal stat checking -\n> 2013-01-22).\n>\n\nWhile I agree that we need to do _something_, I am not sure if this\nchange adds sufficient value. I _think_ those who wonder if they\nwant to configure this want to know what are _not_ looked at\n(relative to the \"default\") more than what are _still_ looked at,\npartly because the description of \"default\" is already bogus and\nsays \"check all fields\", which is horrible for two reasons. It is\nunclear what are in \"all\" fields in the first place, and also we do\nnot look at all fields (e.g. we do not look at atime for obvious\nreasons).\n\nSo perhaps\n\n\tWhen this configuration variable is missing or is set to\n\t`default`, many fields in the stat structure are checked to\n\tdetect if a file has been modified since Git looked at it.\n\tAmong these fields, when this configuration variable is set\n\tto `minimal`, sub-second part of mtime and ctime, the uid\n\tand gid of the owner of the file, the inode number (and the\n\tdevice number, if Git was compiled to use it), are excluded\n\tfrom the check, leaving only the whole-second part of mtime\n\t(and ctime, if `core.trustCtime` is set) and the filesize to\n\tbe checked.\n\nor something?\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> Documentation/config.txt | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index fd8d27e761..5c41314dd5 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -466,6 +466,8 @@ core.checkStat::\n> \tand work tree. The user can set this to 'default' or\n> \t'minimal'. Default (or explicitly 'default'), is to check\n> \tall fields, including the sub-second part of mtime and ctime.\n> +\t'minimal' only checks size and the whole second part of mtime\n> +\tand ctime.\n> \n> core.quotePath::\n> \tCommands that output paths (e.g. 'ls-files', 'diff'), will\n"},{"id":"355865","messageId":"CACsJy8C2r5y0m88yrRQHQ-_QNXemy2pfcjxYK0zSd0J3fFy3rQ@mail.gmail.com","threadId":"48963","inReplyTo":"xmqqin4afdoj.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] config.txt: clarify core.checkStat = minimal","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-08-16T18:19:01Z","receivedAt":"2018-08-16T18:19:30Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Aug 16, 2018 at 7:01 PM Junio C Hamano wrote:\n>\n> Nguyễn Thái Ngọc Duy writes:\n>\n> > The description of this key does not really tell what 'minimal' mode\n> > checks exactly. More information about this mode can be found in the\n> > commit message of c08e4d5b5c (Enable minimal stat checking -\n> > 2013-01-22).\n> >\n>\n> While I agree that we need to do _something_, I am not sure if this\n> change adds sufficient value. I _think_ those who wonder if they\n> want to configure this want to know what are _not_ looked at\n> (relative to the \"default\") more than what are _still_ looked at,\n> partly because the description of \"default\" is already bogus and\n> says \"check all fields\", which is horrible for two reasons. It is\n> unclear what are in \"all\" fields in the first place, and also we do\n> not look at all fields (e.g. we do not look at atime for obvious\n> reasons).\n>\n> So perhaps\n>\n> When this configuration variable is missing or is set to\n> `default`, many fields in the stat structure are checked to\n> detect if a file has been modified since Git looked at it.\n> Among these fields, when this configuration variable is set\n> to `minimal`, sub-second part of mtime and ctime, the uid\n> and gid of the owner of the file, the inode number (and the\n> device number, if Git was compiled to use it), are excluded\n> from the check, leaving only the whole-second part of mtime\n> (and ctime, if `core.trustCtime` is set) and the filesize to\n> be checked.\n>\n> or something?\n\nPerfect. I could wrap it in a patch, but I feel you should take\nauthorship for that one. I'll leave it to you to create this commit.\n-- \nDuy\n"},{"id":"355885","messageId":"xmqqy3d6c5co.fsf@gitster-ct.c.googlers.com","threadId":"48963","inReplyTo":"CACsJy8C2r5y0m88yrRQHQ-_QNXemy2pfcjxYK0zSd0J3fFy3rQ@mail.gmail.com","subject":"Re: [PATCH] config.txt: clarify core.checkStat = minimal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-16T22:29:59Z","receivedAt":"2018-08-16T22:30:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Thu, Aug 16, 2018 at 7:01 PM Junio C Hamano wrote:\n>>\n>> Nguyễn Thái Ngọc Duy writes:\n>>\n>> > The description of this key does not really tell what 'minimal' mode\n>> > checks exactly. More information about this mode can be found in the\n>> > commit message of c08e4d5b5c (Enable minimal stat checking -\n>> > 2013-01-22).\n>> >\n>>\n>> While I agree that we need to do _something_, I am not sure if this\n>> change adds sufficient value. I _think_ those who wonder if they\n>> want to configure this want to know what are _not_ looked at\n>> (relative to the \"default\") more than what are _still_ looked at,\n>> partly because the description of \"default\" is already bogus and\n>> says \"check all fields\", which is horrible for two reasons. It is\n>> unclear what are in \"all\" fields in the first place, and also we do\n>> not look at all fields (e.g. we do not look at atime for obvious\n>> reasons).\n>>\n>> So perhaps\n>>\n>> When this configuration variable is missing or is set to\n>> `default`, many fields in the stat structure are checked to\n>> detect if a file has been modified since Git looked at it.\n>> Among these fields, when this configuration variable is set\n>> to `minimal`, sub-second part of mtime and ctime, the uid\n>> and gid of the owner of the file, the inode number (and the\n>> device number, if Git was compiled to use it), are excluded\n>> from the check, leaving only the whole-second part of mtime\n>> (and ctime, if `core.trustCtime` is set) and the filesize to\n>> be checked.\n>>\n>> or something?\n>\n> Perfect. I could wrap it in a patch, but I feel you should take\n> authorship for that one. I'll leave it to you to create this commit.\n\nIf I find time after today's integration cycle, perhaps I can get to\nit, but not until then (so the above won't be in today's pushout).\n\nThanks for reading it over.\n\n\n"},{"id":"355912","messageId":"xmqqefexc8vm.fsf@gitster-ct.c.googlers.com","threadId":"48963","inReplyTo":"CACsJy8C2r5y0m88yrRQHQ-_QNXemy2pfcjxYK0zSd0J3fFy3rQ@mail.gmail.com","subject":"Re: [PATCH] config.txt: clarify core.checkStat = minimal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-17T15:26:05Z","receivedAt":"2018-08-17T15:26:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> Perfect. I could wrap it in a patch, but I feel you should take\n> authorship for that one. I'll leave it to you to create this commit.\n\nOK, here is what I ended up with. An extra paragraph was taken from\nthe old commit you referrred to, which is probably the only\nremaining part from your contribution, so the attribution has been\ndemoted to \"Helped-by\", but your initiative still is appreciated\nvery much.\n\n-- >8 --\nSubject: [PATCH] config.txt: clarify core.checkStat\n\nThe description of this key does not really tell what the 'minimal'\nmode checks and does not check. The description for the 'default'\nmode is not much better and just says 'all fields', which is unclear\nand is not even correct (e.g. we do not look at 'atime').\n\nSpell out what are and what are not checked under the 'minimal' mode\nrelative to the 'default' mode to help those who want to decide if\nthey want to use the 'minimal' mode, also taking information about\nthis mode from the commit message of c08e4d5b5c (Enable minimal stat\nchecking - 2013-01-22).\n\nHelped-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Junio C Hamano \n---\n Documentation/config.txt | 18 ++++++++++++++----\n 1 file changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex ab641bf5a9..933d719137 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -449,10 +449,20 @@ core.untrackedCache::\n \tSee linkgit:git-update-index[1]. `keep` by default.\n \n core.checkStat::\n-\tDetermines which stat fields to match between the index\n-\tand work tree. The user can set this to 'default' or\n-\t'minimal'. Default (or explicitly 'default'), is to check\n-\tall fields, including the sub-second part of mtime and ctime.\n+\tWhen missing or is set to `default`, many fields in the stat\n+\tstructure are checked to detect if a file has been modified\n+\tsince Git looked at it. When this configuration variable is\n+\tset to `minimal`, sub-second part of mtime and ctime, the\n+\tuid and gid of the owner of the file, the inode number (and\n+\tthe device number, if Git was compiled to use it), are\n+\texcluded from the check among these fields, leaving only the\n+\twhole-second part of mtime (and ctime, if `core.trustCtime`\n+\tis set) and the filesize to be checked.\n++\n+There are implementations of Git that do not leave usable values in\n+some fields (e.g. JGit); by excluding these fields from the\n+comparison, the `minimal` mode may help interoperability when the\n+same repository is used by these other systems at the same time.\n \n core.quotePath::\n \tCommands that output paths (e.g. 'ls-files', 'diff'), will\n-- \n2.18.0-666-g63749b2dea\n\n"},{"id":"355914","messageId":"CACsJy8BmvT5JqckDqA8d_3EwZsav3QPDvHX2o4aWJNJNscQ9kg@mail.gmail.com","threadId":"48963","inReplyTo":"xmqqefexc8vm.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] config.txt: clarify core.checkStat = minimal","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-08-17T15:29:53Z","receivedAt":"2018-08-17T15:30:22Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Aug 17, 2018 at 5:26 PM Junio C Hamano wrote:\n> -- >8 --\n> Subject: [PATCH] config.txt: clarify core.checkStat\n>\n> The description of this key does not really tell what the 'minimal'\n> mode checks and does not check. The description for the 'default'\n> mode is not much better and just says 'all fields', which is unclear\n> and is not even correct (e.g. we do not look at 'atime').\n>\n> Spell out what are and what are not checked under the 'minimal' mode\n> relative to the 'default' mode to help those who want to decide if\n> they want to use the 'minimal' mode, also taking information about\n> this mode from the commit message of c08e4d5b5c (Enable minimal stat\n> checking - 2013-01-22).\n\nLooking good. This does make me want to adjust $GIT_DIR/index format\nto optionally not store extra fields if we know we're not going to use\nthem. But that's a topic for another day.\n\n> Helped-by: Nguyễn Thái Ngọc Duy \n> Signed-off-by: Junio C Hamano \n> ---\n> Documentation/config.txt | 18 ++++++++++++++----\n> 1 file changed, 14 insertions(+), 4 deletions(-)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index ab641bf5a9..933d719137 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -449,10 +449,20 @@ core.untrackedCache::\n> See linkgit:git-update-index[1]. `keep` by default.\n>\n> core.checkStat::\n> - Determines which stat fields to match between the index\n> - and work tree. The user can set this to 'default' or\n> - 'minimal'. Default (or explicitly 'default'), is to check\n> - all fields, including the sub-second part of mtime and ctime.\n> + When missing or is set to `default`, many fields in the stat\n> + structure are checked to detect if a file has been modified\n> + since Git looked at it. When this configuration variable is\n> + set to `minimal`, sub-second part of mtime and ctime, the\n> + uid and gid of the owner of the file, the inode number (and\n> + the device number, if Git was compiled to use it), are\n> + excluded from the check among these fields, leaving only the\n> + whole-second part of mtime (and ctime, if `core.trustCtime`\n> + is set) and the filesize to be checked.\n> ++\n> +There are implementations of Git that do not leave usable values in\n> +some fields (e.g. JGit); by excluding these fields from the\n> +comparison, the `minimal` mode may help interoperability when the\n> +same repository is used by these other systems at the same time.\n>\n> core.quotePath::\n> Commands that output paths (e.g. 'ls-files', 'diff'), will\n> --\n> 2.18.0-666-g63749b2dea\n>\n\n\n-- \nDuy\n"},{"id":"355917","messageId":"20180817161645.28249-1-pclouds@gmail.com","threadId":"48963","inReplyTo":"20180812090714.19060-1-pclouds@gmail.com","subject":"[PATCH v5] clone: report duplicate entries on case-insensitive filesystems","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-08-17T16:16:45Z","receivedAt":"2018-08-17T16:16:59Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Paths that only differ in case work fine in a case-sensitive\nfilesystems, but if those repos are cloned in a case-insensitive one,\nyou'll get problems. The first thing to notice is \"git status\" will\nnever be clean with no indication what exactly is \"dirty\".\n\nThis patch helps the situation a bit by pointing out the problem at\nclone time. Even though this patch talks about case sensitivity, the\npatch makes no assumption about folding rules by the filesystem. It\nsimply observes that if an entry has been already checked out at clone\ntime when we're about to write a new path, some folding rules are\nbehind this.\n\nIn the case that we can't rely on filesystem (via inode number) to do\nthis check, fall back to fspathcmp() which is not perfect but should\nnot give false positives.\n\nThis patch is tested with vim-colorschemes and Sublime-Gitignore\nrepositories on a JFS partition with case insensitive support on\nLinux.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n v5 respects core.checkStat and sorts the output case-insensitively.\n\n I still don't trust magic st_ino zero, or core.checkStat being zero\n on Windows, so the #if condition still remains but it covers smallest\n area possible and I tested it by manually make it \"#if 1\"\n\n The fallback with fspathcmp() is only done when inode can't be\n trusted because strcmp is more expensive and when fspathcmp() learns\n more about real world in the future, it could become even more\n expensive.\n\n The output sorting is the result of Sublime-Gitignore repo being\n reported recently. It's not perfect but it should help seeing the\n groups in normal case.\n\n builtin/clone.c | 1 +\n cache.h | 1 +\n entry.c | 31 +++++++++++++++++++++++++++++++\n t/t5601-clone.sh | 8 +++++++-\n unpack-trees.c | 35 +++++++++++++++++++++++++++++++++++\n unpack-trees.h | 1 +\n 6 files changed, 76 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 5c439f1394..0702b0e9d0 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -747,6 +747,7 @@ static int checkout(int submodule_progress)\n \tmemset(&opts, 0, sizeof opts);\n \topts.update = 1;\n \topts.merge = 1;\n+\topts.clone = 1;\n \topts.fn = oneway_merge;\n \topts.verbose_update = (option_verbosity >= 0);\n \topts.src_index = &the_index;\ndiff --git a/cache.h b/cache.h\nindex 8b447652a7..6d6138f4f1 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1455,6 +1455,7 @@ struct checkout {\n \tunsigned force:1,\n \t\t quiet:1,\n \t\t not_new:1,\n+\t\t clone:1,\n \t\t refresh_cache:1;\n };\n #define CHECKOUT_INIT { NULL, \"\" }\ndiff --git a/entry.c b/entry.c\nindex b5d1d3cf23..8766e27255 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -399,6 +399,34 @@ static int check_path(const char *path, int len, struct stat *st, int skiplen)\n \treturn lstat(path, st);\n }\n \n+static void mark_colliding_entries(const struct checkout *state,\n+\t\t\t\t struct cache_entry *ce, struct stat *st)\n+{\n+\tint i, trust_ino = check_stat;\n+\n+#if defined(GIT_WINDOWS_NATIVE)\n+\ttrust_ino = 0;\n+#endif\n+\n+\tce->ce_flags |= CE_MATCHED;\n+\n+\tfor (i = 0; i < state->istate->cache_nr; i++) {\n+\t\tstruct cache_entry *dup = state->istate->cache[i];\n+\n+\t\tif (dup == ce)\n+\t\t\tbreak;\n+\n+\t\tif (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE))\n+\t\t\tcontinue;\n+\n+\t\tif ((trust_ino && dup->ce_stat_data.sd_ino == st->st_ino) ||\n+\t\t (!trust_ino && !fspathcmp(ce->name, dup->name))) {\n+\t\t\tdup->ce_flags |= CE_MATCHED;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+}\n+\n /*\n * Write the contents from ce out to the working tree.\n *\n@@ -455,6 +483,9 @@ int checkout_entry(struct cache_entry *ce,\n \t\t\treturn -1;\n \t\t}\n \n+\t\tif (state->clone)\n+\t\t\tmark_colliding_entries(state, ce, &st);\n+\n \t\t/*\n \t\t * We unlink the old file, to get the new one with the\n \t\t * right permissions (including umask, which is nasty\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 0b62037744..f2eb73bc74 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -624,10 +624,16 @@ test_expect_success 'clone on case-insensitive fs' '\n \t\t\tgit hash-object -w -t tree --stdin) &&\n \t\tc=$(git commit-tree -m bogus $t) &&\n \t\tgit update-ref refs/heads/bogus $c &&\n-\t\tgit clone -b bogus . bogus\n+\t\tgit clone -b bogus . bogus 2>warning\n \t)\n '\n \n+test_expect_success !MINGW,!CYGWIN,CASE_INSENSITIVE_FS 'colliding file detection' '\n+\tgrep X icasefs/warning &&\n+\tgrep x icasefs/warning &&\n+\ttest_i18ngrep \"the following paths have collided\" icasefs/warning\n+'\n+\n partial_clone () {\n \t SERVER=\"$1\" &&\n \t URL=\"$2\" &&\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex cd0680f11e..4338fee3b7 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -359,6 +359,12 @@ static int check_updates(struct unpack_trees_options *o)\n \tstate.refresh_cache = 1;\n \tstate.istate = index;\n \n+\tif (o->clone) {\n+\t\tstate.clone = 1;\n+\t\tfor (i = 0; i < index->cache_nr; i++)\n+\t\t\tindex->cache[i]->ce_flags &= ~CE_MATCHED;\n+\t}\n+\n \tprogress = get_progress(o);\n \n \tif (o->update)\n@@ -423,6 +429,35 @@ static int check_updates(struct unpack_trees_options *o)\n \terrs |= finish_delayed_checkout(&state);\n \tif (o->update)\n \t\tgit_attr_set_direction(GIT_ATTR_CHECKIN, NULL);\n+\n+\tif (o->clone) {\n+\t\tstruct string_list list = STRING_LIST_INIT_NODUP;\n+\t\tint i;\n+\n+\t\tfor (i = 0; i < index->cache_nr; i++) {\n+\t\t\tstruct cache_entry *ce = index->cache[i];\n+\n+\t\t\tif (!(ce->ce_flags & CE_MATCHED))\n+\t\t\t\tcontinue;\n+\n+\t\t\tstring_list_append(&list, ce->name);\n+\t\t\tce->ce_flags &= ~CE_MATCHED;\n+\t\t}\n+\n+\t\tlist.cmp = fspathcmp;\n+\t\tstring_list_sort(&list);\n+\n+\t\tif (list.nr)\n+\t\t\twarning(_(\"the following paths have collided (e.g. case-sensitive paths\\n\"\n+\t\t\t\t \"on a case-insensitive filesystem) and only one from the same\\n\"\n+\t\t\t\t \"colliding group is in the working tree:\\n\"));\n+\n+\t\tfor (i = 0; i < list.nr; i++)\n+\t\t\tfprintf(stderr, \" '%s'\\n\", list.items[i].string);\n+\n+\t\tstring_list_clear(&list, 0);\n+\t}\n+\n \treturn errs != 0;\n }\n \ndiff --git a/unpack-trees.h b/unpack-trees.h\nindex c2b434c606..d940f1c5c2 100644\n--- a/unpack-trees.h\n+++ b/unpack-trees.h\n@@ -42,6 +42,7 @@ struct unpack_trees_options {\n \tunsigned int reset,\n \t\t merge,\n \t\t update,\n+\t\t clone,\n \t\t index_only,\n \t\t nontrivial_merge,\n \t\t trivial_merges_only,\n-- \n2.18.0.1004.g6639190530\n\n"},{"id":"355923","messageId":"xmqqh8jsc3kr.fsf@gitster-ct.c.googlers.com","threadId":"48963","inReplyTo":"20180817161645.28249-1-pclouds@gmail.com","subject":"Re: [PATCH v5] clone: report duplicate entries on case-insensitive filesystems","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-17T17:20:36Z","receivedAt":"2018-08-17T17:20:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> I still don't trust magic st_ino zero, or core.checkStat being zero\n> on Windows, so the #if condition still remains but it covers smallest\n> area possible and I tested it by manually make it \"#if 1\"\n>\n> The fallback with fspathcmp() is only done when inode can't be\n> trusted because strcmp is more expensive and when fspathcmp() learns\n> more about real world in the future, it could become even more\n> expensive.\n>\n> The output sorting is the result of Sublime-Gitignore repo being\n> reported recently. It's not perfect but it should help seeing the\n> groups in normal case.\n\nLooks small and safe.\n\n> +\n> +\tif (o->clone) {\n> +\t\tstruct string_list list = STRING_LIST_INIT_NODUP;\n> +\t\tint i;\n> +\n> +\t\tfor (i = 0; i < index->cache_nr; i++) {\n> +\t\t\tstruct cache_entry *ce = index->cache[i];\n> +\n> +\t\t\tif (!(ce->ce_flags & CE_MATCHED))\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tstring_list_append(&list, ce->name);\n> +\t\t\tce->ce_flags &= ~CE_MATCHED;\n> +\t\t}\n> +\n> +\t\tlist.cmp = fspathcmp;\n> +\t\tstring_list_sort(&list);\n> +\n> +\t\tif (list.nr)\n> +\t\t\twarning(_(\"the following paths have collided (e.g. case-sensitive paths\\n\"\n> +\t\t\t\t \"on a case-insensitive filesystem) and only one from the same\\n\"\n> +\t\t\t\t \"colliding group is in the working tree:\\n\"));\n> +\n> +\t\tfor (i = 0; i < list.nr; i++)\n> +\t\t\tfprintf(stderr, \" '%s'\\n\", list.items[i].string);\n> +\n> +\t\tstring_list_clear(&list, 0);\n\nI would have written the \"sort, show warning, and list\" all inside\n\"if (list.nr)\" block, leaving list-clear outside, which would have\nmade the logic a bit cleaner. The reader does not have to bother\nthinking \"ah, when list.nr==0, this is a no-op anyway\" to skip them\nif written that way.\n\nI highly suspect that the above was written in that way to reduce\nthe indentation level, but the right way to reduce the indentation\nlevel, if it bothers readers too much, is to make the whole thing\ninside the above if (o->clone) into a dedicated helper function\n\"void report_collided_checkout(void)\", I would think.\n"},{"id":"355928","messageId":"20180817180039.GA31789@duynguyen.home","threadId":"48963","inReplyTo":"xmqqh8jsc3kr.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v5] clone: report duplicate entries on case-insensitive filesystems","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-08-17T18:00:39Z","receivedAt":"2018-08-17T18:00:46Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Aug 17, 2018 at 10:20:36AM -0700, Junio C Hamano wrote:\n> I highly suspect that the above was written in that way to reduce\n> the indentation level, but the right way to reduce the indentation\n> level, if it bothers readers too much, is to make the whole thing\n> inside the above if (o->clone) into a dedicated helper function\n> \"void report_collided_checkout(void)\", I would think.\n\nI read my mind. I thought of separating into a helper function too,\nbut was not happy that the clearing CE_MATCHED in preparation for this\ntest is in check_updates(), but the cleaning up CE_MATCHED() is in the\nhelper function.\n\nSo here is the version that separates _both_ phases into helper\nfunctions.\n\n-- 8< --\nSubject: [PATCH v6] clone: report duplicate entries on case-insensitive filesystems\n\nPaths that only differ in case work fine in a case-sensitive\nfilesystems, but if those repos are cloned in a case-insensitive one,\nyou'll get problems. The first thing to notice is \"git status\" will\nnever be clean with no indication what exactly is \"dirty\".\n\nThis patch helps the situation a bit by pointing out the problem at\nclone time. Even though this patch talks about case sensitivity, the\npatch makes no assumption about folding rules by the filesystem. It\nsimply observes that if an entry has been already checked out at clone\ntime when we're about to write a new path, some folding rules are\nbehind this.\n\nIn the case that we can't rely on filesystem (via inode number) to do\nthis check, fall back to fspathcmp() which is not perfect but should\nnot give false positives.\n\nThis patch is tested with vim-colorschemes and Sublime-Gitignore\nrepositories on a JFS partition with case insensitive support on\nLinux.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/clone.c | 1 +\n cache.h | 1 +\n entry.c | 31 +++++++++++++++++++++++++++++++\n t/t5601-clone.sh | 8 +++++++-\n unpack-trees.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++\n unpack-trees.h | 1 +\n 6 files changed, 88 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 5c439f1394..0702b0e9d0 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -747,6 +747,7 @@ static int checkout(int submodule_progress)\n \tmemset(&opts, 0, sizeof opts);\n \topts.update = 1;\n \topts.merge = 1;\n+\topts.clone = 1;\n \topts.fn = oneway_merge;\n \topts.verbose_update = (option_verbosity >= 0);\n \topts.src_index = &the_index;\ndiff --git a/cache.h b/cache.h\nindex 8b447652a7..6d6138f4f1 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1455,6 +1455,7 @@ struct checkout {\n \tunsigned force:1,\n \t\t quiet:1,\n \t\t not_new:1,\n+\t\t clone:1,\n \t\t refresh_cache:1;\n };\n #define CHECKOUT_INIT { NULL, \"\" }\ndiff --git a/entry.c b/entry.c\nindex b5d1d3cf23..8766e27255 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -399,6 +399,34 @@ static int check_path(const char *path, int len, struct stat *st, int skiplen)\n \treturn lstat(path, st);\n }\n \n+static void mark_colliding_entries(const struct checkout *state,\n+\t\t\t\t struct cache_entry *ce, struct stat *st)\n+{\n+\tint i, trust_ino = check_stat;\n+\n+#if defined(GIT_WINDOWS_NATIVE)\n+\ttrust_ino = 0;\n+#endif\n+\n+\tce->ce_flags |= CE_MATCHED;\n+\n+\tfor (i = 0; i < state->istate->cache_nr; i++) {\n+\t\tstruct cache_entry *dup = state->istate->cache[i];\n+\n+\t\tif (dup == ce)\n+\t\t\tbreak;\n+\n+\t\tif (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE))\n+\t\t\tcontinue;\n+\n+\t\tif ((trust_ino && dup->ce_stat_data.sd_ino == st->st_ino) ||\n+\t\t (!trust_ino && !fspathcmp(ce->name, dup->name))) {\n+\t\t\tdup->ce_flags |= CE_MATCHED;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+}\n+\n /*\n * Write the contents from ce out to the working tree.\n *\n@@ -455,6 +483,9 @@ int checkout_entry(struct cache_entry *ce,\n \t\t\treturn -1;\n \t\t}\n \n+\t\tif (state->clone)\n+\t\t\tmark_colliding_entries(state, ce, &st);\n+\n \t\t/*\n \t\t * We unlink the old file, to get the new one with the\n \t\t * right permissions (including umask, which is nasty\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 0b62037744..f2eb73bc74 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -624,10 +624,16 @@ test_expect_success 'clone on case-insensitive fs' '\n \t\t\tgit hash-object -w -t tree --stdin) &&\n \t\tc=$(git commit-tree -m bogus $t) &&\n \t\tgit update-ref refs/heads/bogus $c &&\n-\t\tgit clone -b bogus . bogus\n+\t\tgit clone -b bogus . bogus 2>warning\n \t)\n '\n \n+test_expect_success !MINGW,!CYGWIN,CASE_INSENSITIVE_FS 'colliding file detection' '\n+\tgrep X icasefs/warning &&\n+\tgrep x icasefs/warning &&\n+\ttest_i18ngrep \"the following paths have collided\" icasefs/warning\n+'\n+\n partial_clone () {\n \t SERVER=\"$1\" &&\n \t URL=\"$2\" &&\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex cd0680f11e..213da8bbb4 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -345,6 +345,46 @@ static struct progress *get_progress(struct unpack_trees_options *o)\n \treturn start_delayed_progress(_(\"Checking out files\"), total);\n }\n \n+static void setup_collided_checkout_detection(struct checkout *state,\n+\t\t\t\t\t struct index_state *index)\n+{\n+\tint i;\n+\n+\tstate->clone = 1;\n+\tfor (i = 0; i < index->cache_nr; i++)\n+\t\tindex->cache[i]->ce_flags &= ~CE_MATCHED;\n+}\n+\n+static void report_collided_checkout(struct index_state *index)\n+{\n+\tstruct string_list list = STRING_LIST_INIT_NODUP;\n+\tint i;\n+\n+\tfor (i = 0; i < index->cache_nr; i++) {\n+\t\tstruct cache_entry *ce = index->cache[i];\n+\n+\t\tif (!(ce->ce_flags & CE_MATCHED))\n+\t\t\tcontinue;\n+\n+\t\tstring_list_append(&list, ce->name);\n+\t\tce->ce_flags &= ~CE_MATCHED;\n+\t}\n+\n+\tlist.cmp = fspathcmp;\n+\tstring_list_sort(&list);\n+\n+\tif (list.nr) {\n+\t\twarning(_(\"the following paths have collided (e.g. case-sensitive paths\\n\"\n+\t\t\t \"on a case-insensitive filesystem) and only one from the same\\n\"\n+\t\t\t \"colliding group is in the working tree:\\n\"));\n+\n+\t\tfor (i = 0; i < list.nr; i++)\n+\t\t\tfprintf(stderr, \" '%s'\\n\", list.items[i].string);\n+\t}\n+\n+\tstring_list_clear(&list, 0);\n+}\n+\n static int check_updates(struct unpack_trees_options *o)\n {\n \tunsigned cnt = 0;\n@@ -359,6 +399,9 @@ static int check_updates(struct unpack_trees_options *o)\n \tstate.refresh_cache = 1;\n \tstate.istate = index;\n \n+\tif (o->clone)\n+\t\tsetup_collided_checkout_detection(&state, index);\n+\n \tprogress = get_progress(o);\n \n \tif (o->update)\n@@ -423,6 +466,10 @@ static int check_updates(struct unpack_trees_options *o)\n \terrs |= finish_delayed_checkout(&state);\n \tif (o->update)\n \t\tgit_attr_set_direction(GIT_ATTR_CHECKIN, NULL);\n+\n+\tif (o->clone)\n+\t\treport_collided_checkout(index);\n+\n \treturn errs != 0;\n }\n \ndiff --git a/unpack-trees.h b/unpack-trees.h\nindex c2b434c606..d940f1c5c2 100644\n--- a/unpack-trees.h\n+++ b/unpack-trees.h\n@@ -42,6 +42,7 @@ struct unpack_trees_options {\n \tunsigned int reset,\n \t\t merge,\n \t\t update,\n+\t\t clone,\n \t\t index_only,\n \t\t nontrivial_merge,\n \t\t trivial_merges_only,\n-- \n2.18.0.1004.g6639190530\n\n-- 8< --\n--\nDuy\n"},{"id":"355935","messageId":"20180817194600.GA10393@tor.lan","threadId":"48963","inReplyTo":"20180817161645.28249-1-pclouds@gmail.com","subject":"Re: [PATCH v5] clone: report duplicate entries on case-insensitive filesystems","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2018-08-17T19:46:00Z","receivedAt":"2018-08-17T19:51:50Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Fri, Aug 17, 2018 at 06:16:45PM +0200, Nguyễn Thái Ngọc Duy wrote:\n\nThe whole patch looks good to me.\n(I was just sending a different version, but your version is better :-)\n\nOne minor remark, should the line\nwarning: the following paths have collided \nstart with a capital letter:\nWarning: the following paths have collided \n\n> Paths that only differ in case work fine in a case-sensitive\n> filesystems, but if those repos are cloned in a case-insensitive one,\n> you'll get problems. The first thing to notice is \"git status\" will\n> never be clean with no indication what exactly is \"dirty\".\n> \n> This patch helps the situation a bit by pointing out the problem at\n> clone time. Even though this patch talks about case sensitivity, the\n> patch makes no assumption about folding rules by the filesystem. It\n> simply observes that if an entry has been already checked out at clone\n> time when we're about to write a new path, some folding rules are\n> behind this.\n> \n> In the case that we can't rely on filesystem (via inode number) to do\n> this check, fall back to fspathcmp() which is not perfect but should\n> not give false positives.\n> \n> This patch is tested with vim-colorschemes and Sublime-Gitignore\n> repositories on a JFS partition with case insensitive support on\n> Linux.\n\nNow even tested under Mac OS/HFS+\n\n[]\n> '\n> \n> +test_expect_success !MINGW,!CYGWIN,CASE_INSENSITIVE_FS 'colliding file detection' '\n\nMy ambition is to run the test under Windows (both CYGWIN and native) next week,\nso that we can remove !MINGW and !CYGWIN \n\n"},{"id":"363620","messageId":"20181119082015.77553-1-carenas@gmail.com","threadId":"48963","inReplyTo":"20180817161645.28249-1-pclouds@gmail.com","subject":"[PATCH v5] clone: report duplicate entries on case-insensitive filesystems","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2018-11-19T08:20:15Z","receivedAt":"2018-11-19T08:20:21Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"While I don't have an HFS+ volume to test, I suspect this patch should be\nneeded for both, even if I have to say thay even the broken output was\nbetter than the current state.\n\nTravis seems to be using a case sensitive filesystem so wouldn't catch this.\n\nWas windows/cygwin tested?\n\nCarlo\n-- >8 --\nSubject: [PATCH] entry: fix t5061 on macOS\n\nb878579ae7 (\"clone: report duplicate entries on case-insensitive filesystems\",\n2018-08-17) was tested on Linux with an excemption for Windows that needs\nto be expanded for macOS (using APFS), which then would show :\n\n$ git clone git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git\nwarning: the following paths have collided (e.g. case-sensitive paths\non a case-insensitive filesystem) and only one from the same\ncolliding group is in the working tree:\n\n 'man2/_Exit.2'\n 'man2/_exit.2'\n 'man3/NAN.3'\n 'man3/nan.3'\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n entry.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/entry.c b/entry.c\nindex 5d136c5d55..3845f570f7 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -404,7 +404,7 @@ static void mark_colliding_entries(const struct checkout *state,\n {\n \tint i, trust_ino = check_stat;\n \n-#if defined(GIT_WINDOWS_NATIVE)\n+#if defined(GIT_WINDOWS_NATIVE) || defined(__APPLE__)\n \ttrust_ino = 0;\n #endif\n \n-- \n2.20.0.rc0\n\n"},{"id":"363631","messageId":"37b7a395-3846-6664-9c4d-66d2e4277618@web.de","threadId":"48963","inReplyTo":"20181119082015.77553-1-carenas@gmail.com","subject":"Re: [PATCH v5] clone: report duplicate entries on case-insensitive filesystems","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2018-11-19T12:28:43Z","receivedAt":"2018-11-19T12:29:04Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2018-11-19 09:20, Carlo Marcelo Arenas Belón wrote:\n> While I don't have an HFS+ volume to test, I suspect this patch should be\n> needed for both, even if I have to say thay even the broken output was\n> better than the current state.\n> \n> Travis seems to be using a case sensitive filesystem so wouldn't catch this.\n> \n> Was windows/cygwin tested?\n> \n> Carlo\n> -- >8 --\n> Subject: [PATCH] entry: fix t5061 on macOS\n> \n> b878579ae7 (\"clone: report duplicate entries on case-insensitive filesystems\",\n> 2018-08-17) was tested on Linux with an excemption for Windows that needs\n> to be expanded for macOS (using APFS), which then would show :\n> \n> $ git clone git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git\n> warning: the following paths have collided (e.g. case-sensitive paths\n> on a case-insensitive filesystem) and only one from the same\n> colliding group is in the working tree:\n> \n> 'man2/_Exit.2'\n> 'man2/_exit.2'\n> 'man3/NAN.3'\n> 'man3/nan.3'\n> \n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> entry.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/entry.c b/entry.c\n> index 5d136c5d55..3845f570f7 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -404,7 +404,7 @@ static void mark_colliding_entries(const struct checkout *state,\n> {\n> \tint i, trust_ino = check_stat;\n> \n> -#if defined(GIT_WINDOWS_NATIVE)\n> +#if defined(GIT_WINDOWS_NATIVE) || defined(__APPLE__)\n> \ttrust_ino = 0;\n> #endif\n> \n> \n\nSorry,\nbut I can't reproduce your problem here.\n\nDid you test it on Mac ?\nIf I run t5601 on a case sensitive files system\n(Mac, mounted NFS, exported from Linux)\nI get:\nok 99 # skip colliding file detection (missing CASE_INSENSITIVE_FS of\n!MINGW,!CYGWIN,CASE_INSENSITIVE_FS)\n\nAnd if I run it on a case-insensitive HFS+,\nI get\nok 99 - colliding file detection\n\nSo what exactly are you trying to fix ?\n\n"},{"id":"363647","messageId":"CAPUEsphrYOV64m08JY_tsVuJ-uwTv=o=m5LdCFOWd+8tWJP54A@mail.gmail.com","threadId":"48963","inReplyTo":"37b7a395-3846-6664-9c4d-66d2e4277618@web.de","subject":"Re: [PATCH v5] clone: report duplicate entries on case-insensitive filesystems","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2018-11-19T17:14:40Z","receivedAt":"2018-11-19T17:14:57Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, Nov 19, 2018 at 4:28 AM Torsten Bögershausen wrote:\n>\n> Did you test it on Mac ?\n\nmacOS 10.14.1 but only using APFS, did you test my patch with HFS+?\n\n> So what exactly are you trying to fix ?\n\nI get\n\nnot ok 99 - colliding file detection\n#\n# grep X icasefs/warning &&\n# grep x icasefs/warning &&\n# test_i18ngrep \"the following paths have collided\" icasefs/warning\n#\n\nand the output of \"warning\" only shows one of the conflicting files,\ninstead of both:\n\nCloning into 'bogus'...\ndone.\nwarning: the following paths have collided (e.g. case-sensitive paths\non a case-insensitive filesystem) and only one from the same\ncolliding group is in the working tree:\n\n 'x'\n\nCarlo\n"},{"id":"363648","messageId":"a4d29a9a-5ac0-2952-42bc-5f822d44d055@ramsayjones.plus.com","threadId":"48963","inReplyTo":"37b7a395-3846-6664-9c4d-66d2e4277618@web.de","subject":"Re: [PATCH v5] clone: report duplicate entries on case-insensitive filesystems","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-11-19T17:21:38Z","receivedAt":"2018-11-19T17:21:44Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 19/11/2018 12:28, Torsten Bögershausen wrote:\n> On 2018-11-19 09:20, Carlo Marcelo Arenas Belón wrote:\n>> While I don't have an HFS+ volume to test, I suspect this patch should be\n>> needed for both, even if I have to say thay even the broken output was\n>> better than the current state.\n>>\n>> Travis seems to be using a case sensitive filesystem so wouldn't catch this.\n>>\n>> Was windows/cygwin tested?\n>>\n>> Carlo\n>> -- >8 --\n>> Subject: [PATCH] entry: fix t5061 on macOS\n>>\n>> b878579ae7 (\"clone: report duplicate entries on case-insensitive filesystems\",\n>> 2018-08-17) was tested on Linux with an excemption for Windows that needs\n>> to be expanded for macOS (using APFS), which then would show :\n>>\n>> $ git clone git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git\n>> warning: the following paths have collided (e.g. case-sensitive paths\n>> on a case-insensitive filesystem) and only one from the same\n>> colliding group is in the working tree:\n>>\n>> 'man2/_Exit.2'\n>> 'man2/_exit.2'\n>> 'man3/NAN.3'\n>> 'man3/nan.3'\n>>\n>> Signed-off-by: Carlo Marcelo Arenas Belón \n>> ---\n>> entry.c | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/entry.c b/entry.c\n>> index 5d136c5d55..3845f570f7 100644\n>> --- a/entry.c\n>> +++ b/entry.c\n>> @@ -404,7 +404,7 @@ static void mark_colliding_entries(const struct checkout *state,\n>> {\n>> \tint i, trust_ino = check_stat;\n>> \n>> -#if defined(GIT_WINDOWS_NATIVE)\n>> +#if defined(GIT_WINDOWS_NATIVE) || defined(__APPLE__)\n>> \ttrust_ino = 0;\n>> #endif\n>> \n>>\n> \n> Sorry,\n> but I can't reproduce your problem here.\n> \n> Did you test it on Mac ?\n> If I run t5601 on a case sensitive files system\n> (Mac, mounted NFS, exported from Linux)\n> I get:\n> ok 99 # skip colliding file detection (missing CASE_INSENSITIVE_FS of\n> !MINGW,!CYGWIN,CASE_INSENSITIVE_FS)\n\nI tested v2.20.0-rc0 on cygwin last night and it passed just fine.\nI just ran t5601-clone.sh on its own and got:\n\n $ ./t5601-clone.sh\n ...\n ok 98 - clone on case-insensitive fs\n ok 99 # skip colliding file detection (missing !CYGWIN of !MINGW,!CYGWIN,CASE_INSENSITIVE_FS)\n ok 100 - partial clone\n ok 101 - partial clone: warn if server does not support object filtering\n ok 102 - batch missing blob request during checkout\n ok 103 - batch missing blob request does not inadvertently try to fetch gitlinks\n # passed all 103 test(s)\n # SKIP no web server found at '/usr/sbin/apache2'\n 1..103\n $ \n\nATB,\nRamsay Jones\n"},{"id":"363651","messageId":"CACsJy8A_c-O5DrZnMvEbsSa+YzatiLH3TLAy3OV1+AwY5rrCjQ@mail.gmail.com","threadId":"48963","inReplyTo":"CAPUEsphrYOV64m08JY_tsVuJ-uwTv=o=m5LdCFOWd+8tWJP54A@mail.gmail.com","subject":"Re: [PATCH v5] clone: report duplicate entries on case-insensitive filesystems","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-19T18:24:26Z","receivedAt":"2018-11-19T18:24:56Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Nov 19, 2018 at 6:14 PM Carlo Arenas wrote:\n>\n> On Mon, Nov 19, 2018 at 4:28 AM Torsten Bögershausen wrote:\n> >\n> > Did you test it on Mac ?\n>\n> macOS 10.14.1 but only using APFS, did you test my patch with HFS+?\n>\n> > So what exactly are you trying to fix ?\n>\n> I get\n>\n> not ok 99 - colliding file detection\n> #\n> # grep X icasefs/warning &&\n> # grep x icasefs/warning &&\n> # test_i18ngrep \"the following paths have collided\" icasefs/warning\n> #\n>\n> and the output of \"warning\" only shows one of the conflicting files,\n> instead of both:\n>\n> Cloning into 'bogus'...\n> done.\n> warning: the following paths have collided (e.g. case-sensitive paths\n> on a case-insensitive filesystem) and only one from the same\n> colliding group is in the working tree:\n>\n> 'x'\n>\n> Carlo\n\nCould you send me the \"index\" file in t/trash\\\ndirectory.t5601-clone/icasefs/bogus/.git/index ? Also the output of\n\"stat /path/to/icase/bogus/x\"\n\nMy only explanation is somehow the inode value we save is not the same\none on disk, which is weird and could even cause other problems. I'd\nlike to know why this happens before trying to fix anything.\n-- \nDuy\n"},{"id":"363662","messageId":"CAPUEsphTsPnhMtQxv499-Qyz2_3OUqgXNTw1p3AehuoUv6tKLQ@mail.gmail.com","threadId":"48963","inReplyTo":"a4d29a9a-5ac0-2952-42bc-5f822d44d055@ramsayjones.plus.com","subject":"Re: [PATCH v5] clone: report duplicate entries on case-insensitive filesystems","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2018-11-19T19:39:57Z","receivedAt":"2018-11-19T19:40:14Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, Nov 19, 2018 at 9:23 AM Ramsay Jones\n wrote:\n> ok 99 # skip colliding file detection (missing !CYGWIN of !MINGW,!CYGWIN,CASE_INSENSITIVE_FS)\n\nyou need to enable this specific test first (removing !CYGWIN) so it\ndoesn't get skipped\n\nCarlo\n"},{"id":"363672","messageId":"20181119210323.GA31963@duynguyen.home","threadId":"48963","inReplyTo":"CACsJy8A_c-O5DrZnMvEbsSa+YzatiLH3TLAy3OV1+AwY5rrCjQ@mail.gmail.com","subject":"Re: [PATCH v5] clone: report duplicate entries on case-insensitive filesystems","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-19T21:03:24Z","receivedAt":"2018-11-19T21:03:31Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"First of all, Ramsay, it would be great if you could test the below\npatch and see if it works on Cygwin. I assume since Cygwin shares the\nunderlying filesystem, it will share the same \"no trusting inode\"\nissue with native builds (or it calculates inodes anyway using some\nother source?).\n\nBack to the APFS problem...\n\nOn Mon, Nov 19, 2018 at 07:24:26PM +0100, Duy Nguyen wrote:\n> Could you send me the \"index\" file in t/trash\\\n> directory.t5601-clone/icasefs/bogus/.git/index ? Also the output of\n> \"stat /path/to/icase/bogus/x\"\n> \n> My only explanation is somehow the inode value we save is not the same\n> one on disk, which is weird and could even cause other problems. I'd\n> like to know why this happens before trying to fix anything.\n\nThanks Carlo for the file and \"stat\" output. The problem is APFS has\n64-bit inode (according to the Internet) while we store inodes as\n32-bit, so it's truncated. Which means this comparison\n\n sd_ino == st_ino\n\nis never true because sd_ino is truncated (0x2121063) while st_ino is\nnot (0x202121063).\n\nCarlo, it would be great if you could test this patch also with\nAPFS. It should fix problem. We will have to deal with the same\ntruncated inode elsewhere to make sure we index refresh performance\ndoes not degrade on APFS. But that's a separate problem. Thank you for\nbringing this up.\n\n-- 8< --\ndiff --git a/entry.c b/entry.c\nindex 5d136c5d55..809d3e2ba7 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -404,13 +404,13 @@ static void mark_colliding_entries(const struct checkout *state,\n {\n \tint i, trust_ino = check_stat;\n \n-#if defined(GIT_WINDOWS_NATIVE)\n+#if defined(GIT_WINDOWS_NATIVE) || defined(__CYGWIN__)\n \ttrust_ino = 0;\n #endif\n \n \tce->ce_flags |= CE_MATCHED;\n \n-\tfor (i = 0; i < state->istate->cache_nr; i++) {\n+\tfor (i = 0; i < trust_ino && state->istate->cache_nr; i++) {\n \t\tstruct cache_entry *dup = state->istate->cache[i];\n \n \t\tif (dup == ce)\n@@ -419,10 +419,24 @@ static void mark_colliding_entries(const struct checkout *state,\n \t\tif (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE))\n \t\t\tcontinue;\n \n-\t\tif ((trust_ino && dup->ce_stat_data.sd_ino == st->st_ino) ||\n-\t\t (!trust_ino && !fspathcmp(ce->name, dup->name))) {\n+\t\tif (dup->ce_stat_data.sd_ino == (unsigned int)st->st_ino) {\n \t\t\tdup->ce_flags |= CE_MATCHED;\n+\t\t\treturn;\n+\t\t}\n+\t}\n+\n+\tfor (i = 0; i < state->istate->cache_nr; i++) {\n+\t\tstruct cache_entry *dup = state->istate->cache[i];\n+\n+\t\tif (dup == ce)\n \t\t\tbreak;\n+\n+\t\tif (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE))\n+\t\t\tcontinue;\n+\n+\t\tif (!fspathcmp(ce->name, dup->name)) {\n+\t\t\tdup->ce_flags |= CE_MATCHED;\n+\t\t\treturn;\n \t\t}\n \t}\n }\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex f1a49e94f5..c28d51bd59 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -628,7 +628,7 @@ test_expect_success 'clone on case-insensitive fs' '\n \t)\n '\n \n-test_expect_success !MINGW,!CYGWIN,CASE_INSENSITIVE_FS 'colliding file detection' '\n+test_expect_success !MINGW,CASE_INSENSITIVE_FS 'colliding file detection' '\n \tgrep X icasefs/warning &&\n \tgrep x icasefs/warning &&\n \ttest_i18ngrep \"the following paths have collided\" icasefs/warning\n-- 8< --\n"},{"id":"363673","messageId":"CACsJy8AYnkrYcgR0-WbP-+PnRS4nrx_On58MEbc82t2zD=6euA@mail.gmail.com","threadId":"48963","inReplyTo":"20181119210323.GA31963@duynguyen.home","subject":"Re: [PATCH v5] clone: report duplicate entries on case-insensitive filesystems","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-19T21:04:37Z","receivedAt":"2018-11-19T21:05:06Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"... and I \"dear Ramsay\" without CCing him.. sigh.. sorry for the noise.\n\nOn Mon, Nov 19, 2018 at 10:03 PM Duy Nguyen wrote:\n>\n> First of all, Ramsay, it would be great if you could test the below\n> patch and see if it works on Cygwin. I assume since Cygwin shares the\n> underlying filesystem, it will share the same \"no trusting inode\"\n> issue with native builds (or it calculates inodes anyway using some\n> other source?).\n>\n> Back to the APFS problem...\n>\n> On Mon, Nov 19, 2018 at 07:24:26PM +0100, Duy Nguyen wrote:\n> > Could you send me the \"index\" file in t/trash\\\n> > directory.t5601-clone/icasefs/bogus/.git/index ? Also the output of\n> > \"stat /path/to/icase/bogus/x\"\n> >\n> > My only explanation is somehow the inode value we save is not the same\n> > one on disk, which is weird and could even cause other problems. I'd\n> > like to know why this happens before trying to fix anything.\n>\n> Thanks Carlo for the file and \"stat\" output. The problem is APFS has\n> 64-bit inode (according to the Internet) while we store inodes as\n> 32-bit, so it's truncated. Which means this comparison\n>\n> sd_ino == st_ino\n>\n> is never true because sd_ino is truncated (0x2121063) while st_ino is\n> not (0x202121063).\n>\n> Carlo, it would be great if you could test this patch also with\n> APFS. It should fix problem. We will have to deal with the same\n> truncated inode elsewhere to make sure we index refresh performance\n> does not degrade on APFS. But that's a separate problem. Thank you for\n> bringing this up.\n>\n> -- 8< --\n> diff --git a/entry.c b/entry.c\n> index 5d136c5d55..809d3e2ba7 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -404,13 +404,13 @@ static void mark_colliding_entries(const struct checkout *state,\n> {\n> int i, trust_ino = check_stat;\n>\n> -#if defined(GIT_WINDOWS_NATIVE)\n> +#if defined(GIT_WINDOWS_NATIVE) || defined(__CYGWIN__)\n> trust_ino = 0;\n> #endif\n>\n> ce->ce_flags |= CE_MATCHED;\n>\n> - for (i = 0; i < state->istate->cache_nr; i++) {\n> + for (i = 0; i < trust_ino && state->istate->cache_nr; i++) {\n> struct cache_entry *dup = state->istate->cache[i];\n>\n> if (dup == ce)\n> @@ -419,10 +419,24 @@ static void mark_colliding_entries(const struct checkout *state,\n> if (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE))\n> continue;\n>\n> - if ((trust_ino && dup->ce_stat_data.sd_ino == st->st_ino) ||\n> - (!trust_ino && !fspathcmp(ce->name, dup->name))) {\n> + if (dup->ce_stat_data.sd_ino == (unsigned int)st->st_ino) {\n> dup->ce_flags |= CE_MATCHED;\n> + return;\n> + }\n> + }\n> +\n> + for (i = 0; i < state->istate->cache_nr; i++) {\n> + struct cache_entry *dup = state->istate->cache[i];\n> +\n> + if (dup == ce)\n> break;\n> +\n> + if (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE))\n> + continue;\n> +\n> + if (!fspathcmp(ce->name, dup->name)) {\n> + dup->ce_flags |= CE_MATCHED;\n> + return;\n> }\n> }\n> }\n> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\n> index f1a49e94f5..c28d51bd59 100755\n> --- a/t/t5601-clone.sh\n> +++ b/t/t5601-clone.sh\n> @@ -628,7 +628,7 @@ test_expect_success 'clone on case-insensitive fs' '\n> )\n> '\n>\n> -test_expect_success !MINGW,!CYGWIN,CASE_INSENSITIVE_FS 'colliding file detection' '\n> +test_expect_success !MINGW,CASE_INSENSITIVE_FS 'colliding file detection' '\n> grep X icasefs/warning &&\n> grep x icasefs/warning &&\n> test_i18ngrep \"the following paths have collided\" icasefs/warning\n> -- 8< --\n\n\n\n-- \nDuy\n"},{"id":"363675","messageId":"CACsJy8Ac_o6M5DZDz6hwn-JGJLGdzK4wtvhAY4bwaSwPoix6Cg@mail.gmail.com","threadId":"48963","inReplyTo":"20181119210323.GA31963@duynguyen.home","subject":"Re: [PATCH v5] clone: report duplicate entries on case-insensitive filesystems","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-19T21:17:24Z","receivedAt":"2018-11-19T21:18:06Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Nov 19, 2018 at 10:03 PM Duy Nguyen wrote:\n> Thanks Carlo for the file and \"stat\" output. The problem is APFS has\n> 64-bit inode (according to the Internet) while we store inodes as\n> 32-bit, so it's truncated.\n> ...\n> We will have to deal with the same\n> truncated inode elsewhere to make sure we index refresh performance\n> does not degrade on APFS.\n\n... and we don't have a problem there. Either Linus predicted dealing\nwith 64-bit inodes, or he had a habit of casting st_ino to unsigned\nint, I cannot tell. This code\n\n ce->st_ino != (unsigned int)st->st_ino\n\nis from e83c516331 (Initial revision of \"git\", the information manager\nfrom hell - 2005-04-07) and it's still used today for comparing sd_ino\nwith st->st_ino in read-cache.c. I guess I should have copied and\npasted more often.\n-- \nDuy\n"},{"id":"363682","messageId":"5ffa3a01-8b76-0b84-a21c-efe912e80333@ramsayjones.plus.com","threadId":"48963","inReplyTo":"20181119210323.GA31963@duynguyen.home","subject":"Re: [PATCH v5] clone: report duplicate entries on case-insensitive filesystems","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-11-19T23:29:05Z","receivedAt":"2018-11-19T23:29:12Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 19/11/2018 21:03, Duy Nguyen wrote:\n> First of all, Ramsay, it would be great if you could test the below\n> patch and see if it works on Cygwin. I assume since Cygwin shares the\n> underlying filesystem, it will share the same \"no trusting inode\"\n> issue with native builds (or it calculates inodes anyway using some\n> other source?).\n\nHmm, I have no idea why you would like me to try this patch - care\nto explain? [I just saw, \"Has this been tested on cygwin?\" and, since\nit has been happily passing for some time, responded yes!]\n\nJust for the giggles, I removed the !CYGWIN prerequisite from the\ntest and when, as expected, the test failed, had a look around:\n\n$ pwd\n/home/ramsay/git/t/trash directory.t5601-clone\n$ cat icasefs/warning \nCloning into 'bogus'...\ndone.\nwarning: the following paths have collided (e.g. case-sensitive paths\non a case-insensitive filesystem) and only one from the same\ncolliding group is in the working tree:\n\n 'x'\n$ cd icasefs/bogus\n$ ls -l\ntotal 0\n-rw-r--r-- 1 ramsay None 0 Nov 19 22:40 x\n$ git ls-files --debug\nignoring EOIE extension\nX\n ctime: 1542667201:664036600\n mtime: 1542667201:663055400\n dev: 2378432\tino: 324352\n uid: 1001\tgid: 513\n size: 0\tflags: 0\nx\n ctime: 1542667201:665026800\n mtime: 1542667201:665026800\n dev: 2378432\tino: 324352\n uid: 1001\tgid: 513\n size: 0\tflags: 0\n$ \n\nSo, both X and x are in the index with the same inode number.\n\nDoes that help?\n\nATB,\nRamsay Jones\n"},{"id":"363684","messageId":"2a6cd6a7-1b6b-2669-c83a-be5483c52fa2@ramsayjones.plus.com","threadId":"48963","inReplyTo":"5ffa3a01-8b76-0b84-a21c-efe912e80333@ramsayjones.plus.com","subject":"Re: [PATCH v5] clone: report duplicate entries on case-insensitive filesystems","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-11-19T23:54:10Z","receivedAt":"2018-11-19T23:54:15Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 19/11/2018 23:29, Ramsay Jones wrote:\n> \n> \n> On 19/11/2018 21:03, Duy Nguyen wrote:\n>> First of all, Ramsay, it would be great if you could test the below\n>> patch and see if it works on Cygwin. I assume since Cygwin shares the\n>> underlying filesystem, it will share the same \"no trusting inode\"\n>> issue with native builds (or it calculates inodes anyway using some\n>> other source?).\n> \n> Hmm, I have no idea why you would like me to try this patch - care\n> to explain? [I just saw, \"Has this been tested on cygwin?\" and, since\n> it has been happily passing for some time, responded yes!]\n> \n> Just for the giggles, I removed the !CYGWIN prerequisite from the\n> test and when, as expected, the test failed, had a look around:\n> \n> $ pwd\n> /home/ramsay/git/t/trash directory.t5601-clone\n> $ cat icasefs/warning \n> Cloning into 'bogus'...\n> done.\n> warning: the following paths have collided (e.g. case-sensitive paths\n> on a case-insensitive filesystem) and only one from the same\n> colliding group is in the working tree:\n> \n> 'x'\n> $ cd icasefs/bogus\n> $ ls -l\n> total 0\n> -rw-r--r-- 1 ramsay None 0 Nov 19 22:40 x\n> $ git ls-files --debug\n> ignoring EOIE extension\n> X\n> ctime: 1542667201:664036600\n> mtime: 1542667201:663055400\n> dev: 2378432\tino: 324352\n> uid: 1001\tgid: 513\n> size: 0\tflags: 0\n> x\n> ctime: 1542667201:665026800\n> mtime: 1542667201:665026800\n> dev: 2378432\tino: 324352\n> uid: 1001\tgid: 513\n> size: 0\tflags: 0\n> $ \n> \n> So, both X and x are in the index with the same inode number.\n> \n> Does that help?\n\nWell, I haven't even looked at the patch, but when I apply it to\nthe current 'pu' branch (just what I happened to have checked out)\nand run that one test:\n\n$ ./t5601-clone.sh\n...\nok 96 - shallow clone locally\nok 97 - GIT_TRACE_PACKFILE produces a usable pack\nok 98 - clone on case-insensitive fs\nok 99 - colliding file detection\nok 100 - partial clone\nok 101 - partial clone: warn if server does not support object filtering\nok 102 - batch missing blob request during checkout\nok 103 - batch missing blob request does not inadvertently try to fetch gitlinks\n# passed all 103 test(s)\n# SKIP no web server found at '/usr/sbin/apache2'\n1..103\n$ \n\n... the colliding file detection test passes!\n\nATB,\nRamsay Jones\n\n\n"},{"id":"363687","messageId":"CAPUEspjRBkUhwAkc6B-=FHoryfMLyeKZTnkWUTMTSz2VTN2t8A@mail.gmail.com","threadId":"48963","inReplyTo":"2a6cd6a7-1b6b-2669-c83a-be5483c52fa2@ramsayjones.plus.com","subject":"Re: [PATCH v5] clone: report duplicate entries on case-insensitive filesystems","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2018-11-20T01:05:50Z","receivedAt":"2018-11-20T01:06:07Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"ok 99 - colliding file detection\n\nas well in macOS with APFS\n\nCarlo\n"},{"id":"363692","messageId":"xmqq36rwcwtu.fsf@gitster-ct.c.googlers.com","threadId":"48963","inReplyTo":"20181119210323.GA31963@duynguyen.home","subject":"Re: [PATCH v5] clone: report duplicate entries on case-insensitive filesystems","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-20T02:22:05Z","receivedAt":"2018-11-20T02:22:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> \n> -\tfor (i = 0; i < state->istate->cache_nr; i++) {\n> +\tfor (i = 0; i < trust_ino && state->istate->cache_nr; i++) {\n\nThere is some typo here, but modulo that this looks like the right\nthing to do.\n\n> @@ -419,10 +419,24 @@ static void mark_colliding_entries(const struct checkout *state,\n> \t\tif (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE))\n> \t\t\tcontinue;\n> \n> -\t\tif ((trust_ino && dup->ce_stat_data.sd_ino == st->st_ino) ||\n> -\t\t (!trust_ino && !fspathcmp(ce->name, dup->name))) {\n> +\t\tif (dup->ce_stat_data.sd_ino == (unsigned int)st->st_ino) {\n\nThis is slightly unfortunate but is the best we can do for now. \n\nThe reason why the design of the \"cached stat info\" mechanism allows\nthe sd_* fields to be narrower than the underlying fields is because\nthey are used only as an early-culling measure (if the value saved\nwith truncation is different from the current value with truncation,\nthen they cannot possibly be the same, so we know that the file\nchanged without looking at the contents).\n\nThis use however is different. Equality of truncated values\nimmediately declare CE_MATCHED here, producing false negative, which\nis not what we want, no?\n\n> \t\t\tdup->ce_flags |= CE_MATCHED;\n> +\t\t\treturn;\n> +\t\t}\n> +\t}\n\n"},{"id":"363754","messageId":"20181120162853.22441-1-pclouds@gmail.com","threadId":"48963","inReplyTo":"xmqq36rwcwtu.fsf@gitster-ct.c.googlers.com","subject":"[PATCH] clone: fix colliding file detection on APFS","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-20T16:28:53Z","receivedAt":"2018-11-20T16:29:01Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Commit b878579ae7 (clone: report duplicate entries on case-insensitive\nfilesystems - 2018-08-17) adds a warning to user when cloning a repo\nwith case-sensitive file names on a case-insensitive file system. The\n\"find duplicate file\" check was doing by comparing inode number (and\nonly fall back to fspathcmp() when inode is known to be unreliable\nbecause fspathcmp() can't cover all case folding cases).\n\nThe inode check is very simple, and wrong. It compares between a\n32-bit number (sd_ino) and potentially a 64-bit number (st_ino). When\nan inode is larger than 2^32 (which seems to be the case for APFS), it\nwill be truncated and stored in sd_ino, but comparing with itself will\nfail.\n\nAs a result, instead of showing a pair of files that have the same\nname, we show just one file (marked before the beginning of the\nloop). We fail to find the original one.\n\nThe fix could be just a simple type cast (*)\n\n dup->ce_stat_data.sd_ino == (unsigned int)st->st_ino\n\nbut this is no longer a reliable test, there are 4G possible inodes\nthat can match sd_ino because we only match the lower 32 bits instead\nof full 64 bits.\n\nThere are two options to go. Either we ignore inode and go with\nfspathcmp() on Apple platform. This means we can't do accurate inode\ncheck on HFS anymore, or even on APFS when inode numbers are still\nbelow 2^32.\n\nOr we just to to reduce the odds of matching a wrong file by checking\nmore attributes, counting mostly on st_size because st_xtime is likely\nthe same. This patch goes with this direction, hoping that false\npositive chances are too small to be seen in practice.\n\nWhile at there, enable the test on Cygwin (verified working by Ramsay\nJones)\n\n(*) this is also already done inside match_stat_data()\n\nReported-by: Carlo Arenas \nHelped-by: Ramsay Jones \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n So I'm going with match_stat_data(). But I don't know, perhaps just\n ignoring inode (like Carlo's original patch) is safer/better?\n\n Tested on case-insensitive JFS on Linux. But I don't think it really\n matters because I'm not even sure if I could push inode above 2^32\n with this. Hacking JFS for this test sounds fun, but no time for that.\n\n entry.c | 4 ++--\n t/t5601-clone.sh | 2 +-\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex 5d136c5d55..0a3c451f5f 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -404,7 +404,7 @@ static void mark_colliding_entries(const struct checkout *state,\n {\n \tint i, trust_ino = check_stat;\n \n-#if defined(GIT_WINDOWS_NATIVE)\n+#if defined(GIT_WINDOWS_NATIVE) || defined(__CYGWIN__)\n \ttrust_ino = 0;\n #endif\n \n@@ -419,7 +419,7 @@ static void mark_colliding_entries(const struct checkout *state,\n \t\tif (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE))\n \t\t\tcontinue;\n \n-\t\tif ((trust_ino && dup->ce_stat_data.sd_ino == st->st_ino) ||\n+\t\tif ((trust_ino && !match_stat_data(&dup->ce_stat_data, st)) ||\n \t\t (!trust_ino && !fspathcmp(ce->name, dup->name))) {\n \t\t\tdup->ce_flags |= CE_MATCHED;\n \t\t\tbreak;\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex f1a49e94f5..c28d51bd59 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -628,7 +628,7 @@ test_expect_success 'clone on case-insensitive fs' '\n \t)\n '\n \n-test_expect_success !MINGW,!CYGWIN,CASE_INSENSITIVE_FS 'colliding file detection' '\n+test_expect_success !MINGW,CASE_INSENSITIVE_FS 'colliding file detection' '\n \tgrep X icasefs/warning &&\n \tgrep x icasefs/warning &&\n \ttest_i18ngrep \"the following paths have collided\" icasefs/warning\n-- \n2.19.1.1327.g328c130451.dirty\n\n"},{"id":"363763","messageId":"1d92df80-02cd-2986-d2f1-f1fe084d8adc@ramsayjones.plus.com","threadId":"48963","inReplyTo":"20181120162853.22441-1-pclouds@gmail.com","subject":"Re: [PATCH] clone: fix colliding file detection on APFS","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-11-20T19:20:08Z","receivedAt":"2018-11-20T19:20:15Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 20/11/2018 16:28, Nguyễn Thái Ngọc Duy wrote:\n> Commit b878579ae7 (clone: report duplicate entries on case-insensitive\n> filesystems - 2018-08-17) adds a warning to user when cloning a repo\n> with case-sensitive file names on a case-insensitive file system. The\n> \"find duplicate file\" check was doing by comparing inode number (and\n> only fall back to fspathcmp() when inode is known to be unreliable\n> because fspathcmp() can't cover all case folding cases).\n> \n> The inode check is very simple, and wrong. It compares between a\n> 32-bit number (sd_ino) and potentially a 64-bit number (st_ino). When\n> an inode is larger than 2^32 (which seems to be the case for APFS), it\n> will be truncated and stored in sd_ino, but comparing with itself will\n> fail.\n> \n> As a result, instead of showing a pair of files that have the same\n> name, we show just one file (marked before the beginning of the\n> loop). We fail to find the original one.\n> \n> The fix could be just a simple type cast (*)\n> \n> dup->ce_stat_data.sd_ino == (unsigned int)st->st_ino\n> \n> but this is no longer a reliable test, there are 4G possible inodes\n> that can match sd_ino because we only match the lower 32 bits instead\n> of full 64 bits.\n> \n> There are two options to go. Either we ignore inode and go with\n> fspathcmp() on Apple platform. This means we can't do accurate inode\n> check on HFS anymore, or even on APFS when inode numbers are still\n> below 2^32.\n> \n> Or we just to to reduce the odds of matching a wrong file by checking\n> more attributes, counting mostly on st_size because st_xtime is likely\n> the same. This patch goes with this direction, hoping that false\n> positive chances are too small to be seen in practice.\n> \n> While at there, enable the test on Cygwin (verified working by Ramsay\n> Jones)\n\nWell, no, I tested the previous version of this patch. However, this\npatch also passes the test. (Note _test_ singular - in order to check\nthat this patch doesn't cause a regression I would need to run the\nwhole test-suite - that takes 3.5 hours, if I'm not doing anything\nelse!)\n\n> \n> (*) this is also already done inside match_stat_data()\n> \n> Reported-by: Carlo Arenas \n> Helped-by: Ramsay Jones \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> So I'm going with match_stat_data(). But I don't know, perhaps just\n> ignoring inode (like Carlo's original patch) is safer/better?\n> \n> Tested on case-insensitive JFS on Linux. But I don't think it really\n> matters because I'm not even sure if I could push inode above 2^32\n> with this. Hacking JFS for this test sounds fun, but no time for that.\n> \n> entry.c | 4 ++--\n> t/t5601-clone.sh | 2 +-\n> 2 files changed, 3 insertions(+), 3 deletions(-)\n> \n> diff --git a/entry.c b/entry.c\n> index 5d136c5d55..0a3c451f5f 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -404,7 +404,7 @@ static void mark_colliding_entries(const struct checkout *state,\n> {\n> \tint i, trust_ino = check_stat;\n> \n> -#if defined(GIT_WINDOWS_NATIVE)\n> +#if defined(GIT_WINDOWS_NATIVE) || defined(__CYGWIN__)\n\nI was a little curious about this (but couldn't be bothered actually\nread the code, post-application), so I removed this hunk from the\npatch, rebuilt and ran the test again: it _passed_ the test. :-D\n\nSo, ...\n\nATB,\nRamsay Jones\n\n> \ttrust_ino = 0;\n> #endif\n> \n> @@ -419,7 +419,7 @@ static void mark_colliding_entries(const struct checkout *state,\n> \t\tif (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE))\n> \t\t\tcontinue;\n> \n> -\t\tif ((trust_ino && dup->ce_stat_data.sd_ino == st->st_ino) ||\n> +\t\tif ((trust_ino && !match_stat_data(&dup->ce_stat_data, st)) ||\n> \t\t (!trust_ino && !fspathcmp(ce->name, dup->name))) {\n> \t\t\tdup->ce_flags |= CE_MATCHED;\n> \t\t\tbreak;\n> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\n> index f1a49e94f5..c28d51bd59 100755\n> --- a/t/t5601-clone.sh\n> +++ b/t/t5601-clone.sh\n> @@ -628,7 +628,7 @@ test_expect_success 'clone on case-insensitive fs' '\n> \t)\n> '\n> \n> -test_expect_success !MINGW,!CYGWIN,CASE_INSENSITIVE_FS 'colliding file detection' '\n> +test_expect_success !MINGW,CASE_INSENSITIVE_FS 'colliding file detection' '\n> \tgrep X icasefs/warning &&\n> \tgrep x icasefs/warning &&\n> \ttest_i18ngrep \"the following paths have collided\" icasefs/warning\n> \n"},{"id":"363765","messageId":"CAPUEsphujJmC8R8acXFDgexeA61JYS8Fcv7Tog+Jt+bZhHrCDQ@mail.gmail.com","threadId":"48963","inReplyTo":"20181120162853.22441-1-pclouds@gmail.com","subject":"Re: [PATCH] clone: fix colliding file detection on APFS","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2018-11-20T19:35:04Z","receivedAt":"2018-11-20T19:35:20Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Tested-by: Carlo Marcelo Arenas Belón \n\nin macOS 10.14.1 with APFS\nin Linux using VFAT (for the lulz)\n\nIMHO it would be ideal if test would be enabled/validated for windows\n(native, not only cygwin) as it might even work without the override\nand if we are to see conflicts, that is probably where most users with\nfile insensitive filesystems might be found\n\nCarlo\n"},{"id":"363766","messageId":"CACsJy8CzH1Xxd_Nus9EJEvcjrxKMvAyQZJGVb+ShMk7GFhdiXg@mail.gmail.com","threadId":"48963","inReplyTo":"CAPUEsphujJmC8R8acXFDgexeA61JYS8Fcv7Tog+Jt+bZhHrCDQ@mail.gmail.com","subject":"Re: [PATCH] clone: fix colliding file detection on APFS","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-20T19:38:45Z","receivedAt":"2018-11-20T19:39:15Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Nov 20, 2018 at 8:35 PM Carlo Arenas wrote:\n> IMHO it would be ideal if test would be enabled/validated for windows\n> (native, not only cygwin) as it might even work without the override\n> and if we are to see conflicts, that is probably where most users with\n> file insensitive filesystems might be found\n\nYes but I can't test on Windows so I will not enable the test until I\ngot a report that it's working there.\n-- \nDuy\n"},{"id":"363921","messageId":"20181122175952.25663-1-tboegi@web.de","threadId":"48963","inReplyTo":"20181120162853.22441-1-pclouds@gmail.com","subject":"[PATCH v1 1/1] t5601-99: Enable colliding file detection for MINGW","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2018-11-22T17:59:52Z","receivedAt":"2018-11-22T18:00:14Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Torsten Bögershausen \n\nCommit b878579ae7 (clone: report duplicate entries on case-insensitive\nfilesystems - 2018-08-17) adds a warning to user when cloning a repo\nwith case-sensitive file names on a case-insensitive file system.\n\nThis test has never been enabled for MINGW.\nIt had been working since day 1, but I forget to report that to the\nauthor.\nEnable it after a re-test.\n\nSigned-off-by: Torsten Bögershausen \n---\n\nThe other day, I wanted to test Duys patch -\nunder MINGW - to see if the problem is catch(ed)\nbut hehe git am failed to apply - not a big desaster,\nbecause is is already in master\nHere is a follow-up, end we can end the match\n\n\n t/t5601-clone.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex c28d51bd59..8bbc7068ac 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -628,7 +628,7 @@ test_expect_success 'clone on case-insensitive fs' '\n \t)\n '\n \n-test_expect_success !MINGW,CASE_INSENSITIVE_FS 'colliding file detection' '\n+test_expect_success CASE_INSENSITIVE_FS 'colliding file detection' '\n \tgrep X icasefs/warning &&\n \tgrep x icasefs/warning &&\n \ttest_i18ngrep \"the following paths have collided\" icasefs/warning\n-- \n2.19.0.271.gfe8321ec05\n\n"},{"id":"363925","messageId":"20181122201640.78495-1-carenas@gmail.com","threadId":"48963","inReplyTo":"20181122175952.25663-1-tboegi@web.de","subject":"[PATCH v1 1/1] t5601-99: Enable colliding file detection for MINGW","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2018-11-22T20:16:40Z","receivedAt":"2018-11-22T20:18:04Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Which FS was this tested on?, is Git LFS I keep hearing about also considered\na \"filesystem\" for git?\n\nCould you also test with the following applied on top?\n\nCarlo\n-- >8 --\nSubject: [PATCH] entry: remove windows fallback to inode checking\n\nthis test is really FS specific, so is better to avoid any compiled\nassumptions about the platform and let the user drive the fallback\nthrough core.checkStat instead\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n entry.c | 4 ----\n 1 file changed, 4 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex 0a3c451f5f..5ae74856e6 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -404,10 +404,6 @@ static void mark_colliding_entries(const struct checkout *state,\n {\n \tint i, trust_ino = check_stat;\n \n-#if defined(GIT_WINDOWS_NATIVE) || defined(__CYGWIN__)\n-\ttrust_ino = 0;\n-#endif\n-\n \tce->ce_flags |= CE_MATCHED;\n \n \tfor (i = 0; i < state->istate->cache_nr; i++) {\n-- \n2.20.0.rc1\n\n"},{"id":"363972","messageId":"nycvar.QRO.7.76.6.1811231221201.41@tvgsbejvaqbjf.bet","threadId":"48963","inReplyTo":"20181122201640.78495-1-carenas@gmail.com","subject":"Re: [PATCH v1 1/1] t5601-99: Enable colliding file detection for MINGW","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-23T11:24:27Z","receivedAt":"2018-11-23T11:24:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Carlo,\n\nOn Thu, 22 Nov 2018, Carlo Marcelo Arenas Belón wrote:\n\n> Subject: [PATCH] entry: remove windows fallback to inode checking\n> \n> this test is really FS specific, so is better to avoid any compiled\n> assumptions about the platform and let the user drive the fallback\n> through core.checkStat instead\n> \n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> entry.c | 4 ----\n> 1 file changed, 4 deletions(-)\n> \n> diff --git a/entry.c b/entry.c\n> index 0a3c451f5f..5ae74856e6 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -404,10 +404,6 @@ static void mark_colliding_entries(const struct checkout *state,\n> {\n> \tint i, trust_ino = check_stat;\n> \n> -#if defined(GIT_WINDOWS_NATIVE) || defined(__CYGWIN__)\n> -\ttrust_ino = 0;\n> -#endif\n> -\n\nNo, we cannot drop this. You may not see it in git.git's source code, but\nin Git for Windows' patches, we have an experimental feature (which had\nseemed to stabilize, but Ben Peart is currently doing wonders with it,\nimproving the performance substantially) for accelerating the file\nmetadata enumeration in a noticeable manner. The only way we can do that\nis by *not* insisting on a correct inode.\n\nBesides, IIRC even our regular stat() now \"fails\" to fill the inode field.\n\nSo no, we cannot do that. We can probably drop the `||\ndefined(__CYGINW__)` part (Cygwin even generates a fake inode for FAT,\nwhere no equivalent is available, by hashing the full normalized path).\nBut you cannot drop the `GIT_WINDOWS_NATIVE` part.\n\nCiao,\nJohannes\n\n> \tce->ce_flags |= CE_MATCHED;\n> \n> \tfor (i = 0; i < state->istate->cache_nr; i++) {\n> -- \n> 2.20.0.rc1\n> \n> "}]}