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:

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

quoted

 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

quoted

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

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

quoted

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

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

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

2013/1/26 Jeff King [off-list ref]:

On Fri, Jan 25, 2013 at 06:44:13PM +0400, Alexey Shumkin wrote:

quoted

 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:

quoted

+     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