Hi Thomas,
Thanks for the review,
>Subject: Re: [dpdk-dev] [PATCH 1/2] devtools: standardize script arguments
>
>Hi,
>
>Thanks for improving tooling.
>
>28/01/2020 16:02, Ciara Power:
>> range=${1:-origin/master..}
>
>If doing a real option management, range should be the remaining argument
>after option parsing.
The goal of this patch is to make the check-git-log and checkpatches scripts
more
consistent, while maintaining backward compatibility.
I think the range value here should remain unchanged, so users can use the
script
as they have been using it before this patch, passing range as $1.
>
>> +if [ "$range" = '--help' ] ; then
>> + print_usage
>
>Missing "exit 0" after usage.
>
>> # convert -N to HEAD~N.. in order to comply with git-log-fixes.sh
>> getopts -if printf -- $range | grep -q '^-[0-9]\+' ; then
>> - range="HEAD$(printf -- $range | sed 's,^-,~,').."
>> +elif printf -- "$range" | grep -q '^-[0-9]\+' ; then
>> + range="HEAD$(printf -- "$range" | sed 's,^-,~,').."
>
>getopts won't be called if $1 starts with -N.
>I think it would be cleaner to handle this in "?" case below.
>
Does the getopts need to be called if $1 is -N?
I am not sure handling this in the "?" case below would work - as far as I
know if the N value is more than one character, it would be treated as
multiple separate characters (e.g. -123 would be treated as -1 2 3)
>> +else
>> + while getopts hr:n: ARG ; do
>> + case $ARG in
>> + n ) range="HEAD~$OPTARG.." ;;
>> + r ) range=$OPTARG ;;
>
>-r is not a git-log option.
>Please handle it without the need for -r.
>
The -r option is added here to standardise the scripts - this is currently being
used in the checkpatches getops.
>> + h ) print_usage ; exit 0 ;;
>> + ? ) print_usage ; exit 1 ;;
>> + esac
>> + done
>> + shift $(($OPTIND - 1))
>
>
Thanks,
Ciara
