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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 12, 2023, 18:19 UTC
Message-ID: <xmqqa5w1t2kp.fsf@gitster.g>
In-Reply-To: <CAPig+cT3tWnSfk+ZoRWU=JTGctMiE38fH5V1ebpP7L1bGsfU4Q@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 22 quoted lines

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