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

[Alexander Turenko]

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

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

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

> +    end
> +    local old_call = m_table.__call
>      getmetatable(box.cfg).__call = function(old_cfg, cfg)

There is no need to call getmetatable() again, we can use box_cfg_mt.

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