[1/2] devtools: standardize script arguments
diff mbox series

Message ID 20200128150256.14339-2-ciara.power@intel.com
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show
Series
  • standardize devtools check scripts
Related show

Checks

Context Check Description
ci/Intel-compilation fail Compilation issues
ci/iol-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-nxp-Performance success Performance Testing PASS
ci/checkpatch success coding style OK

Commit Message

Power, Ciara Jan. 28, 2020, 3:02 p.m. UTC
This patch modifies the arguments expected by the check-git-log script,
to match the format of arguments for the checkpatches script. Both
scripts now take certain argument options in the same format, making
them easier to use.
e.g. Both now take a commit ID range by "-r <range>"

The checkpatches help print is also updated to include the "-h" option.

Signed-off-by: Ciara Power <ciara.power@intel.com>
---
 devtools/check-git-log.sh | 31 +++++++++++++++++++++++--------
 devtools/checkpatches.sh  |  2 +-
 2 files changed, 24 insertions(+), 9 deletions(-)

Comments

Ferruh Yigit Jan. 28, 2020, 3:40 p.m. UTC | #1
On 1/28/2020 3:02 PM, Ciara Power wrote:
> This patch modifies the arguments expected by the check-git-log script,
> to match the format of arguments for the checkpatches script. Both
> scripts now take certain argument options in the same format, making
> them easier to use.
> e.g. Both now take a commit ID range by "-r <range>"
> 
> The checkpatches help print is also updated to include the "-h" option.
> 
> Signed-off-by: Ciara Power <ciara.power@intel.com>

Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>
Thomas Monjalon Feb. 22, 2020, 8:53 p.m. UTC | #2
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.

> +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.

> +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.

> +			h ) print_usage ; exit 0 ;;
> +			? ) print_usage ; exit 1 ;;
> +		esac
> +	done
> +	shift $(($OPTIND - 1))
Power, Ciara March 31, 2020, 1:11 p.m. UTC | #3
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

Patch
diff mbox series

diff --git a/devtools/check-git-log.sh b/devtools/check-git-log.sh
index f9d055039..22b2a6d84 100755
--- a/devtools/check-git-log.sh
+++ b/devtools/check-git-log.sh
@@ -7,23 +7,38 @@ 
 # If any doubt about the formatting, please check in the most recent history:
 #	git log --format='%>|(15)%cr   %s' --reverse | grep -i <pattern>
 
-if [ "$1" = '-h' -o "$1" = '--help' ] ; then
+print_usage () {
 	cat <<- END_OF_HELP
-	usage: $(basename $0) [-h] [range]
+	usage: $(basename $0) [-h] [-nX|-r range]
 
 	Check commit log formatting.
-	The git range can be specified as a "git log" option,
-	e.g. -1 to check only the latest commit.
-	The default range starts from origin/master to HEAD.
+	The git commits to be checked can be specified as a "git log" option,
+	by latest git commits limited with -n option, or commits in the git
+	range specified with -r option.
+	e.g. -n1 to check only the latest commit.
+	The default starts from origin/master to HEAD.
 	END_OF_HELP
 	exit
-fi
+}
 
 selfdir=$(dirname $(readlink -f $0))
 range=${1:-origin/master..}
+
+if [ "$range" = '--help' ] ; then
+	print_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,^-,~,').."
+else
+	while getopts hr:n: ARG ; do
+		case $ARG in
+			n ) range="HEAD~$OPTARG.." ;;
+			r ) range=$OPTARG ;;
+			h ) print_usage ; exit 0 ;;
+			? ) print_usage ; exit 1 ;;
+		esac
+	done
+	shift $(($OPTIND - 1))
 fi
 
 commits=$(git log --format='%h' --reverse $range)
diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
index b16bace92..084191984 100755
--- a/devtools/checkpatches.sh
+++ b/devtools/checkpatches.sh
@@ -38,7 +38,7 @@  options="$options $DPDK_CHECKPATCH_OPTIONS"
 
 print_usage () {
 	cat <<- END_OF_HELP
-	usage: $(basename $0) [-q] [-v] [-nX|-r range|patch1 [patch2] ...]]
+	usage: $(basename $0) [-h] [-q] [-v] [-nX|-r range|patch1 [patch2] ...]
 
 	Run Linux kernel checkpatch.pl with DPDK options.
 	The environment variable DPDK_CHECKPATCH_PATH must be set.