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

[Doron Behar <doron.behar@xxxxxxxxx>]

On Sat, Jun 02, 2018 at 12:45:17AM +0200, Oliver Kiddle wrote:
> 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

Yea I forgot to make them local alongside configured_lua_version
configured_user_tree configured_system_tree. Done, thanks.

> 
> 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

You are right, the `_store_cache` function's definition, uses the
following for every non-array argument given to it:

	print -r "$var=${(Pqq)^^var}"

Since my $var is known, I read the documentation and I understand what
the `P`, and double `q` mean so I'll put:

	print -r var=${(qq)var}

As for the loop running running for every argument `_store_cache` gets,
it ends with >! which should be used here as well according to the
documentation..

> 
> > 	  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.

Done.

> 
> > 	  # 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've blindly took the (( 1 )) from a cache policy function I saw on the
documentation.. It's like a return status write?

> 
> 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.

The idea with the 1st condition that tests the existence of the
manifests files, is that if non of them exists (which usually never
happens if luarocks is installed properly), is that only one of them is
needed for continuing with the modification date of them vs the cache
file..

Now that I'm thinking about it, I think it would be preferred to improve
the readability of the logic behind this procedure and use `return 0`
instead of `(( 1 ))` and `return 1` instead of `(( 0 ))`. In addition, I
think translating the whole thing to something like the equivalent you
wrote won't help either for readability so I was thinking about something
like this:

	local cache_status=1
	if [[ -f ${cache_file} ]]; then
	  if [[ -f ${user_manifest_file} ]]; then
	    if [[ ${user_manifest_file} -nt ${cache_file} ]]; then
	      cache_status=0
	    fi
	  fi
	  if [[ -f ${system_manifest_file} ]]; then
	    if [[ ${system_manifest_file} -nt ${cache_file} ]]; then
	      cache_status=0
	    else
	      cache_status=1
	    fi
	  fi
	fi
	return cache_status

Better?

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

Got it.

> 
> Oliver