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. >>> 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. If you think of something better or want to remove the warnings, please do it in another patch. > 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. Thanks, Christian. -- 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