Re: [PATCH] Use OPT_SET_INT_F() for cmdline option specification

[Duy Nguyen]

On Sun, May 20, 2018 at 11:15 AM, Martin Ågren  wrote:
> On 20 May 2018 at 10:12, Nguyễn Thái Ngọc Duy  wrote:
>> The only thing these commands need is extra parseopt flags which can be
>> passed in by OPT_SET_INT_F() and it is a bit more compact than full
>> struct initialization.
>
>> diff --git a/archive.c b/archive.c
>> index 93ab175b0b..4fe7bec60c 100644
>> --- a/archive.c
>> +++ b/archive.c
>> @@ -411,11 +411,9 @@ static void parse_treeish_arg(const char **argv,
>>  }
>>
>>  #define OPT__COMPR(s, v, h, p) \
>> -   { OPTION_SET_INT, (s), NULL, (v), NULL, (h), \
>> - PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, (p) }
>> +   OPT_SET_INT_F(s, NULL, v, h, p, PARSE_OPT_NONEG)
>>  #define OPT__COMPR_HIDDEN(s, v, p) \
>> -   { OPTION_SET_INT, (s), NULL, (v), NULL, "", \
>> - PARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_HIDDEN, NULL, (p) }
>> +   OPT_SET_INT_F(s, NULL, v, "", p, PARSE_OPT_NONEG | PARSE_OPT_HIDDEN)
>
> Right. We have NULLs in the fifth and the second-to-last positions, and
> we use PARSE_OPT_NOARG.  By switching to OPT_SET_INT_F we get those for
> free.
>
> Do we want to keep "(s)" instead of "s", just to be safe? And same for
> "(v)", "(p)". Macro expansion always makes me paranoid.

They are still wrapped in () in the end by OPT_SET_INT_F() so the
expanded struct initialization  is the same. I think we're fine here.

>> diff --git a/builtin/am.c b/builtin/am.c
>> index d834f9e62b..666287b497 100644
>> --- a/builtin/am.c
>> +++ b/builtin/am.c
>> @@ -2231,12 +2231,12 @@ int cmd_am(int argc, const char **argv, const char 
>> *prefix)
>> N_("pass -b flag to git-mailinfo"), KEEP_NON_PATCH),
>> OPT_BOOL('m', "message-id", _id,
>> N_("pass -m flag to git-mailinfo")),
>> -   { OPTION_SET_INT, 0, "keep-cr", _cr, NULL,
>> - N_("pass --keep-cr flag to git-mailsplit for mbox format"),
>> - PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 1},
>> -   { OPTION_SET_INT, 0, "no-keep-cr", _cr, NULL,
>> - N_("do not pass --keep-cr flag to git-mailsplit 
>> independent of am.keepcr"),
>> - PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 0},
>> +   OPT_SET_INT_F(0, "keep-cr", _cr,
>> +   N_("pass --keep-cr flag to git-mailsplit for mbox 
>> format"),
>> +   1, PARSE_OPT_NONEG),
>> +   OPT_SET_INT_F(0, "no-keep-cr", _cr,
>> +   N_("do not pass --keep-cr flag to git-mailsplit for 
>> mbox format"),
>> +   0, PARSE_OPT_NONEG),
>
> I found `-w` and `--word-diff` useful. You actually change the N_("...")
> for `--no-keep-cr` here: [-independent of am.keepcr-]{+for mbox format+}
> Copy-paste mistake?

Oops. You're correct. Fixed in v2.

>
> Other than that, `--word-diff` has a very structured appearance and
> nothing stood out. The ordering is different (f goes at the end in the
> post-image), which makes the diff busier than it would have had to be.
> (That's obviously nothing this patch can do anything about.)
>
> Martin
-- 
Duy

Re: [PATCH] Use OPT_SET_INT_F() for cmdline option specification

[Martin Ågren]

On 20 May 2018 at 10:12, Nguyễn Thái Ngọc Duy  wrote:
> The only thing these commands need is extra parseopt flags which can be
> passed in by OPT_SET_INT_F() and it is a bit more compact than full
> struct initialization.

> diff --git a/archive.c b/archive.c
> index 93ab175b0b..4fe7bec60c 100644
> --- a/archive.c
> +++ b/archive.c
> @@ -411,11 +411,9 @@ static void parse_treeish_arg(const char **argv,
>  }
>
>  #define OPT__COMPR(s, v, h, p) \
> -   { OPTION_SET_INT, (s), NULL, (v), NULL, (h), \
> - PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, (p) }
> +   OPT_SET_INT_F(s, NULL, v, h, p, PARSE_OPT_NONEG)
>  #define OPT__COMPR_HIDDEN(s, v, p) \
> -   { OPTION_SET_INT, (s), NULL, (v), NULL, "", \
> - PARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_HIDDEN, NULL, (p) }
> +   OPT_SET_INT_F(s, NULL, v, "", p, PARSE_OPT_NONEG | PARSE_OPT_HIDDEN)

Right. We have NULLs in the fifth and the second-to-last positions, and
we use PARSE_OPT_NOARG.  By switching to OPT_SET_INT_F we get those for
free.

Do we want to keep "(s)" instead of "s", just to be safe? And same for
"(v)", "(p)". Macro expansion always makes me paranoid.

> diff --git a/builtin/am.c b/builtin/am.c
> index d834f9e62b..666287b497 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -2231,12 +2231,12 @@ int cmd_am(int argc, const char **argv, const char 
> *prefix)
> N_("pass -b flag to git-mailinfo"), KEEP_NON_PATCH),
> OPT_BOOL('m', "message-id", _id,
> N_("pass -m flag to git-mailinfo")),
> -   { OPTION_SET_INT, 0, "keep-cr", _cr, NULL,
> - N_("pass --keep-cr flag to git-mailsplit for mbox format"),
> - PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 1},
> -   { OPTION_SET_INT, 0, "no-keep-cr", _cr, NULL,
> - N_("do not pass --keep-cr flag to git-mailsplit independent 
> of am.keepcr"),
> - PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 0},
> +   OPT_SET_INT_F(0, "keep-cr", _cr,
> +   N_("pass --keep-cr flag to git-mailsplit for mbox 
> format"),
> +   1, PARSE_OPT_NONEG),
> +   OPT_SET_INT_F(0, "no-keep-cr", _cr,
> +   N_("do not pass --keep-cr flag to git-mailsplit for 
> mbox format"),
> +   0, PARSE_OPT_NONEG),

I found `-w` and `--word-diff` useful. You actually change the N_("...")
for `--no-keep-cr` here: [-independent of am.keepcr-]{+for mbox format+}
Copy-paste mistake?

Other than that, `--word-diff` has a very structured appearance and
nothing stood out. The ordering is different (f goes at the end in the
post-image), which makes the diff busier than it would have had to be.
(That's obviously nothing this patch can do anything about.)

Martin