aliases/cd: break the cd aliases into a separate module

[jjmcdn]

Aliasing cd to a function that manipulates DIRSTACK is surprising to anyone who already incorporates DIRSTACK operations in their workflow. Breaking this alias out into a separate module allows such people to turn off the aliasing. Leaving the module enabled by default preserves the existing behaviour for current omb users.

[jjmcdn]

Thanks for the contribution. The function is recently introduced by PR #105.

Aliasing cd to a function that manipulates DIRSTACK is surprising to anyone who already incorporates DIRSTACK operations in their workflow. Breaking this alias out into a separate module allows such people to turn off the aliasing.

I again checked the implementation. Originally, there are aliases to call cd -1, cd -2, etc. that are imported from Oh My Zsh. They do not work in Bash because Bash's cd doesn't support cd -1, cd -2, etc [Note 1].

[...]

• Note 1: In that sense, the description cd -1, cd -2, etc. for aliases 1, 2, etc. in the table added to aliases/README.md doesn't properly explain the feature.

That's a fair assessment as I didn't understand what the purpose of those aliases were anyway, but it seemed like a gap to not document them in some way, so I took a stab at it. Since I couldn't tell from looking at the code what was really intended by them I'm not surprised I didn't really capture it.

In my understanding, PR #105 attempted to implement them, and it used DIRSTACK as merely a means to implement them. The purpose of PR #105 is not to make cd behave as pushd [Note 2].

[...]

• Note 2: In that sense, the description pushd <dir> for alias cd in the table doesn't also properly explain the alias.

It seems to function as an elaborate pushd/popd from my experience, based on the implementation, and for the sake of brevity I thought giving a quick summary in the table would be good enough for someone turning on the alias module to know what to expect when they enabled it.

For me, I definitely didn't get what I expected, I was trying to understand what had happened to my directory stack where entries were getting added to it that I didn't put there or want there and my investigation eventually led to the discovery that the shell built-in, cd was being aliased on me without warning.

Then, I think a better fix should be to use another array (such as _omb_cd_stack) instead of utilizing DIRSTACK used by pushd/popd.

Yeah, that would be a better way to do it, I imagine, but since I don't really understand the zsh functionality it's trying to emulate, I'm probably not the person to try implementing it. I will be keeping this (or some similar change) in my local tree if we can't merge it or I'll just ensure I unalias cd everywhere if that becomes too much of a pain. It's possible this would be useful to me so I don't necessarily want to turn it off completely, but I can't see ever wanting to alias a shell builtin to something else. That's just going to hurt my ability to log in and use other systems, IMO.

Leaving the module enabled by default preserves the existing behaviour for current omb users.

Making it a separate module could be a good idea, but the existing behavior for the current OMB users wouldn't be preserved by modifying the template because the template is only used when the user installed OMB for the first time on the computer.

That's true, but I couldn't think of a simple way to handle that without adding some additional migration hook that would check whether the user had 'git pull'-d from a version prior to this change and then automatically update it for them, any such scheme seemed likely to break, so I opted for the path of least resistance here. New installations get the old behaviour so anyone doing a fresh install but that had been using it before would still see what they expected. It'd probably just be better to make it a flag day event and not enable the alias by default.

FWIW, I started on this becase, as you can probably tell, I really don't like the idea of making cd behave in any way that isn't what \cd will do and I though it stood out as the only shell built-in that was aliased in this way in OMB. I think that's still true, so it's the oddball, but I see now there's also aliases so that fgrep isn't calling /usr/bin/fgrep but grep -F --color=auto... and so on, so there are a few well-known commands that are hidden by aliases in the default lib directory and the ll alias is set in two different places with different arguments (aliases/general.aliases.sh and lib/directories.sh) so maybe I need to rethink my approach.

Anyway, thanks for taking the time to provide the feedback.

[akinomyoga]

Thank you for your reply. Then, I later try to modify it to use another array instead of DIRSTACK. I would also like to pick some of the fixes and documentation in this PR.

[akinomyoga]

I added an implementation that doesn't rely on pushd (commit 24bd7e71ad7a8c51eaeff001439a266daf992e3a).