{"thread":{"id":"21670","subject":"[PATCH] git-count-objects: Fix a disk-space under-estimate on Cygwin","startedAt":"2009-11-19T18:46:24Z","lastAt":"2009-11-24T22:08:52Z","messageCount":7,"participants":["Ramsay Jones","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"127918","messageId":"4B059280.40902@ramsay1.demon.co.uk","threadId":"21670","inReplyTo":null,"subject":"[PATCH] git-count-objects: Fix a disk-space under-estimate on Cygwin","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2009-11-19T18:46:24Z","receivedAt":"2009-11-19T18:46:24Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nSigned-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n---\n\nWhilst testing an msvc-built git on cygwin, I noticed that\ngit-count-objects was producing different results to the\ncygwin version, viz:\n\n    $ ./git --version\n    git version 1.6.5.4.gf034d.MSVC\n    $ git --version\n    git version 1.6.5\n\n    $ ./git-count-objects\n    60 objects, 283 kilobytes\n\n    $ git count-objects\n    60 objects, 210 kilobytes\n\n    $ git config core.filemode false\n\n    $ git count-objects\n    60 objects, 297 kilobytes\n    $ \n\nNote also that the cygwin version of git gives two different\nanswers, in the *same* repository, depending on the setting\nof core.filemode. (since the \"win32 stat() functions\" in\ncompat/cygwin.c are used when core.filemode is false)\n\nHaving looked at the msvc code-path, I also noticed something\nelse a bit odd; the value printed by the msvc version should\nbe a *lower bound* for the amount of disk-space used (since\nit simply totals the actual file sizes). However, as you can\nsee from the above, when core.filemode is true, the cygwin\nversion of git is *underestimating* even this (210Kb vs 283kb).\n\nA quick trip to the debugger confirmed that (st.st_blocks * 512)\nis less than st.st_size for several files. So, it looked like\nthe block-size was not in units of 512 bytes; so if we look at\n/usr/include/sys/param.h we find:\n\n    #define DEV_BSIZE  1024\n\nand in /usr/include/sys/stat.h we find:\n\n    #define S_BLKSIZE  1024\n\nwhich seems to indicate a 1K block-size instead. Also, different\nfilesystem types may use different block-sizes, which is why an\nst.st_blksize was added to struct stat; it seems that the Cygwin\nstruct stat contains the st_blksize field, so lets try a quick\ntest program:\n\n    $ cat junk.c\n\n    #include <stdio.h>\n    #include <sys/stat.h>\n    \n    int main(int argc, char *argv[])\n    {\n    \tint i, bytes = 0, tot_5 = 0, tot_b = 0;\n    \n    \tfor (i=1; i< argc; i++) {\n    \t\tstruct stat st;\n    \t\tif (lstat(argv[i], &st))\n    \t\t\tprintf(\"can't lstat '%s'\\n\", argv[i]);\n    \t\telse {\n    \t\t\tint s = (int)st.st_size;\n    \t\t\tint b = (int)st.st_blocks;\n    \t\t\tint f = (int)st.st_blksize;\n    \t\t\tint d = (b * 512);\n    \t\t\tint e = (b * f);\n    \n    \t\t\tprintf(\"%8d, \", s);\n    \t\t\tprintf(\"%2d*512=%-6d (%6d), \", b, d, d-s);\n    \t\t\tprintf(\"%2d*%d=%-6d (%6d)\\n\", b, f, e, e-s);\n    \n    \t\t\tbytes += s;\n    \t\t\ttot_5 += d;\n    \t\t\ttot_b += e;\n    \t\t}\n    \t}\n    \tprintf(\"total bytes: %6d\\n\", bytes);\n    \tprintf(\"b * 512:     %6d  (%d)\\n\", tot_5, tot_5 - bytes);\n    \tprintf(\"b * blksize: %6d  (%d)\\n\", tot_b, tot_b - bytes);\n    \texit(0);\n    }\n    \n    $ ls .git/objects/??/* | xargs ./junk.exe\n         148,  1*512=512    (   364),  1*1024=1024   (   876)\n       10058, 12*512=6144   ( -3914), 12*1024=12288  (  2230)\n        3701,  4*512=2048   ( -1653),  4*1024=4096   (   395)\n         463,  4*512=2048   (  1585),  4*1024=4096   (  3633)\n         148,  1*512=512    (   364),  1*1024=1024   (   876)\n       10056, 12*512=6144   ( -3912), 12*1024=12288  (  2232)\n         198,  1*512=512    (   314),  1*1024=1024   (   826)\n        1782,  4*512=2048   (   266),  4*1024=4096   (  2314)\n         192,  1*512=512    (   320),  1*1024=1024   (   832)\n        3801,  4*512=2048   ( -1753),  4*1024=4096   (   295)\n       14851, 16*512=8192   ( -6659), 16*1024=16384  (  1533)\n        3761,  4*512=2048   ( -1713),  4*1024=4096   (   335)\n          52,  1*512=512    (   460),  1*1024=1024   (   972)\n         956,  4*512=2048   (  1092),  4*1024=4096   (  3140)\n         956,  4*512=2048   (  1092),  4*1024=4096   (  3140)\n        3703,  4*512=2048   ( -1655),  4*1024=4096   (   393)\n       10055, 12*512=6144   ( -3911), 12*1024=12288  (  2233)\n    total bytes:  64881\n    b * 512:      45568  (-19313)\n    b * blksize:  91136  (26255)\n    $ \n\nNote that, in addition to confirming the 1K block-size, it appears\nthat (on NTFS) files less than 1K are allocated a single block\nwhereas larger files use a multiple of 4 blocks. Well, that is only\na guess but it at least sounds plausible! ;-)\n\nThis patch implements a simple solution which has the useful property\nof returning a single answer, irrespective of the core.filemode setting.\n\nATB,\nRamsay Jones\n\n Makefile |    4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 5d5976f..5624563 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -783,6 +783,10 @@ ifeq ($(uname_O),Cygwin)\n \tNO_FAST_WORKING_DIRECTORY = UnfortunatelyYes\n \tNO_TRUSTABLE_FILEMODE = UnfortunatelyYes\n \tOLD_ICONV = UnfortunatelyYes\n+\t# The st_blocks field is not in units of 512 bytes, as the code\n+\t# expects, which leads to an under-estimate of the disk space used.\n+\t# In order to use an alternate algorithm, we claim to lack st_blocks.\n+\tNO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease\n \t# There are conflicting reports about this.\n \t# On some boxes NO_MMAP is needed, and not so elsewhere.\n \t# Try commenting this out if you suspect MMAP is more efficient\n-- \n1.6.5\n"},{"id":"127978","messageId":"7vd43d8yva.fsf@alter.siamese.dyndns.org","threadId":"21670","inReplyTo":"4B059280.40902@ramsay1.demon.co.uk","subject":"Re: [PATCH] git-count-objects: Fix a disk-space under-estimate on Cygwin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-11-20T07:00:09Z","receivedAt":"2009-11-20T07:00:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:\n\n> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n> ---\n\nCould you write a concise summary in the commit log message to explain why\nit is a correct fix?  Your detailed analysis after three dash lines was an\namusing read, but people who will be reading \"git log\" output 6 months\ndown the road won't have an access to it.  Something like...\n\n    Cygwin has st_blocks in struct stat, but at least on NTFS, the field\n    counts in blocks of st_blksize bytes, not in 512-byte blocks.\n\nThe Makefile already explains what NO_ST_BLOCKS_IN_STRUCT_STAT is about:\n\n    # Define NO_ST_BLOCKS_IN_STRUCT_STAT if your platform does not have st_blocks\n    # field that counts the on-disk footprint in 512-byte blocks.\n\nso the above explanation should be enough.\n\nNote that this is not any standard compliance.  POSIX cops out like this\nin the <sys/stat.h> description:\n\n    The unit for the st_blocks member of the stat structure is not defined\n    within POSIX.1-2008. In some implementations it is 512 bytes. It may\n    differ on a file system basis. There is no correlation between values\n    of the st_blocks and st_blksize, and the f_bsize (from\n    <sys/statvfs.h>) structure members.\n\nIOW, a system like Cygwin that does not use 512-byte is not in violation.\n\n> diff --git a/Makefile b/Makefile\n> index 5d5976f..5624563 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -783,6 +783,10 @@ ifeq ($(uname_O),Cygwin)\n>  \tNO_FAST_WORKING_DIRECTORY = UnfortunatelyYes\n>  \tNO_TRUSTABLE_FILEMODE = UnfortunatelyYes\n>  \tOLD_ICONV = UnfortunatelyYes\n> +\t# The st_blocks field is not in units of 512 bytes, as the code\n> +\t# expects, which leads to an under-estimate of the disk space used.\n> +\t# In order to use an alternate algorithm, we claim to lack st_blocks.\n> +\tNO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease\n\nThis comment is redundant, as the explanation of the Makefile variable\nalready said the same thing.\n\nAlso it is somewhat wrong to say \"claim to lack\".  The Makefile variable\nmerely means \"do not use this field\".  The reason may be that the platform\nlacks it, or it may have it but it does not count in 512-byte blocks.  It\ndoes not matter---either way the field is not usable.\n"},{"id":"127981","messageId":"7vlji17i02.fsf@alter.siamese.dyndns.org","threadId":"21670","inReplyTo":"7vd43d8yva.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] git-count-objects: Fix a disk-space under-estimate on Cygwin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-11-20T07:49:49Z","receivedAt":"2009-11-20T07:49:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When estimating the on-disk footprint of a file, we either used st_blocks\nthat is counted in 512-byte blocks (traditional unix behaviour), or on\nplatforms that do not have such st_blocks field in struct stat, simply the\nfile size itself.  Building with NO_ST_BLOCKS_IN_STRUCT_STAT will choose\nthe latter implementaion.\n\nPOSIX.1 says in its <sys/stat.h> description on this issue:\n\n    The unit for the st_blocks member of the stat structure is not\n    defined within POSIX.1-2008. In some implementations it is 512\n    bytes. It may differ on a file system basis. There is no\n    correlation between values of the st_blocks and st_blksize,\n    and the f_bsize (from <sys/statvfs.h>) structure members.\n\nEven though the above explicitly states st_blksize does not have any\ncorrelation, at least on one system (Cygwin on NTFS), the st_blocks field\nseems to count in blocks of st_blksize bytes.  A new Makefile variable\nST_BLOCKS_COUNTS_IN_BLKSIZE chooses to use this for the on-disk footprint.\n\n---\n\nNot signed-off yet, but this might be a workable alternative.  Testing on\nwider platforms (especially not on \"Linux with ext?\") might find this\nalternative useful.  I dunno.\n\n Makefile          |    8 +++++++-\n git-compat-util.h |    4 +++-\n 2 files changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 79195c8..5f2c5de 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -148,6 +148,9 @@ all::\n # Define USE_STDEV below if you want git to care about the underlying device\n # change being considered an inode change from the update-index perspective.\n #\n+# Define ST_BLOCKS_COUNTS_IN_ST_BLKSIZE if your platform has st_blocks\n+# field that counts the on-disk footprint in st_blksize-byte blocks.\n+#\n # Define NO_ST_BLOCKS_IN_STRUCT_STAT if your platform does not have st_blocks\n # field that counts the on-disk footprint in 512-byte blocks.\n #\n@@ -776,7 +779,7 @@ ifeq ($(uname_O),Cygwin)\n \tNO_FAST_WORKING_DIRECTORY = UnfortunatelyYes\n \tNO_TRUSTABLE_FILEMODE = UnfortunatelyYes\n \tOLD_ICONV = UnfortunatelyYes\n-\tNO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease\n+\tST_BLOCKS_COUNTS_IN_ST_BLKSIZE = YesPlease\n \t# There are conflicting reports about this.\n \t# On some boxes NO_MMAP is needed, and not so elsewhere.\n \t# Try commenting this out if you suspect MMAP is more efficient\n@@ -1126,6 +1129,9 @@ endif\n ifdef NO_D_INO_IN_DIRENT\n \tBASIC_CFLAGS += -DNO_D_INO_IN_DIRENT\n endif\n+ifdef ST_BLOCKS_COUNTS_IN_ST_BLKSIZE\n+\tBASIC_CFLAGS += -DST_BLOCKS_COUNTS_IN_ST_BLKSIZE\n+endif\n ifdef NO_ST_BLOCKS_IN_STRUCT_STAT\n \tBASIC_CFLAGS += -DNO_ST_BLOCKS_IN_STRUCT_STAT\n endif\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex ef60803..bba9c9e 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -240,7 +240,9 @@ extern int git_munmap(void *start, size_t length);\n \n #endif /* NO_MMAP */\n \n-#ifdef NO_ST_BLOCKS_IN_STRUCT_STAT\n+#ifdef ST_BLOCKS_COUNTS_IN_ST_BLKSIZE\n+#define on_disk_bytes(st) ((st).st_blocks * (st).st_blksize)\n+#elif defined(NO_ST_BLOCKS_IN_STRUCT_STAT) && NO_ST_BLOCKS_IN_STRUCT_STAT\n #define on_disk_bytes(st) ((st).st_size)\n #else\n #define on_disk_bytes(st) ((st).st_blocks * 512)\n"},{"id":"128080","messageId":"4B072D98.6020101@ramsay1.demon.co.uk","threadId":"21670","inReplyTo":"7vlji17i02.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] git-count-objects: Fix a disk-space under-estimate on Cygwin","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2009-11-21T00:00:24Z","receivedAt":"2009-11-21T00:00:24Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Junio C Hamano wrote:\n> When estimating the on-disk footprint of a file, we either used st_blocks\n> that is counted in 512-byte blocks (traditional unix behaviour), or on\n> platforms that do not have such st_blocks field in struct stat, simply the\n> file size itself.  Building with NO_ST_BLOCKS_IN_STRUCT_STAT will choose\n> the latter implementaion.\n> \n> POSIX.1 says in its <sys/stat.h> description on this issue:\n> \n>     The unit for the st_blocks member of the stat structure is not\n>     defined within POSIX.1-2008. In some implementations it is 512\n>     bytes. It may differ on a file system basis. There is no\n>     correlation between values of the st_blocks and st_blksize,\n>     and the f_bsize (from <sys/statvfs.h>) structure members.\n> \n> Even though the above explicitly states st_blksize does not have any\n> correlation, at least on one system (Cygwin on NTFS), the st_blocks field\n> seems to count in blocks of st_blksize bytes.  A new Makefile variable\n> ST_BLOCKS_COUNTS_IN_BLKSIZE chooses to use this for the on-disk footprint.\n\nMy first attempt to fix this problem was very similar to this. ;-)\n\nBTW, I thought that st_blocks and st_blksize were both XSI/SUS extensions\nand not part of POSIX, but your quote above contradicts that. Also, I don't\nknow that you can count on *both* fields always being present (I have not\npersonally used a system that didn't have st_blksize if it had st_blocks,\nbut I don't think it's guaranteed).\n\nAnyway, I decided against this kind of solution because it didn't address\nthe problem of returning different answers depending on the setting of\ncore.filemode.\n\nHaving said that, maybe that's not a big deal; in everyday use I can't\nimagine that anyone would change the core.filemode setting more than once,\nif ever. (I *have* been doing that quite a bit while testing an msvc-built\ngit on cygwin; but again, that's probably *not* an everyday usage :-P )\n\nI haven't tried this patch, but I think you may need to add something like\nthe following (*not tested*):\n\n--- >8 ---\ndiff --git a/compat/cygwin.c b/compat/cygwin.c\nindex b4a51b9..7e9edec 100644\n--- a/compat/cygwin.c\n+++ b/compat/cygwin.c\n@@ -53,6 +53,7 @@ static int do_stat(const char *file_name, struct stat *buf, stat_fn_t cygstat)\n \t\tbuf->st_size = (off_t)fdata.nFileSizeLow;\n #endif\n \t\tbuf->st_blocks = size_to_blocks(buf->st_size);\n+\t\tbuf->st_blksize = 512;\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--- >8 ---\n\nATB,\nRamsay Jones\n"},{"id":"128087","messageId":"7veinrnym4.fsf@alter.siamese.dyndns.org","threadId":"21670","inReplyTo":"4B072D98.6020101@ramsay1.demon.co.uk","subject":"Re: [PATCH] git-count-objects: Fix a disk-space under-estimate on Cygwin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-11-22T01:21:07Z","receivedAt":"2009-11-22T01:21:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:\n\n> I haven't tried this patch, but I think you may need to add something like\n> the following (*not tested*):\n>\n> --- >8 ---\n> diff --git a/compat/cygwin.c b/compat/cygwin.c\n> index b4a51b9..7e9edec 100644\n> --- a/compat/cygwin.c\n> +++ b/compat/cygwin.c\n> @@ -53,6 +53,7 @@ static int do_stat(const char *file_name, struct stat *buf, stat_fn_t cygstat)\n>  \t\tbuf->st_size = (off_t)fdata.nFileSizeLow;\n>  #endif\n>  \t\tbuf->st_blocks = size_to_blocks(buf->st_size);\n> +\t\tbuf->st_blksize = 512;\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> --- >8 ---\n\nDoesn't this contradict with everything you said?\n\nYou are forcing st_blksize to 512 but still return the same old st_blocks;\nI do not understand what that would achieve.\n\nIn your experiments, st_blocks was reported in 1024-byte blocks and that\nsize coincided with what was reported in st_blksize, and that was the\nwhole point of the approach taken by the ST_BLOCKS_COUNTS_IN_BLKSIZE\npatch.\n"},{"id":"128259","messageId":"4B0C3D0D.60309@ramsay1.demon.co.uk","threadId":"21670","inReplyTo":"7veinrnym4.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] git-count-objects: Fix a disk-space under-estimate on Cygwin","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2009-11-24T20:07:41Z","receivedAt":"2009-11-24T20:07: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 <ramsay@ramsay1.demon.co.uk> writes:\n> \n>> I haven't tried this patch, but I think you may need to add something like\n>> the following (*not tested*):\n>>\n>> --- >8 ---\n>> diff --git a/compat/cygwin.c b/compat/cygwin.c\n>> index b4a51b9..7e9edec 100644\n>> --- a/compat/cygwin.c\n>> +++ b/compat/cygwin.c\n>> @@ -53,6 +53,7 @@ static int do_stat(const char *file_name, struct stat *buf, stat_fn_t cygstat)\n>>  \t\tbuf->st_size = (off_t)fdata.nFileSizeLow;\n>>  #endif\n>>  \t\tbuf->st_blocks = size_to_blocks(buf->st_size);\n>> +\t\tbuf->st_blksize = 512;\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>> --- >8 ---\n> \n> Doesn't this contradict with everything you said?\n\nErr... no :-D\n\nNote that my suggested addition to your patch is in the core.filemode == false\ncode path, and so does not affect the \"disk-space under-estimate\" problem at all.\n\n[To be clear: the \"disk-space under-estimate\" problem only happens when\ncore.filemode == true and the regular cygwin lstat()/stat() functions are used.\nWhen core.filemode == false, the code in compat/cygwin.c (namely cygwin_lstat()\nand cygwin_stat()) will (most likely) be called instead. These functions use\nWIN32 api calls to implement equivalent, but presumably faster, versions of the\nstat functions]\n\n> You are forcing st_blksize to 512 but still return the same old st_blocks;\n> I do not understand what that would achieve.\n\nWell, as I said, I haven't tested your patch, or my suggested addition, so I could\nwell be wrong... but what I aimed to achieve was to:\n\n    - avoid \"undefined behaviour\" in on_disk_bytes(), since the value in\n      st_blksize would otherwise be undefined (ie whatever happened to be\n      on the stack-frame of the count_objects() function).\n    - initialize the st_blksize field with a value consistent with the\n      st_blocks field, which is derived from the st_size field, as the\n      number of 512-byte blocks. (see the context line just before the\n      + line in the above diff, along with the size_to_blocks macro)\n    - return the same answer from this code as before.\n\nNote that the answer returned from this core.filemode == false code path\nis different to the core.filemode == true code path. Which is why I *slightly*\nprefer my original patch.\n\nHTH.\n\nATB,\nRamsay Jones\n"},{"id":"128264","messageId":"7vskc3bmob.fsf@alter.siamese.dyndns.org","threadId":"21670","inReplyTo":"4B0C3D0D.60309@ramsay1.demon.co.uk","subject":"Re: [PATCH] git-count-objects: Fix a disk-space under-estimate on Cygwin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-11-24T22:08:52Z","receivedAt":"2009-11-24T22:08:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:\n\n> Err... no :-D\n>\n> Note that my suggested addition to your patch is in the core.filemode == false\n> code path, and so does not affect the \"disk-space under-estimate\" problem at all.\n>\n> [To be clear: the \"disk-space under-estimate\" problem only happens when\n> core.filemode == true and the regular cygwin lstat()/stat() functions are used.\n> When core.filemode == false, the code in compat/cygwin.c (namely cygwin_lstat()\n> and cygwin_stat()) will (most likely) be called instead. These functions use\n> WIN32 api calls to implement equivalent, but presumably faster, versions of the\n> stat functions]\n>\n>> You are forcing st_blksize to 512 but still return the same old st_blocks;\n>> I do not understand what that would achieve.\n>\n> Well, as I said, I haven't tested your patch, or my suggested addition, so I could\n> well be wrong... but what I aimed to achieve was to:\n>\n>     - avoid \"undefined behaviour\" in on_disk_bytes(), since the value in\n>       st_blksize would otherwise be undefined (ie whatever happened to be\n>       on the stack-frame of the count_objects() function).\n>     - initialize the st_blksize field with a value consistent with the\n>       st_blocks field, which is derived from the st_size field, as the\n>       number of 512-byte blocks. (see the context line just before the\n>       + line in the above diff, along with the size_to_blocks macro)\n>     - return the same answer from this code as before.\n\nAh, sorry, so then I misread your comment.  size_to_blocks() in\ncompat/cygwin.c counts blocks in 512 (I just checked) and you are applying\nthe reverse.\n\nBut you are right.  If the emulation used on cygwin is _not_ doing the\n\"blocks * blksize is close to size\" thing that is not POSIX but you saw in\nyour experiment on NTFS, and if we need your follow-up patch to make it do\nso, there is no point in using my patch.\n\n> Note that the answer returned from this core.filemode == false code path\n> is different to the core.filemode == true code path. Which is why I *slightly*\n> prefer my original patch.\n\nThanks for a clarification.  I've already queued both to 'next', but I\nwill revert mine and make your patch graduate to 'master'.\n"}]}