Git clone and case sensitivity

Hi,

Lately, 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.
The 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.

Quoted 11 lines · Paweł Paruzel

Hi,

Lately, 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.
The 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.

If we did what you proposed, it would be impossible to clone such a repository on a case-insensitive system.

While this might be fine for a closed system such as inside a company, this would make many open source repositories unusable, even when the files differing in case are nonfunctional (like README and readme).

This is actually one of a few ways people can make repositories that will show as modified on Windows or macOS systems, due to limitations in those OSes. If you want to be sure that your repository is unmodified after clone, you can ensure that the output of git status --porcelain is empty, such as by checking for a zero exit from
"test $(git status --porcelain | wc -l) -eq 0".

Signature · 2 lines

brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

Quoted 13 lines · brian m. carlson

Quoted 11 lines · Paweł Paruzel

Hi,

Lately, 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.
The 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.

Quoted 2 lines

If we did what you proposed, it would be impossible to clone such a repository on a case-insensitive system.

I agree throwing a real exception would be bad. But how about detecting the problem and trying our best to keep the repo in somewhat usable state like this?

This patch uses sparse checkout to hide all those paths that we fail to checkout, so you can still have a clean worktree to do things, as long as you don't touch those paths.

Quoted 7 lines

I agree throwing a real exception would be bad. But how about detecting the problem and trying our best to keep the repo in somewhat usable state like this?

This patch uses sparse checkout to hide all those paths that we fail to checkout, so you can still have a clean worktree to do things, as long as you don't touch those paths.

Side note. There may still be problems with this patch. Let's use vim-colorschemes.git as an example, which has darkBlue.vim and darkblue.vim.

Say we have checked out darkBlue.vim and hidden darkblue.vim. When you update darkBlue.vim on worktree and then update the index, are we sure we will update darkBlue.vim entry and not (hidden) darkblue.vim? I am not sure. I don't think our lookup function is prepared to deal with this. Maybe it's best to hide both of them.

Quoted 12 lines · Duy Nguyen

Quoted 7 lines

I agree throwing a real exception would be bad. But how about detecting the problem and trying our best to keep the repo in somewhat usable state like this?

This patch uses sparse checkout to hide all those paths that we fail to checkout, so you can still have a clean worktree to do things, as long as you don't touch those paths.

Side note. There may still be problems with this patch. Let's use vim-colorschemes.git as an example, which has darkBlue.vim and darkblue.vim.

Quoted 5 lines

Say we have checked out darkBlue.vim and hidden darkblue.vim. When you update darkBlue.vim on worktree and then update the index, are we sure we will update darkBlue.vim entry and not (hidden) darkblue.vim? I am not sure. I don't think our lookup function is prepared to deal with this. Maybe it's best to hide both of them.

It might be enough to just issue a warning and give an advise() hint that tells the user what's going on. Then they can decide what to do (hide both paths, or just work in the index, or move to a different fs, or complain to upstream).

static int checkout(int submodule_progress)
{

builtin/clone.c +0 -0

 	struct object_id oid;

Quoted 4 lines

It might be enough to just issue a warning and give an advise() hint that tells the user what's going on. Then they can decide what to do (hide both paths, or just work in the index, or move to a different fs, or complain to upstream).

Yeah that may be the best option. Something like this perhaps? Not sure how much detail the advice should be here.

builtin/clone.c +13 -0

+static int has_duplicate_icase_entries(struct index_state *istate)
+{
+	struct string_list list = STRING_LIST_INIT_NODUP;
+	int i;
+	int found = 0;
+
+	for (i = 0; i < istate->cache_nr; i++)
+		string_list_append(&list, istate->cache[i]->name);
+
+	list.cmp = strcasecmp;
+	string_list_sort(&list);
+
+	for (i = 1; i < list.nr; i++) {

strcasecmp() will only catch a subset of the cases. We really need to follow the same folding rules that the filesystem would.

For the case of clone, I actually wonder if we could detect during the checkout step that a file already exists. Since we know that the directory we started with was empty, then if it does, either:

- there's some funny case-folding going on that means two paths in the repository map to the same name in the filesystem; or

- somebody else is writing to the directory at the same time as us

Either of which I think would be worth warning about. I'm not sure if we already lstat() the paths we're writing anyway as part of the checkout, so we might even get the feature "for free".

Quoted 18 lines

builtin/clone.c +18 -0

+static int has_duplicate_icase_entries(struct index_state *istate)
+{
+     struct string_list list = STRING_LIST_INIT_NODUP;
+     int i;
+     int found = 0;
+
+     for (i = 0; i < istate->cache_nr; i++)
+             string_list_append(&list, istate->cache[i]->name);
+
+     list.cmp = strcasecmp;
+     string_list_sort(&list);
+
+     for (i = 1; i < list.nr; i++) {
+             if (strcasecmp(list.items[i-1].string,
+                            list.items[i].string))
+                     continue;
+             found = 1;
+             break;

builtin/clone.c static void update_head(const struct ref *our, const struct ref *remote, · line 736

Quoted 5 lines

builtin/clone.c +5 -0

+     }
+     string_list_clear(&list, 0);
+
+     return found;
+}

strcasecmp() will only catch a subset of the cases. We really need to follow the same folding rules that the filesystem would.

For the case of clone, I actually wonder if we could detect during the checkout step that a file already exists. Since we know that the directory we started with was empty, then if it does, either:

- there's some funny case-folding going on that means two paths in the repository map to the same name in the filesystem; or

- somebody else is writing to the directory at the same time as us

Quoted 2 lines

strcasecmp() will only catch a subset of the cases. We really need to follow the same folding rules that the filesystem would.

True. But that's how we handle case insensitivity internally. If a filesytem has more sophisticated folding rules then git will not work well on that one anyway.

Quoted 8 lines

For the case of clone, I actually wonder if we could detect during the checkout step that a file already exists. Since we know that the directory we started with was empty, then if it does, either:

- there's some funny case-folding going on that means two paths in the repository map to the same name in the filesystem; or

- somebody else is writing to the directory at the same time as us

This is exactly what my first patch does (minus the sparse checkout part).

But without knowing the exact folding rules, I don't think we can locate this "somebody else" who wrote the first path. So if N paths are treated the same by this filesystem, we could only report N-1 of them.

If we want to report just one path when this happens though, then this works quite well.

Paths that only differ in case work fine in a case-sensitive filesystems, but if those repos are cloned in a case-insensitive one, you'll get problems. The first thing to notice is "git status" will never be clean with no indication what's exactly is "dirty".

This patch helps the situation a bit by pointing out the problem at clone time. I have not suggested any way to work around or fix this problem. But I guess we could probably have a section in Documentation/ dedicated to this problem and point there instead of a long advice in this warning.

Another thing we probably should do is catch in "git checkout" too, not just "git clone" since your linux/unix colleage colleague may accidentally add some files that your mac/windows machine is not very happy with. But then there's another problem, once the problem is known, we probably should stop spamming this warning at every checkout, but how?

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 builtin/clone.c | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

builtin/clone.c +27 -0

index 5c439f1394..32738c2737 100644
@@ -711,6 +711,33 @@ static void update_head(const struct ref *our, const struct ref *remote,
 	}
 }
 
+static void find_duplicate_icase_entries(struct index_state *istate,
+					 struct string_list *dup)
+{
+	struct string_list list = STRING_LIST_INIT_NODUP;
+	int i;
+
+	for (i = 0; i < istate->cache_nr; i++)
+		string_list_append(&list, istate->cache[i]->name);
+
+	list.cmp = fspathcmp;
+	string_list_sort(&list);
+
+	for (i = 1; i < list.nr; i++) {
+		const char *cur = list.items[i].string;
+		const char *prev = list.items[i - 1].string;
+
+		if (dup->nr &&
+		    !fspathcmp(cur, dup->items[dup->nr - 1].string)) {
+			string_list_append(dup, cur);
+		} else if (!fspathcmp(cur, prev)) {
+			string_list_append(dup, prev);
+			string_list_append(dup, cur);
+		}
+	}
+	string_list_clear(&list, 0);
+}
+
 static int checkout(int submodule_progress)
 {
 	struct object_id oid;

@@ -761,6 +788,20 @@ static int checkout(int submodule_progress)
 	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))

builtin/clone.c +4 -0

+		}
+		string_list_clear(&dup, 0);
+	}
+
 	err |= run_hook_le(NULL, "post-checkout", sha1_to_hex(null_sha1),
 			   oid_to_hex(&oid), "1", NULL);
 

Signature · 1 line

2.18.0.656.gda699b98b3

I wonder if we can tell the users more about the "problems" and how to avoid them, or to live with them.

This is more loud thinking:

"The following paths only differ in case\n"
"One a case-insensitive file system only one at a time can be present\n" "You may rename one like this:\n"
"git checkout <file> && git mv <file> <file>.1\n"

Quoted 1 line

+ fprintf(stderr, "\t%s\n", dup.items[i].string);

Another question:
Do we need any quote_path() here ?
(This may be overkill, since typically the repos with conflicting names only use ASCII.)

Paths that only differ in case work fine in a case-sensitive filesystems, but if those repos are cloned in a case-insensitive one, you'll get problems. The first thing to notice is "git status" will never be clean with no indication what's exactly is "dirty".

This patch helps the situation a bit by pointing out the problem at clone time. I have not suggested any way to work around or fix this problem. But I guess we could probably have a section in Documentation/ dedicated to this problem and point there instead of a long advice in this warning.

Another thing we probably should do is catch in "git checkout" too, not just "git clone" since your linux/unix colleage colleague may

accidentally add some files that your mac/windows machine is not very happy with. But then there's another problem, once the problem is known, we probably should stop spamming this warning at every checkout, but how?

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 builtin/clone.c | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

builtin/clone.c +1 -0

index 5c439f1394..32738c2737 100644
@@ -711,6 +711,33 @@ static void update_head(const struct ref *our, const struct ref *remote,
        }
 }
 
+static void find_duplicate_icase_entries(struct index_state *istate,

builtin/clone.c +4 -0

+       }
+       string_list_clear(&list, 0);
+}
+
 static int checkout(int submodule_progress)
 {
        struct object_id oid;

Is it worth attempting to also warn about paths that only differ in UTF-normalization on relevant MacOS systems?

Quoted 2 lines · Elijah Newren

Is it worth attempting to also warn about paths that only differ in UTF-normalization on relevant MacOS systems?

I hate to bring up a totally different approach this late in the party, but I wonder if it makes more sense to take advantage of "clone" being a command that starts from an empty working tree.

builtin/clone.c::checkout() drives a single-tree unpack_trees(), using oneway_merge() as its callback and at the end, eventually unpack_trees.c:check_updates() will call into checkout_entry() to perform the usual "unlink and then create" dance.

I wonder if it makes sense to introduce a new option to tell the machinery to report when the final checkout_entry() notices that it needed to remove the working tree file to make room (perhaps that bit would go in "struct unpack_trees_options"). In the initial checkout codepath for a freshly cloned repository, that would only happen when your tree has two (or more) paths that gets smashed by case insensitive or UTF-normalizing filesystem, and the code we would maintain do not have to care how exactly the filesystem collapses two (or more) paths if we go that way. We only need to report "we tried to check out X but it seems your filesystem equates something else that is also in the project to X".

On Tue, Jul 31, 2018 at 12:12:58PM -0700, Junio C Hamano wrote:
...

Quoted 3 lines

collapses two (or more) paths if we go that way. We only need to report "we tried to check out X but it seems your filesystem equates something else that is also in the project to X".

Heh. See my similar suggestion in:

https://public-inbox.org/git/[email protected]/

and the response from Duy.

Quoted 6 lines · Jeff King

On Tue, Jul 31, 2018 at 12:12:58PM -0700, Junio C Hamano wrote:
...

Quoted 3 lines

collapses two (or more) paths if we go that way. We only need to report "we tried to check out X but it seems your filesystem equates something else that is also in the project to X".

Quoted 5 lines

Heh. See my similar suggestion in:

https://public-inbox.org/git/[email protected]/

and the response from Duy.

Yes, but is there a reason why we need to report what that "something else" is?

Presumably we are already in an error codepath, so if it is absolutely necessary, then we can issue a lstat() to grab the inum for the path we are about to create, iterate over the previously checked out paths issuing lstat() and see which one yields the same inum, to find the one who is the culprit.

Quoted 5 lines

Presumably we are already in an error codepath, so if it is absolutely necessary, then we can issue a lstat() to grab the inum for the path we are about to create, iterate over the previously checked out paths issuing lstat() and see which one yields the same inum, to find the one who is the culprit.

Yes, this is the cleverness I was missing in my earlier response.

So it seems do-able, and I like that this incurs no cost in the non-error case.

Quoted 10 lines · Jeff King

Quoted 5 lines

Presumably we are already in an error codepath, so if it is absolutely necessary, then we can issue a lstat() to grab the inum for the path we are about to create, iterate over the previously checked out paths issuing lstat() and see which one yields the same inum, to find the one who is the culprit.

Yes, this is the cleverness I was missing in my earlier response.

So it seems do-able, and I like that this incurs no cost in the non-error case.

Not so fast, unfortunately.

I suspect that some filesystems do not give us inum that we can use for that "identity" purpose, and they tend to be the ones with the case smashing characteristics where we need this code in the error path the most X-<.

Quoted 13 lines · Junio C Hamano

Quoted 10 lines · Jeff King

Quoted 5 lines

Presumably we are already in an error codepath, so if it is absolutely necessary, then we can issue a lstat() to grab the inum for the path we are about to create, iterate over the previously checked out paths issuing lstat() and see which one yields the same inum, to find the one who is the culprit.

Yes, this is the cleverness I was missing in my earlier response.

So it seems do-able, and I like that this incurs no cost in the non-error case.

Quoted 6 lines

Not so fast, unfortunately.

I suspect that some filesystems do not give us inum that we can use for that "identity" purpose, and they tend to be the ones with the case smashing characteristics where we need this code in the error path the most X-<.

But even if inum is unreliable, we should be able to use other clues, perhaps the same set of fields we use for cached stat matching optimization we use for "diff" plumbing commands, to implement the error report. The more important part of the idea is that we already need to notice that we need to remove a path that is in the working tree while doing the checkout, so the alternative approach won't incur any extra cost for normal cases where the project being checked out does not have two files whose pathnames are only different in case (or checking out such an offending project to a case sensitive filesytem, of course).

So I guess it still _is_ workable. Any takers?

Quoted 11 lines

But even if inum is unreliable, we should be able to use other clues, perhaps the same set of fields we use for cached stat matching optimization we use for "diff" plumbing commands, to implement the error report. The more important part of the idea is that we already need to notice that we need to remove a path that is in the working tree while doing the checkout, so the alternative approach won't incur any extra cost for normal cases where the project being checked out does not have two files whose pathnames are only different in case (or checking out such an offending project to a case sensitive filesytem, of course).

Quoted 1 line

So I guess it still _is_ workable. Any takers?

OK so we're going back to the original way of checking that we check out the different files on the same place (because fs is icase) and try to collect all paths for reporting, yes? I can give it another go (but of course if anybody else steps up, I'd very gladly hand this over)

Quoted 5 lines

OK so we're going back to the original way of checking that we check out the different files on the same place (because fs is icase) and try to collect all paths for reporting, yes? I can give it another go (but of course if anybody else steps up, I'd very gladly hand this over)

Detect and report, definitely yes; I am not sure about collect all (personally I am OK if we stopped at reporting "I tried to check out X but your project tree has something else that is turned to X by your pathname-smashing filesystem" without making it a requirement to report what the other one that conflict with X is. Of course, reporting the other side _is_ nicer and I'd be happier if we can do so without too much ugly code, but I do not think it is a hard requirement.

I also wonder if Windows could return some other file-unique identifier that would work in place of an inode here. That would be pretty easy to swap in via an #ifdef's helper function. I'd be OK shipping without that and letting Windows folks fill it in later (as long as we do not do anything too stupid until then, like claim all of the inode==0 files are the same).

PS It occurs to me that doing this naively (re-scan the entries already
checked out when we see a collision) ends up quadratic over the number of entries in the worst case. That may not matter. You'd only have a handful of collisions normally, and anybody malicious can already git-bomb your checkout anyway. If we care, an alternative would be to set a flag for "I saw some collisions", and then follow up with a single pass putting entries into a hashmap of inode->filename.

Quoted 6 lines · Jeff King

I also wonder if Windows could return some other file-unique identifier that would work in place of an inode here. That would be pretty easy to swap in via an #ifdef's helper function. I'd be OK shipping without that and letting Windows folks fill it in later (as long as we do not do anything too stupid until then, like claim all of the inode==0 files are the same).

Yeah, but such a useful file-unique identifier would probably be used in place of inum in their (l)stat emulation already, if exists, no?

Quoted 9 lines · Junio C Hamano

Quoted 6 lines · Jeff King

I also wonder if Windows could return some other file-unique identifier that would work in place of an inode here. That would be pretty easy to swap in via an #ifdef's helper function. I'd be OK shipping without that and letting Windows folks fill it in later (as long as we do not do anything too stupid until then, like claim all of the inode==0 files are the same).

Quoted 3 lines

Yeah, but such a useful file-unique identifier would probably be used in place of inum in their (l)stat emulation already, if exists, no?

Maybe. It might not work as ino_t. Or it might be expensive to get. Or maybe it's simply impossible. I don't know much about Windows. Some searching implies that NTFS does have a "file index" concept which is supposed to be unique.

At any rate, until we have an actual plan for Windows, I think it would make sense only to split the cases into "has working inodes" and "other", and make sure "other" does something sensible in the meantime (like mention the conflict, but skip trying to list duplicates).

When somebody wants to work on Windows support, then we can figure out if it just needs to wrap the "get unique identifier" operation, or if it would use a totally different algorithm.

-Peff

Another thing to keep in mind is that the collision could be because of case folding (or other such nonsense) on a directory in the path. I mean, if someone on Linux builds a commit containing:

    a/b/c/D/e/foo.txt
    a/b/c/d/e/foo.txt

we'll get a similar collision as if one of them were spelled "FOO.txt".

Also, do we need to worry about hard-links or symlinks here?

If checkout populates symlinks, then you might have another collision opportunity. For example:

    a/b/c/D/e/foo.txt
    a/link -> ./b/c/d
    a/link/e/foo.txt

Also, some platforms (like the Mac) allow directory hard-links. Granted, Git doesn't create hard-links during checkout, but the user might.

I'm sure there are other edge cases here that make reporting difficult; these are just a few I thought of. I guess what I'm trying to say is that as a first step just report that you found a collision -- without trying to identify the set existing objects that it collided with.

Quoted 4 lines

Maybe. It might not work as ino_t. Or it might be expensive to get. Or maybe it's simply impossible. I don't know much about Windows. Some searching implies that NTFS does have a "file index" concept which is supposed to be unique.

This is hard and/or expensive on Windows. Yes, you can get the "file index" values for an open file handle with a cost similar to an fstat(). Unfortunately, the FindFirst/FindNext routines (equivalent to the opendir/readdir routines), don't give you that data. So we'd have to scan the directory and then open and stat each file. This is terribly expensive on Windows -- and the reason we have the fscache layer (in the GfW version) to intercept the lstat() calls whenever possible.

Another thing to keep in mind is that the collision could be because of case folding (or other such nonsense) on a directory in the path. I mean, if someone on Linux builds a commit containing:

    a/b/c/D/e/foo.txt
    a/b/c/d/e/foo.txt

we'll get a similar collision as if one of them were spelled "FOO.txt".

Also, do we need to worry about hard-links or symlinks here?

I'm sure there are other edge cases here that make reporting difficult; these are just a few I thought of. I guess what I'm trying to say is that as a first step just report that you found a collision -- without trying to identify the set existing objects that it collided with.

Quoted 4 lines

At any rate, until we have an actual plan for Windows, I think it would make sense only to split the cases into "has working inodes" and "other", and make sure "other" does something sensible in the meantime (like mention the conflict, but skip trying to list duplicates).

Yes, this should be split. Do the "easy" Linux version first. Keep in mind that there may also be a different solution for the Mac.

Quoted 5 lines · Jeff Hostetler

Quoted 4 lines

Maybe. It might not work as ino_t. Or it might be expensive to get. Or maybe it's simply impossible. I don't know much about Windows. Some searching implies that NTFS does have a "file index" concept which is supposed to be unique.

Quoted 8 lines

This is hard and/or expensive on Windows. Yes, you can get the "file index" values for an open file handle with a cost similar to an fstat(). Unfortunately, the FindFirst/FindNext routines (equivalent to the opendir/readdir routines), don't give you that data. So we'd have to scan the directory and then open and stat each file. This is terribly expensive on Windows -- and the reason we have the fscache layer (in the GfW version) to intercept the lstat() calls whenever possible.

I think that high cost might be OK for our purposes here. This code would _only_ kick in during a clone, and then only on the error path once we knew we had a collision during the checkout step.

Quoted 34 lines · Junio C Hamano

Quoted 13 lines · Junio C Hamano

Quoted 10 lines · Jeff King

Quoted 5 lines

Presumably we are already in an error codepath, so if it is absolutely necessary, then we can issue a lstat() to grab the inum for the path we are about to create, iterate over the previously checked out paths issuing lstat() and see which one yields the same inum, to find the one who is the culprit.

Yes, this is the cleverness I was missing in my earlier response.

So it seems do-able, and I like that this incurs no cost in the non-error case.

Quoted 6 lines

Not so fast, unfortunately.

I suspect that some filesystems do not give us inum that we can use for that "identity" purpose, and they tend to be the ones with the case smashing characteristics where we need this code in the error path the most X-<.

But even if inum is unreliable, we should be able to use other clues, perhaps the same set of fields we use for cached stat matching optimization we use for "diff" plumbing commands, to implement the error report. The more important part of the idea is that we already need to notice that we need to remove a path that is in the working tree while doing the checkout, so the alternative approach won't incur any extra cost for normal cases where the project being checked out does not have two files whose pathnames are only different in case (or checking out such an offending project to a case sensitive filesytem, of course).

Quoted 1 line

So I guess it still _is_ workable. Any takers?

OK so we're going back to the original way of checking that we check out the different files on the same place (because fs is icase) and try to collect all paths for reporting, yes?

I can give it another go
(but of course if anybody else steps up, I'd very gladly hand this over)

Is it worth attempting to also warn about paths that only differ in UTF-normalization on relevant MacOS systems?

Another thing we probably should do is catch in "git checkout" too, not just "git clone" since your linux/unix colleage colleague may accidentally add some files that your mac/windows machine is not very happy with.

Quoted 4 lines · Nguyễn Thái Ngọc Duy

Another thing we probably should do is catch in "git checkout" too, not just "git clone" since your linux/unix colleage colleague may accidentally add some files that your mac/windows machine is not very happy with.

Then you would catch it not in checkout but in add, no?

One nice thing about this is we don't need platform specific code for detecting the duplicate entries. I think ce_match_stat() works even on Windows. And it's now equally expensive on all platforms :D

 builtin/clone.c |  1 +
 cache.h         |  2 ++
 entry.c         | 44 ++++++++++++++++++++++++++++++++++++++++++++
 unpack-trees.c  | 23 +++++++++++++++++++++++
 unpack-trees.h  |  1 +
 5 files changed, 71 insertions(+)

entry.c +31 -0

+static void mark_duplicate_entries(const struct checkout *state,
+				   struct cache_entry *ce, struct stat *st)
+{
+	int i;
+	int *count = state->nr_duplicates;
+
+	if (!count)
+		BUG("state->nr_duplicates must not be NULL");
+
+	ce->ce_flags |= CE_MATCHED;
+	(*count)++;
+
+	if (!state->refresh_cache)
+		BUG("We need this to narrow down the set of updated entries");
+
+	for (i = 0; i < state->istate->cache_nr; i++) {
+		struct cache_entry *dup = state->istate->cache[i];
+
+		/*
+		 * This entry has not been checked out yet, otherwise
+		 * its stat info must have been updated. And since we
+		 * check out from top to bottom, the rest is guaranteed
+		 * not checked out. Stop now.
+		 */
+		if (!ce_uptodate(dup))
+			break;
+
+		if (dup->ce_flags & CE_MATCHED)
+			continue;
+
+		if (ce_match_stat(dup, st,

Quoted 3 lines · Nguyễn Thái Ngọc Duy

One nice thing about this is we don't need platform specific code for detecting the duplicate entries. I think ce_match_stat() works even on Windows. And it's now equally expensive on all platforms :D

ce_match_stat() may not be a very good measure to see if two paths refer to the same file, though. After a fresh checkout, I would not be surprised if two completely unrelated paths have the same size and have same mtime/ctime. In its original use case, i.e. "I have one specific path in mind and took a snapshot of its metadata earlier. Is it still the same, or has it been touched?", that may be sufficient to detect that the path has been _modified_, but without reliable inum, it may be a poor measure to say two paths refer to the same.

Quoted 9 lines

ce_match_stat() may not be a very good measure to see if two paths refer to the same file, though. After a fresh checkout, I would not be surprised if two completely unrelated paths have the same size and have same mtime/ctime. In its original use case, i.e. "I have one specific path in mind and took a snapshot of its metadata earlier. Is it still the same, or has it been touched?", that may be sufficient to detect that the path has been _modified_, but without reliable inum, it may be a poor measure to say two paths refer to the same.

I agree with Junio on this one. The mtime values are sloppy at best. On FAT file systems, they have 2 second resolution. Even NTFS IIRC has only 100ns resolution, so we might get a lot of false matches using this technique, right?

It might be better to build an equivalence-class hash-map for the colliding entries. Compute a "normalized" version of the pathname (such as convert to lowercase, strip final-dots/spaces, strip the digits following tilda of a shortname, and etc for the MAC's UTF-isms). Then when you rescan the index entries to find the matches, apply the equivalence operator on the pathname and do the hashmap lookup. When you find a match, you have a "potential" collider pair (I say potential only because of the ambiguity of shortnames). Then we can use inum/file-index/whatever to see if they actually collide.

I think we really want to avoid doing that normalization ourselves if we can. There are just too many filesystem-specific rules.

If we have an equivalence-class hashmap and feed it inodes (or again, some system equivalent) as the keys, we should get buckets of collisions.

Quoted 3 lines

If we have an equivalence-class hashmap and feed it inodes (or again, some system equivalent) as the keys, we should get buckets of collisions.

I guess one way to get "some system equivalent" that can be used as the last resort, when there absolutely is no inum equivalent, is to rehash the working tree file that shouldn't be there when we detect a collision.

If we found that there is something when we tried to write out "Foo.txt", if we open "Foo.txt" on the working tree and hash-object it, we should find the matching blob somewhere in the index _before_ "Foo.txt". On a case-insensitive filesytem, it may well be "foo.txt", but we do not even have to know "foo.txt" and "Foo.txt" only differ in case.

Quoted 9 lines · Junio C Hamano

Quoted 3 lines

If we have an equivalence-class hashmap and feed it inodes (or again, some system equivalent) as the keys, we should get buckets of collisions.

I guess one way to get "some system equivalent" that can be used as the last resort, when there absolutely is no inum equivalent, is to rehash the working tree file that shouldn't be there when we detect a collision.

Quoted 6 lines

If we found that there is something when we tried to write out "Foo.txt", if we open "Foo.txt" on the working tree and hash-object it, we should find the matching blob somewhere in the index _before_ "Foo.txt". On a case-insensitive filesytem, it may well be "foo.txt", but we do not even have to know "foo.txt" and "Foo.txt" only differ in case.

Clever. You might still run into false positives when there is duplicated content in the repository (especially, say, zero-length files). But the fact that you only do the hashing on known duplicates helps with that.

One of the things I did like about the equivalence-class approach is that it can be done in a single linear pass in the worst case. Whereas anything that searches when we see a collision is quite likely to be quadratic. But as I said before, it may not be worth worrying too much about that for an error code path where we expect the number of collisions to be small.

-Peff

Quoted 5 lines

Clever. You might still run into false positives when there is duplicated content in the repository (especially, say, zero-length files). But the fact that you only do the hashing on known duplicates helps with that.

I worry that the false positives make this a non-starter. I mean, if clone creates files 'A' and 'B' (both equal) and then tries to create 'b', would the collision code reports that 'b' collided with 'A' because that was the first OID match? Ideally with this scheme we'd have to search the entire index prior to 'b' and then report that 'b' collided with either 'A' or 'B'. Neither message instills confidence. And there's no way to prefer answer 'B' over 'A' without using knowledge of the FS name mangling/aliasing rules -- unless we want to just assume ignore-case for this iteration.

Sorry to be paranoid, but I have an index with 3.5M entries, the word "quadratic" rings all kinds of alarm bells for me. :-)

Granted, we expect the number of collisions to be small, but searching back for each collision over the already-populated portion of the index could be expensive.

Quoted 8 lines · Jeff King

Quoted 6 lines · Junio C Hamano

If we found that there is something when we tried to write out "Foo.txt", if we open "Foo.txt" on the working tree and hash-object it, we should find the matching blob somewhere in the index _before_ "Foo.txt". On a case-insensitive filesytem, it may well be "foo.txt", but we do not even have to know "foo.txt" and "Foo.txt" only differ in case.

Quoted 4 lines

Clever. You might still run into false positives when there is duplicated content in the repository (especially, say, zero-length files). But the fact that you only do the hashing on known duplicates helps with that.

I worry that the false positives make this a non-starter. I mean, if clone creates files 'A' and 'B' (both equal) and then tries to create 'b', would the collision code reports that 'b' collided with 'A' because that was the first OID match? Ideally with this scheme we'd have to search the entire index prior to 'b' and then report that 'b' collided with either 'A' or 'B'. Neither message instills confidence. And there's no way to prefer answer 'B' over 'A' without using knowledge of the FS name mangling/aliasing rules -- unless we want to just assume ignore-case for this iteration.

Quoted 9 lines

I worry that the false positives make this a non-starter. I mean, if clone creates files 'A' and 'B' (both equal) and then tries to create 'b', would the collision code reports that 'b' collided with 'A' because that was the first OID match? Ideally with this scheme we'd have to search the entire index prior to 'b' and then report that 'b' collided with either 'A' or 'B'. Neither message instills confidence. And there's no way to prefer answer 'B' over 'A' without using knowledge of the FS name mangling/aliasing rules -- unless we want to just assume ignore-case for this iteration.

A possibly crazy idea: Don't bother reporting the other filename; just report the OID instead.

"Error: Foo.txt cannot be checked out because another file with hash <whatever> is in the way." Maybe even add a hint for the user: "Run git ls-files -s to see see all files and their hash".

Whatever the exact wording for the error message, just create a nice post on stackoverflow.com explaining the various weird filesystems out there (VFAT, NTFS, HFS, APFS, etc) and how they cause differing filenames to be written to the same location. Have a bunch of folks vote it up so it has some nice search-engine juice.

The error message isn't quite as good, but does the user really need all the names of the file? If so, we gave them enough information to figure it out, and this is a really unusual case anyway, right? Besides, now we're back to linear performance....

Quoted 4 lines

The error message isn't quite as good, but does the user really need all the names of the file? If so, we gave them enough information to figure it out, and this is a really unusual case anyway, right? Besides, now we're back to linear performance....

Well, it's still quadratic when they run O(n) iterations of "git ls-files -s | grep $colliding_oid". You've just pushed the second linear search onto the user. ;)

Quoted 5 lines · Jeff King

Quoted 4 lines

The error message isn't quite as good, but does the user really need all the names of the file? If so, we gave them enough information to figure it out, and this is a really unusual case anyway, right? Besides, now we're back to linear performance....

Quoted 3 lines

Well, it's still quadratic when they run O(n) iterations of "git ls-files -s | grep $colliding_oid". You've just pushed the second linear search onto the user. ;)

Wouldn't that be their own fault for not running
git ls-files -s | grep -e $colliding_oid_1 ... -e $colliding_oid_n | sort -k 2
? ;-)

Quoted 10 lines · Elijah Newren

Quoted 8 lines · Jeff King

Quoted 4 lines

The error message isn't quite as good, but does the user really need all the names of the file? If so, we gave them enough information to figure it out, and this is a really unusual case anyway, right? Besides, now we're back to linear performance....

Well, it's still quadratic when they run O(n) iterations of "git ls-files -s | grep $colliding_oid". You've just pushed the second linear search onto the user. ;)

Quoted 3 lines

Wouldn't that be their own fault for not running
git ls-files -s | grep -e $colliding_oid_1 ... -e $colliding_oid_n | sort -k 2
? ;-)

Man, this thread is the gift that keeps on giving. :)

That's still quadratic, isn't it? You've just hidden the second dimension in the single grep call.

Now since these are all going to be constant strings, in theory an intelligent grep could stick them all in a search trie, and match each line with complexity k, the length of the matched strings. And since k=40, that's technically still linear overall.

A possibly crazy idea: Don't bother reporting the other filename; just report the OID instead.

"Error: Foo.txt cannot be checked out because another file with hash <whatever> is in the way." Maybe even add a hint for the user: "Run git ls-files -s to see see all files and their hash".

entry.c +8 -0

+		}
+	}
+#endif
+}
+
 /*
  * Write the contents from ce out to the working tree.
  *

@@ -455,6 +484,9 @@ int checkout_entry(struct cache_entry *ce,
 			return -1;
 		}
 
+		if (state->clone)
+			mark_duplicate_entries(state, ce, &st);
+
 		/*
 		 * We unlink the old file, to get the new one with the
 		 * right permissions (including umask, which is nasty

unpack-trees.c +8 -0

index f9efee0836..d4fece913c 100644
@@ -344,12 +344,20 @@ static int check_updates(struct unpack_trees_options *o)
 	struct index_state *index = &o->result;
 	struct checkout state = CHECKOUT_INIT;
 	int i;
+	int nr_duplicates = 0;
 
 	state.force = 1;
 	state.quiet = 1;
 	state.refresh_cache = 1;
 	state.istate = index;
 
+	if (o->clone) {
+		state.clone = 1;
+		state.nr_duplicates = &nr_duplicates;
+		for (i = 0; i < index->cache_nr; i++)
+			index->cache[i]->ce_flags &= ~CE_MATCHED;
+	}
+
 	progress = get_progress(o);
 
 	if (o->update)

unpack-trees.c +9 -0

+		for (i = 0; i < index->cache_nr; i++) {
+			struct cache_entry *ce = index->cache[i];
+
+			if (!(ce->ce_flags & CE_MATCHED))
+				continue;
+			fprintf(stderr, "  '%s'\n", ce->name);
+			ce->ce_flags &= ~CE_MATCHED;
+		}
+	}

Paths that only differ in case work fine in a case-sensitive filesystems, but if those repos are cloned in a case-insensitive one, you'll get problems. The first thing to notice is "git status" will never be clean with no indication what exactly is "dirty".

This patch helps the situation a bit by pointing out the problem at clone time. Even though this patch talks about case sensitivity, the patch makes no assumption about folding rules by the filesystem. It simply observes that if an entry has been already checked out at clone time when we're about to write a new path, some folding rules are behind this.

This patch is tested with vim-colorschemes repository on a JFS partition with case insensitive support on Linux. This repository has two files darkBlue.vim and darkblue.vim.

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

Quoted 14 lines

Paths that only differ in case work fine in a case-sensitive filesystems, but if those repos are cloned in a case-insensitive one, you'll get problems. The first thing to notice is "git status" will never be clean with no indication what exactly is "dirty".

This patch helps the situation a bit by pointing out the problem at clone time. Even though this patch talks about case sensitivity, the patch makes no assumption about folding rules by the filesystem. It simply observes that if an entry has been already checked out at clone time when we're about to write a new path, some folding rules are behind this.

Quoted 5 lines

This patch is tested with vim-colorschemes repository on a JFS partition with case insensitive support on Linux. This repository has two files darkBlue.vim and darkblue.vim.

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

This patch makes 'clone http repository' in
't5551-http-fetch-smart.sh' fail with:

--- exp 2018-08-11 02:29:45.216641851 +0000
+++ actual.smudged 2018-08-11 02:29:45.264642318 +0000
@@ -15,3 +15,5 @@
< Pragma: no-cache
< Cache-Control: no-cache, max-age=0, must-revalidate
< Content-Type: application/x-git-upload-pack-result
+> warning: the following paths have collided and only one from the same +> colliding group is in the working tree:

I also notice that this patch doesn't add any tests... :)

I was careless and checked the wrong variable (should have checked nr_duplicates not state.nr_duplicates; the second is a pointer). So we always get this warning (and with no following list of files)

Quoted 1 line

I also notice that this patch doesn't add any tests... :)

This is platform specific and I was to be frank a bit lazy. Will consider adding a test with CASE_INSENSITIVE_FS after this.

Quoted 3 lines · Duy Nguyen

I was careless and checked the wrong variable (should have checked nr_duplicates not state.nr_duplicates; the second is a pointer). So we always get this warning (and with no following list of files)

Heh, does that bug go away if you got rid of the pointer-ness of the field and store the value directly in there?

There are lots of suggestions on optimizing this stuff, but since this problem does not affect me to begin with, I'm reluctant to make more changes and going to stay simple, stupid and slow. I could continue to do small updates if needed. But for bigger changes, consider this patch dropped by me.

v3 now uses inode on UNIXy platforms for checking colliding items. I still don't try to separate colliding groups because it should be quite obvious once you look at the colliding list (and most of the time I suspect we only have one or two groups).

Paths that only differ in case work fine in a case-sensitive filesystems, but if those repos are cloned in a case-insensitive one, you'll get problems. The first thing to notice is "git status" will never be clean with no indication what exactly is "dirty".

This patch helps the situation a bit by pointing out the problem at clone time. Even though this patch talks about case sensitivity, the patch makes no assumption about folding rules by the filesystem. It simply observes that if an entry has been already checked out at clone time when we're about to write a new path, some folding rules are behind this.

This patch is tested with vim-colorschemes repository on a JFS partition with case insensitive support on Linux. This repository has two files darkBlue.vim and darkblue.vim.

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
v4 removes nr_duplicates (and fixes that false warning Szeder reported). It also hints about case insensitivity as a cause of problem because it's most likely the case when this warning shows up.

  builtin/clone.c  |  1 +
  cache.h          |  1 +
  entry.c          | 28 ++++++++++++++++++++++++++++
  t/t5601-clone.sh |  8 +++++++-
  unpack-trees.c   | 28 ++++++++++++++++++++++++++++
  unpack-trees.h   |  1 +
  6 files changed, 66 insertions(+), 1 deletion(-)

builtin/clone.c +1 -0

index 5c439f1394..0702b0e9d0 100644
@@ -747,6 +747,7 @@ static int checkout(int submodule_progress)
  	memset(&opts, 0, sizeof opts);
  	opts.update = 1;
  	opts.merge = 1;
+	opts.clone = 1;
  	opts.fn = oneway_merge;
  	opts.verbose_update = (option_verbosity >= 0);
  	opts.src_index = &the_index;

cache.h +1 -0

index 8b447652a7..6d6138f4f1 100644
@@ -1455,6 +1455,7 @@ struct checkout {
  	unsigned force:1,
  		 quiet:1,
  		 not_new:1,
+		 clone:1,
  		 refresh_cache:1;
  };
  #define CHECKOUT_INIT { NULL, "" }

entry.c +28 -0

index b5d1d3cf23..c70340df8e 100644
@@ -399,6 +399,31 @@ static int check_path(const char *path, int len, struct stat *st, int skiplen)
  	return lstat(path, st);
  }
 
+static void mark_colliding_entries(const struct checkout *state,
+				   struct cache_entry *ce, struct stat *st)
+{
+	int i;
+
+	ce->ce_flags |= CE_MATCHED;
+
+#if !defined(GIT_WINDOWS_NATIVE) /* inode is always zero on Windows */
+	for (i = 0; i < state->istate->cache_nr; i++) {
+		struct cache_entry *dup = state->istate->cache[i];
+
+		if (dup == ce)
+			break;
+
+		if (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE))
+			continue;
+
+		if (dup->ce_stat_data.sd_ino == st->st_ino) {
+			dup->ce_flags |= CE_MATCHED;
+			break;
+		}
+	}
+#endif
+}
+
  /*
   * Write the contents from ce out to the working tree.
   *

@@ -455,6 +480,9 @@ int checkout_entry(struct cache_entry *ce,
  			return -1;
  		}
 
+		if (state->clone)
+			mark_colliding_entries(state, ce, &st);
+
  		/*
  		 * We unlink the old file, to get the new one with the
  		 * right permissions (including umask, which is nasty

t/t5601-clone.sh +7 -1

index 0b62037744..f2eb73bc74 100755
@@ -624,10 +624,16 @@ test_expect_success 'clone on case-insensitive fs' '
  			git hash-object -w -t tree --stdin) &&
  		c=$(git commit-tree -m bogus $t) &&
  		git update-ref refs/heads/bogus $c &&
-		git clone -b bogus . bogus
+		git clone -b bogus . bogus 2>warning
  	)
  '
 
+test_expect_success !MINGW,!CYGWIN,CASE_INSENSITIVE_FS 'colliding file detection' '
+	grep X icasefs/warning &&
+	grep x icasefs/warning &&
+	test_i18ngrep "the following paths have collided" icasefs/warning
+'
+
  partial_clone () {
  	       SERVER="$1" &&
  	       URL="$2" &&

unpack-trees.c +18 -0

index cd0680f11e..443df048ef 100644
@@ -359,6 +359,12 @@ static int check_updates(struct unpack_trees_options *o)
  	state.refresh_cache = 1;
  	state.istate = index;
 
+	if (o->clone) {
+		state.clone = 1;
+		for (i = 0; i < index->cache_nr; i++)
+			index->cache[i]->ce_flags &= ~CE_MATCHED;
+	}
+
  	progress = get_progress(o);
 
  	if (o->update)

@@ -423,6 +429,28 @@ static int check_updates(struct unpack_trees_options *o)
  	errs |= finish_delayed_checkout(&state);
  	if (o->update)
  		git_attr_set_direction(GIT_ATTR_CHECKIN, NULL);
+
+	if (o->clone) {
+		int printed_warning = 0;
+
+		for (i = 0; i < index->cache_nr; i++) {
+			struct cache_entry *ce = index->cache[i];
+
+			if (!(ce->ce_flags & CE_MATCHED))
+				continue;
+
+			if (!printed_warning) {
+				warning(_("the following paths have collided (e.g. case-sensitive paths\n"

unpack-trees.c +0 -0

  	return errs != 0;
  }
 

unpack-trees.h +1 -0

index c2b434c606..d940f1c5c2 100644
@@ -42,6 +42,7 @@ struct unpack_trees_options {
  	unsigned int reset,
  		     merge,
  		     update,
+		     clone,
  		     index_only,
  		     nontrivial_merge,
  		     trivial_merges_only,

Paths that only differ in case work fine in a case-sensitive filesystems, but if those repos are cloned in a case-insensitive one, you'll get problems. The first thing to notice is "git status" will never be clean with no indication what exactly is "dirty".

This patch helps the situation a bit by pointing out the problem at clone time. Even though this patch talks about case sensitivity, the patch makes no assumption about folding rules by the filesystem. It simply observes that if an entry has been already checked out at clone time when we're about to write a new path, some folding rules are behind this.

This patch is tested with vim-colorschemes repository on a JFS partition with case insensitive support on Linux. This repository has two files darkBlue.vim and darkblue.vim.

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
v4 removes nr_duplicates (and fixes that false warning Szeder reported). It also hints about case insensitivity as a cause of problem because it's most likely the case when this warning shows up.

entry.c +14 -0

index b5d1d3cf23..c70340df8e 100644
@@ -399,6 +399,31 @@ static int check_path(const char *path, int len, struct stat *st, int skiplen)
 	return lstat(path, st);
 }
 
+static void mark_colliding_entries(const struct checkout *state,
+				   struct cache_entry *ce, struct stat *st)
+{
+	int i;
+
+	ce->ce_flags |= CE_MATCHED;
+
+#if !defined(GIT_WINDOWS_NATIVE) /* inode is always zero on Windows */
+	for (i = 0; i < state->istate->cache_nr; i++) {
+		struct cache_entry *dup = state->istate->cache[i];
+
+		if (dup == ce)
+			break;
+

t/t5601-clone.sh +0 -1

index 0b62037744..f2eb73bc74 100755
@@ -624,10 +624,16 @@ test_expect_success 'clone on case-insensitive fs' '
 			git hash-object -w -t tree --stdin) &&
 		c=$(git commit-tree -m bogus $t) &&
 		git update-ref refs/heads/bogus $c &&
-		git clone -b bogus . bogus

Paths that only differ in case work fine in a case-sensitive filesystems, but if those repos are cloned in a case-insensitive one, you'll get problems. The first thing to notice is "git status" will never be clean with no indication what exactly is "dirty".

This patch helps the situation a bit by pointing out the problem at clone time. Even though this patch talks about case sensitivity, the patch makes no assumption about folding rules by the filesystem. It simply observes that if an entry has been already checked out at clone time when we're about to write a new path, some folding rules are behind this.

This patch is tested with vim-colorschemes repository on a JFS partition with case insensitive support on Linux. This repository has two files darkBlue.vim and darkblue.vim.

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
v4 removes nr_duplicates (and fixes that false warning Szeder reported). It also hints about case insensitivity as a cause of problem because it's most likely the case when this warning shows up.

 builtin/clone.c  |  1 +
 cache.h          |  1 +
 entry.c          | 28 ++++++++++++++++++++++++++++
 t/t5601-clone.sh |  8 +++++++-
 unpack-trees.c   | 28 ++++++++++++++++++++++++++++
 unpack-trees.h   |  1 +
 6 files changed, 66 insertions(+), 1 deletion(-)

builtin/clone.c +1 -0

index 5c439f1394..0702b0e9d0 100644
@@ -747,6 +747,7 @@ static int checkout(int submodule_progress)
 	memset(&opts, 0, sizeof opts);
 	opts.update = 1;
 	opts.merge = 1;
+	opts.clone = 1;
 	opts.fn = oneway_merge;
 	opts.verbose_update = (option_verbosity >= 0);
 	opts.src_index = &the_index;

cache.h +1 -0

index 8b447652a7..6d6138f4f1 100644
@@ -1455,6 +1455,7 @@ struct checkout {
 	unsigned force:1,
 		 quiet:1,
 		 not_new:1,
+		 clone:1,
 		 refresh_cache:1;
 };
 #define CHECKOUT_INIT { NULL, "" }

entry.c +7 -0

index b5d1d3cf23..c70340df8e 100644
@@ -399,6 +399,31 @@ static int check_path(const char *path, int len, struct stat *st, int skiplen)
 	return lstat(path, st);
 }
 
+static void mark_colliding_entries(const struct checkout *state,
+				   struct cache_entry *ce, struct stat *st)
+{
+	int i;
+
+	ce->ce_flags |= CE_MATCHED;
+

entry.c static int check_path(const char *path, int len, struct stat *st, int skiplen) · line 418

Quoted 10 lines

entry.c +10 -0

+#if !defined(GIT_WINDOWS_NATIVE) /* inode is always zero on Windows */
+     for (i = 0; i < state->istate->cache_nr; i++) {
+             struct cache_entry *dup = state->istate->cache[i];
+
+             if (dup == ce)
+                     break;
+
+             if (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE))
+                     continue;
+

Should the following be protected by core.checkstat ?

        if (check_stat) {

entry.c static int check_path(const char *path, int len, struct stat *st, int skiplen) · line 424

Quoted 6 lines

entry.c +6 -0

+             if (dup->ce_stat_data.sd_ino == st->st_ino) {
+                     dup->ce_flags |= CE_MATCHED;
+                     break;
+             }
+     }
+#endif

Another thing is that we switch of the ASCII case-folding-detection-logic off for Windows users, even if we otherwise rely on icase.
I think we can use fspathcmp() as a fallback. when inodes fail, because we may be on a network file system.

(I don't have a test setup at the moment, but what happens with inodes when a Windows machine exports a share to Linux or Mac ?)

Is there a chance to get the fspathcmp() back, like this ?

static void mark_colliding_entries(const struct checkout *state,
struct cache_entry *ce, struct stat *st)

{
        int i;
        ce->ce_flags |= CE_MATCHED;

        for (i = 0; i < state->istate->cache_nr; i++) {
                struct cache_entry *dup = state->istate->cache[i];
                int folded = 0;

                if (dup == ce)
                        break;

                if (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE))
                        continue;

                if (!fspathcmp(dup->name, ce->name))
                        folded = 1;

#if !defined(GIT_WINDOWS_NATIVE) /* inode is always zero on Windows */
                if (check_stat && (dup->ce_stat_data.sd_ino == st->st_ino))
                        folded = 1;
#endif
                if (folded) {
                        dup->ce_flags |= CE_MATCHED;
                        break;
                }
        }
}

The description of this key does not really tell what 'minimal' mode checks exactly. More information about this mode can be found in the commit message of c08e4d5b5c (Enable minimal stat checking - 2013-01-22).

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 Documentation/config.txt | 2 ++
 1 file changed, 2 insertions(+)

Documentation/config.txt +2 -0

index fd8d27e761..5c41314dd5 100644
@@ -466,6 +466,8 @@ core.checkStat::
 	and work tree. The user can set this to 'default' or
 	'minimal'. Default (or explicitly 'default'), is to check
 	all fields, including the sub-second part of mtime and ctime.
+	'minimal' only checks size and the whole second part of mtime
+	and ctime.
 
 core.quotePath::
 	Commands that output paths (e.g. 'ls-files', 'diff'), will

Quoted 5 lines · Nguyễn Thái Ngọc Duy

The description of this key does not really tell what 'minimal' mode checks exactly. More information about this mode can be found in the commit message of c08e4d5b5c (Enable minimal stat checking - 2013-01-22).

While I agree that we need to do _something_, I am not sure if this change adds sufficient value. I _think_ those who wonder if they want to configure this want to know what are _not_ looked at (relative to the "default") more than what are _still_ looked at, partly because the description of "default" is already bogus and says "check all fields", which is horrible for two reasons. It is unclear what are in "all" fields in the first place, and also we do not look at all fields (e.g. we do not look at atime for obvious reasons).

So perhaps

When this configuration variable is missing or is set to default, many fields in the stat structure are checked to detect if a file has been modified since Git looked at it. Among these fields, when this configuration variable is set to minimal, sub-second part of mtime and ctime, the uid and gid of the owner of the file, the inode number (and the device number, if Git was compiled to use it), are excluded from the check, leaving only the whole-second part of mtime (and ctime, if core.trustCtime is set) and the filesize to be checked.

or something?

Quoted 32 lines · Junio C Hamano

Quoted 5 lines · Nguyễn Thái Ngọc Duy

The description of this key does not really tell what 'minimal' mode checks exactly. More information about this mode can be found in the commit message of c08e4d5b5c (Enable minimal stat checking - 2013-01-22).

While I agree that we need to do _something_, I am not sure if this change adds sufficient value. I _think_ those who wonder if they want to configure this want to know what are _not_ looked at (relative to the "default") more than what are _still_ looked at, partly because the description of "default" is already bogus and says "check all fields", which is horrible for two reasons. It is unclear what are in "all" fields in the first place, and also we do not look at all fields (e.g. we do not look at atime for obvious reasons).

So perhaps

When this configuration variable is missing or is set to default, many fields in the stat structure are checked to detect if a file has been modified since Git looked at it. Among these fields, when this configuration variable is set to minimal, sub-second part of mtime and ctime, the uid and gid of the owner of the file, the inode number (and the device number, if Git was compiled to use it), are excluded from the check, leaving only the whole-second part of mtime (and ctime, if core.trustCtime is set) and the filesize to be checked.

Quoted 1 line

or something?

Perfect. I could wrap it in a patch, but I feel you should take authorship for that one. I'll leave it to you to create this commit.

Perfect. I could wrap it in a patch, but I feel you should take authorship for that one. I'll leave it to you to create this commit.

-- >8 --
Subject: [PATCH] config.txt: clarify core.checkStat

The description of this key does not really tell what the 'minimal' mode checks and does not check. The description for the 'default' mode is not much better and just says 'all fields', which is unclear and is not even correct (e.g. we do not look at 'atime').

Spell out what are and what are not checked under the 'minimal' mode relative to the 'default' mode to help those who want to decide if they want to use the 'minimal' mode, also taking information about this mode from the commit message of c08e4d5b5c (Enable minimal stat checking - 2013-01-22).

Helped-by Nguyễn Thái Ngọc Duy <[email protected]>

Signed-off-by Junio C Hamano <[email protected]>

---
 Documentation/config.txt | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

Documentation/config.txt +14 -5

index ab641bf5a9..933d719137 100644
@@ -449,10 +449,20 @@ core.untrackedCache::
        See linkgit:git-update-index[1]. `keep` by default.
 
 core.checkStat::
-       Determines which stat fields to match between the index
-       and work tree. The user can set this to 'default' or
-       'minimal'. Default (or explicitly 'default'), is to check
-       all fields, including the sub-second part of mtime and ctime.
+       When missing or is set to `default`, many fields in the stat
+       structure are checked to detect if a file has been modified
+       since Git looked at it.  When this configuration variable is
+       set to `minimal`, sub-second part of mtime and ctime, the
+       uid and gid of the owner of the file, the inode number (and
+       the device number, if Git was compiled to use it), are
+       excluded from the check among these fields, leaving only the
+       whole-second part of mtime (and ctime, if `core.trustCtime`
+       is set) and the filesize to be checked.
++
+There are implementations of Git that do not leave usable values in
+some fields (e.g. JGit); by excluding these fields from the
+comparison, the `minimal` mode may help interoperability when the
+same repository is used by these other systems at the same time.
 
 core.quotePath::
        Commands that output paths (e.g. 'ls-files', 'diff'), will
--

2.18.0-666-g63749b2dea

entry.c static int check_path(const char *path, int len, struct stat *st, int skiplen) · line 418

Quoted 11 lines

entry.c +11 -0

+
+#if !defined(GIT_WINDOWS_NATIVE) /* inode is always zero on Windows */
+	for (i = 0; i < state->istate->cache_nr; i++) {
+		struct cache_entry *dup = state->istate->cache[i];
+
+		if (dup == ce)
+			break;
+
+		if (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE))
+			continue;
+

Should the following be protected by core.checkstat ?

	if (check_stat) {

entry.c static int check_path(const char *path, int len, struct stat *st, int skiplen) · line 424

Quoted 6 lines

entry.c +6 -0

+		if (dup->ce_stat_data.sd_ino == st->st_ino) {
+			dup->ce_flags |= CE_MATCHED;
+			break;
+		}
+	}
+#endif

Another thing is that we switch of the ASCII case-folding-detection-logic off for Windows users, even if we otherwise rely on icase.
I think we can use fspathcmp() as a fallback. when inodes fail, because we may be on a network file system.

(I don't have a test setup at the moment, but what happens with inodes when a Windows machine exports a share to Linux or Mac ?)

Is there a chance to get the fspathcmp() back, like this ?

static void mark_colliding_entries(const struct checkout *state,
struct cache_entry *ce, struct stat *st)

{
	int i;
	ce->ce_flags |= CE_MATCHED;

	for (i = 0; i < state->istate->cache_nr; i++) {
		struct cache_entry *dup = state->istate->cache[i];
		int folded = 0;

		if (dup == ce)
			break;

		if (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE))
			continue;

		if (!fspathcmp(dup->name, ce->name))
			folded = 1;

#if !defined(GIT_WINDOWS_NATIVE) /* inode is always zero on Windows */
		if (check_stat && (dup->ce_stat_data.sd_ino == st->st_ino))
			folded = 1;
#endif
		if (folded) {
			dup->ce_flags |= CE_MATCHED;
			break;
		}
	}
}

Torsten Bögershausen <[email protected]> writes:

Quoted 2 lines

Should the following be protected by core.checkstat ?

	if (check_stat) {

I do not think such a if statement is strictly necessary.

Even if check_stat tells us "when checking if a cached stat information tells us that the path may have modified, use minimum set of fields from the 'struct stat'", we still capture and update the values from the same "full" set of fields when we mark a cache entry up-to-date. So it all depends on why you are limiting with check_stat. Is it because stdev is unusable? Is it because nsec is unusable? Is it because ino is unusable? Only in the last case, paying attention to check_stat will reduce the false positive.

But then you made me wonder what value check_stat has on Windows. If it is false, perhaps we do not even need the conditional compilation, which is a huge plus.

Quoted 13 lines

entry.c static int check_path(const char *path, int len, struct stat *st, int skiplen) · line 424

Quoted 7 lines

entry.c +6 -0

+		if (dup->ce_stat_data.sd_ino == st->st_ino) {
+			dup->ce_flags |= CE_MATCHED;
+			break;
+		}
+	}
+#endif

Another thing is that we switch of the ASCII case-folding-detection-logic off for Windows users, even if we otherwise rely on icase.
I think we can use fspathcmp() as a fallback. when inodes fail, because we may be on a network file system.

Quoted 4 lines

(I don't have a test setup at the moment, but what happens with inodes when a Windows machine exports a share to Linux or Mac ?)

Is there a chance to get the fspathcmp() back, like this ?

If fspathcmp() never gives false positives, I do not think we would mind using it like your update. False negatives are fine, as that is better than just punting the whole thing when there is no usable inum. And we do not care all that much if it is more expensive; this is an error codepath after all.

And from code structure's point of view, I think it makes sense. It would be even better if we can lose the conditional compilation.

Another thing we maybe want to see is if we can update the caller of this function so that we do not overwrite the earlier checkout with the data for this path. When two paths collide, we check out one of the paths without reporting (because we cannot notice), then attempt to check out the other path and report (because we do notice the previous one with lstat()). The current code then goes on and overwrites the file with the contents from the "other" path.

Even if we had false negative in this loop, if we leave the contents for the earlier path while reporting the "other" path, then the user can get curious, inspect what contents the "other" path has on the filesystem, and can notice that it belongs to the (unreported--due to false negative) earlier path.

check_stat is 0 on Windows,

Now back to the compiler switch:
Windows always set inum to 0 and I can't think about a situation where a file in a working tree gets inum = 0, can we use the following:

check_stat is 0 on Windows, and inum is allways 0 in lstat(). I was thinking about systems which don't have inodes and inum, and then generate an inum in memory, sometimes random. After a reboot or a re-mount of the file systems those ino values change.
However, for the initial clone we are fine in any case.

Now back to the compiler switch:
Windows always set inum to 0 and I can't think about a situation where a file in a working tree gets inum = 0, can we use the following:

static void mark_colliding_entries(const struct checkout *state,
struct cache_entry *ce, struct stat *st)

{
	int i;
	ce->ce_flags |= CE_MATCHED;

	for (i = 0; i < state->istate->cache_nr; i++) {
		struct cache_entry *dup = state->istate->cache[i];
		int folded = 0;

		if (dup == ce)
			break;

		if (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE))
			continue;
		/*
		 * Windows sets ino to 0. On other FS ino = 0 will already be
		 *  used, so we don't see it for a file in a Git working tree
		 */
		if (st->st_ino && (dup->ce_stat_data.sd_ino == st->st_ino))
			folded = 1;

		/*
		 * Fallback for NTFS and other case insenstive FS,
		 * which don't use POSIX inums
		 */
		if (!fspathcmp(dup->name, ce->name))
			folded = 1;

		if (folded) {
			dup->ce_flags |= CE_MATCHED;
			break;
		}
	}
}

Quoted 15 lines

Another thing we maybe want to see is if we can update the caller of this function so that we do not overwrite the earlier checkout with the data for this path. When two paths collide, we check out one of the paths without reporting (because we cannot notice), then attempt to check out the other path and report (because we do notice the previous one with lstat()). The current code then goes on and overwrites the file with the contents from the "other" path.

Even if we had false negative in this loop, if we leave the contents for the earlier path while reporting the "other" path, then the user can get curious, inspect what contents the "other" path has on the filesystem, and can notice that it belongs to the (unreported--due to false negative) earlier path.

[snip]

I still don't trust magic st_ino zero, or core.checkStat being zero on Windows, so the #if condition still remains but it covers smallest area possible and I tested it by manually make it "#if 1"

The fallback with fspathcmp() is only done when inode can't be trusted because strcmp is more expensive and when fspathcmp() learns more about real world in the future, it could become even more expensive.

The output sorting is the result of Sublime-Gitignore repo being reported recently. It's not perfect but it should help seeing the groups in normal case.

unpack-trees.c +17 -0

+
+	if (o->clone) {
+		struct string_list list = STRING_LIST_INIT_NODUP;
+		int i;
+
+		for (i = 0; i < index->cache_nr; i++) {
+			struct cache_entry *ce = index->cache[i];
+
+			if (!(ce->ce_flags & CE_MATCHED))
+				continue;
+
+			string_list_append(&list, ce->name);
+			ce->ce_flags &= ~CE_MATCHED;
+		}
+
+		list.cmp = fspathcmp;
+		string_list_sort(&list);

I highly suspect that the above was written in that way to reduce the indentation level, but the right way to reduce the indentation level, if it bothers readers too much, is to make the whole thing inside the above if (o->clone) into a dedicated helper function "void report_collided_checkout(void)", I would think.

Paths that only differ in case work fine in a case-sensitive filesystems, but if those repos are cloned in a case-insensitive one, you'll get problems. The first thing to notice is "git status" will never be clean with no indication what exactly is "dirty".

This patch helps the situation a bit by pointing out the problem at clone time. Even though this patch talks about case sensitivity, the patch makes no assumption about folding rules by the filesystem. It simply observes that if an entry has been already checked out at clone time when we're about to write a new path, some folding rules are behind this.

In the case that we can't rely on filesystem (via inode number) to do this check, fall back to fspathcmp() which is not perfect but should not give false positives.

This patch is tested with vim-colorschemes and Sublime-Gitignore repositories on a JFS partition with case insensitive support on Linux.

'

+test_expect_success !MINGW,!CYGWIN,CASE_INSENSITIVE_FS 'colliding file detection' '

While I don't have an HFS+ volume to test, I suspect this patch should be needed for both, even if I have to say thay even the broken output was better than the current state.

Travis seems to be using a case sensitive filesystem so wouldn't catch this.

Was windows/cygwin tested?

Carlo
-- >8 --
Subject: [PATCH] entry: fix t5061 on macOS

b878579ae7 ("clone: report duplicate entries on case-insensitive filesystems", 2018-08-17) was tested on Linux with an excemption for Windows that needs to be expanded for macOS (using APFS), which then would show :

$ git clone git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git
warning: the following paths have collided (e.g. case-sensitive paths
on a case-insensitive filesystem) and only one from the same
colliding group is in the working tree:

'man2/_Exit.2'
'man2/_exit.2'
'man3/NAN.3'
'man3/nan.3'

Signed-off-by Carlo Marcelo Arenas Belón <[email protected]>

---
 entry.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

entry.c +0 -0

index 5d136c5d55..3845f570f7 100644

Did you test it on Mac ?

So what exactly are you trying to fix ?

Quoted 2 lines · Torsten Bögershausen

Did you test it on Mac ?

macOS 10.14.1 but only using APFS, did you test my patch with HFS+?

Quoted 1 line

So what exactly are you trying to fix ?

I get

not ok 99 - colliding file detection
#

# grep X icasefs/warning &&
# grep x icasefs/warning &&
# test_i18ngrep "the following paths have collided" icasefs/warning
#

and the output of "warning" only shows one of the conflicting files, instead of both:

Cloning into 'bogus'...
done.
warning: the following paths have collided (e.g. case-sensitive paths on a case-insensitive filesystem) and only one from the same colliding group is in the working tree:

'x'

Carlo

Could you send me the "index" file in t/trash\
directory.t5601-clone/icasefs/bogus/.git/index ? Also the output of "stat /path/to/icase/bogus/x"

My only explanation is somehow the inode value we save is not the same one on disk, which is weird and could even cause other problems. I'd like to know why this happens before trying to fix anything.

First of all, Ramsay, it would be great if you could test the below patch and see if it works on Cygwin. I assume since Cygwin shares the underlying filesystem, it will share the same "no trusting inode" issue with native builds (or it calculates inodes anyway using some other source?).

Back to the APFS problem...

Quoted 7 lines · Duy Nguyen

Could you send me the "index" file in t/trash\
directory.t5601-clone/icasefs/bogus/.git/index ? Also the output of "stat /path/to/icase/bogus/x"

My only explanation is somehow the inode value we save is not the same one on disk, which is weird and could even cause other problems. I'd like to know why this happens before trying to fix anything.

Thanks Carlo for the file and "stat" output. The problem is APFS has 64-bit inode (according to the Internet) while we store inodes as 32-bit, so it's truncated. Which means this comparison

    sd_ino == st_ino

is never true because sd_ino is truncated (0x2121063) while st_ino is not (0x202121063).

Carlo, it would be great if you could test this patch also with APFS. It should fix problem. We will have to deal with the same truncated inode elsewhere to make sure we index refresh performance does not degrade on APFS. But that's a separate problem. Thank you for bringing this up.

-- 8< --

entry.c +18 -4

index 5d136c5d55..809d3e2ba7 100644
@@ -404,13 +404,13 @@ static void mark_colliding_entries(const struct checkout *state,
 {
        int i, trust_ino = check_stat;
 
-#if defined(GIT_WINDOWS_NATIVE)
+#if defined(GIT_WINDOWS_NATIVE) || defined(__CYGWIN__)
        trust_ino = 0;
 #endif
 
        ce->ce_flags |= CE_MATCHED;
 
-       for (i = 0; i < state->istate->cache_nr; i++) {
+       for (i = 0; i < trust_ino && state->istate->cache_nr; i++) {
                struct cache_entry *dup = state->istate->cache[i];
 
                if (dup == ce)

@@ -419,10 +419,24 @@ static void mark_colliding_entries(const struct checkout *state,
                if (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE))
                        continue;
 
-               if ((trust_ino && dup->ce_stat_data.sd_ino == st->st_ino) ||
-                   (!trust_ino && !fspathcmp(ce->name, dup->name))) {
+               if (dup->ce_stat_data.sd_ino == (unsigned int)st->st_ino) {
                        dup->ce_flags |= CE_MATCHED;
+                       return;
+               }
+       }
+
+       for (i = 0; i < state->istate->cache_nr; i++) {
+               struct cache_entry *dup = state->istate->cache[i];
+
+               if (dup == ce)
                        break;
+
+               if (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE))
+                       continue;
+
+               if (!fspathcmp(ce->name, dup->name)) {
+                       dup->ce_flags |= CE_MATCHED;
+                       return;
                }
        }
 }

t/t5601-clone.sh +1 -2

index f1a49e94f5..c28d51bd59 100755
@@ -628,7 +628,7 @@ test_expect_success 'clone on case-insensitive fs' '
        )
 '
 
-test_expect_success !MINGW,!CYGWIN,CASE_INSENSITIVE_FS 'colliding file detection' '
+test_expect_success !MINGW,CASE_INSENSITIVE_FS 'colliding file detection' '
        grep X icasefs/warning &&
        grep x icasefs/warning &&
        test_i18ngrep "the following paths have collided" icasefs/warning
-- 8< --

Thanks Carlo for the file and "stat" output. The problem is APFS has 64-bit inode (according to the Internet) while we store inodes as 32-bit, so it's truncated.
...
We will have to deal with the same
truncated inode elsewhere to make sure we index refresh performance does not degrade on APFS.

First of all, Ramsay, it would be great if you could test the below patch and see if it works on Cygwin. I assume since Cygwin shares the underlying filesystem, it will share the same "no trusting inode" issue with native builds (or it calculates inodes anyway using some other source?).

Quoted 5 lines · Duy Nguyen

First of all, Ramsay, it would be great if you could test the below patch and see if it works on Cygwin. I assume since Cygwin shares the underlying filesystem, it will share the same "no trusting inode" issue with native builds (or it calculates inodes anyway using some other source?).

Hmm, I have no idea why you would like me to try this patch - care to explain? [I just saw, "Has this been tested on cygwin?" and, since it has been happily passing for some time, responded yes!]

Just for the giggles, I removed the !CYGWIN prerequisite from the test and when, as expected, the test failed, had a look around:

$ pwd
/home/ramsay/git/t/trash directory.t5601-clone
$ cat icasefs/warning 
Cloning into 'bogus'...
done.
warning: the following paths have collided (e.g. case-sensitive paths
on a case-insensitive filesystem) and only one from the same
colliding group is in the working tree:

'x'

$ cd icasefs/bogus
$ ls -l
total 0
-rw-r--r-- 1 ramsay None 0 Nov 19 22:40 x
$ git ls-files --debug
ignoring EOIE extension
X
  ctime: 1542667201:664036600
  mtime: 1542667201:663055400
  dev: 2378432	ino: 324352
  uid: 1001	gid: 513
  size: 0	flags: 0
x
  ctime: 1542667201:665026800
  mtime: 1542667201:665026800
  dev: 2378432	ino: 324352
  uid: 1001	gid: 513
  size: 0	flags: 0
$ 

So, both X and x are in the index with the same inode number.

Does that help?

entry.c +3 -0

 			dup->ce_flags |= CE_MATCHED;
+			return;
+		}
+	}

Commit b878579ae7 (clone: report duplicate entries on case-insensitive filesystems - 2018-08-17) adds a warning to user when cloning a repo with case-sensitive file names on a case-insensitive file system. The "find duplicate file" check was doing by comparing inode number (and only fall back to fspathcmp() when inode is known to be unreliable because fspathcmp() can't cover all case folding cases).

The inode check is very simple, and wrong. It compares between a 32-bit number (sd_ino) and potentially a 64-bit number (st_ino). When an inode is larger than 2^32 (which seems to be the case for APFS), it will be truncated and stored in sd_ino, but comparing with itself will fail.

As a result, instead of showing a pair of files that have the same name, we show just one file (marked before the beginning of the loop). We fail to find the original one.

The fix could be just a simple type cast (*)

    dup->ce_stat_data.sd_ino == (unsigned int)st->st_ino

but this is no longer a reliable test, there are 4G possible inodes that can match sd_ino because we only match the lower 32 bits instead of full 64 bits.

There are two options to go. Either we ignore inode and go with fspathcmp() on Apple platform. This means we can't do accurate inode check on HFS anymore, or even on APFS when inode numbers are still below 2^32.

Or we just to to reduce the odds of matching a wrong file by checking more attributes, counting mostly on st_size because st_xtime is likely the same. This patch goes with this direction, hoping that false positive chances are too small to be seen in practice.

While at there, enable the test on Cygwin (verified working by Ramsay Jones)

(*) this is also already done inside match_stat_data()

Reported-by Carlo Arenas <[email protected]>

Helped-by Ramsay Jones <[email protected]>

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
So I'm going with match_stat_data(). But I don't know, perhaps just ignoring inode (like Carlo's original patch) is safer/better?

Tested on case-insensitive JFS on Linux. But I don't think it really matters because I'm not even sure if I could push inode above 2^32 with this. Hacking JFS for this test sounds fun, but no time for that.

 entry.c          | 4 ++--
 t/t5601-clone.sh | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

entry.c +0 -0

index 5d136c5d55..0a3c451f5f 100644

entry.c +1 -1

 	trust_ino = 0;
 #endif
 

@@ -419,7 +419,7 @@ static void mark_colliding_entries(const struct checkout *state,
 		if (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE))
 			continue;
 
-		if ((trust_ino && dup->ce_stat_data.sd_ino == st->st_ino) ||
+		if ((trust_ino && !match_stat_data(&dup->ce_stat_data, st)) ||
 		    (!trust_ino && !fspathcmp(ce->name, dup->name))) {
 			dup->ce_flags |= CE_MATCHED;
 			break;

t/t5601-clone.sh +1 -1

index f1a49e94f5..c28d51bd59 100755
@@ -628,7 +628,7 @@ test_expect_success 'clone on case-insensitive fs' '
 	)
 '
 
-test_expect_success !MINGW,!CYGWIN,CASE_INSENSITIVE_FS 'colliding file detection' '
+test_expect_success !MINGW,CASE_INSENSITIVE_FS 'colliding file detection' '
 	grep X icasefs/warning &&
 	grep x icasefs/warning &&
 	test_i18ngrep "the following paths have collided" icasefs/warning

IMHO it would be ideal if test would be enabled/validated for windows (native, not only cygwin) as it might even work without the override and if we are to see conflicts, that is probably where most users with file insensitive filesystems might be found

Subject: [PATCH] entry: remove windows fallback to inode checking

this test is really FS specific, so is better to avoid any compiled assumptions about the platform and let the user drive the fallback through core.checkStat instead

Signed-off-by Carlo Marcelo Arenas Belón <[email protected]>

---
 entry.c | 4 ----
 1 file changed, 4 deletions(-)

entry.c +0 -0

index 0a3c451f5f..5ae74856e6 100644

entry.c +0 -0

 	ce->ce_flags |= CE_MATCHED;
 
 	for (i = 0; i < state->istate->cache_nr; i++) {

Signature · 1 line

2.20.0.rc1

Quoted 37 lines · Carlo Marcelo Arenas Belón

While I don't have an HFS+ volume to test, I suspect this patch should be needed for both, even if I have to say thay even the broken output was better than the current state.

Travis seems to be using a case sensitive filesystem so wouldn't catch this.

Was windows/cygwin tested?

Carlo
-- >8 --
Subject: [PATCH] entry: fix t5061 on macOS

b878579ae7 ("clone: report duplicate entries on case-insensitive filesystems", 2018-08-17) was tested on Linux with an excemption for Windows that needs to be expanded for macOS (using APFS), which then would show :

$ git clone git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git
warning: the following paths have collided (e.g. case-sensitive paths
on a case-insensitive filesystem) and only one from the same
colliding group is in the working tree:

'man2/_Exit.2'
'man2/_exit.2'
'man3/NAN.3'
'man3/nan.3'

Signed-off-by Carlo Marcelo Arenas Belón <[email protected]>

---
 entry.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

entry.c +0 -0

index 5d136c5d55..3845f570f7 100644

entry.c static void mark_colliding_entries(const struct checkout *state, · line 410

Quoted 8 lines

entry.c +1 -1

@@ -404,7 +404,7 @@ static void mark_colliding_entries(const struct checkout *state,
 {
 	int i, trust_ino = check_stat;
 
-#if defined(GIT_WINDOWS_NATIVE)
+#if defined(GIT_WINDOWS_NATIVE) || defined(__APPLE__)
 	trust_ino = 0;
 #endif
 

Sorry,
but I can't reproduce your problem here.

Did you test it on Mac ?
If I run t5601 on a case sensitive files system
(Mac, mounted NFS, exported from Linux)
I get:
ok 99 # skip colliding file detection (missing CASE_INSENSITIVE_FS of !MINGW,!CYGWIN,CASE_INSENSITIVE_FS)

ok 99 # skip colliding file detection (missing !CYGWIN of !MINGW,!CYGWIN,CASE_INSENSITIVE_FS)

Quoted 6 lines · Duy Nguyen

Quoted 2 lines

strcasecmp() will only catch a subset of the cases. We really need to follow the same folding rules that the filesystem would.

True. But that's how we handle case insensitivity internally. If a filesytem has more sophisticated folding rules then git will not work well on that one anyway.

Hrm. Yeah, I guess that's the best we can do for the actual in-memory checks. Everything else depends on doing an actual filesystem operation, and our icase stuff kicks in way before then. I was mostly thinking of HFS+ utf8 normalization weirdness, but I guess people are accustomed to that by now.

Quoted 11 lines

Quoted 8 lines

For the case of clone, I actually wonder if we could detect during the checkout step that a file already exists. Since we know that the directory we started with was empty, then if it does, either:

- there's some funny case-folding going on that means two paths in the repository map to the same name in the filesystem; or

- somebody else is writing to the directory at the same time as us

This is exactly what my first patch does (minus the sparse checkout part).

Right, sorry, I should have read that one more carefully.

Quoted 7 lines

But without knowing the exact folding rules, I don't think we can locate this "somebody else" who wrote the first path. So if N paths are treated the same by this filesystem, we could only report N-1 of them.

If we want to report just one path when this happens though, then this works quite well.

Hmm. Since most such systems are case-preserving, would it be possible to report the name of the existing file? Doing it via opendir/readdir is hacky, and anyway puts the burden on us to find the matching name. Doing it via fstat() on the opened file doesn't work because at that the filesystem has resolved the name to an inode.

So yeah, perhaps strcasecmp() is the best we can do (I do agree that being able to mention all of the conflicting names is a benefit).

I guess we should be using fspathcmp(), though, in case it later learns to be smarter.

-Peff