[PATCH 0/6] web--browse cleanups and extensions

[PATCH 0/6] web--browse cleanups and extensions

[Giuseppe Bilotta]

This patches introduces support for Opera, Seamonkey/Iceape and elinks
to git web-browse, after a little reformatting to adhere to the shell
script coding guidelines (guidelines which were not explicit in the
documentation, and which are made so).

The last two patches are somewhat more system-specific (AFAIK, only
Debain & friends have /usr/bin/{x-,}www-browser, and not many other
distributions have chromium under the chromium-browser executable name),
but I believe it still makes sense to include them in the mainstream
version of git.

Giuseppe Bilotta (6):
  CodingGuidelines: mention whitespace preferences for shell scripts
  web--browse: coding style
  web--browse: split valid_tool list
  web--browse: support opera, seamonkey and elinks
  web--browse: use (x-)www-browser if available
  web--browse: special-case chromium path

 Documentation/CodingGuidelines    |    6 +
 Documentation/git-web--browse.txt |   10 ++
 git-web--browse.sh                |  202 +++++++++++++++++++++----------------
 3 files changed, 133 insertions(+), 85 deletions(-)

-- 
1.7.3.2.624.gec6c7.dirty

[PATCH 1/6] CodingGuidelines: mention whitespace preferences for shell scripts

[Giuseppe Bilotta]

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 Documentation/CodingGuidelines |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 5aa2d34..7ecce51 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -31,6 +31,12 @@ But if you must have a list of rules, here they are.
 
 For shell scripts specifically (not exhaustive):
 
+ - We use tabs for indentation.
+
+ - The choices in case ... esac statement are not indented with respect
+   to the the case and esac lines. The body of the choices however _is_
+   indented (by one tab).
+
  - We prefer $( ... ) for command substitution; unlike ``, it
    properly nests.  It should have been the way Bourne spelled
    it from day one, but unfortunately isn't.
-- 
1.7.3.2.624.gec6c7.dirty

[PATCH 2/6] web--browse: coding style

[Giuseppe Bilotta]

Retab and deindent choices in case statements.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 git-web--browse.sh |  166 ++++++++++++++++++++++++++--------------------------
 1 files changed, 83 insertions(+), 83 deletions(-)

diff --git a/git-web--browse.sh b/git-web--browse.sh
index 3fc4166..7c4568f 100755
--- a/git-web--browse.sh
+++ b/git-web--browse.sh
@@ -31,11 +31,11 @@ valid_custom_tool()
 
 valid_tool() {
 	case "$1" in
-		firefox | iceweasel | chrome | google-chrome | chromium | konqueror | w3m | links | lynx | dillo | open | start)
-			;; # happy
-		*)
-			valid_custom_tool "$1" || return 1
-			;;
+	firefox | iceweasel | chrome | google-chrome | chromium | konqueror | w3m | links | lynx | dillo | open | start)
+		;; # happy
+	*)
+		valid_custom_tool "$1" || return 1
+		;;
 	esac
 }
 
@@ -46,139 +46,139 @@ init_browser_path() {
 
 while test $# != 0
 do
-    case "$1" in
+	case "$1" in
 	-b|--browser*|-t|--tool*)
-	    case "$#,$1" in
+		case "$#,$1" in
 		*,*=*)
-		    browser=`expr "z$1" : 'z-[^=]*=\(.*\)'`
-		    ;;
+			browser=`expr "z$1" : 'z-[^=]*=\(.*\)'`
+			;;
 		1,*)
-		    usage ;;
+			usage ;;
 		*)
-		    browser="$2"
-		    shift ;;
-	    esac
-	    ;;
+			browser="$2"
+			shift ;;
+		esac
+		;;
 	-c|--config*)
-	    case "$#,$1" in
+		case "$#,$1" in
 		*,*=*)
-		    conf=`expr "z$1" : 'z-[^=]*=\(.*\)'`
-		    ;;
+			conf=`expr "z$1" : 'z-[^=]*=\(.*\)'`
+			;;
 		1,*)
-		    usage ;;
+			usage ;;
 		*)
-		    conf="$2"
-		    shift ;;
-	    esac
-	    ;;
+			conf="$2"
+			shift ;;
+		esac
+		;;
 	--)
-	    break
-	    ;;
+		break
+		;;
 	-*)
-	    usage
-	    ;;
+		usage
+		;;
 	*)
-	    break
-	    ;;
-    esac
-    shift
+		break
+		;;
+	esac
+	shift
 done
 
 test $# = 0 && usage
 
 if test -z "$browser"
 then
-    for opt in "$conf" "web.browser"
-    do
-	test -z "$opt" && continue
-	browser="`git config $opt`"
-	test -z "$browser" || break
-    done
-    if test -n "$browser" && ! valid_tool "$browser"; then
-	echo >&2 "git config option $opt set to unknown browser: $browser"
-	echo >&2 "Resetting to default..."
-	unset browser
-    fi
+	for opt in "$conf" "web.browser"
+	do
+		test -z "$opt" && continue
+		browser="`git config $opt`"
+		test -z "$browser" || break
+	done
+	if test -n "$browser" && ! valid_tool "$browser"; then
+		echo >&2 "git config option $opt set to unknown browser: $browser"
+		echo >&2 "Resetting to default..."
+		unset browser
+	fi
 fi
 
 if test -z "$browser" ; then
-    if test -n "$DISPLAY"; then
-	browser_candidates="firefox iceweasel google-chrome chrome chromium konqueror w3m links lynx dillo"
-	if test "$KDE_FULL_SESSION" = "true"; then
-	    browser_candidates="konqueror $browser_candidates"
+	if test -n "$DISPLAY"; then
+		browser_candidates="firefox iceweasel google-chrome chrome chromium konqueror w3m links lynx dillo"
+		if test "$KDE_FULL_SESSION" = "true"; then
+			browser_candidates="konqueror $browser_candidates"
+		fi
+	else
+		browser_candidates="w3m links lynx"
 	fi
-    else
-	browser_candidates="w3m links lynx"
-    fi
-    # SECURITYSESSIONID indicates an OS X GUI login session
-    if test -n "$SECURITYSESSIONID" \
-	    -o "$TERM_PROGRAM" = "Apple_Terminal" ; then
-	browser_candidates="open $browser_candidates"
-    fi
-    # /bin/start indicates MinGW
-    if test -x /bin/start; then
-	browser_candidates="start $browser_candidates"
-    fi
-
-    for i in $browser_candidates; do
-	init_browser_path $i
-	if type "$browser_path" > /dev/null 2>&1; then
-	    browser=$i
-	    break
+	# SECURITYSESSIONID indicates an OS X GUI login session
+	if test -n "$SECURITYSESSIONID" \
+		-o "$TERM_PROGRAM" = "Apple_Terminal" ; then
+		browser_candidates="open $browser_candidates"
 	fi
-    done
-    test -z "$browser" && die "No known browser available."
+	# /bin/start indicates MinGW
+	if test -x /bin/start; then
+		browser_candidates="start $browser_candidates"
+	fi
+
+	for i in $browser_candidates; do
+		init_browser_path $i
+		if type "$browser_path" > /dev/null 2>&1; then
+			browser=$i
+			break
+		fi
+	done
+	test -z "$browser" && die "No known browser available."
 else
-    valid_tool "$browser" || die "Unknown browser '$browser'."
+	valid_tool "$browser" || die "Unknown browser '$browser'."
 
-    init_browser_path "$browser"
+	init_browser_path "$browser"
 
-    if test -z "$browser_cmd" && ! type "$browser_path" > /dev/null 2>&1; then
-	die "The browser $browser is not available as '$browser_path'."
-    fi
+	if test -z "$browser_cmd" && ! type "$browser_path" > /dev/null 2>&1; then
+		die "The browser $browser is not available as '$browser_path'."
+	fi
 fi
 
 case "$browser" in
-    firefox|iceweasel)
+firefox|iceweasel)
 	# Check version because firefox < 2.0 does not support "-new-tab".
 	vers=$(expr "$($browser_path -version)" : '.* \([0-9][0-9]*\)\..*')
 	NEWTAB='-new-tab'
 	test "$vers" -lt 2 && NEWTAB=''
 	"$browser_path" $NEWTAB "$@" &
 	;;
-    google-chrome|chrome|chromium)
+google-chrome|chrome|chromium)
 	# Actual command for chromium is chromium-browser.
 	# No need to specify newTab. It's default in chromium
 	eval "$browser_path" "$@" &
 	;;
-    konqueror)
+konqueror)
 	case "$(basename "$browser_path")" in
-	    konqueror)
+	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 "$@"
 		;;
-	    kfmclient)
+	kfmclient)
 		eval "$browser_path" newTab "$@"
 		;;
-	    *)
+	*)
 		"$browser_path" "$@" &
 		;;
 	esac
 	;;
-    w3m|links|lynx|open)
+w3m|links|lynx|open)
 	eval "$browser_path" "$@"
 	;;
-    start)
-        exec "$browser_path" '"web-browse"' "$@"
-        ;;
-    dillo)
+start)
+	exec "$browser_path" '"web-browse"' "$@"
+	;;
+dillo)
 	"$browser_path" "$@" &
 	;;
-    *)
+*)
 	if test -n "$browser_cmd"; then
-	    ( eval $browser_cmd "$@" )
+		( eval $browser_cmd "$@" )
 	fi
 	;;
 esac
-- 
1.7.3.2.624.gec6c7.dirty

[PATCH 3/6] web--browse: split valid_tool list

[Giuseppe Bilotta]

It was getting too long, and we want to add some more.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 git-web--browse.sh |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/git-web--browse.sh b/git-web--browse.sh
index 7c4568f..b20b0e0 100755
--- a/git-web--browse.sh
+++ b/git-web--browse.sh
@@ -31,7 +31,8 @@ valid_custom_tool()
 
 valid_tool() {
 	case "$1" in
-	firefox | iceweasel | chrome | google-chrome | chromium | konqueror | w3m | links | lynx | dillo | open | start)
+	firefox|iceweasel|chrome|google-chrome|chromium\
+	|konqueror|w3m|links|lynx|dillo|open|start)
 		;; # happy
 	*)
 		valid_custom_tool "$1" || return 1
-- 
1.7.3.2.624.gec6c7.dirty

[PATCH 4/6] web--browse: support opera, seamonkey and elinks

[Giuseppe Bilotta]

The list of supported browsers is also updated in the documentation.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 Documentation/git-web--browse.txt |    6 ++++++
 git-web--browse.sh                |   14 +++++++-------
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-web--browse.txt b/Documentation/git-web--browse.txt
index 51e8e0a..5d3ae07 100644
--- a/Documentation/git-web--browse.txt
+++ b/Documentation/git-web--browse.txt
@@ -20,8 +20,14 @@ The following browsers (or commands) are currently supported:
 
 * firefox (this is the default under X Window when not using KDE)
 * iceweasel
+* seamonkey
+* iceape
+* chromium
+* google-chrome
 * konqueror (this is the default under KDE, see 'Note about konqueror' below)
+* opera
 * w3m (this is the default outside graphical environments)
+* elinks
 * links
 * lynx
 * dillo
diff --git a/git-web--browse.sh b/git-web--browse.sh
index b20b0e0..7b7f45f 100755
--- a/git-web--browse.sh
+++ b/git-web--browse.sh
@@ -31,8 +31,8 @@ valid_custom_tool()
 
 valid_tool() {
 	case "$1" in
-	firefox|iceweasel|chrome|google-chrome|chromium\
-	|konqueror|w3m|links|lynx|dillo|open|start)
+	firefox|iceweasel|seamonkey|iceape|chrome|google-chrome|chromium\
+	|konqueror|opera|w3m|elinks|links|lynx|dillo|open|start)
 		;; # happy
 	*)
 		valid_custom_tool "$1" || return 1
@@ -104,12 +104,12 @@ fi
 
 if test -z "$browser" ; then
 	if test -n "$DISPLAY"; then
-		browser_candidates="firefox iceweasel google-chrome chrome chromium konqueror w3m links lynx dillo"
+		browser_candidates="firefox iceweasel google-chrome chrome chromium konqueror opera seamonkey iceape w3m elinks links lynx dillo"
 		if test "$KDE_FULL_SESSION" = "true"; then
 			browser_candidates="konqueror $browser_candidates"
 		fi
 	else
-		browser_candidates="w3m links lynx"
+		browser_candidates="w3m elinks links lynx"
 	fi
 	# SECURITYSESSIONID indicates an OS X GUI login session
 	if test -n "$SECURITYSESSIONID" \
@@ -140,7 +140,7 @@ else
 fi
 
 case "$browser" in
-firefox|iceweasel)
+firefox|iceweasel|seamonkey|iceape)
 	# Check version because firefox < 2.0 does not support "-new-tab".
 	vers=$(expr "$($browser_path -version)" : '.* \([0-9][0-9]*\)\..*')
 	NEWTAB='-new-tab'
@@ -168,13 +168,13 @@ konqueror)
 		;;
 	esac
 	;;
-w3m|links|lynx|open)
+w3m|elinks|links|lynx|open)
 	eval "$browser_path" "$@"
 	;;
 start)
 	exec "$browser_path" '"web-browse"' "$@"
 	;;
-dillo)
+opera|dillo)
 	"$browser_path" "$@" &
 	;;
 *)
-- 
1.7.3.2.624.gec6c7.dirty

[PATCH 5/6] web--browse: use (x-)www-browser if available

[Giuseppe Bilotta]

Debian and derivatives have an alternatives-based default browser
configuration that uses the /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.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 Documentation/git-web--browse.txt |    4 ++++
 git-web--browse.sh                |   34 +++++++++++++++++++++++++++++++---
 2 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-web--browse.txt b/Documentation/git-web--browse.txt
index 5d3ae07..76b5cdc 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.
+
 OPTIONS
 -------
 -b <browser>::
diff --git a/git-web--browse.sh b/git-web--browse.sh
index 7b7f45f..f991dd0 100755
--- a/git-web--browse.sh
+++ b/git-web--browse.sh
@@ -102,6 +102,33 @@ then
 	fi
 fi
 
+# Debian and derivatives use x-www-browser or www-browser to set
+# the default browser for the system
+if test -z "$browser" ; then
+	wwwbrowser="/usr/bin/www-browser"
+	if test -n "$DISPLAY"; then
+		wwwbrowser="/usr/bin/x-www-browser $wwwbrowser"
+	fi
+	for i in $wwwbrowser; do
+		if test -x $i ; then
+			verstring="$($i --version 2> /dev/null | head -n 1)"
+			browser="$(echo "$verstring" | cut -f1 -d' ' | tr A-Z a-z)"
+			case "$browser" in
+			mozilla)
+				browser="$(echo "$verstring" | cut -f2 -d' ' | tr A-Z a-z)"
+				;;
+			google)
+				browser="google-chrome"
+				;;
+			esac
+			if valid_tool "$browser" ; then
+				browser_path="$i"
+				break
+			fi
+		fi
+	done
+fi
+
 if test -z "$browser" ; then
 	if test -n "$DISPLAY"; then
 		browser_candidates="firefox iceweasel google-chrome chrome chromium konqueror opera seamonkey iceape w3m elinks links lynx dillo"
@@ -129,7 +156,7 @@ if test -z "$browser" ; then
 		fi
 	done
 	test -z "$browser" && die "No known browser available."
-else
+else if test -z "$browser_path"; then
 	valid_tool "$browser" || die "Unknown browser '$browser'."
 
 	init_browser_path "$browser"
@@ -137,12 +164,13 @@ else
 	if test -z "$browser_cmd" && ! type "$browser_path" > /dev/null 2>&1; then
 		die "The browser $browser is not available as '$browser_path'."
 	fi
-fi
+fi 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]*\)\..*')
+	test -z "$verstring" && verstring="$($browser_path -version)"
+	vers=$(expr "$verstring" : '.* \([0-9][0-9]*\)\..*')
 	NEWTAB='-new-tab'
 	test "$vers" -lt 2 && NEWTAB=''
 	"$browser_path" $NEWTAB "$@" &
-- 
1.7.3.2.624.gec6c7.dirty

[PATCH 6/6] web--browse: special-case chromium path

[Giuseppe Bilotta]

On Debian-based distributions, Chromium the browser is available under
the name chromium-browser rather than chromium, to prevent conflicts
with the Chromium B.S.U. game.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 git-web--browse.sh |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/git-web--browse.sh b/git-web--browse.sh
index f991dd0..9cd66e4 100755
--- a/git-web--browse.sh
+++ b/git-web--browse.sh
@@ -42,6 +42,9 @@ valid_tool() {
 
 init_browser_path() {
 	browser_path=$(git config "browser.$1.path")
+	if test "$1" = chromium ; then
+		type chromium-browser > /dev/null 2>&1 && browser_path=chromium-browser
+	fi
 	test -z "$browser_path" && browser_path="$1"
 }
 
-- 
1.7.3.2.624.gec6c7.dirty

Re: [PATCH 5/6] web--browse: use (x-)www-browser if available

[Christian Couder]

Hi,

On Mon, Nov 29, 2010 at 3:47 PM, Giuseppe Bilotta
<giuseppe.bilotta@gmail.com> wrote:
>
> +# Debian and derivatives use x-www-browser or www-browser to set
> +# the default browser for the system
> +if test -z "$browser" ; then
> +       wwwbrowser="/usr/bin/www-browser"
> +       if test -n "$DISPLAY"; then
> +               wwwbrowser="/usr/bin/x-www-browser $wwwbrowser"
> +       fi
> +       for i in $wwwbrowser; do
> +               if test -x $i ; then
> +                       verstring="$($i --version 2> /dev/null | head -n 1)"
> +                       browser="$(echo "$verstring" | cut -f1 -d' ' | tr A-Z a-z)"

Stupid questions:

Did you check that all the browser we support accept the --version option?
What if we add support for a new one that doesn't ?
Shouldn't we add something like :

test -z "$browser" && browser="$(echo $i | cut -f1 -d' ' | tr A-Z a-z)"

And are you sure that when they support --version, the first word of
the output is
better than "$(echo $i | cut -f1 -d' ' | tr A-Z a-z)"?
What if we add support for a new one?

> +                       case "$browser" in
> +                       mozilla)
> +                               browser="$(echo "$verstring" | cut -f2 -d' ' | tr A-Z a-z)"
> +                               ;;
> +                       google)
> +                               browser="google-chrome"
> +                               ;;
> +                       esac
> +                       if valid_tool "$browser" ; then

valid_tool "$browser" is called once here...

> +                               browser_path="$i"
> +                               break
> +                       fi
> +               fi
> +       done
> +fi
> +
>  if test -z "$browser" ; then
>        if test -n "$DISPLAY"; then
>                browser_candidates="firefox iceweasel google-chrome chrome chromium konqueror opera seamonkey iceape w3m elinks links lynx dillo"
> @@ -129,7 +156,7 @@ if test -z "$browser" ; then
>                fi
>        done
>        test -z "$browser" && die "No known browser available."
> -else
> +else if test -z "$browser_path"; then
>        valid_tool "$browser" || die "Unknown browser '$browser'."

...valid_tool "$browser" is called again here if it failed above, and
here we die,
so isn't it clearer to die as soon as we call it and it fails?

Thanks,
Christian.

Re: [PATCH 5/6] web--browse: use (x-)www-browser if available

[Christian Couder]

On Mon, Nov 29, 2010 at 5:18 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> Hi,
>
> On Mon, Nov 29, 2010 at 3:47 PM, Giuseppe Bilotta
> <giuseppe.bilotta@gmail.com> wrote:
>>
>> +# Debian and derivatives use x-www-browser or www-browser to set
>> +# the default browser for the system
>> +if test -z "$browser" ; then
>> +       wwwbrowser="/usr/bin/www-browser"
>> +       if test -n "$DISPLAY"; then
>> +               wwwbrowser="/usr/bin/x-www-browser $wwwbrowser"
>> +       fi
>> +       for i in $wwwbrowser; do
>> +               if test -x $i ; then
>> +                       verstring="$($i --version 2> /dev/null | head -n 1)"
>> +                       browser="$(echo "$verstring" | cut -f1 -d' ' | tr A-Z a-z)"
>
> Stupid questions:
>
> Did you check that all the browser we support accept the --version option?
> What if we add support for a new one that doesn't ?
> Shouldn't we add something like :
>
> test -z "$browser" && browser="$(echo $i | cut -f1 -d' ' | tr A-Z a-z)"

Sorry I mean:

 test -z "$browser" && browser="$(readlink $i)"

> And are you sure that when they support --version, the first word of
> the output is
> better than "$(echo $i | cut -f1 -d' ' | tr A-Z a-z)"?

and: "$(readlink $i)"

Thanks,
Christian.

Re: [PATCH 3/6] web--browse: split valid_tool list

[Jonathan Nieder]

Giuseppe Bilotta wrote:

> --- a/git-web--browse.sh
> +++ b/git-web--browse.sh
> @@ -31,7 +31,8 @@ valid_custom_tool()
>  
>  valid_tool() {
>  	case "$1" in
> -	firefox | iceweasel | chrome | google-chrome | chromium | konqueror | w3m | links | lynx | dillo | open | start)
> +	firefox|iceweasel|chrome|google-chrome|chromium\
> +	|konqueror|w3m|links|lynx|dillo|open|start)

micronit: I think this looks better as

	firefox|iceweasel|chrome|google-chrome|chromium| \
	konqueror|w3m|links|lynx|dillo|open|start)

, with the | on the end of the first line.

Re: [PATCH 5/6] web--browse: use (x-)www-browser if available

[Jonathan Nieder]

Giuseppe Bilotta wrote:

> Debian and derivatives have an alternatives-based default browser
> configuration that uses the /usr/bin/x-www-browser and
> /usr/bin/www-browser symlinks.

Mm, maybe it will be time to look into adding xdg-open support
soon, too.

Re: [PATCH 5/6] web--browse: use (x-)www-browser if available

[Giuseppe Bilotta]

On Mon, Nov 29, 2010 at 5:18 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> Hi,
>
> On Mon, Nov 29, 2010 at 3:47 PM, Giuseppe Bilotta
> <giuseppe.bilotta@gmail.com> wrote:
>>
>> +# Debian and derivatives use x-www-browser or www-browser to set
>> +# the default browser for the system
>> +if test -z "$browser" ; then
>> +       wwwbrowser="/usr/bin/www-browser"
>> +       if test -n "$DISPLAY"; then
>> +               wwwbrowser="/usr/bin/x-www-browser $wwwbrowser"
>> +       fi
>> +       for i in $wwwbrowser; do
>> +               if test -x $i ; then
>> +                       verstring="$($i --version 2> /dev/null | head -n 1)"
>> +                       browser="$(echo "$verstring" | cut -f1 -d' ' | tr A-Z a-z)"
>
> Stupid questions:
>
> Did you check that all the browser we support accept the --version option?

I don't have all of them readily available, so I checked with
Mozilla-based browsers, Opera, Chromium, elinks, lynx, w3m. I'm
missing konqueror and dillo.

> What if we add support for a new one that doesn't ?

I think that the worse issue would be (x-)www-browser linking to
something that doesn't support --version regardless of wether we
support it or not.

> Shouldn't we add something like :
>
> test -z "$browser" && browser="$(readlink $i)"

My first idea was to go for something like browser="$(basename
$(readlink $i))" (not sure why you would need test -z before). Since
it would need special-casing anyway (e.g. chromium-browser ->
chromium), I opted out for the --version way so that (1) we could
catch one of our friendly cases even if the binary was called
something else and (2) some invocation paths try to get the version
anyway, so we could do it once and for all.

> And are you sure that when they support --version, the first word of
> the output is
> better than "$(readlink $i)"?

Probably depends on the metric used for 'better' ;-)

> What if we add support for a new one?

Then when adding it we should also look at its --version output and
see if it needs special treatment.

>> +                       case "$browser" in
>> +                       mozilla)
>> +                               browser="$(echo "$verstring" | cut -f2 -d' ' | tr A-Z a-z)"
>> +                               ;;
>> +                       google)
>> +                               browser="google-chrome"
>> +                               ;;
>> +                       esac
>> +                       if valid_tool "$browser" ; then
>
> valid_tool "$browser" is called once here...
>
>> -else
>> +else if test -z "$browser_path"; then
>>        valid_tool "$browser" || die "Unknown browser '$browser'."
>
> ...valid_tool "$browser" is called again here if it failed above, and
> here we die,
> so isn't it clearer to die as soon as we call it and it fails?

We allow valid_tool to be false in the x-www-browser case, in which
case we test www-browser, and if it's still not valid we go on and use
the previous paths. So we cannot die in case of an invalid
(x-)www-browser. But there's a bug in the www-brower testing, it needs
an else that resets browser to the empty string.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH 5/6] web--browse: use (x-)www-browser if available

[Giuseppe Bilotta]

On Mon, Nov 29, 2010 at 5:48 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Giuseppe Bilotta wrote:
>
>> Debian and derivatives have an alternatives-based default browser
>> configuration that uses the /usr/bin/x-www-browser and
>> /usr/bin/www-browser symlinks.
>
> Mm, maybe it will be time to look into adding xdg-open support
> soon, too.

That's probably a good idea, although I think there is no way with
xdg-open to say "open in a new tab".

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH 3/6] web--browse: split valid_tool list

[Giuseppe Bilotta]

On Mon, Nov 29, 2010 at 5:44 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Giuseppe Bilotta wrote:
>
>> --- a/git-web--browse.sh
>> +++ b/git-web--browse.sh
>> @@ -31,7 +31,8 @@ valid_custom_tool()
>>
>>  valid_tool() {
>>       case "$1" in
>> -     firefox | iceweasel | chrome | google-chrome | chromium | konqueror | w3m | links | lynx | dillo | open | start)
>> +     firefox|iceweasel|chrome|google-chrome|chromium\
>> +     |konqueror|w3m|links|lynx|dillo|open|start)
>
> micronit: I think this looks better as
>
>        firefox|iceweasel|chrome|google-chrome|chromium| \
>        konqueror|w3m|links|lynx|dillo|open|start)
>
> , with the | on the end of the first line.

I opted for the | at the beginning of the line to give a better idea
of the continuation, but I don't have a strong preference in that
direction, so I can fix it up as you suggested, unless there are
specific indications from the 'higher ups' ;-)

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH 3/6] web--browse: split valid_tool list

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Giuseppe Bilotta wrote:
>
>> --- a/git-web--browse.sh
>> +++ b/git-web--browse.sh
>> @@ -31,7 +31,8 @@ valid_custom_tool()
>>  
>>  valid_tool() {
>>  	case "$1" in
>> -	firefox | iceweasel | chrome | google-chrome | chromium | konqueror | w3m | links | lynx | dillo | open | start)
>> +	firefox|iceweasel|chrome|google-chrome|chromium\
>> +	|konqueror|w3m|links|lynx|dillo|open|start)
>
> micronit: I think this looks better as
>
> 	firefox|iceweasel|chrome|google-chrome|chromium| \
> 	konqueror|w3m|links|lynx|dillo|open|start)
>
> , with the | on the end of the first line.

Yes, and with one SP around all "|".

Thanks.

Re: [PATCH 1/6] CodingGuidelines: mention whitespace preferences for shell scripts

[Junio C Hamano]

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  Documentation/CodingGuidelines |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> index 5aa2d34..7ecce51 100644
> --- a/Documentation/CodingGuidelines
> +++ b/Documentation/CodingGuidelines
> @@ -31,6 +31,12 @@ But if you must have a list of rules, here they are.
>  
>  For shell scripts specifically (not exhaustive):
>  
> + - We use tabs for indentation.

Ok.

> + - The choices in case ... esac statement are not indented with respect
> +   to the the case and esac lines. The body of the choices however _is_
> +   indented (by one tab).
> +

Nit; "choices" are called "case arms".

I don't think there is any need to emphasize your surprise that case body
is indented with "however _is_", which implies "against common sense or
normal convention" or somesuch.  The thing is, there is no single _right_
convention, and we want people to folow the one just picked---we should
just describe what it is without being emotional nor emphatic.

Thanks.

Re: [PATCH 5/6] web--browse: use (x-)www-browser if available

[Christian Couder]

On Mon, Nov 29, 2010 at 8:05 PM, Giuseppe Bilotta
<giuseppe.bilotta@gmail.com> wrote:
> On Mon, Nov 29, 2010 at 5:18 PM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> Hi,
>>
>> On Mon, Nov 29, 2010 at 3:47 PM, Giuseppe Bilotta
>> <giuseppe.bilotta@gmail.com> wrote:
>>>
>>> +# Debian and derivatives use x-www-browser or www-browser to set
>>> +# the default browser for the system
>>> +if test -z "$browser" ; then
>>> +       wwwbrowser="/usr/bin/www-browser"
>>> +       if test -n "$DISPLAY"; then
>>> +               wwwbrowser="/usr/bin/x-www-browser $wwwbrowser"
>>> +       fi
>>> +       for i in $wwwbrowser; do
>>> +               if test -x $i ; then
>>> +                       verstring="$($i --version 2> /dev/null | head -n 1)"
>>> +                       browser="$(echo "$verstring" | cut -f1 -d' ' | tr A-Z a-z)"
>>
>> Stupid questions:
>>
>> Did you check that all the browser we support accept the --version option?
>
> I don't have all of them readily available, so I checked with
> Mozilla-based browsers, Opera, Chromium, elinks, lynx, w3m. I'm
> missing konqueror and dillo.
>
>> What if we add support for a new one that doesn't ?
>
> I think that the worse issue would be (x-)www-browser linking to
> something that doesn't support --version regardless of wether we
> support it or not.
>
>> Shouldn't we add something like :
>>
>> test -z "$browser" && browser="$(readlink $i)"
>
> My first idea was to go for something like browser="$(basename
> $(readlink $i))" (not sure why you would need test -z before).

We would need "test -z" before if we add it after:

browser="$(echo "$verstring" | cut -f1 -d' ' | tr A-Z a-z)"

so that we have a fallback if it doesn't support "--version".

> Since
> it would need special-casing anyway (e.g. chromium-browser ->
> chromium), I opted out for the --version way so that (1) we could
> catch one of our friendly cases even if the binary was called
> something else and (2) some invocation paths try to get the version
> anyway, so we could do it once and for all.
>
>> And are you sure that when they support --version, the first word of
>> the output is
>> better than "$(readlink $i)"?
>
> Probably depends on the metric used for 'better' ;-)
>
>> What if we add support for a new one?
>
> Then when adding it we should also look at its --version output and
> see if it needs special treatment.
>
>>> +                       case "$browser" in
>>> +                       mozilla)
>>> +                               browser="$(echo "$verstring" | cut -f2 -d' ' | tr A-Z a-z)"
>>> +                               ;;
>>> +                       google)
>>> +                               browser="google-chrome"
>>> +                               ;;
>>> +                       esac
>>> +                       if valid_tool "$browser" ; then
>>
>> valid_tool "$browser" is called once here...
>>
>>> -else
>>> +else if test -z "$browser_path"; then
>>>        valid_tool "$browser" || die "Unknown browser '$browser'."
>>
>> ...valid_tool "$browser" is called again here if it failed above, and
>> here we die,
>> so isn't it clearer to die as soon as we call it and it fails?
>
> We allow valid_tool to be false in the x-www-browser case, in which
> case we test www-browser, and if it's still not valid we go on and use
> the previous paths. So we cannot die in case of an invalid
> (x-)www-browser.

Yeah, you are right, but we could die after the "for i in $wwwbrowser"
loop if both are invalid.

> But there's a bug in the www-brower testing, it needs
> an else that resets browser to the empty string.

I thought it was by design that you did not reset it...
So yeah it is clearer and nicer for the user if you either reset
browser or just die if both (x-)www-browser are invalid. If you decide
to reset browser, perhaps a warning or an information message telling
that both are unknown would be nice.

Thanks,
Christian.

Re: [PATCH 1/6] CodingGuidelines: mention whitespace preferences for shell scripts

[Giuseppe Bilotta]

On Tue, Nov 30, 2010 at 1:30 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>
>> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
>> ---
>>  Documentation/CodingGuidelines |    6 ++++++
>>  1 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
>> index 5aa2d34..7ecce51 100644
>> --- a/Documentation/CodingGuidelines
>> +++ b/Documentation/CodingGuidelines
>> @@ -31,6 +31,12 @@ But if you must have a list of rules, here they are.
>>
>>  For shell scripts specifically (not exhaustive):
>>
>> + - We use tabs for indentation.
>
> Ok.
>
>> + - The choices in case ... esac statement are not indented with respect
>> +   to the the case and esac lines. The body of the choices however _is_
>> +   indented (by one tab).
>> +
>
> Nit; "choices" are called "case arms".

I'll rephrase.

> I don't think there is any need to emphasize your surprise that case body
> is indented with "however _is_", which implies "against common sense or
> normal convention" or somesuch.  The thing is, there is no single _right_
> convention, and we want people to folow the one just picked---we should
> just describe what it is without being emotional nor emphatic.

Good point, I'll resend with a shorter commit message. Would you be ok
with me adding an example, or do you think it's unnecessary?

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH 5/6] web--browse: use (x-)www-browser if available

[Giuseppe Bilotta]

On Tue, Nov 30, 2010 at 5:02 AM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Mon, Nov 29, 2010 at 8:05 PM, Giuseppe Bilotta
> <giuseppe.bilotta@gmail.com> wrote:
>> On Mon, Nov 29, 2010 at 5:18 PM, Christian Couder
>> <christian.couder@gmail.com> wrote:
>>> Hi,
>>>
>>> On Mon, Nov 29, 2010 at 3:47 PM, Giuseppe Bilotta
>>> <giuseppe.bilotta@gmail.com> wrote:
>>>>
>>>> +# Debian and derivatives use x-www-browser or www-browser to set
>>>> +# the default browser for the system
>>>> +if test -z "$browser" ; then
>>>> +       wwwbrowser="/usr/bin/www-browser"
>>>> +       if test -n "$DISPLAY"; then
>>>> +               wwwbrowser="/usr/bin/x-www-browser $wwwbrowser"
>>>> +       fi
>>>> +       for i in $wwwbrowser; do
>>>> +               if test -x $i ; then
>>>> +                       verstring="$($i --version 2> /dev/null | head -n 1)"
>>>> +                       browser="$(echo "$verstring" | cut -f1 -d' ' | tr A-Z a-z)"
>>>
>>> Stupid questions:
>>>
>>> Did you check that all the browser we support accept the --version option?
>>
>> I don't have all of them readily available, so I checked with
>> Mozilla-based browsers, Opera, Chromium, elinks, lynx, w3m. I'm
>> missing konqueror and dillo.
>>
>>> What if we add support for a new one that doesn't ?
>>
>> I think that the worse issue would be (x-)www-browser linking to
>> something that doesn't support --version regardless of wether we
>> support it or not.
>>
>>> Shouldn't we add something like :
>>>
>>> test -z "$browser" && browser="$(readlink $i)"
>>
>> My first idea was to go for something like browser="$(basename
>> $(readlink $i))" (not sure why you would need test -z before).
>
> We would need "test -z" before if we add it after:
>
> browser="$(echo "$verstring" | cut -f1 -d' ' | tr A-Z a-z)"
>
> so that we have a fallback if it doesn't support "--version".

Oh, I see what you mean here. I thought you wanted to use it as an
alternative to the --version.

>> We allow valid_tool to be false in the x-www-browser case, in which
>> case we test www-browser, and if it's still not valid we go on and use
>> the previous paths. So we cannot die in case of an invalid
>> (x-)www-browser.
>
> Yeah, you are right, but we could die after the "for i in $wwwbrowser"
> loop if both are invalid.

Why? If both are invalid, proceeding with the previous strategy of
looking for a browser we _should_ be looking for any browser we know
about, even if it's not set as the default system browser.

>> But there's a bug in the www-brower testing, it needs
>> an else that resets browser to the empty string.
>
> I thought it was by design that you did not reset it...
> So yeah it is clearer and nicer for the user if you either reset
> browser or just die if both (x-)www-browser are invalid. If you decide
> to reset browser, perhaps a warning or an information message telling
> that both are unknown would be nice.

I can do that. Should it be a warning about reporting the lack of
support to us, or just a warning that we are not going to use it even
if it's defined?

While we're at it: I was considering adding support for the BROWSER
env var (a colon-separated list of browsers executables or "browser
%s" strings).

All of this is going to make the web--browse script very similar to
the sensible-browser script in Debian, with the difference that we go
at length to ensure that stuff is opened in a new tab, whereas
Debian's sensible-browser doesn't. Should we just support
sensible-browser instead of (x-)www-browser/BROWSER, and let it open
anything?


-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH 5/6] web--browse: use (x-)www-browser if available

[Christian Couder]

On Tue, Nov 30, 2010 at 9:22 AM, Giuseppe Bilotta
<giuseppe.bilotta@gmail.com> wrote:
> On Tue, Nov 30, 2010 at 5:02 AM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> On Mon, Nov 29, 2010 at 8:05 PM, Giuseppe Bilotta
>> <giuseppe.bilotta@gmail.com> wrote:
>>> We allow valid_tool to be false in the x-www-browser case, in which
>>> case we test www-browser, and if it's still not valid we go on and use
>>> the previous paths. So we cannot die in case of an invalid
>>> (x-)www-browser.
>>
>> Yeah, you are right, but we could die after the "for i in $wwwbrowser"
>> loop if both are invalid.
>
> Why? If both are invalid, proceeding with the previous strategy of
> looking for a browser we _should_ be looking for any browser we know
> about, even if it's not set as the default system browser.

Currently we have:

97     if test -n "$browser" && ! valid_tool "$browser"; then
98         echo >&2 "git config option $opt set to unknown browser: $browser"
99         echo >&2 "Resetting to default..."
100         unset browser
101     fi

So if we want to be consistent with that behavior, we should probably do the
same thing if (x-)www-browser is set but we don't support it.

>>> But there's a bug in the www-brower testing, it needs
>>> an else that resets browser to the empty string.
>>
>> I thought it was by design that you did not reset it...
>> So yeah it is clearer and nicer for the user if you either reset
>> browser or just die if both (x-)www-browser are invalid. If you decide
>> to reset browser, perhaps a warning or an information message telling
>> that both are unknown would be nice.
>
> I can do that. Should it be a warning about reporting the lack of
> support to us, or just a warning that we are not going to use it even
> if it's defined?

I think it should be a warning that we are not going to use it even
if it's defined, to be consistent with the code pasted above.

If you think of something better or want to remove the warnings, please do
it in another patch.

> While we're at it: I was considering adding support for the BROWSER
> env var (a colon-separated list of browsers executables or "browser
> %s" strings).
>
> All of this is going to make the web--browse script very similar to
> the sensible-browser script in Debian, with the difference that we go
> at length to ensure that stuff is opened in a new tab, whereas
> Debian's sensible-browser doesn't. Should we just support
> sensible-browser instead of (x-)www-browser/BROWSER, and let it open
> anything?

I think most users prefer to open stuff in a new tab if possible.

Thanks,
Christian.

Re: [PATCH 5/6] web--browse: use (x-)www-browser if available

[Giuseppe Bilotta]

On Wed, Dec 1, 2010 at 11:59 AM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Tue, Nov 30, 2010 at 9:22 AM, Giuseppe Bilotta
> <giuseppe.bilotta@gmail.com> wrote:
>> On Tue, Nov 30, 2010 at 5:02 AM, Christian Couder
>> <christian.couder@gmail.com> wrote:
>>> On Mon, Nov 29, 2010 at 8:05 PM, Giuseppe Bilotta
>>> <giuseppe.bilotta@gmail.com> wrote:
>>>> We allow valid_tool to be false in the x-www-browser case, in which
>>>> case we test www-browser, and if it's still not valid we go on and use
>>>> the previous paths. So we cannot die in case of an invalid
>>>> (x-)www-browser.
>>>
>>> Yeah, you are right, but we could die after the "for i in $wwwbrowser"
>>> loop if both are invalid.
>>
>> Why? If both are invalid, proceeding with the previous strategy of
>> looking for a browser we _should_ be looking for any browser we know
>> about, even if it's not set as the default system browser.
>
> Currently we have:
>
> 97     if test -n "$browser" && ! valid_tool "$browser"; then
> 98         echo >&2 "git config option $opt set to unknown browser: $browser"
> 99         echo >&2 "Resetting to default..."
> 100         unset browser
> 101     fi
>
> So if we want to be consistent with that behavior, we should probably do the
> same thing if (x-)www-browser is set but we don't support it.

The (x-)www-browser code is used when the user did _not_ define a
browser. It is not a browser option (git web--browse
--tool=www-browser).
IOW, this is not about the user selecting an unsupported browser, but
about the system default browser not being a supported one.  I don't
think it should be treated the same way.

>>>> But there's a bug in the www-brower testing, it needs
>>>> an else that resets browser to the empty string.
>>>
>>> I thought it was by design that you did not reset it...
>>> So yeah it is clearer and nicer for the user if you either reset
>>> browser or just die if both (x-)www-browser are invalid. If you decide
>>> to reset browser, perhaps a warning or an information message telling
>>> that both are unknown would be nice.
>>
>> I can do that. Should it be a warning about reporting the lack of
>> support to us, or just a warning that we are not going to use it even
>> if it's defined?
>
> I think it should be a warning that we are not going to use it even
> if it's defined, to be consistent with the code pasted above.

Ok.

> If you think of something better or want to remove the warnings, please do
> it in another patch.

I think the warning is fine.

>> While we're at it: I was considering adding support for the BROWSER
>> env var (a colon-separated list of browsers executables or "browser
>> %s" strings).
>>
>> All of this is going to make the web--browse script very similar to
>> the sensible-browser script in Debian, with the difference that we go
>> at length to ensure that stuff is opened in a new tab, whereas
>> Debian's sensible-browser doesn't. Should we just support
>> sensible-browser instead of (x-)www-browser/BROWSER, and let it open
>> anything?
>
> I think most users prefer to open stuff in a new tab if possible.

I will look into adding support for the BROWSER environment variable
to select the browser if no browser is defined, then, but not consider
sensible-browser.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH 5/6] web--browse: use (x-)www-browser if available

[Jonathan Nieder]

Giuseppe Bilotta wrote:

> I will look into adding support for the BROWSER environment variable
> to select the browser if no browser is defined

Thanks, that sounds excellent.

BTW, the semantics of quoting characters/shell metacharacters in
$BROWSER are not well defined (well, they're well defined in one way
by the original spec, which is ignored by xdg-open and some other
tools).  xdg-open's semantics are probably the most reliable standard
here but thought I should give a heads up.

Thinking out loud: longer term, it would be nice to just use xdg-utils
and drop most of this logic from git.  Presumably the ideal behavior
for most apps opening web pages is already "new tab", so the cases of
"new window" not explicitly configured that way by the user are bugs.
Otherwise a request to portland@lists.freedesktop.org (subscription
required) for xdg-open --new-tab/--new-window might be useful.

Regards,
Jonathan