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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 23, 2023, 00:20 UTC
Message-ID: <xmqqpm16p4t3.fsf@gitster.g>
In-Reply-To: <fe3abed8-6be0-4d77-9057-79c9b7c0795c@gmail.com>

Andy Koppe <andy.koppe@gmail.com> writes:

Show 10 quoted lines

>>   [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.

Show 7 quoted lines

>>         .ref = "refs/", the implementation of the search must know
>>         that it is a fallback position (i.e. if it found a match with
>>         the fallback .ref = "refs/" , unless it looked at all other
>>         entries that could begin with "refs/" and are more specific,
>>         it needs to keep going).
>
> Fair points. 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.

What I called a bad design was to reuse the namespace_info code without extending it to suit our needs.

This comment will probably affect everything below.

Show 28 quoted lines

>>   [6/7] This is pretty straight-forward, assuming that the existing
>>         is_pseudoref_syntax() function does the right thing.  I am
>>         not sure about that, though.  A refname with '-' is allowed
>>         to be called a pseudoref???
>>         Also, not a fault of this patch, but the "_syntax" in its
>>         name is totally unnecessary, I would think.  At first glance,
>>         I suspected that the excuse to append _syntax may have been
>>         to signal the fact that the helper function does not check if
>>         there actually is such a ref, but examining a few helpers
>>         defined nearby tells us that such an excuse does not make
>>         sense:
>
> I've dropped the use of that function from the change, checking
> against the actual pseudoref names instead.
>
>>   [7/7] Allowing pseudorefs to optionally used when decorating might
>>         be a good idea, but I do not think it is particularly a good
>>         design decision to enable it by default.
>
> Okay!
>
>>         Each of them forming a separate "namespace" also looks like a
>>         poor design, as being able to group multiple things into one
>>         family and treat them the same way is the primary point of
>>         "namespace", I would think.
>
> 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.

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).

Thanks.