Re: Confused about sparse vs untracked-cache

[David Turner <dturner@twopensource.com>]

On Thu, 2015-07-30 at 21:09 +0700, Duy Nguyen wrote:
> On Thu, Jul 30, 2015 at 9:32 AM, David Turner <dturner@twopensource.com> wrote:
> > 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.

I went to add some tests for this, but I ran into what appears to be an
independent bug in the untracked cache code. 

The call to lookup_untracked in treat_directory() passes dirname, but
dirname is the full name of the directory.  lookup_untracked is looking
for the base name -- that is, the code needs to subtract out the length
of not just the parent dir, but all ancestor directories.  So it ends up
creating an entry named e.g. foo/bar inside foo (instead of just finding
an existing 'bar', or adding one called 'bar')

I wasn't really sure what the right way to fix this is, as I just
started looking into this code, but I thought I should report it.

My test case for this was having two levels of subdirectory e.g.
foo/bar/some-file.  

Let me know if you need more details.