{"thread":{"id":"25938","subject":"[PATCHv2 0/7] web--browse cleanup and extensions","startedAt":"2010-12-03T16:47:34Z","lastAt":"2010-12-07T23:38:10Z","messageCount":25,"participants":["Giuseppe Bilotta","Jonathan Nieder","Christian Couder","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"157215","messageId":"1291394861-11989-1-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25938","inReplyTo":null,"subject":"[PATCHv2 0/7] web--browse cleanup and extensions","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-12-03T16:47:34Z","receivedAt":"2010-12-03T16:47:34Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"This second issue of the web--browse cleanup and extension patchset\nhas seen some stylistic changes in the first patches as discussed on the\nML, and a new patch to add support for the BROWSER env var. Support for\nxdg-open is still missing, due to the lack of an obvious way to suggest\nopening pages in new tabs (same reason why we don't directly support\nDebian's sensible-browser).\n\nI'm quite convinced that the first 4 patches (up to the \"support opera,\nseamonkey and elinks\" patch, inclusive) are ready for merging.\n\nThe fifth patch is slightly more extensive than in the previous\niteration, including chromium-browser as a supported option, but still\nlooking for chromium-browser first even when browser=chromium.\n\nThe last two patches are more RFCish. I tried to include the changes\nsuggested by Christian, unless of course I forgot something. Suggestions\non possible ways to improve the proposed approach are welcome.\n\nGiuseppe Bilotta (7):\n CodingGuidelines: mention whitespace preferences for shell scripts\n web--browse: coding style\n web--browse: split valid_tool list\n web--browse: support opera, seamonkey and elinks\n web--browse: better support for chromium\n web--browse: use (x-)www-browser if available\n web--browse: look at the BROWSER env var\n\n Documentation/CodingGuidelines | 4 +\n Documentation/git-web--browse.txt | 10 ++\n git-web--browse.sh | 276 +++++++++++++++++++++++++------------\n 3 files changed, 203 insertions(+), 87 deletions(-)\n\n-- \n1.7.3.2.664.g294b8.dirty\n"},{"id":"157216","messageId":"1291394861-11989-2-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25938","inReplyTo":"1291394861-11989-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv2 1/7] CodingGuidelines: mention whitespace preferences for shell scripts","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-12-03T16:47:35Z","receivedAt":"2010-12-03T16:47:35Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Signed-off-by: Giuseppe Bilotta \n---\n Documentation/CodingGuidelines | 4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex 5aa2d34..a9191d0 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -31,6 +31,10 @@ But if you must have a list of rules, here they are.\n \n For shell scripts specifically (not exhaustive):\n \n+ - We use tabs for indentation.\n+\n+ - Case arms are not indented with respect to the case and esac lines.\n+\n - We prefer $( ... ) for command substitution; unlike ``, it\n properly nests. It should have been the way Bourne spelled\n it from day one, but unfortunately isn't.\n-- \n1.7.3.2.664.g294b8.dirty\n"},{"id":"157217","messageId":"1291394861-11989-3-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25938","inReplyTo":"1291394861-11989-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv2 2/7] web--browse: coding style","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-12-03T16:47:36Z","receivedAt":"2010-12-03T16:47:36Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Retab and deindent choices in case statements.\n\nSigned-off-by: Giuseppe Bilotta \n---\n git-web--browse.sh | 166 ++++++++++++++++++++++++++--------------------------\n 1 files changed, 83 insertions(+), 83 deletions(-)\n\ndiff --git a/git-web--browse.sh b/git-web--browse.sh\nindex 3fc4166..7c4568f 100755\n--- a/git-web--browse.sh\n+++ b/git-web--browse.sh\n@@ -31,11 +31,11 @@ valid_custom_tool()\n \n valid_tool() {\n \tcase \"$1\" in\n-\t\tfirefox | iceweasel | chrome | google-chrome | chromium | konqueror | w3m | links | lynx | dillo | open | start)\n-\t\t\t;; # happy\n-\t\t*)\n-\t\t\tvalid_custom_tool \"$1\" || return 1\n-\t\t\t;;\n+\tfirefox | iceweasel | chrome | google-chrome | chromium | konqueror | w3m | links | lynx | dillo | open | start)\n+\t\t;; # happy\n+\t*)\n+\t\tvalid_custom_tool \"$1\" || return 1\n+\t\t;;\n \tesac\n }\n \n@@ -46,139 +46,139 @@ init_browser_path() {\n \n while test $# != 0\n do\n- case \"$1\" in\n+\tcase \"$1\" in\n \t-b|--browser*|-t|--tool*)\n-\t case \"$#,$1\" in\n+\t\tcase \"$#,$1\" in\n \t\t*,*=*)\n-\t\t browser=`expr \"z$1\" : 'z-[^=]*=\\(.*\\)'`\n-\t\t ;;\n+\t\t\tbrowser=`expr \"z$1\" : 'z-[^=]*=\\(.*\\)'`\n+\t\t\t;;\n \t\t1,*)\n-\t\t usage ;;\n+\t\t\tusage ;;\n \t\t*)\n-\t\t browser=\"$2\"\n-\t\t shift ;;\n-\t esac\n-\t ;;\n+\t\t\tbrowser=\"$2\"\n+\t\t\tshift ;;\n+\t\tesac\n+\t\t;;\n \t-c|--config*)\n-\t case \"$#,$1\" in\n+\t\tcase \"$#,$1\" in\n \t\t*,*=*)\n-\t\t conf=`expr \"z$1\" : 'z-[^=]*=\\(.*\\)'`\n-\t\t ;;\n+\t\t\tconf=`expr \"z$1\" : 'z-[^=]*=\\(.*\\)'`\n+\t\t\t;;\n \t\t1,*)\n-\t\t usage ;;\n+\t\t\tusage ;;\n \t\t*)\n-\t\t conf=\"$2\"\n-\t\t shift ;;\n-\t esac\n-\t ;;\n+\t\t\tconf=\"$2\"\n+\t\t\tshift ;;\n+\t\tesac\n+\t\t;;\n \t--)\n-\t break\n-\t ;;\n+\t\tbreak\n+\t\t;;\n \t-*)\n-\t usage\n-\t ;;\n+\t\tusage\n+\t\t;;\n \t*)\n-\t break\n-\t ;;\n- esac\n- shift\n+\t\tbreak\n+\t\t;;\n+\tesac\n+\tshift\n done\n \n test $# = 0 && usage\n \n if test -z \"$browser\"\n then\n- for opt in \"$conf\" \"web.browser\"\n- do\n-\ttest -z \"$opt\" && continue\n-\tbrowser=\"`git config $opt`\"\n-\ttest -z \"$browser\" || break\n- done\n- if test -n \"$browser\" && ! valid_tool \"$browser\"; then\n-\techo >&2 \"git config option $opt set to unknown browser: $browser\"\n-\techo >&2 \"Resetting to default...\"\n-\tunset browser\n- fi\n+\tfor opt in \"$conf\" \"web.browser\"\n+\tdo\n+\t\ttest -z \"$opt\" && continue\n+\t\tbrowser=\"`git config $opt`\"\n+\t\ttest -z \"$browser\" || break\n+\tdone\n+\tif test -n \"$browser\" && ! valid_tool \"$browser\"; then\n+\t\techo >&2 \"git config option $opt set to unknown browser: $browser\"\n+\t\techo >&2 \"Resetting to default...\"\n+\t\tunset browser\n+\tfi\n fi\n \n if test -z \"$browser\" ; then\n- if test -n \"$DISPLAY\"; then\n-\tbrowser_candidates=\"firefox iceweasel google-chrome chrome chromium konqueror w3m links lynx dillo\"\n-\tif test \"$KDE_FULL_SESSION\" = \"true\"; then\n-\t browser_candidates=\"konqueror $browser_candidates\"\n+\tif test -n \"$DISPLAY\"; then\n+\t\tbrowser_candidates=\"firefox iceweasel google-chrome chrome chromium konqueror w3m links lynx dillo\"\n+\t\tif test \"$KDE_FULL_SESSION\" = \"true\"; then\n+\t\t\tbrowser_candidates=\"konqueror $browser_candidates\"\n+\t\tfi\n+\telse\n+\t\tbrowser_candidates=\"w3m links lynx\"\n \tfi\n- else\n-\tbrowser_candidates=\"w3m links lynx\"\n- fi\n- # SECURITYSESSIONID indicates an OS X GUI login session\n- if test -n \"$SECURITYSESSIONID\" \\\n-\t -o \"$TERM_PROGRAM\" = \"Apple_Terminal\" ; then\n-\tbrowser_candidates=\"open $browser_candidates\"\n- fi\n- # /bin/start indicates MinGW\n- if test -x /bin/start; then\n-\tbrowser_candidates=\"start $browser_candidates\"\n- fi\n-\n- for i in $browser_candidates; do\n-\tinit_browser_path $i\n-\tif type \"$browser_path\" > /dev/null 2>&1; then\n-\t browser=$i\n-\t break\n+\t# SECURITYSESSIONID indicates an OS X GUI login session\n+\tif test -n \"$SECURITYSESSIONID\" \\\n+\t\t-o \"$TERM_PROGRAM\" = \"Apple_Terminal\" ; then\n+\t\tbrowser_candidates=\"open $browser_candidates\"\n \tfi\n- done\n- test -z \"$browser\" && die \"No known browser available.\"\n+\t# /bin/start indicates MinGW\n+\tif test -x /bin/start; then\n+\t\tbrowser_candidates=\"start $browser_candidates\"\n+\tfi\n+\n+\tfor i in $browser_candidates; do\n+\t\tinit_browser_path $i\n+\t\tif type \"$browser_path\" > /dev/null 2>&1; then\n+\t\t\tbrowser=$i\n+\t\t\tbreak\n+\t\tfi\n+\tdone\n+\ttest -z \"$browser\" && die \"No known browser available.\"\n else\n- valid_tool \"$browser\" || die \"Unknown browser '$browser'.\"\n+\tvalid_tool \"$browser\" || die \"Unknown browser '$browser'.\"\n \n- init_browser_path \"$browser\"\n+\tinit_browser_path \"$browser\"\n \n- if test -z \"$browser_cmd\" && ! type \"$browser_path\" > /dev/null 2>&1; then\n-\tdie \"The browser $browser is not available as '$browser_path'.\"\n- fi\n+\tif test -z \"$browser_cmd\" && ! type \"$browser_path\" > /dev/null 2>&1; then\n+\t\tdie \"The browser $browser is not available as '$browser_path'.\"\n+\tfi\n fi\n \n case \"$browser\" in\n- firefox|iceweasel)\n+firefox|iceweasel)\n \t# Check version because firefox < 2.0 does not support \"-new-tab\".\n \tvers=$(expr \"$($browser_path -version)\" : '.* \\([0-9][0-9]*\\)\\..*')\n \tNEWTAB='-new-tab'\n \ttest \"$vers\" -lt 2 && NEWTAB=''\n \t\"$browser_path\" $NEWTAB \"$@\" &\n \t;;\n- google-chrome|chrome|chromium)\n+google-chrome|chrome|chromium)\n \t# Actual command for chromium is chromium-browser.\n \t# No need to specify newTab. It's default in chromium\n \teval \"$browser_path\" \"$@\" &\n \t;;\n- konqueror)\n+konqueror)\n \tcase \"$(basename \"$browser_path\")\" in\n-\t konqueror)\n+\tkonqueror)\n \t\t# It's simpler to use kfmclient to open a new tab in konqueror.\n \t\tbrowser_path=\"$(echo \"$browser_path\" | sed -e 's/konqueror$/kfmclient/')\"\n \t\ttype \"$browser_path\" > /dev/null 2>&1 || die \"No '$browser_path' found.\"\n \t\teval \"$browser_path\" newTab \"$@\"\n \t\t;;\n-\t kfmclient)\n+\tkfmclient)\n \t\teval \"$browser_path\" newTab \"$@\"\n \t\t;;\n-\t *)\n+\t*)\n \t\t\"$browser_path\" \"$@\" &\n \t\t;;\n \tesac\n \t;;\n- w3m|links|lynx|open)\n+w3m|links|lynx|open)\n \teval \"$browser_path\" \"$@\"\n \t;;\n- start)\n- exec \"$browser_path\" '\"web-browse\"' \"$@\"\n- ;;\n- dillo)\n+start)\n+\texec \"$browser_path\" '\"web-browse\"' \"$@\"\n+\t;;\n+dillo)\n \t\"$browser_path\" \"$@\" &\n \t;;\n- *)\n+*)\n \tif test -n \"$browser_cmd\"; then\n-\t ( eval $browser_cmd \"$@\" )\n+\t\t( eval $browser_cmd \"$@\" )\n \tfi\n \t;;\n esac\n-- \n1.7.3.2.664.g294b8.dirty\n"},{"id":"157218","messageId":"1291394861-11989-4-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25938","inReplyTo":"1291394861-11989-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv2 3/7] web--browse: split valid_tool list","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-12-03T16:47:37Z","receivedAt":"2010-12-03T16:47:37Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"It was getting too long, and we want to add some more.\n\nSigned-off-by: Giuseppe Bilotta \n---\n git-web--browse.sh | 3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/git-web--browse.sh b/git-web--browse.sh\nindex 7c4568f..e48e30d 100755\n--- a/git-web--browse.sh\n+++ b/git-web--browse.sh\n@@ -31,7 +31,8 @@ valid_custom_tool()\n \n valid_tool() {\n \tcase \"$1\" in\n-\tfirefox | iceweasel | chrome | google-chrome | chromium | konqueror | w3m | links | lynx | dillo | open | start)\n+\tfirefox | iceweasel | chrome | google-chrome | chromium |\\\n+\tkonqueror | w3m | links | lynx | dillo | open | start)\n \t\t;; # happy\n \t*)\n \t\tvalid_custom_tool \"$1\" || return 1\n-- \n1.7.3.2.664.g294b8.dirty\n"},{"id":"157222","messageId":"1291394861-11989-5-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25938","inReplyTo":"1291394861-11989-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv2 4/7] web--browse: support opera, seamonkey and elinks","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-12-03T16:47:38Z","receivedAt":"2010-12-03T16:47:38Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"The list of supported browsers is also updated in the documentation.\n\nSigned-off-by: Giuseppe Bilotta \n---\n Documentation/git-web--browse.txt | 6 ++++++\n git-web--browse.sh | 14 +++++++-------\n 2 files changed, 13 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-web--browse.txt b/Documentation/git-web--browse.txt\nindex 51e8e0a..5d3ae07 100644\n--- a/Documentation/git-web--browse.txt\n+++ b/Documentation/git-web--browse.txt\n@@ -20,8 +20,14 @@ The following browsers (or commands) are currently supported:\n \n * firefox (this is the default under X Window when not using KDE)\n * iceweasel\n+* seamonkey\n+* iceape\n+* chromium\n+* google-chrome\n * konqueror (this is the default under KDE, see 'Note about konqueror' below)\n+* opera\n * w3m (this is the default outside graphical environments)\n+* elinks\n * links\n * lynx\n * dillo\ndiff --git a/git-web--browse.sh b/git-web--browse.sh\nindex e48e30d..48e5a28 100755\n--- a/git-web--browse.sh\n+++ b/git-web--browse.sh\n@@ -31,8 +31,8 @@ valid_custom_tool()\n \n valid_tool() {\n \tcase \"$1\" in\n-\tfirefox | iceweasel | chrome | google-chrome | chromium |\\\n-\tkonqueror | w3m | links | lynx | dillo | open | start)\n+\tfirefox | iceweasel | seamonkey | iceape | chrome | google-chrome | chromium |\\\n+\tkonqueror | opera | w3m | elinks | links | lynx | dillo | open | start)\n \t\t;; # happy\n \t*)\n \t\tvalid_custom_tool \"$1\" || return 1\n@@ -104,12 +104,12 @@ fi\n \n if test -z \"$browser\" ; then\n \tif test -n \"$DISPLAY\"; then\n-\t\tbrowser_candidates=\"firefox iceweasel google-chrome chrome chromium konqueror w3m links lynx dillo\"\n+\t\tbrowser_candidates=\"firefox iceweasel google-chrome chrome chromium konqueror opera seamonkey iceape w3m elinks links lynx dillo\"\n \t\tif test \"$KDE_FULL_SESSION\" = \"true\"; then\n \t\t\tbrowser_candidates=\"konqueror $browser_candidates\"\n \t\tfi\n \telse\n-\t\tbrowser_candidates=\"w3m links lynx\"\n+\t\tbrowser_candidates=\"w3m elinks links lynx\"\n \tfi\n \t# SECURITYSESSIONID indicates an OS X GUI login session\n \tif test -n \"$SECURITYSESSIONID\" \\\n@@ -140,7 +140,7 @@ else\n fi\n \n case \"$browser\" in\n-firefox|iceweasel)\n+firefox|iceweasel|seamonkey|iceape)\n \t# Check version because firefox < 2.0 does not support \"-new-tab\".\n \tvers=$(expr \"$($browser_path -version)\" : '.* \\([0-9][0-9]*\\)\\..*')\n \tNEWTAB='-new-tab'\n@@ -168,13 +168,13 @@ konqueror)\n \t\t;;\n \tesac\n \t;;\n-w3m|links|lynx|open)\n+w3m|elinks|links|lynx|open)\n \teval \"$browser_path\" \"$@\"\n \t;;\n start)\n \texec \"$browser_path\" '\"web-browse\"' \"$@\"\n \t;;\n-dillo)\n+opera|dillo)\n \t\"$browser_path\" \"$@\" &\n \t;;\n *)\n-- \n1.7.3.2.664.g294b8.dirty\n"},{"id":"157220","messageId":"1291394861-11989-6-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25938","inReplyTo":"1291394861-11989-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv2 5/7] web--browse: better support for chromium","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-12-03T16:47:39Z","receivedAt":"2010-12-03T16:47:39Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Debian-based distributions, Chromium the browser is available under\nthe name chromium-browser rather than chromium, to prevent conflicts\nwith the Chromium B.S.U. game.\n\nLook for chromium-browser first when setting the path for chromium, and\nalso add chromium-browser as a supported browser name. Document the\ndual-name support, and mention the dual-name support for\n(google-)chrome too.\n\nSigned-off-by: Giuseppe Bilotta \n---\n Documentation/git-web--browse.txt | 4 ++--\n git-web--browse.sh | 11 +++++++----\n 2 files changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-web--browse.txt b/Documentation/git-web--browse.txt\nindex 5d3ae07..c0416e5 100644\n--- a/Documentation/git-web--browse.txt\n+++ b/Documentation/git-web--browse.txt\n@@ -22,8 +22,8 @@ The following browsers (or commands) are currently supported:\n * iceweasel\n * seamonkey\n * iceape\n-* chromium\n-* google-chrome\n+* chromium (also supported as chromium-browser)\n+* google-chrome (also supported as chrome)\n * konqueror (this is the default under KDE, see 'Note about konqueror' below)\n * opera\n * w3m (this is the default outside graphical environments)\ndiff --git a/git-web--browse.sh b/git-web--browse.sh\nindex 48e5a28..d0e99f5 100755\n--- a/git-web--browse.sh\n+++ b/git-web--browse.sh\n@@ -31,7 +31,8 @@ valid_custom_tool()\n \n valid_tool() {\n \tcase \"$1\" in\n-\tfirefox | iceweasel | seamonkey | iceape | chrome | google-chrome | chromium |\\\n+\tfirefox | iceweasel | seamonkey | iceape |\\\n+\tchrome | google-chrome | chromium | chromium-browser |\\\n \tkonqueror | opera | w3m | elinks | links | lynx | dillo | open | start)\n \t\t;; # happy\n \t*)\n@@ -42,6 +43,9 @@ valid_tool() {\n \n init_browser_path() {\n \tbrowser_path=$(git config \"browser.$1.path\")\n+\tif test -z \"$browser_path\" -a \"$1\" = chromium ; then\n+\t\ttype chromium-browser > /dev/null 2>&1 && browser_path=chromium-browser\n+\tfi\n \ttest -z \"$browser_path\" && browser_path=\"$1\"\n }\n \n@@ -104,7 +108,7 @@ fi\n \n if test -z \"$browser\" ; then\n \tif test -n \"$DISPLAY\"; then\n-\t\tbrowser_candidates=\"firefox iceweasel google-chrome chrome chromium konqueror opera seamonkey iceape w3m elinks links lynx dillo\"\n+\t\tbrowser_candidates=\"firefox iceweasel google-chrome chrome chromium chromium-browser konqueror opera seamonkey iceape w3m elinks links lynx dillo\"\n \t\tif test \"$KDE_FULL_SESSION\" = \"true\"; then\n \t\t\tbrowser_candidates=\"konqueror $browser_candidates\"\n \t\tfi\n@@ -147,8 +151,7 @@ firefox|iceweasel|seamonkey|iceape)\n \ttest \"$vers\" -lt 2 && NEWTAB=''\n \t\"$browser_path\" $NEWTAB \"$@\" &\n \t;;\n-google-chrome|chrome|chromium)\n-\t# Actual command for chromium is chromium-browser.\n+google-chrome|chrome|chromium|chromium-browser)\n \t# No need to specify newTab. It's default in chromium\n \teval \"$browser_path\" \"$@\" &\n \t;;\n-- \n1.7.3.2.664.g294b8.dirty\n"},{"id":"157219","messageId":"1291394861-11989-7-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25938","inReplyTo":"1291394861-11989-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv2 6/7] web--browse: use (x-)www-browser if available","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-12-03T16:47:40Z","receivedAt":"2010-12-03T16:47:40Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Debian and derivatives have an alternatives-based default browser\nconfiguration that uses the /usr/bin/gnome-www-browser,\n/usr/bin/x-www-browser and /usr/bin/www-browser symlinks.\n\nWhen no browser is selected by the user and the Debian alternatives are\navailable, try to see if they are one of our recognized selection and\nin the affermative case use it. Otherwise, warn the user about them\nbeing unsupported and move on with the previous detection logic.\n\nSigned-off-by: Giuseppe Bilotta \n---\n Documentation/git-web--browse.txt | 4 ++\n git-web--browse.sh | 59 +++++++++++++++++++++++++++++++++++--\n 2 files changed, 60 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-web--browse.txt b/Documentation/git-web--browse.txt\nindex c0416e5..de034a5 100644\n--- a/Documentation/git-web--browse.txt\n+++ b/Documentation/git-web--browse.txt\n@@ -36,6 +36,10 @@ The following browsers (or commands) are currently supported:\n \n Custom commands may also be specified.\n \n+If no default browser is specified, and /usr/bin/x-www-browser\n+(under X) or /usr/bin/www-browser is present, they are used to determine\n+the browser to use.\n+\n OPTIONS\n -------\n -b ::\ndiff --git a/git-web--browse.sh b/git-web--browse.sh\nindex d0e99f5..48ea168 100755\n--- a/git-web--browse.sh\n+++ b/git-web--browse.sh\n@@ -49,6 +49,38 @@ init_browser_path() {\n \ttest -z \"$browser_path\" && browser_path=\"$1\"\n }\n \n+# check if a given executable is a browser we like\n+valid_exe() {\n+\ttestexe=\"$1\"\n+\tbasename=$(basename $(readlink -f \"$testexe\"))\n+\tif valid_tool \"$basename\" ; then\n+\t\tbrowser=\"$basename\"\n+\t\tbrowser_path=\"$testexe\"\n+\t\treturn 0\n+\tfi\n+\n+\t# if the linked executable doesn't match a browser name,\n+\t# look at the version string\n+\tverstring=\"$(\"$testexe\" --version 2> /dev/null)\"\n+\tbrowser=\"$(echo \"$verstring\" | head -n 1 | cut -f1 -d' ' | tr A-Z a-z)\"\n+\tcase \"$browser\" in\n+\t\tmozilla)\n+\t\t\tbrowser=\"$(echo \"$verstring\" | head -n 1 | cut -f2 -d' ' | tr A-Z a-z)\"\n+\t\t\t;;\n+\t\tgoogle)\n+\t\t\tbrowser=\"google-chrome\"\n+\t\t\t;;\n+\tesac\n+\tif valid_tool \"$browser\" ; then\n+\t\tbrowser_path=\"$i\"\n+\t\treturn 0\n+\tfi\n+\n+\techo >&2 \"$basename ($browser) is not a supported browser, skipping\"\n+\tbrowser=\"\"\n+\treturn 1\n+}\n+\n while test $# != 0\n do\n \tcase \"$1\" in\n@@ -106,6 +138,26 @@ then\n \tfi\n fi\n \n+# Debian and derivatives use gnome-www-browser, x-www-browser or www-browser to\n+# set the default browser for the system. If the user did not specify a tool and\n+# we detect that one of the *www-browser links to a supported one, we pick it.\n+# Otherwise, we warn the user about them being unsupported and proceed to look\n+# for a supported browser.\n+if test -z \"$browser\" ; then\n+\twwwbrowser=\"/usr/bin/www-browser\"\n+\tif test -n \"$DISPLAY\"; then\n+\t\twwwbrowser=\"/usr/bin/x-www-browser $wwwbrowser\"\n+\t\tif test -n \"$GNOME_DESKTOP_SESSION_ID\"; then\n+\t\t\twwwbrowser=\"/usr/bin/gnome-www-browser $wwwbrowser\"\n+\t\tfi\n+\tfi\n+\tfor i in $wwwbrowser; do\n+\t\tif test -x $i && valid_exe $i ; then\n+\t\t\tbreak\n+\t\tfi\n+\tdone\n+fi\n+\n if test -z \"$browser\" ; then\n \tif test -n \"$DISPLAY\"; then\n \t\tbrowser_candidates=\"firefox iceweasel google-chrome chrome chromium chromium-browser konqueror opera seamonkey iceape w3m elinks links lynx dillo\"\n@@ -133,7 +185,7 @@ if test -z \"$browser\" ; then\n \t\tfi\n \tdone\n \ttest -z \"$browser\" && die \"No known browser available.\"\n-else\n+else if test -z \"$browser_path\"; then\n \tvalid_tool \"$browser\" || die \"Unknown browser '$browser'.\"\n \n \tinit_browser_path \"$browser\"\n@@ -141,12 +193,13 @@ else\n \tif test -z \"$browser_cmd\" && ! type \"$browser_path\" > /dev/null 2>&1; then\n \t\tdie \"The browser $browser is not available as '$browser_path'.\"\n \tfi\n-fi\n+fi fi\n \n case \"$browser\" in\n firefox|iceweasel|seamonkey|iceape)\n \t# Check version because firefox < 2.0 does not support \"-new-tab\".\n-\tvers=$(expr \"$($browser_path -version)\" : '.* \\([0-9][0-9]*\\)\\..*')\n+\ttest -z \"$verstring\" && verstring=\"$($browser_path -version)\"\n+\tvers=$(expr \"$verstring\" : '.* \\([0-9][0-9]*\\)\\..*')\n \tNEWTAB='-new-tab'\n \ttest \"$vers\" -lt 2 && NEWTAB=''\n \t\"$browser_path\" $NEWTAB \"$@\" &\n-- \n1.7.3.2.664.g294b8.dirty\n"},{"id":"157221","messageId":"1291394861-11989-8-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25938","inReplyTo":"1291394861-11989-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv2 7/7] web--browse: look at the BROWSER env var","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-12-03T16:47:41Z","receivedAt":"2010-12-03T16:47:41Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"The BROWSER environment variables is used in Debian-based systems to set\nthe user-preferred browser(s).\n\nIt contains a colon-separate list of commands to (try to) execute\nto open a web page. Each item in the list is allowed to have a %s\nplaceholder to be replaced by the URL, in which case we try to run the\ncommand as is. If no placeholder is found, we only look at the command\nname to see if it matches one of our known browsers.\n\nSigned-off-by: Giuseppe Bilotta \n---\n git-web--browse.sh | 45 +++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 45 insertions(+), 0 deletions(-)\n\ndiff --git a/git-web--browse.sh b/git-web--browse.sh\nindex 48ea168..e3dbbe3 100755\n--- a/git-web--browse.sh\n+++ b/git-web--browse.sh\n@@ -138,6 +138,51 @@ then\n \tfi\n fi\n \n+# Debian and derivatives provide a sensible-brower script that tries to\n+# invoke the user or system preferred browser most appropriate for the\n+# current situation (X or no X, GNOME or other/no DE).\n+# We do not use the sensible-browser script directly since it doesn't\n+# allow us to open pages in a new tab, but we can use the underlying\n+# infrastructure to find the browser to use if the user didn't choose\n+# one. This is done by looking at the BROWSER environment variable\n+# first, and at the *www-browser links if the first search is\n+# unsuccesful.\n+\n+# The BROWSER environment variable is a colon-separate list of commands\n+# to (try and) execute to launch the browser. sensible-browser allows\n+# each BROWSER entry to contain a %s placeholder that will be replaced\n+# by the URL to be opened.\n+# If an entry contains a %s we run it as-is, without doing any detection, on\n+# the premise that it represents the exact way the user expects the browser to\n+# be called. If the execution fails, we do not bail out, since the\n+# failure might be due to the entry being for a graphical browser and\n+# the GUI not being available, which is the reason why multiple entries\n+# can be specified in BROWSER in the first place.\n+# An entry without a %s is only taken as indication of the preferred\n+# browser, so we proceed with our usual detection logic.\n+if test -z \"$browser\" -a -n \"$BROWSER\"; then\n+\tOLDIFS=\"$IFS\"\n+\tIFS=:\n+\tfor i in $BROWSER; do\n+\t\tcase \"$i\" in\n+\t\t\t*sensible-browser*)\n+\t\t\t\t;; # skip\n+\t\t\t*%s*)\n+\t\t\t\tIFS=\"$OLDIFS\"\n+\t\t\t\tcmd=$(printf \"$i\\n\" \"$*\")\n+\t\t\t\t$cmd && exit 0\n+\t\t\t\t;;\n+\t\t\t*)\n+\t\t\t\tprog=$(which \"$i\" 2> /dev/null)\n+\t\t\t\tif test -n \"$prog\" -a -x \"$prog\" && valid_exe \"$prog\" ; then\n+\t\t\t\t\tbreak\n+\t\t\t\tfi\n+\t\t\t\t;;\n+\t\tesac\n+\tdone\n+\tIFS=\"$OLDIFS\"\n+fi\n+\n # Debian and derivatives use gnome-www-browser, x-www-browser or www-browser to\n # set the default browser for the system. If the user did not specify a tool and\n # we detect that one of the *www-browser links to a supported one, we pick it.\n-- \n1.7.3.2.664.g294b8.dirty\n"},{"id":"157223","messageId":"20101203170800.GE12910@burratino","threadId":"25938","inReplyTo":"1291394861-11989-8-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv2 7/7] web--browse: look at the BROWSER env var","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-03T17:08:00Z","receivedAt":"2010-12-03T17:08:00Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Giuseppe Bilotta wrote:\n\n> --- a/git-web--browse.sh\n> +++ b/git-web--browse.sh\n> @@ -138,6 +138,51 @@ then\n[...]\n> +if test -z \"$browser\" -a -n \"$BROWSER\"; then\n> +\tOLDIFS=\"$IFS\"\n> +\tIFS=:\n[...]\n> +\tIFS=\"$OLDIFS\"\n\nMicronit: on some shells (e.g., old ash[1]), IFS starts out unset if\nit hasn't been inherited in the environment. How about something like\nthis?\n\n\tLF=\"\n\"\n\tusual_ifs=\" \t$LF\"\n\tIFS=:\n\t...\n\t\t\t\tIFS=$usual_ifs\n\t...\n\tIFS=$usual_ifs\n\nBased on [2] I would guess that any POSIX-style shell would also\nsupport\n\n\tIFS=:\n\t...\n\t\t\t\tunset IFS\n\t...\n\tunset IFS\n\nbut maybe that's playing with fire.\n\nThe rest of this series looks quite nice, so\nAcked-by: Jonathan Nieder \n\n[1] http://bugs.debian.org/95856\n[2] http://www.in-ulm.de/~mascheck/various/ifs/\n"},{"id":"157224","messageId":"20101203171628.GA13721@burratino","threadId":"25938","inReplyTo":"20101203170800.GE12910@burratino","subject":"Re: [PATCHv2 7/7] web--browse: look at the BROWSER env var","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-03T17:16:28Z","receivedAt":"2010-12-03T17:16:28Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n> Giuseppe Bilotta wrote:\n\n>> --- a/git-web--browse.sh\n>> +++ b/git-web--browse.sh\n>> @@ -138,6 +138,51 @@ then\n>[...]\n>> +if test -z \"$browser\" -a -n \"$BROWSER\"; then\n>> +\tOLDIFS=\"$IFS\"\n>> +\tIFS=:\n>[...]\n>> +\tIFS=\"$OLDIFS\"\n>\n> Micronit: on some shells (e.g., old ash[1]), IFS starts out unset if\n> it hasn't been inherited in the environment.\n\nOf course, we are using this idiom in a few places already, so that\ncan be saved for another topic. Sorry for not checking sooner.\n"},{"id":"157225","messageId":"AANLkTimAdrqYezL5jf9OR_J5XdYW-KYhYy9n_VQAUSD8@mail.gmail.com","threadId":"25938","inReplyTo":"1291394861-11989-7-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv2 6/7] web--browse: use (x-)www-browser if available","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2010-12-03T17:40:28Z","receivedAt":"2010-12-03T17:40:28Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi,\n\nOn Fri, Dec 3, 2010 at 5:47 PM, Giuseppe Bilotta\n wrote:\n> Debian and derivatives have an alternatives-based default browser\n> configuration that uses the /usr/bin/gnome-www-browser,\n> /usr/bin/x-www-browser and /usr/bin/www-browser symlinks.\n>\n> When no browser is selected by the user and the Debian alternatives are\n> available, try to see if they are one of our recognized selection and\n> in the affermative case use it. Otherwise, warn the user about them\n> being unsupported and move on with the previous detection logic.\n>\n> Signed-off-by: Giuseppe Bilotta \n> ---\n>  Documentation/git-web--browse.txt |    4 ++\n>  git-web--browse.sh                |   59 +++++++++++++++++++++++++++++++++++--\n>  2 files changed, 60 insertions(+), 3 deletions(-)\n>\n> diff --git a/Documentation/git-web--browse.txt b/Documentation/git-web--browse.txt\n> index c0416e5..de034a5 100644\n> --- a/Documentation/git-web--browse.txt\n> +++ b/Documentation/git-web--browse.txt\n> @@ -36,6 +36,10 @@ The following browsers (or commands) are currently supported:\n>\n>  Custom commands may also be specified.\n>\n> +If no default browser is specified, and /usr/bin/x-www-browser\n> +(under X) or /usr/bin/www-browser is present, they are used to determine\n> +the browser to use.\n\nIt looks like /usr/bin/gnome-www-browser is missing.\n\n> +\n>  OPTIONS\n>  -------\n>  -b ::\n> diff --git a/git-web--browse.sh b/git-web--browse.sh\n> index d0e99f5..48ea168 100755\n> --- a/git-web--browse.sh\n> +++ b/git-web--browse.sh\n> @@ -49,6 +49,38 @@ init_browser_path() {\n>        test -z \"$browser_path\" && browser_path=\"$1\"\n>  }\n>\n> +# check if a given executable is a browser we like\n> +valid_exe() {\n> +       testexe=\"$1\"\n> +       basename=$(basename $(readlink -f \"$testexe\"))\n> +       if valid_tool \"$basename\" ; then\n> +               browser=\"$basename\"\n> +               browser_path=\"$testexe\"\n> +               return 0\n> +       fi\n> +\n> +       # if the linked executable doesn't match a browser name,\n> +       # look at the version string\n> +       verstring=\"$(\"$testexe\" --version 2> /dev/null)\"\n> +       browser=\"$(echo \"$verstring\" | head -n 1 | cut -f1 -d' ' | tr A-Z a-z)\"\n> +       case \"$browser\" in\n> +               mozilla)\n> +                       browser=\"$(echo \"$verstring\" | head -n 1 | cut -f2 -d' ' | tr A-Z a-z)\"\n> +                       ;;\n> +               google)\n> +                       browser=\"google-chrome\"\n> +                       ;;\n> +       esac\n> +       if valid_tool \"$browser\" ; then\n> +               browser_path=\"$i\"\n> +               return 0\n> +       fi\n> +\n> +       echo >&2 \"$basename ($browser) is not a supported browser, skipping\"\n\nWhy not:\n\necho >&2 \"$basename (from $testexe) is not a supported browser, skipping\"\n\n?\n\nOtherwise we might get something like:\n\n\"newbrowser (newbrowser) is not a supported browser, skipping\"\n\n> +       browser=\"\"\n> +       return 1\n> +}\n> +\n>  while test $# != 0\n>  do\n>        case \"$1\" in\n> @@ -106,6 +138,26 @@ then\n>        fi\n>  fi\n>\n> +# Debian and derivatives use gnome-www-browser, x-www-browser or www-browser to\n> +# set the default browser for the system. If the user did not specify a tool and\n> +# we detect that one of the *www-browser links to a supported one, we pick it.\n> +# Otherwise, we warn the user about them being unsupported and proceed to look\n> +# for a supported browser.\n> +if test -z \"$browser\" ; then\n> +       wwwbrowser=\"/usr/bin/www-browser\"\n> +       if test -n \"$DISPLAY\"; then\n> +               wwwbrowser=\"/usr/bin/x-www-browser $wwwbrowser\"\n> +               if test -n \"$GNOME_DESKTOP_SESSION_ID\"; then\n> +                       wwwbrowser=\"/usr/bin/gnome-www-browser $wwwbrowser\"\n> +               fi\n> +       fi\n> +       for i in $wwwbrowser; do\n> +               if test -x $i && valid_exe $i ; then\n> +                       break\n> +               fi\n> +       done\n> +fi\n> +\n>  if test -z \"$browser\" ; then\n>        if test -n \"$DISPLAY\"; then\n>                browser_candidates=\"firefox iceweasel google-chrome chrome chromium chromium-browser konqueror opera seamonkey iceape w3m elinks links lynx dillo\"\n> @@ -133,7 +185,7 @@ if test -z \"$browser\" ; then\n>                fi\n>        done\n>        test -z \"$browser\" && die \"No known browser available.\"\n> -else\n> +else if test -z \"$browser_path\"; then\n\nNow that you reset $browser above, I am not sure this test -z\n\"$browser_path\" is useful...\n\nThanks,\nChristian.\n"},{"id":"157229","messageId":"AANLkTik=pGND-EYQ2HYTmevVd5S9pXZBNSpDsnafOF2V@mail.gmail.com","threadId":"25938","inReplyTo":"AANLkTimAdrqYezL5jf9OR_J5XdYW-KYhYy9n_VQAUSD8@mail.gmail.com","subject":"Re: [PATCHv2 6/7] web--browse: use (x-)www-browser if available","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-12-03T17:57:37Z","receivedAt":"2010-12-03T17:57:37Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Fri, Dec 3, 2010 at 6:40 PM, Christian Couder\n wrote:\n> Hi,\n>\n>>  Custom commands may also be specified.\n>>\n>> +If no default browser is specified, and /usr/bin/x-www-browser\n>> +(under X) or /usr/bin/www-browser is present, they are used to determine\n>> +the browser to use.\n>\n> It looks like /usr/bin/gnome-www-browser is missing.\n\nGood catch. I'll add it.\n\n>> +       echo >&2 \"$basename ($browser) is not a supported browser, skipping\"\n>\n> Why not:\n>\n> echo >&2 \"$basename (from $testexe) is not a supported browser, skipping\"\n>\n> ?\n>\n> Otherwise we might get something like:\n>\n> \"newbrowser (newbrowser) is not a supported browser, skipping\"\n\nI rephrased it to:\n\n\techo >&2 \"$testexe (detected as $browser) is not a supported browser, skipping\"\n\n>> @@ -133,7 +185,7 @@ if test -z \"$browser\" ; then\n>>                fi\n>>        done\n>>        test -z \"$browser\" && die \"No known browser available.\"\n>> -else\n>> +else if test -z \"$browser_path\"; then\n>\n> Now that you reset $browser above, I am not sure this test -z\n> \"$browser_path\" is useful...\n\nI believe it is. If the *www-browser detection was successful,\n_that's_ what we want to launch, so we want to skip the\ninit_browser_path in that else section, which we would hit if we\ndidn't check for $browser_path. We also don't want to repeat the\nvalid_tool check, since we know it's valid already.\n\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"157270","messageId":"7vsjyemt1f.fsf@alter.siamese.dyndns.org","threadId":"25938","inReplyTo":"1291394861-11989-2-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv2 1/7] CodingGuidelines: mention whitespace preferences for shell scripts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-03T21:43:24Z","receivedAt":"2010-12-03T21:43:24Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Giuseppe Bilotta writes:\n\n> Signed-off-by: Giuseppe Bilotta \n> ---\n> Documentation/CodingGuidelines | 4 ++++\n> 1 files changed, 4 insertions(+), 0 deletions(-)\n>\n> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\n> index 5aa2d34..a9191d0 100644\n> --- a/Documentation/CodingGuidelines\n> +++ b/Documentation/CodingGuidelines\n> @@ -31,6 +31,10 @@ But if you must have a list of rules, here they are.\n> \n> For shell scripts specifically (not exhaustive):\n> \n> + - We use tabs for indentation.\n> +\n> + - Case arms are not indented with respect to the case and esac lines.\n\nThanks.\nI am tempted to rephrase the latter as:\n\n Case arms are indented at the same depth as case and esac lines.\n\nIt makes it less hard to read and understand without negation.\n"},{"id":"157273","messageId":"7vipzamsde.fsf@alter.siamese.dyndns.org","threadId":"25938","inReplyTo":"1291394861-11989-6-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv2 5/7] web--browse: better support for chromium","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-03T21:57:49Z","receivedAt":"2010-12-03T21:57:49Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Giuseppe Bilotta writes:\n\n> init_browser_path() {\n> \tbrowser_path=$(git config \"browser.$1.path\")\n> +\tif test -z \"$browser_path\" -a \"$1\" = chromium ; then\n> +\t\ttype chromium-browser > /dev/null 2>&1 && browser_path=chromium-browser\n> +\tfi\n> \ttest -z \"$browser_path\" && browser_path=\"$1\"\n\nWe tolerate\n\n\ttest && test && effect\n\nand even encourage when the construct is short enough, over\n\n\tif test && test\n then\n \teffect\n\tfi\n\nBut because you are writing an \"if\" block anyway, I think the above should\nbe like this:\n\n\tif test -z \"$browser_path\" &&\n test \"$1\" = chromium &&\n type chromium-browser >/dev/null 2>&1\n\tthen\n \tbrowser_path=chromium-browser\n\tfi\n\tbrowser_path=${browser_path:-\"$1\"}\n\nYours is:\n\n\tif test && test\n then\n \ttest && effect\n\tfi\n\nwhich is less than readable, no?\n"},{"id":"157274","messageId":"7vei9yms6u.fsf@alter.siamese.dyndns.org","threadId":"25938","inReplyTo":"1291394861-11989-5-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv2 4/7] web--browse: support opera, seamonkey and elinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-03T22:01:45Z","receivedAt":"2010-12-03T22:01:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Giuseppe Bilotta writes:\n\n> -w3m|links|lynx|open)\n> +w3m|elinks|links|lynx|open)\n> \teval \"$browser_path\" \"$@\"\n> \t;;\n> start)\n> \texec \"$browser_path\" '\"web-browse\"' \"$@\"\n> \t;;\n> -dillo)\n> +opera|dillo)\n> \t\"$browser_path\" \"$@\" &\n> \t;;\n\nNot a complaint on your patch, but is there a reason we say \"eval\", \"exec\"\nand \"(nothing)\" in these three case arms?\n\nThe above makes the interpretation of $browser_path and $@ inconsistent\nbetween lynx family codepath (which would apply $IFS to find the browser)\nand dillo and start family codepath (which would not), and I am wondering\nif that difference is intended.\n"},{"id":"157275","messageId":"AANLkTi=PmGR23AMog4UWQVNKPWrDUM_kYWV_Bn1vvSwE@mail.gmail.com","threadId":"25938","inReplyTo":"7vsjyemt1f.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2 1/7] CodingGuidelines: mention whitespace preferences for shell scripts","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-12-03T22:02:03Z","receivedAt":"2010-12-03T22:02:03Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Fri, Dec 3, 2010 at 10:43 PM, Junio C Hamano wrote:\n> Giuseppe Bilotta writes:\n>\n>> Signed-off-by: Giuseppe Bilotta \n>> ---\n>>  Documentation/CodingGuidelines |    4 ++++\n>>  1 files changed, 4 insertions(+), 0 deletions(-)\n>>\n>> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\n>> index 5aa2d34..a9191d0 100644\n>> --- a/Documentation/CodingGuidelines\n>> +++ b/Documentation/CodingGuidelines\n>> @@ -31,6 +31,10 @@ But if you must have a list of rules, here they are.\n>>\n>>  For shell scripts specifically (not exhaustive):\n>>\n>> + - We use tabs for indentation.\n>> +\n>> + - Case arms are not indented with respect to the case and esac lines.\n>\n> Thanks.\n> I am tempted to rephrase the latter as:\n>\n>    Case arms are indented at the same depth as case and esac lines.\n>\n> It makes it less hard to read and understand without negation.\n\nGood idea. I'll rephrase in that sense and add a couple more entries\nabout the | spacing and long case arm splitting, for the next rehash\nof the series.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"157276","messageId":"7vaakmmrkj.fsf@alter.siamese.dyndns.org","threadId":"25938","inReplyTo":"1291394861-11989-7-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv2 6/7] web--browse: use (x-)www-browser if available","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-03T22:15:08Z","receivedAt":"2010-12-03T22:15:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Giuseppe Bilotta writes:\n\n> Debian and derivatives have an alternatives-based default browser\n> configuration that uses the /usr/bin/gnome-www-browser,\n> /usr/bin/x-www-browser and /usr/bin/www-browser symlinks.\n>\n> When no browser is selected by the user and the Debian alternatives are\n> available, try to see if they are one of our recognized selection and\n> in the affermative case use it. Otherwise, warn the user about them\n> being unsupported and move on with the previous detection logic.\n\nA \"please step back a bit\" question. Does the packaging guideline of\nDebian say that non-browser applications should take these links as \"end\nuser preference\" when opening HTML pages?\n\nThe behaviour of unconfigured git across platforms would become less\nconsistent if we do this, while the behaviour of random programs on one\nparticular platform (Debian) would become more consistent.\n\nI am not saying that is necessarily a bad thing. I just want to\nunderstand the motivation.\n\n> +# check if a given executable is a browser we like\n> +valid_exe() {\n\nCall it valid_browser_executable or something, please.\n\n> +\ttestexe=\"$1\"\n> +\tbasename=$(basename $(readlink -f \"$testexe\"))\n\nAre we saying \"readlink\" must exist on the system? This dependency is\nnew, I think.\n"},{"id":"157278","messageId":"AANLkTi=DdQd5MgU2bp5sb5b5t1bzXpwf8WSJ=boSbc5q@mail.gmail.com","threadId":"25938","inReplyTo":"7vei9yms6u.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2 4/7] web--browse: support opera, seamonkey and elinks","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-12-03T22:22:08Z","receivedAt":"2010-12-03T22:22:08Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Fri, Dec 3, 2010 at 11:01 PM, Junio C Hamano wrote:\n> Giuseppe Bilotta writes:\n>\n>> -w3m|links|lynx|open)\n>> +w3m|elinks|links|lynx|open)\n>>       eval \"$browser_path\" \"$@\"\n>>       ;;\n>>  start)\n>>       exec \"$browser_path\" '\"web-browse\"' \"$@\"\n>>       ;;\n>> -dillo)\n>> +opera|dillo)\n>>       \"$browser_path\" \"$@\" &\n>>       ;;\n>\n> Not a complaint on your patch, but is there a reason we say \"eval\", \"exec\"\n> and \"(nothing)\" in these three case arms?\n>\n> The above makes the interpretation of $browser_path and $@ inconsistent\n> between lynx family codepath (which would apply $IFS to find the browser)\n> and dillo and start family codepath (which would not), and I am wondering\n> if that difference is intended.\n\nI was wondering about this myself, but since I couldn't come up with\nan explanation I opted for leaving things as they were. Lacking an\nexplanation I can also provide a patch to standardize invocation.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"157279","messageId":"AANLkTinGzZQZo9yZtVtztu5CEXT8G10H6=EdbSM3PAwh@mail.gmail.com","threadId":"25938","inReplyTo":"7vipzamsde.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2 5/7] web--browse: better support for chromium","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-12-03T22:25:09Z","receivedAt":"2010-12-03T22:25:09Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Fri, Dec 3, 2010 at 10:57 PM, Junio C Hamano wrote:\n> Giuseppe Bilotta writes:\n>\n>>  init_browser_path() {\n>>       browser_path=$(git config \"browser.$1.path\")\n>> +     if test -z \"$browser_path\" -a \"$1\" = chromium ; then\n>> +             type chromium-browser > /dev/null 2>&1 && browser_path=chromium-browser\n>> +     fi\n>>       test -z \"$browser_path\" && browser_path=\"$1\"\n>\n> We tolerate\n>\n>        test && test && effect\n>\n> and even encourage when the construct is short enough, over\n>\n>        if test && test\n>        then\n>                effect\n>        fi\n>\n> But because you are writing an \"if\" block anyway, I think the above should\n> be like this:\n>\n>        if test -z \"$browser_path\" &&\n>           test \"$1\" = chromium &&\n>           type chromium-browser >/dev/null 2>&1\n>        then\n>                browser_path=chromium-browser\n>        fi\n>        browser_path=${browser_path:-\"$1\"}\n>\n> Yours is:\n>\n>        if test && test\n>        then\n>                test && effect\n>        fi\n>\n> which is less than readable, no?\n\nAbsolutely. Actually, this final form is a side-effect on some\nexperimentations I was doing, thinking about doing something similar\nwith google-chrome vs chrome (in which case the body of the if would\nget a case switch or something like that). I'll clean it up, unless\nsomebody can actually confirm that we do want these other special\ncases.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"157280","messageId":"7v39qemqyc.fsf@alter.siamese.dyndns.org","threadId":"25938","inReplyTo":"AANLkTi=PmGR23AMog4UWQVNKPWrDUM_kYWV_Bn1vvSwE@mail.gmail.com","subject":"Re: [PATCHv2 1/7] CodingGuidelines: mention whitespace preferences for shell scripts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-03T22:28:27Z","receivedAt":"2010-12-03T22:28:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Giuseppe Bilotta writes:\n\n> Good idea. I'll rephrase in that sense and add a couple more entries\n> about the | spacing and long case arm splitting, for the next rehash\n> of the series.\n\nHm, I don't think there needs a rehash of the whole thing. This one is\nready for 'maint' with the rewording), and 2-4 looked Ok.\n"},{"id":"157282","messageId":"AANLkTinWD53M2VjiWgeA0Qwx3OHzR2A09Y+AB2B9o1df@mail.gmail.com","threadId":"25938","inReplyTo":"7vaakmmrkj.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2 6/7] web--browse: use (x-)www-browser if available","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-12-03T22:45:22Z","receivedAt":"2010-12-03T22:45:22Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Fri, Dec 3, 2010 at 11:15 PM, Junio C Hamano wrote:\n> Giuseppe Bilotta writes:\n>\n>> Debian and derivatives have an alternatives-based default browser\n>> configuration that uses the /usr/bin/gnome-www-browser,\n>> /usr/bin/x-www-browser and /usr/bin/www-browser symlinks.\n>>\n>> When no browser is selected by the user and the Debian alternatives are\n>> available, try to see if they are one of our recognized selection and\n>> in the affermative case use it. Otherwise, warn the user about them\n>> being unsupported and move on with the previous detection logic.\n>\n> A \"please step back a bit\" question.  Does the packaging guideline of\n> Debian say that non-browser applications should take these links as \"end\n> user preference\" when opening HTML pages?\n\nI'm not sure this is an actual guideline, and I cannot find much\nspecific information about it online. It _is_ the recommended way to\nset the default browser in Debian (plus the BROWSER override), but\nthere are quite a few applications that don't actually follow it. (And\nFWIW, as a Debian user I find this annoying, e.g. when clicking on a\nlink and having konqueror or iceweasel pop up instead of my preferred\nx-www-browser, that happens to be Opera.)\n\nI do believe that Debian encourages the use of sensible-browser (that\ndoes the BROWSER and *www-browser check itself) rather than manually\ngoing to look at those specifications. But that means giving up the\n\"do anything we can to open stuff in a new tab\" that is among the\nspecified purposes of git-web--browse.\n\n> The behaviour of unconfigured git across platforms would become less\n> consistent if we do this, while the behaviour of random programs on one\n> particular platform (Debian) would become more consistent.\n\nThat's actually the reason why I wasn't so sure it would be\nappropriate for inclusion.\n\n> I am not saying that is necessarily a bad thing.  I just want to\n> understand the motivation.\n\nThe motivation is that, lacking an explicit override, I believe git\nweb--browse should try and get the information about the preferred\nbrowser from wherever it can. In a way, the *www-browser testing is\nakin to the use of start in Windows or open under Mac OS X.\n\nOf course, as I only use Debian, I am not aware of what other\ndistributions do (if they do anything at all) to allow a user to\nspecify a preferred browser.\n\nAn alternative approach would be to get rid of the *www-browser and\nBROWSER patches, and just use xdg-open if it's available. Which again\nraises the issue of how to enforce opening the page in a new tab.\n\n>> +# check if a given executable is a browser we like\n>> +valid_exe() {\n>\n> Call it valid_browser_executable or something, please.\n\nOf course.\n\n>> +     testexe=\"$1\"\n>> +     basename=$(basename $(readlink -f \"$testexe\"))\n>\n> Are we saying \"readlink\" must exist on the system?  This dependency is\n> new, I think.\n\nThe only other use I found of readlink is in a specific area of\nt/test-lib.sh, so this seems like a new requirement indeed. I can\neither wrap that part in a type readlink check, or I can get rid of\nthat specific section of the test, and just leave the --version check,\nbringing the code back to the previous patch version.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"157283","messageId":"AANLkTinDAZ2O6CHjK-7bmtUn42+z06A3BdXfURWV6mZt@mail.gmail.com","threadId":"25938","inReplyTo":"7v39qemqyc.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2 1/7] CodingGuidelines: mention whitespace preferences for shell scripts","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-12-03T22:46:11Z","receivedAt":"2010-12-03T22:46:11Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Fri, Dec 3, 2010 at 11:28 PM, Junio C Hamano wrote:\n> Giuseppe Bilotta writes:\n>\n>> Good idea. I'll rephrase in that sense and add a couple more entries\n>> about the | spacing and long case arm splitting, for the next rehash\n>> of the series.\n>\n> Hm, I don't think there needs a rehash of the whole thing.  This one is\n> ready for 'maint' with the rewording), and 2-4 looked Ok.\n\nOk then. Do you still want the two extra entries or are they superfluous too?\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"157284","messageId":"20101204004225.GA15906@burratino","threadId":"25938","inReplyTo":"AANLkTinWD53M2VjiWgeA0Qwx3OHzR2A09Y+AB2B9o1df@mail.gmail.com","subject":"Re: [PATCHv2 6/7] web--browse: use (x-)www-browser if available","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-04T00:42:25Z","receivedAt":"2010-12-04T00:42:25Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Giuseppe Bilotta wrote:\n\n> I do believe that Debian encourages the use of sensible-browser (that\n> does the BROWSER and *www-browser check itself) rather than manually\n> going to look at those specifications.\n\nMy impression (by analogy with policy §11.4 \"Editors and pagers\") is\nthat one is encouraged to make the default configurable at compile\ntime and use\n\n - first $BROWSER\n - then something desktop-specific\n - then the configured default\n\nand set that default to www-browser or x-www-browser, depending on\nwhether your program uses X. That way, non-Debian systems benefit\nfrom the changes you introduce, too.\n\n> An alternative approach would be to get rid of the *www-browser and\n> BROWSER patches, and just use xdg-open if it's available. Which again\n> raises the issue of how to enforce opening the page in a new tab.\n\nYes, in this case that is the ideal (assuming xdg-utils has wide\nenough adoption).\n\nI think xdg-open has just as much a reason as we do to encourage\nopening the page in a new tab. Would it be hard to make that happen?\n"},{"id":"157298","messageId":"AANLkTinG3moZFNOMBwrc7UTrXuGeUourQp+6OEH5Vcgh@mail.gmail.com","threadId":"25938","inReplyTo":"20101204004225.GA15906@burratino","subject":"Re: [PATCHv2 6/7] web--browse: use (x-)www-browser if available","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-12-04T07:49:09Z","receivedAt":"2010-12-04T07:49:09Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Sat, Dec 4, 2010 at 1:42 AM, Jonathan Nieder wrote:\n> Giuseppe Bilotta wrote:\n>\n>> I do believe that Debian encourages the use of sensible-browser (that\n>> does the BROWSER and *www-browser check itself) rather than manually\n>> going to look at those specifications.\n>\n> My impression (by analogy with policy §11.4 \"Editors and pagers\") is\n> that one is encouraged to make the default configurable at compile\n> time and use\n>\n>  - first $BROWSER\n>  - then something desktop-specific\n>  - then the configured default\n>\n> and set that default to www-browser or x-www-browser, depending on\n> whether your program uses X.  That way, non-Debian systems benefit\n> from the changes you introduce, too.\n\nI'm not entirely sure how non-Debian would benefit from the change at\nthe current state, but I'm willing to present patches for the\nmechanism offered by other distributions (e.g. does anybody know how\nRedHad/Fedora sets the preferred browser?)\n\n>> An alternative approach would be to get rid of the *www-browser and\n>> BROWSER patches, and just use xdg-open if it's available. Which again\n>> raises the issue of how to enforce opening the page in a new tab.\n>\n> Yes, in this case that is the ideal (assuming xdg-utils has wide\n> enough adoption).\n>\n> I think xdg-open has just as much a reason as we do to encourage\n> opening the page in a new tab.  Would it be hard to make that happen?\n\nI have no idea. Also, it's sort-of outside the scope of our reach, and\nwe would have to wait for a change in that effect to 'trickle down'.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"157555","messageId":"7v4oapnogt.fsf@alter.siamese.dyndns.org","threadId":"25938","inReplyTo":"1291394861-11989-3-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv2 2/7] web--browse: coding style","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-07T23:38:10Z","receivedAt":"2010-12-07T23:38:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks for a patch that is easy to verify.\n\n\"git show -w\" gives an empty result ;-)\n"}]}