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

Skip pre/post exec events for empty commands (#4829) #7085

Closed
wants to merge 1 commit into from
Closed

Skip pre/post exec events for empty commands (#4829) #7085

wants to merge 1 commit into from

Conversation

awalgarg
Copy link
Contributor

@awalgarg awalgarg commented Jun 6, 2020

Description

In the same vein as #4926, firing fish_preexec and fish_postexec for empty commands is inconsistent with the way $status, history and $CMD_DURATION treat empty commands. fish_postexec, for example, receives an incorrect value for $CMD_DURATION when an empty command is processed.

As suggested in #4829, this patch skips firing of these events for empty commands altogether.

Fixes issue #4829

TODOs:

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

@faho
Copy link
Member

faho commented Jun 6, 2020

So what are we doing for empty commands? Should we possibly just not be doing anything instead?

@faho faho added this to the fish 3.2.0 milestone Jun 6, 2020
@awalgarg
Copy link
Contributor Author

awalgarg commented Jun 6, 2020

I do think simply ignoring empty commands altogether should be fine, but felt it might be too big a change. Empty commands are often used to "separate" a bunch of commands from the rest for readability, or for some reason to refresh a prompt with a time indicator etc.

This patch simply follows the precedent set from CMD_DURATION to fix the issue with postexec hooks receiving an incorrect duration value. If we are okay with ignoring empty commands altogether, I can just do that then.

@ridiculousfish
Copy link
Member

I agree with faho, let's merge this, and then try skipping even more work for empty commands.

Merged as cb5eb72
Extended as 61e9484

Thank you!

@awalgarg
Copy link
Contributor Author

awalgarg commented Jun 7, 2020

Thanks, @ridiculousfish!

awalgarg added a commit to awalgarg/dotfiles that referenced this pull request Jul 29, 2020
The weird arg length check is due to bug in fish. Sent a patch to fix
it, will be released in fish 3.2.0.

Ref: fish-shell/fish-shell#7085
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 5, 2020
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.

None yet

3 participants