Re: Confused about sparse vs untracked-cache

From: Duy Nguyen <pclouds@gmail.com>
Date: Jul 30, 2015, 14:09 UTC
Message-ID: <CACsJy8C_PUq4htCSF=qUJ0HNWf-E4RGXJ2FuzbRhSRhLN2vrRg@mail.gmail.com>
In-Reply-To: <1438223527.18134.53.camel@twopensource.com>

On Thu, Jul 30, 2015 at 9:32 AM, David Turner <dturner@twopensource.com> wrote:

Show 26 quoted lines

> I'm looking at dir.c, and there's a bit I'm confused about:
>
> prep_exclude() says:
>                      /*
>                       * .. and .gitignore does not exist before
>                       * (i.e. null exclude_sha1 and skip_worktree is
>                       * not set). Then we can skip loading .gitignore,
>                       * which would result in ENOENT anyway.
>                       * skip_worktree is taken care in read_directory()
>                       */
>                      !is_null_sha1(untracked->exclude_sha1))) {
>
> That "skip_worktree is taken care in read_directory()" appears to be
> referring to this bit of validate_untracked_cache():
>         /*
>          * An optimization in prep_exclude() does not play well with
>          * CE_SKIP_WORKTREE. It's a rare case anyway, if a single
>          * entry has that bit set, disable the whole untracked cache.
>          */
>         for (i = 0; i < active_nr; i++)
>                 if (ce_skip_worktree(active_cache[i]))
>                         return NULL;
> ------------
> I'm confused about why skip_worktree needs to be unset.  When I comment
> out the second snippet, all the tests still pass.  What was the reason
> behind that condition?  Is it really necessary?

This code is added in 27b099a (untracked cache: don't open non-existent .gitignore - 2015-03-08) so it's about non-existent .gitignore files. We have two cases: .gitignore does not exist, which we want to avoid opening it, and .gitignore does not exist, but it's in the index and is skip-worktree'd. We would want to call add_excludes() anyway in the second case.

I think I followed that train of thought when I wrote this and to avoid trouble, I just left skip-worktree case of out. But that ce_skip_worktree() check in read_directory() is probably too strong. All we need is disable the cache only when there's an .gitignore file being skip-worktree'd. If we do that and make sure all .gitignore files are not skip-worktree'd, the two can work toghether.

Back to the problem. The question is, is is_null_sha1(untracked->exclude_sha1) enough to satisfy both cases? If so, we don't have to disable the cache in the presence of skip-worktree. I haven't stared at this code again long enough to be confident, but I think we may be alright. exclude_sha1 should reflect the true, effective .gitignore content, wherever it's from. So in skip-worktree case, is_null_sha1(exclude_sha1) should only be true when the entry does not exist in both worktree and index. There will be an unnecessary open() in this case before the index version is used, but that's probably ok.

No I don't think the tests cover skip-worktree + untracked cache combination, so yeah it would pass.

-- 
Duy