<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body>
    <p>Hi, Max!</p>
    <p>Thanks for your comments!</p>
    <p>See my answers below. Updated patch was force-pushed.</p>
    <p><br>
    </p>
    <div class="moz-cite-prefix">On 7/17/23 19:10, Maxim Kokryashkin via
      Tarantool-patches wrote:<br>
    </div>
    <blockquote type="cite"
      cite="mid:1689610201.27637747@f738.i.mail.ru">
      <meta http-equiv="content-type" content="text/html; charset=UTF-8">
      <div>Hi, Sergey!</div>
      <div>Please consider my comments below.</div>
      <div> </div>
      <blockquote style="border-left:1px solid #0857A6; margin:10px;
        padding:0 0 0 10px;">
        <div> 
          <blockquote style="border-left:1px solid #0857A6; margin:10px;
            padding:0 0 0 10px;">
            <div id="">
              <div class="js-helper js-readmsg-msg">
                <div>
                  <div id="style_16896011180277443001_BODY">From: Sergey
                    Bronnikov <<a
                      href="/compose?To=sergeyb@tarantool.org"
                      moz-do-not-send="true">sergeyb@tarantool.org</a>><br>
                    <br>
                    In Tarantool we use our own fork of checkpatch [2]
                    with additional check<br>
                    types. It's logical to use it in a LuaJIT
                    development. We don't need</div>
                </div>
              </div>
            </div>
          </blockquote>
          <div>Typo: s/in a/in/</div>
        </div>
      </blockquote>
    </blockquote>
    Fixed.<br>
    <blockquote type="cite"
      cite="mid:1689610201.27637747@f738.i.mail.ru">
      <blockquote style="border-left:1px solid #0857A6; margin:10px;
        padding:0 0 0 10px;">
        <div>
          <blockquote style="border-left:1px solid #0857A6; margin:10px;
            padding:0 0 0 10px;">
            <div>
              <div class="js-helper js-readmsg-msg">
                <div>
                  <div>check tags in commit messages like NO_DOC,
                    NO_CHANGELOG, NO_TEST and<br>
                    others, so to be able to customize command-line
                    options Github Action, provided<br>
                    by checkpatch repository [3], was added to the
                    repository.</div>
                </div>
              </div>
            </div>
          </blockquote>
          <div>Typo: s/by checkpatch/by the checkpatch/</div>
        </div>
      </blockquote>
    </blockquote>
    Fixed.<br>
    <blockquote type="cite"
      cite="mid:1689610201.27637747@f738.i.mail.ru">
      <blockquote style="border-left:1px solid #0857A6; margin:10px;
        padding:0 0 0 10px;">
        <div>
          <blockquote style="border-left:1px solid #0857A6; margin:10px;
            padding:0 0 0 10px;">
            <div>
              <div class="js-helper js-readmsg-msg">
                <div>
                  <div><br>
                    See documentation for used checkpatch in [4].</div>
                </div>
              </div>
            </div>
          </blockquote>
          <div>Typo: s/documentation/the documentation/</div>
          <div>Typo: s/for used/for the/</div>
        </div>
      </blockquote>
    </blockquote>
    Fixed. Fixed.<br>
    <blockquote type="cite"
      cite="mid:1689610201.27637747@f738.i.mail.ru">
      <blockquote style="border-left:1px solid #0857A6; margin:10px;
        padding:0 0 0 10px;">
        <div>
          <blockquote style="border-left:1px solid #0857A6; margin:10px;
            padding:0 0 0 10px;">
            <div>
              <div class="js-helper js-readmsg-msg">
                <div>
                  <div><br>
                    Patch introduce new CMake targets:
                    LuaJIT-checkpatch, that checks</div>
                </div>
              </div>
            </div>
          </blockquote>
          <div>Typo: s/introduce/introduces/</div>
        </div>
      </blockquote>
    </blockquote>
    Fixed.<br>
    <blockquote type="cite"
      cite="mid:1689610201.27637747@f738.i.mail.ru">
      <blockquote style="border-left:1px solid #0857A6; margin:10px;
        padding:0 0 0 10px;">
        <div>
          <blockquote style="border-left:1px solid #0857A6; margin:10px;
            padding:0 0 0 10px;">
            <div>
              <div class="js-helper js-readmsg-msg">
                <div>
                  <div>patches on top of master branch using script
                    checkpatch.pl [1], and</div>
                </div>
              </div>
            </div>
          </blockquote>
          <div>Typo: s/on top of/on top of the/</div>
        </div>
      </blockquote>
    </blockquote>
    Fixed.<br>
    <blockquote type="cite"
      cite="mid:1689610201.27637747@f738.i.mail.ru">
      <blockquote style="border-left:1px solid #0857A6; margin:10px;
        padding:0 0 0 10px;">
        <div>
          <blockquote style="border-left:1px solid #0857A6; margin:10px;
            padding:0 0 0 10px;">
            <div>
              <div class="js-helper js-readmsg-msg">
                <div>
                  <div>target check, that combines LuaJIT-luacheck and
                    LuaJIT-checkpatch.<br>
                    <br>
                    1. <a
                      href="https://docs.kernel.org/dev-tools/checkpatch.html"
                      target="_blank" moz-do-not-send="true"
                      class="moz-txt-link-freetext">https://docs.kernel.org/dev-tools/checkpatch.html</a><br>
                    2. <a
                      href="https://github.com/tarantool/checkpatch"
                      target="_blank" moz-do-not-send="true"
                      class="moz-txt-link-freetext">https://github.com/tarantool/checkpatch</a><br>
                    3. <a
href="https://github.com/tarantool/checkpatch/blob/master/.github/actions/checkpatch/action.yml"
                      target="_blank" moz-do-not-send="true"
                      class="moz-txt-link-freetext">https://github.com/tarantool/checkpatch/blob/master/.github/actions/checkpatch/action.yml</a><br>
                    4. <a
href="https://github.com/tarantool/checkpatch/blob/master/doc/checkpatch.rst"
                      target="_blank" moz-do-not-send="true"
                      class="moz-txt-link-freetext">https://github.com/tarantool/checkpatch/blob/master/doc/checkpatch.rst</a></div>
                </div>
              </div>
            </div>
          </blockquote>
          <div>Nit: It is kinda strange to see link [1] going after the
            link [4] in the commit message.</div>
          <div>I think, it generally looks clearer, when they are
            ordered, but that’s a matter of taste.</div>
          <div>Feel free to ignore.</div>
        </div>
      </blockquote>
    </blockquote>
    Rewrote description and fixed order of references.<br>
    <blockquote type="cite"
      cite="mid:1689610201.27637747@f738.i.mail.ru">
      <blockquote style="border-left:1px solid #0857A6; margin:10px;
        padding:0 0 0 10px;">
        <div>
          <blockquote style="border-left:1px solid #0857A6; margin:10px;
            padding:0 0 0 10px;">
            <div>
              <div class="js-helper js-readmsg-msg">
                <div>
                  <div><br>
                    --- test/CMakeLists.txt | 33
                    +++++++++++++++++++++++++++++++++<br>
                     1 file changed, 33 insertions(+)<br>
                    <br>
                    diff --git a/test/CMakeLists.txt
                    b/test/CMakeLists.txt<br>
                    index 47296a22..ccbad035 100644<br>
                    --- a/test/CMakeLists.txt<br>
                    +++ b/test/CMakeLists.txt<br>
                    @@ -42,6 +42,39 @@ else()<br>
                       )<br>
                     endif()<br>
                     <br>
                    +find_program(CHECKPATCH checkpatch.pl<br>
                    + HINTS ${PROJECT_SOURCE_DIR}/checkpatch)<br>
                    +if(CHECKPATCH)</div>
                </div>
              </div>
            </div>
          </blockquote>
          <div>I don’t really like that `MASTER_BRANCH` name is
            hardcoded. I think</div>
          <div>it’s possible to implement it similarly to how it’s done
            in the `tarantool/checkpatch`</div>
          <div>github action[1] with revision range. Or, at least, it is
            for sure possible to obtain</div>
          <div>the master branch name dynamically.</div>
        </div>
      </blockquote>
    </blockquote>
    <p>In LuaJIT we have a single branch for merging new patches -
      tarantool/master.</p>
    <p>Why do you need to redefine master branch?<br>
    </p>
    <blockquote type="cite"
      cite="mid:1689610201.27637747@f738.i.mail.ru">
      <blockquote style="border-left:1px solid #0857A6; margin:10px;
        padding:0 0 0 10px;">
        <div>
          <blockquote style="border-left:1px solid #0857A6; margin:10px;
            padding:0 0 0 10px;">
            <div>
              <div class="js-helper js-readmsg-msg">
                <div>
                  <div>+ set(MASTER_BRANCH "tarantool/master")<br>
                    + add_custom_target(${PROJECT_NAME}-checkpatch)<br>
                    + add_custom_command(TARGET
                    ${PROJECT_NAME}-checkpatch<br>
                    + COMMENT "Running checkpatch"<br>
                    + COMMAND<br>
                    + ${CHECKPATCH}<br>
                    + --codespell<br>
                    + --color=always<br>
                    + --git ${MASTER_BRANCH}..HEAD<br>
                    + --ignore COMMIT_LOG_LONG_LINE<br>
                    + # Requires at least two lines in commit message
                    and this<br>
                    + # is annoying.<br>
                    + --ignore COMMIT_MESSAGE<br>
                    + --ignore NO_CHANGELOG<br>
                    + --ignore NO_DOC<br>
                    + --ignore NO_TEST<br>
                    + --show-types<br>
                    + WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}<br>
                    + )<br>
                    +else()<br>
                    + add_custom_target(${PROJECT_NAME}-checkpatch)</div>
                </div>
              </div>
            </div>
          </blockquote>
          <div>It seems like the target definition can be moved out of
            the `if` statement</div>
          <div>just before it.</div>
        </div>
      </blockquote>
    </blockquote>
    Done. As well as definition of MASTER_BRANCH variable.
    <blockquote type="cite"
      cite="mid:1689610201.27637747@f738.i.mail.ru">
      <blockquote style="border-left:1px solid #0857A6; margin:10px;
        padding:0 0 0 10px;">
        <div>
          <blockquote style="border-left:1px solid #0857A6; margin:10px;
            padding:0 0 0 10px;">
            <div>
              <div class="js-helper js-readmsg-msg">
                <div>
                  <div>+ add_custom_command(TARGET
                    ${PROJECT_NAME}-checkpatch<br>
                    + COMMENT "`checkpatch.pl' is not found, so
                    ${PROJECT_NAME}-checkpatch target is dummy"<br>
                    + )<br>
                    +endif()<br>
                    +<br>
                    +add_custom_target(check<br>
                    + DEPENDS ${PROJECT_NAME}-checkpatch
                    ${PROJECT_NAME}-luacheck<br>
                    +)<br>
                    +</div>
                </div>
              </div>
            </div>
          </blockquote>
          <div>As I have already said offline, I think we should include
            the `check` target as a dependency to the `test` target,
            just like it is currently done for the luacheck. It is much
            more convenient for local testing that way.</div>
        </div>
      </blockquote>
    </blockquote>
    Fixed.<br>
    <blockquote type="cite"
      cite="mid:1689610201.27637747@f738.i.mail.ru">
      <blockquote style="border-left:1px solid #0857A6; margin:10px;
        padding:0 0 0 10px;">
        <div>
          <blockquote style="border-left:1px solid #0857A6; margin:10px;
            padding:0 0 0 10px;">
            <div>
              <div class="js-helper js-readmsg-msg">
                <div>
                  <div> set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY}
                    -e dofile[[${LUAJIT_TEST_INIT}]]")<br>
                     separate_arguments(LUAJIT_TEST_COMMAND)<br>
                     <br>
                    --<br>
                    2.34.1</div>
                </div>
              </div>
            </div>
          </blockquote>
          <div>[1]: <a
href="https://github.com/tarantool/checkpatch/blob/master/.github/actions/checkpatch/action.yml"
              moz-do-not-send="true" class="moz-txt-link-freetext">https://github.com/tarantool/checkpatch/blob/master/.github/actions/checkpatch/action.yml</a></div>
          <div>
            <div>--<br>
              Best regards,</div>
            <div>Maxim Kokryashkin</div>
          </div>
        </div>
      </blockquote>
    </blockquote>
  </body>
</html>