From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp49.i.mail.ru (smtp49.i.mail.ru [94.100.177.109]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 0EEEC4696C3 for ; Sun, 5 Apr 2020 18:11:11 +0300 (MSK) References: <3903e055-5853-6606-7fd1-87b5a4c60662@tarantool.org> From: lvasiliev Message-ID: <59b62abf-5860-84fe-5277-df4c84595f9b@tarantool.org> Date: Sun, 5 Apr 2020 18:11:10 +0300 MIME-Version: 1.0 In-Reply-To: <3903e055-5853-6606-7fd1-87b5a4c60662@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 2/3] Add a black list of the tarantoolctl options List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the review. See PATCH v2 and comments bellow. On 04.04.2020 22:02, Vladislav Shpilevoy wrote: > Thanks for the patch! > > See 3 comments below. > > On 25/03/2020 22:50, Leonid Vasiliev wrote: >> Luarocks code style has been used > > 1. This message does not give any useful info. Better explain, why > did you add the list to the luarocks instead of to tarantoolctl. It's about changes in tarantool and luaroks code style) About black list. The main goal of the task: "The point of the task is refuse from double parsing of luarocks options (first in tarantoolctl and second in luarocks module), cleanup help and replace a white-list filter (most options are prohibited) to a black-list filter (most options allowed). So, if the functionality is work at luarocks, it must work at tarantoolctl rocks too" We don't want double parsing. And current parsing in tarantoolctl doesn't take into account matching commands with options. > >> --- >> src/luarocks/util.lua | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/src/luarocks/util.lua b/src/luarocks/util.lua >> index 8ccda27..0172d08 100644 >> --- a/src/luarocks/util.lua >> +++ b/src/luarocks/util.lua >> @@ -171,6 +171,14 @@ local supported_flags = { >> ["version"] = true, >> } >> >> +-- The tarantool unsupported arguments list. >> +local tarantool_black_list = { >> + ["global"] = true, >> + ["local"] = true, >> + ["lua-version"] = true, >> + ["lua-dir"] = true, > > 2. Why can't you just remove these flags from 'supported_flags'? > Or make them false there? Because the black-filter is more clearly. Easy to see the tarantool exceptions. > >> +} >> + >> --- Extract flags from an arguments list. >> -- Given string arguments, extract flag arguments into a flags set. >> -- For example, given "foo", "--tux=beep", "--bla", "bar", "--baz", >> @@ -189,6 +197,9 @@ function util.parse_flags(...) >> elseif state == "initial" and flag then >> local var,val = flag:match("([a-z_%-]*)=(.*)") >> if val then >> + if tarantool_black_list[var] then >> + return { ERROR = "Invalid argument: flag --"..var.." is not supported by tarantoolctl roks." } > > 3. Probably 'rocks' instead of 'roks'? The same below. Fixed > >> + end >> local vartype = supported_flags[var] >> if type(vartype) == "string" then >> if val == "" and vartype:sub(1,1) ~= '"' then >> @@ -204,6 +215,9 @@ function util.parse_flags(...) >> end >> else >> local var = flag >> + if tarantool_black_list[var] then >> + return { ERROR = "Invalid argument: flag --"..var.." is not supported by tarantoolctl roks." } >> + end >> local vartype = supported_flags[var] >> if type(vartype) == "string" then >> i = i + 1 >>