search

rcaloras / bash-preexec

⚡ preexec and precmd functions for Bash just like Zsh.
MIT License
862 stars 94 forks source link

Use POSIX syntax for Bash detection #127

Closed gaelicWizard closed 2 years ago

gaelicWizard commented 2 years ago

Don't depend on running modern Bash/ksh/zsh when we're trying to detect if we're running a modern bash.

dimo414 commented 2 years ago

Is there a particular shell / version where the existing code doesn't work?

gaelicWizard commented 2 years ago

Most major linux distributions ship /bin/sh as Dash, which is explicitly POSIX-compatible and nothing else.

dimo414 commented 2 years ago

Ok, if the goal is to support Dash it'd be good to add test coverage of that. Can't hurt to add a comment here as well, noting the syntax is intentional to support Dash. I'm still not certain there's a need to support Dash, but it doesn't hurt.

gaelicWizard commented 2 years ago

It's not really about dash specifically, but rather about the assumption that /bin/sh is bash. I'm sure there's more than a few places that assumption still survives. Dash is just a good example because it made waves when Debian switched /bin/sh; there's pages and pages of discussion in mailing list archives about it.

dimo414 commented 2 years ago

the assumption that /bin/sh is bash.

I don't mean to come across as thick, but where exactly is that assumption being made? The installation instructions (not to mention the project's name) are clearly specific to bash, and I don't see any reference in this repo to /bin/sh. I don't understand how someone could end up sourcing bash-preexec.sh in a dash shell short of misconfiguring their rc files.

gaelicWizard commented 2 years ago

I don't disagree, but then why check at all?

dimo414 commented 2 years ago

IMO it would be reasonable to not have this check and put the burden on the user to ensure their shell(s) are configured correctly, but it's probably not practical to remove at this point. Notably, #113 reported Zsh users accidentally sourcing bash-preexec (I imagine because they chose to source ~/.bashrc from their ~/.zshrc or similar, which is probably not a good idea but people do it anyways). For reference, the check was added back in bbec4c79 seemingly just out of an abundance of caution.

So just to be clear, you're not actually running into a situation where you expect to source bash-preexec in a Dash shell, right?

gaelicWizard commented 2 years ago

Correct 👍

dimo414 commented 2 years ago

In that case I'm -1 on this change simply because it's not needed. But if you feel strongly this line should be dash-compatible it would be appropriate to include test coverage to enforce this support going forward.

But in that vein it's also worth considering what would happen if bash-preexec ever utilized some bash syntax that dash couldn't even parse (causing it to fail before even reaching this line). IMO it would be wrong to avoid that syntax just because it breaks dash, since the library isn't intended to work with dash in the first place. Which suggests to me that we shouldn't bother with dash compatibility at all.

gaelicWizard commented 2 years ago

That's fair.

Since it's a shellscript, it would exit before reaching anything past those top lines, which I understand to be the point of moving those lines to the top of the file. It won't even parse the whole script at all; it's not compiled 😉

dimo414 commented 2 years ago

My point is simply that if such a situation arose I don't think it'd be worth reverting the (presumably desirable) bashism in order to satisfy dash.

akinomyoga commented 2 years ago

I don't think the current check in the master, which requires Bash to check if it is Bash, makes any sense. I think we should merge this PR or otherwise completely remove the check for Bash. In my opinion, even if it is not consistent with the later codes which use full Bash features, it is better to check the current shell using the POSIX feature. There are always beginner users doing something strange that we cannot predict.

dimo414 commented 2 years ago

I for one don't mind removing the check, but if people feel strongly that the check needs to be dash-compatible there should be test coverage of that behavior.

akinomyoga commented 2 years ago

Thanks for your prompt reply! Then, I feel we can add tests and merge it. [ This is a side note but the check is not only affected by ash family (dash/ash/busybox sh) but also by other (non-ksh-like) POSIX shells including /bin/sh of each BSDs, Solaris, etc. ]

rcaloras commented 2 years ago

Late arrival to this thread! FWIW, this check was originally POSIX compatible and was changed without much consideration to use [[ as part of this PR https://github.com/rcaloras/bash-preexec/pull/7/files.

rcaloras commented 2 years ago

Appreciate the PR and everyone's input. I agree with @dimo414's take. However, in this situation I'm supportive if other contributors are interested in merging. I don't think there's any harm and would rather be pragmatic than turn into a larger debate.