Allow erasing in multiple scopes in one go #9280
Conversation
| // `set -e` is allowed to be called with multiple scopes. | ||
| for (int bit = 0; 1<<bit <= ENV_USER; ++bit) { | ||
| int scope = scopes & 1<<bit; | ||
| if (scope == 0 || (scope == ENV_USER && scopes != ENV_USER)) continue; |
There was a problem hiding this comment.
Supporting set -elgU sounds great!
|
Fixes #7711 |
| int scope = scopes & 1<<bit; | ||
| if (scope == 0 || (scope == ENV_USER && scopes != ENV_USER)) continue; | ||
| for (int i = 0; i < argc; i++) { | ||
| auto split = split_var_and_indexes(argv[i], scope, parser.vars(), streams); |
There was a problem hiding this comment.
I'm assuming with indexes this works like:
set -g foo 1 2 3
set -l foo a b c
set -egl foo[2]makes the global foo "1 3" and the local foo "a c"?
There was a problem hiding this comment.
That is indeed how it works, and it's probably a good test case.
Variable events are also fired for each scope, which is probably okay for now.
(tbh I'm still of the opinion local variables shouldn't fire events - you can't read them in an event handler anyway - but last time that turned out to be unpopular)
There was a problem hiding this comment.
That is indeed how it works, and it's probably a good test case.
Variable events are also fired for each scope, which is probably okay for now.
Yes, what I had in mind the entire time was that set -elg foo should behave exactly as if set -el foo; set -eg foo (except for the return codes, which are already somewhat non-sensical and need to be fixed later), side effects and all. We can always change that behavior later if we have a good reason to, but I wanted to minimize the scope of the changes for this PR to make this an easier yes/no decision.
(tbh I'm still of the opinion local variables shouldn't fire events - you can't read them in an event handler anyway - but last time that turned out to be unpopular)
I must have missed that discussion. Do you happen to remember what issue this was in?
There was a problem hiding this comment.
I must have missed that discussion. Do you happen to remember what issue this was in?
#8384, which asked about for-loop variables.
In that case, we chose to keep it because someone could try to do something with --no-scope-shadowing.
When I was drafting the PR, I originally wrote "A team member recently mentioned in the chat that this either is possible or should be possible" but then I couldn't find it in the chat and was left confused where I had seen it. Thanks for linking it! |
|
Added tests and updated docs. |
|
Nice fix, LGTM! Please changelog before merging. |
mqudsi
force-pushed
the
set_erase_multi
branch
from
477d03f to
f122eb6
Compare
While I was working on
fish_configI found myself trying to executeset -egl ...a few times, and I couldn't come up with a good reason for why this wasn't allowed.This patch extends
set -eto allow unsetting a variable from multiple scopes in one go. (The docs and changelog would also need to be updated.)Closes #7711.