{"thread":{"id":"18962","subject":"[PATCH 2/2] Windows: Skip fstat/lstat optimization in write_entry()","startedAt":"2009-04-20T08:17:00Z","lastAt":"2009-04-20T22:17:35Z","messageCount":12,"participants":["Johannes Sixt","Johannes Schindelin","Dmitry Potapov","Hannu Koivisto","Alex Riesen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"111694","messageId":"49EC2F7C.8070209@viscovery.net","threadId":"18962","inReplyTo":null,"subject":"[PATCH 2/2] Windows: Skip fstat/lstat optimization in write_entry()","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-04-20T08:17:00Z","receivedAt":"2009-04-20T08:17:00Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"From: Johannes Sixt <j6t@kdbg.org>\n\nCommit e4c72923 (write_entry(): use fstat() instead of lstat() when file\nis open, 2009-02-09) introduced an optimization of write_entry().\nUnfortunately, we cannot take advantage of this optimization on Windows\nbecause there is no guarantee that the time stamps are updated before the\nfile is closed:\n\n  \"The only guarantee about a file timestamp is that the file time is\n   correctly reflected when the handle that makes the change is closed.\"\n\n(http://msdn.microsoft.com/en-us/library/ms724290(VS.85).aspx)\n\nThe failure of this optimization on Windows can be observed most easily by\nrunning a 'git checkout' that has to update several large files. In this\ncase, 'git checkout' will report modified files, but infact only the\ntimestamps were incorrectly recorded in the index, as can be verified by a\nsubsequent 'git diff', which shows no change.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n My gut feeling was right: We cannot have this optimization on Windows.\n\n http://thread.gmane.org/gmane.comp.version-control.git/108351/focus=108357\n\n I've a repository where I can reproduce the error quite easily and this\n fixes it.\n\n -- Hannes (who forgot to add Dscho and git@vger on the first send attempt)\n\n Makefile          |    8 ++++++++\n entry.c           |    3 ++-\n git-compat-util.h |    6 ++++++\n 3 files changed, 16 insertions(+), 1 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 076a732..a01d603 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -167,6 +167,10 @@ all::\n # Define NO_EXTERNAL_GREP if you don't want \"git grep\" to ever call\n # your external grep (e.g., if your system lacks grep, if its grep is\n # broken, or spawning external process is slower than built-in grep git has).\n+#\n+# Define UNRELIABLE_FSTAT if your system's fstat does not return the same\n+# information on a not yet closed file that lstat would return for the same\n+# file after it was closed.\n\n GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE\n \t@$(SHELL_PATH) ./GIT-VERSION-GEN\n@@ -833,6 +837,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))\n \tNO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease\n \tNO_NSEC = YesPlease\n \tUSE_WIN32_MMAP = YesPlease\n+\tUNRELIABLE_FSTAT = UnfortunatelyYes\n \tCOMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat -Icompat/regex -Icompat/fnmatch\n \tCOMPAT_CFLAGS += -DSNPRINTF_SIZE_CORR=1\n \tCOMPAT_CFLAGS += -DSTRIP_EXTENSION=\\\".exe\\\"\n@@ -1111,6 +1116,9 @@ endif\n ifdef NO_EXTERNAL_GREP\n \tBASIC_CFLAGS += -DNO_EXTERNAL_GREP\n endif\n+ifdef UNRELIABLE_FSTAT\n+\tBASIC_CFLAGS += -DUNRELIABLE_FSTAT\n+endif\n\n ifeq ($(TCLTK_PATH),)\n NO_TCLTK=NoThanks\ndiff --git a/entry.c b/entry.c\nindex 5daacc2..915514a 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -147,7 +147,8 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout\n\n \t\twrote = write_in_full(fd, new, size);\n \t\t/* use fstat() only when path == ce->name */\n-\t\tif (state->refresh_cache && !to_tempfile && !state->base_dir_len) {\n+\t\tif (fstat_is_reliable() &&\n+\t\t    state->refresh_cache && !to_tempfile && !state->base_dir_len) {\n \t\t\tfstat(fd, &st);\n \t\t\tfstat_done = 1;\n \t\t}\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex d94c683..bf00f35 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -409,4 +409,10 @@ void git_qsort(void *base, size_t nmemb, size_t size,\n #endif\n #endif\n\n+#ifdef UNRELIABLE_FSTAT\n+#define fstat_is_reliable() 0\n+#else\n+#define fstat_is_reliable() 1\n+#endif\n+\n #endif\n\n-- \n1.6.3.rc1.989.ga175e.dirty\n"},{"id":"111707","messageId":"alpine.DEB.1.00.0904201205010.6955@intel-tinevez-2-302","threadId":"18962","inReplyTo":"49EC2F7C.8070209@viscovery.net","subject":"Re: [PATCH 2/2] Windows: Skip fstat/lstat optimization in write_entry()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-04-20T10:05:38Z","receivedAt":"2009-04-20T10:05:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 20 Apr 2009, Johannes Sixt wrote:\n\n> From: Johannes Sixt <j6t@kdbg.org>\n> \n> Commit e4c72923 (write_entry(): use fstat() instead of lstat() when file\n> is open, 2009-02-09) introduced an optimization of write_entry().\n> Unfortunately, we cannot take advantage of this optimization on Windows\n> because there is no guarantee that the time stamps are updated before the\n> file is closed:\n> \n>   \"The only guarantee about a file timestamp is that the file time is\n>    correctly reflected when the handle that makes the change is closed.\"\n> \n> (http://msdn.microsoft.com/en-us/library/ms724290(VS.85).aspx)\n> \n> The failure of this optimization on Windows can be observed most easily by\n> running a 'git checkout' that has to update several large files. In this\n> case, 'git checkout' will report modified files, but infact only the\n> timestamps were incorrectly recorded in the index, as can be verified by a\n> subsequent 'git diff', which shows no change.\n> \n> Signed-off-by: Johannes Sixt <j6t@kdbg.org>\n> ---\n>  My gut feeling was right: We cannot have this optimization on Windows.\n> \n>  http://thread.gmane.org/gmane.comp.version-control.git/108351/focus=108357\n> \n>  I've a repository where I can reproduce the error quite easily and this\n>  fixes it.\n> \n>  -- Hannes (who forgot to add Dscho and git@vger on the first send attempt)\n\nYou want this in 4msysgit's 'devel' branch, correct?\n\nCiao,\nDscho \"who is still interim maintainer ;-)\"\n"},{"id":"111719","messageId":"20090420110302.GB25059@dpotapov.dyndns.org","threadId":"18962","inReplyTo":"49EC2F7C.8070209@viscovery.net","subject":"Re: [PATCH 2/2] Windows: Skip fstat/lstat optimization in write_entry()","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2009-04-20T11:03:02Z","receivedAt":"2009-04-20T11:03:02Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"The cygwin version has the same problem. (In fact, it is even worse,\nbecause we have an optimized version for lstat/stat but not for fstat,\nand they return different values for some fields like i_no). But even\nif we used the only Cygwin functions, we would still face the problem,\nbecause Windows returns the wrong values for timestamps (and maybe\neven size on FAT?). So I think the following patch should be squashed\non top.\n\n-- >8 --\nFrom 1f957680d9b0e0bfeda9bf0e20397b0323b45334 Mon Sep 17 00:00:00 2001\nFrom: Dmitry Potapov <dpotapov@gmail.com>\nDate: Mon, 20 Apr 2009 14:54:16 +0400\nSubject: [PATCH] cygwin: Skip fstat/lstat optimization in write_entry()\n\n---\n Makefile |    1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 2af0dfb..177dc15 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -809,6 +809,7 @@ ifeq ($(uname_S),HP-UX)\n endif\n ifneq (,$(findstring CYGWIN,$(uname_S)))\n \tCOMPAT_OBJS += compat/cygwin.o\n+\tUNRELIABLE_FSTAT = UnfortunatelyYes\n endif\n ifneq (,$(findstring MINGW,$(uname_S)))\n \tNO_PREAD = YesPlease\n-- \n1.6.1.20.gee856\n-- >8 --\n"},{"id":"111725","messageId":"83eivnh5bd.fsf@kalahari.s2.org","threadId":"18962","inReplyTo":"20090420110302.GB25059@dpotapov.dyndns.org","subject":"Re: [PATCH 2/2] Windows: Skip fstat/lstat optimization in write_entry()","fromName":"Hannu Koivisto","fromEmail":"azure@iki.fi","sentAt":"2009-04-20T12:34:30Z","receivedAt":"2009-04-20T12:34:30Z","isPatch":true,"sender":{"key":"azure@iki.fi","avatar":null},"body":"Dmitry Potapov <dpotapov@gmail.com> writes:\n\n> The cygwin version has the same problem. (In fact, it is even worse,\n> because we have an optimized version for lstat/stat but not for fstat,\n> and they return different values for some fields like i_no). But even\n> if we used the only Cygwin functions, we would still face the problem,\n> because Windows returns the wrong values for timestamps (and maybe\n> even size on FAT?). So I think the following patch should be squashed\n> on top.\n\nI can verify that this fixes the rebase problem I've been having in\nCygwin and that I just mailed about separately.\n\n-- \nHannu\n"},{"id":"111727","messageId":"81b0412b0904200558w2d506f18i675d5dfb990005ce@mail.gmail.com","threadId":"18962","inReplyTo":"20090420110302.GB25059@dpotapov.dyndns.org","subject":"Re: [PATCH 2/2] Windows: Skip fstat/lstat optimization in write_entry()","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-04-20T12:58:49Z","receivedAt":"2009-04-20T12:58:49Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2009/4/20 Dmitry Potapov <dpotapov@gmail.com>:\n> The cygwin version has the same problem. (In fact, it is even worse,\n> because we have an optimized version for lstat/stat but not for fstat,\n> and they return different values for some fields like i_no). But even\n> if we used the only Cygwin functions, we would still face the problem,\n> because Windows returns the wrong values for timestamps (and maybe\n> even size on FAT?). So I think the following patch should be squashed\n> on top.\n\nI just sent a patch with an \"optimized\" fstat. I see no problems (at least none\nlike these) with that patch. Timestamps match. Windows XP, yes. But since\nthat MSDN article mentions that it is not guaranteed, I guess I just been lucky.\n"},{"id":"111731","messageId":"20090420133305.GE25059@dpotapov.dyndns.org","threadId":"18962","inReplyTo":"81b0412b0904200558w2d506f18i675d5dfb990005ce@mail.gmail.com","subject":"Re: [PATCH 2/2] Windows: Skip fstat/lstat optimization in write_entry()","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2009-04-20T13:33:05Z","receivedAt":"2009-04-20T13:33:05Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Mon, Apr 20, 2009 at 02:58:49PM +0200, Alex Riesen wrote:\n> 2009/4/20 Dmitry Potapov <dpotapov@gmail.com>:\n> > The cygwin version has the same problem. (In fact, it is even worse,\n> > because we have an optimized version for lstat/stat but not for fstat,\n> > and they return different values for some fields like i_no). But even\n> > if we used the only Cygwin functions, we would still face the problem,\n> > because Windows returns the wrong values for timestamps (and maybe\n> > even size on FAT?). So I think the following patch should be squashed\n> > on top.\n> \n> I just sent a patch with an \"optimized\" fstat. I see no problems (at least none\n> like these) with that patch. Timestamps match. Windows XP, yes. But since\n> that MSDN article mentions that it is not guaranteed, I guess I just been lucky.\n\nIf the time passed between the creating file and end of writing to it is\nsmall (less than timestamp resolution), you may not notice the problem.\nThe following program demonstrates the problem with fstat on Windows.\n(I compiled it using Cygwin). If you remove 'sleep' then you may not\nnotice the problem for a long time.\n\n-- >8 --\n#include <stdio.h>\n#include <string.h>\n#include <unistd.h>\n#include <sys/types.h>\n#include <sys/stat.h>\n#include <fcntl.h>\n\n#define FILENAME \"stat-test.tmp\"\n\nint main()\n{\n\tstruct stat st1, st2;\n\n\tmemset(&st1, 0, sizeof(st1));\n\tmemset(&st2, 0, sizeof(st2));\n\n\tunlink(FILENAME);\n\tint fd = open(FILENAME, O_CREAT|O_RDWR|O_TRUNC, S_IRWXU);\n\tif (fd == -1)\n\t{\n\t\tperror(\"Cannot open \" FILENAME);\n\t\treturn -1;\n\t}\n\tsleep(1); /* It is IMPORTANT! */\n\twrite(fd, \"test\\n\", 5);\n\tfstat(fd, &st1);\n\tclose(fd);\n\tlstat(FILENAME, &st2);\n\tif (memcmp(&st1, &st2, sizeof(st1))==0)\n\t\tprintf(\"fstat is OK\\n\");\n\telse\n\t\tprintf(\"fstat is broken\\n\");\n\treturn 0;\n}\n-- >8 --\n\nDmitry\n"},{"id":"111734","messageId":"81b0412b0904200654w1606a31fu227fa535cc14e10d@mail.gmail.com","threadId":"18962","inReplyTo":"20090420133305.GE25059@dpotapov.dyndns.org","subject":"Re: [PATCH 2/2] Windows: Skip fstat/lstat optimization in write_entry()","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-04-20T13:54:17Z","receivedAt":"2009-04-20T13:54:17Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2009/4/20 Dmitry Potapov <dpotapov@gmail.com>:\n> On Mon, Apr 20, 2009 at 02:58:49PM +0200, Alex Riesen wrote:\n>> 2009/4/20 Dmitry Potapov <dpotapov@gmail.com>:\n>> > The cygwin version has the same problem. (In fact, it is even worse,\n>> > because we have an optimized version for lstat/stat but not for fstat,\n>> > and they return different values for some fields like i_no). But even\n>> > if we used the only Cygwin functions, we would still face the problem,\n>> > because Windows returns the wrong values for timestamps (and maybe\n>> > even size on FAT?). So I think the following patch should be squashed\n>> > on top.\n>>\n>> I just sent a patch with an \"optimized\" fstat. I see no problems (at least none\n>> like these) with that patch. Timestamps match. Windows XP, yes. But since\n>> that MSDN article mentions that it is not guaranteed, I guess I just been lucky.\n>\n> If the time passed between the creating file and end of writing to it is\n> small (less than timestamp resolution), you may not notice the problem.\n> The following program demonstrates the problem with fstat on Windows.\n> (I compiled it using Cygwin). If you remove 'sleep' then you may not\n> notice the problem for a long time.\n\nAnd the Windows being as slow as it is, the problem can stay undetected for\na long time in a real working code.\n"},{"id":"111737","messageId":"49EC845D.6020107@viscovery.net","threadId":"18962","inReplyTo":"81b0412b0904200654w1606a31fu227fa535cc14e10d@mail.gmail.com","subject":"Re: [PATCH 2/2] Windows: Skip fstat/lstat optimization in write_entry()","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-04-20T14:19:09Z","receivedAt":"2009-04-20T14:19:09Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Alex Riesen schrieb:\n> 2009/4/20 Dmitry Potapov <dpotapov@gmail.com>:\n>> If the time passed between the creating file and end of writing to it is\n>> small (less than timestamp resolution), you may not notice the problem.\n>> The following program demonstrates the problem with fstat on Windows.\n>> (I compiled it using Cygwin). If you remove 'sleep' then you may not\n>> notice the problem for a long time.\n> \n> And the Windows being as slow as it is, the problem can stay undetected for\n> a long time in a real working code.\n\nYou got that wrong: If Windows were slow, the error would have been\ntriggered more often and it would have been detected earlier. There you\nhave the proof: Windows is fast ... enough :-P\n\n-- Hannes\n"},{"id":"111738","messageId":"81b0412b0904200725j22de23cajfd4b1cf119e69721@mail.gmail.com","threadId":"18962","inReplyTo":"49EC845D.6020107@viscovery.net","subject":"Re: [PATCH 2/2] Windows: Skip fstat/lstat optimization in write_entry()","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-04-20T14:25:29Z","receivedAt":"2009-04-20T14:25:29Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2009/4/20 Johannes Sixt <j.sixt@viscovery.net>:\n> Alex Riesen schrieb:\n>> 2009/4/20 Dmitry Potapov <dpotapov@gmail.com>:\n>>> If the time passed between the creating file and end of writing to it is\n>>> small (less than timestamp resolution), you may not notice the problem.\n>>> The following program demonstrates the problem with fstat on Windows.\n>>> (I compiled it using Cygwin). If you remove 'sleep' then you may not\n>>> notice the problem for a long time.\n>>\n>> And the Windows being as slow as it is, the problem can stay undetected for\n>> a long time in a real working code.\n>\n> You got that wrong: If Windows were slow, the error would have been\n> triggered more often and it would have been detected earlier. There you\n> have the proof: Windows is fast ... enough :-P\n\nErr, yes. Still a piece of cpp junk\n"},{"id":"111740","messageId":"81b0412b0904200727i169b9aa2od943349b9db0c6bb@mail.gmail.com","threadId":"18962","inReplyTo":"81b0412b0904200725j22de23cajfd4b1cf119e69721@mail.gmail.com","subject":"Re: [PATCH 2/2] Windows: Skip fstat/lstat optimization in write_entry()","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-04-20T14:27:03Z","receivedAt":"2009-04-20T14:27:03Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2009/4/20 Alex Riesen <raa.lkml@gmail.com>:\n> 2009/4/20 Johannes Sixt <j.sixt@viscovery.net>:\n>> You got that wrong: If Windows were slow, the error would have been\n>> triggered more often and it would have been detected earlier. There you\n>> have the proof: Windows is fast ... enough :-P\n>\n> Err, yes. Still a piece of cpp junk\n>\n\nErr, a _pile_.\n"},{"id":"111786","messageId":"7vbpqrdnyn.fsf@gitster.siamese.dyndns.org","threadId":"18962","inReplyTo":"20090420133305.GE25059@dpotapov.dyndns.org","subject":"Re: [PATCH 2/2] Windows: Skip fstat/lstat optimization in write_entry()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-04-20T21:17:36Z","receivedAt":"2009-04-20T21:17:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dmitry Potapov <dpotapov@gmail.com> writes:\n\n> On Mon, Apr 20, 2009 at 02:58:49PM +0200, Alex Riesen wrote:\n>> 2009/4/20 Dmitry Potapov <dpotapov@gmail.com>:\n>> > The cygwin version has the same problem. (In fact, it is even worse,\n>> > because we have an optimized version for lstat/stat but not for fstat,\n>> > and they return different values for some fields like i_no). But even\n>> > if we used the only Cygwin functions, we would still face the problem,\n>> > because Windows returns the wrong values for timestamps (and maybe\n>> > even size on FAT?). So I think the following patch should be squashed\n>> > on top.\n>> \n>> I just sent a patch with an \"optimized\" fstat. I see no problems (at least none\n>> like these) with that patch. Timestamps match. Windows XP, yes. But since\n>> that MSDN article mentions that it is not guaranteed, I guess I just been lucky.\n>\n> If the time passed between the creating file and end of writing to it is\n> small (less than timestamp resolution), you may not notice the problem.\n> The following program demonstrates the problem with fstat on Windows.\n> (I compiled it using Cygwin). If you remove 'sleep' then you may not\n> notice the problem for a long time.\n\nI take that you mean that Alex's patch does not work as intended.  In the\nmeantime, I've squashed your one-liner \"Cygwin-too\" into Hannes's patch.\n\nThanks.\n"},{"id":"111789","messageId":"81b0412b0904201517v2d842528x59ef7bed85efc014@mail.gmail.com","threadId":"18962","inReplyTo":"7vbpqrdnyn.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/2] Windows: Skip fstat/lstat optimization in write_entry()","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-04-20T22:17:35Z","receivedAt":"2009-04-20T22:17:35Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2009/4/20 Junio C Hamano <gitster@pobox.com>:\n> Dmitry Potapov <dpotapov@gmail.com> writes:\n>>\n>> If the time passed between the creating file and end of writing to it is\n>> small (less than timestamp resolution), you may not notice the problem.\n>> The following program demonstrates the problem with fstat on Windows.\n>> (I compiled it using Cygwin). If you remove 'sleep' then you may not\n>> notice the problem for a long time.\n>\n> I take that you mean that Alex's patch does not work as intended.  ...\n\nYes, it just makes the problem harder to notice by providing a faster fstat.\n"}]}