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.

Sign up for GitHub

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

Honor HISTCONTROL "ignorespace" and "ignoreboth" #96

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Honor HISTCONTROL "ignorespace" and "ignoreboth" #96

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ curl https://raw.githubusercontent.com/rcaloras/bash-preexec/master/bash-preexec
echo '[[ -f ~/.bash-preexec.sh ]] && source ~/.bash-preexec.sh' >> ~/.bashrc
```

NOTE: this script may change your `HISTCONTROL` value by removing `ignorespace` and/or replacing `ignoreboth` with `ignoredups`. See [`HISTCONTROL` interaction](#histcontrol-interaction) for details.

## Usage
Two functions **preexec** and **precmd** can now be defined and they'll be automatically invoked by bash-preexec if they exist.

Expand Down Expand Up @@ -91,6 +93,10 @@ export __bp_enable_subshells="true"
```
This is disabled by default due to buggy situations related to to `functrace` and Bash's `DEBUG trap`. See [Issue #25](https://github.com/rcaloras/bash-preexec/issues/25)

## `HISTCONTROL` interaction

In order to be able to provide the last command text to the `preexec` hook, this script will remove `ignorespace` and/or will replace `ignoreboth` with `ignoredups` in your `HISTCONTROL` variable. It will remember if `HISTCONTROL` has been modified and will remove the last command from the history "manually", after reading the last command from the history list. This may cause issues when you have scripts that rely on the literal value of `HISTCONTROL` or manipulate history in their own ways.

## Library authors
If you want to detect bash-preexec in your library (for example, to add hooks to `preexec_functions` when available), use the Bash variable `bash_preexec_imported`:

Expand Down
46 changes: 36 additions & 10 deletions bash-preexec.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ __bp_imported="${bash_preexec_imported}"
__bp_last_ret_value="$?"
BP_PIPESTATUS=("${PIPESTATUS[@]}")
__bp_last_argument_prev_command="$_"
__bp_ignorespace=

__bp_inside_precmd=0
__bp_inside_preexec=0
Expand All @@ -85,18 +86,26 @@ __bp_require_not_readonly() {
done
}

# Remove ignorespace and or replace ignoreboth from HISTCONTROL
# so we can accurately invoke preexec with a command from our
# history even if it starts with a space.
# Remove "ignorespace" and/or replace "ignoreboth" in HISTCONTROL so we can
# accurately invoke preexec with a command from our history even if it starts
# with a space. We then remove commands that start with a space from the
# history "manually", if either "ignorespace" or "ignoreboth" was part of
# HISTCONTROL.
__bp_adjust_histcontrol() {
local histcontrol
histcontrol="${HISTCONTROL:-}"
histcontrol="${histcontrol//ignorespace}"
# Replace ignoreboth with ignoredups
if [[ "$histcontrol" == *"ignoreboth"* ]]; then
histcontrol="ignoredups:${histcontrol//ignoreboth}"
fi;
export HISTCONTROL="$histcontrol"
histcontrol=${HISTCONTROL:-}
histcontrol=":${histcontrol//:/::}:"

if [[ "$histcontrol" == *":ignorespace:"* || "$histcontrol" == *":ignoreboth:"* ]]; then
__bp_ignorespace=yes
fi

histcontrol=${histcontrol//:ignorespace:}
histcontrol=${histcontrol//:ignoreboth:/:ignoredups:}

histcontrol=${histcontrol//::/:}
histcontrol=${histcontrol#:}
export HISTCONTROL=${histcontrol%:}
}

# This variable describes whether we are currently in "interactive mode";
Expand Down Expand Up @@ -260,6 +269,23 @@ __bp_preexec_invoke_exec() {
return
fi

# If we have removed "ignorespace" or "ignoreboth" from HISTCONTROL
# 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can switch the implementation based on BASH_VERSINFO. Reducing a fork for Bash 5.0 is still nice.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@tessus tessus Feb 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#
# builtin history -d -1
#
# Negative indices for `-d` are not supported before 5.0, so we compute the
# length of the history list explicit, to delete the last entry.
if [[ -n "$__bp_ignorespace" && "$this_command" == " "* ]]; then
builtin history -d "$(
export LC_ALL=C
HISTTIMEFORMAT= history 1 | sed '1 s/^ *\([0-9][0-9]*\).*/\1/'
)"
fi

# Invoke every function defined in our function array.
local preexec_function
local preexec_function_ret_value
Expand Down
38 changes: 34 additions & 4 deletions test/bash-preexec.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,18 +333,17 @@ set_exit_code_and_run_precmd() {
# Should remove ignorespace
HISTCONTROL="ignorespace:ignoredups:*"
__bp_adjust_histcontrol
[ "$HISTCONTROL" == ":ignoredups:*" ]
[ "$HISTCONTROL" == "ignoredups:*" ]

# Should remove ignoreboth and replace it with ignoredups
HISTCONTROL="ignoreboth"
__bp_adjust_histcontrol
[ "$HISTCONTROL" == "ignoredups:" ]
[ "$HISTCONTROL" == "ignoredups" ]

# Handle a few inputs
HISTCONTROL="ignoreboth:ignorespace:some_thing_else"
__bp_adjust_histcontrol
echo "$HISTCONTROL"
[ "$HISTCONTROL" == "ignoredups:::some_thing_else" ]
[ "$HISTCONTROL" == "ignoredups:some_thing_else" ]

}

Expand All @@ -367,6 +366,7 @@ set_exit_code_and_run_precmd() {

run '__bp_preexec_invoke_exec'
[ $status -eq 0 ]
echo "__bp_preexec_invoke_exec: output: '$output'"
[ "$output" == " this command has whitespace " ]
}

Expand All @@ -389,3 +389,33 @@ a multiline string'" ]
[ $status -eq 0 ]
[ "$output" == '-n' ]
}

@test "HISTCONTROL is updated, but ignorespace functionality is honoured" {
preexec_functions+=(test_preexec_echo)
HISTCONTROL=ignorespace:ignoreboth

__bp_adjust_histcontrol

[[ "$HISTCONTROL" == "ignoredups" ]]

__bp_interactive_mode

command1="this command is in the history"

history -s "$command1"
run '__bp_preexec_invoke_exec'
[[ $status == 0 ]]
[[ "$output" == "$command1" ]]
last_history=$(HISTTIMEFORMAT= history 1 | sed '1 s/^ *[0-9][0-9]* *//')
[[ "$last_history" == "$command1" ]]

command2=" this should not be in the history"

history -s "$command2"
# we need to extract command history in the subshell, as the parent shell
# history is actually not affected.
output=$(__bp_preexec_invoke_exec && \
printf "last_history: %s\n" "$(HISTTIMEFORMAT= history 1 | sed '1 s/^ *[0-9][0-9]* *//')" )
[[ $status == 0 ]]
[[ "$output" == "$command2"$'\n'"last_history: $command1" ]]
}
Loading