Re: [PATCH] pretty: add %r format specifier for showing refs

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

Eric Sunshine <sunshine@sunshineco.com> 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.