Alexey Shumkin <alex.crezoff@xxxxxxxxx> 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 <Alex.Crezoff@xxxxxxxxx>
> Reviewed-by: Jeff King <peff@xxxxxxxx>
Thanks, both.
> ---
> 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 "$@" &
> 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 &&
> @@ -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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html