Matheus Tavares Bernardino <matheus.bernardino@xxxxxx> writes: > On Thu, Jul 9, 2020 at 2:08 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: >> >> 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. > >> 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).
