| Message ID | 20231023221143.72489-6-andy.koppe@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | log: decorate pseudorefs and other refs | expand |
Andy Koppe <andy.koppe@gmail.com> writes: > Define const array 'pseudorefs' with the names of the pseudorefs that > are documented in gitrevisions.1, and add functions for_each_pseudoref() > and refs_for_each_pseudoref() for iterating over them. Makes sense, and we can later (ab|re)use the same mechanism to extend "git for-each-ref" that currently only knows how to show things under "refs/" hierarchy. > The functions process the pseudorefs in the same way as head_ref() and > refs_head_ref() process HEAD, invoking an each_ref_fn callback on each > pseudoref that exists. Good.
Andy Koppe <andy.koppe@gmail.com> wrote: > Define const array 'pseudorefs' with the names of the pseudorefs that > are documented in gitrevisions.1, and add functions for_each_pseudoref() > and refs_for_each_pseudoref() for iterating over them. > > The functions process the pseudorefs in the same way as head_ref() and > refs_head_ref() process HEAD, invoking an each_ref_fn callback on each > pseudoref that exists. > > This is in preparation for adding pseudorefs to log decorations. > > Signed-off-by: Andy Koppe <andy.koppe@gmail.com> > --- [...] > +/* > + * List of documented pseudorefs. This needs to be kept in sync with the list > + * in Documentation/revisions.txt. > + */ > +static const char *const pseudorefs[] = { > + "FETCH_HEAD", > + "ORIG_HEAD", > + "MERGE_HEAD", > + "REBASE_HEAD", > + "CHERRY_PICK_HEAD", > + "REVERT_HEAD", > + "BISECT_HEAD", > + "AUTO_MERGE", > +}; > + > struct ref_namespace_info ref_namespace[] = { > [NAMESPACE_HEAD] = { > .ref = "HEAD", > @@ -1549,6 +1564,33 @@ int head_ref(each_ref_fn fn, void *cb_data) > return refs_head_ref(get_main_ref_store(the_repository), fn, cb_data); > } The first thing that popped up in my head was "Should we somehow use is_pseudoref_syntax() instead of manually listing these?" (although I read in this thread later that Junio was okay with the listing) but then ... I thought I saw something similar in some other thread (which entered the mailing list much after this patch series was submitted) ... https://lore.kernel.org/git/20231221170715.110565-2-karthik.188@gmail.com/T/ The whole thread is really interesting but some points that are worth to be mentioned in this context are " ... Patrick's reftable work based on Han-Wen's work revealed the need to treat FETCH_HEAD and MERGE_HEAD as "even more pecurilar than pseudorefs" that need different term (tentatively called "special refs") ... " So since we are introducing this array in refs.c, which acts as a "refs API" currently "A lot more reasonable thing to do may be to scan the $GIT_DIR for files whose name satisfy refs.c:is_pseudoref_syntax() and list them, instead of having a hardcoded list of these special refs. In addition, when reftable and other backends that can natively store things outside refs/ hierarchy is in use, they ought to know what they have so enumerating these would not be an issue for them without having such a hardcoded table of names." All that said, the above mentioned thread led to a series of patches for a different purpose than this [1] (which are currently on their way to "master" according to the latest "What's Cooking" email on Feb 2). The ones that have significance w.r.t. to THIS patch series though, are https://lore.kernel.org/git/20240129113527.607022-2-karthik.188@gmail.com/ https://lore.kernel.org/git/20240129113527.607022-4-karthik.188@gmail.com/ (ignoring the reftable part). I find these to make sense HERE because using the functions introduced THERE are much more robust when dealing with pseudorefs and can be used HERE. I haven't given it much thought but I think we would still end up writing "for_each_pseudoref()", although much differently from below (and can't use "refs_for_each_all_refs()" directly) because of how we call this function in PATCH 7/7 when actually doing the decoration - that is the decoration for pseudorefs is different (?) Another approach would be I think to refactor the whole of how decorations with refs work and somehow use "refs_for_each_all_refs()" with its callback handling how we decorate the various refs - I need to dig deeper :) - since the end goal is to support showing all kinds of refs when showing the log $ git log -1 --clear-decorations --oneline master 2a540e432f (ORIG_HEAD, FETCH_HEAD, upstream/master, upstream/HEAD, master) The thirteenth batch (with color enabled) > +int refs_for_each_pseudoref(struct ref_store *refs, > + each_ref_fn fn, void *cb_data) > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(pseudorefs); i++) { > + struct object_id oid; > + int flag; > + > + if (refs_resolve_ref_unsafe(refs, pseudorefs[i], > + RESOLVE_REF_READING, &oid, &flag)) { > + int ret = fn(pseudorefs[i], &oid, flag, cb_data); > + > + if (ret) > + return ret; > + } > + } > + > + return 0; > +} > + > +int for_each_pseudoref(each_ref_fn fn, void *cb_data) > +{ > + return refs_for_each_pseudoref(get_main_ref_store(the_repository), > + fn, cb_data); > +} > + > struct ref_iterator *refs_ref_iterator_begin( > struct ref_store *refs, > const char *prefix, > diff --git a/refs.h b/refs.h > index 23211a5ea1..7b55cced31 100644 > --- a/refs.h > +++ b/refs.h > @@ -320,6 +320,8 @@ typedef int each_repo_ref_fn(struct repository *r, > */ > int refs_head_ref(struct ref_store *refs, > each_ref_fn fn, void *cb_data); > +int refs_for_each_pseudoref(struct ref_store *refs, > + each_ref_fn fn, void *cb_data); > int refs_for_each_ref(struct ref_store *refs, > each_ref_fn fn, void *cb_data); > int refs_for_each_ref_in(struct ref_store *refs, const char *prefix, > @@ -334,6 +336,9 @@ int refs_for_each_remote_ref(struct ref_store *refs, > /* just iterates the head ref. */ > int head_ref(each_ref_fn fn, void *cb_data); > > +/* iterates pseudorefs. */ > +int for_each_pseudoref(each_ref_fn fn, void *cb_data); > + > /* iterates all refs. */ > int for_each_ref(each_ref_fn fn, void *cb_data); > > -- > 2.42.GIT So yeah, I just wanted to point out the above things as we would need to refactor this commit and the commits following this - patches 6/7 and 7/7. Thanks [1]: https://lore.kernel.org/git/20240129113527.607022-1-karthik.188@gmail.com/
Kousik Sanagavarapu <five231003@gmail.com> writes: > Andy Koppe <andy.koppe@gmail.com> wrote: > ... >> +static const char *const pseudorefs[] = { >> + "FETCH_HEAD", >> + "ORIG_HEAD", >> + "MERGE_HEAD", >> + "REBASE_HEAD", >> + "CHERRY_PICK_HEAD", >> + "REVERT_HEAD", >> + "BISECT_HEAD", >> + "AUTO_MERGE", >> +}; >> + >> struct ref_namespace_info ref_namespace[] = { >> [NAMESPACE_HEAD] = { >> .ref = "HEAD", >> @@ -1549,6 +1564,33 @@ int head_ref(each_ref_fn fn, void *cb_data) >> return refs_head_ref(get_main_ref_store(the_repository), fn, cb_data); >> } > > The first thing that popped up in my head was "Should we somehow use > is_pseudoref_syntax() instead of manually listing these?" (although I > read in this thread later that Junio was okay with the listing) but then ... > > I thought I saw something similar in some other thread (which entered > the mailing list much after this patch series was submitted) ... > > https://lore.kernel.org/git/20231221170715.110565-2-karthik.188@gmail.com/T/ We are halting Karthik's topic to rethink its UI for now, but your point stands. We should use a unified definition of what pseudorefs there are across the codebase for consistency, and Karthik's topic would be a better place to do so. Andy, let me drop this topic for now from my tree, and let's wait until Karthik's "iterate over all refs" topic solidifies, at which time an updated iteration (v4?) of this topic hopefully can build on top of it. Thanks.
diff --git a/refs.c b/refs.c index fcae5dddc6..aa7e4c02c5 100644 --- a/refs.c +++ b/refs.c @@ -65,6 +65,21 @@ static unsigned char refname_disposition[256] = { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3, 0, 0, 4, 4 }; +/* + * List of documented pseudorefs. This needs to be kept in sync with the list + * in Documentation/revisions.txt. + */ +static const char *const pseudorefs[] = { + "FETCH_HEAD", + "ORIG_HEAD", + "MERGE_HEAD", + "REBASE_HEAD", + "CHERRY_PICK_HEAD", + "REVERT_HEAD", + "BISECT_HEAD", + "AUTO_MERGE", +}; + struct ref_namespace_info ref_namespace[] = { [NAMESPACE_HEAD] = { .ref = "HEAD", @@ -1549,6 +1564,33 @@ int head_ref(each_ref_fn fn, void *cb_data) return refs_head_ref(get_main_ref_store(the_repository), fn, cb_data); } +int refs_for_each_pseudoref(struct ref_store *refs, + each_ref_fn fn, void *cb_data) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(pseudorefs); i++) { + struct object_id oid; + int flag; + + if (refs_resolve_ref_unsafe(refs, pseudorefs[i], + RESOLVE_REF_READING, &oid, &flag)) { + int ret = fn(pseudorefs[i], &oid, flag, cb_data); + + if (ret) + return ret; + } + } + + return 0; +} + +int for_each_pseudoref(each_ref_fn fn, void *cb_data) +{ + return refs_for_each_pseudoref(get_main_ref_store(the_repository), + fn, cb_data); +} + struct ref_iterator *refs_ref_iterator_begin( struct ref_store *refs, const char *prefix, diff --git a/refs.h b/refs.h index 23211a5ea1..7b55cced31 100644 --- a/refs.h +++ b/refs.h @@ -320,6 +320,8 @@ typedef int each_repo_ref_fn(struct repository *r, */ int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data); +int refs_for_each_pseudoref(struct ref_store *refs, + each_ref_fn fn, void *cb_data); int refs_for_each_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data); int refs_for_each_ref_in(struct ref_store *refs, const char *prefix, @@ -334,6 +336,9 @@ int refs_for_each_remote_ref(struct ref_store *refs, /* just iterates the head ref. */ int head_ref(each_ref_fn fn, void *cb_data); +/* iterates pseudorefs. */ +int for_each_pseudoref(each_ref_fn fn, void *cb_data); + /* iterates all refs. */ int for_each_ref(each_ref_fn fn, void *cb_data);
Define const array 'pseudorefs' with the names of the pseudorefs that are documented in gitrevisions.1, and add functions for_each_pseudoref() and refs_for_each_pseudoref() for iterating over them. The functions process the pseudorefs in the same way as head_ref() and refs_head_ref() process HEAD, invoking an each_ref_fn callback on each pseudoref that exists. This is in preparation for adding pseudorefs to log decorations. Signed-off-by: Andy Koppe <andy.koppe@gmail.com> --- refs.c | 42 ++++++++++++++++++++++++++++++++++++++++++ refs.h | 5 +++++ 2 files changed, 47 insertions(+)