Re: [Tarantool-patches] [PATCH luajit 1/8] test: introduce LUAJIT_TEST_VARDIR variable

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

Sergey,

On 02.09.22, Sergey Bronnikov wrote:
> Hi,
> 
> On 31.08.2022 17:53, Igor Munkin wrote:
> > Sergey,
> >
> > Thanks for your review!
> >
> > On 15.08.22, Sergey Bronnikov wrote:
> >> Igor,
> >>
> >>    thanks for the patch. See my comments inline.
> >>
> >> On 11.08.2022 14:17, Igor Munkin wrote:
> >>> Before the patch both memprof and sysprof artefacts are generated within
> >>> the binary artefacts tree (i.e. in the same directory LuaJIT binary is
> >>> generated). However, more convenient way is producing these temporary
> >>> profiles in a separate directory (e.g. located on the partition with
> >>> more strict space limits). As a result of the patch all memprof and
> >>> sysprof test chunks consider LUAJIT_TEST_VARDIR environment variable to
> >>> set the directory where test profiles are generated. For the case when
> >>> LUAJIT_TEST_VARDIR is not set, everything works as before.
> >> Commit message is inconsistent a bit with a patch itself.
> >>
> >> As far as I understand many hunks are not related to introducing
> >> LUAJIT_TEST_VARDIR.
> >>
> >> Probably it is better to change commit one-line message from "test:
> >> introduce LUAJIT_TEST_VARDIR variable"
> >>
> >> to something like "test: refactoring and introduce LUAJIT_TEST_VARDIR
> >> variable". It allows to keep patch
> >>
> >> as is and change expectations for those who will look at your patch.
> > I believe your commit subject is too long, so I propose another
> > solution. Since the only thing affected by LUAJIT_TEST_VARDIR is
> > <utils.profilename>, so I can write something like "test: introduce
> > utils.profilename helper" and describe its rationale and functions
> > within the commit message. Are you OK with it?
> 
> I'm ok!

I've rewritten the commit message the following way:
| test: introduce utils.profilename helper
|
| Before the patch both memprof and sysprof artefacts were generated
| within the binary artefacts tree (i.e. in the same directory LuaJIT
| binary is generated). However, more convenient way is producing these
| temporary profiles in a separate directory (e.g. located on the
| partition with more strict space limits).
|
| As a result of the patch <utils.profilename> helper is introduced and
| all memprof and sysprof test chunks are patched to use it. The new
| helper takes the basename of the profile to be created and also
| considers LUAJIT_TEST_VARDIR environment variable to set the directory,
| where test profiles are generated. For the case when LUAJIT_TEST_VARDIR
| is not set, everything works as before.
|
| Part of tarantool/tarantool#7472

> 
> <snipped>

-- 
Best regards,
IM