Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> writes:
> 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:
>
>> 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?
You are right. We call this with sha1 that may not necessarily
be the same as ce->sha1.
The code should probably be something like:
ce = active_cache[pos];
/*
* Even if ce matches the work tree, it is not what we can
* reuse for sha1, if the hash is different or not a
* regular blob.
*/
if (hashcmp(sha1, ce->sha1) || !S_ISREG(ntohl(ce->st_mode))
return 0;
/*
* Does ce actually match the work tree? If so we can reuse.
*/
if (ce_uptodate(ce) ||
(!lstat(name, &st) && !ce_match_stat(ce, &st, 0)))
return 1;
return 0;
The expression inside the latter if () condition should probably
be the new abstraction at the level of ce_modified().
Currently ce_modified() assumes that lstat(2) is cheap and the
callers have called it on paths they are interested in already.
-
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
