Disable /etc/bash_completion.d/redefine_filedir

[akinomyoga]

I'm not sure what is the most appropriate way to solve the issue, but let me explain the issue here.

Fedora (and probably its downstream RHEL and CentOS) has /etc/bash_completion.d/redefine_filedir which tries to overwrite the shell function _filedir with the version defined in /usr/share/bash-completion/bash_completion. This doesn't cause the problem as far as the user uses the distribution version of bash-completion in /usr/share. However, if one downloads and uses the latest version of bash-completion, this introduces the inconsistency because _filedir in redefine_filedir is not necessarily the same as the downloaded version.

The reason why Fedora tries to overwrite _filedir is explained in /etc/bash_completion.d/redefine_filedir:

# This is a copy of the _filedir function in bash_completion, included
# and (re)defined separately here because some versions of Adobe
# Reader, if installed, are known to override this function with an
# incompatible version, causing various problems.
#
# https://bugzilla.redhat.com/677446
# http://forums.adobe.com/thread/745833

Actually, its workaround is already implemented in bash-completion itself in commit 2a05603, so there is no need to source redefine_filedir in the latest version of bash-completion. So I'd propose adding redefine_filedir along with acroread.sh in the blacklist.

• [When we want to use acroread.sh] By the way, the blacklist is somehow made empty in the distributed version of bash-completion in Fedora. Maybe it is because they still want to allow acroread.sh. But, even when we intendedly source acroread.sh, I'm not sure if acroread.sh still has the problem after ten years.

• [Background of PR] Actually, I haven't faced with an explicit inconsistency by the older version of _filedir, but the older version of _filedir caused the performance issue. Currently the result of compgen is processed by IFS=$'\n'; COMPREPLY=($(compgen ...)), but an older version of _filedir had used while read -r line: do ... done <<< $(compgen ...) which is very slow in the directory with thousands of files. This was discussed in

  • ble.sh:Perf: menu-complete chokes on directory with 6k+ files banoris/dotfiles#11 (comment) (the second item)

  I tried to replace _filedir with the latest implementation, but the problem is that there is no way to disable redefine_filedir forcing an old _filedir (other than disabling entire /etc/bash_completion.d by setting BASH_COMPLETION_COMPAT_DIR) even though I am using the latest version of bash-completion.

As @scop has mentioned in https://bugzilla.redhat.com/677446, maybe we can think about namespacing bash-completion functions (cf #537) for this. Nevertheless I tend to think we can include redefine_filedir in the blacklist as far as we have acroread.sh there. Maybe there is another approach. What do you think?

[scop]

This is kind of fine. But I would be happier if we'd get out of the blacklisting business altogether. I wasn't happy about it ten years ago, and I haven't grown to like it a single bit more since.

No matter what we do, I suppose we should do is to ask Fedora to just remove redefine_filedir from their future package versions. Granted, this wouldn't help in setups that have redefine_filedir in place and are trying to use a newer bash-completion installed in user dirs instead of the system one, but they'd like to source things from system locations anyway. Then again I'm not that convinced this is too supportable anyway, depends on what is in those system dirs.

I also don't think we want/need to support redefining our functions in the first place. People may opt to do it, but they can then deal with the possible breakage. Namespacing isn't a long term solution either. Even though we do have kind of a stable implicit API here and there, just replacing a function from an old installation with one from a newer bash-completion might or might not work. redefine_filedir isn't cool, but I at least made it (later, to prevent things like https://bugzilla.redhat.com/show_bug.cgi?id=1171396#c27 to happening again) so that it actually would be a copy of the "master" one from the same setup, not a different thing : https://src.fedoraproject.org/rpms/bash-completion/c/78258316ca0ce14c4aa43c234a4306410e2bf945?branch=rawhide

I have no actual info whether acroread.sh even exists these days in setups that are able to run the current bash-completion. But I guess it doesn't, and the blacklisting code could just go away from that point of view as well.

I tried to replace _filedir with the latest implementation, but the problem is that there is no way to disable redefine_filedir forcing an old _filedir

I think it can be done by placing a file with the desired implementation into the compat dir and name it so that it sorts (and thus loads) after redefine_filedir. Or override it in ${BASH_COMPLETION_USER_FILE:-~/.bash_completion} or something loaded off it.

---

Now, I'm not absolutely against this change, but what I'm trying to say with the rantish above that I'd rather like to clean up the landscape on this.

[akinomyoga]

Thank you for your thoughts!

No matter what we do, I suppose we should do is to ask Fedora to just remove redefine_filedir from their future package versions.

I agree with this.

redefine_filedir isn't cool, but I at least made it (later, to prevent things like https://bugzilla.redhat.com/show_bug.cgi?id=1171396#c27 to happening again)

I'm interested in what has caused the problem for this specific case and how it was solved by putting redefine_filedir. Was that caused by an older redefine_filedir?

so that it actually would be a copy of the "master" one from the same setup, not a different thing : https://src.fedoraproject.org/rpms/bash-completion/c/78258316ca0ce14c4aa43c234a4306410e2bf945?branch=rawhide

Yeah. However, as you have mentioned, this wouldn't help the case where there is an old redefine_filedir in place. In the original discussion banoris/dotfiles #11, the author seems to use RHEL 7.8 which is still under support but seems to be bundled with bash-completion 2.1 according to bash-completion - pkgs.org. So I'd be happy if we could see additional workarounds.

I think it can be done by placing a file with the desired implementation into the compat dir and name it so that it sorts (and thus loads) after redefine_filedir. Or override it in ${BASH_COMPLETION_USER_FILE:-~/.bash_completion} or something loaded off it.

You are right. We anyway have several ways to work around the problem, like putting redefine_filedir2 or what else. I think I would rather directly modify the blacklist in my custom bash_completion than always keep consistency between the used bash_completion and the custom redefine_filedir2 or ~/.bash_completion. However, it is a bit hard to correctly explain it to the ble.sh users who include not-so-skilled users; The reality is that someone will definitely do something wrong if I explain it in the documents. I also would like to keep the installation procedure clear and simple.

---

Now, I'm not absolutely against this change, but what I'm trying to say with the rantish above that I'd rather like to clean up the landscape on this.

I agree that we'd like to have a cleaner solution. Besides the clean solution, considering that redefine_filedir---and possibly still acroread.sh---will remain in the Linux distributions of the long-term support for a long time, I also think it would be nice to have some workarounds for redefine_filedir and acroread.sh for a while.

I currently don't have a perfect solution, but below are my current consideration on possible solutions:

1. declare -fr _filedir?

I also don't think we want/need to support redefining our functions in the first place.

I agree. Maybe if we would explicitly forbid redefining bash-completion functions, we might make the functions readonly by

declare -fr _filedir

As you have already mentioned at https://bugzilla.redhat.com/show_bug.cgi?id=677446#c7, this will produce warning messages when other scripts try to redefine the functions, but in this way, the author of such a script should notice that they are not allowed to redefine the bash-completion functions before they distribute the script.

The downside of this approach is that, even if a user intentionally wants to redefine the function knowing what they are trying to do, the user cannot redefine the functions. Actually, my framework ble.sh redefines bash-completion functions to insert/hook additional processing on invocations of bash-completion functions and to fix bugs of older implementations of bash-completions where the original behavior is kept as something like (Note: this is just a schematic code for illustration):

# copy the definition of the function to another name
eval "_original$(declare -f _filedir)"

# overwrite the original function
_filedir() {
  _inserted_command1
  _original_filedir "$@"; local exit_status=$?
  _inserted_command2
  return "$exit_status"
}

Currently, ble.sh modifies _filedir, _longopt, _parse_help and _parse_usage.

2. Namespacing?

Namespacing isn't a long term solution either.

I agree that namespacing doesn't solve the problems in every possible case, but for external completion writers, I think the namespace implies that the function name is somehow reserved so that the chances that the other people try to redefine the function will decrease. Of course, the original name should also be preserved because existing external completions rely on these functions. We may write as

_comp_filedir() {
  ...
}
# Deprecated: this is the old name for compatibility.
_filedir () { _comp_filedir "$@"; }

3. Exposing the shell variable for the blacklist?

This is the least intrusive change in my mind.

_comp_init_blacklist_glob=${BASH_COMPLETION_COMPAT_BLACKLIST-}

In this way, we don't have to bother with the blacklist management by ourselves in the bash-completion project. I can instruct the users to simply set BASH_COMPLETION_COMPAT_BLACKLIST='@(acroread.sh|redefine_filedir)' to avoid the problem of an older _filedir.

[scop]

No matter what we do, I suppose we should do is to ask Fedora to just remove redefine_filedir from their future package versions.

https://src.fedoraproject.org/rpms/bash-completion/pull-request/7

redefine_filedir isn't cool, but I at least made it (later, to prevent things like https://bugzilla.redhat.com/show_bug.cgi?id=1171396#c27 to happening again)

I'm interested in what has caused the problem for this specific case and how it was solved by putting redefine_filedir. Was that caused by an older redefine_filedir?

Comments in that bug indicate that a fix was applied to the _filedir contained in bash_completion, but I forgot about the separately (un)maintained copy in redefine_filedir so the fix never took place.

I agree that we'd like to have a cleaner solution. Besides the clean solution, considering that redefine_filedir---and possibly still acroread.sh---will remain in the Linux distributions of the long-term support for a long time, I also think it would be nice to have some workarounds for redefine_filedir and acroread.sh for a while.

I would personally be fine with assuming acroread.sh is no longer a thing unless shown otherwise, and thus we'd need no blacklist -> could purge all of that. But I'm not insisting on this.

I agree. Maybe if we would explicitly forbid redefining bash-completion functions, we might make the functions readonly by

This I'm not a fan of. There's no way of knowing if a user knows what they're doing and they really do want to replace _filedir -- we should not prevent them. By "not supporting" I meant just that if it breaks, they get to keep all the pieces, there is no promise that we'd spend time helping with that. Of course some time will be spent in figuring out if they've done so, but ~~

3. Exposing the shell variable for the blacklist?

Something like this I could accept. (Or the option to just removing all blacklist support, as mentioned above.)

I can instruct the users to simply set BASH_COMPLETION_COMPAT_BLACKLIST='@(acroread.sh|redefine_filedir)' to avoid the problem of an older _filedir.

I haven't followed all the discussion through, but this wouldn't be enough for users with older versions of bash-completion installed, such as the 2.1 on RHEL 7.8 -- they would additionally need to upgrade their bash-completion. But maybe that was implied.

[akinomyoga]

https://src.fedoraproject.org/rpms/bash-completion/pull-request/7

Thank you for creating changes to the Fedora bash-completion.

Comments in that bug indicate that a fix was applied to the _filedir contained in bash_completion, but I forgot about the separately (un)maintained copy in redefine_filedir so the fix never took place.

I see. Thank you for the explanation.

I would personally be fine with assuming acroread.sh is no longer a thing unless shown otherwise, and thus we'd need no blacklist -> could purge all of that. But I'm not insisting on this.

OK. However, now I think redefine_filedir may remain for the next ten years though I don't think these older versions of _filedir will cause serious problems.

Something like this I could accept.

I actually have a question related to the usage of this user settings BASH_COMPLETION_COMPAT_BLACKLIST. In bash_completion, there are already several shell variables for the initial configuration of bash-completion, such as BASH_COMPLETION_COMPAT_DIR and BASH_COMPLETION_USER_FILE. On the other hand, the login shell first loads /etc/profile (which will load bash-completion via /etc/profile.d/bash_completion.sh) before loading ~/.bash_profile or ~/.bash_login. My question is where these configurations BASH_COMPLETION_* are supposed to be set for login shells.

(Or the option to just removing all blacklist support, as mentioned above.)

I'd like to use it to disable redefine_filedir.

this wouldn't be enough for users with older versions [...] But maybe that was implied.

Yes, I was implicitly assuming that the user gets the latest version of bash-completion. I was discussing the inconsistency of _filedir between the distro redefine_filedir and the actually used bash-completion.

[scop]

My question is where these configurations BASH_COMPLETION_* are supposed to be set for login shells.

For per user config, the profile.d loader sources ${XDG_CONFIG_HOME:-$HOME/.config}/bash_completion. For systemwide config a profile.d script that gets sourced before the bash-completion one is one way to do it.

[akinomyoga]

Oh, I see. I have been somehow thinking that ~/.config/bash_completion is supposed to be the main script of bash-completion that the user has downloaded, which turned out to be wrong. Now I realized it is actually explained in README. Sorry for the noise.

I will later update the PR so that it adds the variable BASH_COMPLETION_COMPAT_BLACKLIST.

[akinomyoga]

I have updated the patch.

I thought maybe I also need to update doc/bashrc, but doc/{bashrc,inputrc} are actually the symbolic links to test/config/{bashrc,inputrc} and contain settings for tests, which are abnormal to be used in the real configuration. I was not sure whether I should add BASH_COMPLETION_COMPAT_BLACKLIST for the tests or the documentation, but I am now adding the setting for tests. Maybe we should create separate files at doc/{bashrc,inputrc} for documentation purposes (or we can just remove these symbolic links)?

[scop]

LGTM with a few doc suggestions, feel free to merge after addressing the way you see fit.

WDYT, should we note that we're ignoring files we treat as editor backups etc unconditionally under the hood, i.e. one does not need to address them by setting BASH_COMPLETION_COMPAT_IGNORE? No strong feelings here.

[scop]

Re doc/{bashrc,inputrc}, I think we should just remove the symlinks. If you agree, feel free to just go ahead and commit+push it, no need for a PR or separate approval for that.

[akinomyoga]

LGTM with a few doc suggestions, feel free to merge after addressing the way you see fit.

Thank you! I have applied these changes.

WDYT, should we note that we're ignoring files we treat as editor backups etc unconditionally under the hood, i.e. one does not need to address them by setting BASH_COMPLETION_COMPAT_IGNORE? No strong feelings here.

Yeah, this sounds nice! But it seems not quite practical after some consideration. Looking at the definition of _backup_glob, I found that it contains a relatively long string and is also used in other places.

./bash_completion:1327:_backup_glob='@(#*#|*@(~|.@(bak|orig|rej|swp|dpkg*|rpm@(orig|new|save))))'
./bash_completion:1337:        local -a svcs=($xinetddir/!($_backup_glob))
./bash_completion:1354:        $(printf '%s\n' ${sysvdirs[0]}/!($_backup_glob|functions|README)))
./bash_completion:1401:        for svc in "$svcdir"/!($_backup_glob); do
./bash_completion:2419:        [[ ${_comp_file##*/} != @($_backup_glob|Makefile*|${BASH_COMPLETION_COMPAT_IGNORE-}) &&
./completions/cvs:171:                    COMPREPLY=($(compgen -X "$_backup_glob" -W '"${files[@]}"' \
./completions/invoke-rc.d:15:    services=($sysvdir/!(README*|*.sh|$_backup_glob))
./completions/update-rc.d:15:    services=($sysvdir/!(README*|*.sh|$_backup_glob))

If we want to manage the backup-filename pattern consistently, we need to set as BASH_COMPLETION_COMPAT_IGNORE="@($_backup_glob|Makefile*|$_user_specified_compat_ignore)", but _backup_glob is unavailable as BASH_COMPLETION_COMPAT_IGNORE is supposed to be set before bash_completion is loaded. So, actually, the user needs to explicitly set this long value:

BASH_COMPLETION_COMPAT_IGNORE='@(#*#|*@(~|.@(bak|orig|rej|swp|dpkg*|rpm@(orig|new|save)))|Makefile*|$_user_specified_compat_ignore)'

I don't have a simpler solution for this for now. I think we can just keep the current setup until someone raises an issue related to the ignored backup files in the compatibility directory.

[akinomyoga]

Re doc/{bashrc,inputrc}, I think we should just remove the symlinks. If you agree, feel free to just go ahead and commit+push it, no need for a PR or separate approval for that.

Thank you! I have now pushed it to the mater 2b6785d.

[scop]

WDYT, should we note that we're ignoring files we treat as editor backups etc unconditionally under the hood, i.e. one does not need to address them by setting BASH_COMPLETION_COMPAT_IGNORE? No strong feelings here.

Yeah, this sounds nice! But it seems not quite practical after some consideration.

I agree it's not practical to ask users to manage the backup globs in any way at this point. But that's not what I meant, by "should we note" I meant if we should add a comment near BASH_COMPLETION_COMPAT_IGNORE docs explicitly saying that one does not need to consider them with the ignore, they are handled automatically. I'm leaning towards that we should, something like "Note that common editor backup filenames and the like in this directory are ignored automatically, they do not need to be covered by this glob."

[akinomyoga]

I meant if we should add a comment near BASH_COMPLETION_COMPAT_IGNORE docs explicitly saying that one does not need to consider them with the ignore, they are handled automatically. I'm leaning towards that we should, something like "..."

Ah, I see. I misunderstood your previous comment. Thank you for your explanation. I'll later update it.

[scop]

https://src.fedoraproject.org/rpms/bash-completion/pull-request/7

This was merged 2020-01-07.

[akinomyoga]

Thank you. I also updated the description in bash_completion.txt about backup files and Makefiles ignored by default.

[rathann]

I've no idea how but this (https://src.fedoraproject.org/rpms/bash-completion/pull-request/7) broke directory completion for me: https://bugzilla.redhat.com/show_bug.cgi?id=2070852 . In short, <TAB> is now appending a space instead of a slash to the end of the directory name forcing me to delete the space and type / manually if I want to TAB-complete lower subdirectory levels, which I usually do.

[rathann]

Ok, false alarm. It turns out I did have this old acroread installed and it was overriding the _filedir() function. Ensuring that the acroread bash script is not sourced makes the issue go away. I guess I'll repackage the RPM without the offending script.

[akinomyoga]

Thanks for the information! So, does it mean that the up-to-date version of Acrobat Reader still installs the broken script?

[rathann]

Thanks for the information! So, does it mean that the up-to-date version of Acrobat Reader still installs the broken script?

There's no up-to-date version. I'm talking about the ancient 9.5.5 version from 2013 that I keep around to open some PDF forms unsupported by open-source PDF readers.

[akinomyoga]

Thank you for the information! After #539 is settled, we are going to merge this PR where we provide a way to work around the problem for users (cf the updated description in doc/bash_completion.txt). If you have any thoughts on how we should work around the problem, please feel free to tell us that! Thank you.