Re: Possible bug in git-completion.sh

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 8, 2010, 18:21 UTC
Message-ID: <7v8wc8jw3k.fsf@alter.siamese.dyndns.org>
In-Reply-To: <7vskag1r5o.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

Show 12 quoted lines

> Jeff King <peff@peff.net> writes:
>
>> Well, yes, if you wanted to compare apples to apples. But actually, my
>> point in showing "status" at all was to note that Michael's statement
>> that they would be the same is wrong.
>>
>> But just looking at the ls-files output, do you not agree that there is
>> a bug?
>
> If I agreed, I wouldn't have suggested _you_ to cd up and use pathspec,
> but instead would have suggested to patch ls-files to make it do so for
> you.

Nah, I should have checked the code.

The implementation of ls-files does cd up and uses pathspec, so the intent is to apply higher level gitignore.

It however feeds paths from the already ignored directories, which _is_ the real bug.

For example, if you have

    .gitignore (ignores t/)
    t/
    t/junk

in your work tree, it will read .gitignore at the top level, and eventually end up feeding "t/junk" to dir.c::excluded_1(), which does:

		for (i = el->nr - 1; 0 <= i; i--) {
			struct exclude *x = el->excludes[i];
			const char *exclude = x->pattern;
			int to_exclude = x->to_exclude;

			if (x->flags & EXC_FLAG_MUSTBEDIR) {
				if (*dtype == DT_UNKNOWN)
					*dtype = get_dtype(NULL, pathname, pathlen);
				if (*dtype != DT_DIR)
					continue;
			}
			...

where *x has "'t/' that must be directory". but the path "t/junk" does not match with "t/" and is not excluded by this rule; this part notices "t/junk" is not a directory, and continues without giving the later part a chance to intervene. Of course, the later part also is not prepared to do a "leading path" match, as the function is not meant to be fed entries from ignored "t/" directory in the first place.

I think a proper fix should be in dir.c::read_directory() where it calls read_directory_recursive() without first checking if the directory itself should be ignored. read_directory_recursive() itself has a logic to see if the dirent it found is worth recursing into and a similar logic should be in the toplevel caller.