Re: [PATCH 2/4] refs: propagate errno when reading special refs fails

From: Taylor Blau <me@ttaylorr.com>
Date: Nov 29, 2023, 21:51 UTC
Message-ID: <ZWeyUU/NqmGUvyOL@nand.local>
In-Reply-To: <691552a17ec587b0c03e758437c33d58767803aa.1701243201.git.ps@pks.im>

On Wed, Nov 29, 2023 at 09:14:16AM +0100, Patrick Steinhardt wrote:

Show 10 quoted lines

> diff --git a/refs.c b/refs.c
> index fcae5dddc6..7d4a057f36 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1806,8 +1806,12 @@ static int refs_read_special_head(struct ref_store *ref_store,
>  	int result = -1;
>  	strbuf_addf(&full_path, "%s/%s", ref_store->gitdir, refname);
>
> -	if (strbuf_read_file(&content, full_path.buf, 0) < 0)
> +	errno = 0;

Do we need to set errno to 0 here? Looking at the implementation of strbuf_read_file(), it looks like we return early in two cases. Either open() fails, in which errno is set for us, or strbuf_read() fails, in which case we set errno to whatever it was right after the failed read (preventing the subsequent close() call from tainting the value of errno).

So I think in either case, we have the right value in errno, and don't need to worry about setting it to "0" ahead of time.

> +test_expect_success '--exists with existing special ref' '
> +	git rev-parse HEAD >.git/FETCH_HEAD &&
> +	git show-ref --exists FETCH_HEAD
> +'

I don't think that it matters here, but do we need to worry about cleaning up .git/FETCH_HEAD for future tests?

Thanks, Taylor