[PATCH] git-web--browse: invoke kfmclient directly

From: Chris Packham <judge.packham@gmail.com>
Date: Sep 18, 2011, 10:20 UTC
Message-ID: <1316341224-4359-1-git-send-email-judge.packham@gmail.com>
In-Reply-To: <20110918032933.GA17977@sigill.intra.peff.net>

Instead of using eval which causes problems when a URL contains an appropriately escaped ampersand (\&).

Cc: peff@peff.net
Cc: chriscool@tuxfamily.org
Cc: jepler@unpythonic.net
Signed-off-by: Chris Packham <judge.packham@gmail.com>
---

Show 7 quoted lines

> Which implies that "$browser_path" must be the actual
> executable. In which case, I would think that:
>
>   "$browser_path" "$@" &
>
> would be the right thing. And indeed, that is what the firefox arm of
> the case statement does. But chrome, konqueror, and others use eval.

So here is my attempt at a fix for kfmclient.

For what it's worth I've included a testcase that detects my problem. I'm not sure if the testcase is really worth it because the test library suppresses X applications and even if it didn't the testcase is fairly trivial and might just annoy people by opening web-browsers (and it snaps up the last t99xx prefix).

 git-web--browse.sh         |    4 ++--
 t/t9901-git-web--browse.sh |   43 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 2 deletions(-)
 create mode 100755 t/t9901-git-web--browse.sh

diff --git a/git-web--browse.sh b/git-web--browse.sh
index e9de241..1164a22 100755
--- a/git-web--browse.sh
+++ b/git-web--browse.sh
@@ -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" "$@" &
diff --git a/t/t9901-git-web--browse.sh b/t/t9901-git-web--browse.sh
new file mode 100755
index 0000000..7ed38a0
--- /dev/null
+++ b/t/t9901-git-web--browse.sh
@@ -0,0 +1,43 @@
+#!/bin/sh
+#
+# Copyright (c) 2011 Chris Packham
+#
+
+test_description='git web--browse basic tests
+
+This test checks that git web--browse can handle various valid URLs with
+the supported browsers that are installed on the host system.'
+
+. ./test-lib.sh
+
+test -x /usr/bin/firefox && test_set_prereq FIREFOX
+test -x /usr/bin/konqueror && test_set_prereq KONQUEROR
+test -x /usr/bin/google-chrome && test_set_prereq CHROME
+test -x /usr/bin/opera && test_set_prereq OPERA
+
+test_expect_success \
+	'accepts a URL with an ampersand in it (default)' '
+    git web--browse http://example.com/foo\&bar/
+'
+
+test_expect_success FIREFOX \
+	'accepts a URL with an ampersand in it (firefox)' '
+    git web--browse --browser=firefox http://example.com/foo\&bar/
+'
+
+test_expect_success KONQUEROR \
+	'accepts a URL with an ampersand in it (konqueror)' '
+    git web--browse --browser=konqueror http://example.com/foo\&bar/
+'
+
+test_expect_success OPERA \
+	'accepts a URL with an ampersand in it (opera)' '
+    git web--browse --browser=opera http://example.com/foo\&bar/
+'
+
+test_expect_success CHROME \
+	'accepts a URL with an ampersand in it (chrome)' '
+    git web--browse --browser=google-chrome http://example.com/foo\&bar/
+'
+
+test_done

-- 
1.7.7.rc1.3.g5593.dirty