From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2224 invoked by alias); 5 Jun 2018 15:41:25 -0000 Mailing-List: contact zsh-workers-help@zsh.org; run by ezmlm Precedence: bulk X-No-Archive: yes List-Id: Zsh Workers List List-Post: List-Help: List-Unsubscribe: X-Seq: 42933 Received: (qmail 23696 invoked by uid 1010); 5 Jun 2018 15:41:25 -0000 X-Qmail-Scanner-Diagnostics: from mail-wm0-f65.google.com by f.primenet.com.au (envelope-from , uid 7791) with qmail-scanner-2.11 (clamdscan: 0.99.2/21882. spamassassin: 3.4.1. Clear:RC:0(74.125.82.65):SA:0(-1.9/5.0):. Processed in 1.731775 secs); 05 Jun 2018 15:41:25 -0000 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on f.primenet.com.au X-Spam-Level: X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,FREEMAIL_FROM, RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_PASS, T_DKIM_INVALID autolearn=ham autolearn_force=no version=3.4.1 X-Envelope-From: doron.behar@gmail.com X-Qmail-Scanner-Mime-Attachments: | X-Qmail-Scanner-Zip-Files: | DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:subject:message-id:mail-followup-to:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=uW4hf0feAaDN42e74dAXJwsPGTKB5QIipXgzlX10bU4=; b=W+Z5GBbuaJZ9qMc9udGxkDZ1I1kxpgDWGgTMk2LpBxAKIu9HKvqoO9o73hcdLoqfHS 1Ate3zbpsruwCCrE3KqCYDac5gkEoUh2YQC8tlpE7JI7nH5Lv6qngk/cr6y8Z7waoWSv GXRBV1S3MGLFHRTggfr1n+bQXIfv4kJjcipMyko8QoInCscdz8Tvztv3/P+j9y7Qyw56 epEIIz+6/kjICGx36KlThNLvOoY7K9kAZ6Bm/a/dDQhoA9QGvRAlDpBZsf5DQ5Bu9qYW C5L7LG1DDvzQUpiPWw4lPsBMAxNqOVqLhDsqufaETL1dy717aSHQmVxvIwoT0jKCIxbB RQjA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:subject:message-id:mail-followup-to :references:mime-version:content-disposition :content-transfer-encoding:in-reply-to:user-agent; bh=uW4hf0feAaDN42e74dAXJwsPGTKB5QIipXgzlX10bU4=; b=pMhiEvuMqjEZPUhbZ1kCwgkQLku+0L0kJLiRZKs6nxdurS4r1G//UIPFvYVdwg1nHr W2oj5YQuvlaw0ISAQSEE6+kioIlwpXebG9n3EIieVPsSl2bpS+tFAvXssSH4q7LS7+at Ux68O322j0ndPhkOFceh21T6ezEr23aLpsOfEq1r4KWqesTX+UJqh6NIROCxcbCjE50q 8DbjuoMPGZhf1uyZH7d3rEHcMNo0V7pcMcdIz4ryoTp0CsPfxFw9k5ChW84Ag0yN2qsA XK1Yr1x13YlSU1btMmCL+dtXa5tJEUox2bbyLjbJmo0atqd3KXpXpVFCyhXpuTDQmPvG re3Q== X-Gm-Message-State: APt69E3n+XReptjWuMzh9MALTcnKOkGX0y/E9Bp8xUCIsxWvhvGxrCYH EJDdlT5gI2jWnGsEqPEzFkFrp5jc X-Google-Smtp-Source: ADUXVKLP2IgqnXsf0AdZj595TZFfYNq7fFg0HbVV03JEnMei6vUDfn0Wu0emU72Bb68uFkjcZXkDWA== X-Received: by 2002:a1c:d905:: with SMTP id q5-v6mr7069440wmg.78.1528213279338; Tue, 05 Jun 2018 08:41:19 -0700 (PDT) Date: Tue, 5 Jun 2018 18:41:27 +0300 From: Doron Behar To: zsh-workers@zsh.org Subject: Re: [PATCH 1/1] Squashed commit of the following: Message-ID: <20180605154121.tfrw6oheq3ce6qkr@NUC.doronbehar.com> Mail-Followup-To: zsh-workers@zsh.org References: <20180526161403.4860-1-doron.behar@gmail.com> <20180526161403.4860-2-doron.behar@gmail.com> <5942.1527504539@thecus> <20180529153821.nmwa5yojlusioxti@NUC.doronbehar.com> <13666.1527631249@thecus> <20180530124707.xp6loetwnplkypk2@NUC.doronbehar.com> <16604.1527694987@thecus> <20180601071850.4z3h6cvkuvva3nby@NUC.doronbehar.com> <20180601133048.m7crvdzodzntxcsq@NUC.doronbehar.com> <31972.1527893117@thecus> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <31972.1527893117@thecus> User-Agent: NeoMutt/20180512 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