Junio C Hamano <gitster@xxxxxxxxx> writes:
> Kjetil Barvik <barvik@xxxxxxxxxxxx> writes:
>
>> Start using the optimised, faster and more effective symlink/directory
>> cache. The previously used call:
>>
>> has_symlink_leading_path(len, name);
>>
>> should be identically with the following call to lstat_cache():
>>
>> lstat_cache(len, name,
>> LSTAT_SYMLINK|LSTAT_DIR,
>> LSTAT_SYMLINK);
>> ...
>
> Care to enlighten why some of callers use the above, but not others?
> Namely, check_removed() in diff-lib.c
I though that first it would be a good thing to introduce as little
changes as possible, and then later on do some cleanups. Regarding
the 'check_removed()' function, I though that it could later have been
written something like this after cleanups:
[....]
static int check_removed(const struct cache_entry *ce, struct stat *st)
{
unsigned int ret_flags =
check_lstat_cache(ce_namelen(ce), ce->name,
LSTAT_SYMLINK|LSTAT_NOTDIR|LSTAT_DIR);
if (ret_flags & (LSTAT_SYMLINK|LSTAT_NOTDIR))
return 1;
if (ret_flags & LSTAT_LSTATERR)
return -1;
if (ret_flags & LSTAT_DIR) {
unsigned char sub[20];
[....]
This would have saved one more lstat() call in some cases. But after
a test, I now see that it does not work. The reason is that it does
not set the 'struct stat *st' parameter and/or that for the moment you
can not tell the 'lstat_cache()' function to also always test the last
path component. It could be extended to do this, if someone ask for
it and if it would be useful to extend the lstat_cache() for this
fact.
I will remove the '|LSTAT_NOTDIR' part from the call to lstat_cache()
in 'check_removed()' in the next version of the patch.
> and callers in unpack-trees.c care about NOTDIR unlike others, even
> though the original code checked for exactly the same condition.
Regarding the 'verify_absent()' function in unpack-trees.c, the
'|LSTAT_NOTDIR' part of the call to lstat_cache() helps to avoid 16047
lstat() calls for the given test case mentioned in the cover-letter.
And from the source code:
[...]
if (lstat_cache(ce_namelen(ce), ce->name,
LSTAT_SYMLINK|LSTAT_NOTDIR|LSTAT_DIR,
LSTAT_SYMLINK|LSTAT_NOTDIR))
return 0;
if (!lstat(ce->name, &st)) {
[...]
it should be easy to see that if we from the lstat_cache() could
already spot that some path component of ce->name does not exists,
then we can avoid the following lstat() call, as it then should known
to be failing.
regarding the 'unlink_entry()' function in unpack-trees.c, the
'|LSTAT_NOTDIR' part of the call to lstat_cache() does not for the
moment helps to avoid any lstat() calls, as far as I can see. But,
again, from the source code:
[..]
char *name = ce->name;
if (lstat_cache(ce_namelen(ce), ce->name,
LSTAT_SYMLINK|LSTAT_NOTDIR|LSTAT_DIR,
LSTAT_SYMLINK|LSTAT_NOTDIR))
return;
if (unlink(name))
return;
[...]
it should be correct, since if we already know that some path
component of ce-name does not exist, the call to unlink(name) would
always fail (with ENOENT).
> Does this mean that some callers of has_symlink_leading_path() checked
> only for leading symlinks when they should also have checked for a leading
> non-directory, and this patch is also a bugfix?
Yes, as indicated above, has_symlink_leading_path() should have
checked for leading non-directories when called from for the
'verify_absent()' function to be able to optimise away some more
lstat() calls.
I admit that I do not know the source code good enough to decide if
this is an indication of a bug somewhere, or just an optimisation.
-- kjetil
--
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
