oh-my-bash icon indicating copy to clipboard operation
oh-my-bash copied to clipboard

Issue with rm command in omb when autocompleting wrong capitalization

Open amrinder-cs opened this issue 1 year ago • 8 comments

So i had a bunch of temporary files created in capital letters, named:

ABCD.zip ABCD-arm.zip ABCD-X86.zip

etc. I also had: abcd/ abcd-arm/

I tried to delete those zips, so i did rm -rf ABCD* while using oh-my-bash, however it also deleted my important folders abcd/ and abcd-arm/ I'd be in great trouble if i hadn't committed to git.

Let bash handle the way it normally does so it doesn't happen with someone else.

an example:

rofl

amrinder-cs avatar Oct 04 '24 03:10 amrinder-cs

This is related to the following setting in OMB's ~/.bashrc.

https://github.com/ohmybash/oh-my-bash/blob/e712342f4455e085b9c86120de335d242c39865a/templates/bashrc.osh-template#L18

This variable is referenced here to set up Bash's option nocaseglob.

https://github.com/ohmybash/oh-my-bash/blob/e712342f4455e085b9c86120de335d242c39865a/lib/shopt.sh#L27-L33

Your report seems to suggest that turning nocaseglob on by default is not a good idea, but there might be another reason for the current behavior. We need to investigate the background of why we have been turning on nocaseglob by default. In addition, nocaseglob seems to have been turned on by default for a long time, so if we change the default behavior now, it will affect a large number of users.

The setting CASE_SENSITIVE originates in the upstream Oh My Zsh (OMZ):

https://github.com/ohmyzsh/ohmyzsh/blob/62cf1201b031399e7251abeee859e895ee825a48/templates/zshrc.zsh-template#L20

In the upstream OMZ, this variable is used to set the behavior of the completion. It is not used to control the behavior related to the pathname expansions.

https://github.com/ohmyzsh/ohmyzsh/blob/62cf1201b031399e7251abeee859e895ee825a48/lib/completion.zsh#L16-L26

The current behavior in OMB was introduced in #399. Originally, CASE_SENSITIVE only affected the behavior of the completion (bind "set completion-ignore-case on"), but @jjmcdn has extended it to affect also nocaseglob and nocasematch (though nocasematch was reverted in #503). Before that, we had been always turning on nocaseglob. This nocaseglob seems to come from bash-sensible:

https://github.com/mrzool/bash-sensible/blob/89fa380e3d46210a85b4236098ada2c2ae280ac4/sensible.bash#L33-L34

The line was added by @mrzool in the following commit in bash-sensible, which implies that turning on nocaseglob is considered sensible:

https://github.com/mrzool/bash-sensible/commit/e2cabcb9bc79e585b42e10b1a3e83ce2990231fd

@jjmcdn @mrzool Do you have any thoughts on the default setting for the Bash option shopt -s nocaseglob?

akinomyoga avatar Oct 04 '24 05:10 akinomyoga

I mean it is fine at auto-completing case-sensitive stuff, however it should not override the command behavior based on that, it should be limited to just Autocomplete.

Such as:

  • Suggesting me based on non case autocomplete: GOOD
  • Also acting while ignoring the case: BAD. It should be: "what i see is what i get".

amrinder-cs avatar Oct 07 '24 04:10 amrinder-cs

As described above, the story is that even before the setting for the completion started to override the behavior for the pathname expansions in #399, nocaseglob (i.e. the setting for the pathname expansions to ignore the case) had been turned on for a long time. That is, #399 technically opened a way to turn off nocaseglob through OMB_CASE_SENSITIVE.

I kind of agree with you that nocaseglob shouldn't be turned on by default, but it contradicts bash-sensible. We want to check the background.

It should be: "what i see is what i get".

What you see is TEST*, but it is obvious that you don't expect to delete the file of the literal name as if rm -rf 'TEST*'. The meaning of TEST* changes depending on nocaseglob, and this is a feature of Bash.

akinomyoga avatar Oct 07 '24 05:10 akinomyoga

@mrzool Sorry for bothering you after eight years, but if you have time, do you remember the background of https://github.com/mrzool/bash-sensible/commit/e2cabcb9bc79e585b42e10b1a3e83ce2990231fd in bash-sensible?

akinomyoga avatar Oct 07 '24 05:10 akinomyoga

Hey @akinomyoga, sorry for the delay. That’s a really good question. I tried to recall and checked my notes, but I couldn’t find anything. I also checked my personal configuration and noticed I’m not using that option on my machine. If I ever had a reason to add it, that reason is long forgotten by now, and I’m happy to remove it before anyone risks losing data. Interestingly, I don’t think it’s ever been reported.

mrzool avatar Oct 12 '24 14:10 mrzool

@mrzool Thank you for taking the time to check them and reply to us! I understand that it would often be difficult to recall the background of the options after eight years.

Since the native assumption by the users that the glob matching would be case sensitive would cause an actual problem as reported here, if there isn't an obvious breakage by turning off nocaseglob, I'm now thinking of turning off nocaseglob by default and see if any problems are reported by users. If there are problems, we can revert the change.

akinomyoga avatar Oct 14 '24 07:10 akinomyoga

@akinomyoga Sounds good. Done now!

mrzool avatar Oct 17 '24 19:10 mrzool

@mrzool Thank you for applying this in the upstream! I also added a change in the corresponding part in this repository in commit 4238db509548b15ef11f942d4eaf1ed6361576bb.

akinomyoga avatar Oct 18 '24 07:10 akinomyoga

I think we can now close this issue as no reports arose after applying the change. Thank you all.

akinomyoga avatar Mar 21 '25 08:03 akinomyoga