Re: [PATCH 1/1] Squashed commit of the following:

[Oliver Kiddle <okiddle@xxxxxxxxxxx>]

Doron Behar wrote:
> However, the bad news is that I'm afraid that calling `luarocks config`
> twice like that whenever I query the cache validity, is a huge
> performance hit.

> The solution which will most likely best solve this issue is to use a
> similar cache mechanism for these values as well. This *inner* cache's

Sounds rather complicated but perhaps necessary depending on just how
slow it is. A global variable – typically named _cache_… – is one
option for caching that avoids much of the complexity of the disk cache
mechanism if the lifetime of the session is an appropriate policy for
how long to retain the cached information.

> I'd be glad to get some feedback, thanks!


> 	(( $+functions[___luarocks_manually_store_cache_configs_paths] )) ||
> 	___luarocks_manually_store_cache_configs_paths(){
> 	  user_config_path="$(_call_program user_config_path luarocks config --user-config)"
> 	  system_config_path="$(_call_program system_config_path luarocks config --system-config)"

These variables should be declared local. If the intention is for them
to be global, use typeset -g and prefix the names with something like
_cache_luarocks_.

> 	  print user_config_path=$user_config_path > ${cache_dir}/luarocks_configs_paths
> 	  print system_config_path=$system_config_path >> ${cache_dir}/luarocks_configs_paths

You might need to quote the values with ${(qq)user_config_path} in case
they have spaces in their values. By using braces around the print
statements only one redirection will be needed instead of the
redirection and append: { print ...; print ... } > cache

> 	  local where_luarocks=$(where luarocks)

Use $commands[luarocks] rather than where in a command substitution.
Command substitution typically requires a forked subshell which will be
less efficient.

> 	  # luarocks_configured_values
> 	  local configured_lua_version configured_user_tree configured_system_tree
> 	  # luarocks_configs_paths
> 	  local config
> 	  if [[ -e ${cache_dir}/luarocks_configs_paths ]]; then
> 	    if [ ${where_luarocks} -nt ${cache_dir}/luarocks_configs_paths ]; then

It is generally better to use [[ ... ]] for all conditions unless you're
writing a script targeted at /bin/sh – which is not the case here.

> 	  if [[ -f ${user_manifest_file} ]] || [[ -f ${system_manifest_file} ]]; then
> 	    if [[ -f ${cache_file} ]]; then
> 	      # if either one of the manifest files is newer then the cache:
> 	      if [ ${user_manifest_file} -nt ${cache_file} ] || [ ${system_manifest_file} -nt ${cache_file} ]; then
> 	        (( 1 ))
> 	      else
> 	        (( 0 ))
> 	      fi
> 	    else
> 	      (( 1 ))
> 	    fi
> 	  else
> 	    (( 1 ))
> 	  fi

I find this (( 1 )) stuff confusing. If I'm not mistaken the whole thing
is equivalent to:

  [[ ( ! -f ${user_manifest_file} && ! -f ${system_manifest_file} ) ||
      ! -f ${cache_file} || ${user_manifest_file} -nt ${cache_file} ||
      ${system_manifest_file} -nt ${cache_file} ]]

As for the logic, I'd mainly question what happens when one but not both
of the manifest files is found to not exist.

Note that && and || can be used inside [[ ... ]].

Oliver