xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook URL: https://github.com/apache/incubator-nuttx/pull/448#discussion_r389369776
########## File path: tools/git-hooks/pre-commit ########## @@ -0,0 +1,30 @@ +#!/bin/sh + +############################################################################ +# tools/git-hooks/pre-commit +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. The +# ASF licenses this file to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance with the +# License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. +# +############################################################################ + +# This file is intended to be used as .git/hooks/pre-commit + +if ! type nxstyle > /dev/null 2>&1; then Review comment: > > i'd like to suggest checkpatch.sh minimum. do not install tools, or set up git hooks. > > instead, introduce a separate script to do those "heavy" jobs. > > how do you think? > > +1 - have a separate script for assing the hook This patch looks good, except it need to invoke checkpatch.sh to do the real work, actually we has the similar pre-commit in our local git: ``` #!/usr/bin/env bash # tools/pre-commit # git hook to run check-patch on the output and stop any commits # that do not pass. Note, only for git-commit, and not for any of the # other scenarios # # Copyright 2010 Ben Dooks, <ben-li...@fluff.org> if git rev-parse --verify HEAD 2>/dev/null >/dev/null then against=HEAD else # Initial commit: diff against an empty tree object against=4b825dc642cb6eb9a060e54bf8d69288fbee4904 fi git diff --cached $against -- | ./tools/checkpatch.sh - ``` Why we don't upstream our pre-commit and suggest to generate it dynamiclly instead? Because pre-commit is a very special script, it must put into .git/hooks/ to work as expect, the copy in nuttx/tools can't work standalone but give user the bad expression. If you look at Linux kernel codebase, you can find checkpatch.pl but no pre-commit, it may one of reason. But anyway, as I said before ,it isn't a big issue. > I would add the formatting to make "make check_format " - Hide the tooling from the user. @davids5 as I said before there isn't real difference between checkpatch.sh and make check_format, and it's very easy to implement check_format on top of checkpatch.sh and the patch is welcome. We talk before the reason to select checkpatch.sh as the base and let pre-commit/check_format to inovke checkpatch.sh, let me reemphasis again here: 1.The bash script can be invoke easily from most environment(jenkins, travis, github action, git hook...). 2.The coding style isn't the only check we need to do, we need do more: a.spell check b.copyright check c.defconfig check If we call nxstyle directly from pre-commit/check_format/checkpatch/..., could you tell me how can we improve(most likely) the precheck flow in the furture? 3.Since we can pass the argument to a bash script, it's very easy to let checkpatch.sh support many different usecase(patch file, source file, commit id, or even stdin). Could you tell me how can you do this in Makefile? 4.Since nxstyle isn't perfect, I have saw many discussion(at least three times) in email list to replace nxstyle with clang-format, uncrustify and more. We can adapter this change quickly if all place invoke checkpath.sh instead of nxstyle. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
