[tarantool-patches] Re: [PATCH] tarantoolctl: remove metatable assumptions in start()

[Roman Khabibov <roman.habibov@tarantool.org>]

Hi! Thanks for the review.

> On Apr 11, 2019, at 5:57 PM, Alexander Turenko <alexander.turenko@tarantool.org> wrote:
> 
> The behaviour is almost okay, but you need to work on wording of the
> commit message to make it useful for a reader. Feel free to ask more if
> you have questions.
> 
> See comments below.
> 
> WBR, Alexander Turenko.
> 
>> commit 4cdf8e79bb530288098fb30e5014498fc0ad4262
>> Author: Roman Khabibov <roman.habibov@tarantool.org>
>> Date:   Mon Mar 25 20:52:05 2019 +0300
>> 
>>    tarantoolctl: error when box.cfg didn't called
> 
> Cite from our developer guidelines [1]:
> 
>> 6. Use the imperative mood in the subject line. A properly formed Git
>> commit subject line should always be able to complete the following
>> sentence: “If applied, this commit will /your subject line here/”.
> 
> [1]: https://www.tarantool.io/en/doc/1.10/dev_guide/developer_guidelines/#how-to-write-a-commit-message
> 
> Also you construct passive voice wrongly, it should be 'to be + [not] + Ved'.
> 
>> 
>>    There was assumpted uninitialized metamethods of box.cfg{} if user
>>    did't call it in his instance.
> 
> This sentence raises more questions then give answers. What is
> 'uninitialized metamethods'? Whether 'of box.cfg{}' means result of the
> call or a new value of the 'cfg' field of the 'box' table?
> 
> Also I doubt 'There was assumpted' is valid phrase.

    tarantoolctl: raise error when box.cfg isn't called
    
    There was no check whether box.cfg{} was initialized in an
    instance. If so, an error should be raised.

>> 
>>    Closes #3953
>> 
>> diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in
>> index 47fcf895f..396fa7957 100755
>> --- a/extra/dist/tarantoolctl.in
>> +++ b/extra/dist/tarantoolctl.in
>> @@ -547,7 +547,11 @@ local function start()
>>         end
>>         os.exit(1)
>>     end
>> -    local old_call = getmetatable(box.cfg).__call
>> +    local m_table = getmetatable(box.cfg)
>> +    if m_table == nil then
> 
> Cite from our Lua style guide [2]:
> 
>> `*_mt` and `*_methods` defines metatable and methods table
> 
> Suggested to name it box_cfg_mt.
+    local box_cfg_mt = getmetatable(box.cfg)

> [2]: https://www.tarantool.io/en/doc/1.10/dev_guide/lua_style_guide/
> 
>> +        error("Plaese, call box.cfg{} within your instance")
> 
> Typo: 'Plaese'.
> 
> I would change wording to just state what is wrong, e.g.: 'box.cfg() is
> not called in an instance file'.
> 
> See also how errors are reported in other places: log.error() and
> os.exit(1). I guess it does not matter much from a behaviour point of
> view, but it would be good to report errors in one way with surrounding
> code.
> 
> (BTW, found one behaviour difference: error() reports a line number when
> called with one argument or non-zero second argument.)
> 
> Please, install tarantool and tarantoolctl and check the behaviour:
> messages and exit codes. Currently it looks so:
> 
> # tarantoolctl start foo
> Starting instance foo...
> Hello, World!
> /usr/bin/tarantoolctl:552: Plaese, call box.cfg{} within your instance
> # echo $?
> 1
> 
> It is okay except file:line part, but anyway I suggest you to use
> log.error() and os.exit(1).
+    if box_cfg_mt == nil then
+        log.error('box.cfg() is not called in an instance file')
+        os.exit(1)

> 
>>         if old_cfg.pid_file ~= nil and cfg ~= nil and cfg.pid_file ~= nil then
>>             cfg.pid_file = old_cfg.pid_file
>> diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua
>> index db046e03f..428b074e2 100755
>> --- a/test/app-tap/tarantoolctl.test.lua
>> +++ b/test/app-tap/tarantoolctl.test.lua
>> @@ -165,10 +165,13 @@ do
>>     local dir = fio.tempdir()
>>     local code = [[ box.cfg{memtx_memory = 104857600} ]]
>>     create_script(dir, 'script.lua', code)
>> +    create_script(dir, 'init.lua', [[ print('Hi!') ]])
>> 
>>     local status, err = pcall(function()
>>         test:test("basic test", function(test_i)
>> -            test_i:plan(16)
>> +            test_i:plan(18)
>> +            check_ok(test_i, dir, 'start', 'init.lua', 1, nil, "Starting instance init...",
>> +                     "Plaese, call box.cfg{} within your instance")
>>             check_ok(test_i, dir, 'start',  'script', 0, nil, "Starting instance")
>>             tctl_wait_start(dir, 'script')
>>             check_ok(test_i, dir, 'status', 'script', 0, nil, "is running")
>> 
>> 
> 
> I propose to make the code of the test a bit more uniform and use a
> meaningful instance file name:
> 
> diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua
> index 428b074e2..7d7c63371 100755
> --- a/test/app-tap/tarantoolctl.test.lua
> +++ b/test/app-tap/tarantoolctl.test.lua
> @@ -163,15 +163,14 @@ test:plan(7)
> -- must be stopped afterwards
> do
>     local dir = fio.tempdir()
> -    local code = [[ box.cfg{memtx_memory = 104857600} ]]
> -    create_script(dir, 'script.lua', code)
> -    create_script(dir, 'init.lua', [[ print('Hi!') ]])
> +    create_script(dir, 'script.lua', [[ box.cfg{memtx_memory = 104857600} ]])
> +    create_script(dir, 'no_box_cfg.lua', [[ print('Hi!') ]])
> 
>     local status, err = pcall(function()
>         test:test("basic test", function(test_i)
>             test_i:plan(18)
> -            check_ok(test_i, dir, 'start', 'init.lua', 1, nil, "Starting instance init...",
> -                     "Plaese, call box.cfg{} within your instance")
> +            check_ok(test_i, dir, 'start', 'no_box_cfg', 1, nil, "Starting instance",
> +                     "box.cfg() is not called in an instance file")
>             check_ok(test_i, dir, 'start',  'script', 0, nil, "Starting instance")
>             tctl_wait_start(dir, 'script')
>             check_ok(test_i, dir, 'status', 'script', 0, nil, "is running”)
-    local code = [[ box.cfg{memtx_memory = 104857600} ]]
-    create_script(dir, 'script.lua', code)
+    create_script(dir, 'script.lua', [[ box.cfg{memtx_memory = 104857600} ]])
+    create_script(dir, 'no_box_cfg.lua', [[ print('Hi!') ]])
 
     local status, err = pcall(function()
         test:test("basic test", function(test_i)
-            test_i:plan(16)
+            test_i:plan(18)
+            check_ok(test_i, dir, 'start', 'no_box_cfg', 1, nil, "Starting instance",
+                     "box.cfg() is not called in an instance file”)

commit 834545cc83965f6d612fcccd39dda03074e7f227
Author: Roman Khabibov <roman.habibov@tarantool.org>
Date:   Mon Mar 25 20:52:05 2019 +0300

    tarantoolctl: raise error when box.cfg isn't called
    
    There was no check whether box.cfg{} was initialized in an
    instance. If so, an error should be raised.
    
    Closes #3953

diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in
index 47fcf895f..37ded5333 100755
--- a/extra/dist/tarantoolctl.in
+++ b/extra/dist/tarantoolctl.in
@@ -547,8 +547,13 @@ local function start()
         end
         os.exit(1)
     end
-    local old_call = getmetatable(box.cfg).__call
-    getmetatable(box.cfg).__call = function(old_cfg, cfg)
+    local box_cfg_mt = getmetatable(box.cfg)
+    if box_cfg_mt == nil then
+        log.error('box.cfg() is not called in an instance file')
+        os.exit(1)
+    end
+    local old_call = box_cfg_mt.__call
+    box_cfg_mt.__call = function(old_cfg, cfg)
         if old_cfg.pid_file ~= nil and cfg ~= nil and cfg.pid_file ~= nil then
             cfg.pid_file = old_cfg.pid_file
         end
diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua
index db046e03f..7d7c63371 100755
--- a/test/app-tap/tarantoolctl.test.lua
+++ b/test/app-tap/tarantoolctl.test.lua
@@ -163,12 +163,14 @@ test:plan(7)
 -- must be stopped afterwards
 do
     local dir = fio.tempdir()
-    local code = [[ box.cfg{memtx_memory = 104857600} ]]
-    create_script(dir, 'script.lua', code)
+    create_script(dir, 'script.lua', [[ box.cfg{memtx_memory = 104857600} ]])
+    create_script(dir, 'no_box_cfg.lua', [[ print('Hi!') ]])
 
     local status, err = pcall(function()
         test:test("basic test", function(test_i)
-            test_i:plan(16)
+            test_i:plan(18)
+            check_ok(test_i, dir, 'start', 'no_box_cfg', 1, nil, "Starting instance",
+                     "box.cfg() is not called in an instance file")
             check_ok(test_i, dir, 'start',  'script', 0, nil, "Starting instance")
             tctl_wait_start(dir, 'script')
             check_ok(test_i, dir, 'status', 'script', 0, nil, "is running")