| Message ID | e90b2f8aa98493e9cd3f2c04cb58318780f9f6e5.1714479928.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Clarify pseudo-ref terminology | expand |
On 24/04/30 02:26PM, Patrick Steinhardt wrote: > Before this patch series, root refs except for "HEAD" and our special > refs were classified as pseudorefs. Furthermore, our terminology > clarified that pseudorefs must not be symbolic refs. This restriction > is enforced in `is_root_ref()`, which explicitly checks that a supposed > root ref resolves to an object ID without recursing. > > This has been extremely confusing right from the start because (in old > terminology) a ref name may sometimes be a pseudoref and sometimes not > depending on whether it is a symbolic or regular ref. This behaviour > does not seem reasonable at all and I very much doubt that it results in > anything sane. > > Furthermore, the behaviour is different to `is_headref()`, which only > checks for the ref to exist. While that is in line with our glossary, > this inconsistency only adds to the confusion. > > Last but not least, the current behaviour can actually lead to a > segfault when calling `is_root_ref()` with a reference that either does > not exist or that is a symbolic ref because we never intialized `oid`. s/intialized/initialized/ > Let's loosen the restrictions in accordance to the new definition of > root refs, which are simply plain refs that may as well be a symbolic > ref. Consequently, we can just check for the ref to exist instead of > requiring it to be a regular ref. > > Add a test that verifies that this does not change user-visible > behaviour. Namely, we still don't want to show broken refs to the user > by default in git-for-each-ref(1). What this does allow though is for > internal callers to surface dangling root refs when they pass in the > `DO_FOR_EACH_INCLUDE_BROKEN` flag. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > refs.c | 50 ++++++++++++++++++++++++---------- > t/t6302-for-each-ref-filter.sh | 17 ++++++++++++ > 2 files changed, 53 insertions(+), 14 deletions(-) > > diff --git a/refs.c b/refs.c > index 5b89e83ad7..ca9844bc3e 100644 > --- a/refs.c > +++ b/refs.c ... > int is_headref(struct ref_store *refs, const char *refname) > { > - if (!strcmp(refname, "HEAD")) > - return refs_ref_exists(refs, refname); > + struct strbuf referent = STRBUF_INIT; > + struct object_id oid = { 0 }; > + int failure_errno, ret = 0; > + unsigned int flags; > > - return 0; > + /* > + * Note that we cannot use `refs_ref_exists()` here because that also > + * checks whether its target ref exists in case refname is a symbolic > + * ref. > + */ > + if (!strcmp(refname, "HEAD")) { > + ret = !refs_read_raw_ref(refs, refname, &oid, &referent, > + &flags, &failure_errno); > + } > + > + strbuf_release(&referent); > + return ret; > } I'm not quite sure I understand why we are changing the behavior of `is_headref()` here. Do we no longer want to validate the ref exists if it is symbolic? In a prior commit, `is_headref()` is commented to mention that we check whether the reference exists. Maybe that could use some additional clarification? -Justin
On Tue, Apr 30, 2024 at 12:09:57PM -0500, Justin Tobler wrote: > On 24/04/30 02:26PM, Patrick Steinhardt wrote: [snip] > > diff --git a/refs.c b/refs.c > > index 5b89e83ad7..ca9844bc3e 100644 > > --- a/refs.c > > +++ b/refs.c > ... > > int is_headref(struct ref_store *refs, const char *refname) > > { > > - if (!strcmp(refname, "HEAD")) > > - return refs_ref_exists(refs, refname); > > + struct strbuf referent = STRBUF_INIT; > > + struct object_id oid = { 0 }; > > + int failure_errno, ret = 0; > > + unsigned int flags; > > > > - return 0; > > + /* > > + * Note that we cannot use `refs_ref_exists()` here because that also > > + * checks whether its target ref exists in case refname is a symbolic > > + * ref. > > + */ > > + if (!strcmp(refname, "HEAD")) { > > + ret = !refs_read_raw_ref(refs, refname, &oid, &referent, > > + &flags, &failure_errno); > > + } > > + > > + strbuf_release(&referent); > > + return ret; > > } > > I'm not quite sure I understand why we are changing the behavior of > `is_headref()` here. Do we no longer want to validate the ref exists if > it is symbolic? The implementation does not conform to the definition of a "HEAD" ref. Even before this patch series, a "HEAD" ref could either be a symbolic or a regular ref. So to answer the question of "Is this a HEAD ref?" you only need to check whether the ref exists, not whether its target exists. > In a prior commit, `is_headref()` is commented to mention that we check > whether the reference exists. Maybe that could use some additional > clarification? Which particular commit do you refer to? It's not part of this series, is it? Patrick
On 24/05/02 10:07AM, Patrick Steinhardt wrote: > On Tue, Apr 30, 2024 at 12:09:57PM -0500, Justin Tobler wrote: > > I'm not quite sure I understand why we are changing the behavior of > > `is_headref()` here. Do we no longer want to validate the ref exists if > > it is symbolic? > > The implementation does not conform to the definition of a "HEAD" ref. > Even before this patch series, a "HEAD" ref could either be a symbolic > or a regular ref. So to answer the question of "Is this a HEAD ref?" you > only need to check whether the ref exists, not whether its target > exists. Thanks Patrick! I think this explantion might be good to add to the commit message. > > In a prior commit, `is_headref()` is commented to mention that we check > > whether the reference exists. Maybe that could use some additional > > clarification? > > Which particular commit do you refer to? It's not part of this series, > is it? I'm refering to the comment added above `is_headref()` in (refs: classify HEAD as a root ref, 2024-04-30): "Check whether the reference is "HEAD" and whether it exists." Maybe I misunderstand its intent though. -Justin
On Fri, May 03, 2024 at 03:49:25PM -0500, Justin Tobler wrote: > On 24/05/02 10:07AM, Patrick Steinhardt wrote: > > On Tue, Apr 30, 2024 at 12:09:57PM -0500, Justin Tobler wrote: > > > > I'm not quite sure I understand why we are changing the behavior of > > > `is_headref()` here. Do we no longer want to validate the ref exists if > > > it is symbolic? > > > > The implementation does not conform to the definition of a "HEAD" ref. > > Even before this patch series, a "HEAD" ref could either be a symbolic > > or a regular ref. So to answer the question of "Is this a HEAD ref?" you > > only need to check whether the ref exists, not whether its target > > exists. > > Thanks Patrick! I think this explantion might be good to add to the > commit message. I'll restructure this a bit. In fact, we can even get rid of `is_headref()` completely as it is now covered by `is_root_ref()`, and there are no callers of `is_headref()`. > > > In a prior commit, `is_headref()` is commented to mention that we check > > > whether the reference exists. Maybe that could use some additional > > > clarification? > > > > Which particular commit do you refer to? It's not part of this series, > > is it? > > I'm refering to the comment added above `is_headref()` in > (refs: classify HEAD as a root ref, 2024-04-30): > > "Check whether the reference is "HEAD" and whether it exists." > > Maybe I misunderstand its intent though. Ah, now I get what you're saying. Yeah, this could indeed use a clarification. I'll add it to `is_root_ref()` though given that `is_headref()` will go away. Patrick
diff --git a/refs.c b/refs.c index 5b89e83ad7..ca9844bc3e 100644 --- a/refs.c +++ b/refs.c @@ -869,7 +869,10 @@ int is_root_ref(struct ref_store *refs, const char *refname) "NOTES_MERGE_REF", "MERGE_AUTOSTASH", }; - struct object_id oid; + struct strbuf referent = STRBUF_INIT; + struct object_id oid = { 0 }; + int failure_errno, ret = 0; + unsigned int flags; size_t i; if (!is_root_ref_syntax(refname)) @@ -877,30 +880,49 @@ int is_root_ref(struct ref_store *refs, const char *refname) if (is_headref(refs, refname)) return 1; + /* + * Note that we cannot use `refs_ref_exists()` here because that also + * checks whether its target ref exists in case refname is a symbolic + * ref. + */ if (ends_with(refname, "_HEAD")) { - refs_resolve_ref_unsafe(refs, refname, - RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, - &oid, NULL); - return !is_null_oid(&oid); + ret = !refs_read_raw_ref(refs, refname, &oid, &referent, + &flags, &failure_errno); + goto done; } - for (i = 0; i < ARRAY_SIZE(irregular_root_refs); i++) + for (i = 0; i < ARRAY_SIZE(irregular_root_refs); i++) { if (!strcmp(refname, irregular_root_refs[i])) { - refs_resolve_ref_unsafe(refs, refname, - RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, - &oid, NULL); - return !is_null_oid(&oid); + ret = !refs_read_raw_ref(refs, refname, &oid, &referent, + &flags, &failure_errno); + goto done; } + } - return 0; +done: + strbuf_release(&referent); + return ret; } int is_headref(struct ref_store *refs, const char *refname) { - if (!strcmp(refname, "HEAD")) - return refs_ref_exists(refs, refname); + struct strbuf referent = STRBUF_INIT; + struct object_id oid = { 0 }; + int failure_errno, ret = 0; + unsigned int flags; - return 0; + /* + * Note that we cannot use `refs_ref_exists()` here because that also + * checks whether its target ref exists in case refname is a symbolic + * ref. + */ + if (!strcmp(refname, "HEAD")) { + ret = !refs_read_raw_ref(refs, refname, &oid, &referent, + &flags, &failure_errno); + } + + strbuf_release(&referent); + return ret; } static int is_current_worktree_ref(const char *ref) { diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh index 948f1bb5f4..92ed8957c8 100755 --- a/t/t6302-for-each-ref-filter.sh +++ b/t/t6302-for-each-ref-filter.sh @@ -62,6 +62,23 @@ test_expect_success '--include-root-refs with other patterns' ' test_cmp expect actual ' +test_expect_success '--include-root-refs omits dangling symrefs' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + test_commit initial && + git symbolic-ref DANGLING_HEAD refs/heads/missing && + cat >expect <<-EOF && + HEAD + $(git symbolic-ref HEAD) + refs/tags/initial + EOF + git for-each-ref --format="%(refname)" --include-root-refs >actual && + test_cmp expect actual + ) +' + test_expect_success 'filtering with --points-at' ' cat >expect <<-\EOF && refs/heads/main
Before this patch series, root refs except for "HEAD" and our special refs were classified as pseudorefs. Furthermore, our terminology clarified that pseudorefs must not be symbolic refs. This restriction is enforced in `is_root_ref()`, which explicitly checks that a supposed root ref resolves to an object ID without recursing. This has been extremely confusing right from the start because (in old terminology) a ref name may sometimes be a pseudoref and sometimes not depending on whether it is a symbolic or regular ref. This behaviour does not seem reasonable at all and I very much doubt that it results in anything sane. Furthermore, the behaviour is different to `is_headref()`, which only checks for the ref to exist. While that is in line with our glossary, this inconsistency only adds to the confusion. Last but not least, the current behaviour can actually lead to a segfault when calling `is_root_ref()` with a reference that either does not exist or that is a symbolic ref because we never intialized `oid`. Let's loosen the restrictions in accordance to the new definition of root refs, which are simply plain refs that may as well be a symbolic ref. Consequently, we can just check for the ref to exist instead of requiring it to be a regular ref. Add a test that verifies that this does not change user-visible behaviour. Namely, we still don't want to show broken refs to the user by default in git-for-each-ref(1). What this does allow though is for internal callers to surface dangling root refs when they pass in the `DO_FOR_EACH_INCLUDE_BROKEN` flag. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- refs.c | 50 ++++++++++++++++++++++++---------- t/t6302-for-each-ref-filter.sh | 17 ++++++++++++ 2 files changed, 53 insertions(+), 14 deletions(-)