On Thu, 2015-07-30 at 21:09 +0700, Duy Nguyen wrote:
> On Thu, Jul 30, 2015 at 9:32 AM, David Turner <dturner@xxxxxxxxxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html