* Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions [not found] ` <7va9mf6txq.fsf@alter.siamese.dyndns.org> @ 2013-06-25 19:23 ` Johannes Sixt 2013-06-25 20:23 ` Torsten Bögershausen 2013-06-25 21:18 ` Junio C Hamano 0 siblings, 2 replies; 24+ messages in thread From: Johannes Sixt @ 2013-06-25 19:23 UTC (permalink / raw) To: Junio C Hamano Cc: dpotapov, Mark Levedahl, Ramsay Jones, mhagger, Jeff King, Shawn O. Pearce, tboegi, Git Mailing List 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions 2013-06-25 19:23 ` [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions Johannes Sixt @ 2013-06-25 20:23 ` Torsten Bögershausen 2013-06-25 21:18 ` Junio C Hamano 1 sibling, 0 replies; 24+ messages in thread From: Torsten Bögershausen @ 2013-06-25 20:23 UTC (permalink / raw) To: Johannes Sixt Cc: Junio C Hamano, dpotapov, Mark Levedahl, Ramsay Jones, mhagger, Jeff King, Shawn O. Pearce, tboegi, Git Mailing List 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions 2013-06-25 19:23 ` [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions Johannes Sixt 2013-06-25 20:23 ` Torsten Bögershausen @ 2013-06-25 21:18 ` Junio C Hamano 2013-06-26 14:19 ` Torsten Bögershausen 1 sibling, 1 reply; 24+ messages in thread From: Junio C Hamano @ 2013-06-25 21:18 UTC (permalink / raw) To: Johannes Sixt Cc: dpotapov, Mark Levedahl, Ramsay Jones, mhagger, Jeff King, Shawn O. Pearce, tboegi, Git Mailing List 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions 2013-06-25 21:18 ` Junio C Hamano @ 2013-06-26 14:19 ` Torsten Bögershausen 2013-06-26 21:54 ` Ramsay Jones 2013-06-27 2:37 ` Mark Levedahl 0 siblings, 2 replies; 24+ messages in thread From: Torsten Bögershausen @ 2013-06-26 14:19 UTC (permalink / raw) To: Junio C Hamano Cc: Johannes Sixt, dpotapov, Mark Levedahl, Ramsay Jones, mhagger, Jeff King, Shawn O. Pearce, tboegi, Git Mailing List 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions 2013-06-26 14:19 ` Torsten Bögershausen @ 2013-06-26 21:54 ` Ramsay Jones 2013-06-27 15:19 ` Torsten Bögershausen 2013-06-27 2:37 ` Mark Levedahl 1 sibling, 1 reply; 24+ messages in thread From: Ramsay Jones @ 2013-06-26 21:54 UTC (permalink / raw) To: Torsten Bögershausen Cc: Junio C Hamano, Johannes Sixt, dpotapov, Mark Levedahl, mhagger, Jeff King, Shawn O. Pearce, Git Mailing List 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions 2013-06-26 21:54 ` Ramsay Jones @ 2013-06-27 15:19 ` Torsten Bögershausen 2013-06-27 23:18 ` Ramsay Jones 0 siblings, 1 reply; 24+ messages in thread From: Torsten Bögershausen @ 2013-06-27 15:19 UTC (permalink / raw) To: Ramsay Jones Cc: Torsten Bögershausen, Junio C Hamano, Johannes Sixt, dpotapov, Mark Levedahl, mhagger, Jeff King, Shawn O. Pearce, Git Mailing List 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions 2013-06-27 15:19 ` Torsten Bögershausen @ 2013-06-27 23:18 ` Ramsay Jones 0 siblings, 0 replies; 24+ messages in thread From: Ramsay Jones @ 2013-06-27 23:18 UTC (permalink / raw) To: Torsten Bögershausen Cc: Junio C Hamano, Johannes Sixt, dpotapov, Mark Levedahl, mhagger, Jeff King, Shawn O. Pearce, Git Mailing List 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions 2013-06-26 14:19 ` Torsten Bögershausen 2013-06-26 21:54 ` Ramsay Jones @ 2013-06-27 2:37 ` Mark Levedahl 1 sibling, 0 replies; 24+ messages in thread From: Mark Levedahl @ 2013-06-27 2:37 UTC (permalink / raw) To: Torsten Bögershausen Cc: Junio C Hamano, Johannes Sixt, dpotapov, Ramsay Jones, mhagger, Jeff King, Shawn O. Pearce, Git Mailing List 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
[parent not found: <51C6BC4B.9030905@web.de>]
[parent not found: <51C8BF2C.2050203@ramsay1.demon.co.uk>]
[parent not found: <7vy59y4w3r.fsf@alter.siamese.dyndns.org>]
* Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions [not found] ` <7vy59y4w3r.fsf@alter.siamese.dyndns.org> @ 2013-06-26 21:39 ` Ramsay Jones [not found] ` <51C94425.7050006@alum.mit.edu> 1 sibling, 0 replies; 24+ messages in thread From: Ramsay Jones @ 2013-06-26 21:39 UTC (permalink / raw) To: Junio C Hamano Cc: Torsten Bögershausen, mhagger, Jeff King, Johannes Sixt, Shawn O. Pearce, mlevedahl, dpotapov, GIT Mailing-list 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
[parent not found: <51C94425.7050006@alum.mit.edu>]
* Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions [not found] ` <51C94425.7050006@alum.mit.edu> @ 2013-06-26 21:45 ` Ramsay Jones 2013-06-26 22:35 ` Jeff King 0 siblings, 1 reply; 24+ messages in thread From: Ramsay Jones @ 2013-06-26 21:45 UTC (permalink / raw) To: Michael Haggerty Cc: Junio C Hamano, Torsten Bögershausen, Jeff King, Johannes Sixt, Shawn O. Pearce, mlevedahl, dpotapov, GIT Mailing-list 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions 2013-06-26 21:45 ` Ramsay Jones @ 2013-06-26 22:35 ` Jeff King 2013-06-26 22:43 ` Jeff King ` (2 more replies) 0 siblings, 3 replies; 24+ messages in thread From: Jeff King @ 2013-06-26 22:35 UTC (permalink / raw) To: Ramsay Jones Cc: Michael Haggerty, Junio C Hamano, Torsten Bögershausen, Johannes Sixt, Shawn O. Pearce, mlevedahl, dpotapov, GIT Mailing-list 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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions 2013-06-26 22:35 ` Jeff King @ 2013-06-26 22:43 ` Jeff King 2013-06-27 22:58 ` Ramsay Jones 2013-06-27 5:51 ` Michael Haggerty 2013-06-27 22:17 ` Ramsay Jones 2 siblings, 1 reply; 24+ messages in thread From: Jeff King @ 2013-06-26 22:43 UTC (permalink / raw) To: Ramsay Jones Cc: Michael Haggerty, Junio C Hamano, Torsten Bögershausen, Johannes Sixt, Shawn O. Pearce, mlevedahl, dpotapov, GIT Mailing-list 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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions 2013-06-26 22:43 ` Jeff King @ 2013-06-27 22:58 ` Ramsay Jones 2013-06-28 2:31 ` Mark Levedahl 0 siblings, 1 reply; 24+ messages in thread From: Ramsay Jones @ 2013-06-27 22:58 UTC (permalink / raw) To: Jeff King Cc: Michael Haggerty, Junio C Hamano, Torsten Bögershausen, Johannes Sixt, Shawn O. Pearce, mlevedahl, dpotapov, GIT Mailing-list 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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions 2013-06-27 22:58 ` Ramsay Jones @ 2013-06-28 2:31 ` Mark Levedahl 0 siblings, 0 replies; 24+ messages in thread From: Mark Levedahl @ 2013-06-28 2:31 UTC (permalink / raw) To: Ramsay Jones Cc: Jeff King, Michael Haggerty, Junio C Hamano, Torsten Bögershausen, Johannes Sixt, Shawn O. Pearce, dpotapov, GIT Mailing-list 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions 2013-06-26 22:35 ` Jeff King 2013-06-26 22:43 ` Jeff King @ 2013-06-27 5:51 ` Michael Haggerty 2013-06-27 19:58 ` Jeff King 2013-06-27 23:09 ` Ramsay Jones 2013-06-27 22:17 ` Ramsay Jones 2 siblings, 2 replies; 24+ messages in thread From: Michael Haggerty @ 2013-06-27 5:51 UTC (permalink / raw) To: Jeff King Cc: Ramsay Jones, Junio C Hamano, Torsten Bögershausen, Johannes Sixt, Shawn O. Pearce, mlevedahl, dpotapov, GIT Mailing-list 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/ ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions 2013-06-27 5:51 ` Michael Haggerty @ 2013-06-27 19:58 ` Jeff King 2013-06-27 21:04 ` Junio C Hamano 2013-06-27 23:09 ` Ramsay Jones 1 sibling, 1 reply; 24+ messages in thread From: Jeff King @ 2013-06-27 19:58 UTC (permalink / raw) To: Michael Haggerty Cc: Ramsay Jones, Junio C Hamano, Torsten Bögershausen, Johannes Sixt, Shawn O. Pearce, mlevedahl, dpotapov, GIT Mailing-list 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions 2013-06-27 19:58 ` Jeff King @ 2013-06-27 21:04 ` Junio C Hamano 0 siblings, 0 replies; 24+ messages in thread From: Junio C Hamano @ 2013-06-27 21:04 UTC (permalink / raw) To: Jeff King Cc: Michael Haggerty, Ramsay Jones, Torsten Bögershausen, Johannes Sixt, Shawn O. Pearce, mlevedahl, dpotapov, GIT Mailing-list 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions 2013-06-27 5:51 ` Michael Haggerty 2013-06-27 19:58 ` Jeff King @ 2013-06-27 23:09 ` Ramsay Jones 2013-06-30 17:28 ` Ramsay Jones 1 sibling, 1 reply; 24+ messages in thread From: Ramsay Jones @ 2013-06-27 23:09 UTC (permalink / raw) To: Michael Haggerty Cc: Jeff King, Junio C Hamano, Torsten Bögershausen, Johannes Sixt, Shawn O. Pearce, mlevedahl, dpotapov, GIT Mailing-list 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions 2013-06-27 23:09 ` Ramsay Jones @ 2013-06-30 17:28 ` Ramsay Jones 2013-06-30 19:50 ` Junio C Hamano 2013-07-09 11:02 ` Torsten Bögershausen 0 siblings, 2 replies; 24+ messages in thread From: Ramsay Jones @ 2013-06-30 17:28 UTC (permalink / raw) To: Michael Haggerty Cc: Jeff King, Junio C Hamano, Torsten Bögershausen, Johannes Sixt, Shawn O. Pearce, mlevedahl, dpotapov, GIT Mailing-list 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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions 2013-06-30 17:28 ` Ramsay Jones @ 2013-06-30 19:50 ` Junio C Hamano 2013-07-04 18:18 ` Ramsay Jones 2013-07-09 11:02 ` Torsten Bögershausen 1 sibling, 1 reply; 24+ messages in thread From: Junio C Hamano @ 2013-06-30 19:50 UTC (permalink / raw) To: Ramsay Jones Cc: Michael Haggerty, Jeff King, Torsten Bögershausen, Johannes Sixt, Shawn O. Pearce, mlevedahl, dpotapov, GIT Mailing-list 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions 2013-06-30 19:50 ` Junio C Hamano @ 2013-07-04 18:18 ` Ramsay Jones 0 siblings, 0 replies; 24+ messages in thread From: Ramsay Jones @ 2013-07-04 18:18 UTC (permalink / raw) To: Junio C Hamano Cc: Michael Haggerty, Jeff King, Torsten Bögershausen, Johannes Sixt, Shawn O. Pearce, mlevedahl, dpotapov, GIT Mailing-list 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions 2013-06-30 17:28 ` Ramsay Jones 2013-06-30 19:50 ` Junio C Hamano @ 2013-07-09 11:02 ` Torsten Bögershausen 2013-07-11 17:31 ` Ramsay Jones 1 sibling, 1 reply; 24+ messages in thread From: Torsten Bögershausen @ 2013-07-09 11:02 UTC (permalink / raw) To: Ramsay Jones Cc: Michael Haggerty, Jeff King, Junio C Hamano, Torsten Bögershausen, Johannes Sixt, Shawn O. Pearce, mlevedahl, dpotapov, GIT Mailing-list 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions 2013-07-09 11:02 ` Torsten Bögershausen @ 2013-07-11 17:31 ` Ramsay Jones 0 siblings, 0 replies; 24+ messages in thread From: Ramsay Jones @ 2013-07-11 17:31 UTC (permalink / raw) To: Torsten Bögershausen Cc: Michael Haggerty, Jeff King, Junio C Hamano, Johannes Sixt, Shawn O. Pearce, mlevedahl, dpotapov, GIT Mailing-list 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions 2013-06-26 22:35 ` Jeff King 2013-06-26 22:43 ` Jeff King 2013-06-27 5:51 ` Michael Haggerty @ 2013-06-27 22:17 ` Ramsay Jones 2 siblings, 0 replies; 24+ messages in thread From: Ramsay Jones @ 2013-06-27 22:17 UTC (permalink / raw) To: Jeff King Cc: Michael Haggerty, Junio C Hamano, Torsten Bögershausen, Johannes Sixt, Shawn O. Pearce, mlevedahl, dpotapov, GIT Mailing-list 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2013-07-11 17:40 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <51C5FD28.1070004@ramsay1.demon.co.uk> [not found] ` <51C7A875.6020205@gmail.com> [not found] ` <7va9mf6txq.fsf@alter.siamese.dyndns.org> 2013-06-25 19:23 ` [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions Johannes Sixt 2013-06-25 20:23 ` Torsten Bögershausen 2013-06-25 21:18 ` Junio C Hamano 2013-06-26 14:19 ` Torsten Bögershausen 2013-06-26 21:54 ` Ramsay Jones 2013-06-27 15:19 ` Torsten Bögershausen 2013-06-27 23:18 ` Ramsay Jones 2013-06-27 2:37 ` Mark Levedahl [not found] ` <51C6BC4B.9030905@web.de> [not found] ` <51C8BF2C.2050203@ramsay1.demon.co.uk> [not found] ` <7vy59y4w3r.fsf@alter.siamese.dyndns.org> 2013-06-26 21:39 ` Ramsay Jones [not found] ` <51C94425.7050006@alum.mit.edu> 2013-06-26 21:45 ` Ramsay Jones 2013-06-26 22:35 ` Jeff King 2013-06-26 22:43 ` Jeff King 2013-06-27 22:58 ` Ramsay Jones 2013-06-28 2:31 ` Mark Levedahl 2013-06-27 5:51 ` Michael Haggerty 2013-06-27 19:58 ` Jeff King 2013-06-27 21:04 ` Junio C Hamano 2013-06-27 23:09 ` Ramsay Jones 2013-06-30 17:28 ` Ramsay Jones 2013-06-30 19:50 ` Junio C Hamano 2013-07-04 18:18 ` Ramsay Jones 2013-07-09 11:02 ` Torsten Bögershausen 2013-07-11 17:31 ` Ramsay Jones 2013-06-27 22:17 ` Ramsay Jones
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).