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

From: Jeff King <peff@peff.net>
Date: Jan 5, 2013, 23:03 UTC
Message-ID: <20130105230303.GA5195@sigill.intra.peff.net>
In-Reply-To: <1357418563-6626-1-git-send-email-apelisse@gmail.com>

On Sat, Jan 05, 2013 at 09:42:43PM +0100, Antoine Pelisse wrote:

Show 8 quoted lines

> Tracked directories (i.e. directories containing tracked files) that
> are ignored must be reported as ignored if they contain untracked files.
> 
> Currently, tracked files or directories can't be reported untracked or ignored.
> Remove that constraint when searching ignored files.
> 
> Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
> ---

I was expecting to see some explanation of the user-visible bug here. In other words, what does this fix, and why does the bug only happen when core.ignorecase is set.

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.

But that makes me wonder. In the ignorecase=false case, I claimed that the check in dir_add_name is a no-op for mixed tracked/ignored directories. But it is presumably not a no-op for other cases. Your patch only turns it off when DIR_SHOW_IGNORED is set. But is it possible for us to have DIR_SHOW_IGNORED set, _and_ to pass in a path that exists in the index as a regular file?

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?

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.

-Peff