Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> writes:
> 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 <pclouds@xxxxxxxxx>
> ---
> 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.
Ah, you no longer have that counter and the pointer to the counter,
as you do not even report how many paths collide ;-) Makes sense.
> diff --git a/entry.c b/entry.c
> index b5d1d3cf23..c70340df8e 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -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
> +}
OK. The whole loop might want to become a call to a helper function
whose implementation is platform dependent in the future, but that
should be kept outside the topic and left for a future enhancement.
> @@ -455,6 +480,9 @@ int checkout_entry(struct cache_entry *ce,
> return -1;
> }
>
> + if (state->clone)
> + mark_colliding_entries(state, ce, &st);
OK. I haven't carefully looked at the codepath but is it more
involved to instead *not* check out this ce (and leave the working
tree file that is already there for another path in the index
alone)? I suspect it won't be as simple as
if (state->clone) {
mark_colliding_entries(state, ce, &st);
return -1;
}
but I think it would give much more pleasant end-user experience if
we can do so, especially on GIT_WINDOWS_NATIVE. I would imagine
that the first thing those who see the message "foo.txt have
collided with something else we are not telling you" would want to
do is to see what "foo.txt" contains---and it may be obvious to
human that it contains the contents intended for "Foo.txt" instead,
if we somehow refrained from overwriting it here, which would
compensate for the lack of "this is the other path that collided
with your file."
It is perfectly an acceptable answer if it is "I looked at it, and
it is a lot more involved as there are these fallouts from the
codepaths that the control flows later from this point you haven't
checked and considered---let's keep overwriting, it is much safer."
> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
> index 0b62037744..f2eb73bc74 100755
> --- a/t/t5601-clone.sh
> +++ b/t/t5601-clone.sh
> @@ -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
> +'
> +
Ah, I was wondering why possible error message needs to be hidden in
the previous test---it is not hiding; it is capturing to look for
the paths in the message. Makes sense.
