Re: [Tarantool-patches] [PATCH 1/4] test: resolving program name

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Max,

Thanks for the patch! I left several comments below, please consider
them in addition to the main comments in the cover letter.

On 05.07.21, Maxim Kokryashkin wrote:
> From: Maxim Kokryashkin <m.kokryashkin@tarantool.org>
> 
> Part of tarantool/tarantool#5970
> ---
> The patch `fperrad/lua-Harness@1be25a8` from lua-Harness suite is the 
> same as the patch `tarantool/luajit@8376885`, except for 

We use the different scheme to mention the particular patch.
* If this is the patch from this repository, then we just use 40-digit
  hash + commit subject enclosed in a single quotes and parenthesis.
  E.g. 837688590919fcf8de47ef90479fd2a640c8fddc ('test: adjust
  lua-Harness suite to CMake machinery').
* If this is the patch from another repository, we just use the full
  link. For the link in plain text (i.e. commit message) we use the next
  policy: <link label>[<index>] + [<index>]: <link> at the end. E.g.
  this commit[1] links to this particular patch pushed to our LuaJIT
  fork (look for the link at the end of the message).

> `get_lua_binary_name()`, which was renamed to `_retrieve_progname()` 
> in `fperadd/lua-Harness`.

It also worth to mention the patch, where <get_lua_binary_name> is
introduced, since the original patch in lua-Harness differs from yours
one. Considering everything written above, I propose the following
rewording for commit message:

| The patch[1] from lua-Harness suite fixes the same issue commit
| 837688590919fcf8de47ef90479fd2a640c8fddc ('test: adjust lua-Harness
| suite to CMake machinery') does except the single difference:
| `get_lua_binary_name()` is called `_retrieved_progname()`. As a result
| of this patch the function is renamed to follow the original naming.
|
| [1]: https://framagit.org/fperrad/lua-Harness/-/commit/1be25a8
|
| Part of tarantool/tarantool#5970
| Part of tarantool/tarantool#4473

If you are OK with the part above, feel free to paste it as is into the
commit message for the next version of the patch.

One more comment: I see Sergey confused you with his review. As it was
mentioned in the previous review, the patch "body" is splitted into two
parts by three dashes ('---').
* The part above is the commit message and it should contain only the
  polished description of the patch (in other words, everything you have
  written before my comment).
* The part below is your blackboard/canvas/etc: you can put there any
  supplementary or auxiliary information, such as branch name (missing
  both here and in the cover letter), link to the issue (also missing),
  test results (also missing).

> 
>  test/lua-Harness-tests/241-standalone.t.disabled | 2 +-
>  test/lua-Harness-tests/242-luac.t                | 2 +-
>  test/lua-Harness-tests/301-basic.t               | 2 +-
>  test/lua-Harness-tests/308-io.t                  | 2 +-
>  test/lua-Harness-tests/309-os.t                  | 2 +-
>  test/lua-Harness-tests/320-stdin.t               | 2 +-
>  test/lua-Harness-tests/411-luajit.t.disabled     | 2 +-
>  test/lua-Harness-tests/tap.lua                   | 8 ++++++++
>  8 files changed, 15 insertions(+), 7 deletions(-)
> 

<snipped>

> diff --git a/test/lua-Harness-tests/tap.lua b/test/lua-Harness-tests/tap.lua
> index a8454ae0..86cca4e0 100644
> --- a/test/lua-Harness-tests/tap.lua
> +++ b/test/lua-Harness-tests/tap.lua
> @@ -9,6 +9,14 @@
>  
>  ]]
>  
> +function _retrieve_progname ()
> +    local i = 0
> +    while arg[i] do
> +        i = i - 1
> +    end
> +    return arg[i + 1]
> +end

Please also remove <get_lua_binary_name> within this patch.

> +
>  if pcall(require, 'Test.More') then
>      diag 'Test.More loaded'
>      return
> -- 
> 2.31.1
> 

[1]: https://github.com/tarantool/luajit/commit/d377115

-- 
Best regards,
IM