Re: [PATCH] status: report ignored yet tracked directories

[Jeff King <peff@peff.net>]

On Sun, Jan 06, 2013 at 05:40:46PM +0100, Antoine Pelisse wrote:

> > Looking at your fix and remembering how the index hashing works, I think
> > the answer is that:
> >
> >   1. This bug only affects directories, because they are the only thing
> >      that can be simultaneously "ignored and untracked" and "tracked"
> >      (i.e., they have entries of both, and we are using
> >      DIR_SHOW_OTHER_DIRECTORIES).
> >
> >   2. When core.ignorecase is false, the index name hash contains only
> >      the file entries, and cache_name_exists returns an exact match. So
> >      it doesn't matter if we make an extra check when adding the
> >      directory via dir_add_name; we know that it will not be there, and
> >      the final check is a no-op.
> >
> >   3. When core.ignorecase is true, we also store directory entries in
> >      the index name hash, and this extra check is harmful; the entry
> >      does not really exist in the index, and we still need to add it.
> 
> Yes, because of this couple of lines I guess (name-hash.c, hash_index_entry()):
> 
>   if (ignore_case)
>     hash_index_entry_directories(istate, ce);

Exactly. I couldn't remember at first why this was the case, but after
reading 5102c61 (Add case insensitivity support for directories when
using git status, 2010-10-03) again, I think it is because we cannot do
a partial-name lookup via the hash (i.e., the hash for "foo/" and
"foo/bar" have no relation to each other). Not related to your patch,
obviously, but it was the missing piece for me to understand why the
code was doing what it does.

> > I think in the normal file case, we'd expect treat_path to just tell us
> > that it is handled, and we would not ever call dir_add_name in the first
> > place. But what if we have an index entry for a file, but the working
> > tree now contains a directory?
> 
> The directory is treated as any other untracked directory (it never
> matches indexed file because of the trailing /).

Ah, right. That makes sense.

> > I _think_ we still do not hit this code path in that instance, because
> > we will end up in treat_directory, and we will end up checking
> > directory_exists_in_index. And I cannot get it to misbehave in practice.
> > So I think your fix is correct, but the exact how and why is a bit
> > subtle.
> 
> Thanks a lot for the help, I will try to come up with a better commit
> message now.

Thanks. I think the patch is right, but the reasoning is just a bit
subtle.

-Peff