Honor HISTCONTROL "ignorespace" and "ignoreboth" #96
Conversation
|
I've just submitted a very slight improvement to this PR, which uses @cornfeedhobo's suggestion to not require Bash 5+. I couldn't find a way to "fork" a PR, so I just submitted a new one which includes @ilya-bobyr's patch. It seems like this unexpected behavior has been brought up numerous times in several PRs and Issues, and I'm highly interested in getting this regression fixed to support all use cases and committed. |
Awesome :) |
|
Thanks @ilya-bobyr! Could you alsö rebase on master? It can't be merged as is 👍 |
ilya-bobyr
force-pushed
the
master
branch
2 times, most recently
from
77a5515 to
9b35b56
Compare
Sorry for the delay. It is done. |
|
There is an alternative suggestion in #119, so we anyway need to decide which one to pick.
But maybe it's still better than nothing. |
| # during setup, we need to remove commands that start with a space from | ||
| # the history ourselves. | ||
|
|
||
| # With bash 5.0 or above, we could have just ran |
There was a problem hiding this comment.
We can switch the implementation based on BASH_VERSINFO. Reducing a fork for Bash 5.0 is still nice.
There was a problem hiding this comment.
Just to make sure we are on the same page, I would like to clarify the choices here.
My understanding is as follows.
If the script wants to support bash versions earlier than 5.0 it needs to have this block that computes the history length explicit.
If we add a check for the bash version and have two different code paths, it means two time the testing and two times the maintenance.
Should the script fully drop bash support for versions before 5.0, then it would be possible to rewrite this code to use a better approach, and have only one code path.
For example, anyone running tests with bash 5.0 will not be checking pre-5.0 code anymore.
I guess, what you have in mind might be a "slow upgrade".
Where support for bash before version 5.0 is not explicitly dropped, but in practice there are less and less users of the older versions.
And bash 5.0 code path becomes the only actually used path.
With the pre-5.0 code is still there, just really unmaintained.
I'm not a big fan of unmaintained code, so it seems to me that it would be less desirable.
But if you want, I can certainly add an if statement.
There was a problem hiding this comment.
While I agree with you partly, I have to say that keeping compatibility with ancient bash versions is also a problem. Why do people have to "suffer", because of it?
How many people are really on bash <5?
I come from a performance background and doing a regex where it is not needed seems a waste to me. Of course this is not a huge strain on resources, but preexec is a hook, which means it is called all the time, thus is should be as sleek and performant as possible, wouldn't you agree?
Maintenance is a good point, however, I would still favor the code path that is most likely taken, which in my book is the bash >=5 path.
P.S.: Let's see what input akinomyoga has.
P.P.S.: After reading akinomyoga's reply below, I wanted to clarify that I didn't mean to drop support for bash < 5. I meant the if statement should be implemented.
There was a problem hiding this comment.
Thank you for updating. Bash-preexec already has a version check and supports Bash 3.1+. I'm not a maintainer, but I think it is not realistic to drop the support for Bash < 5. The situation for Bash and bash-preexec seems to be different from a typical project.
- Maybe we can drop the support for 3.1, but I think we still need to support 3.2 because 3.2 will continue to be shipped with macOS for the license issue.
- Also, LTS distributions will continue to provide a Bash version for ten years. Since Bash is tightly bound to the system, it's non-trivial to update the system-installed Bash for distributors. Although users can install the latest version of Bash in their home directory, it is difficult to force all the users to do so correctly.
- In a typical project, another option might be to maintain several branches of versions, where new features are added to the well-tested latest version, while older versions receive the corrections for real problems. However, the feature set of bash-preexec is stable and no feature was added for a long time, yet the implementation has been always unstable and incomplete. There hasn't been any stable version, and we would probably want to apply many patches retroactively even if we adopted such a model. Anyway, this model would add even more maintenance costs.
Although you seem to suggest two options, dropping support for Bash < 5.0 entirely or if-statement, if you care about the test coverage, I think we will need to keep the current version based on the old Bash features.
There was a problem hiding this comment.
I've updated my comment above. I just wanted to clarify that I didn't mean to drop support for bash < 5. I meant the if statement should be used to allow bash 5 users to use the more performant processing option.
There was a problem hiding this comment.
I prefer the #96 approach to #119. BASH_COMMAND is not reliable. After either one is merged, I'll submit a PR of further changes addressing the points in #96 (comment).
@rcaloras Could you take a look at this?
|
@ilya-bobyr Would it be possible to take a bit of time to resolve the conflicts? |
After 3458480 Remove ignorespace from $HISTCONTROL and after 7e55ac1 Follow up commit for issue rcaloras#6 -Replace ignoreboth with simpley ignoredups this script would remove 'ignorespace' and would replace 'ignoreboth' with 'ignoredups'. This effectively disables the functionality of not adding space prefixed commands into history. It used to happen siliently and could be quite confusing to users who use this feature. This script relies on the command to be in the history, but we can mostly fix the issue by "manual" removing a whitespace prefixed command from the history after reading it from there.
|
Rebased and applied 2 out of 3 suggestions. |
After 3458480
and after 7e55ac1
this script would remove 'ignorespace' and would replace 'ignoreboth'
with 'ignoredups'. This effectively disables the functionality of not
adding space prefixed commands into history. It used to happen
siliently and could be quite confusing to users who use this feature.
This script relies on the command to be in the history, but we can
mostly fix the issue by "manually" removing a whitespace prefixed command
from the history after reading it from there.