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

From: Martin Ågren <martin.agren@gmail.com>
Date: May 20, 2018, 09:15 UTC
Message-ID: <CAN0heSpdtiUVSYGsT+VnUaSy0BO+p+mnod6Ut4Y3UrNVVQkrbA@mail.gmail.com>
In-Reply-To: <20180520081244.14619-1-pclouds@gmail.com>

On 20 May 2018 at 10:12, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> 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.

Show 15 quoted lines

> 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.

Show 20 quoted lines

> 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", &state.message_id,
>                         N_("pass -m flag to git-mailinfo")),
> -               { OPTION_SET_INT, 0, "keep-cr", &keep_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", &keep_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", &keep_cr,
> +                       N_("pass --keep-cr flag to git-mailsplit for mbox format"),
> +                       1, PARSE_OPT_NONEG),
> +               OPT_SET_INT_F(0, "no-keep-cr", &keep_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