[tarantool-patches] [PATCH v2 0/3] lua-yaml null/boolean fixes

[tarantool-patches] [PATCH v2 0/3] lua-yaml null/boolean fixes

[Alexander Turenko]

https://github.com/tarantool/tarantool/issues/3476
https://github.com/tarantool/tarantool/issues/3662
https://github.com/tarantool/tarantool/issues/3583
https://github.com/tarantool/tarantool/tree/kh/gh-3662-yaml-2.1

AKhatskevich (1):
  lua-yaml: verify arguments count

Alexander Turenko (2):
  lua-yaml: fix boolean/null representation in yaml
  lua-yaml: treat an empty document/value as null

 test/app-tap/console.test.lua   |   37 +-
 test/app/fio.result             |    2 +-
 test/app/socket.result          |   26 +-
 test/box/access.result          |    8 +-
 test/box/misc.result            |    2 +-
 test/box/role.result            |    8 +-
 test/box/sequence.result        |    2 +-
 test/vinyl/errinj_tx.result     |   10 +-
 test/vinyl/hermitage.result     |  122 ++--
 test/vinyl/mvcc.result          | 1066 +++++++++++++++----------------
 test/vinyl/mvcc.test.lua        |    4 +-
 test/vinyl/tx_conflict.result   |    2 +-
 test/vinyl/tx_conflict.test.lua |    2 +-
 test/vinyl/tx_gap_lock.result   |  189 +++---
 test/vinyl/tx_gap_lock.test.lua |    3 +-
 test/vinyl/tx_serial.result     |    2 +-
 test/vinyl/tx_serial.test.lua   |    2 +-
 third_party/lua-yaml/lyaml.cc   |   87 ++-
 18 files changed, 819 insertions(+), 755 deletions(-)

-- 
2.20.1

[tarantool-patches] [PATCH v2 1/3] lua-yaml: verify arguments count

[Alexander Turenko]

From: AKhatskevich <avkhatskevich@tarantool.org>

Added arguments count check for yaml.encode() and decode.decode()
functions.

Without these checks the functions could read garbage outside of a Lua
stack when called w/o arguments.
---
 third_party/lua-yaml/lyaml.cc | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc
index c6d118a79..9b07992d8 100644
--- a/third_party/lua-yaml/lyaml.cc
+++ b/third_party/lua-yaml/lyaml.cc
@@ -400,7 +400,8 @@ static void load(struct lua_yaml_loader *loader) {
  */
 static int l_load(lua_State *L) {
    struct lua_yaml_loader loader;
-   if (! lua_isstring(L, 1)) {
+   int top = lua_gettop(L);
+   if (!(top == 1 || top == 2) || !lua_isstring(L, 1)) {
 usage_error:
       return luaL_error(L, "Usage: yaml.decode(document, "\
                         "[{tag_only = boolean}])");
@@ -416,7 +417,7 @@ usage_error:
       return luaL_error(L, OOM_ERRMSG);
    yaml_parser_set_input_string(&loader.parser, (yaml_char_t *) document, len);
    bool tag_only;
-   if (lua_gettop(L) > 1) {
+   if (lua_gettop(L) == 2) {
       if (! lua_istable(L, 2))
          goto usage_error;
       lua_getfield(L, 2, "tag_only");
@@ -794,7 +795,7 @@ error:
 static int l_dump(lua_State *L) {
    struct luaL_serializer *serializer = luaL_checkserializer(L);
    int top = lua_gettop(L);
-   if (top > 2) {
+   if (!(top == 1 || top == 2)) {
 usage_error:
       return luaL_error(L, "Usage: encode(object, {tag_prefix = <string>, "\
                         "tag_handle = <string>})");
-- 
2.20.1

[tarantool-patches] Re: [PATCH v2 1/3] lua-yaml: verify arguments count

[Vladislav Shpilevoy]

Thanks for the patch!

On 22/01/2019 05:12, Alexander Turenko wrote:
> From: AKhatskevich <avkhatskevich@tarantool.org>
> 
> Added arguments count check for yaml.encode() and decode.decode()

Typo: 'decode.decode' -> 'yaml.decode'.

> functions.
> 
> Without these checks the functions could read garbage outside of a Lua
> stack when called w/o arguments.

Honestly, I do not understand how is it possible. Please,
provide a test for both functions. See my 3 doubts below.

> ---
>   third_party/lua-yaml/lyaml.cc | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc
> index c6d118a79..9b07992d8 100644
> --- a/third_party/lua-yaml/lyaml.cc
> +++ b/third_party/lua-yaml/lyaml.cc
> @@ -400,7 +400,8 @@ static void load(struct lua_yaml_loader *loader) {
>    */
>   static int l_load(lua_State *L) {
>      struct lua_yaml_loader loader;
> -   if (! lua_isstring(L, 1)) {
> +   int top = lua_gettop(L);
> +   if (!(top == 1 || top == 2) || !lua_isstring(L, 1)) {

1. How could the old code lead to a bug, if there was a
check if the first argument is a string? The second argument
is not used until the next hunk, about which see my next
comment

>   usage_error:
>         return luaL_error(L, "Usage: yaml.decode(document, "\
>                           "[{tag_only = boolean}])");
> @@ -416,7 +417,7 @@ usage_error:
>         return luaL_error(L, OOM_ERRMSG);
>      yaml_parser_set_input_string(&loader.parser, (yaml_char_t *) document, len);
>      bool tag_only;
> -   if (lua_gettop(L) > 1) {
> +   if (lua_gettop(L) == 2) {

2. This function never touches anything beyond second value on
the stack, so here lua_gettop(L) > 1 means the same as
lua_gettop(L) == 2 - the second argument exist. Third and next
values do not matter.

>         if (! lua_istable(L, 2))
>            goto usage_error;
>         lua_getfield(L, 2, "tag_only");
> @@ -794,7 +795,7 @@ error:
>   static int l_dump(lua_State *L) {
>      struct luaL_serializer *serializer = luaL_checkserializer(L);
>      int top = lua_gettop(L);
> -   if (top > 2) {
> +   if (!(top == 1 || top == 2)) {

3. Here my reasoning is the same - the previous checking works
as well.

>   usage_error:
>         return luaL_error(L, "Usage: encode(object, {tag_prefix = <string>, "\
>                           "tag_handle = <string>})");
> 

[tarantool-patches] Re: [PATCH v2 1/3] lua-yaml: verify arguments count

[Alexander Turenko]

Hi!

See answers below.

I'll send the updated patchset as v3 soon.

WBR, Alexander Turenko.

On Fri, Jan 25, 2019 at 12:26:53AM +0300, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> On 22/01/2019 05:12, Alexander Turenko wrote:
> > From: AKhatskevich <avkhatskevich@tarantool.org>
> > 
> > Added arguments count check for yaml.encode() and decode.decode()
> 
> Typo: 'decode.decode' -> 'yaml.decode'.

Thx. Fixed.

> 
> > functions.
> > 
> > Without these checks the functions could read garbage outside of a Lua
> > stack when called w/o arguments.
> 
> Honestly, I do not understand how is it possible. Please,
> provide a test for both functions. See my 3 doubts below.

lua_isstring(L, 1) checks a garbage w/o preliminary lua_gettop() check.
yaml.encode() gives me "unsupported Lua type 'thread'" on the current
tarantool 2.1.

Anyway, added bad API usage test cases. Also I changed this:

diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc
index 3a427263e..46374970f 100644
--- a/third_party/lua-yaml/lyaml.cc
+++ b/third_party/lua-yaml/lyaml.cc
@@ -453,7 +453,7 @@ usage_error:
       return luaL_error(L, OOM_ERRMSG);
    yaml_parser_set_input_string(&loader.parser, (yaml_char_t *) document, len);
    bool tag_only;
-   if (lua_gettop(L) == 2) {
+   if (lua_gettop(L) == 2 && ! lua_isnil(L, 2)) {
       if (! lua_istable(L, 2))
          goto usage_error;
       lua_getfield(L, 2, "tag_only");

We should not raise an usage error for yaml.decode(object, nil).

> 
> > ---
> >   third_party/lua-yaml/lyaml.cc | 7 ++++---
> >   1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc
> > index c6d118a79..9b07992d8 100644
> > --- a/third_party/lua-yaml/lyaml.cc
> > +++ b/third_party/lua-yaml/lyaml.cc
> > @@ -400,7 +400,8 @@ static void load(struct lua_yaml_loader *loader) {
> >    */
> >   static int l_load(lua_State *L) {
> >      struct lua_yaml_loader loader;
> > -   if (! lua_isstring(L, 1)) {
> > +   int top = lua_gettop(L);
> > +   if (!(top == 1 || top == 2) || !lua_isstring(L, 1)) {
> 
> 1. How could the old code lead to a bug, if there was a
> check if the first argument is a string? The second argument

It does not check a stack top (arguments count).

> is not used until the next hunk, about which see my next
> comment

As I see it is usual way to write such functions: check whether count of
arguments and types of mandatory arguments are valid at start of the
function.

> 
> >   usage_error:
> >         return luaL_error(L, "Usage: yaml.decode(document, "\
> >                           "[{tag_only = boolean}])");
> > @@ -416,7 +417,7 @@ usage_error:
> >         return luaL_error(L, OOM_ERRMSG);
> >      yaml_parser_set_input_string(&loader.parser, (yaml_char_t *) document, len);
> >      bool tag_only;
> > -   if (lua_gettop(L) > 1) {
> > +   if (lua_gettop(L) == 2) {
> 
> 2. This function never touches anything beyond second value on
> the stack, so here lua_gettop(L) > 1 means the same as
> lua_gettop(L) == 2 - the second argument exist. Third and next
> values do not matter.

I read this as 'those are equivalent' (correct me if I'm wrong). Ok. I'd
prefer to leave it with ==. Also note the fix I pasted above.

> 
> >         if (! lua_istable(L, 2))
> >            goto usage_error;
> >         lua_getfield(L, 2, "tag_only");
> > @@ -794,7 +795,7 @@ error:
> >   static int l_dump(lua_State *L) {
> >      struct luaL_serializer *serializer = luaL_checkserializer(L);
> >      int top = lua_gettop(L);
> > -   if (top > 2) {
> > +   if (!(top == 1 || top == 2)) {
> 
> 3. Here my reasoning is the same - the previous checking works
> as well.

It will not give an error in case of yaml.encode() and yaml.encode({},
{}, {}).

> 
> >   usage_error:
> >         return luaL_error(L, "Usage: encode(object, {tag_prefix = <string>, "\
> >                           "tag_handle = <string>})");
> > 

[tarantool-patches] Re: [PATCH v2 1/3] lua-yaml: verify arguments count

[Vladislav Shpilevoy]

Hi! Thanks for the fixes!

>>> functions.
>>>
>>> Without these checks the functions could read garbage outside of a Lua
>>> stack when called w/o arguments.
>>
>> Honestly, I do not understand how is it possible. Please,
>> provide a test for both functions. See my 3 doubts below.
> 
> lua_isstring(L, 1) checks a garbage w/o preliminary lua_gettop() check.
> yaml.encode() gives me "unsupported Lua type 'thread'" on the current
> tarantool 2.1.

I looked at lua_isstring implementation, and I see, that it checks
top. If an index is above top, then the type is nil.

	static TValue *index2adr(lua_State *L, int idx)
	{
	  if (idx > 0) {
	    TValue *o = L->base + (idx - 1);
	    return o < L->top ? o : niltv(L);
	...

> 
> Anyway, added bad API usage test cases. Also I changed this:
> 
> diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc
> index 3a427263e..46374970f 100644
> --- a/third_party/lua-yaml/lyaml.cc
> +++ b/third_party/lua-yaml/lyaml.cc
> @@ -453,7 +453,7 @@ usage_error:
>         return luaL_error(L, OOM_ERRMSG);
>      yaml_parser_set_input_string(&loader.parser, (yaml_char_t *) document, len);
>      bool tag_only;
> -   if (lua_gettop(L) == 2) {
> +   if (lua_gettop(L) == 2 && ! lua_isnil(L, 2)) {
>         if (! lua_istable(L, 2))
>            goto usage_error;
>         lua_getfield(L, 2, "tag_only");
> 
> We should not raise an usage error for yaml.decode(object, nil).

Why? It is said, that the second value either does not exist, or
is a table. Nil is not a table. So why? If your logic was about
considering nil as a not existing value, then why don't we handle
cases like this: yaml.decode(object, nil, nil, nil, nil) ? The same
for l_dump() and encode.

>>>    usage_error:
>>>          return luaL_error(L, "Usage: yaml.decode(document, "\
>>>                            "[{tag_only = boolean}])");
>>> @@ -416,7 +417,7 @@ usage_error:
>>>          return luaL_error(L, OOM_ERRMSG);
>>>       yaml_parser_set_input_string(&loader.parser, (yaml_char_t *) document, len);
>>>       bool tag_only;
>>> -   if (lua_gettop(L) > 1) {
>>> +   if (lua_gettop(L) == 2) {
>>
>> 2. This function never touches anything beyond second value on
>> the stack, so here lua_gettop(L) > 1 means the same as
>> lua_gettop(L) == 2 - the second argument exist. Third and next
>> values do not matter.
> 
> I read this as 'those are equivalent' (correct me if I'm wrong). Ok. I'd
> prefer to leave it with ==. Also note the fix I pasted above.

Why? Again. I do not see any reason behind this change except personal
preference. I reverted all the changes about l_load() function, and the
tests passed. So why do we need to make diff bigger?

> 
>>
>>>          if (! lua_istable(L, 2))
>>>             goto usage_error;
>>>          lua_getfield(L, 2, "tag_only");
>>> @@ -794,7 +795,7 @@ error:
>>>    static int l_dump(lua_State *L) {
>>>       struct luaL_serializer *serializer = luaL_checkserializer(L);
>>>       int top = lua_gettop(L);
>>> -   if (top > 2) {
>>> +   if (!(top == 1 || top == 2)) {
>>
>> 3. Here my reasoning is the same - the previous checking works
>> as well.
> 
> It will not give an error in case of yaml.encode() and yaml.encode({},
> {}, {}).

Decent. Here you are right.

> 
>>
>>>    usage_error:
>>>          return luaL_error(L, "Usage: encode(object, {tag_prefix = <string>, "\
>>>                            "tag_handle = <string>})");
>>>

My diff, which reverts some changes and makes this patch one-liner:

diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc
index 354cafe86..854794dd1 100644
--- a/third_party/lua-yaml/lyaml.cc
+++ b/third_party/lua-yaml/lyaml.cc
@@ -400,8 +400,7 @@ static void load(struct lua_yaml_loader *loader) {
   */
  static int l_load(lua_State *L) {
     struct lua_yaml_loader loader;
-   int top = lua_gettop(L);
-   if (!(top == 1 || top == 2) || !lua_isstring(L, 1)) {
+   if (! lua_isstring(L, 1)) {
  usage_error:
        return luaL_error(L, "Usage: yaml.decode(document, "\
                          "[{tag_only = boolean}])");
@@ -417,7 +416,7 @@ usage_error:
        return luaL_error(L, OOM_ERRMSG);
     yaml_parser_set_input_string(&loader.parser, (yaml_char_t *) document, len);
     bool tag_only;
-   if (lua_gettop(L) == 2 && ! lua_isnil(L, 2)) {
+   if (lua_gettop(L) > 1) {
        if (! lua_istable(L, 2))
           goto usage_error;
        lua_getfield(L, 2, "tag_only");

[tarantool-patches] Re: [PATCH v2 1/3] lua-yaml: verify arguments count

[Alexander Turenko]

lua_is* really checks whether an acceptable index is a valid one, so
there are two possible approaches, one of which we should stick I think:

* Verify lua_gettop() upper and lower bounds right at start of a
  function.
* Use lua_is* (including lua_isnone() and lua_isnoneornil()) and don't
  verify arguments count explicitly.

I think we should use one of these ways within a module: this is more
important then the patch size. The only difference for a user is that
the latter approach does not check for extra arguments.

Now I implemented the latter approach as I see you want to minimize
explicit checks. See the patch at end of the email.

It is possible to reduce the patch further, but loss consistency in what
we check: lua_is* or lua_gettop(). I'll do if you insist, but don't
think it is the right way to proceed.

NB: branch: kh/gh-3662-yaml-2.1

WBR, Alexander Turenko.

On Tue, Feb 05, 2019 at 10:36:41PM +0300, Vladislav Shpilevoy wrote:
> Hi! Thanks for the fixes!
> 
> > > > functions.
> > > > 
> > > > Without these checks the functions could read garbage outside of a Lua
> > > > stack when called w/o arguments.
> > > 
> > > Honestly, I do not understand how is it possible. Please,
> > > provide a test for both functions. See my 3 doubts below.
> > 
> > lua_isstring(L, 1) checks a garbage w/o preliminary lua_gettop() check.
> > yaml.encode() gives me "unsupported Lua type 'thread'" on the current
> > tarantool 2.1.
> 
> I looked at lua_isstring implementation, and I see, that it checks
> top. If an index is above top, then the type is nil.
> 
> 	static TValue *index2adr(lua_State *L, int idx)
> 	{
> 	  if (idx > 0) {
> 	    TValue *o = L->base + (idx - 1);
> 	    return o < L->top ? o : niltv(L);
> 	...
> 

Ouch, lua_isstring() is called only in case of top == 2, so this is out
of scope of the discussion. The real cause of this weird "unsupported
Lua type 'thread'" error is lua_yaml_encode() code: it calls
`lua_newthread(L)` and then `lua_pushvalue(L, 1);`. A 1st stack item
should be a value we encode, but when there are no arguments for
yaml.encode() the new lua thread is the 1st item.

Anyway, I don't think that "unsupported Lua type 'thread'" is the right
error message for `yaml.encode()`. Are you agree?

> > 
> > Anyway, added bad API usage test cases. Also I changed this:
> > 
> > diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc
> > index 3a427263e..46374970f 100644
> > --- a/third_party/lua-yaml/lyaml.cc
> > +++ b/third_party/lua-yaml/lyaml.cc
> > @@ -453,7 +453,7 @@ usage_error:
> >         return luaL_error(L, OOM_ERRMSG);
> >      yaml_parser_set_input_string(&loader.parser, (yaml_char_t *) document, len);
> >      bool tag_only;
> > -   if (lua_gettop(L) == 2) {
> > +   if (lua_gettop(L) == 2 && ! lua_isnil(L, 2)) {
> >         if (! lua_istable(L, 2))
> >            goto usage_error;
> >         lua_getfield(L, 2, "tag_only");
> > 
> > We should not raise an usage error for yaml.decode(object, nil).
> 
> Why? It is said, that the second value either does not exist, or
> is a table. Nil is not a table. So why? If your logic was about
> considering nil as a not existing value, then why don't we handle
> cases like this: yaml.decode(object, nil, nil, nil, nil) ? The same
> for l_dump() and encode.

There is the difference between `yaml.decode(object, nil)` and
`yaml.decode(object, nil, nil, nil, nil)`. The former one is likely to
appear due to passing though the 2nd argument, say:

```
local function load_cfg(raw, opts)
    local object = yaml.decode(raw, opts)
    ...some post-processing...
    return object
end
```

The latter is definitely wrong usage.

But now I removed checks for extra args, see above.

> 
> > > >    usage_error:
> > > >          return luaL_error(L, "Usage: yaml.decode(document, "\
> > > >                            "[{tag_only = boolean}])");
> > > > @@ -416,7 +417,7 @@ usage_error:
> > > >          return luaL_error(L, OOM_ERRMSG);
> > > >       yaml_parser_set_input_string(&loader.parser, (yaml_char_t *) document, len);
> > > >       bool tag_only;
> > > > -   if (lua_gettop(L) > 1) {
> > > > +   if (lua_gettop(L) == 2) {
> > > 
> > > 2. This function never touches anything beyond second value on
> > > the stack, so here lua_gettop(L) > 1 means the same as
> > > lua_gettop(L) == 2 - the second argument exist. Third and next
> > > values do not matter.
> > 
> > I read this as 'those are equivalent' (correct me if I'm wrong). Ok. I'd
> > prefer to leave it with ==. Also note the fix I pasted above.
> 
> Why? Again. I do not see any reason behind this change except personal
> preference.

It does not matter much, because I anyway need to add ` && !
lua_isnil(L, 2)` or use `! lua_isnoneornil(L, 2)` here to make decode
behaviour consistent with encode one (against 2nd argument). Yep, it is
personal preference. Anyway, now it is `! lua_isnoneornil(L, 2)`.

> I reverted all the changes about l_load() function, and the
> tests passed. So why do we need to make diff bigger?

yaml.decode('', nil, {}) don't pass before (don't raise an error).
Other tests are passed, because of two reasons:

* no test on yaml.decode('', nil);
* lua_isstring() checks stack size.

Re test: added for encode and decode.

Re lua_isstring(): okay, now I understood that it checks given index by
the API:

http://pgl.yoyo.org/luai/i/lua_isstring ("acceptable index")
https://www.lua.org/manual/5.3/manual.html#4.3 ("Valid and Acceptable Indices")
https://www.lua.org/manual/5.1/manual.html#3.2 (the same for Lua 5.1)

So I changed the description of the commit to make it clear that the
reason of the change is to make the code more consistent.

> 
> > 
> > > 
> > > >          if (! lua_istable(L, 2))
> > > >             goto usage_error;
> > > >          lua_getfield(L, 2, "tag_only");
> > > > @@ -794,7 +795,7 @@ error:
> > > >    static int l_dump(lua_State *L) {
> > > >       struct luaL_serializer *serializer = luaL_checkserializer(L);
> > > >       int top = lua_gettop(L);
> > > > -   if (top > 2) {
> > > > +   if (!(top == 1 || top == 2)) {
> > > 
> > > 3. Here my reasoning is the same - the previous checking works
> > > as well.
> > 
> > It will not give an error in case of yaml.encode() and yaml.encode({},
> > {}, {}).
> 
> Decent. Here you are right.
> 
> > 
> > > 
> > > >    usage_error:
> > > >          return luaL_error(L, "Usage: encode(object, {tag_prefix = <string>, "\
> > > >                            "tag_handle = <string>})");
> > > > 
> 
> My diff, which reverts some changes and makes this patch one-liner:
> 
> diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc
> index 354cafe86..854794dd1 100644
> --- a/third_party/lua-yaml/lyaml.cc
> +++ b/third_party/lua-yaml/lyaml.cc
> @@ -400,8 +400,7 @@ static void load(struct lua_yaml_loader *loader) {
>   */
>  static int l_load(lua_State *L) {
>     struct lua_yaml_loader loader;
> -   int top = lua_gettop(L);
> -   if (!(top == 1 || top == 2) || !lua_isstring(L, 1)) {
> +   if (! lua_isstring(L, 1)) {
>  usage_error:
>        return luaL_error(L, "Usage: yaml.decode(document, "\
>                          "[{tag_only = boolean}])");
> @@ -417,7 +416,7 @@ usage_error:
>        return luaL_error(L, OOM_ERRMSG);
>     yaml_parser_set_input_string(&loader.parser, (yaml_char_t *) document, len);
>     bool tag_only;
> -   if (lua_gettop(L) == 2 && ! lua_isnil(L, 2)) {
> +   if (lua_gettop(L) > 1) {
>        if (! lua_istable(L, 2))
>           goto usage_error;
>        lua_getfield(L, 2, "tag_only");

----

The new patch description and diff (w/o tests):

lua-yaml: verify args in a consistent manner

Use lua_is*() functions instead of explicit lua_gettop() checks in
yaml.encode() and yaml.decode() functions.

Behaviour changes:

* yaml.decode(object, nil) ignores nil (it is consistent with encode
  behaviour).
* yaml.encode() gives an usage error instead of "unsupported Lua type
  'thread'".
* yaml.encode('', {}, {}) ignores 3rd argument (it is consistent with
  decode behaviour).

diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc
index c6d118a79..bd876ab29 100644
--- a/third_party/lua-yaml/lyaml.cc
+++ b/third_party/lua-yaml/lyaml.cc
@@ -416,7 +416,7 @@ usage_error:
       return luaL_error(L, OOM_ERRMSG);
    yaml_parser_set_input_string(&loader.parser, (yaml_char_t *) document, len);
    bool tag_only;
-   if (lua_gettop(L) > 1) {
+   if (! lua_isnoneornil(L, 2)) {
       if (! lua_istable(L, 2))
          goto usage_error;
       lua_getfield(L, 2, "tag_only");
@@ -793,14 +793,13 @@ error:
  */
 static int l_dump(lua_State *L) {
    struct luaL_serializer *serializer = luaL_checkserializer(L);
-   int top = lua_gettop(L);
-   if (top > 2) {
+   if (lua_isnone(L, 1)) {
 usage_error:
       return luaL_error(L, "Usage: encode(object, {tag_prefix = <string>, "\
                         "tag_handle = <string>})");
    }
    const char *prefix = NULL, *handle = NULL;
-   if (top == 2 && !lua_isnil(L, 2)) {
+   if (! lua_isnoneornil(L, 2)) {
       if (! lua_istable(L, 2))
          goto usage_error;
       lua_getfield(L, 2, "tag_prefix");

[tarantool-patches] Re: [PATCH v2 1/3] lua-yaml: verify arguments count

[Vladislav Shpilevoy]

This commit LGTM. Next is pending of fixes.

On 11/02/2019 14:32, Alexander Turenko wrote:
> lua_is* really checks whether an acceptable index is a valid one, so
> there are two possible approaches, one of which we should stick I think:
> 
> * Verify lua_gettop() upper and lower bounds right at start of a
>    function.
> * Use lua_is* (including lua_isnone() and lua_isnoneornil()) and don't
>    verify arguments count explicitly.
> 
> I think we should use one of these ways within a module: this is more
> important then the patch size. The only difference for a user is that
> the latter approach does not check for extra arguments.
> 
> Now I implemented the latter approach as I see you want to minimize
> explicit checks. See the patch at end of the email.
> 
> It is possible to reduce the patch further, but loss consistency in what
> we check: lua_is* or lua_gettop(). I'll do if you insist, but don't
> think it is the right way to proceed.
> 
> NB: branch: kh/gh-3662-yaml-2.1
> 
> WBR, Alexander Turenko.
> 
> On Tue, Feb 05, 2019 at 10:36:41PM +0300, Vladislav Shpilevoy wrote:
>> Hi! Thanks for the fixes!
>>
>>>>> functions.
>>>>>
>>>>> Without these checks the functions could read garbage outside of a Lua
>>>>> stack when called w/o arguments.
>>>>
>>>> Honestly, I do not understand how is it possible. Please,
>>>> provide a test for both functions. See my 3 doubts below.
>>>
>>> lua_isstring(L, 1) checks a garbage w/o preliminary lua_gettop() check.
>>> yaml.encode() gives me "unsupported Lua type 'thread'" on the current
>>> tarantool 2.1.
>>
>> I looked at lua_isstring implementation, and I see, that it checks
>> top. If an index is above top, then the type is nil.
>>
>> 	static TValue *index2adr(lua_State *L, int idx)
>> 	{
>> 	  if (idx > 0) {
>> 	    TValue *o = L->base + (idx - 1);
>> 	    return o < L->top ? o : niltv(L);
>> 	...
>>
> 
> Ouch, lua_isstring() is called only in case of top == 2, so this is out
> of scope of the discussion. The real cause of this weird "unsupported
> Lua type 'thread'" error is lua_yaml_encode() code: it calls
> `lua_newthread(L)` and then `lua_pushvalue(L, 1);`. A 1st stack item
> should be a value we encode, but when there are no arguments for
> yaml.encode() the new lua thread is the 1st item.
> 
> Anyway, I don't think that "unsupported Lua type 'thread'" is the right
> error message for `yaml.encode()`. Are you agree?
> 
>>>
>>> Anyway, added bad API usage test cases. Also I changed this:
>>>
>>> diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc
>>> index 3a427263e..46374970f 100644
>>> --- a/third_party/lua-yaml/lyaml.cc
>>> +++ b/third_party/lua-yaml/lyaml.cc
>>> @@ -453,7 +453,7 @@ usage_error:
>>>          return luaL_error(L, OOM_ERRMSG);
>>>       yaml_parser_set_input_string(&loader.parser, (yaml_char_t *) document, len);
>>>       bool tag_only;
>>> -   if (lua_gettop(L) == 2) {
>>> +   if (lua_gettop(L) == 2 && ! lua_isnil(L, 2)) {
>>>          if (! lua_istable(L, 2))
>>>             goto usage_error;
>>>          lua_getfield(L, 2, "tag_only");
>>>
>>> We should not raise an usage error for yaml.decode(object, nil).
>>
>> Why? It is said, that the second value either does not exist, or
>> is a table. Nil is not a table. So why? If your logic was about
>> considering nil as a not existing value, then why don't we handle
>> cases like this: yaml.decode(object, nil, nil, nil, nil) ? The same
>> for l_dump() and encode.
> 
> There is the difference between `yaml.decode(object, nil)` and
> `yaml.decode(object, nil, nil, nil, nil)`. The former one is likely to
> appear due to passing though the 2nd argument, say:
> 
> ```
> local function load_cfg(raw, opts)
>      local object = yaml.decode(raw, opts)
>      ...some post-processing...
>      return object
> end
> ```
> 
> The latter is definitely wrong usage.
> 
> But now I removed checks for extra args, see above.
> 
>>
>>>>>     usage_error:
>>>>>           return luaL_error(L, "Usage: yaml.decode(document, "\
>>>>>                             "[{tag_only = boolean}])");
>>>>> @@ -416,7 +417,7 @@ usage_error:
>>>>>           return luaL_error(L, OOM_ERRMSG);
>>>>>        yaml_parser_set_input_string(&loader.parser, (yaml_char_t *) document, len);
>>>>>        bool tag_only;
>>>>> -   if (lua_gettop(L) > 1) {
>>>>> +   if (lua_gettop(L) == 2) {
>>>>
>>>> 2. This function never touches anything beyond second value on
>>>> the stack, so here lua_gettop(L) > 1 means the same as
>>>> lua_gettop(L) == 2 - the second argument exist. Third and next
>>>> values do not matter.
>>>
>>> I read this as 'those are equivalent' (correct me if I'm wrong). Ok. I'd
>>> prefer to leave it with ==. Also note the fix I pasted above.
>>
>> Why? Again. I do not see any reason behind this change except personal
>> preference.
> 
> It does not matter much, because I anyway need to add ` && !
> lua_isnil(L, 2)` or use `! lua_isnoneornil(L, 2)` here to make decode
> behaviour consistent with encode one (against 2nd argument). Yep, it is
> personal preference. Anyway, now it is `! lua_isnoneornil(L, 2)`.
> 
>> I reverted all the changes about l_load() function, and the
>> tests passed. So why do we need to make diff bigger?
> 
> yaml.decode('', nil, {}) don't pass before (don't raise an error).
> Other tests are passed, because of two reasons:
> 
> * no test on yaml.decode('', nil);
> * lua_isstring() checks stack size.
> 
> Re test: added for encode and decode.
> 
> Re lua_isstring(): okay, now I understood that it checks given index by
> the API:
> 
> http://pgl.yoyo.org/luai/i/lua_isstring ("acceptable index")
> https://www.lua.org/manual/5.3/manual.html#4.3 ("Valid and Acceptable Indices")
> https://www.lua.org/manual/5.1/manual.html#3.2 (the same for Lua 5.1)
> 
> So I changed the description of the commit to make it clear that the
> reason of the change is to make the code more consistent.
> 
>>
>>>
>>>>
>>>>>           if (! lua_istable(L, 2))
>>>>>              goto usage_error;
>>>>>           lua_getfield(L, 2, "tag_only");
>>>>> @@ -794,7 +795,7 @@ error:
>>>>>     static int l_dump(lua_State *L) {
>>>>>        struct luaL_serializer *serializer = luaL_checkserializer(L);
>>>>>        int top = lua_gettop(L);
>>>>> -   if (top > 2) {
>>>>> +   if (!(top == 1 || top == 2)) {
>>>>
>>>> 3. Here my reasoning is the same - the previous checking works
>>>> as well.
>>>
>>> It will not give an error in case of yaml.encode() and yaml.encode({},
>>> {}, {}).
>>
>> Decent. Here you are right.
>>
>>>
>>>>
>>>>>     usage_error:
>>>>>           return luaL_error(L, "Usage: encode(object, {tag_prefix = <string>, "\
>>>>>                             "tag_handle = <string>})");
>>>>>
>>
>> My diff, which reverts some changes and makes this patch one-liner:
>>
>> diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc
>> index 354cafe86..854794dd1 100644
>> --- a/third_party/lua-yaml/lyaml.cc
>> +++ b/third_party/lua-yaml/lyaml.cc
>> @@ -400,8 +400,7 @@ static void load(struct lua_yaml_loader *loader) {
>>    */
>>   static int l_load(lua_State *L) {
>>      struct lua_yaml_loader loader;
>> -   int top = lua_gettop(L);
>> -   if (!(top == 1 || top == 2) || !lua_isstring(L, 1)) {
>> +   if (! lua_isstring(L, 1)) {
>>   usage_error:
>>         return luaL_error(L, "Usage: yaml.decode(document, "\
>>                           "[{tag_only = boolean}])");
>> @@ -417,7 +416,7 @@ usage_error:
>>         return luaL_error(L, OOM_ERRMSG);
>>      yaml_parser_set_input_string(&loader.parser, (yaml_char_t *) document, len);
>>      bool tag_only;
>> -   if (lua_gettop(L) == 2 && ! lua_isnil(L, 2)) {
>> +   if (lua_gettop(L) > 1) {
>>         if (! lua_istable(L, 2))
>>            goto usage_error;
>>         lua_getfield(L, 2, "tag_only");
> 
> ----
> 
> The new patch description and diff (w/o tests):
> 
> lua-yaml: verify args in a consistent manner
> 
> Use lua_is*() functions instead of explicit lua_gettop() checks in
> yaml.encode() and yaml.decode() functions.
> 
> Behaviour changes:
> 
> * yaml.decode(object, nil) ignores nil (it is consistent with encode
>    behaviour).
> * yaml.encode() gives an usage error instead of "unsupported Lua type
>    'thread'".
> * yaml.encode('', {}, {}) ignores 3rd argument (it is consistent with
>    decode behaviour).
> 
> diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc
> index c6d118a79..bd876ab29 100644
> --- a/third_party/lua-yaml/lyaml.cc
> +++ b/third_party/lua-yaml/lyaml.cc
> @@ -416,7 +416,7 @@ usage_error:
>         return luaL_error(L, OOM_ERRMSG);
>      yaml_parser_set_input_string(&loader.parser, (yaml_char_t *) document, len);
>      bool tag_only;
> -   if (lua_gettop(L) > 1) {
> +   if (! lua_isnoneornil(L, 2)) {
>         if (! lua_istable(L, 2))
>            goto usage_error;
>         lua_getfield(L, 2, "tag_only");
> @@ -793,14 +793,13 @@ error:
>    */
>   static int l_dump(lua_State *L) {
>      struct luaL_serializer *serializer = luaL_checkserializer(L);
> -   int top = lua_gettop(L);
> -   if (top > 2) {
> +   if (lua_isnone(L, 1)) {
>   usage_error:
>         return luaL_error(L, "Usage: encode(object, {tag_prefix = <string>, "\
>                           "tag_handle = <string>})");
>      }
>      const char *prefix = NULL, *handle = NULL;
> -   if (top == 2 && !lua_isnil(L, 2)) {
> +   if (! lua_isnoneornil(L, 2)) {
>         if (! lua_istable(L, 2))
>            goto usage_error;
>         lua_getfield(L, 2, "tag_prefix");
> 

[tarantool-patches] [PATCH v2 2/3] lua-yaml: fix boolean/null representation in yaml

[Alexander Turenko]

Encode changes:

Wrap the following string literals into single quotes: 'false', 'no',
'true', 'yes', '~', 'null', 'Null', 'NULL'. The content of these strings
is equals to null or boolean values representation in YAML.

Reverted the a2d7643c commit to use single quotes instead of multiline
encoding for 'true' and 'false' string literals.

'~', 'null', 'Null', 'NULL', 'no' and 'yes' were encoded w/o quotes
before, so '~', 'null', 'no', 'yes' were decoded with a wrong type then.
Multiline encoding was used for 'true' and 'false'.

Decode changes:

Treat 'Null', 'NULL' as null in addition to '~' and 'null'.

Note: this commit leaves an empty document/value treatment as an empty
string, which is not standard (should be treated as null). This is fixed
in the following commit.

Follows up #3476
Closes #3662
Closes #3583
---
 test/app-tap/console.test.lua | 23 +++++++---
 test/box/misc.result          |  2 +-
 third_party/lua-yaml/lyaml.cc | 80 ++++++++++++++++++++++++++---------
 3 files changed, 80 insertions(+), 25 deletions(-)

diff --git a/test/app-tap/console.test.lua b/test/app-tap/console.test.lua
index 4f915afd7..68f273234 100755
--- a/test/app-tap/console.test.lua
+++ b/test/app-tap/console.test.lua
@@ -21,7 +21,7 @@ local EOL = "\n...\n"
 
 test = tap.test("console")
 
-test:plan(59)
+test:plan(68)
 
 -- Start console and connect to it
 local server = console.listen(CONSOLE_SOCKET)
@@ -77,11 +77,24 @@ test:is(yaml.decode(client:read(EOL)), '', "clear delimiter")
 
 --
 -- gh-3476: yaml.encode encodes 'false' and 'true' incorrectly.
+-- gh-3662: yaml.encode encodes booleans with multiline format.
+-- gh-3583: yaml.encode encodes null incorrectly.
 --
-test:is(type(yaml.decode(yaml.encode('false'))), 'string')
-test:is(type(yaml.decode(yaml.encode('true'))), 'string')
-test:is(type(yaml.decode(yaml.encode({a = 'false'})).a), 'string')
-test:is(type(yaml.decode(yaml.encode({a = 'false'})).a), 'string')
+
+test:is(yaml.encode(false), "--- false\n...\n")
+test:is(yaml.encode(true), "--- true\n...\n")
+test:is(yaml.encode('false'), "--- 'false'\n...\n")
+test:is(yaml.encode('true'), "--- 'true'\n...\n")
+test:is(yaml.encode(nil), "--- null\n...\n")
+
+test:is(yaml.decode('false'), false)
+test:is(yaml.decode('no'), false)
+test:is(yaml.decode('true'), true)
+test:is(yaml.decode('yes'), true)
+test:is(yaml.decode('~'), nil)
+test:is(yaml.decode('null'), nil)
+test:is(yaml.decode('Null'), nil)
+test:is(yaml.decode('NULL'), nil)
 
 box.cfg{
     listen=IPROTO_SOCKET;
diff --git a/test/box/misc.result b/test/box/misc.result
index c3cabcc8a..94ee11b63 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -55,7 +55,7 @@ t = {} for n in pairs(box) do table.insert(t, tostring(n)) end table.sort(t)
 ...
 t
 ---
-- - NULL
+- - 'NULL'
   - atomic
   - backup
   - begin
diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc
index 9b07992d8..73e5d2c31 100644
--- a/third_party/lua-yaml/lyaml.cc
+++ b/third_party/lua-yaml/lyaml.cc
@@ -94,6 +94,46 @@ struct lua_yaml_dumper {
    luaL_Buffer yamlbuf;
 };
 
+enum yaml_type {
+   YAML_NO_MATCH = 0,
+   YAML_FALSE,
+   YAML_TRUE,
+   YAML_NULL
+};
+
+/**
+ * Verify whether a string represents a boolean literal in YAML.
+ *
+ * Non-standard: only subset of YAML 1.1 boolean literals are
+ * treated as boolean values.
+ */
+static yaml_type
+yaml_get_bool(const char *str, const size_t len)
+{
+   if (len > 5)
+      return YAML_NO_MATCH;
+   if (strcmp(str, "false") == 0 || strcmp(str, "no") == 0)
+      return YAML_FALSE;
+   if (strcmp(str, "true") == 0 || strcmp(str, "yes") == 0)
+      return YAML_TRUE;
+   return YAML_NO_MATCH;
+}
+
+/**
+ * Verify whether a string represents a null literal in YAML.
+ *
+ * Non-standard: don't match an empty string as null.
+ */
+static yaml_type
+yaml_get_null(const char *str, const size_t len){
+   if (len == 1 && str[0] == '~')
+      return YAML_NULL;
+   if (len == 4 && (strcmp(str, "null") == 0 || strcmp(str, "Null") == 0 ||
+       strcmp(str, "NULL") == 0))
+      return YAML_NULL;
+   return YAML_NO_MATCH;
+}
+
 static void generate_error_message(struct lua_yaml_loader *loader) {
    char buf[256];
    luaL_Buffer b;
@@ -209,7 +249,7 @@ static void load_scalar(struct lua_yaml_loader *loader) {
          lua_pushnumber(loader->L, dval);
          return;
       } else if (!strcmp(tag, "bool")) {
-         lua_pushboolean(loader->L, !strcmp(str, "true") || !strcmp(str, "yes"));
+         lua_pushboolean(loader->L, yaml_get_bool(str, length) == YAML_TRUE);
          return;
       } else if (!strcmp(tag, "binary")) {
          frombase64(loader->L, (const unsigned char *)str, length);
@@ -218,20 +258,20 @@ static void load_scalar(struct lua_yaml_loader *loader) {
    }
 
    if (loader->event.data.scalar.style == YAML_PLAIN_SCALAR_STYLE) {
-      if (!strcmp(str, "~")) {
-         luaL_pushnull(loader->L);
-         return;
-      } else if (!strcmp(str, "true") || !strcmp(str, "yes")) {
-         lua_pushboolean(loader->L, 1);
-         return;
-      } else if (!strcmp(str, "false") || !strcmp(str, "no")) {
-         lua_pushboolean(loader->L, 0);
+      yaml_type type;
+      if (!length) {
+         /*
+          * Non-standard: an empty value/document is null
+          * according to the standard, but we decode it as an
+          * empty string.
+          */
+         lua_pushliteral(loader->L, "");
          return;
-      } else if (!strcmp(str, "null")) {
+      } else if (yaml_get_null(str, length) == YAML_NULL) {
          luaL_pushnull(loader->L);
          return;
-      } else if (!length) {
-         lua_pushliteral(loader->L, "");
+      } else if ((type = yaml_get_bool(str, length)) != YAML_NO_MATCH) {
+         lua_pushboolean(loader->L, type == YAML_TRUE);
          return;
       }
 
@@ -548,7 +588,6 @@ static int yaml_is_flow_mode(struct lua_yaml_dumper *dumper) {
                (evp->type == YAML_MAPPING_START_EVENT &&
                 evp->data.mapping_start.style == YAML_FLOW_MAPPING_STYLE)) {
             return 1;
-            break;
          }
       }
    }
@@ -597,8 +636,13 @@ static int dump_node(struct lua_yaml_dumper *dumper)
       return dump_table(dumper, &field);
    case MP_STR:
       str = lua_tolstring(dumper->L, -1, &len);
-      if (lua_isnumber(dumper->L, -1)) {
-         /* string is convertible to number, quote it to preserve type */
+      if ((yaml_get_null(str, len) == YAML_NULL)
+         || (yaml_get_bool(str, len) != YAML_NO_MATCH)
+         || (lua_isnumber(dumper->L, -1))) {
+         /*
+          * The string is convertible to a null, a boolean or
+          * a number, quote it to preserve its type.
+          */
          style = YAML_SINGLE_QUOTED_SCALAR_STYLE;
          break;
       }
@@ -606,12 +650,10 @@ static int dump_node(struct lua_yaml_dumper *dumper)
       if (utf8_check_printable(str, len)) {
          if (yaml_is_flow_mode(dumper)) {
             style = YAML_SINGLE_QUOTED_SCALAR_STYLE;
-         } else if (strstr(str, "\n\n") != NULL || strcmp(str, "true") == 0 ||
-		    strcmp(str, "false") == 0) {
+         } else if (strstr(str, "\n\n") != 0) {
             /*
              * Tarantool-specific: use literal style for string
-             * with empty lines and strings representing boolean
-             * types.
+             * with empty lines.
              * Useful for tutorial().
              */
             style = YAML_LITERAL_SCALAR_STYLE;
-- 
2.20.1

[tarantool-patches] Re: [PATCH v2 2/3] lua-yaml: fix boolean/null representation in yaml

[Vladislav Shpilevoy]

Thanks for the patch!

On 22/01/2019 05:12, Alexander Turenko wrote:
> Encode changes:
> 
> Wrap the following string literals into single quotes: 'false', 'no',
> 'true', 'yes', '~', 'null', 'Null', 'NULL'. The content of these strings
> is equals to null or boolean values representation in YAML.
> 
> Reverted the a2d7643c commit to use single quotes instead of multiline
> encoding for 'true' and 'false' string literals.
> 
> '~', 'null', 'Null', 'NULL', 'no' and 'yes' were encoded w/o quotes
> before, so '~', 'null', 'no', 'yes' were decoded with a wrong type then.
> Multiline encoding was used for 'true' and 'false'.

Unfortunately, I hardly understood what happened in this commit
by reading this message, sorry. Maybe I am confused by imperative
in the message body, or by too frequent repetition of this infinite
sequence of strings: "'~', 'null', 'Null', 'NULL', 'no' and 'yes'",
or by too mixed style of narration of what is the true and standard
way of these values encoding - with quotes or without.

Could you, please, make it simpler somehow?

> 
> Decode changes:
> 
> Treat 'Null', 'NULL' as null in addition to '~' and 'null'.
> 
> Note: this commit leaves an empty document/value treatment as an empty
> string, which is not standard (should be treated as null). This is fixed
> in the following commit.
> 
> Follows up #3476
> Closes #3662
> Closes #3583
> ---
>   test/app-tap/console.test.lua | 23 +++++++---
>   test/box/misc.result          |  2 +-
>   third_party/lua-yaml/lyaml.cc | 80 ++++++++++++++++++++++++++---------
>   3 files changed, 80 insertions(+), 25 deletions(-)
> 
> diff --git a/test/app-tap/console.test.lua b/test/app-tap/console.test.lua
> index 4f915afd7..68f273234 100755
> --- a/test/app-tap/console.test.lua
> +++ b/test/app-tap/console.test.lua
> @@ -77,11 +77,24 @@ test:is(yaml.decode(client:read(EOL)), '', "clear delimiter")
>   
>   --
>   -- gh-3476: yaml.encode encodes 'false' and 'true' incorrectly.
> +-- gh-3662: yaml.encode encodes booleans with multiline format.
> +-- gh-3583: yaml.encode encodes null incorrectly.
>   --
> -test:is(type(yaml.decode(yaml.encode('false'))), 'string')
> -test:is(type(yaml.decode(yaml.encode('true'))), 'string')
> -test:is(type(yaml.decode(yaml.encode({a = 'false'})).a), 'string')
> -test:is(type(yaml.decode(yaml.encode({a = 'false'})).a), 'string')

1. Are you sure that this is the correct behaviour? So now it is
not true that decode(encode(obj)) == obj? Looks utterly confusing.
Or why else you removed these tests?

> +
> +test:is(yaml.encode(false), "--- false\n...\n")
> +test:is(yaml.encode(true), "--- true\n...\n")
> +test:is(yaml.encode('false'), "--- 'false'\n...\n")
> +test:is(yaml.encode('true'), "--- 'true'\n...\n")
> +test:is(yaml.encode(nil), "--- null\n...\n")
> +
> +test:is(yaml.decode('false'), false)
> +test:is(yaml.decode('no'), false)
> +test:is(yaml.decode('true'), true)
> +test:is(yaml.decode('yes'), true)
> +test:is(yaml.decode('~'), nil)
> +test:is(yaml.decode('null'), nil)
> +test:is(yaml.decode('Null'), nil)
> +test:is(yaml.decode('NULL'), nil)
>   
>   box.cfg{
>       listen=IPROTO_SOCKET;
> diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc
> index 9b07992d8..73e5d2c31 100644
> --- a/third_party/lua-yaml/lyaml.cc
> +++ b/third_party/lua-yaml/lyaml.cc
> @@ -94,6 +94,46 @@ struct lua_yaml_dumper {
>      luaL_Buffer yamlbuf;
>   };
>   
> +enum yaml_type {
> +   YAML_NO_MATCH = 0,
> +   YAML_FALSE,
> +   YAML_TRUE,
> +   YAML_NULL
> +};
> +
> +/**
> + * Verify whether a string represents a boolean literal in YAML.
> + *
> + * Non-standard: only subset of YAML 1.1 boolean literals are
> + * treated as boolean values.
> + */
> +static yaml_type
> +yaml_get_bool(const char *str, const size_t len)

2. Usually in such cases we use int 0/-1 returned value
and return the result via an out-parameter. So it should be

     static inline int
     yaml_parse_bool(const char *str, size_t len, bool *value);

Yes, I've changed 'get' to 'parse' and inlined the function,
if you do not mind.

The same for the next function, parsing null. After that you
can remove enum yaml_type.

Which, btw, is not a type, strictly speaking, because it
does not have YAML_INT, YAML_DOUBLE ...

Also, I do not see any point in making 'len' be 'const'.

Scarcely it is a case when you need a parameter, passed by
value instead of pointer, be explicitly const. The compiler
can detect it easily.

> +{
> +   if (len > 5)
> +      return YAML_NO_MATCH;
> +   if (strcmp(str, "false") == 0 || strcmp(str, "no") == 0)
> +      return YAML_FALSE;
> +   if (strcmp(str, "true") == 0 || strcmp(str, "yes") == 0)

3. I think, it is redundant to firstly check length and then
call strcmp, which also calculates length (by checking zero
byte). We need either use only strcmp(), or use length comparison
+ memcmp. The same below for the null parser.

> +      return YAML_TRUE;
> +   return YAML_NO_MATCH;
> +}
> +
> +/**
> + * Verify whether a string represents a null literal in YAML.
> + *
> + * Non-standard: don't match an empty string as null.
> + */
> +static yaml_type
> +yaml_get_null(const char *str, const size_t len){

4. ?

     static inline int
     yaml_parse_null(const char *str, size_t len, bool *is_null);

> +   if (len == 1 && str[0] == '~')
> +      return YAML_NULL;
> +   if (len == 4 && (strcmp(str, "null") == 0 || strcmp(str, "Null") == 0 ||

5. Hmm. Length of "null" and "Null" is 3, not 4. As I understand,
len does not count terminating zero. And I can prove it - load_scalar()
in case if !strcmp(tag, "str") does lua_pushlstring(loader->L, str, length),
so length == strlen(str).

Even if I am mistaken about it, why two lines above you consider len
of "~" as 1, not 2?

> +       strcmp(str, "NULL") == 0))
> +      return YAML_NULL;
> +   return YAML_NO_MATCH;
> +}
> +
>   static void generate_error_message(struct lua_yaml_loader *loader) {
>      char buf[256];
>      luaL_Buffer b;
> @@ -597,8 +636,13 @@ static int dump_node(struct lua_yaml_dumper *dumper)
>         return dump_table(dumper, &field);
>      case MP_STR:
>         str = lua_tolstring(dumper->L, -1, &len);
> -      if (lua_isnumber(dumper->L, -1)) {
> -         /* string is convertible to number, quote it to preserve type */
> +      if ((yaml_get_null(str, len) == YAML_NULL)
> +         || (yaml_get_bool(str, len) != YAML_NO_MATCH)
> +         || (lua_isnumber(dumper->L, -1))) {
> +         /*
> +          * The string is convertible to a null, a boolean or
> +          * a number, quote it to preserve its type.
> +          */

6. I think, it is related to my first comment. So why did you
delete those tests?

>            style = YAML_SINGLE_QUOTED_SCALAR_STYLE;
>            break;
>         }

[tarantool-patches] Re: [PATCH v2 2/3] lua-yaml: fix boolean/null representation in yaml

[Vladislav Shpilevoy]

>      static inline int
>      yaml_parse_null(const char *str, size_t len, bool *is_null);
> 
>> +   if (len == 1 && str[0] == '~')
>> +      return YAML_NULL;
>> +   if (len == 4 && (strcmp(str, "null") == 0 || strcmp(str, "Null") == 0 ||
> 
> 5. Hmm. Length of "null" and "Null" is 3, not 4. As I understand,
> len does not count terminating zero. And I can prove it - load_scalar()
> in case if !strcmp(tag, "str") does lua_pushlstring(loader->L, str, length),
> so length == strlen(str).
> 
> Even if I am mistaken about it, why two lines above you consider len
> of "~" as 1, not 2?

Sorry, my fault. I was too tired. Of course, strlen('null') == 4. I
counted 'll' for one. But please, still consider my point about
strcmp -> memcmp.

> 
>> +       strcmp(str, "NULL") == 0))
>> +      return YAML_NULL;
>> +   return YAML_NO_MATCH;
>> +}
>> +

[tarantool-patches] Re: [PATCH v2 2/3] lua-yaml: fix boolean/null representation in yaml

[Alexander Turenko]

Hi!

I dropped support of 'Null' and 'NULL' as null values to stick with the
old approach (only 'null' and '~' are decoded as nulls). At least while
changes are not requested explicitly it is better to keep backward
compatibility even if some parts of behaviour is not standard.

Answered your comments below.

I'll resend the updated patchset as v3 soon.

WBR, Alexander Turenko.

On Fri, Jan 25, 2019 at 12:26:56AM +0300, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> On 22/01/2019 05:12, Alexander Turenko wrote:
> > Encode changes:
> > 
> > Wrap the following string literals into single quotes: 'false', 'no',
> > 'true', 'yes', '~', 'null', 'Null', 'NULL'. The content of these strings
> > is equals to null or boolean values representation in YAML.
> > 
> > Reverted the a2d7643c commit to use single quotes instead of multiline
> > encoding for 'true' and 'false' string literals.
> > 
> > '~', 'null', 'Null', 'NULL', 'no' and 'yes' were encoded w/o quotes
> > before, so '~', 'null', 'no', 'yes' were decoded with a wrong type then.
> > Multiline encoding was used for 'true' and 'false'.
> 
> Unfortunately, I hardly understood what happened in this commit
> by reading this message, sorry. Maybe I am confused by imperative
> in the message body, or by too frequent repetition of this infinite
> sequence of strings: "'~', 'null', 'Null', 'NULL', 'no' and 'yes'",
> or by too mixed style of narration of what is the true and standard
> way of these values encoding - with quotes or without.
> 
> Could you, please, make it simpler somehow?

You are right, it was horrible. I had changed wording and reflected the
patch changes (described above).

```
lua-yaml: fix strings literals encoding in yaml

yaml.encode() now wraps a string literal whose content is equal to a
null or a boolean value representation in YAML into single quotes. Those
literals are: 'false', 'no', 'true', 'yes', '~', 'null'.

Reverted the a2d7643c commit to use single quotes instead of multiline
encoding for 'true' and 'false' string literals.

Follows up #3476
Closes #3662
Closes #3583
```

> 
> > 
> > Decode changes:
> > 
> > Treat 'Null', 'NULL' as null in addition to '~' and 'null'.
> > 
> > Note: this commit leaves an empty document/value treatment as an empty
> > string, which is not standard (should be treated as null). This is fixed
> > in the following commit.
> > 
> > Follows up #3476
> > Closes #3662
> > Closes #3583
> > ---
> >   test/app-tap/console.test.lua | 23 +++++++---
> >   test/box/misc.result          |  2 +-
> >   third_party/lua-yaml/lyaml.cc | 80 ++++++++++++++++++++++++++---------
> >   3 files changed, 80 insertions(+), 25 deletions(-)
> > 
> > diff --git a/test/app-tap/console.test.lua b/test/app-tap/console.test.lua
> > index 4f915afd7..68f273234 100755
> > --- a/test/app-tap/console.test.lua
> > +++ b/test/app-tap/console.test.lua
> > @@ -77,11 +77,24 @@ test:is(yaml.decode(client:read(EOL)), '', "clear delimiter")
> >   --
> >   -- gh-3476: yaml.encode encodes 'false' and 'true' incorrectly.
> > +-- gh-3662: yaml.encode encodes booleans with multiline format.
> > +-- gh-3583: yaml.encode encodes null incorrectly.
> >   --
> > -test:is(type(yaml.decode(yaml.encode('false'))), 'string')
> > -test:is(type(yaml.decode(yaml.encode('true'))), 'string')
> > -test:is(type(yaml.decode(yaml.encode({a = 'false'})).a), 'string')
> > -test:is(type(yaml.decode(yaml.encode({a = 'false'})).a), 'string')
> 
> 1. Are you sure that this is the correct behaviour? So now it is
> not true that decode(encode(obj)) == obj? Looks utterly confusing.
> Or why else you removed these tests?

They are work as before, but now superseded with simpler ones.

> 
> > +
> > +test:is(yaml.encode(false), "--- false\n...\n")
> > +test:is(yaml.encode(true), "--- true\n...\n")
> > +test:is(yaml.encode('false'), "--- 'false'\n...\n")
> > +test:is(yaml.encode('true'), "--- 'true'\n...\n")
> > +test:is(yaml.encode(nil), "--- null\n...\n")
> > +
> > +test:is(yaml.decode('false'), false)
> > +test:is(yaml.decode('no'), false)
> > +test:is(yaml.decode('true'), true)
> > +test:is(yaml.decode('yes'), true)
> > +test:is(yaml.decode('~'), nil)
> > +test:is(yaml.decode('null'), nil)
> > +test:is(yaml.decode('Null'), nil)
> > +test:is(yaml.decode('NULL'), nil)
> >   box.cfg{
> >       listen=IPROTO_SOCKET;
> > diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc
> > index 9b07992d8..73e5d2c31 100644
> > --- a/third_party/lua-yaml/lyaml.cc
> > +++ b/third_party/lua-yaml/lyaml.cc
> > @@ -94,6 +94,46 @@ struct lua_yaml_dumper {
> >      luaL_Buffer yamlbuf;
> >   };
> > +enum yaml_type {
> > +   YAML_NO_MATCH = 0,
> > +   YAML_FALSE,
> > +   YAML_TRUE,
> > +   YAML_NULL
> > +};
> > +
> > +/**
> > + * Verify whether a string represents a boolean literal in YAML.
> > + *
> > + * Non-standard: only subset of YAML 1.1 boolean literals are
> > + * treated as boolean values.
> > + */
> > +static yaml_type
> > +yaml_get_bool(const char *str, const size_t len)
> 
> 2. Usually in such cases we use int 0/-1 returned value
> and return the result via an out-parameter. So it should be
> 
>     static inline int
>     yaml_parse_bool(const char *str, size_t len, bool *value);
> 
> Yes, I've changed 'get' to 'parse' and inlined the function,
> if you do not mind.
> 
> The same for the next function, parsing null. After that you
> can remove enum yaml_type.
> 
> Which, btw, is not a type, strictly speaking, because it
> does not have YAML_INT, YAML_DOUBLE ...
> 
> Also, I do not see any point in making 'len' be 'const'.
> 
> Scarcely it is a case when you need a parameter, passed by
> value instead of pointer, be explicitly const. The compiler
> can detect it easily.

Agreed with all these comments and fixed them.

> 
> > +{
> > +   if (len > 5)
> > +      return YAML_NO_MATCH;
> > +   if (strcmp(str, "false") == 0 || strcmp(str, "no") == 0)
> > +      return YAML_FALSE;
> > +   if (strcmp(str, "true") == 0 || strcmp(str, "yes") == 0)
> 
> 3. I think, it is redundant to firstly check length and then
> call strcmp, which also calculates length (by checking zero
> byte). We need either use only strcmp(), or use length comparison
> + memcmp. The same below for the null parser.

Okay, it was the premature optimization.

The more important thing is that is was incorrect: "fals" gives
YAML_FALSE.

I had fixed it like so, is it okay?

```
if ((len == 5 && memcmp(str, "false", 5) == 0) ||
    (len == 2 && memcmp(str, "no", 2) == 0)) {
   if (out != NULL)
      *out = false;
   return 0;
}
...
```

> 
> > +      return YAML_TRUE;
> > +   return YAML_NO_MATCH;
> > +}
> > +
> > +/**
> > + * Verify whether a string represents a null literal in YAML.
> > + *
> > + * Non-standard: don't match an empty string as null.
> > + */
> > +static yaml_type
> > +yaml_get_null(const char *str, const size_t len){
> 
> 4. ?
> 
>     static inline int
>     yaml_parse_null(const char *str, size_t len, bool *is_null);

is_null is redundant here, because a return code contain all needed
information. Agreed except that. Changed.

> 
> > +   if (len == 1 && str[0] == '~')
> > +      return YAML_NULL;
> > +   if (len == 4 && (strcmp(str, "null") == 0 || strcmp(str, "Null") == 0 ||
> 
> 5. Hmm. Length of "null" and "Null" is 3, not 4. As I understand,
> len does not count terminating zero. And I can prove it - load_scalar()
> in case if !strcmp(tag, "str") does lua_pushlstring(loader->L, str, length),
> so length == strlen(str).
> 
> Even if I am mistaken about it, why two lines above you consider len
> of "~" as 1, not 2?

Just for the record: you fixed youself in a separate email: null is 4
chars length.

> 
> > +       strcmp(str, "NULL") == 0))
> > +      return YAML_NULL;
> > +   return YAML_NO_MATCH;
> > +}
> > +
> >   static void generate_error_message(struct lua_yaml_loader *loader) {
> >      char buf[256];
> >      luaL_Buffer b;
> > @@ -597,8 +636,13 @@ static int dump_node(struct lua_yaml_dumper *dumper)
> >         return dump_table(dumper, &field);
> >      case MP_STR:
> >         str = lua_tolstring(dumper->L, -1, &len);
> > -      if (lua_isnumber(dumper->L, -1)) {
> > -         /* string is convertible to number, quote it to preserve type */
> > +      if ((yaml_get_null(str, len) == YAML_NULL)
> > +         || (yaml_get_bool(str, len) != YAML_NO_MATCH)
> > +         || (lua_isnumber(dumper->L, -1))) {
> > +         /*
> > +          * The string is convertible to a null, a boolean or
> > +          * a number, quote it to preserve its type.
> > +          */
> 
> 6. I think, it is related to my first comment. So why did you
> delete those tests?

No-no, see my answer for the first comment.

> 
> >            style = YAML_SINGLE_QUOTED_SCALAR_STYLE;
> >            break;
> >         }

[tarantool-patches] Re: [PATCH v2 2/3] lua-yaml: fix boolean/null representation in yaml

[Vladislav Shpilevoy]

Thanks for the fixes!

>>> diff --git a/test/app-tap/console.test.lua b/test/app-tap/console.test.lua
>>> index 4f915afd7..68f273234 100755
>>> --- a/test/app-tap/console.test.lua
>>> +++ b/test/app-tap/console.test.lua
>>> @@ -77,11 +77,24 @@ test:is(yaml.decode(client:read(EOL)), '', "clear delimiter")
>>>    --
>>>    -- gh-3476: yaml.encode encodes 'false' and 'true' incorrectly.
>>> +-- gh-3662: yaml.encode encodes booleans with multiline format.
>>> +-- gh-3583: yaml.encode encodes null incorrectly.
>>>    --
>>> -test:is(type(yaml.decode(yaml.encode('false'))), 'string')
>>> -test:is(type(yaml.decode(yaml.encode('true'))), 'string')
>>> -test:is(type(yaml.decode(yaml.encode({a = 'false'})).a), 'string')
>>> -test:is(type(yaml.decode(yaml.encode({a = 'false'})).a), 'string')
>>
>> 1. Are you sure that this is the correct behaviour? So now it is
>> not true that decode(encode(obj)) == obj? Looks utterly confusing.
>> Or why else you removed these tests?
> 
> They are work as before, but now superseded with simpler ones.

It is not obvious. I see a test

     yaml.encode(false) == "--- false\n...\n"

I see another test:

     yaml.decode('false') == false

But where is this? -

     yaml.decode("--- false\n...\n") == false

And why "false" == "--- false\n...\n" according to the tests
in the patch?

>>
>>> +{
>>> +   if (len > 5)
>>> +      return YAML_NO_MATCH;
>>> +   if (strcmp(str, "false") == 0 || strcmp(str, "no") == 0)
>>> +      return YAML_FALSE;
>>> +   if (strcmp(str, "true") == 0 || strcmp(str, "yes") == 0)
>>
>> 3. I think, it is redundant to firstly check length and then
>> call strcmp, which also calculates length (by checking zero
>> byte). We need either use only strcmp(), or use length comparison
>> + memcmp. The same below for the null parser.
> 
> Okay, it was the premature optimization.
> 
> The more important thing is that is was incorrect: "fals" gives
> YAML_FALSE.
> 
> I had fixed it like so, is it okay?
> 
> ```
> if ((len == 5 && memcmp(str, "false", 5) == 0) ||
>      (len == 2 && memcmp(str, "no", 2) == 0)) {
>     if (out != NULL)
>        *out = false;
>     return 0;
> }
> ...
> ```

Yes, this fix is ok, thanks.

> 
>>
>>> +      return YAML_TRUE;
>>> +   return YAML_NO_MATCH;
>>> +}
>>> +
>>> +/**
>>> + * Verify whether a string represents a null literal in YAML.
>>> + *
>>> + * Non-standard: don't match an empty string as null.
>>> + */
>>> +static yaml_type
>>> +yaml_get_null(const char *str, const size_t len){
>>
>> 4. ?
>>
>>      static inline int
>>      yaml_parse_null(const char *str, size_t len, bool *is_null);
> 
> is_null is redundant here, because a return code contain all needed
> information. Agreed except that. Changed.

My point was to standardize parser functions so as to always
return 0/-1 on success/error, and always return an actual value
via an out parameter. But now I see, that -1 here is not really an
error, but rather 'not being of a requested type'. So here we
do not have even a notion of 'error'.

In such a case, please, consider the fixes below. Also, I removed
optionality of out parameter in yaml_parse_bool in order to get rid
of 'if'.

diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc
index 5afcace1b..a4e03a8ba 100644
--- a/third_party/lua-yaml/lyaml.cc
+++ b/third_party/lua-yaml/lyaml.cc
@@ -96,46 +96,43 @@ struct lua_yaml_dumper {
  
  /**
   * Verify whether a string represents a boolean literal in YAML.
- *
   * Non-standard: only subset of YAML 1.1 boolean literals are
   * treated as boolean values.
- *
- * Return 0 at success, -1 at error.
+ * @param str Literal.
+ * @param len Length of @a str.
+ * @param[out] out Result boolean value.
+ * @retval Whether @a str represents a boolean value or not.
   */
-static inline int
-yaml_parse_bool(const char *str, size_t len, bool *out)
+static inline bool
+yaml_is_bool(const char *str, size_t len, bool *out)
  {
     if ((len == 5 && memcmp(str, "false", 5) == 0) ||
         (len == 2 && memcmp(str, "no", 2) == 0)) {
-      if (out != NULL)
-         *out = false;
-      return 0;
+      *out = false;
+      return true;
     }
     if ((len == 4 && memcmp(str, "true", 4) == 0) ||
         (len == 3 && memcmp(str, "yes", 3) == 0)) {
-      if (out != NULL)
-         *out = true;
-      return 0;
+      *out = true;
+      return true;
     }
-   return -1;
+   return false;
  }
  
  /**
   * Verify whether a string represents a null literal in YAML.
- *
   * Non-standard: don't match an empty string, 'Null' and 'NULL' as
   * null.
- *
- * Return 0 at success, -1 at error.
+ * @retval Whether @a str represents NULL or not.
   */
-static inline int
-yaml_parse_null(const char *str, size_t len)
+static inline bool
+yaml_is_null(const char *str, size_t len)
  {
     if (len == 1 && str[0] == '~')
-      return 0;
+      return true;
     if (len == 4 && memcmp(str, "null", 4) == 0)
-      return 0;
-   return -1;
+      return true;
+   return false;
  }
  
  static void generate_error_message(struct lua_yaml_loader *loader) {
@@ -254,9 +251,9 @@ static void load_scalar(struct lua_yaml_loader *loader) {
           return;
        } else if (!strcmp(tag, "bool")) {
           bool value = false;
-         int rc = yaml_parse_bool(str, length, &value);
+         bool rc = yaml_is_bool(str, length, &value);
           (void) rc;
-         assert(rc == 0);
+         assert(rc);
           lua_pushboolean(loader->L, value);
           return;
        } else if (!strcmp(tag, "binary")) {
@@ -275,10 +272,10 @@ static void load_scalar(struct lua_yaml_loader *loader) {
            */
           lua_pushliteral(loader->L, "");
           return;
-      } else if (yaml_parse_null(str, length) == 0) {
+      } else if (yaml_is_null(str, length)) {
           luaL_pushnull(loader->L);
           return;
-      } else if (yaml_parse_bool(str, length, &value) == 0) {
+      } else if (yaml_is_bool(str, length, &value)) {
           lua_pushboolean(loader->L, value);
           return;
        }
@@ -610,6 +607,7 @@ static int dump_node(struct lua_yaml_dumper *dumper)
     yaml_event_t ev;
     yaml_scalar_style_t style = YAML_PLAIN_SCALAR_STYLE;
     int is_binary = 0;
+   bool unused;
     char buf[FPCONV_G_FMT_BUFSIZE];
     struct luaL_field field;
  
@@ -644,9 +642,8 @@ static int dump_node(struct lua_yaml_dumper *dumper)
        return dump_table(dumper, &field);
     case MP_STR:
        str = lua_tolstring(dumper->L, -1, &len);
-      if ((yaml_parse_null(str, len) == 0)
-         || (yaml_parse_bool(str, len, NULL) == 0)
-         || (lua_isnumber(dumper->L, -1))) {
+      if (yaml_is_null(str, len) || yaml_is_bool(str, len, &unused) ||
+          lua_isnumber(dumper->L, -1)) {
           /*
            * The string is convertible to a null, a boolean or
            * a number, quote it to preserve its type.

[tarantool-patches] Re: [PATCH v2 2/3] lua-yaml: fix boolean/null representation in yaml

[Vladislav Shpilevoy]

Hi! Any news here?

On 05/02/2019 20:36, Vladislav Shpilevoy wrote:
> Thanks for the fixes!
> 
>>>> diff --git a/test/app-tap/console.test.lua b/test/app-tap/console.test.lua
>>>> index 4f915afd7..68f273234 100755
>>>> --- a/test/app-tap/console.test.lua
>>>> +++ b/test/app-tap/console.test.lua
>>>> @@ -77,11 +77,24 @@ test:is(yaml.decode(client:read(EOL)), '', "clear delimiter")
>>>>    --
>>>>    -- gh-3476: yaml.encode encodes 'false' and 'true' incorrectly.
>>>> +-- gh-3662: yaml.encode encodes booleans with multiline format.
>>>> +-- gh-3583: yaml.encode encodes null incorrectly.
>>>>    --
>>>> -test:is(type(yaml.decode(yaml.encode('false'))), 'string')
>>>> -test:is(type(yaml.decode(yaml.encode('true'))), 'string')
>>>> -test:is(type(yaml.decode(yaml.encode({a = 'false'})).a), 'string')
>>>> -test:is(type(yaml.decode(yaml.encode({a = 'false'})).a), 'string')
>>>
>>> 1. Are you sure that this is the correct behaviour? So now it is
>>> not true that decode(encode(obj)) == obj? Looks utterly confusing.
>>> Or why else you removed these tests?
>>
>> They are work as before, but now superseded with simpler ones.
> 
> It is not obvious. I see a test
> 
>      yaml.encode(false) == "--- false\n...\n"
> 
> I see another test:
> 
>      yaml.decode('false') == false
> 
> But where is this? -
> 
>      yaml.decode("--- false\n...\n") == false
> 
> And why "false" == "--- false\n...\n" according to the tests
> in the patch?
> 
>>>
>>>> +{
>>>> +   if (len > 5)
>>>> +      return YAML_NO_MATCH;
>>>> +   if (strcmp(str, "false") == 0 || strcmp(str, "no") == 0)
>>>> +      return YAML_FALSE;
>>>> +   if (strcmp(str, "true") == 0 || strcmp(str, "yes") == 0)
>>>
>>> 3. I think, it is redundant to firstly check length and then
>>> call strcmp, which also calculates length (by checking zero
>>> byte). We need either use only strcmp(), or use length comparison
>>> + memcmp. The same below for the null parser.
>>
>> Okay, it was the premature optimization.
>>
>> The more important thing is that is was incorrect: "fals" gives
>> YAML_FALSE.
>>
>> I had fixed it like so, is it okay?
>>
>> ```
>> if ((len == 5 && memcmp(str, "false", 5) == 0) ||
>>      (len == 2 && memcmp(str, "no", 2) == 0)) {
>>     if (out != NULL)
>>        *out = false;
>>     return 0;
>> }
>> ...
>> ```
> 
> Yes, this fix is ok, thanks.
> 
>>
>>>
>>>> +      return YAML_TRUE;
>>>> +   return YAML_NO_MATCH;
>>>> +}
>>>> +
>>>> +/**
>>>> + * Verify whether a string represents a null literal in YAML.
>>>> + *
>>>> + * Non-standard: don't match an empty string as null.
>>>> + */
>>>> +static yaml_type
>>>> +yaml_get_null(const char *str, const size_t len){
>>>
>>> 4. ?
>>>
>>>      static inline int
>>>      yaml_parse_null(const char *str, size_t len, bool *is_null);
>>
>> is_null is redundant here, because a return code contain all needed
>> information. Agreed except that. Changed.
> 
> My point was to standardize parser functions so as to always
> return 0/-1 on success/error, and always return an actual value
> via an out parameter. But now I see, that -1 here is not really an
> error, but rather 'not being of a requested type'. So here we
> do not have even a notion of 'error'.
> 
> In such a case, please, consider the fixes below. Also, I removed
> optionality of out parameter in yaml_parse_bool in order to get rid
> of 'if'.
> 
> diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc
> index 5afcace1b..a4e03a8ba 100644
> --- a/third_party/lua-yaml/lyaml.cc
> +++ b/third_party/lua-yaml/lyaml.cc
> @@ -96,46 +96,43 @@ struct lua_yaml_dumper {
> 
>   /**
>    * Verify whether a string represents a boolean literal in YAML.
> - *
>    * Non-standard: only subset of YAML 1.1 boolean literals are
>    * treated as boolean values.
> - *
> - * Return 0 at success, -1 at error.
> + * @param str Literal.
> + * @param len Length of @a str.
> + * @param[out] out Result boolean value.
> + * @retval Whether @a str represents a boolean value or not.
>    */
> -static inline int
> -yaml_parse_bool(const char *str, size_t len, bool *out)
> +static inline bool
> +yaml_is_bool(const char *str, size_t len, bool *out)
>   {
>      if ((len == 5 && memcmp(str, "false", 5) == 0) ||
>          (len == 2 && memcmp(str, "no", 2) == 0)) {
> -      if (out != NULL)
> -         *out = false;
> -      return 0;
> +      *out = false;
> +      return true;
>      }
>      if ((len == 4 && memcmp(str, "true", 4) == 0) ||
>          (len == 3 && memcmp(str, "yes", 3) == 0)) {
> -      if (out != NULL)
> -         *out = true;
> -      return 0;
> +      *out = true;
> +      return true;
>      }
> -   return -1;
> +   return false;
>   }
> 
>   /**
>    * Verify whether a string represents a null literal in YAML.
> - *
>    * Non-standard: don't match an empty string, 'Null' and 'NULL' as
>    * null.
> - *
> - * Return 0 at success, -1 at error.
> + * @retval Whether @a str represents NULL or not.
>    */
> -static inline int
> -yaml_parse_null(const char *str, size_t len)
> +static inline bool
> +yaml_is_null(const char *str, size_t len)
>   {
>      if (len == 1 && str[0] == '~')
> -      return 0;
> +      return true;
>      if (len == 4 && memcmp(str, "null", 4) == 0)
> -      return 0;
> -   return -1;
> +      return true;
> +   return false;
>   }
> 
>   static void generate_error_message(struct lua_yaml_loader *loader) {
> @@ -254,9 +251,9 @@ static void load_scalar(struct lua_yaml_loader *loader) {
>            return;
>         } else if (!strcmp(tag, "bool")) {
>            bool value = false;
> -         int rc = yaml_parse_bool(str, length, &value);
> +         bool rc = yaml_is_bool(str, length, &value);
>            (void) rc;
> -         assert(rc == 0);
> +         assert(rc);
>            lua_pushboolean(loader->L, value);
>            return;
>         } else if (!strcmp(tag, "binary")) {
> @@ -275,10 +272,10 @@ static void load_scalar(struct lua_yaml_loader *loader) {
>             */
>            lua_pushliteral(loader->L, "");
>            return;
> -      } else if (yaml_parse_null(str, length) == 0) {
> +      } else if (yaml_is_null(str, length)) {
>            luaL_pushnull(loader->L);
>            return;
> -      } else if (yaml_parse_bool(str, length, &value) == 0) {
> +      } else if (yaml_is_bool(str, length, &value)) {
>            lua_pushboolean(loader->L, value);
>            return;
>         }
> @@ -610,6 +607,7 @@ static int dump_node(struct lua_yaml_dumper *dumper)
>      yaml_event_t ev;
>      yaml_scalar_style_t style = YAML_PLAIN_SCALAR_STYLE;
>      int is_binary = 0;
> +   bool unused;
>      char buf[FPCONV_G_FMT_BUFSIZE];
>      struct luaL_field field;
> 
> @@ -644,9 +642,8 @@ static int dump_node(struct lua_yaml_dumper *dumper)
>         return dump_table(dumper, &field);
>      case MP_STR:
>         str = lua_tolstring(dumper->L, -1, &len);
> -      if ((yaml_parse_null(str, len) == 0)
> -         || (yaml_parse_bool(str, len, NULL) == 0)
> -         || (lua_isnumber(dumper->L, -1))) {
> +      if (yaml_is_null(str, len) || yaml_is_bool(str, len, &unused) ||
> +          lua_isnumber(dumper->L, -1)) {
>            /*
>             * The string is convertible to a null, a boolean or
>             * a number, quote it to preserve its type.
> 
> 

[tarantool-patches] Re: [PATCH v2 2/3] lua-yaml: fix boolean/null representation in yaml

[Alexander Turenko]

Hi! Sorry, but no. I'll update the patch on the next week.

WBR, Alexander Turenko.

On Sat, Feb 16, 2019 at 12:06:48AM +0300, Vladislav Shpilevoy wrote:
> Hi! Any news here?
> 
> On 05/02/2019 20:36, Vladislav Shpilevoy wrote:
> > Thanks for the fixes!
> > 
> > > > > diff --git a/test/app-tap/console.test.lua b/test/app-tap/console.test.lua
> > > > > index 4f915afd7..68f273234 100755
> > > > > --- a/test/app-tap/console.test.lua
> > > > > +++ b/test/app-tap/console.test.lua
> > > > > @@ -77,11 +77,24 @@ test:is(yaml.decode(client:read(EOL)), '', "clear delimiter")
> > > > >    --
> > > > >    -- gh-3476: yaml.encode encodes 'false' and 'true' incorrectly.
> > > > > +-- gh-3662: yaml.encode encodes booleans with multiline format.
> > > > > +-- gh-3583: yaml.encode encodes null incorrectly.
> > > > >    --
> > > > > -test:is(type(yaml.decode(yaml.encode('false'))), 'string')
> > > > > -test:is(type(yaml.decode(yaml.encode('true'))), 'string')
> > > > > -test:is(type(yaml.decode(yaml.encode({a = 'false'})).a), 'string')
> > > > > -test:is(type(yaml.decode(yaml.encode({a = 'false'})).a), 'string')
> > > > 
> > > > 1. Are you sure that this is the correct behaviour? So now it is
> > > > not true that decode(encode(obj)) == obj? Looks utterly confusing.
> > > > Or why else you removed these tests?
> > > 
> > > They are work as before, but now superseded with simpler ones.
> > 
> > It is not obvious. I see a test
> > 
> >      yaml.encode(false) == "--- false\n...\n"
> > 
> > I see another test:
> > 
> >      yaml.decode('false') == false
> > 
> > But where is this? -
> > 
> >      yaml.decode("--- false\n...\n") == false
> > 
> > And why "false" == "--- false\n...\n" according to the tests
> > in the patch?
> > 
> > > > 
> > > > > +{
> > > > > +   if (len > 5)
> > > > > +      return YAML_NO_MATCH;
> > > > > +   if (strcmp(str, "false") == 0 || strcmp(str, "no") == 0)
> > > > > +      return YAML_FALSE;
> > > > > +   if (strcmp(str, "true") == 0 || strcmp(str, "yes") == 0)
> > > > 
> > > > 3. I think, it is redundant to firstly check length and then
> > > > call strcmp, which also calculates length (by checking zero
> > > > byte). We need either use only strcmp(), or use length comparison
> > > > + memcmp. The same below for the null parser.
> > > 
> > > Okay, it was the premature optimization.
> > > 
> > > The more important thing is that is was incorrect: "fals" gives
> > > YAML_FALSE.
> > > 
> > > I had fixed it like so, is it okay?
> > > 
> > > ```
> > > if ((len == 5 && memcmp(str, "false", 5) == 0) ||
> > >      (len == 2 && memcmp(str, "no", 2) == 0)) {
> > >     if (out != NULL)
> > >        *out = false;
> > >     return 0;
> > > }
> > > ...
> > > ```
> > 
> > Yes, this fix is ok, thanks.
> > 
> > > 
> > > > 
> > > > > +      return YAML_TRUE;
> > > > > +   return YAML_NO_MATCH;
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * Verify whether a string represents a null literal in YAML.
> > > > > + *
> > > > > + * Non-standard: don't match an empty string as null.
> > > > > + */
> > > > > +static yaml_type
> > > > > +yaml_get_null(const char *str, const size_t len){
> > > > 
> > > > 4. ?
> > > > 
> > > >      static inline int
> > > >      yaml_parse_null(const char *str, size_t len, bool *is_null);
> > > 
> > > is_null is redundant here, because a return code contain all needed
> > > information. Agreed except that. Changed.
> > 
> > My point was to standardize parser functions so as to always
> > return 0/-1 on success/error, and always return an actual value
> > via an out parameter. But now I see, that -1 here is not really an
> > error, but rather 'not being of a requested type'. So here we
> > do not have even a notion of 'error'.
> > 
> > In such a case, please, consider the fixes below. Also, I removed
> > optionality of out parameter in yaml_parse_bool in order to get rid
> > of 'if'.
> > 
> > diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc
> > index 5afcace1b..a4e03a8ba 100644
> > --- a/third_party/lua-yaml/lyaml.cc
> > +++ b/third_party/lua-yaml/lyaml.cc
> > @@ -96,46 +96,43 @@ struct lua_yaml_dumper {
> > 
> >   /**
> >    * Verify whether a string represents a boolean literal in YAML.
> > - *
> >    * Non-standard: only subset of YAML 1.1 boolean literals are
> >    * treated as boolean values.
> > - *
> > - * Return 0 at success, -1 at error.
> > + * @param str Literal.
> > + * @param len Length of @a str.
> > + * @param[out] out Result boolean value.
> > + * @retval Whether @a str represents a boolean value or not.
> >    */
> > -static inline int
> > -yaml_parse_bool(const char *str, size_t len, bool *out)
> > +static inline bool
> > +yaml_is_bool(const char *str, size_t len, bool *out)
> >   {
> >      if ((len == 5 && memcmp(str, "false", 5) == 0) ||
> >          (len == 2 && memcmp(str, "no", 2) == 0)) {
> > -      if (out != NULL)
> > -         *out = false;
> > -      return 0;
> > +      *out = false;
> > +      return true;
> >      }
> >      if ((len == 4 && memcmp(str, "true", 4) == 0) ||
> >          (len == 3 && memcmp(str, "yes", 3) == 0)) {
> > -      if (out != NULL)
> > -         *out = true;
> > -      return 0;
> > +      *out = true;
> > +      return true;
> >      }
> > -   return -1;
> > +   return false;
> >   }
> > 
> >   /**
> >    * Verify whether a string represents a null literal in YAML.
> > - *
> >    * Non-standard: don't match an empty string, 'Null' and 'NULL' as
> >    * null.
> > - *
> > - * Return 0 at success, -1 at error.
> > + * @retval Whether @a str represents NULL or not.
> >    */
> > -static inline int
> > -yaml_parse_null(const char *str, size_t len)
> > +static inline bool
> > +yaml_is_null(const char *str, size_t len)
> >   {
> >      if (len == 1 && str[0] == '~')
> > -      return 0;
> > +      return true;
> >      if (len == 4 && memcmp(str, "null", 4) == 0)
> > -      return 0;
> > -   return -1;
> > +      return true;
> > +   return false;
> >   }
> > 
> >   static void generate_error_message(struct lua_yaml_loader *loader) {
> > @@ -254,9 +251,9 @@ static void load_scalar(struct lua_yaml_loader *loader) {
> >            return;
> >         } else if (!strcmp(tag, "bool")) {
> >            bool value = false;
> > -         int rc = yaml_parse_bool(str, length, &value);
> > +         bool rc = yaml_is_bool(str, length, &value);
> >            (void) rc;
> > -         assert(rc == 0);
> > +         assert(rc);
> >            lua_pushboolean(loader->L, value);
> >            return;
> >         } else if (!strcmp(tag, "binary")) {
> > @@ -275,10 +272,10 @@ static void load_scalar(struct lua_yaml_loader *loader) {
> >             */
> >            lua_pushliteral(loader->L, "");
> >            return;
> > -      } else if (yaml_parse_null(str, length) == 0) {
> > +      } else if (yaml_is_null(str, length)) {
> >            luaL_pushnull(loader->L);
> >            return;
> > -      } else if (yaml_parse_bool(str, length, &value) == 0) {
> > +      } else if (yaml_is_bool(str, length, &value)) {
> >            lua_pushboolean(loader->L, value);
> >            return;
> >         }
> > @@ -610,6 +607,7 @@ static int dump_node(struct lua_yaml_dumper *dumper)
> >      yaml_event_t ev;
> >      yaml_scalar_style_t style = YAML_PLAIN_SCALAR_STYLE;
> >      int is_binary = 0;
> > +   bool unused;
> >      char buf[FPCONV_G_FMT_BUFSIZE];
> >      struct luaL_field field;
> > 
> > @@ -644,9 +642,8 @@ static int dump_node(struct lua_yaml_dumper *dumper)
> >         return dump_table(dumper, &field);
> >      case MP_STR:
> >         str = lua_tolstring(dumper->L, -1, &len);
> > -      if ((yaml_parse_null(str, len) == 0)
> > -         || (yaml_parse_bool(str, len, NULL) == 0)
> > -         || (lua_isnumber(dumper->L, -1))) {
> > +      if (yaml_is_null(str, len) || yaml_is_bool(str, len, &unused) ||
> > +          lua_isnumber(dumper->L, -1)) {
> >            /*
> >             * The string is convertible to a null, a boolean or
> >             * a number, quote it to preserve its type.
> > 
> > 

[tarantool-patches] Re: [PATCH v2 2/3] lua-yaml: fix boolean/null representation in yaml

[Alexander Turenko]

I answered to the comments below and noted the fixes were made. Rebased
the patchset on top of fresh 2.1.

The new patch is at end of the email.

WBR, Alexander Turenko.

On Tue, Feb 05, 2019 at 10:36:40PM +0300, Vladislav Shpilevoy wrote:
> Thanks for the fixes!
> 
> > > > diff --git a/test/app-tap/console.test.lua b/test/app-tap/console.test.lua
> > > > index 4f915afd7..68f273234 100755
> > > > --- a/test/app-tap/console.test.lua
> > > > +++ b/test/app-tap/console.test.lua
> > > > @@ -77,11 +77,24 @@ test:is(yaml.decode(client:read(EOL)), '', "clear delimiter")
> > > >    --
> > > >    -- gh-3476: yaml.encode encodes 'false' and 'true' incorrectly.
> > > > +-- gh-3662: yaml.encode encodes booleans with multiline format.
> > > > +-- gh-3583: yaml.encode encodes null incorrectly.
> > > >    --
> > > > -test:is(type(yaml.decode(yaml.encode('false'))), 'string')
> > > > -test:is(type(yaml.decode(yaml.encode('true'))), 'string')
> > > > -test:is(type(yaml.decode(yaml.encode({a = 'false'})).a), 'string')
> > > > -test:is(type(yaml.decode(yaml.encode({a = 'false'})).a), 'string')
> > > 
> > > 1. Are you sure that this is the correct behaviour? So now it is
> > > not true that decode(encode(obj)) == obj? Looks utterly confusing.
> > > Or why else you removed these tests?
> > 
> > They are work as before, but now superseded with simpler ones.
> 
> It is not obvious. I see a test
> 
>     yaml.encode(false) == "--- false\n...\n"
> 
> I see another test:
> 
>     yaml.decode('false') == false
> 
> But where is this? -
> 
>     yaml.decode("--- false\n...\n") == false

The removed test cases covered one case that is not covered with the new
cases: they decode a yaml document, not just a scalar. And it is
confusing why I remove test cases in the patch. So it worth to just add
them back.

Added.

> 
> And why "false" == "--- false\n...\n" according to the tests
> in the patch?

`---` and `...` is a yaml document start and end, see [1]. A sequence of
documents forms a yaml stream like a sequence of stanzas forms a xml
stream. We decode one document to its value. Multiple documents however
forms a table. Anyway, it does not matter much in context of this issue.

[1]: https://yaml.org/spec/1.2/spec.html#id2800401

> 
> > > 
> > > > +{
> > > > +   if (len > 5)
> > > > +      return YAML_NO_MATCH;
> > > > +   if (strcmp(str, "false") == 0 || strcmp(str, "no") == 0)
> > > > +      return YAML_FALSE;
> > > > +   if (strcmp(str, "true") == 0 || strcmp(str, "yes") == 0)
> > > 
> > > 3. I think, it is redundant to firstly check length and then
> > > call strcmp, which also calculates length (by checking zero
> > > byte). We need either use only strcmp(), or use length comparison
> > > + memcmp. The same below for the null parser.
> > 
> > Okay, it was the premature optimization.
> > 
> > The more important thing is that is was incorrect: "fals" gives
> > YAML_FALSE.
> > 
> > I had fixed it like so, is it okay?
> > 
> > ```
> > if ((len == 5 && memcmp(str, "false", 5) == 0) ||
> >      (len == 2 && memcmp(str, "no", 2) == 0)) {
> >     if (out != NULL)
> >        *out = false;
> >     return 0;
> > }
> > ...
> > ```
> 
> Yes, this fix is ok, thanks.

Ok.

> 
> > 
> > > 
> > > > +      return YAML_TRUE;
> > > > +   return YAML_NO_MATCH;
> > > > +}
> > > > +
> > > > +/**
> > > > + * Verify whether a string represents a null literal in YAML.
> > > > + *
> > > > + * Non-standard: don't match an empty string as null.
> > > > + */
> > > > +static yaml_type
> > > > +yaml_get_null(const char *str, const size_t len){
> > > 
> > > 4. ?
> > > 
> > >      static inline int
> > >      yaml_parse_null(const char *str, size_t len, bool *is_null);
> > 
> > is_null is redundant here, because a return code contain all needed
> > information. Agreed except that. Changed.
> 
> My point was to standardize parser functions so as to always
> return 0/-1 on success/error, and always return an actual value
> via an out parameter. But now I see, that -1 here is not really an
> error, but rather 'not being of a requested type'. So here we
> do not have even a notion of 'error'.
> 
> In such a case, please, consider the fixes below. Also, I removed
> optionality of out parameter in yaml_parse_bool in order to get rid
> of 'if'.

I don't mind. Integrated these changes into the patch (with minor
wording / formatting changes).

> 
> diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc
> index 5afcace1b..a4e03a8ba 100644
> --- a/third_party/lua-yaml/lyaml.cc
> +++ b/third_party/lua-yaml/lyaml.cc
> @@ -96,46 +96,43 @@ struct lua_yaml_dumper {
>  /**
>   * Verify whether a string represents a boolean literal in YAML.
> - *
>   * Non-standard: only subset of YAML 1.1 boolean literals are
>   * treated as boolean values.
> - *
> - * Return 0 at success, -1 at error.
> + * @param str Literal.
> + * @param len Length of @a str.
> + * @param[out] out Result boolean value.
> + * @retval Whether @a str represents a boolean value or not.
>   */
> -static inline int
> -yaml_parse_bool(const char *str, size_t len, bool *out)
> +static inline bool
> +yaml_is_bool(const char *str, size_t len, bool *out)
>  {
>     if ((len == 5 && memcmp(str, "false", 5) == 0) ||
>         (len == 2 && memcmp(str, "no", 2) == 0)) {
> -      if (out != NULL)
> -         *out = false;
> -      return 0;
> +      *out = false;
> +      return true;
>     }
>     if ((len == 4 && memcmp(str, "true", 4) == 0) ||
>         (len == 3 && memcmp(str, "yes", 3) == 0)) {
> -      if (out != NULL)
> -         *out = true;
> -      return 0;
> +      *out = true;
> +      return true;
>     }
> -   return -1;
> +   return false;
>  }
>  /**
>   * Verify whether a string represents a null literal in YAML.
> - *
>   * Non-standard: don't match an empty string, 'Null' and 'NULL' as
>   * null.
> - *
> - * Return 0 at success, -1 at error.
> + * @retval Whether @a str represents NULL or not.
>   */
> -static inline int
> -yaml_parse_null(const char *str, size_t len)
> +static inline bool
> +yaml_is_null(const char *str, size_t len)
>  {
>     if (len == 1 && str[0] == '~')
> -      return 0;
> +      return true;
>     if (len == 4 && memcmp(str, "null", 4) == 0)
> -      return 0;
> -   return -1;
> +      return true;
> +   return false;
>  }
>  static void generate_error_message(struct lua_yaml_loader *loader) {
> @@ -254,9 +251,9 @@ static void load_scalar(struct lua_yaml_loader *loader) {
>           return;
>        } else if (!strcmp(tag, "bool")) {
>           bool value = false;
> -         int rc = yaml_parse_bool(str, length, &value);
> +         bool rc = yaml_is_bool(str, length, &value);
>           (void) rc;
> -         assert(rc == 0);
> +         assert(rc);

I removed the assert() here, because yaml.decode('!!bool null') should
not lead to an assertion fail. I didn't investigate why the behaviour is
to produce `false` rather then raise an error, just keep the behaviour
the same as before. However I don't add a test, because it is unclear
whether the module behaves in this way with intention or by occasion.

>           lua_pushboolean(loader->L, value);
>           return;
>        } else if (!strcmp(tag, "binary")) {
> @@ -275,10 +272,10 @@ static void load_scalar(struct lua_yaml_loader *loader) {
>            */
>           lua_pushliteral(loader->L, "");
>           return;
> -      } else if (yaml_parse_null(str, length) == 0) {
> +      } else if (yaml_is_null(str, length)) {
>           luaL_pushnull(loader->L);
>           return;
> -      } else if (yaml_parse_bool(str, length, &value) == 0) {
> +      } else if (yaml_is_bool(str, length, &value)) {
>           lua_pushboolean(loader->L, value);
>           return;
>        }
> @@ -610,6 +607,7 @@ static int dump_node(struct lua_yaml_dumper *dumper)
>     yaml_event_t ev;
>     yaml_scalar_style_t style = YAML_PLAIN_SCALAR_STYLE;
>     int is_binary = 0;
> +   bool unused;
>     char buf[FPCONV_G_FMT_BUFSIZE];
>     struct luaL_field field;
> @@ -644,9 +642,8 @@ static int dump_node(struct lua_yaml_dumper *dumper)
>        return dump_table(dumper, &field);
>     case MP_STR:
>        str = lua_tolstring(dumper->L, -1, &len);
> -      if ((yaml_parse_null(str, len) == 0)
> -         || (yaml_parse_bool(str, len, NULL) == 0)
> -         || (lua_isnumber(dumper->L, -1))) {
> +      if (yaml_is_null(str, len) || yaml_is_bool(str, len, &unused) ||
> +          lua_isnumber(dumper->L, -1)) {
>           /*
>            * The string is convertible to a null, a boolean or
>            * a number, quote it to preserve its type.
> 

----

commit 8531b4af03016a69d82bff35504ef6ae3ffbdb82
Author: Alexander Turenko <alexander.turenko@tarantool.org>
Date:   Tue Jan 22 01:03:01 2019 +0300

    lua-yaml: fix strings literals encoding in yaml
    
    yaml.encode() now wraps a string literal whose content is equal to a
    null or a boolean value representation in YAML into single quotes. Those
    literals are: 'false', 'no', 'true', 'yes', '~', 'null'.
    
    Reverted the a2d7643c commit to use single quotes instead of multiline
    encoding for 'true' and 'false' string literals.
    
    Follows up #3476
    Closes #3662
    Closes #3583

diff --git a/test/app-tap/console.test.lua b/test/app-tap/console.test.lua
index 4f915afd7..a03df5277 100755
--- a/test/app-tap/console.test.lua
+++ b/test/app-tap/console.test.lua
@@ -21,7 +21,7 @@ local EOL = "\n...\n"
 
 test = tap.test("console")
 
-test:plan(59)
+test:plan(70)
 
 -- Start console and connect to it
 local server = console.listen(CONSOLE_SOCKET)
@@ -77,12 +77,28 @@ test:is(yaml.decode(client:read(EOL)), '', "clear delimiter")
 
 --
 -- gh-3476: yaml.encode encodes 'false' and 'true' incorrectly.
+-- gh-3662: yaml.encode encodes booleans with multiline format.
+-- gh-3583: yaml.encode encodes null incorrectly.
 --
+
 test:is(type(yaml.decode(yaml.encode('false'))), 'string')
 test:is(type(yaml.decode(yaml.encode('true'))), 'string')
 test:is(type(yaml.decode(yaml.encode({a = 'false'})).a), 'string')
 test:is(type(yaml.decode(yaml.encode({a = 'false'})).a), 'string')
 
+test:is(yaml.encode(false), "--- false\n...\n")
+test:is(yaml.encode(true), "--- true\n...\n")
+test:is(yaml.encode('false'), "--- 'false'\n...\n")
+test:is(yaml.encode('true'), "--- 'true'\n...\n")
+test:is(yaml.encode(nil), "--- null\n...\n")
+
+test:is(yaml.decode('false'), false)
+test:is(yaml.decode('no'), false)
+test:is(yaml.decode('true'), true)
+test:is(yaml.decode('yes'), true)
+test:is(yaml.decode('~'), nil)
+test:is(yaml.decode('null'), nil)
+
 box.cfg{
     listen=IPROTO_SOCKET;
     memtx_memory = 107374182,
diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc
index bd876ab29..46c98bde1 100644
--- a/third_party/lua-yaml/lyaml.cc
+++ b/third_party/lua-yaml/lyaml.cc
@@ -94,6 +94,55 @@ struct lua_yaml_dumper {
    luaL_Buffer yamlbuf;
 };
 
+/**
+ * Verify whether a string represents a boolean literal in YAML.
+ *
+ * Non-standard: only subset of YAML 1.1 boolean literals are
+ * treated as boolean values.
+ *
+ * @param str Literal to check.
+ * @param len Length of @a str.
+ * @param[out] out Result boolean value.
+ *
+ * @retval Whether @a str represents a boolean value.
+ */
+static inline bool
+yaml_is_bool(const char *str, size_t len, bool *out)
+{
+   if ((len == 5 && memcmp(str, "false", 5) == 0) ||
+       (len == 2 && memcmp(str, "no", 2) == 0)) {
+      *out = false;
+      return true;
+   }
+   if ((len == 4 && memcmp(str, "true", 4) == 0) ||
+       (len == 3 && memcmp(str, "yes", 3) == 0)) {
+      *out = true;
+      return true;
+   }
+   return false;
+}
+
+/**
+ * Verify whether a string represents a null literal in YAML.
+ *
+ * Non-standard: don't match an empty string, 'Null' and 'NULL' as
+ * null.
+ *
+ * @param str Literal to check.
+ * @param len Length of @a str.
+ *
+ * @retval Whether @a str represents a null value.
+ */
+static inline bool
+yaml_is_null(const char *str, size_t len)
+{
+   if (len == 1 && str[0] == '~')
+      return true;
+   if (len == 4 && memcmp(str, "null", 4) == 0)
+      return true;
+   return false;
+}
+
 static void generate_error_message(struct lua_yaml_loader *loader) {
    char buf[256];
    luaL_Buffer b;
@@ -209,7 +258,9 @@ static void load_scalar(struct lua_yaml_loader *loader) {
          lua_pushnumber(loader->L, dval);
          return;
       } else if (!strcmp(tag, "bool")) {
-         lua_pushboolean(loader->L, !strcmp(str, "true") || !strcmp(str, "yes"));
+         bool value = false;
+         yaml_is_bool(str, length, &value);
+         lua_pushboolean(loader->L, value);
          return;
       } else if (!strcmp(tag, "binary")) {
          frombase64(loader->L, (const unsigned char *)str, length);
@@ -218,20 +269,20 @@ static void load_scalar(struct lua_yaml_loader *loader) {
    }
 
    if (loader->event.data.scalar.style == YAML_PLAIN_SCALAR_STYLE) {
-      if (!strcmp(str, "~")) {
-         luaL_pushnull(loader->L);
-         return;
-      } else if (!strcmp(str, "true") || !strcmp(str, "yes")) {
-         lua_pushboolean(loader->L, 1);
-         return;
-      } else if (!strcmp(str, "false") || !strcmp(str, "no")) {
-         lua_pushboolean(loader->L, 0);
+      bool value;
+      if (!length) {
+         /*
+          * Non-standard: an empty value/document is null
+          * according to the standard, but we decode it as an
+          * empty string.
+          */
+         lua_pushliteral(loader->L, "");
          return;
-      } else if (!strcmp(str, "null")) {
+      } else if (yaml_is_null(str, length)) {
          luaL_pushnull(loader->L);
          return;
-      } else if (!length) {
-         lua_pushliteral(loader->L, "");
+      } else if (yaml_is_bool(str, length, &value)) {
+         lua_pushboolean(loader->L, value);
          return;
       }
 
@@ -547,7 +598,6 @@ static int yaml_is_flow_mode(struct lua_yaml_dumper *dumper) {
                (evp->type == YAML_MAPPING_START_EVENT &&
                 evp->data.mapping_start.style == YAML_FLOW_MAPPING_STYLE)) {
             return 1;
-            break;
          }
       }
    }
@@ -564,6 +614,8 @@ static int dump_node(struct lua_yaml_dumper *dumper)
    int is_binary = 0;
    char buf[FPCONV_G_FMT_BUFSIZE];
    struct luaL_field field;
+   bool unused;
+   (void) unused;
 
    int top = lua_gettop(dumper->L);
    luaL_checkfield(dumper->L, dumper->cfg, top, &field);
@@ -596,8 +648,12 @@ static int dump_node(struct lua_yaml_dumper *dumper)
       return dump_table(dumper, &field);
    case MP_STR:
       str = lua_tolstring(dumper->L, -1, &len);
-      if (lua_isnumber(dumper->L, -1)) {
-         /* string is convertible to number, quote it to preserve type */
+      if (yaml_is_null(str, len) || yaml_is_bool(str, len, &unused) ||
+          lua_isnumber(dumper->L, -1)) {
+         /*
+          * The string is convertible to a null, a boolean or
+          * a number, quote it to preserve its type.
+          */
          style = YAML_SINGLE_QUOTED_SCALAR_STYLE;
          break;
       }
@@ -605,12 +661,10 @@ static int dump_node(struct lua_yaml_dumper *dumper)
       if (utf8_check_printable(str, len)) {
          if (yaml_is_flow_mode(dumper)) {
             style = YAML_SINGLE_QUOTED_SCALAR_STYLE;
-         } else if (strstr(str, "\n\n") != NULL || strcmp(str, "true") == 0 ||
-		    strcmp(str, "false") == 0) {
+         } else if (strstr(str, "\n\n") != 0) {
             /*
              * Tarantool-specific: use literal style for string
-             * with empty lines and strings representing boolean
-             * types.
+             * with empty lines.
              * Useful for tutorial().
              */
             style = YAML_LITERAL_SCALAR_STYLE;

[tarantool-patches] Re: [PATCH v2 2/3] lua-yaml: fix boolean/null representation in yaml

[Vladislav Shpilevoy]

LGTM.

On 18/02/2019 21:55, Alexander Turenko wrote:
> I answered to the comments below and noted the fixes were made. Rebased
> the patchset on top of fresh 2.1.
> 
> The new patch is at end of the email.
> 
> WBR, Alexander Turenko.
> 
> On Tue, Feb 05, 2019 at 10:36:40PM +0300, Vladislav Shpilevoy wrote:
>> Thanks for the fixes!
>>
>>>>> diff --git a/test/app-tap/console.test.lua b/test/app-tap/console.test.lua
>>>>> index 4f915afd7..68f273234 100755
>>>>> --- a/test/app-tap/console.test.lua
>>>>> +++ b/test/app-tap/console.test.lua
>>>>> @@ -77,11 +77,24 @@ test:is(yaml.decode(client:read(EOL)), '', "clear delimiter")
>>>>>     --
>>>>>     -- gh-3476: yaml.encode encodes 'false' and 'true' incorrectly.
>>>>> +-- gh-3662: yaml.encode encodes booleans with multiline format.
>>>>> +-- gh-3583: yaml.encode encodes null incorrectly.
>>>>>     --
>>>>> -test:is(type(yaml.decode(yaml.encode('false'))), 'string')
>>>>> -test:is(type(yaml.decode(yaml.encode('true'))), 'string')
>>>>> -test:is(type(yaml.decode(yaml.encode({a = 'false'})).a), 'string')
>>>>> -test:is(type(yaml.decode(yaml.encode({a = 'false'})).a), 'string')
>>>>
>>>> 1. Are you sure that this is the correct behaviour? So now it is
>>>> not true that decode(encode(obj)) == obj? Looks utterly confusing.
>>>> Or why else you removed these tests?
>>>
>>> They are work as before, but now superseded with simpler ones.
>>
>> It is not obvious. I see a test
>>
>>      yaml.encode(false) == "--- false\n...\n"
>>
>> I see another test:
>>
>>      yaml.decode('false') == false
>>
>> But where is this? -
>>
>>      yaml.decode("--- false\n...\n") == false
> 
> The removed test cases covered one case that is not covered with the new
> cases: they decode a yaml document, not just a scalar. And it is
> confusing why I remove test cases in the patch. So it worth to just add
> them back.
> 
> Added.
> 
>>
>> And why "false" == "--- false\n...\n" according to the tests
>> in the patch?
> 
> `---` and `...` is a yaml document start and end, see [1]. A sequence of
> documents forms a yaml stream like a sequence of stanzas forms a xml
> stream. We decode one document to its value. Multiple documents however
> forms a table. Anyway, it does not matter much in context of this issue.
> 
> [1]: https://yaml.org/spec/1.2/spec.html#id2800401
> 
>>
>>>>
>>>>> +{
>>>>> +   if (len > 5)
>>>>> +      return YAML_NO_MATCH;
>>>>> +   if (strcmp(str, "false") == 0 || strcmp(str, "no") == 0)
>>>>> +      return YAML_FALSE;
>>>>> +   if (strcmp(str, "true") == 0 || strcmp(str, "yes") == 0)
>>>>
>>>> 3. I think, it is redundant to firstly check length and then
>>>> call strcmp, which also calculates length (by checking zero
>>>> byte). We need either use only strcmp(), or use length comparison
>>>> + memcmp. The same below for the null parser.
>>>
>>> Okay, it was the premature optimization.
>>>
>>> The more important thing is that is was incorrect: "fals" gives
>>> YAML_FALSE.
>>>
>>> I had fixed it like so, is it okay?
>>>
>>> ```
>>> if ((len == 5 && memcmp(str, "false", 5) == 0) ||
>>>       (len == 2 && memcmp(str, "no", 2) == 0)) {
>>>      if (out != NULL)
>>>         *out = false;
>>>      return 0;
>>> }
>>> ...
>>> ```
>>
>> Yes, this fix is ok, thanks.
> 
> Ok.
> 
>>
>>>
>>>>
>>>>> +      return YAML_TRUE;
>>>>> +   return YAML_NO_MATCH;
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * Verify whether a string represents a null literal in YAML.
>>>>> + *
>>>>> + * Non-standard: don't match an empty string as null.
>>>>> + */
>>>>> +static yaml_type
>>>>> +yaml_get_null(const char *str, const size_t len){
>>>>
>>>> 4. ?
>>>>
>>>>       static inline int
>>>>       yaml_parse_null(const char *str, size_t len, bool *is_null);
>>>
>>> is_null is redundant here, because a return code contain all needed
>>> information. Agreed except that. Changed.
>>
>> My point was to standardize parser functions so as to always
>> return 0/-1 on success/error, and always return an actual value
>> via an out parameter. But now I see, that -1 here is not really an
>> error, but rather 'not being of a requested type'. So here we
>> do not have even a notion of 'error'.
>>
>> In such a case, please, consider the fixes below. Also, I removed
>> optionality of out parameter in yaml_parse_bool in order to get rid
>> of 'if'.
> 
> I don't mind. Integrated these changes into the patch (with minor
> wording / formatting changes).
> 
>>
>> diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc
>> index 5afcace1b..a4e03a8ba 100644
>> --- a/third_party/lua-yaml/lyaml.cc
>> +++ b/third_party/lua-yaml/lyaml.cc
>> @@ -96,46 +96,43 @@ struct lua_yaml_dumper {
>>   /**
>>    * Verify whether a string represents a boolean literal in YAML.
>> - *
>>    * Non-standard: only subset of YAML 1.1 boolean literals are
>>    * treated as boolean values.
>> - *
>> - * Return 0 at success, -1 at error.
>> + * @param str Literal.
>> + * @param len Length of @a str.
>> + * @param[out] out Result boolean value.
>> + * @retval Whether @a str represents a boolean value or not.
>>    */
>> -static inline int
>> -yaml_parse_bool(const char *str, size_t len, bool *out)
>> +static inline bool
>> +yaml_is_bool(const char *str, size_t len, bool *out)
>>   {
>>      if ((len == 5 && memcmp(str, "false", 5) == 0) ||
>>          (len == 2 && memcmp(str, "no", 2) == 0)) {
>> -      if (out != NULL)
>> -         *out = false;
>> -      return 0;
>> +      *out = false;
>> +      return true;
>>      }
>>      if ((len == 4 && memcmp(str, "true", 4) == 0) ||
>>          (len == 3 && memcmp(str, "yes", 3) == 0)) {
>> -      if (out != NULL)
>> -         *out = true;
>> -      return 0;
>> +      *out = true;
>> +      return true;
>>      }
>> -   return -1;
>> +   return false;
>>   }
>>   /**
>>    * Verify whether a string represents a null literal in YAML.
>> - *
>>    * Non-standard: don't match an empty string, 'Null' and 'NULL' as
>>    * null.
>> - *
>> - * Return 0 at success, -1 at error.
>> + * @retval Whether @a str represents NULL or not.
>>    */
>> -static inline int
>> -yaml_parse_null(const char *str, size_t len)
>> +static inline bool
>> +yaml_is_null(const char *str, size_t len)
>>   {
>>      if (len == 1 && str[0] == '~')
>> -      return 0;
>> +      return true;
>>      if (len == 4 && memcmp(str, "null", 4) == 0)
>> -      return 0;
>> -   return -1;
>> +      return true;
>> +   return false;
>>   }
>>   static void generate_error_message(struct lua_yaml_loader *loader) {
>> @@ -254,9 +251,9 @@ static void load_scalar(struct lua_yaml_loader *loader) {
>>            return;
>>         } else if (!strcmp(tag, "bool")) {
>>            bool value = false;
>> -         int rc = yaml_parse_bool(str, length, &value);
>> +         bool rc = yaml_is_bool(str, length, &value);
>>            (void) rc;
>> -         assert(rc == 0);
>> +         assert(rc);
> 
> I removed the assert() here, because yaml.decode('!!bool null') should
> not lead to an assertion fail. I didn't investigate why the behaviour is
> to produce `false` rather then raise an error, just keep the behaviour
> the same as before. However I don't add a test, because it is unclear
> whether the module behaves in this way with intention or by occasion.
> 
>>            lua_pushboolean(loader->L, value);
>>            return;
>>         } else if (!strcmp(tag, "binary")) {
>> @@ -275,10 +272,10 @@ static void load_scalar(struct lua_yaml_loader *loader) {
>>             */
>>            lua_pushliteral(loader->L, "");
>>            return;
>> -      } else if (yaml_parse_null(str, length) == 0) {
>> +      } else if (yaml_is_null(str, length)) {
>>            luaL_pushnull(loader->L);
>>            return;
>> -      } else if (yaml_parse_bool(str, length, &value) == 0) {
>> +      } else if (yaml_is_bool(str, length, &value)) {
>>            lua_pushboolean(loader->L, value);
>>            return;
>>         }
>> @@ -610,6 +607,7 @@ static int dump_node(struct lua_yaml_dumper *dumper)
>>      yaml_event_t ev;
>>      yaml_scalar_style_t style = YAML_PLAIN_SCALAR_STYLE;
>>      int is_binary = 0;
>> +   bool unused;
>>      char buf[FPCONV_G_FMT_BUFSIZE];
>>      struct luaL_field field;
>> @@ -644,9 +642,8 @@ static int dump_node(struct lua_yaml_dumper *dumper)
>>         return dump_table(dumper, &field);
>>      case MP_STR:
>>         str = lua_tolstring(dumper->L, -1, &len);
>> -      if ((yaml_parse_null(str, len) == 0)
>> -         || (yaml_parse_bool(str, len, NULL) == 0)
>> -         || (lua_isnumber(dumper->L, -1))) {
>> +      if (yaml_is_null(str, len) || yaml_is_bool(str, len, &unused) ||
>> +          lua_isnumber(dumper->L, -1)) {
>>            /*
>>             * The string is convertible to a null, a boolean or
>>             * a number, quote it to preserve its type.
>>
> 
> ----
> 
> commit 8531b4af03016a69d82bff35504ef6ae3ffbdb82
> Author: Alexander Turenko <alexander.turenko@tarantool.org>
> Date:   Tue Jan 22 01:03:01 2019 +0300
> 
>      lua-yaml: fix strings literals encoding in yaml
>      
>      yaml.encode() now wraps a string literal whose content is equal to a
>      null or a boolean value representation in YAML into single quotes. Those
>      literals are: 'false', 'no', 'true', 'yes', '~', 'null'.
>      
>      Reverted the a2d7643c commit to use single quotes instead of multiline
>      encoding for 'true' and 'false' string literals.
>      
>      Follows up #3476
>      Closes #3662
>      Closes #3583
> 
> diff --git a/test/app-tap/console.test.lua b/test/app-tap/console.test.lua
> index 4f915afd7..a03df5277 100755
> --- a/test/app-tap/console.test.lua
> +++ b/test/app-tap/console.test.lua
> @@ -21,7 +21,7 @@ local EOL = "\n...\n"
>   
>   test = tap.test("console")
>   
> -test:plan(59)
> +test:plan(70)
>   
>   -- Start console and connect to it
>   local server = console.listen(CONSOLE_SOCKET)
> @@ -77,12 +77,28 @@ test:is(yaml.decode(client:read(EOL)), '', "clear delimiter")
>   
>   --
>   -- gh-3476: yaml.encode encodes 'false' and 'true' incorrectly.
> +-- gh-3662: yaml.encode encodes booleans with multiline format.
> +-- gh-3583: yaml.encode encodes null incorrectly.
>   --
> +
>   test:is(type(yaml.decode(yaml.encode('false'))), 'string')
>   test:is(type(yaml.decode(yaml.encode('true'))), 'string')
>   test:is(type(yaml.decode(yaml.encode({a = 'false'})).a), 'string')
>   test:is(type(yaml.decode(yaml.encode({a = 'false'})).a), 'string')
>   
> +test:is(yaml.encode(false), "--- false\n...\n")
> +test:is(yaml.encode(true), "--- true\n...\n")
> +test:is(yaml.encode('false'), "--- 'false'\n...\n")
> +test:is(yaml.encode('true'), "--- 'true'\n...\n")
> +test:is(yaml.encode(nil), "--- null\n...\n")
> +
> +test:is(yaml.decode('false'), false)
> +test:is(yaml.decode('no'), false)
> +test:is(yaml.decode('true'), true)
> +test:is(yaml.decode('yes'), true)
> +test:is(yaml.decode('~'), nil)
> +test:is(yaml.decode('null'), nil)
> +
>   box.cfg{
>       listen=IPROTO_SOCKET;
>       memtx_memory = 107374182,
> diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc
> index bd876ab29..46c98bde1 100644
> --- a/third_party/lua-yaml/lyaml.cc
> +++ b/third_party/lua-yaml/lyaml.cc
> @@ -94,6 +94,55 @@ struct lua_yaml_dumper {
>      luaL_Buffer yamlbuf;
>   };
>   
> +/**
> + * Verify whether a string represents a boolean literal in YAML.
> + *
> + * Non-standard: only subset of YAML 1.1 boolean literals are
> + * treated as boolean values.
> + *
> + * @param str Literal to check.
> + * @param len Length of @a str.
> + * @param[out] out Result boolean value.
> + *
> + * @retval Whether @a str represents a boolean value.
> + */
> +static inline bool
> +yaml_is_bool(const char *str, size_t len, bool *out)
> +{
> +   if ((len == 5 && memcmp(str, "false", 5) == 0) ||
> +       (len == 2 && memcmp(str, "no", 2) == 0)) {
> +      *out = false;
> +      return true;
> +   }
> +   if ((len == 4 && memcmp(str, "true", 4) == 0) ||
> +       (len == 3 && memcmp(str, "yes", 3) == 0)) {
> +      *out = true;
> +      return true;
> +   }
> +   return false;
> +}
> +
> +/**
> + * Verify whether a string represents a null literal in YAML.
> + *
> + * Non-standard: don't match an empty string, 'Null' and 'NULL' as
> + * null.
> + *
> + * @param str Literal to check.
> + * @param len Length of @a str.
> + *
> + * @retval Whether @a str represents a null value.
> + */
> +static inline bool
> +yaml_is_null(const char *str, size_t len)
> +{
> +   if (len == 1 && str[0] == '~')
> +      return true;
> +   if (len == 4 && memcmp(str, "null", 4) == 0)
> +      return true;
> +   return false;
> +}
> +
>   static void generate_error_message(struct lua_yaml_loader *loader) {
>      char buf[256];
>      luaL_Buffer b;
> @@ -209,7 +258,9 @@ static void load_scalar(struct lua_yaml_loader *loader) {
>            lua_pushnumber(loader->L, dval);
>            return;
>         } else if (!strcmp(tag, "bool")) {
> -         lua_pushboolean(loader->L, !strcmp(str, "true") || !strcmp(str, "yes"));
> +         bool value = false;
> +         yaml_is_bool(str, length, &value);
> +         lua_pushboolean(loader->L, value);
>            return;
>         } else if (!strcmp(tag, "binary")) {
>            frombase64(loader->L, (const unsigned char *)str, length);
> @@ -218,20 +269,20 @@ static void load_scalar(struct lua_yaml_loader *loader) {
>      }
>   
>      if (loader->event.data.scalar.style == YAML_PLAIN_SCALAR_STYLE) {
> -      if (!strcmp(str, "~")) {
> -         luaL_pushnull(loader->L);
> -         return;
> -      } else if (!strcmp(str, "true") || !strcmp(str, "yes")) {
> -         lua_pushboolean(loader->L, 1);
> -         return;
> -      } else if (!strcmp(str, "false") || !strcmp(str, "no")) {
> -         lua_pushboolean(loader->L, 0);
> +      bool value;
> +      if (!length) {
> +         /*
> +          * Non-standard: an empty value/document is null
> +          * according to the standard, but we decode it as an
> +          * empty string.
> +          */
> +         lua_pushliteral(loader->L, "");
>            return;
> -      } else if (!strcmp(str, "null")) {
> +      } else if (yaml_is_null(str, length)) {
>            luaL_pushnull(loader->L);
>            return;
> -      } else if (!length) {
> -         lua_pushliteral(loader->L, "");
> +      } else if (yaml_is_bool(str, length, &value)) {
> +         lua_pushboolean(loader->L, value);
>            return;
>         }
>   
> @@ -547,7 +598,6 @@ static int yaml_is_flow_mode(struct lua_yaml_dumper *dumper) {
>                  (evp->type == YAML_MAPPING_START_EVENT &&
>                   evp->data.mapping_start.style == YAML_FLOW_MAPPING_STYLE)) {
>               return 1;
> -            break;
>            }
>         }
>      }
> @@ -564,6 +614,8 @@ static int dump_node(struct lua_yaml_dumper *dumper)
>      int is_binary = 0;
>      char buf[FPCONV_G_FMT_BUFSIZE];
>      struct luaL_field field;
> +   bool unused;
> +   (void) unused;
>   
>      int top = lua_gettop(dumper->L);
>      luaL_checkfield(dumper->L, dumper->cfg, top, &field);
> @@ -596,8 +648,12 @@ static int dump_node(struct lua_yaml_dumper *dumper)
>         return dump_table(dumper, &field);
>      case MP_STR:
>         str = lua_tolstring(dumper->L, -1, &len);
> -      if (lua_isnumber(dumper->L, -1)) {
> -         /* string is convertible to number, quote it to preserve type */
> +      if (yaml_is_null(str, len) || yaml_is_bool(str, len, &unused) ||
> +          lua_isnumber(dumper->L, -1)) {
> +         /*
> +          * The string is convertible to a null, a boolean or
> +          * a number, quote it to preserve its type.
> +          */
>            style = YAML_SINGLE_QUOTED_SCALAR_STYLE;
>            break;
>         }
> @@ -605,12 +661,10 @@ static int dump_node(struct lua_yaml_dumper *dumper)
>         if (utf8_check_printable(str, len)) {
>            if (yaml_is_flow_mode(dumper)) {
>               style = YAML_SINGLE_QUOTED_SCALAR_STYLE;
> -         } else if (strstr(str, "\n\n") != NULL || strcmp(str, "true") == 0 ||
> -		    strcmp(str, "false") == 0) {
> +         } else if (strstr(str, "\n\n") != 0) {
>               /*
>                * Tarantool-specific: use literal style for string
> -             * with empty lines and strings representing boolean
> -             * types.
> +             * with empty lines.
>                * Useful for tutorial().
>                */
>               style = YAML_LITERAL_SCALAR_STYLE;
> 

[tarantool-patches] [PATCH v2 3/3] lua-yaml: treat an empty document/value as null

[Alexander Turenko]

It improves our compatibility with the YAML standard.

yaml.encode('') result changed from

---
...

to

--- ''
...

yaml.decode('') returns zero values count before, now it returns
box.NULL. yaml.decode('- ') returns {''} before, now {box.NULL}.

This commit touches many tests and test result files, which use console,
without behaviour changes. It also adds two test cases to
app-tap/console.test.lua.
---
 test/app-tap/console.test.lua   |   16 +-
 test/app/fio.result             |    2 +-
 test/app/socket.result          |   26 +-
 test/box/access.result          |    8 +-
 test/box/role.result            |    8 +-
 test/box/sequence.result        |    2 +-
 test/vinyl/errinj_tx.result     |   10 +-
 test/vinyl/hermitage.result     |  122 ++--
 test/vinyl/mvcc.result          | 1066 +++++++++++++++----------------
 test/vinyl/mvcc.test.lua        |    4 +-
 test/vinyl/tx_conflict.result   |    2 +-
 test/vinyl/tx_conflict.test.lua |    2 +-
 test/vinyl/tx_gap_lock.result   |  189 +++---
 test/vinyl/tx_gap_lock.test.lua |    3 +-
 test/vinyl/tx_serial.result     |    2 +-
 test/vinyl/tx_serial.test.lua   |    2 +-
 third_party/lua-yaml/lyaml.cc   |   22 +-
 17 files changed, 747 insertions(+), 739 deletions(-)

diff --git a/test/app-tap/console.test.lua b/test/app-tap/console.test.lua
index 68f273234..b694a2787 100755
--- a/test/app-tap/console.test.lua
+++ b/test/app-tap/console.test.lua
@@ -21,7 +21,7 @@ local EOL = "\n...\n"
 
 test = tap.test("console")
 
-test:plan(68)
+test:plan(74)
 
 -- Start console and connect to it
 local server = console.listen(CONSOLE_SOCKET)
@@ -68,12 +68,12 @@ client_info = nil
 
 -- Check console.delimiter()
 client:write("require('console').delimiter(';')\n")
-test:is(yaml.decode(client:read(EOL)), '', "set delimiter to ';'")
+test:is(yaml.decode(client:read(EOL)), nil, "set delimiter to ';'")
 test:is(state.delimiter, ';', "state.delimiter is ';'")
 client:write("require('console').delimiter();\n")
 test:is(yaml.decode(client:read(EOL))[1], ';', "get delimiter is ';'")
 client:write("require('console').delimiter('');\n")
-test:is(yaml.decode(client:read(EOL)), '', "clear delimiter")
+test:is(yaml.decode(client:read(EOL)), nil, "clear delimiter")
 
 --
 -- gh-3476: yaml.encode encodes 'false' and 'true' incorrectly.
@@ -86,16 +86,26 @@ test:is(yaml.encode(true), "--- true\n...\n")
 test:is(yaml.encode('false'), "--- 'false'\n...\n")
 test:is(yaml.encode('true'), "--- 'true'\n...\n")
 test:is(yaml.encode(nil), "--- null\n...\n")
+test:is(yaml.encode('false'), "--- 'false'\n...\n")
+test:is(yaml.encode('true'), "--- 'true'\n...\n")
+test:is(yaml.encode(''), "--- ''\n...\n")
 
 test:is(yaml.decode('false'), false)
 test:is(yaml.decode('no'), false)
 test:is(yaml.decode('true'), true)
 test:is(yaml.decode('yes'), true)
 test:is(yaml.decode('~'), nil)
+test:is(yaml.decode(''), nil)
 test:is(yaml.decode('null'), nil)
 test:is(yaml.decode('Null'), nil)
 test:is(yaml.decode('NULL'), nil)
 
+-- Verify that yaml.decode() correctly handles nested nulls.
+test:is(yaml.decode('-')[1], nil)
+
+-- Verify that yaml.decode() returns null, not zero values count.
+test:is(select('#', yaml.decode('')), 1)
+
 box.cfg{
     listen=IPROTO_SOCKET;
     memtx_memory = 107374182,
diff --git a/test/app/fio.result b/test/app/fio.result
index 486cb8043..2f3530509 100644
--- a/test/app/fio.result
+++ b/test/app/fio.result
@@ -69,7 +69,7 @@ err:match("basename") ~= nil
 ...
 fio.basename('/')
 ---
-- 
+- ''
 ...
 fio.basename('abc')
 ---
diff --git a/test/app/socket.result b/test/app/socket.result
index 1209ec218..35c53d253 100644
--- a/test/app/socket.result
+++ b/test/app/socket.result
@@ -1235,7 +1235,7 @@ c:error()
 ...
 x, type(x), #x
 ---
-- 
+- ''
 - string
 - 0
 ...
@@ -1248,7 +1248,7 @@ c:error()
 ...
 x, type(x), #x
 ---
-- 
+- ''
 - string
 - 0
 ...
@@ -1285,7 +1285,7 @@ c:error()
 ...
 x, type(x), #x
 ---
-- 
+- ''
 - string
 - 0
 ...
@@ -1298,7 +1298,7 @@ c:error()
 ...
 x, type(x), #x
 ---
-- 
+- ''
 - string
 - 0
 ...
@@ -1311,7 +1311,7 @@ c:error()
 ...
 x, type(x), #x
 ---
-- 
+- ''
 - string
 - 0
 ...
@@ -1975,7 +1975,7 @@ c:receive(4)
 ...
 c:receive("*l")
 ---
-- 
+- ''
 ...
 wch:put("Fu")
 ---
@@ -1999,19 +1999,19 @@ c:receive()
 ---
 - null
 - closed
-- 
+- ''
 ...
 c:receive(10)
 ---
 - null
 - closed
-- 
+- ''
 ...
 c:receive("*a")
 ---
 - null
 - closed
-- 
+- ''
 ...
 c:close()
 ---
@@ -2345,7 +2345,7 @@ received_message == '' -- expected true
 ...
 received_message
 ---
-- 
+- ''
 ...
 e == 0 -- expected true
 ---
@@ -2368,7 +2368,7 @@ received_message == '' -- expected true
 ...
 received_message
 ---
-- 
+- ''
 ...
 from ~= nil -- expected true
 ---
@@ -2445,7 +2445,7 @@ received_message == '' -- expected true
 ...
 received_message
 ---
-- 
+- ''
 ...
 e == 0 -- expected true
 ---
@@ -2468,7 +2468,7 @@ received_message == '' -- expected true
 ...
 received_message
 ---
-- 
+- ''
 ...
 from ~= nil -- expected true
 ---
diff --git a/test/box/access.result b/test/box/access.result
index 9c190240f..269881bc1 100644
--- a/test/box/access.result
+++ b/test/box/access.result
@@ -999,7 +999,7 @@ box.schema.user.info('test_user')
     - test_space
   - - session,usage
     - universe
-    - 
+    - ''
   - - alter
     - user
     - test_user
@@ -1032,7 +1032,7 @@ box.schema.user.info('test_user')
     - public
   - - session,usage
     - universe
-    - 
+    - ''
   - - alter
     - user
     - test_user
@@ -1350,7 +1350,7 @@ e.type, e.access_type, e.object_type, e.message
 obj_type, obj_name, op_type
 ---
 - universe
-- 
+- ''
 - Usage
 ...
 euid, auid
@@ -1370,7 +1370,7 @@ c = (require 'net.box').connect(LISTEN.host, LISTEN.service, {user="test_user",
 obj_type, obj_name, op_type
 ---
 - universe
-- 
+- ''
 - Session
 ...
 euid, auid
diff --git a/test/box/role.result b/test/box/role.result
index 3a54e2460..9b044667a 100644
--- a/test/box/role.result
+++ b/test/box/role.result
@@ -28,7 +28,7 @@ box.schema.role.info('iddqd')
 ---
 - - - execute
     - universe
-    - 
+    - ''
 ...
 box.schema.role.revoke('iddqd', 'execute', 'universe')
 ---
@@ -48,7 +48,7 @@ box.schema.user.info('tester')
     - public
   - - session,usage
     - universe
-    - 
+    - ''
   - - alter
     - user
     - tester
@@ -66,7 +66,7 @@ box.schema.user.info('tester')
     - iddqd
   - - session,usage
     - universe
-    - 
+    - ''
   - - alter
     - user
     - tester
@@ -940,7 +940,7 @@ box.schema.user.info('test_user')
     - public
   - - session,usage
     - universe
-    - 
+    - ''
   - - alter
     - user
     - test_user
diff --git a/test/box/sequence.result b/test/box/sequence.result
index b3907659f..f429d7dae 100644
--- a/test/box/sequence.result
+++ b/test/box/sequence.result
@@ -1361,7 +1361,7 @@ box.schema.user.info()
     - _priv
   - - session,usage
     - universe
-    - 
+    - ''
   - - alter
     - user
     - user
diff --git a/test/vinyl/errinj_tx.result b/test/vinyl/errinj_tx.result
index a7583b2e2..2df9bc6d0 100644
--- a/test/vinyl/errinj_tx.result
+++ b/test/vinyl/errinj_tx.result
@@ -207,28 +207,28 @@ c0 = txn_proxy.new()
 ...
 c0:begin()
 ---
-- 
+- null
 ...
 c1 = txn_proxy.new()
 ---
 ...
 c1:begin()
 ---
-- 
+- null
 ...
 c2 = txn_proxy.new()
 ---
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c3 = txn_proxy.new()
 ---
 ...
 c3:begin()
 ---
-- 
+- null
 ...
 --
 -- Prepared transactions
@@ -336,7 +336,7 @@ c3('s:select{3}') -- c2 is not visible
 ...
 c3:commit()
 ---
-- 
+- null
 ...
 s:drop()
 ---
diff --git a/test/vinyl/hermitage.result b/test/vinyl/hermitage.result
index 23495fde1..fbe2090ce 100644
--- a/test/vinyl/hermitage.result
+++ b/test/vinyl/hermitage.result
@@ -51,11 +51,11 @@ t:replace{2, 20}
 ...
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:replace{1, 11}")
 ---
@@ -71,7 +71,7 @@ c1("t:replace{2, 21}")
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 c2("t:replace{2, 22}")
 ---
@@ -79,7 +79,7 @@ c2("t:replace{2, 22}")
 ...
 c2:commit() -- success, the last writer wins
 ---
-- 
+- null
 ...
 t:get{1} -- {1, 12}
 ---
@@ -107,11 +107,11 @@ t:replace{2, 20}
 ...
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:update(1, {{'=', 2, 11}})")
 ---
@@ -127,7 +127,7 @@ c1("t:update(2, {{'=', 2, 21}})")
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 c2("t:update(2, {{'=', 2, 22}})")
 ---
@@ -163,11 +163,11 @@ t:replace{2, 20}
 ...
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:replace{1, 101}")
 ---
@@ -179,7 +179,7 @@ c2("t:replace{1, 10}")
 ...
 c1:rollback()
 ---
-- 
+- null
 ...
 c2("t:get{1}") -- {1, 10}
 ---
@@ -187,7 +187,7 @@ c2("t:get{1}") -- {1, 10}
 ...
 c2:commit() -- true
 ---
-- 
+- null
 ...
 -- teardown
 t:truncate()
@@ -207,11 +207,11 @@ t:replace{2, 20}
 ...
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:replace{1, 101}")
 ---
@@ -227,7 +227,7 @@ c1("t:replace{1, 11}")
 ...
 c1:commit() -- ok
 ---
-- 
+- null
 ...
 c2("t:get{1}") -- {1, 10}
 ---
@@ -235,7 +235,7 @@ c2("t:get{1}") -- {1, 10}
 ...
 c2:commit() -- ok
 ---
-- 
+- null
 ...
 -- teardown
 t:truncate()
@@ -255,11 +255,11 @@ t:replace{2, 20}
 ...
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:replace{1, 11}")
 ---
@@ -279,7 +279,7 @@ c2("t:get{1}") -- {1, 10}
 ...
 c1:commit() -- ok
 ---
-- 
+- null
 ...
 c2:commit() -- rollback (@fixme: not necessary)
 ---
@@ -303,15 +303,15 @@ t:replace{2, 20}
 ...
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c3:begin()
 ---
-- 
+- null
 ...
 c1("t:replace{1, 11}")
 ---
@@ -327,7 +327,7 @@ c2("t:replace{1, 12}")
 ...
 c1:commit() -- ok
 ---
-- 
+- null
 ...
 c3("t:get{1}") -- {1, 11}
 ---
@@ -343,7 +343,7 @@ c3("t:get{2}") -- {2, 19}
 ...
 c2:commit() -- write only transaction - OK to commit
 ---
-- 
+- null
 ...
 c3("t:get{2}") -- {2, 19}
 ---
@@ -355,7 +355,7 @@ c3("t:get{1}") -- {1, 11}
 ...
 c3:commit() -- read only transaction - OK to commit, stays with its read view
 ---
-- 
+- null
 ...
 -- teardown
 t:truncate()
@@ -375,11 +375,11 @@ t:replace{2, 20}
 ...
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:select()") -- {1, 10}, {2, 20}
 ---
@@ -391,7 +391,7 @@ c2("t:replace{3, 30}")
 ...
 c2:commit() -- ok
 ---
-- 
+- null
 ...
 c1("t:select()") -- still {1, 10}, {2, 20}
 ---
@@ -399,7 +399,7 @@ c1("t:select()") -- still {1, 10}, {2, 20}
 ...
 c1:commit() -- ok
 ---
-- 
+- null
 ...
 -- teardown
 t:truncate()
@@ -419,11 +419,11 @@ t:replace{2, 20}
 ...
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:replace{1, 20}")
 ---
@@ -443,11 +443,11 @@ c2("t:get{2}") -- {2, 20}
 ...
 c2("t:delete{2}")
 ---
-- 
+- null
 ...
 c1:commit() -- ok
 ---
-- 
+- null
 ...
 c2("t:get{1}") -- {1, 10}
 ---
@@ -483,11 +483,11 @@ t:replace{2, 20}
 ...
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{1}") -- {1, 10}
 ---
@@ -507,7 +507,7 @@ c2("t:replace{1, 12}")
 ...
 c1:commit() -- ok
 ---
-- 
+- null
 ...
 c2:commit() -- rollback -- conflict
 ---
@@ -531,11 +531,11 @@ t:replace{2, 20}
 ...
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{1}") -- {1, 10}
 ---
@@ -559,7 +559,7 @@ c2("t:replace{2, 18}")
 ...
 c2:commit() -- ok
 ---
-- 
+- null
 ...
 c1("t:get{2}") -- {2, 20}
 ---
@@ -567,7 +567,7 @@ c1("t:get{2}") -- {2, 20}
 ...
 c1:commit() -- ok
 ---
-- 
+- null
 ...
 -- teardown
 t:truncate()
@@ -587,11 +587,11 @@ t:replace{2, 20}
 ...
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{1}") -- {1, 10}
 ---
@@ -615,15 +615,15 @@ c2("t:replace{2, 18}")
 ...
 c2:commit() -- T2
 ---
-- 
+- null
 ...
 c1("t:delete{2}")
 ---
-- 
+- null
 ...
 c1("t:get{2}") -- finds nothing
 ---
-- 
+- null
 ...
 c1:commit() -- rollback
 ---
@@ -647,11 +647,11 @@ t:replace{2, 20}
 ...
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{1}") -- {1, 10}
 ---
@@ -679,7 +679,7 @@ c2("t:replace{1, 21}")
 ...
 c1:commit() -- ok
 ---
-- 
+- null
 ...
 c2:commit() -- rollback -- conflict
 ---
@@ -703,11 +703,11 @@ t:replace{2, 20}
 ...
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 -- select * from test where value % 3 = 0
 c1("t:select()") -- {1, 10}, {2, 20}
@@ -728,7 +728,7 @@ c2("t:replace{4, 42}")
 ...
 c1:commit() -- ok
 ---
-- 
+- null
 ...
 c2:commit() -- rollback
 ---
@@ -752,7 +752,7 @@ t:replace{2, 20}
 ...
 c1:begin()
 ---
-- 
+- null
 ...
 c1("t:get{1}") -- {1, 10}
 ---
@@ -764,7 +764,7 @@ c1("t:get{2}") -- {2, 20}
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c2("t:replace{2, 25}")
 ---
@@ -772,11 +772,11 @@ c2("t:replace{2, 25}")
 ...
 c2:commit() -- ok
 ---
-- 
+- null
 ...
 c3:begin()
 ---
-- 
+- null
 ...
 c3("t:get{1}") -- {1, 10}
 ---
@@ -788,7 +788,7 @@ c3("t:get{2}") -- {2, 25}
 ...
 c3:commit() -- ok
 ---
-- 
+- null
 ...
 c1("t:replace{1, 0}")
 ---
@@ -816,7 +816,7 @@ t:replace{2, 20}
 ...
 c1:begin()
 ---
-- 
+- null
 ...
 c1("t:get{1}") -- {1, 10}
 ---
@@ -828,7 +828,7 @@ c1("t:get{2}") -- {2, 20}
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c2("t:replace{2, 25}")
 ---
@@ -836,11 +836,11 @@ c2("t:replace{2, 25}")
 ...
 c2:commit() -- ok
 ---
-- 
+- null
 ...
 c3:begin()
 ---
-- 
+- null
 ...
 c3("t:get{1}") -- {1, 10}
 ---
@@ -852,12 +852,12 @@ c3("t:get{2}") -- {2, 25}
 ...
 c3:commit() -- ok
 ---
-- 
+- null
 ...
 -- c1("t:replace{1, 0)")
 c1:commit() -- ok
 ---
-- 
+- null
 ...
 -- teardown
 t:truncate()
diff --git a/test/vinyl/mvcc.result b/test/vinyl/mvcc.result
index 1941744b9..db7fc8e83 100644
--- a/test/vinyl/mvcc.result
+++ b/test/vinyl/mvcc.result
@@ -41,29 +41,29 @@ t = box.space.test
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 --
 -- empty transaction rollback
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c1:rollback()
 ---
-- 
+- null
 ...
 --
 -- single-statement transaction commit
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c1("t:replace{1}")
 ---
@@ -71,7 +71,7 @@ c1("t:replace{1}")
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 c1("t:get{1}")
 ---
@@ -80,14 +80,14 @@ c1("t:get{1}")
 -- cleanup
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 --
 -- single-statement transaction rollback
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c1("t:replace{1}")
 ---
@@ -95,18 +95,18 @@ c1("t:replace{1}")
 ...
 c1:rollback()
 ---
-- 
+- null
 ...
 c1("t:get{1}")
 ---
-- 
+- null
 ...
 --
 -- basic effects: if a transaction is rolled back, it has no effect
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c1("t:insert{1}")
 ---
@@ -118,15 +118,15 @@ c1("t:get{1}")
 ...
 c1:rollback()
 ---
-- 
+- null
 ...
 c1("t:get{1}")
 ---
-- 
+- null
 ...
 c2("t:get{1}")
 ---
-- 
+- null
 ...
 --
 -- multi-statement transaction
@@ -137,7 +137,7 @@ test_run:cmd("setopt delimiter ';'")
 ...
 c1:begin();
 ---
-- 
+- null
 ...
 for i = 1,100 do
     c1(string.format("t:insert{%d}", i))
@@ -147,7 +147,7 @@ end;
 ...
 c1:commit();
 ---
-- 
+- null
 ...
 for i = 1,100 do
     c1(string.format("t:delete{%d}", i))
@@ -155,7 +155,7 @@ end;
 ---
 ...
 for i = 1,100 do
-    assert(#c1(string.format("t:get{%d}", i)) == 0)
+    assert(c1(string.format("t:get{%d}", i)) == nil)
 end;
 ---
 ...
@@ -172,7 +172,7 @@ test_run:cmd("setopt delimiter ';'")
 ...
 c1:begin();
 ---
-- 
+- null
 ...
 for i = 1,100 do
     c1(string.format("t:insert{%d}", i))
@@ -182,10 +182,10 @@ end;
 ...
 c1:rollback();
 ---
-- 
+- null
 ...
 for i = 1,100 do
-    assert(#c1(string.format("t:get{%d}", i)) == 0)
+    assert(c1(string.format("t:get{%d}", i)) == nil)
 end;
 ---
 ...
@@ -196,7 +196,7 @@ test_run:cmd("setopt delimiter ''");
 -- transaction_set_set_get_commit(void)
 c1:begin()
 ---
-- 
+- null
 ...
 c1("t:replace{1, 1}")
 ---
@@ -212,7 +212,7 @@ c1("t:get{1}")
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 c1("t:get{1}")
 ---
@@ -220,12 +220,12 @@ c1("t:get{1}")
 ...
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 -- transaction_set_set_commit_get(void)
 c1:begin()
 ---
-- 
+- null
 ...
 c1("t:replace{1}")
 ---
@@ -237,11 +237,11 @@ c1("t:replace{1, 2}")
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c2("t:get{1}")
 ---
@@ -249,16 +249,16 @@ c2("t:get{1}")
 ...
 c2:rollback()
 ---
-- 
+- null
 ...
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 -- transaction_set_set_rollback_get(void)
 c1:begin()
 ---
-- 
+- null
 ...
 c1("t:replace{1}")
 ---
@@ -270,24 +270,24 @@ c1("t:replace{1, 2}")
 ...
 c1:rollback()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c2("t:get{1}")
 ---
-- 
+- null
 ...
 c2:rollback()
 ---
-- 
+- null
 ...
 -- transaction_set_delete_get_commit(void)
 c1:begin()
 ---
-- 
+- null
 ...
 c1("t:insert{1}")
 ---
@@ -295,20 +295,20 @@ c1("t:insert{1}")
 ...
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 c1("t:get{1}")
 ---
-- 
+- null
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 -- transaction_set_delete_get_commit_get(void)
 c1:begin()
 ---
-- 
+- null
 ...
 c1("t:insert{1}")
 ---
@@ -316,26 +316,26 @@ c1("t:insert{1}")
 ...
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 c1("t:get{1}")
 ---
-- 
+- null
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 c1("t:get{1}")
 ---
-- 
+- null
 ...
 --
 -- transaction_set_delete_set_commit_get(void)
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c1("t:insert{1, 1}")
 ---
@@ -343,7 +343,7 @@ c1("t:insert{1, 1}")
 ...
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 c1("t:insert{1, 2}")
 ---
@@ -355,7 +355,7 @@ c1("t:get{1}")
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 c2("t:get{1}")
 ---
@@ -366,14 +366,14 @@ c2("t:get{1}")
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 --
 -- transaction_set_delete_commit_get_set(void)
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c1("t:insert{1}")
 ---
@@ -381,15 +381,15 @@ c1("t:insert{1}")
 ...
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 c1("t:get{1}")
 ---
-- 
+- null
 ...
 c1("t:insert{1}")
 ---
@@ -401,22 +401,22 @@ c1("t:get{1}")
 ...
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 c1("t:get{1}")
 ---
-- 
+- null
 ...
 --
 -- transaction_p_set_commit(void)
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:replace{1, 10}")
 ---
@@ -424,7 +424,7 @@ c1("t:replace{1, 10}")
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 c2("t:replace{2, 15}");
 ---
@@ -432,7 +432,7 @@ c2("t:replace{2, 15}");
 ...
 c2:commit()
 ---
-- 
+- null
 ...
 c1("t:get{1}")
 ---
@@ -444,11 +444,11 @@ c1("t:get{2}")
 ...
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 c1("t:delete{2}")
 ---
-- 
+- null
 ...
 --
 -- no dirty reads: if a transaction is not committed, its effects are not
@@ -456,7 +456,7 @@ c1("t:delete{2}")
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c1("t:insert{1}")
 ---
@@ -471,11 +471,11 @@ c1("t:get{1}")
 --
 c2("t:get{1}")
 ---
-- 
+- null
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 --
 -- become visible in c2 after c1 commits (c2 runs in autocommit)
@@ -491,7 +491,7 @@ c2("t:get{1}")
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c1("t:get{1}")
 ---
@@ -510,7 +510,7 @@ c1("t:get{1}")
 ...
 c2:commit()
 ---
-- 
+- null
 ...
 --
 -- still not visible, even though c2 has committed
@@ -522,7 +522,7 @@ c1("t:get{1}")
 -- commits ok since is a read only transaction
 c1:commit()
 ---
-- 
+- null
 ...
 --
 -- now visible
@@ -533,7 +533,7 @@ c1("t:get{1}")
 ...
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 -- *******************************
 -- tx manager tests  from sophia *
@@ -544,19 +544,19 @@ c1("t:delete{1}")
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c2("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c1("t:replace{1, 10}")
 ---
@@ -569,7 +569,7 @@ c1("t:get{1}") -- {1, 10}
 --
 c1:commit()
 ---
-- 
+- null
 ...
 --
 --
@@ -585,18 +585,18 @@ c2("t:get{2}") -- {2, 15}
 --
 c2:commit()
 ---
-- 
+- null
 ...
 --
 -- cleanup
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 c1("t:delete{2}")
 ---
-- 
+- null
 ...
 --
 -- --------------------------------------------------------------------------
@@ -605,19 +605,19 @@ c1("t:delete{2}")
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c2("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 c1("t:replace{1, 10}")
@@ -627,7 +627,7 @@ c1("t:replace{1, 10}")
 --
 c1:commit()
 ---
-- 
+- null
 ...
 --
 c2("t:replace{2, 15}")
@@ -636,12 +636,12 @@ c2("t:replace{2, 15}")
 ...
 c2:commit()
 ---
-- 
+- null
 ...
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c1("t:get{1}") -- {1, 10}
 ---
@@ -654,18 +654,18 @@ c1("t:get{2}") -- {2, 15}
 ...
 c1:rollback()
 ---
-- 
+- null
 ...
 --
 -- cleanup
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 c1("t:delete{2}")
 ---
-- 
+- null
 ...
 -- --------------------------------------------------------------------------
 -- transaction_p_set_commit_get1(void)
@@ -673,19 +673,19 @@ c1("t:delete{2}")
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}")
 ---
-- 
+- null
 ...
 c2("t:get{200}")
 ---
-- 
+- null
 ...
 --
 c2("t:replace{1, 10}")
@@ -694,7 +694,7 @@ c2("t:replace{1, 10}")
 ...
 c2:commit()
 ---
-- 
+- null
 ...
 --
 -- try writing an unrelated key
@@ -705,12 +705,12 @@ c1("t:replace{2, 15}")
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 --
 c2:begin()
 ---
-- 
+- null
 ...
 c2("t:get{1}") -- {1, 10}
 ---
@@ -718,37 +718,37 @@ c2("t:get{1}") -- {1, 10}
 ...
 c2:rollback()
 ---
-- 
+- null
 ...
 --
 -- cleanup
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 c1("t:delete{2}")
 ---
-- 
+- null
 ...
 -- --
 --  now try the same key
 -- --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}")
 ---
-- 
+- null
 ...
 c2("t:get{200}")
 ---
-- 
+- null
 ...
 --
 c2("t:replace{1, 10}")
@@ -757,7 +757,7 @@ c2("t:replace{1, 10}")
 ...
 c2:commit()
 ---
-- 
+- null
 ...
 --
 c1("t:replace{1, 15}")
@@ -766,12 +766,12 @@ c1("t:replace{1, 15}")
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 --
 c2:begin()
 ---
-- 
+- null
 ...
 c2("t:get{1}") -- {1, 15}
 ---
@@ -779,14 +779,14 @@ c2("t:get{1}") -- {1, 15}
 ...
 c2:rollback()
 ---
-- 
+- null
 ...
 --
 -- cleanup
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 -- --------------------------------------------------------------------------
 -- transaction_p_set_commit_get2(void)
@@ -794,19 +794,19 @@ c1("t:delete{1}")
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}")
 ---
-- 
+- null
 ...
 c2("t:get{200}")
 ---
-- 
+- null
 ...
 --
 --
@@ -816,7 +816,7 @@ c1("t:replace{2, 15}")
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 --
 --
@@ -826,12 +826,12 @@ c2("t:replace{1, 10}")
 ...
 c2:commit() -- commits successfully
 ---
-- 
+- null
 ...
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c1("t:get{1}") -- {1, 10}
 ---
@@ -844,18 +844,18 @@ c1("t:get{2}") -- {2, 15}
 ...
 c1:rollback()
 ---
-- 
+- null
 ...
 --
 -- cleanup
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 c1("t:delete{2}")
 ---
-- 
+- null
 ...
 -- --------------------------------------------------------------------------
 -- transaction_p_set_rollback_get0(void)
@@ -863,19 +863,19 @@ c1("t:delete{2}")
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}")
 ---
-- 
+- null
 ...
 c2("t:get{200}")
 ---
-- 
+- null
 ...
 --
 --
@@ -885,7 +885,7 @@ c1("t:replace{1, 10}")
 ...
 c1:rollback()
 ---
-- 
+- null
 ...
 --
 c2("t:replace{2, 15}")
@@ -894,24 +894,24 @@ c2("t:replace{2, 15}")
 ...
 c2:rollback()
 ---
-- 
+- null
 ...
 --
 c3:begin()
 ---
-- 
+- null
 ...
 c3("t:get{1}") -- finds nothing
 ---
-- 
+- null
 ...
 c3("t:get{2}") -- finds nothing
 ---
-- 
+- null
 ...
 c3:rollback()
 ---
-- 
+- null
 ...
 -- --------------------------------------------------------------------------
 -- transaction_p_set_rollback_get1(void)
@@ -920,19 +920,19 @@ c3:rollback()
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c2("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 c2("t:replace{1, 10}")
@@ -941,7 +941,7 @@ c2("t:replace{1, 10}")
 ...
 c2:rollback()
 ---
-- 
+- null
 ...
 --
 c1("t:replace{2, 15}")
@@ -950,24 +950,24 @@ c1("t:replace{2, 15}")
 ...
 c1:rollback()
 ---
-- 
+- null
 ...
 --
 c3:begin()
 ---
-- 
+- null
 ...
 c3("t:get{1}") -- finds nothing
 ---
-- 
+- null
 ...
 c3("t:get{2}") -- finds nothing
 ---
-- 
+- null
 ...
 c3:rollback()
 ---
-- 
+- null
 ...
 --
 -- --------------------------------------------------------------------------
@@ -976,19 +976,19 @@ c3:rollback()
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c2("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 --
@@ -998,7 +998,7 @@ c2("t:replace{1, 10}")
 ...
 c2:rollback()
 ---
-- 
+- null
 ...
 --
 c1("t:replace{1, 15}")
@@ -1007,29 +1007,29 @@ c1("t:replace{1, 15}")
 ...
 c1:rollback()
 ---
-- 
+- null
 ...
 --
 c3("t:get{1}") -- finds nothing
 ---
-- 
+- null
 ...
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c2("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 --
@@ -1039,7 +1039,7 @@ c2("t:replace{1, 10}")
 ...
 c2:rollback()
 ---
-- 
+- null
 ...
 --
 c1("t:replace{1, 15}")
@@ -1048,7 +1048,7 @@ c1("t:replace{1, 15}")
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 --
 c3("t:get{1}") -- {1, 15}
@@ -1060,7 +1060,7 @@ c3("t:get{1}") -- {1, 15}
 --
 c3("t:delete{1}")
 ---
-- 
+- null
 ...
 -- --------------------------------------------------------------------------
 -- transaction_c_set_commit0(void)
@@ -1068,19 +1068,19 @@ c3("t:delete{1}")
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c2("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c1("t:replace{1, 10}")
 ---
@@ -1088,7 +1088,7 @@ c1("t:replace{1, 10}")
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 --
 c2("t:replace{1, 15}")
@@ -1097,7 +1097,7 @@ c2("t:replace{1, 15}")
 ...
 c2:commit()
 ---
-- 
+- null
 ...
 --
 c2("t:get{1}")  -- {1,15}
@@ -1108,7 +1108,7 @@ c2("t:get{1}")  -- {1,15}
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 -- --------------------------------------------------------------------------
 -- transaction_c_set_commit1(void)
@@ -1116,19 +1116,19 @@ c1("t:delete{1}")
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c2("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 c2("t:replace{1, 10}")
@@ -1137,7 +1137,7 @@ c2("t:replace{1, 10}")
 ...
 c2:commit()
 ---
-- 
+- null
 ...
 --
 c1("t:replace{1, 15}")
@@ -1146,7 +1146,7 @@ c1("t:replace{1, 15}")
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 --
 c3("t:get{1}") -- {1, 15}
@@ -1158,7 +1158,7 @@ c3("t:get{1}") -- {1, 15}
 --
 c3("t:delete{1}")
 ---
-- 
+- null
 ...
 -- --------------------------------------------------------------------------
 -- transaction_c_set_commit2(void)
@@ -1166,19 +1166,19 @@ c3("t:delete{1}")
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c2("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 c1("t:replace{1, 15}")
@@ -1193,11 +1193,11 @@ c2("t:replace{1, 10}")
 --
 c2:commit()
 ---
-- 
+- null
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 --
 c3("t:get{1}") -- {1, 15}
@@ -1209,24 +1209,24 @@ c3("t:get{1}") -- {1, 15}
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c2("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 c1("t:replace{1, 15}")
@@ -1242,11 +1242,11 @@ c2("t:replace{1, 10}")
 -- sic: commit order
 c1:commit()
 ---
-- 
+- null
 ...
 c2:commit() -- write after write is ok, the last writer to commit wins
 ---
-- 
+- null
 ...
 --
 c3("t:get{1}") -- {1, 10}
@@ -1258,7 +1258,7 @@ c3("t:get{1}") -- {1, 10}
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 -- --------------------------------------------------------------------------
 -- transaction_c_set_commit_rollback_a0(void)
@@ -1266,19 +1266,19 @@ c1("t:delete{1}")
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c2("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 c2("t:replace{1, 10}")
@@ -1288,7 +1288,7 @@ c2("t:replace{1, 10}")
 --
 c2:rollback()
 ---
-- 
+- null
 ...
 --
 c1("t:replace{1, 15}")
@@ -1298,7 +1298,7 @@ c1("t:replace{1, 15}")
 --
 c1:commit()
 ---
-- 
+- null
 ...
 --
 c3("t:get{1}")
@@ -1310,26 +1310,26 @@ c3("t:get{1}")
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 --
 --
 -- statement order is irrelevant, rollback order is important
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c2("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 c1("t:replace{1, 10}")
@@ -1343,11 +1343,11 @@ c2("t:replace{1, 15}")
 --
 c2:rollback()
 ---
-- 
+- null
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 --
 c3("t:get{1}")
@@ -1359,7 +1359,7 @@ c3("t:get{1}")
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 -- --------------------------------------------------------------------------
 -- transaction_c_set_commit_rollback_a1(void)
@@ -1367,19 +1367,19 @@ c1("t:delete{1}")
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c2("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 c2("t:replace{1, 10}")
@@ -1393,37 +1393,37 @@ c1("t:replace{1, 15}")
 --
 c2:rollback()
 ---
-- 
+- null
 ...
 c1:commit() -- success
 ---
-- 
+- null
 ...
 --
 -- cleanup
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 --
 -- statements in different order now
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c2("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 c1("t:replace{1, 10}")
@@ -1437,18 +1437,18 @@ c2("t:replace{1, 15}")
 --
 c2:rollback()
 ---
-- 
+- null
 ...
 c1:commit() -- success
 ---
-- 
+- null
 ...
 --
 -- cleanup
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 --
 -- --------------------------------------------------------------------------
@@ -1457,19 +1457,19 @@ c1("t:delete{1}")
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c2("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 c2("t:replace{1, 10}")
@@ -1478,7 +1478,7 @@ c2("t:replace{1, 10}")
 ...
 c2:commit() -- success
 ---
-- 
+- null
 ...
 --
 c1("t:replace{1, 15}")
@@ -1487,7 +1487,7 @@ c1("t:replace{1, 15}")
 ...
 c1:rollback() -- success
 ---
-- 
+- null
 ...
 --
 c3("t:get{1}")
@@ -1498,7 +1498,7 @@ c3("t:get{1}")
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 -- --------------------------------------------------------------------------
 -- transaction_c_set_commit_rollback_b1(void)
@@ -1506,19 +1506,19 @@ c1("t:delete{1}")
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c2("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 c2("t:replace{1, 15}")
@@ -1532,11 +1532,11 @@ c1("t:replace{1, 10}")
 --
 c2:commit()
 ---
-- 
+- null
 ...
 c1:rollback()
 ---
-- 
+- null
 ...
 --
 c3("t:get{1}")
@@ -1548,26 +1548,26 @@ c3("t:get{1}")
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 --
 -- now commit the second transaction
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c2("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 c2("t:replace{1, 15}")
@@ -1581,11 +1581,11 @@ c1("t:replace{1, 10}")
 --
 c2:commit()
 ---
-- 
+- null
 ...
 c1:commit() -- ok, the last committer wins
 ---
-- 
+- null
 ...
 --
 c3("t:get{1}") -- {1, 10}
@@ -1597,7 +1597,7 @@ c3("t:get{1}") -- {1, 10}
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 --
 -- --------------------------------------------------------------------------
@@ -1606,19 +1606,19 @@ c1("t:delete{1}")
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c2("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 c2("t:replace{1, 15}")
@@ -1627,7 +1627,7 @@ c2("t:replace{1, 15}")
 ...
 c2:rollback()
 ---
-- 
+- null
 ...
 --
 c1("t:replace{1, 10}")
@@ -1636,38 +1636,38 @@ c1("t:replace{1, 10}")
 ...
 c1:rollback()
 ---
-- 
+- null
 ...
 --
 c3("t:get{1}")
 ---
-- 
+- null
 ...
 --
 -- cleanup
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 --
 -- now commit the second transaction
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c2("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 c2("t:replace{1, 15}")
@@ -1676,7 +1676,7 @@ c2("t:replace{1, 15}")
 ...
 c2:rollback()
 ---
-- 
+- null
 ...
 --
 c1("t:replace{1, 10}")
@@ -1685,7 +1685,7 @@ c1("t:replace{1, 10}")
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 --
 c3("t:get{1}")
@@ -1697,7 +1697,7 @@ c3("t:get{1}")
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 --
 -- --------------------------------------------------------------------------
@@ -1706,19 +1706,19 @@ c1("t:delete{1}")
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c2("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 c2("t:replace{1, 10}")
@@ -1732,23 +1732,23 @@ c1("t:replace{1, 15}")
 --
 c2:rollback()
 ---
-- 
+- null
 ...
 c1:rollback()
 ---
-- 
+- null
 ...
 --
 c3("t:get{1}")
 ---
-- 
+- null
 ...
 --
 -- cleanup
 --
 c2("t:delete{1}")
 ---
-- 
+- null
 ...
 --
 -- --------------------------------------------------------------------------
@@ -1757,19 +1757,19 @@ c2("t:delete{1}")
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c2("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 c2("t:replace{1, 15}")
@@ -1784,11 +1784,11 @@ c1("t:replace{1, 10}")
 --
 c1:commit() -- success
 ---
-- 
+- null
 ...
 c2:commit() -- success, the last writer wins
 ---
-- 
+- null
 ...
 --
 c2("t:get{1}") -- {1, 15}
@@ -1800,7 +1800,7 @@ c2("t:get{1}") -- {1, 15}
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 --
 -- --------------------------------------------------------------------------
@@ -1809,19 +1809,19 @@ c1("t:delete{1}")
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c2("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 c1("t:replace{1, 10}")
@@ -1836,18 +1836,18 @@ c2("t:replace{1, 15}")
 --
 c2:commit() -- success
 ---
-- 
+- null
 ...
 c1:commit() -- success, the last writer wins
 ---
-- 
+- null
 ...
 --
 -- cleanup
 --
 c2("t:delete{1}")
 ---
-- 
+- null
 ...
 --
 -- --------------------------------------------------------------------------
@@ -1856,19 +1856,19 @@ c2("t:delete{1}")
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c2("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 c1("t:replace{1, 10}")
@@ -1883,18 +1883,18 @@ c2("t:replace{1, 15}")
 --
 c2:commit() -- success
 ---
-- 
+- null
 ...
 c1:commit() -- success
 ---
-- 
+- null
 ...
 --
 -- cleanup
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 --
 -- --------------------------------------------------------------------------
@@ -1903,19 +1903,19 @@ c1("t:delete{1}")
 --
 c2:begin()
 ---
-- 
+- null
 ...
 c1:begin()
 ---
-- 
+- null
 ...
 c2("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c1("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 c1("t:replace{1, 10}")
@@ -1930,18 +1930,18 @@ c2("t:replace{1, 15}")
 --
 c2:commit() -- success
 ---
-- 
+- null
 ...
 c1:commit() -- success
 ---
-- 
+- null
 ...
 --
 -- cleanup
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 --
 -- --------------------------------------------------------------------------
@@ -1950,19 +1950,19 @@ c1("t:delete{1}")
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c2("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 c1("t:replace{1, 10}")
@@ -1977,18 +1977,18 @@ c2("t:replace{1, 15}")
 --
 c2:commit() -- success
 ---
-- 
+- null
 ...
 c1:commit() -- success
 ---
-- 
+- null
 ...
 --
 -- cleanup
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 --
 -- --------------------------------------------------------------------------
@@ -1997,19 +1997,19 @@ c1("t:delete{1}")
 --
 c2:begin()
 ---
-- 
+- null
 ...
 c1:begin()
 ---
-- 
+- null
 ...
 c2("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c1("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 c1("t:replace{1, 10}")
@@ -2024,18 +2024,18 @@ c2("t:replace{1, 15}")
 --
 c2:commit() -- success
 ---
-- 
+- null
 ...
 c1:rollback() -- success
 ---
-- 
+- null
 ...
 --
 -- cleanup
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 --
 -- --------------------------------------------------------------------------
@@ -2044,19 +2044,19 @@ c1("t:delete{1}")
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c2("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 c1("t:replace{1, 10}")
@@ -2071,22 +2071,22 @@ c2("t:replace{1, 15}")
 --
 c2:commit() -- success
 ---
-- 
+- null
 ...
 c2:rollback() -- not in transaction
 ---
-- 
+- null
 ...
 c1:commit() -- success
 ---
-- 
+- null
 ...
 --
 -- cleanup
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 --
 -- --------------------------------------------------------------------------
@@ -2095,19 +2095,19 @@ c1("t:delete{1}")
 --
 c2:begin()
 ---
-- 
+- null
 ...
 c1:begin()
 ---
-- 
+- null
 ...
 c2("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c1("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 c1("t:replace{1, 10}")
@@ -2122,26 +2122,26 @@ c2("t:replace{1, 15}")
 --
 c2:commit() -- success
 ---
-- 
+- null
 ...
 c2:rollback() -- not in transaction
 ---
-- 
+- null
 ...
 c1:commit() -- success
 ---
-- 
+- null
 ...
 --
 -- cleanup
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 c1("t:delete{2}")
 ---
-- 
+- null
 ...
 --
 -- --------------------------------------------------------------------------
@@ -2150,27 +2150,27 @@ c1("t:delete{2}")
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c2("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c3:begin()
 ---
-- 
+- null
 ...
 c3("t:get{300}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 --
@@ -2191,23 +2191,23 @@ c3("t:replace{1, 20}")
 --
 c2:commit() -- success
 ---
-- 
+- null
 ...
 c3:commit() -- success
 ---
-- 
+- null
 ...
 c1:commit() -- success, the last committer wins
 ---
-- 
+- null
 ...
 c2:commit() -- not in transaction
 ---
-- 
+- null
 ...
 c3:commit() -- not in transaction
 ---
-- 
+- null
 ...
 --
 c3:get{1} -- {1, 20}
@@ -2219,7 +2219,7 @@ c3:get{1} -- {1, 20}
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 --
 -- --------------------------------------------------------------------------
@@ -2228,27 +2228,27 @@ c1("t:delete{1}")
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c3:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c3("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c2("t:get{300}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 --
@@ -2269,15 +2269,15 @@ c3("t:replace{1, 30}")
 --
 c1:commit() -- success
 ---
-- 
+- null
 ...
 c2:commit() -- success
 ---
-- 
+- null
 ...
 c3:commit() -- success
 ---
-- 
+- null
 ...
 --
 c3("t:get{1}") -- {1, 30}
@@ -2288,7 +2288,7 @@ c3("t:get{1}") -- {1, 30}
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 --
 -- --------------------------------------------------------------------------
@@ -2297,27 +2297,27 @@ c1("t:delete{1}")
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c3:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c2("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c3("t:get{300}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 --
@@ -2338,22 +2338,22 @@ c3("t:replace{1, 20}")
 --
 c2:commit() -- success
 ---
-- 
+- null
 ...
 c3:commit() -- rollback
 ---
-- 
+- null
 ...
 c1:rollback() -- success
 ---
-- 
+- null
 ...
 --
 -- cleanup
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 --
 -- --------------------------------------------------------------------------
@@ -2362,27 +2362,27 @@ c1("t:delete{1}")
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c3:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c2("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c3("t:get{300}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 --
@@ -2403,30 +2403,30 @@ c3("t:replace{1, 20}")
 --
 c2:commit()  -- success
 ---
-- 
+- null
 ...
 c3:commit() -- rollback
 ---
-- 
+- null
 ...
 c2:rollback() -- success, not in transaction in tarantool
 ---
-- 
+- null
 ...
 c3:commit() -- success, not in transaction in tarantool
 ---
-- 
+- null
 ...
 c1:commit() -- rollback
 ---
-- 
+- null
 ...
 --
 -- cleanup
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 --
 -- --------------------------------------------------------------------------
@@ -2435,27 +2435,27 @@ c1("t:delete{1}")
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c3:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c2("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c3("t:get{300}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 --
@@ -2476,22 +2476,22 @@ c3("t:replace{1, 20}")
 --
 c3:rollback()
 ---
-- 
+- null
 ...
 c2:commit()
 ---
-- 
+- null
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 --
 -- cleanup
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 --
 -- --------------------------------------------------------------------------
@@ -2500,27 +2500,27 @@ c1("t:delete{1}")
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c3:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c2("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c3("t:get{300}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 c1("t:replace{1, 10}")
@@ -2540,22 +2540,22 @@ c3("t:replace{1, 20}")
 --
 c2:commit()
 ---
-- 
+- null
 ...
 c3:rollback()
 ---
-- 
+- null
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 --
 -- cleanup
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 --
 -- --------------------------------------------------------------------------
@@ -2564,27 +2564,27 @@ c1("t:delete{1}")
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c3:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c2("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c3("t:get{300}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 --
@@ -2605,22 +2605,22 @@ c3("t:replace{1, 20}")
 --
 c2:commit()
 ---
-- 
+- null
 ...
 c3:rollback()
 ---
-- 
+- null
 ...
 c1:rollback()
 ---
-- 
+- null
 ...
 --
 -- cleanup
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 --
 -- --------------------------------------------------------------------------
@@ -2629,19 +2629,19 @@ c1("t:delete{1}")
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c2("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 c1("t:replace{1, 10}")
@@ -2650,7 +2650,7 @@ c1("t:replace{1, 10}")
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 --
 c2("t:get{1}") -- find newest {1, 10}
@@ -2664,12 +2664,12 @@ c2("t:replace{1, 15}")
 ...
 c2:commit() -- rollback
 ---
-- 
+- null
 ...
 --
 c3:begin()
 ---
-- 
+- null
 ...
 c3("t:get{1}") -- {1, 10}
 ---
@@ -2677,14 +2677,14 @@ c3("t:get{1}") -- {1, 10}
 ...
 c3:commit()
 ---
-- 
+- null
 ...
 --
 -- cleanup
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 --
 -- --------------------------------------------------------------------------
@@ -2693,19 +2693,19 @@ c1("t:delete{1}")
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c2("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 c1("t:replace{1, 10}")
@@ -2715,12 +2715,12 @@ c1("t:replace{1, 10}")
 --
 c1:rollback()
 ---
-- 
+- null
 ...
 --
 c2("t:get{1}") -- finds nothing
 ---
-- 
+- null
 ...
 --
 c2("t:replace{1, 15}")
@@ -2729,12 +2729,12 @@ c2("t:replace{1, 15}")
 ...
 c2:commit()
 ---
-- 
+- null
 ...
 --
 c3:begin()
 ---
-- 
+- null
 ...
 c3("t:get{1}") -- {1, 15}
 ---
@@ -2742,14 +2742,14 @@ c3("t:get{1}") -- {1, 15}
 ...
 c3:commit()
 ---
-- 
+- null
 ...
 --
 -- cleanup
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 --
 -- --------------------------------------------------------------------------
@@ -2758,15 +2758,15 @@ c1("t:delete{1}")
 --
 c7:begin()
 ---
-- 
+- null
 ...
 c7("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c1:begin()
 ---
-- 
+- null
 ...
 --
 c1("t:replace{1, 1}")
@@ -2776,7 +2776,7 @@ c1("t:replace{1, 1}")
 --
 c2:begin()
 ---
-- 
+- null
 ...
 --
 c2("t:replace{1, 2}")
@@ -2786,7 +2786,7 @@ c2("t:replace{1, 2}")
 --
 c4:begin()
 ---
-- 
+- null
 ...
 c4("t:replace{1, 4}")
 ---
@@ -2795,7 +2795,7 @@ c4("t:replace{1, 4}")
 --
 c5:begin()
 ---
-- 
+- null
 ...
 c5("t:replace{1, 5}")
 ---
@@ -2804,11 +2804,11 @@ c5("t:replace{1, 5}")
 --
 c6:begin()
 ---
-- 
+- null
 ...
 c6("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 c1("t:get{1}") -- {1, 1}
@@ -2833,62 +2833,62 @@ c5("t:get{1}") -- {1, 5}
 --
 c6("t:get{1}") --  nothing
 ---
-- 
+- null
 ...
 --
 c7("t:get{1}") --  nothing
 ---
-- 
+- null
 ...
 --
 c3:begin()
 ---
-- 
+- null
 ...
 --
 c3("t:get{1}") -- nothing
 ---
-- 
+- null
 ...
 c3:rollback()
 ---
-- 
+- null
 ...
 --
 c1:rollback()
 ---
-- 
+- null
 ...
 c2:rollback()
 ---
-- 
+- null
 ...
 c3:rollback()
 ---
-- 
+- null
 ...
 c4:rollback()
 ---
-- 
+- null
 ...
 c5:rollback()
 ---
-- 
+- null
 ...
 c6:rollback()
 ---
-- 
+- null
 ...
 c7:rollback()
 ---
-- 
+- null
 ...
 --
 -- cleanup
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 --
 -- --------------------------------------------------------------------------
@@ -2897,24 +2897,24 @@ c1("t:delete{1}")
 --
 c7:begin()
 ---
-- 
+- null
 ...
 c1:begin()
 ---
-- 
+- null
 ...
 c7("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c1("t:get{1}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 c3:begin()
 ---
-- 
+- null
 ...
 c3("t:replace{1, 3}")
 ---
@@ -2922,24 +2922,24 @@ c3("t:replace{1, 3}")
 ...
 c3:commit()
 ---
-- 
+- null
 ...
 --
 c2:begin()
 ---
-- 
+- null
 ...
 c3:begin()
 ---
-- 
+- null
 ...
 c2("t:get{500}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c3("t:get{600}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c2("t:get{1}") -- {1, 3}
 ---
@@ -2952,16 +2952,16 @@ c3("t:replace{1, 6}")
 ...
 c3:commit() -- c2 goes to read view now
 ---
-- 
+- null
 ...
 --
 c4:begin()
 ---
-- 
+- null
 ...
 c3:begin()
 ---
-- 
+- null
 ...
 --
 c3("t:replace{1, 9}")
@@ -2970,24 +2970,24 @@ c3("t:replace{1, 9}")
 ...
 c3:commit()
 ---
-- 
+- null
 ...
 --
 c5:begin()
 ---
-- 
+- null
 ...
 c3:begin()
 ---
-- 
+- null
 ...
 c5("t:get{800}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c3("t:get{900}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 c3("t:replace{1, 12}")
@@ -2996,16 +2996,16 @@ c3("t:replace{1, 12}")
 ...
 c3:commit()
 ---
-- 
+- null
 ...
 --
 c6:begin()
 ---
-- 
+- null
 ...
 c6("t:get{1000}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 --
 c2("t:get{1}") -- {1, 3}
@@ -3030,7 +3030,7 @@ c6("t:get{1}") -- {1, 12}
 --
 c3:begin()
 ---
-- 
+- null
 ...
 c3("t:get{1}") -- {1, 12}
 ---
@@ -3038,12 +3038,12 @@ c3("t:get{1}") -- {1, 12}
 ...
 c3:rollback()
 ---
-- 
+- null
 ...
 --
 c1("t:get{1}") -- nothing
 ---
-- 
+- null
 ...
 --
 c7("t:get{1}") -- {1, 12}
@@ -3053,7 +3053,7 @@ c7("t:get{1}") -- {1, 12}
 --
 c2:rollback()
 ---
-- 
+- null
 ...
 --
 c4("t:get{1}") -- {1, 12}
@@ -3073,7 +3073,7 @@ c6("t:get{1}") -- {1, 12}
 --
 c3:begin()
 ---
-- 
+- null
 ...
 c3("t:get{1}") -- {1, 12}
 ---
@@ -3081,12 +3081,12 @@ c3("t:get{1}") -- {1, 12}
 ...
 c3:rollback()
 ---
-- 
+- null
 ...
 --
 c1("t:get{1}") -- nothing
 ---
-- 
+- null
 ...
 --
 c7("t:get{1}") -- {1, 12}
@@ -3096,7 +3096,7 @@ c7("t:get{1}") -- {1, 12}
 --
 c4:rollback()
 ---
-- 
+- null
 ...
 --
 c5("t:get{1}") -- {1, 12}
@@ -3111,7 +3111,7 @@ c6("t:get{1}") -- {1, 12}
 --
 c3:begin()
 ---
-- 
+- null
 ...
 c3("t:get{1}") -- {1, 12}
 ---
@@ -3119,12 +3119,12 @@ c3("t:get{1}") -- {1, 12}
 ...
 c3:rollback()
 ---
-- 
+- null
 ...
 --
 c1("t:get{1}") -- nothing
 ---
-- 
+- null
 ...
 --
 c7("t:get{1}") -- {1, 12}
@@ -3134,7 +3134,7 @@ c7("t:get{1}") -- {1, 12}
 --
 c5:rollback()
 ---
-- 
+- null
 ...
 --
 c6("t:get{1}") -- {1, 12}
@@ -3144,7 +3144,7 @@ c6("t:get{1}") -- {1, 12}
 --
 c3:begin()
 ---
-- 
+- null
 ...
 c3("t:get{1}") -- {1, 12}
 ---
@@ -3152,12 +3152,12 @@ c3("t:get{1}") -- {1, 12}
 ...
 c3:rollback()
 ---
-- 
+- null
 ...
 --
 c1("t:get{1}") -- nothing
 ---
-- 
+- null
 ...
 --
 c7("t:get{1}") -- {1, 12}
@@ -3167,12 +3167,12 @@ c7("t:get{1}") -- {1, 12}
 --
 c6:rollback()
 ---
-- 
+- null
 ...
 --
 c3:begin()
 ---
-- 
+- null
 ...
 c3("t:get{1}") -- {1, 12}
 ---
@@ -3180,12 +3180,12 @@ c3("t:get{1}") -- {1, 12}
 ...
 c3:rollback()
 ---
-- 
+- null
 ...
 --
 c1("t:get{1}") -- nothing
 ---
-- 
+- null
 ...
 --
 c7("t:get{1}") -- {1, 12}
@@ -3195,16 +3195,16 @@ c7("t:get{1}") -- {1, 12}
 --
 c1:rollback()
 ---
-- 
+- null
 ...
 c7:rollback()
 ---
-- 
+- null
 ...
 --
 c3:begin()
 ---
-- 
+- null
 ...
 c3("t:get{1}") -- {1, 12}
 ---
@@ -3212,14 +3212,14 @@ c3("t:get{1}") -- {1, 12}
 ...
 c3:rollback()
 ---
-- 
+- null
 ...
 --
 -- cleanup
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 --
 -- --------------------------------------------------------------------------
@@ -3228,19 +3228,19 @@ c1("t:delete{1}")
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:get{100}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c2("t:get{200}") -- start transaction in the engine
 ---
-- 
+- null
 ...
 c1("t:replace{1, 10}")
 ---
@@ -3253,7 +3253,7 @@ c2("t:replace{1, 15}")
 --
 c1:commit()
 ---
-- 
+- null
 ...
 --
 c2("t:replace{1, 20}") -- should not reset conflict flag
@@ -3263,7 +3263,7 @@ c2("t:replace{1, 20}") -- should not reset conflict flag
 --
 c2:commit() --  rollback
 ---
-- 
+- null
 ...
 --
 c3("t:get{1}")
@@ -3275,7 +3275,7 @@ c3("t:get{1}")
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 --
 -- --------------------------------------------------------------------------
@@ -3284,7 +3284,7 @@ c1("t:delete{1}")
 --
 c1:begin()
 ---
-- 
+- null
 ...
 --
 c1("t:replace{1, 10}")
@@ -3301,7 +3301,7 @@ c2("t:replace{1, 15}")
 --
 c1:commit()
 ---
-- 
+- null
 ...
 --
 c2("t:get{1}") -- {1, 10}
@@ -3311,7 +3311,7 @@ c2("t:get{1}") -- {1, 10}
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 -- --------------------------------------------------------------------------
 -- transaction_sc_get(void)
@@ -3324,7 +3324,7 @@ c1("t:replace{1, 7}")
 --
 c2:begin()
 ---
-- 
+- null
 ...
 --
 c2("t:replace{1, 8}")
@@ -3339,7 +3339,7 @@ c1("t:get{1}") -- {1, 7}
 --
 c2:commit()
 ---
-- 
+- null
 ...
 --
 c1("t:get{1}") -- {1, 8}
@@ -3356,18 +3356,18 @@ c3("t:get{1}") -- {1, 8}
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 -- --------------------------------------------------------------------------
 -- two conflicting inserts
 -- --------------------------------------------------------------------------
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 --
 c1("t:insert{1, 10}")
@@ -3382,7 +3382,7 @@ c2("t:insert{1, 15}")
 --
 c1:commit() -- success
 ---
-- 
+- null
 ...
 c2:commit() -- rollback, c2 reads {1} before writing it
 ---
@@ -3399,16 +3399,16 @@ c3("t:get{1}") -- {1, 10}
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 --
 c1("t:insert{1, 10}")
@@ -3423,7 +3423,7 @@ c2("t:insert{1, 15}")
 --
 c2:commit() -- success
 ---
-- 
+- null
 ...
 c1:commit() -- rollback, c1 reads {1} before writing it
 ---
@@ -3440,7 +3440,7 @@ c3("t:get{1}") -- {1, 15}
 --
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 --
 -- --------------------------------------------------------------------------
@@ -3456,7 +3456,7 @@ t:insert{2, 20}
 ...
 c7:begin()
 ---
-- 
+- null
 ...
 c7("t:insert{8, 800}")
 ---
@@ -3464,7 +3464,7 @@ c7("t:insert{8, 800}")
 ...
 c3:begin()
 ---
-- 
+- null
 ...
 c3("t:get{1}")
 ---
@@ -3472,15 +3472,15 @@ c3("t:get{1}")
 ...
 c3:commit()
 ---
-- 
+- null
 ...
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 --
 c1("t:replace{4, 40}")
@@ -3495,7 +3495,7 @@ c2("t:get{1}")
 --
 c3:begin()
 ---
-- 
+- null
 ...
 c3("t:insert{3, 30}")
 ---
@@ -3503,7 +3503,7 @@ c3("t:insert{3, 30}")
 ...
 c3:commit()
 ---
-- 
+- null
 ...
 --
 c2("t:replace{5, 50}")
@@ -3516,15 +3516,15 @@ c1("t:get{1}")
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 c2:commit()
 ---
-- 
+- null
 ...
 c7:rollback()
 ---
-- 
+- null
 ...
 --
 -- cleanup
@@ -3557,11 +3557,11 @@ t:insert{2, 20}
 ...
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:select{}")
 ---
@@ -3581,7 +3581,7 @@ c2("t:replace{2, 'new'}")
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 c2:commit() -- rollback
 ---
@@ -3593,7 +3593,7 @@ c2:commit() -- rollback
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c1("t:select{}")
 ---
@@ -3645,7 +3645,7 @@ c1("t:select({3}, {iterator='eq'})")
 ...
 c1("t:delete{3}")
 ---
-- 
+- null
 ...
 c1("t:select({3}, {iterator='ge'})")
 ---
@@ -3669,7 +3669,7 @@ c1("t:replace{3}")
 ...
 c1("t:delete{2}")
 ---
-- 
+- null
 ...
 c1("t:select({3}, {iterator='lt'})")
 ---
@@ -3685,7 +3685,7 @@ c1("t:replace{2}")
 ...
 c1("t:delete{1}")
 ---
-- 
+- null
 ...
 c1("t:select({3}, {iterator='lt'})")
 ---
@@ -3697,7 +3697,7 @@ c1("t:select({3}, {iterator='le'})")
 ...
 c1("t:delete{3}")
 ---
-- 
+- null
 ...
 c1("t:select({3}, {iterator='lt'})")
 ---
@@ -3709,7 +3709,7 @@ c1("t:select({3}, {iterator='le'})")
 ...
 c1:rollback()
 ---
-- 
+- null
 ...
 c1("t:select{}")
 ---
@@ -3722,7 +3722,7 @@ c1("t:select{}")
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c1("t:select{1}")
 ---
@@ -3734,11 +3734,11 @@ c1("for k, v in box.space.test:pairs() do box.commit() end")
 ...
 c1:rollback()
 ---
-- 
+- null
 ...
 c1:begin()
 ---
-- 
+- null
 ...
 c1("t:select{1}")
 ---
@@ -3750,7 +3750,7 @@ c1("for k, v in box.space.test:pairs() do box.rollback() end")
 ...
 c1:rollback()
 ---
-- 
+- null
 ...
 t:truncate()
 ---
@@ -3764,7 +3764,7 @@ t:replace{1}
 ...
 c1:begin()
 ---
-- 
+- null
 ...
 c1("t.index.pk:max()") -- {1}
 ---
@@ -3780,7 +3780,7 @@ c1("t.index.pk:count()") -- 1
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c2("t:replace{2}") -- conflicts with c1 so c1 starts using a read view
 ---
@@ -3788,7 +3788,7 @@ c2("t:replace{2}") -- conflicts with c1 so c1 starts using a read view
 ...
 c2:commit()
 ---
-- 
+- null
 ...
 c1("t.index.pk:max()") -- {1}
 ---
@@ -3804,7 +3804,7 @@ c1("t.index.pk:count()") -- 1
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 --
 -- Convert the reader to a read view: in this test we have
@@ -3813,7 +3813,7 @@ c1:commit()
 --
 c1:begin()
 ---
-- 
+- null
 ...
 c1("t.index.pk:max()") -- {2}
 ---
@@ -3829,7 +3829,7 @@ c1("t.index.pk:count()") -- 2
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c2("t:replace{1, 'new'}") -- conflits with c1 so c1 starts using a read view
 ---
@@ -3841,7 +3841,7 @@ c2("t:replace{3}")
 ...
 c2:commit()
 ---
-- 
+- null
 ...
 c1("t.index.pk:max()") -- {2}
 ---
@@ -3857,7 +3857,7 @@ c1("t.index.pk:count()") -- 2
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 t:truncate()
 ---
@@ -3876,7 +3876,7 @@ t:replace{2}
 ...
 c1:begin()
 ---
-- 
+- null
 ...
 c1("t:select({}, {limit = 0})") -- none
 ---
@@ -3884,7 +3884,7 @@ c1("t:select({}, {limit = 0})") -- none
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c2("t:replace{1, 'new'}")
 ---
@@ -3892,7 +3892,7 @@ c2("t:replace{1, 'new'}")
 ...
 c2:commit()
 ---
-- 
+- null
 ...
 c1("t:select({}, {limit = 1})") -- {1, 'new'}
 ---
@@ -3900,7 +3900,7 @@ c1("t:select({}, {limit = 1})") -- {1, 'new'}
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c2("t:replace{2, 'new'}")
 ---
@@ -3908,7 +3908,7 @@ c2("t:replace{2, 'new'}")
 ...
 c2:commit()
 ---
-- 
+- null
 ...
 c1("t:select()") -- {1, 'new'}, {2, 'new'}
 ---
@@ -3916,7 +3916,7 @@ c1("t:select()") -- {1, 'new'}, {2, 'new'}
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 t:truncate()
 ---
@@ -3929,11 +3929,11 @@ _ = t:create_index('sk', {parts = {2, 'unsigned'}, unique = true})
 ...
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("t:insert{1, 2}")
 ---
@@ -3945,7 +3945,7 @@ c2("t:insert{2, 2}")
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 c2:commit() -- rollback
 ---
@@ -4034,19 +4034,19 @@ c4 = txn_proxy.new()
 ...
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c3:begin()
 ---
-- 
+- null
 ...
 c4:begin()
 ---
-- 
+- null
 ...
 box.stat.vinyl().tx.read_views -- 0 (no read views needed)
 ---
@@ -4094,7 +4094,7 @@ box.stat.vinyl().tx.transactions -- 4
 ...
 c4:commit()
 ---
-- 
+- null
 ...
 box.stat.vinyl().tx.read_views -- 1 (one read view for all TXs)
 ---
@@ -4106,7 +4106,7 @@ box.stat.vinyl().tx.transactions -- 3
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 box.stat.vinyl().tx.read_views -- 1 (one read view for all TXs)
 ---
@@ -4118,7 +4118,7 @@ box.stat.vinyl().tx.transactions -- 2
 ...
 c2:rollback()
 ---
-- 
+- null
 ...
 box.stat.vinyl().tx.read_views -- 1 (one read view for all TXs)
 ---
@@ -4130,7 +4130,7 @@ box.stat.vinyl().tx.transactions -- 1
 ...
 c3:commit()
 ---
-- 
+- null
 ...
 box.stat.vinyl().tx.read_views -- 0 (no read views needed)
 ---
diff --git a/test/vinyl/mvcc.test.lua b/test/vinyl/mvcc.test.lua
index 79b1f3428..9455e66fa 100644
--- a/test/vinyl/mvcc.test.lua
+++ b/test/vinyl/mvcc.test.lua
@@ -73,7 +73,7 @@ for i = 1,100 do
     c1(string.format("t:delete{%d}", i))
 end;
 for i = 1,100 do
-    assert(#c1(string.format("t:get{%d}", i)) == 0)
+    assert(c1(string.format("t:get{%d}", i)) == nil)
 end;
 test_run:cmd("setopt delimiter ''");
 
@@ -88,7 +88,7 @@ for i = 1,100 do
 end;
 c1:rollback();
 for i = 1,100 do
-    assert(#c1(string.format("t:get{%d}", i)) == 0)
+    assert(c1(string.format("t:get{%d}", i)) == nil)
 end;
 test_run:cmd("setopt delimiter ''");
 
diff --git a/test/vinyl/tx_conflict.result b/test/vinyl/tx_conflict.result
index 03cc62a49..7809cb1e3 100644
--- a/test/vinyl/tx_conflict.result
+++ b/test/vinyl/tx_conflict.result
@@ -186,7 +186,7 @@ function apply(t, k, op)
         table.insert(order_of_commit, t)
         num_committed = num_committed + 1
         local res = tx.con:commit()
-        if res ~= "" and res[1]['error'] then
+        if res ~= nil and res[1]['error'] then
             tx.conflicted = true
         else
             tx.select_all = s1:select{}
diff --git a/test/vinyl/tx_conflict.test.lua b/test/vinyl/tx_conflict.test.lua
index 9208c256e..a02b87246 100644
--- a/test/vinyl/tx_conflict.test.lua
+++ b/test/vinyl/tx_conflict.test.lua
@@ -153,7 +153,7 @@ function apply(t, k, op)
         table.insert(order_of_commit, t)
         num_committed = num_committed + 1
         local res = tx.con:commit()
-        if res ~= "" and res[1]['error'] then
+        if res ~= nil and res[1]['error'] then
             tx.conflicted = true
         else
             tx.select_all = s1:select{}
diff --git a/test/vinyl/tx_gap_lock.result b/test/vinyl/tx_gap_lock.result
index a456c017e..c1fa996b6 100644
--- a/test/vinyl/tx_gap_lock.result
+++ b/test/vinyl/tx_gap_lock.result
@@ -42,7 +42,7 @@ _ = s:insert{3}
 ...
 c:begin()
 ---
-- 
+- null
 ...
 c("s:select()") -- {1}, {3}
 ---
@@ -57,7 +57,7 @@ c("s:select()") -- {1}, {3}
 ...
 c:commit()
 ---
-- 
+- null
 ...
 s:truncate()
 ---
@@ -71,7 +71,7 @@ _ = s:insert{2}
 ...
 c:begin()
 ---
-- 
+- null
 ...
 c("s:select()") -- {1}, {2}
 ---
@@ -86,7 +86,7 @@ c("s:select()") -- {1}, {2}
 ...
 c:commit()
 ---
-- 
+- null
 ...
 s:truncate()
 ---
@@ -100,7 +100,7 @@ _ = s:insert{3}
 ...
 c:begin()
 ---
-- 
+- null
 ...
 c("s:select()") -- {2}, {3}
 ---
@@ -115,7 +115,7 @@ c("s:select()") -- {2}, {3}
 ...
 c:commit()
 ---
-- 
+- null
 ...
 s:truncate()
 ---
@@ -126,11 +126,11 @@ _ = s:insert{123}
 ...
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("s:select({}, {iterator = 'GT'})") -- {123}
 ---
@@ -153,11 +153,11 @@ c2("s:select({}, {iterator = 'LT'})") -- {123}
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 c2:commit()
 ---
-- 
+- null
 ...
 s:truncate()
 ---
@@ -176,27 +176,27 @@ _ = s:insert{30}
 ...
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c3:begin()
 ---
-- 
+- null
 ...
 c4:begin()
 ---
-- 
+- null
 ...
 c5:begin()
 ---
-- 
+- null
 ...
 c6:begin()
 ---
-- 
+- null
 ...
 c1("s:select({10}, {iterator = 'GE'})") -- {10}, {20}, {30}
 ---
@@ -254,11 +254,11 @@ _ = s:replace{15, 2} -- send c2 and c3 to read view
 ...
 c2("s:get(15)") -- none
 ---
-- 
+- null
 ...
 c3("s:get(15)") -- none
 ---
-- 
+- null
 ...
 c4("s:get(15)") -- {15, 2}
 ---
@@ -277,39 +277,39 @@ _ = s:replace{35, 3} -- send c4, c5, and c6 to read view
 ...
 c4("s:get(35)") -- none
 ---
-- 
+- null
 ...
 c5("s:get(35)") -- none
 ---
-- 
+- null
 ...
 c6("s:get(35)") -- none
 ---
-- 
+- null
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 c2:commit()
 ---
-- 
+- null
 ...
 c3:commit()
 ---
-- 
+- null
 ...
 c4:commit()
 ---
-- 
+- null
 ...
 c5:commit()
 ---
-- 
+- null
 ...
 c6:commit()
 ---
-- 
+- null
 ...
 s:truncate()
 ---
@@ -328,27 +328,27 @@ _ = s:insert{30}
 ...
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c3:begin()
 ---
-- 
+- null
 ...
 c4:begin()
 ---
-- 
+- null
 ...
 c5:begin()
 ---
-- 
+- null
 ...
 c6:begin()
 ---
-- 
+- null
 ...
 c1("s:select({30}, {iterator = 'LE'})") -- {30}, {20}, {10}
 ---
@@ -406,11 +406,11 @@ _ = s:replace{25, 2} -- send c2 and c3 to read view
 ...
 c2("s:get(25)") -- none
 ---
-- 
+- null
 ...
 c3("s:get(25)") -- none
 ---
-- 
+- null
 ...
 c4("s:get(25)") -- {25, 2}
 ---
@@ -429,39 +429,39 @@ _ = s:replace{5, 3} -- send c4, c5, and c6 to read view
 ...
 c4("s:get(5)") -- none
 ---
-- 
+- null
 ...
 c5("s:get(5)") -- none
 ---
-- 
+- null
 ...
 c6("s:get(5)") -- none
 ---
-- 
+- null
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 c2:commit()
 ---
-- 
+- null
 ...
 c3:commit()
 ---
-- 
+- null
 ...
 c4:commit()
 ---
-- 
+- null
 ...
 c5:commit()
 ---
-- 
+- null
 ...
 c6:commit()
 ---
-- 
+- null
 ...
 s:truncate()
 ---
@@ -474,19 +474,19 @@ for i = 1, 9 do s:insert{i * 10} end
 ...
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c3:begin()
 ---
-- 
+- null
 ...
 c4:begin()
 ---
-- 
+- null
 ...
 c1("s:select({20}, {iterator = 'GE', limit = 3})") -- {20}, {30}, {40}
 ---
@@ -562,7 +562,7 @@ _ = s:replace{25, 4} -- send c3 to read view
 ...
 c3("s:get(25)") -- none
 ---
-- 
+- null
 ...
 c4("s:get(25)") -- {25, 4}
 ---
@@ -573,23 +573,23 @@ _ = s:replace{75, 5} -- send c4 to read view
 ...
 c4("s:get(75)") -- none
 ---
-- 
+- null
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 c2:commit()
 ---
-- 
+- null
 ...
 c3:commit()
 ---
-- 
+- null
 ...
 c4:commit()
 ---
-- 
+- null
 ...
 s:drop()
 ---
@@ -620,11 +620,11 @@ _ = s:insert{3, 3}
 ...
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c1("s:select({2}, {iterator = 'EQ'})")  -- {2, 1}, {2, 2}, {2, 3}
 ---
@@ -669,11 +669,11 @@ c2("s:select({2}, {iterator = 'REQ'})") -- {2, 3}, {2, 2}, {2, 1}
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 c2:commit()
 ---
-- 
+- null
 ...
 s:drop()
 ---
@@ -708,7 +708,7 @@ gap_lock_count() -- 0
 ...
 c:begin()
 ---
-- 
+- null
 ...
 c("s:select({10}, {iterator = 'GE', limit = 4})") -- locks [10, 40]
 ---
@@ -769,11 +769,11 @@ _ = s:insert{25} -- send c to read view
 ...
 c("s:get(25)") -- none
 ---
-- 
+- null
 ...
 c:commit()
 ---
-- 
+- null
 ...
 s:truncate()
 ---
@@ -797,7 +797,7 @@ gap_lock_count() -- 0
 ...
 c:begin()
 ---
-- 
+- null
 ...
 c("s:select({1},  {iterator = 'GT', limit = 1})") -- locks  (1, 10]
 ---
@@ -842,11 +842,11 @@ _ = s:insert{5} -- send c to read view
 ...
 c("s:get(5)") -- none
 ---
-- 
+- null
 ...
 c:commit()
 ---
-- 
+- null
 ...
 s:truncate()
 ---
@@ -861,7 +861,7 @@ gap_lock_count() -- 0
 ...
 c:begin()
 ---
-- 
+- null
 ...
 c("s:select({100}, {iterator = 'GT'})") -- locks (100, +inf)
 ---
@@ -888,11 +888,11 @@ _ = s:insert{1000} -- send c to read view
 ...
 c("s:get(1000)") -- none
 ---
-- 
+- null
 ...
 c:commit()
 ---
-- 
+- null
 ...
 s:truncate()
 ---
@@ -916,7 +916,7 @@ gap_lock_count() -- 0
 ...
 c:begin()
 ---
-- 
+- null
 ...
 c("s:select({1}, {iterator = 'GE', limit = 2})") -- locks [1, 2]
 ---
@@ -932,7 +932,7 @@ gap_lock_count() -- 1
 ...
 c:commit()
 ---
-- 
+- null
 ...
 s:drop()
 ---
@@ -950,19 +950,19 @@ gap_lock_count() -- 0
 ...
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c3:begin()
 ---
-- 
+- null
 ...
 c4:begin()
 ---
-- 
+- null
 ...
 c1("s:select({100}, {iterator = 'GE'})") -- c1: locks [{100}, +inf)
 ---
@@ -1005,11 +1005,11 @@ _ = s:insert{100, 50} -- send c1 and c2 to read view
 ...
 c1("s:get({100, 50})") -- none
 ---
-- 
+- null
 ...
 c2("s:get({100, 50})") -- none
 ---
-- 
+- null
 ...
 c3("s:get({100, 50})") -- {100, 50}
 ---
@@ -1028,7 +1028,7 @@ _ = s:insert{100, 100} -- send c3 to read view
 ...
 c3("s:get({100, 100})") -- none
 ---
-- 
+- null
 ...
 c4("s:get({100, 100})") -- {100, 100}
 ---
@@ -1043,7 +1043,7 @@ _ = s:insert{100, 101} -- send c4 to read view
 ...
 c4("s:get({100, 101})") -- none
 ---
-- 
+- null
 ...
 gap_lock_count() -- 6
 ---
@@ -1051,19 +1051,19 @@ gap_lock_count() -- 6
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 c2:commit()
 ---
-- 
+- null
 ...
 c3:commit()
 ---
-- 
+- null
 ...
 c4:commit()
 ---
-- 
+- null
 ...
 s:truncate()
 ---
@@ -1075,19 +1075,19 @@ gap_lock_count() -- 0
 ...
 c1:begin()
 ---
-- 
+- null
 ...
 c2:begin()
 ---
-- 
+- null
 ...
 c3:begin()
 ---
-- 
+- null
 ...
 c4:begin()
 ---
-- 
+- null
 ...
 c1("s:select({100}, {iterator = 'LE'})") -- c1: locks (-inf, {100}]
 ---
@@ -1130,11 +1130,11 @@ _ = s:insert{100, 150} -- send c1 and c2 to read view
 ...
 c1("s:get({100, 150})") -- none
 ---
-- 
+- null
 ...
 c2("s:get({100, 150})") -- none
 ---
-- 
+- null
 ...
 c3("s:get({100, 150})") -- {100, 150}
 ---
@@ -1153,7 +1153,7 @@ _ = s:insert{100, 100} -- send c3 to read view
 ...
 c3("s:get({100, 100})") -- none
 ---
-- 
+- null
 ...
 c4("s:get({100, 100})") -- {100, 100}
 ---
@@ -1168,7 +1168,7 @@ _ = s:insert{100, 99} -- send c4 to read view
 ...
 c4("s:get({100, 99})") -- none
 ---
-- 
+- null
 ...
 gap_lock_count() -- 6
 ---
@@ -1176,19 +1176,19 @@ gap_lock_count() -- 6
 ...
 c1:commit()
 ---
-- 
+- null
 ...
 c2:commit()
 ---
-- 
+- null
 ...
 c3:commit()
 ---
-- 
+- null
 ...
 c4:commit()
 ---
-- 
+- null
 ...
 s:drop()
 ---
@@ -1391,7 +1391,8 @@ invalid = {};
 for i = 1, TX_COUNT do
     local tx = tx_list[i]
     local v = tx.conn(string.format("s:get({%d, %d})",
-                      conflict[1], conflict[2]))[1]
+                      conflict[1], conflict[2]))
+    v = v ~= nil and v[1] or nil
     local was_aborted = false
     if v == nil or v[PAYLOAD_FIELD] == nil then
         was_aborted = true
diff --git a/test/vinyl/tx_gap_lock.test.lua b/test/vinyl/tx_gap_lock.test.lua
index 4ad558608..b57bf0426 100644
--- a/test/vinyl/tx_gap_lock.test.lua
+++ b/test/vinyl/tx_gap_lock.test.lua
@@ -508,7 +508,8 @@ invalid = {};
 for i = 1, TX_COUNT do
     local tx = tx_list[i]
     local v = tx.conn(string.format("s:get({%d, %d})",
-                      conflict[1], conflict[2]))[1]
+                      conflict[1], conflict[2]))
+    v = v ~= nil and v[1] or nil
     local was_aborted = false
     if v == nil or v[PAYLOAD_FIELD] == nil then
         was_aborted = true
diff --git a/test/vinyl/tx_serial.result b/test/vinyl/tx_serial.result
index 37c3f4467..133fec710 100644
--- a/test/vinyl/tx_serial.result
+++ b/test/vinyl/tx_serial.result
@@ -152,7 +152,7 @@ function apply(t, k, op)
         table.insert(order_of_commit, t)
         num_committed = num_committed + 1
         local res = tx.con:commit()
-        if res ~= "" and res[1]['error'] then
+        if res ~= nil and res[1]['error'] then
             tx.conflicted = true
         else
             tx.select_all = s1:select{}
diff --git a/test/vinyl/tx_serial.test.lua b/test/vinyl/tx_serial.test.lua
index 0a695a7e5..7395ef36a 100644
--- a/test/vinyl/tx_serial.test.lua
+++ b/test/vinyl/tx_serial.test.lua
@@ -123,7 +123,7 @@ function apply(t, k, op)
         table.insert(order_of_commit, t)
         num_committed = num_committed + 1
         local res = tx.con:commit()
-        if res ~= "" and res[1]['error'] then
+        if res ~= nil and res[1]['error'] then
             tx.conflicted = true
         else
             tx.select_all = s1:select{}
diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc
index 73e5d2c31..3a427263e 100644
--- a/third_party/lua-yaml/lyaml.cc
+++ b/third_party/lua-yaml/lyaml.cc
@@ -121,12 +121,10 @@ yaml_get_bool(const char *str, const size_t len)
 
 /**
  * Verify whether a string represents a null literal in YAML.
- *
- * Non-standard: don't match an empty string as null.
  */
 static yaml_type
 yaml_get_null(const char *str, const size_t len){
-   if (len == 1 && str[0] == '~')
+   if (len == 0 || (len == 1 && str[0] == '~'))
       return YAML_NULL;
    if (len == 4 && (strcmp(str, "null") == 0 || strcmp(str, "Null") == 0 ||
        strcmp(str, "NULL") == 0))
@@ -259,15 +257,7 @@ static void load_scalar(struct lua_yaml_loader *loader) {
 
    if (loader->event.data.scalar.style == YAML_PLAIN_SCALAR_STYLE) {
       yaml_type type;
-      if (!length) {
-         /*
-          * Non-standard: an empty value/document is null
-          * according to the standard, but we decode it as an
-          * empty string.
-          */
-         lua_pushliteral(loader->L, "");
-         return;
-      } else if (yaml_get_null(str, length) == YAML_NULL) {
+      if (yaml_get_null(str, length) == YAML_NULL) {
          luaL_pushnull(loader->L);
          return;
       } else if ((type = yaml_get_bool(str, length)) != YAML_NO_MATCH) {
@@ -407,8 +397,14 @@ static void load(struct lua_yaml_loader *loader) {
       if (!do_parse(loader))
          return;
 
-      if (loader->event.type == YAML_STREAM_END_EVENT)
+      if (loader->event.type == YAML_STREAM_END_EVENT) {
+         if (loader->document_count == 0) {
+            /* Return null, not zero values count. */
+            loader->document_count++;
+            luaL_pushnull(loader->L);
+         }
          return;
+      }
 
       loader->document_count++;
       if (load_node(loader) != 1)
-- 
2.20.1

[tarantool-patches] Re: [PATCH v2 3/3] lua-yaml: treat an empty document/value as null

[Vladislav Shpilevoy]

Thanks for the patch!

On 22/01/2019 05:12, Alexander Turenko wrote:
> It improves our compatibility with the YAML standard.
> 
> yaml.encode('') result changed from
> 
> ---
> ...
> 
> to
> 
> --- ''
> ...
> 
> yaml.decode('') returns zero values count before, now it returns
> box.NULL. yaml.decode('- ') returns {''} before, now {box.NULL}.
> 
> This commit touches many tests and test result files, which use console,
> without behaviour changes. It also adds two test cases to
> app-tap/console.test.lua.

Not sure if it makes sense listing factual changes here, because
literally the same I see below and in "git diff --stat".

> ---
>   test/app-tap/console.test.lua   |   16 +-
>   test/app/fio.result             |    2 +-
>   test/app/socket.result          |   26 +-
>   test/box/access.result          |    8 +-
>   test/box/role.result            |    8 +-
>   test/box/sequence.result        |    2 +-
>   test/vinyl/errinj_tx.result     |   10 +-
>   test/vinyl/hermitage.result     |  122 ++--
>   test/vinyl/mvcc.result          | 1066 +++++++++++++++----------------
>   test/vinyl/mvcc.test.lua        |    4 +-
>   test/vinyl/tx_conflict.result   |    2 +-
>   test/vinyl/tx_conflict.test.lua |    2 +-
>   test/vinyl/tx_gap_lock.result   |  189 +++---
>   test/vinyl/tx_gap_lock.test.lua |    3 +-
>   test/vinyl/tx_serial.result     |    2 +-
>   test/vinyl/tx_serial.test.lua   |    2 +-
>   third_party/lua-yaml/lyaml.cc   |   22 +-
>   17 files changed, 747 insertions(+), 739 deletions(-)
> 
> diff --git a/test/vinyl/mvcc.result b/test/vinyl/mvcc.result
> index 1941744b9..db7fc8e83 100644
> --- a/test/vinyl/mvcc.result
> +++ b/test/vinyl/mvcc.result
> @@ -41,29 +41,29 @@ t = box.space.test
>   --
>   c1:begin()
>   ---
> -- 
> +- null

How about to start returning nothing instead of returning null so
as to do not change the tests (or at least make the diff smaller)?

I see, that most of the diff is from txn_proxy, which always does
return 'something'. You can change it so as to do not just return, but
check if a result is not null, and only then return. Otherwise do nothing
or empty return.

I am sure it will work since space:get(), for example, still can
return nothing (!= null), and this is why other tests didn't fail.

[tarantool-patches] Re: [PATCH v2 3/3] lua-yaml: treat an empty document/value as null

[Alexander Turenko]

Hi!

Thanks. Your scepsis pushed me to rethink the change and I decided to
discard this patch. See below for more info.

WBR, Alexander Turenko.

On Fri, Jan 25, 2019 at 12:26:49AM +0300, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> On 22/01/2019 05:12, Alexander Turenko wrote:
> > It improves our compatibility with the YAML standard.
> > 
> > yaml.encode('') result changed from
> > 
> > ---
> > ...
> > 
> > to
> > 
> > --- ''
> > ...
> > 
> > yaml.decode('') returns zero values count before, now it returns
> > box.NULL. yaml.decode('- ') returns {''} before, now {box.NULL}.
> > 
> > This commit touches many tests and test result files, which use console,
> > without behaviour changes. It also adds two test cases to
> > app-tap/console.test.lua.
> 
> Not sure if it makes sense listing factual changes here, because
> literally the same I see below and in "git diff --stat".

The idea of the message was to mark which test changes are just due to
the change and which one are specifically designed to test the new
feature. I tried to clarify that and would rewrite as follows (but
dropped the patch, see below).

```
Several test cases were added to the app-tap/console.test.lua test,
other test and test result changes are just induced by the encode/decode
behaviour change.
```

> 
> > ---
> >   test/app-tap/console.test.lua   |   16 +-
> >   test/app/fio.result             |    2 +-
> >   test/app/socket.result          |   26 +-
> >   test/box/access.result          |    8 +-
> >   test/box/role.result            |    8 +-
> >   test/box/sequence.result        |    2 +-
> >   test/vinyl/errinj_tx.result     |   10 +-
> >   test/vinyl/hermitage.result     |  122 ++--
> >   test/vinyl/mvcc.result          | 1066 +++++++++++++++----------------
> >   test/vinyl/mvcc.test.lua        |    4 +-
> >   test/vinyl/tx_conflict.result   |    2 +-
> >   test/vinyl/tx_conflict.test.lua |    2 +-
> >   test/vinyl/tx_gap_lock.result   |  189 +++---
> >   test/vinyl/tx_gap_lock.test.lua |    3 +-
> >   test/vinyl/tx_serial.result     |    2 +-
> >   test/vinyl/tx_serial.test.lua   |    2 +-
> >   third_party/lua-yaml/lyaml.cc   |   22 +-
> >   17 files changed, 747 insertions(+), 739 deletions(-)
> > 
> > diff --git a/test/vinyl/mvcc.result b/test/vinyl/mvcc.result
> > index 1941744b9..db7fc8e83 100644
> > --- a/test/vinyl/mvcc.result
> > +++ b/test/vinyl/mvcc.result
> > @@ -41,29 +41,29 @@ t = box.space.test
> >   --
> >   c1:begin()
> >   ---
> > -- 
> > +- null
> 
> How about to start returning nothing instead of returning null so
> as to do not change the tests (or at least make the diff smaller)?
> 
> I see, that most of the diff is from txn_proxy, which always does
> return 'something'. You can change it so as to do not just return, but
> check if a result is not null, and only then return. Otherwise do nothing
> or empty return.
> 
> I am sure it will work since space:get(), for example, still can
> return nothing (!= null), and this is why other tests didn't fail.

Changing tests is not the main question here. The question is what is
the right thing to do with encoding of an empty document. It is null
according to the standard. But our approach allows to encode null and
empty values set differently.

I factored out this patch from the Alex's Kh. original commit to discuss
it separately. After a yesterday discussion with Vladimir I understood
that current approach has its benefit (see above). So there are no
reason to change it at least until it will be requested by someone. I
dropped the patch from the patchset.

[tarantool-patches] Re: [PATCH v2 0/3] lua-yaml null/boolean fixes

[Vladislav Shpilevoy]

Hi! Thanks for the patchset.

On 22/01/2019 05:12, Alexander Turenko wrote:
> https://github.com/tarantool/tarantool/issues/3476
> https://github.com/tarantool/tarantool/issues/3662
> https://github.com/tarantool/tarantool/issues/3583
> https://github.com/tarantool/tarantool/tree/kh/gh-3662-yaml-2.1

Please, next time write a cover-letter.

> 
> AKhatskevich (1):
>    lua-yaml: verify arguments count
> 
> Alexander Turenko (2):
>    lua-yaml: fix boolean/null representation in yaml
>    lua-yaml: treat an empty document/value as null
> 
>   test/app-tap/console.test.lua   |   37 +-
>   test/app/fio.result             |    2 +-
>   test/app/socket.result          |   26 +-
>   test/box/access.result          |    8 +-
>   test/box/misc.result            |    2 +-
>   test/box/role.result            |    8 +-
>   test/box/sequence.result        |    2 +-
>   test/vinyl/errinj_tx.result     |   10 +-
>   test/vinyl/hermitage.result     |  122 ++--
>   test/vinyl/mvcc.result          | 1066 +++++++++++++++----------------
>   test/vinyl/mvcc.test.lua        |    4 +-
>   test/vinyl/tx_conflict.result   |    2 +-
>   test/vinyl/tx_conflict.test.lua |    2 +-
>   test/vinyl/tx_gap_lock.result   |  189 +++---
>   test/vinyl/tx_gap_lock.test.lua |    3 +-
>   test/vinyl/tx_serial.result     |    2 +-
>   test/vinyl/tx_serial.test.lua   |    2 +-
>   third_party/lua-yaml/lyaml.cc   |   87 ++-
>   18 files changed, 819 insertions(+), 755 deletions(-)
> 

[tarantool-patches] Re: [PATCH v2 0/3] lua-yaml null/boolean fixes

[Kirill Yukhin]

Hello,

On 22 Jan 05:12, Alexander Turenko wrote:
> https://github.com/tarantool/tarantool/issues/3476
> https://github.com/tarantool/tarantool/issues/3662
> https://github.com/tarantool/tarantool/issues/3583
> https://github.com/tarantool/tarantool/tree/kh/gh-3662-yaml-2.1
> 
> AKhatskevich (1):
>   lua-yaml: verify arguments count
> 
> Alexander Turenko (2):
>   lua-yaml: fix boolean/null representation in yaml
>   lua-yaml: treat an empty document/value as null

I've checked your patches (1/3 and 2/3) into 2.1 branch.

Could you please rebase the patchset on top of 1.10 as well?

--
Regards, Kirill Yukhin

[tarantool-patches] Re: [PATCH v2 0/3] lua-yaml null/boolean fixes

[Alexander Turenko]

On Mon, Feb 25, 2019 at 02:27:24PM +0300, Kirill Yukhin wrote:
> Hello,
> 
> On 22 Jan 05:12, Alexander Turenko wrote:
> > https://github.com/tarantool/tarantool/issues/3476
> > https://github.com/tarantool/tarantool/issues/3662
> > https://github.com/tarantool/tarantool/issues/3583
> > https://github.com/tarantool/tarantool/tree/kh/gh-3662-yaml-2.1
> > 
> > AKhatskevich (1):
> >   lua-yaml: verify arguments count
> > 
> > Alexander Turenko (2):
> >   lua-yaml: fix boolean/null representation in yaml
> >   lua-yaml: treat an empty document/value as null
> 
> I've checked your patches (1/3 and 2/3) into 2.1 branch.
> 
> Could you please rebase the patchset on top of 1.10 as well?

I cherry-picked these two commits on top of fresh 1.10 and all seems to
be okay (the commits are applied cleanly and all tests are passed).

I pushed it to https://github.com/tarantool/tarantool/commits/kh/gh-3662-yaml-1.10
Travis-CI: https://travis-ci.org/tarantool/tarantool/builds/502095402

WBR, Alexander Turenko.

> 
> --
> Regards, Kirill Yukhin

[tarantool-patches] Re: [PATCH v2 0/3] lua-yaml null/boolean fixes

[Kirill Yukhin]

Hello,

On 05 Mar 19:40, Alexander Turenko wrote:
> On Mon, Feb 25, 2019 at 02:27:24PM +0300, Kirill Yukhin wrote:
> > Hello,
> > 
> > On 22 Jan 05:12, Alexander Turenko wrote:
> > > https://github.com/tarantool/tarantool/issues/3476
> > > https://github.com/tarantool/tarantool/issues/3662
> > > https://github.com/tarantool/tarantool/issues/3583
> > > https://github.com/tarantool/tarantool/tree/kh/gh-3662-yaml-2.1
> > > 
> > > AKhatskevich (1):
> > >   lua-yaml: verify arguments count
> > > 
> > > Alexander Turenko (2):
> > >   lua-yaml: fix boolean/null representation in yaml
> > >   lua-yaml: treat an empty document/value as null
> > 
> > I've checked your patches (1/3 and 2/3) into 2.1 branch.
> > 
> > Could you please rebase the patchset on top of 1.10 as well?
> 
> I cherry-picked these two commits on top of fresh 1.10 and all seems to
> be okay (the commits are applied cleanly and all tests are passed).
> 
> I pushed it to https://github.com/tarantool/tarantool/commits/kh/gh-3662-yaml-1.10
> Travis-CI: https://travis-ci.org/tarantool/tarantool/builds/502095402

Strange. Anyway, I've comitted it to 1.10 as well.

--
Regards, Kirill Yukhin