| Message ID | 24032a62e54fb37dad46c3ede7151efc8a7a8818.1702365291.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | refs: improve handling of special refs | expand |
Patrick Steinhardt <ps@pks.im> writes: > 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. Hmph, I thought, judging from what [1/4] did, that your plan was to use the refs API, instead of peeking directly into the filesystem, to access these pseudo refs, and am a bit puzzled if it makes all that much difference to fix errno handling while still reading directly from the filesystem. Perhaps such a conversion happens in later steps of this series (or a follow on series after this series lands)? > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > refs.c | 4 +++- > t/t1403-show-ref.sh | 10 ++++++++++ > 2 files changed, 13 insertions(+), 1 deletion(-) > > diff --git a/refs.c b/refs.c > index fcae5dddc6..00e72a2abf 100644 > --- a/refs.c > +++ b/refs.c > @@ -1806,8 +1806,10 @@ 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) > + 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..66e6e77fa9 100755 > --- a/t/t1403-show-ref.sh > +++ b/t/t1403-show-ref.sh > @@ -266,4 +266,14 @@ 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' ' > + test_when_finished "rm .git/FETCH_HEAD" && > + git rev-parse HEAD >.git/FETCH_HEAD && > + git show-ref --exists FETCH_HEAD > +' > + > test_done
On Tue, Dec 12, 2023 at 12:28:49PM -0800, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > 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. > > Hmph, I thought, judging from what [1/4] did, that your plan was to > use the refs API, instead of peeking directly into the filesystem, > to access these pseudo refs, and am a bit puzzled if it makes all > that much difference to fix errno handling while still reading > directly from the filesystem. Perhaps such a conversion happens in > later steps of this series (or a follow on series after this series > lands)? Yes, that's ultimately the goal. The patch series here is only setting the stage by recording what we have, and addressing cases where we are inconsistently accessing refs via the filesystem _and_ via the ref API. Once this lands I do want create a follow up patch series that converts all refs to be non-special to the extent possible. I say "to the extent possible" though because in the end there will be two refs that we must continue to treat specially: FETCH_HEAD and MERGE_HEAD, which we were treating special before this patch series already. Both of these are not normal refs because they may contain multiple values with annotations, so they will need to stay special. Patrick
diff --git a/refs.c b/refs.c index fcae5dddc6..00e72a2abf 100644 --- a/refs.c +++ b/refs.c @@ -1806,8 +1806,10 @@ 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) + 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..66e6e77fa9 100755 --- a/t/t1403-show-ref.sh +++ b/t/t1403-show-ref.sh @@ -266,4 +266,14 @@ 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' ' + test_when_finished "rm .git/FETCH_HEAD" && + 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 | 4 +++- t/t1403-show-ref.sh | 10 ++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-)