| Message ID | 95d7547b2e8c5305e76888f7dc0a41d2b9e2f558.1714479928.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Clarify pseudo-ref terminology | expand |
In the subject: s/distinuish/distinguish Patrick Steinhardt <ps@pks.im> writes: > The ref-filter interfaces currently define root refs as either a > detached HEAD or a pseudo ref. Pseudo refs aren't root refs though, so > let's properly distinguish those ref types. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > builtin/for-each-ref.c | 2 +- > ref-filter.c | 16 +++++++++------- > ref-filter.h | 4 ++-- > refs.c | 18 +----------------- > refs.h | 18 ++++++++++++++++++ > 5 files changed, 31 insertions(+), 27 deletions(-) > > diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c > index 919282e12a..5517a4a1c0 100644 > --- a/builtin/for-each-ref.c > +++ b/builtin/for-each-ref.c > @@ -98,7 +98,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix) > } > > if (include_root_refs) > - flags |= FILTER_REFS_ROOT_REFS; > + flags |= FILTER_REFS_ROOT_REFS | FILTER_REFS_DETACHED_HEAD; The only issue I see with this patch is that it makes me think that HEAD is not a root ref anymore. I get that this is the best way to define the directives because otherwise you'd need a new flag something like `FILTER_REFS_ROOT_REFS_WITHOUT_HEAD` and `FILTER_REFS_ROOT_REFS` would be the summation of that and the HEAD flag. Apart from this, the patch looks good.
On Tue, Apr 30, 2024 at 06:11:05AM -0700, Karthik Nayak wrote: > In the subject: s/distinuish/distinguish > > Patrick Steinhardt <ps@pks.im> writes: > > > The ref-filter interfaces currently define root refs as either a > > detached HEAD or a pseudo ref. Pseudo refs aren't root refs though, so > > let's properly distinguish those ref types. > > > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > > --- > > builtin/for-each-ref.c | 2 +- > > ref-filter.c | 16 +++++++++------- > > ref-filter.h | 4 ++-- > > refs.c | 18 +----------------- > > refs.h | 18 ++++++++++++++++++ > > 5 files changed, 31 insertions(+), 27 deletions(-) > > > > diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c > > index 919282e12a..5517a4a1c0 100644 > > --- a/builtin/for-each-ref.c > > +++ b/builtin/for-each-ref.c > > @@ -98,7 +98,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix) > > } > > > > if (include_root_refs) > > - flags |= FILTER_REFS_ROOT_REFS; > > + flags |= FILTER_REFS_ROOT_REFS | FILTER_REFS_DETACHED_HEAD; > > The only issue I see with this patch is that it makes me think that HEAD > is not a root ref anymore. I get that this is the best way to define the > directives because otherwise you'd need a new flag something like > `FILTER_REFS_ROOT_REFS_WITHOUT_HEAD` and `FILTER_REFS_ROOT_REFS` would > be the summation of that and the HEAD flag. > > Apart from this, the patch looks good. Well, it is a root ref, but we treat it differently in the ref-filter interface because it's rendered differently than any other root ref. Furthermore, the ref-filter interfaces allow you to _only_ list the HEAD ref, which is another reason why it's singled out. Renaming this to FILTER_REFS_ROOT_REFS_WITHOUT_HEAD ould be quite misleading, too, because we have the following snippet: @@ -2794,11 +2796,11 @@ static struct ref_array_item *apply_ref_filter(const char *refname, const struct /* * Generally HEAD refs are printed with special description denoting a rebase, * detached state and so forth. This is useful when only printing the HEAD ref * But when it is being printed along with other root refs, it makes sense to * keep the formatting consistent. So we mask the type to act like a root ref. */ if (filter->kind & FILTER_REFS_ROOT_REFS && kind == FILTER_REFS_DETACHED_HEAD) kind = FILTER_REFS_ROOT_REFS; else if (!(kind & filter->kind)) return NULL; If we named this FILTER_REFS_ROOT_REFS_WITHOUT_HEAD then the above code would be even more surprising. So yeah, it's a bit weird, but I think it's more sensible to retain the code as proposed. Patrick
Patrick Steinhardt <ps@pks.im> writes: > On Tue, Apr 30, 2024 at 06:11:05AM -0700, Karthik Nayak wrote: >> In the subject: s/distinuish/distinguish >> >> Patrick Steinhardt <ps@pks.im> writes: >> >> > The ref-filter interfaces currently define root refs as either a >> > detached HEAD or a pseudo ref. Pseudo refs aren't root refs though, so >> > let's properly distinguish those ref types. >> > >> > Signed-off-by: Patrick Steinhardt <ps@pks.im> >> > --- >> > builtin/for-each-ref.c | 2 +- >> > ref-filter.c | 16 +++++++++------- >> > ref-filter.h | 4 ++-- >> > refs.c | 18 +----------------- >> > refs.h | 18 ++++++++++++++++++ >> > 5 files changed, 31 insertions(+), 27 deletions(-) >> > >> > diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c >> > index 919282e12a..5517a4a1c0 100644 >> > --- a/builtin/for-each-ref.c >> > +++ b/builtin/for-each-ref.c >> > @@ -98,7 +98,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix) >> > } >> > >> > if (include_root_refs) >> > - flags |= FILTER_REFS_ROOT_REFS; >> > + flags |= FILTER_REFS_ROOT_REFS | FILTER_REFS_DETACHED_HEAD; >> >> The only issue I see with this patch is that it makes me think that HEAD >> is not a root ref anymore. I get that this is the best way to define the >> directives because otherwise you'd need a new flag something like >> `FILTER_REFS_ROOT_REFS_WITHOUT_HEAD` and `FILTER_REFS_ROOT_REFS` would >> be the summation of that and the HEAD flag. >> >> Apart from this, the patch looks good. > > Well, it is a root ref, but we treat it differently in the ref-filter > interface because it's rendered differently than any other root ref. > Furthermore, the ref-filter interfaces allow you to _only_ list the HEAD > ref, which is another reason why it's singled out. > > Renaming this to FILTER_REFS_ROOT_REFS_WITHOUT_HEAD ould be quite > misleading, too, because we have the following snippet: > > @@ -2794,11 +2796,11 @@ static struct ref_array_item *apply_ref_filter(const char *refname, const struct > /* > * Generally HEAD refs are printed with special description denoting a rebase, > * detached state and so forth. This is useful when only printing the HEAD ref > * But when it is being printed along with other root refs, it makes sense to > * keep the formatting consistent. So we mask the type to act like a root ref. > */ > if (filter->kind & FILTER_REFS_ROOT_REFS && kind == FILTER_REFS_DETACHED_HEAD) > kind = FILTER_REFS_ROOT_REFS; > else if (!(kind & filter->kind)) > return NULL; > Right.. > If we named this FILTER_REFS_ROOT_REFS_WITHOUT_HEAD then the above code > would be even more surprising. > > So yeah, it's a bit weird, but I think it's more sensible to retain the > code as proposed. Agreed. Let's keep it as is!
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c index 919282e12a..5517a4a1c0 100644 --- a/builtin/for-each-ref.c +++ b/builtin/for-each-ref.c @@ -98,7 +98,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix) } if (include_root_refs) - flags |= FILTER_REFS_ROOT_REFS; + flags |= FILTER_REFS_ROOT_REFS | FILTER_REFS_DETACHED_HEAD; filter.match_as_path = 1; filter_and_format_refs(&filter, flags, sorting, &format); diff --git a/ref-filter.c b/ref-filter.c index 361beb6619..d72113edfe 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -2628,7 +2628,7 @@ static int for_each_fullref_in_pattern(struct ref_filter *filter, each_ref_fn cb, void *cb_data) { - if (filter->kind == FILTER_REFS_KIND_MASK) { + if (filter->kind & FILTER_REFS_ROOT_REFS) { /* In this case, we want to print all refs including root refs. */ return refs_for_each_include_root_refs(get_main_ref_store(the_repository), cb, cb_data); @@ -2756,8 +2756,10 @@ static int ref_kind_from_refname(const char *refname) return ref_kind[i].kind; } - if (is_root_ref(get_main_ref_store(the_repository), refname)) + if (is_pseudo_ref(refname)) return FILTER_REFS_PSEUDOREFS; + if (is_root_ref(get_main_ref_store(the_repository), refname)) + return FILTER_REFS_ROOT_REFS; return FILTER_REFS_OTHERS; } @@ -2794,11 +2796,11 @@ static struct ref_array_item *apply_ref_filter(const char *refname, const struct /* * Generally HEAD refs are printed with special description denoting a rebase, * detached state and so forth. This is useful when only printing the HEAD ref - * But when it is being printed along with other pseudorefs, it makes sense to - * keep the formatting consistent. So we mask the type to act like a pseudoref. + * But when it is being printed along with other root refs, it makes sense to + * keep the formatting consistent. So we mask the type to act like a root ref. */ - if (filter->kind == FILTER_REFS_KIND_MASK && kind == FILTER_REFS_DETACHED_HEAD) - kind = FILTER_REFS_PSEUDOREFS; + if (filter->kind & FILTER_REFS_ROOT_REFS && kind == FILTER_REFS_DETACHED_HEAD) + kind = FILTER_REFS_ROOT_REFS; else if (!(kind & filter->kind)) return NULL; @@ -3072,7 +3074,7 @@ static int do_filter_refs(struct ref_filter *filter, unsigned int type, each_ref * When printing all ref types, HEAD is already included, * so we don't want to print HEAD again. */ - if (!ret && (filter->kind != FILTER_REFS_KIND_MASK) && + if (!ret && !(filter->kind & FILTER_REFS_ROOT_REFS) && (filter->kind & FILTER_REFS_DETACHED_HEAD)) head_ref(fn, cb_data); } diff --git a/ref-filter.h b/ref-filter.h index 0ca28d2bba..27ae1aa0d1 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -23,9 +23,9 @@ FILTER_REFS_REMOTES | FILTER_REFS_OTHERS) #define FILTER_REFS_DETACHED_HEAD 0x0020 #define FILTER_REFS_PSEUDOREFS 0x0040 -#define FILTER_REFS_ROOT_REFS (FILTER_REFS_DETACHED_HEAD | FILTER_REFS_PSEUDOREFS) +#define FILTER_REFS_ROOT_REFS 0x0080 #define FILTER_REFS_KIND_MASK (FILTER_REFS_REGULAR | FILTER_REFS_DETACHED_HEAD | \ - FILTER_REFS_PSEUDOREFS) + FILTER_REFS_PSEUDOREFS | FILTER_REFS_ROOT_REFS) struct atom_value; struct ref_sorting; diff --git a/refs.c b/refs.c index dec9dbdc2d..50d679b7e7 100644 --- a/refs.c +++ b/refs.c @@ -844,24 +844,8 @@ int is_per_worktree_ref(const char *refname) starts_with(refname, "refs/rewritten/"); } -static int is_pseudo_ref(const char *refname) +int is_pseudo_ref(const char *refname) { - /* - * Pseudorefs are refs that have different semantics compared to - * "normal" refs. These refs can thus not be stored in the ref backend, - * but must always be accessed via the filesystem. The following refs - * are pseudorefs: - * - * - FETCH_HEAD may contain multiple object IDs, and each one of them - * carries additional metadata like where it came from. - * - * - MERGE_HEAD may contain multiple object IDs when merging multiple - * heads. - * - * Reading, writing or deleting references must consistently go either - * through the filesystem (pseudorefs) or through the reference - * backend (normal ones). - */ static const char * const pseudo_refs[] = { "FETCH_HEAD", "MERGE_HEAD", diff --git a/refs.h b/refs.h index 4ac454b0c3..8255989e7e 100644 --- a/refs.h +++ b/refs.h @@ -1084,4 +1084,22 @@ int is_root_ref(struct ref_store *refs, const char *refname); */ int is_headref(struct ref_store *refs, const char *refname); +/* + * Pseudorefs are refs that have different semantics compared to + * "normal" refs. These refs can thus not be stored in the ref backend, + * but must always be accessed via the filesystem. The following refs + * are pseudorefs: + * + * - FETCH_HEAD may contain multiple object IDs, and each one of them + * carries additional metadata like where it came from. + * + * - MERGE_HEAD may contain multiple object IDs when merging multiple + * heads. + * + * Reading, writing or deleting references must consistently go either + * through the filesystem (pseudorefs) or through the reference + * backend (normal ones). + */ +int is_pseudo_ref(const char *refname); + #endif /* REFS_H */
The ref-filter interfaces currently define root refs as either a detached HEAD or a pseudo ref. Pseudo refs aren't root refs though, so let's properly distinguish those ref types. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/for-each-ref.c | 2 +- ref-filter.c | 16 +++++++++------- ref-filter.h | 4 ++-- refs.c | 18 +----------------- refs.h | 18 ++++++++++++++++++ 5 files changed, 31 insertions(+), 27 deletions(-)