[Bug] HISTCONTROL ignore* setting should be respected

[cornfeedhobo]

I understand the current reasoning for removing ignorespace and ignoreboth but I think they break such an important expectation for users that, if set, preexec should manually cleanup history, to guarantee this expectation is maintained.

[rcaloras]

@cornfeedhobo thanks for the feedback! This has been brought up a few times in different ways. Here's a few thoughts related that could help or shed light:

1. There's an open PR currently for what I believe you're suggesting. My take on this is that it inflates the job bash-preexec is doing to account for this edge case. In addition, actual histcontrol settings will be inconsistent with the result since bash-preexec will effectively be managing the 'ignorespace' usecase and overriding histcontrol.
2. If $1 isn't needed by preexec. You can delete bp_adjust_histcontrol invocation in the script and preserve original functionality. (See earlier question on this which is used by iTerm2 https://github.com/rcaloras/bash-preexec/issues/48)
3. Another option is for us to not manipulate histcontrol is to instead use $BASH_COMMAND in place of using history when a command is prefixed with a space. The issue with this is it creates other edge cases where it doesn't capture multiple commands on a single line (e.g. | or &&)

Appreciate any thoughts or other options.

[cornfeedhobo]

@rcaloras Thanks for the thoughtful reply! I had not seen iTerm's issue when searching. After playing around and thinking about this more, I've come to the conclusion that preexec should do this heavy lifting. I realize that this will make histcontrol inconsistent, but given the fact that this library already requires total control over DEBUG and PROMPT_COMMAND, I think adding this inconsistency is safer than users realizing that commands have been saved to history that contain sensitive information. I've made a comment on the open PR to make the solution work in all versions of bash.

I do hope you reconsider this, but in the meantime I will go through our codebase and ask the team what they think about removing bp_adjust_histcontrol.

[davidpfarrell]

Greetings ! I'm a contributor to Bash-It and collaborate with @cornfeedhobo.

I've just been made aware of this issue thanks to @cornfeedhobo starting a bash-It discussion:

• Newest preexec version disables ignorespace and ignoreboth

Having spent a couple of hours this am researching/thinking on this, here's some unsolicited feedback :)

• If you're going to make a "There's an open PR" statement, its always worth taking a moment to track down the actual PR and link to it (or edit/add link later). always - I presume this is the one:
  • Honor HISTCONTROL "ignorespace" and "ignoreboth"
• It is a TERRIBLE idea to override HISTCONTROL without the user's consent.
• It was always a bad idea to try to use history to determine the command being executed.
• I would very much like to be pointed to a single, useful, meaningful pre-exec command that needs the full command-line being executed. i.e other than some kind of logging (which history already does), what use does a pre-exec command have for, say:

  cat my_file | grep data | sort
  

  Is it going to parse on pipes and determine something meaningful? I just don't buy it.
• Bash already provides something very close what's being done here: From https://www.gnu.org/software/bash/manual/html_node/Bash-Variables.html :

  BASH_COMMAND

  The command currently being executed or about to be executed, unless the shell is executing a command as the result of a trap, in which case it is the command executing at the time of the trap. If BASH_COMMAND is unset, it loses its special properties, even if it is subsequently reset.

  Examples of BASH_COMMAND in action:

  $ trap 'echo "cmd: ${BASH_COMMAND}"' DEBUG
  
  $ echo hello world > /dev/null
  cmd: echo hello world > /dev/null
  
  $ echo hello world | wc -c
  cmd: echo hello world
  cmd: wc -c
        12
  

So, my current feeling is that the way forward is:

• Remove usages of history

• Pass $BASH_COMMAND AS $1

  OR

  Let the scripts fetch $BASH_COMMAND themselves when needed

  UNLESS

  It turns out that $BASH_COMMAND get clobbered in the course of processing many/complicated pre-exec scripts

  THEN

  Capture the value in $PREEXEC_BASH_COMMAND (or somesuch) where it can be found when needed.

Thanks for reading - Hopefully some of this was helpful,

-DF

[edit] Just want to call out that I missed that @rcaloras did mention BASH_COMMAND (and its caveats) - Just to confirm though, I do feel its a better fit and bash-supported and should be the way forward.

[gaelicWizard]

I've come to understand over the last 24 hours (after I submitted an admittedly grumpy PR to just rip out the history manipulation) that the use case is largely to set the window title or similar with the actual command line entered by the user. Of course this can alsö be used for logging, or perhaps debugging. Aliases are alsö lost from $BASH_COMMAND, so the as-typed may appear substantially different. (E.g., alias halo='echo $BASH_COMMAND'; halo prints echo $BASH_COMMAND not halo.)

The open PR #96 fully solves the issue for Bash v5 and up by just removing the command from history after saving it; basically bash-preexec emulates the intended behavior. @cornfeedhobo added a comment that makes it compatible with downversion Bash, but the PR was never updated or really touched since then. I've submitted a new PR #119 which integrates the version compatibility. Result is that the user's intended behavior is preserved without compromising the use case requiring the full as-typed command line.

I alsö took the opportunity to add handling to the hook so that if HISTCONTROL is set again to include 'ignorespace' then it falls back to $BASH_COMMAND after all. I hope this addresses everyone's needs and can be committed.

Thanks, JP2

[gaelicWizard]

@rcaloras, this alsö partially addresses your concern that it's editing history even if the user adds ignorespace again after __bp_adjust_histcontrol. In that case, when I use $BASH_COMMAND, I alsö clear __bp_ignorespace to bypass history deletion. (It would alsö bypass as the command doesn't start with a space, but belt-and-suspenders.)

BUT, if the user were to examine $HISTCONTROL they would not see ignorespace. One option would be to add _bp_ignorespace to $HISTCONTROL so the user would see that the option is there, but Bash wouldn't act on it. I may add this after thinking about it a bit more.

[gaelicWizard]

@rcaloras, this alsö partially addresses your concern that it's editing history even if the user adds ignorespace again after __bp_adjust_histcontrol. In that case, when I use $BASH_COMMAND, I alsö clear __bp_ignorespace to bypass history deletion. (It would alsö bypass as the command doesn't start with a space, but belt-and-suspenders.)

BUT, if the user were to examine $HISTCONTROL they would not see ignorespace. One option would be to add _bp_ignorespace to $HISTCONTROL so the user would see that the option is there, but Bash wouldn't act on it. I may add this after thinking about it a bit more.

[gaelicWizard]

And I have, at just this moment, learned that @rcaloras appears to be behind BashHub.com which is a significant use case for preserving the command line as-user-typed.