Allow deciding if a command should be saved to history#10302
Allow deciding if a command should be saved to history#10302faho merged 4 commits intofish-shell:masterfrom
Conversation
src/reader.rs
Outdated
| if !eval_res.no_status { | ||
| STATUS_COUNT.fetch_add(1, Ordering::Relaxed); | ||
| } | ||
| data.add_to_history(command.clone(), parser); |
There was a problem hiding this comment.
We need to think about whether the behavior change is worth it.
Logging the command before running it seems like a safe default behavior,
which makes a difference in some cases like exec you mention.
If I start a long-running command, open a new terminal and press Up-arrow
before the first command is finished, what should the behavior be?
Maybe it doesn't matter maybe it does.
I think we don't have to decide now; we can err on the side of caution and merge this without support for querying
$status etc, especially since no one explicitly asked for that.
I think $status is not super relevant because most failed commands will be shadowed by their fixed versions.
Users who use a history pager might see things differently.. but only if they configure this behavior, so I wouldn't care too much.
Intuitively I'd tell users who want to use $status to remove those command from history in a postexec hook?
There was a problem hiding this comment.
We need to think about whether the behavior change is worth it.
Yup.
$status etc, especially since no one explicitly asked for that.
We have had people ask for it, e.g. #5 is relevant.
sponge features it.
If $status isn't available, there's the question what fish_should_add_history would in practice do. If it's just matching against a list or maybe regex we could also get away with a variable.
Would people check against, say, $PWD?
|
Would people check against, say, $PWD?
Yeah that sounds like something that should be possible
|
a45fb60 to
9c22672
Compare
If it returns 0, it will be saved, if it returns anything else, it will be ephemeral. It gets the right-trimmed text as the argument. If it doesn't exist, we do the historical behavior of checking for a leading space. That means you can now turn that off by defining a `fish_should_add_history` that just doesn't check it.
|
Alright, considering that this was a change (tho I can't find where it was changed) and specifically asked for in #4754, I don't believe we want to move history back to after the command. So checking $status is gone. If this is needed, we could add a "defer" mode, where you return e.g. 2 and then we run a second function after the command to decide? Tho that sounds pretty complicated. The alternative is to do what people do and delete it on postexec. |
There was a problem hiding this comment.
LGTM, thanks. How about calling it fish_should_add_to_history instead of the ambiguous fish_should_add_history?
|
Renamed to fish_should_add_to_history and merged. |
Description
This adds a function called
fish_should_add_history(EDIT: name changed tofish_should_add_to_historyafter review) that is run when fish decides whether to add a command to history. If it returns 0 the command is saved, if it returns anything else the command is ephemeral (and only available until the next command, like anything space-prefixed now).(EDIT: This was removed) One notable change needed here is that we now save the history after the command was run. This is necessary if we want to allow people to access e.g. $status.
It's visible in some cases, like
history clearor setting $fish_private_mode - nowset -g fish_private_mode 1is not stored, butset -eg fish_private_modeis, because the history setting is already changed by the time fish uses it.This will also affect commands that end the shell like
execor anything that crashes fish. I don't see any way out of that (adding and then removing later doesn't gel with not keeping history), so the alternative would be to not allow access to $status.Another decision is that this keeps the current setting - space-prefixed commands aren't saved - when no fish_should_add_history exists. It doesn't add a default, and once a function exists it is entirely responsible and would have to check for the space itself. This is in contrast to @ridiculousfish's comment in 5924. My rationale is that I want fewer default functions.
A rerun of #9298
Fixes issue #5924
TODOs: