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

From: Antoine Pelisse <apelisse@gmail.com>
Date: Jan 6, 2013, 16:40 UTC
Message-ID: <CALWbr2yRQai2Z08G2qFbA3AvsgivR-8kQ64SZ4pEktyrf+ZXiQ@mail.gmail.com>
In-Reply-To: <20130105230303.GA5195@sigill.intra.peff.net>

On Sun, Jan 6, 2013 at 12:03 AM, Jeff King <peff@peff.net> wrote:

Show 14 quoted lines

> On Sat, Jan 05, 2013 at 09:42:43PM +0100, Antoine Pelisse wrote:
>
>> 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.

I spent a couple of hours trying to understand that issue, and even if I ended-up with pretty much the same points as you do below, I was not confident enough to phrase it like you just did.

Show 17 quoted lines

> 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);

Show 6 quoted lines

> 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 don't think so, because of the optimization I added in my previous patch, in treat_file():

  /*
   * Optimization:
   * Don't spend time on indexed files, they won't be
   * added to the list anyway
   */
  struct cache_entry *ce = index_name_exists(&the_index,
    path->buf, path->len, ignore_case);

It's no longer an optimization but a required step, I will update the comment.

> 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 /).

Show 5 quoted lines

> 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.

> -Peff