Re: [PATCH] instaweb: make it compatible with Mac OS X 10.5's apache installation.

From: nathan spindel <nathans@gmail.com>
Date: May 11, 2008, 17:11 UTC
Message-ID: <843D3D0E-0121-42E2-B285-E1F4AB9415E4@gmail.com>
In-Reply-To: <7vhcd572e3.fsf@gitster.siamese.dyndns.org>

On May 10, 2008, at 11:58 PM, Junio C Hamano wrote:

Show 9 quoted lines

> nathan spindel <nathans@gmail.com> writes:
>
>> When in apache2 mode if there isn't an apache2 binary on the system  
>> but
>> there is a httpd command in /usr/sbin/ (like there is on Mac OS X)
>> use that instead.
>
> How would you ensure that httpd is actually Apache and not something  
> else?

Good point. How about running it with -v and searching for Apache in the first line?

Show 10 quoted lines

>> +	httpd_only="`echo $httpd | cut -f1 -d' '`"
>> +	type $httpd_only > /dev/null 2>&1;
>> +	test $? != 0 && test -x /usr/sbin/httpd && httpd=${httpd/apache2/ 
>> httpd}
>
> I see the same "type" issue as I mentioned, but I see that we use  
> them in
> mergetool and web--browse and we haven't heard breakages so perhaps  
> this
> is portable enough ;-)

Yes, inspection of mergetool and web--browse were what led me to use type. I will see if the other method you suggested can work and use that here if so.

> Please avoid ${parameter/pattern/string} expansion, which is not  
> even in
> POSIX.  It is bashism and unportable.

I didn't know that was just a bashism. I'll fix that usage.

Show 8 quoted lines

>> +	if test -z "$module_path"
>> +	then
>> +		for path in /usr/lib/apache2/modules /usr/libexec/apache2; do
>
> Hmm.  If you do discovery like this, maybe you would want to do  
> discovery
> for "httpd" the same way?  After all, why look for it only in /usr/ 
> sbin?

Yes, it should look in multiple locations.