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

[Elijah Newren <newren@gmail.com>]

Hi Dscho,

On Mon, Dec 16, 2019 at 4:04 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Mon, 16 Dec 2019, Elijah Newren wrote:
> > On Mon, Dec 16, 2019 at 5:51 AM Elijah Newren <newren@gmail.com> wrote:
> > >
> > > On Sun, Dec 15, 2019 at 2:29 AM Johannes Schindelin
> > > <Johannes.Schindelin@gmx.de> wrote:
> > > >
> > > > Hi Elijah,
> > > >
> > > > I have not had time to dive deeply into this, but I know that it _does_
> > > > cause a ton of segmentation faults in the `shears/pu` branch (where all of
> > > > Git for Windows' patches are rebased on top of `pu`):
> > >
> > > Weird.  If it's going to cause segmentation faults at all, it would
> > > certainly do it all over the place, but I tested the patches on the
> > > major platforms using your Azure Pipelines setup on git.git so it
> > > should be good on all the platforms.  Did your shears/pu branch make
> > > some other changes to the setup?
>
> Not really.
>
> >
> > Actually, it looks like I looked up the definition of dirent
> > previously and forgot by the time you emailed.  On linux, from
> > /usr/include/bits/dirent.h:
...
> > and from compat/win32/dirent.h defines it as:
> >
> > struct dirent {
> >         unsigned char d_type;      /* file type to prevent lstat after
> > readdir */
> >         char d_name[MAX_PATH * 3]; /* file name (* 3 for UTF-8 conversion) */
> > };
...
>
> If you care to look at our very own `compat/win32/dirent.h`, you will see
> this:

Interesting, we both brought up compat/win32/dirent.h and quoted from
it in our emails...

> struct dirent {
>         unsigned char d_type; /* file type to prevent lstat after readdir */
>         char *d_name;         /* file name */
> };

...but the contents were different?  Looks like git-for-windows forked
compat/win32/dirent.h, possibly in a way that violates POSIX as
pointed out by Junio.  Any reason those changes weren't sent back
upstream, by chance?  Feels odd having a compat/win32/ directory that
our downstream windows users aren't actually using.  It also means the
testing I'm getting from gitgitgadget and your Azure setup (which all
is really, really nice by the way), is far less reassuring and helpful
than I hoped.

> 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.

Doh, yeah, I messed that up too.

Anyway, as I mentioned to Junio, I'll resubmit after gutting the
series.  I'll still include a fix for the issue that a real world user
reported, but all the other ancillary bugs I found that have been
around for over a decade aren't important enough to merit a major
refactor, IMO.

Elijah