Re: [PATCH] git-web--browser: avoid errors in terminal when running Firefox on Windows

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 25, 2013, 19:49 UTC
Message-ID: <7v1ud93uw8.fsf@alter.siamese.dyndns.org>
In-Reply-To: <3eeabf4989f7f1b4593e89e4c6bcfa8710a7b793.1359125053.git.Alex.Crezoff@gmail.com>

Alexey Shumkin <alex.crezoff@gmail.com> writes:

Show 8 quoted lines

> 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@gmail.com>
> Reviewed-by: Jeff King <peff@peff.net>

Thanks, both.

Show 18 quoted lines

> ---
>  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 "$@" &

Show 24 quoted lines

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

Show 5 quoted lines

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

Show 5 quoted lines

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

Show 5 quoted lines

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