From: Mark Levedahl <mlevedahl@gmail.com>
To: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Cc: mhagger@alum.mit.edu, Junio C Hamano <gitster@pobox.com>,
Jeff King <peff@peff.net>, Johannes Sixt <j6t@kdbg.org>,
"Shawn O. Pearce" <spearce@spearce.org>,
tboegi@web.de, dpotapov@gmail.com,
GIT Mailing-list <git@vger.kernel.org>
Subject: Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions
Date: Tue, 16 Jul 2013 19:13:18 -0400 [thread overview]
Message-ID: <51E5D38E.6080202@gmail.com> (raw)
In-Reply-To: <51E5BCDF.5070004@ramsay1.demon.co.uk>
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
next prev parent reply other threads:[~2013-07-16 23:13 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-07-10 20:23 [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions Ramsay Jones
2013-07-14 16:15 ` Mark Levedahl
2013-07-15 19:49 ` Junio C Hamano
2013-07-16 2:06 ` Torsten Bögershausen
2013-07-16 3:54 ` Mark Levedahl
2013-07-16 15:42 ` Dmitry Potapov
2013-07-16 22:52 ` Mark Levedahl
2013-07-18 17:50 ` Ramsay Jones
2013-07-18 21:49 ` Torsten Bögershausen
2013-07-18 22:36 ` Mark Levedahl
2013-07-18 23:32 ` Junio C Hamano
2013-07-19 15:34 ` Mark Levedahl
2013-07-16 3:44 ` Mark Levedahl
2013-07-16 21:36 ` Ramsay Jones
2013-07-16 23:13 ` Mark Levedahl [this message]
2013-07-18 17:43 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=51E5D38E.6080202@gmail.com \
--to=mlevedahl@gmail.com \
--cc=dpotapov@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=j6t@kdbg.org \
--cc=mhagger@alum.mit.edu \
--cc=peff@peff.net \
--cc=ramsay@ramsay1.demon.co.uk \
--cc=spearce@spearce.org \
--cc=tboegi@web.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).