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@xxxxxx>
> ---
> 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
