Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions

On 07/16/2013 05:36 PM, Ramsay Jones wrote:

Caveats:
1) I don't find any speed improvement of the current patch over the
previous one (the tests actually ran faster with the earlier patch,
though the difference was less than 1%).
2) I still question this whole approach, especially having this
non-POSIX compliant mode be the default. Running in this mode breaks
interoperability with Linux, but providing a Linux environment is the
*primary* goal of Cygwin.
Yes, I agree. Since I _always_ disable the Win32 stat functions (by
setting core.filemode by hand - yes it's a little annoying), I don't
get any "benefit" from the added complexity.

However, I don't use git on cygwin with *large* repositories. git.git
is pretty much as large as it gets. So, the difference in performance
for me amounts to something like 0.120s -> 0.260s, which I can barely
notice.

Other people may not be so lucky ...

I would be happy if my original patch (removing the win32 stat funcs)
was applied, but others may not be. :-P

ATB,
Ramsay Jones

Cygwin 1.7 is very different than the earlier, no longer supported, and 
no longer available Cygwin variants in many ways, but stat is one of 
them. Cygwin 1.7 uses Windows ACLs to represent file permissions, and 
therefore gets the file permissions directly from the underlying OS 
calls. Earlier Cygwin versions (attempted to) overlay POSIX permissions 
on Windows systems using extended attributes and other means, and in 
many cases had to resort to opening the file and examining it to 
determine executability. This is not true in 1.7.

Therefore, your later patch would be expected to have much less benefit 
for 1.7 than for 1.5 (I don't detect *any* benefit on 1.7 when I set 
core.filemode=false). There are many choices, three are:

a) Remove the win32 stat funcs, eliminating all of the troublesome code 
paths and maintenance burden (your original patch).
b) Add your latest patch, with attendant complexity and maintenance 
burden, to support a version of Cygwin that is no longer available and 
was last updated over four years ago.
c) Like b, except make this triggered only by a "CYGWIN_15" macro, 
limiting this to use by the legacy cygwin platform.

I strongly vote for a, could support c, but fear b is just going to keep 
us chasing down bugs. Especially so when we consider that this patch can 
only speed things up when core.filemode=false, which mode:
a) causes git to fail its test suite.
b) breaks compatibility with Linux
c) violates the primary goal of the Cygwin project, which is to provide 
a Linux environment on Windows.

Mark