[PATCHv3] git-web--browse: avoid the use of eval

[Chris Packham <judge.packham@gmail.com>]

Using eval causes problems when the URL contains an appropriately
escaped ampersand (\&). Dropping eval from the built-in browser
invocation avoids the problem.

Helped-by: Jeff King <peff@peff.net> (test case)
Signed-off-by: Chris Packham <judge.packham@gmail.com>

---
The consensus from the last round of discussion [1] seemed to be to
remove the eval from the built in browsers but quote custom browser
commands appropriately.

I've expanded the tests a little. A semi-colon had the same error as
the ampersand. A hash was another common character that had meaning in
a shell and in URL.

[1] http://article.gmane.org/gmane.comp.version-control.git/181671

 git-web--browse.sh         |   10 +++++-----
 t/t9901-git-web--browse.sh |   37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 5 deletions(-)
 create mode 100755 t/t9901-git-web--browse.sh

diff --git a/git-web--browse.sh b/git-web--browse.sh
index e9de241..1e82726 100755
--- a/git-web--browse.sh
+++ b/git-web--browse.sh
@@ -156,7 +156,7 @@ firefox|iceweasel|seamonkey|iceape)
 	;;
 google-chrome|chrome|chromium|chromium-browser)
 	# No need to specify newTab. It's default in chromium
-	eval "$browser_path" "$@" &
+	"$browser_path" "$@" &
 	;;
 konqueror)
 	case "$(basename "$browser_path")" in
@@ -164,10 +164,10 @@ konqueror)
 		# It's simpler to use kfmclient to open a new tab in konqueror.
 		browser_path="$(echo "$browser_path" | sed -e 's/konqueror$/kfmclient/')"
 		type "$browser_path" > /dev/null 2>&1 || die "No '$browser_path' found."
-		eval "$browser_path" newTab "$@"
+		"$browser_path" newTab "$@" &
 		;;
 	kfmclient)
-		eval "$browser_path" newTab "$@"
+		"$browser_path" newTab "$@" &
 		;;
 	*)
 		"$browser_path" "$@" &
@@ -175,7 +175,7 @@ konqueror)
 	esac
 	;;
 w3m|elinks|links|lynx|open)
-	eval "$browser_path" "$@"
+	"$browser_path" "$@"
 	;;
 start)
 	exec "$browser_path" '"web-browse"' "$@"
@@ -185,7 +185,7 @@ opera|dillo)
 	;;
 *)
 	if test -n "$browser_cmd"; then
-		( eval $browser_cmd "$@" )
+		( eval "$browser_cmd \"\$@\"" )
 	fi
 	;;
 esac
diff --git a/t/t9901-git-web--browse.sh b/t/t9901-git-web--browse.sh
new file mode 100755
index 0000000..c6f48a9
--- /dev/null
+++ b/t/t9901-git-web--browse.sh
@@ -0,0 +1,37 @@
+#!/bin/sh
+#
+
+test_description='git web--browse basic tests
+
+This test checks that git web--browse can handle various valid URLs.'
+
+. ./test-lib.sh
+
+test_expect_success \
+	'URL with an ampersand in it' '
+	echo http://example.com/foo\&bar >expect &&
+	git config browser.custom.cmd echo &&
+	git web--browse --browser=custom \
+		http://example.com/foo\&bar >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success \
+	'URL with a semi-colon in it' '
+	echo http://example.com/foo\;bar >expect &&
+	git config browser.custom.cmd echo &&
+	git web--browse --browser=custom \
+		http://example.com/foo\;bar >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success \
+	'URL with a hash in it' '
+	echo http://example.com/foo#bar >expect &&
+	git config browser.custom.cmd echo &&
+	git web--browse --browser=custom \
+		http://example.com/foo#bar >actual &&
+	test_cmp expect actual
+'
+
+test_done
-- 
1.7.7