Skip to content

Comments

Use command to execute sed#169

Closed
jparise wants to merge 1 commit intorcaloras:masterfrom
jparise:sed-command
Closed

Use command to execute sed#169
jparise wants to merge 1 commit intorcaloras:masterfrom
jparise:sed-command

Conversation

@jparise
Copy link
Contributor

@jparise jparise commented Jan 13, 2025

We want our execution environment to be a predictable as possible. Similar to how we use builtin to ensure we're running the shell's builtin function (and not a redefinition), use command to ensure we're running sed and not a shell function or alias.

@jparise
Copy link
Contributor Author

jparise commented Jan 13, 2025

Some other notes:

  • We could also use the \sed syntax if that's preferred.
  • It would be nice to remove this sed call entirely, but it looks like it might be difficult to re-implement this in "native" bash.

The motivation for this change is to avoid conflicts with (incompatible) sed aliases: ghostty-org/ghostty#4991

We want our execution environment to be a predictable as possible.
Similar to how we use `builtin` to ensure we're running the shell's
builtin function (and not a redefinition), use `command` to ensure we're
running `sed` and not a shell function or alias.
Copy link
Contributor

@akinomyoga akinomyoga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • It would be nice to remove this sed call entirely, but it looks like it might be difficult to re-implement this in "native" bash.

Actually. you could simply do

eval "local trap_argv=(${__bp_trap_string:-})"
prior_trap=${trap_argv[2]}

and it's more accurate. The current version in the master branch is broken when the trap string contains '. See this:

$ (trap "echo 'hello'" DEBUG; trap -p DEBUG)
hello
trap -- 'echo '\''hello'\''' DEBUG

# we can't easily do this with variable expansion. Leaving as sed command.
# shellcheck disable=SC2001
prior_trap=$(sed "s/[^']*'\(.*\)'[^']*/\1/" <<<"${__bp_trap_string:-}")
prior_trap=$(builtin command sed "s/[^']*'\(.*\)'[^']*/\1/" <<<"${__bp_trap_string:-}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, we have been simply using command in bash-completion and oh-my-bash, though I know kitty shell integration prefixes builtin to everything where applicable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I made that as a last minute change given that I referenced builtin in my commit message. I think it's unlikely someone would attempt to redefine command, but using builtin command here does make this the most explicit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, either works for me, but then the next question would be whether we should prefix builtin to everything like builtin local, builtin return, builtin unset, builtin set, builtin shopt, etc.

@jparise
Copy link
Contributor Author

jparise commented Jan 13, 2025

  • It would be nice to remove this sed call entirely, but it looks like it might be difficult to re-implement this in "native" bash.

Actually. you could simply do

eval "local trap_argv=(${__bp_trap_string:-})"
prior_trap=${trap_argv[2]}

That is much nicer! Will you create a new pull request using that approach?

@akinomyoga
Copy link
Contributor

You can modify this PR. I don't care about the credit about it.

@jparise
Copy link
Contributor Author

jparise commented Jan 13, 2025

You can modify this PR. I don't care about the credit about it.

I created #170 so the maintainers can compare the two approaches.

@akinomyoga
Copy link
Contributor

I created #170 so the maintainers can compare the two approaches.

Thanks!

Copy link
Contributor

@akinomyoga akinomyoga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think #170 is better.

@akinomyoga
Copy link
Contributor

@rcaloras Thanks for checking and merging PRs! Now that #170 is merged, this PR (which implements an alternative approach to #170) can be closed.

@rcaloras rcaloras closed this Aug 3, 2025
@jparise jparise deleted the sed-command branch August 3, 2025 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants