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

2013-01-25 Thread 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.crez...@gmail.com
Reviewed-by: Jeff King p...@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

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-01-25 Thread Junio C Hamano 
Alexey Shumkin alex.crez...@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.crez...@gmail.com
 Reviewed-by: Jeff King p...@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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-01-25 Thread 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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-01-25 Thread Shumkin Alexey 
2013/1/26 Jeff King p...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html