Use a function substitution in $PS0 for preexec in Bash >= 5.3

[akinomyoga]

This implements the approach to preexec using a function substitution in PS0 mentioned in #28 (comment). This should solve the issues #164 (subshells), #6 (function definitions), #158 (comments), #147 (custom HISTIGNORE/HISTCONTROL) in Bash 5.3 (which is now under the beta testing). It should be noted that this doesn't change the situation in Bash <= 5.2.

edit: The issue with HISTIGNORE and HISTCONTROL isn't solved by this PR.

[hpfr]

Bash 5.3 has been out for about six months, and sourcing bash-preexec from this PR works for me with atuin. @akinomyoga anything to add other than a rebase? @rcaloras would you be able to take a look?

[akinomyoga]

@akinomyoga anything to add other than a rebase?

No.

[hpfr]

On reflection, @rcaloras what do you think about bringing on @akinomyoga as a collaborator with merge permissions (assuming he is amenable to it, of course)? Based on his recent work in bash-preexec, atuin’s Bash integration, ble.sh, bash-completion, and oils (a promising Bash-compatible shell), he seems like an excellent candidate.

No affiliation, just an appreciative user!

[akinomyoga]

If @rcaloras is comfortable with it, I can be a collaborator, but I'm not sure if that actually changes the situation. It depends on the goal of this project and what kind of role and discretion in the project would be given to me as a collaborator. Even if I become a collaborator, @rcaloras makes the final decisions on the design and the direction of the project. If the project goal is to show the simplest implementation compromising stability and robustness (as guessed from past replies), any changes that increase the number of lines of code would never be adopted, even if it solves real problems.

[hpfr]

If the project goal is to show the simplest implementation compromising stability and robustness (as guessed from past replies), any changes that increase the number of lines of code would never be adopted, even if it solves real problems.

Ah, thanks for the context. Based on the tastes I’ve observed in your comments and commit messages here and elsewhere, if you were to maintain a fork, I’d certainly adopt it. I’m already using the version from this branch, but I don’t think there’s one branch integrating the various other patches you and others have submitted. It would be great if Ryan is open to collaborate, since forks have low visibility and have to build reputation mostly from scratch. But that’s how FOSS works sometimes!

[tessus]

@akinomyoga another option is to create a detached fork (fork and then ask github support to make it a standalone repo). While I am not sure I understand whether this project is dead or going in a different direction, what is clear is that this project's master branch does not work for a bunch of apps (one of which is atuin).

I also understand that you probably don't want to take this on as another one of your repos, in which case maybe you could create a matrix or explanation which of your PRs are the minimum requirement to make preexec work again. (master is not working with atuin or bash 5.3, but I am using ble anyway)

[rcaloras]

Hey all, happy to support a bit here as I'm currently on a sabbatical. Apologies as this project hasn't been actively maintained outside of @akinomyoga's support.

(master is not working with atuin or bash 5.3, but I am using ble anyway)

@tessus What's the current issue with Bash 5.3? Seems to be working locally for me. With Atuin, is the issue just support for bash-preexec's primary limitations? i.e. situations preexec is not currently executed

[rcaloras]

@akinomyoga would be interested to connect on collaborating further and granting you merge access too! You've been an amazing contributor and would love to avoid unnecessary forking so we can retain the repo/community.

[tessus]

@rcaloras I screwed up my test VM and unfortunately the snapsshot is bonked as well. I should have a backup somewhere. I came across this problem a while back (definitely after upgrading to bash 5.3) when I tested on a VM w/o ble and none of the commands were saved into atuin's history.

I have used a slightly modified bash-preexec.sh (available for 7 days) that has a few PRs incorporated. I can't recall which though. This version worked with atuin in the past.
The only things that changed since then were atuin and bash.

I'll restore the VM (as soon as find that backup) and run a few tests within the next few days. Maybe somebody else has some input until then.

[akinomyoga]

@tessus What's the current issue with Bash 5.3?

That is actually explained in the original explanation of this PR:

This should solve the issues #164 (subshells), #6 (function definitions), #158 (comments) in Bash 5.3

In particular, the issues with function definitions and comments are frequently reported to Atuin.

@rcaloras I'm curious if there is any chance that this PR is merged.

Also, I'd like to know the factors that determine which kind of PRs would be merged or rejected in this repository. Is it purely a matter of the free time you have? Or are there any additional constraints or a specific design goal that might affect the merging of PRs? Maybe there has't been clear design policies for this project, in which case I'd appreciate it if you could share your current feelings (which might later be changed after thorough considerations).

[akinomyoga]

@akinomyoga would be interested to connect on collaborating further and granting you merge access too! You've been an amazing contributor and would love to avoid unnecessary forking so we can retain the repo/community.

I'd be happy to join the project as a Collaborator (in the sense of the GitHub terminology for the privileges). In that case, I think I'll later ask several questions about the policy and how we work together for the maintenance.

[Copilot]

Pull request overview

This PR updates bash-preexec to use a PS0-based function substitution for preexec on Bash ≥ 5.3, avoiding reliance on the DEBUG trap for those versions and aiming to address several long-standing edge cases (e.g., subshells, function definitions, commented commands) specific to the DEBUG-trap approach.

Changes:

• Switch preexec installation to PS0 hook for Bash ≥ 5.3, retaining the DEBUG trap hook for older Bash versions.
• Refactor hook invocation to pass through “previous $? / $_” context and to compute a “last non-zero status” from hook functions.
• Add Bats tests for the new __bp_invoke_precmd_functions / __bp_invoke_preexec_functions semantics.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.

File	Description
bash-preexec.sh	Adds PS0-based preexec hook for Bash ≥ 5.3 and refactors hook invocation/return-status handling.
test/bash-preexec.bats	Updates DEBUG-trap preservation expectations for older Bash and adds new tests for hook transparency/return behavior.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[rcaloras]

@akinomyoga Isn't this variable kind of redundant now since the trap string is being set conditionally in the install? Right now it's a string variable containing a substring of its own name, so I think just writing __bp_install is simpler rather than keeping this around.

[akinomyoga]

I'm neutral on this.

• It is in general a good practice to keep a magic constant (i.e., an integer or a string that is used for identification) in a global constant, though whether "__bp_install" is considered such a string is subtle. Currently, $__bp_install_string is used in four places (two places in bash-preexec.sh and two places in tests). By the lexical context, if they were replaced by the value, it is not very clear that PROMPT_COMMAND+='__bp_install' and "${existing_prompt_command//__bp_install/:}" should be updated consistently. In the future, one might modify PROMPT_COMMAND and forget updating "${existing_prompt_command//__bp_install/:}" (and two additional places in the tests). They might be detected (I'm actually unsure about this because I haven't checked), but it is still non-trivial to rewrite those four places consistently.
• Another aspect I'd like to point out is backward compatibility related to #129. Since #129 hasn't been merged and has been pending, external libraries have had to rely on internal variables such as __bp_install_string. I actually use this variable in ble.sh for bash-preexec integration. If the variable is removed now, those integrations are broken. I can still update my code to work with the new version of bash-preexec, but that complicates the code for the version detection of bash-preexec, and also the integration with an old version of ble.sh in the market continues to be broken, until distributions' packages are replaced with the new version of ble.sh.

If you think it is still better to use the immediate values after considering those points, I'll update the PR. Or we might defer the discussion and the change in a new separate PR. @rcaloras What do you think?

[akinomyoga]

@rcaloras After the AI showed a concern about $_ handling (Note: the issue isn't introduced by this PR but exists in the master branch too), I decided to use __bp_install "$_" for the value (14ff607).

[akinomyoga]

The Bats failures reported in 3.2, 4.4, and 5.0 are unrelated to this PR. These seem to be broken tests introduced by #170. Older Bash versions do not support the lowercase debug apparently.

@jparise #177 started to test the behavior for different versions of Bash (3.2, 4.4, 5.0, and 5.3). Could you check these Bats failures?

• https://github.com/rcaloras/bash-preexec/actions/runs/22785836720/job/66102320298?pr=166
• https://github.com/rcaloras/bash-preexec/actions/runs/22785836720/job/66102320286?pr=166
• https://github.com/rcaloras/bash-preexec/actions/runs/22785836720/job/66102320297?pr=166

edit: I'm sorry. Actually, the lowercase debug seems to be unrelated to the present issue. The error message happens even with the uppercase DEBUG. I'll investigate it.

[akinomyoga]

I rebased the commits on top of the latest master branch and squashed fixup commits.

[akinomyoga]

The Bats failures reported in 3.2, 4.4, and 5.0 are unrelated to this PR. These seem to be broken tests introduced by #170. Older Bash versions do not support the lowercase debug apparently.

@jparise #177 started to test the behavior for different versions of Bash (3.2, 4.4, 5.0, and 5.3). Could you check these Bats failures?

Sorry, it turned out that Bats identifies the wrong test case for failure. The test cases that actually failed were

• [[ $(declare -f __bp_original_debug_trap) == *$'\n'" ..."$'\n'* ]] || return 1, and
• [ "$(cut -d' ' -f3-7 <<< "$original_debug_trap")" == "'foo && echo '\''hello'\'' >/dev/null'" ]

but Bats reported

• trap "foo && echo 'hello' >/dev/null" debug

to be failing. That is confusing.

I fixed the tests in 9c7dca2..cb7bd5d.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

In the Bash >= 5.3 (PS0-based) path there is no DEBUG trap invocation before __bp_precmd_invoke_cmd, so __bp_last_argument_prev_command is never refreshed to the last argument of the just-executed user command. As a result, the $_ value passed into __bp_invoke_precmd_functions (and later used for preexec) can lag by one command. Capture the current $_ at the very start of __bp_precmd_invoke_cmd (without disturbing the captured $?/PIPESTATUS) and store it into __bp_last_argument_prev_command before using it here.

[akinomyoga]

I checked the code, but even the current master doesn't seem to process this properly.

• Inside preexec functions, $_ is the exit status of the previous command with a plain Bash plus bash-preexec.sh. This is because the last argument was not properly set inside __bp_preexec_invoke_exec
• Inside precmd functions, $_ is always precmd with a plain Bash plus bash-preexec.sh. This is because the DEBUG trap is called for the previous precmd registered to precmd_functions, and the DEBUG trap overwrites __bp_last_argument_prev_command.

I decided to fix the handling of the last argument altogether: abd0737

Actually, this is a change independent of the primary purpose of this PR, so please tell me if we should separate the PR.

[akinomyoga]

I have updated the tests abd0737..14ff607.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[akinomyoga]

@rcaloras I requested your review in the GitHub interface, but there doesn't seem to be an interface to re-request it until there is one, so let me manually mention you for the request of a review.

I've processed most of the AI comments that make sense. I try to convince the AI in the review-revision loop, but AI seems to have started to raise different issues unrelated to this PR (some of them are the issues discussed in other PRs), so I stopped the looping.

• For the AI comment (comment), I added a new commit 21a26bf, but this is slightly off-topic in this PR. I can drop the commit and submit it in a separate PR if necessary.
• For your comment (comment), __bp_install_string='__bp_install' will again be update to __install_string='__bp_install "$_"' in commit 21a26bf, so I think we can keep the variable.

[rcaloras]

Tested locally for myself on 5.4 and 3.2. Working on my system.

With this change, I think the script is definitely getting more complicated/complex e.g. now that we're supporting variants across bash versions and also fixing some of the bash variable support which was broken. Would be interested to consider refactors or other clean ups to simplify.

Thank you @akinomyoga!

[rcaloras]

@akinomyoga I've also given you collaborator access as well. Feel free to merge this and would recommend also tagging a version release of 0.7.0 with it.

[akinomyoga]

Thank you! I joined the project.

Tested locally for myself on 5.4 and 3.2. Working on my system.

With this change, I think the script is definitely getting more complicated/complex e.g. now that we're supporting variants across bash versions and also fixing some of the bash variable support which was broken.

Thank you for testing the branch too!

Would be interested to consider refactors or other clean ups to simplify.

The first three commits of this PR have been some refactorings, but I agree that we may consider the global refactoring. In particular, I think we can consider reordering the functions. Currently, after incremental changes in the change history, functions of different purposes are interleaved. I can later review the code again.

I'll later check the PR again and merge it.

[akinomyoga]

I'm merging this PR. I keep the respective commits because they contain multiple independent changes (partly because of the AI suggestions), and because I paid attention to keep additional changes properly squashed into the appropriate commits.

[akinomyoga]

Before tagging a new release, I want to see if #143 can be merged, since it seems that these days we are receiving several issues and PRs that would ultimately be solved by #143. Smaller patches have been submitted by PRs #154 and #180, but they will finally be replaced by the global re-implementation by #143, so I hesitate to apply the temporary changes #154 and #180.

[hpfr]

🎉 no fork! Thank you both!