Re: [PATCH] git-web--browser: avoid errors in terminal when running Firefox on Windows
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:55:54
Possibly related (same subject, not in this thread)
- 2016-06-15 · Re: [PATCH] git-web--browser: avoid errors in terminal when running Firefox on Windows · Jeff King <hidden>
- 2016-06-15 · Re: [PATCH] git-web--browser: avoid errors in terminal when running Firefox on Windows · Shumkin Alexey <hidden>
- 2016-06-15 · [PATCH] git-web--browser: avoid errors in terminal when running Firefox on Windows · Alexey Shumkin <hidden>
- 2016-06-15 · Re: [PATCH] git-web--browser: avoid errors in terminal when running Firefox on Windows · Jeff King <hidden>
- 2016-06-15 · Re: [PATCH] git-web--browser: avoid errors in terminal when running Firefox on Windows · Alexey Shumkin <hidden>
Alexey Shumkin [off-list ref] writes:
Firefox on Windows by default is placed in "C:\Program Files\Mozilla Firefox" folder, i.e. its path contains spaces. Before running this browser "git-web--browse" tests version of Firefox to decide whether to use "-new-tab" option or not. Quote browser path to avoid error during this test. Signed-off-by: Alexey Shumkin <redacted> Reviewed-by: Jeff King <redacted>
Thanks, both.
quoted hunk
--- git-web--browse.sh | 2 +- t/t9901-git-web--browse.sh | 57 +++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 57 insertions(+), 2 deletions(-)diff --git a/git-web--browse.sh b/git-web--browse.sh index 1e82726..f96e5bd 100755 --- a/git-web--browse.sh +++ b/git-web--browse.sh@@ -149,7 +149,7 @@ fi case "$browser" in firefox|iceweasel|seamonkey|iceape) # Check version because firefox < 2.0 does not support "-new-tab". - vers=$(expr "$($browser_path -version)" : '.* \([0-9][0-9]*\)\..*') + vers=$(expr "$("$browser_path" -version)" : '.* \([0-9][0-9]*\)\..*') NEWTAB='-new-tab' test "$vers" -lt 2 && NEWTAB='' "$browser_path" $NEWTAB "$@" &
quoted hunk
diff --git a/t/t9901-git-web--browse.sh b/t/t9901-git-web--browse.sh index b0a6bad..30d5294 100755 --- a/t/t9901-git-web--browse.sh +++ b/t/t9901-git-web--browse.sh@@ -8,8 +8,21 @@ This test checks that git web--browse can handle various valid URLs.' . ./test-lib.sh test_web_browse () { - # browser=$1 url=$2 + # browser=$1 url=$2 sleep_timeout=$3 + sleep_timeout="$3" git web--browse --browser="$1" "$2" >actual && + # if $3 is set + # as far as Firefox is run in background (it is run with &) + # we trying to avoid race condition + # by waiting for "$sleep_timeout" seconds of timeout for 'fake_browser_ran' file appearance + (test -z "$sleep_timeout" || ( + for timeout in $(seq 1 $sleep_timeout); do + test -f fake_browser_ran && break + sleep 1 + done + test $timeout -ne $sleep_timeout + ) + ) &&
Style: - do/then/else begin a new line (a good rule of thumb is remember this rule is to write control structures without using semicolon). - do not use "seq"; it is not available in some places. I do not think of a reason why you want ( nested (subshell) ), but if you don't need them, perhaps I'd write the above this way: if test -n $sleep_timeout then for timeout in $(test_seq $sleep_timeout) do test -f fake_browser_ran && break sleep 1 done test $timeout -ne $sleep_timeout fi &&
quoted hunk
@@ -48,6 +61,48 @@ test_expect_success \ ' test_expect_success \ + 'Firefox below v2.0 paths are properly quoted' '
-ECANNOTPARSE. "Paths to firefox older than v2.0 are properly quoted" you mean, perhaps? I dunno.
+ echo fake: http://example.com/foo >expect && + rm -f fake_browser_ran && + cat >"fake browser" <<-\EOF && + #!/bin/sh
Consider using "write_script" helper so that you get the path to the shell the user specified via $SHELL_PATH.
+ + : > fake_browser_ran
Style: no SP between redirection operator and filename, i.e. : >fake_browser_ran
+ if test "$1" = "-version"; then
Style (see above).
+ echo Fake Firefox browser version 1.2.3 + else + # Firefox (in contrast to w3m) is run in background (with &) + # so redirect output to "actual" + echo fake: "$@" > actual
Style (see above).
+ fi + EOF + chmod +x "fake browser" && + git config browser.firefox.path "`pwd`/fake browser" &&
We tend to prefer $(pwd) over `pwd`.
+ test_web_browse firefox http://example.com/foo 5 +' + +test_expect_success \ + 'Firefox not lower v2.0 paths are properly quoted' '
s/not lower v2.0/v2.0 and above/, but again -ECANNOTPARSE.
+ echo fake: -new-tab http://example.com/foo >expect &&
I'd feel safer if you quoted the arguments to "echo", i.e. echo "fake: -new-tab http://example.com/foo" >expect && The same style comments as above apply to the remainder of patch. Thanks.