Skip to content

oh-my-zsh: preserve shell expansion in custom and theme#9095

Open
ooojustin wants to merge 1 commit intonix-community:masterfrom
ooojustin:fix/zsh-omz-expansion
Open

oh-my-zsh: preserve shell expansion in custom and theme#9095
ooojustin wants to merge 1 commit intonix-community:masterfrom
ooojustin:fix/zsh-omz-expansion

Conversation

@ooojustin
Copy link
Copy Markdown

@ooojustin ooojustin commented Apr 13, 2026

Description

Fixes a regression from #9042.

programs.zsh.oh-my-zsh.custom and programs.zsh.oh-my-zsh.theme currently use escapeShellArg, which single-quotes values like $HOME/.config/shell.
That prevents shell expansion and makes oh-my-zsh look for custom themes and plugins in a literal $HOME/... path.

This switches the generated assignments back to double-quoted shell strings so runtime expansion still works:

  • ZSH_CUSTOM="..."
  • ZSH_THEME="..."

Validation:

  • adding a small regression test (zsh-shell-expansion) covering the generated .zshrc output
  • running nix run .#tests -- zsh-shell-expansion
  • confirming the same test fails against unpatched upstream/master
  • pointing my NixOS flake at this branch, rebuilding, and verifying that the generated .zshrc contains double-quoted ZSH_CUSTOM / ZSH_THEME and that a custom oh-my-zsh theme loads again

Checklist

  • Change is backwards compatible.

  • Code formatted with nix fmt or
    nix-shell -A dev --run treefmt.

  • Code tested through nix run .#tests -- test-all or
    nix-shell --pure tests -A run.all.

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.
    • Generate a news entry. See News
    • Basic tests added. See Tests
  • If this PR adds an exciting new feature or contains a breaking change.

    • Generate a news entry. See News

Copy link
Copy Markdown
Contributor

@Zocker1999NET Zocker1999NET left a comment

Choose a reason for hiding this comment

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

I'm sorry that I haven't thought about the possibility that users might legitimatly use shell variables in this config, otherwise I would have declared my PR as a potential breaking change.

While I'm personally not really for making this behavior explicit, I'm in principle open for discussion that there might be use cases for it.

The example you gave with $HOME seems weird to be, because IMO you can just use ${config.home.homeDirectory} instead. But, as I said, I'm open to accept the possibility that using shell variables instead might has an advantage I do not know yet.

But I'm explicitly against just reverting back to the old behavior because it may break users expectations should they be wanting to refer to a path containing chars like $ or ", because they can also just not use a function like escapeShellArg on their side because this function might escape its input using '', which are then taken literally.

I would even be more open to not escape those config values at all (i.e. remove the surrounding "), document that this behavior is explicit in the options documentations, declare that a breaking change & requesting updating users to escape their values using escapeShellArg.

Another possibility I see is adding a themeRaw option, defaulting theme = escapeShellArg cfg.themeRaw; and using themeRaw in .zshrc instead.

Or expanding escapeShellArg to not escape values which were surrounded by a new rawShellValue function to allow users to opt in to non-escaping that way. But while this would make it easy to port this change to arbitary values, it would require more code & adding more documentation so users know how to use it.

(and of course applying the same to custom resp.)

@ooojustin
Copy link
Copy Markdown
Author

you can just use ${config.home.homeDirectory} instead

I agree for that case, and I’m not trying to make shell-variable expansion a new explicit feature. In my case I noticed because I had one of these pointing at a $DOTFILES var. I could move that to sessionVariables, but my thought process for the PR was that the previous behavior should probably be preserved unless the intent was for this to be a breaking change.

If you’d rather handle this with something more explicit like themeRaw / customRaw, I’m open to that too, but I thought it might make sense as a follow-up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants