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

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

Checks

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

Commit Message

Ciara Power May 6, 2020, 9:55 a.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>
Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>

---
v2: Added exit 0 after print usage.
---
 devtools/check-git-log.sh | 33 ++++++++++++++++++++++++---------
 devtools/checkpatches.sh  |  2 +-
 2 files changed, 25 insertions(+), 10 deletions(-)

Comments

Thomas Monjalon May 24, 2020, 8:57 p.m. UTC | #1
06/05/2020 11:55, Ciara Power:
> 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>"
[...]
> -	usage: $(basename $0) [-h] [range]
> +	usage: $(basename $0) [-h] [-nX|-r range]

Why not specifying that range can be also the first argument?
It is a discrepancy with what is documented in
doc/guides/contributing/patches.rst


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

This line "e.g. -n1" looks disconnected from the above lines.

> +	The default starts from origin/master to HEAD.

The rest looks OK.
Ciara Power May 28, 2020, 2:37 p.m. UTC | #2
Hi Thomas,


>-----Original Message-----
>From: Thomas Monjalon <thomas@monjalon.net>
>Sent: Sunday 24 May 2020 21:58
>To: Power, Ciara <ciara.power@intel.com>
>Cc: dev@dpdk.org
>Subject: Re: [dpdk-dev] [PATCH v2 1/2] devtools: standardize script
>arguments
>
>06/05/2020 11:55, Ciara Power:
>> 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>"
>[...]
>> -	usage: $(basename $0) [-h] [range]
>> +	usage: $(basename $0) [-h] [-nX|-r range]
>
>Why not specifying that range can be also the first argument?
>It is a discrepancy with what is documented in
>doc/guides/contributing/patches.rst
>

I didn't include the old format (that takes range as first argument) here to encourage
using the standardised format moving forward, but anyone that uses the old format
via scripts etc. will not see any difference in use.
I can also update the doc to show this standardised format to avoid discrepancy.

<snip>


Thanks,
Ciara
Thomas Monjalon May 28, 2020, 3:03 p.m. UTC | #3
28/05/2020 16:37, Power, Ciara:
> From: Thomas Monjalon <thomas@monjalon.net>
> >06/05/2020 11:55, Ciara Power:
> >> 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>"
> >[...]
> >> -	usage: $(basename $0) [-h] [range]
> >> +	usage: $(basename $0) [-h] [-nX|-r range]
> >
> >Why not specifying that range can be also the first argument?
> >It is a discrepancy with what is documented in
> >doc/guides/contributing/patches.rst
> >
> 
> I didn't include the old format (that takes range as first argument) here to encourage
> using the standardised format moving forward, but anyone that uses the old format
> via scripts etc. will not see any difference in use.

OK
So maybe just add a comment in the script on the old argument parsing
to mention it is the old syntax.

> I can also update the doc to show this standardised format to avoid discrepancy.

Yes please replace with the new syntax in the doc.

Patch
diff mbox series

diff --git a/devtools/check-git-log.sh b/devtools/check-git-log.sh
index 4e65be0e4b..efdfca27cc 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
+	exit 0
 # 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 42b833e0d7..e111c31d7d 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.