Revert "fix(bash): avoid unexpected atuin history start for keybindings"
#1727
Conversation
|
I submitted a proper fix to rcaloras/bash-preexec#152. |
|
Great! Does that fix mean we don't need this patch at all? I'm probably going to need to release v18.0.2 shortly |
Yes, after the PR is merged, we shouldn't need this revert patch. As explained in rcaloras/bash-preexec#152, the problem is that preexec is called for the keybinding and passes the previous command. This behavior of bash-preexec caused overwriting the entry of the previous command and produced 0s duration. #1509 originally tried to fix the problem at Atuin's side (as at that time, I haven't thought about the possibility that it is possible to fix it at bash-preexec's side). Now that we reverted #1509, I guess we start to have the 0s-duration issue again. Let me look at the behavior more closely. Maybe I'll submit a rework of #1509. |
I'm afraid we will still need it, as we have no way of ensuring existing users upgrade.
In my testing so far, without this revert, we don't really record history at all. I'd rather have the 0-s duration than not record history |
Let me check the behavior. As far as I look at the change in #1509, I don't quite get why removing this part would fix the problem. |
|
As far as I test the latest In the first place, since bash-preexec doesn't pass the command to Atuin at all, there doesn't seem to be a way for Atuin to get the command. If the command is added to Atuin's history with Atuin of the current |
|
Ah, OK. I think you started and canceled the Atuin history search at every line in the testing. In that case, the command is added to Atuin's history before the Atuin history search is started. In that use pattern, the problem doesn't arise indeed. However, in the use case where the Atuin history search is called now and then, the commands are still randomly added or missing. |
|
Hmmm I'm sorry, I think you're right. Perhaps this revert was a little hasty 🙈 The behaviour that does seem consistent, and not random, is that
Try running a bunch in a series between accepting/cancelling a search and you'll see that the first isn't recorded depending on whether you've accepted or cancelled Edit: this happens before/after the revert, so I'll un-revert it. |
|
I'm now preparing a rework of #1509. I'll submit it soon. Could you take a look at the reworked version after I submit it? |
Of course! I've undone the revert as it was a red-herring. I've also just checked v17.2.1, and the behaviour I've described in my previous comments occurs there too |
In GitHub atuinsh#1509, we blocked the unintended preexec caused by the keybinding of Atuin. However, with that fix, the preexec event for the intended user command is still missing. In this patch, we try to manually run the preexec hook when we detected the unintended preexec (which means the missing intended preexec). References: atuinsh#1509 atuinsh#1727 atuinsh#1728
In GitHub atuinsh#1509, we blocked the unintended preexec caused by the keybinding of Atuin. However, with that fix, the preexec event for the intended user command is still missing. In this patch, we try to manually run the preexec hook when we detected the unintended preexec (which means the missing intended preexec). References: atuinsh#1509 atuinsh#1727 atuinsh#1728
In GitHub atuinsh#1509, we blocked the unintended preexec caused by the keybinding of Atuin. However, with that fix, the preexec event for the intended user command is still missing. In this patch, we try to manually run the preexec hook when we detected the unintended preexec (which means the missing intended preexec). References: atuinsh#1509 https://forum.atuin.sh/t/atuin-bash-and-ble-sh/175 atuinsh#1003 (comment) atuinsh#1727 atuinsh#1728
In GitHub atuinsh#1509, we blocked the unintended preexec caused by the keybinding of Atuin. However, with that fix, the preexec event for the intended user command is still missing. In this patch, we try to manually run the preexec hook when we detected the unintended preexec (which means the missing intended preexec). References: atuinsh#1509 https://forum.atuin.sh/t/atuin-bash-and-ble-sh/175 atuinsh#1003 (comment) atuinsh#1727 atuinsh#1728
In GitHub atuinsh#1509, we blocked the unintended preexec event caused by the keybinding of Atuin. However, with that fix, the preexec event for the intended user command is still missing. In this patch, we try to manually run the preexec hook when we detected the unintended preexec (which means the missing intended preexec). References: atuinsh#1509 https://forum.atuin.sh/t/atuin-bash-and-ble-sh/175 atuinsh#1003 (comment) atuinsh#1727 atuinsh#1728
In GitHub #1509, we blocked the unintended preexec event caused by the keybinding of Atuin. However, with that fix, the preexec event for the intended user command is still missing. In this patch, we try to manually run the preexec hook when we detected the unintended preexec (which means the missing intended preexec). References: #1509 https://forum.atuin.sh/t/atuin-bash-and-ble-sh/175 #1003 (comment) #1727 #1728
In GitHub #1509, we blocked the unintended preexec event caused by the keybinding of Atuin. However, with that fix, the preexec event for the intended user command is still missing. In this patch, we try to manually run the preexec hook when we detected the unintended preexec (which means the missing intended preexec). References: #1509 https://forum.atuin.sh/t/atuin-bash-and-ble-sh/175 #1003 (comment) #1727 #1728
Reverts #1509
cc @akinomyoga
Looks like this is causing some of the "bash not recording commands" issues, from what I've managed to replicate. I'd rather take inaccurate timings vs missed commands :/