From: Phillip Wood <phillip.wood123@gmail.com>
Date: Sun, 23 Jul 2023 16:25:58 GMT
Subject: Re: [PATCH v2] pretty: add %(decorate[:<options>]) format
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:
>>   	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/