Re: [PATCH] ls-files: fix overeager pathspec optimization

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Jan 8, 2010, 23:24 UTC
Message-ID: <alpine.LFD.2.00.1001081520240.7821@localhost.localdomain>
In-Reply-To: <7veim0w68q.fsf_-_@alter.siamese.dyndns.org>

On Fri, 8 Jan 2010, Junio C Hamano wrote:

Show 8 quoted lines

>
> Given pathspecs that share a common prefix, ls-files optimized its call
> into recursive directory reader by starting at the common prefix
> directory.
> 
> If you have a directory "t" with an untracked file "t/junk" in it, but the
> top-level .gitignore file told us to ignore "t/", this resulted in an
> unexpected behaviour:

Ok, I'm not sure how "unexpected" this is, since arguably you are overriding the ignore file by _being_ in that directory (the same way index contents override ignore files), but I could go either way on that.

Your patch looks fine, although I think you did this in a very odd way.

Show 11 quoted lines

> +	at = 0;
> +	memcpy(path, path_, len);
> +	while (1) {
> +		char *cp;
> +		path[at] = '\0';
> +		/*
> +		 * NOTE! NOTE! NOTE!: we might want to actually lstat(2)
> +		 * path[] to make sure it is a directory.
> +		 */
> +		if (excluded(dir, path, &dtype))
> +			return 1;

The above starts by testing the empty string, and then after that test it goes on to the next directory component. That is just _odd_.

Wouldn't it be more natural to write the loop the other way around, ie _first_ look up the next directory component, and _then_ do the exclude processing for thoose components?

Or is there some subtle reason I'm missing for actually checking the empty name?

				Linus