rename --metadata to --details

Discussion in issue #3295 resulted in a decisions to rename the
functions --metadata flag to --details.

This also fixes a bug in the definition of the short flags for the
`functions` command. The `-e` flag does not take an argument and
therefore should not be defined as `e:`. Notice that the long form,
`--erase`, specifies `no_argument`. This discrepency happened to work
due to a quirk of how the flag parsing loop was written.

CHANGELOG.md

@@ -16,7 +16,7 @@
 - Fish is now more forgiving of missing or invalid $TERM values (#3850).
 - The `string` command now supports a `repeat` subcommand with the obvious behavior (#3864).
 - The `string match` command now supports a `--filter` flag to emit the entire string partially matched by a pattern (#3957).
-- The `functions --metadata --verbose` output now includes the function description (#597).
+- The `functions --details --verbose` output now includes the function description (#597).
 - Completions for `helm` added (#3829).
 - Empty components in $CDPATH, $MANPATH and $PATH are now converted to "." (#2106, #3914).
 - `type` now no longer requires `which`, which means it is no longer a dependency (#3912, #3945).

doc_src/functions.txt

@@ -3,7 +3,7 @@
 \subsection functions-synopsis Synopsis
 \fish{synopsis}
 functions [ -a | --all ] [ -n | --names ]
-functions [ -m | --metadata ] [ -v ] FUNCTION
+functions [ -D | --details ] [ -v ] FUNCTION
 functions -c OLDNAME NEWNAME
 functions -d DESCRIPTION FUNCTION
 functions [ -e | -q ] FUNCTIONS...
@@ -23,7 +23,7 @@ The following options are available:
 
 - `-e` or `--erase` causes the specified functions to be erased.
 
-- `-m` or `--metadata` reports the path name where each function is defined or could be autoloaded, `stdin` if the function was defined interactively or on the command line or by reading stdin, and `n/a` if the function isn't available. If the `--verbose` option is also specified then five lines are written:
+- `-D` or `--details` reports the path name where each function is defined or could be autoloaded, `stdin` if the function was defined interactively or on the command line or by reading stdin, and `n/a` if the function isn't available. If the `--verbose` option is also specified then five lines are written:
 
     -# the pathname as already described,
     -# `autoloaded`, `not-autoloaded` or `n/a`,

src/builtin.cpp

@@ -1084,50 +1084,38 @@ static int report_function_metadata(const wchar_t *funcname, bool verbose, io_st
 
 /// The functions builtin, used for listing and erasing functions.
 static int builtin_functions(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
-    wgetopter_t w;
+    wchar_t *desc = NULL;
-    int i;
-    int erase = 0;
-    wchar_t *desc = 0;
-
     int argc = builtin_count_args(argv);
-    int list = 0;
+    bool erase = false;
-    int show_hidden = 0;
+    bool list = false;
-    int res = STATUS_BUILTIN_OK;
+    bool show_hidden = false;
-    int query = 0;
+    bool query = false;
-    int copy = 0;
+    bool copy = false;
     bool report_metadata = false;
     bool verbose = false;
+    int res = STATUS_BUILTIN_OK;
 
+    static const wchar_t *short_options = L"Dacehnqv";
     static const struct woption long_options[] = {
         {L"erase", no_argument, NULL, 'e'},   {L"description", required_argument, NULL, 'd'},
         {L"names", no_argument, NULL, 'n'},   {L"all", no_argument, NULL, 'a'},
         {L"help", no_argument, NULL, 'h'},    {L"query", no_argument, NULL, 'q'},
-        {L"copy", no_argument, NULL, 'c'},    {L"metadata", no_argument, NULL, 'm'},
+        {L"copy", no_argument, NULL, 'c'},    {L"details", no_argument, NULL, 'D'},
         {L"verbose", no_argument, NULL, 'v'}, {NULL, 0, NULL, 0}};
 
-    while (1) {
+    int opt;
-        int opt_index = 0;
+    wgetopter_t w;
-
+    while ((opt = w.wgetopt_long(argc, argv, short_options, long_options, NULL)) != -1) {
-        int opt = w.wgetopt_long(argc, argv, L"ed:mnahqcv", long_options, &opt_index);
-        if (opt == -1) break;
-
         switch (opt) {
-            case 0: {
-                if (long_options[opt_index].flag != 0) break;
-                streams.err.append_format(BUILTIN_ERR_UNKNOWN, argv[0],
-                                          long_options[opt_index].name);
-                builtin_print_help(parser, streams, argv[0], streams.err);
-                return STATUS_BUILTIN_ERROR;
-            }
             case 'v': {
                 verbose = true;
                 break;
             }
             case 'e': {
-                erase = 1;
+                erase = true;
                 break;
             }
-            case 'm': {
+            case 'D': {
                 report_metadata = true;
                 break;
             }
@@ -1136,11 +1124,11 @@ static int builtin_functions(parser_t &parser, io_streams_t &streams, wchar_t **
                 break;
             }
             case 'n': {
-                list = 1;
+                list = true;
                 break;
             }
             case 'a': {
-                show_hidden = 1;
+                show_hidden = true;
                 break;
             }
             case 'h': {
@@ -1148,11 +1136,11 @@ static int builtin_functions(parser_t &parser, io_streams_t &streams, wchar_t **
                 return STATUS_BUILTIN_OK;
             }
             case 'q': {
-                query = 1;
+                query = true;
                 break;
             }
             case 'c': {
-                copy = 1;
+                copy = true;
                 break;
             }
             case '?': {
@@ -1170,15 +1158,12 @@ static int builtin_functions(parser_t &parser, io_streams_t &streams, wchar_t **
     int describe = desc ? 1 : 0;
     if (erase + describe + list + query + copy > 1) {
         streams.err.append_format(_(L"%ls: Invalid combination of options\n"), argv[0]);
-
         builtin_print_help(parser, streams, argv[0], streams.err);
-
         return STATUS_BUILTIN_ERROR;
     }
 
     if (erase) {
-        int i;
+        for (int i = w.woptind; i < argc; i++) function_remove(argv[i]);
-        for (i = w.woptind; i < argc; i++) function_remove(argv[i]);
         return STATUS_BUILTIN_OK;
     } else if (desc) {
         wchar_t *func;
@@ -1186,15 +1171,13 @@ static int builtin_functions(parser_t &parser, io_streams_t &streams, wchar_t **
         if (argc - w.woptind != 1) {
             streams.err.append_format(_(L"%ls: Expected exactly one function name\n"), argv[0]);
             builtin_print_help(parser, streams, argv[0], streams.err);
-
             return STATUS_BUILTIN_ERROR;
         }
+
         func = argv[w.woptind];
         if (!function_exists(func)) {
             streams.err.append_format(_(L"%ls: Function '%ls' does not exist\n"), argv[0], func);
-
             builtin_print_help(parser, streams, argv[0], streams.err);
-
             return STATUS_BUILTIN_ERROR;
         }
 
@@ -1202,29 +1185,26 @@ static int builtin_functions(parser_t &parser, io_streams_t &streams, wchar_t **
         return STATUS_BUILTIN_OK;
     } else if (report_metadata) {
         if (argc - w.woptind != 1) {
-            streams.err.append_format(
+            streams.err.append_format(_(L"%ls: Expected exactly one function name for --details\n"),
-                _(L"%ls: Expected exactly one function name for --metadata\n"), argv[0]);
+                                      argv[0]);
             return STATUS_BUILTIN_ERROR;
         }
 
         const wchar_t *funcname = argv[w.woptind];
         return report_function_metadata(funcname, verbose, streams, false);
     } else if (list || (argc == w.woptind)) {
-        int is_screen = !streams.out_is_redirected && isatty(STDOUT_FILENO);
-        size_t i;
         wcstring_list_t names = function_get_names(show_hidden);
         std::sort(names.begin(), names.end());
+        bool is_screen = !streams.out_is_redirected && isatty(STDOUT_FILENO);
         if (is_screen) {
             wcstring buff;
-
+            for (size_t i = 0; i < names.size(); i++) {
-            for (i = 0; i < names.size(); i++) {
                 buff.append(names.at(i));
                 buff.append(L", ");
             }
-
             streams.out.append(reformat_for_screen(buff));
         } else {
-            for (i = 0; i < names.size(); i++) {
+            for (size_t i = 0; i < names.size(); i++) {
                 streams.out.append(names.at(i).c_str());
                 streams.out.append(L"\n");
             }
@@ -1240,7 +1220,6 @@ static int builtin_functions(parser_t &parser, io_streams_t &streams, wchar_t **
                                         L"and new function name)\n"),
                                       argv[0]);
             builtin_print_help(parser, streams, argv[0], streams.err);
-
             return STATUS_BUILTIN_ERROR;
         }
         current_func = argv[w.woptind];
@@ -1250,7 +1229,6 @@ static int builtin_functions(parser_t &parser, io_streams_t &streams, wchar_t **
             streams.err.append_format(_(L"%ls: Function '%ls' does not exist\n"), argv[0],
                                       current_func.c_str());
             builtin_print_help(parser, streams, argv[0], streams.err);
-
             return STATUS_BUILTIN_ERROR;
         }
 
@@ -1267,7 +1245,6 @@ static int builtin_functions(parser_t &parser, io_streams_t &streams, wchar_t **
                 _(L"%ls: Function '%ls' already exists. Cannot create copy '%ls'\n"), argv[0],
                 new_func.c_str(), current_func.c_str());
             builtin_print_help(parser, streams, argv[0], streams.err);
-
             return STATUS_BUILTIN_ERROR;
         }
 
@@ -1275,10 +1252,10 @@ static int builtin_functions(parser_t &parser, io_streams_t &streams, wchar_t **
         return STATUS_BUILTIN_ERROR;
     }
 
-    for (i = w.woptind; i < argc; i++) {
+    for (int i = w.woptind; i < argc; i++) {
-        if (!function_exists(argv[i]))
+        if (!function_exists(argv[i])) {
             res++;
-        else {
+        } else {
             if (!query) {
                 if (i != w.woptind) streams.out.append(L"\n");
                 const wchar_t *funcname = argv[w.woptind];

tests/functions.in

@@ -6,57 +6,57 @@ function f1
 end
 
 # ==========
-# Verify that `functions --metadata` works as expected when given too many args.
+# Verify that `functions --details` works as expected when given too many args.
-set x (functions --metadata f1 f2 2>&1)
+set x (functions --details f1 f2 2>&1)
-if test "$x" != "functions: Expected exactly one function name for --metadata"
+if test "$x" != "functions: Expected exactly one function name for --details"
-    echo "Unexpected output for 'functions --metadata f1 f2': $x" >&2
+    echo "Unexpected output for 'functions --details f1 f2': $x" >&2
 end
 
 # ==========
-# Verify that `functions --metadata` works as expected when given the name of a
+# Verify that `functions --details` works as expected when given the name of a
 # known function.
-set x (functions --metadata f1)
+set x (functions --details f1)
 if test "$x" != "stdin"
-    echo "Unexpected output for 'functions --metadata f1': $x" >&2
+    echo "Unexpected output for 'functions --details f1': $x" >&2
 end
 
 # ==========
-# Verify that `functions --metadata` works as expected when given the name of an
+# Verify that `functions --details` works as expected when given the name of an
 # unknown function.
-set x (functions -m f2)
+set x (functions -D f2)
 if test "$x" != "n/a"
-    echo "Unexpected output for 'functions --metadata f2': $x" >&2
+    echo "Unexpected output for 'functions --details f2': $x" >&2
 end
 
 # ==========
-# Verify that `functions --metadata` works as expected when given the name of a
+# Verify that `functions --details` works as expected when given the name of a
 # function that could be autoloaded but isn't currently loaded.
-set x (functions -m abbr)
+set x (functions -D abbr)
 if test (count $x) -ne 1
 or not string match -q '*/share/functions/abbr.fish' "$x"
-    echo "Unexpected output for 'functions -m abbr': $x" >&2
+    echo "Unexpected output for 'functions -D abbr': $x" >&2
 end
 
 # ==========
-# Verify that `functions --verbose --metadata` works as expected when given the name of a
+# Verify that `functions --verbose --details` works as expected when given the name of a
 # function that was autoloaded.
-set x (functions -v -m abbr)
+set x (functions -v -D abbr)
 if test (count $x) -ne 5
 or not string match -q '*/share/functions/abbr.fish' $x[1]
 or test $x[2] != autoloaded
 or test $x[3] != 1
 or test $x[4] != scope-shadowing
 or test $x[5] != 'Manage abbreviations'
-    echo "Unexpected output for 'functions -v -m abbr': $x" >&2
+    echo "Unexpected output for 'functions -v -D abbr': $x" >&2
 end
 
 # ==========
-# Verify that `functions --verbose --metadata` properly escapes a function
+# Verify that `functions --verbose --details` properly escapes a function
 # with a multiline description.
 function multiline_descr -d 'line 1\n
 line 2 & more; way more'
 end
-set x (functions -v -m multiline_descr)
+set x (functions -v -D multiline_descr)
 if test $x[5] != 'line 1\\\\n\\nline 2 & more; way more'
-    echo "Unexpected output for 'functions -v -m multiline_descr': $x" >&2
+    echo "Unexpected output for 'functions -v -D multiline_descr': $x" >&2
 end