Fixes user defined aliases being overridden by OMB

[solonovamax]

Fixes #304.

Currenty, oh-my-bash currently overrides any aliases that has been set for ls, grep, egrep, and fgrep, if the file /usr/bin/dircolors exists.
This means if a user specifies any aliases before OMB is sourced, they will be overwritten in a manner which is non transparent to the user (It is not stated anywhere, and the user cannot explicitly disabled it).

The goal of this PR is to not overwrite the aliases if they already exist, preferring the pre-defined aliases instead.
Alternatively, a means of disabling the new OMB aliases could be introduced through an environment variable, to allow the user to not have them set.

Currently, it wraps each alias call in an if statement, to check if they exist prior to applying them.
But, as per suggestions from akinomyoga, a function shall be introduced to streamline this process, in addition to refactors to all the other aliases which are defined by OMB that are not in the aliases folder or the plugins folder.

[akinomyoga]

• I don't know if there is any such workflow of creating an issue with the description and a PR with no description at the same time, but why don't you put all the relevant information in the PR itself?
• I'm actually not sure if the existing aliases should be preferred to the ones defined by OMB in general. Aren't there any cases that the aliases in OMB preferred to the existing ones? I feel the situation is usually opposite to this PR: First, some system-default aliases are defined by /etc/profile.d/*. Next, each user overwrites the aliases with the custom ones in ~/.bashrc if necessary. If we extend this, shouldn't the custom aliases by the user be defined after sourcing oh-my-bash.sh? If the user first defines the aliases and then sources oh-my-bash.sh, OMB cannot distinguish whether the existing aliases are the system-default ones (that can be overwritten) or the user-defined ones (that shouldn't be overwritten). What is your use case?
• There seem to be still other places that overwrite these names with new aliases: lib/directories.sh, lib/grep.sh. If we should apply the change of this PR, don't you also need to check these aliases?
• If we should apply the change of this PR, why do you check only a part of the aliases defined in lib/*.sh? There are also similar aliases even in the same file (bourne-shell.sh#L76-L78). There are many others in other files. What are the differences between the ones checked for the existing definitions and the ones unchecked? If there is no difference, maybe we should add the same checks to all the aliases.
• Maybe we may define a new function for this purpose (checking the existing one and defining a new alias).

[solonovamax]

I don't know if there is any such workflow of creating an issue with the description and a PR with no description at the same time, but why don't you put all the relevant information in the PR itself?

My bad.
I initially created the issue, and then realize "this is pretty simple, I could open a PR for it", so I did.

I'll add more information to the PR in a few seconds.

I'm actually not sure if the existing aliases should be preferred to the ones defined by OMB in general. Aren't there any cases that the aliases in OMB preferred to the existing ones? I feel the situation is usually opposite to this PR: First, some system-default aliases are defined by /etc/profile.d/*. Next, each user overwrites the aliases with the custom ones in ~/.bashrc if necessary. If we extend this, shouldn't the custom aliases by the user be defined after sourcing oh-my-bash.sh? If the user first defines the aliases and then sources oh-my-bash.sh, OMB cannot distinguish whether the existing aliases are the system-default ones (that can be overwritten) or the user-defined ones (that shouldn't be overwritten). What is your use case?

I personally happened to define my aliases before sourcing OMB, which is why I opened the issue & pr. But, I don't think that OMB should be overwriting these aliases, especially with how it's currently done. (Transparently to the user)
Because, if it was done in the ls.aliases.sh file, I would find this much more reasonable, as the user explicitly enabled the ls aliases. But, with the current implementation, it is overwritten in a core component of OMB, which means it cannot be disabled by the user if they desire.

If we should apply the change of this PR, why do you check only a part of the aliases defined in lib/*.sh? There are also similar aliases even in the same file (bourne-shell.sh#L76-L78). There are many others in other files. What are the differences between the ones checked for the existing definitions and the ones unchecked? If there is no difference, maybe we should add the same checks to all the aliases.

I would agree, that the aliases in the other transparent components of OMB should also have checks, to prevent from overwriting user aliases without the user explicitly wanting it.

Maybe we may define a new function for this purpose (checking the existing one and defining a new alias).

Yes, definitely.
I was on my laptop at the time, so I only authored a very quick fix, but I can look at a much larger scope fix for all the other aliases that are created, and a function to make doing so simpler.

[solonovamax]

Updated PR description to contain more information

[solonovamax]

Further, should the aliases in the grep.sh, directories.sh, and bourne-shell.sh not be moved into the aliases folder, so that the user can explicitly enable/disable them?

That seems much more reasonable to me, as I don't believe these aliases are actually used anywhere in OMB components.

[akinomyoga]

Thank you for updating the description. Additionally, you may edit the PR title.

Because, if it was done in the ls.aliases.sh file, I would find this much more reasonable, as the user explicitly enabled the ls aliases. But, with the current implementation, it is overwritten in a core component of OMB, which means it cannot be disabled by the user if they desire.

It's a very fair argument.

Further, should the aliases in the grep.sh, directories.sh, and bourne-shell.sh not be moved into the aliases folder, so that the user can explicitly enable/disable them?

Yeah, I agree. I also think we should clean up these things. One thing that I'm not sure of is whether we can introduce such breaking changes without notice. I guess there are a few hundred thousand users of OMB (at least, there were about twenty thousand clones by ten thousand unique cloners in the last two weeks), and I guess most of them are using OMB with the default configuration plus a theme. I doubt most of them actually check what aliases are defined in each *.aliases.sh and can recover the aliases moved to e.g. ls.aliases.sh by turning on aliases+=(ls) when they have liked it. But probably, we don't have to care about the users of the default configuration too much. If you could clean them up, it would be very helpful.

[solonovamax]

Yes, the breaking change is my one concern.
One possible idea would be to instead introduce them via an environment variable instead?

[solonovamax]

Also, I am converting the PR to a draft until we decide which option to go with.

[akinomyoga]

Thank you for the reply. It's actually hard for me to judge whether we can break the existing behavior.

One possible idea would be to instead introduce them via an environment variable instead?

Can you describe it in more detail? Is it the same environment variable as the one mentioned in the cover (as quoted below)?

Alternatively, a means of disabling the new OMB aliases could be introduced through an environment variable, to allow the user to not have them set.

My current consideration is this: For example, if that variable contains the value no, new aliases are not defined by lib/*.sh at all. If the value is yes, the new aliases in lib/*.sh always override existing ones. If the value is check, the new aliases are defined only when there is no existing definition. The "default behavior" when the variable is not defined is yes so that the behavior in the existing environment does not change by upgrading. We put OMB_XXXX=check in templates/bashrc.osh-template so that the existing aliases are checked in the new environments. After a long time, we may switch the default behavior from yes to check. But maybe we can just forcibly change the default behavior from yes to check from the beginning, which is maybe cleaner.

Or maybe you are thinking of introducing a variable to control the aliases defined in directories.sh and ls.aliases.sh in a different way?

[solonovamax]

Thank you for the reply. It's actually hard for me to judge whether we can break the existing behavior.

One possible idea would be to instead introduce them via an environment variable instead?

Can you describe it in more detail? Is it the same environment variable as the one mentioned in the cover (as quoted below)?

Alternatively, a means of disabling the new OMB aliases could be introduced through an environment variable, to allow the user to not have them set.

Yes, that is what I meant.

An environment variable like DISABLE_AUTO_UPDATE, COMPLETION_WAITING_DOTS, OSH_THEME, etc. could be introduced, with the default set to true.

My current consideration is this: For example, if that variable contains the value no, new aliases are not defined by lib/*.sh at all. If the value is yes, the new aliases in lib/*.sh always override existing ones. If the value is check, the new aliases are defined only when there is no existing definition. The "default behavior" when the variable is not defined is yes so that the behavior in the existing environment does not change by upgrading. We put OMB_XXXX=check in templates/bashrc.osh-template so that the existing aliases are checked in the new environments. After a long time, we may switch the default behavior from yes to check. But maybe we can just forcibly change the default behavior from yes to check from the beginning, which is maybe cleaner.

Or maybe you are thinking of introducing a variable to control the aliases defined in directories.sh and ls.aliases.sh in a different way?

That is what I was thinking, although it would not control any aliases in ls.aliases.sh, only those in lib/*.sh.

Also, enable, disable, and check might be better.

[akinomyoga]

I thought, ideally, it'd be better if we could finally reorganize things in ls.aliases.sh and other *.aliases.sh, but the suggested shell variable (to control the aliases in lib/*.sh) may be the practical solution if we consider the compatibility. You can decide which option to take.

Also, enable, disable, and check might be better.

Yes, though maybe it depends on the choice of the variable name. We have been defining variable names like DISABLE_AUTO_UPDATE and COMPLETION_WAITING_DOTS, but now we want to prefix all the OMB variables with OMB_. (If you are interested the related discussion is #280 and there is a pending fix at PR #270)

[akinomyoga]

Can I take over this PR and implement the suggested configuration variable (say, OMB_DEFAULT_ALIASES={enable,disable,check})?

[solonovamax]

Of course!

I don't mind at all if you take over this & PR it, as I haven't been working on it at all in quite a while

[akinomyoga]

Thank you for your response! I'll later work on it when I have time!

[akinomyoga]

I've implemented OMB_DEFAULT_ALIASES={enable,disable,check} and pushed it to the PR branch.

[akinomyoga]

@solonovamax I've merged the PR. Thank you for raising the issue and attempting to solve the issue!