Honor HISTCONTROL "ignorespace" and "ignoreboth"#96
Honor HISTCONTROL "ignorespace" and "ignoreboth"#96illia-bobyr wants to merge 1 commit intorcaloras:masterfrom
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 👍 |
illia-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 just realized I didn't address this comment.
If the consensus is to add an if statement, I can do it.
There was a problem hiding this comment.
I suggested adding an if-statement for reducing the fork(2) cost, but, as you replied, the issue with the maintenance cost also seems a valid concern. The maintainer can choose either one by their preferences, but I'm not a maintainer. Both "using an if-statement" and "using a single compatible approach" work for me.
edit: Yeah, I replied the same in #96 (review) in the past.
akinomyoga
left a comment
akinomyoga
left a comment
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? |
illia-bobyr
force-pushed
the
master
branch
2 times, most recently
from
85b9e3e to
6bcdfb3
Compare
|
Rebased and applied 2 out of 3 suggestions. |
akinomyoga
left a comment
akinomyoga
left a comment
There was a problem hiding this comment.
Anyway, either the current version or the if-statement works for me. No need to further delay it. Thank you for your quick response to the old PR! I'm not a maintainer, but let me approve the PR of the current state.
illia-bobyr
force-pushed
the
master
branch
2 times, most recently
from
79d419c to
17441e9
Compare
|
I've created a separate PR to update the test runner image: #171 |
|
@ilya-bobyr Thanks for updating the PR! @rcaloras Thank you for checking the existing PRs! Could you also take a look at this PR? I think the change in this PR should be incorporated, though this PR has been rebased many times to be consistent with the master branch. The original manipulation of |
|
@akinomyoga, thank you for fixing style issues in tests. |
akinomyoga
left a comment
akinomyoga
left a comment
There was a problem hiding this comment.
Actually, it was not simply a style issue, but Bats, the underlying testing framework, distinguishes [ ] and [[ ]]. Bats doesn't treat the latter as a test item.
I recently obtained write access to this repository. I can technically merge this PR and want to, but I haven't yet understood my role in this repository. Let us ping @rcaloras: This PR has been open for seven years, and the author has been consistently responsive during that period. I feel it is unfair to delay merging further. I think the actual changes also make sense.
Got it. Thank you for the explanation.
Do not feel pressured by the time the PR was open :) |
|
Thanks for the ping and continued interest all. On the surface, I've been against this change since we manipulate the history preferences on purpose and intentionally to get a clean version of the command string. It's effectively a compromise/requirement even if some users may be unaware of the manipulation. What this change does is effectively then try to preserve them even though it'd no longer be configured to do so in With that said, @akinomyoga I'll defer to you if you want to merge this as a primary maintainer now. My only request @illia-bobyr would be to rewrite as well to remove the use of |
illia-bobyr
force-pushed
the
master
branch
7 times, most recently
from
e6bd221 to
8ef77ad
Compare
I can see how the fact that Without this change, if I use What happened to me is that I thought that certain commands are not recorded. I think a compromise of the In the first option, the value of I doubt that changing I am not aware of a way to get the ideal solution.
Removed P.S. Not sure if it is OK to keep |
|
@rcaloras Thank you for sharing your thoughts!
The status of the current Lines 425 to 432 in 8ef77ad Or, in the previous version you reviewed, Lines 420 to 425 in d2376b7
I think we can leave a custom name, such as
Yes, you are right...
I don't think it is possible to reliably get the previous command even in the latest Bash version.
|
| history_last=$(LC_ALL=C HISTTIMEFORMAT='' builtin history 1) | ||
| history_last="${history_last#*[[:digit:]][* ] }" |
There was a problem hiding this comment.
Could you use __bp_load_this_command_from_history? The same for line 678.
There was a problem hiding this comment.
New version is different here, but it is possible that a similar argument can be made there.
I still extract values from history explicitly in the test, rather than calling __bp_load_this_command_from_history.
It was a deliberate decision in the old version.
With the idea being, we do not want to rely on the code that we are testing while we are testing it.
In the old version __bp_load_this_command_from_history was pretty trivial, so it was a somewhat weak argument.
But in the currently proposed version, I've moved quite a lot of functionality into __bp_load_this_command_from_history, so, I think, it now is a pretty strong argument for not using __bp_load_this_command_from_history as a helper in tests.
I can add a helper function that wraps specifically the "take the last element from history" logic.
It is only repeated twice, so I'm not sure if it is worth the effort :)
But I can do it, if you insist.
|
Sorry for the delay. Here is an updated version that uses This turned out to be a pretty substantial change. Initially I've used For the same reason I've added verbose warnings in cases when the behavior could be confusing. The behavior is described in details in the Please take a look and let me know what you think. |
| local ignorespace='' | ||
| local ignoredups='' | ||
|
|
||
| if [[ "$histcontrol" == *":ignorespace:"* ]]; then | ||
| ignorespace=yes | ||
| fi | ||
|
|
||
| if [[ "$histcontrol" == *":ignoreboth:"* ]]; then | ||
| ignorespace=yes | ||
| ignoredups=yes | ||
| fi | ||
|
|
||
| if [[ "$histcontrol" == *":ignoredups:"* ]]; then | ||
| ignoredups=yes | ||
| fi | ||
|
|
||
| if [[ -z "$ignorespace" ]]; then | ||
| return | ||
| fi | ||
| export HISTCONTROL="$histcontrol" | ||
|
|
||
| histcontrol=${histcontrol//:ignorespace:} | ||
| histcontrol=${histcontrol//:ignoreboth:} | ||
| histcontrol=${histcontrol//:ignoredups:} | ||
|
|
||
| if [[ -n "$ignoredups" ]]; then | ||
| histcontrol="${histcontrol}:ignoredups:" | ||
| fi | ||
| histcontrol="${histcontrol}:bash-preexec_ignorespace:" |
There was a problem hiding this comment.
The logic seems far more complicated than it should be. The new implementation appears to attempt to resolve the duplicates that the user has introduced, but that's unrelated to what we need. I don't think we should do extra modifications (in particular if it significantly increases lines of code).
| local ignorespace='' | |
| local ignoredups='' | |
| if [[ "$histcontrol" == *":ignorespace:"* ]]; then | |
| ignorespace=yes | |
| fi | |
| if [[ "$histcontrol" == *":ignoreboth:"* ]]; then | |
| ignorespace=yes | |
| ignoredups=yes | |
| fi | |
| if [[ "$histcontrol" == *":ignoredups:"* ]]; then | |
| ignoredups=yes | |
| fi | |
| if [[ -z "$ignorespace" ]]; then | |
| return | |
| fi | |
| export HISTCONTROL="$histcontrol" | |
| histcontrol=${histcontrol//:ignorespace:} | |
| histcontrol=${histcontrol//:ignoreboth:} | |
| histcontrol=${histcontrol//:ignoredups:} | |
| if [[ -n "$ignoredups" ]]; then | |
| histcontrol="${histcontrol}:ignoredups:" | |
| fi | |
| histcontrol="${histcontrol}:bash-preexec_ignorespace:" | |
| histcontrol=${histcontrol//:ignorespace:/:bash-preexec_ignorespace:} | |
| histcontrol=${histcontrol//:ignoreboth:/:ignoredups::bash-preexec_ignorespace:} |
There was a problem hiding this comment.
The name bash-preexec_ignorespace (with different separators - and _) feels a bit awkward to me. In particular, _ appears to have a stronger connectivity, so it looks "bash" + "preexec_ignorespace" at first glance. We already have variables in the name bash_preexec_imported (introduced by #123), so if you don't like __bp_, can we instead use bash_preexec_ignorespace?
There was a problem hiding this comment.
The logic seems far more complicated than it should be. The new implementation appears to attempt to resolve the duplicates that the user has introduced, but that's unrelated to what we need. I don't think we should do extra modifications (in particular if it significantly increases lines of code).
I'm not sure, I would agree to the "far too complicated" classification %)
I think it is pretty straightforward.
First we check, if there are any flags in $HISTCONTROL that we care about.
Then we act accordingly.
The deduplication is more of a side effect, rather than the goal.
In order to show a warning, I need to know if $HISTCONTROL was changed.
And separating the check from the action here helps.
But if you really feel strongly about it, I've replaced explicit checks with an unconditional replacement.
Also, if the warning is not going to be displayed, the need to check if any changes were performed is gone.
The name
bash-preexec_ignorespace(with different separators-and_) feels a bit awkward to me. In particular,_appears to have a stronger connectivity, so it looks "bash" + "preexec_ignorespace" at first glance. We already have variables in the namebash_preexec_imported(introduced by #123), so if you don't like__bp_, can we instead usebash_preexec_ignorespace?
This is not a name of a variable inside bash-preexect code, but something a user might see without the bash-preexec context.
This is why, I figured, using the proper name of the extension is the best.
I think it is absolutely the case that some people can read it as "bash" + "preexec_ignorespace".
But using the same separate does not make it less so, does it?
The reason I've used a different separator is exactly to make it more likely that people will read it as "bash-preexec" and "ignorespace".
I do not care much, so I would go with whatever you will agree to merge.
But if you ask me, I'm not sure that bash_preexec_ignorespace is really better than bash-preexec_ignorespace, especially considering the extension is called "bash-preexec".
We may consider is a different separator.
For example, it can be bash-preexec/ignorespace, or bash_preexec/ignorespace.
I think with a forward slash the chances of someone reading it as "bash" and "preexec/ignorespace" are lower.
What do you think?
P.S. It is better to create new threads for unrelated suggestions.
It makes it easier to understand which replies apply to which suggestions, and they can be resolved independently of one another.
| histcontrol=${histcontrol#:} | ||
| histcontrol=${histcontrol%:} | ||
|
|
||
| if [[ -z "${BP_HISTCONTROL_ACK:-}" ]]; then |
There was a problem hiding this comment.
Does this mean we start to show messages always when the user sets ignorespace or ignoredups? I don't think we should do this. Setting ignorespace or ignoredups isn't a special setting. In addition, bash-preexec has recently been included by the integration code of many terminals by default, and users don't necessarily recognize it. I don't think that always requiring BP_HISTCONTROL_ACK when the user wants to set ignorespace and ignoredups is a good idea. In addition, we haven't required this, while we've modified HISTCONTROL for a long time.
Could you make the feature of showing this message opt-in instead of opt-out?
There was a problem hiding this comment.
Also, I would like to perform the re-adjustment of HISTCONTROL in the preexec processing.
After the shell initialization, HISTCONTROL may be modified by user commands, so we can check and adjust HISTCONTROL in the precmd timing (which is similar to postexec). We are already adjusting PROMPT_COMMAND in __bp_precmd_invoke_cmd through ___bp_install_prompt_command, so we can also call __bp_adjust_histcontrol there. As a related matter, the current PR head tries to detect the user's modification in __bp_load_this_command_from_history and doesn't try to resolve the issue of grabbing the wrong entry. In this perspective, I think that should be done in the precmd timing, and doing so would give a chance to adjust HISTCONTROL before the commands starting with whitespace are lost, which reduces to the idea of re-adjustment of HISTCONTROL in the preexec processing.
If you want to show messages by default when user commands modify HISTCONTROL to include ignorespace or ignoredups, __bp_adjust_histcontrol may issue the messages only when it is called from __bp_precmd_invoke_cmd (i.e., when we detect the change of HISTCONTROL after the shell initialization completes). The function __bp_precmd_invoke_cmd may pass an argument to __bp_adjust_histcontrol to enable the message.
There was a problem hiding this comment.
The prefix BP_ seems unclear to users, equally as __bp_ignorespace that you rejected in HISTCONTROL.
Can we instead use bash_preexec_histcontrol_* in the same variable naming as bash_preexec_imported? BP_PIPESTATUS provides the same feature as PIPESTATUS, so I feel it is unavoidable to use a similar and short name (i.e., using bash_preexec_pipestatus in place of PIPESTATUS makes the code appearance significantly be different, and it doesn't associate itself with the original PIPESTATUS compared to BP_PIPESTATUS). I'm not sure if we should extend the naming BP_ to more places, while we already have bash_preexec_imported. The opposite direction might be to unify bash_preexec_imported to bp_imported or BP_IMPORTED, but we already changed the variable name from __bp_imported to bash_preexec_imported, and I'd like to avoid changing the variable name multiple times.
There was a problem hiding this comment.
Does this mean we start to show messages always when the user sets
ignorespaceorignoredups? I don't think we should do this. Settingignorespaceorignoredupsisn't a special setting. In addition,bash-preexechas recently been included by the integration code of many terminals by default, and users don't necessarily recognize it. I don't think that always requiringBP_HISTCONTROL_ACKwhen the user wants to setignorespaceandignoredupsis a good idea. In addition, we haven't required this, while we've modifiedHISTCONTROLfor a long time.Could you make the feature of showing this message opt-in instead of opt-out?
Yes, the idea was to show the warning once if $HISTCONTROL is modified.
Users who use bash-preexec can set the ack flag variable in their .bashrc before loading bash-preexec. And they would not see this warning anymore.
This would be the most explicit way, hopefully making sure nothing unexpected happens due to the $HISTCONTROL manipulation.
If bash-preexec is part of an existing profile configuration that the user did not setup themselves, indeed it might be confusing that after an update they now see a warning.
I do not think, it is not such a big deal, as the warning explicitly explains why is it shown and one can just copy/paste a line from the warning into their .bashrc to never see it again, right?
Here is the warning text:
bash-preexec: NOTE: HISTCONTROL has been modified to support command extraction.
Original: $histcontrol_orig
Modified: $histcontrol
You can set BP_HISTCONTROL_ACK to a non-emtpy value to avoid this message.
export BP_HISTCONTROL_ACK=yes
I personally would prefer a warning, but I can see how some people might be upset that their existing profile starts to show warnings after an update.
I have removed it from the new version, as you seems to be against it.
But if my arguments for it sounds reasonable to you, I'll bring it back.
Also, I would like to perform the re-adjustment of
HISTCONTROLin thepreexecprocessing.After the shell initialization,
HISTCONTROLmay be modified by user commands, so we can check and adjustHISTCONTROLin theprecmdtiming (which is similar topostexec). We are already adjustingPROMPT_COMMANDin__bp_precmd_invoke_cmdthrough___bp_install_prompt_command, so we can also call__bp_adjust_histcontrolthere. As a related matter, the current PR head tries to detect the user's modification in__bp_load_this_command_from_historyand doesn't try to resolve the issue of grabbing the wrong entry. In this perspective, I think that should be done in theprecmdtiming, and doing so would give a chance to adjustHISTCONTROLbefore the commands starting with whitespace are lost, which reduces to the idea of re-adjustment ofHISTCONTROLin thepreexecprocessing.If you want to show messages by default when user commands modify
HISTCONTROLto includeignorespaceorignoredups,__bp_adjust_histcontrolmay issue the messages only when it is called from__bp_precmd_invoke_cmd(i.e., when we detect the change ofHISTCONTROLafter the shell initialization completes). The function__bp_precmd_invoke_cmdmay pass an argument to__bp_adjust_histcontrolto enable the message.
If we invoke __bp_adjust_histcontrol on every prompt update it would make it impossible for the user to add ignorespace or ignoreboth into $HISTCONTROL at all.
Current behavior is that $HISTCONTROL is updated only during bash-preexec setup.
And I personally would prefer to keep it this way.
I think changing that would be orthogonal to the intent of this PR.
So, if you are OK with that, I would like to avoid this change for now.
The second warning I've added covers this case - one that is shown when $HISTCONTROL contains ignorespace or ignoreboth despite the adjustment at installation time.
bash-preexec does not prevent the user from setting $HISTCONTROL to anything they want, but informs if the new configuration breaks some bash-preexec functionality.
Not all tools that rely on bash-preexec need the last command text in the first place.
I personally use bash-preexec for the command execution stats.
And it does not care for the last command text.
But I do use ignorespace.
The prefix
BP_seems unclear to users, equally as__bp_ignorespacethat you rejected inHISTCONTROL.Can we instead use
bash_preexec_histcontrol_*in the same variable naming asbash_preexec_imported?BP_PIPESTATUSprovides the same feature asPIPESTATUS, so I feel it is unavoidable to use a similar and short name (i.e., usingbash_preexec_pipestatusin place ofPIPESTATUSmakes the code appearance significantly be different, and it doesn't associate itself with the originalPIPESTATUScompared toBP_PIPESTATUS). I'm not sure if we should extend the namingBP_to more places, while we already havebash_preexec_imported. The opposite direction might be to unifybash_preexec_importedtobp_importedorBP_IMPORTED, but we already changed the variable name from__bp_importedtobash_preexec_imported, and I'd like to avoid changing the variable name multiple times.
There is a difference in what is used in $HISTCONTROL and a variable user sets in .bashrc.
Flags in $HISTCONTROL need to make sense in the context of just bash.
And this is why, I think, using an abbreviation could make it harder to understand what is going on.
I imagine people would only care about the value in $HISTCONTROL when something does not work as expect.
I think, this was one of the arguments from @rcaloras against this PR - that the behavior is different from what $HISTCONTROl prescribes.
So, if I, as a user of bash, am confused as to why commands are not retained, I would look at $HISTCONTROL. I might not be aware that bash-preexec is involved in any way.
Seeing bp_ignorespace gives me less information on to who is erasing commands from the command history, compared to bash-preexec_ignorespace.
BP_HISTCONTROL_ACK (and $BP_EMPTY_LAST_COMMAND_ACK), on the other hand, are variables that the user will set fully aware that they are dealing with bash-preexec.
They will set these variables either after reading the documentation, or after seeing the warning messages.
And they add assignments into these variables into their .bashrc.
Most likely after the code that loads bash-preexec.
I have this in my .bashrc:
if [[ -d "$bashMods/bash-preexec" && -r "$bashMods/bash-preexec/bash-preexec.sh" ]]; then
source "$bashMods/bash-preexec/bash-preexec.sh"
fiI think it is clear enough what the BP_ prefix would mean in this context.
And turning it into BASH_PREXEC_ is a bit of an overkill.
Compare
if [[ -d "$bashMods/bash-preexec" && -r "$bashMods/bash-preexec/bash-preexec.sh" ]]; then
export BP_HISTCONTROL_ACK=yes
source "$bashMods/bash-preexec/bash-preexec.sh"
fito
if [[ -d "$bashMods/bash-preexec" && -r "$bashMods/bash-preexec/bash-preexec.sh" ]]; then
export BASH_PREEXEC_HISTCONTROL_ACK=yes
source "$bashMods/bash-preexec/bash-preexec.sh"
fiBut if you want, I can change it.
I do not think a comparison with bash_preexec_imported and BP_PIPESTATUS is 100% apples to apples. Both $bash_preexec_imported and $BP_PIPESTATUS are used by other libraries that rely on bash-preexec.
While $BP_HISTCONTROL_ACK and $BP_EMPTY_LAST_COMMAND_ACK are more of an end user interface.
But I can see how one would want to have a consistent interface.
| export BP_HISTCONTROL_ACK=yes | ||
|
|
||
| EOM | ||
| export BP_HISTCONTROL_ACK=yes |
There was a problem hiding this comment.
export is unnecessary.
Actually, export for HISTCONTROL is unnecessary too, but I haven't pointed it out because that's an existing code, and fixing it is unrelated to the scope of this PR.
There was a problem hiding this comment.
Why is it unnecessary?
I do not want to show the same warning again.
If the "ack" flag is set when the user profile is loaded, the warning is displayed once, and all the subshells started by the shell that read .bashrc will inherit the ack flag.
If export is removed, I think, the warning will be displayed multiple times - by every new bash instance.
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.
|
Thank you for a quick review. |
6 participants
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.