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

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

[Chris Packham]

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

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

[Jeff King]

On Sun, Oct 02, 2011 at 01:44:17PM +1300, Chris Packham wrote:

> 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

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

[Jeff King]

On Fri, Nov 11, 2011 at 08:18:03PM +0400, Alexey Shumkin wrote:

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

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

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

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

[Alexey Shumkin]

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

> 
> > +		# 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 )

> 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

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

[Jeff King]

On Fri, Nov 11, 2011 at 11:48:30PM +0400, Alexey Shumkin wrote:

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

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

[Alexey Shumkin]

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

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

[Junio C Hamano]

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

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.

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

[Alexey Shumkin]

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

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

[Alexey Shumkin]

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

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

[Alexey Shumkin]

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

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

[Jeff King]

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.

  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:

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

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

[Shumkin Alexey]

2013/1/26 Jeff King <peff@peff.net>:
> 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
>
> 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