threads / discuss / 28549

[PATCHv3] git-web--browse: avoid the use of eval

Subject: [PATCHv3] git-web--browse: avoid the use of eval

## tl;dr

12 messages between Oct 2, 2011 and Jan 26, 2013.

replies: 11people: 4as markdown or json

Chris Packham· Oct 2, 2011, 00:44 UTC · lore

Using eval causes problems when the URL contains an appropriately escaped ampersand (\&). Dropping eval from the built-in browser invocation avoids the problem.

Helped-by: Jeff King <peff@peff.net> (test case)
Signed-off-by: Chris Packham <judge.packham@gmail.com>

--- The consensus from the last round of discussion [1] seemed to be to remove the eval from the built in browsers but quote custom browser commands appropriately.

I've expanded the tests a little. A semi-colon had the same error as the ampersand. A hash was another common character that had meaning in a shell and in URL.

[1] http://article.gmane.org/gmane.comp.version-control.git/181671
 git-web--browse.sh         |   10 +++++-----
 t/t9901-git-web--browse.sh |   37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 5 deletions(-)
 create mode 100755 t/t9901-git-web--browse.sh
diff --git a/git-web--browse.sh b/git-web--browse.sh
index e9de241..1e82726 100755
--- a/git-web--browse.sh
+++ b/git-web--browse.sh
@@ -156,7 +156,7 @@ firefox|iceweasel|seamonkey|iceape)
 	;;
 google-chrome|chrome|chromium|chromium-browser)
 	# No need to specify newTab. It's default in chromium
-	eval "$browser_path" "$@" &
+	"$browser_path" "$@" &
 	;;
 konqueror)
 	case "$(basename "$browser_path")" in
@@ -164,10 +164,10 @@ konqueror)
 		# It's simpler to use kfmclient to open a new tab in konqueror.
 		browser_path="$(echo "$browser_path" | sed -e 's/konqueror$/kfmclient/')"
 		type "$browser_path" > /dev/null 2>&1 || die "No '$browser_path' found."
-		eval "$browser_path" newTab "$@"
+		"$browser_path" newTab "$@" &
 		;;
 	kfmclient)
-		eval "$browser_path" newTab "$@"
+		"$browser_path" newTab "$@" &
 		;;
 	*)
 		"$browser_path" "$@" &
@@ -175,7 +175,7 @@ konqueror)
 	esac
 	;;
 w3m|elinks|links|lynx|open)
-	eval "$browser_path" "$@"
+	"$browser_path" "$@"
 	;;
 start)
 	exec "$browser_path" '"web-browse"' "$@"
@@ -185,7 +185,7 @@ opera|dillo)
 	;;
 *)
 	if test -n "$browser_cmd"; then
-		( eval $browser_cmd "$@" )
+		( eval "$browser_cmd \"\$@\"" )
 	fi
 	;;
 esac
diff --git a/t/t9901-git-web--browse.sh b/t/t9901-git-web--browse.sh
new file mode 100755
index 0000000..c6f48a9
--- /dev/null
+++ b/t/t9901-git-web--browse.sh
@@ -0,0 +1,37 @@
+#!/bin/sh
+#
+
+test_description='git web--browse basic tests
+
+This test checks that git web--browse can handle various valid URLs.'
+
+. ./test-lib.sh
+
+test_expect_success \
+	'URL with an ampersand in it' '
+	echo http://example.com/foo\&bar >expect &&
+	git config browser.custom.cmd echo &&
+	git web--browse --browser=custom \
+		http://example.com/foo\&bar >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success \
+	'URL with a semi-colon in it' '
+	echo http://example.com/foo\;bar >expect &&
+	git config browser.custom.cmd echo &&
+	git web--browse --browser=custom \
+		http://example.com/foo\;bar >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success \
+	'URL with a hash in it' '
+	echo http://example.com/foo#bar >expect &&
+	git config browser.custom.cmd echo &&
+	git web--browse --browser=custom \
+		http://example.com/foo#bar >actual &&
+	test_cmp expect actual
+'
+
+test_done

-- 
1.7.7
Jeff King· Oct 3, 2011, 09:57 UTC · re: Chris Packham · lore

Re: [PATCHv3] git-web--browse: avoid the use of eval

On Sun, Oct 02, 2011 at 01:44:17PM +1300, Chris Packham wrote:
Show 15 quoted lines
> Using eval causes problems when the URL contains an appropriately
> escaped ampersand (\&). Dropping eval from the built-in browser
> invocation avoids the problem.
> 
> Helped-by: Jeff King <peff@peff.net> (test case)
> Signed-off-by: Chris Packham <judge.packham@gmail.com>
> 
> ---
> The consensus from the last round of discussion [1] seemed to be to
> remove the eval from the built in browsers but quote custom browser
> commands appropriately.
> 
> I've expanded the tests a little. A semi-colon had the same error as
> the ampersand. A hash was another common character that had meaning in
> a shell and in URL.

This looks good to me. I think we may want to squash in the two tests below, too, which make sure we treat $browser_path and $browser_cmd appropriately (the former is a filename, and the latter is a shell snippet).

diff --git a/t/t9901-git-web--browse.sh b/t/t9901-git-web--browse.sh
index c6f48a9..7906e5d 100755
--- a/t/t9901-git-web--browse.sh
+++ b/t/t9901-git-web--browse.sh
@@ -34,4 +34,33 @@ test_expect_success \
 	test_cmp expect actual
 '
 
+test_expect_success \
+	'browser paths are properly quoted' '
+	echo fake: http://example.com/foo >expect &&
+	cat >"fake browser" <<-\EOF &&
+	#!/bin/sh
+	echo fake: "$@"
+	EOF
+	chmod +x "fake browser" &&
+	git config browser.w3m.path "`pwd`/fake browser" &&
+	git web--browse --browser=w3m \
+		http://example.com/foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success \
+	'browser command allows arbitrary shell code' '
+	echo "arg: http://example.com/foo" >expect &&
+	git config browser.custom.cmd "
+		f() {
+			for i in \"\$@\"; do
+				echo arg: \$i
+			done
+		}
+		f" &&
+	git web--browse --browser=custom \
+		http://example.com/foo >actual &&
+	test_cmp expect actual
+'
+
 test_done
Jeff King· Nov 11, 2011, 18:35 UTC · lore

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

On Fri, Nov 11, 2011 at 08:18:03PM +0400, Alexey Shumkin wrote:
Show 5 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.
Thanks. I even noticed this bug early on in the previous discussion:
  http://article.gmane.org/gmane.comp.version-control.git/181600

but forgot about it by the time the final patch rolled around. Your fix looks correct, but:

Show 7 quoted lines
>  test_expect_success \
> +	'Firefox below v2.0 paths are properly quoted' '
> +	echo fake: http://example.com/foo >expect &&
> +	cat >"fake browser" <<-\EOF &&
> +	#!/bin/sh
> +
> +	if [ "$1" == "-version" ]; then
Using "==" is a bashism. Just use "=".

Also, a style nit, but we usually spell this "test" and not "[". I admit I don't care much, though.

Show 10 quoted lines
> +		# Firefox (in contrast to w3m) is run in background (with &)
> +		# so redirect output to "actual"
> +		echo fake: "$@" > actual
> +	fi
> +	EOF
> +	chmod +x "fake browser" &&
> +	git config browser.firefox.path "`pwd`/fake browser" &&
> +	git web--browse --browser=firefox \
> +		http://example.com/foo &&
> +	test_cmp expect actual

Hmm. So we are running the fake browser in the background, but then check that it has written something as soon as web--browse exits. Isn't that a race condition? I.e., we could run "test_cmp" before the browser has actually written anything?

I'm not sure there's a good way to do it. You would need either to wait some pre-determined "it could not possibly take it longer than N seconds to run" sleep, or we need some kind of synchronization point. We can't wait call "wait" on the child PID (if we even have it, because it's not our child).

-Peff
Alexey Shumkin· Nov 11, 2011, 19:48 UTC · re: Jeff King · lore

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

Show 9 quoted lines
> >  test_expect_success \
> > +	'Firefox below v2.0 paths are properly quoted' '
> > +	echo fake: http://example.com/foo >expect &&
> > +	cat >"fake browser" <<-\EOF &&
> > +	#!/bin/sh
> > +
> > +	if [ "$1" == "-version" ]; then
> 
> Using "==" is a bashism. Just use "=".
Thanks (I have no skills enough in this area)
> 
> Also, a style nit, but we usually spell this "test" and not "[". I
> admit I don't care much, though.
Oh, I see
Show 17 quoted lines
> 
> > +		# Firefox (in contrast to w3m) is run in
> > background (with &)
> > +		# so redirect output to "actual"
> > +		echo fake: "$@" > actual
> > +	fi
> > +	EOF
> > +	chmod +x "fake browser" &&
> > +	git config browser.firefox.path "`pwd`/fake browser" &&
> > +	git web--browse --browser=firefox \
> > +		http://example.com/foo &&
> > +	test_cmp expect actual
> 
> Hmm. So we are running the fake browser in the background, but then
> check that it has written something as soon as web--browse exits.
> Isn't that a race condition? I.e., we could run "test_cmp" before the
> browser has actually written anything?

eeehh... you're right... but even on slow Windows Cygwin it is passed )

Show 5 quoted lines
> I'm not sure there's a good way to do it.  You would need either to
> wait some pre-determined "it could not possibly take it longer than N
> seconds to run" sleep, or we need some kind of synchronization point.
> We can't wait call "wait" on the child PID (if we even have it,
> because it's not our child).

hmm... we can delete "actual" file and wait its appearance (with some timeout), no ? but I didn't see in tests anything like this

> -Peff
Jeff King· Nov 11, 2011, 20:26 UTC · re: Alexey Shumkin · lore

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

On Fri, Nov 11, 2011 at 11:48:30PM +0400, Alexey Shumkin wrote:
Show 7 quoted lines
> > I'm not sure there's a good way to do it.  You would need either to
> > wait some pre-determined "it could not possibly take it longer than N
> > seconds to run" sleep, or we need some kind of synchronization point.
> > We can't wait call "wait" on the child PID (if we even have it,
> > because it's not our child).
> hmm... we can delete "actual" file and wait its appearance (with
> some timeout), no ? but I didn't see in tests anything like this

Even that's not foolproof, as the open and write are not atomic (so you could see it's there, but read an empty file). But in this case, we really just care that the thing ran, not that it writes any specific output. So you could probably get away with something like:

  cat >fake-browser <<\EOF &&
  #!/bin/sh
  >fake-browser-ran
  EOF
  git web--browse ... &&
  {
    for timeout in 1 2 3 4 5; do
          test -f fake-browser-ran && break
          sleep 1
    done
    test "$timeout" -ne 5
  }

which would note success as soon as possible (to within a one second margin), but would eventually give up after 5 seconds. So you'd get a false positive on a _very_ loaded system, but that's kind of unlikely.

I dunno. Maybe this hackery is OK, or maybe it just isn't worth it, and we should declare this as something that's too hard to test to make it into our test suite.

-Peff
Alexey Shumkin· Jan 25, 2013, 14:44 UTC · re: Jeff King · lore

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

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>
---
 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
+		)
+	) &&
 	tr -d '\015' <actual >text &&
 	test_cmp expect text
 }
@@ -48,6 +61,48 @@ test_expect_success \
 '
 
 test_expect_success \
+	'Firefox below v2.0 paths are properly quoted' '
+	echo fake: http://example.com/foo >expect &&
+	rm -f fake_browser_ran &&
+	cat >"fake browser" <<-\EOF &&
+	#!/bin/sh
+
+	: > fake_browser_ran
+	if test "$1" = "-version"; then
+		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
+	fi
+	EOF
+	chmod +x "fake browser" &&
+	git config browser.firefox.path "`pwd`/fake browser" &&
+	test_web_browse firefox http://example.com/foo 5
+'
+
+test_expect_success \
+	'Firefox not lower v2.0 paths are properly quoted' '
+	echo fake: -new-tab http://example.com/foo >expect &&
+	rm -f fake_browser_ran &&
+	cat >"fake browser" <<-\EOF &&
+	#!/bin/sh
+
+	: > fake_browser_ran
+	if test "$1" = "-version"; then
+		echo Fake Firefox browser version 2.0.0
+	else
+		# Firefox (in contrast to w3m) is run in background (with &)
+		# so redirect output to "actual"
+		echo fake: "$@" > actual
+	fi
+	EOF
+	chmod +x "fake browser" &&
+	git config browser.firefox.path "`pwd`/fake browser" &&
+	test_web_browse firefox http://example.com/foo 5
+'
+
+test_expect_success \
 	'browser command allows arbitrary shell code' '
 	echo "arg: http://example.com/foo" >expect &&
 	git config browser.custom.cmd "

-- 
1.8.1.1.10.g9255f3f
Junio C Hamano· Jan 25, 2013, 19:49 UTC · re: Alexey Shumkin · lore

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

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.
Alexey Shumkin· Jan 26, 2013, 00:40 UTC · re: Junio C Hamano · lore

[PATCH v2 0/2] git-web--browser: avoid errors in terminal when running

Reroll patch after all suggestions
Alexey Shumkin (2):
  t9901-git-web--browse.sh: Use "write_script" helper
  git-web--browser: avoid errors in terminal when running Firefox on
    Windows
 git-web--browse.sh         |  2 +-
 t/t9901-git-web--browse.sh | 59 ++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 55 insertions(+), 6 deletions(-)
-- 
1.8.1.1.10.g71fa0b7
Alexey Shumkin· Jan 26, 2013, 00:40 UTC · re: Alexey Shumkin · lore

[PATCH v2 1/2] t9901-git-web--browse.sh: Use "write_script" helper

Use "write_script" helper as suggested by Junio C Hamano. Also, replace `pwd` with $(pwd) call convention.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Alexey Shumkin <Alex.Crezoff@gmail.com>
---
 t/t9901-git-web--browse.sh | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/t/t9901-git-web--browse.sh b/t/t9901-git-web--browse.sh
index b0a6bad..b0dabf7 100755
--- a/t/t9901-git-web--browse.sh
+++ b/t/t9901-git-web--browse.sh
@@ -38,12 +38,10 @@ test_expect_success \
 test_expect_success \
 	'browser paths are properly quoted' '
 	echo fake: http://example.com/foo >expect &&
-	cat >"fake browser" <<-\EOF &&
-	#!/bin/sh
+	write_script "fake browser" <<-\EOF &&
 	echo fake: "$@"
 	EOF
-	chmod +x "fake browser" &&
-	git config browser.w3m.path "`pwd`/fake browser" &&
+	git config browser.w3m.path "$(pwd)/fake browser" &&
 	test_web_browse w3m http://example.com/foo
 '
 

-- 
1.8.1.1.10.g71fa0b7
Alexey Shumkin· Jan 26, 2013, 00:40 UTC · re: Alexey Shumkin · lore

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

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>
---
 git-web--browse.sh         |  2 +-
 t/t9901-git-web--browse.sh | 53 +++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 53 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 b0dabf7..c1ee813 100755
--- a/t/t9901-git-web--browse.sh
+++ b/t/t9901-git-web--browse.sh
@@ -8,8 +8,23 @@ 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"
+	rm -f fake_browser_ran &&
 	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
+	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 &&
 	tr -d '\015' <actual >text &&
 	test_cmp expect text
 }
@@ -46,6 +61,42 @@ test_expect_success \
 '
 
 test_expect_success \
+	'Paths are properly quoted for Firefox. Version older then v2.0' '
+	echo "fake: http://example.com/foo" >expect &&
+	write_script "fake browser" <<-\EOF &&
+
+	if test "$1" = "-version"; then
+		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
+	fi
+	: >fake_browser_ran
+	EOF
+	git config browser.firefox.path "$(pwd)/fake browser" &&
+	test_web_browse firefox http://example.com/foo 5
+'
+
+test_expect_success \
+	'Paths are properly quoted for Firefox. Version v2.0 and above' '
+	echo "fake: -new-tab http://example.com/foo" >expect &&
+	write_script "fake browser" <<-\EOF &&
+
+	if test "$1" = "-version"; then
+		echo "Fake Firefox browser version 2.0.0"
+	else
+		# Firefox (in contrast to w3m) is run in background (with &)
+		# so redirect output to "actual"
+		echo "fake: ""$@" >actual
+	fi
+	: >fake_browser_ran
+	EOF
+	git config browser.firefox.path "$(pwd)/fake browser" &&
+	test_web_browse firefox http://example.com/foo 5
+'
+
+test_expect_success \
 	'browser command allows arbitrary shell code' '
 	echo "arg: http://example.com/foo" >expect &&
 	git config browser.custom.cmd "

-- 
1.8.1.1.10.g71fa0b7
Jeff King· Jan 25, 2013, 22:06 UTC · re: Alexey Shumkin · lore

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

On Fri, Jan 25, 2013 at 06:44:13PM +0400, Alexey Shumkin wrote:
Show 18 quoted lines
>  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
> +		)
> +	) &&
>  	tr -d '\015' <actual >text &&

Gross, but I don't really see another way to handle the asynchronous nature of spawning background browsers.

Two things, though:
  1. Should test_web_browse just delete fake_browser_ran for us? Then
     later tests do not have to remember to do so.
  2. Seeing fake_browser_ran appeared, we know that the script has
     started.  But there is still a race condition in which it may not
     have written anything to "actual" yet.
In this implementation:
Show 12 quoted lines
> +	cat >"fake browser" <<-\EOF &&
> +	#!/bin/sh
> +
> +	: > fake_browser_ran
> +	if test "$1" = "-version"; then
> +		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
> +	fi
> +	EOF

There is a period where fake_browser_ran exists, but nothing is in actual. You can solve it by setting fake_browser_ran at the end rather than the beginning.

Or you can drop fake_browser_ran entirely, and just atomically move actual into place, like:

  echo "fake: $*" >actual.tmp
  mv actual.tmp actual
and then test_web_browse can just spin waiting for "actual" to appear.
-Peff
Shumkin Alexey· Jan 25, 2013, 22:52 UTC · re: Jeff King · lore

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

2013/1/26 Jeff King <peff@peff.net>:
Show 29 quoted lines
> On Fri, Jan 25, 2013 at 06:44:13PM +0400, Alexey Shumkin wrote:
>
>>  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
>> +             )
>> +     ) &&
>>       tr -d '\015' <actual >text &&
>
> Gross, but I don't really see another way to handle the asynchronous
> nature of spawning background browsers.
>
> Two things, though:
>
>   1. Should test_web_browse just delete fake_browser_ran for us? Then
>      later tests do not have to remember to do so.
Yep, you're right
>
>   2. Seeing fake_browser_ran appeared, we know that the script has
>      started.  But there is still a race condition in which it may not
>      have written anything to "actual" yet.
Definitely right
Show 28 quoted lines
>
> In this implementation:
>
>> +     cat >"fake browser" <<-\EOF &&
>> +     #!/bin/sh
>> +
>> +     : > fake_browser_ran
>> +     if test "$1" = "-version"; then
>> +             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
>> +     fi
>> +     EOF
>
> There is a period where fake_browser_ran exists, but nothing is in
> actual. You can solve it by setting fake_browser_ran at the end rather
> than the beginning.
>
> Or you can drop fake_browser_ran entirely, and just atomically move
> actual into place, like:
>
>   echo "fake: $*" >actual.tmp
>   mv actual.tmp actual
>
> and then tes-t_web_browse can just spin waiting for "actual" to appear.
Not exactly, because, as I see, "actual" file is a result of redirection of
> git web--browse --browser="$1" "$2" >actual &&
command
>
> -Peff

← back to recent threads