Re: [PATCH 2/3] git-help: add -w|--web option to display html man page in a browser.

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 7, 2007, 06:04 UTC
Message-ID: <7v1w9z9h2k.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <200712070635.18018.chriscool@tuxfamily.org>

Christian Couder <chriscool@tuxfamily.org> writes:

Show 9 quoted lines

>> > +# Directories that may contain html documentation:
>> > +install_html_dir="$PREFIX/share/doc/git-doc"
>> > +rpm_dir="$PREFIX/share/doc/git-core-$GIT_VERSION"
>>
>> ... doing this is wrong. People can set htmldir to somewhere other than
>> $(prefix)/share/doc/git-doc while building and installing, but you are
>> not telling the munged script where it is.
>
> Yeah, I sent a fix for this.

Why do you even need to fallback? I'd rather drop these two fallbacks entirely.

Distros have their own html documentation layout policy, so I suspect they will patch this part to their liking anyway, and this point will mostly become moot. For source distribution, I'd prefer to point at the place we know we are installing in.

Show 25 quoted lines

>> > +init_browser_path() {
>> > +	browser_path=`git config browser.$1.path`
>> > +	test -z "$browser_path" && browser_path=$1
>> > +}
>>
>> Please do not contaminate the config file with something the user can
>> easily use a lot more standardized way (iow $PATH) to configure to his
>> taste.
>>
>> I'd suggest dropping this bit.
>
> I stole this part from "git-mergetool.sh":
>
> init_merge_tool_path() {
>         merge_tool_path=`git config mergetool.$1.path`
>         if test -z "$merge_tool_path" ; then
>                 case "$1" in
>                         emerge)
>                                 merge_tool_path=emacs
>                                 ;;
> ...
> }
>
> So we should either drop it in "git-mergetool.sh" too or keep it in both 
> scripts.

I think this is an irrelevant defense. If others are doing bad, that is not a justification to make things worse.

In the case of mergetool, it has case "$merge_tool" that can be spelled totally differently from path (e.g. emerge and emacs), so that function itself is semi justified. For browser I do not think there isn't such justification.