search

rcaloras / bash-preexec

⚡ preexec and precmd functions for Bash just like Zsh.
MIT License
862 stars 94 forks source link

Remove all history manipulation #117

Closed gaelicWizard closed 3 years ago

gaelicWizard commented 3 years ago

Remove mangling of HISTCONTROL and remove use of history as a mechanism for detecting the "current" command. Instead, use $BASH_COMMAND like Jesus intended.

dimo414 commented 3 years ago

Can you detail exactly what behavior differences users will observe with this change, and why you feel they are acceptable?

A notable regression with this change appears to be that pipelines will no longer be captured properly (e.g. if you run echo foo | grep bar then history will output the full pipeline, but BASH_COMMAND will only contain echo foo). Without digging through the commit history I can't recall if there are other similar limitations, but I expect you will need to resolve this regression if you want to land this PR.

rcaloras commented 3 years ago

@gaelicWizard thanks for the initiative with the PR and @dimo414 thanks for chiming in.

My sentiment on this change is the same as mentioned prior in: https://github.com/rcaloras/bash-preexec/issues/115#issuecomment-779395772 I don't believe the regression of not capturing multiple commands is an acceptable one and favor the trade off here with overriding history to get the command line as its typed.

With that said, @gaelicWizard if you don't require $1 on preexec, feel free to to remove the history manipulation as mentioned https://github.com/rcaloras/bash-preexec/issues/48#issuecomment-315352858.

If there's another way to get access to the command exactly as it's typed without history in Bash I'd love to update this!

gaelicWizard commented 3 years ago

Sorry, this PR is marked draft because I both haven't committed everything nor put in any explanation.

After reading other open issues and PRs, I'm going to actually try to just make the history manipulation conditional rather than remove it. It's now clear to me that it's necessary for a particular use case given constraints.

My motivation is twofold: (1) don't change things behind my back; (2) allow hook to run before each command, rather than before each command line.

dimo414 commented 3 years ago

allow hook to run before each command, rather than before each command line.

That's totally reasonable to want, but it's not how bash-preexec is implemented (by design). Are you sure you even want to use bash-preexec? Maybe you just want to implement your own DEBUG trap?

gaelicWizard commented 3 years ago

Lol, I can't use bash-preexec and a custom debug trap unless I write a wrapper with conditional behavior and...that's why we use bash-preexec: so we get the benefit of each other's improvements.

I'm not the first one to want to be able to use debug hooks along with bash-preexec.

See #52, from someone who uses bash-preexec and is asking for DEBUG traps to run for every command.

gaelicWizard commented 3 years ago

Alsö, #109 would run the preexec hook multiple times on purpose.

gaelicWizard commented 3 years ago

I'm closing this PR and will open a whole new one based on #96 , which does 98% of what I need. It requires Bash version 5 or higher, but @cornfeedhobo made a comment that makes it not need v5 so I'm just doing that.