Re: [PATCH 4/4] resolve_ref_unsafe(): close race condition reading loose refs

[Jeff King <peff@peff.net>]

On Tue, Jun 11, 2013 at 04:26:20PM +0200, Michael Haggerty wrote:

> Please note that if there is some bizarre filesystem somewhere for
> which, for a single, static file
> 
>     lstat() reports S_ISLNK and readlink() fails with ENOENT or EINVAL
> [...]
> then the inner loop would never terminate.

Yeah, I had the exact same thought while reading your description above.
I think we can call that "too crazy to worry about", and deal with it if
it ever actually comes up.

Overall your series looks correct to me. The use of the extra "for(;;)"
and its control flow feels a little tortured to me.  Would it be
simpler to just do:

  stat_ref:
          if (lstat(...) < 0)
            ...
          if (readlink(...) < 0)
                  if (errno == ENOENT || errno == EINVAL)
                          /* inconsistent with lstat; retry */
                          goto stat_ref;

          if (open(...) < 0)
                  if (errno == ENOENT)
                          /* inconsistent with lstat; retry */
                          goto stat_ref;

It is really the same thing, but somehow the goto makes it more obvious
to me that it is the exceptional case to loop at all (and I think your
patch 3/4 could just go away entirely).  But I don't mind that much if
it stays with the for loop.

Thanks for a pleasant read; the split made the refactoring easy to
follow.

-Peff