On 22/10/2023 01:13, Junio C Hamano wrote:
Andy Koppe <andy.koppe@xxxxxxxxx> writes:
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@xxxxxxxxx
If that is the case, it probably would have been nicer to mark the
series as [PATCH v2].
Thanks, I wasn't sure about that due to the change in title and increase
in scope. I shall err towards version-bumping in any future such cases.
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.
Not quite sure how that happened, but I think my mistake was passing
--in-reply-to to git-format-patch instead of git-send-email.
[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.)
[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
Agreed.
[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?
They would, because add_ref_decoration() skips ref_namespace entries
without a decoration type, so they would fall through to "refs/" and
pick up the DECORATION_REF type.
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).
Fair points. I've rewritten things to not touch the ref_namespace array.
[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. I had vacillated about shoe-horning the pseudorefs in there,
and was swayed by having a single place to define which (pseudo)refs
should be included in decorations by default. That motivation goes away
with all the pseudorefs off by default.
I've rewritten things to handle the pseudorefs separately from the
ref_namespace array, with iteration functions similar to the ones used
for HEAD and proper refs.
> 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".
They can now all be enabled with --clear-decorations or
log.initialDecorationSet=all, or be controlled individually with the
other filter options.
Thank you very much for the review!
Andy
