On Thu, 9 Jul 2009, Junio C Hamano wrote: > > + > > + /* 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. > > + 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 -- 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