Eric Blake <ebl...@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 <aligu...@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
