Re: [PATCH 0/7] log: decorate pseudorefs and other refs

[Andy Koppe <andy.koppe@gmail.com>]

On 23/10/2023 01:20, Junio C Hamano wrote:
> Andy Koppe <andy.koppe@gmail.com> 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