Re: [PATCH v2 2/3] pretty: add tag option to %(describe)

[Eli Schwartz <eschwartz@archlinux.org>]

[-- Attachment #1.1: Type: text/plain, Size: 2100 bytes --]

On 10/26/21 1:25 AM, Eric Sunshine wrote:
> On Mon, Oct 25, 2021 at 9:36 PM Eli Schwartz <eschwartz@archlinux.org> 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@archlinux.org>
>> ---
>> 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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]