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.

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

replace perl with bash in the bash history widget #3310

Closed

Conversation

step-
Copy link
Contributor

@step- step- commented May 25, 2023

While awk is POSIX, perl isn't pre-installed on all *nix flavors. This commit replaces perl with a bash script to eliminate dependency on perl.

This commit passed the current test suite with zero errors :

  • make error => all test sections 'ok'
  • make docker-test => Finished in 52.150847s, 4.1227 runs/s, 36.0684 assertions/s. 215 runs, 1881 assertions, 0 failures, 0 errors, 0 skips

See also: #3295 (same idea replacing awk for perl).

I manually tested this commit on macOS Catalina with bash 3.2, and Linux with bash 3.2 and 4.4. While testing on Linux I reported issue #3309.

Note: This commit needs bash support for associative arrays, available since bash 4, to filter out duplicate history items. On macOS, where 3.2 is the default bash version, fzf will not de-duplicate items unless a newer bash version is installed.

While awk is POSIX, perl isn't pre-installed on all *nix flavors. This
commit replaces perl with a bash script to eliminate dependency on perl.

This commit passed the current test suite with zero errors :
* `make error` => all test sections 'ok'
* `make docker-test` =>
  Finished in 52.150847s, 4.1227 runs/s, 36.0684 assertions/s.
  215 runs, 1881 assertions, 0 failures, 0 errors, 0 skips

See also:  junegunn#3295 (same idea replacing awk for perl).

I manually tested this commit on macOS Catalina with bash 3.2, and Linux
with bash 3.2 and 4.4. While testing on Linux I reported issue junegunn#3309.

Note: This commit needs bash support for associative arrays, available
since bash 4, to filter out duplicate history items. On macOS, where
3.2 is the default bash version, fzf will not de-duplicate items unless
a newer bash version is installed.
@step-
Copy link
Contributor Author

step- commented May 25, 2023

I have just played the failing ruby test manually but can't get an error. The test script sends

echo 1st<enter>
echo 2nd<enter>
echo 3d<enter>
echo 3rd<enter>
echo 3rd<enter>
echo 3rd<enter>
echo 4th<enter>
<c-r>
e3d
<c-r>
<enter>
<enter>

and expects fzf to match exactly two lines with " echo 3d" selected.
fzf-20230525

Then, after the second <c-r> with " echo 3rd" selected.
fzf-20230525-001

Then, after the next <enter> with "echo 3rd" in the input buffer. Finally, after the last <enter> with "3rd" in the output buffer.
fzf-20230525-002

What's failing? Perhaps it's a white space issue? I noticed that the test expects a single space character before the selected line. My code lines 69 and 77 delete a space that builtin fc introduces. Maybe that?

What baffles me is that I have also run make docker-test again here and it doesn't report an error:

Run options: --seed 64802

# Running:

.......................................................................................................................................................................................................................

Finished in 52.049617s, 4.1307 runs/s, 36.1578 assertions/s.

215 runs, 1882 assertions, 0 failures, 0 errors, 0 skips

@step-
Copy link
Contributor Author

step- commented May 25, 2023

The integration test action is still failing on the same ruby test. I noticed another difference in the test setup: the action runs ruby tests with LC_ALL=C, unlike the DockerFile.

@junegunn
Copy link
Owner

junegunn commented May 26, 2023

Thanks for working on this. But I don't think this is going to make it. I liked your previous awk version because the required code change was minimal, but that is clearly not true here; I don't want to maintain these extra code lines just for a fairly small number of users.

The real deal breaker is the performance. I have 170K+ entries in my command history, and with the original Perl version, the processing is instantaneous, but with this version, it takes longer than a minute to fully process the list, which is unacceptable.

@junegunn junegunn closed this May 26, 2023
@step-
Copy link
Contributor Author

step- commented May 26, 2023

Thanks for working on this. But I don't think this is going to make it.
The real deal breaker is the performance.

I would do the same in your place.

I liked your previous awk version because the required code change was minimal.

I liked it too better than this version but the deal breaker with macOS awk is that I know no way to ascertain the last character read in streaming mode. If I had that in my pocket the awk script could work in macOS too with minimal changes. I will keep thinking of a way to do it with awk, which is a favorite tool of mine.

I have 170K+ entries in my command history.

Wow. Good to know to raise to the challenge!

step- added a commit to step-/fzf that referenced this pull request May 30, 2023
While awk is POSIX, perl isn't pre-installed on all *nix flavors. This
commit replaces perl with awk to eliminate the dependency on perl.

Related: junegunn#3295, junegunn#3309, junegunn#3310.

Test suite passed:
* `make error` all test sections 'PASS'
* `make docker-test` 215 runs, 1884 assertions, 0 failures, 0 errors, 0 skips.

Manually tested in the following environments:
* Linux amd64 with bash 3.2, 4.4, 5.2; gawk -P, one true awk, mawk, busybox awk.
* macOS Catalina, bash 3.2, macOS awk 20070501.

**Performance comparison:**

Mawk turned out the fastest, then perl.
One true awk's implementation should be the closest to macOS awk.
Test data: 230 KB history, 15102 entries, including multi-line and duplicates.
Linux, bash 4.4. Times in milliseconds.

| Command                 | Mean | Min  | Max  | Relative |
| :---                    | ---: | ---: | ---: | -------: |
| `mawk 1.3.4`            | 22.9 | 22.3 | 25.6 | **1.00** |
| `perl 5.26.1`           | 34.3 | 33.6 | 35.1 |   1.49   |
| `one true awk 20221215` | 41.9 | 40.6 | 46.3 |   1.83   |
| `gawk 5.1.0`            | 46.1 | 44.4 | 50.3 |   2.01   |
| `busybox awk 1.27.0`    | 64.8 | 63.2 | 70.0 |   2.82   |

**Other Notes**

A bug affects bash, which fails restoring a saved multi-line history entry as a single entry. Bug fixed in version 5.0.[^1]

While developing this PR I discovered two unsubmitted issues affecting the current perl script. The output stream ends with `$'\n\0000'` instead of `$'\0000'`. Because of this, the script does not deduplicate a duplicated entry located at the end of the history list; therefore fzf displays two identical (not necessarily adjacent) entries. A minor point about the first issue is that the top fzf entry ends with a dangling line feed symbol, which is visible in the terminal.

[^1]: https://github.com/bminor/bash/blob/ec8113b9861375e4e17b3307372569d429dec814/CHANGES#L1511
  To enable: `shopt -s cmdhist lithist; HISTTIMEFORMAT='%F %T '`.
junegunn pushed a commit that referenced this pull request Sep 22, 2023
While awk is POSIX, perl isn't pre-installed on all *nix flavors.
This commit eliminates the mandatory dependency on perl by using awk
when perl is not available.

Related: #3295, #3309, #3310.

Test suite passed:
* `make error` all test sections 'PASS'
* `make docker-test` 215 runs, 1884 assertions, 0 failures, 0 errors, 0 skips.

Manually tested in the following environments:
* Linux amd64 with bash 3.2, 4.4, 5.2; gawk -P, one true awk, mawk, busybox awk.
* macOS Catalina, bash 3.2, macOS awk 20070501.

**Performance comparison:**

Mawk turned out the fastest, then perl.
One true awk's implementation should be the closest to macOS awk.
Test data: 230 KB history, 15102 entries, including multi-line and duplicates.
Linux, bash 4.4. Times in milliseconds.

| Command                 | Mean | Min  | Max  | Relative |
| :---                    | ---: | ---: | ---: | -------: |
| `mawk 1.3.4`            | 22.9 | 22.3 | 25.6 | **1.00** |
| `perl 5.26.1`           | 34.3 | 33.6 | 35.1 |   1.49   |
| `one true awk 20221215` | 41.9 | 40.6 | 46.3 |   1.83   |
| `gawk 5.1.0`            | 46.1 | 44.4 | 50.3 |   2.01   |
| `busybox awk 1.27.0`    | 64.8 | 63.2 | 70.0 |   2.82   |

**Other Notes**

A bug affects bash, which fails restoring a saved multi-line history entry as a single entry. Bug fixed in version 5.0.[^1]

While developing this PR I discovered two unsubmitted issues affecting the current perl script. The output stream ends with `$'\n\0000'` instead of `$'\0000'`. Because of this, the script does not deduplicate a duplicated entry located at the end of the history list; therefore fzf displays two identical (not necessarily adjacent) entries. A minor point about the first issue is that the top fzf entry ends with a dangling line feed symbol, which is visible in the terminal.

[^1]: https://github.com/bminor/bash/blob/ec8113b9861375e4e17b3307372569d429dec814/CHANGES#L1511
  To enable: `shopt -s cmdhist lithist; HISTTIMEFORMAT='%F %T '`.
@step-
Copy link
Contributor Author

step- commented Sep 28, 2024

@glensc, TLDR; #3313 (comment)

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