Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes: > On Fri, Aug 2, 2024 at 5:32 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: >> > [...] >> > Set `errno = 0;` prior to exiting from `strbuf_getcwd` successfully. >> > This matches the behavior in functions like `run_transaction_hook` >> > (refs.c:2176) and `read_ref_internal` (refs/files-backend.c:564). >> >> I am still uneasy to see this unconditional clearing, which looks >> more like spreading the bad practice from two places you identified >> than following good behaviour modelled after these two places. >> >> But I'll let it pass. >> >> As long as our programmers understand that across strbuf_getcwd(), >> errno will *not* be preserved, even if the function returns success, >> it would be OK. As the usual convention around errno is that a >> successful call would leave errno intact, not clear it to 0, it >> would make it a bit harder to learn our API for newcomers, though. > > For what it's worth, I share your misgivings about this change and > consider the suggestion[*] to make it save/restore `errno` upon > success more sensible. It would also be a welcome change to see the > function documentation in strbuf.h updated to mention that it follows > the usual convention of leaving `errno` untouched upon success and > clobbered upon error. > > [*]: https://lore.kernel.org/git/xmqqv80jeza5.fsf@gitster.g/ Yup, of course save/restore would be safer, and probably easier to reason about for many people. Thanks.
