

<HTML><BODY><div>Hi!</div><div>Thanks for the fixes!</div><div>LGTM</div><div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Best regards,</div><div>Maxim Kokryashkin</div></div></div><div> </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_16898768740721986797_BODY"><div class="cl_893451"><p>Hi, Max!</p><p>thanks for your review!</p><p> </p><div class="moz-cite-prefix_mr_css_attr">On 7/17/23 21:48, Maxim Kokryashkin via Tarantool-patches wrote:</div><blockquote type="cite"><div>Hi, Sergey!</div><div>Thanks for the patch!</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_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div id="style_16896011841430095759_BODY_mr_css_attr">From: Sergey Bronnikov <<a>sergeyb@tarantool.org</a>><br><br>Patch enables running checkpatch [1] for checking patch on a pre-commit</div></div></div></div></blockquote><div>The pre-commit stage makes no sense when we are talking about CI — the</div><div>commit has already occurred. That’s just a conventional workflow configuration.</div><div> </div></div></blockquote></blockquote>In my mind "pre-commit" means commit to a target branch, in our case it is a "tarantool/master".<blockquote type="cite"><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div>Having that in mind, along with some grammar related issues. I suggest</div><div>rephrasing it like:</div><div>| Patch adds a CI workflow with the checkpatch[1] run.</div></div></blockquote></blockquote> Updated.<blockquote type="cite"><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_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div>stage.<br><br>1. <a href="https://github.com/tarantool/checkpatch" class="moz-txt-link-freetext_mr_css_attr" target="_blank">https://github.com/tarantool/checkpatch</a><br>---<br> .github/actions/checkpatch/action.yml | 11 +++++++++++<br> .github/workflows/lint.yml | 20 ++++++++++++++++++++<br> 2 files changed, 31 insertions(+)<br> create mode 100644 .github/actions/checkpatch/action.yml<br><br>diff --git a/.github/actions/checkpatch/action.yml b/.github/actions/checkpatch/action.yml<br>new file mode 100644<br>index 00000000..2336fb15<br>--- /dev/null<br>+++ b/.github/actions/checkpatch/action.yml<br>@@ -0,0 +1,11 @@<br>+name: Checkpatch<br>+description: Check patches against LuaJIT development guidelines</div></div></div></div></blockquote><div>I think it’s better to change the action name to `Setup checkpatch`,</div><div>since the actual run is not performed here.</div><div>The description should be updated correspondingly. Also, AFAIK, there</div><div>is no such thing as `LuaJIT guidelines`, so maybe it’s better to change it</div><div>to `Tarantool guidelines` if you want to keep that part.</div></div></blockquote></blockquote> Updated.<p> </p><p><snipped></p><blockquote type="cite"><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div> </div></div></blockquote></blockquote></div></div></div></div></div></blockquote><div> </div></div></blockquote></BODY></HTML>