On Tue, Sep 28, 2010 at 17:42, Erik Faye-Lund <kusmabite@xxxxxxxxx> wrote:
> On Tue, Sep 28, 2010 at 7:07 PM, Ãvar ArnfjÃrà Bjarmason
> <avarab@xxxxxxxxx> wrote:
>> On Tue, Sep 28, 2010 at 16:05, Erik Faye-Lund <kusmabite@xxxxxxxxx> wrote:
>>> libcharset provides an even more portable way of quering the charset
>>> of the current locale.
>>>
>>> Use that instead of nl_langinfo unless NO_LIBCHARSET is set.
>>>
>>> Signed-off-by: Erik Faye-Lund <kusmabite@xxxxxxxxx>
>>> ---
>>>
>>> Windows doesn't have langinfo.h and nl_langinfo(), but libcharset was
>>> invented for this very purpose. With this patch on top, ab/i18n
>>> compiles without errors in msysGit.
>>>
>>> There's still a bunch of lower-level issues on Windows, like gettext
>>> ending up overloading our winansi-wrappings for printf and friends,
>>> but let's take thinks one step at the time :)
>>>
>>> Âconfigure.ac | Â Â6 ++++++
>>> Âgettext.c  Â|  10 +++++++++-
>>> Â2 files changed, 15 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/configure.ac b/configure.ac
>>> index 1821d89..d3139cd 100644
>>> --- a/configure.ac
>>> +++ b/configure.ac
>>> @@ -810,6 +810,12 @@ AC_CHECK_HEADER([libintl.h],
>>> Â[NO_GETTEXT=YesPlease])
>>> ÂAC_SUBST(NO_GETTEXT)
>>> Â#
>>> +# Define NO_LIBCHARSET if you don't have libcharset.h
>>> +AC_CHECK_HEADER([libcharset.h],
>>> +[NO_LIBCHARSET=],
>>> +[NO_LIBCHARSET=YesPlease])
>>> +AC_SUBST(NO_LIBCHARSET)
>>> +#
>>> Â# Define NO_STRCASESTR if you don't have strcasestr.
>>> ÂGIT_CHECK_FUNC(strcasestr,
>>> Â[NO_STRCASESTR=],
>>> diff --git a/gettext.c b/gettext.c
>>> index 8644098..902268c 100644
>>> --- a/gettext.c
>>> +++ b/gettext.c
>>> @@ -1,13 +1,17 @@
>>> Â#include "exec_cmd.h"
>>> Â#include <locale.h>
>>> Â#include <libintl.h>
>>> +#ifndef NO_LIBCHARSET
>>> +#include <libcharset.h>
>>> +#else
>>> Â#include <langinfo.h>
>>> +#endif
>>> Â#include <stdlib.h>
>>>
>>> Âextern void git_setup_gettext(void) {
>>> Â Â Â Âchar *podir;
>>> Â Â Â Âchar *envdir = getenv("GIT_TEXTDOMAINDIR");
>>> - Â Â Â char *charset;
>>> + Â Â Â const char *charset;
>>>
>>> Â Â Â Âif (envdir) {
>>> Â Â Â Â Â Â Â Â(void)bindtextdomain("git", envdir);
>>> @@ -20,7 +24,11 @@ extern void git_setup_gettext(void) {
>>>
>>> Â Â Â Â(void)setlocale(LC_MESSAGES, "");
>>> Â Â Â Â(void)setlocale(LC_CTYPE, "");
>>> +#ifndef NO_LIBCHARSET
>>> + Â Â Â charset = locale_charset();
>>> +#else
>>> Â Â Â Âcharset = nl_langinfo(CODESET);
>>> +#endif
>>> Â Â Â Â(void)bind_textdomain_codeset("git", charset);
>>> Â Â Â Â(void)setlocale(LC_CTYPE, "C");
>>> Â Â Â Â(void)textdomain("git");
>>
>> Thanks for porting it to Windows. Some points:
>>
>> Â* Nit: Should be NEEDS_LIBCHARSET instead of NO_LIBCHARSET, all the
>> Â variables that set library inclusions in the Makefile use the
>> Â NEED_* names.
>>
>
> That's not true, at least NO_OPENSSL, NO_PTHREADS, NO_ICONV,
> NO_LIBGEN_H, NO_MMAP and NO_SYS_SELECT_H use the NO_-prefix to include
> libraries (as opposed to the NEEDS_-prefix). And to be honest, I think
> the NEEDS_-prefix would be a lie in this case; we don't NEED it, we
> take advantage of it if it's there.
>
> To be honest, I just mimicked what was done for detection of gettext.
> Perhaps HAVE_LIBCHARSET_H would be the appropriate define? It's what
> we use for paths.h (HAVE_PATHS_H)...
You're right, a NO_* makes more sense here.
>> Â* Their patch compiles a program that includes libcharset.h and
>> Â compiles "const char* charset = locale_charset();". I don't know if
>> Â this is needed, or whether just checking the header name like
>> Â you've done will do.
>>
>
> I don't think there's any point to it - libcharset.h is pretty much
> just about that one function. And if it turns out I'm wrong, we can
> add a check like that in the future.
>
>> Â* They also have a HAVE_LANGINFO_H define and fall back on just
>> Â returning "", which works on GNU iconv. Maybe we should do this
>> Â too?
>>
>
> Perhaps, but can't that wait until we encounter a system that needs it?
To both of the above: It's completely reasonable as-is, I just wanted
to mention it in case a list member knew of a reason to do it like GHC
did it. But just checking for the header will do fine until we
encounter issues with that.
> What's interesting there is what you say about GNU iconv handling "".
> This means that the minimal fix for Windows could be even smaller -
> we're using GNU iconv :)
Yeah, but probably better to do it by using libcharset.h instead.
>> I'm not sure about any of this, since I've just been testing on
>> Solaris, Linux and FreeBSD.
>
> I think Solaris, Linux, FreeBSD and Windows is a pretty wide selection
> of platforms, so I hope it should be pretty painless once this hits
> 'next'. Sure, there might be some platforms that needs a fix-up, but
> there always is with new code. And besides, there's even time to test
> more platforms before merging to 'next', no? :)
Yeah, I'm hoping it'll get into next soon so we can get more
reports/fixes like these. Anyway, I amended your patches a bit, here
are the changes:
* Split up the s/char*/const char*/ change into its own patch, or is
there a reason for why this needs to be there along with the
libcharset.h change?
* Added docs about the define to the Makefile
* Added defaults for NO_LIBCHARSET to the default, I only changed the
defaults for the MINGW entry, maybe it should be changed on Cygwin
and Windows too? And probably on OpenBSD and NetBSD too.
Erik Faye-Lund (2):
gettext: use const char* instead of char*
gettext: use libcharset when available
Makefile | 17 +++++++++++++++++
configure.ac | 6 ++++++
gettext.c | 10 +++++++++-
3 files changed, 32 insertions(+), 1 deletions(-)
--
1.7.3.159.g610493
--
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