Re: [PATCH v2 6/8] dir: fix checks on common prefix directory

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Dec 17, 2019, 11:08 UTC
Message-ID: <nycvar.QRO.7.76.6.1912171157140.46@tvgsbejvaqbjf.bet>
In-Reply-To: <xmqqr213iz9z.fsf@gitster-ct.c.googlers.com>

Hi Junio,

On Mon, 16 Dec 2019, Junio C Hamano wrote:

Show 31 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > If you care to look at our very own `compat/win32/dirent.h`, you will see
> > this:
> >
> > struct dirent {
> >         unsigned char d_type; /* file type to prevent lstat after readdir */
> >         char *d_name;         /* file name */
> > };
> >
> > And looking at
> > https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/dirent.h.html, I
> > do not see any guarantee of that `[256]` at all:
> >
> > The <dirent.h> header shall [...] define the structure dirent which shall
> > include the following members:
> >
> > [XSI][Option Start]
> > ino_t  d_ino       File serial number.
> > [Option End]
> > char   d_name[]    Filename string of entry.
> >
> > You will notice that not even `d_type` is guaranteed.
>
> I am reasonably sure that the code (without Elijah's patches anyway)
> takes the possibility of missing d_type into account already.
>
> Doesn't the above mean d_name[] has to be an in-place array of some
> size (i.e. even a flex-array is OK)?  It does not look to me that it
> allows for it to be a pointer pointing at elsewhere (possibly on
> heap), which may be asking for trouble.

You are right, of course.

I also was not _quite_ spot on, as I had looked at Git for Windows' `shears/pu` branch, not at the `pu` branch. Alas, we have patches in Git for Windows that allow for switching to a faster, caching way to access the stat() and readdir() data (it is called the "FSCache" and it is responsible for some rather dramatic speed-ups). And these patches change `struct dirent` to the form that is quoted above, to allow switching back and forth between two _different_ backends, storing the actual `d_name` not in `struct dirent` but in `DIR`.

Is this compliant with POSIX? I guess not. Does it work? Yes, it does.

I can't know for sure that it makes a dent, but FSCache is designed for speed, and it actually does not even store the `d_name` in the `DIR`, but directly in the cache structure, avoiding copying at all.

In short: if we can allow FSCache to keep operating that way (i.e. keep `d_name` as a pointer), then that would be helpful to keep the performance on Windows somewhat within acceptable boundaries.

Ciao, Dscho