Re: [PATCH 5/6] web--browse: use (x-)www-browser if available

From: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
Date: Dec 1, 2010, 11:46 UTC
Message-ID: <AANLkTikfAr-KUVyqeTooxYKvKWYM4pRmw51if+349dpJ@mail.gmail.com>
In-Reply-To: <AANLkTinzCeaiFoL4a-+c6wuJoUQ68zC9vE8AoHfmvN-F@mail.gmail.com>

On Wed, Dec 1, 2010 at 11:59 AM, Christian Couder <christian.couder@gmail.com> wrote:

Show 28 quoted lines

> On Tue, Nov 30, 2010 at 9:22 AM, Giuseppe Bilotta
> <giuseppe.bilotta@gmail.com> wrote:
>> On Tue, Nov 30, 2010 at 5:02 AM, Christian Couder
>> <christian.couder@gmail.com> wrote:
>>> On Mon, Nov 29, 2010 at 8:05 PM, Giuseppe Bilotta
>>> <giuseppe.bilotta@gmail.com> wrote:
>>>> We allow valid_tool to be false in the x-www-browser case, in which
>>>> case we test www-browser, and if it's still not valid we go on and use
>>>> the previous paths. So we cannot die in case of an invalid
>>>> (x-)www-browser.
>>>
>>> Yeah, you are right, but we could die after the "for i in $wwwbrowser"
>>> loop if both are invalid.
>>
>> Why? If both are invalid, proceeding with the previous strategy of
>> looking for a browser we _should_ be looking for any browser we know
>> about, even if it's not set as the default system browser.
>
> Currently we have:
>
> 97     if test -n "$browser" && ! valid_tool "$browser"; then
> 98         echo >&2 "git config option $opt set to unknown browser: $browser"
> 99         echo >&2 "Resetting to default..."
> 100         unset browser
> 101     fi
>
> So if we want to be consistent with that behavior, we should probably do the
> same thing if (x-)www-browser is set but we don't support it.

The (x-)www-browser code is used when the user did _not_ define a browser. It is not a browser option (git web--browse --tool=www-browser). IOW, this is not about the user selecting an unsupported browser, but about the system default browser not being a supported one. I don't think it should be treated the same way.

Show 15 quoted lines

>>>> But there's a bug in the www-brower testing, it needs
>>>> an else that resets browser to the empty string.
>>>
>>> I thought it was by design that you did not reset it...
>>> So yeah it is clearer and nicer for the user if you either reset
>>> browser or just die if both (x-)www-browser are invalid. If you decide
>>> to reset browser, perhaps a warning or an information message telling
>>> that both are unknown would be nice.
>>
>> I can do that. Should it be a warning about reporting the lack of
>> support to us, or just a warning that we are not going to use it even
>> if it's defined?
>
> I think it should be a warning that we are not going to use it even
> if it's defined, to be consistent with the code pasted above.

Ok.

> If you think of something better or want to remove the warnings, please do
> it in another patch.

I think the warning is fine.

Show 12 quoted lines

>> While we're at it: I was considering adding support for the BROWSER
>> env var (a colon-separated list of browsers executables or "browser
>> %s" strings).
>>
>> All of this is going to make the web--browse script very similar to
>> the sensible-browser script in Debian, with the difference that we go
>> at length to ensure that stuff is opened in a new tab, whereas
>> Debian's sensible-browser doesn't. Should we just support
>> sensible-browser instead of (x-)www-browser/BROWSER, and let it open
>> anything?
>
> I think most users prefer to open stuff in a new tab if possible.

I will look into adding support for the BROWSER environment variable to select the browser if no browser is defined, then, but not consider sensible-browser.

-- 
Giuseppe "Oblomov" Bilotta