Hi Junio, On Sun, 1 Dec 2019, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes: > > > 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". > > But is the caller that ends up saying "No error" you mention below > by calling strerror() exists *only* on Windows port somewhere in > compat/ directory? If the call to strerror() is made from a > codepath that is common to all platforms, then that caller *is* > wrong to do so after seeing *no* errors. Yes, that would be a difference between platforms, but _at least_ the Windows side would help diagnose the bug better. I'd prefer such a half solution to having no solution at all. > So, no, sorry, but I do not understand the above line of reasoning. > > > 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. > > Not really. A BUG() that catches a call to this function that > should not be called when there is no error would help identify the > problematic caller a lot faster by being louder---those users who > get hit will raise their hands more readily than those who see "No > error" and wonders "why does Git give that error message when there > is no error?". IOW, "No error" sounds like sweeping the issue under > the rug. This is a very good point. It _is_ a bug to hit that code path when `GetLastError()` returned `ERROR_SUCCESS`, and having such a `BUG()` call would have prevented the bug that I fix in 1/2 from slipping through. Therefore, I changed 2/2 to raise a BUG() and am currently waiting for the PR build to finish before sending another iteration of this seris. Ciao, Dscho > The argument could be "I do not want to be pressured to hunt for the > problematic caller, and taking the approach to be less loud than the > BUG() approach would allow the end users keep operating as if > nothing bad happened, so that I can hunt for the caller iff/when I > feel like it, and with no urgent need I can just ignore the hunt > altogether. Not hitting with BUG() is a service to the end users > who shouldn't be made a guinea pig---that is why we want to sweep > the problematic caller under the rug. It may delay the discovery > and fixing of problematic callers, which is a downside, though." > > I would understand such an argument, even if I might not think that > such an argument makes the right tradeoff between not hitting the > users with BUG() and not being effective in finding the problematic > caller. > > Thanks. > >
