{"thread":{"id":"34276","subject":"Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions","startedAt":"2013-06-25T19:23:18Z","lastAt":"2013-07-11T17:31:54Z","messageCount":24,"participants":["Johannes Sixt","Torsten Bögershausen","Junio C Hamano","Ramsay Jones","Jeff King","Mark Levedahl","Michael Haggerty"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"221962","messageId":"51C9EE26.9010006@kdbg.org","threadId":"34276","inReplyTo":"7va9mf6txq.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2013-06-25T19:23:18Z","receivedAt":"2013-06-25T19:23:18Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 25.06.2013 00:10, schrieb Junio C Hamano:\n> Mark Levedahl writes:\n> \n>> On 06/22/2013 03:38 PM, Ramsay Jones wrote:\n>>> Also, apart from running the git test-suite, I have the Win32\n>>> l/stat functions disabled on all of my repos. In particular, I have\n>>> core.filemode set to true. (At one point, I used to build git with\n>>> NO_TRUSTABLE_FILEMODE reset so that I wouldn't have to remember to\n>>> reset core.filemode by hand after a git-clone or git-init). I should\n>>> also note that I run MinGW git on the same laptop and, using git.git\n>>> as an example, it does not seem that much faster (if at all) than\n>>> cygwin git.\n>>\n>> After applying your patch to master, I've had the test-suite running\n>> in a VM using WinXP + current cygwin (v1.7.x) for about 8 hours, no\n>> failures so far but it could take another day to complete.\n>>\n>> I never found any real speed up using the Win32 stat/lstat functions,\n>> and the lack of Posix compatibility breaking cross-platform projects,\n>> links, etc., made this function a mis-feature in my opinion for\n>> years. As I found no positive benefit from the Win32 lstat, I modified\n>> git for local use years ago to set core.filemode=true when cloning /\n>> initing to avoid as many issues as possible.\n> \n> So that's two votes to use the vanilla Cygwin stat/lstat,\n> essentially reverting adbc0b6b (cygwin: Use native Win32 API for\n> stat, 2008-09-30), which was added by Dmitry and Shawn while I was\n> away.\n> \n> Let's wait and see if people give us more data points and decide.\n\nThat'll be more productive if we let the list know ;-)\n\nSome context: This is about a patch by Ramsay that removes the\n\"schizophrenic lstat\" hack for Cygwin. Junio, can you please queue that\npatch in pu?\n\n-- Hannes\n"},{"id":"221966","messageId":"51C9FC53.3090509@web.de","threadId":"34276","inReplyTo":"51C9EE26.9010006@kdbg.org","subject":"Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-06-25T20:23:47Z","receivedAt":"2013-06-25T20:23:47Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 25.06.13 21:23, Johannes Sixt wrote:\n> Am 25.06.2013 00:10, schrieb Junio C Hamano:\n>> Mark Levedahl writes:\n>>\n>>> On 06/22/2013 03:38 PM, Ramsay Jones wrote:\n>>>> Also, apart from running the git test-suite, I have the Win32\n>>>> l/stat functions disabled on all of my repos. In particular, I have\n>>>> core.filemode set to true. (At one point, I used to build git with\n>>>> NO_TRUSTABLE_FILEMODE reset so that I wouldn't have to remember to\n>>>> reset core.filemode by hand after a git-clone or git-init). I should\n>>>> also note that I run MinGW git on the same laptop and, using git.git\n>>>> as an example, it does not seem that much faster (if at all) than\n>>>> cygwin git.\n>>>\n>>> After applying your patch to master, I've had the test-suite running\n>>> in a VM using WinXP + current cygwin (v1.7.x) for about 8 hours, no\n>>> failures so far but it could take another day to complete.\n>>>\n>>> I never found any real speed up using the Win32 stat/lstat functions,\n>>> and the lack of Posix compatibility breaking cross-platform projects,\n>>> links, etc., made this function a mis-feature in my opinion for\n>>> years. As I found no positive benefit from the Win32 lstat, I modified\n>>> git for local use years ago to set core.filemode=true when cloning /\n>>> initing to avoid as many issues as possible.\n>>\n>> So that's two votes to use the vanilla Cygwin stat/lstat,\n>> essentially reverting adbc0b6b (cygwin: Use native Win32 API for\n>> stat, 2008-09-30), which was added by Dmitry and Shawn while I was\n>> away.\n>>\n>> Let's wait and see if people give us more data points and decide.\n> \n> That'll be more productive if we let the list know ;-)\n> \n> Some context: This is about a patch by Ramsay that removes the\n> \"schizophrenic lstat\" hack for Cygwin. Junio, can you please queue that\n> patch in pu?\n> \n> -- Hannes\n>\n\nHere some benchmarks:\nClone the linux kernel from August 2012 (which was \"in house\") to the windows box,\n2.3 GhZ Core duo.\nRun with and without core.filemode, which triggers cygwinfstricks (and is shorter to type)\nNumbers are typical \"hot cache\", cold cache is 6..8 seconds \"real\"\n\n\n$ git --version\ngit version 1.8.3.1.g6f17ca7\n__________________________________\n\n$ time git -c core.filemode=true status -uno\n# On branch master\n# Changes not staged for commit:\n# (use \"git add ...\" to update what will be committed)\n# (use \"git checkout -- ...\" to discard changes in working directory)\n#\n# modified: include/linux/netfilter/xt_CONNMARK.h\n# modified: include/linux/netfilter/xt_DSCP.h\n# modified: include/linux/netfilter/xt_MARK.h\n# modified: include/linux/netfilter/xt_RATEEST.h\n# modified: include/linux/netfilter/xt_TCPMSS.h\n# modified: include/linux/netfilter_ipv4/ipt_ECN.h\n# modified: include/linux/netfilter_ipv4/ipt_TTL.h\n# modified: include/linux/netfilter_ipv6/ip6t_HL.h\n# modified: net/netfilter/xt_DSCP.c\n# modified: net/netfilter/xt_HL.c\n# modified: net/netfilter/xt_RATEEST.c\n# modified: net/netfilter/xt_TCPMSS.c\n#\nno changes added to commit (use \"git add\" and/or \"git commit -a\")\n\nreal 0m2.313s\nuser 0m0.577s\nsys 0m1.765s\n------------------------------------------------------\ntb@snoopy ~/projects/linux-2.6\n$ time git -c core.filemode=false status -uno\n# On branch master\n# Changes not staged for commit:\n# (use \"git add ...\" to update what will be committed)\n# (use \"git checkout -- ...\" to discard changes in working directory)\n#\n# modified: include/linux/netfilter/xt_CONNMARK.h\n# modified: include/linux/netfilter/xt_DSCP.h\n# modified: include/linux/netfilter/xt_MARK.h\n# modified: include/linux/netfilter/xt_RATEEST.h\n# modified: include/linux/netfilter/xt_TCPMSS.h\n# modified: include/linux/netfilter_ipv4/ipt_ECN.h\n# modified: include/linux/netfilter_ipv4/ipt_TTL.h\n# modified: include/linux/netfilter_ipv6/ip6t_HL.h\n# modified: net/netfilter/xt_DSCP.c\n# modified: net/netfilter/xt_HL.c\n# modified: net/netfilter/xt_RATEEST.c\n# modified: net/netfilter/xt_TCPMSS.c\n#\nno changes added to commit (use \"git add\" and/or \"git commit -a\")\n\nreal 0m1.047s\nuser 0m0.311s\nsys 0m0.765s\n\n/Torsten\n"},{"id":"221971","messageId":"7vppv928jx.fsf@alter.siamese.dyndns.org","threadId":"34276","inReplyTo":"51C9EE26.9010006@kdbg.org","subject":"Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-25T21:18:42Z","receivedAt":"2013-06-25T21:18:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Some context: This is about a patch by Ramsay that removes the\n> \"schizophrenic lstat\" hack for Cygwin. Junio, can you please queue that\n> patch in pu?\n\nSure. Thanks.\n"},{"id":"222018","messageId":"51CAF863.4020803@web.de","threadId":"34276","inReplyTo":"7vppv928jx.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-06-26T14:19:15Z","receivedAt":"2013-06-26T14:19:15Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2013-06-25 23.18, Junio C Hamano wrote:\n> Johannes Sixt writes:\n> \n>> Some context: This is about a patch by Ramsay that removes the\n>> \"schizophrenic lstat\" hack for Cygwin. Junio, can you please queue that\n>> patch in pu?\n> \n> Sure. Thanks.\n\nFirst of all,\nthanks for the work.\n\nHere some \"benchmark\" results, \n(The test run of the test suite did the same amout of time).\n\nBut:\ngit status -uno in real life takes double the time,\ngit 1.8.3 compared against \"pu with the vanilla l/stat\"\n \n 1 second -> 2 seconds on linux kernel\n0.2 seconds -> 0.4 seconds on git.git \n\nDo we have any known problems with the current implementation ?\nDoes speed matter ?\n\nOne vote to keep the special cygwin functions.\n(And have a look how to improve the core.filemode)\n\n/Torsten\n"},{"id":"222041","messageId":"51CB5F9D.3060202@ramsay1.demon.co.uk","threadId":"34276","inReplyTo":"7vy59y4w3r.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2013-06-26T21:39:41Z","receivedAt":"2013-06-26T21:39:41Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Junio C Hamano wrote:\n> Ramsay Jones writes:\n> \n>> Michael Haggerty and Jeff King have been re-vamping the reference\n>> handling code. The failures noted above were provoked by patches\n>> in the 'mh/ref-races' branch. At the time I wrote this patch, that\n>> branch was only included in 'pu', but I notice that this topic has\n>> now progressed to 'next' (see commit 71f1a182).\n> \n> I had an impression that up to 98eeb09e (for_each_ref: load all\n> loose refs before packed refs, 2013-06-20) that is now in 'next'\n> does not agressively use the lstat timestamp of the packed-refs\n> file, and the \"optional\" bit 5d478f5c (refs: do not invalidate the\n> packed-refs cache unnecessarily, 2013-06-20), and the one in 'next'\n> should be safe with the cheating-lstat. Isn't it the case?\n\nThe next branch (from a couple of days ago, namely commit 4f488db) is\ncurrently broken on cygwin, like so:\n\n $ cd t\n $ ./t3211-peel-ref.sh -i -v\n \n [ ... ]\n \n ok 7 - refs are peeled outside of refs/tags (old packed)\n \n expecting success:\n git pack-refs --all &&\n cp .git/packed-refs fully-peeled &&\n git branch yadda &&\n git pack-refs --all &&\n git branch -d yadda &&\n test_cmp fully-peeled .git/packed-refs\n \n fatal: internal error: packed-ref cache cleared while locked\n not ok 8 - peeled refs survive deletion of packed ref\n #\n # git pack-refs --all &&\n # cp .git/packed-refs fully-peeled &&\n # git branch yadda &&\n # git pack-refs --all &&\n # git branch -d yadda &&\n # test_cmp fully-peeled .git/packed-refs\n #\n $ cd trash\\ directory.t3211-peel-ref/\n $ ls\n actual base.t expect\n $ ../../bin-wrappers/git pack-refs --all\n fatal: internal error: packed-ref cache cleared while locked\n $ gdb ../../git.exe\n GNU gdb 6.5.50.20060706-cvs (cygwin-special)\n Copyright (C) 2006 Free Software Foundation, Inc.\n GDB is free software, covered by the GNU General Public License, and you are\n welcome to change it and/or distribute copies of it under certain conditions.\n Type \"show copying\" to see the conditions.\n There is absolutely no warranty for GDB. Type \"show warranty\" for details.\n This GDB was configured as \"i686-pc-cygwin\"...\n (gdb) b stat_validity_check\n Breakpoint 1 at 0x48a33f: file read-cache.c, line 1965.\n (gdb) run pack-refs --all\n Starting program: /home/ramsay/git/git.exe pack-refs --all\n Loaded symbols for /cygdrive/c/WINDOWS/system32/ntdll.dll\n Loaded symbols for /cygdrive/c/WINDOWS/system32/kernel32.dll\n Loaded symbols for /usr/bin/cygcrypto-0.9.8.dll\n Loaded symbols for /usr/bin/cygwin1.dll\n Loaded symbols for /cygdrive/c/WINDOWS/system32/advapi32.dll\n Loaded symbols for /cygdrive/c/WINDOWS/system32/rpcrt4.dll\n Loaded symbols for /cygdrive/c/WINDOWS/system32/secur32.dll\n Loaded symbols for /usr/bin/cygiconv-2.dll\n Loaded symbols for /usr/bin/cygz.dll\n \n Breakpoint 1, stat_validity_check (sv=0xa611a4,\n path=0x57d98c \".git/packed-refs\") at read-cache.c:1965\n 1965 if (stat(path, &st) < 0)\n (gdb) n\n 1967 if (!sv->sd)\n (gdb) p sv\n $1 = (struct stat_validity *) 0xa611a4\n (gdb) p *sv\n $2 = {sd = 0xa62478}\n (gdb) p sv->sd\n $3 = (struct stat_data *) 0xa62478\n (gdb) p *sv->sd\n $4 = {sd_ctime = {sec = 1372194879, nsec = 671875000}, sd_mtime = {\n sec = 1372194879, nsec = 656250000}, sd_dev = 2899104371,\n sd_ino = 180184, sd_uid = 1005, sd_gid = 513, sd_size = 296}\n (gdb) p st\n $5 = {st_dev = 0, st_ino = 0, st_mode = 33152, st_nlink = 1, st_uid = 0,\n st_gid = 0, st_rdev = 0, st_size = 296, st_atim = {tv_sec = 1372195048,\n tv_nsec = 890625000}, st_mtim = {tv_sec = 1372194879,\n tv_nsec = 656250000}, st_ctim = {tv_sec = 1372194879,\n tv_nsec = 15625000}, st_blksize = 5636381, st_blocks = 1, st_spare4 = {\n 10883480, 5757324}}\n (gdb) c\n Continuing.\n fatal: internal error: packed-ref cache cleared while locked\n \n Program exited with code 0200.\n (gdb) quit\n $ ../../test-stat .git/packed-refs\n stat for '.git/packed-refs':\n *dev: -1395862925, 0\n *ino: 180184, 0\n *mode: 100644 -rw-, 100600 -rw-\n nlink: 1, 1\n *uid: 1005, 0\n *gid: 513, 0\n *rdev: -1395862925, 0\n size: 296, 296\n atime: 1372195048, 1372195048 Tue Jun 25 22:17:28 2013\n mtime: 1372194879, 1372194879 Tue Jun 25 22:14:39 2013\n ctime: 1372194879, 1372194879 Tue Jun 25 22:14:39 2013\n $\n \nNote that, as noted before, (at least) the following tests fail\ndue to the 'mh/ref-races' branch:\n\n t3210-pack-refs.sh (Wstat: 256 Tests: 16 Failed: 12)\n Failed tests: 4-9, 11-16\n Non-zero exit status: 1\n t3211-peel-ref.sh (Wstat: 256 Tests: 8 Failed: 1)\n Failed test: 8\n Non-zero exit status: 1\n t5500-fetch-pack.sh (Wstat: 256 Tests: 54 Failed: 1)\n Failed test: 43\n Non-zero exit status: 1\n\n> In any case, if removing the cheating-lstat will improve the\n> robustness without hurting performance, I am all for it.\n\nI'm sure it will improve robustness (and help with maintainability),\nbut as for performance ...\n\n> The less platform specific hacks, the better ;-).\n\nAgreed.\n\nATB,\nRamsay Jones\n"},{"id":"222043","messageId":"51CB610C.7050501@ramsay1.demon.co.uk","threadId":"34276","inReplyTo":"51C94425.7050006@alum.mit.edu","subject":"Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2013-06-26T21:45:48Z","receivedAt":"2013-06-26T21:45:48Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Michael Haggerty wrote:\n> On 06/25/2013 07:07 AM, Junio C Hamano wrote:\n>> Ramsay Jones writes:\n>>\n>>> Michael Haggerty and Jeff King have been re-vamping the reference\n>>> handling code. The failures noted above were provoked by patches\n>>> in the 'mh/ref-races' branch. At the time I wrote this patch, that\n>>> branch was only included in 'pu', but I notice that this topic has\n>>> now progressed to 'next' (see commit 71f1a182).\n>>\n>> I had an impression that up to 98eeb09e (for_each_ref: load all\n>> loose refs before packed refs, 2013-06-20) that is now in 'next'\n>> does not agressively use the lstat timestamp of the packed-refs\n>> file, and the \"optional\" bit 5d478f5c (refs: do not invalidate the\n>> packed-refs cache unnecessarily, 2013-06-20), and the one in 'next'\n>> should be safe with the cheating-lstat. Isn't it the case?\n>>\n>> In any case, if removing the cheating-lstat will improve the\n>> robustness without hurting performance, I am all for it.\n>>\n>> The less platform specific hacks, the better ;-).\n> \n> The following patch in the \"non-optional\" commits (which has already\n> been merged to next) uses stat() via the new stat_validity API:\n> \n> ca9199300e get_packed_ref_cache: reload packed-refs file when it changes\n> \n> This patch adds some *extra* cache invalidation that was heretofore\n> missing. If stat() is broken it could\n> \n> (a) cause a false positive, resulting in some unnecessary cache\n> invalidation and re-reading of packed-refs, which will hurt performance\n> but not correctness; or\n> \n> (b) cause a false negative, in which case the stale cache might be used\n> for reading (but not writing), just as was *always* the case before this\n> patch.\n> \n> As far as I understand, the concern for cygwin is (a). I will leave it\n> to others to measure and/or decide whether the performance loss is too\n> grave to endure until the cygwin stat() situation is fixed.\n> \n\nHmm, I'm not sure I understand ... However, I can confirm that the\n'mh/ref-races' branch in next is broken on cygwin. (i.e. it is not\njust a speed issue; it provokes fatal errors).\n\nSee reply to Junio.\n\nATB,\nRamsay Jones\n"},{"id":"222042","messageId":"51CB62FC.4070001@ramsay1.demon.co.uk","threadId":"34276","inReplyTo":"51CAF863.4020803@web.de","subject":"Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2013-06-26T21:54:04Z","receivedAt":"2013-06-26T21:54:04Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Torsten Bögershausen wrote:\n> On 2013-06-25 23.18, Junio C Hamano wrote:\n>> Johannes Sixt writes:\n>>\n>>> Some context: This is about a patch by Ramsay that removes the\n>>> \"schizophrenic lstat\" hack for Cygwin. Junio, can you please queue that\n>>> patch in pu?\n>>\n>> Sure. Thanks.\n> \n> First of all,\n> thanks for the work.\n> \n> Here some \"benchmark\" results, \n> (The test run of the test suite did the same amout of time).\n\nThe test suite runs noticeably faster for me.\n\n> \n> But:\n> git status -uno in real life takes double the time,\n> git 1.8.3 compared against \"pu with the vanilla l/stat\"\n> \n> 1 second -> 2 seconds on linux kernel\n> 0.2 seconds -> 0.4 seconds on git.git \n\nHmm, OK, I guess I will have to try something else. Sigh :(\n\n> Do we have any known problems with the current implementation ?\n\nYes. The next branch is currently broken. (see reply to Junio)\n\n> Does speed matter ?\n> \n> One vote to keep the special cygwin functions.\n> (And have a look how to improve the core.filemode)\n\nI don't understand this (parenthetical) comment; could you\nelaborate on this.\n\nATB,\nRamsay Jones\n"},{"id":"222045","messageId":"20130626223552.GA12785@sigill.intra.peff.net","threadId":"34276","inReplyTo":"51CB610C.7050501@ramsay1.demon.co.uk","subject":"Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-26T22:35:52Z","receivedAt":"2013-06-26T22:35:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 26, 2013 at 10:45:48PM +0100, Ramsay Jones wrote:\n\n> > This patch adds some *extra* cache invalidation that was heretofore\n> > missing. If stat() is broken it could\n> > \n> > (a) cause a false positive, resulting in some unnecessary cache\n> > invalidation and re-reading of packed-refs, which will hurt performance\n> > but not correctness; or\n> > \n> > (b) cause a false negative, in which case the stale cache might be used\n> > for reading (but not writing), just as was *always* the case before this\n> > patch.\n> > \n> > As far as I understand, the concern for cygwin is (a). I will leave it\n> > to others to measure and/or decide whether the performance loss is too\n> > grave to endure until the cygwin stat() situation is fixed.\n> \n> Hmm, I'm not sure I understand ... However, I can confirm that the\n> 'mh/ref-races' branch in next is broken on cygwin. (i.e. it is not\n> just a speed issue; it provokes fatal errors).\n\nI think Michael's assessment above is missing one thing. It is true that\na false positive is just a performance problem in most cases, as we\nunnecessarily reload the file, thinking it has changed.\n\nHowever, when we have taken a lock on the file, there is an additional\nsafety measure: if we find the file is changed, we abort, as that should\nnever happen (it means somebody else modified the file while we had it\nlocked). But of course Cygwin's false positive here triggers the safety\nvalve, and we die without even realizing that nothing changed.\n\nIn theory we can drop the safety valve; it should never actually happen.\nBut I'd like to keep it there for working systems. Perhaps it is worth\ndoing something like this:\n\ndiff --git a/refs.c b/refs.c\nindex 4302206..7cac42d 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1080,6 +1080,16 @@ static struct packed_ref_cache *get_packed_ref_cache(struct ref_cache *refs)\n \t\tpacked_refs_file = git_path(\"packed-refs\");\n \n \tif (refs->packed &&\n+#ifdef STAT_VALIDITY_GIVES_FALSE_POSITIVES\n+\t /*\n+\t * If we get false positives from our stat calls on this platform,\n+\t * then we must assume that once we have locked the packed-refs\n+\t * file it does not change. Otherwise it looks like somebody else\n+\t * has changed it out from under us (while we have it locked!), and\n+\t * we die().\n+\t */\n+\t !refs->packed->lock &&\n+#endif\n \t !stat_validity_check(&refs->packed->validity, packed_refs_file))\n \t\tclear_packed_ref_cache(refs);\n \n\nand then we can add that define to Cygwin in the Makefile. I verified\nthe issue on Linux by \"breaking\" stat_validity_check to always return 0\n(i.e., to always give a false positive that the file has changed, which\nis what Cygwin is doing).\n\nI am curious how often Cygwin gives us the false positive. If it is\nevery time, then the check is not doing much good at all. Is it possible\nfor you to instrument stat_validity_check to report how often it does or\ndoes not do anything useful?\n\n-Peff\n"},{"id":"222046","messageId":"20130626224336.GA22486@sigill.intra.peff.net","threadId":"34276","inReplyTo":"20130626223552.GA12785@sigill.intra.peff.net","subject":"Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-26T22:43:36Z","receivedAt":"2013-06-26T22:43:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 26, 2013 at 06:35:52PM -0400, Jeff King wrote:\n\n> I am curious how often Cygwin gives us the false positive. If it is\n> every time, then the check is not doing much good at all. Is it possible\n> for you to instrument stat_validity_check to report how often it does or\n> does not do anything useful?\n\nMaybe like this:\n\ndiff --git a/read-cache.c b/read-cache.c\nindex d5201f9..19dcb69 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1958,6 +1958,14 @@ void stat_validity_clear(struct stat_validity *sv)\n \tsv->sd = NULL;\n }\n \n+static int unchanged;\n+static int attempts;\n+static void print_stats(void)\n+{\n+\tfprintf(stderr, \"stat data was unchanged %d/%d\\n\",\n+\t\tunchanged, attempts);\n+}\n+\n int stat_validity_check(struct stat_validity *sv, const char *path)\n {\n \tstruct stat st;\n@@ -1966,7 +1974,16 @@ int stat_validity_check(struct stat_validity *sv, const char *path)\n \t\treturn sv->sd == NULL;\n \tif (!sv->sd)\n \t\treturn 0;\n-\treturn S_ISREG(st.st_mode) && !match_stat_data(sv->sd, &st);\n+\tif (!S_ISREG(st.st_mode))\n+\t\treturn 0;\n+\tif (!attempts++)\n+\t\tatexit(print_stats);\n+\tif (!match_stat_data(sv->sd, &st)) {\n+\t\tunchanged++;\n+\t\treturn 1;\n+\t}\n+\telse\n+\t\treturn 0;\n }\n \n void stat_validity_update(struct stat_validity *sv, int fd)\n\nRunning \"t3211 -v\", I see things like:\n\n stat data was unchanged 3/3\n stat data was unchanged 20/20\n stat data was unchanged 2/2\n Deleted branch yadda (was d1ff1c9).\n stat data was unchanged 8/8\n ok 8 - peeled refs survive deletion of packed ref\n\nI am curious if you will see 0/20 or 19/20 there on Cygwin.\n\n-Peff\n"},{"id":"222060","messageId":"51CBA552.80309@gmail.com","threadId":"34276","inReplyTo":"51CAF863.4020803@web.de","subject":"Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions","fromName":"Mark Levedahl","fromEmail":"mlevedahl@gmail.com","sentAt":"2013-06-27T02:37:06Z","receivedAt":"2013-06-27T02:37:06Z","isPatch":true,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"On 06/26/2013 10:19 AM, Torsten Bögershausen wrote:\n> On 2013-06-25 23.18, Junio C Hamano wrote:\n>> Johannes Sixt writes:\n>>\n>>> Some context: This is about a patch by Ramsay that removes the\n>>> \"schizophrenic lstat\" hack for Cygwin. Junio, can you please queue that\n>>> patch in pu?\n>> Sure. Thanks.\n> First of all,\n> thanks for the work.\n>\n> Here some \"benchmark\" results,\n> (The test run of the test suite did the same amout of time).\n>\n> But:\n> git status -uno in real life takes double the time,\n> git 1.8.3 compared against \"pu with the vanilla l/stat\"\n> \n> 1 second -> 2 seconds on linux kernel\n> 0.2 seconds -> 0.4 seconds on git.git\n>\n> Do we have any known problems with the current implementation ?\n> Does speed matter ?\n>\n> One vote to keep the special cygwin functions.\n> (And have a look how to improve the core.filemode)\n>\n> /Torsten\n>\n>\nThere have been threads on the cygwin mailing lists for at least a \ndecade looking to speed up cygwin's posix stat / lstat (and fork). If \nimprovement were merely difficult, it would have been done long ago. As \ngit cares about things like execute bits, file / repository permissions, \nand soft links, whatever stat / lstat git uses needs to fully support \nthose under cygwin, either by using what cygwin provides or providing a \ncomplete replacement. Note my other reply - with Ramsay's patch I can \ncomplete the test suite (except for t0008.sh that has a known hang) \nwhile without it I find the test suite randomly (unrepeatable) hangs in \nmany tests. So, this stat/lstat replacement is at least implicated in \ncurrent troubles.\n\nMark\n"},{"id":"222066","messageId":"51CBD2FD.5070905@alum.mit.edu","threadId":"34276","inReplyTo":"20130626223552.GA12785@sigill.intra.peff.net","subject":"Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-06-27T05:51:57Z","receivedAt":"2013-06-27T05:51:57Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 06/27/2013 12:35 AM, Jeff King wrote:\n> On Wed, Jun 26, 2013 at 10:45:48PM +0100, Ramsay Jones wrote:\n> \n>>> This patch adds some *extra* cache invalidation that was heretofore\n>>> missing. If stat() is broken it could\n>>>\n>>> (a) cause a false positive, resulting in some unnecessary cache\n>>> invalidation and re-reading of packed-refs, which will hurt performance\n>>> but not correctness; or\n>>>\n>>> (b) cause a false negative, in which case the stale cache might be used\n>>> for reading (but not writing), just as was *always* the case before this\n>>> patch.\n>>>\n>>> As far as I understand, the concern for cygwin is (a). I will leave it\n>>> to others to measure and/or decide whether the performance loss is too\n>>> grave to endure until the cygwin stat() situation is fixed.\n>>\n>> Hmm, I'm not sure I understand ... However, I can confirm that the\n>> 'mh/ref-races' branch in next is broken on cygwin. (i.e. it is not\n>> just a speed issue; it provokes fatal errors).\n> \n> I think Michael's assessment above is missing one thing.\n\nPeff is absolutely right; for some unknown reason I was thinking of the\nconsistency check as having been already fixed.\n\n> However, when we have taken a lock on the file, there is an additional\n> safety measure: if we find the file is changed, we abort, as that should\n> never happen (it means somebody else modified the file while we had it\n> locked). But of course Cygwin's false positive here triggers the safety\n> valve, and we die without even realizing that nothing changed.\n> \n> In theory we can drop the safety valve; it should never actually happen.\n> But I'd like to keep it there for working systems. Perhaps it is worth\n> doing something like this:\n> \n> [...#ifdef out consistency check on cygwin when lock is held...]\n\nYes, this would work.\n\nBut, taking a step back, I think it is a bad idea to have an unreliable\nstat() masquerading as a real stat(). If we want to allow the use of an\nunreliable stat for certain purposes, let's have two stat() interfaces:\n\n* the true stat() (in this case I guess cygwin's slow-but-correct\nimplementation)\n\n* some fast_but_maybe_unreliable_stat(), which would map to stat() on\nmost platforms but might map to the Windows stat() on cygwin when so\nconfigured.\n\nBy default the true stat() would always be used. It should have to be a\nconscious decision, taken only in specific, vetted scenarios, to use the\nunreliable stat.\n\nFor example, I can't imagine that checking the freshness of the index or\nof the packed-refs file is ever going to be a bottleneck, so there is no\nreason at all to use an unreliable stat() here.\n\nOn the other hand, stat() seems definitely to be a bottleneck when\ntesting for changes in a 100,000 file working tree, and here occasional\nmistakes might be considered acceptable. So for this purpose the\nunreliable stat() might be used.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"222087","messageId":"51CC57F2.8010805@web.de","threadId":"34276","inReplyTo":"51CB62FC.4070001@ramsay1.demon.co.uk","subject":"Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-06-27T15:19:14Z","receivedAt":"2013-06-27T15:19:14Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2013-06-26 23.54, Ramsay Jones wrote:\n> Torsten Bögershausen wrote:\n>> On 2013-06-25 23.18, Junio C Hamano wrote:\n>>> Johannes Sixt writes:\n>>>\n>>>> Some context: This is about a patch by Ramsay that removes the\n>>>> \"schizophrenic lstat\" hack for Cygwin. Junio, can you please queue that\n>>>> patch in pu?\n>>>\n>>> Sure. Thanks.\n>>\n>> First of all,\n>> thanks for the work.\n>>\n>> Here some \"benchmark\" results, \n>> (The test run of the test suite did the same amout of time).\n> \n> The test suite runs noticeably faster for me.\n> \n>>\n>> But:\n>> git status -uno in real life takes double the time,\n>> git 1.8.3 compared against \"pu with the vanilla l/stat\"\n>> \n>> 1 second -> 2 seconds on linux kernel\n>> 0.2 seconds -> 0.4 seconds on git.git \n> \n> Hmm, OK, I guess I will have to try something else. Sigh :(\n> \n>> Do we have any known problems with the current implementation ?\n> \n> Yes. The next branch is currently broken. (see reply to Junio)\n> \n>> Does speed matter ?\n>>\n>> One vote to keep the special cygwin functions.\n>> (And have a look how to improve the core.filemode)\n> \n> I don't understand this (parenthetical) comment; could you\n> elaborate on this.\n> \n> ATB,\n> Ramsay Jones\nThis is probably wrong information:\nI had in mind that cygwin sets core.filemode=false,\nwhich is quite annoying when exchanging .sh files with linux.\nBut that seems to be wrong, a quick test shows that core.filemode=true.\nSorry for confusion.\n"},{"id":"222128","messageId":"20130627195827.GB17601@sigill.intra.peff.net","threadId":"34276","inReplyTo":"51CBD2FD.5070905@alum.mit.edu","subject":"Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-27T19:58:27Z","receivedAt":"2013-06-27T19:58:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 27, 2013 at 07:51:57AM +0200, Michael Haggerty wrote:\n\n> > In theory we can drop the safety valve; it should never actually happen.\n> > But I'd like to keep it there for working systems. Perhaps it is worth\n> > doing something like this:\n> > \n> > [...#ifdef out consistency check on cygwin when lock is held...]\n> \n> Yes, this would work.\n> \n> But, taking a step back, I think it is a bad idea to have an unreliable\n> stat() masquerading as a real stat(). If we want to allow the use of an\n> unreliable stat for certain purposes, let's have two stat() interfaces:\n> \n> * the true stat() (in this case I guess cygwin's slow-but-correct\n> implementation)\n> \n> * some fast_but_maybe_unreliable_stat(), which would map to stat() on\n> most platforms but might map to the Windows stat() on cygwin when so\n> configured.\n\nYeah, that makes sense to me. I don't have a particular opinion on which\nway to go, as I do not use cygwin at all (and on most platforms, the\ntwo stat interfaces would just both call stat()).\n\nI will leave it up to Cygwin folks whether they want to do something\nlike my patch as a band-aid while working up the two-stat() solution.\n\n-Peff\n"},{"id":"222135","messageId":"7vip0zqn8a.fsf@alter.siamese.dyndns.org","threadId":"34276","inReplyTo":"20130627195827.GB17601@sigill.intra.peff.net","subject":"Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-27T21:04:37Z","receivedAt":"2013-06-27T21:04:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> But, taking a step back, I think it is a bad idea to have an unreliable\n>> stat() masquerading as a real stat(). If we want to allow the use of an\n>> unreliable stat for certain purposes, let's have two stat() interfaces:\n>> \n>> * the true stat() (in this case I guess cygwin's slow-but-correct\n>> implementation)\n>> \n>> * some fast_but_maybe_unreliable_stat(), which would map to stat() on\n>> most platforms but might map to the Windows stat() on cygwin when so\n>> configured.\n>\n> Yeah, that makes sense to me. I don't have a particular opinion on which\n> way to go, as I do not use cygwin at all (and on most platforms, the\n> two stat interfaces would just both call stat()).\n>\n> I will leave it up to Cygwin folks whether they want to do something\n> like my patch as a band-aid while working up the two-stat() solution.\n\nI think in the longer term two-stat solution is a good thing. \n\n0117c2f0 (Make core.sharedRepository work under cygwin 1.7,\n2013-03-23) introduced get_st_mode_bits() to work around another\nglitch in the fast-but-cheating-and-unreliable replacement, which\nwe may be able to revert once it is done.\n"},{"id":"222141","messageId":"51CCB9E5.3010907@ramsay1.demon.co.uk","threadId":"34276","inReplyTo":"20130626223552.GA12785@sigill.intra.peff.net","subject":"Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2013-06-27T22:17:09Z","receivedAt":"2013-06-27T22:17:09Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Jeff King wrote:\n> On Wed, Jun 26, 2013 at 10:45:48PM +0100, Ramsay Jones wrote:\n[ ... ]\n> I think Michael's assessment above is missing one thing. It is true that\n> a false positive is just a performance problem in most cases, as we\n> unnecessarily reload the file, thinking it has changed.\n> \n> However, when we have taken a lock on the file, there is an additional\n> safety measure: if we find the file is changed, we abort, as that should\n> never happen (it means somebody else modified the file while we had it\n> locked). But of course Cygwin's false positive here triggers the safety\n> valve, and we die without even realizing that nothing changed.\n\nIndeed, this is exactly the situation.\n\nATB,\nRamsay Jones\n"},{"id":"222142","messageId":"51CCC397.3060705@ramsay1.demon.co.uk","threadId":"34276","inReplyTo":"20130626224336.GA22486@sigill.intra.peff.net","subject":"Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2013-06-27T22:58:31Z","receivedAt":"2013-06-27T22:58:31Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Jeff King wrote:\n> On Wed, Jun 26, 2013 at 06:35:52PM -0400, Jeff King wrote:\n> \n>> I am curious how often Cygwin gives us the false positive. If it is\n>> every time, then the check is not doing much good at all. Is it possible\n>> for you to instrument stat_validity_check to report how often it does or\n>> does not do anything useful?\n> \n> Maybe like this:\n> \n> diff --git a/read-cache.c b/read-cache.c\n> index d5201f9..19dcb69 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -1958,6 +1958,14 @@ void stat_validity_clear(struct stat_validity *sv)\n> \tsv->sd = NULL;\n> }\n> \n> +static int unchanged;\n> +static int attempts;\n> +static void print_stats(void)\n> +{\n> +\tfprintf(stderr, \"stat data was unchanged %d/%d\\n\",\n> +\t\tunchanged, attempts);\n> +}\n> +\n> int stat_validity_check(struct stat_validity *sv, const char *path)\n> {\n> \tstruct stat st;\n> @@ -1966,7 +1974,16 @@ int stat_validity_check(struct stat_validity *sv, const char *path)\n> \t\treturn sv->sd == NULL;\n> \tif (!sv->sd)\n> \t\treturn 0;\n> -\treturn S_ISREG(st.st_mode) && !match_stat_data(sv->sd, &st);\n> +\tif (!S_ISREG(st.st_mode))\n> +\t\treturn 0;\n> +\tif (!attempts++)\n> +\t\tatexit(print_stats);\n> +\tif (!match_stat_data(sv->sd, &st)) {\n> +\t\tunchanged++;\n> +\t\treturn 1;\n> +\t}\n> +\telse\n> +\t\treturn 0;\n> }\n> \n> void stat_validity_update(struct stat_validity *sv, int fd)\n> \n> Running \"t3211 -v\", I see things like:\n> \n> stat data was unchanged 3/3\n> stat data was unchanged 20/20\n> stat data was unchanged 2/2\n> Deleted branch yadda (was d1ff1c9).\n> stat data was unchanged 8/8\n> ok 8 - peeled refs survive deletion of packed ref\n> \n> I am curious if you will see 0/20 or 19/20 there on Cygwin.\n\nAh, no ... the problem is not as subtle as this! :-D\n\nI'm sorry if I failed to mention that I already had a patch\nto solve the problem, but that I *don't* want to submit it,\nsince it is ugly and just serves to spread the madness! ;-)\n\nThis is why I tried the \"cygwin: Remove the Win32 l/stat()\nfunctions\" patch first; I think this is the correct approach\nto fixing this problem (and similar *future* problems).\n\nHowever, since that is no longer an option, on performance\ngrounds, I have other ideas I want to try. (see later email)\n\nThe \"schizophrenic stat\" functions have caused many subtle\nproblems, but this is not one of them; a brick to the head\nwould be more subtle. The problem is caused by the \"stat\nvalidity\" code using both fstat() and stat() on the same\nfile. Note that there is no Win32 implementation of the\nfstat() function.\n\nSo, a solution to the problem would be to provide just such\nan implementation. The patch to do so is given below, just\nfor illustration purposes.\n\nThis fixes the failures to t3210, t3211 and t5500. However, I\nhave not run the full test suite. This may cause some further\n*subtle* problems (just grep for fstat and consider what may\ngo wrong; I have not done that), especially when you consider\nthat cygwin has the UNRELIABLE_FSTAT build variable set.\n\nHTH\n\nATB,\nRamsay Jones\n\n-- >8 --\nSubject: [PATCH] cygwin: Add an Win32 fstat implementation\n\nSigned-off-by: Ramsay Jones \n---\n compat/cygwin.c | 41 +++++++++++++++++++++++++++++++++++++++++\n compat/cygwin.h | 3 +++\n 2 files changed, 44 insertions(+)\n\ndiff --git a/compat/cygwin.c b/compat/cygwin.c\nindex 91ce5d4..d59c9fb 100644\n--- a/compat/cygwin.c\n+++ b/compat/cygwin.c\n@@ -4,6 +4,7 @@\n #include \n #include \"win32.h\"\n #include \"../git-compat-util.h\"\n+#include \n #include \"../cache.h\" /* to read configuration */\n \n /*\n@@ -100,6 +101,38 @@ static int cygwin_stat(const char *path, struct stat *buf)\n \treturn do_stat(path, buf, stat);\n }\n \n+static int cygwin_fstat(int fd, struct stat *buf)\n+{\n+\tHANDLE fh = (HANDLE)get_osfhandle(fd);\n+\tBY_HANDLE_FILE_INFORMATION fdata;\n+\n+\tif (fh == INVALID_HANDLE_VALUE) {\n+\t\terrno = EBADF;\n+\t\treturn -1;\n+\t}\n+\tif (GetFileType(fh) != FILE_TYPE_DISK)\n+\t\treturn (fstat)(fd, buf);\n+\tif (GetFileInformationByHandle(fh, &fdata)) {\n+\t\tbuf->st_dev = buf->st_rdev = 0; /* not used by Git */\n+\t\tbuf->st_ino = 0;\n+\t\tbuf->st_mode = file_attr_to_st_mode(fdata.dwFileAttributes);\n+\t\tbuf->st_nlink = 1;\n+\t\tbuf->st_uid = buf->st_gid = 0;\n+#ifdef __CYGWIN_USE_BIG_TYPES__\n+\t\tbuf->st_size = ((_off64_t)fdata.nFileSizeHigh << 32) +\n+\t\t\tfdata.nFileSizeLow;\n+#else\n+\t\tbuf->st_size = (off_t)fdata.nFileSizeLow;\n+#endif\n+\t\tbuf->st_blocks = size_to_blocks(buf->st_size);\n+\t\tfiletime_to_timespec(&fdata.ftLastAccessTime, &buf->st_atim);\n+\t\tfiletime_to_timespec(&fdata.ftLastWriteTime, &buf->st_mtim);\n+\t\tfiletime_to_timespec(&fdata.ftCreationTime, &buf->st_ctim);\n+\t\treturn 0;\n+\t}\n+\terrno = EBADF;\n+\treturn -1;\n+}\n \n /*\n * At start up, we are trying to determine whether Win32 API or cygwin stat\n@@ -133,9 +166,11 @@ static int init_stat(void)\n \t\tif (!core_filemode && native_stat) {\n \t\t\tcygwin_stat_fn = cygwin_stat;\n \t\t\tcygwin_lstat_fn = cygwin_lstat;\n+\t\t\tcygwin_fstat_fn = cygwin_fstat;\n \t\t} else {\n \t\t\tcygwin_stat_fn = stat;\n \t\t\tcygwin_lstat_fn = lstat;\n+\t\t\tcygwin_fstat_fn = fstat;\n \t\t}\n \t\treturn 0;\n \t}\n@@ -152,6 +187,12 @@ static int cygwin_lstat_stub(const char *file_name, struct stat *buf)\n \treturn (init_stat() ? lstat : *cygwin_lstat_fn)(file_name, buf);\n }\n \n+static int cygwin_fstat_stub(int fd, struct stat *buf)\n+{\n+\treturn (init_stat() ? fstat : *cygwin_fstat_fn)(fd, buf);\n+}\n+\n stat_fn_t cygwin_stat_fn = cygwin_stat_stub;\n stat_fn_t cygwin_lstat_fn = cygwin_lstat_stub;\n+fstat_fn_t cygwin_fstat_fn = cygwin_fstat_stub;\n \ndiff --git a/compat/cygwin.h b/compat/cygwin.h\nindex c04965a..f3ca3d2 100644\n--- a/compat/cygwin.h\n+++ b/compat/cygwin.h\n@@ -2,8 +2,10 @@\n #include \n \n typedef int (*stat_fn_t)(const char*, struct stat*);\n+typedef int (*fstat_fn_t)(int, struct stat*);\n extern stat_fn_t cygwin_stat_fn;\n extern stat_fn_t cygwin_lstat_fn;\n+extern fstat_fn_t cygwin_fstat_fn;\n int cygwin_get_st_mode_bits(const char *path, int *mode);\n \n #define get_st_mode_bits(p,m) cygwin_get_st_mode_bits((p),(m))\n@@ -11,4 +13,5 @@ int cygwin_get_st_mode_bits(const char *path, int *mode);\n /* cygwin.c needs the original lstat() */\n #define stat(path, buf) (*cygwin_stat_fn)(path, buf)\n #define lstat(path, buf) (*cygwin_lstat_fn)(path, buf)\n+#define fstat(fd, buf) (*cygwin_fstat_fn)(fd, buf)\n #endif\n-- \n1.8.3\n"},{"id":"222143","messageId":"51CCC643.1050702@ramsay1.demon.co.uk","threadId":"34276","inReplyTo":"51CBD2FD.5070905@alum.mit.edu","subject":"Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2013-06-27T23:09:55Z","receivedAt":"2013-06-27T23:09:55Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Michael Haggerty wrote:\n> On 06/27/2013 12:35 AM, Jeff King wrote:\n[ ... ]\n>> I think Michael's assessment above is missing one thing.\n> \n> Peff is absolutely right; for some unknown reason I was thinking of the\n> consistency check as having been already fixed.\n\nWell, the \"cygwin: Remove the Win32 l/stat() functions\" patch *does* fix\nthe problem. :-D It's just a pity we can't use it on performance grounds. :(\n\n>> [...#ifdef out consistency check on cygwin when lock is held...]\n> \n> Yes, this would work.\n> \n> But, taking a step back, I think it is a bad idea to have an unreliable\n> stat() masquerading as a real stat(). If we want to allow the use of an\n> unreliable stat for certain purposes, let's have two stat() interfaces:\n> \n> * the true stat() (in this case I guess cygwin's slow-but-correct\n> implementation)\n> \n> * some fast_but_maybe_unreliable_stat(), which would map to stat() on\n> most platforms but might map to the Windows stat() on cygwin when so\n> configured.\n> \n> By default the true stat() would always be used. It should have to be a\n> conscious decision, taken only in specific, vetted scenarios, to use the\n> unreliable stat.\n\nYou have just described my second patch! :D\n\n> \n> For example, I can't imagine that checking the freshness of the index or\n> of the packed-refs file is ever going to be a bottleneck, so there is no\n> reason at all to use an unreliable stat() here.\n> \n> On the other hand, stat() seems definitely to be a bottleneck when\n> testing for changes in a 100,000 file working tree, and here occasional\n> mistakes might be considered acceptable. So for this purpose the\n> unreliable stat() might be used.\n\nI have already written the first pass at this patch, but I'm having\ndifficulty with naming (get_cached_stat_data, get_index_stat_data,\nget_stat_data, ... ;-)\n\nATB,\nRamsay Jones\n"},{"id":"222144","messageId":"51CCC860.2070404@ramsay1.demon.co.uk","threadId":"34276","inReplyTo":"51CC57F2.8010805@web.de","subject":"Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2013-06-27T23:18:56Z","receivedAt":"2013-06-27T23:18:56Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Torsten Bögershausen wrote:\n[ ... ]\n\n>>> (And have a look how to improve the core.filemode)\n>>\n>> I don't understand this (parenthetical) comment; could you\n>> elaborate on this.\n>>\n\n> This is probably wrong information:\n> I had in mind that cygwin sets core.filemode=false,\n\nIt does, see commit c869753e (\"Force core.filemode to false on\nCygwin\", 30-12-2006).\n\n> which is quite annoying when exchanging .sh files with linux.\n\nIndeed, I used to build git with NO_TRUSTABLE_FILEMODE reset so\nthat I wouldn't have to edit the config file by hand after a\ngit-clone or git-init.\n\n> But that seems to be wrong, a quick test shows that core.filemode=true.\n\nHmm, it shouldn't - confused!\n\n> Sorry for confusion.\n\nATB\nRamsay Jones\n"},{"id":"222149","messageId":"51CCF57A.3020900@gmail.com","threadId":"34276","inReplyTo":"51CCC397.3060705@ramsay1.demon.co.uk","subject":"Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions","fromName":"Mark Levedahl","fromEmail":"mlevedahl@gmail.com","sentAt":"2013-06-28T02:31:22Z","receivedAt":"2013-06-28T02:31:22Z","isPatch":true,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"On 06/27/2013 06:58 PM, Ramsay Jones wrote:\n\n> This is why I tried the \"cygwin: Remove the Win32 l/stat()\n> functions\" patch first; I think this is the correct approach\n> to fixing this problem (and similar *future* problems).\nI adamantly agree.\n> However, since that is no longer an option, on performance\n> grounds, I have other ideas I want to try. (see later email)\n\nCorrectness first, speed later.\n\n1) Keep the patch to remove the buggy and unreliable stat / lstat.\n2) We fix the remaining test failures.\n3) With the test suite passing, stat optimization(s) that cause no \nfailures / regressions can be accepted.\n\nWith the msys/mingw git available for years now, there really is not a \nreason to make Cygwin's git violate the Posix expectations of that \nplatform. msys makes no such promises, so is the right tool for those on \nWindows who just want git as fast as possible on Windows (still \nslooooow) and don't care about file modes, softlinks, etc.\n\nI'm keeping your patch in my tree.\n\nMark\n"},{"id":"222255","messageId":"51D06AC8.70206@ramsay1.demon.co.uk","threadId":"34276","inReplyTo":"51CCC643.1050702@ramsay1.demon.co.uk","subject":"Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2013-06-30T17:28:40Z","receivedAt":"2013-06-30T17:28:40Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Ramsay Jones wrote:\n> Michael Haggerty wrote:\n>> On 06/27/2013 12:35 AM, Jeff King wrote:\n> [ ... ]\n>>> I think Michael's assessment above is missing one thing.\n>>\n>> Peff is absolutely right; for some unknown reason I was thinking of the\n>> consistency check as having been already fixed.\n> \n> Well, the \"cygwin: Remove the Win32 l/stat() functions\" patch *does* fix\n> the problem. :-D It's just a pity we can't use it on performance grounds. :(\n> \n>>> [...#ifdef out consistency check on cygwin when lock is held...]\n>>\n>> Yes, this would work.\n>>\n>> But, taking a step back, I think it is a bad idea to have an unreliable\n>> stat() masquerading as a real stat(). If we want to allow the use of an\n>> unreliable stat for certain purposes, let's have two stat() interfaces:\n>>\n>> * the true stat() (in this case I guess cygwin's slow-but-correct\n>> implementation)\n>>\n>> * some fast_but_maybe_unreliable_stat(), which would map to stat() on\n>> most platforms but might map to the Windows stat() on cygwin when so\n>> configured.\n>>\n>> By default the true stat() would always be used. It should have to be a\n>> conscious decision, taken only in specific, vetted scenarios, to use the\n>> unreliable stat.\n> \n> You have just described my second patch! :D\n\nUnfortunately, I have not had any time to work on the patch this weekend.\nHowever, despite the patch being a bit rough around the edges, I decided\nto send it out (see below) to get some early feedback.\n\nNote that it passes the t3210, t3211, t5500, t3200, t3301, t7606 and t1301\ntests, but I have not run the full test suite.\n\nComments welcome.\n\nATB,\nRamsay Jones\n\n-- >8 --\nSubject: [PATCH] cygwin: Add fast_[l]stat() functions\n\nSigned-off-by: Ramsay Jones \n---\n builtin/apply.c | 6 +++---\n builtin/commit.c | 2 +-\n builtin/ls-files.c | 2 +-\n builtin/rm.c | 2 +-\n builtin/update-index.c | 2 +-\n check-racy.c | 2 +-\n compat/cygwin.c | 12 ------------\n compat/cygwin.h | 10 +++-------\n diff-lib.c | 2 +-\n diff.c | 2 +-\n entry.c | 4 ++--\n git-compat-util.h | 13 +++++++++++--\n help.c | 5 +----\n path.c | 9 +--------\n preload-index.c | 2 +-\n read-cache.c | 6 +++---\n unpack-trees.c | 8 ++++----\n 17 files changed, 36 insertions(+), 53 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 0e9b631..ca26caa 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -3253,7 +3253,7 @@ static int load_current(struct image *image, struct patch *patch)\n \tif (pos < 0)\n \t\treturn error(_(\"%s: does not exist in index\"), name);\n \tce = active_cache[pos];\n-\tif (lstat(name, &st)) {\n+\tif (fast_lstat(name, &st)) {\n \t\tif (errno != ENOENT)\n \t\t\treturn error(_(\"%s: %s\"), name, strerror(errno));\n \t\tif (checkout_target(ce, &st))\n@@ -3396,7 +3396,7 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s\n \tif (previous) {\n \t\tst_mode = previous->new_mode;\n \t} else if (!cached) {\n-\t\tstat_ret = lstat(old_name, st);\n+\t\tstat_ret = fast_lstat(old_name, st);\n \t\tif (stat_ret && errno != ENOENT)\n \t\t\treturn error(_(\"%s: %s\"), old_name, strerror(errno));\n \t}\n@@ -3850,7 +3850,7 @@ static void add_index_file(const char *path, unsigned mode, void *buf, unsigned\n \t\t\tdie(_(\"corrupt patch for subproject %s\"), path);\n \t} else {\n \t\tif (!cached) {\n-\t\t\tif (lstat(path, &st) < 0)\n+\t\t\tif (fast_lstat(path, &st) < 0)\n \t\t\t\tdie_errno(_(\"unable to stat newly created file '%s'\"),\n \t\t\t\t\t path);\n \t\t\tfill_stat_cache_info(ce, &st);\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 6b693c1..1d208c6 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -231,7 +231,7 @@ static void add_remove_files(struct string_list *list)\n \t\tif (p->util)\n \t\t\tcontinue;\n \n-\t\tif (!lstat(p->string, &st)) {\n+\t\tif (!fast_lstat(p->string, &st)) {\n \t\t\tif (add_to_cache(p->string, &st, 0))\n \t\t\t\tdie(_(\"updating files failed\"));\n \t\t} else\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 08d9786..db66a0e 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -251,7 +251,7 @@ static void show_files(struct dir_struct *dir)\n \t\t\t\tcontinue;\n \t\t\tif (ce_skip_worktree(ce))\n \t\t\t\tcontinue;\n-\t\t\terr = lstat(ce->name, &st);\n+\t\t\terr = fast_lstat(ce->name, &st);\n \t\t\tif (show_deleted && err)\n \t\t\t\tshow_ce_entry(tag_removed, ce);\n \t\t\tif (show_modified && ce_modified(ce, &st, 0))\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 06025a2..4b783e7 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -143,7 +143,7 @@ static int check_local_mod(unsigned char *head, int index_only)\n \t\t}\n \t\tce = active_cache[pos];\n \n-\t\tif (lstat(ce->name, &st) < 0) {\n+\t\tif (fast_lstat(ce->name, &st) < 0) {\n \t\t\tif (errno != ENOENT && errno != ENOTDIR)\n \t\t\t\twarning(\"'%s': %s\", ce->name, strerror(errno));\n \t\t\t/* It already vanished from the working tree */\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 5c7762e..4790e4c 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -206,7 +206,7 @@ static int process_path(const char *path)\n \t * First things first: get the stat information, to decide\n \t * what to do about the pathname!\n \t */\n-\tif (lstat(path, &st) < 0)\n+\tif (fast_lstat(path, &st) < 0)\n \t\treturn process_lstat_error(path, errno);\n \n \tif (S_ISDIR(st.st_mode))\ndiff --git a/check-racy.c b/check-racy.c\nindex 00d92a1..6124355 100644\n--- a/check-racy.c\n+++ b/check-racy.c\n@@ -11,7 +11,7 @@ int main(int ac, char **av)\n \t\tstruct cache_entry *ce = active_cache[i];\n \t\tstruct stat st;\n \n-\t\tif (lstat(ce->name, &st)) {\n+\t\tif (fast_lstat(ce->name, &st)) {\n \t\t\terror(\"lstat(%s): %s\", ce->name, strerror(errno));\n \t\t\tcontinue;\n \t\t}\ndiff --git a/compat/cygwin.c b/compat/cygwin.c\nindex 91ce5d4..f07cbd3 100644\n--- a/compat/cygwin.c\n+++ b/compat/cygwin.c\n@@ -6,18 +6,6 @@\n #include \"../git-compat-util.h\"\n #include \"../cache.h\" /* to read configuration */\n \n-/*\n- * Return POSIX permission bits, regardless of core.ignorecygwinfstricks\n- */\n-int cygwin_get_st_mode_bits(const char *path, int *mode)\n-{\n-\tstruct stat st;\n-\tif (lstat(path, &st) < 0)\n-\t\treturn -1;\n-\t*mode = st.st_mode;\n-\treturn 0;\n-}\n-\n static inline void filetime_to_timespec(const FILETIME *ft, struct timespec *ts)\n {\n \tlong long winTime = ((long long)ft->dwHighDateTime << 32) +\ndiff --git a/compat/cygwin.h b/compat/cygwin.h\nindex c04965a..8299f58 100644\n--- a/compat/cygwin.h\n+++ b/compat/cygwin.h\n@@ -4,11 +4,7 @@\n typedef int (*stat_fn_t)(const char*, struct stat*);\n extern stat_fn_t cygwin_stat_fn;\n extern stat_fn_t cygwin_lstat_fn;\n-int cygwin_get_st_mode_bits(const char *path, int *mode);\n \n-#define get_st_mode_bits(p,m) cygwin_get_st_mode_bits((p),(m))\n-#ifndef CYGWIN_C\n-/* cygwin.c needs the original lstat() */\n-#define stat(path, buf) (*cygwin_stat_fn)(path, buf)\n-#define lstat(path, buf) (*cygwin_lstat_fn)(path, buf)\n-#endif\n+#define fast_stat(path, buf) (*cygwin_stat_fn)(path, buf)\n+#define fast_lstat(path, buf) (*cygwin_lstat_fn)(path, buf)\n+#define GIT_FAST_STAT\ndiff --git a/diff-lib.c b/diff-lib.c\nindex b6f4b21..401dab6 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -27,7 +27,7 @@\n */\n static int check_removed(const struct cache_entry *ce, struct stat *st)\n {\n-\tif (lstat(ce->name, st) < 0) {\n+\tif (fast_lstat(ce->name, st) < 0) {\n \t\tif (errno != ENOENT && errno != ENOTDIR)\n \t\t\treturn -1;\n \t\treturn 1;\ndiff --git a/diff.c b/diff.c\nindex 208094f..212d3ff 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2642,7 +2642,7 @@ static int reuse_worktree_file(const char *name, const unsigned char *sha1, int\n \t * If ce matches the file in the work tree, we can reuse it.\n \t */\n \tif (ce_uptodate(ce) ||\n-\t (!lstat(name, &st) && !ce_match_stat(ce, &st, 0)))\n+\t (!fast_lstat(name, &st) && !ce_match_stat(ce, &st, 0)))\n \t\treturn 1;\n \n \treturn 0;\ndiff --git a/entry.c b/entry.c\nindex d7c131d..4d2ac73 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -210,7 +210,7 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout\n finish:\n \tif (state->refresh_cache) {\n \t\tif (!fstat_done)\n-\t\t\tlstat(ce->name, &st);\n+\t\t\tfast_lstat(ce->name, &st);\n \t\tfill_stat_cache_info(ce, &st);\n \t}\n \treturn 0;\n@@ -230,7 +230,7 @@ static int check_path(const char *path, int len, struct stat *st, int skiplen)\n \t\terrno = ENOENT;\n \t\treturn -1;\n \t}\n-\treturn lstat(path, st);\n+\treturn fast_lstat(path, st);\n }\n \n int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *topath)\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex ff193f4..7997bdb 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -129,8 +129,6 @@\n #include \n #endif\n \n-extern int get_st_mode_bits(const char *path, int *mode);\n-\n #if defined(__MINGW32__)\n /* pull in Windows compatibility stuff */\n #include \"compat/mingw.h\"\n@@ -179,6 +177,17 @@ typedef unsigned long uintptr_t;\n #endif\n #endif\n \n+#ifndef GIT_FAST_STAT\n+static inline int fast_stat(const char *path, struct stat *st)\n+{\n+\treturn stat(path, st);\n+}\n+static inline int fast_lstat(const char *path, struct stat *st)\n+{\n+\treturn lstat(path, st);\n+}\n+#endif\n+\n /* used on Mac OS X */\n #ifdef PRECOMPOSE_UNICODE\n #include \"compat/precompose_utf8.h\"\ndiff --git a/help.c b/help.c\nindex 08c54ef..f068925 100644\n--- a/help.c\n+++ b/help.c\n@@ -107,10 +107,7 @@ static int is_executable(const char *name)\n \t !S_ISREG(st.st_mode))\n \t\treturn 0;\n \n-#if defined(GIT_WINDOWS_NATIVE) || defined(__CYGWIN__)\n-#if defined(__CYGWIN__)\n-if ((st.st_mode & S_IXUSR) == 0)\n-#endif\n+#if defined(GIT_WINDOWS_NATIVE)\n {\t/* cannot trust the executable bit, peek into the file instead */\n \tchar buf[3] = { 0 };\n \tint n;\ndiff --git a/path.c b/path.c\nindex 7f3324a..3d244d3 100644\n--- a/path.c\n+++ b/path.c\n@@ -5,13 +5,7 @@\n #include \"strbuf.h\"\n #include \"string-list.h\"\n \n-#ifndef get_st_mode_bits\n-/*\n- * The replacement lstat(2) we use on Cygwin is incomplete and\n- * may return wrong permission bits. Most of the time we do not care,\n- * but the callsites of this wrapper do care.\n- */\n-int get_st_mode_bits(const char *path, int *mode)\n+static int get_st_mode_bits(const char *path, int *mode)\n {\n \tstruct stat st;\n \tif (lstat(path, &st) < 0)\n@@ -19,7 +13,6 @@ int get_st_mode_bits(const char *path, int *mode)\n \t*mode = st.st_mode;\n \treturn 0;\n }\n-#endif\n \n static char bad_path[] = \"/bad-path/\";\n \ndiff --git a/preload-index.c b/preload-index.c\nindex 49cb08d..1bece91 100644\n--- a/preload-index.c\n+++ b/preload-index.c\n@@ -57,7 +57,7 @@ static void *preload_thread(void *_data)\n \t\t\tcontinue;\n \t\tif (threaded_has_symlink_leading_path(&cache, ce->name, ce_namelen(ce)))\n \t\t\tcontinue;\n-\t\tif (lstat(ce->name, &st))\n+\t\tif (fast_lstat(ce->name, &st))\n \t\t\tcontinue;\n \t\tif (ie_match_stat(index, ce, &st, CE_MATCH_RACY_IS_DIRTY))\n \t\t\tcontinue;\ndiff --git a/read-cache.c b/read-cache.c\nindex d5201f9..ed33d9e 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -689,7 +689,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n int add_file_to_index(struct index_state *istate, const char *path, int flags)\n {\n \tstruct stat st;\n-\tif (lstat(path, &st))\n+\tif (fast_lstat(path, &st))\n \t\tdie_errno(\"unable to stat '%s'\", path);\n \treturn add_to_index(istate, path, &st, flags);\n }\n@@ -1049,7 +1049,7 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,\n \t\treturn ce;\n \t}\n \n-\tif (lstat(ce->name, &st) < 0) {\n+\tif (fast_lstat(ce->name, &st) < 0) {\n \t\tif (err)\n \t\t\t*err = errno;\n \t\treturn NULL;\n@@ -1635,7 +1635,7 @@ static void ce_smudge_racily_clean_entry(struct cache_entry *ce)\n \t */\n \tstruct stat st;\n \n-\tif (lstat(ce->name, &st) < 0)\n+\tif (fast_lstat(ce->name, &st) < 0)\n \t\treturn;\n \tif (ce_match_stat_basic(ce, &st))\n \t\treturn;\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex b27f2a6..1fe9b63 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1215,7 +1215,7 @@ static int verify_uptodate_1(const struct cache_entry *ce,\n \telse if (o->reset || ce_uptodate(ce))\n \t\treturn 0;\n \n-\tif (!lstat(ce->name, &st)) {\n+\tif (!fast_lstat(ce->name, &st)) {\n \t\tint flags = CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE;\n \t\tunsigned changed = ie_match_stat(o->src_index, ce, &st, flags);\n \t\tif (!changed)\n@@ -1432,13 +1432,13 @@ static int verify_absent_1(const struct cache_entry *ce,\n \t\tchar path[PATH_MAX + 1];\n \t\tmemcpy(path, ce->name, len);\n \t\tpath[len] = 0;\n-\t\tif (lstat(path, &st))\n+\t\tif (fast_lstat(path, &st))\n \t\t\treturn error(\"cannot stat '%s': %s\", path,\n \t\t\t\t\tstrerror(errno));\n \n \t\treturn check_ok_to_remove(path, len, DT_UNKNOWN, NULL, &st,\n \t\t\t\terror_type, o);\n-\t} else if (lstat(ce->name, &st)) {\n+\t} else if (fast_lstat(ce->name, &st)) {\n \t\tif (errno != ENOENT)\n \t\t\treturn error(\"cannot stat '%s': %s\", ce->name,\n \t\t\t\t strerror(errno));\n@@ -1852,7 +1852,7 @@ int oneway_merge(const struct cache_entry * const *src,\n \t\tint update = 0;\n \t\tif (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) {\n \t\t\tstruct stat st;\n-\t\t\tif (lstat(old->name, &st) ||\n+\t\t\tif (fast_lstat(old->name, &st) ||\n \t\t\t ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))\n \t\t\t\tupdate |= CE_UPDATE;\n \t\t}\n-- \n1.8.3\n"},{"id":"222263","messageId":"7v4ncfjs32.fsf@alter.siamese.dyndns.org","threadId":"34276","inReplyTo":"51D06AC8.70206@ramsay1.demon.co.uk","subject":"Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-30T19:50:41Z","receivedAt":"2013-06-30T19:50:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones writes:\n\n> Ramsay Jones wrote:\n>> Michael Haggerty wrote:\n>>> On 06/27/2013 12:35 AM, Jeff King wrote:\n>> [ ... ]\n>>>> I think Michael's assessment above is missing one thing.\n>>>\n>>> Peff is absolutely right; for some unknown reason I was thinking of the\n>>> consistency check as having been already fixed.\n>> \n>> Well, the \"cygwin: Remove the Win32 l/stat() functions\" patch *does* fix\n>> the problem. :-D It's just a pity we can't use it on performance grounds. :(\n>> \n>>>> [...#ifdef out consistency check on cygwin when lock is held...]\n>>>\n>>> Yes, this would work.\n>>>\n>>> But, taking a step back, I think it is a bad idea to have an unreliable\n>>> stat() masquerading as a real stat(). If we want to allow the use of an\n>>> unreliable stat for certain purposes, let's have two stat() interfaces:\n>>>\n>>> * the true stat() (in this case I guess cygwin's slow-but-correct\n>>> implementation)\n>>>\n>>> * some fast_but_maybe_unreliable_stat(), which would map to stat() on\n>>> most platforms but might map to the Windows stat() on cygwin when so\n>>> configured.\n>>>\n>>> By default the true stat() would always be used. It should have to be a\n>>> conscious decision, taken only in specific, vetted scenarios, to use the\n>>> unreliable stat.\n>> ...\n\nI like the part that gets rid of that \"get-mode-bits\" but at the\nsame time, I find this part wanting a reasonable in-code comment.\n\nAt least, with the earlier get-mode-bits, it was clear why we are\ndoing something special in that codepath, both from the name of the\nhelper function/macro and the comment attached to it describing how\nthe \"regular\" one is cheating.\n\nWe must say why this \"fast\" is not used everywhere and what criteria\nyou should apply when deciding to use it (or not use it). And the\n\"fast\" name is much less descriptive.\n\nI suspect (without thinking it through) that the rule would be\nsomething like:\n\n The \"fast\" variant is to be used to read from the filesystem the\n \"stat\" bits that are stuffed into the index for quick \"touch\n detection\" (aka \"cached stat info\") and/or that are compared\n with the cached stat info, and should not be used anywhere else.\n\nBut then we always use lstat(2) and not stat(2) for that, so...\n\n> +#ifndef GIT_FAST_STAT\n> +static inline int fast_stat(const char *path, struct stat *st)\n> +{\n> +\treturn stat(path, st);\n> +}\n> +static inline int fast_lstat(const char *path, struct stat *st)\n> +{\n> +\treturn lstat(path, st);\n> +}\n> +#endif\n"},{"id":"222579","messageId":"51D5BC83.20706@ramsay1.demon.co.uk","threadId":"34276","inReplyTo":"7v4ncfjs32.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2013-07-04T18:18:43Z","receivedAt":"2013-07-04T18:18:43Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Junio C Hamano wrote:\n> I like the part that gets rid of that \"get-mode-bits\" but at the\n> same time, I find this part wanting a reasonable in-code comment.\n\nIndeed. (As I said, a bit rough around the edges ;-)\n\n> At least, with the earlier get-mode-bits, it was clear why we are\n> doing something special in that codepath, both from the name of the\n> helper function/macro and the comment attached to it describing how\n> the \"regular\" one is cheating.\n> \n> We must say why this \"fast\" is not used everywhere and what criteria\n> you should apply when deciding to use it (or not use it). And the\n> \"fast\" name is much less descriptive.\n> \n> I suspect (without thinking it through) that the rule would be\n> something like:\n> \n> The \"fast\" variant is to be used to read from the filesystem the\n> \"stat\" bits that are stuffed into the index for quick \"touch\n> detection\" (aka \"cached stat info\") and/or that are compared\n> with the cached stat info, and should not be used anywhere else.\n\nSounds good to me.\n\n> But then we always use lstat(2) and not stat(2) for that, so...\n\nIndeed. Although there may be need of an \"fast_fstat\" (see below). :(\n\n>> +#ifndef GIT_FAST_STAT\n>> +static inline int fast_stat(const char *path, struct stat *st)\n>> +{\n>> +\treturn stat(path, st);\n>> +}\n>> +static inline int fast_lstat(const char *path, struct stat *st)\n>> +{\n>> +\treturn lstat(path, st);\n>> +}\n>> +#endif\n\nYes, I'm not very good at naming things, so suggestions welcome!\n\nNote that I missed at least one lstat() call which needed to be renamed\nto fast_lstat() (builtin/apply.c, line 3094 in checkout_target()).\nThis is my main concern with this patch (i.e. that I have missed some\nmore lstat calls that need to be renamed). I was a little surprised\nat the size of the patch; direct index manipulation is more widespread\nthan I had expected.\n\nAlso, since cygwin has UNRELIABLE_FSTAT defined, on the first pass of\nthe patch, I ignored the use of fstat() in write_entry(). However, *if*\nwe allow for some other platform, which has a reliable fstat, but wants\nto provide \"fast\" stat variants, then these fstat calls should logically\nbe \"fast\". Alternatively, we could drop the use of fstat(), like so:\n\n diff --git a/entry.c b/entry.c\n index 4d2ac73..d04d7a1 100644\n --- a/entry.c\n +++ b/entry.c\n @@ -104,21 +104,9 @@ static int open_output_fd(char *path, struct cache_entry *ce, int to_tempfile)\n \t}\n }\n \n -static int fstat_output(int fd, const struct checkout *state, struct stat *st)\n -{\n -\t/* use fstat() only when path == ce->name */\n -\tif (fstat_is_reliable() &&\n -\t state->refresh_cache && !state->base_dir_len) {\n -\t\tfstat(fd, st);\n -\t\treturn 1;\n -\t}\n -\treturn 0;\n -}\n -\n static int streaming_write_entry(struct cache_entry *ce, char *path,\n \t\t\t\t struct stream_filter *filter,\n -\t\t\t\t const struct checkout *state, int to_tempfile,\n -\t\t\t\t int *fstat_done, struct stat *statbuf)\n +\t\t\t\t const struct checkout *state, int to_tempfile)\n {\n \tint result = 0;\n \tint fd;\n @@ -128,7 +116,6 @@ static int streaming_write_entry(struct cache_entry *ce, char *path,\n \t\treturn -1;\n \n \tresult |= stream_blob_to_fd(fd, ce->sha1, filter, 1);\n -\t*fstat_done = fstat_output(fd, state, statbuf);\n \tresult |= close(fd);\n \n \tif (result)\n @@ -139,7 +126,7 @@ static int streaming_write_entry(struct cache_entry *ce, char *path,\n static int write_entry(struct cache_entry *ce, char *path, const struct checkout *state, int to_tempfile)\n {\n \tunsigned int ce_mode_s_ifmt = ce->ce_mode & S_IFMT;\n -\tint fd, ret, fstat_done = 0;\n +\tint fd, ret;\n \tchar *new;\n \tstruct strbuf buf = STRBUF_INIT;\n \tunsigned long size;\n @@ -150,8 +137,7 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout\n \t\tstruct stream_filter *filter = get_stream_filter(ce->name, ce->sha1);\n \t\tif (filter &&\n \t\t !streaming_write_entry(ce, path, filter,\n -\t\t\t\t\t state, to_tempfile,\n -\t\t\t\t\t &fstat_done, &st))\n +\t\t\t\t\t state, to_tempfile))\n \t\t\tgoto finish;\n \t}\n \n @@ -190,8 +176,6 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout\n \t\t}\n \n \t\twrote = write_in_full(fd, new, size);\n -\t\tif (!to_tempfile)\n -\t\t\tfstat_done = fstat_output(fd, state, &st);\n \t\tclose(fd);\n \t\tfree(new);\n \t\tif (wrote != size)\n @@ -209,8 +193,7 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout\n \n finish:\n \tif (state->refresh_cache) {\n -\t\tif (!fstat_done)\n -\t\t\tfast_lstat(ce->name, &st);\n +\t\tfast_lstat(ce->name, &st);\n \t\tfill_stat_cache_info(ce, &st);\n \t}\n \treturn 0;\n -- \n\nI would need to do some timing tests (on Linux) to see what effect this\nwould have on performance. (I noticed that the test suite ran about 2%\nslower with the above applied).\n\nA simple pass-though fast_fstat(), including on cygwin, is probably the\nway to go.\n\nSorry for being a bit slow on this - I'm very busy at the moment. :(\n\nATB,\nRamsay Jones\n"},{"id":"222913","messageId":"51DBEDAF.30708@web.de","threadId":"34276","inReplyTo":"51D06AC8.70206@ramsay1.demon.co.uk","subject":"Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-07-09T11:02:07Z","receivedAt":"2013-07-09T11:02:07Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 30.06.13 19:28, Ramsay Jones wrote:\n> Ramsay Jones wrote:\n>> Michael Haggerty wrote:\n>>> On 06/27/2013 12:35 AM, Jeff King wrote:\n>> [ ... ]\n>>>> I think Michael's assessment above is missing one thing.\n>>> Peff is absolutely right; for some unknown reason I was thinking of the\n>>> consistency check as having been already fixed.\n>> Well, the \"cygwin: Remove the Win32 l/stat() functions\" patch *does* fix\n>> the problem. :-D It's just a pity we can't use it on performance grounds. :(\n>>\n>>>> [...#ifdef out consistency check on cygwin when lock is held...]\n>>> Yes, this would work.\n>>>\n>>> But, taking a step back, I think it is a bad idea to have an unreliable\n>>> stat() masquerading as a real stat(). If we want to allow the use of an\n>>> unreliable stat for certain purposes, let's have two stat() interfaces:\n>>>\n>>> * the true stat() (in this case I guess cygwin's slow-but-correct\n>>> implementation)\n>>>\n>>> * some fast_but_maybe_unreliable_stat(), which would map to stat() on\n>>> most platforms but might map to the Windows stat() on cygwin when so\n>>> configured.\n>>>\n>>> By default the true stat() would always be used. It should have to be a\n>>> conscious decision, taken only in specific, vetted scenarios, to use the\n>>> unreliable stat.\n>> You have just described my second patch! :D\n> Unfortunately, I have not had any time to work on the patch this weekend.\n> However, despite the patch being a bit rough around the edges, I decided\n> to send it out (see below) to get some early feedback.\n>\n> Note that it passes the t3210, t3211, t5500, t3200, t3301, t7606 and t1301\n> tests, but I have not run the full test suite.\n>\n> Comments welcome.\n>\n> ATB,\n> Ramsay Jones\n>\n> -- >8 --\n> Subject: [PATCH] cygwin: Add fast_[l]stat() functions\n>\n> Signed-off-by: Ramsay Jones \n> ---\n> builtin/apply.c | 6 +++---\n> builtin/commit.c | 2 +-\n> builtin/ls-files.c | 2 +-\n> builtin/rm.c | 2 +-\n> builtin/update-index.c | 2 +-\n> check-racy.c | 2 +-\n> compat/cygwin.c | 12 ------------\n> compat/cygwin.h | 10 +++-------\n> diff-lib.c | 2 +-\n> diff.c | 2 +-\n> entry.c | 4 ++--\n> git-compat-util.h | 13 +++++++++++--\n> help.c | 5 +----\n> path.c | 9 +--------\n> preload-index.c | 2 +-\n> read-cache.c | 6 +++---\n> unpack-trees.c | 8 ++++----\n> 17 files changed, 36 insertions(+), 53 deletions(-)\n>\n> diff --git a/builtin/apply.c b/builtin/apply.c\n> index 0e9b631..ca26caa 100644\n> --- a/builtin/apply.c\n> +++ b/builtin/apply.c\n> @@ -3253,7 +3253,7 @@ static int load_current(struct image *image, struct patch *patch)\n> \tif (pos < 0)\n> \t\treturn error(_(\"%s: does not exist in index\"), name);\n> \tce = active_cache[pos];\n> -\tif (lstat(name, &st)) {\n> +\tif (fast_lstat(name, &st)) {\n> \t\tif (errno != ENOENT)\n> \t\t\treturn error(_(\"%s: %s\"), name, strerror(errno));\n> \t\tif (checkout_target(ce, &st))\n> @@ -3396,7 +3396,7 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s\n> \tif (previous) {\n> \t\tst_mode = previous->new_mode;\n> \t} else if (!cached) {\n> -\t\tstat_ret = lstat(old_name, st);\n> +\t\tstat_ret = fast_lstat(old_name, st);\n> \t\tif (stat_ret && errno != ENOENT)\n> \t\t\treturn error(_(\"%s: %s\"), old_name, strerror(errno));\n> \t}\n> @@ -3850,7 +3850,7 @@ static void add_index_file(const char *path, unsigned mode, void *buf, unsigned\n> \t\t\tdie(_(\"corrupt patch for subproject %s\"), path);\n> \t} else {\n> \t\tif (!cached) {\n> -\t\t\tif (lstat(path, &st) < 0)\n> +\t\t\tif (fast_lstat(path, &st) < 0)\n> \t\t\t\tdie_errno(_(\"unable to stat newly created file '%s'\"),\n> \t\t\t\t\t path);\n> \t\t\tfill_stat_cache_info(ce, &st);\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 6b693c1..1d208c6 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -231,7 +231,7 @@ static void add_remove_files(struct string_list *list)\n> \t\tif (p->util)\n> \t\t\tcontinue;\n> \n> -\t\tif (!lstat(p->string, &st)) {\n> +\t\tif (!fast_lstat(p->string, &st)) {\n> \t\t\tif (add_to_cache(p->string, &st, 0))\n> \t\t\t\tdie(_(\"updating files failed\"));\n> \t\t} else\n> diff --git a/builtin/ls-files.c b/builtin/ls-files.c\n> index 08d9786..db66a0e 100644\n> --- a/builtin/ls-files.c\n> +++ b/builtin/ls-files.c\n> @@ -251,7 +251,7 @@ static void show_files(struct dir_struct *dir)\n> \t\t\t\tcontinue;\n> \t\t\tif (ce_skip_worktree(ce))\n> \t\t\t\tcontinue;\n> -\t\t\terr = lstat(ce->name, &st);\n> +\t\t\terr = fast_lstat(ce->name, &st);\n> \t\t\tif (show_deleted && err)\n> \t\t\t\tshow_ce_entry(tag_removed, ce);\n> \t\t\tif (show_modified && ce_modified(ce, &st, 0))\n> diff --git a/builtin/rm.c b/builtin/rm.c\n> index 06025a2..4b783e7 100644\n> --- a/builtin/rm.c\n> +++ b/builtin/rm.c\n> @@ -143,7 +143,7 @@ static int check_local_mod(unsigned char *head, int index_only)\n> \t\t}\n> \t\tce = active_cache[pos];\n> \n> -\t\tif (lstat(ce->name, &st) < 0) {\n> +\t\tif (fast_lstat(ce->name, &st) < 0) {\n> \t\t\tif (errno != ENOENT && errno != ENOTDIR)\n> \t\t\t\twarning(\"'%s': %s\", ce->name, strerror(errno));\n> \t\t\t/* It already vanished from the working tree */\n> diff --git a/builtin/update-index.c b/builtin/update-index.c\n> index 5c7762e..4790e4c 100644\n> --- a/builtin/update-index.c\n> +++ b/builtin/update-index.c\n> @@ -206,7 +206,7 @@ static int process_path(const char *path)\n> \t * First things first: get the stat information, to decide\n> \t * what to do about the pathname!\n> \t */\n> -\tif (lstat(path, &st) < 0)\n> +\tif (fast_lstat(path, &st) < 0)\n> \t\treturn process_lstat_error(path, errno);\n> \n> \tif (S_ISDIR(st.st_mode))\n> diff --git a/check-racy.c b/check-racy.c\n> index 00d92a1..6124355 100644\n> --- a/check-racy.c\n> +++ b/check-racy.c\n> @@ -11,7 +11,7 @@ int main(int ac, char **av)\n> \t\tstruct cache_entry *ce = active_cache[i];\n> \t\tstruct stat st;\n> \n> -\t\tif (lstat(ce->name, &st)) {\n> +\t\tif (fast_lstat(ce->name, &st)) {\n> \t\t\terror(\"lstat(%s): %s\", ce->name, strerror(errno));\n> \t\t\tcontinue;\n> \t\t}\n> diff --git a/compat/cygwin.c b/compat/cygwin.c\n> index 91ce5d4..f07cbd3 100644\n> --- a/compat/cygwin.c\n> +++ b/compat/cygwin.c\n> @@ -6,18 +6,6 @@\n> #include \"../git-compat-util.h\"\n> #include \"../cache.h\" /* to read configuration */\n> \n> -/*\n> - * Return POSIX permission bits, regardless of core.ignorecygwinfstricks\n> - */\n> -int cygwin_get_st_mode_bits(const char *path, int *mode)\n> -{\n> -\tstruct stat st;\n> -\tif (lstat(path, &st) < 0)\n> -\t\treturn -1;\n> -\t*mode = st.st_mode;\n> -\treturn 0;\n> -}\n> -\n> static inline void filetime_to_timespec(const FILETIME *ft, struct timespec *ts)\n> {\n> \tlong long winTime = ((long long)ft->dwHighDateTime << 32) +\n> diff --git a/compat/cygwin.h b/compat/cygwin.h\n> index c04965a..8299f58 100644\n> --- a/compat/cygwin.h\n> +++ b/compat/cygwin.h\n> @@ -4,11 +4,7 @@\n> typedef int (*stat_fn_t)(const char*, struct stat*);\n> extern stat_fn_t cygwin_stat_fn;\n> extern stat_fn_t cygwin_lstat_fn;\n> -int cygwin_get_st_mode_bits(const char *path, int *mode);\n> \n> -#define get_st_mode_bits(p,m) cygwin_get_st_mode_bits((p),(m))\n> -#ifndef CYGWIN_C\n> -/* cygwin.c needs the original lstat() */\n> -#define stat(path, buf) (*cygwin_stat_fn)(path, buf)\n> -#define lstat(path, buf) (*cygwin_lstat_fn)(path, buf)\n> -#endif\n> +#define fast_stat(path, buf) (*cygwin_stat_fn)(path, buf)\n> +#define fast_lstat(path, buf) (*cygwin_lstat_fn)(path, buf)\n> +#define GIT_FAST_STAT\n> diff --git a/diff-lib.c b/diff-lib.c\n> index b6f4b21..401dab6 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -27,7 +27,7 @@\n> */\n> static int check_removed(const struct cache_entry *ce, struct stat *st)\n> {\n> -\tif (lstat(ce->name, st) < 0) {\n> +\tif (fast_lstat(ce->name, st) < 0) {\n> \t\tif (errno != ENOENT && errno != ENOTDIR)\n> \t\t\treturn -1;\n> \t\treturn 1;\n> diff --git a/diff.c b/diff.c\n> index 208094f..212d3ff 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -2642,7 +2642,7 @@ static int reuse_worktree_file(const char *name, const unsigned char *sha1, int\n> \t * If ce matches the file in the work tree, we can reuse it.\n> \t */\n> \tif (ce_uptodate(ce) ||\n> -\t (!lstat(name, &st) && !ce_match_stat(ce, &st, 0)))\n> +\t (!fast_lstat(name, &st) && !ce_match_stat(ce, &st, 0)))\n> \t\treturn 1;\n> \n> \treturn 0;\n> diff --git a/entry.c b/entry.c\n> index d7c131d..4d2ac73 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -210,7 +210,7 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout\n> finish:\n> \tif (state->refresh_cache) {\n> \t\tif (!fstat_done)\n> -\t\t\tlstat(ce->name, &st);\n> +\t\t\tfast_lstat(ce->name, &st);\n> \t\tfill_stat_cache_info(ce, &st);\n> \t}\n> \treturn 0;\n> @@ -230,7 +230,7 @@ static int check_path(const char *path, int len, struct stat *st, int skiplen)\n> \t\terrno = ENOENT;\n> \t\treturn -1;\n> \t}\n> -\treturn lstat(path, st);\n> +\treturn fast_lstat(path, st);\n> }\n> \n> int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *topath)\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index ff193f4..7997bdb 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -129,8 +129,6 @@\n> #include \n> #endif\n> \n> -extern int get_st_mode_bits(const char *path, int *mode);\n> -\n> #if defined(__MINGW32__)\n> /* pull in Windows compatibility stuff */\n> #include \"compat/mingw.h\"\n> @@ -179,6 +177,17 @@ typedef unsigned long uintptr_t;\n> #endif\n> #endif\n> \n> +#ifndef GIT_FAST_STAT\n> +static inline int fast_stat(const char *path, struct stat *st)\n> +{\n> +\treturn stat(path, st);\n> +}\n> +static inline int fast_lstat(const char *path, struct stat *st)\n> +{\n> +\treturn lstat(path, st);\n> +}\n> +#endif\n> +\n> /* used on Mac OS X */\n> #ifdef PRECOMPOSE_UNICODE\n> #include \"compat/precompose_utf8.h\"\n> diff --git a/help.c b/help.c\n> index 08c54ef..f068925 100644\n> --- a/help.c\n> +++ b/help.c\n> @@ -107,10 +107,7 @@ static int is_executable(const char *name)\n> \t !S_ISREG(st.st_mode))\n> \t\treturn 0;\n> \n> -#if defined(GIT_WINDOWS_NATIVE) || defined(__CYGWIN__)\n> -#if defined(__CYGWIN__)\n> -if ((st.st_mode & S_IXUSR) == 0)\n> -#endif\n> +#if defined(GIT_WINDOWS_NATIVE)\n> {\t/* cannot trust the executable bit, peek into the file instead */\n> \tchar buf[3] = { 0 };\n> \tint n;\n> diff --git a/path.c b/path.c\n> index 7f3324a..3d244d3 100644\n> --- a/path.c\n> +++ b/path.c\n> @@ -5,13 +5,7 @@\n> #include \"strbuf.h\"\n> #include \"string-list.h\"\n> \n> -#ifndef get_st_mode_bits\n> -/*\n> - * The replacement lstat(2) we use on Cygwin is incomplete and\n> - * may return wrong permission bits. Most of the time we do not care,\n> - * but the callsites of this wrapper do care.\n> - */\n> -int get_st_mode_bits(const char *path, int *mode)\n> +static int get_st_mode_bits(const char *path, int *mode)\n> {\n> \tstruct stat st;\n> \tif (lstat(path, &st) < 0)\n> @@ -19,7 +13,6 @@ int get_st_mode_bits(const char *path, int *mode)\n> \t*mode = st.st_mode;\n> \treturn 0;\n> }\n> -#endif\n> \n> static char bad_path[] = \"/bad-path/\";\n> \n> diff --git a/preload-index.c b/preload-index.c\n> index 49cb08d..1bece91 100644\n> --- a/preload-index.c\n> +++ b/preload-index.c\n> @@ -57,7 +57,7 @@ static void *preload_thread(void *_data)\n> \t\t\tcontinue;\n> \t\tif (threaded_has_symlink_leading_path(&cache, ce->name, ce_namelen(ce)))\n> \t\t\tcontinue;\n> -\t\tif (lstat(ce->name, &st))\n> +\t\tif (fast_lstat(ce->name, &st))\n> \t\t\tcontinue;\n> \t\tif (ie_match_stat(index, ce, &st, CE_MATCH_RACY_IS_DIRTY))\n> \t\t\tcontinue;\n> diff --git a/read-cache.c b/read-cache.c\n> index d5201f9..ed33d9e 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -689,7 +689,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n> int add_file_to_index(struct index_state *istate, const char *path, int flags)\n> {\n> \tstruct stat st;\n> -\tif (lstat(path, &st))\n> +\tif (fast_lstat(path, &st))\n> \t\tdie_errno(\"unable to stat '%s'\", path);\n> \treturn add_to_index(istate, path, &st, flags);\n> }\n> @@ -1049,7 +1049,7 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,\n> \t\treturn ce;\n> \t}\n> \n> -\tif (lstat(ce->name, &st) < 0) {\n> +\tif (fast_lstat(ce->name, &st) < 0) {\n> \t\tif (err)\n> \t\t\t*err = errno;\n> \t\treturn NULL;\n> @@ -1635,7 +1635,7 @@ static void ce_smudge_racily_clean_entry(struct cache_entry *ce)\n> \t */\n> \tstruct stat st;\n> \n> -\tif (lstat(ce->name, &st) < 0)\n> +\tif (fast_lstat(ce->name, &st) < 0)\n> \t\treturn;\n> \tif (ce_match_stat_basic(ce, &st))\n> \t\treturn;\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index b27f2a6..1fe9b63 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -1215,7 +1215,7 @@ static int verify_uptodate_1(const struct cache_entry *ce,\n> \telse if (o->reset || ce_uptodate(ce))\n> \t\treturn 0;\n> \n> -\tif (!lstat(ce->name, &st)) {\n> +\tif (!fast_lstat(ce->name, &st)) {\n> \t\tint flags = CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE;\n> \t\tunsigned changed = ie_match_stat(o->src_index, ce, &st, flags);\n> \t\tif (!changed)\n> @@ -1432,13 +1432,13 @@ static int verify_absent_1(const struct cache_entry *ce,\n> \t\tchar path[PATH_MAX + 1];\n> \t\tmemcpy(path, ce->name, len);\n> \t\tpath[len] = 0;\n> -\t\tif (lstat(path, &st))\n> +\t\tif (fast_lstat(path, &st))\n> \t\t\treturn error(\"cannot stat '%s': %s\", path,\n> \t\t\t\t\tstrerror(errno));\n> \n> \t\treturn check_ok_to_remove(path, len, DT_UNKNOWN, NULL, &st,\n> \t\t\t\terror_type, o);\n> -\t} else if (lstat(ce->name, &st)) {\n> +\t} else if (fast_lstat(ce->name, &st)) {\n> \t\tif (errno != ENOENT)\n> \t\t\treturn error(\"cannot stat '%s': %s\", ce->name,\n> \t\t\t\t strerror(errno));\n> @@ -1852,7 +1852,7 @@ int oneway_merge(const struct cache_entry * const *src,\n> \t\tint update = 0;\n> \t\tif (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) {\n> \t\t\tstruct stat st;\n> -\t\t\tif (lstat(old->name, &st) ||\n> +\t\t\tif (fast_lstat(old->name, &st) ||\n> \t\t\t ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))\n> \t\t\t\tupdate |= CE_UPDATE;\n> \t\t}\nI managed to apply your patch on next, and run the test suite before and after\nyour patch.\nThe performance running \"git status\" on a linux kernel is the same as in v1.8.3.\n\nRunning test suite did not show new failures.\nThe following test cases had failed, and are fixed now:\n< t1400-update-ref\n< t3210-pack-refs\n< t3211-peel-ref\n< t3306-notes-prune\n< t5304-prune\n< t5404-tracking-branches\n< t5500-fetch-pack\n< t5505-remote\n< t5514-fetch-multiple\n< t5515-fetch-merge-logic\n< t6030-bisect-porcelain\n< t9300-fast-import\n< t9903-bash-prompt\n\nIn short: Thanks, This looks good to me.\n"},{"id":"223087","messageId":"51DEEC0A.9060001@ramsay1.demon.co.uk","threadId":"34276","inReplyTo":"51DBEDAF.30708@web.de","subject":"Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2013-07-11T17:31:54Z","receivedAt":"2013-07-11T17:31:54Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Torsten Bögershausen wrote:\n> On 30.06.13 19:28, Ramsay Jones wrote:\n\n[ ... ]\n\n>>> You have just described my second patch! :D\n>> Unfortunately, I have not had any time to work on the patch this weekend.\n>> However, despite the patch being a bit rough around the edges, I decided\n>> to send it out (see below) to get some early feedback.\n>>\n>> Note that it passes the t3210, t3211, t5500, t3200, t3301, t7606 and t1301\n>> tests, but I have not run the full test suite.\n>>\n>> Comments welcome.\n>>\n>> ATB,\n>> Ramsay Jones\n>>\n>> -- >8 --\n>> Subject: [PATCH] cygwin: Add fast_[l]stat() functions\n>>\n>> Signed-off-by: Ramsay Jones \n>> ---\n>> builtin/apply.c | 6 +++---\n>> builtin/commit.c | 2 +-\n>> builtin/ls-files.c | 2 +-\n>> builtin/rm.c | 2 +-\n>> builtin/update-index.c | 2 +-\n>> check-racy.c | 2 +-\n>> compat/cygwin.c | 12 ------------\n>> compat/cygwin.h | 10 +++-------\n>> diff-lib.c | 2 +-\n>> diff.c | 2 +-\n>> entry.c | 4 ++--\n>> git-compat-util.h | 13 +++++++++++--\n>> help.c | 5 +----\n>> path.c | 9 +--------\n>> preload-index.c | 2 +-\n>> read-cache.c | 6 +++---\n>> unpack-trees.c | 8 ++++----\n>> 17 files changed, 36 insertions(+), 53 deletions(-)\n>>\n\n[ ... ]\n\n> I managed to apply your patch on next, and run the test suite before and after\n> your patch.\n> The performance running \"git status\" on a linux kernel is the same as in v1.8.3.\n> \n> Running test suite did not show new failures.\n> The following test cases had failed, and are fixed now:\n> < t1400-update-ref\n> < t3210-pack-refs\n> < t3211-peel-ref\n> < t3306-notes-prune\n> < t5304-prune\n> < t5404-tracking-branches\n> < t5500-fetch-pack\n> < t5505-remote\n> < t5514-fetch-multiple\n> < t5515-fetch-merge-logic\n> < t6030-bisect-porcelain\n> < t9300-fast-import\n> < t9903-bash-prompt\n> \n> In short: Thanks, This looks good to me.\n\nThank you for testing this! It is much appreciated.\n\nI sent a v2 patch to the list last night. It passes the full\ntest suite for me on cygwin 1.5, although there appears to be\na slight performance problem on MinGW (perhaps!). :(\n\nATB,\nRamsay Jones\n"}]}