[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook
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_r390074355 ## 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: > @xiaoxiang781216 Make can run all the pieces. Please do not only think abut this ONLY the way you have to work. Good tools solve everyone problems are self documenting. I already suggest you provide a patch to add check_format(actually check_patch may a better name) in Makefile, what I oppose is to call nxstyle directly in Makefile. It's better to call checkpatch.sh so devloeper can get the same result in his machine as the github CI. > What if a user is doing a style cleanup NOW and not concerned about about licenses in the PR? > Do you monitor the recent comment or PR from @patacongo? https://github.com/apache/incubator-nuttx/pull/414 https://github.com/apache/incubator-nuttx/pull/508/files ... All new source code need use Apache license, but most people forget this and copy BSD license from other files. It's the right time to enforce the license check like coding style. > The documentation of the script tools is poor. There are no examples of what the arguments take as values. Do you think the following help isn't enough? ``` ./tools/checkpatch.sh -h USAGE: ./tools/checkpatch.sh [options] [list|-] Options: -h -c spell check with codespell(install with: pip install codespell -r range check only (used with -p and -g) -p (default) -g -f - read standard input mainly used by git pre-commit hook as below: git diff --cached | ./tools/checkpatch.sh - ``` If not, please enhance the description so everyone can get the benefit. > Forcing a user to decodes the script when it is not necessary is not inviting. Why is it it tools/configure.sh imx-1060evk/nsh not `make imx-1060evk/nsh'? > It's hard to teach people invoke make with argument every time. It's also hard to discover how many targets supported by a specific Makefile. BTW, how can you show something like this from Makefile naturally? ``` ./tools/configure.sh -h USAGE: ./tools/configure.sh [-d] [-s] [-l|m|c|u|g|n] [-a ] : Where: -d enables script debug output -s skip the .config/Make.defs existence check -l selects the Linux (l) host environment. -m selects the macOS (m) host environment. -c selects the Windows host and Cygwin (c) environment. -u selects the Windows host and Ubuntu under Windows 10 (u) environment. -g selects the Windows host and MinGW/MSYS environment. -n selects the Windows host and Windows native (n) environment. Default: Use host setup in the defconfig file Default Windows: Cygwin -a is the path to the apps/ directory, relative to the nuttx directory is the name of the board in the boards directory configs/ is the name of the board configuration sub-directory ``` > A lot of pieces all over the place is fine jut keep them out the the users face and have reasonable granularity. > > > The correct version is always the latest one on the master, we need rebase/cherry-pick our patch with the master anyway, why don't we run checkpatch.sh before sending PR? > > No we do not. I explained this already (in email and comments) that work flow is a waste of time and effort with back and forth chery-picking. You do not have a good solution. 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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook
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_r389686312 ## 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: > 1. Just do not make it the the swiss arm knife. Break this stuff out to files that have names that make sense. (checkpatch should check a patch, or call it checkcontrib [ution] if it does it all things, spellcheck should check spelling...) Do this to reduce coupling. If you have common code include it. Have a separate install script for the hooks. The goal of checkpatch.sh is to ensure the patch is good to merge, so it make sense to check license/spell in checkpatch.sh too. You can implement license/spelling in another script/program just like nxstyle, but user just need run checkpatch.sh to know whether his/her patch is ready for merging. 1.Do you want user invoke check_format/check_license/check_spelling for every PR? 2.Do you want to update all pre-commit/Jenkins/github action scripts if you need add a new precheck? Or all callers(user/pre-commit/jenkin/makefile/github) invoke one script and we just modify this one script to adapter the new workflow requirement. Please remember we may change the precheck frequently before we lockdown the workflow. > 2. Add simple targets to make' - this will simplify the docs and user experiences and add an abstraction. Thy all can run the script. Do as much for the users as you can - this will empower them and make more contributors. > > `make check_format` > `make check_license` > `make check_spelling` > `make check_` It's better to just has one target(check_patch), so the user don't need to invoke make several time to know his/her patch is good. Or let user invoke check_format today, check_format/check_license tomorrow etc. > > 1. Solve the issue running the correct version of the nxstlye without the uses needed to deal with the mess. If you feel submodules are to difficult. wget the file from githup from master, add it to the ignore and build it. We just need this to not be a repeated point of error. It waste peoples time over and over again. The correct vesion is always the latest one on the master, we need rebase/cherry-pick our patch with the master anyway, why don't we run checkpatch.sh before sending PR? 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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook
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_r389686312 ## 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: > 1. Just do not make it the the swiss arm knife. Break this stuff out to files that have names that make sense. (checkpatch should check a patch, or call it checkcontrib [ution] if it does it all things, spellcheck should check spelling...) Do this to reduce coupling. If you have common code include it. Have a separate install script for the hooks. The goal of checkpatch.sh is to ensure the patch is good to merge, so it make sense to check license/spell in checkpatch.sh too. You can implement license/spelling in another script/program just like nxstyle, but user just need run checkpatch.sh to know whether his/her patch is ready for merging. 1.Do you want user invoke check_format/check_license/check_spelling for every PR? 2.Do you want to update all pre-commit/Jenkins/github action scripts if you need add a new precheck? > 2. Add simple targets to make' - this will simplify the docs and user experiences and add an abstraction. Thy all can run the script. Do as much for the users as you can - this will empower them and make more contributors. > > `make check_format` > `make check_license` > `make check_spelling` > `make check_` It's better to just has one target(check_patch), so the user don't need to invoke make several time to know his/her patch is good. > > 1. Solve the issue running the correct version of the nxstlye without the uses needed to deal with the mess. If you feel submodules are to difficult. wget the file from githup from master, add it to the ignore and build it. We just need this to not be a repeated point of error. It waste peoples time over and over again. The correct vesion is always the latest one on the master, we need rebase/cherry-pick our patch with the master anyway, why don't we run checkpatch.sh before sending PR? 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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook
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_r389473361 ## 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: @yamt you can run checkpatch.sh with -r option to ensure your modification don't introduce the new coding style problem. It's better to give your experience here: https://lists.apache.org/thread.html/r6e7b2ed2d5ca1d5b49c5898b591182a8d7475bee2d5de344c026b3f5%40 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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook
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_r389473361 ## 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: @yamt you can run checkpatch.sh with -r option to ensure your modification don't introduce the new coding style problem. 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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook
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, 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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook
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, 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: 1.spell check 2.copyright check 3.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 you can 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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook
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, 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: 1.spell check 2.copyright check 3.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? 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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook
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_r388709203 ## 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: It's fine to add pre-commit in tools folder or generate it from checkpatch.sh, the key point is the pre-commit need to forward the real action to checkpatch.sh because: 1.We will continue to enhance the workflow, especially in the current(initial) phase, it is very important that we can modify the workflow in one place. 2.pre-commit normally need copy to nuttx/.git/hooks and apps/.git/hooks, so it's hard to update it automatically through git pull. And If something in checkpatch.sh isn't good, we can send PR to enhance it, then all caller get the update autoamtically. 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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook
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_r388699113 ## 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: BTW, I am considering that is it better that we add one option in checkpatch.sh to generate pre-commit and put it into .git/hook? 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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook
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_r388698278 ## 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: But it is helping newbie to setup environment, it's hard to generate nxstyle from command line manually. Anyway, we can disscuss whether checkpatch.sh should or not invoke make automatically, but it's better to call checkpatch.sh here to ensure the consistent result with precheck process. 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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook
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_r388691596 ## 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: Call tools/checkpatch.sh intead, the benefit include: 1.Generate nxstyle automatically 2.Get more precheck in the furture a.copyright check b.spell check 3.The same result as github action(it call checkpatch.sh too) 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
