* [PATCH] don't exit shell on [[ -o invalid@option ]] @ 2017-11-10 18:24 Martijn Dekker 2017-11-10 18:53 ` Eric Cook 2017-11-10 22:37 ` Bart Schaefer 0 siblings, 2 replies; 19+ messages in thread From: Martijn Dekker @ 2017-11-10 18:24 UTC (permalink / raw) To: Zsh hackers list [-- Attachment #1: Type: text/plain, Size: 744 bytes --] Does it make sense for [[ -o invalid@option ]] to exit the shell with an error message? Other shells with '[[' (bash, ksh93 and pdksh/mksh variants) quietly return an unsuccessful status for a non-existent shell option. That behaviour makes more sense to me because of: (1) backwards compatibility: a script that uses '[[' to test if a shell option introduced in a recent zsh is set, would still work on an older zsh that doesn't have that shell option. (2) cross-shell compatibility: treating a non-existent option as not set would make it easier to write a script that works on bash, ksh93, and pdksh/mksh as well as zsh. The attached patch brings zsh in line with those other shells. Up to you to decide what to do with it... - Martijn [-- Attachment #2: dont-exit-on-testing-invalid-option.patch --] [-- Type: text/plain, Size: 504 bytes --] diff --git a/Src/cond.c b/Src/cond.c index b9a47ce..5eb59b2 100644 --- a/Src/cond.c +++ b/Src/cond.c @@ -492,7 +492,7 @@ dolstat(char *s) /* - * optison returns evalcond-friendly statuses (true, false, error). + * optison returns evalcond-friendly statuses (true, false). */ /**/ @@ -506,8 +506,7 @@ optison(char *name, char *s) else i = optlookup(s); if (!i) { - zwarnnam(name, "no such option: %s", s); - return 2; + return 1; } else if(i < 0) return !unset(-i); else ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] don't exit shell on [[ -o invalid@option ]] 2017-11-10 18:24 [PATCH] don't exit shell on [[ -o invalid@option ]] Martijn Dekker @ 2017-11-10 18:53 ` Eric Cook 2017-11-10 19:03 ` Martijn Dekker 2017-11-10 22:37 ` Bart Schaefer 1 sibling, 1 reply; 19+ messages in thread From: Eric Cook @ 2017-11-10 18:53 UTC (permalink / raw) To: zsh-workers On 11/10/2017 01:24 PM, Martijn Dekker wrote: > The attached patch brings zsh in line with those other shells. Up to you > to decide what to do with it... > > - Martijn > The passive aggression is real ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] don't exit shell on [[ -o invalid@option ]] 2017-11-10 18:53 ` Eric Cook @ 2017-11-10 19:03 ` Martijn Dekker 0 siblings, 0 replies; 19+ messages in thread From: Martijn Dekker @ 2017-11-10 19:03 UTC (permalink / raw) To: zsh-workers Op 10-11-17 om 18:53 schreef Eric Cook: > On 11/10/2017 01:24 PM, Martijn Dekker wrote: > >> The attached patch brings zsh in line with those other shells. Up to you >> to decide what to do with it... > > The passive aggression is real No passive aggression was intended, I apologise if it came across as such. - Martijn ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] don't exit shell on [[ -o invalid@option ]] 2017-11-10 18:24 [PATCH] don't exit shell on [[ -o invalid@option ]] Martijn Dekker 2017-11-10 18:53 ` Eric Cook @ 2017-11-10 22:37 ` Bart Schaefer 2017-11-11 12:45 ` Peter Stephenson 1 sibling, 1 reply; 19+ messages in thread From: Bart Schaefer @ 2017-11-10 22:37 UTC (permalink / raw) To: Zsh hackers list On Nov 10, 6:24pm, Martijn Dekker wrote: } } Does it make sense for [[ -o invalid@option ]] to exit the shell with an } error message? I have the impression that this was discussed on the list before, some time ago, and the conclusion was that it was useful to be able to tell [[ -o an_option_typo ]] apart from [[ -o an_unset_option ]]. In other words, if the option doesn't exist at all, then for any code depending on the result of the test, both the true and false branches may very well be wrong. It's easy enough to write backwards-compatible code by simply adding a subshell around the test, if that's what's intended. We could certainly suppress the error in emulation modes given that's what other shells do. Further, I'm not strongly invested in the current behavior even for native mode, but we should consider the ramifications. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] don't exit shell on [[ -o invalid@option ]] 2017-11-10 22:37 ` Bart Schaefer @ 2017-11-11 12:45 ` Peter Stephenson 2017-11-11 19:01 ` Martijn Dekker 0 siblings, 1 reply; 19+ messages in thread From: Peter Stephenson @ 2017-11-11 12:45 UTC (permalink / raw) To: Zsh hackers list On Fri, 10 Nov 2017 14:37:17 -0800 Bart Schaefer <schaefer@brasslantern.com> wrote: > We could certainly suppress the error in emulation modes given that's > what other shells do. Further, I'm not strongly invested in the current > behavior even for native mode, but we should consider the ramifications. It's not clear if it matters in practice rather than theory, no. The test failing is probably usually good enough. We could attach it to POSIXBUILTINS as that does control some aspects of reutrn / exit behaviour, and [[ ... ]] behaves like a specially parsed builtin. pws ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] don't exit shell on [[ -o invalid@option ]] 2017-11-11 12:45 ` Peter Stephenson @ 2017-11-11 19:01 ` Martijn Dekker 2017-11-11 23:19 ` Bart Schaefer 0 siblings, 1 reply; 19+ messages in thread From: Martijn Dekker @ 2017-11-11 19:01 UTC (permalink / raw) To: zsh-workers [-- Attachment #1: Type: text/plain, Size: 870 bytes --] Op 11-11-17 om 12:45 schreef Peter Stephenson: > On Fri, 10 Nov 2017 14:37:17 -0800 > Bart Schaefer <schaefer@brasslantern.com> wrote: >> We could certainly suppress the error in emulation modes given that's >> what other shells do. Further, I'm not strongly invested in the current >> behavior even for native mode, but we should consider the ramifications. > > It's not clear if it matters in practice rather than theory, no. The > test failing is probably usually good enough. > > We could attach it to POSIXBUILTINS as that does control some aspects of > reutrn / exit behaviour, and [[ ... ]] behaves like a specially parsed > builtin. Something like this? Seems a bit wrong as this is not POSIX at all. Also, POSIXBUILTINS makes 'set -o' exit the shell on an invalid option, which is the opposite effect. But I can't find a better shell option either. - M. [-- Attachment #2: dont-exit-on-testing-invalid-option.patch --] [-- Type: text/plain, Size: 898 bytes --] diff --git a/Doc/Zsh/cond.yo b/Doc/Zsh/cond.yo index e08fc0d..0ea5fc1 100644 --- a/Doc/Zsh/cond.yo +++ b/Doc/Zsh/cond.yo @@ -45,6 +45,8 @@ item(tt(-o) var(option))( true if option named var(option) is on. var(option) may be a single character, in which case it is a single letter option name. (See noderef(Specifying Options).) +If the var(option) does not exist, the shell exits with an error message, +unless the shell option tt(POSIX_BUILTINS) is set. ) item(tt(-p) var(file))( true if var(file) exists and is a FIFO special file (named pipe). diff --git a/Src/cond.c b/Src/cond.c index b9a47ce..814f4b0 100644 --- a/Src/cond.c +++ b/Src/cond.c @@ -506,6 +506,8 @@ optison(char *name, char *s) else i = optlookup(s); if (!i) { + if (isset(POSIXBUILTINS)) + return 1; /* act like bash and *ksh */ zwarnnam(name, "no such option: %s", s); return 2; } else if(i < 0) ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] don't exit shell on [[ -o invalid@option ]] 2017-11-11 19:01 ` Martijn Dekker @ 2017-11-11 23:19 ` Bart Schaefer 2017-11-12 19:56 ` Peter Stephenson 0 siblings, 1 reply; 19+ messages in thread From: Bart Schaefer @ 2017-11-11 23:19 UTC (permalink / raw) To: zsh-workers On Nov 11, 7:01pm, Martijn Dekker wrote: } } Op 11-11-17 om 12:45 schreef Peter Stephenson: } > On Fri, 10 Nov 2017 14:37:17 -0800 } > Bart Schaefer <schaefer@brasslantern.com> wrote: } >> We could certainly suppress the error in emulation modes } > } > We could attach it to POSIXBUILTINS as that does control some aspects } } Seems a bit wrong as this is not POSIX at all. I was thinking more along the lines of tying it to EMULATION(EMULATE_SH) rather directly to a given option bit. So you get it if the shell is started as sh/ksh/etc., but you can't switch it on if started as zsh. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] don't exit shell on [[ -o invalid@option ]] 2017-11-11 23:19 ` Bart Schaefer @ 2017-11-12 19:56 ` Peter Stephenson 2017-11-14 12:26 ` Daniel Shahaf 0 siblings, 1 reply; 19+ messages in thread From: Peter Stephenson @ 2017-11-12 19:56 UTC (permalink / raw) To: zsh-workers On Sat, 11 Nov 2017 15:19:05 -0800 Bart Schaefer <schaefer@brasslantern.com> wrote: > On Nov 11, 7:01pm, Martijn Dekker wrote: > } > } Op 11-11-17 om 12:45 schreef Peter Stephenson: > } > On Fri, 10 Nov 2017 14:37:17 -0800 > } > Bart Schaefer <schaefer@brasslantern.com> wrote: > } >> We could certainly suppress the error in emulation modes > } > > } > We could attach it to POSIXBUILTINS as that does control some aspects > } > } Seems a bit wrong as this is not POSIX at all. > > I was thinking more along the lines of tying it to EMULATION(EMULATE_SH) > rather directly to a given option bit. So you get it if the shell is > started as sh/ksh/etc., but you can't switch it on if started as zsh. This is tending to hide the knob under the sticky out bit at the top where the logo is attached (as it were). Maybe we should just accept the original patch and note the incimpatibility. It's a non-issue for properly written shell code anyway. pws ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] don't exit shell on [[ -o invalid@option ]] 2017-11-12 19:56 ` Peter Stephenson @ 2017-11-14 12:26 ` Daniel Shahaf 2017-11-14 13:22 ` Martijn Dekker 0 siblings, 1 reply; 19+ messages in thread From: Daniel Shahaf @ 2017-11-14 12:26 UTC (permalink / raw) To: zsh-workers Sorry for the late response; family reasons. Peter Stephenson wrote on Sun, Nov 12, 2017 at 19:56:57 +0000: > On Sat, 11 Nov 2017 15:19:05 -0800 > Bart Schaefer <schaefer@brasslantern.com> wrote: > > I was thinking more along the lines of tying it to EMULATION(EMULATE_SH) > > rather directly to a given option bit. So you get it if the shell is > > started as sh/ksh/etc., but you can't switch it on if started as zsh. > > This is tending to hide the knob under the sticky out bit at the top where > the logo is attached (as it were). > > Maybe we should just accept the original patch and note the > incimpatibility. It's a non-issue for properly written shell code > anyway. I wasn't sold by the OP's reasoning: Martijn Dekker wrote on Fri, Nov 10, 2017 at 18:24:29 +0000: > Does it make sense for [[ -o invalid@option ]] to exit the shell with an > error message? > Yes: "invalid@option" is neither set nor unset; it does not exist. > Other shells with '[[' (bash, ksh93 and pdksh/mksh variants) quietly > return an unsuccessful status for a non-existent shell option. That > behaviour makes more sense to me because of: > > (1) backwards compatibility: a script that uses '[[' to test if a shell > option introduced in a recent zsh is set, would still work on an older > zsh that doesn't have that shell option. > That use-case is addressable by probing ${options[schroedingerscat]} or `set -o | grep`. As Bart hinted, the proposed change is backwards *incompatible*: it makes [[ -o invalid@option ]] return 1 where currently it returns 2. (User code might be relying on the distinction between $? == 1 and $? == 2.) Moreover, if cross-version compatibility is the goal, why is it a good thing to lump "This shell does not have INVALID_OPTION" and "This shell has INVALID_OPTION and it's unset"? It's easy to imagine a situation in which that'd be a bug: if INVALID_OPTION was added in zsh version N, is set by default, and a plugin that was developed against version N is installed by a user running version N-1. With the current code that situation would result in a (proper) warning. I suppose we could plug the latter concern somewhat if we decided that whenever we add a new option, it'd be unset by default. > (2) cross-shell compatibility: treating a non-existent option as not set > would make it easier to write a script that works on bash, ksh93, and > pdksh/mksh as well as zsh. Devil's advocate, but why can't people just do, today, if [[ -o INVALID_OPTION ]] 2>/dev/null; then or if [[ ${options[invalidoption]:-off} == off ]]; then ? Cheers, Daniel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] don't exit shell on [[ -o invalid@option ]] 2017-11-14 12:26 ` Daniel Shahaf @ 2017-11-14 13:22 ` Martijn Dekker 2017-11-14 23:52 ` Daniel Shahaf 0 siblings, 1 reply; 19+ messages in thread From: Martijn Dekker @ 2017-11-14 13:22 UTC (permalink / raw) To: Zsh hackers list Op 14-11-17 om 14:26 schreef Daniel Shahaf: > As Bart hinted, the proposed change is backwards *incompatible*: it > makes [[ -o invalid@option ]] return 1 where currently it returns 2. Actually, it exits/aborts the shell with status 2. This is an essential difference from simply returning status 2. > Moreover, if cross-version compatibility is the goal, why is it a good > thing to lump "This shell does not have INVALID_OPTION" and "This shell > has INVALID_OPTION and it's unset"? Because (a) that's how all other [[ -o ... ]] implementations work, (b) that's what the current documentation in zshmisc.1 says that [[ -o ...] does, and (c) the canonical way to check if an option exists if (set +o someoption) 2>/dev/null; then ... works fine on all shells. Re (b), zshmisc.1 simply says that [[ -o ... ]] checks if the option is on. It says nothing about checking if the option exists, much less aborting the shell if it doesn't. > It's easy to imagine a situation in > which that'd be a bug: if INVALID_OPTION was added in zsh version N, is > set by default, and a plugin that was developed against version N is > installed by a user running version N-1. I don't see how that would introduce a bug. If a new option NEW_OPTION is introduced in zsh version N, default on, then [[ -o NEW_OPTION ]] can be used to run code dependent on that new option only if that new option is on. That code will then never be run on version N-1, which is what is expected. > With the current code that > situation would result in a (proper) warning. A mere warning would be fine, but what actually happens is that the shell aborts with an error message. > Devil's advocate, but why can't people just do, today, > > if [[ -o INVALID_OPTION ]] 2>/dev/null; then Because, on zsh (unlike bash and *ksh) that will cause the shell to exit, as in, the program aborts. Not only that, adding 2>/dev/null will cause the program to abort silently, because the error message is suppressed. You can of course do if ([[ -o INVALID_OPTION ]]) 2>/dev/null; then but that comes at the cost of forking a subshell, so that had better not be within a loop with many iterations. > if [[ ${options[invalidoption]:-off} == off ]]; then That's fine if the script needs to work on zsh only. Not very intuitive, though. - Martijn ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] don't exit shell on [[ -o invalid@option ]] 2017-11-14 13:22 ` Martijn Dekker @ 2017-11-14 23:52 ` Daniel Shahaf 2017-11-16 0:52 ` Martijn Dekker 0 siblings, 1 reply; 19+ messages in thread From: Daniel Shahaf @ 2017-11-14 23:52 UTC (permalink / raw) To: Martijn Dekker; +Cc: Zsh hackers list Martijn Dekker wrote on Tue, Nov 14, 2017 at 15:22:35 +0200: > Op 14-11-17 om 14:26 schreef Daniel Shahaf: > > As Bart hinted, the proposed change is backwards *incompatible*: it > > makes [[ -o invalid@option ]] return 1 where currently it returns 2. > > Actually, it exits/aborts the shell with status 2. This is an essential > difference from simply returning status 2. > Good point, I'd overlooked that. > > Moreover, if cross-version compatibility is the goal, why is it a good > > thing to lump "This shell does not have INVALID_OPTION" and "This shell > > has INVALID_OPTION and it's unset"? > > Because > (a) that's how all other [[ -o ... ]] implementations work, > (b) that's what the current documentation in zshmisc.1 says that [[ -o > ...] does, and > (c) the canonical way to check if an option exists > if (set +o someoption) 2>/dev/null; then ... > works fine on all shells. > > Re (b), zshmisc.1 simply says that [[ -o ... ]] checks if the option is > on. It says nothing about checking if the option exists, much less > aborting the shell if it doesn't. (a) That's a fair point, but I'm not convinced it swings the balance. (I agree that being compatible with bash/ksh is a plus.) (b) zshmisc.1 says "true if the named option var(option) is on". It does not say that it's valid for var(option) not to be an option name at all. Therefore, «[[ -o not_an_option ]]» is undefined behaviour; the documentation does NOT promise that it would behave identically to [[ -o an_unset_option ]]. In other words, the current behaviour is consistent with the documentation. (c) I don't follow your argument. Okay, so «(set +o option)» works, what does that have to do with the behaviour of [[ -o not_an_option ]]? > > It's easy to imagine a situation in > > which that'd be a bug: if INVALID_OPTION was added in zsh version N, is > > set by default, and a plugin that was developed against version N is > > installed by a user running version N-1. > > I don't see how that would introduce a bug. If a new option NEW_OPTION > is introduced in zsh version N, default on, then [[ -o NEW_OPTION ]] can > be used to run code dependent on that new option only if that new option > is on. That code will then never be run on version N-1, which is what is > expected. > You're considering the "if" branch, I'm considering the "else" branch: if [[ -o NEW_OPTION ]]; then lorem; else ipsum; fi "lorem" will only run on zsh vN when the option is set, but "ipsum" will run under vN with the option explicitly unset by the user AND under vN-1. Since the option is on by default, vN-1 should take the "then" branch. > > With the current code that > > situation would result in a (proper) warning. > > A mere warning would be fine, but what actually happens is that the > shell aborts with an error message. > Okay, so how about if we demoted the fatal error to a warning? Like this: % [[ -o not_an_option ]] || echo This gets run zsh: [[: no such option: not_an_option This gets run % > > Devil's advocate, but why can't people just do, today, > > > > if [[ -o INVALID_OPTION ]] 2>/dev/null; then > > Because, on zsh (unlike bash and *ksh) that will cause the shell to > exit, as in, the program aborts. Not only that, adding 2>/dev/null will > cause the program to abort silently, because the error message is > suppressed. > If the fatal error were a warning, the 2> redirection would hide [['s warning but the program would continue. > You can of course do > > if ([[ -o INVALID_OPTION ]]) 2>/dev/null; then > > but that comes at the cost of forking a subshell, so that had better not > be within a loop with many iterations. > > > if [[ ${options[invalidoption]:-off} == off ]]; then > > That's fine if the script needs to work on zsh only. Not very intuitive, > though. The «:-off» is just there in case 'set -u' is in effect. If that's not a concern, the code would be «[[ ${options[invalidoption]} == on ]]», which may be somewhat more bearable. Cheers, Daniel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] don't exit shell on [[ -o invalid@option ]] 2017-11-14 23:52 ` Daniel Shahaf @ 2017-11-16 0:52 ` Martijn Dekker 2017-11-18 18:22 ` Daniel Shahaf 0 siblings, 1 reply; 19+ messages in thread From: Martijn Dekker @ 2017-11-16 0:52 UTC (permalink / raw) To: zsh-workers Op 15-11-17 om 01:52 schreef Daniel Shahaf: > Martijn Dekker wrote on Tue, Nov 14, 2017 at 15:22:35 +0200: >> Because >> (a) that's how all other [[ -o ... ]] implementations work, >> (b) that's what the current documentation in zshmisc.1 says that [[ -o >> ...] does, and >> (c) the canonical way to check if an option exists >> if (set +o someoption) 2>/dev/null; then ... >> works fine on all shells. >> >> Re (b), zshmisc.1 simply says that [[ -o ... ]] checks if the option is >> on. It says nothing about checking if the option exists, much less >> aborting the shell if it doesn't. > > (a) That's a fair point, but I'm not convinced it swings the balance. > (I agree that being compatible with bash/ksh is a plus.) I would note additionally that '[[' is a feature originally copied from ksh, so to me it does not seem like a good idea to differ from ksh behaviour without strong reasons. > (b) zshmisc.1 says "true if the named option var(option) is on". It > does not say that it's valid for var(option) not to be an option name at > all. Therefore, «[[ -o not_an_option ]]» is undefined behaviour; the > documentation does NOT promise that it would behave identically to [[ -o > an_unset_option ]]. In other words, the current behaviour is consistent > with the documentation. I don't follow that reasoning. By definition, a non-existent option cannot be on, so the condition "true if the named option var(option) is on" is always false for a non-existent option. Had it said "false if the named option var(option) is off", your point would have made sense to me. In either case, not documenting that the shell aborts on a non-existent option is definitely an omission. In fact that omission makes me wonder if that behaviour was intended in the first place. > (c) I don't follow your argument. Okay, so «(set +o option)» works, > what does that have to do with the behaviour of [[ -o not_an_option ]]? Nothing, except that there is already a well-known way of checking if an option exists, so it's not really necessary for [[ -o ... ]] to have that functionality, particularly if it comes at the expense of not being compatible with ksh and bash. >>> It's easy to imagine a situation in >>> which that'd be a bug: if INVALID_OPTION was added in zsh version N, is >>> set by default, and a plugin that was developed against version N is >>> installed by a user running version N-1. >> >> I don't see how that would introduce a bug. If a new option NEW_OPTION >> is introduced in zsh version N, default on, then [[ -o NEW_OPTION ]] can >> be used to run code dependent on that new option only if that new option >> is on. That code will then never be run on version N-1, which is what is >> expected. > > You're considering the "if" branch, I'm considering the "else" branch: > > if [[ -o NEW_OPTION ]]; then lorem; else ipsum; fi > > "lorem" will only run on zsh vN when the option is set, but "ipsum" will > run under vN with the option explicitly unset by the user AND under > vN-1. Since the option is on by default, vN-1 should take the "then" > branch. Fair point. However, zsh makes this easy to work around with its dynamic 'no-' prefix for all options. if [[ -o noNEW_OPTION ]]; then ipsum; else lorem; fi [...] > Okay, so how about if we demoted the fatal error to a warning? Like > this: > > % [[ -o not_an_option ]] || echo This gets run > zsh: [[: no such option: not_an_option > This gets run > % I think that would be fine, as well as returning status 2 along with it. I do think the warning (and the status 2) should be suppressed if POSIX_IDENTIFIERS or some other emulation-relevant shell option is active, so that 'emulate ksh' and 'emulate sh' reproduce ksh behaviour. Thanks, - Martijn ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] don't exit shell on [[ -o invalid@option ]] 2017-11-16 0:52 ` Martijn Dekker @ 2017-11-18 18:22 ` Daniel Shahaf 2017-11-19 14:46 ` Martijn Dekker 2017-11-19 19:41 ` Bart Schaefer 0 siblings, 2 replies; 19+ messages in thread From: Daniel Shahaf @ 2017-11-18 18:22 UTC (permalink / raw) To: zsh-workers Martijn Dekker wrote on Thu, 16 Nov 2017 02:52 +0200: > Op 15-11-17 om 01:52 schreef Daniel Shahaf: > > Okay, so how about if we demoted the fatal error to a warning? Like > > this: > > > > % [[ -o not_an_option ]] || echo This gets run > > zsh: [[: no such option: not_an_option > > This gets run > > % > > I think that would be fine, as well as returning status 2 along with it. > > I do think the warning (and the status 2) should be suppressed if > POSIX_IDENTIFIERS or some other emulation-relevant shell option is > active, so that 'emulate ksh' and 'emulate sh' reproduce ksh behaviour. How about the following. It uses status 3 because status 2 currently means "syntax error in [[" and I didn't want to overload that; and it uses POSIX_BUILTINS because that seemed more closely related than POSIX_IDENTIFIERS. Consider it a work in progress, i.e., particulars are still malleable. There are good arguments in favour of all sides here… (incumbent behaviour, Martijn's patch, this patch) Cheers, Daniel diff --git a/Doc/Zsh/cond.yo b/Doc/Zsh/cond.yo index e08fc0d36..4ca132a26 100644 --- a/Doc/Zsh/cond.yo +++ b/Doc/Zsh/cond.yo @@ -45,6 +45,10 @@ item(tt(-o) var(option))( true if option named var(option) is on. var(option) may be a single character, in which case it is a single letter option name. (See noderef(Specifying Options).) + +When no option named var(option) exists, and the tt(POSIX_BUILTINS) option +hasn't been set, return 3 with a warning. If that option is set, return 1 +with no warning. ) item(tt(-p) var(file))( true if var(file) exists and is a FIFO special file (named pipe). diff --git a/Doc/Zsh/options.yo b/Doc/Zsh/options.yo index 28155d796..d043cf398 100644 --- a/Doc/Zsh/options.yo +++ b/Doc/Zsh/options.yo @@ -2169,6 +2169,9 @@ command found in the path. Furthermore, the tt(getopts) builtin behaves in a POSIX-compatible fashion in that the associated variable tt(OPTIND) is not made local to functions. + +Moreover, the warning and special exit code from +tt([[ -o )var(non_existent_option)tt( ]]) are suppressed. ) pindex(POSIX_IDENTIFIERS) pindex(NO_POSIX_IDENTIFIERS) diff --git a/Src/cond.c b/Src/cond.c index b9a47cea5..9f13e07d7 100644 --- a/Src/cond.c +++ b/Src/cond.c @@ -61,7 +61,8 @@ static void cond_subst(char **strp, int glob_ok) * of functionality. * * Return status is the final shell status, i.e. 0 for true, - * 1 for false and 2 for error. + * 1 for false, 2 for syntax error, 3 for "option in tested in + * -o does not exist". */ /**/ @@ -86,10 +87,10 @@ case COND_NOT: if (tracingcond) fprintf(xtrerr, " %s", condstr[ctype]); ret = evalcond(state, fromtest); - if (ret == 2) - return ret; - else + if (ret == 0 || ret == 1) return !ret; + else + return ret; case COND_AND: if (!(ret = evalcond(state, fromtest))) { if (tracingcond) @@ -100,7 +101,8 @@ case COND_AND: return ret; } case COND_OR: - if ((ret = evalcond(state, fromtest)) == 1) { + ret = evalcond(state, fromtest); + if (ret == 1 || ret == 3) { if (tracingcond) fprintf(xtrerr, " %s", condstr[ctype]); goto rec; @@ -506,8 +508,12 @@ optison(char *name, char *s) else i = optlookup(s); if (!i) { - zwarnnam(name, "no such option: %s", s); - return 2; + if (isset(POSIXBUILTINS)) + return 1; + else { + zwarnnam(name, "no such option: %s", s); + return 3; + } } else if(i < 0) return !unset(-i); else diff --git a/Test/C02cond.ztst b/Test/C02cond.ztst index 38525016c..04e1ca8f2 100644 --- a/Test/C02cond.ztst +++ b/Test/C02cond.ztst @@ -440,6 +440,23 @@ F:Failures in these cases do not indicate a problem in the shell. > [[ 'a' == 'b' || 'b' = 'c' || 'c' != 'd' ]] >} + (setopt posixbuiltins; eval '[[ -o invalidoption ]]; echo set: $?'; echo "no warning" >&2) + (unsetopt posixbuiltins; [[ -o invalidoption ]]; echo unset: $?) + [[ -o invalidoption || -n nonempty ]]; echo "in disjunction, true: $?" + [[ -o invalidoption || -z nonempty ]]; echo "in disjunction, false: $?" + [[ ! -o invalidoption ]]; echo "negated: $?" +0:-o invalidoption +>set: 1 +?no warning +>unset: 3 +?(eval):2: no such option: invalidoption +>in disjunction, true: 0 +?(eval):3: no such option: invalidoption +>in disjunction, false: 1 +?(eval):4: no such option: invalidoption +>negated: 3 +?(eval):5: no such option: invalidoption + %clean # This works around a bug in rm -f in some versions of Cygwin chmod 644 unmodish ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] don't exit shell on [[ -o invalid@option ]] 2017-11-18 18:22 ` Daniel Shahaf @ 2017-11-19 14:46 ` Martijn Dekker 2017-11-19 19:41 ` Bart Schaefer 1 sibling, 0 replies; 19+ messages in thread From: Martijn Dekker @ 2017-11-19 14:46 UTC (permalink / raw) To: zsh-workers Op 18-11-17 om 20:22 schreef Daniel Shahaf: > How about the following. I, for one, am happy with it. Thanks, - M. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] don't exit shell on [[ -o invalid@option ]] 2017-11-18 18:22 ` Daniel Shahaf 2017-11-19 14:46 ` Martijn Dekker @ 2017-11-19 19:41 ` Bart Schaefer 2017-11-19 19:53 ` Peter Stephenson 1 sibling, 1 reply; 19+ messages in thread From: Bart Schaefer @ 2017-11-19 19:41 UTC (permalink / raw) To: zsh-workers On Sat, Nov 18, 2017 at 10:22 AM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote: > > How about the following. > > It uses status 3 because status 2 currently means "syntax error in [[" > and I didn't want to overload that; and it uses POSIX_BUILTINS because > that seemed more closely related than POSIX_IDENTIFIERS. For reasons I can't entirely define, it bothers me that an operation to test the setting of options changes its behavior based on the setting of an option. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] don't exit shell on [[ -o invalid@option ]] 2017-11-19 19:41 ` Bart Schaefer @ 2017-11-19 19:53 ` Peter Stephenson 2017-11-20 1:22 ` Daniel Shahaf 0 siblings, 1 reply; 19+ messages in thread From: Peter Stephenson @ 2017-11-19 19:53 UTC (permalink / raw) To: zsh-workers On Sun, 19 Nov 2017 11:41:57 -0800 Bart Schaefer <schaefer@brasslantern.com> wrote: > On Sat, Nov 18, 2017 at 10:22 AM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote: > > > > How about the following. > > > > It uses status 3 because status 2 currently means "syntax error in [[" > > and I didn't want to overload that; and it uses POSIX_BUILTINS because > > that seemed more closely related than POSIX_IDENTIFIERS. > > For reasons I can't entirely define, it bothers me that an operation > to test the setting of options changes its behavior based on the > setting of an option. I'm actually more worried if this *isn't* an option, given it's (in a more touchy feely sense) optional behaviour. Having something of that kind based on emulation seems a bit gross. But we do have gross syntax changes based on emulation, it's true, such as variables that never appear. pws ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] don't exit shell on [[ -o invalid@option ]] 2017-11-19 19:53 ` Peter Stephenson @ 2017-11-20 1:22 ` Daniel Shahaf 2017-11-24 21:50 ` Daniel Shahaf 0 siblings, 1 reply; 19+ messages in thread From: Daniel Shahaf @ 2017-11-20 1:22 UTC (permalink / raw) To: zsh-workers Peter Stephenson wrote on Sun, 19 Nov 2017 19:53 +0000: > On Sun, 19 Nov 2017 11:41:57 -0800 > Bart Schaefer <schaefer@brasslantern.com> wrote: > > On Sat, Nov 18, 2017 at 10:22 AM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote: > > > > > > How about the following. > > > > > > It uses status 3 because status 2 currently means "syntax error in [[" > > > and I didn't want to overload that; and it uses POSIX_BUILTINS because > > > that seemed more closely related than POSIX_IDENTIFIERS. > > > > For reasons I can't entirely define, it bothers me that an operation > > to test the setting of options changes its behavior based on the > > setting of an option. > Are you concerned about self-reference? I.e., that code meant to run under any options (example: z-sy-h) wouldn't be able to test the settedness of an option using [[ -o foo ]] because the behaviour of that command depends on whether foo is set? (That's basically the Liar Paradox) I don't think that's a problem in this case, but I'm listening... > I'm actually more worried if this *isn't* an option, given it's (in a > more touchy feely sense) optional behaviour. Having something of that > kind based on emulation seems a bit gross. But we do have gross syntax > changes based on emulation, it's true, such as variables that never > appear. I'm quite swamped this week (in a good way) but when I get a chance I'll finish the patch and push. (Needs more tests and a README blurb, at least) Cheers, Daniel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] don't exit shell on [[ -o invalid@option ]] 2017-11-20 1:22 ` Daniel Shahaf @ 2017-11-24 21:50 ` Daniel Shahaf 2017-12-03 7:28 ` Mikael Magnusson 0 siblings, 1 reply; 19+ messages in thread From: Daniel Shahaf @ 2017-11-24 21:50 UTC (permalink / raw) To: zsh-workers Daniel Shahaf wrote on Mon, Nov 20, 2017 at 01:22:41 +0000: > I'm quite swamped this week (in a good way) but when I get a chance I'll > finish the patch and push. (Needs more tests and a README blurb, at least) Interdiff: diff --git a/README b/README index 6fad1d516..59abf29b2 100644 --- a/README +++ b/README @@ -54,6 +54,18 @@ foo=([aeiou]\=vowel) This is only required for array values contained within parentheses; command line expansion for normal arguments has not changed. +3) The syntax + +[[ -o foo ]] + +where foo is not the name of a shell option (with optional underscores +and optional "no" prefix) used to be treated as a syntax error, i.e., +the enclosing command line or file were aborted. It now emits a warning +and returns a non-zero exit code. For further details, see the +documentation of the -o switch in the chapter "Conditional Expressions" +in the zshmisc(1) manual. + + Incompatibilities between 5.3.1 and 5.4.2 ----------------------------------------- diff --git a/Test/C02cond.ztst b/Test/C02cond.ztst index 04e1ca8f2..4ffb07dd4 100644 --- a/Test/C02cond.ztst +++ b/Test/C02cond.ztst @@ -440,14 +440,15 @@ F:Failures in these cases do not indicate a problem in the shell. > [[ 'a' == 'b' || 'b' = 'c' || 'c' != 'd' ]] >} - (setopt posixbuiltins; eval '[[ -o invalidoption ]]; echo set: $?'; echo "no warning" >&2) + (setopt posixbuiltins; [[ -o invalidoption ]]; echo set: $?; echo "line 1: no warning" >&2) (unsetopt posixbuiltins; [[ -o invalidoption ]]; echo unset: $?) [[ -o invalidoption || -n nonempty ]]; echo "in disjunction, true: $?" [[ -o invalidoption || -z nonempty ]]; echo "in disjunction, false: $?" [[ ! -o invalidoption ]]; echo "negated: $?" + [[ -o invalidoption && -n nonempty ]] || echo "in conjunction: $?" 0:-o invalidoption >set: 1 -?no warning +?line 1: no warning >unset: 3 ?(eval):2: no such option: invalidoption >in disjunction, true: 0 @@ -456,6 +457,8 @@ F:Failures in these cases do not indicate a problem in the shell. ?(eval):4: no such option: invalidoption >negated: 3 ?(eval):5: no such option: invalidoption +>in conjunction: 3 +?(eval):6: no such option: invalidoption %clean # This works around a bug in rm -f in some versions of Cygwin ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] don't exit shell on [[ -o invalid@option ]] 2017-11-24 21:50 ` Daniel Shahaf @ 2017-12-03 7:28 ` Mikael Magnusson 0 siblings, 0 replies; 19+ messages in thread From: Mikael Magnusson @ 2017-12-03 7:28 UTC (permalink / raw) To: Daniel Shahaf; +Cc: zsh workers On Fri, Nov 24, 2017 at 10:50 PM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote: > Daniel Shahaf wrote on Mon, Nov 20, 2017 at 01:22:41 +0000: >> I'm quite swamped this week (in a good way) but when I get a chance I'll >> finish the patch and push. (Needs more tests and a README blurb, at least) > > Interdiff: > > diff --git a/README b/README > index 6fad1d516..59abf29b2 100644 > --- a/README > +++ b/README > @@ -54,6 +54,18 @@ foo=([aeiou]\=vowel) > This is only required for array values contained within parentheses; > command line expansion for normal arguments has not changed. > > +3) The syntax > + > +[[ -o foo ]] > + > +where foo is not the name of a shell option (with optional underscores > +and optional "no" prefix) used to be treated as a syntax error, i.e., > +the enclosing command line or file were aborted. It now emits a warning > +and returns a non-zero exit code. For further details, see the > +documentation of the -o switch in the chapter "Conditional Expressions" > +in the zshmisc(1) manual. I don't know if it's also worth pointing out that you can do { [[ -o invalid_option ]] } always { TRY_BLOCK_ERROR=0 } since 4.2.1 or so? (It will suppress the syntax error but preserve the return code). -- Mikael Magnusson ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2017-12-03 7:28 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-11-10 18:24 [PATCH] don't exit shell on [[ -o invalid@option ]] Martijn Dekker 2017-11-10 18:53 ` Eric Cook 2017-11-10 19:03 ` Martijn Dekker 2017-11-10 22:37 ` Bart Schaefer 2017-11-11 12:45 ` Peter Stephenson 2017-11-11 19:01 ` Martijn Dekker 2017-11-11 23:19 ` Bart Schaefer 2017-11-12 19:56 ` Peter Stephenson 2017-11-14 12:26 ` Daniel Shahaf 2017-11-14 13:22 ` Martijn Dekker 2017-11-14 23:52 ` Daniel Shahaf 2017-11-16 0:52 ` Martijn Dekker 2017-11-18 18:22 ` Daniel Shahaf 2017-11-19 14:46 ` Martijn Dekker 2017-11-19 19:41 ` Bart Schaefer 2017-11-19 19:53 ` Peter Stephenson 2017-11-20 1:22 ` Daniel Shahaf 2017-11-24 21:50 ` Daniel Shahaf 2017-12-03 7:28 ` Mikael Magnusson
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/zsh/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).