On Wed, Dec 1, 2010 at 11:59 AM, Christian Couder <christian.couder@xxxxxxxxx> wrote: > On Tue, Nov 30, 2010 at 9:22 AM, Giuseppe Bilotta > <giuseppe.bilotta@xxxxxxxxx> wrote: >> On Tue, Nov 30, 2010 at 5:02 AM, Christian Couder >> <christian.couder@xxxxxxxxx> wrote: >>> On Mon, Nov 29, 2010 at 8:05 PM, Giuseppe Bilotta >>> <giuseppe.bilotta@xxxxxxxxx> 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. >>>> 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. >> 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html