Re: [Tarantool-patches] [PATCH 2/3] Add a black list of the tarantoolctl options

[lvasiliev <lvasiliev@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
>>