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

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

quoted

+
+	/* 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. 

quoted

+		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