{"thread":{"id":"28549","subject":"[PATCHv3] git-web--browse: avoid the use of eval","startedAt":"2011-10-02T00:44:17Z","lastAt":"2013-01-26T00:40:33Z","messageCount":12,"participants":["Chris Packham","Jeff King","Alexey Shumkin","Junio C Hamano","Shumkin Alexey"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"176669","messageId":"1317516257-24435-1-git-send-email-judge.packham@gmail.com","threadId":"28549","inReplyTo":null,"subject":"[PATCHv3] git-web--browse: avoid the use of eval","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2011-10-02T00:44:17Z","receivedAt":"2011-10-02T00:44:17Z","isPatch":false,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"Using eval causes problems when the URL contains an appropriately\nescaped ampersand (\\&). Dropping eval from the built-in browser\ninvocation avoids the problem.\n\nHelped-by: Jeff King <peff@peff.net> (test case)\nSigned-off-by: Chris Packham <judge.packham@gmail.com>\n\n---\nThe consensus from the last round of discussion [1] seemed to be to\nremove the eval from the built in browsers but quote custom browser\ncommands appropriately.\n\nI've expanded the tests a little. A semi-colon had the same error as\nthe ampersand. A hash was another common character that had meaning in\na shell and in URL.\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/181671\n\n git-web--browse.sh         |   10 +++++-----\n t/t9901-git-web--browse.sh |   37 +++++++++++++++++++++++++++++++++++++\n 2 files changed, 42 insertions(+), 5 deletions(-)\n create mode 100755 t/t9901-git-web--browse.sh\n\ndiff --git a/git-web--browse.sh b/git-web--browse.sh\nindex e9de241..1e82726 100755\n--- a/git-web--browse.sh\n+++ b/git-web--browse.sh\n@@ -156,7 +156,7 @@ firefox|iceweasel|seamonkey|iceape)\n \t;;\n google-chrome|chrome|chromium|chromium-browser)\n \t# No need to specify newTab. It's default in chromium\n-\teval \"$browser_path\" \"$@\" &\n+\t\"$browser_path\" \"$@\" &\n \t;;\n konqueror)\n \tcase \"$(basename \"$browser_path\")\" in\n@@ -164,10 +164,10 @@ konqueror)\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\"$browser_path\" newTab \"$@\" &\n \t\t;;\n \tkfmclient)\n-\t\teval \"$browser_path\" newTab \"$@\"\n+\t\t\"$browser_path\" newTab \"$@\" &\n \t\t;;\n \t*)\n \t\t\"$browser_path\" \"$@\" &\n@@ -175,7 +175,7 @@ konqueror)\n \tesac\n \t;;\n w3m|elinks|links|lynx|open)\n-\teval \"$browser_path\" \"$@\"\n+\t\"$browser_path\" \"$@\"\n \t;;\n start)\n \texec \"$browser_path\" '\"web-browse\"' \"$@\"\n@@ -185,7 +185,7 @@ opera|dillo)\n \t;;\n *)\n \tif test -n \"$browser_cmd\"; then\n-\t\t( eval $browser_cmd \"$@\" )\n+\t\t( eval \"$browser_cmd \\\"\\$@\\\"\" )\n \tfi\n \t;;\n esac\ndiff --git a/t/t9901-git-web--browse.sh b/t/t9901-git-web--browse.sh\nnew file mode 100755\nindex 0000000..c6f48a9\n--- /dev/null\n+++ b/t/t9901-git-web--browse.sh\n@@ -0,0 +1,37 @@\n+#!/bin/sh\n+#\n+\n+test_description='git web--browse basic tests\n+\n+This test checks that git web--browse can handle various valid URLs.'\n+\n+. ./test-lib.sh\n+\n+test_expect_success \\\n+\t'URL with an ampersand in it' '\n+\techo http://example.com/foo\\&bar >expect &&\n+\tgit config browser.custom.cmd echo &&\n+\tgit web--browse --browser=custom \\\n+\t\thttp://example.com/foo\\&bar >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success \\\n+\t'URL with a semi-colon in it' '\n+\techo http://example.com/foo\\;bar >expect &&\n+\tgit config browser.custom.cmd echo &&\n+\tgit web--browse --browser=custom \\\n+\t\thttp://example.com/foo\\;bar >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success \\\n+\t'URL with a hash in it' '\n+\techo http://example.com/foo#bar >expect &&\n+\tgit config browser.custom.cmd echo &&\n+\tgit web--browse --browser=custom \\\n+\t\thttp://example.com/foo#bar >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n1.7.7\n"},{"id":"176729","messageId":"20111003095731.GB16078@sigill.intra.peff.net","threadId":"28549","inReplyTo":"1317516257-24435-1-git-send-email-judge.packham@gmail.com","subject":"Re: [PATCHv3] git-web--browse: avoid the use of eval","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-03T09:57:31Z","receivedAt":"2011-10-03T09:57:31Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 02, 2011 at 01:44:17PM +1300, Chris Packham wrote:\n\n> Using eval causes problems when the URL contains an appropriately\n> escaped ampersand (\\&). Dropping eval from the built-in browser\n> invocation avoids the problem.\n> \n> Helped-by: Jeff King <peff@peff.net> (test case)\n> Signed-off-by: Chris Packham <judge.packham@gmail.com>\n> \n> ---\n> The consensus from the last round of discussion [1] seemed to be to\n> remove the eval from the built in browsers but quote custom browser\n> commands appropriately.\n> \n> I've expanded the tests a little. A semi-colon had the same error as\n> the ampersand. A hash was another common character that had meaning in\n> a shell and in URL.\n\nThis looks good to me. I think we may want to squash in the two tests\nbelow, too, which make sure we treat $browser_path and $browser_cmd\nappropriately (the former is a filename, and the latter is a shell\nsnippet).\n\ndiff --git a/t/t9901-git-web--browse.sh b/t/t9901-git-web--browse.sh\nindex c6f48a9..7906e5d 100755\n--- a/t/t9901-git-web--browse.sh\n+++ b/t/t9901-git-web--browse.sh\n@@ -34,4 +34,33 @@ test_expect_success \\\n \ttest_cmp expect actual\n '\n \n+test_expect_success \\\n+\t'browser paths are properly quoted' '\n+\techo fake: http://example.com/foo >expect &&\n+\tcat >\"fake browser\" <<-\\EOF &&\n+\t#!/bin/sh\n+\techo fake: \"$@\"\n+\tEOF\n+\tchmod +x \"fake browser\" &&\n+\tgit config browser.w3m.path \"`pwd`/fake browser\" &&\n+\tgit web--browse --browser=w3m \\\n+\t\thttp://example.com/foo >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success \\\n+\t'browser command allows arbitrary shell code' '\n+\techo \"arg: http://example.com/foo\" >expect &&\n+\tgit config browser.custom.cmd \"\n+\t\tf() {\n+\t\t\tfor i in \\\"\\$@\\\"; do\n+\t\t\t\techo arg: \\$i\n+\t\t\tdone\n+\t\t}\n+\t\tf\" &&\n+\tgit web--browse --browser=custom \\\n+\t\thttp://example.com/foo >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n"},{"id":"179324","messageId":"20111111183555.GC16055@sigill.intra.peff.net","threadId":"28549","inReplyTo":"1321028283-17307-1-git-send-email-Alex.Crezoff@gmail.com","subject":"Re: [PATCH] git-web--browser: avoid errors in terminal when running Firefox on Windows","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-11-11T18:35:55Z","receivedAt":"2011-11-11T18:35:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 11, 2011 at 08:18:03PM +0400, Alexey Shumkin wrote:\n\n> Firefox on Windows by default is placed in \"C:\\Program Files\\Mozilla Firefox\"\n> folder, i.e. its path contains spaces. Before running this browser \"git-web--browse\"\n> tests version of Firefox to decide whether to use \"-new-tab\" option or not.\n> \n> Quote browser path to avoid error during this test.\n\nThanks. I even noticed this bug early on in the previous discussion:\n\n  http://article.gmane.org/gmane.comp.version-control.git/181600\n\nbut forgot about it by the time the final patch rolled around. Your fix\nlooks correct, but:\n\n>  test_expect_success \\\n> +\t'Firefox below v2.0 paths are properly quoted' '\n> +\techo fake: http://example.com/foo >expect &&\n> +\tcat >\"fake browser\" <<-\\EOF &&\n> +\t#!/bin/sh\n> +\n> +\tif [ \"$1\" == \"-version\" ]; then\n\nUsing \"==\" is a bashism. Just use \"=\".\n\nAlso, a style nit, but we usually spell this \"test\" and not \"[\". I admit\nI don't care much, though.\n\n> +\t\t# Firefox (in contrast to w3m) is run in background (with &)\n> +\t\t# so redirect output to \"actual\"\n> +\t\techo fake: \"$@\" > actual\n> +\tfi\n> +\tEOF\n> +\tchmod +x \"fake browser\" &&\n> +\tgit config browser.firefox.path \"`pwd`/fake browser\" &&\n> +\tgit web--browse --browser=firefox \\\n> +\t\thttp://example.com/foo &&\n> +\ttest_cmp expect actual\n\nHmm. So we are running the fake browser in the background, but then\ncheck that it has written something as soon as web--browse exits. Isn't\nthat a race condition? I.e., we could run \"test_cmp\" before the browser\nhas actually written anything?\n\nI'm not sure there's a good way to do it.  You would need either to wait\nsome pre-determined \"it could not possibly take it longer than N seconds\nto run\" sleep, or we need some kind of synchronization point. We can't\nwait call \"wait\" on the child PID (if we even have it, because it's not\nour child).\n\n-Peff\n"},{"id":"179325","messageId":"20111111234830.32dccd87@zappedws","threadId":"28549","inReplyTo":"20111111183555.GC16055@sigill.intra.peff.net","subject":"Re: [PATCH] git-web--browser: avoid errors in terminal when running Firefox on Windows","fromName":"Alexey Shumkin","fromEmail":"alex.crezoff@gmail.com","sentAt":"2011-11-11T19:48:30Z","receivedAt":"2011-11-11T19:48:30Z","isPatch":true,"sender":{"key":"alex.crezoff@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1183752?v=4"},"body":"> >  test_expect_success \\\n> > +\t'Firefox below v2.0 paths are properly quoted' '\n> > +\techo fake: http://example.com/foo >expect &&\n> > +\tcat >\"fake browser\" <<-\\EOF &&\n> > +\t#!/bin/sh\n> > +\n> > +\tif [ \"$1\" == \"-version\" ]; then\n> \n> Using \"==\" is a bashism. Just use \"=\".\nThanks (I have no skills enough in this area)\n> \n> Also, a style nit, but we usually spell this \"test\" and not \"[\". I\n> admit I don't care much, though.\n\nOh, I see\n\n> \n> > +\t\t# Firefox (in contrast to w3m) is run in\n> > background (with &)\n> > +\t\t# so redirect output to \"actual\"\n> > +\t\techo fake: \"$@\" > actual\n> > +\tfi\n> > +\tEOF\n> > +\tchmod +x \"fake browser\" &&\n> > +\tgit config browser.firefox.path \"`pwd`/fake browser\" &&\n> > +\tgit web--browse --browser=firefox \\\n> > +\t\thttp://example.com/foo &&\n> > +\ttest_cmp expect actual\n> \n> Hmm. So we are running the fake browser in the background, but then\n> check that it has written something as soon as web--browse exits.\n> Isn't that a race condition? I.e., we could run \"test_cmp\" before the\n> browser has actually written anything?\neeehh... you're right...\nbut even on slow Windows Cygwin it is passed )\n\n> I'm not sure there's a good way to do it.  You would need either to\n> wait some pre-determined \"it could not possibly take it longer than N\n> seconds to run\" sleep, or we need some kind of synchronization point.\n> We can't wait call \"wait\" on the child PID (if we even have it,\n> because it's not our child).\nhmm... we can delete \"actual\" file and wait its appearance (with\nsome timeout), no ? but I didn't see in tests anything like this\n> -Peff\n"},{"id":"179326","messageId":"20111111202636.GA20515@sigill.intra.peff.net","threadId":"28549","inReplyTo":"20111111234830.32dccd87@zappedws","subject":"Re: [PATCH] git-web--browser: avoid errors in terminal when running Firefox on Windows","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-11-11T20:26:36Z","receivedAt":"2011-11-11T20:26:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 11, 2011 at 11:48:30PM +0400, Alexey Shumkin wrote:\n\n> > I'm not sure there's a good way to do it.  You would need either to\n> > wait some pre-determined \"it could not possibly take it longer than N\n> > seconds to run\" sleep, or we need some kind of synchronization point.\n> > We can't wait call \"wait\" on the child PID (if we even have it,\n> > because it's not our child).\n> hmm... we can delete \"actual\" file and wait its appearance (with\n> some timeout), no ? but I didn't see in tests anything like this\n\nEven that's not foolproof, as the open and write are not atomic (so you\ncould see it's there, but read an empty file). But in this case, we\nreally just care that the thing ran, not that it writes any specific\noutput. So you could probably get away with something like:\n\n  cat >fake-browser <<\\EOF &&\n  #!/bin/sh\n  >fake-browser-ran\n  EOF\n  git web--browse ... &&\n  {\n    for timeout in 1 2 3 4 5; do\n          test -f fake-browser-ran && break\n          sleep 1\n    done\n    test \"$timeout\" -ne 5\n  }\n\nwhich would note success as soon as possible (to within a one second\nmargin), but would eventually give up after 5 seconds. So you'd get a\nfalse positive on a _very_ loaded system, but that's kind of unlikely.\n\nI dunno. Maybe this hackery is OK, or maybe it just isn't worth it, and\nwe should declare this as something that's too hard to test to make it\ninto our test suite.\n\n-Peff\n"},{"id":"207812","messageId":"3eeabf4989f7f1b4593e89e4c6bcfa8710a7b793.1359125053.git.Alex.Crezoff@gmail.com","threadId":"28549","inReplyTo":"20111111202636.GA20515@sigill.intra.peff.net","subject":"[PATCH] git-web--browser: avoid errors in terminal when running Firefox on Windows","fromName":"Alexey Shumkin","fromEmail":"alex.crezoff@gmail.com","sentAt":"2013-01-25T14:44:13Z","receivedAt":"2013-01-25T14:44:13Z","isPatch":true,"sender":{"key":"alex.crezoff@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1183752?v=4"},"body":"Firefox on Windows by default is placed in \"C:\\Program Files\\Mozilla Firefox\"\nfolder, i.e. its path contains spaces. Before running this browser \"git-web--browse\"\ntests version of Firefox to decide whether to use \"-new-tab\" option or not.\n\nQuote browser path to avoid error during this test.\n\nSigned-off-by: Alexey Shumkin <Alex.Crezoff@gmail.com>\nReviewed-by: Jeff King <peff@peff.net>\n---\n git-web--browse.sh         |  2 +-\n t/t9901-git-web--browse.sh | 57 +++++++++++++++++++++++++++++++++++++++++++++-\n 2 files changed, 57 insertions(+), 2 deletions(-)\n\ndiff --git a/git-web--browse.sh b/git-web--browse.sh\nindex 1e82726..f96e5bd 100755\n--- a/git-web--browse.sh\n+++ b/git-web--browse.sh\n@@ -149,7 +149,7 @@ fi\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+\tvers=$(expr \"$(\"$browser_path\" -version)\" : '.* \\([0-9][0-9]*\\)\\..*')\n \tNEWTAB='-new-tab'\n \ttest \"$vers\" -lt 2 && NEWTAB=''\n \t\"$browser_path\" $NEWTAB \"$@\" &\ndiff --git a/t/t9901-git-web--browse.sh b/t/t9901-git-web--browse.sh\nindex b0a6bad..30d5294 100755\n--- a/t/t9901-git-web--browse.sh\n+++ b/t/t9901-git-web--browse.sh\n@@ -8,8 +8,21 @@ This test checks that git web--browse can handle various valid URLs.'\n . ./test-lib.sh\n \n test_web_browse () {\n-\t# browser=$1 url=$2\n+\t# browser=$1 url=$2 sleep_timeout=$3\n+\tsleep_timeout=\"$3\"\n \tgit web--browse --browser=\"$1\" \"$2\" >actual &&\n+\t# if $3 is set\n+\t# as far as Firefox is run in background (it is run with &)\n+\t# we trying to avoid race condition\n+\t# by waiting for \"$sleep_timeout\" seconds of timeout for 'fake_browser_ran' file appearance\n+\t(test -z \"$sleep_timeout\" || (\n+\t    for timeout in $(seq 1 $sleep_timeout); do\n+\t\t\ttest -f fake_browser_ran && break\n+\t\t\tsleep 1\n+\t\tdone\n+\t\ttest $timeout -ne $sleep_timeout\n+\t\t)\n+\t) &&\n \ttr -d '\\015' <actual >text &&\n \ttest_cmp expect text\n }\n@@ -48,6 +61,48 @@ test_expect_success \\\n '\n \n test_expect_success \\\n+\t'Firefox below v2.0 paths are properly quoted' '\n+\techo fake: http://example.com/foo >expect &&\n+\trm -f fake_browser_ran &&\n+\tcat >\"fake browser\" <<-\\EOF &&\n+\t#!/bin/sh\n+\n+\t: > fake_browser_ran\n+\tif test \"$1\" = \"-version\"; then\n+\t\techo Fake Firefox browser version 1.2.3\n+\telse\n+\t\t# Firefox (in contrast to w3m) is run in background (with &)\n+\t\t# so redirect output to \"actual\"\n+\t\techo fake: \"$@\" > actual\n+\tfi\n+\tEOF\n+\tchmod +x \"fake browser\" &&\n+\tgit config browser.firefox.path \"`pwd`/fake browser\" &&\n+\ttest_web_browse firefox http://example.com/foo 5\n+'\n+\n+test_expect_success \\\n+\t'Firefox not lower v2.0 paths are properly quoted' '\n+\techo fake: -new-tab http://example.com/foo >expect &&\n+\trm -f fake_browser_ran &&\n+\tcat >\"fake browser\" <<-\\EOF &&\n+\t#!/bin/sh\n+\n+\t: > fake_browser_ran\n+\tif test \"$1\" = \"-version\"; then\n+\t\techo Fake Firefox browser version 2.0.0\n+\telse\n+\t\t# Firefox (in contrast to w3m) is run in background (with &)\n+\t\t# so redirect output to \"actual\"\n+\t\techo fake: \"$@\" > actual\n+\tfi\n+\tEOF\n+\tchmod +x \"fake browser\" &&\n+\tgit config browser.firefox.path \"`pwd`/fake browser\" &&\n+\ttest_web_browse firefox http://example.com/foo 5\n+'\n+\n+test_expect_success \\\n \t'browser command allows arbitrary shell code' '\n \techo \"arg: http://example.com/foo\" >expect &&\n \tgit config browser.custom.cmd \"\n-- \n1.8.1.1.10.g9255f3f\n"},{"id":"207828","messageId":"7v1ud93uw8.fsf@alter.siamese.dyndns.org","threadId":"28549","inReplyTo":"3eeabf4989f7f1b4593e89e4c6bcfa8710a7b793.1359125053.git.Alex.Crezoff@gmail.com","subject":"Re: [PATCH] git-web--browser: avoid errors in terminal when running Firefox on Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-25T19:49:43Z","receivedAt":"2013-01-25T19:49:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alexey Shumkin <alex.crezoff@gmail.com> writes:\n\n> Firefox on Windows by default is placed in \"C:\\Program Files\\Mozilla Firefox\"\n> folder, i.e. its path contains spaces. Before running this browser \"git-web--browse\"\n> tests version of Firefox to decide whether to use \"-new-tab\" option or not.\n>\n> Quote browser path to avoid error during this test.\n>\n> Signed-off-by: Alexey Shumkin <Alex.Crezoff@gmail.com>\n> Reviewed-by: Jeff King <peff@peff.net>\n\nThanks, both.\n\n> ---\n>  git-web--browse.sh         |  2 +-\n>  t/t9901-git-web--browse.sh | 57 +++++++++++++++++++++++++++++++++++++++++++++-\n>  2 files changed, 57 insertions(+), 2 deletions(-)\n>\n> diff --git a/git-web--browse.sh b/git-web--browse.sh\n> index 1e82726..f96e5bd 100755\n> --- a/git-web--browse.sh\n> +++ b/git-web--browse.sh\n> @@ -149,7 +149,7 @@ fi\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> +\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\n\n> diff --git a/t/t9901-git-web--browse.sh b/t/t9901-git-web--browse.sh\n> index b0a6bad..30d5294 100755\n> --- a/t/t9901-git-web--browse.sh\n> +++ b/t/t9901-git-web--browse.sh\n> @@ -8,8 +8,21 @@ This test checks that git web--browse can handle various valid URLs.'\n>  . ./test-lib.sh\n>  \n>  test_web_browse () {\n> -\t# browser=$1 url=$2\n> +\t# browser=$1 url=$2 sleep_timeout=$3\n> +\tsleep_timeout=\"$3\"\n>  \tgit web--browse --browser=\"$1\" \"$2\" >actual &&\n> +\t# if $3 is set\n> +\t# as far as Firefox is run in background (it is run with &)\n> +\t# we trying to avoid race condition\n> +\t# by waiting for \"$sleep_timeout\" seconds of timeout for 'fake_browser_ran' file appearance\n> +\t(test -z \"$sleep_timeout\" || (\n> +\t    for timeout in $(seq 1 $sleep_timeout); do\n> +\t\t\ttest -f fake_browser_ran && break\n> +\t\t\tsleep 1\n> +\t\tdone\n> +\t\ttest $timeout -ne $sleep_timeout\n> +\t\t)\n> +\t) &&\n\nStyle:\n\n - do/then/else begin a new line (a good rule of thumb is remember\n   this rule is to write control structures without using\n   semicolon).\n\n - do not use \"seq\"; it is not available in some places.\n\nI do not think of a reason why you want ( nested (subshell) ), but\nif you don't need them, perhaps I'd write the above this way:\n\n\tif test -n $sleep_timeout\n\tthen\n\t\tfor timeout in $(test_seq $sleep_timeout)\n\t\tdo\n\t\t\ttest -f fake_browser_ran && break\n\t\t\tsleep 1\n\t\tdone\n\t\ttest $timeout -ne $sleep_timeout\n\tfi &&\n\n> @@ -48,6 +61,48 @@ test_expect_success \\\n>  '\n>  \n>  test_expect_success \\\n> +\t'Firefox below v2.0 paths are properly quoted' '\n\n-ECANNOTPARSE.\n\n\"Paths to firefox older than v2.0 are properly quoted\" you mean,\nperhaps?  I dunno.\n\n> +\techo fake: http://example.com/foo >expect &&\n> +\trm -f fake_browser_ran &&\n> +\tcat >\"fake browser\" <<-\\EOF &&\n> +\t#!/bin/sh\n\nConsider using \"write_script\" helper so that you get the path to the\nshell the user specified via $SHELL_PATH.\n\n> +\n> +\t: > fake_browser_ran\n\nStyle: no SP between redirection operator and filename, i.e.\n\n\t: >fake_browser_ran\n\n> +\tif test \"$1\" = \"-version\"; then\n\nStyle (see above).\n\n> +\t\techo Fake Firefox browser version 1.2.3\n> +\telse\n> +\t\t# Firefox (in contrast to w3m) is run in background (with &)\n> +\t\t# so redirect output to \"actual\"\n> +\t\techo fake: \"$@\" > actual\n\nStyle (see above).\n\n> +\tfi\n> +\tEOF\n> +\tchmod +x \"fake browser\" &&\n> +\tgit config browser.firefox.path \"`pwd`/fake browser\" &&\n\nWe tend to prefer $(pwd) over `pwd`.\n\n> +\ttest_web_browse firefox http://example.com/foo 5\n> +'\n> +\n> +test_expect_success \\\n> +\t'Firefox not lower v2.0 paths are properly quoted' '\n\ns/not lower v2.0/v2.0 and above/, but again -ECANNOTPARSE.\n\n> +\techo fake: -new-tab http://example.com/foo >expect &&\n\nI'd feel safer if you quoted the arguments to \"echo\", i.e.\n\n\techo \"fake: -new-tab http://example.com/foo\" >expect &&\n\nThe same style comments as above apply to the remainder of patch.\n\nThanks.\n"},{"id":"207845","messageId":"20130125220617.GA23626@sigill.intra.peff.net","threadId":"28549","inReplyTo":"3eeabf4989f7f1b4593e89e4c6bcfa8710a7b793.1359125053.git.Alex.Crezoff@gmail.com","subject":"Re: [PATCH] git-web--browser: avoid errors in terminal when running Firefox on Windows","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-25T22:06:17Z","receivedAt":"2013-01-25T22:06:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 25, 2013 at 06:44:13PM +0400, Alexey Shumkin wrote:\n\n>  test_web_browse () {\n> -\t# browser=$1 url=$2\n> +\t# browser=$1 url=$2 sleep_timeout=$3\n> +\tsleep_timeout=\"$3\"\n>  \tgit web--browse --browser=\"$1\" \"$2\" >actual &&\n> +\t# if $3 is set\n> +\t# as far as Firefox is run in background (it is run with &)\n> +\t# we trying to avoid race condition\n> +\t# by waiting for \"$sleep_timeout\" seconds of timeout for 'fake_browser_ran' file appearance\n> +\t(test -z \"$sleep_timeout\" || (\n> +\t    for timeout in $(seq 1 $sleep_timeout); do\n> +\t\t\ttest -f fake_browser_ran && break\n> +\t\t\tsleep 1\n> +\t\tdone\n> +\t\ttest $timeout -ne $sleep_timeout\n> +\t\t)\n> +\t) &&\n>  \ttr -d '\\015' <actual >text &&\n\nGross, but I don't really see another way to handle the asynchronous\nnature of spawning background browsers.\n\nTwo things, though:\n\n  1. Should test_web_browse just delete fake_browser_ran for us? Then\n     later tests do not have to remember to do so.\n\n  2. Seeing fake_browser_ran appeared, we know that the script has\n     started.  But there is still a race condition in which it may not\n     have written anything to \"actual\" yet.\n\nIn this implementation:\n\n> +\tcat >\"fake browser\" <<-\\EOF &&\n> +\t#!/bin/sh\n> +\n> +\t: > fake_browser_ran\n> +\tif test \"$1\" = \"-version\"; then\n> +\t\techo Fake Firefox browser version 1.2.3\n> +\telse\n> +\t\t# Firefox (in contrast to w3m) is run in background (with &)\n> +\t\t# so redirect output to \"actual\"\n> +\t\techo fake: \"$@\" > actual\n> +\tfi\n> +\tEOF\n\nThere is a period where fake_browser_ran exists, but nothing is in\nactual. You can solve it by setting fake_browser_ran at the end rather\nthan the beginning.\n\nOr you can drop fake_browser_ran entirely, and just atomically move\nactual into place, like:\n\n  echo \"fake: $*\" >actual.tmp\n  mv actual.tmp actual\n\nand then test_web_browse can just spin waiting for \"actual\" to appear.\n\n-Peff\n"},{"id":"207851","messageId":"CAEFUfsH9Xdd4R-uHZGYH4jXvv_z2SzRmtaZwj0_o0d4A9ynPBg@mail.gmail.com","threadId":"28549","inReplyTo":"20130125220617.GA23626@sigill.intra.peff.net","subject":"Re: [PATCH] git-web--browser: avoid errors in terminal when running Firefox on Windows","fromName":"Shumkin Alexey","fromEmail":"alex.crezoff@gmail.com","sentAt":"2013-01-25T22:52:50Z","receivedAt":"2013-01-25T22:52:50Z","isPatch":true,"sender":{"key":"alex.crezoff@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1183752?v=4"},"body":"2013/1/26 Jeff King <peff@peff.net>:\n> On Fri, Jan 25, 2013 at 06:44:13PM +0400, Alexey Shumkin wrote:\n>\n>>  test_web_browse () {\n>> -     # browser=$1 url=$2\n>> +     # browser=$1 url=$2 sleep_timeout=$3\n>> +     sleep_timeout=\"$3\"\n>>       git web--browse --browser=\"$1\" \"$2\" >actual &&\n>> +     # if $3 is set\n>> +     # as far as Firefox is run in background (it is run with &)\n>> +     # we trying to avoid race condition\n>> +     # by waiting for \"$sleep_timeout\" seconds of timeout for\n>> 'fake_browser_ran' file appearance\n>> +     (test -z \"$sleep_timeout\" || (\n>> +         for timeout in $(seq 1 $sleep_timeout); do\n>> +                     test -f fake_browser_ran && break\n>> +                     sleep 1\n>> +             done\n>> +             test $timeout -ne $sleep_timeout\n>> +             )\n>> +     ) &&\n>>       tr -d '\\015' <actual >text &&\n>\n> Gross, but I don't really see another way to handle the asynchronous\n> nature of spawning background browsers.\n>\n> Two things, though:\n>\n>   1. Should test_web_browse just delete fake_browser_ran for us? Then\n>      later tests do not have to remember to do so.\nYep, you're right\n>\n>   2. Seeing fake_browser_ran appeared, we know that the script has\n>      started.  But there is still a race condition in which it may not\n>      have written anything to \"actual\" yet.\nDefinitely right\n>\n> In this implementation:\n>\n>> +     cat >\"fake browser\" <<-\\EOF &&\n>> +     #!/bin/sh\n>> +\n>> +     : > fake_browser_ran\n>> +     if test \"$1\" = \"-version\"; then\n>> +             echo Fake Firefox browser version 1.2.3\n>> +     else\n>> +             # Firefox (in contrast to w3m) is run in background (with\n>> &)\n>> +             # so redirect output to \"actual\"\n>> +             echo fake: \"$@\" > actual\n>> +     fi\n>> +     EOF\n>\n> There is a period where fake_browser_ran exists, but nothing is in\n> actual. You can solve it by setting fake_browser_ran at the end rather\n> than the beginning.\n>\n> Or you can drop fake_browser_ran entirely, and just atomically move\n> actual into place, like:\n>\n>   echo \"fake: $*\" >actual.tmp\n>   mv actual.tmp actual\n>\n> and then tes-t_web_browse can just spin waiting for \"actual\" to appear.\nNot exactly, because, as I see, \"actual\" file is a result of redirection of\n> git web--browse --browser=\"$1\" \"$2\" >actual &&\ncommand\n>\n> -Peff\n"},{"id":"207857","messageId":"cover.1359160531.git.Alex.Crezoff@gmail.com","threadId":"28549","inReplyTo":"7v1ud93uw8.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2 0/2] git-web--browser: avoid errors in terminal when running","fromName":"Alexey Shumkin","fromEmail":"alex.crezoff@gmail.com","sentAt":"2013-01-26T00:40:31Z","receivedAt":"2013-01-26T00:40:31Z","isPatch":true,"sender":{"key":"alex.crezoff@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1183752?v=4"},"body":"Reroll patch after all suggestions\n\nAlexey Shumkin (2):\n  t9901-git-web--browse.sh: Use \"write_script\" helper\n  git-web--browser: avoid errors in terminal when running Firefox on\n    Windows\n\n git-web--browse.sh         |  2 +-\n t/t9901-git-web--browse.sh | 59 ++++++++++++++++++++++++++++++++++++++++++----\n 2 files changed, 55 insertions(+), 6 deletions(-)\n\n-- \n1.8.1.1.10.g71fa0b7\n"},{"id":"207859","messageId":"c4cde7280a34d1a636c2eb93cfec6ce2f4ac222f.1359160531.git.Alex.Crezoff@gmail.com","threadId":"28549","inReplyTo":"cover.1359160531.git.Alex.Crezoff@gmail.com","subject":"[PATCH v2 1/2] t9901-git-web--browse.sh: Use \"write_script\" helper","fromName":"Alexey Shumkin","fromEmail":"alex.crezoff@gmail.com","sentAt":"2013-01-26T00:40:32Z","receivedAt":"2013-01-26T00:40:32Z","isPatch":true,"sender":{"key":"alex.crezoff@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1183752?v=4"},"body":"Use \"write_script\" helper as suggested by Junio C Hamano.\nAlso, replace `pwd` with $(pwd) call convention.\n\nSuggested-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Alexey Shumkin <Alex.Crezoff@gmail.com>\n---\n t/t9901-git-web--browse.sh | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t9901-git-web--browse.sh b/t/t9901-git-web--browse.sh\nindex b0a6bad..b0dabf7 100755\n--- a/t/t9901-git-web--browse.sh\n+++ b/t/t9901-git-web--browse.sh\n@@ -38,12 +38,10 @@ test_expect_success \\\n test_expect_success \\\n \t'browser paths are properly quoted' '\n \techo fake: http://example.com/foo >expect &&\n-\tcat >\"fake browser\" <<-\\EOF &&\n-\t#!/bin/sh\n+\twrite_script \"fake browser\" <<-\\EOF &&\n \techo fake: \"$@\"\n \tEOF\n-\tchmod +x \"fake browser\" &&\n-\tgit config browser.w3m.path \"`pwd`/fake browser\" &&\n+\tgit config browser.w3m.path \"$(pwd)/fake browser\" &&\n \ttest_web_browse w3m http://example.com/foo\n '\n \n-- \n1.8.1.1.10.g71fa0b7\n"},{"id":"207858","messageId":"b62d19fa972caa17054502168ae4d153c05b0363.1359160531.git.Alex.Crezoff@gmail.com","threadId":"28549","inReplyTo":"cover.1359160531.git.Alex.Crezoff@gmail.com","subject":"[PATCH v2 2/2] git-web--browser: avoid errors in terminal when running Firefox on Windows","fromName":"Alexey Shumkin","fromEmail":"alex.crezoff@gmail.com","sentAt":"2013-01-26T00:40:33Z","receivedAt":"2013-01-26T00:40:33Z","isPatch":true,"sender":{"key":"alex.crezoff@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1183752?v=4"},"body":"Firefox on Windows by default is placed in \"C:\\Program Files\\Mozilla Firefox\"\nfolder, i.e. its path contains spaces. Before running this browser \"git-web--browse\"\ntests version of Firefox to decide whether to use \"-new-tab\" option or not.\n\nQuote browser path to avoid error during this test.\n\nSigned-off-by: Alexey Shumkin <Alex.Crezoff@gmail.com>\nReviewed-by: Jeff King <peff@peff.net>\n---\n git-web--browse.sh         |  2 +-\n t/t9901-git-web--browse.sh | 53 +++++++++++++++++++++++++++++++++++++++++++++-\n 2 files changed, 53 insertions(+), 2 deletions(-)\n\ndiff --git a/git-web--browse.sh b/git-web--browse.sh\nindex 1e82726..f96e5bd 100755\n--- a/git-web--browse.sh\n+++ b/git-web--browse.sh\n@@ -149,7 +149,7 @@ fi\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+\tvers=$(expr \"$(\"$browser_path\" -version)\" : '.* \\([0-9][0-9]*\\)\\..*')\n \tNEWTAB='-new-tab'\n \ttest \"$vers\" -lt 2 && NEWTAB=''\n \t\"$browser_path\" $NEWTAB \"$@\" &\ndiff --git a/t/t9901-git-web--browse.sh b/t/t9901-git-web--browse.sh\nindex b0dabf7..c1ee813 100755\n--- a/t/t9901-git-web--browse.sh\n+++ b/t/t9901-git-web--browse.sh\n@@ -8,8 +8,23 @@ This test checks that git web--browse can handle various valid URLs.'\n . ./test-lib.sh\n \n test_web_browse () {\n-\t# browser=$1 url=$2\n+\t# browser=$1 url=$2 sleep_timeout=$3\n+\tsleep_timeout=\"$3\"\n+\trm -f fake_browser_ran &&\n \tgit web--browse --browser=\"$1\" \"$2\" >actual &&\n+\t# if $3 is set\n+\t# as far as Firefox is run in background (it is run with &)\n+\t# we trying to avoid race condition\n+\t# by waiting for \"$sleep_timeout\" seconds of timeout for 'fake_browser_ran' file appearance\n+\tif test -n \"$sleep_timeout\"\n+\tthen\n+\t    for timeout in $(test_seq $sleep_timeout)\n+\t\tdo\n+\t\t\ttest -f fake_browser_ran && break\n+\t\t\tsleep 1\n+\t\tdone\n+\t\ttest $timeout -ne $sleep_timeout\n+\tfi &&\n \ttr -d '\\015' <actual >text &&\n \ttest_cmp expect text\n }\n@@ -46,6 +61,42 @@ test_expect_success \\\n '\n \n test_expect_success \\\n+\t'Paths are properly quoted for Firefox. Version older then v2.0' '\n+\techo \"fake: http://example.com/foo\" >expect &&\n+\twrite_script \"fake browser\" <<-\\EOF &&\n+\n+\tif test \"$1\" = \"-version\"; then\n+\t\techo \"Fake Firefox browser version 1.2.3\"\n+\telse\n+\t\t# Firefox (in contrast to w3m) is run in background (with &)\n+\t\t# so redirect output to \"actual\"\n+\t\techo \"fake: \"\"$@\" >actual\n+\tfi\n+\t: >fake_browser_ran\n+\tEOF\n+\tgit config browser.firefox.path \"$(pwd)/fake browser\" &&\n+\ttest_web_browse firefox http://example.com/foo 5\n+'\n+\n+test_expect_success \\\n+\t'Paths are properly quoted for Firefox. Version v2.0 and above' '\n+\techo \"fake: -new-tab http://example.com/foo\" >expect &&\n+\twrite_script \"fake browser\" <<-\\EOF &&\n+\n+\tif test \"$1\" = \"-version\"; then\n+\t\techo \"Fake Firefox browser version 2.0.0\"\n+\telse\n+\t\t# Firefox (in contrast to w3m) is run in background (with &)\n+\t\t# so redirect output to \"actual\"\n+\t\techo \"fake: \"\"$@\" >actual\n+\tfi\n+\t: >fake_browser_ran\n+\tEOF\n+\tgit config browser.firefox.path \"$(pwd)/fake browser\" &&\n+\ttest_web_browse firefox http://example.com/foo 5\n+'\n+\n+test_expect_success \\\n \t'browser command allows arbitrary shell code' '\n \techo \"arg: http://example.com/foo\" >expect &&\n \tgit config browser.custom.cmd \"\n-- \n1.8.1.1.10.g71fa0b7\n"}]}