bash shell integration resets HISTCONTROL

[tristan957]

I confirmed this by unsetting shell-integration and also trying ptyxis just to make sure my bashrc works the way I intend.

[tristan957]

Hmm:

ghostty/src/shell-integration/bash/bash-preexec.sh

Lines 91 to 100 in 3aad8ec

	__bp_adjust_histcontrol() {
	local histcontrol
	histcontrol="${HISTCONTROL:-}"
	histcontrol="${histcontrol//ignorespace}"
	# Replace ignoreboth with ignoredups
	if [[ "$histcontrol" == *"ignoreboth"* ]]; then
	histcontrol="ignoredups:${histcontrol//ignoreboth}"
	fi;
	export HISTCONTROL="$histcontrol"
	}

[tristan957]

cc @jparise

[jparise]

@tristan957 do you think your issue is the same as what's described in rcaloras/bash-preexec#115?

[tristan957]

Yep. Sounds like it

[tristan957]

I personally feel like this is a security issue. I use a leading space all the time to keep commands out of my history. I wish I knew how to solve my own bash issues like I can with libadwaita 😆

[jparise]

I haven't had time to look at this in detail beyond some initial research, but we might consider just applying one of the proposed upstream patches, like: rcaloras/bash-preexec#119

[tristan957]

No sense in rushing. Just thought I would bring up an affected use case.

I now see that PR is 3 years old. Whoops.

[tristan957]

I went ahead and checked the VSCode integrated terminal since it has bash shell integration, I think, and HISTCONTROL is not being changed. Makes me wonder what the ghostty bash shell integration is doing that requires this change over VSCode and VTE.

[jparise]

Kitty (which doesn't use bash-preexec) just prints a warning when HISTCONTROL contains ignoreboth or ignorespace:

https://github.com/kovidgoyal/kitty/blob/d31459b0926f2afddc317d76314e4afd0d07d473/shell-integration/bash/kitty.bash#L227-L232

[mitchellh]

Nice find @jparise. I'm fine with this approach in our shell integration as well.

[tristan957]

What do you actually lose out on with HISTCONTROL set to that? It isn't clear.

[jparise]

I think we have three options:

1. Document the current HISTCONTROL behavior and do nothing more. This requires no code changes but means that people who use HISTCONTROL=ignore{space|both} won't get the history behavior they expect.
2. Fork our copy of bash-preexec to emit a warning when ignore{space|both} is used (like kitty) instead of modifying HISTCONTROL. When HISTCONTROL=ignore{space|both}, PROMPT_COMMAND won't be run for commands starting with a space, and that means our per-command shell hooks also won't be executed for those commands. In practice, this means we won't communicate those commands back to ghostty and potentially other consequences.
3. Fork our copy of bash-preexec to manually remove commands from history, effectively re-implementing HISTCONTROL=ignore{space|both} (e.g. Honor HISTCONTROL "ignorespace" and "ignoreboth" rcaloras/bash-preexec#119). This is the most complex solution but should yield the expected user (and ghostty) behavior.

[tristan957]

I'm pro 3, but unable to put in the work for it.

[jparise]

I'm pro 3, but unable to put in the work for it.

I'm happy to do the work, but I'll wait a little bit to see if there are any other strong opinions. No matter what, it's an easy change to make and revert if it causes problems in practice.

[tristan957]

I'm pro 3, but unable to put in the work for it.

I'm happy to do the work, but I'll wait a little bit to see if there are any other strong opinions. No matter what, it's an easy change to make and revert if it causes problems in practice.

You're a complete saint. Could you explain the answer to: "What do you actually lose out on with HISTCONTROL set to ignorespace or ignoreboth? It isn't clear."

[jparise]

Could you explain the answer to: "What do you actually lose out on with HISTCONTROL set to ignorespace or ignoreboth? It isn't clear."

I previously wrote this:

When HISTCONTROL=ignore{space|both}, PROMPT_COMMAND won't be run for commands starting with a space, and that means our per-command shell hooks also won't be executed for those commands. In practice, this means we won't communicate those commands back to ghostty and potentially other consequences.

... which isn't quite correct now that I read more of the details. PROMPT_COMMAND will be run for commands starting with a space, but it isn't able to access the command (via history 1) and pass it to the preexec function. It doesn't look like we actually use that argument, so this might not be much of an issue at all. Our preexec function:

function __ghostty_preexec() {
    PS0="$_GHOSTTY_SAVE_PS0"
    PS1="$_GHOSTTY_SAVE_PS1"
    PS2="$_GHOSTTY_SAVE_PS2"
    builtin printf "\033]133;C;\007"
    _ghostty_executing=1
}

... so for us, I think we might be able to rip out this whole HISTCONTROL manipulation code and move on. I'll have to look at it a little more to be sure though.

[tristan957]

Bash usage just got a whole lot better. Only one more bash issue to go (for me)!