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