Re: [PATCH/RFC 1/4] Optimised, faster, more effective symlink/directory detection

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