threads / patch / 42110

patchtest-lib: simplify '--option=value' parsing

Subject: [PATCH] test-lib: simplify '--option=value' parsing

## tl;dr

4 messages between Apr 22, 2016 and Apr 22, 2016. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

SZEDER Gábor· Apr 22, 2016, 15:38 UTC · lore

To get the 'value' from '--option=value', test-lib.sh parses said option running 'expr' with a regexp. This involves a subshell, an external process, and a lot of non-alphanumeric characters in the regexp.

Use a much simpler shell parameter expansion instead to do the same.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 t/test-lib.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Show changes to t/test-lib.sh +4 −4

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 0b47eb6bb299..8373d5b5c900 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -208,7 +208,7 @@ do
 	-v|--v|--ve|--ver|--verb|--verbo|--verbos|--verbose)
 		verbose=t; shift ;;
 	--verbose-only=*)
-		verbose_only=$(expr "z$1" : 'z[^=]*=\(.*\)')
+		verbose_only=${1#--*=}
 		shift ;;
 	-q|--q|--qu|--qui|--quie|--quiet)
 		# Ignore --quiet under a TAP::Harness. Saying how many tests
@@ -222,15 +222,15 @@ do
 		valgrind=memcheck
 		shift ;;
 	--valgrind=*)
-		valgrind=$(expr "z$1" : 'z[^=]*=\(.*\)')
+		valgrind=${1#--*=}
 		shift ;;
 	--valgrind-only=*)
-		valgrind_only=$(expr "z$1" : 'z[^=]*=\(.*\)')
+		valgrind_only=${1#--*=}
 		shift ;;
 	--tee)
 		shift ;; # was handled already
 	--root=*)
-		root=$(expr "z$1" : 'z[^=]*=\(.*\)')
+		root=${1#--*=}
 		shift ;;
 	--chain-lint)
 		GIT_TEST_CHAIN_LINT=1

-- 
2.8.1.99.g5d5236f

Jeff King· Apr 22, 2016, 18:37 UTC · re: SZEDER Gábor · lore

Re: [PATCH] test-lib: simplify '--option=value' parsing

On Fri, Apr 22, 2016 at 05:38:23PM +0200, SZEDER Gábor wrote:

Show 6 quoted lines

> To get the 'value' from '--option=value', test-lib.sh parses said
> option running 'expr' with a regexp.  This involves a subshell, an
> external process, and a lot of non-alphanumeric characters in the
> regexp.
> 
> Use a much simpler shell parameter expansion instead to do the same.

Looks OK to me. I doubt the extra process is a killer (we only parse options once per script), but the result is definitely more readable, IMHO.

For some reason I had always assumed that complicated pattern matching with "#" was non-POSIX, but I checked and it is definitely in there.

> ---
>  t/test-lib.sh | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

I count 5 cases in my copy of test-lib.sh. I think you are missing "--run".

-Peff

SZEDER Gábor· Apr 22, 2016, 20:32 UTC · re: Jeff King · lore

[PATCH v2] test-lib: simplify '--option=value' parsing

Use a much simpler POSIX-defined shell parameter expansion instead to do the same.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---

>>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> I count 5 cases in my copy of test-lib.sh. I think you are missing
> "--run".

Oh, indeed, '--run' managed to hide between these -l|--l|--lo|--lon|--long patterns.

 t/test-lib.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Show changes to t/test-lib.sh +5 −5

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 0b47eb6bb299..79afa8748eec 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -202,13 +202,13 @@ do
 		}
 		run_list=$1; shift ;;
 	--run=*)
-		run_list=$(expr "z$1" : 'z[^=]*=\(.*\)'); shift ;;
+		run_list=${1#--*=}; shift ;;
 	-h|--h|--he|--hel|--help)
 		help=t; shift ;;
 	-v|--v|--ve|--ver|--verb|--verbo|--verbos|--verbose)
 		verbose=t; shift ;;
 	--verbose-only=*)
-		verbose_only=$(expr "z$1" : 'z[^=]*=\(.*\)')
+		verbose_only=${1#--*=}
 		shift ;;
 	-q|--q|--qu|--qui|--quie|--quiet)
 		# Ignore --quiet under a TAP::Harness. Saying how many tests
@@ -222,15 +222,15 @@ do
 		valgrind=memcheck
 		shift ;;
 	--valgrind=*)
-		valgrind=$(expr "z$1" : 'z[^=]*=\(.*\)')
+		valgrind=${1#--*=}
 		shift ;;
 	--valgrind-only=*)
-		valgrind_only=$(expr "z$1" : 'z[^=]*=\(.*\)')
+		valgrind_only=${1#--*=}
 		shift ;;
 	--tee)
 		shift ;; # was handled already
 	--root=*)
-		root=$(expr "z$1" : 'z[^=]*=\(.*\)')
+		root=${1#--*=}
 		shift ;;
 	--chain-lint)
 		GIT_TEST_CHAIN_LINT=1

-- 
2.8.1.99.g5d5236f

Junio C Hamano· Apr 22, 2016, 20:54 UTC · re: Jeff King · lore

Re: [PATCH] test-lib: simplify '--option=value' parsing

Jeff King <peff@peff.net> writes:

> For some reason I had always assumed that complicated pattern matching
> with "#" was non-POSIX, but I checked and it is definitely in there.

No, the offending code really dates back to the days back when I was writing most of the shell scripts (or others were mostly copying from what I have written). I had this irrational aversion against variable substitutions that are beyond ${var-default}, ${var=assign} and ${var+isset}.

The conversion looks fine (modulo one missed one you noticed).

Thanks for cleaning things up.