From fed64999bc05add44d5eace0d768b013fc93cf01 Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Sat, 15 Oct 2022 16:05:36 -0500 Subject: [PATCH 1/4] Allow erasing in multiple scopes in one go --- src/builtins/set.cpp | 92 ++++++++++++++++++++++++-------------------- 1 file changed, 50 insertions(+), 42 deletions(-) diff --git a/src/builtins/set.cpp b/src/builtins/set.cpp index 69993d7a6..fe2fc7469 100644 --- a/src/builtins/set.cpp +++ b/src/builtins/set.cpp @@ -204,11 +204,14 @@ static int validate_cmd_opts(const wchar_t *cmd, const set_cmd_opts_t &opts, int return STATUS_INVALID_ARGS; } - // Variables can only have one scope. + // Variables can only have one scope... if (opts.local + opts.function + opts.global + opts.universal > 1) { - streams.err.append_format(BUILTIN_ERR_GLOCAL, cmd); - builtin_print_error_trailer(parser, streams.err, cmd); - return STATUS_INVALID_ARGS; + // ..unless we are erasing a variable, in which case we can erase from several in one go. + if (!opts.erase) { + streams.err.append_format(BUILTIN_ERR_GLOCAL, cmd); + builtin_print_error_trailer(parser, streams.err, cmd); + return STATUS_INVALID_ARGS; + } } // Variables can only have one export status. @@ -391,7 +394,7 @@ static maybe_t split_var_and_indexes(const wchar_t *arg, env_mode_f return res; } -/// Given a list of values and 1-based indexes, return a new list, with those elements removed. +/// Given a list of values and 1-based indexes, return a new list with those elements removed. /// Note this deliberately accepts both args by value, as it modifies them both. static wcstring_list_t erased_at_indexes(wcstring_list_t input, std::vector indexes) { // Sort our indexes into *descending* order. @@ -620,45 +623,50 @@ static int builtin_set_show(const wchar_t *cmd, const set_cmd_opts_t &opts, int static int builtin_set_erase(const wchar_t *cmd, set_cmd_opts_t &opts, int argc, const wchar_t **argv, parser_t &parser, io_streams_t &streams) { int ret = STATUS_CMD_OK; - env_mode_flags_t scope = compute_scope(opts); - for (int i = 0; i < argc; i++) { - auto split = split_var_and_indexes(argv[i], scope, parser.vars(), streams); - if (!split) { - builtin_print_error_trailer(parser, streams.err, cmd); - return STATUS_CMD_ERROR; - } - - if (!valid_var_name(split->varname)) { - streams.err.append_format(BUILTIN_ERR_VARNAME, cmd, split->varname.c_str()); - builtin_print_error_trailer(parser, streams.err, cmd); - return STATUS_INVALID_ARGS; - } - - int retval = STATUS_CMD_OK; - if (split->indexes.empty()) { // unset the var - retval = parser.vars().remove(split->varname, scope); - // When a non-existent-variable is unset, return ENV_NOT_FOUND as $status - // but do not emit any errors at the console as a compromise between user - // friendliness and correctness. - if (retval != ENV_NOT_FOUND) { - handle_env_return(retval, cmd, split->varname, streams); + env_mode_flags_t scopes = compute_scope(opts); + // `set -e` is allowed to be called with multiple scopes. + for (int bit = 0; 1<varname)); - } - } else { // remove just the specified indexes of the var - if (!split->var) return STATUS_CMD_ERROR; - wcstring_list_t result = erased_at_indexes(split->var->as_list(), split->indexes); - retval = env_set_reporting_errors(cmd, split->varname, scope, std::move(result), - streams, parser); - } - // Set $status to the last error value. - // This is cheesy, but I don't expect this to be checked often. - if (retval != STATUS_CMD_OK) { - ret = retval; - } else { - warn_if_uvar_shadows_global(cmd, opts, split->varname, streams, parser); + if (!valid_var_name(split->varname)) { + streams.err.append_format(BUILTIN_ERR_VARNAME, cmd, split->varname.c_str()); + builtin_print_error_trailer(parser, streams.err, cmd); + return STATUS_INVALID_ARGS; + } + + int retval = STATUS_CMD_OK; + if (split->indexes.empty()) { // unset the var + retval = parser.vars().remove(split->varname, scope); + // When a non-existent-variable is unset, return ENV_NOT_FOUND as $status + // but do not emit any errors at the console as a compromise between user + // friendliness and correctness. + if (retval != ENV_NOT_FOUND) { + handle_env_return(retval, cmd, split->varname, streams); + } + if (retval == ENV_OK) { + event_fire(parser, event_t::variable_erase(split->varname)); + } + } else { // remove just the specified indexes of the var + if (!split->var) return STATUS_CMD_ERROR; + wcstring_list_t result = erased_at_indexes(split->var->as_list(), split->indexes); + retval = env_set_reporting_errors(cmd, split->varname, scope, std::move(result), + streams, parser); + } + + // Set $status to the last error value. + // This is cheesy, but I don't expect this to be checked often. + if (retval != STATUS_CMD_OK) { + ret = retval; + } else { + warn_if_uvar_shadows_global(cmd, opts, split->varname, streams, parser); + } } } return ret; From fb7f2d97e9d91025d13893f55821485f84c9e92d Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Mon, 17 Oct 2022 22:06:05 -0500 Subject: [PATCH 2/4] Add tests for erasing from multiple scopes --- tests/checks/set.fish | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/checks/set.fish b/tests/checks/set.fish index 1fbc671f1..db936a54d 100644 --- a/tests/checks/set.fish +++ b/tests/checks/set.fish @@ -921,3 +921,23 @@ foo=bar $FISH -c 'set foo 1 2 3; set --show foo' # CHECK: $foo[2]: |2| # CHECK: $foo[3]: |3| # CHECK: $foo: originally inherited as |bar| + +# Verify behavior of erasing in multiple scopes simultaneously +set -U marbles global +set -g marbles global +set -l marbles local + +set -eUg marbles +set -ql marbles || echo "erased in more scopes than it should!" +set -qg marbles && echo "didn't erase from global scope!" +set -qU marbles && echo "didn't erase from universal scope!" + +begin + set -l secret local 4 8 15 16 23 42 + set -g secret global 4 8 15 16 23 42 + set -egl secret[3..5] + echo $secret + # CHECK: local 4 23 42 +end +echo $secret +# CHECK: global 4 23 42 From 994049d33b01b9a457bf668d12d0a6c264479bd3 Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Tue, 18 Oct 2022 13:35:55 -0500 Subject: [PATCH 3/4] Document support for erasing from multiple scopes --- doc_src/cmds/set.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc_src/cmds/set.rst b/doc_src/cmds/set.rst index 96a52e179..780274d51 100644 --- a/doc_src/cmds/set.rst +++ b/doc_src/cmds/set.rst @@ -68,7 +68,7 @@ The following other options are available: Causes the values to be prepended to the current set of values for the variable. This can be used with **--append** to both append and prepend at the same time. This cannot be used when assigning to a variable slice. **-e** or **--erase** - Causes the specified shell variables to be erased + Causes the specified shell variables to be erased. Supports erasing from multiple scopes at once. **-q** or **--query** Test if the specified variable names are defined. Does not output anything, but the builtins exit status is the number of variables specified that were not defined, up to a maximum of 255. If no variable was given, it also returns 255. From f122eb666bfa2b7a1f2cffa3e2766812deb60094 Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Thu, 20 Oct 2022 11:19:48 -0500 Subject: [PATCH 4/4] Changelog: Mention new `set -eglU` support --- CHANGELOG.rst | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index a4efa172e..567038555 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -25,6 +25,9 @@ Notable improvements and fixes for a,b in y 1 z 3 ^~^ - A new helper function ``fish_delta`` can be used to show the difference to fish's stock configuration (:issue:`9255`). +- It is now possible to specify multiple scopes for ``set -e`` and all of the named variables present in any of the specified scopes will be erased. This makes it possible to remove all instances of a variable in all scopes (``set -efglU foo``) in one go (:issue:`7711`). + +======= Deprecations and removed features ---------------------------------