Skip to content

command duration fix#2366

Open
BarbUk wants to merge 18 commits intoBash-it:masterfrom
BarbUk:command_duration_fix
Open

command duration fix#2366
BarbUk wants to merge 18 commits intoBash-it:masterfrom
BarbUk:command_duration_fix

Conversation

@BarbUk
Copy link
Contributor

@BarbUk BarbUk commented Feb 3, 2026

Fix command duration lib to display the actual time a command took

Description

  • Delete preexec vendor
  • Squashed 'vendor/github.com/rcaloras/bash-preexec/' content from commit b73ed5f7
  • Update precommit
  • Rework command duration plugin to handle locale with . or , and remove subshell
  • Sh format

Motivation and Context

Command duration lib was updated in #2271 to fix an issue with the locale defined in the command duration lib.
The locale was forced to ensure that the decimal point of EPOCHREALTIME is the period.

The fix introduce a subshell, the preexec vendor has an issue with subshells:
https://github.com/rcaloras/bash-preexec/?tab=readme-ov-file#subshells

This PR update preexec to the last stable tag (0.6.0), update the command duration lib to remove the subshell and handle locale with period or comma.

How Has This Been Tested?

Before:

~ ❯ sleep 1

~ 🕒 3.6s ❯ sleep 2

~ 🕒 3.8s ❯

After:

 ~ ❯ sleep 1

 ~ 🕐 1.0s ❯ sleep 2

 ~ 🕑 2.1s ❯

Screenshots (if appropriate):

Before: left, after: right

image

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • If my change requires a change to the documentation, I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • If I have added a new file, I also added it to clean_files.txt and formatted it using lint_clean_files.sh.
  • I have added tests to cover my changes, and all the new and existing tests pass.

…it b73ed5f7

git-subtree-dir: vendor/github.com/rcaloras/bash-preexec
git-subtree-split: b73ed5f7f953207b958f15b1773721dded697ac3
git-vendor-name: preexec
git-vendor-dir: vendor/github.com/rcaloras/bash-preexec
git-vendor-repository: https://github.com/rcaloras/bash-preexec
git-vendor-ref: 0.6.0
Technically, one can define a locale with decimal_point being an arbitrary string.
For example, ps_AF seems to use U+066B as the decimal point

Thanks @akinomyoga for the feedback
@akinomyoga
Copy link
Contributor

The fix introduce a subshell, the preexec vendor has an issue with subshells:
https://github.com/rcaloras/bash-preexec/?tab=readme-ov-file#subshells

The subshell mentioned in the bash-preexec README is unrelated to the present subshell for the shell function. bash-preexec may not reliably trigger the preexec hook before the execution of a user command starting with ( cmd... ). It has an option to try to fix it, but it causes other problems.

On the other hand, the code that this PR tries to fix is not used in a form (...). It is not a code executed inside a user command either. It is rather a command executed inside the preexec hook itself. In addition, even if any subshells (not only the ones called in the form (...)) are affected, this PR still uses a subshell through $(...).

I doubt bash-prexec is related. What is your original problem?

@BarbUk
Copy link
Contributor Author

BarbUk commented Feb 3, 2026

What is your original problem?

The shell duration lib was not counting the duration between the start of a command to the end of that command, but from start of the prompt to the end of the next command.

So the time was always wrong.

The subshell mentioned in the bash-preexec README is unrelated to the present subshell for the shell function. bash-preexec may not reliably trigger the preexec hook before the execution of a user command starting with ( cmd... ). It has an option to try to fix it, but it causes other problems.

I maybe made a mental shortcut about this.

But updating the preexec vendor and removing the sub-shell to get EPOCHREALTIME fixed the issue.

@BarbUk
Copy link
Contributor Author

BarbUk commented Feb 3, 2026

image

@akinomyoga
Copy link
Contributor

But updating the preexec vendor and removing the sub-shell to get EPOCHREALTIME fixed the issue.

So you applied two changes to fix the problem. Have you checked that both changes are actually needed to fix the problem? In particular, does changing { ... } of the function body of _command_duration_current_time() back to ( ... ) re-introduce the original problem?

@BarbUk
Copy link
Contributor Author

BarbUk commented Feb 3, 2026

So you applied two changes to fix the problem. Have you checked that both changes are actually needed to fix the problem? In particular, does changing { ... } of the function body of _command_duration_current_time() back to ( ... ) re-introduce the original problem?

Just made a test case.
This is definitely the vendors update that fix the issue.

The precedent version was 0.4.1.
Testing with the 0.5.0 release, I still have the issue.
The 0.6.0 fix the issue.

I see that you made several PR for this release (thank you).
Just testing the PR patch from the 0.5.0 version, this is your PR that fix the issue:
rcaloras/bash-preexec#141

   ~/.dotfiles/modules/bash-it on  detached:d4fa26e5  b ✓ ❯ git vendor update preexec 0.5.0
From https://github.com/rcaloras/bash-preexec
 * tag                 0.5.0      -> FETCH_HEAD
From https://github.com/rcaloras/bash-preexec
 * tag                 0.5.0      -> FETCH_HEAD
Subtree is already at commit da64ad4b7bb965d19dbeb5bb7447f1a63e3de2e3.

   ~/.dotfiles/modules/bash-it on  detached:d4fa26e5  b ✓ 🕑 2.3s ❯ cd vendor/github.com/rcaloras/bash-preexec/

   ~/.../github.com/rcaloras/bash-preexec on  detached:d4fa26e5  b ✓ ❯ patch -p1 < <(curl -s https://patch-diff.githubusercontent.com/raw/rcaloras/bash-preexec/pull/141.patch)

   ~/.../github.com/rcaloras/bash-preexec on  detached:d4fa26e5  b ✓  130 ❯ patch -p1 < <(curl -s https://patch-diff.githubusercontent.com/raw/rcaloras/bash-preexec/pull/128.patch)
patching file bash-preexec.sh
Hunk #1 succeeded at 309 (offset 2 lines).

   ~/.../github.com/rcaloras/bash-preexec on  detached:d4fa26e5 •1 ⌀1  b ✗ ❯ patch -p1 < <(curl -s https://patch-diff.githubusercontent.com/raw/rcaloras/bash-preexec/pull/127.patch)
patching file bash-preexec.sh

   ~/.../github.com/rcaloras/bash-preexec on  detached:d4fa26e5 •1 ⌀1  b ✗ 🕐 1.5s ❯ patch -p1 < <(curl -s https://patch-diff.githubusercontent.com/raw/rcaloras/bash-preexec/pull/136.patch)
patching file README.md
patching file bash-preexec.sh
Hunk #1 succeeded at 38 (offset -3 lines).
patching file test/bash-preexec.bats

   ~/.../github.com/rcaloras/bash-preexec on  detached:d4fa26e5 •3 ⌀1  b ✗ ❯ patch -p1 < <(curl -s https://patch-diff.githubusercontent.com/raw/rcaloras/bash-preexec/pull/141.patch)
patching file bash-preexec.sh
Hunk #1 succeeded at 176 (offset -7 lines).
Hunk #2 succeeded at 283 (offset -7 lines).
Hunk #3 succeeded at 321 (offset -10 lines).
Hunk #4 succeeded at 357 (offset -10 lines).
patching file test/bash-preexec.bats

My test case is working after the PR 141 patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants