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

From: Andy Koppe <andy.koppe@gmail.com>
Date: Jul 12, 2023, 20:47 UTC
Message-ID: <CAHWeT-agn87wc82xdMzB07Y=xe6H-yR_oxS_CGf2tE-szQ=T-Q@mail.gmail.com>
In-Reply-To: <xmqqa5w1t2kp.fsf@gitster.g>

Show 7 quoted lines

> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> Not a proper review... just running my eye quickly over the patch...
>> ...
>> Missing sign-off.
>>
>> Indent with TAB, not spaces.

Thanks for the check, and apologies for those avoidable mistakes. Must remember to run checkpatch …

I'll send a corrected patch, if only for completeness.

Show 5 quoted lines

>>> +enum decoration_format {
>> 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".

I took existing decoration-related types as precedent, in particular enum decoration_type and structs decoration_entry and decoration_filter, whereby the latter is in the same header.

Junio C Hamano <gitster@pobox.com> writes:

Show 5 quoted lines

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

The proposed %r placeholder basically is the minimised version of %d, which could save space in one-line logs and generally reduce visual noise in custom log formats. Post-processing is rather more difficult and error-prone than a built-in feature.

Show 5 quoted lines

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

I'd be happy to look into that.

What have you got in mind for the <options>?

Something like:
  %(decorate) for %d
  %(decorate:unwrapped) for %D
  %(decorate:bare) instead of the proposed %r

Or something with separate options for each element, similar to the separator option of %(trailers)?

%r might look as follows, with a space for the separator and empty strings for the other elements:

  %(decorate:prefix=,separator= ,suffix=,tag=)

(Each option would default to its %d value if not specified.)

Thanks, Andy