ohmybash / oh-my-bash

A delightful community-driven framework for managing your bash configuration, and an auto-update tool so that makes it easy to keep up with the latest updates from the community.
https://ohmybash.github.io
MIT License
5.54k stars 626 forks source link

lib/shopt: remove nocasematch #503

Closed akinomyoga closed 6 months ago

akinomyoga commented 6 months ago

The setting shopt -s nocasematch affects every pattern matching in Bash including the case statement, [[ str == pat ]] matching, ${var/pat/rep}, and ${var#pat}. Since the affected range is too large, this potentially breaks the existing functions. I think this setting shopt -s nocasematch should be enabled locally when it is specifically needed rather than enabling it globally.


These settings seem to have been introduced in #399 where the proper support for CASE_SENSITIVE was attempted. Before this PR, only nocaseglob has been specified in OMB.

@jjmcdn Could you take a look at this PR? Since you have introduced nocasematch, I need to check the background and motivations of making nocasematch controlled by CASE_SENSITIVE. As I've described above, nocasematch (in the latest Bash version) affects all the matching behavior using glob to be case insensitive by default. However, this caused an issue at https://github.com/akinomyoga/ble.sh/discussions/376. The problem is that unexpected replacement happens with ${var/pat/str} when the pattern pat matches the text content of $var in a case-insensitive way. The affected range also includes the case statement. For example,

(shopt -s nocasematch; case A in a) echo small ;; A) echo capital ;; esac)

will output small instead of capital. Also the string comparison by [[ lhs == rhs ]] is also affected by nocasematch.

I'm now thinking about the possibility of excluding nocasematch from the list of settings controlled by CASE_SENSITIVE (as in this PR). The setting nocasematch doesn't seem to be the one that should be turned on globally since the affected range is too large.

jjmcdn commented 6 months ago

Sure, I'll take a look at it today. I need to refresh my memory on what I was trying to address specifically in #399 since it's been a while. I know my use-case is the traditional UNIX style, where I don't want anything to match implicitly (ie. if I want to match upper case or lower case I'll use the shell regex engine for that), but I know I was trying to make sure that worked without negatively impacting the case-insensitive users.

I do think that since bash has a very rich and robust built-in regex engine it's probably better used in any internal functions where nocasematch could have a negative impact on the user experience.

I'll follow up later with a more detailed comment, though.

akinomyoga commented 6 months ago

Thank you for your reply!

I do think that since bash has a very rich and robust built-in regex engine it's probably better used in any internal functions where nocasematch could have a negative impact on the user experience.

Ah, sorry. I forgot to tell the regex matching of Bash is also affected by nocasematch. For example, you may check the results of the following commands:

$ (shopt -u nocasematch; [[ alpha =~ ALPHA ]]; echo $?)
1
$ (shopt -s nocasematch; [[ alpha =~ ALPHA ]]; echo $?)
0

With nocasematch, ALPHA matches alpha. So switching to regex matching wouldn't change the situation where nocasematch has a negative impact.

akinomyoga commented 6 months ago

Thank you for your confirmation.

For me, nocasematch is important since I don't want case-insensitive matching on my tab-completions for things like branch names, for example, but I can try to work on something in the background that might allow it to be re-enabled later on without the negative side-effects.

I'm confused. Maybe your assumption is the opposite? If you don't want case-insensitive matching, you need to turn off nocasematch. The Bash default is shopt -u nocasematch, i.e., case-sensitive. The current master branch of OMB is case-insensitive by default because of #399, which is the problem. This PR tries to keep the Bash default for nocasematch regardless of the setting OMB_CASE_SENSITIVE, i.e., always keep the case-sensitive matching, which seems to be the behavior you want.

jjmcdn commented 6 months ago

Thank you for your confirmation.

For me, nocasematch is important since I don't want case-insensitive matching on my tab-completions for things like branch names, for example, but I can try to work on something in the background that might allow it to be re-enabled later on without the negative side-effects.

I'm confused. Maybe your assumption is the opposite? If you don't want case-insensitive matching, you need to turn off nocasematch. The Bash default is shopt -u nocasematch, i.e., case-sensitive. The current master branch of OMB is case-insensitive by default because of #399, which is the problem. This PR tries to keep the Bash default for nocasematch regardless of the setting OMB_CASE_SENSITIVE, i.e., always keep the case-sensitive matching, which seems to be the behavior you want.

Oh yes, yes, you're absolutely right. I was confusing what I thought was the desired default behaviour for OMB and the OMB_CASE_SENSITIVE setting and what we were seeing! That's for the clarification and I'm sorry about the confusion. :smiley:

akinomyoga commented 6 months ago

Thank you!