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

Reply via email to