Function autoloading in bash

Question

I'm wondering if anyone can code review my code below. That's the core code of my little framework for bash in GitHub.

## ==========================================================================================
## FUNCTION : autoload_functions
## PURPOSE : To check functions that are required, try to autoload them if NOT YET defined
##
## INPUT: array of $req_func
## OUTPUT: exit 1 - if any required function cannot be autoloaded (meaning, not found in standard
## library location.
## ==========================================================================================
autoload_functions()
{
 local req_func=$*
 local ret_val=0
 ## check first parameter. If it's a directory, then
 ## it is where functions should be loaded from
 local new_libdir=1ドル
 [[ -d $new_libdir ]] && {
 shift
 LIB_DIR=$new_libdir
 }
 req_func=$* ## redeclare
 ## iterate through all $req_func, set $ret_val to 1
 ## if any required function cannot be autoloaded
 for each_func in $req_func; do
 func_name=`basename $each_func`
 ## if function undefined, try to auto-include (reference from specified location)
 type $func_name &> /dev/null || source $LIB_DIR/${each_func}.bash.inc
 ## recheck, ensure it has been defined.
 type $func_name &> /dev/null || (echo "ERROR: Missing required ${each_func}"; ret_val=1)
 done
 ## abort (exit 1) if any required func cannot be autoloaded
 [ $ret_val -eq 1 ] && exit $ret_val
} ## END: autoload_functions()

Answer 1

Bug because of subshell

You are losing the value of ret_val in a subshell here:

... || (echo "ERROR: Missing required ${each_func}"; ret_val=1)

Variable assignments in a subshell are local to the subshell, so the variable in your function won't change. Instead of a subshell, you just want to group these commands using { ...; } like this:

... || { echo "ERROR: Missing required ${each_func}"; ret_val=1; }

Unnecessary variable

You don't need the req_func variable. The top of your function could have been written simpler like this:

autoload_functions()
{
 local ret_val=0
 ## check first parameter. If it's a directory, then
 ## it is where functions should be loaded from
 local new_libdir=1ドル
 [[ -d $new_libdir ]] && {
 shift
 LIB_DIR=$new_libdir
 }
 ## iterate through all arguments, set $ret_val to 1
 ## if any required function cannot be autoloaded
 for each_func; do
 # ...

Prefer $(...)

The `...` form for command substitution is deprecated, use $(...) instead:

func_name=$(basename $each_func)

Quoting variables with paths

This line will not work if LIB_DIR contains spaces:

type $func_name &> /dev/null || source $LIB_DIR/${each_func}.bash.inc

This would be safer:

type $func_name &> /dev/null || source "$LIB_DIR"/${each_func}.bash.inc

Though this is probably a bit paranoid.

The same goes for $func_name and ${each_func} too, but that would seem even more paranoid. (Who in his right mind would create bash scripts with spaces in the name?)

Naming

The names you chose for some variables are not great. I'd recommend these alternatives:

• Instead of each_func: func_path or func_relpath (as in relative path from LIB_DIR)
• Instead of ret_val: exit_code, because you may exit with this

• icasimpan: Hi janos. Thanks for the code review. I'll implement it on my script :)
• chepner: Not only is req_func unnecessary, it is actually a bug if for some reason your arguments contain whitespace: autoload_functions /home/me/libdir "my func.txt". Iterate over "$@" directly (or implicitly as with for each_func handles such names correctly.
• Etan Reisner: If you are going to suggest quoting $LIB_DIR on that line you should probably also suggest quoting $func_name and ${each_func}.
• janos: You're right @EtanReisner, but that would seem pretty paranoid here. It was worth adding a note about it, so thanks again!

Answer 2

• Error messages should be written to stderr, so

  echo "ERROR: Missing required ${each_func}">&2

• The comment at the beginning does not describe your function correctly. INPUT: array of $req_func does not really make sense. Your function accepts 0,1 or more arguements. Under some cirucmstances the first is interpreted as directory where the functions should be searched and the rest are the functions that should be loaded. So it is also wrong that it looks in the standard library location

• The comment does not mention the variable LIB_DIR explicite.

• The behaviour of your function depends on circumstances that may not be clear to the user. autoload func1 func2 tries to load func2 from the directory func1 if there is a directory ./func1 and it tries to load func1 and func2 if from $LIB_DIR if there is not a directory ./func1. That is not a reall good design.

• Under some circumstances it changes the environment variable LIB_DIR for the whole script that is calling autoload_function.

• The type command does not only exit with 0 if $func_name is a command. It also exists with success under some other circumstances. You should explicitly check if type says that its argument is a function.

• I would prefer reporting the first missing function and then exiting the program.

• rec_func is assigned a variable that is overwritten before its first usage. That is not wrong but I don't like it.

• LIB_DIR may be an existing environment variable or you create a new variable that is not an environment variable because you don't export it.

• of course the points @janos already mentioned.