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

[Junio C Hamano <gitster@pobox.com>]

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

> This patch series adds three slots to the color.decorate.<slot> config
> option:
> - 'symbol' for coloring the punctuation symbols used around the refs in
>   decorations, which currently use the same color as the commit hash.
> - 'ref' for coloring refs other than branches, remote-tracking branches,
>   tags and the stash, which currently are not colored when included in
>   decorations through custom decoration filter options.
> - 'pseudoref' for coloring pseudorefs such as ORIG_HEAD or MERGE_HEAD.
>   Include them in decorations by default.
>
> This series is to replace the 'decorate: add color.decorate.symbols
> config option' patch proposed at:
> https://lore.kernel.org/git/20231003205442.22963-1-andy.koppe@gmail.com

If that is the case, it probably would have been nicer to mark the
series as [PATCH v2].

Also, can you make messages [1/7]..[7/7] replies to [0/7] when you
send them out?  It seems that all 8 of them (including the cover
letter) are replies to the previous round, which looked a bit
unusual.


As to the contents of the series:

 [1/7] nicely lays out the color documentation; I do not think the
       extra verbosity was absolutely needed for existing ones
       (e.g., when a reader sees 'tag', the reader knows the color
       will be applied to tags), but the more exotic ones the series
       will be adding may deserve extra explanation on what they
       are, so I guess it is OK.

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

 [3/7] They way _NIL color is used to control the defaulting looked
       a bit unusual, but clever way to use a non-constant color
       defined elsewhere as its default.  A similar trick is used in
       wt-status.c:color() for STATUS_ONBRANCH, so this is nothing
       new.

 [4/7] The name of new member .include added to ref_namespace_info
       will not be understood by anybody unless they are too deeply
       obsessed by decoration mechansim.  As the namespace_info
       covers far wider interest, so a name that *shouts* that it is
       about decoration filter must be used to be understood by
       readers of the code.

       To be quite honest, "decoration filter" is probably a name
       that will not be understood by anybody, but coming up with a
       better name for it is probably outside the scope of this
       series.

 [5/7] I am not sure if "other refs" should be an item in the
       namespace_info array.  If it is truly "catch-all", then
       shouldn't the refs in other namespaces without their own
       decoration (e.g. ones in refs/notes/ and refs/prefetch/) be
       colored in the same way as this new class?  And if so, having
       it as an independent element that sits next to these other
       classes smells like a strange design.

       Another more worrying thing is that existing .ref members are
       designed to never overlap with each other, but this one
       obviously does.  When a caller with a ref (or a pseudoref)
       asks "which namespace does this one belong to", does the
       existing code still do the right thing with this new element?
       Without it, because there was no overlap, an implementation
       can randomly search in the namespace_info table and stop at
       the first hit, but now with the overlapping and widely open
       .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).

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

           int is_per_worktree_ref(const char *) {
		   return starts_with(refname, "refs/worktree/") ||
			  starts_with(refname, "refs/bisect/") ||
			  starts_with(refname, "refs/rewritten/");
	   }
           int is_pseudoref_syntax(const char *);
           int is_current_worktree_ref(const char *ref) {
                   return is_pseudoref_syntax(ref) || is_per_worktree_ref(ref);
           }

       All these three work on the refname and based on what is in
       that refname string, decides what kind of ref it is.  There
       is nothing especially "syntax" about the second one, and we
       should rename it as part of #leftoverbits clean-up effort.

       Another unrelated tangent is that is_per_worktree_ref() shown
       above and the namespace_info array we saw earlier are not
       even aware of each other, which is maintenance nightmare
       waiting to happen.

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

       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.  You do not want to say "I want
       to decorate off of ORIG_HEAD and FETCH_HEAD"; instead you
       would want to say "I want to decorate off of any pseudoref".