> Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 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.
>>> +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@xxxxxxxxx> writes:
> 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.
> 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
