Re: [PATCH] entry: check for fstat() errors after checkout

Matheus Tavares Bernardino [off-list ref] writes:

On Thu, Jul 9, 2020 at 2:08 PM Junio C Hamano [off-list ref] wrote:

quoted

The returned value from fstat_output() is suppsed to be "have we
done fstat() so that we do not need to do a lstat()?"  Don't you
instead want to extend it to "0 means we didn't, 1 means we did
successfully, and -1 means we did and failed"?  At least, the way
_this_ function is modified by this patch is in line with that.

Makes sense, thanks for spotting this issue.

quoted

Which means that we'd need to update the caller(s) to match, to
avoid risking this change to be just half a change, very similarly
to how the change in 11179eb311 was just half a change.

Thinking about this again, you _could_ argue that your version is
being more defensive.  fstat_is_reliable() might lie and tell us it
is OK to use fstat() when we should do lstat(), and in such a case,
we take a failure from fstat() as a sign to pretend that we didn't
even call it, and tell the caller to do an lstat().  I am actually
OK to go in that direction, but then we probably should save away
errno before making this fstat() call, and restore it after it when
we see an error, if we were to truly pretend that we didn't make a
call.  Otherwise error_errno() call we will make later in the flow
would end up reporting the error from the fstat() we chose to pretend
that we didn't call.

And having said all that, I think fstat_is_reliable() can be trusted
(it says false on Windows and says true on all others).