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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 1518 bytes --]

On Wed, Nov 29, 2023 at 04:51:13PM -0500, Taylor Blau wrote:
> On Wed, Nov 29, 2023 at 09:14:16AM +0100, Patrick Steinhardt wrote:
> > 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.

True. I'll drop this when rerolling.

> > +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?

There's so many tests where I wish that we did a better job of cleanup,
so I agree that it is sensible to clean up after ourselves. Will drop
when rerolling.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]