Hi Junio,
On Sat, 30 Nov 2019, Junio C Hamano wrote:
> Junio C Hamano <gitster@xxxxxxxxx> writes:
>
> > Johannes Sixt <j6t@xxxxxxxx> writes:
> >
> >> Just like on POSIX the value of errno is indeterminate after a
> >> successful system call, the value of GetLastError() indeterminate after
> >> a successful Windows API call. Therefore, the err_win_to_posix() would
> >> not be able to point at a bogus caller reliably. For this reason, let's
> >> consider the function as a simple error code translator, and then
> >> translating ERROR_SUCCESS to 0 (or is there ESUCCESS?) makes total sense.
> >
> > OK, that makes sense.
>
> Actually, I do not think it makes that much sense, especially in the
> context of this patch that claims to map success to 0 (assuming that
> no E_ANYTHING has the value of zero---I am not sure POSIX gives such
> a guarantee) "for good measure".
But the intention is not to help correct callers! The purpose of mapping
`ERROR_SUCCESS` to 0 will help _only_ callers that try to report an error
when none happened. On Windows, which is the only platform where the
discussed function is compiled and used, `strerror(0)` has the
well-established behavior of returning "No error".
Now imagine that a user encounters a bug where a code path does indeed
incorrectly report an error where no error occurred (or where Git is
looking for the error too late or something like that). You probably agree
with me that we should really avoid reporting "Function not implemented"
in such a case and rather say "No error", which will help figure out the
bug much quicker.
And that is what this patch is all about.
Ciao,
Dscho
> Even if Windows API makes the GetLastError() unusable after a
> success call (unlike POSIX, where errno is left alone), the API
> calls themselves would be signaling their own success or failure,
> right? So I would have imagined that any kosher caller would be
> doing this:
>
> if (SomeWinAPI() != SUCCESS) {
> errno = err_win_to_posix(GetLastError());
> ... possibly other reactions to the error ...
> }
>
> If using a value grabbed from GetLastError() after a successfull
> Windows API call is a wrong thing to do as you taught us in your
> message, then an unconditional
>
> SomeWinAPI();
> errno = err_win_to_posix(GetLastError());
>
> would be wrong anyway, and touching errno unconditionally, when the
> previous call may have succeeded without checking, makes the pattern
> doubly wrong, no?
>
> Having said that, I do not expect myself to be looking into and
> fixing anything in compat/*win*.[ch], so I do not care too deeply
> either way, but I thought that it would help keep the sanity of
> developers involved if we touched errno only upon a failure from an
> underlying system.
>
> Thanks.
>
