Re: [PATCH v2 2/3] pretty: add tag option to %(describe)
- From
Eli Schwartz <eschwartz@archlinux.org>
- Date
- Oct 26, 2021, 20:06 UTC
- Message-ID
- <5e2233d8-f63c-d786-ee9f-31865d6c208c@archlinux.org>
- In-Reply-To
- <CAPig+cTe9iMCteUYZZP_8cYoOzbg-95ptuVdvvk0SKUGMgrDjg@mail.gmail.com>
On 10/26/21 1:25 AM, Eric Sunshine wrote:
Show 18 quoted lines
> 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.
Show 18 quoted lines
>> + 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