Re: [PATCH] git-web--browser: avoid errors in terminal when running Firefox on Windows
- From
Jeff King <peff@peff.net>
- Date
- Nov 11, 2011, 18:35 UTC
- Message-ID
- <20111111183555.GC16055@sigill.intra.peff.net>
- In-Reply-To
- <1321028283-17307-1-git-send-email-Alex.Crezoff@gmail.com>
On Fri, Nov 11, 2011 at 08:18:03PM +0400, Alexey Shumkin wrote:
Show 5 quoted lines
> Firefox on Windows by default is placed in "C:\Program Files\Mozilla Firefox" > folder, i.e. its path contains spaces. Before running this browser "git-web--browse" > tests version of Firefox to decide whether to use "-new-tab" option or not. > > Quote browser path to avoid error during this test.
Thanks. I even noticed this bug early on in the previous discussion:
http://article.gmane.org/gmane.comp.version-control.git/181600
but forgot about it by the time the final patch rolled around. Your fix looks correct, but:
Show 7 quoted lines
> test_expect_success \ > + 'Firefox below v2.0 paths are properly quoted' ' > + echo fake: http://example.com/foo >expect && > + cat >"fake browser" <<-\EOF && > + #!/bin/sh > + > + if [ "$1" == "-version" ]; then
Using "==" is a bashism. Just use "=".
Also, a style nit, but we usually spell this "test" and not "[". I admit I don't care much, though.
Show 10 quoted lines
> + # Firefox (in contrast to w3m) is run in background (with &) > + # so redirect output to "actual" > + echo fake: "$@" > actual > + fi > + EOF > + chmod +x "fake browser" && > + git config browser.firefox.path "`pwd`/fake browser" && > + git web--browse --browser=firefox \ > + http://example.com/foo && > + test_cmp expect actual
Hmm. So we are running the fake browser in the background, but then check that it has written something as soon as web--browse exits. Isn't that a race condition? I.e., we could run "test_cmp" before the browser has actually written anything?
I'm not sure there's a good way to do it. You would need either to wait some pre-determined "it could not possibly take it longer than N seconds to run" sleep, or we need some kind of synchronization point. We can't wait call "wait" on the child PID (if we even have it, because it's not our child).
-Peff