fix: use builtin sed

[konosuke-sakai]

This commit fixes alias conflicts by explicitly using \sed. This ensures compatibility in environments where sed is aliased to a non-compatible tool, such as alias sed=sd (chmln/sd).

Fixes #4990

[jparise]

I'll give this a deeper look shortly, but I also want to be cognizant of diverging from the upstream project.

https://github.com/rcaloras/bash-preexec

[jcollie]

I'll give this a deeper look shortly, but I also want to be cognizant of diverging from the upstream project.

https://github.com/rcaloras/bash-preexec

Seems as if we already diverge from upstream a bit. Perhaps we should upstream this change and then use build.zig.zon to pull in the file rather than vendoring it.

--- /home/jeff/tmp/bash-preexec/bash-preexec.sh 2025-01-12 11:50:03.904439951 -0600
+++ src/shell-integration/bash/bash-preexec.sh  2024-10-27 12:44:20.553235037 -0500
@@ -310,7 +310,13 @@
     fi

     # Adjust our HISTCONTROL Variable if needed.
-    __bp_adjust_histcontrol
+    #
+    # GHOSTTY: Don't modify HISTCONTROL. This hack is only needed to improve the
+    # accuracy of the command argument passed to the preexec functions, and we
+    # don't use that argument in our bash shell integration script (and nor does
+    # the __bp_original_debug_trap function above, which is the only other active
+    # preexec function).
+    #__bp_adjust_histcontrol

     # Issue #25. Setting debug trap for subshells causes sessions to exit for
     # backgrounded subshell commands (e.g. (pwd)& ). Believe this is a bug in Bash.

[mitchellh]

I'll give this a deeper look shortly, but I also want to be cognizant of diverging from the upstream project.

https://github.com/rcaloras/bash-preexec

If it's actively maintained (looks like it), I think we should upstream these changes and try to remain mainlined.

[jparise]

A few thoughts:

• Is it common to alias sed to sd? That seems risky given that sd doesn't provide a sed-compatible command line interface (in a "you're knowingly voiding the warranty" kind of way).
• This proposed change seems fine to me, although it does flip the assumption that someone who intentionally aliased sed to a sed-compatible tool won't get that benefit here. I doubt that matters in practice though.
• I'd ultimately prefer to drop the dependency on sed entirely, which we should be able to do with bash's shell parameter expansion syntax, although I'd need to look more closely to see if what's needed would be supported by the full range of bash versions that bash-preexec targets.

For example, we previously used something like this in our own shell integration script to avoid calling sed for this use case:

cmd=$(builtin history 1)
cmd="${cmd#*[[:digit:]]*[[:space:]]}" # remove leading history number
cmd="${cmd#"${cmd%%[![:space:]]*}"}"  # remove remaining leading whitespace

I think that would make for a more attractive upstream change (assuming it's version-compatible).

But for the purposes of this change, I'm good with integrating it in the short term if we agree this is an issue that needs addressing while we look at the long-term options.

[mitchellh]

This proposed change seems fine to me, although it does flip the assumption that someone who intentionally aliased sed to a sed-compatible tool won't get that benefit here. I doubt that matters in practice though.

I think in general for scripts like this, we want to use the tool we expect and DON'T want user aliases coming through. As long as we can get to a point where we're mostly doing POSIX-standard things then I think that's especially true as we expect POSIX behavior.

[jparise]

I'll give this a deeper look shortly, but I also want to be cognizant of diverging from the upstream project.
rcaloras/bash-preexec

Seems as if we already diverge from upstream a bit. Perhaps we should upstream this change and then use build.zig.zon to pull in the file rather than vendoring it.

--- /home/jeff/tmp/bash-preexec/bash-preexec.sh 2025-01-12 11:50:03.904439951 -0600
+++ src/shell-integration/bash/bash-preexec.sh  2024-10-27 12:44:20.553235037 -0500
@@ -310,7 +310,13 @@
     fi

     # Adjust our HISTCONTROL Variable if needed.
-    __bp_adjust_histcontrol
+    #
+    # GHOSTTY: Don't modify HISTCONTROL. This hack is only needed to improve the
+    # accuracy of the command argument passed to the preexec functions, and we
+    # don't use that argument in our bash shell integration script (and nor does
+    # the __bp_original_debug_trap function above, which is the only other active
+    # preexec function).
+    #__bp_adjust_histcontrol

     # Issue #25. Setting debug trap for subshells causes sessions to exit for
     # backgrounded subshell commands (e.g. (pwd)& ). Believe this is a bug in Bash.

I think that would work if we make the __bp_adjust_histcontrol behavior runtime-configurable and the bash-preexec maintainers were cool with that approach.

[jparise]

This proposed change seems fine to me, although it does flip the assumption that someone who intentionally aliased sed to a sed-compatible tool won't get that benefit here. I doubt that matters in practice though.

I think in general for scripts like this, we want to use the tool we expect and DON'T want user aliases coming through. As long as we can get to a point where we're mostly doing POSIX-standard things then I think that's especially true as we expect POSIX behavior.

Yes, I agree with that (for the same reason we want to use builtin ... consistently in our bash scripts).

My point was more about people who were expecting it to work that way before (in a compatible way), but I can't think of why that would be important in practice.

[jparise]

I'd ultimately prefer to drop the dependency on sed entirely, which we should be able to do with bash's shell parameter expansion syntax, although I'd need to look more closely to see if what's needed would be supported by the full range of bash versions that bash-preexec targets.

Here's an upstream change that aims to remove one instance of sed: rcaloras/bash-preexec#167

[jparise]

I have two pending upstream changes that should also address this issue:

• Remove dependency on sed(1) for history processing rcaloras/bash-preexec#167
• Use command to execute sed rcaloras/bash-preexec#169 (option 1)
• Improve prior_trap processing rcaloras/bash-preexec#170 (option 2)

Let's wait a few on this PR. If those get merged, we can pull them down instead.

[jparise]

While those upstream pull requests are under consideration (which might take some time), I've moved forward with our own copies of those changes:

• bash: remove sed dependency for history processing #5141
• bash: improve prior_trap processing #5142

[jparise]

I think we can close this now that #5141 and #5142 have landed.

[mitchellh]

Was going to ask this. Agree