[v2] checkpatch: add a little script to run checkpatch against a git refspec

This makes it easier to use checkpatch with a git hook or via patches.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
v1 -> v2
 - Add the subject to the output to indicate which patch failed
 - Only display output for patches that fail the check
---
 scripts/check-patches.sh | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100755 scripts/check-patches.sh

Hi,

Thank you for submitting your patch series.  checkpatch.pl has
detected that one or more of the patches in this series violate
the QEMU coding style.

If you believe this message was sent in error, please ignore it
or respond here with an explanation.

Otherwise, please correct the coding style issues and resubmit a
new version of the patch.

For more information about QEMU coding style, see:

http://git.qemu.org/?p=qemu.git;a=blob_plain;f=CODING_STYLE;hb=HEAD

Here is the output from checkpatch.pl:

Subject: checkpatch: add a little script to run checkpatch against a git refspec
ERROR: trailing whitespace
#42: FILE: scripts/check-patches.sh:26:
+    $

total: 1 errors, 0 warnings, 35 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.


Regards,

Anthony Liguori

On 01/21/2013 12:20 PM, Anthony Liguori wrote:
> This makes it easier to use checkpatch with a git hook or via patches.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
> v1 -> v2
>  - Add the subject to the output to indicate which patch failed
>  - Only display output for patches that fail the check
> ---
>  scripts/check-patches.sh | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>  create mode 100755 scripts/check-patches.sh
> 

> +TMPFILE=/tmp/check-patches.out.$$

This name is highly predictable, and thus rather insecure.  Is it worth
using common idioms for safer temporary files?

> +
> +ret=0
> +git log --format="%H %s" "$@" | while read LINE; do
> +    commit="`echo $LINE | cut -f1 -d' '`"
> +    subject="`echo $LINE | cut -f2- -d' '`"
> +    echo "Subject: $subject"

This won't work if $subject contains backslash.  You must use printf(1)
to be portable here.

> +    git show --format=email $commit | scripts/checkpatch.pl - > $TMPFILE
> +    
> +    rc=$?
> +    if test $rc -ne 0; then
> +	ret=rc
> +	cat $TMPFILE
> +	echo
> +    fi
> +done
> +
> +rm -f $TMPFILE
> 

Where do you use $ret?  Also, you are executing the assignment to ret
inside a while loop that was part of a pipeline, but POSIX says a
compliant shell might execute that loop in a subshell (and bash does
just that), so that the parent shell cannot not see the change in the
value of $ret.  If you really must propagate errors outside of the while
loop, then instead of doing:

command | while read; done
use results from loop

you have to instead use an alternative such as:

command | { while read; done
  use results from loop
}

or a named fifo (but that gets back to my question of how do you intend
to generate a secure name for your fifo).

http://mywiki.wooledge.org/BashFAQ/024

Eric Blake <eblake@redhat.com> writes:

> On 01/21/2013 12:20 PM, Anthony Liguori wrote:
>> This makes it easier to use checkpatch with a git hook or via patches.
>> 
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>> ---
>> v1 -> v2
>>  - Add the subject to the output to indicate which patch failed
>>  - Only display output for patches that fail the check
>> ---
>>  scripts/check-patches.sh | 35 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 35 insertions(+)
>>  create mode 100755 scripts/check-patches.sh
>> 
>
>> +TMPFILE=/tmp/check-patches.out.$$
>
> This name is highly predictable, and thus rather insecure.  Is it worth
> using common idioms for safer temporary files?

I don't really consider this to be a problem but since I have to respin
anyway, I'll use mktemp -t.


>> +ret=0
>> +git log --format="%H %s" "$@" | while read LINE; do
>> +    commit="`echo $LINE | cut -f1 -d' '`"
>> +    subject="`echo $LINE | cut -f2- -d' '`"
>> +    echo "Subject: $subject"
>
> This won't work if $subject contains backslash.  You must use printf(1)
> to be portable here.

What won't work, echo or read?  -r should fix the read bit but echo
doesn't interpret newlines by default.... or is that a GNU-ism?

>
>> +    git show --format=email $commit | scripts/checkpatch.pl - > $TMPFILE
>> +    
>> +    rc=$?
>> +    if test $rc -ne 0; then
>> +	ret=rc
>> +	cat $TMPFILE
>> +	echo
>> +    fi
>> +done
>> +
>> +rm -f $TMPFILE
>> 
>
> Where do you use $ret?

That's a bug.

> Also, you are executing the assignment to ret
> inside a while loop that was part of a pipeline, but POSIX says a
> compliant shell might execute that loop in a subshell (and bash does
> just that), so that the parent shell cannot not see the change in the
> value of $ret.  If you really must propagate errors outside of the while
> loop, then instead of doing:
>
> command | while read; done
> use results from loop
>
> you have to instead use an alternative such as:
>
> command | { while read; done
>   use results from loop
> }

Indeed.  I didn't see this because of the above bug.

Regards,

Anthony Liguori

>
> or a named fifo (but that gets back to my question of how do you intend
> to generate a secure name for your fifo).
>
> http://mywiki.wooledge.org/BashFAQ/024
>
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org

On 01/21/2013 02:56 PM, Anthony Liguori wrote:

>>> +ret=0
>>> +git log --format="%H %s" "$@" | while read LINE; do
>>> +    commit="`echo $LINE | cut -f1 -d' '`"
>>> +    subject="`echo $LINE | cut -f2- -d' '`"
>>> +    echo "Subject: $subject"
>>
>> This won't work if $subject contains backslash.  You must use printf(1)
>> to be portable here.
> 
> What won't work, echo or read?

Both.  read without -r might interpret \ before populating $LINE; and if
\ makes it through $LINE and into $subject, then echo on any subject
containing a \ is non-portable.

>  -r should fix the read bit but echo
> doesn't interpret newlines by default.... or is that a GNU-ism?

'read -r' and 'printf' are both POSIX.  The default behavior of bash
leaving \ alone in echo is a violation of POSIX; but you can force bash
to obey POSIX with 'shopt -s xpg_echo'.  Hence, 'printf %s\\n
"$subject"' is always safer than 'echo "$subject"'.