Junio C Hamano [off-list ref] writes:
Let me see if I understand the logic behind this caching....
...
quoted
+ last_slash = match_len;
+ cache_path[last_slash] = '\0';
+
+ if (lstat(cache_path, &st)) {
+ ret_flags = LSTAT_LSTATERR;
+ if (errno == ENOENT || errno == ENOTDIR)
+ ret_flags |= LSTAT_NOTDIR;
If you tested "A/B" here and got ENOENT back, you know "A/B" does not
exist; you cache this knowledge as "A/B is not a directory" (I also think
you could use it as a cached knowledge that "A exists and is a directory".
I am not sure if you are taking advantage of that).
What I do not understand about this code is the ENOTDIR case. If you
tested "A/B" and got ENOTDIR back, what you know is that "A" is not a
directory (if the path tested this round were deeper like "X/Y/A/B", you
know "X/Y/A" is not a directory, and you know "X" and "X/Y" are true
directories; otherwise the loop would have exited before this round when
you tested "X" or "X/Y" in the earlier rounds).
So as far as I can think of, ENOENT case and ENOTDIR case would give you
different information (ENOENT would say "A is a dir, A/B is not"; ENOTDIR
would say "A is not a dir"). I am confused how you can cache the same
path and same flag between these two cases here.
I take 1/4 of the above back.
If everything is working correctly, I do not think you will ever have a
situation where you check "A/B" here and get ENOTDIR back. lstat("A/B")
would yield ENOTDIR if "A" is not a directory, but:
(1) If you did test "A" in the earlier round in the loop, you would have
already found it is not a directory and would have exited the loop
with LSTAT_ERR. You cannot test "A/B" in such a case;
(2) If you did not test "A" in the loop, that has to be because you had a
cached information for it, and the caching logic would have returned
early if "A" was a non-directory without entering the loop; in other
words, you would test "A/B" here without testing "A" in the loop only
when the cache says "A" was a directory. You cannot get ENOTDIR in
such a case.
In any case, my main puzzlement still stands. I think ENOTDIR case should
behave differently from ENOENT case.
And I think it is an indication of a grave error, either this code is
racing with an external process that is actively mucking with the work
tree to make your cached information unusable, or somebody forgot to call
clear_lstat_cache().
Hmm?
Junio C Hamano [off-list ref] writes:
<snipp>
If everything is working correctly, I do not think you will ever have a
situation where you check "A/B" here and get ENOTDIR back. lstat("A/B")
would yield ENOTDIR if "A" is not a directory, but:
(1) If you did test "A" in the earlier round in the loop, you would have
already found it is not a directory and would have exited the loop
with LSTAT_ERR. You cannot test "A/B" in such a case;
No, if the test lstat("A") already returned -1 it would then have set
errno to ENOTDIR in this case, I think. So in this case the cache
would have only stored "A".
(2) If you did not test "A" in the loop, that has to be because you had a
cached information for it, and the caching logic would have returned
early if "A" was a non-directory without entering the loop; in other
words, you would test "A/B" here without testing "A" in the loop only
when the cache says "A" was a directory. You cannot get ENOTDIR in
such a case.
In any case, my main puzzlement still stands. I think ENOTDIR case should
behave differently from ENOENT case.
And I think it is an indication of a grave error, either this code is
racing with an external process that is actively mucking with the work
tree to make your cached information unusable, or somebody forgot to call
clear_lstat_cache().
Hmm?
I think that you do not see that the when the lstat() function returns
errno =
ENOENT A component of the path path does not exist, or the path is
an empty string.
ENOTDIR A component of the path is not a directory.
the cache logic will/must assume that the component we got an error on
will/must always be the _last_ component in the path.
I take 1/4 of the above back.
Will this means that I do not have to update the patch to version 2?
Note, one thing what I personally do not like with this patch so far,
is that it makes a new file, and then it kinda hides the comments from
Linus which talks aboth extending the logic. It would maybe be better
to have reused the same file (symlinks.c), but then the function name
('lstat_cache()') and the filename would not match.
-- kjetil
Junio C Hamano [off-list ref] writes:
<snipp>
If everything is working correctly, I do not think you will ever have a
situation where you check "A/B" here and get ENOTDIR back. lstat("A/B")
would yield ENOTDIR if "A" is not a directory, but:
(1) If you did test "A" in the earlier round in the loop, you would have
already found it is not a directory and would have exited the loop
with LSTAT_ERR. You cannot test "A/B" in such a case;
(2) If you did not test "A" in the loop, that has to be because you had a
cached information for it, and the caching logic would have returned
early if "A" was a non-directory without entering the loop; in other
words, you would test "A/B" here without testing "A" in the loop only
when the cache says "A" was a directory. You cannot get ENOTDIR in
such a case.
In any case, my main puzzlement still stands. I think ENOTDIR case should
behave differently from ENOENT case.
And I think it is an indication of a grave error, either this code is
racing with an external process that is actively mucking with the work
tree to make your cached information unusable, or somebody forgot to call
clear_lstat_cache().
Hmm?
I have looked at this once more, and I have to admit that what you
have said is correct. Sorry for being confusing! I will update the
patch by removing the 'errno == ENOTDIR' part from the if-test.
-- kjetil