Re: [PATCH v2] pretty: add %(decorate[:<options>]) format
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Jul 23, 2023, 16:25 UTC
- Message-ID
- <783011d8-53ea-15cb-a9c7-6cb0c15bd5aa@gmail.com>
- In-Reply-To
- <kl6l351j22dr.fsf@chooglen-macbookpro.roam.corp.google.com>
Hi Andy
On 19/07/2023 19:16, Glen Choo wrote:
Show 8 quoted lines
>> case 'D':
>> - format_decorations_extended(sb, commit, c->auto_color, "", ", ", "");
>> + format_decorations(sb, commit, c->auto_color,
>> + &(struct decoration_options){"", ""});
>
> I don't remember if C99 lets you name .prefix and .suffix here, but if
> so, it would be good to name them. Otherwise it's easy to get the order
> wrong, e.g. if someone reorders the fields in struct decoration_options.That's a good suggestion. I think this would be the first use of a compound literal in the code base so it would be helpful to mention that in the commit message.
We've been depending on C99 for a while now so I'd support adding this compound literal as a test balloon for compiler support. Ævar reported a while back that they are supported by IBM xlc, Oracle SunCC and HP/UX's aCC[1] and back then I looked at NonStop which seemed to offer support with the right compiler flag.
Overall this is a well written, well motivated patch with a good commit message.
Best Wishes
Phillip
[1] https://lore.kernel.org/git/87h7e61duk.fsf@evledraar.gmail.com/