[PATCH] add --help option in parse_opts(), not at every call site

[Denys Vlasenko]

This patch removes the need to add --help option to every program.
It is added (and handled) by parse_opts().

Run tested. Please review.
-- 
vda



diff -x '*.po' -d -urpN abrt.0/lib/utils/parse_options.c
abrt.1/lib/utils/parse_options.c
--- abrt.0/lib/utils/parse_options.c	2010-10-05 17:23:41.000000000 +0200
+++ abrt.1/lib/utils/parse_options.c	2010-10-08 13:28:34.655230855 +0200
@@ -67,13 +67,18 @@ void parse_usage_and_die(const char * co
 void parse_opts(int argc, char **argv, const struct options *opt,
                 const char * const usage[])
 {
+    int help = 0;
     int size = parse_opt_size(opt);
 
     struct strbuf *shortopts = strbuf_new();
 
-    struct option *longopts = xcalloc(sizeof(struct options), size);
+    struct option *longopts = xzalloc(sizeof(longopts[0]) * (size+1));
+    longopts[0].name = "help";
+    /*longopts[0].has_arg = 0; - xzalloc did it */
+    longopts[0].flag = &help;
+    longopts[0].val = 1;
     int ii;
-    for (ii = 0; ii < size - 1; ++ii)
+    for (ii = 1; ii < size; ++ii)
     {
         longopts[ii].name = opt[ii].long_name;
 
@@ -97,21 +102,21 @@ void parse_opts(int argc, char **argv, c
         longopts[ii].flag = 0;
         longopts[ii].val = opt[ii].short_name;
     }
-
-    longopts[ii].name = 0;
+    /* xzalloc did it already:
+    longopts[ii].name = NULL;
     longopts[ii].has_arg = 0;
-    longopts[ii].flag = 0;
+    longopts[ii].flag = NULL;
     longopts[ii].val = 0;
+    */
 
-    int option_index = 0;
     while (1)
     {
-        int c = getopt_long(argc, argv, shortopts->buf, longopts, &option_index);
+        int c = getopt_long(argc, argv, shortopts->buf, longopts, /*longindex:*/
NULL);
 
         if (c == -1)
             break;
 
-        if (c == '?')
+        if (c == '?' || (c == 0 && help))
         {
             free(longopts);
             strbuf_free(shortopts);
diff -x '*.po' -d -urpN abrt.0/lib/utils/parse_options.h
abrt.1/lib/utils/parse_options.h
--- abrt.0/lib/utils/parse_options.h	2010-10-05 17:23:41.000000000 +0200
+++ abrt.1/lib/utils/parse_options.h	2010-10-08 13:30:26.604480641 +0200
@@ -38,7 +38,6 @@ struct options {
 #define OPT_STRING(s, l, v, a, h)   { OPTION_STRING, (s), (l), (v), (a), (h) }
 
 #define OPT__VERBOSE(v)     OPT_BOOL('v', "verbose", (v), "be
verbose")
-#define OPT__HELP(v)        OPT_BOOL('h', "help", (v), "show this
help message")
 
 void parse_opts(int argc, char **argv, const struct options *opt,
                 const char * const usage[]);
diff -x '*.po' -d -urpN abrt.0/lib/utils/xfuncs.c abrt.1/lib/utils/xfuncs.c
--- abrt.0/lib/utils/xfuncs.c	2010-10-05 17:23:41.000000000 +0200
+++ abrt.1/lib/utils/xfuncs.c	2010-10-08 13:31:24.878480382 +0200
@@ -39,6 +39,7 @@ int close_on_exec_on(int fd)
     return fcntl(fd, F_SETFD, FD_CLOEXEC);
 }
 
+#if 0 /* unused */
 void *xcalloc(size_t nmemb, size_t size)
 {
     void *ptr = calloc(nmemb, size);
@@ -46,6 +47,7 @@ void *xcalloc(size_t nmemb, size_t size)
         die_out_of_memory();
     return ptr;
 }
+#endif
 
 // Die if we can't allocate size bytes of memory.
 void* xmalloc(size_t size)
diff -x '*.po' -d -urpN abrt.0/src/daemon/abrt-action-generate-backtrace.c
abrt.1/src/daemon/abrt-action-generate-backtrace.c
--- abrt.0/src/daemon/abrt-action-generate-backtrace.c	2010-10-08 13:06:56.000000000
+0200
+++ abrt.1/src/daemon/abrt-action-generate-backtrace.c	2010-10-08 13:30:50.959230457
+0200
@@ -23,11 +23,6 @@
 
 #define DEBUGINFO_CACHE_DIR     LOCALSTATEDIR"/cache/abrt-di"
 
-static const char * const abrt_action_generage_backtrace_usage[] = {
-    "abrt-action-generate-backtrace [options] -d DIR",
-    NULL
-};
-
 static const char *dump_dir_name = ".";
 static char *debuginfo_dirs;
 static int exec_timeout_sec = 60;
@@ -246,10 +241,14 @@ static char *get_backtrace(struct dump_d
 }
 
 static char *d_opt, *i_opt;
-static int s_opt, help_opt;
+static int s_opt;
+
+static const char * const abrt_action_generage_backtrace_usage[] = {
+    "abrt-action-generate-backtrace [options] -d DIR",
+    NULL
+};
 
 static struct options abrt_action_generate_backtrace_options[] = {
-    OPT__HELP(&help_opt),
     OPT__VERBOSE(&g_verbose),
     OPT_GROUP(""),
     OPT_STRING( 'd' , 0, &d_opt, "dir", "Crash dump
directory"),
@@ -268,7 +267,7 @@ int main(int argc, char **argv)
     parse_opts(argc, argv, abrt_action_generate_backtrace_options,
                            abrt_action_generage_backtrace_usage);
 
-    if (help_opt || !d_opt)
+    if (!d_opt)
         parse_usage_and_die(abrt_action_generage_backtrace_usage,
                             abrt_action_generate_backtrace_options);
 
diff -x '*.po' -d -urpN abrt.0/src/daemon/abrt-action-save-package-data.cpp
abrt.1/src/daemon/abrt-action-save-package-data.cpp
--- abrt.0/src/daemon/abrt-action-save-package-data.cpp	2010-10-08 13:06:56.000000000
+0200
+++ abrt.1/src/daemon/abrt-action-save-package-data.cpp	2010-10-08 13:30:34.221480398
+0200
@@ -23,11 +23,6 @@
 #include "rpm.h"
 #include "parse_options.h"
 
-static const char * const abrt_action_save_package_data_usage[] = {
-    "abrt-action-save-package-data [options] -d DIR",
-    NULL
-};
-
 /**
  * Returns the first full path argument in the command line or NULL.
  * Skips options are in form "-XXX".
@@ -273,10 +268,14 @@ static int SavePackageDescriptionToDebug
 }
 
 static char *d_opt;
-static int s_opt, help_opt;
+static int s_opt;
+
+static const char * const abrt_action_save_package_data_usage[] = {
+    "abrt-action-save-package-data [options] -d DIR",
+    NULL
+};
 
 static struct options abrt_action_save_package_data_options[] = {
-    OPT__HELP(&help_opt),
     OPT__VERBOSE(&g_verbose),
     OPT_GROUP(""),
     OPT_STRING( 'd' , 0, &d_opt, "dir", "Crash dump
directory"),
@@ -295,7 +294,7 @@ int main(int argc, char **argv)
     parse_opts(argc, argv, abrt_action_save_package_data_options,
                            abrt_action_save_package_data_usage);
 
-    if (help_opt || !d_opt)
+    if (!d_opt)
         parse_usage_and_die(abrt_action_save_package_data_usage,
                             abrt_action_save_package_data_options);
 
diff -x '*.po' -d -urpN abrt.0/src/daemon/abrt-server.c
abrt.1/src/daemon/abrt-server.c
--- abrt.0/src/daemon/abrt-server.c	2010-10-08 13:06:56.000000000 +0200
+++ abrt.1/src/daemon/abrt-server.c	2010-10-08 13:30:20.911480328 +0200
@@ -76,11 +76,6 @@ Finalizing dump creation:
    \0
 */
 
-static const char * const abrt_server_usage[] = {
-    "abrt-server [options]",
-    NULL
-};
-
 /* Buffer for incomplete incoming messages. */
 static char *messagebuf_data = NULL;
 static unsigned messagebuf_len = 0;
@@ -282,10 +277,14 @@ static void process_message(const char *
 
 static void dummy_handler(int sig_unused) {}
 
-static int s_opt, help_opt;
+static int s_opt;
+
+static const char * const abrt_server_usage[] = {
+    "abrt-server [options]",
+    NULL
+};
 
 static struct options abrt_server_options[] = {
-    OPT__HELP(&help_opt),
     OPT__VERBOSE(&g_verbose),
     OPT_GROUP(""),
     OPT_INTEGER( 'u' , 0, &client_uid, "Use UID as client uid"),
@@ -299,14 +298,9 @@ int main(int argc, char **argv)
     if (env_verbose)
         g_verbose = atoi(env_verbose);
 
-
     parse_opts(argc, argv, abrt_server_options,
                            abrt_server_usage);
 
-    if (help_opt)
-        parse_usage_and_die(abrt_server_usage,
-                            abrt_server_options);
-
     putenv(xasprintf("ABRT_VERBOSE=%u", g_verbose));
     msg_prefix = xasprintf("abrt-server[%u]", getpid());
     if (s_opt)
diff -x '*.po' -d -urpN abrt.0/src/daemon/Daemon.cpp abrt.1/src/daemon/Daemon.cpp
--- abrt.0/src/daemon/Daemon.cpp	2010-10-08 13:06:56.000000000 +0200
+++ abrt.1/src/daemon/Daemon.cpp	2010-10-08 13:30:08.044480622 +0200
@@ -85,11 +85,6 @@ using namespace std;
  *      Both are sent as unicast to last client set by set_client_name(name).
  *      If set_client_name(NULL) was done, they are not sent.
  */
-static const char * const abrtd_usage[] = {
-    _("abrtd [options]"),
-    NULL
-};
-
 CCommLayerServer* g_pCommLayer;
 
 static bool daemonize = true;
@@ -865,11 +860,15 @@ static void sanitize_dump_dir_rights()
     ensure_writable_dir(VAR_RUN"/abrt", 0755, "root");
 }
 
-static int daemonize_opt, syslog_opt, help_opt;
+static int daemonize_opt, syslog_opt;
 static char *timeout_opt;
 
+static const char * const abrtd_usage[] = {
+    _("abrtd [options]"),
+    NULL
+};
+
 static struct options abrtd_options[] = {
-    OPT__HELP(&help_opt),
     OPT__VERBOSE(&g_verbose),
     OPT_GROUP(""),
     OPT_BOOL( 'd' , 0, &daemonize_opt, _("Do not daemonize")),
@@ -898,9 +897,6 @@ int main(int argc, char** argv)
 
     parse_opts(argc, argv, abrtd_options, abrtd_usage);
 
-    if (help_opt)
-        parse_usage_and_die(abrtd_usage, abrtd_options);
-
     if (daemonize_opt)
         daemonize = false;

[PATCH] Remove arrays in usage text, use a single string instead

[Denys Vlasenko]

Currently, usage is an array of pointers to string.
It is meant to support this:

usage: foo --bar
   or: foo --baz
   or: foo -z

The patch removes this and makes usage just a string:

usage: foo --bar

Why remove this?

(1) We can achieve old format by using the usage string with many lines:
usage = "foo --bar\n"
"   or: foo --baz\n"
"   or: foo -z";

(2) We don't use multi-line usage anyway.

Run tested.

Please review.
-- 
vda



diff -x '*.po' -d -urpN abrt.1/lib/utils/parse_options.c
abrt.2/lib/utils/parse_options.c
--- abrt.1/lib/utils/parse_options.c	2010-10-08 13:58:00.822230377 +0200
+++ abrt.2/lib/utils/parse_options.c	2010-10-08 13:58:26.789480231 +0200
@@ -15,12 +15,9 @@ static int parse_opt_size(const struct o
 #define USAGE_OPTS_WIDTH 24
 #define USAGE_GAP         2
 
-void parse_usage_and_die(const char * const * usage, const struct options *opt)
+void parse_usage_and_die(const char *usage, const struct options *opt)
 {
-    fprintf(stderr, _("usage: %s\n"), *usage++);
-
-    while (*usage && **usage)
-        fprintf(stderr, _("   or: %s\n"), *usage++);
+    fprintf(stderr, _("Usage: %s\n"), usage);
 
     if (opt->type != OPTION_GROUP)
         fputc('\n', stderr);
@@ -65,7 +62,7 @@ void parse_usage_and_die(const char * co
 }
 
 void parse_opts(int argc, char **argv, const struct options *opt,
-                const char * const usage[])
+                const char *usage)
 {
     int help = 0;
     int size = parse_opt_size(opt);
@@ -128,7 +125,6 @@ void parse_opts(int argc, char **argv, c
         {
             if (opt[ii].short_name == c)
             {
-
                 switch (opt[ii].type)
                 {
                     case OPTION_BOOL:
diff -x '*.po' -d -urpN abrt.1/lib/utils/parse_options.h
abrt.2/lib/utils/parse_options.h
--- abrt.1/lib/utils/parse_options.h	2010-10-08 13:30:26.604480641 +0200
+++ abrt.2/lib/utils/parse_options.h	2010-10-08 13:45:05.351480477 +0200
@@ -37,12 +37,12 @@ struct options {
 #define OPT_INTEGER(s, l, v, h)     { OPTION_INTEGER, (s), (l), (v), "n", (h)
}
 #define OPT_STRING(s, l, v, a, h)   { OPTION_STRING, (s), (l), (v), (a), (h) }
 
-#define OPT__VERBOSE(v)     OPT_BOOL('v', "verbose", (v), "be
verbose")
+#define OPT__VERBOSE(v)     OPT_BOOL('v', "verbose", (v), "Be
verbose")
 
 void parse_opts(int argc, char **argv, const struct options *opt,
-                const char * const usage[]);
+                const char *usage);
 
-void parse_usage_and_die(const char * const * usage, const struct options *opt);
+void parse_usage_and_die(const char *usage, const struct options *opt);
 
 #ifdef __cplusplus
 }
diff -x '*.po' -d -urpN abrt.1/src/daemon/abrt-action-generate-backtrace.c
abrt.2/src/daemon/abrt-action-generate-backtrace.c
--- abrt.1/src/daemon/abrt-action-generate-backtrace.c	2010-10-08 13:30:50.959230457
+0200
+++ abrt.2/src/daemon/abrt-action-generate-backtrace.c	2010-10-08 14:06:36.810230965
+0200
@@ -243,10 +243,7 @@ static char *get_backtrace(struct dump_d
 static char *d_opt, *i_opt;
 static int s_opt;
 
-static const char * const abrt_action_generage_backtrace_usage[] = {
-    "abrt-action-generate-backtrace [options] -d DIR",
-    NULL
-};
+static const char abrt_action_generage_backtrace_usage[] =
"abrt-action-generate-backtrace [options] -d DIR";
 
 static struct options abrt_action_generate_backtrace_options[] = {
     OPT__VERBOSE(&g_verbose),
diff -x '*.po' -d -urpN abrt.1/src/daemon/abrt-action-save-package-data.cpp
abrt.2/src/daemon/abrt-action-save-package-data.cpp
--- abrt.1/src/daemon/abrt-action-save-package-data.cpp	2010-10-08 13:30:34.221480398
+0200
+++ abrt.2/src/daemon/abrt-action-save-package-data.cpp	2010-10-08 14:09:48.925231033
+0200
@@ -270,10 +270,7 @@ static int SavePackageDescriptionToDebug
 static char *d_opt;
 static int s_opt;
 
-static const char * const abrt_action_save_package_data_usage[] = {
-    "abrt-action-save-package-data [options] -d DIR",
-    NULL
-};
+static const char abrt_action_save_package_data_usage[] =
"abrt-action-save-package-data [options] -d DIR";
 
 static struct options abrt_action_save_package_data_options[] = {
     OPT__VERBOSE(&g_verbose),
diff -x '*.po' -d -urpN abrt.1/src/daemon/abrt-server.c
abrt.2/src/daemon/abrt-server.c
--- abrt.1/src/daemon/abrt-server.c	2010-10-08 13:30:20.911480328 +0200
+++ abrt.2/src/daemon/abrt-server.c	2010-10-08 13:41:39.833480840 +0200
@@ -279,10 +279,7 @@ static void dummy_handler(int sig_unused
 
 static int s_opt;
 
-static const char * const abrt_server_usage[] = {
-    "abrt-server [options]",
-    NULL
-};
+static const char abrt_server_usage[] = "abrt-server [options]";
 
 static struct options abrt_server_options[] = {
     OPT__VERBOSE(&g_verbose),
diff -x '*.po' -d -urpN abrt.1/src/daemon/Daemon.cpp abrt.2/src/daemon/Daemon.cpp
--- abrt.1/src/daemon/Daemon.cpp	2010-10-08 13:30:08.044480622 +0200
+++ abrt.2/src/daemon/Daemon.cpp	2010-10-08 14:07:54.858480511 +0200
@@ -863,10 +863,7 @@ static void sanitize_dump_dir_rights()
 static int daemonize_opt, syslog_opt;
 static char *timeout_opt;
 
-static const char * const abrtd_usage[] = {
-    _("abrtd [options]"),
-    NULL
-};
+static const char* abrtd_usage = _("abrtd [options]");
 
 static struct options abrtd_options[] = {
     OPT__VERBOSE(&g_verbose),

[PATCH] Remove arrays in usage text, use a single string instead

[Nikola Pajkovsky]

On 10/08/2010 02:10 PM, Denys Vlasenko wrote:
>  Currently, usage is an array of pointers to string.
>  It is meant to support this:
>
>  usage: foo --bar
>      or: foo --baz
>      or: foo -z
>
>  The patch removes this and makes usage just a string:
>
>  usage: foo --bar
>
>  Why remove this?
>
>  (1) We can achieve old format by using the usage string with many lines:
>  usage = "foo --bar\n"
>  "   or: foo --baz\n"
>  "   or: foo -z";
>
>  (2) We don't use multi-line usage anyway.
>
>  Run tested.
>
>  Please review. 
+#define OPT__VERBOSE(v)     OPT_BOOL('v', "verbose", (v), "Be
verbose")

Why upper 'B' in `Be verbose'?

First I don't see in any helps, second it looks bad and third I don't like it.

-- 
Nikola

[PATCH] Remove arrays in usage text, use a single string instead

[Denys Vlasenko]

On Fri, 2010-10-08 at 14:23 +0200, Nikola Pajkovsky wrote:
>  On 10/08/2010 02:10 PM, Denys Vlasenko wrote:
>  > Currently, usage is an array of pointers to string.
>  > It is meant to support this:
>  >
>  > usage: foo --bar
>  >     or: foo --baz
>  >     or: foo -z
>  >
>  > The patch removes this and makes usage just a string:
>  >
>  > usage: foo --bar
>  >
>  > Why remove this?
>  >
>  > (1) We can achieve old format by using the usage string with many lines:
>  > usage = "foo --bar\n"
>  > "   or: foo --baz\n"
>  > "   or: foo -z";
>  >
>  > (2) We don't use multi-line usage anyway.
>  >
>  > Run tested.
>  >
>  > Please review.
>  
>  +#define OPT__VERBOSE(v)     OPT_BOOL('v', "verbose", (v), "Be
> verbose")
>  
>  Why upper 'B' in `Be verbose'?
>  
>  First I don't see in any helps, second it looks bad and third I don't like it.
The usage text currently looks like this:

# abrtd --help
usage: abrtd [options]
    -v, --verbose         be verbose

    -d                    Do not daemonize
    -s                    Log to syslog even with -d
    -t n                  Exit after SEC seconds of inactivity

The option descriptions should be consistent: either all start with a
capital letter, or all start with a small one. Currently, they are not
consistent.

So, should we make them all small or all capital?

I use capitalized description because sometimes there is more than one
sentence. Random example:

   -C N[:MSG]      Allow only up to N connections from the same IP.
                   New connections from this IP address are closed
                   immediately. MSG is written to the peer before close.

having it start with a small letter and then suddenly turn into
properly punctuated sentences would be weird.

-- 
vda

[PATCH] Remove arrays in usage text, use a single string instead

[Nikola Pajkovsky]

On 10/08/2010 03:06 PM, Denys Vlasenko wrote:
>  On Fri, 2010-10-08 at 14:23 +0200, Nikola Pajkovsky wrote:
> > On 10/08/2010 02:10 PM, Denys Vlasenko wrote:
> >> Currently, usage is an array of pointers to string.
> >> It is meant to support this:
> >>
> >> usage: foo --bar
> >>      or: foo --baz
> >>      or: foo -z
> >>
> >> The patch removes this and makes usage just a string:
> >>
> >> usage: foo --bar
> >>
> >> Why remove this?
> >>
> >> (1) We can achieve old format by using the usage string with many lines:
> >> usage = "foo --bar\n"
> >> "   or: foo --baz\n"
> >> "   or: foo -z";
> >>
> >> (2) We don't use multi-line usage anyway.
> >>
> >> Run tested.
> >>
> >> Please review.
> >
> > +#define OPT__VERBOSE(v)     OPT_BOOL('v', "verbose", (v), "Be
> verbose")
> >
> > Why upper 'B' in `Be verbose'?
> >
> > First I don't see in any helps, second it looks bad and third I don't like
> it.
>
>  The usage text currently looks like this:
>
>  # abrtd --help
>  usage: abrtd [options]
>       -v, --verbose         be verbose
>
>       -d                    Do not daemonize
>       -s                    Log to syslog even with -d
>       -t n                  Exit after SEC seconds of inactivity
>
>  The option descriptions should be consistent: either all start with a
>  capital letter, or all start with a small one. Currently, they are not
>  consistent.
>
>  So, should we make them all small or all capital?
>  
I'd like to see all small.

>  I use capitalized description because sometimes there is more than
> one
>  sentence. Random example:
>
>      -C N[:MSG]      Allow only up to N connections from the same IP.
>                      New connections from this IP address are closed
>                      immediately. MSG is written to the peer before close.
>
>  having it start with a small letter and then suddenly turn into
>  properly punctuated sentences would be weird.
>  
Hmm, that's true but we have always only one sentence.

-- 
Nikola

[PATCH] Remove arrays in usage text, use a single string instead

[Denys Vlasenko]

On Fri, 2010-10-08 at 15:36 +0200, Nikola Pajkovsky wrote:
>  On 10/08/2010 03:06 PM, Denys Vlasenko wrote:
>  > On Fri, 2010-10-08 at 14:23 +0200, Nikola Pajkovsky wrote:
>  >> On 10/08/2010 02:10 PM, Denys Vlasenko wrote:
>  >>> Currently, usage is an array of pointers to string.
>  >>> It is meant to support this:
>  >>>
>  >>> usage: foo --bar
>  >>>      or: foo --baz
>  >>>      or: foo -z
>  >>>
>  >>> The patch removes this and makes usage just a string:
>  >>>
>  >>> usage: foo --bar
>  >>>
>  >>> Why remove this?
>  >>>
>  >>> (1) We can achieve old format by using the usage string with many lines:
>  >>> usage = "foo --bar\n"
>  >>> "   or: foo --baz\n"
>  >>> "   or: foo -z";
>  >>>
>  >>> (2) We don't use multi-line usage anyway.
>  >>>
>  >>> Run tested.
>  >>>
>  >>> Please review.
>  >>
>  >> +#define OPT__VERBOSE(v)     OPT_BOOL('v', "verbose", (v),
> "Be verbose")
>  >>
>  >> Why upper 'B' in `Be verbose'?
>  >>
>  >> First I don't see in any helps, second it looks bad and third I don't
> like it.
>  >
>  > The usage text currently looks like this:
>  >
>  > # abrtd --help
>  > usage: abrtd [options]
>  >      -v, --verbose         be verbose
>  >
>  >      -d                    Do not daemonize
>  >      -s                    Log to syslog even with -d
>  >      -t n                  Exit after SEC seconds of inactivity
>  >
>  > The option descriptions should be consistent: either all start with a
>  > capital letter, or all start with a small one. Currently, they are not
>  > consistent.
>  >
>  > So, should we make them all small or all capital?
>
>  I'd like to see all small. 
I have no objections. Feel free convert them all to small.

-- 
vda

[PATCH] Remove arrays in usage text, use a single string instead

[Jiri Moskovcak]

On 10/08/2010 03:36 PM, Nikola Pajkovsky wrote:
>  On 10/08/2010 03:06 PM, Denys Vlasenko wrote:
> > On Fri, 2010-10-08 at 14:23 +0200, Nikola Pajkovsky wrote:
> >> On 10/08/2010 02:10 PM, Denys Vlasenko wrote:
> >>> Currently, usage is an array of pointers to string.
> >>> It is meant to support this:
> >>>
> >>> usage: foo --bar
> >>>       or: foo --baz
> >>>       or: foo -z
> >>>
> >>> The patch removes this and makes usage just a string:
> >>>
> >>> usage: foo --bar
> >>>
> >>> Why remove this?
> >>>
> >>> (1) We can achieve old format by using the usage string with many lines:
> >>> usage = "foo --bar\n"
> >>> "   or: foo --baz\n"
> >>> "   or: foo -z";
> >>>
> >>> (2) We don't use multi-line usage anyway.
> >>>
> >>> Run tested.
> >>>
> >>> Please review.
> >>
> >> +#define OPT__VERBOSE(v)     OPT_BOOL('v', "verbose", (v),
> "Be verbose")
> >>
> >> Why upper 'B' in `Be verbose'?
> >>
> >> First I don't see in any helps, second it looks bad and third I don't
> like it.
> >
> > The usage text currently looks like this:
> >
> > # abrtd --help
> > usage: abrtd [options]
> >       -v, --verbose         be verbose
> >
> >       -d                    Do not daemonize
> >       -s                    Log to syslog even with -d
> >       -t n                  Exit after SEC seconds of inactivity
> >
> > The option descriptions should be consistent: either all start with a
> > capital letter, or all start with a small one. Currently, they are not
> > consistent.
> >
> > So, should we make them all small or all capital?
> >
>  I'd like to see all small.
>  
I'd prefer it to start with capital (to Nikola: check the help of your 
beloved git..)

Jirka

> > I use capitalized description because sometimes there is more
> than one
> > sentence. Random example:
> >
> >      -C N[:MSG]      Allow only up to N connections from the same IP.
> >                      New connections from this IP address are closed
> >                      immediately. MSG is written to the peer before close.
> >
> > having it start with a small letter and then suddenly turn into
> > properly punctuated sentences would be weird.
> >
>  Hmm, that's true but we have always only one sentence.
>

[PATCH] Remove arrays in usage text, use a single string instead

[Denys Vlasenko]

On Fri, 2010-10-08 at 14:23 +0200, Nikola Pajkovsky wrote:
>  On 10/08/2010 02:10 PM, Denys Vlasenko wrote:
>  > Currently, usage is an array of pointers to string.
>  > It is meant to support this:
>  >
>  > usage: foo --bar
>  >     or: foo --baz
>  >     or: foo -z
>  >
>  > The patch removes this and makes usage just a string:
>  >
>  > usage: foo --bar
>  >
>  > Why remove this?
>  >
>  > (1) We can achieve old format by using the usage string with many lines:
>  > usage = "foo --bar\n"
>  > "   or: foo --baz\n"
>  > "   or: foo -z";
>  >
>  > (2) We don't use multi-line usage anyway.
>  >
>  > Run tested.
>  >
>  > Please review.
>  
>  +#define OPT__VERBOSE(v)     OPT_BOOL('v', "verbose", (v), "Be
> verbose")
>  
>  Why upper 'B' in `Be verbose'?
>  
>  First I don't see in any helps, second it looks bad and third I don't like it.
I committed the patch sans this capitalization change.
-- 
vda

[PATCH] Remove arrays in usage text, use a single string instead

[Nikola Pajkovsky]

On 10/08/2010 04:20 PM, Denys Vlasenko wrote:
>  On Fri, 2010-10-08 at 14:23 +0200, Nikola Pajkovsky wrote:
> > On 10/08/2010 02:10 PM, Denys Vlasenko wrote:
> >> Currently, usage is an array of pointers to string.
> >> It is meant to support this:
> >>
> >> usage: foo --bar
> >>      or: foo --baz
> >>      or: foo -z
> >>
> >> The patch removes this and makes usage just a string:
> >>
> >> usage: foo --bar
> >>
> >> Why remove this?
> >>
> >> (1) We can achieve old format by using the usage string with many lines:
> >> usage = "foo --bar\n"
> >> "   or: foo --baz\n"
> >> "   or: foo -z";
> >>
> >> (2) We don't use multi-line usage anyway.
> >>
> >> Run tested.
> >>
> >> Please review.
> >
> > +#define OPT__VERBOSE(v)     OPT_BOOL('v', "verbose", (v), "Be
> verbose")
> >
> > Why upper 'B' in `Be verbose'?
> >
> > First I don't see in any helps, second it looks bad and third I don't like
> it.
>
>  I committed the patch sans this capitalization change. 
Thank you!
-- 
Nikola

[PATCH] Return a bitmask of found options from parse_opts()

[Denys Vlasenko]

This patch makes it easier to analyze "bool" type options -
now they are all just bits in a single return value,
no need to have an integer variable for each option.

The previous behavior is retained too: it is useful for
"cumulative" options like -vvv.

The patch also removes all OPT_GROUPs.

Run tested. Please review.

-- 
vda


diff -x '*.po' -d -urpN abrt.2/lib/utils/parse_options.c
abrt.3/lib/utils/parse_options.c
--- abrt.2/lib/utils/parse_options.c	2010-10-08 15:14:49.727480330 +0200
+++ abrt.3/lib/utils/parse_options.c	2010-10-08 15:09:04.736481218 +0200
@@ -61,7 +61,7 @@ static int parse_opt_size(const struct o
     return size;
 }
 
-void parse_opts(int argc, char **argv, const struct options *opt,
+unsigned parse_opts(int argc, char **argv, const struct options *opt,
                 const char *usage)
 {
     int help = 0;
@@ -107,10 +107,10 @@ void parse_opts(int argc, char **argv, c
     longopts[ii].val = 0;
     */
 
-    int option_index = 0;
+    unsigned retval = 0;
     while (1)
     {
-        int c = getopt_long(argc, argv, shortopts->buf, longopts, &option_index);
+        int c = getopt_long(argc, argv, shortopts->buf, longopts, NULL);
 
         if (c == -1)
             break;
@@ -126,7 +126,10 @@ void parse_opts(int argc, char **argv, c
         {
             if (opt[ii].short_name == c)
             {
-                switch (opt[ii].type)
+                if (ii < sizeof(retval)*8)
+                    retval |= (1 << ii);
+
+                if (opt[ii].value != NULL) switch (opt[ii].type)
                 {
                     case OPTION_BOOL:
                         *(int*)opt[ii].value += 1;
@@ -148,4 +151,6 @@ void parse_opts(int argc, char **argv, c
 
     free(longopts);
     strbuf_free(shortopts);
+
+    return retval;
 }
diff -x '*.po' -d -urpN abrt.2/lib/utils/parse_options.h
abrt.3/lib/utils/parse_options.h
--- abrt.2/lib/utils/parse_options.h	2010-10-08 13:45:05.351480477 +0200
+++ abrt.3/lib/utils/parse_options.h	2010-10-08 14:11:49.404480788 +0200
@@ -39,7 +39,7 @@ struct options {
 
 #define OPT__VERBOSE(v)     OPT_BOOL('v', "verbose", (v), "Be
verbose")
 
-void parse_opts(int argc, char **argv, const struct options *opt,
+unsigned parse_opts(int argc, char **argv, const struct options *opt,
                 const char *usage);
 
 void parse_usage_and_die(const char *usage, const struct options *opt);
diff -x '*.po' -d -urpN abrt.2/src/daemon/abrt-action-generate-backtrace.c
abrt.3/src/daemon/abrt-action-generate-backtrace.c
--- abrt.2/src/daemon/abrt-action-generate-backtrace.c	2010-10-08 14:06:36.810230965
+0200
+++ abrt.3/src/daemon/abrt-action-generate-backtrace.c	2010-10-08 14:26:01.017480544
+0200
@@ -240,18 +240,24 @@ static char *get_backtrace(struct dump_d
     return bt;
 }
 
-static char *d_opt, *i_opt;
-static int s_opt;
+enum {
+    OPT_v = 1 << 0,
+    OPT_d = 1 << 1,
+    OPT_i = 1 << 2,
+    OPT_t = 1 << 3,
+    OPT_s = 1 << 4,
+};
+
+static char *i_opt;
 
 static const char abrt_action_generage_backtrace_usage[] =
"abrt-action-generate-backtrace [options] -d DIR";
 
 static struct options abrt_action_generate_backtrace_options[] = {
     OPT__VERBOSE(&g_verbose),
-    OPT_GROUP(""),
-    OPT_STRING( 'd' , 0, &d_opt, "dir", "Crash dump
directory"),
+    OPT_STRING( 'd' , 0, &dump_dir_name, "dir", "Crash dump
directory"),
     OPT_STRING( 'i' , 0, &i_opt, "dir1[:dir2]...", "Additional
debuginfo directories"),
     OPT_INTEGER( 't' , 0, &exec_timeout_sec, "Kill gdb if it runs for
more than SECONDS"),
-    OPT_BOOL( 's' , 0, &s_opt, "Log to syslog even with -d"),
+    OPT_BOOL( 's' , 0, NULL, "Log to syslog even with -d"),
     OPT_END()
 };
 
@@ -261,26 +267,18 @@ int main(int argc, char **argv)
     if (env_verbose)
         g_verbose = atoi(env_verbose);
 
-    parse_opts(argc, argv, abrt_action_generate_backtrace_options,
+    unsigned opts = parse_opts(argc, argv, abrt_action_generate_backtrace_options,
                            abrt_action_generage_backtrace_usage);
 
-    if (!d_opt)
-        parse_usage_and_die(abrt_action_generage_backtrace_usage,
-                            abrt_action_generate_backtrace_options);
-
-    dump_dir_name = d_opt;
-
-    debuginfo_dirs = xstrdup("");
     if (i_opt)
-    {
-        free(debuginfo_dirs);
         debuginfo_dirs = xasprintf("%s:%s", DEBUGINFO_CACHE_DIR, i_opt);
-    }
+    else
+        debuginfo_dirs = xstrdup("");
 
     putenv(xasprintf("ABRT_VERBOSE=%u", g_verbose));
     msg_prefix = xasprintf("abrt-action-generate-backtrace[%u]", getpid());
 
-    if (s_opt)
+    if (opts & OPT_s)
     {
         openlog(msg_prefix, 0, LOG_DAEMON);
         logmode = LOGMODE_SYSLOG;
diff -x '*.po' -d -urpN abrt.2/src/daemon/abrt-action-save-package-data.cpp
abrt.3/src/daemon/abrt-action-save-package-data.cpp
--- abrt.2/src/daemon/abrt-action-save-package-data.cpp	2010-10-08 14:09:48.925231033
+0200
+++ abrt.3/src/daemon/abrt-action-save-package-data.cpp	2010-10-08 14:30:10.713230481
+0200
@@ -267,16 +267,20 @@ static int SavePackageDescriptionToDebug
     return error;
 }
 
-static char *d_opt;
-static int s_opt;
+enum {
+    OPT_v = 1 << 0,
+    OPT_d = 1 << 1,
+    OPT_s = 1 << 2,
+};
+
+static const char *dump_dir_name = ".";
 
 static const char abrt_action_save_package_data_usage[] =
"abrt-action-save-package-data [options] -d DIR";
 
 static struct options abrt_action_save_package_data_options[] = {
     OPT__VERBOSE(&g_verbose),
-    OPT_GROUP(""),
-    OPT_STRING( 'd' , 0, &d_opt, "dir", "Crash dump
directory"),
-    OPT_BOOL( 's' , 0, &s_opt, "Log to syslog"),
+    OPT_STRING( 'd' , 0, &dump_dir_name, "dir", "Crash dump
directory"),
+    OPT_BOOL( 's' , 0, NULL, "Log to syslog"),
     OPT_END()
 };
 
@@ -286,20 +290,13 @@ int main(int argc, char **argv)
     if (env_verbose)
         g_verbose = atoi(env_verbose);
 
-    const char *dump_dir_name = ".";
-
-    parse_opts(argc, argv, abrt_action_save_package_data_options,
+    unsigned opts = parse_opts(argc, argv, abrt_action_save_package_data_options,
                            abrt_action_save_package_data_usage);
 
-    if (!d_opt)
-        parse_usage_and_die(abrt_action_save_package_data_usage,
-                            abrt_action_save_package_data_options);
-
-    dump_dir_name = d_opt;
-
     putenv(xasprintf("ABRT_VERBOSE=%u", g_verbose));
     msg_prefix = xasprintf("abrt-action-save-package-data[%u]", getpid());
-    if (s_opt)
+
+    if (opts & OPT_s)
     {
         openlog(msg_prefix, 0, LOG_DAEMON);
         logmode = LOGMODE_SYSLOG;
diff -x '*.po' -d -urpN abrt.2/src/daemon/abrt-server.c
abrt.3/src/daemon/abrt-server.c
--- abrt.2/src/daemon/abrt-server.c	2010-10-08 13:41:39.833480840 +0200
+++ abrt.3/src/daemon/abrt-server.c	2010-10-08 14:19:56.006230679 +0200
@@ -277,15 +277,18 @@ static void process_message(const char *
 
 static void dummy_handler(int sig_unused) {}
 
-static int s_opt;
+enum {
+    OPT_v = 1 << 0,
+    OPT_u = 1 << 1,
+    OPT_s = 1 << 2,
+};
 
 static const char abrt_server_usage[] = "abrt-server [options]";
 
 static struct options abrt_server_options[] = {
     OPT__VERBOSE(&g_verbose),
-    OPT_GROUP(""),
     OPT_INTEGER( 'u' , 0, &client_uid, "Use UID as client uid"),
-    OPT_BOOL( 's' , 0, &s_opt, "Log to syslog"),
+    OPT_BOOL( 's' , 0, NULL, "Log to syslog"),
     OPT_END()
 };
 
@@ -295,12 +298,12 @@ int main(int argc, char **argv)
     if (env_verbose)
         g_verbose = atoi(env_verbose);
 
-    parse_opts(argc, argv, abrt_server_options,
+    unsigned opts = parse_opts(argc, argv, abrt_server_options,
                            abrt_server_usage);
 
     putenv(xasprintf("ABRT_VERBOSE=%u", g_verbose));
     msg_prefix = xasprintf("abrt-server[%u]", getpid());
-    if (s_opt)
+    if (opts & OPT_s)
     {
         openlog(msg_prefix, 0, LOG_DAEMON);
         logmode = LOGMODE_SYSLOG;
diff -x '*.po' -d -urpN abrt.2/src/daemon/Daemon.cpp abrt.3/src/daemon/Daemon.cpp
--- abrt.2/src/daemon/Daemon.cpp	2010-10-08 14:07:54.858480511 +0200
+++ abrt.3/src/daemon/Daemon.cpp	2010-10-08 14:29:41.929480303 +0200
@@ -87,8 +87,6 @@ using namespace std;
  */
 CCommLayerServer* g_pCommLayer;
 
-static bool daemonize = true;
-
 static volatile sig_atomic_t s_sig_caught;
 static int s_signal_pipe[2];
 static int s_signal_pipe_write = -1;
@@ -860,16 +858,21 @@ static void sanitize_dump_dir_rights()
     ensure_writable_dir(VAR_RUN"/abrt", 0755, "root");
 }
 
-static int daemonize_opt, syslog_opt;
+enum {
+    OPT_v = 1 << 0,
+    OPT_d = 1 << 1,
+    OPT_s = 1 << 2,
+    OPT_t = 1 << 3,
+};
+
 static char *timeout_opt;
 
 static const char* abrtd_usage = _("abrtd [options]");
 
 static struct options abrtd_options[] = {
     OPT__VERBOSE(&g_verbose),
-    OPT_GROUP(""),
-    OPT_BOOL( 'd' , 0, &daemonize_opt, _("Do not daemonize")),
-    OPT_BOOL( 's' , 0, &syslog_opt, _("Log to syslog even with
-d")),
+    OPT_BOOL( 'd' , 0, NULL, _("Do not daemonize")),
+    OPT_BOOL( 's' , 0, NULL, _("Log to syslog even with -d")),
     OPT_INTEGER( 't' , 0, &timeout_opt, _("Exit after SEC seconds of
inactivity")),
     OPT_END()
 };
@@ -892,12 +895,9 @@ int main(int argc, char** argv)
     if (env_verbose)
         g_verbose = atoi(env_verbose);
 
-    parse_opts(argc, argv, abrtd_options, abrtd_usage);
-
-    if (daemonize_opt)
-        daemonize = false;
+    unsigned opts = parse_opts(argc, argv, abrtd_options, abrtd_usage);
 
-    if (syslog_opt)
+    if (opts & OPT_s)
         start_syslog_logging();
 
     putenv(xasprintf("ABRT_VERBOSE=%u", g_verbose));
@@ -914,7 +914,7 @@ int main(int argc, char** argv)
         signal(SIGALRM, handle_signal);
 
     /* Daemonize unless -d */
-    if (daemonize)
+    if (!(opts & OPT_d))
     {
         /* forking to background */
         pid_t pid = fork();
@@ -1042,13 +1042,13 @@ int main(int argc, char** argv)
         /* Initialization error */
         error_msg("Error while initializing daemon");
         /* Inform parent that initialization failed */
-        if (daemonize)
+        if (!(opts & OPT_d))
             kill(parent_pid, SIGINT);
         goto cleanup;
     }
 
     /* Inform parent that we initialized ok */
-    if (daemonize)
+    if (!(opts & OPT_d))
     {
         VERB1 log("Signalling parent");
         kill(parent_pid, SIGTERM);

[PATCH] Return a bitmask of found options from parse_opts()

[Nikola Pajkovsky]

On 10/08/2010 03:29 PM, Denys Vlasenko wrote:
>  This patch makes it easier to analyze "bool" type options
> -
>  now they are all just bits in a single return value,
>  no need to have an integer variable for each option.
>
>  The previous behavior is retained too: it is useful for
>  "cumulative" options like -vvv.
>  
cumulative options is done by  *(int*)opt[ii].value += 1, right?

>  The patch also removes all OPT_GROUPs.
>
>  Run tested. Please review.
>  
This is not wise.

+enum {
+    OPT_v = 1 << 0,
+    OPT_d = 1 << 1,
+    OPT_i = 1 << 2,
+    OPT_t = 1 << 3,
+    OPT_s = 1 << 4,
+};

+                if (ii < sizeof(retval)*8)
+                    retval |= (1 << ii);

this means that it's position related.

If I swap something in

static struct options abrt_action_generate_backtrace_options[] = {
      OPT__VERBOSE(&g_verbose),
-    OPT_GROUP(""),
-    OPT_STRING( 'd' , 0, &d_opt, "dir", "Crash dump
directory"),
+    OPT_STRING( 'd' , 0, &dump_dir_name, "dir", "Crash dump
directory"),
      OPT_STRING( 'i' , 0, &i_opt, "dir1[:dir2]...",
"Additional debuginfo directories"),
      OPT_INTEGER( 't' , 0, &exec_timeout_sec, "Kill gdb if it runs for
more than SECONDS"),
-    OPT_BOOL( 's' , 0, &s_opt, "Log to syslog even with -d"),
+    OPT_BOOL( 's' , 0, NULL, "Log to syslog even with -d"),
      OPT_END()
  };

it won't fit to enum.

And also I don't see any problem with that if arg is not preset then is set to 0(due
to static) or
is bump up by*(int*)opt[ii].value += 1

-- 
Nikola

[PATCH] Return a bitmask of found options from parse_opts()

[Denys Vlasenko]

On Fri, 2010-10-08 at 16:04 +0200, Nikola Pajkovsky wrote:
>  On 10/08/2010 03:29 PM, Denys Vlasenko wrote:
>  > This patch makes it easier to analyze "bool" type options -
>  > now they are all just bits in a single return value,
>  > no need to have an integer variable for each option.
>  >
>  > The previous behavior is retained too: it is useful for
>  > "cumulative" options like -vvv.
>  >
>  cumulative options is done by  *(int*)opt[ii].value += 1, right? 
Yes.

>  > The patch also removes all OPT_GROUPs.
>  >
>  > Run tested. Please review.
>  >
>  
>  This is not wise.
>  
>  +enum {
>  +    OPT_v = 1 << 0,
>  +    OPT_d = 1 << 1,
>  +    OPT_i = 1 << 2,
>  +    OPT_t = 1 << 3,
>  +    OPT_s = 1 << 4,
>  +};
>  
>  +                if (ii < sizeof(retval)*8)
>  +                    retval |= (1 << ii);
>  
>  this means that it's position related.
>  
>  If I swap something in
>  
>  static struct options abrt_action_generate_backtrace_options[] = {
>        OPT__VERBOSE(&g_verbose),
>  -    OPT_GROUP(""),
>  -    OPT_STRING( 'd' , 0, &d_opt, "dir", "Crash dump
> directory"),
>  +    OPT_STRING( 'd' , 0, &dump_dir_name, "dir", "Crash dump
> directory"),
>        OPT_STRING( 'i' , 0, &i_opt, "dir1[:dir2]...",
> "Additional debuginfo directories"),
>        OPT_INTEGER( 't' , 0, &exec_timeout_sec, "Kill gdb if it runs for
> more than SECONDS"),
>  -    OPT_BOOL( 's' , 0, &s_opt, "Log to syslog even with -d"),
>  +    OPT_BOOL( 's' , 0, NULL, "Log to syslog even with -d"),
>        OPT_END()
>    };
>  
>  it won't fit to enum. 
It will fit, but the value of the enum constants should be changed too,
yes.


>  And also I don't see any problem with that if arg is not preset
> then is set to 0(due to static) or
>  is bump up by*(int*)opt[ii].value += 1 
I did not understand this part.

-- 
vda

[Nikola Pajkovsky]

On 10/08/2010 01:56 PM, Denys Vlasenko wrote:
>  This patch removes the need to add --help option to every program.
>  It is added (and handled) by parse_opts().
>
>  Run tested. Please review. 
apply, thanks

-- 
Nikola

[Denys Vlasenko]

On Fri, 2010-10-08 at 14:19 +0200, Nikola Pajkovsky wrote:
>  On 10/08/2010 01:56 PM, Denys Vlasenko wrote:
>  > This patch removes the need to add --help option to every program.
>  > It is added (and handled) by parse_opts().
>  >
>  > Run tested. Please review.
>  apply, thanks 
Applied with small changes. Thanks.
-- 
vda