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

[Duy Nguyen <pclouds@gmail.com>]

... and I "dear Ramsay" without CCing him.. sigh.. sorry for the noise.

On Mon, Nov 19, 2018 at 10:03 PM Duy Nguyen <pclouds@gmail.com> wrote:
>
> 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...
>
> On Mon, Nov 19, 2018 at 07:24:26PM +0100, Duy Nguyen wrote:
> > 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< --
> diff --git a/entry.c b/entry.c
> index 5d136c5d55..809d3e2ba7 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -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;
>                 }
>         }
>  }
> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
> index f1a49e94f5..c28d51bd59 100755
> --- a/t/t5601-clone.sh
> +++ b/t/t5601-clone.sh
> @@ -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< --



-- 
Duy