Re: [PATCH] fix handling of iconv configuration options

From: Marco Nelissen <marcone@xs4all.nl>
Date: Jun 8, 2009, 23:51 UTC
Message-ID: <3a3d9e520906081651r33b485c2v745c4275a25c0f58@mail.gmail.com>
In-Reply-To: <7vhbyqwg2y.fsf@alter.siamese.dyndns.org>

On Mon, Jun 8, 2009 at 2:50 PM, Junio C Hamano<gitster@pobox.com> wrote:

Show 36 quoted lines

> Marco Nelissen <marcone@xs4all.nl> writes:
>
>> diff --git a/configure.ac b/configure.ac
>> index 108a97f..3388036 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -385,6 +385,8 @@ AC_SUBST(NO_EXPAT)
>>  # some Solaris installations).
>>  # Define NO_ICONV if neither libc nor libiconv support iconv.
>>
>> +if test -z $NO_ICONV; then
>> +
>>  GIT_STASH_FLAGS($ICONVDIR)
>>
>>  AC_DEFUN([ICONVTEST_SRC], [
>> @@ -431,6 +433,12 @@ GIT_UNSTASH_FLAGS($ICONVDIR)
>>  AC_SUBST(NEEDS_LIBICONV)
>>  AC_SUBST(NO_ICONV)
>>
>> +if test -n $NO_ICONV; then
>> +    NEEDS_LIBICONV=
>> +fi
>> +
>> +fi
>> +
>
> Hmm, have you tested this with both NO_ICONV defined and undefined?
>
> Because ...
>
>        $ test -z ; echo $?
>        0
>        $ test -n ; echo $?
>        0
>
> ... I would feel better if you had dq around $NO_ICONV in both tests.

Ah, you're right. I tested that it didn't incorrectly set NEEDS_LIBICONV on a system that doesn't have/need it, but didn't test that it doesn't unset it on a system that does need it (because I don't have such a system).