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

[Kjetil Barvik <barvik@broadpark.no>]

Junio C Hamano <gitster@pobox.com> 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