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

Hi Elijah,

On Mon, 16 Dec 2019, Elijah Newren wrote:

On Mon, Dec 16, 2019 at 4:04 PM Johannes Schindelin
[off-list ref] wrote:

quoted

On Mon, 16 Dec 2019, Elijah Newren wrote:

quoted

On Mon, Dec 16, 2019 at 5:51 AM Elijah Newren [off-list ref] wrote:

quoted

On Sun, Dec 15, 2019 at 2:29 AM Johannes Schindelin
[off-list ref] wrote:

quoted

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.

quoted

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:

...

quoted

quoted

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) */
};

...

quoted

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

quoted

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.

Yep, I messed that up, sorry.

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.

Yes. I was ready to submit the FSCache feature to the Git mailing list for
review some 2.5 years ago when along came Ben Peart, finding ways to speed
up FSCache even further. That is the reason why I held off, and I still
have to condense the patches (which currently form a topology of 17 patch
series!!!) into a nice small patch series that does not reflect the
meandering history of the FSCache history, but instead presents one neat
story.

quoted

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.

Hmm. I am really sorry that I nudged you to go down this route. Quite
honestly, I'd rather add an ugly work-around that is Windows-only just so
that you can fix those ancillary bugs.

I might even go so far as to adjust the FSCache's internal data structure
to _store_ `struct dirent` items, then the fast `readdir()` implementation
could be even faster by just pointing at those items.

What do you think? Can we strike a deal to keep those bug fixes?

Ciao,
Dscho