Re: performance problem: "git commit filename"

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Jan 15, 2008, 00:18 UTC
Message-ID: <alpine.LFD.1.00.0801141611560.2806@woody.linux-foundation.org>
In-Reply-To: <7vr6glnrvp.fsf@gitster.siamese.dyndns.org>

On Sun, 13 Jan 2008, Junio C Hamano wrote:

> 
> I've reworked the patch, and in the kernel repository, a
> single-path commit after touching that path now calls 23k
> lstat(2).  It used to call 46k lstat(2) after your fix.

Hmm. This part of it looks incorrect:

Show 15 quoted lines

> diff --git a/diff.c b/diff.c
> index b18c140..62d0c06 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1510,6 +1510,10 @@ static int reuse_worktree_file(const char *name, const unsigned char *sha1, int
>  	if (pos < 0)
>  		return 0;
>  	ce = active_cache[pos];
> +
> +	if (ce_uptodate(ce))
> +		return 1;
> +
>  	if ((lstat(name, &st) < 0) ||
>  	    !S_ISREG(st.st_mode) || /* careful! */
>  	    ce_match_stat(ce, &st, 0) ||

Isn't this wrong? I think it also needs to check that ce->sha1 matches the right SHA1, because even if the lstat() information may be fine, if the SHA1 doesn't match what we want, we still shouldn't use the checked-out copy, of course.

The old code continues with a

	   hashcmp(sha1, ce->sha1))
		return 0;

in that if-statement that is partially visible in the context, and it's that hashcmp() that got incorrectly cut off from the logic.

(Of course, maybe we never call this function unless we've already checked that the cache-entry SHA1 matches, but if so, that subsequent hashcmp should just be removed instead).

		Linus