On 23/10/2023 01:20, Junio C Hamano wrote:
Andy Koppe <andy.koppe@xxxxxxxxx> writes:
[2/7] is a trivial readability improvement. It obviously should be
left outside the scope of this series, but we should notice
the same pattern in similar color tables (e.g., wt-status.c
has one, diff.c has another) and perform the same clean-up as
a #leftoverbits item.
Okay, I've removed that commit in v2. (I should have mentioned in the
commit message that it was triggered by the inconsistency with the
immediately following color_decorate_slots array, which uses
designated initializers.)
Sorry, that is not what I meant. [2/7] as a preliminary clean-up to
work in the same area does make very much sense. What I meant to be
"outside the scope" was to make similar fixes to other color tables
that this series does not care about.
Ah, sorry for misreading. Commit reinstated in v3.
Fair enough, although the array already contains HEAD and refs/stash
as singletons.
But these deserve to be singletons, don't they? There is no other
thing that behaves like HEAD; there is no other thing that behaves
like stash; and they do not behave like each other.
They do indeed, but arguably the pseudorefs are singletons rather than a
namespace like refs/heads as well, as there is a defined and documented
set of them.
I've rewritten things to not touch the ref_namespace array.
Well, the namespace_info mechanism still may be a good place to have
the necessary information; it may be that the current implementation
detail of how a given ref is classified to one of the namespaces is
too limiting---it essentially allows the string match with the .ref
member. But we can imagine that it could be extended a bit, e.g.
struct ref_namespace_info {
char *ref;
int (*membership)(const char *, const struct ref_namespace_info *);
... other members ...;
};
where the .membership member is used in add_ref_decoration() to
determine the membership of a given "refname" to the namespace "i"
perhaps like so:
struct ref_namespace_info *info = &ref_namespace[i];
if (!info->decoration)
continue;
+ if (info->membership) {
+ if (info->membership(refname, info)) {
+ deco_type = info->decoration;
+ break;
+ }
+ } else if (info->exact) {
- if (info->exact) {
if (!strcmp(refname, info->ref)) {
deco_type = info_decoration;
break;
}
Then you can arrange the pseudoref class to use .membership function
perhaps like this:
static int pseudoref_namespace_membership(
const char *refname, const struct ref_namespace_info *info UNUSED
)
{
return is_pseudoref(refname);
}
and make them all into a single class.
That's an interesting idea, but I'm not convinced it would buy us much,
while also potentially complicating things for any other uses of the
ref_namespace array.
My premise here is that we do need a list of the documented pseudorefs,
so that we can iterate through them and add the ones that do exist to
the decorations, whereby I admit that shoe-horning that list into the
ref_namespace array wasn't a good idea. If that premise is wrong, and
there's a better way to discover the pseudorefs, the following might be
moot.
Sending each found pseudoref through add_ref_decoration() and its lookup
of ref_namespace would just confirm what we already know: it's a
pseudoref. Which is why both my initial attempt and the current one
don't actually invoke add_ref_decoration() for them.
Could you have a closer look at the current design? It handles the
pseudorefs separately from proper refs, with their own iteration and
callback functions, which I think makes for simpler more self-contained
changes than v1 or the approach suggested above.
Having said that, I do not think it makes much sense to decorate a
commit off of refs/stash, as the true richeness of the stash is not
in its history but in its reflog, which the decoration code does not
dig into. But obviously it is not a part of the topic we are
discussing (unless, of course, we are not "adding" new decoration
sources and colors, but we are improving the decoration sources and
colors by adding new useful ones while retiring existing useless
ones).
I agree refs/stash is a weird one, and that it could be subsumed into
the color.decoration.ref setting for 'refs/*' that I'm adding here,
which is also why I chose the same default color for it. I'd be happy to
drop color.decoration.stash if the minor break in compatibility for
anyone who has customized it is acceptable. The setting would be quietly
ignored.
Another related thought: the '--clear-decorations' option of git-log
seems unfortunately named as it suggests the opposite of what it
actually does, which is to enable all decorations (unless subsequently
constrained with '--decorate-refs{,--exclude}=...').
Regards,
Andy
