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

From: Mark Levedahl <mlevedahl@gmail.com>
Date: Jul 16, 2013, 23:13 UTC
Message-ID: <51E5D38E.6080202@gmail.com>
In-Reply-To: <51E5BCDF.5070004@ramsay1.demon.co.uk>

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

Show 25 quoted lines

> 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