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.

Sign up for GitHub

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

Jump to bottom

lib/bourne-shell.sh: Don't force-load bash-completion #572

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

lib/bourne-shell.sh: Don't force-load bash-completion #572

wants to merge 6 commits into from
+43 −36

Conversation

ifireball
Copy link
Contributor

Its already done better by the "system" completion module, and doing it there too makes disabling it impossible and debgging issues harder.

ifireball and others added 6 commits May 16, 2024 17:46
@ifireball @akinomyoga
lib/bourne-shell: Do not load bash-completion
ebb8c36 
It's already done better by the "system" completion module, and doing
it there too makes disabling it impossible and debgging issues harder.
@akinomyoga
feat(bashrc): enable "completions/system" by default
4f4c84c
@akinomyoga
style(completions/system): adjust coding styles
3a03814
@akinomyoga
refactor(rsync): normalize detection of macOS
8ed02a8
@akinomyoga
fix(completions/system): load bash-completion only once
fcce66a 
We have been sourcing bash-completion as many times as the number of
the installations and entry points that we can find.  However, only
one instance of bash-completion is enough.

We also re-order the detection to adjust the precedence.
@akinomyoga
feat(completions/system): integrate bash-completion loader in lib/bou…
d291140 
…rne-shell

The search that has been in lib/bourne-shell.sh is slightly different
from that in completions/system.completion.sh.  We integrate the
former into the latter.

* We add a search location
  `/usr/share/bash-completion/bash_completion` for bash-completion.
  This is the standard location for bash-completion v2.  We have been
  only checking /etc/bash_completion which is bash-completion v1.

* We also add a guard for the POSIX mode.  Older versions of
  bash-completion have an issue with the POSIX mode.  In particular,
  bash-completion v1 can only be used with with the macOS Bash 3.2,
  but bash-completion v1 does not work well in the POSIX mode.

* We also add a guard for already loaded bash-completion.  Other
  system configuration might already load bash-completion.  We skip
  loading bash-completion when we detect an existing bash-completion
  settings in tbe current shell environment.
akinomyoga
akinomyoga reviewed May 16, 2024
View reviewed changes
Copy link
Contributor

@akinomyoga akinomyoga left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks. I added system to the default set of the completions in the ~/.bashrc template because bash-completion is not loaded by default after the suggested change. The users who want to disable bash-completion can remove system from the completions array.

However, one issue is that we cannot enable the system completion for existing users because the updating process of Oh-My-Bash doesn't update the user's ~/.bashrc (which might already be edited by the user). This means that by applying those changes, we disable bash-completion of existing users, which is a problem. Do you have a solution?

@ifireball
Copy link
Contributor Author

ifireball commented May 16, 2024 •
edited
Loading

@akinomyoga is there a way to perhaps print a warning message during upgrade?

Wasn't "system" in the default ~/.bashrc template already for a while now? I was sure I wasn't the one that put it there in my local file...

If we absolutely want to keep the current behaviour, we can:

  1. Add an small hack in oh-my-bash.sh to force add system to the completions array.
  2. Have a variable to switch that hack off.
  3. Set the variable by default in the ~/.bashrc template.

That way newer users and those who explicitly add the variable it can get the new behaviour while users with older ~/.bashrc keep the existing behaviour.

We can call the variable something like _compat_force_load_system_completion to indicate to people this may be a temporary thing that will eventually go away.

@ifireball
Copy link
Contributor Author

ifireball commented May 16, 2024 •
edited
Loading

Please let me know if you want me to add what I've described. Since you've modified the PR I don't want to make further changes without your approval

@akinomyoga
Copy link
Contributor

@akinomyoga is there a way to perhaps print a warning message during upgrade?

It seems no. I've checked the code to update OMB. The upgrading is performed by a script in the older version of OMB (before upgrading), and currently, there doesn't seem to be a way to change the behavior for the new version of OMB after upgrading.

Wasn't "system" in the default ~/.bashrc template already for a while now?

No. It's never been there. From the beginning when the completions array is added to the template, its content has been git, composer, and ssh, and has never been changed.

I was sure I wasn't the one that put it there in my local file...

I'm not sure...

If we absolutely want to keep the current behaviour, we can:

  1. Add an small hack in oh-my-bash.sh to force add system to the completions array.
  2. Have a variable to switch that hack off.
  3. Set the variable by default in the ~/.bashrc template.

That's technically possible, but if we would have such a variable, I'd rather do that the other way around. That is,

  1. We keep the code in lib/bourne-shell.sh
  2. Add a variable to turn off loading bash-completion in lib/bourne-shell.sh

Please let me know if you want me to add what I've described. Since you've modified the PR I don't want to make further changes without your approval

I want to think about it more.

@ifireball
Copy link
Contributor Author

ifireball commented May 20, 2024 •
edited
Loading

That's technically possible, but if we would have such a variable, I'd rather do that the other way around. That is,

  1. We keep the code in lib/bourne-shell.sh

  2. Add a variable to turn off loading bash-completion in lib/bourne-shell.sh

That will mean we keep having duplicate code. Perhaps we can do one of:

  1. Source completions/system.completion.sh from lib/bourne-shell.sh.
  2. Put the code in a function defined in lib/bourne-shell.sh and then call it from lib/bourne-shell.sh (Conditioned by a variable) and from completions/system.completion.sh

I want to think about it more.

Go ahead.
Perhaps consider adding a mechanism to inform users about changes they may want to make to their configuration after an upgrade. Having that would have been handy now. If you want, I can try to write down an issue to describe what I think it aught to look like and we can discuss the details there.

@akinomyoga akinomyoga added the omb-next This issue has been fixed in the next oh_my_bash upgrade runs. label Sep 5, 2024
@akinomyoga akinomyoga mentioned this pull request Oct 1, 2024
Open
RobLoach
RobLoach reviewed Oct 1, 2024
View reviewed changes
completions/system.completion.sh
. "$BREW_PREFIX"/etc/bash_completion
# homebrew/versions/bash-completion2 (required for projects.completion.bash)
# is installed to this path
if [[ -f "$BREW_PREFIX"/share/bash-completion/bash_completion ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

One request I would make here is the ability to set a BASH_COMPLETION_DIR, and check it as a potential location.

Copy link
Contributor

@akinomyoga akinomyoga Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure if that is useful. If you know the location of bash_completion and needs to specify it explicitly, how is it different from source '<path to bash_completion>/bash_completion'? A plugin that allows users to write

BASH_COMPLETION_DIR='<path to bash_completion>'

plugins+=(system)

instead of

source '<path to bash_completion>/bash_completion'

doesn't seem useful. It actually complicates the configuration.

Also, the plugin name system implies loading bash-completion in a system package. Allowing the configuration for an arbitrary location of bash-completion doesn't match the plugin's name. If the configuration would be added, the plugin name should also be changed.

In addition, BASH_COMPLETION_* is a kind of variable namespace used by bash-completion [1,2], so we should avoid defining a variable of the form BASH_COMPLETION_* outside bash-completion. It might conflict in the future. We should use OMB_COMPLETION_SYSTEM_BASH_COMPLETION_DIR or something.

Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't think of just sourcing the file directly. Good call

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

@RobLoach RobLoach RobLoach left review comments

@akinomyoga akinomyoga akinomyoga left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
omb-next This issue has been fixed in the next oh_my_bash upgrade runs.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ifireball @akinomyoga @RobLoach