Re: [PATCH v4] clone: report duplicate entries on case-insensitive filesystems
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 15, 2018, 19:38 UTC
- Message-ID
- <xmqqtvnvh12u.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20180815190816.GA26521@tor.lan>
Torsten Bögershausen <tboegi@web.de> writes:
Show 14 quoted lines
>> +
>> +#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.
Show 16 quoted lines
>> + 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.
Show 29 quoted lines
> 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;
> }
> }
> }