[PATCH] Mac OS X 10.5 does not require the OLD_ICONV flag set

[PATCH] Mac OS X 10.5 does not require the OLD_ICONV flag set

[Blake Ramsdell]

Signed-off-by: Blake Ramsdell <blaker@gmail.com>
---
 Makefile |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index 71479a2..5d83756 100644
--- a/Makefile
+++ b/Makefile
@@ -401,7 +401,9 @@ endif
 ifeq ($(uname_S),Darwin)
 	NEEDS_SSL_WITH_CRYPTO = YesPlease
 	NEEDS_LIBICONV = YesPlease
-	OLD_ICONV = UnfortunatelyYes
+	ifneq ($(uname_R),9.0.0)
+		OLD_ICONV = UnfortunatelyYes
+	endif
 	NO_STRLCPY = YesPlease
 	NO_MEMMEM = YesPlease
 endif
-- 
1.5.3.GIT

[PATCH] Fixed a gcc 4.0.1 complaint about an uninitialized variable

[Blake Ramsdell]

Signed-off-by: Blake Ramsdell <blaker@gmail.com>
---
 transport.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/transport.c b/transport.c
index 400af71..cac1870 100644
--- a/transport.c
+++ b/transport.c
@@ -107,7 +107,7 @@ static void insert_packed_refs(const char *packed_refs, struct ref **list)
 		return;
 
 	for (;;) {
-		int cmp, len;
+		int cmp = 0, len;
 
 		if (!fgets(buffer, sizeof(buffer), f)) {
 			fclose(f);
-- 
1.5.3.GIT

Re: [PATCH] Fixed a gcc 4.0.1 complaint about an uninitialized variable

[Junio C Hamano]

Blake Ramsdell <blaker@gmail.com> writes:

> diff --git a/transport.c b/transport.c
> index 400af71..cac1870 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -107,7 +107,7 @@ static void insert_packed_refs(const char *packed_refs, struct ref **list)
>  		return;
>  
>  	for (;;) {
> -		int cmp, len;
> +		int cmp = 0, len;

Yeah, if you follow the logic, it is clear that the variable is
never used while unset, but gcc is not careful enough to see it.

It is customary to use

	int cmp = cmp;

for something like this.  There are already other instances of
such phony initializations in the code elsewhere.

Re: [PATCH] Mac OS X 10.5 does not require the OLD_ICONV flag set

[Junio C Hamano]

Blake Ramsdell <blaker@gmail.com> writes:

> Signed-off-by: Blake Ramsdell <blaker@gmail.com>
> ---
>  Makefile |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 71479a2..5d83756 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -401,7 +401,9 @@ endif
>  ifeq ($(uname_S),Darwin)
>  	NEEDS_SSL_WITH_CRYPTO = YesPlease
>  	NEEDS_LIBICONV = YesPlease
> -	OLD_ICONV = UnfortunatelyYes
> +	ifneq ($(uname_R),9.0.0)
> +		OLD_ICONV = UnfortunatelyYes
> +	endif
>  	NO_STRLCPY = YesPlease
>  	NO_MEMMEM = YesPlease
>  endif

I do not have an access to a Darwin box, but do you mean 10.5
gives 9.0.0 as uname_R?

Re: [PATCH] Mac OS X 10.5 does not require the OLD_ICONV flag set

[Blake Ramsdell]

On 11/2/07, Junio C Hamano <gitster@pobox.com> wrote:
> I do not have an access to a Darwin box, but do you mean 10.5
> gives 9.0.0 as uname_R?

Yeah, apparently that's how they roll.

~/Source/OpenSource/git$ uname -r
9.0.0
~/Source/OpenSource/git$ system_profiler SPSoftwareDataType
Software:

    System Software Overview:

      System Version: Mac OS X 10.5 (9A581)
      Kernel Version: Darwin 9.0.0

Blake
-- 
Blake Ramsdell | http://www.blakeramsdell.com

Re: [PATCH] Mac OS X 10.5 does not require the OLD_ICONV flag set

[Mike Hommey]

On Fri, Nov 02, 2007 at 02:03:52AM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> Blake Ramsdell <blaker@gmail.com> writes:
> 
> > Signed-off-by: Blake Ramsdell <blaker@gmail.com>
> > ---
> >  Makefile |    4 +++-
> >  1 files changed, 3 insertions(+), 1 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index 71479a2..5d83756 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -401,7 +401,9 @@ endif
> >  ifeq ($(uname_S),Darwin)
> >  	NEEDS_SSL_WITH_CRYPTO = YesPlease
> >  	NEEDS_LIBICONV = YesPlease
> > -	OLD_ICONV = UnfortunatelyYes
> > +	ifneq ($(uname_R),9.0.0)
> > +		OLD_ICONV = UnfortunatelyYes
> > +	endif
> >  	NO_STRLCPY = YesPlease
> >  	NO_MEMMEM = YesPlease
> >  endif
> 
> I do not have an access to a Darwin box, but do you mean 10.5
> gives 9.0.0 as uname_R?

Be it that or not, it looks wrong to me to check the Darwin version to
know what to use. Do you rely on the Linux kernel version to know whether
iconv is present ?

Mike

Re: [PATCH] Mac OS X 10.5 does not require the OLD_ICONV flag set

[Benoit SIGOURE]

[-- Attachment #1: Type: text/plain, Size: 1450 bytes --]

On Nov 2, 2007, at 10:30 AM, Mike Hommey wrote:

> On Fri, Nov 02, 2007 at 02:03:52AM -0700, Junio C Hamano  
> <gitster@pobox.com> wrote:
>> Blake Ramsdell <blaker@gmail.com> writes:
>>
>>> Signed-off-by: Blake Ramsdell <blaker@gmail.com>
>>> ---
>>>  Makefile |    4 +++-
>>>  1 files changed, 3 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/Makefile b/Makefile
>>> index 71479a2..5d83756 100644
>>> --- a/Makefile
>>> +++ b/Makefile
>>> @@ -401,7 +401,9 @@ endif
>>>  ifeq ($(uname_S),Darwin)
>>>  	NEEDS_SSL_WITH_CRYPTO = YesPlease
>>>  	NEEDS_LIBICONV = YesPlease
>>> -	OLD_ICONV = UnfortunatelyYes
>>> +	ifneq ($(uname_R),9.0.0)
>>> +		OLD_ICONV = UnfortunatelyYes
>>> +	endif
>>>  	NO_STRLCPY = YesPlease
>>>  	NO_MEMMEM = YesPlease
>>>  endif
>>
>> I do not have an access to a Darwin box, but do you mean 10.5
>> gives 9.0.0 as uname_R?
>
> Be it that or not, it looks wrong to me to check the Darwin version to
> know what to use. Do you rely on the Linux kernel version to know  
> whether
> iconv is present ?

It's very different, on OSX you don't change your own kernel as you  
want, the kernel isn't a standalone component, it comes packaged with  
the entire system of MacOSX.  When you do an update to 10.5 (aka  
Leopard) you will have a new version of iconv so you're guaranteed  
that someone with 10.5 has a system-wide iconv that is not OLD_ICONV.

-- 
Benoit Sigoure aka Tsuna
EPITA Research and Development Laboratory



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 186 bytes --]

Re: [PATCH] Mac OS X 10.5 does not require the OLD_ICONV flag set

[Mike Hommey]

On Fri, Nov 02, 2007 at 10:39:35AM +0100, Benoit SIGOURE <tsuna@lrde.epita.fr> wrote:
> On Nov 2, 2007, at 10:30 AM, Mike Hommey wrote:
> 
> >On Fri, Nov 02, 2007 at 02:03:52AM -0700, Junio C Hamano  
> ><gitster@pobox.com> wrote:
> >>Blake Ramsdell <blaker@gmail.com> writes:
> >>
> >>>Signed-off-by: Blake Ramsdell <blaker@gmail.com>
> >>>---
> >>> Makefile |    4 +++-
> >>> 1 files changed, 3 insertions(+), 1 deletions(-)
> >>>
> >>>diff --git a/Makefile b/Makefile
> >>>index 71479a2..5d83756 100644
> >>>--- a/Makefile
> >>>+++ b/Makefile
> >>>@@ -401,7 +401,9 @@ endif
> >>> ifeq ($(uname_S),Darwin)
> >>> 	NEEDS_SSL_WITH_CRYPTO = YesPlease
> >>> 	NEEDS_LIBICONV = YesPlease
> >>>-	OLD_ICONV = UnfortunatelyYes
> >>>+	ifneq ($(uname_R),9.0.0)
> >>>+		OLD_ICONV = UnfortunatelyYes
> >>>+	endif
> >>> 	NO_STRLCPY = YesPlease
> >>> 	NO_MEMMEM = YesPlease
> >>> endif
> >>
> >>I do not have an access to a Darwin box, but do you mean 10.5
> >>gives 9.0.0 as uname_R?
> >
> >Be it that or not, it looks wrong to me to check the Darwin version to
> >know what to use. Do you rely on the Linux kernel version to know  
> >whether
> >iconv is present ?
> 
> It's very different, on OSX you don't change your own kernel as you  
> want, the kernel isn't a standalone component, it comes packaged with  
> the entire system of MacOSX.  When you do an update to 10.5 (aka  
> Leopard) you will have a new version of iconv so you're guaranteed  
> that someone with 10.5 has a system-wide iconv that is not OLD_ICONV.

The fact is you can also use Darwin without OSX...

Mike

Re: [PATCH] Mac OS X 10.5 does not require the OLD_ICONV flag set

[David Symonds]

On 11/2/07, Junio C Hamano <gitster@pobox.com> wrote:
> Blake Ramsdell <blaker@gmail.com> writes:
>
> > Signed-off-by: Blake Ramsdell <blaker@gmail.com>
> > ---
> >  Makefile |    4 +++-
> >  1 files changed, 3 insertions(+), 1 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index 71479a2..5d83756 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -401,7 +401,9 @@ endif
> >  ifeq ($(uname_S),Darwin)
> >       NEEDS_SSL_WITH_CRYPTO = YesPlease
> >       NEEDS_LIBICONV = YesPlease
> > -     OLD_ICONV = UnfortunatelyYes
> > +     ifneq ($(uname_R),9.0.0)
> > +             OLD_ICONV = UnfortunatelyYes
> > +     endif
> >       NO_STRLCPY = YesPlease
> >       NO_MEMMEM = YesPlease
> >  endif
>
> I do not have an access to a Darwin box, but do you mean 10.5
> gives 9.0.0 as uname_R?

Further, that comparison is going to fail as soon as the next revision
of Darwin (9.0.1, etc.) is released.


Dave.

Re: [PATCH] Mac OS X 10.5 does not require the OLD_ICONV flag set

[Junio C Hamano]

"David Symonds" <dsymonds@gmail.com> writes:

> On 11/2/07, Junio C Hamano <gitster@pobox.com> wrote:
>> Blake Ramsdell <blaker@gmail.com> writes:
>>
>> > Signed-off-by: Blake Ramsdell <blaker@gmail.com>
>> > ---
>> >  Makefile |    4 +++-
>> >  1 files changed, 3 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/Makefile b/Makefile
>> > index 71479a2..5d83756 100644
>> > --- a/Makefile
>> > +++ b/Makefile
>> > @@ -401,7 +401,9 @@ endif
>> >  ifeq ($(uname_S),Darwin)
>> >       NEEDS_SSL_WITH_CRYPTO = YesPlease
>> >       NEEDS_LIBICONV = YesPlease
>> > -     OLD_ICONV = UnfortunatelyYes
>> > +     ifneq ($(uname_R),9.0.0)
>> > +             OLD_ICONV = UnfortunatelyYes
>> > +     endif
>> >       NO_STRLCPY = YesPlease
>> >       NO_MEMMEM = YesPlease
>> >  endif
>>
>> I do not have an access to a Darwin box, but do you mean 10.5
>> gives 9.0.0 as uname_R?
>
> Further, that comparison is going to fail as soon as the next revision
> of Darwin (9.0.1, etc.) is released.

Can we do something intelligent with $(shell iconv --version)
there instead, I wonder, then?

Re: [PATCH] Mac OS X 10.5 does not require the OLD_ICONV flag set

[David Symonds]

On 11/2/07, Junio C Hamano <gitster@pobox.com> wrote:
> "David Symonds" <dsymonds@gmail.com> writes:
>
> > Further, that comparison is going to fail as soon as the next revision
> > of Darwin (9.0.1, etc.) is released.
>
> Can we do something intelligent with $(shell iconv --version)
> there instead, I wonder, then?

It would probably be most appropriate for the autoconf script. Now
that I look at configure.ac, there's already a test for iconv in
there; is it not used?


Dave.

Re: [PATCH] Mac OS X 10.5 does not require the OLD_ICONV flag set

[Blake Ramsdell]

On 11/2/07, David Symonds <dsymonds@gmail.com> wrote:
> It would probably be most appropriate for the autoconf script. Now
> that I look at configure.ac, there's already a test for iconv in
> there; is it not used?

The crux of this problem is that the prototype for iconv in
/usr/include/iconv.h is different between OS X 10.4 and OS X 10.5. So
the "right thing" is definitely to determine what is in the function
prototype, and then act accordingly.

>From OS X 10.4.10:

#define _LIBICONV_VERSION 0x0109    /* version number: (major<<8) + minor */
...
extern size_t iconv (iconv_t cd, const char* * inbuf, size_t
*inbytesleft, char* * outbuf, size_t *outbytesleft);

>From OS X 10.5:

#define _LIBICONV_VERSION 0x010B    /* version number: (major<<8) + minor */
...
size_t iconv (iconv_t /*cd*/,
        char ** __restrict /*inbuf*/,  size_t * __restrict /*inbytesleft*/,
        char ** __restrict /*outbuf*/, size_t * __restrict /*outbytesleft*/);

So what happened in git is that someone put in OLD_ICONV to
dynamically adjust the const-ness of parameter 2 to the iconv
function, and the way they chose to do that is to identify the OS
(more accurately, the kernel), and then I went and furthered that by
identifying the version.

Now that I look at it further, it seems that yanking OLD_ICONV
altogether is a better approach, and to just check _LIBICONV_VERSION
to make sure it's "new enough". Now, I'm not sure what that comparison
would be, but we know that "later than 0x0109" is a good start. Based
on the version difference between OS X 10.4 and 10.5 I note that there
is only 0x010A intervening.

This strategy presumes that this const parameter was const all the way
up to a particular point, and then stopped being const, which is
probably a reasonable assumption.

Blake
-- 
Blake Ramsdell | http://www.blakeramsdell.com

[PATCH] Removed OLD_ICONV in favor of checking _LIBICONV_VERSION directly

[Blake Ramsdell]

Signed-off-by: Blake Ramsdell <blaker@gmail.com>
---
 Makefile |    8 --------
 utf8.c   |    2 +-
 2 files changed, 1 insertions(+), 9 deletions(-)

diff --git a/Makefile b/Makefile
index 71479a2..96c8a73 100644
--- a/Makefile
+++ b/Makefile
@@ -95,9 +95,6 @@ all::
 #
 # Define NO_ICONV if your libc does not properly support iconv.
 #
-# Define OLD_ICONV if your library has an old iconv(), where the second
-# (input buffer pointer) parameter is declared with type (const char **).
-#
 # Define NO_R_TO_GCC_LINKER if your gcc does not like "-R/path/lib"
 # that tells runtime paths to dynamic libraries;
 # "-Wl,-rpath=/path/lib" is used instead.
@@ -401,7 +398,6 @@ endif
 ifeq ($(uname_S),Darwin)
 	NEEDS_SSL_WITH_CRYPTO = YesPlease
 	NEEDS_LIBICONV = YesPlease
-	OLD_ICONV = UnfortunatelyYes
 	NO_STRLCPY = YesPlease
 	NO_MEMMEM = YesPlease
 endif
@@ -657,10 +653,6 @@ ifdef NO_ICONV
 	BASIC_CFLAGS += -DNO_ICONV
 endif
 
-ifdef OLD_ICONV
-	BASIC_CFLAGS += -DOLD_ICONV
-endif
-
 ifdef PPC_SHA1
 	SHA1_HEADER = "ppc/sha1.h"
 	LIB_OBJS += ppc/sha1.o ppc/sha1ppc.o
diff --git a/utf8.c b/utf8.c
index 4efef6f..a7feb4f 100644
--- a/utf8.c
+++ b/utf8.c
@@ -300,7 +300,7 @@ int is_encoding_utf8(const char *name)
  * with iconv.  If the conversion fails, returns NULL.
  */
 #ifndef NO_ICONV
-#ifdef OLD_ICONV
+#if _LIBICONV_VERSION <= 0x0109
 	typedef const char * iconv_ibp;
 #else
 	typedef char * iconv_ibp;
-- 
1.5.3.GIT

Re: [PATCH] Removed OLD_ICONV in favor of checking _LIBICONV_VERSION directly

[Junio C Hamano]

Blake Ramsdell <blaker@gmail.com> writes:

> diff --git a/utf8.c b/utf8.c
> index 4efef6f..a7feb4f 100644
> --- a/utf8.c
> +++ b/utf8.c
> @@ -300,7 +300,7 @@ int is_encoding_utf8(const char *name)
>   * with iconv.  If the conversion fails, returns NULL.
>   */
>  #ifndef NO_ICONV
> -#ifdef OLD_ICONV
> +#if _LIBICONV_VERSION <= 0x0109
>  	typedef const char * iconv_ibp;
>  #else
>  	typedef char * iconv_ibp;

Does everybody's iconv use the same _LIBICONV_VERSION scheme?

Compiling this:

        #include <iconv.h>
        int i = _LIBICONV_VERSION;

on a Linux box with GNU C library gives:

	i.c:3: error: '_LIBICONV_VERSION' undeclared here (not in a function)

here.

Re: [PATCH] Removed OLD_ICONV in favor of checking _LIBICONV_VERSION directly

[Blake Ramsdell]

On 11/2/07, Junio C Hamano <gitster@pobox.com> wrote:
> Does everybody's iconv use the same _LIBICONV_VERSION scheme?

Apparently not. And furthermore, my assumption about the version
number being tied to the function prototype was not valid -- I have a
DarwinPorts iconv.h that is version 0x010B but uses const for the
second parameter.

Sorry for the mess. Let me try and come up with another plan. That
Darwin version check doesn't seem so bad now ;).

Blake
-- 
Blake Ramsdell | http://www.blakeramsdell.com