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

On Sun, May 20, 2018 at 11:15 AM, Martin Ågren [off-list ref] wrote:

On 20 May 2018 at 10:12, Nguyễn Thái Ngọc Duy [off-list ref] wrote:

quoted

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.

quoted

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.

quoted

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?

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