Hi Alexandr!

Thanks for the review! I have fixed all nits and rewrite the commit message.

Fix the return value of box.info:memory(). It used to return the same table as the box.info(). | box.info.memory() = box.info[“memory”]() | box.info.memory() -> getmetatable(box.info.memory).__call(box.info.memory) | box.info:memory() = box.info[“memory”](box.info) | box.info:memory() -> getmetatable(box.info...).__call(box.info.., box.info) See https://www.lua.org/manual/5.1/manual.html#2.8. An extra argument on Lua stack in the second case is box.info table with __serialize metamethod. When all internal functions are over, memory table is pushed on the stack. When console prints the output (converts lua table to yaml format and prints) __serialize metamethod of box.info is called. __serilize metamethod of box.info shows only some predefined fields and completely ignores memory table. To avoid this call, patch pushes an extra empty table on Lua stack. When console calls ymal.serializer to print lua table, box.info will be ignored. Closes #4688

09.07.2020 4:08, Alexander Turenko пишет:
Pasted the actual patch to comment in-place.

commit ad00de576ab0300a3e48be2cdda2bef5938eb40e
Author: Olga Arkhangelskaia <arkholga@tarantool.org>
Date:   Mon Jun 29 12:14:24 2020 +0300

    box: fix box.info:memory()
    
    Fix the output of box.info:memory(). It used to return the same table as the
    box.info().
Nit: 72 symbols at max. Here and below.

Nit: I would say 'the return value', because 'the output' looks more
fixed

    
    Any box.info.xxx() is the same as box.info[“xxx”]().
    E.g. box.info.memory() ->
    getmetatable(box.info.memory).__call(box.info.memory)[1]
It is ambiguous: whether [1] is first element or an array or a reference
for the link below.
fixed

    After __index and __call metamethods, the final function that fills xxx-table,
    has the only argument - empty table to fill.
    When box.info:xxx() is invoked it automatically passes one argument:
    box.info[“xxx”](box.info). So the resulting call has 2 arguments on the stack.
    box.info:xxx()->getmetatable(box.info.xxx).__call(box.info.xxx, box.info)
Nit: I would surround '->' with spaces for readability.
fixed

      
    When function tries to fill box.info table - __call metamethod of box.info is
    trigged.
It is not correct. It would be __newindex metamethod, but it is not
defined on the table. I guess you was misguided by a console output,
because of __serialize method. In fact <box.info> table will be filled
with 'cache', 'lua', 'data' and other box.info.memory() fields. You can
verify it youself:
fixed

 | tarantool> setmetatable(box.info:memory(), nil)
 | ---
 | - cache: 0
 |   lua: 1076262
 |   data: 37816
 |   index: 1097728
 |   net: 589824
 |   tx: 0
 |   version: 2.5.0-208-gcf6975793
 |   package: Tarantool
 | ...

Despite changes I requested above I appecitate the intention to clarify
the change.

    
    box.info.gc does not have this problem because of an extra table that is
    created in the beginning of the bottom function. box.info.memory follows
    the same way.
    
    [1] https://www.lua.org/manual/5.1/manual.html#2.8
Changed

      
Nit: Markdown provides '[1]: https://' syntax for reference style links,
but mayble there are others markups, where syntax is the same as above.
I don't know for sure. Personally I use markdown (but sometimes with
asciidoc titles) for texts with several simple markup elements like
hyperlinks. Many developers aware of this syntax.

    
    Closes 4688
Typo: no hash symbol.
Fixed

      
diff --git a/src/box/lua/info.c b/src/box/lua/info.c
index d0e553b1d..3d515ae9e 100644
--- a/src/box/lua/info.c
+++ b/src/box/lua/info.c
@@ -322,6 +322,7 @@ lbox_info_memory_call(struct lua_State *L)
 	struct engine_memory_stat stat;
 	engine_memory_stat(&stat);
 
+	lua_newtable(L);
Nit: Six same structured blocks are below. But after the change the
first one will differs. Please, add an empty line after lua_newtable().
added an epmty line.

BTW, we can use lua_createtable() to allocate a hashmap of necessary
size before inserting elements. It is to avoid resizing of the table
(don't know whether it is actual for small map sizes like 6).

The change itself is okay for me.

 	lua_pushstring(L, "data");
 	luaL_pushuint64(L, stat.data);
 	lua_settable(L, -3);
diff --git a/test/box-tap/gh-4688-box-info-memory.test.lua b/test/box-tap/gh-4688-box-info-memory.test.lua
new file mode 100755
index 000000000..63dcdab8f
--- /dev/null
+++ b/test/box-tap/gh-4688-box-info-memory.test.lua
@@ -0,0 +1,15 @@
+#!/usr/bin/env tarantool
+--
+-- gh-4688: box.info:memory() displayed full content of box.info
+--
+local tap = require('tap')
+local test = tap.test("Tarantool 4688")
Nit: Single and double quotes are used without any system.

Nit: See how other top level test cases are named: `grep tap.test
test/*/gh-*`.

      
+test:plan(1)
+
+box.cfg()
+
+a = box.info.memory()
+b = box.info:memory()
+
+test:is(table.concat(a), table.concat(b), "box.info:memory")
First, 'lua' values likely will be different. Second, table.concat()
concatenates elements of an array like {'x', 'y', 'z'}. It is not for
maps (it just gives an empty string). The result is that the test passes
ever without the fix.

Most obvious way would be using of test:is_deeply(), but since 'lua'
field may vary, we can do one of the following ways:

 | a.lua = a.lua and '<stripped>' or nil
 | b.lua = b.lua and '<stripped>' or nil
 |
 | test:is_deeply(a, b, 'box.info:memory() is the same as box.info.memory()')

Or

 | local function get_keys(t)
 |     local keys = {}
 |     for k, v in pairs(t) do
 |         table.insert(keys, k)
 |     end
 |     return keys
 | end
 |
 | local keys_1 = get_keys(box.info.memory())
 | local keys_2 = get_keys(box.info:memory())
 | test:is_deeply(keys_1, keys_2, <...>)
New test:                                                                                 
 --                                                                                
 -- gh-4688: box.info:memory() displayed full content of box.info                  
 --                                                                                
 local tap = require('tap')                                                        
-local test = tap.test("Tarantool 4688")                                           
-test:plan(1)                                                                      
~                                                                                  
 box.cfg()                                                                         
~                                                                                  
-a = box.info.memory()                                                             
-b = box.info:memory()                                                             
+local test = tap.test('gh-4688-box.info:memory-wrong-result')                     
+test:plan(1)                                                                      
+                                                                                  
+local a = box.info.memory()                                                       
+local b = box.info:memory()                                                       
+                                                                                  
+local function get_keys(t)                                                        
+    local keys = {}                                                               
+    for k, v in pairs(t) do                                                       
+        table.insert(keys, k)                                                     
+    end                                                                           
+    return keys                                                                   
+end                                                                               
+                                                                                  
+local keys_1 = get_keys(box.info.memory())                                        
+local keys_2 = get_keys(box.info:memory())                                        
+test:is_deeply(keys_1, keys_2, "box.info:memory coincide with box.info.memory")
~                                                                                  
-test:is(table.concat(a), table.concat(b), "box.info:memory")                      
-os.exit(0)                                                                        
+os.exit(test:check() and 0 or 1)     

Feel free to use any variant or provide your own.

+os.exit(0)
Please, set exit code appropriately (see [1]).

[1]: https://github.com/tarantool/doc/issues/1004