Replace which with command -v

[mchaker]

Debian has deprecated usage of which, as per:
https://salsa.debian.org/debian/debianutils/-/commit/3a8dd10b4502f7bae8fc6973c13ce23fc9da7efb

The recommendation is now to use command -v.

[mchaker]

@nntoan can you take a look?

[levilovearch]

I got the same problem in Debian sid.
https://linux.debian.bugs.dist.narkive.com/eCDBKTyB/bug-992401-debianutils-warning-message-usr-bin-which-this-version-of-which-is-deprecated-and-should-
I suggest there are more 'which' (around 10 more) to replace in the code.

[mchaker]

Thank you for the additional information.
@levilovearch I have fixed the remaining instances of which in the repo.

[mchaker]

@nntoan can you please take a look?

[akinomyoga]

I think these are generally good fixes, but have you noticed the existing shell function type_exists for this purpose? I feel we should update the definition of type_exists to something more efficient like type -P "$@" &>/dev/null or command -v "$@" &>/dev/null and then use type_exists everywhere, which is more consistent and idiomatic. What do you think?

[akinomyoga]

I also found the uses of `type CommandName &>/dev/null':

./completions/docker-compose.completion.sh:42:  type compopt &>/dev/null && compopt -o nospace
./completions/docker.completion.sh:409: type compopt &>/dev/null && compopt -o nospace
./completions/git.completion.sh:158:if ! type _get_comp_words_by_ref >/dev/null 2>&1; then
./lib/base.sh:117:      if type "mkisofs" > /dev/null; then
./lib/base.sh:268:      type "$1" &> /dev/null ;
./plugins/nvm/nvm.plugin.sh:8:if ! type "nvm" &> /dev/null; then
./plugins/sdkman/sdkman.plugin.sh:7:if ! type "sdk" &> /dev/null; then

[mchaker]

I think these are generally good fixes, but have you noticed the existing shell function type_exists for this purpose? I feel we should update the definition of type_exists to something more efficient like type -P "$@" &>/dev/null or command -v "$@" &>/dev/null and then use type_exists everywhere, which is more consistent and idiomatic. What do you think?

I think this is a good idea. Do you mean that we should update the builtin bash function somehow? Or, is type_exists something provided by ohmybash?

[akinomyoga]

Do you mean that we should update the builtin bash function somehow? Or, is type_exists something provided by ohmybash?

Thank you for your reply! I meant the latter. Currently, the definition of type_exists is given here. We can replace the function body with a more efficient implementation.

[akinomyoga]

I found a related PR #171. I think we need to care about the differences between the builtin commands that can be used to detect the commands. I summarized in the following table which command can detect each type of the commands.

Command	file	builtin	function	keyword	alias
command -v -- CMD &>/dev/null	v	v	v	v	v
type -- CMD &>/dev/null	v	v	v	v	v
hash -- CMD &>/dev/null	v	v	v
type -P -- CMD &>/dev/null	v
declare -F -- CMD &>/dev/null			v
compgen -X '!CMD' -k &>/dev/null				v
alias CMD &>/dev/null					v
which -- CMD &>/dev/null	v
(alias;declare -f) | which --i --read-functions -- CMD &>/dev/null (GNU)	v		v		v

Those different variations can be found here and there in the codebase, which I think should be made consistent. #171 implies that we might need to prepare several functions like type_exists for different purposes.

Edit: Bash-it seems to prepare two shell functions, _command_exists and _binary_exists, for this purpose.

[akinomyoga]

I have adopted the change made in #171 and then renamed type_exists to _omb_util_command_exists. I have also prepared _omb_util_binary_exists for the older behavior using type -P "$1". @mchaker Can you update the PR by using the newly introduced functions (_omb_util_command_exists or _omb_util_binary_exists)?

It'd be great if you could also update other uses of command -v &>/dev/null, type ... &>/dev/null, hash ... &>/dev/null, etc. but that's not mandatory.

[mchaker]

Thank you so much @akinomyoga !

I will reply in detail once I return from holiday travel.

I appreciate your collaboration and work very much.

[akinomyoga]

Thank you for your reply! OK! Have a good year and holidays!

[mchaker]

I have adopted the change made in #171 and then renamed type_exists to _omb_util_command_exists. I have also prepared _omb_util_binary_exists for the older behavior using type -P "$1". @mchaker Can you update the PR by using the newly introduced functions (_omb_util_command_exists or _omb_util_binary_exists)?

It'd be great if you could also update other uses of command -v &>/dev/null, type ... &>/dev/null, hash ... &>/dev/null, etc. but that's not mandatory.

Ok, so looking back on your comments, I am very grateful for your collaboration.

Now for the next step: You have made _omb_util_command_exists and _omb_util_binary_exists. I think that is a good idea.

Let me see if I am understanding what I should do next:

1. Use _omb_util_binary_exists for all instances of type -P.
2. Use _omb_util_command_exists for all other instances of commands in the table from your earlier comment.

Is that correct?

[akinomyoga]

Thank you!

Let me see if I am understanding what I should do next:

1. Use _omb_util_binary_exists for all instances of type -P.
2. Use _omb_util_command_exists for all other instances of commands in the table from your earlier comment.

Is that correct?

Basically, yes. Additionally, the tests declare -f / declare -F are supposed to be replaced by _omb_util_function_exists. Currently there are no replacements for alias CMD &>/dev/null and compgen -k -X \!CMD, so they should be kept as is for now (although I'm not sure if there are any instances of them).

Also, the rewriting is not strict as I don't think the existing instances of these commands are so carefully chosen. E.g., if you think _omb_util_binary_exists is more appropriate for a specific instance of command -v, etc., you can change it as you like.

Also, be careful that some of the codes are supposed to be executed before loading lib/utils.sh or without loading lib/util.sh (such as tools/{install,uninstall,upgrade}.sh). In such cases, we cannot use these utilities defined in lib/utils.sh but can directly write type -P or command -v. I have introduced a new function _omb_module_require in #274 to control the loading order of lib/*, so we can put _omb_module_require lib:utils at the beginning of lib/*.sh that rely on these utilities so that lib/utils.sh is ensured to be loaded first, but I can take care of it after you made the changes if it is not so obvious for you.

[akinomyoga]

I noticed a function command_exists is also defined in lib/base.sh and used in plugins/battery/battery.plugin.sh. I think we can remove this function and replace it with _omb_util_command_exists.

[mchaker]

@akinomyoga Thank you for your patience -- I have attempted to make the changes requested. What do you think?

[akinomyoga]

@akinomyoga Thank you for your patience -- I have attempted to make the changes requested. What do you think?

Great! Thank you for your work! I have comments as follows. Could you check them?

[akinomyoga]

@mchaker Hi! If you are busy, I think I can take over the remaining changes in this PR. What do you think? Thank you!

[mchaker]

@akinomyoga Hi Koichi! I apologize for the radio silence. I am still willing to finish the changes -- will go through the requested changes now.

[akinomyoga]

@mchaker The changes are still incomplete for a long time. Or are you still working on it? Can I take over this PR?

[mchaker]

@akinomyoga Yes, you can take over this PR. Thank you for your patience, I wish I could have completed it sooner.

[akinomyoga]

Thanks!

[akinomyoga]

I've squashed related commits and fixed the present problems.

There are many conflicts. I'm going to rebase it to the latest master branch.

[akinomyoga]

rebase done

[akinomyoga]

These are the list of _omb_util_command_exists that has originally been which. I think it is better to make them _omb_util_binary_exists. See also #239 (comment).

[akinomyoga]

@mchaker I think I have finished the changes that I mentioned before. Also, I have added some other changes and adjustments. I'll keep this PR open for a while. If you have any comments on the updated version of the PR, please feel free to leave them here.

@nntoan Could you review the PR? The repository setting seems to have been changed recently so that any approval is needed to merge the PR. (I can actually self-approve my changes and forcibly merge the PR, but that doesn't seem like the neat way.) We have unified all the existence tests of commands (which varied to use which, type, hash, declare -f, type -P, command -v, and alias) in the utility functions _omb_util_{binary,command,function}_exists. The exceptions are the scripts in the subdirectory tools; They can be called as independent scripts, so the utility functions cannot be used there.

[akinomyoga]

@nntoan Can I merge the PR if there are no comments or concerns?

[nntoan]

@nntoan Can I merge the PR if there are no comments or concerns?

@akinomyoga Hey, yap seems all good to me. This can be merged, forgot to reply the thread earlier.

[akinomyoga]

@nntoan Thank you very much for the quick reply!

Edit: I have merged it!

[mchaker]

🙌🙏🥳 thank you so much!