Re: [PATCHv2 5/7] web--browse: better support for chromium
- From
Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
- Date
- Dec 3, 2010, 22:25 UTC
- 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:
Show 39 quoted lines
> 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