shared/options: fix --help indentation for long options

In 4339197f5d4f712bc900d8e09c892015d48b19bb the helper to format -o/--opt=
was split out, but the indentation was for --long-options was messed up.
We'd print:
Options:
  -h --help             Show this help
  --version             Show package version
  --no-ask-password     Do not prompt for password
  ...
But we want
  -h --help             Show this help
     --version          Show package version
     --no-ask-password  Do not prompt for password
  ...

The prefix argument was arguably ugly, even if it allowed one alloc to be
avoided. Let's get rid of it and let the handler prefix the string as
appropriate. This makes other callers nicer too.

Fixup for 4339197f5d4f712bc900d8e09c892015d48b19bb.

diff --git a/src/shared/options.c b/src/shared/options.c
index 644c7ae27c3db049d0d4d55216de9cb17ff044b5..74f697edbe921d3b866ee9b25d2dbcdf9c4f1216 100644 (file)
--- a/src/shared/options.c
+++ b/src/shared/options.c
@@ -306,18 +306,15 @@ size_t option_parser_get_n_args(const OptionParser *state) {
         return state->argc - state->positional_offset;
 }
 
-char* option_get_synopsis(const char *prefix, const Option *opt, const char *joiner, bool show_metavar) {
+char* option_get_synopsis(const Option *opt, const char *joiner, bool show_metavar) {
         assert(opt);
         assert(!FLAGS_SET(opt->flags, OPTION_GROUP_MARKER));  /* A group marker should not be displayed */
 
-        if (!prefix)
-                prefix = "";
-
         if (opt->flags & (OPTION_HELP_ENTRY_VERBATIM | OPTION_POSITIONAL_ENTRY))
-                return strjoin(prefix, ASSERT_PTR(opt->long_code));
+                return strdup(ASSERT_PTR(opt->long_code));
 
         /* The option formatted appropriately for --help strings, error messages, and similar:
-         *   <prefix>-<short><joiner>--<long>=[<metavar>]
+         *   -<short><joiner>--<long>=[<metavar>]
          * "=" is shown only when a long form is defined: -l --long=ARG, --long=ARG, -s ARG.
          * The joiner arg is used between the short and long forms.
          * As a special case, if the option has no long form and show_metavar is true,
@@ -338,16 +335,14 @@ char* option_get_synopsis(const char *prefix, const Option *opt, const char *joi
 
         bool need_eq = option_takes_arg(opt) && opt->long_code;
         if (!show_metavar)
-                return strjoin(prefix,
-                               sc,
+                return strjoin(sc,
                                joiner,
                                opt->long_code ? "--" : "",
                                strempty(opt->long_code),
                                need_eq ? "=" : "");
 
         bool need_quote = opt->metavar && strchr(opt->metavar, ' ');
-        return strjoin(prefix,
-                       sc,
+        return strjoin(sc,
                        joiner,
                        opt->long_code ? "--" : "",
                        strempty(opt->long_code),
@@ -389,22 +384,27 @@ int _option_parser_get_help_table(
                         /* No help string — we do not show the option */
                         continue;
 
+                _cleanup_free_ char *s = option_get_synopsis(opt, " ", /* show_metavar= */ true);
+                if (!s)
+                        return log_oom();
+
                 /* We indent the option string by two spaces. We could set the minimum cell width and
                  * right-align for a similar result, but that'd be more work. This is only used for
                  * display. */
-                _cleanup_free_ char *s = option_get_synopsis("  ", opt, " ", /* show_metavar= */ true);
-                if (!s)
+                const char *prefix = opt->short_code != 0 ? "  " : "     ";
+                _cleanup_free_ char *t = strjoin(prefix, s);
+                if (!t)
                         return log_oom();
 
-                r = table_add_many(table, TABLE_STRING, s);
+                r = table_add_many(table, TABLE_STRING, t);
                 if (r < 0)
                         return table_log_add_error(r);
 
-                _cleanup_strv_free_ char **t = strv_split(opt->help, /* separators= */ NULL);
-                if (!t)
+                _cleanup_strv_free_ char **split = strv_split(opt->help, /* separators= */ NULL);
+                if (!split)
                         return log_oom();
 
-                r = table_add_many(table, TABLE_STRV_WRAPPED, t);
+                r = table_add_many(table, TABLE_STRV_WRAPPED, split);
                 if (r < 0)
                         return table_log_add_error(r);
         }

diff --git a/src/shared/options.h b/src/shared/options.h
index 3d08390b33e90a835e097e4934889351ee320512..f9913951f425177492616c85df1cb9506c88439e 100644 (file)
--- a/src/shared/options.h
+++ b/src/shared/options.h
@@ -191,7 +191,7 @@ char* option_parser_consume_next_arg(OptionParser *state);
 
 char** option_parser_get_args(const OptionParser *state);
 size_t option_parser_get_n_args(const OptionParser *state);
-char* option_get_synopsis(const char *prefix, const Option *opt, const char *joiner, bool show_metavar);
+char* option_get_synopsis(const Option *opt, const char *joiner, bool show_metavar);
 
 int _option_parser_get_help_table(
                 const Option options[],

diff --git a/src/test/test-options.c b/src/test/test-options.c
index 6a0744e67363d1eb47bb06e24fdef103ef07a950..4bdec73d401aee23c9384ee45177469f1d09bd5a 100644 (file)
--- a/src/test/test-options.c
+++ b/src/test/test-options.c
@@ -1306,47 +1306,47 @@ static void test_option_get_synopsis_one(
                 bool show_metavar,
                 const char *expected) {
         log_debug("%s", expected);
-        _cleanup_free_ char *s = option_get_synopsis(". ", opt, joiner, show_metavar);
+        _cleanup_free_ char *s = option_get_synopsis(opt, joiner, show_metavar);
         ASSERT_STREQ(s, expected);
 }
 
 TEST(option_get_synopsis) {
-        test_option_get_synopsis_one(&(const Option) { 0, 0, 'x', "xxx", "X" }, "/",  true,  ". -x/--xxx=X");
-        test_option_get_synopsis_one(&(const Option) { 0, 0, 'x', "xxx", "X" }, NULL, true,  ". -x --xxx=X");
-        test_option_get_synopsis_one(&(const Option) { 0, 0, 'x', "xxx", "X" }, "/",  false, ". -x/--xxx=" );
-        test_option_get_synopsis_one(&(const Option) { 0, 0, 'x', "xxx", "X" }, " ",  true,  ". -x --xxx=X");
-        test_option_get_synopsis_one(&(const Option) { 0, 0, 'x', "xxx", "X" }, " ",  false, ". -x --xxx=" );
-        test_option_get_synopsis_one(&(const Option) { 0, 0,   0, "xxx", "X" }, "+",  true,  ". --xxx=X"   );
-        test_option_get_synopsis_one(&(const Option) { 0, 0,   0, "xxx", "X" }, "+",  false, ". --xxx="    );
-        test_option_get_synopsis_one(&(const Option) { 0, 0, 'x', NULL,  "X" }, " ",  true,  ". -x X"      );
-        test_option_get_synopsis_one(&(const Option) { 0, 0, 'x', NULL,  "X" }, "/",  false, ". -x"        );
-
-        test_option_get_synopsis_one(&(const Option) { 0, 0, 'x', "xxx", "A B" }, "/", true,  ". -x/--xxx='A B'");
-        test_option_get_synopsis_one(&(const Option) { 0, 0, 'x', "xxx", "A B" }, " ", true,  ". -x --xxx='A B'");
-        test_option_get_synopsis_one(&(const Option) { 0, 0,   0, "xxx", "A B" }, "+", true,  ". --xxx='A B'"   );
-        test_option_get_synopsis_one(&(const Option) { 0, 0, 'x', NULL,  "A B" }, " ", true,  ". -x 'A B'"      );
-
-        test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG, 'x', "xxx", "X" }, "/",  true,  ". -x/--xxx[=X]");
-        test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG, 'x', "xxx", "X" }, NULL, true,  ". -x --xxx[=X]");
+        test_option_get_synopsis_one(&(const Option) { 0, 0, 'x', "xxx", "X" }, "/",  true,  "-x/--xxx=X");
+        test_option_get_synopsis_one(&(const Option) { 0, 0, 'x', "xxx", "X" }, NULL, true,  "-x --xxx=X");
+        test_option_get_synopsis_one(&(const Option) { 0, 0, 'x', "xxx", "X" }, "/",  false, "-x/--xxx=" );
+        test_option_get_synopsis_one(&(const Option) { 0, 0, 'x', "xxx", "X" }, " ",  true,  "-x --xxx=X");
+        test_option_get_synopsis_one(&(const Option) { 0, 0, 'x', "xxx", "X" }, " ",  false, "-x --xxx=" );
+        test_option_get_synopsis_one(&(const Option) { 0, 0,   0, "xxx", "X" }, "+",  true,  "--xxx=X"   );
+        test_option_get_synopsis_one(&(const Option) { 0, 0,   0, "xxx", "X" }, "+",  false, "--xxx="    );
+        test_option_get_synopsis_one(&(const Option) { 0, 0, 'x', NULL,  "X" }, " ",  true,  "-x X"      );
+        test_option_get_synopsis_one(&(const Option) { 0, 0, 'x', NULL,  "X" }, "/",  false, "-x"        );
+
+        test_option_get_synopsis_one(&(const Option) { 0, 0, 'x', "xxx", "A B" }, "/", true,  "-x/--xxx='A B'");
+        test_option_get_synopsis_one(&(const Option) { 0, 0, 'x', "xxx", "A B" }, " ", true,  "-x --xxx='A B'");
+        test_option_get_synopsis_one(&(const Option) { 0, 0,   0, "xxx", "A B" }, "+", true,  "--xxx='A B'"   );
+        test_option_get_synopsis_one(&(const Option) { 0, 0, 'x', NULL,  "A B" }, " ", true,  "-x 'A B'"      );
+
+        test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG, 'x', "xxx", "X" }, "/",  true,  "-x/--xxx[=X]");
+        test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG, 'x', "xxx", "X" }, NULL, true,  "-x --xxx[=X]");
         /* Note: --xxx[=] would be silly, so we show --xxx=. It's a corner case. Maybe this should change. */
-        test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG, 'x', "xxx", "X" }, "/",  false, ". -x/--xxx="   );
-        test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG, 'x', "xxx", "X" }, " ",  true,  ". -x --xxx[=X]");
-        test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG, 'x', "xxx", "X" }, " ",  false, ". -x --xxx="   );
-        test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG,   0, "xxx", "X" }, "+",  true,  ". --xxx[=X]"   );
-        test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG,   0, "xxx", "X" }, "+",  false, ". --xxx="      );
-        test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG, 'x', NULL,  "X" }, " ",  true,  ". -x [X]"      );
-        test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG, 'x', NULL,  "X" }, "/",  false, ". -x"          );
-
-        test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG, 'x', "xxx", "A B" }, "/", true,  ". -x/--xxx[='A B']");
-        test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG, 'x', "xxx", "A B" }, " ", true,  ". -x --xxx[='A B']");
-        test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG,   0, "xxx", "A B" }, "+", true,  ". --xxx[='A B']"   );
-        test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG, 'x', NULL,  "A B" }, " ", true,  ". -x ['A B']"      );
+        test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG, 'x', "xxx", "X" }, "/",  false, "-x/--xxx="   );
+        test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG, 'x', "xxx", "X" }, " ",  true,  "-x --xxx[=X]");
+        test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG, 'x', "xxx", "X" }, " ",  false, "-x --xxx="   );
+        test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG,   0, "xxx", "X" }, "+",  true,  "--xxx[=X]"   );
+        test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG,   0, "xxx", "X" }, "+",  false, "--xxx="      );
+        test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG, 'x', NULL,  "X" }, " ",  true,  "-x [X]"      );
+        test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG, 'x', NULL,  "X" }, "/",  false, "-x"          );
+
+        test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG, 'x', "xxx", "A B" }, "/", true,  "-x/--xxx[='A B']");
+        test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG, 'x', "xxx", "A B" }, " ", true,  "-x --xxx[='A B']");
+        test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG,   0, "xxx", "A B" }, "+", true,  "--xxx[='A B']"   );
+        test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG, 'x', NULL,  "A B" }, " ", true,  "-x ['A B']"      );
 
         test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG | OPTION_HELP_ENTRY | OPTION_STOPS_PARSING,
-                                                       'x', "xxx", "A B" }, "/", true,  ". -x/--xxx[='A B']");
+                                                       'x', "xxx", "A B" }, "/", true,  "-x/--xxx[='A B']");
 
-        test_option_get_synopsis_one(&(const Option) { 0, OPTION_HELP_ENTRY_VERBATIM, 'u', "special special",  "unused" }, "/",  true, ". special special");
-        test_option_get_synopsis_one(&(const Option) { 0, OPTION_POSITIONAL_ENTRY, 'u', "(fixed)", "unused" }, "/",  true, ". (fixed)");
+        test_option_get_synopsis_one(&(const Option) { 0, OPTION_HELP_ENTRY_VERBATIM, 'u', "special special",  "unused" }, "/",  true, "special special");
+        test_option_get_synopsis_one(&(const Option) { 0, OPTION_POSITIONAL_ENTRY, 'u', "(fixed)", "unused" }, "/",  true, "(fixed)");
 }
 
 DEFINE_TEST_MAIN(LOG_DEBUG);

diff --git a/src/udev/scsi_id/scsi_id.c b/src/udev/scsi_id/scsi_id.c
index 41140db816281aff518ab272925ecfc981488f8d..f272648c420c91aea421be24145378a074d9267c 100644 (file)
--- a/src/udev/scsi_id/scsi_id.c
+++ b/src/udev/scsi_id/scsi_id.c
@@ -331,7 +331,7 @@ static int per_dev_options(struct scsi_id_device *dev_scsi, int *good_bad, enum
                 } else
                         return log_error_errno(SYNTHETIC_ERRNO(EINVAL),
                                                "Option %s not supported in the config file.",
-                                               strnull(option_get_synopsis("", opts.opt, "/", /* show_metavar=*/ false)));
+                                               strnull(option_get_synopsis(opts.opt, "/", /* show_metavar=*/ false)));
 
         return 0;
 }