From: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
Date: Fri, 03 Dec 2010 22:25:09 GMT
Subject: Re: [PATCHv2 5/7] web--browse: better support for chromium
Message-ID: <AANLkTinGzZQZo9yZtVtztu5CEXT8G10H6=EdbSM3PAwh@mail.gmail.com>
In-Reply-To: <7vipzamsde.fsf@alter.siamese.dyndns.org>

On Fri, Dec 3, 2010 at 10:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>
>>  init_browser_path() {
>>       browser_path=$(git config "browser.$1.path")
>> +     if test -z "$browser_path" -a "$1" = chromium ; then
>> +             type chromium-browser > /dev/null 2>&1 && browser_path=chromium-browser
>> +     fi
>>       test -z "$browser_path" && browser_path="$1"
>
> We tolerate
>
>        test && test && effect
>
> and even encourage when the construct is short enough, over
>
>        if test && test
>        then
>                effect
>        fi
>
> But because you are writing an "if" block anyway, I think the above should
> be like this:
>
>        if test -z "$browser_path" &&
>           test "$1" = chromium &&
>           type chromium-browser >/dev/null 2>&1
>        then
>                browser_path=chromium-browser
>        fi
>        browser_path=${browser_path:-"$1"}
>
> Yours is:
>
>        if test && test
>        then
>                test && effect
>        fi
>
> which is less than readable, no?

Absolutely. Actually, this final form is a side-effect on some
experimentations I was doing, thinking about doing something similar
with google-chrome vs chrome (in which case the body of the if would
get a case switch or something like that). I'll clean it up, unless
somebody can actually confirm that we do want these other special
cases.

-- 
Giuseppe "Oblomov" Bilotta