Skip to content

Remove a waccess call when completing executables #9931

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

Merged
merged 1 commit into from
Aug 3, 2023

Conversation

faho
Copy link
Member

@faho faho commented Aug 3, 2023

When we run these completions, we have already run waccess with X_OK. We already know the file is executable.

There is no reason to check again.

Restores some of the speedup from the fast_waccess hack that was removed to fix #9699.

In my tests it's ~1.14x - if fish with c67d77f reverted takes 1x, this takes 1.2 times as long and fish right before this takes 1.37x as long. (vs 3.6.1 it's 1.22x reverted / 1.5x this / 1.7x master)

The absolute time difference will scale with the size of $PATH.

Note: This will conflict with any port of wildcard, like #9916. I would still like to merge this because it should be in 3.6.2 and offset some of the loss of #9699.

I'm aware that this adds another argument to an already argument-heavy function, but it's only called in one spot and I would leave that cleanup to after a port (a lazy "file information" object would be quite cool to have here, but might be awkward considering TOCTTOU issues, but at the very least the "stat_res" and "stat buf" arguments would just be some combined thing like an Option or Result).

TODOs:

  • [N/A] Changes to fish usage are reflected in user documentation/manpages.
  • [N/A] Tests have been added for regressions fixed
  • [N/A] User-visible changes noted in CHANGELOG.rst

@faho faho added this to the fish 3.6.2 milestone Aug 3, 2023
Copy link
Contributor

@henrikhorluck henrikhorluck left a comment

Choose a reason for hiding this comment

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

I'm fine with adding the parameter, that function will have way fewer parameters in Rust

We have already run waccess with X_OK. We already *know* the file is
executable.

There is no reason to check again.

Restores some of the speedup from the fast_waccess hack that was
removed to fix fish-shell#9699.
@faho faho force-pushed the file-get-desc-remove-access branch from 3839704 to 4cf2423 Compare August 3, 2023 16:34
@faho faho merged commit ee75b45 into fish-shell:master Aug 3, 2023
@faho faho deleted the file-get-desc-remove-access branch August 3, 2023 17:53
@zanchey
Copy link
Member

zanchey commented Aug 4, 2023

Note for me - if we're taking this for 3.6.2 we will need to take #9699 as well

henrikhorluck added a commit to henrikhorluck/fish-shell that referenced this pull request Sep 15, 2023
henrikhorluck added a commit to henrikhorluck/fish-shell that referenced this pull request Sep 15, 2023
krobelus pushed a commit that referenced this pull request Sep 15, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fish Running as Root Fails to Suggest All Executable Commands in PATH
3 participants