[GitHub] [incubator-nuttx] davids5 commented on a change in pull request #448: Add a sample of git pre-commit hook
davids5 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_r389883624 ## 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. What if a user is doing a style cleanup NOW and not concerned about about licenses in the PR? The documentation of the script tools is poor. There are no examples of what the arguments take as values. 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'? A lot of pieces all over the place is fine jut keep them out the the users face and have reasonable granularity. Yes, we can add more small step like spell/copyright, but it's better that: 1.Implment these check as the bash script or program 2.Invoke them from Makefile/checkpatch.sh/pre-commit 3.check_patch should be the target invoked in normal case, so we can improve the action as need with the furture workflow. Other targets can be used in the special case. > 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 at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-nuttx] davids5 commented on a change in pull request #448: Add a sample of git pre-commit hook
davids5 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_r389883624 ## 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. What if a user is doing a style cleanup NOW and not concerned about about licenses in the PR? The documentation of the script tools is poor. There are no examples of what the arguments take as values. 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'? 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 at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-nuttx] davids5 commented on a change in pull request #448: Add a sample of git pre-commit hook
davids5 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_r389652821 ## 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: >@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. I am not arguing against having the script. I never have. 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. 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_` 3) 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. 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] davids5 commented on a change in pull request #448: Add a sample of git pre-commit hook
davids5 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_r389099219 ## 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 I would add the formatting to make "make check_format " - Hide the tooling from the user. 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
