Tarantool development patches archive
 help / color / mirror / Atom feed
From: Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Maxim Kokryashkin <m.kokryashkin@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH 1/4] test: resolving program name
Date: Tue, 6 Jul 2021 23:29:23 +0300
Message-ID: <20210706202923.GB11494@tarantool.org> (raw)
In-Reply-To: <d377115fa4b7cb3d7f22eb248dccefc5a1ac3c3d.1625484589.git.max.kokryashkin@gmail.com>

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

  reply	other threads:[~2021-07-06 20:53 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-05 11:49 [Tarantool-patches] [PATCH 0/4] test: bump lua-Harness suite Maxim Kokryashkin via Tarantool-patches
2021-07-05 11:49 ` [Tarantool-patches] [PATCH 1/4] test: resolving program name Maxim Kokryashkin via Tarantool-patches
2021-07-06 20:29   ` Igor Munkin via Tarantool-patches [this message]
2021-07-05 11:49 ` [Tarantool-patches] [PATCH 2/4] test: out-of-source testing Maxim Kokryashkin via Tarantool-patches
2021-07-06 20:29   ` Igor Munkin via Tarantool-patches
2021-07-05 11:49 ` [Tarantool-patches] [PATCH 3/4] test: CI-environment Maxim Kokryashkin via Tarantool-patches
2021-07-06 20:29   ` Igor Munkin via Tarantool-patches
2021-07-05 11:49 ` [Tarantool-patches] [PATCH 4/4] test: TAP module name collisions Maxim Kokryashkin via Tarantool-patches
2021-07-06 20:30   ` Igor Munkin via Tarantool-patches
2021-07-06 20:29 ` [Tarantool-patches] [PATCH 0/4] test: bump lua-Harness suite Igor Munkin via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210706202923.GB11494@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imun@tarantool.org \
    --cc=m.kokryashkin@tarantool.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Tarantool development patches archive

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lists.tarantool.org/tarantool-patches/0 tarantool-patches/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 tarantool-patches tarantool-patches/ https://lists.tarantool.org/tarantool-patches \
		tarantool-patches@dev.tarantool.org.
	public-inbox-index tarantool-patches

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git