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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 2, 2024, 15:10 UTC
Message-ID: <xmqqv80jeza5.fsf@gitster.g>
In-Reply-To: <0ed09e9abb85e73a80d044c1ddaed303517752ac.1722571853.git.gitgitgadget@gmail.com>

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

> 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).

This deep in the call chain, there is nothing that assures us that the caller of this function does not care about the error before entering this function, so I feel a bit uneasy about the approach, and my initial reaction was "wouldn't it be safer to do the usual

	int saved_errno = errno;

	for (guessed_len = 128;; guessed_len *= 2) {
		... do things ...
		if (...) {
			... happy ...
			errno = saved_errno;
			return 0;
		}
	}

pattern.

Who calls this function, and inspects errno when this function returns 0? I do not mind adding the "save and restore" fix to this function, but if there is a caller that looks at errno from a call that returns success, that caller may also have to be looked at and fixed if necessary.

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;
>  		}