Re: [PATCH 2/4] refs: propagate errno when reading special refs fails
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Nov 30, 2023, 07:43 UTC
- Message-ID
- <ZWg9KfpeyLRdX1b8@tanuki>
- In-Reply-To
- <ZWeyUU/NqmGUvyOL@nand.local>
On Wed, Nov 29, 2023 at 04:51:13PM -0500, Taylor Blau wrote:
Show 20 quoted lines
> 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.
Show 7 quoted lines
> > +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