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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 2, 2024, 21:32 UTC
Message-ID: <xmqqv80ia9wf.fsf@gitster.g>
In-Reply-To: <0ed09e9abb85e73a80d044c1ddaed303517752ac.1722632287.git.gitgitgadget@gmail.com>

"Kyle Lippincott via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 14 quoted lines

> From: Kyle Lippincott <spectral@google.com>
>
> If the loop executes more than once due to cwd being longer than 128
> bytes, then `errno = ERANGE` might persist outside of this function.
> This technically shouldn't be a problem, as all locations where the
> value in `errno` is tested should either (a) call a function that's
> guaranteed to set `errno` to 0 on success, or (b) set `errno` to 0 prior
> to calling the function that only conditionally sets errno, such as the
> `strtod` function. In the case of functions in category (b), it's easy
> to forget to do that.
>
> 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.

Thanks.

Show 16 quoted lines

> Signed-off-by: Kyle Lippincott <spectral@google.com>
> ---
>  strbuf.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/strbuf.c b/strbuf.c
> index 3d2189a7f64..b94ef040ab0 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -601,6 +601,7 @@ int strbuf_getcwd(struct strbuf *sb)
>  		strbuf_grow(sb, guessed_len);
>  		if (getcwd(sb->buf, sb->alloc)) {
>  			strbuf_setlen(sb, strlen(sb->buf));
> +			errno = 0;
>  			return 0;
>  		}