Re: [PATCH v4] clone: report duplicate entries on case-insensitive filesystems

[Junio C Hamano <gitster@pobox.com>]

Torsten Bögershausen <tboegi@web.de> writes:

>> +
>> +#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) {

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.

>> +		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 ?

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.

> 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;
> 		}
> 	}
> }