# [PATCH] test-lib: simplify '--option=value' parsing 4 messages from 2016-04-22 to 2016-04-22. Participants: SZEDER Gábor, Jeff King, Junio C Hamano. Thread: https://gitlist.dev/t/42110 ## SZEDER Gábor, 2016-04-22 15:38 Subject: [PATCH] test-lib: simplify '--option=value' parsing Message-ID: <1461339503-6854-1-git-send-email-szeder@ira.uka.de> URL: https://gitlist.dev/e/1461339503-6854-1-git-send-email-szeder%40ira.uka.de ``` 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 --- t/test-lib.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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, 2016-04-22 18:37 Subject: Re: [PATCH] test-lib: simplify '--option=value' parsing Message-ID: <20160422183703.GA7595@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160422183703.GA7595%40sigill.intra.peff.net In-Reply-To: <1461339503-6854-1-git-send-email-szeder@ira.uka.de> ``` On Fri, Apr 22, 2016 at 05:38:23PM +0200, SZEDER Gábor wrote: > 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, 2016-04-22 20:32 Subject: [PATCH v2] test-lib: simplify '--option=value' parsing Message-ID: <1461357141-12417-1-git-send-email-szeder@ira.uka.de> URL: https://gitlist.dev/e/1461357141-12417-1-git-send-email-szeder%40ira.uka.de In-Reply-To: <20160422183703.GA7595@sigill.intra.peff.net> ``` 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 POSIX-defined shell parameter expansion instead to do the same. Signed-off-by: SZEDER Gábor --- >> 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(-) 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, 2016-04-22 20:54 Subject: Re: [PATCH] test-lib: simplify '--option=value' parsing Message-ID: URL: https://gitlist.dev/e/xmqq1t5xpdnk.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20160422183703.GA7595@sigill.intra.peff.net> ``` Jeff King 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. ```