Re: [Tarantool-patches] [PATCH v2 05/10] box/error: don't set error created via box.error.new to diag

[Vladislav Shpilevoy <v.shpilevoy@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.