zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.49k stars 6.42k forks source link

checkpatch: not expected behavior for multiple git commit check. #14927

Closed arnopo closed 4 years ago

arnopo commented 5 years ago

The checkpatch.pl script should allows to check multi commits by defining a range of git commit. For instance following command should review the 4 last commits: ./scripts/checkpatch.pl --strict --git HEAD-4

But the result of this command is the check of the 4th commit from the head not the 4 last commits of the branch.

To Reproduce ./scripts/checkpatch.pl --strict --git HEAD-4

Expected behavior review the last 4 commits in git branch

Impact minor

Environment (please complete the following information):

Additional context same test with other syntax: ./scripts/checkpatch.pl --strict --git HEAD... works ./scripts/checkpatch.pl --strict --git HEAD.. doesn't work

baw-serafin commented 5 years ago

Yes this is anoying, I have a little script on our jenkins maybee one can use this a base fo a solutions

#This only works if Branch is fast-foward mergable
target_rev=`git show-ref --hash $gitlabTargetBranch | head -n1`
revisions=`git log --no-color --no-merges --pretty=format:'%H'  HEAD...$target_rev`
fail=0

for rev in $revisions
do
echo "****************************************************"
echo `git log -log --no-color --format='%H %s' -1 $rev`
echo "****************************************************"
scripts/checkpatch.pl --no-signoff --strict -g $rev | tee .output.txt || echo 0
! grep "[1-9][0-9]* errors" .output.txt
if [ $? != 0 ]
then fail=1
fi
done
exit $fail
carlescufi commented 5 years ago

I don't think this is a Zephyr bug though, we use git diff to invoke checkpatch so I think this might need to be reported to the upstream checkpatch maintainers:

https://github.com/zephyrproject-rtos/ci-tools/blob/master/scripts/check_compliance.py#L191

arnopo commented 5 years ago

I performed news tests on my side. For this I compared the script with the Linux kernel one.

In fact the parse of the multi git commit is working well. The issue is that the script exists on first patch checked without error or warning because the $mailback variable is set to 1 ( set to 0 for Linux kernel). This result in an exit of the script if no issue found for a commit, without parsing the rest of the patch list.

I can not figure out where this variable is forced to 1. is it in the compliance file? https://github.com/zephyrproject-rtos/ci-tools/blob/2538b53a18dc700af2af692eecc0de90206fa274/scripts/check_compliance.py#L194

I can not find this py script in my environment...

baw-serafin commented 5 years ago

The issue is that the script exists on first patch checked without error or warning because the $mailback variable is set to 1 ( set to 0 for Linux kernel). This result in an exit of the script if no issue found for a commit, without parsing the rest of the patch list.

That's sounds like a Bug to me;) Or the variable is very strangely named. None the less good work. Maybe we can get it working without upstream fixes than.

galak commented 5 years ago

Does this work with the upstream version of checkpatch?

arnopo commented 5 years ago

@galak not sure to get the upstream project but this variable exist in Linux kernel.

The behavior is related to the .checkpatch.conf which defines the option --mailback. This option has been introduced in the initial revision of the file (commit 9020c57934d52aeea9566322e20bd5fd56bf1d0b)

By cleaning the related line in .checkpatch.conf file the issue is not reproduced... Any idea about the reason of the use of this option, for a report or something like this? Could we clean it?

nashif commented 4 years ago

--mailback is set in .checkpatch.conf

arnopo commented 4 years ago

Yes this is the point. The fact that this variable is set in .checkpatch.conf does not allow to check patch series directly in git tree. To understand the delta you can test following command with and without ---mailback defined: ./scripts/checkpatch.pl --strict -g HEAD-4

One question is: Is there a rational to keep this option in .checkpatch.conf file?

nashif commented 4 years ago

One question is: Is there a rational to keep this option in .checkpatch.conf file?

dont remember the history, but AFAIK this was added to support execution from a hook.

erwango commented 4 years ago

@nashif, @arnopo :

dont remember the history, but AFAIK this was added to support execution from a hook.

Best way to know if the hook is --mailback is used is to remove it and wait..

arnopo commented 4 years ago

i will send a PR to propose the fix, associated to this issue, so you can decide if you merge it or not.