On 10/26/21 1:25 AM, Eric Sunshine wrote:
> On Mon, Oct 25, 2021 at 9:36 PM Eli Schwartz <eschwartz@xxxxxxxxxxxxx> wrote:
>> The %(describe) placeholder by default, like `git describe`, only
>> supports annotated tags. However, some people do use lightweight tags
>> for releases, and would like to describe those anyway. The command line
>> tool has an option to support this.
>>
>> Teach the placeholder to support this as well.
>>
>> Signed-off-by: Eli Schwartz <eschwartz@xxxxxxxxxxxxx>
>> ---
>> diff --git a/pretty.c b/pretty.c
>> @@ -1229,10 +1230,21 @@ static size_t parse_describe_args(const char *start, struct strvec *args)
>> for (i = 0; !found && i < ARRAY_SIZE(option); i++) {
>> switch(option[i].type) {
>> + case OPT_BOOL:
>> + if(match_placeholder_bool_arg(arg, option[i].name, &arg, &optval)) {
>
> Style nit: add space after `if`
Oops, I am not sure how this happened. It's wrong in the switch too.
>> + if (optval) {
>> + strvec_pushf(args, "--%s", option[i].name);
>> + } else {
>> + strvec_pushf(args, "--no-%s", option[i].name);
>> + }
>
> We would normally omit the braces for this simple `if`:
>
> if (optval)
> strvec_pushf(...);
> else
> strvec_pushf(...);
>
> ... or maybe even use the ternary operator:
>
> strvec_pushf(args, "--%s%s", optval ? "" : "no-", option[i].name);
>
> but it's highly subjective whether or not that's more readable.
Although the braces feel more natural to me for clarity purposes, it's a
good point that the git coding style says to omit them for single
statements, and I should have followed that here.
The ternary doesn't feel readable to me, however.
...
Thanks for the style review!
--
Eli Schwartz
Arch Linux Bug Wrangler and Trusted User
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
