Re: [dpdk-dev] [PATCH 1/2] devtools: standardize script arguments

["Power, Ciara" <ciara.power@intel.com>]

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