Re: [Tarantool-patches] [PATCH] lua/key_def: fix compare_with_key() part count check

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

On 17.12.2020 14:27, Sergey Nikiforov via Tarantool-patches wrote:
> Hi!
> 
> On 17.12.2020 1:34, Vladislav Shpilevoy wrote:
>> Hi!
>>
>> On 14.12.2020 16:35, Sergey Nikiforov wrote:
>>> Added corresponding test
>>>
>>> Fixes: #5307
>>> ---
>>
>> Please, try to follow the guidelines.
>> https://github.com/tarantool/tarantool/wiki/Code-review-procedure
>>
>> I don't see the issue and branch links. Also I can't find the
>> branch in `git branch -a | grep 5307`. Also in the ticket's
>> webpage I don't see any links to the commit, which github usually
>> adds automatically. So I suspect you simply didn't push it anywhere.
> 
> Of course I did not push my branches for the first batch of patches - because no one said I had to. I am new to github and Tarantool workflow, sorry.

Ok, now you know! But where is the branch? I don't see a link,
and `git branch -a | grep 5307` is empty.

>>> diff --git a/src/box/lua/key_def.c b/src/box/lua/key_def.c
>>> index a781aeff9..674891a85 100644
>>> --- a/src/box/lua/key_def.c
>>> +++ b/src/box/lua/key_def.c
>>> @@ -362,6 +362,15 @@ lbox_key_def_compare_with_key(struct lua_State *L)
>>>       size_t key_len;
>>>       const char *key_end, *key = lbox_encode_tuple_on_gc(L, 3, &key_len);
>>>       uint32_t part_count = mp_decode_array(&key);
>>> +
>>> +    if (part_count > key_def->part_count) {
>>> +        region_truncate(region, region_svp);
>>> +        tuple_unref(tuple);
>>> +        diag_set(ClientError, ER_KEY_PART_COUNT,
>>> +             key_def->part_count, part_count);
>>> +        return luaT_error(L);
>>> +    }
>>
>> Why this check and the call below can't be all simply
>> replaces with box_key_def_validate_key() call?
> 
> Because we need part_count later. With box_key_def_validate_key() we would have to call mp_decode_array() twice or add yet another parameter to box_key_def_validate_key(). Is that good idea?

Ok, then the patch looks fine in the email. I would like
to see it on the branch to check how it works.

You may also try to move region_truncate + tuple_unref to
some common place and do goto to there. These actions are
done 3 times now.