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

[Ramsay Jones <ramsay@ramsay1.demon.co.uk>]

Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
> 
>> 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