{"thread":{"id":"25869","subject":"[PATCH 0/6] web--browse cleanups and extensions","startedAt":"2010-11-29T14:47:49Z","lastAt":"2010-12-01T15:49:59Z","messageCount":22,"participants":["Giuseppe Bilotta","Christian Couder","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"156801","messageId":"1291042075-19983-1-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25869","inReplyTo":null,"subject":"[PATCH 0/6] web--browse cleanups and extensions","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-11-29T14:47:49Z","receivedAt":"2010-11-29T14:47:49Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"This patches introduces support for Opera, Seamonkey/Iceape and elinks\nto git web-browse, after a little reformatting to adhere to the shell\nscript coding guidelines (guidelines which were not explicit in the\ndocumentation, and which are made so).\n\nThe last two patches are somewhat more system-specific (AFAIK, only\nDebain & friends have /usr/bin/{x-,}www-browser, and not many other\ndistributions have chromium under the chromium-browser executable name),\nbut I believe it still makes sense to include them in the mainstream\nversion of git.\n\nGiuseppe Bilotta (6):\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: use (x-)www-browser if available\n web--browse: special-case chromium path\n\n Documentation/CodingGuidelines | 6 +\n Documentation/git-web--browse.txt | 10 ++\n git-web--browse.sh | 202 +++++++++++++++++++++----------------\n 3 files changed, 133 insertions(+), 85 deletions(-)\n\n-- \n1.7.3.2.624.gec6c7.dirty\n"},{"id":"156806","messageId":"1291042075-19983-2-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25869","inReplyTo":"1291042075-19983-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCH 1/6] CodingGuidelines: mention whitespace preferences for shell scripts","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-11-29T14:47:50Z","receivedAt":"2010-11-29T14:47:50Z","isPatch":true,"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 | 6 ++++++\n 1 files changed, 6 insertions(+), 0 deletions(-)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex 5aa2d34..7ecce51 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -31,6 +31,12 @@ 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+ - The choices in case ... esac statement are not indented with respect\n+ to the the case and esac lines. The body of the choices however _is_\n+ indented (by one tab).\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.624.gec6c7.dirty\n"},{"id":"156802","messageId":"1291042075-19983-3-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25869","inReplyTo":"1291042075-19983-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCH 2/6] web--browse: coding style","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-11-29T14:47:51Z","receivedAt":"2010-11-29T14:47:51Z","isPatch":true,"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.624.gec6c7.dirty\n"},{"id":"156807","messageId":"1291042075-19983-4-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25869","inReplyTo":"1291042075-19983-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCH 3/6] web--browse: split valid_tool list","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-11-29T14:47:52Z","receivedAt":"2010-11-29T14:47:52Z","isPatch":true,"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..b20b0e0 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+\t|konqueror|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.624.gec6c7.dirty\n"},{"id":"156804","messageId":"1291042075-19983-5-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25869","inReplyTo":"1291042075-19983-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCH 4/6] web--browse: support opera, seamonkey and elinks","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-11-29T14:47:53Z","receivedAt":"2010-11-29T14:47:53Z","isPatch":true,"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 b20b0e0..7b7f45f 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-\t|konqueror|w3m|links|lynx|dillo|open|start)\n+\tfirefox|iceweasel|seamonkey|iceape|chrome|google-chrome|chromium\\\n+\t|konqueror|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.624.gec6c7.dirty\n"},{"id":"156803","messageId":"1291042075-19983-6-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25869","inReplyTo":"1291042075-19983-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCH 5/6] web--browse: use (x-)www-browser if available","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-11-29T14:47:54Z","receivedAt":"2010-11-29T14:47:54Z","isPatch":true,"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/x-www-browser and\n/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.\n\nSigned-off-by: Giuseppe Bilotta \n---\n Documentation/git-web--browse.txt | 4 ++++\n git-web--browse.sh | 34 +++++++++++++++++++++++++++++++---\n 2 files changed, 35 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-web--browse.txt b/Documentation/git-web--browse.txt\nindex 5d3ae07..76b5cdc 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 7b7f45f..f991dd0 100755\n--- a/git-web--browse.sh\n+++ b/git-web--browse.sh\n@@ -102,6 +102,33 @@ then\n \tfi\n fi\n \n+# Debian and derivatives use x-www-browser or www-browser to set\n+# the default browser for the system\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+\tfi\n+\tfor i in $wwwbrowser; do\n+\t\tif test -x $i ; then\n+\t\t\tverstring=\"$($i --version 2> /dev/null | head -n 1)\"\n+\t\t\tbrowser=\"$(echo \"$verstring\" | cut -f1 -d' ' | tr A-Z a-z)\"\n+\t\t\tcase \"$browser\" in\n+\t\t\tmozilla)\n+\t\t\t\tbrowser=\"$(echo \"$verstring\" | cut -f2 -d' ' | tr A-Z a-z)\"\n+\t\t\t\t;;\n+\t\t\tgoogle)\n+\t\t\t\tbrowser=\"google-chrome\"\n+\t\t\t\t;;\n+\t\t\tesac\n+\t\t\tif valid_tool \"$browser\" ; then\n+\t\t\t\tbrowser_path=\"$i\"\n+\t\t\t\tbreak\n+\t\t\tfi\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 konqueror opera seamonkey iceape w3m elinks links lynx dillo\"\n@@ -129,7 +156,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@@ -137,12 +164,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.624.gec6c7.dirty\n"},{"id":"156805","messageId":"1291042075-19983-7-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25869","inReplyTo":"1291042075-19983-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCH 6/6] web--browse: special-case chromium path","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-11-29T14:47:55Z","receivedAt":"2010-11-29T14:47:55Z","isPatch":true,"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\nSigned-off-by: Giuseppe Bilotta \n---\n git-web--browse.sh | 3 +++\n 1 files changed, 3 insertions(+), 0 deletions(-)\n\ndiff --git a/git-web--browse.sh b/git-web--browse.sh\nindex f991dd0..9cd66e4 100755\n--- a/git-web--browse.sh\n+++ b/git-web--browse.sh\n@@ -42,6 +42,9 @@ valid_tool() {\n \n init_browser_path() {\n \tbrowser_path=$(git config \"browser.$1.path\")\n+\tif test \"$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-- \n1.7.3.2.624.gec6c7.dirty\n"},{"id":"156808","messageId":"AANLkTinGustedMv9rm+RgGMQckfxgbTPA+Yqpz7vqTfm@mail.gmail.com","threadId":"25869","inReplyTo":"1291042075-19983-6-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCH 5/6] web--browse: use (x-)www-browser if available","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2010-11-29T16:18:47Z","receivedAt":"2010-11-29T16:18:47Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi,\n\nOn Mon, Nov 29, 2010 at 3:47 PM, Giuseppe Bilotta\n wrote:\n>\n> +# Debian and derivatives use x-www-browser or www-browser to set\n> +# the default browser for the system\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> +       fi\n> +       for i in $wwwbrowser; do\n> +               if test -x $i ; then\n> +                       verstring=\"$($i --version 2> /dev/null | head -n 1)\"\n> +                       browser=\"$(echo \"$verstring\" | cut -f1 -d' ' | tr A-Z a-z)\"\n\nStupid questions:\n\nDid you check that all the browser we support accept the --version option?\nWhat if we add support for a new one that doesn't ?\nShouldn't we add something like :\n\ntest -z \"$browser\" && browser=\"$(echo $i | cut -f1 -d' ' | tr A-Z a-z)\"\n\nAnd are you sure that when they support --version, the first word of\nthe output is\nbetter than \"$(echo $i | cut -f1 -d' ' | tr A-Z a-z)\"?\nWhat if we add support for a new one?\n\n> +                       case \"$browser\" in\n> +                       mozilla)\n> +                               browser=\"$(echo \"$verstring\" | 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\nvalid_tool \"$browser\" is called once here...\n\n> +                               browser_path=\"$i\"\n> +                               break\n> +                       fi\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 konqueror opera seamonkey iceape w3m elinks links lynx dillo\"\n> @@ -129,7 +156,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>        valid_tool \"$browser\" || die \"Unknown browser '$browser'.\"\n\n...valid_tool \"$browser\" is called again here if it failed above, and\nhere we die,\nso isn't it clearer to die as soon as we call it and it fails?\n\nThanks,\nChristian.\n"},{"id":"156809","messageId":"AANLkTi=Ob=7fvZUGNBy22r4+Pz31tgA+LeY4RqSTf1KU@mail.gmail.com","threadId":"25869","inReplyTo":"AANLkTinGustedMv9rm+RgGMQckfxgbTPA+Yqpz7vqTfm@mail.gmail.com","subject":"Re: [PATCH 5/6] web--browse: use (x-)www-browser if available","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2010-11-29T16:25:58Z","receivedAt":"2010-11-29T16:25:58Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Nov 29, 2010 at 5:18 PM, Christian Couder\n wrote:\n> Hi,\n>\n> On Mon, Nov 29, 2010 at 3:47 PM, Giuseppe Bilotta\n> wrote:\n>>\n>> +# Debian and derivatives use x-www-browser or www-browser to set\n>> +# the default browser for the system\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>> +       fi\n>> +       for i in $wwwbrowser; do\n>> +               if test -x $i ; then\n>> +                       verstring=\"$($i --version 2> /dev/null | head -n 1)\"\n>> +                       browser=\"$(echo \"$verstring\" | cut -f1 -d' ' | tr A-Z a-z)\"\n>\n> Stupid questions:\n>\n> Did you check that all the browser we support accept the --version option?\n> What if we add support for a new one that doesn't ?\n> Shouldn't we add something like :\n>\n> test -z \"$browser\" && browser=\"$(echo $i | cut -f1 -d' ' | tr A-Z a-z)\"\n\nSorry I mean:\n\n test -z \"$browser\" && browser=\"$(readlink $i)\"\n\n> And are you sure that when they support --version, the first word of\n> the output is\n> better than \"$(echo $i | cut -f1 -d' ' | tr A-Z a-z)\"?\n\nand: \"$(readlink $i)\"\n\nThanks,\nChristian.\n"},{"id":"156812","messageId":"20101129164435.GI8037@burratino","threadId":"25869","inReplyTo":"1291042075-19983-4-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCH 3/6] web--browse: split valid_tool list","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-11-29T16:44:35Z","receivedAt":"2010-11-29T16:44:35Z","isPatch":true,"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> @@ -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> +\t|konqueror|w3m|links|lynx|dillo|open|start)\n\nmicronit: I think this looks better as\n\n\tfirefox|iceweasel|chrome|google-chrome|chromium| \\\n\tkonqueror|w3m|links|lynx|dillo|open|start)\n\n, with the | on the end of the first line.\n"},{"id":"156813","messageId":"20101129164837.GJ8037@burratino","threadId":"25869","inReplyTo":"1291042075-19983-6-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCH 5/6] web--browse: use (x-)www-browser if available","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-11-29T16:48:37Z","receivedAt":"2010-11-29T16:48:37Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Giuseppe Bilotta wrote:\n\n> Debian and derivatives have an alternatives-based default browser\n> configuration that uses the /usr/bin/x-www-browser and\n> /usr/bin/www-browser symlinks.\n\nMm, maybe it will be time to look into adding xdg-open support\nsoon, too.\n"},{"id":"156827","messageId":"AANLkTimec0prJ29MR3N7GtHJ1FCkk+7=1UJtqiaLZqhK@mail.gmail.com","threadId":"25869","inReplyTo":"AANLkTinGustedMv9rm+RgGMQckfxgbTPA+Yqpz7vqTfm@mail.gmail.com","subject":"Re: [PATCH 5/6] web--browse: use (x-)www-browser if available","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-11-29T19:05:01Z","receivedAt":"2010-11-29T19:05:01Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Mon, Nov 29, 2010 at 5:18 PM, Christian Couder\n wrote:\n> Hi,\n>\n> On Mon, Nov 29, 2010 at 3:47 PM, Giuseppe Bilotta\n> wrote:\n>>\n>> +# Debian and derivatives use x-www-browser or www-browser to set\n>> +# the default browser for the system\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>> +       fi\n>> +       for i in $wwwbrowser; do\n>> +               if test -x $i ; then\n>> +                       verstring=\"$($i --version 2> /dev/null | head -n 1)\"\n>> +                       browser=\"$(echo \"$verstring\" | cut -f1 -d' ' | tr A-Z a-z)\"\n>\n> Stupid questions:\n>\n> Did you check that all the browser we support accept the --version option?\n\nI don't have all of them readily available, so I checked with\nMozilla-based browsers, Opera, Chromium, elinks, lynx, w3m. I'm\nmissing konqueror and dillo.\n\n> What if we add support for a new one that doesn't ?\n\nI think that the worse issue would be (x-)www-browser linking to\nsomething that doesn't support --version regardless of wether we\nsupport it or not.\n\n> Shouldn't we add something like :\n>\n> test -z \"$browser\" && browser=\"$(readlink $i)\"\n\nMy first idea was to go for something like browser=\"$(basename\n$(readlink $i))\" (not sure why you would need test -z before). Since\nit would need special-casing anyway (e.g. chromium-browser ->\nchromium), I opted out for the --version way so that (1) we could\ncatch one of our friendly cases even if the binary was called\nsomething else and (2) some invocation paths try to get the version\nanyway, so we could do it once and for all.\n\n> And are you sure that when they support --version, the first word of\n> the output is\n> better than \"$(readlink $i)\"?\n\nProbably depends on the metric used for 'better' ;-)\n\n> What if we add support for a new one?\n\nThen when adding it we should also look at its --version output and\nsee if it needs special treatment.\n\n>> +                       case \"$browser\" in\n>> +                       mozilla)\n>> +                               browser=\"$(echo \"$verstring\" | 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>\n> valid_tool \"$browser\" is called once here...\n>\n>> -else\n>> +else if test -z \"$browser_path\"; then\n>>        valid_tool \"$browser\" || die \"Unknown browser '$browser'.\"\n>\n> ...valid_tool \"$browser\" is called again here if it failed above, and\n> here we die,\n> so isn't it clearer to die as soon as we call it and it fails?\n\nWe allow valid_tool to be false in the x-www-browser case, in which\ncase we test www-browser, and if it's still not valid we go on and use\nthe previous paths. So we cannot die in case of an invalid\n(x-)www-browser. But there's a bug in the www-brower testing, it needs\nan else that resets browser to the empty string.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"156828","messageId":"AANLkTikXSx7WDaWes2nNH0XqP_xJui-k0ewLtrM6GL3g@mail.gmail.com","threadId":"25869","inReplyTo":"20101129164837.GJ8037@burratino","subject":"Re: [PATCH 5/6] web--browse: use (x-)www-browser if available","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-11-29T19:16:08Z","receivedAt":"2010-11-29T19:16:08Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Mon, Nov 29, 2010 at 5:48 PM, Jonathan Nieder wrote:\n> Giuseppe Bilotta wrote:\n>\n>> Debian and derivatives have an alternatives-based default browser\n>> configuration that uses the /usr/bin/x-www-browser and\n>> /usr/bin/www-browser symlinks.\n>\n> Mm, maybe it will be time to look into adding xdg-open support\n> soon, too.\n\nThat's probably a good idea, although I think there is no way with\nxdg-open to say \"open in a new tab\".\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"156829","messageId":"AANLkTik7611p5gun+CmyvOVpcm=_dXVo9FzP_tw7cbRU@mail.gmail.com","threadId":"25869","inReplyTo":"20101129164435.GI8037@burratino","subject":"Re: [PATCH 3/6] web--browse: split valid_tool list","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-11-29T19:19:06Z","receivedAt":"2010-11-29T19:19:06Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Mon, Nov 29, 2010 at 5:44 PM, Jonathan Nieder wrote:\n> Giuseppe Bilotta wrote:\n>\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>>       case \"$1\" in\n>> -     firefox | iceweasel | chrome | google-chrome | chromium | konqueror | w3m | links | lynx | dillo | open | start)\n>> +     firefox|iceweasel|chrome|google-chrome|chromium\\\n>> +     |konqueror|w3m|links|lynx|dillo|open|start)\n>\n> micronit: I think this looks better as\n>\n>        firefox|iceweasel|chrome|google-chrome|chromium| \\\n>        konqueror|w3m|links|lynx|dillo|open|start)\n>\n> , with the | on the end of the first line.\n\nI opted for the | at the beginning of the line to give a better idea\nof the continuation, but I don't have a strong preference in that\ndirection, so I can fix it up as you suggested, unless there are\nspecific indications from the 'higher ups' ;-)\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"156838","messageId":"7vsjyj280k.fsf@alter.siamese.dyndns.org","threadId":"25869","inReplyTo":"20101129164435.GI8037@burratino","subject":"Re: [PATCH 3/6] web--browse: split valid_tool list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-11-29T20:24:11Z","receivedAt":"2010-11-29T20:24:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Giuseppe Bilotta wrote:\n>\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>> +\t|konqueror|w3m|links|lynx|dillo|open|start)\n>\n> micronit: I think this looks better as\n>\n> \tfirefox|iceweasel|chrome|google-chrome|chromium| \\\n> \tkonqueror|w3m|links|lynx|dillo|open|start)\n>\n> , with the | on the end of the first line.\n\nYes, and with one SP around all \"|\".\n\nThanks.\n"},{"id":"156865","messageId":"7vy68by7om.fsf@alter.siamese.dyndns.org","threadId":"25869","inReplyTo":"1291042075-19983-2-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCH 1/6] CodingGuidelines: mention whitespace preferences for shell scripts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-11-30T00:30:17Z","receivedAt":"2010-11-30T00:30:17Z","isPatch":true,"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 | 6 ++++++\n> 1 files changed, 6 insertions(+), 0 deletions(-)\n>\n> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\n> index 5aa2d34..7ecce51 100644\n> --- a/Documentation/CodingGuidelines\n> +++ b/Documentation/CodingGuidelines\n> @@ -31,6 +31,12 @@ 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\nOk.\n\n> + - The choices in case ... esac statement are not indented with respect\n> + to the the case and esac lines. The body of the choices however _is_\n> + indented (by one tab).\n> +\n\nNit; \"choices\" are called \"case arms\".\n\nI don't think there is any need to emphasize your surprise that case body\nis indented with \"however _is_\", which implies \"against common sense or\nnormal convention\" or somesuch. The thing is, there is no single _right_\nconvention, and we want people to folow the one just picked---we should\njust describe what it is without being emotional nor emphatic.\n\nThanks.\n"},{"id":"156881","messageId":"AANLkTik9R2dRDDBUHBZ_OKPKto0wDAxr1V0uL=q4bU+4@mail.gmail.com","threadId":"25869","inReplyTo":"AANLkTimec0prJ29MR3N7GtHJ1FCkk+7=1UJtqiaLZqhK@mail.gmail.com","subject":"Re: [PATCH 5/6] web--browse: use (x-)www-browser if available","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2010-11-30T04:02:28Z","receivedAt":"2010-11-30T04:02:28Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Nov 29, 2010 at 8:05 PM, Giuseppe Bilotta\n wrote:\n> On Mon, Nov 29, 2010 at 5:18 PM, Christian Couder\n> wrote:\n>> Hi,\n>>\n>> On Mon, Nov 29, 2010 at 3:47 PM, Giuseppe Bilotta\n>> wrote:\n>>>\n>>> +# Debian and derivatives use x-www-browser or www-browser to set\n>>> +# the default browser for the system\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>>> +       fi\n>>> +       for i in $wwwbrowser; do\n>>> +               if test -x $i ; then\n>>> +                       verstring=\"$($i --version 2> /dev/null | head -n 1)\"\n>>> +                       browser=\"$(echo \"$verstring\" | cut -f1 -d' ' | tr A-Z a-z)\"\n>>\n>> Stupid questions:\n>>\n>> Did you check that all the browser we support accept the --version option?\n>\n> I don't have all of them readily available, so I checked with\n> Mozilla-based browsers, Opera, Chromium, elinks, lynx, w3m. I'm\n> missing konqueror and dillo.\n>\n>> What if we add support for a new one that doesn't ?\n>\n> I think that the worse issue would be (x-)www-browser linking to\n> something that doesn't support --version regardless of wether we\n> support it or not.\n>\n>> Shouldn't we add something like :\n>>\n>> test -z \"$browser\" && browser=\"$(readlink $i)\"\n>\n> My first idea was to go for something like browser=\"$(basename\n> $(readlink $i))\" (not sure why you would need test -z before).\n\nWe would need \"test -z\" before if we add it after:\n\nbrowser=\"$(echo \"$verstring\" | cut -f1 -d' ' | tr A-Z a-z)\"\n\nso that we have a fallback if it doesn't support \"--version\".\n\n> Since\n> it would need special-casing anyway (e.g. chromium-browser ->\n> chromium), I opted out for the --version way so that (1) we could\n> catch one of our friendly cases even if the binary was called\n> something else and (2) some invocation paths try to get the version\n> anyway, so we could do it once and for all.\n>\n>> And are you sure that when they support --version, the first word of\n>> the output is\n>> better than \"$(readlink $i)\"?\n>\n> Probably depends on the metric used for 'better' ;-)\n>\n>> What if we add support for a new one?\n>\n> Then when adding it we should also look at its --version output and\n> see if it needs special treatment.\n>\n>>> +                       case \"$browser\" in\n>>> +                       mozilla)\n>>> +                               browser=\"$(echo \"$verstring\" | 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>>\n>> valid_tool \"$browser\" is called once here...\n>>\n>>> -else\n>>> +else if test -z \"$browser_path\"; then\n>>>        valid_tool \"$browser\" || die \"Unknown browser '$browser'.\"\n>>\n>> ...valid_tool \"$browser\" is called again here if it failed above, and\n>> here we die,\n>> so isn't it clearer to die as soon as we call it and it fails?\n>\n> We allow valid_tool to be false in the x-www-browser case, in which\n> case we test www-browser, and if it's still not valid we go on and use\n> the previous paths. So we cannot die in case of an invalid\n> (x-)www-browser.\n\nYeah, you are right, but we could die after the \"for i in $wwwbrowser\"\nloop if both are invalid.\n\n> But there's a bug in the www-brower testing, it needs\n> an else that resets browser to the empty string.\n\nI thought it was by design that you did not reset it...\nSo yeah it is clearer and nicer for the user if you either reset\nbrowser or just die if both (x-)www-browser are invalid. If you decide\nto reset browser, perhaps a warning or an information message telling\nthat both are unknown would be nice.\n\nThanks,\nChristian.\n"},{"id":"156886","messageId":"AANLkTin5KWZAHegFHykau6gWHbijA7B8q_7WjfnV2mus@mail.gmail.com","threadId":"25869","inReplyTo":"7vy68by7om.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/6] CodingGuidelines: mention whitespace preferences for shell scripts","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-11-30T08:11:20Z","receivedAt":"2010-11-30T08:11:20Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Tue, Nov 30, 2010 at 1:30 AM, Junio C Hamano wrote:\n> Giuseppe Bilotta writes:\n>\n>> Signed-off-by: Giuseppe Bilotta \n>> ---\n>>  Documentation/CodingGuidelines |    6 ++++++\n>>  1 files changed, 6 insertions(+), 0 deletions(-)\n>>\n>> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\n>> index 5aa2d34..7ecce51 100644\n>> --- a/Documentation/CodingGuidelines\n>> +++ b/Documentation/CodingGuidelines\n>> @@ -31,6 +31,12 @@ 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> Ok.\n>\n>> + - The choices in case ... esac statement are not indented with respect\n>> +   to the the case and esac lines. The body of the choices however _is_\n>> +   indented (by one tab).\n>> +\n>\n> Nit; \"choices\" are called \"case arms\".\n\nI'll rephrase.\n\n> I don't think there is any need to emphasize your surprise that case body\n> is indented with \"however _is_\", which implies \"against common sense or\n> normal convention\" or somesuch.  The thing is, there is no single _right_\n> convention, and we want people to folow the one just picked---we should\n> just describe what it is without being emotional nor emphatic.\n\nGood point, I'll resend with a shorter commit message. Would you be ok\nwith me adding an example, or do you think it's unnecessary?\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"156891","messageId":"AANLkTik-OKsUK2aJCDR0Q-FyQM=rrFQmx=Hwdyr5NzYt@mail.gmail.com","threadId":"25869","inReplyTo":"AANLkTik9R2dRDDBUHBZ_OKPKto0wDAxr1V0uL=q4bU+4@mail.gmail.com","subject":"Re: [PATCH 5/6] web--browse: use (x-)www-browser if available","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-11-30T08:22:20Z","receivedAt":"2010-11-30T08:22:20Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Tue, Nov 30, 2010 at 5:02 AM, Christian Couder\n wrote:\n> On Mon, Nov 29, 2010 at 8:05 PM, Giuseppe Bilotta\n> wrote:\n>> On Mon, Nov 29, 2010 at 5:18 PM, Christian Couder\n>> wrote:\n>>> Hi,\n>>>\n>>> On Mon, Nov 29, 2010 at 3:47 PM, Giuseppe Bilotta\n>>> wrote:\n>>>>\n>>>> +# Debian and derivatives use x-www-browser or www-browser to set\n>>>> +# the default browser for the system\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>>>> +       fi\n>>>> +       for i in $wwwbrowser; do\n>>>> +               if test -x $i ; then\n>>>> +                       verstring=\"$($i --version 2> /dev/null | head -n 1)\"\n>>>> +                       browser=\"$(echo \"$verstring\" | cut -f1 -d' ' | tr A-Z a-z)\"\n>>>\n>>> Stupid questions:\n>>>\n>>> Did you check that all the browser we support accept the --version option?\n>>\n>> I don't have all of them readily available, so I checked with\n>> Mozilla-based browsers, Opera, Chromium, elinks, lynx, w3m. I'm\n>> missing konqueror and dillo.\n>>\n>>> What if we add support for a new one that doesn't ?\n>>\n>> I think that the worse issue would be (x-)www-browser linking to\n>> something that doesn't support --version regardless of wether we\n>> support it or not.\n>>\n>>> Shouldn't we add something like :\n>>>\n>>> test -z \"$browser\" && browser=\"$(readlink $i)\"\n>>\n>> My first idea was to go for something like browser=\"$(basename\n>> $(readlink $i))\" (not sure why you would need test -z before).\n>\n> We would need \"test -z\" before if we add it after:\n>\n> browser=\"$(echo \"$verstring\" | cut -f1 -d' ' | tr A-Z a-z)\"\n>\n> so that we have a fallback if it doesn't support \"--version\".\n\nOh, I see what you mean here. I thought you wanted to use it as an\nalternative to the --version.\n\n>> We allow valid_tool to be false in the x-www-browser case, in which\n>> case we test www-browser, and if it's still not valid we go on and use\n>> the previous paths. So we cannot die in case of an invalid\n>> (x-)www-browser.\n>\n> Yeah, you are right, but we could die after the \"for i in $wwwbrowser\"\n> loop if both are invalid.\n\nWhy? If both are invalid, proceeding with the previous strategy of\nlooking for a browser we _should_ be looking for any browser we know\nabout, even if it's not set as the default system browser.\n\n>> But there's a bug in the www-brower testing, it needs\n>> an else that resets browser to the empty string.\n>\n> I thought it was by design that you did not reset it...\n> So yeah it is clearer and nicer for the user if you either reset\n> browser or just die if both (x-)www-browser are invalid. If you decide\n> to reset browser, perhaps a warning or an information message telling\n> that both are unknown would be nice.\n\nI can do that. Should it be a warning about reporting the lack of\nsupport to us, or just a warning that we are not going to use it even\nif it's defined?\n\nWhile we're at it: I was considering adding support for the BROWSER\nenv var (a colon-separated list of browsers executables or \"browser\n%s\" strings).\n\nAll of this is going to make the web--browse script very similar to\nthe sensible-browser script in Debian, with the difference that we go\nat length to ensure that stuff is opened in a new tab, whereas\nDebian's sensible-browser doesn't. Should we just support\nsensible-browser instead of (x-)www-browser/BROWSER, and let it open\nanything?\n\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"156907","messageId":"AANLkTinzCeaiFoL4a-+c6wuJoUQ68zC9vE8AoHfmvN-F@mail.gmail.com","threadId":"25869","inReplyTo":"AANLkTik-OKsUK2aJCDR0Q-FyQM=rrFQmx=Hwdyr5NzYt@mail.gmail.com","subject":"Re: [PATCH 5/6] web--browse: use (x-)www-browser if available","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2010-12-01T10:59:13Z","receivedAt":"2010-12-01T10:59:13Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Nov 30, 2010 at 9:22 AM, Giuseppe Bilotta\n wrote:\n> On Tue, Nov 30, 2010 at 5:02 AM, Christian Couder\n> wrote:\n>> On Mon, Nov 29, 2010 at 8:05 PM, Giuseppe Bilotta\n>> wrote:\n>>> We allow valid_tool to be false in the x-www-browser case, in which\n>>> case we test www-browser, and if it's still not valid we go on and use\n>>> the previous paths. So we cannot die in case of an invalid\n>>> (x-)www-browser.\n>>\n>> Yeah, you are right, but we could die after the \"for i in $wwwbrowser\"\n>> loop if both are invalid.\n>\n> Why? If both are invalid, proceeding with the previous strategy of\n> looking for a browser we _should_ be looking for any browser we know\n> about, even if it's not set as the default system browser.\n\nCurrently we have:\n\n97 if test -n \"$browser\" && ! valid_tool \"$browser\"; then\n98 echo >&2 \"git config option $opt set to unknown browser: $browser\"\n99 echo >&2 \"Resetting to default...\"\n100 unset browser\n101 fi\n\nSo if we want to be consistent with that behavior, we should probably do the\nsame thing if (x-)www-browser is set but we don't support it.\n\n>>> But there's a bug in the www-brower testing, it needs\n>>> an else that resets browser to the empty string.\n>>\n>> I thought it was by design that you did not reset it...\n>> So yeah it is clearer and nicer for the user if you either reset\n>> browser or just die if both (x-)www-browser are invalid. If you decide\n>> to reset browser, perhaps a warning or an information message telling\n>> that both are unknown would be nice.\n>\n> I can do that. Should it be a warning about reporting the lack of\n> support to us, or just a warning that we are not going to use it even\n> if it's defined?\n\nI think it should be a warning that we are not going to use it even\nif it's defined, to be consistent with the code pasted above.\n\nIf you think of something better or want to remove the warnings, please do\nit in another patch.\n\n> While we're at it: I was considering adding support for the BROWSER\n> env var (a colon-separated list of browsers executables or \"browser\n> %s\" strings).\n>\n> All of this is going to make the web--browse script very similar to\n> the sensible-browser script in Debian, with the difference that we go\n> at length to ensure that stuff is opened in a new tab, whereas\n> Debian's sensible-browser doesn't. Should we just support\n> sensible-browser instead of (x-)www-browser/BROWSER, and let it open\n> anything?\n\nI think most users prefer to open stuff in a new tab if possible.\n\nThanks,\nChristian.\n"},{"id":"156909","messageId":"AANLkTikfAr-KUVyqeTooxYKvKWYM4pRmw51if+349dpJ@mail.gmail.com","threadId":"25869","inReplyTo":"AANLkTinzCeaiFoL4a-+c6wuJoUQ68zC9vE8AoHfmvN-F@mail.gmail.com","subject":"Re: [PATCH 5/6] web--browse: use (x-)www-browser if available","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-12-01T11:46:57Z","receivedAt":"2010-12-01T11:46:57Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Wed, Dec 1, 2010 at 11:59 AM, Christian Couder\n wrote:\n> On Tue, Nov 30, 2010 at 9:22 AM, Giuseppe Bilotta\n> wrote:\n>> On Tue, Nov 30, 2010 at 5:02 AM, Christian Couder\n>> wrote:\n>>> On Mon, Nov 29, 2010 at 8:05 PM, Giuseppe Bilotta\n>>> wrote:\n>>>> We allow valid_tool to be false in the x-www-browser case, in which\n>>>> case we test www-browser, and if it's still not valid we go on and use\n>>>> the previous paths. So we cannot die in case of an invalid\n>>>> (x-)www-browser.\n>>>\n>>> Yeah, you are right, but we could die after the \"for i in $wwwbrowser\"\n>>> loop if both are invalid.\n>>\n>> Why? If both are invalid, proceeding with the previous strategy of\n>> looking for a browser we _should_ be looking for any browser we know\n>> about, even if it's not set as the default system browser.\n>\n> Currently we have:\n>\n> 97     if test -n \"$browser\" && ! valid_tool \"$browser\"; then\n> 98         echo >&2 \"git config option $opt set to unknown browser: $browser\"\n> 99         echo >&2 \"Resetting to default...\"\n> 100         unset browser\n> 101     fi\n>\n> So if we want to be consistent with that behavior, we should probably do the\n> same thing if (x-)www-browser is set but we don't support it.\n\nThe (x-)www-browser code is used when the user did _not_ define a\nbrowser. It is not a browser option (git web--browse\n--tool=www-browser).\nIOW, this is not about the user selecting an unsupported browser, but\nabout the system default browser not being a supported one. I don't\nthink it should be treated the same way.\n\n>>>> But there's a bug in the www-brower testing, it needs\n>>>> an else that resets browser to the empty string.\n>>>\n>>> I thought it was by design that you did not reset it...\n>>> So yeah it is clearer and nicer for the user if you either reset\n>>> browser or just die if both (x-)www-browser are invalid. If you decide\n>>> to reset browser, perhaps a warning or an information message telling\n>>> that both are unknown would be nice.\n>>\n>> I can do that. Should it be a warning about reporting the lack of\n>> support to us, or just a warning that we are not going to use it even\n>> if it's defined?\n>\n> I think it should be a warning that we are not going to use it even\n> if it's defined, to be consistent with the code pasted above.\n\nOk.\n\n> If you think of something better or want to remove the warnings, please do\n> it in another patch.\n\nI think the warning is fine.\n\n>> While we're at it: I was considering adding support for the BROWSER\n>> env var (a colon-separated list of browsers executables or \"browser\n>> %s\" strings).\n>>\n>> All of this is going to make the web--browse script very similar to\n>> the sensible-browser script in Debian, with the difference that we go\n>> at length to ensure that stuff is opened in a new tab, whereas\n>> Debian's sensible-browser doesn't. Should we just support\n>> sensible-browser instead of (x-)www-browser/BROWSER, and let it open\n>> anything?\n>\n> I think most users prefer to open stuff in a new tab if possible.\n\nI will look into adding support for the BROWSER environment variable\nto select the browser if no browser is defined, then, but not consider\nsensible-browser.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"156948","messageId":"20101201154959.GA26120@burratino","threadId":"25869","inReplyTo":"AANLkTikfAr-KUVyqeTooxYKvKWYM4pRmw51if+349dpJ@mail.gmail.com","subject":"Re: [PATCH 5/6] web--browse: use (x-)www-browser if available","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-01T15:49:59Z","receivedAt":"2010-12-01T15:49:59Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Giuseppe Bilotta wrote:\n\n> I will look into adding support for the BROWSER environment variable\n> to select the browser if no browser is defined\n\nThanks, that sounds excellent.\n\nBTW, the semantics of quoting characters/shell metacharacters in\n$BROWSER are not well defined (well, they're well defined in one way\nby the original spec, which is ignored by xdg-open and some other\ntools). xdg-open's semantics are probably the most reliable standard\nhere but thought I should give a heads up.\n\nThinking out loud: longer term, it would be nice to just use xdg-utils\nand drop most of this logic from git. Presumably the ideal behavior\nfor most apps opening web pages is already \"new tab\", so the cases of\n\"new window\" not explicitly configured that way by the user are bugs.\nOtherwise a request to portland@lists.freedesktop.org (subscription\nrequired) for xdg-open --new-tab/--new-window might be useful.\n\nRegards,\nJonathan\n"}]}