Re: [PATCH] git-web--browser: avoid errors in terminal when running Firefox on Windows

From: Jeff King <peff@peff.net>
Date: Jan 25, 2013, 22:06 UTC
Message-ID: <20130125220617.GA23626@sigill.intra.peff.net>
In-Reply-To: <3eeabf4989f7f1b4593e89e4c6bcfa8710a7b793.1359125053.git.Alex.Crezoff@gmail.com>

On Fri, Jan 25, 2013 at 06:44:13PM +0400, Alexey Shumkin wrote:

Show 18 quoted lines

>  test_web_browse () {
> -	# browser=$1 url=$2
> +	# browser=$1 url=$2 sleep_timeout=$3
> +	sleep_timeout="$3"
>  	git web--browse --browser="$1" "$2" >actual &&
> +	# if $3 is set
> +	# as far as Firefox is run in background (it is run with &)
> +	# we trying to avoid race condition
> +	# by waiting for "$sleep_timeout" seconds of timeout for 'fake_browser_ran' file appearance
> +	(test -z "$sleep_timeout" || (
> +	    for timeout in $(seq 1 $sleep_timeout); do
> +			test -f fake_browser_ran && break
> +			sleep 1
> +		done
> +		test $timeout -ne $sleep_timeout
> +		)
> +	) &&
>  	tr -d '\015' <actual >text &&

Gross, but I don't really see another way to handle the asynchronous nature of spawning background browsers.

Two things, though:

  1. Should test_web_browse just delete fake_browser_ran for us? Then
     later tests do not have to remember to do so.

  2. Seeing fake_browser_ran appeared, we know that the script has
     started.  But there is still a race condition in which it may not
     have written anything to "actual" yet.

In this implementation:

Show 12 quoted lines

> +	cat >"fake browser" <<-\EOF &&
> +	#!/bin/sh
> +
> +	: > fake_browser_ran
> +	if test "$1" = "-version"; then
> +		echo Fake Firefox browser version 1.2.3
> +	else
> +		# Firefox (in contrast to w3m) is run in background (with &)
> +		# so redirect output to "actual"
> +		echo fake: "$@" > actual
> +	fi
> +	EOF

There is a period where fake_browser_ran exists, but nothing is in actual. You can solve it by setting fake_browser_ran at the end rather than the beginning.

Or you can drop fake_browser_ran entirely, and just atomically move actual into place, like:

  echo "fake: $*" >actual.tmp
  mv actual.tmp actual

and then test_web_browse can just spin waiting for "actual" to appear.

-Peff