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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 9, 2009, 16:32 UTC
Message-ID: <7vws6h3ji4.fsf@alter.siamese.dyndns.org>
In-Reply-To: <alpine.LFD.2.01.0907090832200.3352@localhost.localdomain>

Linus Torvalds <torvalds@linux-foundation.org> writes:

Show 14 quoted lines

> On Thu, 9 Jul 2009, Junio C Hamano wrote:
>> 
>> I was wondering if we could also say that D exists as a directory when we
>> know there is D/F in the index and is up to date.
>
> Yeah, that would probably be a good thing, but is slightly slower to look 
> up (we have the name hashing for the case-ignoring code anyway, but that 
> only works for exact names, so you can't look up directories that way).
>
> You'd have to use the regular binary search for that (or we'd have to 
> change it to hash directories too - which we might want to do for other 
> reasons, but don't do now).
>
> Something like this?

Yeah, in Dmitry's response that crossed with this update patch from you, he says lstat() on directories are still problem---it would be interesting to hear what he sees after applying this patch and retesting.

Show 9 quoted lines

> +static int get_index_mode(const char *path, int len)
> +{
> +	int pos;
> +	struct cache_entry *ce;
> +
> +	ce = cache_name_exists(path, len, 0);
> +	if (ce) {
> +		if (ce_uptodate(ce))
> +			return ce->ce_mode;

You return ce->ce_mode for up-to-date entries. I do not remember what ce_uptodate(ce) says for gitlinks, but ce->ce_mode for them would be 160000 that is not very kosher to give to S_ISDIR(). I realize that this worry actually applies to your patch from yesterday, the one Dmitry already tested.

Show 7 quoted lines

> +		return 0;
> +	}
> +
> +	/* 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? Shouldn't this be a BUG() instead of "It somehow exists as a blob or submodule, and we'll let the regular lstat() codepath take care of it by returning 0"?

Show 11 quoted lines

> +	pos = -pos-1;
> +	while (pos < active_nr) {
> +		ce = active_cache[pos++];
> +		if (strncmp(ce->name, path, len))
> +			break;
> +		if (ce->name[len] > '/')
> +			break;
> +		if (ce->name[len] < '/')
> +			continue;
> +		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.

Show 22 quoted lines

> +		return S_IFDIR;
> +	}
> +	return 0;
> +}
> +
>  static int get_dtype(struct dirent *de, const char *path, int len)
>  {
>  	int dtype = de ? DTYPE(de) : DT_UNKNOWN;
> -	struct cache_entry *ce;
>  	struct stat st;
>  
>  	if (dtype != DT_UNKNOWN)
>  		return dtype;
> -	ce = cache_name_exists(path, len, 0);
> -	if (ce && ce_uptodate(ce))
> -		st.st_mode = ce->ce_mode;
> -	else if (lstat(path, &st))
> +	st.st_mode = get_index_mode(path, len);
> +	if (!st.st_mode && lstat(path, &st))
>  		return dtype;
>  	if (S_ISREG(st.st_mode))
>  		return DT_REG;