Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't allow abbreviated long options in getopt #7341

Open
faho opened this issue Sep 19, 2020 · 4 comments
Open

Don't allow abbreviated long options in getopt #7341

faho opened this issue Sep 19, 2020 · 4 comments
Labels
Milestone

Comments

@faho
Copy link
Member

faho commented Sep 19, 2020

I just noticed that our getopt implementation, taken from glibc and modified for wchar, allows unambiguous long option abbreviations.

That means string match --reg works even tho the option is --regex.

I don't think I have ever seen anyone use this on purpose.

Personally, I consider this to be a terrible design decision because it means that adding new long flags can break previous calls (e.g if we added --regex-dialect it would break all uses of string match --reg), and would like to remove it.

This is likely to be controversial, and the change would have to be backwards-incompatible (although, like I said, I have never seen anyone use this on purpose).

@faho faho added the RFC label Sep 19, 2020
@faho faho added this to the fish-future milestone Sep 19, 2020
@ridiculousfish
Copy link
Member

I think we should do this too. I wonder if we need to stage it at all. Maybe just land it after 3.2 and see whether it causes any trouble in HEAD.

@mqudsi
Copy link
Contributor

mqudsi commented Sep 23, 2020

I was going to add my +1 to ridiculousfish's second suggestion, but upon second thought: there's no real benefit to doing this at all. At best, it prevents new scripts written after the next release from incorrectly/inadvertently using aliases to long options that might break if we add another option that results in an ambiguous abbreviation, but at the cost of definitely breaking any and all existing scripts that do that.

On the flip side, we don't advertise this option anywhere, we've certainly shipped new long opts that potentially broke abbreviations of old long opts that had the same prefix in the past, and we'd be well within our rights to continue to do so as if faho never stumbled upon this quirk going forward.

I don't think it's "controversial" either way, but I just don't think there's really a measurable benefit to making the change.

@faho
Copy link
Member Author

faho commented Sep 23, 2020

I mean the obvious benefit is that we're removing a weird and confusing stumbling block.

It's a misfeature that people might accidentally use, that might then break their stuff when we add otherwise backwards-compatible features.

Removing it is ripping off the band-aid in one go.

@PatrickF1
Copy link

Just adding my 2 cents here: as a plugin developer, I would love to get this fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants