Re: [PATCH] git-count-objects: Fix a disk-space under-estimate on Cygwin

Ramsay Jones [off-list ref] writes:

Doesn't this contradict with everything you said?

You are forcing st_blksize to 512 but still return the same old st_blocks;
I do not understand what that would achieve.

In your experiments, st_blocks was reported in 1024-byte blocks and that
size coincided with what was reported in st_blksize, and that was the
whole point of the approach taken by the ST_BLOCKS_COUNTS_IN_BLKSIZE
patch.

Junio C Hamano wrote:

Ramsay Jones [off-list ref] writes:

quoted

I haven't tried this patch, but I think you may need to add something like
the following (*not tested*):

--- >8 ---
diff --git a/compat/cygwin.c b/compat/cygwin.c
index b4a51b9..7e9edec 100644
--- a/compat/cygwin.c
+++ b/compat/cygwin.c

@@ -53,6 +53,7 @@ static int do_stat(const char *file_name, struct stat *buf, stat_fn_t cygstat)
 		buf->st_size = (off_t)fdata.nFileSizeLow;
 #endif
 		buf->st_blocks = size_to_blocks(buf->st_size);
+		buf->st_blksize = 512;
 		filetime_to_timespec(&fdata.ftLastAccessTime, &buf->st_atim);
 		filetime_to_timespec(&fdata.ftLastWriteTime, &buf->st_mtim);
 		filetime_to_timespec(&fdata.ftCreationTime, &buf->st_ctim);
--- >8 ---

Doesn't this contradict with everything you said?

Err... no :-D

Note that my suggested addition to your patch is in the core.filemode == false
code path, and so does not affect the "disk-space under-estimate" problem at all.

[To be clear: the "disk-space under-estimate" problem only happens when
core.filemode == true and the regular cygwin lstat()/stat() functions are used.
When core.filemode == false, the code in compat/cygwin.c (namely cygwin_lstat()
and cygwin_stat()) will (most likely) be called instead. These functions use
WIN32 api calls to implement equivalent, but presumably faster, versions of the
stat functions]

You are forcing st_blksize to 512 but still return the same old st_blocks;
I do not understand what that would achieve.

Well, as I said, I haven't tested your patch, or my suggested addition, so I could
well be wrong... but what I aimed to achieve was to:

    - avoid "undefined behaviour" in on_disk_bytes(), since the value in
      st_blksize would otherwise be undefined (ie whatever happened to be
      on the stack-frame of the count_objects() function).
    - initialize the st_blksize field with a value consistent with the
      st_blocks field, which is derived from the st_size field, as the
      number of 512-byte blocks. (see the context line just before the
      + line in the above diff, along with the size_to_blocks macro)
    - return the same answer from this code as before.

Note that the answer returned from this core.filemode == false code path
is different to the core.filemode == true code path. Which is why I *slightly*
prefer my original patch.

HTH.

ATB,
Ramsay Jones