Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update directory aliases to use cd command #11658

Closed
wants to merge 1 commit into from

Conversation

mattdodge
Copy link

Standards checklist:

  • The PR title is descriptive.
  • The PR doesn't replicate another PR which is already open.
  • I have read the contribution guide and followed all the instructions.
  • The code follows the code style guide detailed in the wiki.
  • The code is mine or it's from somewhere with an MIT-compatible license.
  • The code is efficient, to the best of my ability, and does not waste computer resources.
  • The code is stable and I have tested it myself, to the best of my abilities.

Changes:

  • Use the cd command inside the ..., ...., etc aliases

Other comments:

Ever since #11550 has shipped, my ... aliases (which I used to override) have broken.

[user:~]$ ...
zsh: permission denied: ../..

The new behavior of actually unaliasing the aliases, rather than just bouncing out early means that my overrides no longer work. There is likely somewhere downstream I could move my aliases in my network of dotfiles, but I actually do like a lot of the directory aliases and I see no reason we can't just use the cd command in them rather than relying on some other shell shortcut.

@ohmyzsh ohmyzsh bot added Area: core Issue or PR related to core parts of the project Topic: alias Pull Request or issue regarding aliases labels May 1, 2023
@carlosala
Copy link
Member

carlosala commented May 1, 2023

Those are global aliases, that means that are not only commands. For example, mv foo ... would actually expand to mv foo ../...
That's why we can't accept your proposal. In that case, I don't see why you have issues with the new system, you can just create the aliases and omz will keep them for you.

EDIT: I see the regression, if there was an alias defined with the same name and omz overwrites it, our function will not be smart enough to detect it. We'll work to fix it.

@carlosala carlosala closed this May 1, 2023
@mattdodge
Copy link
Author

Ah neat, I didn't realize global aliases worked that way. That's actually quite convenient.

Is it expected that ..., which expands to ../.., would actually traverse up the directory tree? How does ../.. do that without a cd? Is that a zsh thing, an OMZ thing, or just a system shortcut?

@carlosala
Copy link
Member

It's a zsh option, that is set up here in ohmyzsh. It is disabled by default.

carlosala added a commit to carlosala/ohmyzsh that referenced this pull request May 1, 2023
Fix regression introduced in ohmyzsh#11550. If an existing alias was present in
the moment of sourcing, and oh-my-zsh aliases were disabled for that
file, it'd be overwritten aswell. See ohmyzsh#11658.
@carlosala
Copy link
Member

@mattdodge all your problems should be fixed in #11659. Could you try it with your existing config with omz pr test 11659 and opening a new terminal? Then, when exiting the first terminal, make sure to answer yes to go back to previous branch.

@mattdodge
Copy link
Author

@carlosala I'm still having issues, even on that branch. Although it seems like my root problem is something with auto_cd getting permission denied errors. I get the error when I run /, ../.., or even ~. All of those directories have correct chmod/chown permissions too. Weird.

To at least test your PR, I've overridden the ... alias and set zstyle ':omz:lib:directories' aliases no - and at least that is respecting my alias. So I think the work you've done there is good - thanks for that! Sounds like I just have something else wrong with my auto_cd setup and this recent change is just revealing it.

@carlosala
Copy link
Member

nice, thanks for the testing!

carlosala added a commit that referenced this pull request May 1, 2023
Fix regression introduced in #11550. If an existing alias was present in
the moment of sourcing, and oh-my-zsh aliases were disabled for that
file, it'd be overwritten aswell. See #11658.
hron84 pushed a commit to hron84/oh-my-zsh that referenced this pull request May 2, 2023
Fix regression introduced in ohmyzsh#11550. If an existing alias was present in
the moment of sourcing, and oh-my-zsh aliases were disabled for that
file, it'd be overwritten aswell. See ohmyzsh#11658.
kis87988 pushed a commit to kis87988/ohmyzsh that referenced this pull request May 21, 2023
Fix regression introduced in ohmyzsh#11550. If an existing alias was present in
the moment of sourcing, and oh-my-zsh aliases were disabled for that
file, it'd be overwritten aswell. See ohmyzsh#11658.
nbaronov pushed a commit to nbaronov/oh-my-zsh that referenced this pull request Aug 3, 2023
Fix regression introduced in ohmyzsh#11550. If an existing alias was present in
the moment of sourcing, and oh-my-zsh aliases were disabled for that
file, it'd be overwritten aswell. See ohmyzsh#11658.
cschuyle pushed a commit to cschuyle/ohmyzsh that referenced this pull request Apr 18, 2024
Fix regression introduced in ohmyzsh#11550. If an existing alias was present in
the moment of sourcing, and oh-my-zsh aliases were disabled for that
file, it'd be overwritten aswell. See ohmyzsh#11658.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: core Issue or PR related to core parts of the project Topic: alias Pull Request or issue regarding aliases
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants