Re: [PATCH v2 2/3] strbuf: set errno to 0 after strbuf_getcwd

[Junio C Hamano <gitster@pobox.com>]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Fri, Aug 2, 2024 at 5:32 PM Junio C Hamano <gitster@pobox.com> 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.