| Message ID | 691552a17ec587b0c03e758437c33d58767803aa.1701243201.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | refs: improve handling of special refs | expand |
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. > +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
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
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; + + if (strbuf_read_file(&content, full_path.buf, 0) < 0) { + *failure_errno = errno; goto done; + } result = parse_loose_ref_contents(content.buf, oid, referent, type, failure_errno); diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh index b50ae6fcf1..37af154d27 100755 --- a/t/t1403-show-ref.sh +++ b/t/t1403-show-ref.sh @@ -266,4 +266,13 @@ test_expect_success '--exists with directory fails with generic error' ' test_cmp expect err ' +test_expect_success '--exists with non-existent special ref' ' + test_expect_code 2 git show-ref --exists FETCH_HEAD +' + +test_expect_success '--exists with existing special ref' ' + git rev-parse HEAD >.git/FETCH_HEAD && + git show-ref --exists FETCH_HEAD +' + test_done
Some refs in Git are more special than others due to reasons explained in the next commit. These refs are read via `refs_read_special_head()`, but this function doesn't behave the same as when we try to read a normal ref. Most importantly, we do not propagate `failure_errno` in the case where the reference does not exist, which is behaviour that we rely on in many parts of Git. Fix this bug by propagating errno when `strbuf_read_file()` fails. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- refs.c | 6 +++++- t/t1403-show-ref.sh | 9 +++++++++ 2 files changed, 14 insertions(+), 1 deletion(-)