Re: [PATCH 3/3] Avoid doing extra 'lstat()'s for d_type if we have an up-to-date cache entry

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Jul 9, 2009, 16:59 UTC
Message-ID: <alpine.LFD.2.01.0907090954090.3352@localhost.localdomain>
In-Reply-To: <7vws6h3ji4.fsf@alter.siamese.dyndns.org>

On Thu, 9 Jul 2009, Junio C Hamano wrote:

Show 8 quoted lines

> > +
> > +	/* Try to look it up as a directory */
> > +	pos = cache_name_pos(path, len);
> > +	if (pos >= 0)
> > +		return 0;
> 
> How can this find an exact entry for the path?  Assuming that the name
> hash cache_name_exists() is not out of sync?

Hopefully it would never trigger. But I'd rather write robust code that doesn't make any fancy assumptions. Keep it simple - and keep it working even if surprising things happen.

Show 5 quoted lines

> > +		if (!ce_uptodate(ce))
> > +			break;	/* continue? */
> 
> I think this should be continue, as the directory D you are interested in
> may have two files, one modified, the other uptodate.

The thing is, the directory may have subdirectories, and there may be tens of thousands of files there. And maybe this gets called by code that hasn't done any cache preloading at all, so nothing will be up-to-date.

Do we want to loop over thousands of entries? Or do we want to loop as little as possible, and just say "most of the time the first entry will be representative".

But I did put the 'continue' in a comment, because it's not a correctness issue, it's a gut feel.

			Linus