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

From

Jeff King <peff@peff.net>

Date

Oct 3, 2011, 09:57 UTC

Message-ID

<20111003095731.GB16078@sigill.intra.peff.net>

In-Reply-To

<1317516257-24435-1-git-send-email-judge.packham@gmail.com>

On Sun, Oct 02, 2011 at 01:44:17PM +1300, Chris Packham wrote:

> 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.

diff --git a/t/t9901-git-web--browse.sh b/t/t9901-git-web--browse.sh
index c6f48a9..7906e5d 100755
--- a/t/t9901-git-web--browse.sh
+++ b/t/t9901-git-web--browse.sh
@@ -34,4 +34,33 @@ test_expect_success \
 	test_cmp expect actual
 '
 
+test_expect_success \
+	'browser paths are properly quoted' '
+	echo fake: http://example.com/foo >expect &&
+	cat >"fake browser" <<-\EOF &&
+	#!/bin/sh
+	echo fake: "$@"
+	EOF
+	chmod +x "fake browser" &&
+	git config browser.w3m.path "`pwd`/fake browser" &&
+	git web--browse --browser=w3m \
+		http://example.com/foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success \
+	'browser command allows arbitrary shell code' '
+	echo "arg: http://example.com/foo" >expect &&
+	git config browser.custom.cmd "
+		f() {
+			for i in \"\$@\"; do
+				echo arg: \$i
+			done
+		}
+		f" &&
+	git web--browse --browser=custom \
+		http://example.com/foo >actual &&
+	test_cmp expect actual
+'
+
 test_done