From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <v.shpilevoy@tarantool.org>
Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3])
 (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
 (No client certificate requested)
 by dev.tarantool.org (Postfix) with ESMTPS id EC640442BB6
 for <tarantool-patches@dev.tarantool.org>;
 Fri, 27 Mar 2020 03:19:31 +0300 (MSK)
References: <cover.1585097339.git.korablev@tarantool.org>
 <1816245c768216733779be5c7420cc60725287ea.1585097339.git.korablev@tarantool.org>
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Message-ID: <536126ef-fef2-9f01-2d34-e0d1440d8eee@tarantool.org>
Date: Fri, 27 Mar 2020 01:19:30 +0100
MIME-Version: 1.0
In-Reply-To: <1816245c768216733779be5c7420cc60725287ea.1585097339.git.korablev@tarantool.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Subject: Re: [Tarantool-patches] [PATCH v2 05/10] box/error: don't set error
 created via box.error.new to diag
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Nikita Pettik <korablev@tarantool.org>, tarantool-patches@dev.tarantool.org

Hi! Thanks for the patch!

See 2 comments below.

On 25/03/2020 02:43, Nikita Pettik wrote:
> To achieve this let's refactor luaT_error_create() to return error
> object instead of setting it via box_error_set().
> luaT_error_create() is used both to handle box.error() and
> box.error.new() invocations, and box.error() is still expected to set
> error to diagnostic area. So, luaT_error_call() which implements
> box.error() processing at the end calls diag_set_error().
> It is worth mentioning that net.box module relied on the fact that
> box.error.new() set error to diagnostic area: otherwise request errors
> don't get to diagnostic area on client side.
> 
> Needed for #1148
> Closes #4778
> 
> @TarantoolBot document
> Title: Don't promote error created via box.error.new to diagnostic area
> 
> Now box.error.new() only creates error object, but doesn't set it to
> Tarantool's diagnostic area:
> ```
> box.error.clear()
> e = box.error.new({code = 111, reason = "cause"})
> assert(box.error.last() == nil)
> ---
> - true
> ...
> ```
> To set error in diagnostic area explicitly box.error.set() has been
> introduced. It accepts error object which is set as last system error
> (i.e. becomes available via box.error.last()).
> Finally, box.error.new() does not longer accept error object as an
> argument (this was undocumented feature).
> Note that patch does not affect box.error(), which still pushed error to

1. 'pushed' -> 'pushes'.

> diff --git a/src/box/lua/error.cc b/src/box/lua/error.cc
> index 640e33910..ff285d7eb 100644
> --- a/src/box/lua/error.cc
> +++ b/src/box/lua/error.cc
> @@ -139,9 +144,12 @@ luaT_error_new(lua_State *L)
>  {
>  	if (lua_gettop(L) == 0)
>  		return luaL_error(L, "Usage: box.error.new(code, args)");
> -	luaT_error_create(L, 1);
> +	struct error *e = luaT_error_create(L, 1);
> +	if (e == NULL)
> +		return luaL_error(L, "box.error.new(): bad arguments");

2. I think it would be better to return the same 'Usage' error
in case of a problem with arguments. Not two different messages
meaning essentially the same. Or both should return 'bad arguments'.
However with both 'Usage' the diff would be 1 line smaller.

Other than these comments, LGTM. If you fix them both - then push right
away.