Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes:
> Not a proper review... just running my eye quickly over the patch...
> ...
>> Amend t4207-log-decoration-colors.sh to reflect the added color
>> controls, and t4202-log.sh to test the %r format.
>> ---
>
> Missing sign-off.
>
>> diff --git a/log-tree.h b/log-tree.h
>> @@ -13,17 +13,18 @@ struct decoration_filter {
>> +enum decoration_format {
>> + DECO_FMT_BARE = 0,
>> + DECO_FMT_UNWRAPPED,
>> + DECO_FMT_WRAPPED,
>> +};
>
> Indent with TAB, not spaces.
>
> Is this enum name a bit too generic for a public header? A quick scan
> of other enums in the project shows that they usually incorporate the
> "subsystem" into their names somehow (often as a prefix); for
> instance, "enum apply_ws_ignore", "enum bisect_error".
Everything you said makes sense.
But more importantly, I doubt the wisdom of adding any more %<single
letter> placeholders to the vocabulary. Even though I personally do
not see any need for variants other than just the plain "%d" to show
the "decorate" information (if you want anything else, just
post-process the output), if we really want to, the way we should
extend the format placeholders is to add %(decorate:<options>) that
is extensible enough that it can produce the identical output as
existing "%d" and "%D" placeholders do, and add new ones as a new
option to %(decorate).
Thanks.
