I do not know how Macintosh libc implements "struc dirent", but
this approach does not work in general. For example, on Linux
boxes with glibc, "struct dirent" is defined like this (pardon
the funny indentation --- that is from the original):
struct dirent
{
#ifndef __USE_FILE_OFFSET64
__ino_t d_ino;
__off_t d_off;
#else
__ino64_t d_ino;
__off64_t d_off;
#endif
unsigned short int d_reclen;
unsigned char d_type;
char d_name[256]; /* We must not include limits.h! */
};
yet you can obtain a path component longer than 256 bytes.
Apparently the library allocates longer d_name[] field than what
is shown to the user.
From: Mark Junker <hidden> Date: 2016-06-15 22:44:07
Junio C Hamano schrieb:
I do not know how Macintosh libc implements "struc dirent", but
this approach does not work in general.
IMHO there is no need that this approach works in general because this
is a fix for MacOSX systems only. I also use d_namlen which might not be
available on other systems. But on MacOSX this works as expected.
yet you can obtain a path component longer than 256 bytes.
Apparently the library allocates longer d_name[] field than what
is shown to the user.
This is not a problem either because on MacOSX we get decomposed UTF8
and we always convert to composed UTF8. This means that the string
returned from reencode_string will always be smaller than the original
filename that had to be reencoded.
Regards,
Mark
From: "H. Peter Anvin" <hpa@zytor.com> Date: 2016-06-15 22:44:07
Mark Junker wrote:
Junio C Hamano schrieb:
quoted
I do not know how Macintosh libc implements "struc dirent", but
this approach does not work in general.
IMHO there is no need that this approach works in general because this
is a fix for MacOSX systems only. I also use d_namlen which might not be
available on other systems. But on MacOSX this works as expected.
quoted
yet you can obtain a path component longer than 256 bytes.
Apparently the library allocates longer d_name[] field than what
is shown to the user.
This is not a problem either because on MacOSX we get decomposed UTF8
and we always convert to composed UTF8. This means that the string
returned from reencode_string will always be smaller than the original
filename that had to be reencoded.
That's not true! There are strings which gets longer when a composing
normalization is applied. Please see section 3.3 of Unicode Techical
Report 36:
http://www.unicode.org/reports/tr36/
> People assume that NFC always composes, and thus is the same or
> shorter length than the original source. However, some characters
> decompose in NFC.
(NFC = Normalization Form Composing.)
U+1D160 MUSICAL SYMBOL EIGHT NOTE is given as an example with a 3x
expansion factor when encoded in UTF-8 (I don't know what it expands to;
seems odd to me.)
-hpa
yet you can obtain a path component longer than 256 bytes.
Individual components are limited to 255 bytes by most filesystems
(PATH_MAX is the whole path, not any individual component).
That said, you're right. It's not really a design requirement, and since
you never get an array of "struct dirent", just a pointer to a single one,
it would be perfectly normal and natural for "struct dirent" to be
declared with a unsized d_name[].
It's also quite possible that some implementations might even have
d_name[] not as an array, but as a pointer to somewhere else (POSIX may
require it to be an array, I didn't check).
That said, I bet that Mark isn't the only one to have written code like
that, so I suspect Mark's code probably works in practice pretty much
everywhere, even if I don't think it's necessarily _required_ to work
correctly.
I do suspect that if you really want to make this portable, and able to
handle an expanding d_name[] too, I think you need to make sure you
allocate a big-enough one. And if you worry about d_name perhaps being a
pointer, that really does mean that you'd need to convert the
system-supplied "struct dirent" into a "git_dirent_t" that you can
control.
That said, I think this patch has a bigger problem, namely just
fundamentally that
char *utf8 = reencode_string(entry, "UTF8", "UTF8-MAC");
is just unbelievably slow. That's just not how it should be done.
First off, the common case is that the filename likely has everything in
plain 7-bit ascii. So rather than re-encoding by default, the first thing
to do is to just see if it even needs re-encoding. Even if it's as simple
as saying "does it have any high bits at all", that's going to be a *huge*
performance win.
So start off with something like
int is_usascii(const char *p)
{
char c;
do {
c = *p++;
} while (c > 0);
return !c;
}
and now you can do
if (is_usascii(entry->d_name))
return entry;
before you even *look* at re-encoding it (and this basically works for all
cases - we really don't care about EBCDIC, do we? So even if this routine
was meant to do Latin1<->utf8, the above "is_usascii()" test is always the
right thing to do).
Anyway, even if you do that, our "reencode_string()" is really *so*
expensive that you really don't want to do it on a filename by filename
basis. It literally does a malloc() for each allocation. It might well be
worth it to find something that is more utf-8-specific (and I could well
imagine that Mac OS X comes with some UTF libraries, if only because we
cannot possibly be the only people with this issue).
(Same goes for Latin1<->UTF conversion, for that matter. If somebody wants
to add that, I suspect it's best done by hand, not using iconv and our
rather expensive layer around it. That said, latin1->utf8 is actually
much *easier* than utf-8 NFD->NFC).
Linus
I do suspect that if you really want to make this portable, and able to
handle an expanding d_name[] too, I think you need to make sure you
allocate a big-enough one. And if you worry about d_name perhaps being a
pointer, that really does mean that you'd need to convert the
system-supplied "struct dirent" into a "git_dirent_t" that you can
control.
That said, I think this patch has a bigger problem, namely just
fundamentally that
char *utf8 = reencode_string(entry, "UTF8", "UTF8-MAC");
is just unbelievably slow. That's just not how it should be done.
Having thought about this some more, I'm starting to suspect that the
"readdir()" wrapper thing won't work very well.
Yes, it will work on OS X, but for all the wrong reasons. It works there
just because of the stupid normalization that OS X does both on filename
input and output, so if we hook into readdir() and munge the name there,
we'll still be able to use the munged name for lstat() and open().
However, we'll never be able to test it on a sane Unix system, and it
won't ever be able to handle the case of a filesystem actually being
Latin1 but git being asked to try to transparently convert it to utf-8 in
order to work with others.
Because most of those readdir() calls will just be fed back not just to
the filesystem as lstat() calls later, but also to the recursive directory
traversal itself, so if we munge the name, we're also going to screw name
lookup.
Again, as an OSX-only workaround it's probably acceptable, and perhaps
that's the only thing to look at right now. But it does strike me as a
design mistake to do it at that level.
It would be conceptually nicer to do it in "add_file_to_index()" instead.
Ie anything that creates a "struct cache_entry" would do the
conversion.
So it would be good if somebody looked at what happens if you do the OSX
hack in add_file_to_index() instead, and see if it works there..
Linus
On Mon, Jan 21, 2008 at 08:59:56PM -0800, Linus Torvalds wrote:
Anyway, even if you do that, our "reencode_string()" is really *so*
expensive that you really don't want to do it on a filename by filename
basis. It literally does a malloc() for each allocation. It might well be
worth it to find something that is more utf-8-specific (and I could well
imagine that Mac OS X comes with some UTF libraries, if only because we
cannot possibly be the only people with this issue).
Yes, starting with Mac OS X 10.2 there are functions for that.
http://developer.apple.com/qa/qa2001/qa1235.html
Anyway, even if iconv is to be used, I believe it should be possible to
avoid malloc here (I usually allocate 256 on stack and use malloc()/free()
only when I need more than that which in practice never happens!). It is
also avoidable to call iconv_open/iconv_close for each name by putting the
allocated descriptor for character set conversion into a static variable.
Thus leaving iconv() alone, which should not be big overhead provided that
it is done only for non-ASCII names.
Dmitry
On Mon, Jan 21, 2008 at 11:16:54PM -0800, Linus Torvalds wrote:
Yes, it will work on OS X, but for all the wrong reasons. It works there
just because of the stupid normalization that OS X does both on filename
input and output, so if we hook into readdir() and munge the name there,
we'll still be able to use the munged name for lstat() and open().
Yes, when I proposed the readdir() wrapper, I meant it to be as OS X
specific hack. Just because HFS+ munges names and does that by converting
them in the form that is HFS+ specific, we can safely convert then into
NFC, as we do not lose more information than it is lost already, and
more importantly, AFAIK, everything that a user types on Mac is in NFC,
whether they are names in the command line or names in .gitatributes.
However, we'll never be able to test it on a sane Unix system, and it
won't ever be able to handle the case of a filesystem actually being
Latin1 but git being asked to try to transparently convert it to utf-8 in
order to work with others.
Yes, but that is a separate issue, which unfortunately is much more
difficult to deal with. Basically, there are two approaches -- either
to wrap all input/output functions, or to find another point where it
is possible to convert names without re-writing too much code in Git.
It seems to me that the first approach may requires wrapping too much
functions, but looking at the code I am not sure that the second will
be much easier. There are many places where a filename in the local
encoding will interact with Git internal encoding used by repo.
If we spoke about Windows only, I would say that the first approach makes
much more sense, because all i/o functions used on Windows are already
wrappers over Unicode functions. So, converting UTF-8 <-> UTF-16 makes
much more sense than UTF-8 <-> some-local-encoding(*) <-> UTF-16.
(*) In fact, two different encodings for the same locale setting --
one for console and the other for non-console programs!
It would be conceptually nicer to do it in "add_file_to_index()" instead.
Ie anything that creates a "struct cache_entry" would do the
conversion.
I don't think it is going to work, without changing a lot of code,
because filenames entered by user and those that are returned by
readdir() are different. Also, .gitignore or .gitattributes files will
have filenames in the form that differs from returned by readdir().
Dmitry
From: Nicolas Pitre <hidden> Date: 2016-06-15 22:44:07
On Mon, 21 Jan 2008, Linus Torvalds wrote:
First off, the common case is that the filename likely has everything in
plain 7-bit ascii. So rather than re-encoding by default, the first thing
to do is to just see if it even needs re-encoding. Even if it's as simple
as saying "does it have any high bits at all", that's going to be a *huge*
performance win.
So start off with something like
int is_usascii(const char *p)
{
char c;
do {
c = *p++;
} while (c > 0);
return !c;
}
You need to use "signed char" here. On ARM a char is unsigned by
default. That's the case on some other systems too.
Nicolas