From: Christian Couder <christian.couder@gmail.com>
Date: Fri, 03 Dec 2010 17:40:28 GMT
Subject: Re: [PATCHv2 6/7] web--browse: use (x-)www-browser if available
Message-ID: <AANLkTimAdrqYezL5jf9OR_J5XdYW-KYhYy9n_VQAUSD8@mail.gmail.com>
In-Reply-To: <1291394861-11989-7-git-send-email-giuseppe.bilotta@gmail.com>

Hi,

On Fri, Dec 3, 2010 at 5:47 PM, Giuseppe Bilotta
<giuseppe.bilotta@gmail.com> wrote:
> Debian and derivatives have an alternatives-based default browser
> configuration that uses the /usr/bin/gnome-www-browser,
> /usr/bin/x-www-browser and /usr/bin/www-browser symlinks.
>
> When no browser is selected by the user and the Debian alternatives are
> available, try to see if they are one of our recognized selection and
> in the affermative case use it. Otherwise, warn the user about them
> being unsupported and move on with the previous detection logic.
>
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  Documentation/git-web--browse.txt |    4 ++
>  git-web--browse.sh                |   59 +++++++++++++++++++++++++++++++++++--
>  2 files changed, 60 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-web--browse.txt b/Documentation/git-web--browse.txt
> index c0416e5..de034a5 100644
> --- a/Documentation/git-web--browse.txt
> +++ b/Documentation/git-web--browse.txt
> @@ -36,6 +36,10 @@ The following browsers (or commands) are currently supported:
>
>  Custom commands may also be specified.
>
> +If no default browser is specified, and /usr/bin/x-www-browser
> +(under X) or /usr/bin/www-browser is present, they are used to determine
> +the browser to use.

It looks like /usr/bin/gnome-www-browser is missing.

> +
>  OPTIONS
>  -------
>  -b <browser>::
> diff --git a/git-web--browse.sh b/git-web--browse.sh
> index d0e99f5..48ea168 100755
> --- a/git-web--browse.sh
> +++ b/git-web--browse.sh
> @@ -49,6 +49,38 @@ init_browser_path() {
>        test -z "$browser_path" && browser_path="$1"
>  }
>
> +# check if a given executable is a browser we like
> +valid_exe() {
> +       testexe="$1"
> +       basename=$(basename $(readlink -f "$testexe"))
> +       if valid_tool "$basename" ; then
> +               browser="$basename"
> +               browser_path="$testexe"
> +               return 0
> +       fi
> +
> +       # if the linked executable doesn't match a browser name,
> +       # look at the version string
> +       verstring="$("$testexe" --version 2> /dev/null)"
> +       browser="$(echo "$verstring" | head -n 1 | cut -f1 -d' ' | tr A-Z a-z)"
> +       case "$browser" in
> +               mozilla)
> +                       browser="$(echo "$verstring" | head -n 1 | cut -f2 -d' ' | tr A-Z a-z)"
> +                       ;;
> +               google)
> +                       browser="google-chrome"
> +                       ;;
> +       esac
> +       if valid_tool "$browser" ; then
> +               browser_path="$i"
> +               return 0
> +       fi
> +
> +       echo >&2 "$basename ($browser) is not a supported browser, skipping"

Why not:

echo >&2 "$basename (from $testexe) is not a supported browser, skipping"

?

Otherwise we might get something like:

"newbrowser (newbrowser) is not a supported browser, skipping"

> +       browser=""
> +       return 1
> +}
> +
>  while test $# != 0
>  do
>        case "$1" in
> @@ -106,6 +138,26 @@ then
>        fi
>  fi
>
> +# Debian and derivatives use gnome-www-browser, x-www-browser or www-browser to
> +# set the default browser for the system. If the user did not specify a tool and
> +# we detect that one of the *www-browser links to a supported one, we pick it.
> +# Otherwise, we warn the user about them being unsupported and proceed to look
> +# for a supported browser.
> +if test -z "$browser" ; then
> +       wwwbrowser="/usr/bin/www-browser"
> +       if test -n "$DISPLAY"; then
> +               wwwbrowser="/usr/bin/x-www-browser $wwwbrowser"
> +               if test -n "$GNOME_DESKTOP_SESSION_ID"; then
> +                       wwwbrowser="/usr/bin/gnome-www-browser $wwwbrowser"
> +               fi
> +       fi
> +       for i in $wwwbrowser; do
> +               if test -x $i && valid_exe $i ; then
> +                       break
> +               fi
> +       done
> +fi
> +
>  if test -z "$browser" ; then
>        if test -n "$DISPLAY"; then
>                browser_candidates="firefox iceweasel google-chrome chrome chromium chromium-browser konqueror opera seamonkey iceape w3m elinks links lynx dillo"
> @@ -133,7 +185,7 @@ if test -z "$browser" ; then
>                fi
>        done
>        test -z "$browser" && die "No known browser available."
> -else
> +else if test -z "$browser_path"; then

Now that you reset $browser above, I am not sure this test -z
"$browser_path" is useful...

Thanks,
Christian.