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@xxxxxxxx> (test case) > Signed-off-by: Chris Packham <judge.packham@xxxxxxxxx> > > --- > 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. This looks good to me. I think we may want to squash in the two tests below, too, which make sure we treat $browser_path and $browser_cmd appropriately (the former is a filename, and the latter is a shell snippet). 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html