Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions

Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions

[Johannes Sixt]

Am 25.06.2013 00:10, schrieb Junio C Hamano:
> Mark Levedahl <mlevedahl@gmail.com> writes:
> 
>> On 06/22/2013 03:38 PM, Ramsay Jones wrote:
>>> Also, apart from running the git test-suite, I have the Win32
>>> l/stat functions disabled on all of my repos. In particular, I have
>>> core.filemode set to true. (At one point, I used to build git with
>>> NO_TRUSTABLE_FILEMODE reset so that I wouldn't have to remember to
>>> reset core.filemode by hand after a git-clone or git-init). I should
>>> also note that I run MinGW git on the same laptop and, using git.git
>>> as an example, it does not seem that much faster (if at all) than
>>> cygwin git.
>>
>> After applying your patch to master, I've had the test-suite running
>> in a VM using WinXP + current cygwin (v1.7.x) for about 8 hours, no
>> failures so far but it could take another day to complete.
>>
>> I never found any real speed up using the Win32 stat/lstat functions,
>> and the lack of Posix compatibility breaking cross-platform projects,
>> links, etc.,  made this function a mis-feature in my opinion for
>> years. As I found no positive benefit from the Win32 lstat, I modified
>> git for local use years ago to set core.filemode=true when cloning /
>> initing to avoid as many issues as possible.
> 
> So that's two votes to use the vanilla Cygwin stat/lstat,
> essentially reverting adbc0b6b (cygwin: Use native Win32 API for
> stat, 2008-09-30), which was added by Dmitry and Shawn while I was
> away.
> 
> Let's wait and see if people give us more data points and decide.

That'll be more productive if we let the list know ;-)

Some context: This is about a patch by Ramsay that removes the
"schizophrenic lstat" hack for Cygwin. Junio, can you please queue that
patch in pu?

-- Hannes

Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions

[Torsten Bögershausen]

On 25.06.13 21:23, Johannes Sixt wrote:
> Am 25.06.2013 00:10, schrieb Junio C Hamano:
>> Mark Levedahl <mlevedahl@gmail.com> writes:
>>
>>> On 06/22/2013 03:38 PM, Ramsay Jones wrote:
>>>> Also, apart from running the git test-suite, I have the Win32
>>>> l/stat functions disabled on all of my repos. In particular, I have
>>>> core.filemode set to true. (At one point, I used to build git with
>>>> NO_TRUSTABLE_FILEMODE reset so that I wouldn't have to remember to
>>>> reset core.filemode by hand after a git-clone or git-init). I should
>>>> also note that I run MinGW git on the same laptop and, using git.git
>>>> as an example, it does not seem that much faster (if at all) than
>>>> cygwin git.
>>>
>>> After applying your patch to master, I've had the test-suite running
>>> in a VM using WinXP + current cygwin (v1.7.x) for about 8 hours, no
>>> failures so far but it could take another day to complete.
>>>
>>> I never found any real speed up using the Win32 stat/lstat functions,
>>> and the lack of Posix compatibility breaking cross-platform projects,
>>> links, etc.,  made this function a mis-feature in my opinion for
>>> years. As I found no positive benefit from the Win32 lstat, I modified
>>> git for local use years ago to set core.filemode=true when cloning /
>>> initing to avoid as many issues as possible.
>>
>> So that's two votes to use the vanilla Cygwin stat/lstat,
>> essentially reverting adbc0b6b (cygwin: Use native Win32 API for
>> stat, 2008-09-30), which was added by Dmitry and Shawn while I was
>> away.
>>
>> Let's wait and see if people give us more data points and decide.
> 
> That'll be more productive if we let the list know ;-)
> 
> Some context: This is about a patch by Ramsay that removes the
> "schizophrenic lstat" hack for Cygwin. Junio, can you please queue that
> patch in pu?
> 
> -- Hannes
>

Here some benchmarks:
Clone the linux kernel from August 2012 (which was "in house") to the windows box,
2.3 GhZ Core duo.
Run with and without core.filemode, which triggers cygwinfstricks (and is shorter to type)
Numbers are typical "hot cache", cold cache is 6..8 seconds "real"


$ git --version
git version 1.8.3.1.g6f17ca7
__________________________________

$ time git -c core.filemode=true  status -uno
# On branch master
# Changes not staged for commit:
#   (use "git add <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working directory)
#
#       modified:   include/linux/netfilter/xt_CONNMARK.h
#       modified:   include/linux/netfilter/xt_DSCP.h
#       modified:   include/linux/netfilter/xt_MARK.h
#       modified:   include/linux/netfilter/xt_RATEEST.h
#       modified:   include/linux/netfilter/xt_TCPMSS.h
#       modified:   include/linux/netfilter_ipv4/ipt_ECN.h
#       modified:   include/linux/netfilter_ipv4/ipt_TTL.h
#       modified:   include/linux/netfilter_ipv6/ip6t_HL.h
#       modified:   net/netfilter/xt_DSCP.c
#       modified:   net/netfilter/xt_HL.c
#       modified:   net/netfilter/xt_RATEEST.c
#       modified:   net/netfilter/xt_TCPMSS.c
#
no changes added to commit (use "git add" and/or "git commit -a")

real    0m2.313s
user    0m0.577s
sys     0m1.765s
------------------------------------------------------
tb@snoopy ~/projects/linux-2.6
$ time git -c core.filemode=false status -uno
# On branch master
# Changes not staged for commit:
#   (use "git add <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working directory)
#
#       modified:   include/linux/netfilter/xt_CONNMARK.h
#       modified:   include/linux/netfilter/xt_DSCP.h
#       modified:   include/linux/netfilter/xt_MARK.h
#       modified:   include/linux/netfilter/xt_RATEEST.h
#       modified:   include/linux/netfilter/xt_TCPMSS.h
#       modified:   include/linux/netfilter_ipv4/ipt_ECN.h
#       modified:   include/linux/netfilter_ipv4/ipt_TTL.h
#       modified:   include/linux/netfilter_ipv6/ip6t_HL.h
#       modified:   net/netfilter/xt_DSCP.c
#       modified:   net/netfilter/xt_HL.c
#       modified:   net/netfilter/xt_RATEEST.c
#       modified:   net/netfilter/xt_TCPMSS.c
#
no changes added to commit (use "git add" and/or "git commit -a")

real    0m1.047s
user    0m0.311s
sys     0m0.765s

/Torsten

Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Some context: This is about a patch by Ramsay that removes the
> "schizophrenic lstat" hack for Cygwin. Junio, can you please queue that
> patch in pu?

Sure.  Thanks.

Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions

[Torsten Bögershausen]

On 2013-06-25 23.18, Junio C Hamano wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
> 
>> Some context: This is about a patch by Ramsay that removes the
>> "schizophrenic lstat" hack for Cygwin. Junio, can you please queue that
>> patch in pu?
> 
> Sure.  Thanks.

First of all,
thanks for the work.

Here some "benchmark" results, 
(The test run of the test suite did the same amout of time).

But:
git status -uno in real life takes double the time,
git 1.8.3 compared against "pu with the vanilla l/stat"
   
    1 second ->  2 seconds on linux kernel
0.2 seconds -> 0.4 seconds on git.git 

Do we have any known problems with the current implementation ?
Does speed matter ?

One vote to keep the special cygwin functions.
(And have a look how to improve the core.filemode)

/Torsten

Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions

[Ramsay Jones]

Torsten Bögershausen wrote:
> On 2013-06-25 23.18, Junio C Hamano wrote:
>> Johannes Sixt <j6t@kdbg.org> writes:
>>
>>> Some context: This is about a patch by Ramsay that removes the
>>> "schizophrenic lstat" hack for Cygwin. Junio, can you please queue that
>>> patch in pu?
>>
>> Sure.  Thanks.
> 
> First of all,
> thanks for the work.
> 
> Here some "benchmark" results, 
> (The test run of the test suite did the same amout of time).

The test suite runs noticeably faster for me.

> 
> But:
> git status -uno in real life takes double the time,
> git 1.8.3 compared against "pu with the vanilla l/stat"
>    
>     1 second ->  2 seconds on linux kernel
> 0.2 seconds -> 0.4 seconds on git.git 

Hmm, OK, I guess I will have to try something else. Sigh :(

> Do we have any known problems with the current implementation ?

Yes. The next branch is currently broken. (see reply to Junio)

> Does speed matter ?
> 
> One vote to keep the special cygwin functions.
> (And have a look how to improve the core.filemode)

I don't understand this (parenthetical) comment; could you
elaborate on this.

ATB,
Ramsay Jones

Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions

[Torsten Bögershausen]

On 2013-06-26 23.54, Ramsay Jones wrote:
> Torsten Bögershausen wrote:
>> On 2013-06-25 23.18, Junio C Hamano wrote:
>>> Johannes Sixt <j6t@kdbg.org> writes:
>>>
>>>> Some context: This is about a patch by Ramsay that removes the
>>>> "schizophrenic lstat" hack for Cygwin. Junio, can you please queue that
>>>> patch in pu?
>>>
>>> Sure.  Thanks.
>>
>> First of all,
>> thanks for the work.
>>
>> Here some "benchmark" results, 
>> (The test run of the test suite did the same amout of time).
> 
> The test suite runs noticeably faster for me.
> 
>>
>> But:
>> git status -uno in real life takes double the time,
>> git 1.8.3 compared against "pu with the vanilla l/stat"
>>    
>>     1 second ->  2 seconds on linux kernel
>> 0.2 seconds -> 0.4 seconds on git.git 
> 
> Hmm, OK, I guess I will have to try something else. Sigh :(
> 
>> Do we have any known problems with the current implementation ?
> 
> Yes. The next branch is currently broken. (see reply to Junio)
> 
>> Does speed matter ?
>>
>> One vote to keep the special cygwin functions.
>> (And have a look how to improve the core.filemode)
> 
> I don't understand this (parenthetical) comment; could you
> elaborate on this.
> 
> ATB,
> Ramsay Jones
This is probably wrong information:
I had in mind that cygwin sets core.filemode=false,
which is quite annoying when exchanging .sh files with linux.
But that seems to be wrong, a quick test shows that core.filemode=true.
Sorry for confusion.

Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions

[Ramsay Jones]

Torsten Bögershausen wrote:
[ ... ]

>>> (And have a look how to improve the core.filemode)
>>
>> I don't understand this (parenthetical) comment; could you
>> elaborate on this.
>>

> This is probably wrong information:
> I had in mind that cygwin sets core.filemode=false,

It does, see commit c869753e ("Force core.filemode to false on
Cygwin", 30-12-2006).

> which is quite annoying when exchanging .sh files with linux.

Indeed, I used to build git with NO_TRUSTABLE_FILEMODE reset so
that I wouldn't have to edit the config file by hand after a
git-clone or git-init.

> But that seems to be wrong, a quick test shows that core.filemode=true.

Hmm, it shouldn't - confused!

> Sorry for confusion.

ATB
Ramsay Jones

Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions

[Mark Levedahl]

On 06/26/2013 10:19 AM, Torsten Bögershausen wrote:
> On 2013-06-25 23.18, Junio C Hamano wrote:
>> Johannes Sixt <j6t@kdbg.org> writes:
>>
>>> Some context: This is about a patch by Ramsay that removes the
>>> "schizophrenic lstat" hack for Cygwin. Junio, can you please queue that
>>> patch in pu?
>> Sure.  Thanks.
> First of all,
> thanks for the work.
>
> Here some "benchmark" results,
> (The test run of the test suite did the same amout of time).
>
> But:
> git status -uno in real life takes double the time,
> git 1.8.3 compared against "pu with the vanilla l/stat"
>     
>      1 second ->  2 seconds on linux kernel
> 0.2 seconds -> 0.4 seconds on git.git
>
> Do we have any known problems with the current implementation ?
> Does speed matter ?
>
> One vote to keep the special cygwin functions.
> (And have a look how to improve the core.filemode)
>
> /Torsten
>
>
There have been threads on the cygwin mailing lists for at least a 
decade looking to speed up cygwin's posix stat / lstat (and fork). If 
improvement were merely difficult, it would have been done long ago. As 
git cares about things like execute bits, file / repository permissions, 
and soft links, whatever stat / lstat git uses needs to fully support 
those under cygwin, either by using what cygwin provides or providing a 
complete replacement. Note my other reply - with Ramsay's patch I can 
complete the test suite (except for t0008.sh that has a known hang) 
while without it I find the test suite randomly (unrepeatable) hangs in 
many tests. So, this stat/lstat replacement is at least implicated in 
current troubles.

Mark

Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions

[Ramsay Jones]

Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
> 
>> Michael Haggerty and Jeff King have been re-vamping the reference
>> handling code. The failures noted above were provoked by patches
>> in the 'mh/ref-races' branch. At the time I wrote this patch, that
>> branch was only included in 'pu', but I notice that this topic has
>> now progressed to 'next' (see commit 71f1a182).
> 
> I had an impression that up to 98eeb09e (for_each_ref: load all
> loose refs before packed refs, 2013-06-20) that is now in 'next'
> does not agressively use the lstat timestamp of the packed-refs
> file, and the "optional" bit 5d478f5c (refs: do not invalidate the
> packed-refs cache unnecessarily, 2013-06-20), and the one in 'next'
> should be safe with the cheating-lstat.  Isn't it the case?

The next branch (from a couple of days ago, namely commit 4f488db) is
currently broken on cygwin, like so:

  $ cd t
  $ ./t3211-peel-ref.sh -i -v
  
  [ ... ]
  
  ok 7 - refs are peeled outside of refs/tags (old packed)
  
  expecting success:
          git pack-refs --all &&
          cp .git/packed-refs fully-peeled &&
          git branch yadda &&
          git pack-refs --all &&
          git branch -d yadda &&
          test_cmp fully-peeled .git/packed-refs
  
  fatal: internal error: packed-ref cache cleared while locked
  not ok 8 - peeled refs survive deletion of packed ref
  #
  #               git pack-refs --all &&
  #               cp .git/packed-refs fully-peeled &&
  #               git branch yadda &&
  #               git pack-refs --all &&
  #               git branch -d yadda &&
  #               test_cmp fully-peeled .git/packed-refs
  #
  $ cd trash\ directory.t3211-peel-ref/
  $ ls
  actual  base.t  expect
  $ ../../bin-wrappers/git pack-refs --all
  fatal: internal error: packed-ref cache cleared while locked
  $ gdb ../../git.exe
  GNU gdb 6.5.50.20060706-cvs (cygwin-special)
  Copyright (C) 2006 Free Software Foundation, Inc.
  GDB is free software, covered by the GNU General Public License, and you are
  welcome to change it and/or distribute copies of it under certain conditions.
  Type "show copying" to see the conditions.
  There is absolutely no warranty for GDB.  Type "show warranty" for details.
  This GDB was configured as "i686-pc-cygwin"...
  (gdb) b stat_validity_check
  Breakpoint 1 at 0x48a33f: file read-cache.c, line 1965.
  (gdb) run pack-refs --all
  Starting program: /home/ramsay/git/git.exe pack-refs --all
  Loaded symbols for /cygdrive/c/WINDOWS/system32/ntdll.dll
  Loaded symbols for /cygdrive/c/WINDOWS/system32/kernel32.dll
  Loaded symbols for /usr/bin/cygcrypto-0.9.8.dll
  Loaded symbols for /usr/bin/cygwin1.dll
  Loaded symbols for /cygdrive/c/WINDOWS/system32/advapi32.dll
  Loaded symbols for /cygdrive/c/WINDOWS/system32/rpcrt4.dll
  Loaded symbols for /cygdrive/c/WINDOWS/system32/secur32.dll
  Loaded symbols for /usr/bin/cygiconv-2.dll
  Loaded symbols for /usr/bin/cygz.dll
  
  Breakpoint 1, stat_validity_check (sv=0xa611a4,
      path=0x57d98c ".git/packed-refs") at read-cache.c:1965
  1965            if (stat(path, &st) < 0)
  (gdb) n
  1967            if (!sv->sd)
  (gdb) p sv
  $1 = (struct stat_validity *) 0xa611a4
  (gdb) p *sv
  $2 = {sd = 0xa62478}
  (gdb) p sv->sd
  $3 = (struct stat_data *) 0xa62478
  (gdb) p *sv->sd
  $4 = {sd_ctime = {sec = 1372194879, nsec = 671875000}, sd_mtime = {
      sec = 1372194879, nsec = 656250000}, sd_dev = 2899104371,
    sd_ino = 180184, sd_uid = 1005, sd_gid = 513, sd_size = 296}
  (gdb) p st
  $5 = {st_dev = 0, st_ino = 0, st_mode = 33152, st_nlink = 1, st_uid = 0,
    st_gid = 0, st_rdev = 0, st_size = 296, st_atim = {tv_sec = 1372195048,
      tv_nsec = 890625000}, st_mtim = {tv_sec = 1372194879,
      tv_nsec = 656250000}, st_ctim = {tv_sec = 1372194879,
      tv_nsec = 15625000}, st_blksize = 5636381, st_blocks = 1, st_spare4 = {
      10883480, 5757324}}
  (gdb) c
  Continuing.
  fatal: internal error: packed-ref cache cleared while locked
  
  Program exited with code 0200.
  (gdb) quit
  $ ../../test-stat .git/packed-refs
  stat for '.git/packed-refs':
  *dev:   -1395862925, 0
  *ino:   180184, 0
  *mode:  100644 -rw-, 100600 -rw-
   nlink: 1, 1
  *uid:   1005, 0
  *gid:   513, 0
  *rdev:  -1395862925, 0
   size:  296, 296
   atime: 1372195048, 1372195048 Tue Jun 25 22:17:28 2013
   mtime: 1372194879, 1372194879 Tue Jun 25 22:14:39 2013
   ctime: 1372194879, 1372194879 Tue Jun 25 22:14:39 2013
  $
  
Note that, as noted before, (at least) the following tests fail
due to the 'mh/ref-races' branch:

  t3210-pack-refs.sh                        (Wstat: 256 Tests: 16 Failed: 12)
    Failed tests:  4-9, 11-16
    Non-zero exit status: 1
  t3211-peel-ref.sh                         (Wstat: 256 Tests: 8 Failed: 1)
    Failed test:  8
    Non-zero exit status: 1
  t5500-fetch-pack.sh                       (Wstat: 256 Tests: 54 Failed: 1)
    Failed test:  43
    Non-zero exit status: 1

> In any case, if removing the cheating-lstat will improve the
> robustness without hurting performance, I am all for it.

I'm sure it will improve robustness (and help with maintainability),
but as for performance ...

> The less platform specific hacks, the better ;-).

Agreed.

ATB,
Ramsay Jones

Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions

[Ramsay Jones]

Michael Haggerty wrote:
> On 06/25/2013 07:07 AM, Junio C Hamano wrote:
>> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
>>
>>> Michael Haggerty and Jeff King have been re-vamping the reference
>>> handling code. The failures noted above were provoked by patches
>>> in the 'mh/ref-races' branch. At the time I wrote this patch, that
>>> branch was only included in 'pu', but I notice that this topic has
>>> now progressed to 'next' (see commit 71f1a182).
>>
>> I had an impression that up to 98eeb09e (for_each_ref: load all
>> loose refs before packed refs, 2013-06-20) that is now in 'next'
>> does not agressively use the lstat timestamp of the packed-refs
>> file, and the "optional" bit 5d478f5c (refs: do not invalidate the
>> packed-refs cache unnecessarily, 2013-06-20), and the one in 'next'
>> should be safe with the cheating-lstat.  Isn't it the case?
>>
>> In any case, if removing the cheating-lstat will improve the
>> robustness without hurting performance, I am all for it.
>>
>> The less platform specific hacks, the better ;-).
> 
> The following patch in the "non-optional" commits (which has already
> been merged to next) uses stat() via the new stat_validity API:
> 
> ca9199300e get_packed_ref_cache: reload packed-refs file when it changes
> 
> This patch adds some *extra* cache invalidation that was heretofore
> missing.  If stat() is broken it could
> 
> (a) cause a false positive, resulting in some unnecessary cache
> invalidation and re-reading of packed-refs, which will hurt performance
> but not correctness; or
> 
> (b) cause a false negative, in which case the stale cache might be used
> for reading (but not writing), just as was *always* the case before this
> patch.
> 
> As far as I understand, the concern for cygwin is (a).  I will leave it
> to others to measure and/or decide whether the performance loss is too
> grave to endure until the cygwin stat() situation is fixed.
> 

Hmm, I'm not sure I understand ... However, I can confirm that the
'mh/ref-races' branch in next is broken on cygwin. (i.e. it is not
just a speed issue; it provokes fatal errors).

See reply to Junio.

ATB,
Ramsay Jones

Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions

[Jeff King]

On Wed, Jun 26, 2013 at 10:45:48PM +0100, Ramsay Jones wrote:

> > This patch adds some *extra* cache invalidation that was heretofore
> > missing.  If stat() is broken it could
> > 
> > (a) cause a false positive, resulting in some unnecessary cache
> > invalidation and re-reading of packed-refs, which will hurt performance
> > but not correctness; or
> > 
> > (b) cause a false negative, in which case the stale cache might be used
> > for reading (but not writing), just as was *always* the case before this
> > patch.
> > 
> > As far as I understand, the concern for cygwin is (a).  I will leave it
> > to others to measure and/or decide whether the performance loss is too
> > grave to endure until the cygwin stat() situation is fixed.
> 
> Hmm, I'm not sure I understand ... However, I can confirm that the
> 'mh/ref-races' branch in next is broken on cygwin. (i.e. it is not
> just a speed issue; it provokes fatal errors).

I think Michael's assessment above is missing one thing. It is true that
a false positive is just a performance problem in most cases, as we
unnecessarily reload the file, thinking it has changed.

However, when we have taken a lock on the file, there is an additional
safety measure: if we find the file is changed, we abort, as that should
never happen (it means somebody else modified the file while we had it
locked). But of course Cygwin's false positive here triggers the safety
valve, and we die without even realizing that nothing changed.

In theory we can drop the safety valve; it should never actually happen.
But I'd like to keep it there for working systems. Perhaps it is worth
doing something like this:

diff --git a/refs.c b/refs.c
index 4302206..7cac42d 100644
--- a/refs.c
+++ b/refs.c
@@ -1080,6 +1080,16 @@ static struct packed_ref_cache *get_packed_ref_cache(struct ref_cache *refs)
 		packed_refs_file = git_path("packed-refs");
 
 	if (refs->packed &&
+#ifdef STAT_VALIDITY_GIVES_FALSE_POSITIVES
+	    /*
+	     * If we get false positives from our stat calls on this platform,
+	     * then we must assume that once we have locked the packed-refs
+	     * file it does not change. Otherwise it looks like somebody else
+	     * has changed it out from under us (while we have it locked!), and
+	     * we die().
+	     */
+	    !refs->packed->lock &&
+#endif
 	    !stat_validity_check(&refs->packed->validity, packed_refs_file))
 		clear_packed_ref_cache(refs);
 

and then we can add that define to Cygwin in the Makefile. I verified
the issue on Linux by "breaking" stat_validity_check to always return 0
(i.e., to always give a false positive that the file has changed, which
is what Cygwin is doing).

I am curious how often Cygwin gives us the false positive. If it is
every time, then the check is not doing much good at all. Is it possible
for you to instrument stat_validity_check to report how often it does or
does not do anything useful?

-Peff

Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions

[Jeff King]

On Wed, Jun 26, 2013 at 06:35:52PM -0400, Jeff King wrote:

> I am curious how often Cygwin gives us the false positive. If it is
> every time, then the check is not doing much good at all. Is it possible
> for you to instrument stat_validity_check to report how often it does or
> does not do anything useful?

Maybe like this:

diff --git a/read-cache.c b/read-cache.c
index d5201f9..19dcb69 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1958,6 +1958,14 @@ void stat_validity_clear(struct stat_validity *sv)
 	sv->sd = NULL;
 }
 
+static int unchanged;
+static int attempts;
+static void print_stats(void)
+{
+	fprintf(stderr, "stat data was unchanged %d/%d\n",
+		unchanged, attempts);
+}
+
 int stat_validity_check(struct stat_validity *sv, const char *path)
 {
 	struct stat st;
@@ -1966,7 +1974,16 @@ int stat_validity_check(struct stat_validity *sv, const char *path)
 		return sv->sd == NULL;
 	if (!sv->sd)
 		return 0;
-	return S_ISREG(st.st_mode) && !match_stat_data(sv->sd, &st);
+	if (!S_ISREG(st.st_mode))
+		return 0;
+	if (!attempts++)
+		atexit(print_stats);
+	if (!match_stat_data(sv->sd, &st)) {
+		unchanged++;
+		return 1;
+	}
+	else
+		return 0;
 }
 
 void stat_validity_update(struct stat_validity *sv, int fd)

Running "t3211 -v", I see things like:

  stat data was unchanged 3/3
  stat data was unchanged 20/20
  stat data was unchanged 2/2
  Deleted branch yadda (was d1ff1c9).
  stat data was unchanged 8/8
  ok 8 - peeled refs survive deletion of packed ref

I am curious if you will see 0/20 or 19/20 there on Cygwin.

-Peff

Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions

[Ramsay Jones]

Jeff King wrote:
> On Wed, Jun 26, 2013 at 06:35:52PM -0400, Jeff King wrote:
> 
>> I am curious how often Cygwin gives us the false positive. If it is
>> every time, then the check is not doing much good at all. Is it possible
>> for you to instrument stat_validity_check to report how often it does or
>> does not do anything useful?
> 
> Maybe like this:
> 
> diff --git a/read-cache.c b/read-cache.c
> index d5201f9..19dcb69 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1958,6 +1958,14 @@ void stat_validity_clear(struct stat_validity *sv)
>  	sv->sd = NULL;
>  }
>  
> +static int unchanged;
> +static int attempts;
> +static void print_stats(void)
> +{
> +	fprintf(stderr, "stat data was unchanged %d/%d\n",
> +		unchanged, attempts);
> +}
> +
>  int stat_validity_check(struct stat_validity *sv, const char *path)
>  {
>  	struct stat st;
> @@ -1966,7 +1974,16 @@ int stat_validity_check(struct stat_validity *sv, const char *path)
>  		return sv->sd == NULL;
>  	if (!sv->sd)
>  		return 0;
> -	return S_ISREG(st.st_mode) && !match_stat_data(sv->sd, &st);
> +	if (!S_ISREG(st.st_mode))
> +		return 0;
> +	if (!attempts++)
> +		atexit(print_stats);
> +	if (!match_stat_data(sv->sd, &st)) {
> +		unchanged++;
> +		return 1;
> +	}
> +	else
> +		return 0;
>  }
>  
>  void stat_validity_update(struct stat_validity *sv, int fd)
> 
> Running "t3211 -v", I see things like:
> 
>   stat data was unchanged 3/3
>   stat data was unchanged 20/20
>   stat data was unchanged 2/2
>   Deleted branch yadda (was d1ff1c9).
>   stat data was unchanged 8/8
>   ok 8 - peeled refs survive deletion of packed ref
> 
> I am curious if you will see 0/20 or 19/20 there on Cygwin.

Ah, no ... the problem is not as subtle as this! :-D

I'm sorry if I failed to mention that I already had a patch
to solve the problem, but that I *don't* want to submit it,
since it is ugly and just serves to spread the madness! ;-)

This is why I tried the "cygwin: Remove the Win32 l/stat()
functions" patch first; I think this is the correct approach
to fixing this problem (and similar *future* problems).

However, since that is no longer an option, on performance
grounds, I have other ideas I want to try. (see later email)

The "schizophrenic stat" functions have caused many subtle
problems, but this is not one of them; a brick to the head
would be more subtle. The problem is caused by the "stat
validity" code using both fstat() and stat() on the same
file. Note that there is no Win32 implementation of the
fstat() function.

So, a solution to the problem would be to provide just such
an implementation. The patch to do so is given below, just
for illustration purposes.

This fixes the failures to t3210, t3211 and t5500. However, I
have not run the full test suite. This may cause some further
*subtle* problems (just grep for fstat and consider what may
go wrong; I have not done that), especially when you consider
that cygwin has the UNRELIABLE_FSTAT build variable set.

HTH

ATB,
Ramsay Jones

-- >8 --
Subject: [PATCH] cygwin: Add an Win32 fstat implementation

Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---
 compat/cygwin.c | 41 +++++++++++++++++++++++++++++++++++++++++
 compat/cygwin.h |  3 +++
 2 files changed, 44 insertions(+)

diff --git a/compat/cygwin.c b/compat/cygwin.c
index 91ce5d4..d59c9fb 100644
--- a/compat/cygwin.c
+++ b/compat/cygwin.c
@@ -4,6 +4,7 @@
 #include <sys/errno.h>
 #include "win32.h"
 #include "../git-compat-util.h"
+#include <io.h>
 #include "../cache.h" /* to read configuration */
 
 /*
@@ -100,6 +101,38 @@ static int cygwin_stat(const char *path, struct stat *buf)
 	return do_stat(path, buf, stat);
 }
 
+static int cygwin_fstat(int fd, struct stat *buf)
+{
+	HANDLE fh = (HANDLE)get_osfhandle(fd);
+	BY_HANDLE_FILE_INFORMATION fdata;
+
+	if (fh == INVALID_HANDLE_VALUE) {
+		errno = EBADF;
+		return -1;
+	}
+	if (GetFileType(fh) != FILE_TYPE_DISK)
+		return (fstat)(fd, buf);
+	if (GetFileInformationByHandle(fh, &fdata)) {
+		buf->st_dev = buf->st_rdev = 0; /* not used by Git */
+		buf->st_ino = 0;
+		buf->st_mode = file_attr_to_st_mode(fdata.dwFileAttributes);
+		buf->st_nlink = 1;
+		buf->st_uid = buf->st_gid = 0;
+#ifdef __CYGWIN_USE_BIG_TYPES__
+		buf->st_size = ((_off64_t)fdata.nFileSizeHigh << 32) +
+			fdata.nFileSizeLow;
+#else
+		buf->st_size = (off_t)fdata.nFileSizeLow;
+#endif
+		buf->st_blocks = size_to_blocks(buf->st_size);
+		filetime_to_timespec(&fdata.ftLastAccessTime, &buf->st_atim);
+		filetime_to_timespec(&fdata.ftLastWriteTime, &buf->st_mtim);
+		filetime_to_timespec(&fdata.ftCreationTime, &buf->st_ctim);
+		return 0;
+	}
+	errno = EBADF;
+	return -1;
+}
 
 /*
  * At start up, we are trying to determine whether Win32 API or cygwin stat
@@ -133,9 +166,11 @@ static int init_stat(void)
 		if (!core_filemode && native_stat) {
 			cygwin_stat_fn = cygwin_stat;
 			cygwin_lstat_fn = cygwin_lstat;
+			cygwin_fstat_fn = cygwin_fstat;
 		} else {
 			cygwin_stat_fn = stat;
 			cygwin_lstat_fn = lstat;
+			cygwin_fstat_fn = fstat;
 		}
 		return 0;
 	}
@@ -152,6 +187,12 @@ static int cygwin_lstat_stub(const char *file_name, struct stat *buf)
 	return (init_stat() ? lstat : *cygwin_lstat_fn)(file_name, buf);
 }
 
+static int cygwin_fstat_stub(int fd, struct stat *buf)
+{
+	return (init_stat() ? fstat : *cygwin_fstat_fn)(fd, buf);
+}
+
 stat_fn_t cygwin_stat_fn = cygwin_stat_stub;
 stat_fn_t cygwin_lstat_fn = cygwin_lstat_stub;
+fstat_fn_t cygwin_fstat_fn = cygwin_fstat_stub;
 
diff --git a/compat/cygwin.h b/compat/cygwin.h
index c04965a..f3ca3d2 100644
--- a/compat/cygwin.h
+++ b/compat/cygwin.h
@@ -2,8 +2,10 @@
 #include <sys/stat.h>
 
 typedef int (*stat_fn_t)(const char*, struct stat*);
+typedef int (*fstat_fn_t)(int, struct stat*);
 extern stat_fn_t cygwin_stat_fn;
 extern stat_fn_t cygwin_lstat_fn;
+extern fstat_fn_t cygwin_fstat_fn;
 int cygwin_get_st_mode_bits(const char *path, int *mode);
 
 #define get_st_mode_bits(p,m) cygwin_get_st_mode_bits((p),(m))
@@ -11,4 +13,5 @@ int cygwin_get_st_mode_bits(const char *path, int *mode);
 /* cygwin.c needs the original lstat() */
 #define stat(path, buf) (*cygwin_stat_fn)(path, buf)
 #define lstat(path, buf) (*cygwin_lstat_fn)(path, buf)
+#define fstat(fd, buf) (*cygwin_fstat_fn)(fd, buf)
 #endif
-- 
1.8.3

Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions

[Mark Levedahl]

On 06/27/2013 06:58 PM, Ramsay Jones wrote:

> This is why I tried the "cygwin: Remove the Win32 l/stat()
> functions" patch first; I think this is the correct approach
> to fixing this problem (and similar *future* problems).
I adamantly agree.
> However, since that is no longer an option, on performance
> grounds, I have other ideas I want to try. (see later email)

Correctness first, speed later.

1) Keep the patch to remove the buggy and unreliable stat / lstat.
2) We fix the remaining test failures.
3) With the test suite passing, stat optimization(s) that cause no 
failures / regressions can be accepted.

With the msys/mingw git available for years now, there really is not a 
reason to make Cygwin's git violate the Posix expectations of that 
platform. msys makes no such promises, so is the right tool for those on 
Windows who just want git as fast as possible on Windows (still 
slooooow) and don't care about file modes, softlinks, etc.

I'm keeping your patch in my tree.

Mark

Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions

[Michael Haggerty]

On 06/27/2013 12:35 AM, Jeff King wrote:
> On Wed, Jun 26, 2013 at 10:45:48PM +0100, Ramsay Jones wrote:
> 
>>> This patch adds some *extra* cache invalidation that was heretofore
>>> missing.  If stat() is broken it could
>>>
>>> (a) cause a false positive, resulting in some unnecessary cache
>>> invalidation and re-reading of packed-refs, which will hurt performance
>>> but not correctness; or
>>>
>>> (b) cause a false negative, in which case the stale cache might be used
>>> for reading (but not writing), just as was *always* the case before this
>>> patch.
>>>
>>> As far as I understand, the concern for cygwin is (a).  I will leave it
>>> to others to measure and/or decide whether the performance loss is too
>>> grave to endure until the cygwin stat() situation is fixed.
>>
>> Hmm, I'm not sure I understand ... However, I can confirm that the
>> 'mh/ref-races' branch in next is broken on cygwin. (i.e. it is not
>> just a speed issue; it provokes fatal errors).
> 
> I think Michael's assessment above is missing one thing.

Peff is absolutely right; for some unknown reason I was thinking of the
consistency check as having been already fixed.

> However, when we have taken a lock on the file, there is an additional
> safety measure: if we find the file is changed, we abort, as that should
> never happen (it means somebody else modified the file while we had it
> locked). But of course Cygwin's false positive here triggers the safety
> valve, and we die without even realizing that nothing changed.
> 
> In theory we can drop the safety valve; it should never actually happen.
> But I'd like to keep it there for working systems. Perhaps it is worth
> doing something like this:
> 
> [...#ifdef out consistency check on cygwin when lock is held...]

Yes, this would work.

But, taking a step back, I think it is a bad idea to have an unreliable
stat() masquerading as a real stat().  If we want to allow the use of an
unreliable stat for certain purposes, let's have two stat() interfaces:

* the true stat() (in this case I guess cygwin's slow-but-correct
implementation)

* some fast_but_maybe_unreliable_stat(), which would map to stat() on
most platforms but might map to the Windows stat() on cygwin when so
configured.

By default the true stat() would always be used.  It should have to be a
conscious decision, taken only in specific, vetted scenarios, to use the
unreliable stat.

For example, I can't imagine that checking the freshness of the index or
of the packed-refs file is ever going to be a bottleneck, so there is no
reason at all to use an unreliable stat() here.

On the other hand, stat() seems definitely to be a bottleneck when
testing for changes in a 100,000 file working tree, and here occasional
mistakes might be considered acceptable.  So for this purpose the
unreliable stat() might be used.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions

[Jeff King]

On Thu, Jun 27, 2013 at 07:51:57AM +0200, Michael Haggerty wrote:

> > In theory we can drop the safety valve; it should never actually happen.
> > But I'd like to keep it there for working systems. Perhaps it is worth
> > doing something like this:
> > 
> > [...#ifdef out consistency check on cygwin when lock is held...]
> 
> Yes, this would work.
> 
> But, taking a step back, I think it is a bad idea to have an unreliable
> stat() masquerading as a real stat().  If we want to allow the use of an
> unreliable stat for certain purposes, let's have two stat() interfaces:
> 
> * the true stat() (in this case I guess cygwin's slow-but-correct
> implementation)
> 
> * some fast_but_maybe_unreliable_stat(), which would map to stat() on
> most platforms but might map to the Windows stat() on cygwin when so
> configured.

Yeah, that makes sense to me. I don't have a particular opinion on which
way to go, as I do not use cygwin at all (and on most platforms, the
two stat interfaces would just both call stat()).

I will leave it up to Cygwin folks whether they want to do something
like my patch as a band-aid while working up the two-stat() solution.

-Peff

Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> But, taking a step back, I think it is a bad idea to have an unreliable
>> stat() masquerading as a real stat().  If we want to allow the use of an
>> unreliable stat for certain purposes, let's have two stat() interfaces:
>> 
>> * the true stat() (in this case I guess cygwin's slow-but-correct
>> implementation)
>> 
>> * some fast_but_maybe_unreliable_stat(), which would map to stat() on
>> most platforms but might map to the Windows stat() on cygwin when so
>> configured.
>
> Yeah, that makes sense to me. I don't have a particular opinion on which
> way to go, as I do not use cygwin at all (and on most platforms, the
> two stat interfaces would just both call stat()).
>
> I will leave it up to Cygwin folks whether they want to do something
> like my patch as a band-aid while working up the two-stat() solution.

I think in the longer term two-stat solution is a good thing.  

0117c2f0 (Make core.sharedRepository work under cygwin 1.7,
2013-03-23) introduced get_st_mode_bits() to work around another
glitch in the fast-but-cheating-and-unreliable replacement, which
we may be able to revert once it is done.

Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions

[Ramsay Jones]

Michael Haggerty wrote:
> On 06/27/2013 12:35 AM, Jeff King wrote:
[ ... ]
>> I think Michael's assessment above is missing one thing.
> 
> Peff is absolutely right; for some unknown reason I was thinking of the
> consistency check as having been already fixed.

Well, the "cygwin: Remove the Win32 l/stat() functions" patch *does* fix
the problem. :-D It's just a pity we can't use it on performance grounds. :(

>> [...#ifdef out consistency check on cygwin when lock is held...]
> 
> Yes, this would work.
> 
> But, taking a step back, I think it is a bad idea to have an unreliable
> stat() masquerading as a real stat().  If we want to allow the use of an
> unreliable stat for certain purposes, let's have two stat() interfaces:
> 
> * the true stat() (in this case I guess cygwin's slow-but-correct
> implementation)
> 
> * some fast_but_maybe_unreliable_stat(), which would map to stat() on
> most platforms but might map to the Windows stat() on cygwin when so
> configured.
> 
> By default the true stat() would always be used.  It should have to be a
> conscious decision, taken only in specific, vetted scenarios, to use the
> unreliable stat.

You have just described my second patch! :D

> 
> For example, I can't imagine that checking the freshness of the index or
> of the packed-refs file is ever going to be a bottleneck, so there is no
> reason at all to use an unreliable stat() here.
> 
> On the other hand, stat() seems definitely to be a bottleneck when
> testing for changes in a 100,000 file working tree, and here occasional
> mistakes might be considered acceptable.  So for this purpose the
> unreliable stat() might be used.

I have already written the first pass at this patch, but I'm having
difficulty with naming (get_cached_stat_data, get_index_stat_data,
get_stat_data, ... ;-)

ATB,
Ramsay Jones

Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions

[Ramsay Jones]

Ramsay Jones wrote:
> Michael Haggerty wrote:
>> On 06/27/2013 12:35 AM, Jeff King wrote:
> [ ... ]
>>> I think Michael's assessment above is missing one thing.
>>
>> Peff is absolutely right; for some unknown reason I was thinking of the
>> consistency check as having been already fixed.
> 
> Well, the "cygwin: Remove the Win32 l/stat() functions" patch *does* fix
> the problem. :-D It's just a pity we can't use it on performance grounds. :(
> 
>>> [...#ifdef out consistency check on cygwin when lock is held...]
>>
>> Yes, this would work.
>>
>> But, taking a step back, I think it is a bad idea to have an unreliable
>> stat() masquerading as a real stat().  If we want to allow the use of an
>> unreliable stat for certain purposes, let's have two stat() interfaces:
>>
>> * the true stat() (in this case I guess cygwin's slow-but-correct
>> implementation)
>>
>> * some fast_but_maybe_unreliable_stat(), which would map to stat() on
>> most platforms but might map to the Windows stat() on cygwin when so
>> configured.
>>
>> By default the true stat() would always be used.  It should have to be a
>> conscious decision, taken only in specific, vetted scenarios, to use the
>> unreliable stat.
> 
> You have just described my second patch! :D

Unfortunately, I have not had any time to work on the patch this weekend.
However, despite the patch being a bit rough around the edges, I decided
to send it out (see below) to get some early feedback.

Note that it passes the t3210, t3211, t5500, t3200, t3301, t7606 and t1301
tests, but I have not run the full test suite.

Comments welcome.

ATB,
Ramsay Jones

-- >8 --
Subject: [PATCH] cygwin: Add fast_[l]stat() functions

Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---
 builtin/apply.c        |  6 +++---
 builtin/commit.c       |  2 +-
 builtin/ls-files.c     |  2 +-
 builtin/rm.c           |  2 +-
 builtin/update-index.c |  2 +-
 check-racy.c           |  2 +-
 compat/cygwin.c        | 12 ------------
 compat/cygwin.h        | 10 +++-------
 diff-lib.c             |  2 +-
 diff.c                 |  2 +-
 entry.c                |  4 ++--
 git-compat-util.h      | 13 +++++++++++--
 help.c                 |  5 +----
 path.c                 |  9 +--------
 preload-index.c        |  2 +-
 read-cache.c           |  6 +++---
 unpack-trees.c         |  8 ++++----
 17 files changed, 36 insertions(+), 53 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 0e9b631..ca26caa 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3253,7 +3253,7 @@ static int load_current(struct image *image, struct patch *patch)
 	if (pos < 0)
 		return error(_("%s: does not exist in index"), name);
 	ce = active_cache[pos];
-	if (lstat(name, &st)) {
+	if (fast_lstat(name, &st)) {
 		if (errno != ENOENT)
 			return error(_("%s: %s"), name, strerror(errno));
 		if (checkout_target(ce, &st))
@@ -3396,7 +3396,7 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s
 	if (previous) {
 		st_mode = previous->new_mode;
 	} else if (!cached) {
-		stat_ret = lstat(old_name, st);
+		stat_ret = fast_lstat(old_name, st);
 		if (stat_ret && errno != ENOENT)
 			return error(_("%s: %s"), old_name, strerror(errno));
 	}
@@ -3850,7 +3850,7 @@ static void add_index_file(const char *path, unsigned mode, void *buf, unsigned
 			die(_("corrupt patch for subproject %s"), path);
 	} else {
 		if (!cached) {
-			if (lstat(path, &st) < 0)
+			if (fast_lstat(path, &st) < 0)
 				die_errno(_("unable to stat newly created file '%s'"),
 					  path);
 			fill_stat_cache_info(ce, &st);
diff --git a/builtin/commit.c b/builtin/commit.c
index 6b693c1..1d208c6 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -231,7 +231,7 @@ static void add_remove_files(struct string_list *list)
 		if (p->util)
 			continue;
 
-		if (!lstat(p->string, &st)) {
+		if (!fast_lstat(p->string, &st)) {
 			if (add_to_cache(p->string, &st, 0))
 				die(_("updating files failed"));
 		} else
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 08d9786..db66a0e 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -251,7 +251,7 @@ static void show_files(struct dir_struct *dir)
 				continue;
 			if (ce_skip_worktree(ce))
 				continue;
-			err = lstat(ce->name, &st);
+			err = fast_lstat(ce->name, &st);
 			if (show_deleted && err)
 				show_ce_entry(tag_removed, ce);
 			if (show_modified && ce_modified(ce, &st, 0))
diff --git a/builtin/rm.c b/builtin/rm.c
index 06025a2..4b783e7 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -143,7 +143,7 @@ static int check_local_mod(unsigned char *head, int index_only)
 		}
 		ce = active_cache[pos];
 
-		if (lstat(ce->name, &st) < 0) {
+		if (fast_lstat(ce->name, &st) < 0) {
 			if (errno != ENOENT && errno != ENOTDIR)
 				warning("'%s': %s", ce->name, strerror(errno));
 			/* It already vanished from the working tree */
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 5c7762e..4790e4c 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -206,7 +206,7 @@ static int process_path(const char *path)
 	 * First things first: get the stat information, to decide
 	 * what to do about the pathname!
 	 */
-	if (lstat(path, &st) < 0)
+	if (fast_lstat(path, &st) < 0)
 		return process_lstat_error(path, errno);
 
 	if (S_ISDIR(st.st_mode))
diff --git a/check-racy.c b/check-racy.c
index 00d92a1..6124355 100644
--- a/check-racy.c
+++ b/check-racy.c
@@ -11,7 +11,7 @@ int main(int ac, char **av)
 		struct cache_entry *ce = active_cache[i];
 		struct stat st;
 
-		if (lstat(ce->name, &st)) {
+		if (fast_lstat(ce->name, &st)) {
 			error("lstat(%s): %s", ce->name, strerror(errno));
 			continue;
 		}
diff --git a/compat/cygwin.c b/compat/cygwin.c
index 91ce5d4..f07cbd3 100644
--- a/compat/cygwin.c
+++ b/compat/cygwin.c
@@ -6,18 +6,6 @@
 #include "../git-compat-util.h"
 #include "../cache.h" /* to read configuration */
 
-/*
- * Return POSIX permission bits, regardless of core.ignorecygwinfstricks
- */
-int cygwin_get_st_mode_bits(const char *path, int *mode)
-{
-	struct stat st;
-	if (lstat(path, &st) < 0)
-		return -1;
-	*mode = st.st_mode;
-	return 0;
-}
-
 static inline void filetime_to_timespec(const FILETIME *ft, struct timespec *ts)
 {
 	long long winTime = ((long long)ft->dwHighDateTime << 32) +
diff --git a/compat/cygwin.h b/compat/cygwin.h
index c04965a..8299f58 100644
--- a/compat/cygwin.h
+++ b/compat/cygwin.h
@@ -4,11 +4,7 @@
 typedef int (*stat_fn_t)(const char*, struct stat*);
 extern stat_fn_t cygwin_stat_fn;
 extern stat_fn_t cygwin_lstat_fn;
-int cygwin_get_st_mode_bits(const char *path, int *mode);
 
-#define get_st_mode_bits(p,m) cygwin_get_st_mode_bits((p),(m))
-#ifndef CYGWIN_C
-/* cygwin.c needs the original lstat() */
-#define stat(path, buf) (*cygwin_stat_fn)(path, buf)
-#define lstat(path, buf) (*cygwin_lstat_fn)(path, buf)
-#endif
+#define fast_stat(path, buf) (*cygwin_stat_fn)(path, buf)
+#define fast_lstat(path, buf) (*cygwin_lstat_fn)(path, buf)
+#define GIT_FAST_STAT
diff --git a/diff-lib.c b/diff-lib.c
index b6f4b21..401dab6 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -27,7 +27,7 @@
  */
 static int check_removed(const struct cache_entry *ce, struct stat *st)
 {
-	if (lstat(ce->name, st) < 0) {
+	if (fast_lstat(ce->name, st) < 0) {
 		if (errno != ENOENT && errno != ENOTDIR)
 			return -1;
 		return 1;
diff --git a/diff.c b/diff.c
index 208094f..212d3ff 100644
--- a/diff.c
+++ b/diff.c
@@ -2642,7 +2642,7 @@ static int reuse_worktree_file(const char *name, const unsigned char *sha1, int
 	 * If ce matches the file in the work tree, we can reuse it.
 	 */
 	if (ce_uptodate(ce) ||
-	    (!lstat(name, &st) && !ce_match_stat(ce, &st, 0)))
+	    (!fast_lstat(name, &st) && !ce_match_stat(ce, &st, 0)))
 		return 1;
 
 	return 0;
diff --git a/entry.c b/entry.c
index d7c131d..4d2ac73 100644
--- a/entry.c
+++ b/entry.c
@@ -210,7 +210,7 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout
 finish:
 	if (state->refresh_cache) {
 		if (!fstat_done)
-			lstat(ce->name, &st);
+			fast_lstat(ce->name, &st);
 		fill_stat_cache_info(ce, &st);
 	}
 	return 0;
@@ -230,7 +230,7 @@ static int check_path(const char *path, int len, struct stat *st, int skiplen)
 		errno = ENOENT;
 		return -1;
 	}
-	return lstat(path, st);
+	return fast_lstat(path, st);
 }
 
 int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *topath)
diff --git a/git-compat-util.h b/git-compat-util.h
index ff193f4..7997bdb 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -129,8 +129,6 @@
 #include <poll.h>
 #endif
 
-extern int get_st_mode_bits(const char *path, int *mode);
-
 #if defined(__MINGW32__)
 /* pull in Windows compatibility stuff */
 #include "compat/mingw.h"
@@ -179,6 +177,17 @@ typedef unsigned long uintptr_t;
 #endif
 #endif
 
+#ifndef GIT_FAST_STAT
+static inline int fast_stat(const char *path, struct stat *st)
+{
+	return stat(path, st);
+}
+static inline int fast_lstat(const char *path, struct stat *st)
+{
+	return lstat(path, st);
+}
+#endif
+
 /* used on Mac OS X */
 #ifdef PRECOMPOSE_UNICODE
 #include "compat/precompose_utf8.h"
diff --git a/help.c b/help.c
index 08c54ef..f068925 100644
--- a/help.c
+++ b/help.c
@@ -107,10 +107,7 @@ static int is_executable(const char *name)
 	    !S_ISREG(st.st_mode))
 		return 0;
 
-#if defined(GIT_WINDOWS_NATIVE) || defined(__CYGWIN__)
-#if defined(__CYGWIN__)
-if ((st.st_mode & S_IXUSR) == 0)
-#endif
+#if defined(GIT_WINDOWS_NATIVE)
 {	/* cannot trust the executable bit, peek into the file instead */
 	char buf[3] = { 0 };
 	int n;
diff --git a/path.c b/path.c
index 7f3324a..3d244d3 100644
--- a/path.c
+++ b/path.c
@@ -5,13 +5,7 @@
 #include "strbuf.h"
 #include "string-list.h"
 
-#ifndef get_st_mode_bits
-/*
- * The replacement lstat(2) we use on Cygwin is incomplete and
- * may return wrong permission bits. Most of the time we do not care,
- * but the callsites of this wrapper do care.
- */
-int get_st_mode_bits(const char *path, int *mode)
+static int get_st_mode_bits(const char *path, int *mode)
 {
 	struct stat st;
 	if (lstat(path, &st) < 0)
@@ -19,7 +13,6 @@ int get_st_mode_bits(const char *path, int *mode)
 	*mode = st.st_mode;
 	return 0;
 }
-#endif
 
 static char bad_path[] = "/bad-path/";
 
diff --git a/preload-index.c b/preload-index.c
index 49cb08d..1bece91 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -57,7 +57,7 @@ static void *preload_thread(void *_data)
 			continue;
 		if (threaded_has_symlink_leading_path(&cache, ce->name, ce_namelen(ce)))
 			continue;
-		if (lstat(ce->name, &st))
+		if (fast_lstat(ce->name, &st))
 			continue;
 		if (ie_match_stat(index, ce, &st, CE_MATCH_RACY_IS_DIRTY))
 			continue;
diff --git a/read-cache.c b/read-cache.c
index d5201f9..ed33d9e 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -689,7 +689,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
 int add_file_to_index(struct index_state *istate, const char *path, int flags)
 {
 	struct stat st;
-	if (lstat(path, &st))
+	if (fast_lstat(path, &st))
 		die_errno("unable to stat '%s'", path);
 	return add_to_index(istate, path, &st, flags);
 }
@@ -1049,7 +1049,7 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,
 		return ce;
 	}
 
-	if (lstat(ce->name, &st) < 0) {
+	if (fast_lstat(ce->name, &st) < 0) {
 		if (err)
 			*err = errno;
 		return NULL;
@@ -1635,7 +1635,7 @@ static void ce_smudge_racily_clean_entry(struct cache_entry *ce)
 	 */
 	struct stat st;
 
-	if (lstat(ce->name, &st) < 0)
+	if (fast_lstat(ce->name, &st) < 0)
 		return;
 	if (ce_match_stat_basic(ce, &st))
 		return;
diff --git a/unpack-trees.c b/unpack-trees.c
index b27f2a6..1fe9b63 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1215,7 +1215,7 @@ static int verify_uptodate_1(const struct cache_entry *ce,
 	else if (o->reset || ce_uptodate(ce))
 		return 0;
 
-	if (!lstat(ce->name, &st)) {
+	if (!fast_lstat(ce->name, &st)) {
 		int flags = CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE;
 		unsigned changed = ie_match_stat(o->src_index, ce, &st, flags);
 		if (!changed)
@@ -1432,13 +1432,13 @@ static int verify_absent_1(const struct cache_entry *ce,
 		char path[PATH_MAX + 1];
 		memcpy(path, ce->name, len);
 		path[len] = 0;
-		if (lstat(path, &st))
+		if (fast_lstat(path, &st))
 			return error("cannot stat '%s': %s", path,
 					strerror(errno));
 
 		return check_ok_to_remove(path, len, DT_UNKNOWN, NULL, &st,
 				error_type, o);
-	} else if (lstat(ce->name, &st)) {
+	} else if (fast_lstat(ce->name, &st)) {
 		if (errno != ENOENT)
 			return error("cannot stat '%s': %s", ce->name,
 				     strerror(errno));
@@ -1852,7 +1852,7 @@ int oneway_merge(const struct cache_entry * const *src,
 		int update = 0;
 		if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) {
 			struct stat st;
-			if (lstat(old->name, &st) ||
+			if (fast_lstat(old->name, &st) ||
 			    ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))
 				update |= CE_UPDATE;
 		}
-- 
1.8.3

Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions

[Junio C Hamano]

Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:

> Ramsay Jones wrote:
>> Michael Haggerty wrote:
>>> On 06/27/2013 12:35 AM, Jeff King wrote:
>> [ ... ]
>>>> I think Michael's assessment above is missing one thing.
>>>
>>> Peff is absolutely right; for some unknown reason I was thinking of the
>>> consistency check as having been already fixed.
>> 
>> Well, the "cygwin: Remove the Win32 l/stat() functions" patch *does* fix
>> the problem. :-D It's just a pity we can't use it on performance grounds. :(
>> 
>>>> [...#ifdef out consistency check on cygwin when lock is held...]
>>>
>>> Yes, this would work.
>>>
>>> But, taking a step back, I think it is a bad idea to have an unreliable
>>> stat() masquerading as a real stat().  If we want to allow the use of an
>>> unreliable stat for certain purposes, let's have two stat() interfaces:
>>>
>>> * the true stat() (in this case I guess cygwin's slow-but-correct
>>> implementation)
>>>
>>> * some fast_but_maybe_unreliable_stat(), which would map to stat() on
>>> most platforms but might map to the Windows stat() on cygwin when so
>>> configured.
>>>
>>> By default the true stat() would always be used.  It should have to be a
>>> conscious decision, taken only in specific, vetted scenarios, to use the
>>> unreliable stat.
>>  ...

I like the part that gets rid of that "get-mode-bits" but at the
same time, I find this part wanting a reasonable in-code comment.

At least, with the earlier get-mode-bits, it was clear why we are
doing something special in that codepath, both from the name of the
helper function/macro and the comment attached to it describing how
the "regular" one is cheating.

We must say why this "fast" is not used everywhere and what criteria
you should apply when deciding to use it (or not use it).  And the
"fast" name is much less descriptive.

I suspect (without thinking it through) that the rule would be
something like:

    The "fast" variant is to be used to read from the filesystem the
    "stat" bits that are stuffed into the index for quick "touch
    detection" (aka "cached stat info") and/or that are compared
    with the cached stat info, and should not be used anywhere else.

But then we always use lstat(2) and not stat(2) for that, so...

> +#ifndef GIT_FAST_STAT
> +static inline int fast_stat(const char *path, struct stat *st)
> +{
> +	return stat(path, st);
> +}
> +static inline int fast_lstat(const char *path, struct stat *st)
> +{
> +	return lstat(path, st);
> +}
> +#endif

Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions

[Ramsay Jones]

Junio C Hamano wrote:
> I like the part that gets rid of that "get-mode-bits" but at the
> same time, I find this part wanting a reasonable in-code comment.

Indeed. (As I said, a bit rough around the edges ;-)

> At least, with the earlier get-mode-bits, it was clear why we are
> doing something special in that codepath, both from the name of the
> helper function/macro and the comment attached to it describing how
> the "regular" one is cheating.
> 
> We must say why this "fast" is not used everywhere and what criteria
> you should apply when deciding to use it (or not use it).  And the
> "fast" name is much less descriptive.
> 
> I suspect (without thinking it through) that the rule would be
> something like:
> 
>     The "fast" variant is to be used to read from the filesystem the
>     "stat" bits that are stuffed into the index for quick "touch
>     detection" (aka "cached stat info") and/or that are compared
>     with the cached stat info, and should not be used anywhere else.

Sounds good to me.

> But then we always use lstat(2) and not stat(2) for that, so...

Indeed. Although there may be need of an "fast_fstat" (see below). :(

>> +#ifndef GIT_FAST_STAT
>> +static inline int fast_stat(const char *path, struct stat *st)
>> +{
>> +	return stat(path, st);
>> +}
>> +static inline int fast_lstat(const char *path, struct stat *st)
>> +{
>> +	return lstat(path, st);
>> +}
>> +#endif

Yes, I'm not very good at naming things, so suggestions welcome!

Note that I missed at least one lstat() call which needed to be renamed
to fast_lstat() (builtin/apply.c, line 3094 in checkout_target()).
This is my main concern with this patch (i.e. that I have missed some
more lstat calls that need to be renamed). I was a little surprised
at the size of the patch; direct index manipulation is more widespread
than I had expected.

Also, since cygwin has UNRELIABLE_FSTAT defined, on the first pass of
the patch, I ignored the use of fstat() in write_entry(). However, *if*
we allow for some other platform, which has a reliable fstat, but wants
to provide "fast" stat variants, then these fstat calls should logically
be "fast". Alternatively, we could drop the use of fstat(), like so:

  diff --git a/entry.c b/entry.c
  index 4d2ac73..d04d7a1 100644
  --- a/entry.c
  +++ b/entry.c
  @@ -104,21 +104,9 @@ static int open_output_fd(char *path, struct cache_entry *ce, int to_tempfile)
   	}
   }
 
  -static int fstat_output(int fd, const struct checkout *state, struct stat *st)
  -{
  -	/* use fstat() only when path == ce->name */
  -	if (fstat_is_reliable() &&
  -	    state->refresh_cache && !state->base_dir_len) {
  -		fstat(fd, st);
  -		return 1;
  -	}
  -	return 0;
  -}
  -
   static int streaming_write_entry(struct cache_entry *ce, char *path,
   				 struct stream_filter *filter,
  -				 const struct checkout *state, int to_tempfile,
  -				 int *fstat_done, struct stat *statbuf)
  +				 const struct checkout *state, int to_tempfile)
   {
   	int result = 0;
   	int fd;
  @@ -128,7 +116,6 @@ static int streaming_write_entry(struct cache_entry *ce, char *path,
   		return -1;
 
   	result |= stream_blob_to_fd(fd, ce->sha1, filter, 1);
  -	*fstat_done = fstat_output(fd, state, statbuf);
   	result |= close(fd);
 
   	if (result)
  @@ -139,7 +126,7 @@ static int streaming_write_entry(struct cache_entry *ce, char *path,
   static int write_entry(struct cache_entry *ce, char *path, const struct checkout *state, int to_tempfile)
   {
   	unsigned int ce_mode_s_ifmt = ce->ce_mode & S_IFMT;
  -	int fd, ret, fstat_done = 0;
  +	int fd, ret;
   	char *new;
   	struct strbuf buf = STRBUF_INIT;
   	unsigned long size;
  @@ -150,8 +137,7 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout
   		struct stream_filter *filter = get_stream_filter(ce->name, ce->sha1);
   		if (filter &&
   		    !streaming_write_entry(ce, path, filter,
  -					   state, to_tempfile,
  -					   &fstat_done, &st))
  +					   state, to_tempfile))
   			goto finish;
   	}
 
  @@ -190,8 +176,6 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout
   		}
 
   		wrote = write_in_full(fd, new, size);
  -		if (!to_tempfile)
  -			fstat_done = fstat_output(fd, state, &st);
   		close(fd);
   		free(new);
   		if (wrote != size)
  @@ -209,8 +193,7 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout
 
   finish:
   	if (state->refresh_cache) {
  -		if (!fstat_done)
  -			fast_lstat(ce->name, &st);
  +		fast_lstat(ce->name, &st);
   		fill_stat_cache_info(ce, &st);
   	}
   	return 0;
  -- 

I would need to do some timing tests (on Linux) to see what effect this
would have on performance. (I noticed that the test suite ran about 2%
slower with the above applied).

A simple pass-though fast_fstat(), including on cygwin, is probably the
way to go.

Sorry for being a bit slow on this - I'm very busy at the moment. :(

ATB,
Ramsay Jones

Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions

[Torsten Bögershausen]

On 30.06.13 19:28, Ramsay Jones wrote:
> Ramsay Jones wrote:
>> Michael Haggerty wrote:
>>> On 06/27/2013 12:35 AM, Jeff King wrote:
>> [ ... ]
>>>> I think Michael's assessment above is missing one thing.
>>> Peff is absolutely right; for some unknown reason I was thinking of the
>>> consistency check as having been already fixed.
>> Well, the "cygwin: Remove the Win32 l/stat() functions" patch *does* fix
>> the problem. :-D It's just a pity we can't use it on performance grounds. :(
>>
>>>> [...#ifdef out consistency check on cygwin when lock is held...]
>>> Yes, this would work.
>>>
>>> But, taking a step back, I think it is a bad idea to have an unreliable
>>> stat() masquerading as a real stat().  If we want to allow the use of an
>>> unreliable stat for certain purposes, let's have two stat() interfaces:
>>>
>>> * the true stat() (in this case I guess cygwin's slow-but-correct
>>> implementation)
>>>
>>> * some fast_but_maybe_unreliable_stat(), which would map to stat() on
>>> most platforms but might map to the Windows stat() on cygwin when so
>>> configured.
>>>
>>> By default the true stat() would always be used.  It should have to be a
>>> conscious decision, taken only in specific, vetted scenarios, to use the
>>> unreliable stat.
>> You have just described my second patch! :D
> Unfortunately, I have not had any time to work on the patch this weekend.
> However, despite the patch being a bit rough around the edges, I decided
> to send it out (see below) to get some early feedback.
>
> Note that it passes the t3210, t3211, t5500, t3200, t3301, t7606 and t1301
> tests, but I have not run the full test suite.
>
> Comments welcome.
>
> ATB,
> Ramsay Jones
>
> -- >8 --
> Subject: [PATCH] cygwin: Add fast_[l]stat() functions
>
> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
> ---
>  builtin/apply.c        |  6 +++---
>  builtin/commit.c       |  2 +-
>  builtin/ls-files.c     |  2 +-
>  builtin/rm.c           |  2 +-
>  builtin/update-index.c |  2 +-
>  check-racy.c           |  2 +-
>  compat/cygwin.c        | 12 ------------
>  compat/cygwin.h        | 10 +++-------
>  diff-lib.c             |  2 +-
>  diff.c                 |  2 +-
>  entry.c                |  4 ++--
>  git-compat-util.h      | 13 +++++++++++--
>  help.c                 |  5 +----
>  path.c                 |  9 +--------
>  preload-index.c        |  2 +-
>  read-cache.c           |  6 +++---
>  unpack-trees.c         |  8 ++++----
>  17 files changed, 36 insertions(+), 53 deletions(-)
>
> diff --git a/builtin/apply.c b/builtin/apply.c
> index 0e9b631..ca26caa 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -3253,7 +3253,7 @@ static int load_current(struct image *image, struct patch *patch)
>  	if (pos < 0)
>  		return error(_("%s: does not exist in index"), name);
>  	ce = active_cache[pos];
> -	if (lstat(name, &st)) {
> +	if (fast_lstat(name, &st)) {
>  		if (errno != ENOENT)
>  			return error(_("%s: %s"), name, strerror(errno));
>  		if (checkout_target(ce, &st))
> @@ -3396,7 +3396,7 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s
>  	if (previous) {
>  		st_mode = previous->new_mode;
>  	} else if (!cached) {
> -		stat_ret = lstat(old_name, st);
> +		stat_ret = fast_lstat(old_name, st);
>  		if (stat_ret && errno != ENOENT)
>  			return error(_("%s: %s"), old_name, strerror(errno));
>  	}
> @@ -3850,7 +3850,7 @@ static void add_index_file(const char *path, unsigned mode, void *buf, unsigned
>  			die(_("corrupt patch for subproject %s"), path);
>  	} else {
>  		if (!cached) {
> -			if (lstat(path, &st) < 0)
> +			if (fast_lstat(path, &st) < 0)
>  				die_errno(_("unable to stat newly created file '%s'"),
>  					  path);
>  			fill_stat_cache_info(ce, &st);
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 6b693c1..1d208c6 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -231,7 +231,7 @@ static void add_remove_files(struct string_list *list)
>  		if (p->util)
>  			continue;
>  
> -		if (!lstat(p->string, &st)) {
> +		if (!fast_lstat(p->string, &st)) {
>  			if (add_to_cache(p->string, &st, 0))
>  				die(_("updating files failed"));
>  		} else
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index 08d9786..db66a0e 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -251,7 +251,7 @@ static void show_files(struct dir_struct *dir)
>  				continue;
>  			if (ce_skip_worktree(ce))
>  				continue;
> -			err = lstat(ce->name, &st);
> +			err = fast_lstat(ce->name, &st);
>  			if (show_deleted && err)
>  				show_ce_entry(tag_removed, ce);
>  			if (show_modified && ce_modified(ce, &st, 0))
> diff --git a/builtin/rm.c b/builtin/rm.c
> index 06025a2..4b783e7 100644
> --- a/builtin/rm.c
> +++ b/builtin/rm.c
> @@ -143,7 +143,7 @@ static int check_local_mod(unsigned char *head, int index_only)
>  		}
>  		ce = active_cache[pos];
>  
> -		if (lstat(ce->name, &st) < 0) {
> +		if (fast_lstat(ce->name, &st) < 0) {
>  			if (errno != ENOENT && errno != ENOTDIR)
>  				warning("'%s': %s", ce->name, strerror(errno));
>  			/* It already vanished from the working tree */
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index 5c7762e..4790e4c 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -206,7 +206,7 @@ static int process_path(const char *path)
>  	 * First things first: get the stat information, to decide
>  	 * what to do about the pathname!
>  	 */
> -	if (lstat(path, &st) < 0)
> +	if (fast_lstat(path, &st) < 0)
>  		return process_lstat_error(path, errno);
>  
>  	if (S_ISDIR(st.st_mode))
> diff --git a/check-racy.c b/check-racy.c
> index 00d92a1..6124355 100644
> --- a/check-racy.c
> +++ b/check-racy.c
> @@ -11,7 +11,7 @@ int main(int ac, char **av)
>  		struct cache_entry *ce = active_cache[i];
>  		struct stat st;
>  
> -		if (lstat(ce->name, &st)) {
> +		if (fast_lstat(ce->name, &st)) {
>  			error("lstat(%s): %s", ce->name, strerror(errno));
>  			continue;
>  		}
> diff --git a/compat/cygwin.c b/compat/cygwin.c
> index 91ce5d4..f07cbd3 100644
> --- a/compat/cygwin.c
> +++ b/compat/cygwin.c
> @@ -6,18 +6,6 @@
>  #include "../git-compat-util.h"
>  #include "../cache.h" /* to read configuration */
>  
> -/*
> - * Return POSIX permission bits, regardless of core.ignorecygwinfstricks
> - */
> -int cygwin_get_st_mode_bits(const char *path, int *mode)
> -{
> -	struct stat st;
> -	if (lstat(path, &st) < 0)
> -		return -1;
> -	*mode = st.st_mode;
> -	return 0;
> -}
> -
>  static inline void filetime_to_timespec(const FILETIME *ft, struct timespec *ts)
>  {
>  	long long winTime = ((long long)ft->dwHighDateTime << 32) +
> diff --git a/compat/cygwin.h b/compat/cygwin.h
> index c04965a..8299f58 100644
> --- a/compat/cygwin.h
> +++ b/compat/cygwin.h
> @@ -4,11 +4,7 @@
>  typedef int (*stat_fn_t)(const char*, struct stat*);
>  extern stat_fn_t cygwin_stat_fn;
>  extern stat_fn_t cygwin_lstat_fn;
> -int cygwin_get_st_mode_bits(const char *path, int *mode);
>  
> -#define get_st_mode_bits(p,m) cygwin_get_st_mode_bits((p),(m))
> -#ifndef CYGWIN_C
> -/* cygwin.c needs the original lstat() */
> -#define stat(path, buf) (*cygwin_stat_fn)(path, buf)
> -#define lstat(path, buf) (*cygwin_lstat_fn)(path, buf)
> -#endif
> +#define fast_stat(path, buf) (*cygwin_stat_fn)(path, buf)
> +#define fast_lstat(path, buf) (*cygwin_lstat_fn)(path, buf)
> +#define GIT_FAST_STAT
> diff --git a/diff-lib.c b/diff-lib.c
> index b6f4b21..401dab6 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -27,7 +27,7 @@
>   */
>  static int check_removed(const struct cache_entry *ce, struct stat *st)
>  {
> -	if (lstat(ce->name, st) < 0) {
> +	if (fast_lstat(ce->name, st) < 0) {
>  		if (errno != ENOENT && errno != ENOTDIR)
>  			return -1;
>  		return 1;
> diff --git a/diff.c b/diff.c
> index 208094f..212d3ff 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -2642,7 +2642,7 @@ static int reuse_worktree_file(const char *name, const unsigned char *sha1, int
>  	 * If ce matches the file in the work tree, we can reuse it.
>  	 */
>  	if (ce_uptodate(ce) ||
> -	    (!lstat(name, &st) && !ce_match_stat(ce, &st, 0)))
> +	    (!fast_lstat(name, &st) && !ce_match_stat(ce, &st, 0)))
>  		return 1;
>  
>  	return 0;
> diff --git a/entry.c b/entry.c
> index d7c131d..4d2ac73 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -210,7 +210,7 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout
>  finish:
>  	if (state->refresh_cache) {
>  		if (!fstat_done)
> -			lstat(ce->name, &st);
> +			fast_lstat(ce->name, &st);
>  		fill_stat_cache_info(ce, &st);
>  	}
>  	return 0;
> @@ -230,7 +230,7 @@ static int check_path(const char *path, int len, struct stat *st, int skiplen)
>  		errno = ENOENT;
>  		return -1;
>  	}
> -	return lstat(path, st);
> +	return fast_lstat(path, st);
>  }
>  
>  int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *topath)
> diff --git a/git-compat-util.h b/git-compat-util.h
> index ff193f4..7997bdb 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -129,8 +129,6 @@
>  #include <poll.h>
>  #endif
>  
> -extern int get_st_mode_bits(const char *path, int *mode);
> -
>  #if defined(__MINGW32__)
>  /* pull in Windows compatibility stuff */
>  #include "compat/mingw.h"
> @@ -179,6 +177,17 @@ typedef unsigned long uintptr_t;
>  #endif
>  #endif
>  
> +#ifndef GIT_FAST_STAT
> +static inline int fast_stat(const char *path, struct stat *st)
> +{
> +	return stat(path, st);
> +}
> +static inline int fast_lstat(const char *path, struct stat *st)
> +{
> +	return lstat(path, st);
> +}
> +#endif
> +
>  /* used on Mac OS X */
>  #ifdef PRECOMPOSE_UNICODE
>  #include "compat/precompose_utf8.h"
> diff --git a/help.c b/help.c
> index 08c54ef..f068925 100644
> --- a/help.c
> +++ b/help.c
> @@ -107,10 +107,7 @@ static int is_executable(const char *name)
>  	    !S_ISREG(st.st_mode))
>  		return 0;
>  
> -#if defined(GIT_WINDOWS_NATIVE) || defined(__CYGWIN__)
> -#if defined(__CYGWIN__)
> -if ((st.st_mode & S_IXUSR) == 0)
> -#endif
> +#if defined(GIT_WINDOWS_NATIVE)
>  {	/* cannot trust the executable bit, peek into the file instead */
>  	char buf[3] = { 0 };
>  	int n;
> diff --git a/path.c b/path.c
> index 7f3324a..3d244d3 100644
> --- a/path.c
> +++ b/path.c
> @@ -5,13 +5,7 @@
>  #include "strbuf.h"
>  #include "string-list.h"
>  
> -#ifndef get_st_mode_bits
> -/*
> - * The replacement lstat(2) we use on Cygwin is incomplete and
> - * may return wrong permission bits. Most of the time we do not care,
> - * but the callsites of this wrapper do care.
> - */
> -int get_st_mode_bits(const char *path, int *mode)
> +static int get_st_mode_bits(const char *path, int *mode)
>  {
>  	struct stat st;
>  	if (lstat(path, &st) < 0)
> @@ -19,7 +13,6 @@ int get_st_mode_bits(const char *path, int *mode)
>  	*mode = st.st_mode;
>  	return 0;
>  }
> -#endif
>  
>  static char bad_path[] = "/bad-path/";
>  
> diff --git a/preload-index.c b/preload-index.c
> index 49cb08d..1bece91 100644
> --- a/preload-index.c
> +++ b/preload-index.c
> @@ -57,7 +57,7 @@ static void *preload_thread(void *_data)
>  			continue;
>  		if (threaded_has_symlink_leading_path(&cache, ce->name, ce_namelen(ce)))
>  			continue;
> -		if (lstat(ce->name, &st))
> +		if (fast_lstat(ce->name, &st))
>  			continue;
>  		if (ie_match_stat(index, ce, &st, CE_MATCH_RACY_IS_DIRTY))
>  			continue;
> diff --git a/read-cache.c b/read-cache.c
> index d5201f9..ed33d9e 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -689,7 +689,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
>  int add_file_to_index(struct index_state *istate, const char *path, int flags)
>  {
>  	struct stat st;
> -	if (lstat(path, &st))
> +	if (fast_lstat(path, &st))
>  		die_errno("unable to stat '%s'", path);
>  	return add_to_index(istate, path, &st, flags);
>  }
> @@ -1049,7 +1049,7 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,
>  		return ce;
>  	}
>  
> -	if (lstat(ce->name, &st) < 0) {
> +	if (fast_lstat(ce->name, &st) < 0) {
>  		if (err)
>  			*err = errno;
>  		return NULL;
> @@ -1635,7 +1635,7 @@ static void ce_smudge_racily_clean_entry(struct cache_entry *ce)
>  	 */
>  	struct stat st;
>  
> -	if (lstat(ce->name, &st) < 0)
> +	if (fast_lstat(ce->name, &st) < 0)
>  		return;
>  	if (ce_match_stat_basic(ce, &st))
>  		return;
> diff --git a/unpack-trees.c b/unpack-trees.c
> index b27f2a6..1fe9b63 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -1215,7 +1215,7 @@ static int verify_uptodate_1(const struct cache_entry *ce,
>  	else if (o->reset || ce_uptodate(ce))
>  		return 0;
>  
> -	if (!lstat(ce->name, &st)) {
> +	if (!fast_lstat(ce->name, &st)) {
>  		int flags = CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE;
>  		unsigned changed = ie_match_stat(o->src_index, ce, &st, flags);
>  		if (!changed)
> @@ -1432,13 +1432,13 @@ static int verify_absent_1(const struct cache_entry *ce,
>  		char path[PATH_MAX + 1];
>  		memcpy(path, ce->name, len);
>  		path[len] = 0;
> -		if (lstat(path, &st))
> +		if (fast_lstat(path, &st))
>  			return error("cannot stat '%s': %s", path,
>  					strerror(errno));
>  
>  		return check_ok_to_remove(path, len, DT_UNKNOWN, NULL, &st,
>  				error_type, o);
> -	} else if (lstat(ce->name, &st)) {
> +	} else if (fast_lstat(ce->name, &st)) {
>  		if (errno != ENOENT)
>  			return error("cannot stat '%s': %s", ce->name,
>  				     strerror(errno));
> @@ -1852,7 +1852,7 @@ int oneway_merge(const struct cache_entry * const *src,
>  		int update = 0;
>  		if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) {
>  			struct stat st;
> -			if (lstat(old->name, &st) ||
> +			if (fast_lstat(old->name, &st) ||
>  			    ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))
>  				update |= CE_UPDATE;
>  		}
I managed to apply your patch on next, and run the test suite before and after
your patch.
The performance running "git status" on a linux kernel is the same as in v1.8.3.

Running test suite did not show new failures.
The following test cases had failed, and are fixed now:
< t1400-update-ref
< t3210-pack-refs
< t3211-peel-ref
< t3306-notes-prune
< t5304-prune
< t5404-tracking-branches
< t5500-fetch-pack
< t5505-remote
< t5514-fetch-multiple
< t5515-fetch-merge-logic
< t6030-bisect-porcelain
< t9300-fast-import
< t9903-bash-prompt

In short: Thanks, This looks good to me.

Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions

[Ramsay Jones]

Torsten Bögershausen wrote:
> On 30.06.13 19:28, Ramsay Jones wrote:

[ ... ]

>>> You have just described my second patch! :D
>> Unfortunately, I have not had any time to work on the patch this weekend.
>> However, despite the patch being a bit rough around the edges, I decided
>> to send it out (see below) to get some early feedback.
>>
>> Note that it passes the t3210, t3211, t5500, t3200, t3301, t7606 and t1301
>> tests, but I have not run the full test suite.
>>
>> Comments welcome.
>>
>> ATB,
>> Ramsay Jones
>>
>> -- >8 --
>> Subject: [PATCH] cygwin: Add fast_[l]stat() functions
>>
>> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
>> ---
>>  builtin/apply.c        |  6 +++---
>>  builtin/commit.c       |  2 +-
>>  builtin/ls-files.c     |  2 +-
>>  builtin/rm.c           |  2 +-
>>  builtin/update-index.c |  2 +-
>>  check-racy.c           |  2 +-
>>  compat/cygwin.c        | 12 ------------
>>  compat/cygwin.h        | 10 +++-------
>>  diff-lib.c             |  2 +-
>>  diff.c                 |  2 +-
>>  entry.c                |  4 ++--
>>  git-compat-util.h      | 13 +++++++++++--
>>  help.c                 |  5 +----
>>  path.c                 |  9 +--------
>>  preload-index.c        |  2 +-
>>  read-cache.c           |  6 +++---
>>  unpack-trees.c         |  8 ++++----
>>  17 files changed, 36 insertions(+), 53 deletions(-)
>>

[ ... ]

> I managed to apply your patch on next, and run the test suite before and after
> your patch.
> The performance running "git status" on a linux kernel is the same as in v1.8.3.
> 
> Running test suite did not show new failures.
> The following test cases had failed, and are fixed now:
> < t1400-update-ref
> < t3210-pack-refs
> < t3211-peel-ref
> < t3306-notes-prune
> < t5304-prune
> < t5404-tracking-branches
> < t5500-fetch-pack
> < t5505-remote
> < t5514-fetch-multiple
> < t5515-fetch-merge-logic
> < t6030-bisect-porcelain
> < t9300-fast-import
> < t9903-bash-prompt
> 
> In short: Thanks, This looks good to me.

Thank you for testing this! It is much appreciated.

I sent a v2 patch to the list last night. It passes the full
test suite for me on cygwin 1.5, although there appears to be
a slight performance problem on MinGW (perhaps!). :(

ATB,
Ramsay Jones

Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions

[Ramsay Jones]

Jeff King wrote:
> On Wed, Jun 26, 2013 at 10:45:48PM +0100, Ramsay Jones wrote:
[ ... ]
> I think Michael's assessment above is missing one thing. It is true that
> a false positive is just a performance problem in most cases, as we
> unnecessarily reload the file, thinking it has changed.
> 
> However, when we have taken a lock on the file, there is an additional
> safety measure: if we find the file is changed, we abort, as that should
> never happen (it means somebody else modified the file while we had it
> locked). But of course Cygwin's false positive here triggers the safety
> valve, and we die without even realizing that nothing changed.

Indeed, this is exactly the situation.

ATB,
Ramsay Jones